fix(sandbox): use WRITE_RESTRICTED token when no DenyRead paths are configured#658
Conversation
…onfigured Removing the WRITE_RESTRICTED flag in Gitlawb#612 made the restricted-SID check apply to reads as well as writes. Default Windows DACLs grant BUILTIN\Users rather than any SID in the token's restricted list (random capability SIDs, logon SID, Everyone), so the sandboxed process could no longer open any executable or non-KnownDlls DLL: every spawned command failed with exit 1 and no output, including cmd builtins' children (gh, git, where.exe). The existing TestWindowsRestrictedTokenNestedPipeCapture smoke test reproduces the regression. Restore WRITE_RESTRICTED, but only when the permission profile has no DenyRead paths. The flag makes the kernel skip restricted-SID deny ACEs for reads, which is exactly the DenyRead bypass Gitlawb#612 fixed, so profiles that configure DenyRead keep the fully restricted token and trade spawn capability for read-deny enforcement. DenyRead is empty by default, so the common case regains a working sandbox while Gitlawb#612's guarantee holds for the profiles that rely on it.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughChangesWindows token access control
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant SandboxCommandRunner
participant RestrictedTokenBuilder
participant WindowsCreateRestrictedToken
SandboxCommandRunner->>RestrictedTokenBuilder: derive writeRestricted from DenyRead
RestrictedTokenBuilder->>WindowsCreateRestrictedToken: pass computed token flags
WindowsCreateRestrictedToken-->>SandboxCommandRunner: return restricted token
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
I read through the token logic in internal/sandbox/windows_token_windows.go and the call site in windows_command_runner_windows.go. The gating is right: WRITE_RESTRICTED is only set when DenyRead is empty, so profiles that rely on read-deny keep the strict post-#612 token, and the flag never relaxes write checks — it only stops the restricted-SID check for reads, so the workspace write-jail stays intact. Build, vet, gofmt, and the sandbox package tests are clean on Windows and the branch merges cleanly. Approving.
One note: TestWindowsUnelevatedRealSandboxSmoke and TestWindowsRestrictedTokenNestedPipeCapture both stay skipped without ZERO_SANDBOX_REAL_SMOKE=1, so neither the spawn-capability fix nor the read-deny path is exercised in default CI — worth a manual smoke run before this lands.
3 participants
Problem
Since #612 removed the WRITE_RESTRICTED flag, the Windows sandbox token applies the restricted-SID check to reads as well as writes. Default Windows DACLs grant BUILTIN\Users, not any SID in the token's restricted list (the random capability SIDs, the logon SID, and Everyone). The result is that a wrapped command cannot open any executable or non-KnownDlls DLL anywhere on the system:
The env-gated smoke test TestWindowsRestrictedTokenNestedPipeCapture (spawns whoami.exe from inside the sandbox) reproduces this on current main.
Fix
Restore WRITE_RESTRICTED, but only when the permission profile has no DenyRead paths. The flag makes the kernel skip restricted-SID deny ACEs for reads, which is exactly the DenyRead bypass #612 fixed, so profiles that configure DenyRead keep the fully restricted token and trade spawn capability for read-deny enforcement. DenyRead is empty by default, so the common case gets a working sandbox back while the #612 guarantee holds for the profiles that rely on it.
Verification (Windows 11, real smoke tests)
With ZERO_SANDBOX_REAL_SMOKE=1 and the runner built from this branch:
cmd /d /c "git --version & gh --version"under an unelevated restricted-token profile prints both versions and exits 0; on main it exits 1 with no output.go build ./...andgo test ./internal/sandbox ./internal/tools ./internal/agentpass.Relation to #640
#640 attacks the same regression by adding Users and Authenticated Users to the restricted-SID list. That restores executable reads, but it also widens the write jail to every location where those groups hold write access, and it still blocks reads of per-user files (profile directories grant the individual user, not the Users group), so gh can spawn but cannot read its own config. This PR keeps the write jail scoped to the capability SIDs and restores all reads the user can normally perform. Happy to close one in favor of the other.
Summary by CodeRabbit