fix(sandbox): make Windows token fully restricted to enforce DenyRead policies - #612
Merged
kevincodex1 merged 1 commit intoJul 10, 2026
Conversation
On Windows, creating restricted tokens with windowsWriteRestricted (0x08) instructs the kernel to skip checking the restricted SIDs list for read operations. This bypasses DenyRead path policies on NTFS. Remove the flag to enforce restricted SID constraints on both read and write operations, and add integration test assertions to verify that DenyRead paths are correctly blocked.
WalkthroughRemoves the WRITE_RESTRICTED flag from the CreateRestrictedToken attribute mask in createWindowsRestrictedTokenFromBase, and extends the Windows unelevated sandbox integration test with a new DenyRead filesystem policy assertion that verifies reads from a denied directory fail with exit code 1. ChangesWindows sandbox token and test update
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Test as TestWindowsUnelevatedRealSandboxSmoke
participant Policy as FileSystemPolicy
participant Sandbox as SandboxRunner
participant Cmd as cmd.exe
Test->>Test: create private dir + secret.txt
Test->>Policy: add DenyRead entry for private dir
Test->>Sandbox: run cmd.exe "type secret.txt"
Sandbox->>Cmd: execute under restricted token
Cmd-->>Sandbox: exit code 1 (access denied)
Sandbox-->>Test: return exit code 1
Test->>Test: assert exit code == 1
Possibly related PRs
Suggested reviewers: 🚥 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_token_windows.go (1)
108-108: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate the stale restricted-token comments The token creation now uses
windowsDisableMaxPrivilege|windowsLUAToken, so theWRITE_RESTRICTEDwording inbroadenWindowsRestrictedTokenDefaultDaclis stale. Replace it with “restricted token” / “restricted-SID access check”; ifwindowsWriteRestrictedis only kept for the old path, remove it too.🤖 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_token_windows.go` at line 108, The restricted-token handling in broadenWindowsRestrictedTokenDefaultDacl is still described using stale WRITE_RESTRICTED wording, but the creation path now uses windowsDisableMaxPrivilege and windowsLUAToken. Update the comments to refer to a restricted token and restricted-SID access check instead, and remove windowsWriteRestricted if it is only used for the old path so the terminology matches the current token behavior.
🤖 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_token_windows.go`:
- Line 108: The restricted-token handling in
broadenWindowsRestrictedTokenDefaultDacl is still described using stale
WRITE_RESTRICTED wording, but the creation path now uses
windowsDisableMaxPrivilege and windowsLUAToken. Update the comments to refer to
a restricted token and restricted-SID access check instead, and remove
windowsWriteRestricted if it is only used for the old path so the terminology
matches the current token behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7db0c7d5-db3f-4dec-859c-64518b09df65
📒 Files selected for processing (2)
internal/sandbox/runner_windows_integration_test.gointernal/sandbox/windows_token_windows.go
anandh8x
approved these changes
Jul 9, 2026
anandh8x
left a comment
anandh8x
left a comment
Collaborator
There was a problem hiding this comment.
LGTM. The fix is correct and the regression test pins it.
The WRITE_RESTRICTED flag on CreateRestrictedToken enforces restricted-SID deny-ACE only for writes, not reads, so any DenyRead policy tied to the capability SIDs was silently bypassed on read paths. Removing the flag makes the token fully restricted on both paths. The new test (TestWindowsUnelevatedRealSandboxSmoke) writes a secret into a DenyRead subdir and asserts cmd type returns exit 1, which is the right way to assert the deny actually fires.
Two files, one line of code, one real-world test. No collateral changes to the flag set, the call shape, or the DACL construction. Ship it.
One thing for the commit body (not blocking): the flag change tightens read enforcement for every Windows sandbox invocation, not just DenyRead paths. Worth a one-liner noting the broader posture change so future readers don't undo it on a hunch.
gnanam1990
approved these changes
Jul 9, 2026
gnanam1990
left a comment
gnanam1990
left a comment
Collaborator
There was a problem hiding this comment.
Review
Verdict: Approve with notes. Agree with @anandh8x on the core fix.
Correct
Removing windowsWriteRestricted from CreateRestrictedToken is the right fix: under WRITE_RESTRICTED, restricted-SID checks apply only to writes, so capability-SID DenyRead ACEs never blocked reads. The unelevated smoke addition is the right shape for a regression pin.
Notes (non-blocking but should not be ignored)
-
Posture change is broader than DenyRead. Fully restricted tokens check restricted SIDs on all access. That can deny reads outside capability/World/logon grants and sits in tension with the read-all/write-jail model in
profile.go. Please document this (commit/PR body), and run the real Windows smokes (Unelevated,RestrictedToken, nested pipe) withZERO_SANDBOX_REAL_SMOKE=1before merge if not already done on a Windows host. -
CI does not run the new pin by default. The assertion is only in
TestWindowsUnelevatedRealSandboxSmoke.scripts/sandbox-smoke.shrunsTestWindowsRestrictedTokenRealSandboxSmokeonly. Please wire the unelevated (or a DenyRead case on the restricted-token smoke) into the path that actually runs, and consider a positive control that allowed reads still succeed. -
Cleanup:
windowsWriteRestrictedis dead;broadenWindowsRestrictedTokenDefaultDaclcomments still say WRITE_RESTRICTED-only — reword so nobody removes the DACL broaden thinking the flag is gone. Broaden still looks necessary under full restriction. -
Nit: quote the
typepath and optionally assert secret bytes are absent from output. -
Process: no linked parent issue /
issue-approved— please open/link one for the DenyRead bypass audit trail.
No disagreement with the APPROVE on the one-line security fix.
kevincodex1
approved these changes
Jul 10, 2026
This was referenced Jul 10, 2026
kevincodex1
pushed a commit
that referenced
this pull request
Jul 13, 2026
…onfigured (#658) Removing the WRITE_RESTRICTED flag in #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 #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 #612's guarantee holds for the profiles that rely on it.
Vasanthdev2004
added a commit
that referenced
this pull request
Aug 4, 2026
…paths (#865) * fix(sandbox): stop the Windows write jail honouring Everyone-granted paths The restricted token put the World SID (S-1-1-0, Everyone) in its restricted-SID list unconditionally, from the original sandbox baseline and with no comment saying why. A WRITE_RESTRICTED token runs two checks for a write and needs both: the normal one against its enabled SIDs, and a second against the restricted list. The write jail rests entirely on that second check only succeeding where Zero has explicitly ACL'd a capability SID. Everyone is a SID every principal carries, so on any path whose DACL grants Everyone write, the restricted half passed for free and confinement fell back to the ordinary user's own permissions -- the exact boundary this token exists to be stricter than. Reachable with no privilege, no symlink and no race: an Everyone-writable directory is enough, and share roots opened with Everyone:F and loose third-party installer ACLs supply them. Ruled out as the common case: C:\Users\Public\Documents grants BATCH, not Everyone. The runner already stated the rule this broke -- it declines to add the user SID, Administrators or SYSTEM because "each has write access nearly everywhere" -- while listing Everyone among the restricted SIDs. Removed only from the WRITE_RESTRICTED token, because without that flag it is load-bearing rather than gratuitous. The restricted-SID check then applies to reads too, and default Windows DACLs grant BUILTIN\Users rather than anything in this list, so a token with no Everyone cannot open cmd.exe and dies at launch with STATUS_ACCESS_DENIED (0xC0000022). Dropping it there was tried first and takes TestWindowsUnelevatedRealSandboxSmoke with it. That path is taken only when a profile configures DenyRead, which is already the posture trading capability for read-deny enforcement (#612), so the bypass survives there deliberately and narrowly; closing it needs a read-side grant that is not a universal group. Verified against real tokens rather than reasoned about. The new smoke test drives the real runner and asserts three outcomes that only agree when the jail is intact: a granted root is writable, an ordinary outside path is denied, and an Everyone-writable outside path is denied too. Before the change its third case wrote the file with exit 0; the first two were already correct, which is what made the gap invisible. Restoring the SID unconditionally turns it red again. * test(sandbox): make a denied write distinguishable from a runner failure The denial assertions expected exit 1, which the runner also returns for its own errors -- a failed marker validation, a bad argument, a token it could not build. Written that way the test passes both when the sandbox denies the write and when nothing ever ran, and the second case proves nothing while being indistinguishable from success. That is a poor property for any test and a bad one for this test, whose entire job is to notice a confinement regression. The leading "granted root is writable" assertion covered part of it, but said nothing about whether the two later commands launched. The denial commands now report a distinctive exit code the runner cannot produce for itself, so the code proves cmd.exe actually ran and its redirect was refused. Re-checked in both directions rather than assumed: the test passes against the fix, and restoring the World SID unconditionally fails it with "exit code = 0, want 77" -- still catching the bug it was written for. Raised by CodeRabbit on #865.
Vasanthdev2004
added a commit
that referenced
this pull request
Aug 21, 2026
… DenyRead trade #865 removed the World SID from the WRITE_RESTRICTED token, which is what makes the write jail hold: every principal carries Everyone, so while it was a restricting SID the write half of the check passed for free on any Everyone-writable path. That fix had no CI protection. The only test covering it sits behind ZERO_SANDBOX_REAL_SMOKE=1 and no workflow sets that variable, so a rebase or refactor restoring the unconditional World SID goes green. #640's branch conflicts on exactly that hunk. CreateRestrictedToken works unelevated against the caller's own token, so the invariant can be checked in an ordinary unit test. Added four: - the WRITE_RESTRICTED token must not carry the World SID (this fails against a reverted #865, verified by mutation) - neither token shape may carry Users, Authenticated Users, INTERACTIVE, BATCH, Administrators, SYSTEM, SERVICE, NETWORK, or the user's own SID. #869 names these as the ones that would reopen the same class of bypass - the capability SID must be present, so a token with an empty list cannot pass by having no keys at all - the non-WRITE_RESTRICTED shape still carries the World SID, which documents the open gap rather than asserting the end state. It skips with a note if that stops being true, so whoever closes #869 is told to replace it Second half: the trade was invisible. Setting denyRead selects the token shape without WRITE_RESTRICTED, and nothing told the person who set it that they had given up write confinement to get read-deny. The plan now carries a warning saying so, keyed off the same field the runner reads and scoped to the Windows restricted-token backend, so the default posture stays quiet. Zero never populates denyRead on Windows itself, so this only reaches users who configured it. This does NOT close #869. Closing it needs a read-side grant that is not a universal group (AppContainer, or the per-workspace principals in #808), which is a different piece of work. What changes here is that the fixed shape can no longer regress silently, and the unfixed shape no longer looks enforced. Refs #869, #865, #612, #640
Vasanthdev2004
added a commit
that referenced
this pull request
Aug 22, 2026
… DenyRead trade #865 removed the World SID from the WRITE_RESTRICTED token, which is what makes the write jail hold: every principal carries Everyone, so while it was a restricting SID the write half of the check passed for free on any Everyone-writable path. That fix had no CI protection. The only test covering it sits behind ZERO_SANDBOX_REAL_SMOKE=1 and no workflow sets that variable, so a rebase or refactor restoring the unconditional World SID goes green. #640's branch conflicts on exactly that hunk. CreateRestrictedToken works unelevated against the caller's own token, so the invariant can be checked in an ordinary unit test. Added four: - the WRITE_RESTRICTED token must not carry the World SID (this fails against a reverted #865, verified by mutation) - neither token shape may carry Users, Authenticated Users, INTERACTIVE, BATCH, Administrators, SYSTEM, SERVICE, NETWORK, or the user's own SID. #869 names these as the ones that would reopen the same class of bypass - the capability SID must be present, so a token with an empty list cannot pass by having no keys at all - the non-WRITE_RESTRICTED shape still carries the World SID, which documents the open gap rather than asserting the end state. It skips with a note if that stops being true, so whoever closes #869 is told to replace it Second half: the trade was invisible. Setting denyRead selects the token shape without WRITE_RESTRICTED, and nothing told the person who set it that they had given up write confinement to get read-deny. The plan now carries a warning saying so, keyed off the same field the runner reads and scoped to the Windows restricted-token backend, so the default posture stays quiet. Zero never populates denyRead on Windows itself, so this only reaches users who configured it. This does NOT close #869. Closing it needs a read-side grant that is not a universal group (AppContainer, or the per-workspace principals in #808), which is a different piece of work. What changes here is that the fixed shape can no longer regress silently, and the unfixed shape no longer looks enforced. Refs #869, #865, #612, #640
Vasanthdev2004
added a commit
that referenced
this pull request
Aug 27, 2026
… DenyRead trade #865 removed the World SID from the WRITE_RESTRICTED token, which is what makes the write jail hold: every principal carries Everyone, so while it was a restricting SID the write half of the check passed for free on any Everyone-writable path. That fix had no CI protection. The only test covering it sits behind ZERO_SANDBOX_REAL_SMOKE=1 and no workflow sets that variable, so a rebase or refactor restoring the unconditional World SID goes green. #640's branch conflicts on exactly that hunk. CreateRestrictedToken works unelevated against the caller's own token, so the invariant can be checked in an ordinary unit test. Added four: - the WRITE_RESTRICTED token must not carry the World SID (this fails against a reverted #865, verified by mutation) - neither token shape may carry Users, Authenticated Users, INTERACTIVE, BATCH, Administrators, SYSTEM, SERVICE, NETWORK, or the user's own SID. #869 names these as the ones that would reopen the same class of bypass - the capability SID must be present, so a token with an empty list cannot pass by having no keys at all - the non-WRITE_RESTRICTED shape still carries the World SID, which documents the open gap rather than asserting the end state. It skips with a note if that stops being true, so whoever closes #869 is told to replace it Second half: the trade was invisible. Setting denyRead selects the token shape without WRITE_RESTRICTED, and nothing told the person who set it that they had given up write confinement to get read-deny. The plan now carries a warning saying so, keyed off the same field the runner reads and scoped to the Windows restricted-token backend, so the default posture stays quiet. Zero never populates denyRead on Windows itself, so this only reaches users who configured it. This does NOT close #869. Closing it needs a read-side grant that is not a universal group (AppContainer, or the per-workspace principals in #808), which is a different piece of work. What changes here is that the fixed shape can no longer regress silently, and the unfixed shape no longer looks enforced. Refs #869, #865, #612, #640
Vasanthdev2004
added a commit
that referenced
this pull request
Aug 29, 2026
… DenyRead trade #865 removed the World SID from the WRITE_RESTRICTED token, which is what makes the write jail hold: every principal carries Everyone, so while it was a restricting SID the write half of the check passed for free on any Everyone-writable path. That fix had no CI protection. The only test covering it sits behind ZERO_SANDBOX_REAL_SMOKE=1 and no workflow sets that variable, so a rebase or refactor restoring the unconditional World SID goes green. #640's branch conflicts on exactly that hunk. CreateRestrictedToken works unelevated against the caller's own token, so the invariant can be checked in an ordinary unit test. Added four: - the WRITE_RESTRICTED token must not carry the World SID (this fails against a reverted #865, verified by mutation) - neither token shape may carry Users, Authenticated Users, INTERACTIVE, BATCH, Administrators, SYSTEM, SERVICE, NETWORK, or the user's own SID. #869 names these as the ones that would reopen the same class of bypass - the capability SID must be present, so a token with an empty list cannot pass by having no keys at all - the non-WRITE_RESTRICTED shape still carries the World SID, which documents the open gap rather than asserting the end state. It skips with a note if that stops being true, so whoever closes #869 is told to replace it Second half: the trade was invisible. Setting denyRead selects the token shape without WRITE_RESTRICTED, and nothing told the person who set it that they had given up write confinement to get read-deny. The plan now carries a warning saying so, keyed off the same field the runner reads and scoped to the Windows restricted-token backend, so the default posture stays quiet. Zero never populates denyRead on Windows itself, so this only reaches users who configured it. This does NOT close #869. Closing it needs a read-side grant that is not a universal group (AppContainer, or the per-workspace principals in #808), which is a different piece of work. What changes here is that the fixed shape can no longer regress silently, and the unfixed shape no longer looks enforced. Refs #869, #865, #612, #640
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
4 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.
Summary
Fixes a security vulnerability on Windows where sandboxed processes could completely bypass
DenyReadsandbox file policies.When creating the restricted token, we previously passed the
windowsWriteRestrictedflag (0x08). Under this mode, the Windows kernel only checks the restricted SID list during write operations, skipping it entirely for read operations. Consequently, NTFS deny-read ACEs associated with the capability SIDs were ignored, allowing the sandboxed process to read files located inDenyReaddirectories.This PR removes the
windowsWriteRestrictedflag so that the token is fully restricted, enforcing SID constraints for both read and write operations.Changes
internal/sandbox/windows_token_windows.gowindowsWriteRestrictedfrom the flags passed toprocCreateRestrictedToken.Call.internal/sandbox/runner_windows_integration_test.goTestWindowsUnelevatedRealSandboxSmoketo write a secret file to aDenyReadsubdirectory, and assert that attempting to type/read it from the sandboxed process fails with an access-denied exit code.Test plan
go test ./internal/sandbox/...— okSummary by CodeRabbit