feat: add native Windows sandbox booter (restricted token + job object) - #9895
feat: add native Windows sandbox booter (restricted token + job object)#9895yunyancuo wants to merge 2 commits into
Conversation
Run agent-generated code under a restricted token of the current user (pywin32 CreateRestrictedToken) with a synthetic-SID double gate: file writes require an ACE for the synthetic SID, so they are confined to a per-session work directory. The child runs on a private desktop (isolated from the user session) inside a kill-on-close Job Object. Requires pywin32 on Windows (declared for sys_platform == win32). Network denial is advisory in this phase (environment poisoning); kernel-enforced WFP per-user rules land after the elevated-setup step. Known limits: managed shell sessions and browser/GUI capabilities are gated off; skill sync is skipped (POSIX-only commands).
Add a native option to provider_settings.sandbox.booter and select NativeBooter in get_booter. Skill sync is skipped for this booter: its commands are POSIX-only and would not run in the sandbox shell.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 8 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="astrbot/core/computer/booters/native.py" line_range="208-214" />
<code_context>
+ wintypes.DWORD,
+ ctypes.c_void_p,
+ ]
+ hdesk = user32.CreateDesktopW(
+ self.desktop_name.rsplit("\\", 1)[-1],
+ None,
+ None,
+ 0,
+ GENERIC_ALL,
+ ctypes.byref(sa),
+ )
+ if not hdesk:
</code_context>
<issue_to_address>
**issue (bug_risk):** `CreateDesktopW` is called with seven arguments even though the Windows API accepts six: the extra `None` shifts `GENERIC_ALL` into the `SECURITY_ATTRIBUTES` parameter and leaves the actual security descriptor pointer unused. Desktop creation therefore fails or receives an invalid pointer, so native sandbox boot does not start.
**Suggested fix:** Pass the six API arguments in the correct order: name, device, devmode, flags, desired access, and security attributes.
</issue_to_address>
### Comment 2
<location path="astrbot/core/computer/booters/native.py" line_range="299-304" />
<code_context>
+ self._job = job
+
+ timed_out = False
+ wait = win32event.WaitForSingleObject(proc[0], int((timeout or 120) * 1000))
+ if wait == win32con.WAIT_TIMEOUT:
+ timed_out = True
+ win32job.TerminateJobObject(job, 1)
+ outw.Close()
+ chunks = []
+ while True:
+ try:
</code_context>
<issue_to_address>
**issue (bug_risk):** The parent waits for process termination before reading from the child’s pipe. A command that writes more than the pipe buffer fills the pipe, blocks in `WriteFile`, and never exits; the parent then waits until the timeout and kills an otherwise valid command.
**Triggers:** When a sandbox command produces enough stdout or stderr to fill the inherited pipe buffer.
**Suggested fix:** Drain the pipe concurrently while waiting, or use an asynchronous/subprocess communication mechanism that reads output as the child runs.
</issue_to_address>
### Comment 3
<location path="astrbot/core/computer/booters/native.py" line_range="297-319" />
<code_context>
+ clean,
+ None,
+ )
+
+
+class NativeSandbox:
+ """One sandboxed execution context: desktop + restricted token + grants."""
+
+ def __init__(self, workdir: Path) -> None:
+ self.workdir = Path(workdir)
+ self.desktop_name = f"astrbot_native_{uuid.uuid4().hex[:8]}"
+ self._job = None
+ self._hdesk = None
+ self._prepared = False
</code_context>
<issue_to_address>
**issue (bug_risk):** Concurrent executions overwrite the single `self._job` reference and each execution clears it when it finishes. `terminate()` and shutdown can therefore terminate only the last job, leaving an earlier execution running outside the tracked handle while its work directory and ACLs are being cleaned up.
**Triggers:** When Python and shell executions overlap, or shutdown runs while more than one execution is active.
**Suggested fix:** Track all active jobs and synchronize job registration/removal and shutdown, or serialize executions per sandbox.
</issue_to_address>
### Comment 4
<location path="astrbot/core/computer/booters/native.py" line_range="351-364" />
<code_context>
+ async def exec(
+ self,
+ command: str,
+ cwd: str | None = None,
+ env: dict[str, str] | None = None,
+ timeout: int | None = 300,
+ shell: bool = True,
+ background: bool = False,
+ ) -> dict[str, Any]:
+ """Execute a shell command inside the sandbox."""
+ if background:
+ raise NotImplementedError(
+ "Background shell is not supported by the native booter."
+ )
+ args = ["cmd", "/d", "/s", "/c", command]
+ out, rc = await asyncio.to_thread(
+ self._sandbox.run, args, env=env, timeout=timeout
+ )
+ return {"stdout": out, "stderr": "", "exit_code": rc}
</code_context>
<issue_to_address>
**issue (bug_risk):** Both native shell and Python components accept `cwd` but never use it; every command runs with `self.workdir` as the working directory. Callers that request a different directory silently execute in the wrong location and can read or write different relative paths than requested.
**Triggers:** When a caller supplies a non-default `cwd`.
**Suggested fix:** Validate the requested directory is inside the sandbox root and pass it as the working directory to `NativeSandbox.run` or use it when creating the script.
</issue_to_address>
### Comment 5
<location path="astrbot/core/computer/booters/native.py" line_range="253-254" />
<code_context>
+ if not self._prepared:
+ raise RuntimeError("sandbox is not prepared")
+ child_env = {
+ **{k: v for k, v in os.environ.items() if not k.lower().endswith("_proxy")},
+ # Advisory-only network denial (elevation + WFP would make this
+ # kernel-enforced; without them this is best-effort).
+ "HTTP_PROXY": "http://127.0.0.1:9",
+ "HTTPS_PROXY": "http://127.0.0.1:9",
+ "GIT_SSH_COMMAND": "exit 1",
+ "PYTHONIOENCODING": "utf-8",
+ "PYTHONPATH": get_astrbot_site_packages_path(),
+ }
+ if env:
+ child_env.update({str(k): str(v) for k, v in env.items()})
+
+ sa = pywintypes.SECURITY_ATTRIBUTES()
+ sa.bInheritHandle = True
</code_context>
<issue_to_address>
**🚨 issue (security):** Caller-provided `env` is merged after the proxy poisoning values, so it can overwrite `HTTP_PROXY`, `HTTPS_PROXY`, or `GIT_SSH_COMMAND` with unrestricted values. The advertised network-denial advisory is therefore disabled by the normal environment override path.
**Triggers:** When a shell or execution caller supplies one of the poisoned environment variable names.
**Suggested fix:** Reject or reapply protected network-related variables after merging caller environment values.
```suggestion
if env:
child_env.update({str(k): str(v) for k, v in env.items()})
child_env.update(
{
"HTTP_PROXY": "http://127.0.0.1:9",
"HTTPS_PROXY": "http://127.0.0.1:9",
"GIT_SSH_COMMAND": "exit 1",
}
)
```
</issue_to_address>
### Comment 6
<location path="astrbot/core/computer/booters/native.py" line_range="107-119" />
<code_context>
+ if dacl is None:
+ return
+ clean = win32security.ACL()
+ for index in range(dacl.GetAceCount()):
+ (ace_type, ace_flags), access_mask, ace_sid = dacl.GetAce(index)
+ if win32security.ConvertSidToStringSid(ace_sid) == sid_text:
+ continue
+ if ace_type == win32security.ACCESS_ALLOWED_ACE_TYPE:
+ clean.AddAccessAllowedAceEx(
+ win32security.ACL_REVISION_DS, ace_flags, access_mask, ace_sid
+ )
+ elif ace_type == win32security.ACCESS_DENIED_ACE_TYPE:
+ clean.AddAccessDeniedAceEx(
+ win32security.ACL_REVISION_DS, ace_flags, access_mask, ace_sid
+ )
+ win32security.SetNamedSecurityInfo(
+ str(path),
+ win32security.SE_FILE_OBJECT,
</code_context>
<issue_to_address>
**🚨 issue (security):** ACL revocation rebuilds the DACL while preserving only allowed and denied ACEs. Audit, object-specific, callback, and other ACE types are silently discarded from `sys.base_prefix` and `sys.exec_prefix`, changing security and auditing behavior of the interpreter trees whenever those ACEs are present.
**Triggers:** When either interpreter tree contains a non-basic allowed or denied ACE.
**Suggested fix:** Remove only matching synthetic-SID ACEs while preserving every other ACE type, or use an API that edits the existing ACL in place.
</issue_to_address>
### Comment 7
<location path="astrbot/core/computer/booters/native.py" line_range="518-521" />
<code_context>
+ """Prepare the desktop, token, and work directory for this session."""
+
+ def _boot() -> None:
+ key = uuid.uuid5(uuid.NAMESPACE_DNS, session_id).hex[:12]
+ workdir = Path(get_astrbot_temp_path()) / "native" / key / "work"
+ sandbox = NativeSandbox(workdir)
+ sandbox.prepare()
+ self._sandbox = sandbox
+ self._fs = NativeFileSystemComponent(workdir)
</code_context>
<issue_to_address>
**issue (bug_risk):** If preparation fails after granting the synthetic SID ACEs but before `self._sandbox` is assigned, `NativeBooter` has no sandbox reference during boot-error cleanup. The cleanup returns without revoking the ACEs, leaving stale synthetic permissions on the interpreter trees.
**Triggers:** When desktop creation or restricted-token creation fails after the ACL grants succeed.
**Suggested fix:** Assign the sandbox before preparation and make preparation transactional, or explicitly revoke all grants in a `finally` block when preparation fails.
</issue_to_address>
### Comment 8
<location path="astrbot/core/computer/booters/native.py" line_range="299-321" />
<code_context>
+ self._job = job
+
+ timed_out = False
+ wait = win32event.WaitForSingleObject(proc[0], int((timeout or 120) * 1000))
+ if wait == win32con.WAIT_TIMEOUT:
+ timed_out = True
+ win32job.TerminateJobObject(job, 1)
+ outw.Close()
+ chunks = []
+ while True:
+ try:
+ _hr, data = win32file.ReadFile(outr, 65536)
+ except pywintypes.error:
+ break
+ if not data:
+ break
+ chunks.append(data.decode("utf-8", errors="replace"))
+ outr.Close()
+ text = "".join(chunks)
+ rc = win32process.GetExitCodeProcess(proc[0])
+ proc[0].Close()
+ proc[1].Close()
+ job.Close()
+ self._job = None
+ if timed_out:
+ raise subprocess.TimeoutExpired(args, timeout or 120, text)
+ return text, rc
+
</code_context>
<issue_to_address>
**issue (bug_risk):** The implementation converts a `None` or zero timeout into 120 seconds via `timeout or 120`, despite the shell contract allowing `timeout: int | None`; callers requesting no timeout are forcibly terminated after 120 seconds, and a zero timeout is not honored.
**Triggers:** When shell execution passes `timeout=None` or `timeout=0`.
**Suggested fix:** Define the intended no-timeout behavior explicitly and avoid using truthiness to replace valid timeout values.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 8 findings to address first, and if the restricted-token or ACL design is wrong, agent-generated code could escape the intended work-directory boundary and read or modify host files, credentials, or other user data; the broad restricted-group grants and global ACL changes make that failure potentially unbounded. Reverting removes the booter, but any data exposed or modified before the revert cannot be recovered by reverting.
Blocking findings: astrbot/core/computer/booters/native.py:214, astrbot/core/computer/booters/native.py:304, astrbot/core/computer/booters/native.py:319, astrbot/core/computer/booters/native.py:364, astrbot/core/computer/booters/native.py:254, and 3 more
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| hdesk = user32.CreateDesktopW( | ||
| self.desktop_name.rsplit("\\", 1)[-1], | ||
| None, | ||
| None, | ||
| 0, | ||
| GENERIC_ALL, | ||
| ctypes.byref(sa), |
There was a problem hiding this comment.
issue (bug_risk): CreateDesktopW is called with seven arguments even though the Windows API accepts six: the extra None shifts GENERIC_ALL into the SECURITY_ATTRIBUTES parameter and leaves the actual security descriptor pointer unused. Desktop creation therefore fails or receives an invalid pointer, so native sandbox boot does not start.
Suggested fix: Pass the six API arguments in the correct order: name, device, devmode, flags, desired access, and security attributes.
| wait = win32event.WaitForSingleObject(proc[0], int((timeout or 120) * 1000)) | ||
| if wait == win32con.WAIT_TIMEOUT: | ||
| timed_out = True | ||
| win32job.TerminateJobObject(job, 1) | ||
| outw.Close() | ||
| chunks = [] |
There was a problem hiding this comment.
issue (bug_risk): The parent waits for process termination before reading from the child’s pipe. A command that writes more than the pipe buffer fills the pipe, blocks in WriteFile, and never exits; the parent then waits until the timeout and kills an otherwise valid command.
Triggers: When a sandbox command produces enough stdout or stderr to fill the inherited pipe buffer.
Suggested fix: Drain the pipe concurrently while waiting, or use an asynchronous/subprocess communication mechanism that reads output as the child runs.
|
|
||
| timed_out = False | ||
| wait = win32event.WaitForSingleObject(proc[0], int((timeout or 120) * 1000)) | ||
| if wait == win32con.WAIT_TIMEOUT: | ||
| timed_out = True | ||
| win32job.TerminateJobObject(job, 1) | ||
| outw.Close() | ||
| chunks = [] | ||
| while True: | ||
| try: | ||
| _hr, data = win32file.ReadFile(outr, 65536) | ||
| except pywintypes.error: | ||
| break | ||
| if not data: | ||
| break | ||
| chunks.append(data.decode("utf-8", errors="replace")) | ||
| outr.Close() | ||
| text = "".join(chunks) | ||
| rc = win32process.GetExitCodeProcess(proc[0]) | ||
| proc[0].Close() | ||
| proc[1].Close() | ||
| job.Close() | ||
| self._job = None |
There was a problem hiding this comment.
issue (bug_risk): Concurrent executions overwrite the single self._job reference and each execution clears it when it finishes. terminate() and shutdown can therefore terminate only the last job, leaving an earlier execution running outside the tracked handle while its work directory and ACLs are being cleaned up.
Triggers: When Python and shell executions overlap, or shutdown runs while more than one execution is active.
Suggested fix: Track all active jobs and synchronize job registration/removal and shutdown, or serialize executions per sandbox.
| cwd: str | None = None, | ||
| env: dict[str, str] | None = None, | ||
| timeout: int | None = 300, | ||
| shell: bool = True, | ||
| background: bool = False, | ||
| ) -> dict[str, Any]: | ||
| """Execute a shell command inside the sandbox.""" | ||
| if background: | ||
| raise NotImplementedError( | ||
| "Background shell is not supported by the native booter." | ||
| ) | ||
| args = ["cmd", "/d", "/s", "/c", command] | ||
| out, rc = await asyncio.to_thread( | ||
| self._sandbox.run, args, env=env, timeout=timeout |
There was a problem hiding this comment.
issue (bug_risk): Both native shell and Python components accept cwd but never use it; every command runs with self.workdir as the working directory. Callers that request a different directory silently execute in the wrong location and can read or write different relative paths than requested.
Triggers: When a caller supplies a non-default cwd.
Suggested fix: Validate the requested directory is inside the sandbox root and pass it as the working directory to NativeSandbox.run or use it when creating the script.
| if env: | ||
| child_env.update({str(k): str(v) for k, v in env.items()}) |
There was a problem hiding this comment.
🚨 issue (security): Caller-provided env is merged after the proxy poisoning values, so it can overwrite HTTP_PROXY, HTTPS_PROXY, or GIT_SSH_COMMAND with unrestricted values. The advertised network-denial advisory is therefore disabled by the normal environment override path.
Triggers: When a shell or execution caller supplies one of the poisoned environment variable names.
Suggested fix: Reject or reapply protected network-related variables after merging caller environment values.
| if env: | |
| child_env.update({str(k): str(v) for k, v in env.items()}) | |
| if env: | |
| child_env.update({str(k): str(v) for k, v in env.items()}) | |
| child_env.update( | |
| { | |
| "HTTP_PROXY": "http://127.0.0.1:9", | |
| "HTTPS_PROXY": "http://127.0.0.1:9", | |
| "GIT_SSH_COMMAND": "exit 1", | |
| } | |
| ) |
| for index in range(dacl.GetAceCount()): | ||
| (ace_type, ace_flags), access_mask, ace_sid = dacl.GetAce(index) | ||
| if win32security.ConvertSidToStringSid(ace_sid) == sid_text: | ||
| continue | ||
| if ace_type == win32security.ACCESS_ALLOWED_ACE_TYPE: | ||
| clean.AddAccessAllowedAceEx( | ||
| win32security.ACL_REVISION_DS, ace_flags, access_mask, ace_sid | ||
| ) | ||
| elif ace_type == win32security.ACCESS_DENIED_ACE_TYPE: | ||
| clean.AddAccessDeniedAceEx( | ||
| win32security.ACL_REVISION_DS, ace_flags, access_mask, ace_sid | ||
| ) | ||
| win32security.SetNamedSecurityInfo( |
There was a problem hiding this comment.
🚨 issue (security): ACL revocation rebuilds the DACL while preserving only allowed and denied ACEs. Audit, object-specific, callback, and other ACE types are silently discarded from sys.base_prefix and sys.exec_prefix, changing security and auditing behavior of the interpreter trees whenever those ACEs are present.
Triggers: When either interpreter tree contains a non-basic allowed or denied ACE.
Suggested fix: Remove only matching synthetic-SID ACEs while preserving every other ACE type, or use an API that edits the existing ACL in place.
| key = uuid.uuid5(uuid.NAMESPACE_DNS, session_id).hex[:12] | ||
| workdir = Path(get_astrbot_temp_path()) / "native" / key / "work" | ||
| sandbox = NativeSandbox(workdir) | ||
| sandbox.prepare() |
There was a problem hiding this comment.
issue (bug_risk): If preparation fails after granting the synthetic SID ACEs but before self._sandbox is assigned, NativeBooter has no sandbox reference during boot-error cleanup. The cleanup returns without revoking the ACEs, leaving stale synthetic permissions on the interpreter trees.
Triggers: When desktop creation or restricted-token creation fails after the ACL grants succeed.
Suggested fix: Assign the sandbox before preparation and make preparation transactional, or explicitly revoke all grants in a finally block when preparation fails.
| wait = win32event.WaitForSingleObject(proc[0], int((timeout or 120) * 1000)) | ||
| if wait == win32con.WAIT_TIMEOUT: | ||
| timed_out = True | ||
| win32job.TerminateJobObject(job, 1) | ||
| outw.Close() | ||
| chunks = [] | ||
| while True: | ||
| try: | ||
| _hr, data = win32file.ReadFile(outr, 65536) | ||
| except pywintypes.error: | ||
| break | ||
| if not data: | ||
| break | ||
| chunks.append(data.decode("utf-8", errors="replace")) | ||
| outr.Close() | ||
| text = "".join(chunks) | ||
| rc = win32process.GetExitCodeProcess(proc[0]) | ||
| proc[0].Close() | ||
| proc[1].Close() | ||
| job.Close() | ||
| self._job = None | ||
| if timed_out: | ||
| raise subprocess.TimeoutExpired(args, timeout or 120, text) |
There was a problem hiding this comment.
issue (bug_risk): The implementation converts a None or zero timeout into 120 seconds via timeout or 120, despite the shell contract allowing timeout: int | None; callers requesting no timeout are forcibly terminated after 120 seconds, and a zero timeout is not honored.
Triggers: When shell execution passes timeout=None or timeout=0.
Suggested fix: Define the intended no-timeout behavior explicitly and avoid using truthiness to replace valid timeout values.
📄 Knowledge review🆕 New pages1 new page was drafted from this PR.
|
|
Closing per maintainer feedback — agreed that this is too large for a single PR and touches too many surfaces (frontend config, packaging, CLI) to land as-is. The branch stays available on the fork; if there is interest in Windows-native isolation as a feature, I'd like to start with an issue to scope it into smaller, reviewable pieces first. Thanks for the feedback. |
1 participant
What
Adds an opt-in native Windows sandbox booter (
provider_settings.sandbox.booter = "native") so agent-generated commands/code can run isolated on Windows without Docker, third-party drivers, or admin rights:CreateRestrictedToken) whose restricted-SID list includes a synthetic SID; the per-session work directory is the only tree granted an ACE for it, so writes anywhere else are denied by the kernel (verified:%TEMP%write raises PermissionError, no file leaked).pywin32declared forsys_platform == "win32"(previously only a transitive dependency).Network denial is advisory in this phase (proxy/
GIT_SSH_COMMANDpoisoning visible in the child env); kernel-enforced per-user WFP rules are planned as a follow-up once the elevated-setup step lands (see notes).Verification
Smoke-tested end-to-end on a real Windows 11 machine (26200): boot → shell exec (
cmdin the sandbox; PowerShell's .NET init fails under restricted tokens, so cmd is used) → Python exec (base interpreter +PYTHONPATHto venv packages) → workdir confinement (fs layer + OS layer) → upload/download → shutdown with ACL revocation — all green,ruff format/checkclean.Primitive-level proofs (P0) were validated separately: Job tree-kill, restricted-token double gate, dedicated sandbox users. A full write-up of the Windows gotchas (restricted-token desktop DACLs,
FILE_GENERIC_READlacking FILE_EXECUTE, Everyone-ACE requirement, venv launcher breakinglpDesktop, etc.) is available if reviewers want it.Known limits (documented in code)
FWP_E_NOT_INITIALIZEDdespite BFE running). The WFP code path exists but ships after the elevated-setup step is proven on clean machines. Until then, network denial is advisory only — this booter targets accidental damage, not determined adversaries.Note
A companion branch (
feat/sandboxie-booter) exploring a Sandboxie-Plus–driven booter exists on the fork and can be shared if an alternative backend is of interest.Summary by Sourcery
Add an opt-in native Windows sandbox for isolated agent code execution without Docker or administrative privileges.
New Features:
Enhancements:
Build: