feat(sandbox): unelevated Windows fallback tier instead of prompts-only degrade#427
Merged
kevincodex1 merged 1 commit intoJul 3, 2026
Conversation
…ly degrade Before the one-time elevated `zero sandbox setup` has run, Windows dropped straight from the native sandbox to the in-process policy gate: commands ran with no OS enforcement at all. But only the WFP network filters actually need Administrator rights - the write-restricted token and the workspace ACL grants (DACL edits on user-owned roots) work unelevated. Add an `unelevated` enforcement tier between native and degraded: when the elevated setup marker is missing and the profile restricts the filesystem, the command still wraps through the command runner at a new `unelevated` sandbox level. The runner applies the workspace ACL plan itself (recording applied plans by hash in windows-unelevated-setup.json so repeat commands skip the apply) and launches under the same write-restricted token, so the OS-level write-jail holds. Network enforcement stays with the per-command approval gate at this tier, and the downgrade reason says so; profiles that need the sandbox only for network isolation keep the old degrade behavior, and `--sandbox require` still errors with the setup hint. Verified end to end on Windows 11 without elevation: a write inside the workspace succeeds under the restricted token, a write outside every granted root is denied by the OS (new ZERO_SANDBOX_REAL_SMOKE integration test, using cmd.exe because managed PowerShell cannot initialize its crypto provider under a write-restricted token on some hosts). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
WalkthroughThis PR adds a Windows-specific "unelevated" sandbox enforcement tier between native and degraded. It introduces ChangesWindows unelevated enforcement tier
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant Manager as BuildExecutionRequest
participant Runner as runWindowsSandboxCommand
participant Setup as ensureWindowsUnelevatedSetup
participant Marker as UnelevatedSetupMarker
Caller->>Manager: request execution
Manager->>Manager: check OS sandbox marker
alt marker missing, non-strict, restricted filesystem
Manager-->>Caller: EnforcementUnelevated, wrapped=true, downgradeReason set
Caller->>Runner: run wrapped command (SandboxLevel=Unelevated)
Runner->>Setup: ensureWindowsUnelevatedSetup(config)
Setup->>Marker: loadWindowsUnelevatedSetupMarker
Marker-->>Setup: applied plan present?
alt not applied
Setup->>Setup: applyWindowsACLPlan
Setup->>Marker: recordWindowsUnelevatedAppliedPlan
end
Setup-->>Runner: success/error
Runner-->>Caller: exit code
else marker missing, non-restricted filesystem
Manager-->>Caller: EnforcementDegraded, wrapped=false
end
Suggested reviewers: Fix the Windows ownership error message copy-paste risk and verify 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/sandbox/windows_unelevated.go (1)
79-85: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winA corrupt marker file bricks the unelevated path; make it self-heal like the schema-mismatch case.
An unknown
schemaVersionis deliberately tolerated (reset to empty and re-applied), but a malformed/truncated JSON body returns a hard error that propagates throughensureWindowsUnelevatedSetupand fails the command with exit 1. That contradicts the "never brick the command on the auto path" intent. Since re-applying an ACL plan is documented as harmless, treat a parse failure the same way — reset instead of failing.♻️ Reset on unparseable marker instead of erroring
var marker WindowsUnelevatedSetupMarker if err := json.Unmarshal(bytes, &marker); err != nil { - return WindowsUnelevatedSetupMarker{}, fmt.Errorf("parse windows unelevated setup marker %s: %w", path, err) + // A corrupt marker is treated like an unknown schema: reset to empty so + // the runner re-applies (harmless) and rewrites, rather than bricking the command. + return WindowsUnelevatedSetupMarker{SchemaVersion: windowsUnelevatedSetupMarkerSchemaVersion}, nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/sandbox/windows_unelevated.go` around lines 79 - 85, The WindowsUnelevatedSetupMarker loading path currently hard-fails on malformed JSON, which can brick the unelevated setup flow. Update the marker parse logic in windowsUnelevatedSetupMarker loading so that a json.Unmarshal error is treated the same as an unknown SchemaVersion: return a reset WindowsUnelevatedSetupMarker with windowsUnelevatedSetupMarkerSchemaVersion and no error. Keep ensureWindowsUnelevatedSetup able to continue by self-healing the marker instead of propagating a parse failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/sandbox/windows_unelevated.go`:
- Around line 79-85: The WindowsUnelevatedSetupMarker loading path currently
hard-fails on malformed JSON, which can brick the unelevated setup flow. Update
the marker parse logic in windowsUnelevatedSetupMarker loading so that a
json.Unmarshal error is treated the same as an unknown SchemaVersion: return a
reset WindowsUnelevatedSetupMarker with
windowsUnelevatedSetupMarkerSchemaVersion and no error. Keep
ensureWindowsUnelevatedSetup able to continue by self-healing the marker instead
of propagating a parse failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8c943c2b-29fb-4a16-8cef-ab2ee62d82ff
📒 Files selected for processing (8)
internal/sandbox/manager.gointernal/sandbox/runner_windows_integration_test.gointernal/sandbox/types.gointernal/sandbox/windows_command_runner_windows.gointernal/sandbox/windows_degrade_test.gointernal/sandbox/windows_runner.gointernal/sandbox/windows_unelevated.gointernal/sandbox/windows_unelevated_test.go
anandh8x
approved these changes
Jul 3, 2026
anandh8x
left a comment
anandh8x
left a comment
Collaborator
There was a problem hiding this comment.
Solid design and execution. The insight that only WFP filters need elevation — filesystem ACLs and restricted tokens work unelevated — is correct, and the implementation handles the edge cases well: network-only profiles still degrade, --sandbox require still errors, marker dedup by hash avoids redundant applies, and eviction caps prevent unbounded growth.
One minor thing: a corrupt marker JSON returns a hard error where the schema-mismatch path self-heals. Consistent self-healing would be better, but corrupt markers are an anomaly rather than a routine state — not blocking.
LGTM.
Contributor
Author
kevincodex1
pushed a commit
that referenced
this pull request
Jul 3, 2026
A malformed/truncated windows-unelevated-setup.json returned a hard error from loadWindowsUnelevatedSetupMarker, bricking every unelevated command until the file was hand-deleted, while the unknown-schema path already reset to an empty marker. Treat corrupt JSON the same way: the marker only memoizes idempotent ACL applies, so a reset can only cost redundant work, never skip enforcement. Addresses the review feedback from coderabbitai and @anandh8x on #427. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
kevincodex1
pushed a commit
that referenced
this pull request
Jul 5, 2026
…oken (#456) * fix(sandbox): fix nested pipe creation under the Windows restricted token Any subprocess spawned FROM WITHIN a process already running under Zero's write-restricted token, that captures the child's output via a pipe, failed with ERROR_ACCESS_DENIED. Reproduces with `gh pr list` (gh shells out to git and captures its output), with a bare Go program doing exec.Command(...). Output(), and with cmd.exe's own FOR /F ('...') construct, which uses the identical CreatePipe+CreateProcess pattern internally with no external tools involved. Root cause: a WRITE_RESTRICTED token requires a WRITE-type access check to match BOTH the normal enabled-SID grant and a separate grant to one of the token's restricted SIDs. CreateRestrictedToken does not modify the default DACL it inherits from the base token, so a pipe created with a default security descriptor (the common case; every language runtime's "spawn a subprocess and capture output" primitive does this) only carries ACEs for the base identity, none of which are in the restricted-SID list, so the second check fails. This is the same failure family already documented for Schannel/SEC_E_NO_CREDENTIALS, but broader in scope: it affects any pipe, event, mutex, or other kernel object created with default security by a process running under the token. Fix: add the token's own logon SID (already unconditionally present in its restricted-SID list) to the token's default DACL, so objects it creates without an explicit security descriptor automatically satisfy the extra check. This does not touch filesystem access: NTFS write-jailing is enforced by the explicit ACL grants applied to workspace paths, created WITH an explicit security descriptor, and the token's default DACL is only consulted when none is supplied. Exposure is bounded to the same logon session (only other processes running as the same signed-in user could, in principle, open a NAMED object the sandboxed process creates with default security; anonymous pipes have no name and are reachable only via an inherited handle, so this is a no-op for the actual objects this fixes). This is not specific to the unelevated tier added in #427 - the affected token-construction code is shared by both the fully elevated and unelevated restricted-token paths, so this predates that work. Verified on Windows 11 (before/after, using an unfixed build for comparison): - Before: FOR /F fails with Win32 error 5 (ERROR_ACCESS_DENIED), masked behind cmd.exe's generic "not recognized" message; `gh pr list` fails with "failed to run git: pipe: Access is denied."; a bare Go nested exec.Command(...).Output() fails with "pipe: Access is denied." - After: all three succeed; `gh pr list` returns real PR data. - Write-jail regression check: a write outside every granted root is still denied by the OS after the fix (unchanged from before). New TestWindowsRestrictedTokenNestedPipeCapture (ZERO_SANDBOX_REAL_SMOKE=1) pins the FOR /F case using only cmd.exe, no external dependency. go vet and the full internal/sandbox suite are clean (excluding pre-existing failures already present identically on unmodified main, unrelated to this change). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(sandbox): fix dangling-buffer use-after-scope in default DACL merge Addresses the blocker from review on this PR (Vasanthdev2004, gnanam1990, coderabbitai): windowsTokenDefaultDacl returned a *windows.ACL that points INTO the local buf byte slice backing TOKEN_DEFAULT_DACL - Windows embeds the ACL data in the same allocation GetTokenInformation fills. The function's own runtime.KeepAlive(buf) only covered buf up to that function's own return; nothing kept it alive across the caller's later use. In broadenWindowsRestrictedTokenDefaultDacl, the returned ACL pointer was then dereferenced by native code in setEntriesInACL after intervening allocations (the EXPLICIT_ACCESS literal, TrusteeValueFromSID) that are GC-safepoint-eligible - a real use-after-free window in code that constructs part of the sandbox's own restricted token, even though Go's current non-moving GC meant it didn't manifest in testing. windowsTokenDefaultDacl now returns the backing buffer alongside the ACL; the caller holds runtime.KeepAlive(oldDaclBuf) until after SetEntriesInAclW (the last point that dereferences it) returns. Also from review: SetEntriesInAclW reports its Win32 error directly in the return value, not via GetLastError, so setEntriesInACL now uses that return value directly instead of the (potentially stale/unrelated) syscall error. And narrowed the default-DACL grant from GENERIC_ALL to GENERIC_READ | GENERIC_WRITE - the second WRITE_RESTRICTED access check only needs a read/write match; GENERIC_ALL also implied DELETE/WRITE_DAC/WRITE_OWNER that the pipe/event/mutex use case never needed. Re-verified on Windows 11 with the fixed runner: nested pipe capture still succeeds (pure Go exec.Command(...).Output(), and TestWindowsRestrictedTokenNestedPipeCapture via go test itself), and the write-jail regression check (write denied outside every granted root, allowed inside) is unchanged. Full internal/sandbox suite green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
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
3 participants
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
Adds an
unelevatedenforcement tier to the Windows sandbox: when the elevatedzero sandbox setuphas not run, commands with a restricted-filesystem profile still wrap through the command runner under the write-restricted token, with the runner applying the workspace ACL plan itself (no Administrator rights needed). Previously this state dropped to the in-process policy gate with no OS enforcement.Why
Only the WFP network filters actually require elevation. The write-restricted token (
CreateRestrictedToken(WRITE_RESTRICTED)) and the workspace ACL grants (DACL edits on user-owned roots) work from a normal process, so the filesystem write-jail can and should hold on fresh installs and non-admin machines. Same elevated/unelevated split Codex ships on Windows.How
EnforcementUnelevatedbetweennativeanddegraded(types.go); the manager selects it when the elevated marker is missing, the preference isauto, and the profile restricts the filesystem (manager.go). Network-only profiles keep the old degrade;--sandbox requirestill errors with the setup hint.WindowsSandboxLevelUnelevatedrunner level (windows_runner.go): same restricted-token launch path, but instead of validating the elevated setup marker, the runner applies the ACL plan via the existingapplyWindowsACLPlanand records it (keyed byWindowsACLPlanHash) inwindows-unelevated-setup.jsonso repeat commands skip the apply (windows_command_runner_windows.go,windows_unelevated.go). Grants are left in place like the elevated setup's; they only name synthetic capability SIDs no other token carries.Testing
go test ./internal/sandbox/: green (two pre-existing failures on Windows hosts,TestPermissionProfileFromPolicyIncludesDefaultTempWriteRootsandTestNewScopeNormalizesAndValidatesExtraRoots, fail identically onmain; they assume POSIX/tmp).TestWindowsUnelevatedRealSandboxSmoke(gated byZERO_SANDBOX_REAL_SMOKE=1), verified on real Windows 11 without elevation: write inside the workspace succeeds under the restricted token; write outside every granted root is denied by the OS; marker recorded. Usescmd.exebecause managed PowerShell cannot initialize its crypto provider under a write-restricted token on some hosts (0x8009001d, same restricted-token family as the Schannel limitation already documented in the runner).go build ./...,go vet ./internal/sandbox/..., gofmt clean.Limitations (by design, stated in the downgrade reason)
--sandbox forbid.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes