fix(windows): add Users and Authenticated Users SIDs to restricted token SIDs#640
fix(windows): add Users and Authenticated Users SIDs to restricted token SIDs#640euxaristia wants to merge 8 commits into
Conversation
|
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)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughWindows restricted-token ACL planning now adds non-inheriting shared-directory deny-write entries, while token creation conditionally broadens read SIDs by sandbox level. Windows ACL tests and integration smoke tests cover restricted-token, unelevated, Public, and ProgramData behavior. ChangesWindows sandbox enforcement
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SandboxCommand
participant BuildWindowsACLPlan
participant SharedPathResolver
participant RestrictedToken
participant SharedDirectory
SandboxCommand->>BuildWindowsACLPlan: Build restricted-token ACL plan
BuildWindowsACLPlan->>SharedPathResolver: Resolve shared Windows paths
SharedPathResolver-->>BuildWindowsACLPlan: Return system and shared roots
BuildWindowsACLPlan->>RestrictedToken: Add deny-write entries for capability SID
SandboxCommand->>RestrictedToken: Create token with broadenReadSIDs
RestrictedToken->>SharedDirectory: Attempt sandboxed write
SharedDirectory-->>RestrictedToken: Reject write
Possibly related issues
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.
Reviewed this against #612 (which removed the windowsWriteRestricted flag so the restricting-SID access check runs on both reads and writes, fixing the DenyRead bypass). I traced the security tradeoff carefully.
What I verified:
- The #612 fix is preserved.
createWindowsRestrictedTokenFromBasestill callsCreateRestrictedTokenwithwindowsDisableMaxPrivilege|windowsLUATokenonly (windows_token_windows.go:118) —windowsWriteRestricted(0x08) is not re-added, so check-2 of the restricting SIDs runs for read and write. - DenyRead is NOT reintroduced. DenyRead is enforced by a Deny ACE for the read-deny capability SID, which is itself in the restricting-SID set (windowsRuntimeTokenSIDs -> capabilitySIDs). Deny ACEs override Allow ACEs in check-2, so adding Users/Authenticated Users — which only match Allow ACEs on DenyRead paths — does not let reads past the deny. The #612 integration assertion (TestWindowsUnelevatedRealSandboxSmoke, runner_windows_integration_test.go:192-205) still holds.
- The read fix is legitimate. After #612, check-2 runs on reads, and the old restricting list {capability SIDs, logon SID, world SID} could not read binaries in C:\Program Files / C:\Windows, whose ACLs grant read/execute to BUILTIN\Users / Authenticated Users rather than to Everyone. That broke the documented "full disk is readable" posture (profile.go:104-108) and stopped go/python/node from running out of Program Files. Adding these two SIDs to the restricting list makes check-2 pass for those reads, which restores the intended read-all behavior.
Build/vet/test: go build ./..., go vet ./..., and go test ./internal/sandbox/... all pass; gofmt clean.
The concern that blocks me:
The restricting-SID access check can't scope a SID to read-only — a restricting SID grants whatever the object's DACL grants to it, for reads AND writes. The write jail is enforced solely by check-2: there is no blanket deny-elsewhere ACL, only AllowWrite on workspace roots plus explicit DenyWrite/DenyRead entries (BuildWindowsACLPlan / applyWindowsACLPlan). So adding two well-known SIDs that carry real WRITE grants widens the jail.
I checked the actual ACLs on this host with icacls:
C:\ProgramDatagrantsBUILTIN\Users:(CI)(WD,AD,WEA,WA)— Users can create files/subdirectories.C:\grantsNT AUTHORITY\Authenticated Users:(AD)— Authenticated Users can create top-level directories.
Before this PR, check-2 failed for writes to those paths (no restricting SID matched the Users/AuthUsers grant), so the writes were denied. After this PR, check-2 passes, so a sandboxed process can create files in C:\ProgramData and new folders under C:, all outside the workspace. This is a demonstrable widening of the write jail — the sandbox's core property.
The codebase already documents this principle. windows_command_runner_windows.go:54-70 states, for the MSYS signal-pipe SIDs, that "None of the granted SIDs can be added to the restricted list without collapsing the write jail (each has write access nearly everywhere)." The same reasoning applies to BUILTIN\Users and Authenticated Users for shared system paths.
Requesting changes. To land this I'd want one of:
- Add explicit DenyWrite ACEs for the affected shared roots (e.g. C:, C:\ProgramData, and any other Users-writable system path the sandbox should not write to) in BuildWindowsACLPlan, so the deny-ACE override restores the jail despite the added SIDs; plus an integration assertion that a write to a Users-writable shared path outside the workspace is blocked. Or
- If the widening is judged acceptable for the unelevated tier, state that explicitly in the PR body and add a test that asserts the new boundary (e.g. a write to C:\ProgramData is now permitted-by-design, or explicitly denied if mitigated), so the posture change is recorded rather than implicit.
I'm comfortable with the read-side intent and confirmed DenyRead stays intact — my objection is only that the write-side tradeoff is real, unmitigated, and not acknowledged in the change.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Keep global system ACL changes out of the unelevated setup path
internal/sandbox/windows_acl.go:82
BuildWindowsACLPlannow unconditionally putsC:\\,%ProgramData%, and%SystemRoot%\\Tempin every plan. The unelevated runner applies that exact plan before launching the command, andapplyWindowsACLPlanfails the setup whenSetNamedSecurityInfocannot update a target DACL. Ordinary users do not haveWRITE_DACon those system-owned directories, so an unelevated sandbox command now aborts with access denied before it starts; the tier is specifically intended to require edits only to user-owned workspace/temp roots. Do not apply these global DACL mutations from the unelevated runner (or use an enforcement mechanism that does not require administrator rights), and cover a non-admin run. -
[P1] Preserve the deny below the system drive instead of making it root-only
internal/sandbox/windows_acl.go:113
A normal workspace or%TEMP%write root lies underC:\\, causing this code to setNoInheriton theC:\\deny. The ACL builder then emits an ACE with zero inheritance, so it protects only the drive root; the two direct denies cover only ProgramData and Windows Temp. Since this PR addsBUILTIN\\UsersandAuthenticated Usersto the restricting SID set, any other shared child that grants either group write (for exampleC:\\Users\\Public) passes the restricted-token check and can be modified outside the configured write roots. Retain a deny that covers non-carved-out descendants (with a safe explicit allowed-root exception) and add a real-Windows regression probe for an independent shared writable child.
Only the elevated restricted-token tier (zero sandbox setup, run as Administrator) now gets WinBuiltinUsersSid/WinAuthenticatedUserSid on the restricted token. That tier is also the only one with the WRITE_DAC needed to enforce BuildWindowsACLPlan's DenyWrite mitigation on shared system paths, so the unelevated tier keeps the narrower pre-widening SID set instead of aborting every command with access denied when it can't edit C:\, ProgramData, or Windows\Temp. Also add C:\Users\Public as an explicit DenyWrite target: inheriting a deny from C:\ never actually protected pre-existing children like it, since NTFS does not retroactively propagate inherited ACEs onto objects that already exist. Drop the NoInherit toggle that tried to route around this by disabling inheritance whenever a write root sat under C:\; it wasn't needed, since a write root's own explicit Allow ACE already takes precedence over anything inherited by canonical ACE ordering.
|
Pushed a fixup addressing both P1 findings. Root cause: the DenyWrite mitigation for the widened Users/Authenticated Users SIDs was applied on the same code path used by both the elevated and unelevated tiers, but the unelevated tier has no WRITE_DAC on system-owned paths like C:, ProgramData, or Windows\Temp, so it aborted every command. Fix: WinBuiltinUsersSid/WinAuthenticatedUserSid are now only added to the restricted token for the elevated restricted-token tier (zero sandbox setup, run as Administrator). That's also the only tier with the rights to enforce the DenyWrite mitigation on shared system paths, so BuildWindowsACLPlan now only emits those entries for that tier. The unelevated tier keeps the original, narrower restricting-SID set (no Program Files/System32 read widening there), which sidesteps the access-denied abort entirely. Also fixed the C:\Users\Public gap: inheriting a deny ACE from C:\ never actually protected pre-existing children like it, since NTFS doesn't retroactively propagate an inherited ACE onto objects that already exist. Added it as an explicit DenyWrite target instead, and dropped the NoInherit toggle that tried to route around this by disabling inheritance whenever a write root sat under C:\ (it wasn't needed: a write root's own explicit Allow ACE already takes precedence over anything inherited, by canonical ACE ordering). Added a unit test (TestBuildWindowsACLPlanOmitsSharedDenyPathsWhenUnelevated) covering the unelevated non-admin path, and a real-Windows regression probe for the C:\Users\Public write jail in the elevated smoke test. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
internal/sandbox/windows_acl.go (3)
91-113: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate env-var default logic vs. test helper.
This exact SystemDrive/SystemRoot/ProgramData/PUBLIC default-resolution logic is duplicated verbatim in
windowsSharedDenyPathsForTest(windows_acl_test.go, lines 107-124). If the defaults ever change here, the test helper can silently drift out of sync and stop catching regressions.♻️ Extract a shared helper
func windowsSharedDenyPaths() (systemDrive, systemRoot, programData, publicDir string) { systemDrive = os.Getenv("SystemDrive") if systemDrive == "" { systemDrive = "C:" } systemRoot = os.Getenv("SystemRoot") if systemRoot == "" { systemRoot = systemDrive + `\Windows` } programData = os.Getenv("ProgramData") if programData == "" { programData = systemDrive + `\ProgramData` } publicDir = os.Getenv("PUBLIC") if publicDir == "" { publicDir = systemDrive + `\Users\Public` } return systemDrive, systemRoot, programData, publicDir }Then have both
BuildWindowsACLPlanandwindowsSharedDenyPathsForTestcall into it.🤖 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_acl.go` around lines 91 - 113, Extract the duplicated Windows environment default resolution into a shared windowsSharedDenyPaths helper, then update BuildWindowsACLPlan and the test helper windowsSharedDenyPathsForTest to use it. Preserve the existing SystemDrive, SystemRoot, ProgramData, and PUBLIC fallback values and resulting deny paths.
125-128: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExemption branch has no direct test coverage.
None of the existing tests set a
WriteRootthat exactly equals a shared deny path (e.g.C:\ProgramData), so thewindowsPathEqualsAnyRootcontinuehere is never actually exercised. Given this is write-jail-relevant logic, a dedicated case verifying a write root at, say,C:\Windows\Tempgets the Allow entry without a conflicting DenyWrite would give confidence this exemption behaves correctly.🤖 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_acl.go` around lines 125 - 128, Add a focused test for the ACL generation flow around windowsPathEqualsAnyRoot using a WriteRoot that exactly matches a shared deny path, such as C:\Windows\Temp. Assert the resulting ACL includes an Allow entry for that path and no conflicting DenyWrite entry, exercising the continue branch in the sharedDenyPaths loop.
115-123: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueReuse
writeSIDsfor the shared-deny list.windowsWriteCapabilitySIDsalready dedupes the write-root SIDs, so rebuilding them here is unnecessary; appendcaps.ReadOnlyto a copy ofwriteSIDsinstead. If the extra capability-file read matters on theDenyRead-only path, pass the loaded value through sowindowsReadDenyCapabilitySIDsdoes not reload it.🤖 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_acl.go` around lines 115 - 123, Update the shared-deny construction in the Windows ACL planning flow to copy the already-deduplicated writeSIDs and append caps.ReadOnly, rather than rebuilding SIDs from writeCapabilities. Reuse the loaded capability value by passing it through to windowsReadDenyCapabilitySIDs so the DenyRead-only path does not reload the capability file.internal/sandbox/windows_command_runner_windows.go (1)
72-76: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winCentralize the sandbox-level condition gating this mitigation.
The same
config.SandboxLevel == WindowsSandboxLevelRestrictedTokencheck independently gates the ACL DenyWrite entries inBuildWindowsACLPlan(windows_acl.go) and the SID broadening here. These two must always agree, or you either widen reads without the write mitigation or try to apply DenyWrite ACEs from a tier lacking WRITE_DAC. Consider a single source of truth:+// broadensReadSIDs reports whether this sandbox level both broadens the +// restricted token's read SIDs (Users/Authenticated Users) and has the +// Administrator rights needed to apply the corresponding DenyWrite ACL +// mitigation to shared system paths. +func (level WindowsSandboxLevel) broadensReadSIDs() bool { + return level == WindowsSandboxLevelRestrictedToken +}- broadenReadSIDs := config.SandboxLevel == WindowsSandboxLevelRestrictedToken + broadenReadSIDs := config.SandboxLevel.broadensReadSIDs()and in windows_acl.go:
- if config.SandboxLevel == WindowsSandboxLevelRestrictedToken { + if config.SandboxLevel.broadensReadSIDs() {🤖 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_command_runner_windows.go` around lines 72 - 76, Centralize the restricted-token condition that enables the DenyWrite mitigation and broadened read SIDs. Define or reuse a shared predicate for this sandbox-level capability, then update both BuildWindowsACLPlan and the createWindowsRestrictedTokenForCapabilitySIDs call site to use it instead of comparing config.SandboxLevel independently. Ensure both paths always enable or disable the mitigation together.
🤖 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.
Inline comments:
In `@internal/sandbox/runner_windows_integration_test.go`:
- Around line 229-244: Update the programDataMarker os.Stat check in the Windows
smoke test to handle unexpected errors like the sibling outsideMarker and
publicMarker checks: retain the missing-file success path, but add an else-if
using os.IsNotExist(err) and fail the test with a diagnostic for any other Stat
error.
---
Nitpick comments:
In `@internal/sandbox/windows_acl.go`:
- Around line 91-113: Extract the duplicated Windows environment default
resolution into a shared windowsSharedDenyPaths helper, then update
BuildWindowsACLPlan and the test helper windowsSharedDenyPathsForTest to use it.
Preserve the existing SystemDrive, SystemRoot, ProgramData, and PUBLIC fallback
values and resulting deny paths.
- Around line 125-128: Add a focused test for the ACL generation flow around
windowsPathEqualsAnyRoot using a WriteRoot that exactly matches a shared deny
path, such as C:\Windows\Temp. Assert the resulting ACL includes an Allow entry
for that path and no conflicting DenyWrite entry, exercising the continue branch
in the sharedDenyPaths loop.
- Around line 115-123: Update the shared-deny construction in the Windows ACL
planning flow to copy the already-deduplicated writeSIDs and append
caps.ReadOnly, rather than rebuilding SIDs from writeCapabilities. Reuse the
loaded capability value by passing it through to windowsReadDenyCapabilitySIDs
so the DenyRead-only path does not reload the capability file.
In `@internal/sandbox/windows_command_runner_windows.go`:
- Around line 72-76: Centralize the restricted-token condition that enables the
DenyWrite mitigation and broadened read SIDs. Define or reuse a shared predicate
for this sandbox-level capability, then update both BuildWindowsACLPlan and the
createWindowsRestrictedTokenForCapabilitySIDs call site to use it instead of
comparing config.SandboxLevel independently. Ensure both paths always enable or
disable the mitigation together.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b15d1528-6c30-403b-af7f-13a4109aff7a
📒 Files selected for processing (6)
internal/sandbox/runner_windows_integration_test.gointernal/sandbox/windows_acl.gointernal/sandbox/windows_acl_apply_windows.gointernal/sandbox/windows_acl_test.gointernal/sandbox/windows_command_runner_windows.gointernal/sandbox/windows_token_windows.go
Align the ProgramData marker check with its outsideMarker and publicMarker siblings, which already report a fatal error if Stat fails for a reason other than the file not existing.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Avoid inheritable denies on system roots
internal/sandbox/windows_acl.go:139
The new shared-path mitigation addsDenyWriteentries toC:\,C:\ProgramData,C:\Windows\Temp, andC:\Users\Public, andwindowsExplicitAccessEntriesmakes every directory entry inheritable withSUB_CONTAINERS_AND_OBJECTS_INHERIT. Elevatedzero sandbox setupthen applies that plan withSetNamedSecurityInfoand leaves successful ACL changes in place. This is much broader than the intended four target directories: Microsoft documents that setting a DACL propagates inheritable ACEs to existing child objects, so theC:\deny can recursively stamp synthetic capability-SID deny ACEs across existing descendants of the system drive. That can make setup slow or brittle on protected descendants, pollute unrelated machine ACLs permanently, and can also interfere with ordinary allowed workspace writes when a repo lives under the system drive and descendants inherit both the broad deny and the workspace allow. Please avoid applying inheritable deny ACEs to broad system roots; use non-propagating entries or a targeted mechanism that does not rewrite arbitrary existing descendants. -
[P1] Resolve shared deny paths from trusted Windows locations
internal/sandbox/windows_acl.go:91
The security boundary now depends onSystemDrive,SystemRoot,ProgramData, andPUBLICfrom the setup process environment to decide which shared locations receive the compensating DenyWrite ACEs. If elevated setup is launched with any of those variables spoofed or unusual, the marker hash is computed from the same spoofed plan and validation later passes, while the restricted-token runner still broadens the token withWinBuiltinUsersSidandWinAuthenticatedUserSid. The realC:\Users\Public,C:\ProgramData, orC:\Windows\Tempcan therefore remain uncovered, letting the widened token write through the existing Users/Authenticated Users grants outside the configured write roots. Please resolve these critical paths from trusted Windows APIs or canonical system locations, and make the tests assert those canonical targets rather than mirroringos.Getenv.
…estricted-token-sids # Conflicts: # internal/sandbox/windows_command_runner_windows.go # internal/sandbox/windows_token_windows.go
…itance propagation Resolve the shared DenyWrite target paths (system drive, ProgramData, Windows\Temp, Users\Public) via GetSystemWindowsDirectory and SHGetKnownFolderPath instead of trusting the SystemDrive/SystemRoot/ ProgramData/PUBLIC environment variables, which an attacker able to influence the elevated setup process's environment could spoof to leave the real system paths unprotected. Also stop marking those four DenyWrite entries as inheritable. SetNamedSecurityInfo automatically propagates inheritable ACEs onto a target's existing descendants, so the previous entries recursively stamped a deny ACE across the entire existing subtree of the system drive rather than just the four intended directories, which was slow, polluted unrelated machine ACLs, and could shadow legitimate workspace allows for repos under the system drive. A plain, non-inherited deny directly on each of the four paths already blocks writes (including new children) at that path without touching any descendant's ACL.
|
Merged main in first (this branch was 14 commits behind), then pushed 3597b62 for jatmn's second round of P1s:
The merge conflict was two different PRs adding a bool parameter to the same function (this PR's broadenReadSIDs, #658's writeRestricted on main) - kept both as separate parameters. One trade-off worth flagging: stripping inheritance entirely (rather than NO_PROPAGATE_INHERIT_ACE) means a pre-existing nested subfolder under one of these four paths that already has its own Users/Authenticated-Users write grant from Windows' defaults is no longer separately covered by this mitigation. The four explicit paths themselves remain fully protected either way. Ran the full internal/sandbox suite on a native Windows host, all green. |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Link an approved parent issue before accepting this external contribution
CONTRIBUTING.md:30
The author is aCONTRIBUTOR, not a maintainer/collaborator, and the PR body has no linked issue or approved case. The contribution policy requires each community PR to be tied to an issue carryingissue-approved; without that approval, it says the PR must be closed without review. Please link the approved parent issue (or obtain the approval) before this proceeds. -
[P1] Do not add the broad restricting SIDs to write-restricted commands
internal/sandbox/windows_command_runner_windows.go:75
writeRestrictedis true for the normal profile with noDenyRead, and that token mode already performs reads using the normal token identity. AddingBUILTIN\\UsersandAuthenticated Usersto its restricted SID list therefore cannot fix Program Files/System32 reads, but it does make their write grants pass the second, restricted-SID write check. This makes the default elevated sandbox take the write-jail risk without the claimed benefit. Only broaden the SIDs on the fully restricted (DenyRead) path where they are needed for read access, or avoid the broadening altogether. -
[P1] Preserve the write jail for existing shared-directory descendants
internal/sandbox/windows_acl.go:129
The compensating DenyWrite ACEs are deliberately non-inheriting, so they protect only the four directory objects themselves. An existing writable descendant of%ProgramData%,%PUBLIC%,%SystemRoot%\\Temp, or the system drive retains its own Users/Authenticated Users allow ACE and no synthetic deny. Once the elevated fully restricted token carries those SIDs, a command can modify that descendant outside every configured write root. The new smoke check writes only directly under%PUBLIC%, so it cannot exercise this bypass. Use an enforcement design that covers reachable existing writable descendants without recursively stamping unrelated system ACLs, and add a real-Windows regression probe for a nested writable child. -
[P2] Avoid permanently appending per-workspace SIDs to machine-wide DACLs
internal/sandbox/windows_acl.go:119
Each elevated setup adds four permanent deny ACEs for every workspace/write-root capability SID, while successful setup discards its rollback and those capability SIDs are minted and retained per distinct root. Re-running setup for different projects therefore grows the DACLs onC:\\, ProgramData, Windows Temp, and Public indefinitely with obsolete SIDs. Eventually this bloats or exhausts those shared system DACLs and can make setup fail for later projects. Use stable shared denial identities or replace/remove the prior setup entries as part of the setup lifecycle.
… deny identity - Users/Authenticated Users are only added to the restricted-SID list when the token is fully restricted (a DenyRead profile): a WRITE_RESTRICTED token already reads with its normal identity, so broadening it gained nothing for Program Files/System32 reads while letting those groups' write grants pass the restricted-SID write check. The default elevated profile therefore no longer takes the write-jail risk at all. - The shared system-path DenyWrite mitigation (C:\, ProgramData, Windows\Temp, Public) is only planned for DenyRead profiles, the ones whose tokens actually carry the broadened SIDs, and its deny ACEs name only the stable read-only capability SID, which every broadened token now carries. Machine-wide DACLs stay at a constant four entries total instead of growing by four per distinct sandboxed project. - The real-Windows smoke's Public-directory probe now pins that a non-DenyRead profile's token cannot reach the Users write grant at all, and plan-level tests pin both the stable deny identity and the absence of shared entries for non-DenyRead profiles.
|
Pushed 949397f for the two technical P1s and the P2.
The issue-approval finding is a process item and is not addressed by this push. go build, go vet, and go test ./internal/sandbox pass locally. The real-Windows smoke (ZERO_SANDBOX_REAL_SMOKE=1) requires elevation this environment does not have; the plan-level unit tests cover the new scoping and identity assertions. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/sandbox/windows_token_windows.go (1)
113-114: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winEnforce mutually exclusive token flags programmatically.
The documentation thoroughly explains why
broadenReadSIDsmust remainfalsewhenwriteRestrictedis set (otherwise it would widen the write jail without mitigation). Enforcing this invariant in code acts as a robust guardrail, preventing future refactoring in the caller from accidentally breaking this critical security property.🛡️ Proposed invariant check
func createWindowsRestrictedTokenFromBase(base windows.Token, capabilitySIDs []windowsLocalSID, broadenReadSIDs bool, writeRestricted bool) (windows.Token, error) { + if broadenReadSIDs && writeRestricted { + return 0, errors.New("broadenReadSIDs cannot be combined with writeRestricted") + } logonSID, err := copyWindowsLogonSID(base)🤖 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` around lines 113 - 114, Update createWindowsRestrictedTokenFromBase to reject or otherwise fail immediately when broadenReadSIDs and writeRestricted are both true, enforcing that these token flags are mutually exclusive before any token manipulation occurs.
🤖 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.
Inline comments:
In `@internal/sandbox/windows_acl.go`:
- Around line 130-156: Update dedupeWindowsACLEntries to include
WindowsACLEntry.NoInherit in its deduplication key, ensuring entries with
different inheritance behavior remain distinct. Leave windowsPathEqualsAnyRoot
unchanged.
---
Nitpick comments:
In `@internal/sandbox/windows_token_windows.go`:
- Around line 113-114: Update createWindowsRestrictedTokenFromBase to reject or
otherwise fail immediately when broadenReadSIDs and writeRestricted are both
true, enforcing that these token flags are mutually exclusive before any token
manipulation occurs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 35704100-e1c5-4c9a-982c-5ccf2d18d5ae
📒 Files selected for processing (8)
internal/sandbox/runner_windows_integration_test.gointernal/sandbox/windows_acl.gointernal/sandbox/windows_acl_apply_windows.gointernal/sandbox/windows_acl_paths_other.gointernal/sandbox/windows_acl_paths_windows.gointernal/sandbox/windows_acl_test.gointernal/sandbox/windows_command_runner_windows.gointernal/sandbox/windows_token_windows.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/sandbox/windows_command_runner_windows.go
- internal/sandbox/runner_windows_integration_test.go
A direct-only deny and an inheritable deny on the same path and SID are different ACL shapes; collapsing them could silently promote a deliberately non-inherited shared-path deny into an inheritable one that SetNamedSecurityInfo would propagate across a huge existing subtree.
|
Pushed a fix for CodeRabbit's dedupe finding: dedupeWindowsACLEntries now includes NoInherit in its key, so a direct-only deny and an inheritable deny on the same path and SID stay distinct instead of collapsing into one shape. TestDedupeWindowsACLEntriesKeepsInheritanceVariants pins it. |
3 participants
This PR adds the WinBuiltinUsersSid and WinAuthenticatedUserSid well-known SIDs to the Windows restricted token's SIDs list. This allows the sandboxed process to read and execute programs like Go, Python, or Node from folders like C:\Program Files or C:\Windows where permissions are granted to the Users/Authenticated Users groups, without compromising the write jail.
Summary by CodeRabbit
Bug Fixes
Tests