fix(sandbox/windows): stop the DACL cost storm, fix DLL-init/cwd bugs, add Relaxed Sandbox Mode for trusted_local - #254
Open
frankforges wants to merge 2 commits into
Conversation
…, add Relaxed Sandbox Mode for trusted_local Four Windows AppContainer bugs, found while chasing #520/#618/#743/#921 (shell execution "still not working" on Windows across three months of reports). All measured live against real spawns. 1. workspace_policy.rs: trusted_local put $HOME and the whole host %TEMP% into the AppContainer read/write allowlist. Every grant becomes an inheritable ACE rewritten across the ENTIRE subtree (SetNamedSecurityInfoW), paid twice (grant+revoke) per spawn. On a real profile that is tens of seconds per command -> every command times out. Measured: full trusted_local allowlist 35s -> timeout; same allowlist with $HOME dropped and %TEMP% replaced by a small scratch subdir: ~20ms. A crash-leaked ACE also used to land on the profile root and could brick unrelated Electron/Chromium apps (electron/electron#51761); with $HOME out of the allowlist a leak lands on a workspace subdir instead. 2. appcontainer.rs: CreateRestrictedToken marked BUILTIN\Users deny-only. Some System32 DLLs grant read/execute via Users but not via ALL APPLICATION PACKAGES, so every external executable (not just PowerShell/git-bash) died at image init with STATUS_DLL_NOT_FOUND. Disabling only Administrators fixes it without weakening the actual isolation boundary (AppContainer SID + Low IL + deny ACEs). 3. appcontainer.rs: the workspace cwd is canonicalize()'d to the \?\C:\... verbatim-disk form. cmd.exe treats a \-prefixed cwd as UNC and silently falls back to C:\Windows, breaking every child spawn. Strip the \?\ prefix before CreateProcessAsUserW (same object, a spelling cmd accepts). 4. New: Relaxed Sandbox Mode ([tools] windows_relaxed_sandbox / windows_allow_admin, config-driven -- the desktop's ENGINE_ENV_ALLOWLIST strips arbitrary env vars before the spawned engine sees them, so an env var alone does not reach a desktop-launched process). With 1-3 fixed, msys/git-bash, PowerShell, `git init`, and `npm` STILL fail even with an amortized $HOME grant (measured: git init/npm fail even after 66s of granting all of $HOME) -- every Windows mechanism that scopes writes by identity (AppContainer, Low IL, a restricted token) also blocks the reads these tools need at startup. There is no allowlist that gets both. Relaxed mode is an explicit opt-in for trusted_local only: medium integrity, no AppContainer capability, restricted token (admin disabled unless windows_allow_admin is also set AND the host process is itself elevated -- never a silent escalation). Containment becomes Job Object resource limits + no-admin/no-privilege + the network policy, not filesystem confinement -- the same trust model most local agent CLIs use. `contained` (untrusted/remote) sessions are untouched and keep the full AppContainer. Verified end-to-end: ls/pwd/git init/npm/powershell all exit 0 with correct output through the real AppContainerBackend::execute path, driven purely by config (env var explicitly unset in the test).
…ng artifact Live-reproduced with Relaxed Mode active, auto-approve on (approval-gate latency ruled out -- every command in the session returned in under 300ms, confirming the sandbox fixes hold under real agent-driven load): cmd /C powershell -Command "..." -> parse error OR exit 0 + EMPTY stdout whoami && uname -a && pwd (no nested quotes) -> exit 0, correct output cmd.exe's own /C re-tokenizing mangles nested double quotes before PowerShell ever sees the -Command script. Not a sandbox bug (the capture path is raw CreatePipe/ReadFile, no pty anywhere) -- but the agent has no way to know this and will keep retrying a shape that can never work. BashTool::description() (Windows-only via #[cfg(windows)]; non-Windows unchanged) now tells the agent to wrap -Command in single quotes instead (cmd.exe does not treat ' as special) or use -EncodedCommand/-File for longer scripts.
This was referenced Jul 23, 2026
FerroxLabs
pushed a commit
that referenced
this pull request
Jul 28, 2026
Recommendation: do not merge #254 as-is. Take two small fixes re-authored (the %TEMP% scratch_dirs narrowing and the \\?\ cwd strip, both still live at HEAD), drop the SidsToDisable change as superseded by a better upstream fix, and reject Relaxed Sandbox Mode. Relaxed Mode is a hole rather than a tradeoff: its 'trusted_local only' restriction exists only in doc comments with zero code paths, and its two new config keys use plain project.or(global) while allow_no_sandbox, auto_approve and allow_list are clamped in the same function -- so a cloned repo's .wayland-core.toml disables the Windows sandbox. Recorded CRITICAL, siding with the panel minority on Phase 28's own rubric. The contributor found two real bugs nobody upstream fixed and proposed the split themselves; the rejection is of one implementation, not of the work.
FerroxLabs
pushed a commit
that referenced
this pull request
Jul 28, 2026
std::fs::canonicalize returns the verbatim `\\?\C:\...` spelling for every local path on Windows, so a cwd canonicalized anywhere upstream arrives at the AppContainer backend in that form. We passed it to CreateProcessAsUserW unmodified. The command processor reads the leading `\\` as UNC, refuses it as a current directory, and silently defaults to `C:\Windows` -- the child then runs somewhere other than where the caller asked, and nothing reports an error. Rewrite VerbatimDisk to its ordinary drive-letter spelling for that one call. This does not widen the sandbox: `\\?\C:\a` and `C:\a` name the same filesystem object and the AppContainer allow/deny ACEs are applied to the object, not to the spelling. Verbatim-UNC, device and plain UNC paths are deliberately left byte-identical -- those name genuinely remote objects, so stripping their prefix would change which object is named. The strip runs on the wide encoding rather than a to_str() round-trip so a non-UTF-8 filename is handled exactly instead of silently passing through unstripped. is_verbatim_disk_path is un-gated from #[cfg(test)]; it was already the correct Prefix::VerbatimDisk classifier, just unreachable from production. Guards, both asserting observable behaviour rather than that a string helper ran: - resolve_cwd() is extracted so the actual UTF-16 buffer handed to lpCurrentDirectory can be decoded and asserted, including the untouched negatives and the absolute/NULL contract. - live_cwd_verbatim.rs spawns a real child with a canonicalized cwd and asserts where the child says it landed -- and that it is not the Windows directory. Gated like live_fs_acl.rs: require_live_acceptance() asserts on WAYLAND_SANDBOX_LIVE_WINDOWS rather than returning early, so an unset variable fails these instead of silently passing them. Found and reported by frankforges in wayland-core #254, along with the proposal to split that PR. Re-authored here rather than merged: #254's base predates the appcontainer/ module split, so its hunks target a file layout that no longer exists.
FerroxLabs
pushed a commit
that referenced
this pull request
Jul 28, 2026
scratch_dirs() returned vec![canon(temp_dir())] -- the entire host temp tree, granted writable to every sandboxed session. On Windows each writable root is materialized as an inheritable ACE via SetNamedSecurityInfoW per spawn and revoked after, so granting %TEMP% is O(subtree) in cost and enormous in blast radius: a crash between grant and revoke strands an ACE on a directory shared with every other application on the machine. It is also far more authority than a sandboxed child needs -- it could read and rewrite any other process's temp state. Narrow it to a bounded scratch directory inside the temp tree, and key that directory BY TRUST. One shared name would have handed an untrusted Contained session a writable host directory that a Trusted local session also writes to and reads back -- a trust-crossing channel created by the narrowing itself. trusted_local and contained now get sibling directories, never nested, never equal. Two details the narrowing forces: - The directory has to exist for a write grant to be materializable, so it is created. If it cannot be established the grant is EMPTY, never a fallback to %TEMP%: failing closed costs a session its scratch space, failing open would silently restore the defect. - On unix temp_dir() is the shared, world-writable /tmp, so the uid goes in the top component rather than a subdirectory -- a shared parent would let whichever user created it first own the permissions for everyone else. Since /tmp is world-writable another user can also pre-create the name as a symlink, and create_dir_all follows symlinks, so we verify we got a real directory that we own before granting a write ACE to it. Guards assert the property at the public surface (writable_roots()), not on the private helper alone: the whole temp root must not be granted, the bounded dir must actually be granted (so a version granting nothing cannot pass), and no writable root of a Contained session may appear among a Trusted session's. The narrowing was found and proposed by frankforges in wayland-core #254. The per-trust keying is added here; #254 used a single fixed name for both.
FerroxLabs
pushed a commit
that referenced
this pull request
Jul 28, 2026
Found by frankforges on PR #254 and re-authored here; the PR itself is Sean's call and no GitHub action was taken on it. %TEMP% narrowing: scratch_dirs() now takes a WorkspaceTrust and returns a bounded dir rather than the whole host temp tree. Three things beyond a literal re-author -- keyed BY TRUST, because one shared name would have created a trust-crossing writable dir via the narrowing itself; fails closed to an empty grant, never back to %TEMP%; and on unix the uid goes in the top component with a symlink/ownership check, since temp_dir() there is world-writable /tmp and create_dir_all follows symlinks. \?\ cwd strip: resolve_cwd() extracted so the actual lpCurrentDirectory buffer is assertable. Strips on the wide encoding rather than a to_str() round trip, so non-UTF-8 filenames stay exact. Live proof on Windows: the un-fixed child reported C:\WINDOWS, the fixed one the requested directory. Not taken: the $HOME change (upstream's curated allowlist is better), SidsToDisable (superseded by HEAD's 0/null), and no part of Relaxed Sandbox Mode. # Conflicts: # .planning/BACKLOG.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What and why
Fixes the Windows AppContainer sandbox bugs described in FerroxLabs/wayland#922 (three separate reports over three months: #520, #618, #743, plus my own #921 yesterday — shell execution "not working" on Windows). Three of the four fixes are straightforward bug fixes. The fourth introduces a new opt-in posture, Relaxed Sandbox Mode, and needs a maintainer call — see below.
The five changes
crates/wcore-tools/src/workspace_policy.rs—trusted_localno longer puts$HOMEin the AppContainer read allowlist on Windows, andscratch_dirs()grants a small dedicated subdir of%TEMP%instead of the whole thing. Both were inheritable-ACE grants rewritten across their entire subtree on every spawn (SetNamedSecurityInfoW) — measured 35s timeout → ~20ms for the full allowlist. Non-Windows behavior unchanged (#[cfg(not(windows))]keeps$HOME/%TEMP%— bind mounts have no per-file ACL cost there).crates/wcore-sandbox/src/backends/appcontainer.rs—CreateRestrictedTokennow disables onlyBUILTIN\Administrators, notUsers/Authenticated Users. Some System32 DLLs grant read/execute viaUsersbut notALL APPLICATION PACKAGES; disabling it made every external executable fail image init withSTATUS_DLL_NOT_FOUND. Verified by A/B toggle — isolation boundary is unaffected (AppContainer SID + Low IL + deny ACEs are untouched).crates/wcore-sandbox/src/backends/appcontainer.rs— strip the\\?\verbatim-disk prefix from the workspace cwd beforeCreateProcessAsUserW.cmd.exetreats a\\-prefixed cwd as UNC and silently falls back toC:\Windows, breaking every child spawn from a canonicalized workspace root.New: Relaxed Sandbox Mode (
crates/wcore-sandbox/src/lib.rs,crates/wcore-config/src/config.rs,crates/wcore-agent/src/bootstrap.rs, wiring inappcontainer.rs) —[tools] windows_relaxed_sandbox/[tools] windows_allow_admin, opt-in,trusted_localonly.This is not a fix to the existing sandbox — it's a different posture, because the existing one is structurally incompatible with running the toolchain. With 1–3 applied,
git init,npm, msysls/pwd, and PowerShell still failed. We tried amortizing a full$HOMEread grant once per session to dodge the cost from [mutants-nightly] wcore-cron — surviving mutants (2026-06-08) #1 — measured that it still doesn't fixgit init/npmeven after paying 66 seconds for the grant. A/B'ing the AppContainer mechanisms directly showed why: every Windows primitive that scopes writes by identity (the AppContainer SID, Low integrity, a restricted token) also blocks the reads these tools need at startup. There is no allowlist, amortized or not, that gets both — we could not make the stock AppContainer-contained model run the full toolchain, however we configured it.Relaxed Mode is medium integrity, no AppContainer capability, restricted token (disables
Administrators; disables nothing ifwindows_allow_adminis also set — which only grants actual admin if the host process is itself elevated, never silently). Containment becomes Job Object resource limits + no-admin/no-privilege + the existing network policy, not filesystem confinement.contained(untrusted/remote) sessions are completely untouched — full AppContainer stays the default and only default there.Config-driven rather than env-var-only because the desktop's
ENGINE_ENV_ALLOWLIST(in thewaylandrepo) strips arbitrary env vars before the spawned engine process sees them — confirmed this cost us a full extra debugging pass on our own machine. The env var still works for direct CLI use (checked first, config is the fallback), butconfig.tomlis the only channel that reliably reaches a desktop-spawned engine. Flagging for the desktop team too: if there's ever a Settings-UI toggle for this, it should writeconfig.toml(same pattern as the existing[tools] windows_shell), not a new env var.Verified end-to-end via
crates/wcore-sandbox/tests/live_relaxed_windows.rs(new, included):ls/pwd/git init/npm --version/powershell -Commandall exit 0 with correct output through the realAppContainerBackend::executepath.crates/wcore-tools/src/bash.rs— the WindowsBashTool::description()(the LLM-facing tool description, not just docs) now warns the agent about acmd /Cquoting artifact and gives it the fix. Live-reproduced after enabling Relaxed Mode, with auto-approve on (so approval-gate latency is ruled out — every command in that session returned in under 300ms, confirming 1–4 hold under real load):cmd /C powershell -Command "..."either fails with a PowerShell parse error or returnsexit 0with empty stdout, becausecmd's/Cre-tokenizing mangles the nested double quotes before PowerShell ever sees the script. A same-sessionwhoami && uname -a && pwd(no nested quotes) worked and returned correct output — isolating the quoting as the sole variable. Not a sandbox bug (verified the capture path is rawCreatePipe/ReadFile, no pty involved), but worth fixing at the description level since it makes the agent retry a way that will never work. Fix: recommend wrapping-Commandin single quotes (cmd.exedoesn't treat'as special, so nothing gets mangled) or-EncodedCommand/-Filefor longer scripts. New unit test:description_warns_about_powershell_double_quote_mangling.Scope
wcore-sandbox/wcore-tools/wcore-config/wcore-agent(no touched dependency is in the exact-pinned security-boundary list)Checks
cargo test -p wcore-sandbox -p wcore-tools -p wcore-config --lib) — no regressions; the two PowerShell/bash-rejection tests correctly flip behavior when relaxed mode is active (proves the fix, not a break)live_relaxed_windows.rs, gatedWAYLAND_SANDBOX_LIVE_WINDOWS), passes against a real Windows boxTesting notes for reviewers without a native Windows AppContainer box
Every claim above has a
WAYLAND_SANDBOX_LIVE_WINDOWS=1-gated live test backing it (existing pattern in this crate — seelive_fs_acl.rs). Happy to paste the full raw measurement logs from our session if useful for review, or re-run any specific scenario you want checked.