fix(sandbox): keep Windows restricted-token SIDs narrow (no Users broaden) - #640
fix(sandbox): keep Windows restricted-token SIDs narrow (no Users broaden)#640euxaristia wants to merge 26 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughWindows restricted-token execution conditionally broadens read SIDs after volume and descendant-coverage checks. Restricted-token ACL plans deny writes on shared paths and writable descendants, while tests validate ACL parsing, rollback, path exclusions, and blocked writes. ChangesWindows sandbox enforcement
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant SandboxCommand
participant ACLPlan
participant ACLApplier
participant DescendantScanner
participant RestrictedToken
SandboxCommand->>ACLPlan: build restricted-token ACL plan
ACLPlan->>ACLApplier: provide shared-root deny-write entries
ACLApplier->>DescendantScanner: scan writable descendants
DescendantScanner-->>ACLApplier: return descendant coverage
SandboxCommand->>RestrictedToken: create conditionally broadened token
SandboxCommand->>ACLApplier: execute sandboxed shared-path write
ACLApplier-->>SandboxCommand: deny write
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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.
|
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
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.
|
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.
|
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
|
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. |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Findings
-
[P1] Do not enable the broad SIDs while descendant writes remain unenforced —
internal/sandbox/windows_acl.go:150This is an acknowledged trade-off, but it is still an actionable security gap in the configuration that this PR enables.
NoInherit: truemakes each compensating shared-pathDenyWriteACE direct-only; the application layer supplies zero inheritance flags (windows_acl_apply_windows.go:158-161). At the same time, the elevatedDenyReadpath addsBUILTIN\\UsersandAuthenticated Usersto the fully restricted token (windows_command_runner_windows.go:77-102,windows_token_windows.go:131-143). An access check for an existing child does not evaluate a non-inherited ACE on its parent. Thus an existing writable child below one of the four shared roots can retain a Users/Authenticated Users allow ACE, satisfy the restricted-SID check, and permit a write outside the configuredWriteRoots. For example, after elevated setup for a profile withDenyRead, a sandboxed command can write beneath an existingC:\\Users\\Publicchild that grantsUsersModify. Scope changes remove this risk from the default non-DenyReadprofile, but they do not remove it from theDenyReadprofile that this PR now broadens. Either preserve descendant-safe enforcement or do not enable the broad SIDs for that profile.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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_descendants_windows_test.go`:
- Around line 107-154: Extend
TestWindowsEnumerateWritableDescendantsFindsExistingWritableChildren with a
depth-3 writable directory whose parent remains non-writable, verifying
windowsEnumerateWritableDescendants discovers it instead of pruning traversal at
non-writable ancestors. Also add coverage for cap exhaustion, asserting
enumeration stops or reports the expected capped result when the descendant
limit is reached.
In `@internal/sandbox/windows_acl_descendants_windows.go`:
- Around line 138-178: The bounded descendant scan in
internal/sandbox/windows_acl_descendants_windows.go, within the traversal using
windowsDirGrantsBroadenedWrite, must continue enqueueing eligible directories
beyond baseline depth regardless of parent writability so writable descendants
are discovered; treat directory-listing, DACL-inspection, and scan-bound
exhaustion as incomplete scans and fail closed instead of returning partial
results as success. Update
internal/sandbox/windows_acl_descendants_windows_test.go at lines 107-154 to add
a writable depth-3 child beneath a non-writable ancestor and verify that
exhausting the traversal bound produces the expected failure behavior.
- Around line 206-226: Update the ACE parsing loop around GetAce and the
ace.Header.AceType switch to inspect the ACE type before interpreting its layout
as ACCESS_ALLOWED_ACE. Handle ACCESS_ALLOWED_OBJECT_ACE and
ACCESS_DENIED_OBJECT_ACE using their correct SID offset, or skip unsupported
types, then apply the existing Users/Authenticated Users and write-mask logic so
object ACE grants are not missed.
🪄 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: ac440c33-466a-4f82-b5f7-59ccaf1c2dc9
📒 Files selected for processing (5)
internal/sandbox/windows_acl.gointernal/sandbox/windows_acl_apply_windows.gointernal/sandbox/windows_acl_descendants_windows.gointernal/sandbox/windows_acl_descendants_windows_test.gointernal/sandbox/windows_acl_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/sandbox/windows_acl.go
- internal/sandbox/windows_acl_test.go
|
Pushed a fix for the last open finding: existing writable descendants of the shared deny roots ( Known limits, documented rather than silently left open: the scan only descends past a shallow baseline depth into directories that are themselves already writable, so a writable directory reachable only through a non-writable ancestor further down the tree isn't covered; writable files (as opposed to directories) directly under a shared root aren't covered either; and the scan has depth/count caps, so an unusually large tree could leave some deeper descendants unscanned. |
92261ad to
6d76db7
Compare
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 the broadened SIDs out of unprotected volumes
internal/sandbox/windows_acl.go:123
The restricted token gainsBUILTIN\\UsersandAuthenticated Usersfor every filesystem access, but the compensating denies only cover four paths on the system drive. On a multi-volume machine, a pre-existingD:\\sharedACL granting either group write now satisfies the restricted-SID check and permits an out-of-root write; the existing test configuration already models aD:writable root. Protect every reachable volume/path covered by the broadened identities, or do not add those identities until that invariant can be enforced. -
[P1] Do not leave existing writable files outside the jail
internal/sandbox/windows_acl_descendants_windows.go:147
The scan discards every non-directory and applies direct, non-inheriting denies only to returned directories. A pre-existing file under Public, ProgramData, or a discovered writable directory that grants Users/Auth Users write therefore retains that grant: neither the root nor parent direct deny participates in the file access check. A DenyRead sandbox can overwrite or delete that file outside its configured write roots. -
[P1] Fail closed when the descendant scan cannot cover a writable path
internal/sandbox/windows_acl_descendants_windows.go:159
The traversal returns success at 8,192 entries, stops at depth 24, and below depth two prunes a non-writable parent. A writable child can validly sit beneath read/traverse-only ancestors, so such a child (or one beyond either cap) is never denied while the setup marker is still written. The broadened token can then write it through its Users/Auth Users ACE; this is the unresolved security issue in the current review thread, not merely a best-effort limitation. -
[P1] Do not skip existing reparse points without preserving the write boundary
internal/sandbox/windows_acl_descendants_windows.go:156
A pre-existing junction or symlink below a shared root is skipped, yet a sandboxed process can traverse it. If its target has a Users/Auth Users write grant, the non-inherited deny on the shared-root parent is not evaluated for the target, so the newly broadened token can write outside the configured roots. Resolve and protect safe targets, or reject setup when such an uncovered reparse point is present. -
[P1] Re-establish descendant protection after setup-time filesystem changes
internal/sandbox/windows_acl_apply_windows.go:49
Descendants are scanned only during elevated setup, while later commands validate a static marker and never rescan. Because the root ACEs are deliberately non-inheriting, another normal process or installer can create a Users-writable child after setup; it receives no capability-SID deny, the marker still validates, and the subsequent broadened sandbox token can write it. The enforcement must remain valid as these mutable shared trees change, rather than being a point-in-time scan. -
[P1] Parse object ACEs before deciding whether a descendant is writable
internal/sandbox/windows_acl_descendants_windows.go:207
GetAceis interpreted asACCESS_ALLOWED_ACEandSidStartis dereferenced before the ACE type is checked. Object ACEs place optional GUID fields before the SID and are not handled by the subsequent switch, so a Users/Auth Users object ACE granting write is missed and its directory remains outside the jail. Branch on ACE type and use the corresponding layout (with regression coverage) before testing the SID and mask. -
[P2] Preserve configured write roots when applying shared-path denies
internal/sandbox/windows_acl.go:145
The skip test recognizes only a write root exactly equal to the shared path. A valid broader root such asC:\\Usersstill receives a direct deny onC:\\Users\\Public, which wins for every broadened token despite Public being within the configured allowed root. In addition, once a prior setup has persisted that capability-SID deny, a later configuration that makes the path a write root has no reconciliation path to remove it. Make the shared-deny plan and its persistent ACL state root-aware so allowed subtrees remain writable across setup changes.
|
Pushed a follow-up commit (5a6c530) addressing three of the findings from the latest review round:
All three have regression tests confirmed to fail without their fix. Cross-compiles and vets clean on windows/linux/darwin. Four items from the last review round are architecture calls rather than local bugs, and I'd like a read from @Vasanthdev2004 @jatmn @gnanam1990 on which to close now vs. track as follow-ups:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/sandbox/windows_acl_descendants_windows_test.go (1)
200-205: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winVerify rollback preserves the pre-existing ACL.
Checking only that the deny disappeared allows a rollback that also deletes the original Users write ACE to pass. Reassert the original writable state after rollback.
Proposed assertion
if dirDeniesSID(t, writable, caps.ReadOnly) { t.Fatalf("descendant %q still denies %q after rollback", writable, caps.ReadOnly) } +restoredWritable, err := windowsDirGrantsBroadenedWrite(writable) +if err != nil { + t.Fatalf("windowsDirGrantsBroadenedWrite after rollback: %v", err) +} +if !restoredWritable { + t.Fatalf("rollback did not restore the original writable DACL on %q", writable) +}🤖 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_descendants_windows_test.go` around lines 200 - 205, Extend the rollback verification around rollbackWindowsACLSnapshots to assert that the pre-existing writable ACL is restored, not merely that the deny for caps.ReadOnly is gone. Reuse the existing writable-state check or ACL assertion for writable and fail the test if the original Users write permission is not present after rollback.
🤖 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_descendants_windows.go`:
- Around line 260-284: Update the ACE loop around windowsAceSID to skip entries
whose Header.AceFlags include INHERIT_ONLY_ACE before modifying deniedWrite or
evaluating allowed write bits. Continue processing applicable ACEs unchanged so
inherit-only denies cannot affect the allow check.
---
Nitpick comments:
In `@internal/sandbox/windows_acl_descendants_windows_test.go`:
- Around line 200-205: Extend the rollback verification around
rollbackWindowsACLSnapshots to assert that the pre-existing writable ACL is
restored, not merely that the deny for caps.ReadOnly is gone. Reuse the existing
writable-state check or ACL assertion for writable and fail the test if the
original Users write permission is not present after rollback.
🪄 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: d5c91353-d42c-4dd5-a28d-b6dec81fefee
📒 Files selected for processing (10)
internal/sandbox/runner_windows_integration_test.gointernal/sandbox/windows_acl.gointernal/sandbox/windows_acl_apply_windows.gointernal/sandbox/windows_acl_descendants_windows.gointernal/sandbox/windows_acl_descendants_windows_test.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 (8)
- internal/sandbox/windows_acl_paths_other.go
- internal/sandbox/windows_acl_paths_windows.go
- internal/sandbox/runner_windows_integration_test.go
- internal/sandbox/windows_command_runner_windows.go
- internal/sandbox/windows_acl.go
- internal/sandbox/windows_token_windows.go
- internal/sandbox/windows_acl_apply_windows.go
- internal/sandbox/windows_acl_test.go
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Still requesting changes, but this is close now — most of my original block is resolved. The shared-root DenyWrite entries plus the Public/ProgramData write assertions in the smoke tests are exactly what I asked for, and scoping the broadened SIDs to the elevated DenyRead tier (leaving WRITE_RESTRICTED and unelevated tokens on the narrow set) is a better shape than what I proposed. The trusted-API path resolution and the object-ACE offset fix with its layout tests are solid work too.
The one thing still blocking me is the uncovered-volume case jatmn flagged: the broadened token applies to every access on every volume, but the compensating denies cover exactly four paths on the system drive. A stock non-system NTFS data volume grants Authenticated Users Modify at the root by default with (OI)(CI)(IO) inheritance, so a DenyRead profile can write anywhere on such a volume, outside every write root — and this isn't hypothetical, the existing test config already models a writable D: root. The descendant scan can't patch that either; with the grant inherited volume-wide, the dir cap is exhausted immediately. So this needs either fail-closed handling (enumerate fixed volumes and keep the narrow SID set when an uncovered writable one exists) or an explicit sign-off from kevin that the DenyRead-tier write jail is scoped to the system drive. The documented scan-cap/TOCTOU residuals on the covered roots I can live with as bounded best-effort — a whole-volume hole is different. Close that one out and I'm ready to approve.
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 the broadened identities off unprotected volumes
internal/sandbox/windows_acl.go:123
The fully restricted token gainsBUILTIN\UsersandAuthenticated Usersfor every filesystem access, but the compensating plan covers only four paths on the system drive. On a normal multi-volume host, a path such asD:\Sharedwhose DACL grants either group Modify now satisfies the restricted-SID half of the access check and remains writable outside every configured write root. This is the unresolved cross-volume boundary from the current maintainer review; enumerate and protect every reachable volume, fail closed when one cannot be covered, or do not add these globally write-capable SIDs. -
[P1] Fail closed when descendant coverage is incomplete
internal/sandbox/windows_acl_descendants_windows.go:159
The scan returns partial results as success at 8,192 entries, stops silently at depth 24, and below depth two prunes a traversable parent merely because that parent is not itself writable. It also treatsReadDirand DACL-read errors as proof that a subtree is locked down, although list/READ_CONTROLand write rights are independent. A deeper child with an explicit Users/Auth-Users write grant is therefore omitted, setup writes a valid marker, and the broadened token can write it outside the jail. Incomplete enumeration must abort broadening/setup rather than certify a partial result. -
[P1] Preserve the boundary across reparse points
internal/sandbox/windows_acl_descendants_windows.go:156
Every existing junction or symlink is skipped, but sandboxed access can still follow it. For example,C:\ProgramData\escapecan target an Authenticated-Users-writable directory onD:, and neither the non-inherited ProgramData deny nor the descendant scan applies to the target. The broadened token can then write through the junction outside configured roots. Resolve and protect safe targets or reject setup when an uncovered reparse point is reachable. -
[P1] Re-establish protection as shared trees change
internal/sandbox/windows_acl_apply_windows.go:49
Descendants are scanned only during elevated setup; later commands validate a static plan marker and never rescan. Because every compensating root/descendant deny is non-inheriting, an installer or ordinary process can create a Users-writable child after setup (or race creation between the scan and marker write), and every later DenyRead command can write it while marker validation continues to pass. The enforcement must cover future filesystem state rather than remain a point-in-time snapshot. -
[P1] Use effective Windows access semantics for the write probe
internal/sandbox/windows_acl_descendants_windows.go:266
The hand-written DACL evaluator can classify writable objects as safe: it appliesINHERIT_ONLY_ACEentries to the current object, skips callback/conditional allow ACEs that can grant a matching trustee write access, and its mask omits the specificFILE_WRITE_ATTRIBUTESandFILE_WRITE_EArights. For example, an inherit-only Users deny followed by an applicable Users allow is writable to Windows but is suppressed bydeniedWritehere, so no capability deny is applied. Use a native effective-access check or fully model ACE applicability/types and every mapped write right before treating an object as non-writable. -
[P2] Remove persistent denies that a new plan excludes
internal/sandbox/windows_acl.go:145
The new root-aware skip only avoids adding a deny in the current plan; successful setup permanently leaves prior stable-ReadOnly-SID denies in place. If a later configuration promotesC:\Users, Public, or a previously scanned descendant to a write root, the old deny remains and wins over the new per-root allow because every broadened token still carries that stable SID. Reconcile/remove obsolete setup-owned ACEs so changing the configured roots can actually make the path writable. -
[P2] Do not append duplicate permanent denies on every scan/setup
internal/sandbox/windows_acl_apply_windows.go:50
C:\is scanned before its explicit ProgramData, Windows Temp, and Public child groups, so a broad-writable child root or descendant can receive the sameDENY_ACCESSentry once from the drive scan and again from the child scan. Later setup runs select it again because the write probe ignores the synthetic deny, andSetEntriesInAclinserts a new deny rather than replacing an equivalent one. The duplicates therefore accumulate in persistent machine DACLs across overlapping scans and setup reruns, eventually bloating an affected ACL or making later setup fail. Detect/reuse an existing equivalent ACE and avoid applying the same descendant through overlapping root scans. -
[P2] Canonicalize write-root exclusions before mutating descendants
internal/sandbox/windows_acl_descendants_windows.go:116
Exclusions compare only cleaned lexical strings, not the filesystem object they name. If a configured workspace/write root names a directory through a junction, volume mount path, or 8.3 alias while the shared-root walk reaches that same directory through another pathname, the equality/prefix checks do not recognize it as excluded. Setup then stamps the stable deny onto the object that the plan explicitly allows, and that deny wins when the workspace is accessed through its configured path. Resolve aliases and compare stable object identities or canonical paths before applying descendant denies.
|
Pushed 2c2867f for the cross-volume blocker: the runner now enumerates fixed volumes from trusted Win32 APIs and only broadens the restricting-SID set when the system drive is the machine's sole fixed volume. Any other layout — including the stock data-volume case where Authenticated Users has Modify with volume-wide inheritance, which no bounded descendant scan can patch — fails closed to the narrow SID set, keeping the write jail at the cost of the read fix on those hosts; enumeration failure also fails closed. Also fixed the descendant write probe to skip INHERIT_ONLY ACEs, which don't apply to the object itself and could suppress an applicable allow in deniedWrite. The remaining descendant-scan hardening items from jatmn's latest round (post-setup rescan, reparse-point targets, native effective-access semantics, deny reconciliation/dedupe across reruns) are still open — several want a design decision on scan-vs-native-access-check that's worth settling before more code, and the DenyRead-tier posture overall still needs kevin's sign-off per the thread. |
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] Reject mounted volumes before widening the restricting-SID set
internal/sandbox/windows_volumes_windows.go:35
GetLogicalDriveStringsonly returns drive-letter roots, so a second fixed NTFS volume mounted at (for example)C:\\mnt\\datais absent from this check. The function consequently returns true when C: is the only lettered fixed drive, while the descendant walker skips that mount point as a reparse point. A DenyRead command then receivesAuthenticated Users; if the mounted volume has the normal group write grant, it can write there outside its configured roots. Enumerate mounted volumes too, or keep the narrow SID set unless the full writable-volume surface is covered. -
[P1] Do not report setup success after an incomplete descendant scan
internal/sandbox/windows_acl_descendants_windows.go:141
The mitigation silently skips unreadable entries, reparse points, every descendant below a non-writable depth-two parent, and everything after the depth/8,192-entry limits. A known child can have an explicit Users/Authenticated Users write ACE even when its parents are not writable; such a child is never denied, but the runner still broadens the token and permits an outside-root write. This leaves the current CodeRabbit request for complete/fail-closed descendant coverage unaddressed. Setup must fail closed whenever it cannot establish coverage, or use an enforcement mechanism that covers every reachable write target. -
[P1] Keep descendant denies current after setup
internal/sandbox/windows_acl.go:164
The shared-root and discovered-child denies are deliberately non-inheriting and are only applied duringzero sandbox setup; later commands validate a deterministic marker but never rescan. A service, installer, or user can create or make a child under ProgramData/Public group-writable after setup. That new child has no synthetic deny, yet a later DenyRead command has the broadened Users/Authenticated Users SIDs and can write it outside the configured roots. Revalidate/reapply this protection before broadening, or do not broaden without an enforcement mechanism that also covers future children.
|
Pushed 29a980f. Fixed:
Not fixing, with reasons:
-race wasn't runnable in my environment (no gcc for cgo), worth running before merge. |
|
Pushed 900f1c4 (and earlier 29a980f) for the open P1s from the latest review. 1. Reject mounted volumes before widening restricting SIDsAlready landed in 29a980f, kept in place: 2. Do not report setup success after an incomplete descendant scan
3. Keep descendant denies current after setup900f1c4: before broadening, the elevated command runner calls Also: skip re-stamping descendants that already carry the stable capability deny (avoids duplicate ACE growth on reruns). Linux-runnable policy tests: |
…root-nested shared paths Three fixes to the shared-root descendant scan/deny that compensates for broadening the Windows restricted token with Users/Authenticated Users: - windowsDirGrantsBroadenedWrite read every ACE as a plain ACCESS_ALLOWED_ACE, but an object ACE (ACCESS_ALLOWED_OBJECT_ACE / ACCESS_DENIED_OBJECT_ACE) inserts a Flags DWORD and up to two conditionally-present 16-byte GUIDs between Mask and the real SID. Reading &ace.SidStart directly for one of these misinterprets those bytes as SID bytes and silently computes the wrong trustee, missing a real Users/Authenticated Users grant hidden inside an object ACE. Add windowsAceSID to locate the SID at the correct offset per ACE type. - windowsEnumerateWritableDescendants skipped every non-directory entry, so a writable file directly under a shared root (as much an escape surface as a writable directory) was never found or denied. Files are now checked and denied like directories; only the recursion step remains directory-only. - The shared-path skip in BuildWindowsACLPlan only checked for exact equality with a configured write root, so a write root that merely CONTAINS a shared path (e.g. C:\Users configured as writable, with the shared C:\Users\Public path nested under it) still got a direct DenyWrite. That Deny would sit ahead of the write root's Allow for every broadened token and win under Windows' deny-before-allow evaluation, jailing a directory the user explicitly configured as writable. windowsPathUnderAnyRoot (renamed from windowsPathEqualsAnyRoot) now also matches nested paths. Verified via GOOS=windows/linux/darwin build+vet and a windows test binary compile (no Windows machine available to run it); the new cross-platform regression test and the windows-only ACE-offset/file- scan tests are confirmed to fail without their respective fixes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…NLY ACEs Two review findings: - The broadened Users/Authenticated Users restricting SIDs participate in every access check on every volume, but the compensating DenyWrite mitigation covers only four system-drive paths. A stock non-system NTFS data volume grants Authenticated Users Modify at its root with volume-wide inheritance, so a DenyRead-profile command on a multi-volume host could write anywhere on that volume, outside every configured write root — and no bounded descendant scan can patch a grant inherited volume-wide. The runner now enumerates fixed volumes from trusted APIs and only broadens when the system drive is the sole fixed volume; any other layout (or an enumeration failure) fails closed to the narrow SID set, keeping the write jail at the cost of the read fix on those hosts. - The descendant write probe evaluated INHERIT_ONLY ACEs against the object itself; an inherit-only deny preceding an applicable allow could suppress it in deniedWrite and misclassify a writable directory as safe. Inherit-only entries are now skipped. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Enumerate every volume and its mount paths instead of only drive letters, so a second fixed volume mounted at an NTFS folder is no longer missed. Fail closed instead of silently returning a partial result when the descendant ACL scan hits its depth or directory caps. Widen the write-probe mask to include FILE_WRITE_ATTRIBUTES and FILE_WRITE_EA, and add an explicit invariant check on restricted token creation.
Always descend shared-root trees within scan caps so a writable child under non-writable ancestors is denied, and treat reparse points, unknown unreadable entries, and cap exhaustion as incomplete coverage. Revalidate and reapply (or verify) descendant denies immediately before broadening Users/Authenticated Users on the restricted token; keep the narrow SID set when coverage cannot be re-established. Folder-mounted fixed volumes stay rejected via FindFirstVolume mount-path enumeration.
…ir probe - windowsPathDeniesCapabilitySID skipped straight to SID comparison without filtering INHERIT_ONLY_ACE entries, unlike its sibling windowsDirGrantsBroadenedWrite. An inherited-but-inapplicable deny ACE on a descendant could read as "already denied," causing applyWindowsSharedDescendantDenies to skip applying the real, effective deny and leave that descendant writable. - The basename prune list in the descendant BFS matched at any depth, not just the scan root's direct children, so a nested directory that happens to share a stock-tree name (e.g. a subfolder literally named "Program Files") could get pruned even with a writable descendant beneath it. Gate the prune on depth == 0. - The C:\Users\Public write-jail probe used t.Skip when PUBLIC is unset, which aborts the whole test function and silently drops the unrelated loopback network-deny assertions later in the same test. Wrap the probe instead, matching the existing ProgramData pattern.
Fail closed for non-system volumes, scan shared-root descendants before broadening, require full deny-write masks, restrict system-dir skip to canonical roots, reconcile stale shared denies, and shrink the scan-to-token window with residual TOCTOU documented. Refs Gitlawb#640
Address review findings on the DenyRead coverage path: stop hard-failing on reparse points (stock C:\ has compatibility junctions), raise depth/entry bounds so a normal system drive can finish, and revoke stale descendant denies when a path is promoted to a write root so deny-before-allow no longer leaves the root partly unwritable.
…eduplication, and complete write deny
…e, and make root denies idempotent
…dle unlistable system dirs
…broadening on network commands Strip SYNCHRONIZE from WindowsACLDenyWrite and probe mask so read opens succeed, disable SID broadening for network-allowed commands, fail closed on reparse points, and handle unmounted partitions on UEFI systems. Refs Gitlawb#640
7782708 to
2d8c5e4
Compare
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.
Pre-summary
Several findings repeat earlier review themes because the recent commits repair individual failure modes without resolving the assumption that causes them. A fully restricted token cannot execute ordinary Windows binaries because its narrow restricting-SID list does not match the BUILTIN\\Users grants on Windows and Program Files. This PR adds BUILTIN\\Users and Authenticated Users, which makes those grants usable for both reads and writes, then tries to recover the filesystem write boundary by discovering mutable paths and permanently editing their DACLs.
That compensation has grown across review rounds: non-inheriting shared-root denies, writable-descendant enumeration, command-time rescanning, volume gating, stale-deny reconciliation, and now special reparse behavior. Those changes fix real local defects, but a preflight snapshot cannot establish an access-time invariant over filesystem state that other processes can change after or during the scan. The repeated findings below are therefore current-head verification of the same unresolved design boundary, not unrelated new objections.
The recommendations intentionally do not propose another broader scan. Such a patch would still be vulnerable to changes made after the scan and would produce the same class of follow-up finding. The safe interim outcome is to leave broadenReadSIDs disabled. A functional implementation needs an OS-enforced write boundary that remains in force for the command’s lifetime; only after that exists should DenyRead command execution be re-enabled.
The most concrete supported Windows design to evaluate is an AppContainer or LPAC token with explicit read/execute grants for required runtime paths and read/write grants only for configured write roots. AppContainer access is the intersection of the normal user/group grants and the package/capability grants, so a broad Users grant alone does not become a sandbox write grant, and access is checked by the kernel when the file is opened rather than inferred by a preflight tree walk. Microsoft also documents a newer Experimental_CreateProcessInSandbox API with Bound File System read-only/read-write path policies, but it is Windows 11-only, has no public header, and is explicitly experimental; it is useful as architectural validation, not as the sole production recommendation for this PR. If the supported CLI compatibility matrix cannot be met with a stable access-time mechanism, the complete safe resolution is to report DenyRead execution as unsupported instead of broadening the restricted SID list.
Findings
-
[P1] Preserve the write boundary instead of certifying skipped reparse subtrees
internal/sandbox/windows_acl_descendants_windows.go:204
On the branch where broadening is enabled—an elevated, fully restricted DenyRead profile with non-allow network mode on a single-fixed-volume host—the latest change stops failing immediately on a reparse entry, probes the resolved target root’s DACL, and then skips descent at line 222. That is not complete coverage. A junction or symlink can resolve to a same-volume target whose root is not Users/AuthUsers-writable but whose child has an independent Users/AuthUsers write ACE. The child is never enumerated or given the stable deny,windowsEnsureSharedDescendantCoveragereturns success, and the command receives the broadened token that can write the child through the reparse path. This is why the prior reparse concern remains after the requested compatibility-junction fix: the current head exchanges the stock-junction availability failure for a silent coverage gap.Do not fix this by following the reparse pathname during the existing scan. That would conflict with the no-follow handle protection in
openWindowsACLTarget, would require cycle and cross-volume handling, and—most importantly—would still be only a snapshot that can become stale during the command. Rejecting every reparse is safe but makes the feature unusable on stock compatibility junctions, so it is not a complete functional resolution either. Keep SID broadening disabled until write confinement is enforced independently of pathname enumeration. With an AppContainer-style dual-principal check, an ordinary Users/AuthUsers ACE on the reparse target is insufficient by itself; the target must also have the applicable package/capability grant. If a path-policy mechanism is selected, its reparse resolution behavior must be verified rather than assumed. Add real-Windows regressions for a stock compatibility junction, a non-writable target with a writable nested child, a cross-volume target, and a cycle before enabling the feature. -
[P1] Enforce descendant protection for the command lifetime, not at one point in time
internal/sandbox/windows_acl_descendants_windows.go:432
windowsEnsureSharedDescendantCoveragescans shared roots sequentially and the runner broadens the token afterward. A normal process can create a Users/AuthUsers-writable child beneath an already-scanned root while later roots are being walked; the earlier root is not revisited, so the new child lacks the synthetic deny when token creation occurs. The exposure is longer than the runner’s stated few-statement, microsecond interval because each root can inspect up to 500,000 entries and the overlapping roots are rescanned separately.More fundamentally, the race does not end at token creation. The compensating ACEs are deliberately non-inheriting, so another ordinary process can create a group-writable child at any point while the sandboxed command is running. The broadened token can then use that group grant even if the pre-token scan was perfectly complete. A second or final rescan only moves the race, and a cooperative user-mode lock cannot constrain other processes or installers.
The complete resolution must make the write decision at access time for the command’s entire lifetime. A concrete route is to replace the broad restricting SIDs with an AppContainer/LPAC package identity, grant that identity read/execute on the runtime locations the supported commands actually need, preserve the configured DenyRead exclusions against that identity, and grant read/write only on configured write roots. Do not assume this is a drop-in token swap: first validate process creation, DLL loading, child processes and pipes, required registry/COM access, and the existing network allow/deny modes on every supported Windows version. The experimental Windows 11 sandbox/BFS API may simplify the same path policy in the future, but its current experimental contract is not a sufficient compatibility baseline. If the stable AppContainer route cannot satisfy the supported CLI workload, retain the narrow restricted SID set and surface DenyRead command execution as unsupported rather than enabling an acknowledged write escape.
Before enabling the feature, add a synchronized real-Windows test that creates a Users/AuthUsers-writable descendant after the sandboxed command token already exists and proves the command still cannot write it. Also rerun the existing executable, DLL, subprocess/pipe, network, configured-write-root, reparse, and cross-volume cases under the replacement token. A pre-launch-only test cannot establish this invariant.
Preflight DenyWrite scans cannot enforce a write boundary for the command lifetime (reparse coverage and post-scan group-writable children). Keep the narrow restricting-SID set until access-time confinement exists. Stop planning shared system-path DenyWrite ACEs; retain write-root revoke cleanup for hosts that ran earlier builds.
euxaristia
changed the title
|
Addressed @jatmn findings in
Please re-review when you can. A follow-up for AppContainer/LPAC (or equivalent) is the path to re-enable fully restricted DenyRead with ordinary Program Files launches. |
Harden the shared-root descendant walker for jatmn's PR Gitlawb#640 review: skip reparse points without following, track directory object identity to avoid re-enumeration, keep complete write-deny coverage checks, and add partial-deny plus junction regression tests. Refs Gitlawb#640
|
Addressed review:
|
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] Remove or replace the residual system-directory helpers before merging
internal/sandbox/windows_acl_paths_windows.go:25
The current runner deliberately hard-codesbroadenReadSIDs = false, and
BuildWindowsACLPlanno longer emits the shared-root entries that used the
descendant scanner and volume gate. Those old helpers are therefore not on
the shipped broadening path, but Go still type-checks them. Both this helper
andwindows_volumes_windows.go:40call
windows.GetSystemWindowsDirectory, which does not exist in the pinned
golang.org/x/sys v0.47.0; its Windows package exposes volume APIs but no
exported wrapper for that function.GOOS=windows go test/go buildwill
consequently stop withundefined: windows.GetSystemWindowsDirectorybefore
any Windows binary can be produced.Since the follow-up intentionally disables broadening, the simplest repair
is to remove the now-unreachable shared-path, descendant-scan, and
volume-gate code together with their tests. If any of it is intentionally
retained for a future access-time design, add a small local Win32 wrapper
forGetSystemWindowsDirectoryWinstead and add a Windows cross-compile
check so this cannot regress. -
[P1] Revoke only the experimental write deny, not every ACE for
ReadOnly
internal/sandbox/windows_acl.go:143
I agree that hosts which ran an experimental broadening build need cleanup:
a stale directDenyWritecan otherwise continue to shadow a newly allowed
write root. The cleanup entry, however, is keyed only by the stable
sandbox-homecaps.ReadOnlySID. Its implementation intentionally converts
the entry to zero-maskSET_ACCESS(windows_acl_apply_windows.go:370-379),
which removes all ACEs for that SID at the target rather than identifying a
staleDenyWriteACE.This can remove a live read boundary. For example, profile A has no write
roots andDenyRead=[P];windowsReadDenyCapabilitySIDstherefore installs
DenyReadfor the same persistentReadOnlySID atP, and A's restricted
token carries that SID. While A is running, setup for profile B using the
same sandbox home can configurePas a write root and any non-empty
DenyReadlist. The new B plan emitsWindowsACLRevokeCapabilityatP,
deleting A'sDenyReadACE. The setup marker prevents later A launches with
the old profile, but it does not revoke or stop an already-running A token;
that process can now readP.Please make the migration selective: remove only ACEs that are demonstrably
the old fullDenyWriteshape, preserveDenyReadentries for the stable
SID, and add a Windows ACL regression that applies both profiles against one
sandbox home. The recursive cleanup should use the same predicate, since it
currently applies the same all-ACE revoke to a descendant carrying a full
stale write deny.
Migration revoke for promoted write roots now strips only experimental DenyWrite ACEs for the stable SID and re-applies co-resident DenyRead. Drop the unused volume-gate helper and resolve system paths via GetWindowsDirectory.
|
Addressed the 2026-08-01 review:
|
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] Surface DenyRead execution as unsupported instead of launching a token that cannot execute normal tools
internal/sandbox/windows_command_runner_windows.go:77
A profile with anyDenyReadselectswriteRestricted=false, which makes the restricted-SID access check apply to reads and executable loads as well as writes. This PR then unconditionally leavesBUILTIN\\UsersandAuthenticated Usersout of the restricting set at lines 95–96. The remaining capability, logon, and Everyone SIDs generally do not match the ACLs on ordinaryC:\\Program FilesandC:\\Windowsexecutables or their DLLs;windows_token_windows.go:141-148describes this exact outcome as every executable outside an ACL-granted write root being unable to open. Consequently, a normal DenyRead profile can fail before its requested command begins, even though the profile is otherwise accepted and setup succeeded. The current smoke coverage does not establish this path: its elevated run has noDenyRead, and the only DenyRead run is unelevated and invokescmd.exe.This is not a request to restore the unsafe SID broadening. The root cause is that the current restricted-token mechanism uses one SID set for both executable-read access and write authorization; adding the groups needed to load common binaries also admits their existing write grants, while omitting them makes fully restricted DenyRead tokens unable to run the normal CLI workload. The prior review allowed keeping the narrow set only if DenyRead execution is surfaced as unsupported. Until an access-time confinement mechanism exists (for example, an AppContainer/LPAC-style identity with explicit runtime read/execute grants and write grants limited to configured roots), reject this combination before launch with a clear, actionable unsupported-mode error. Add regression coverage for that rejection. When implementing the replacement confinement, verify executable and DLL loading, child processes and pipes, configured write roots, DenyRead paths, network modes, reparse points, and the supported Windows-version matrix before enabling it.
Fully restricted tokens without Users/AuthUsers cannot load ordinary system binaries, and SID broadening is permanently off. Fail closed with an actionable unsupported-mode error before launch, with regression coverage.
|
Addressed the 20:08 review: DenyRead + elevated restricted-token is unsupported. The runner no longer launches a fully restricted narrow-SID token that cannot load Program Files / Windows executables. It rejects before token creation with a clear message (use |
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] Do not send DenyRead profiles to the unelevated token path
internal/sandbox/windows_command_runner_windows.go:25
This is an incomplete follow-through on the prior request to surface the narrow-token limitation, not a request to restore the unsafe SID broadening. The new unsupported-mode check correctly coversrestricted-token(the elevated path), but not the separateunelevatedrunner level. Before elevated setup,SandboxManagerautomatically selectsEnforcementUnelevatedfor a restricted filesystem profile, andwindowsRestrictedTokenCommandPlanturns that into theunelevatedrunner level. That branch bypasses the new check entirely, then reaches the common token builder withwriteRestricted=falseandbroadenReadSIDs=false. It is therefore the same fully restricted narrow-SID token which this PR documents as unable to read or execute ordinary binaries whose ACLs grantBUILTIN\\UsersorAuthenticated Users; a normal DenyRead command can still fail before its program starts. The only real-Windows unelevated smoke invokescmd.exe, so it does not establish compatibility with the Program Files/Windows binaries at issue. Conversely, oncezero sandbox setuphas written the marker, auto mode selects the elevated tier and exits with the new unsupported-mode error. The error's advice to use the unelevated tier is thus neither selectable for initialized installations nor a functional workaround before setup.Treat this as one capability limitation across both restricted-token tiers: reject a nonempty DenyRead profile before either setup/runner path can provision or launch it, and remove the unelevated recommendation. That makes the supported behavior explicit without weakening the write jail. The root cause is that this token mechanism uses the same restricting-SID access check for executable reads and writes: adding Users/AuthUsers lets normal binaries load but also admits their write grants outside
WriteRoots, while omitting them preserves the jail but prevents normal executable loading. A functional future solution needs access-time confinement that separates read/execute grants from write authority (for example, a validated AppContainer/LPAC-style design); it should not restore SID broadening or attempt another preflight DACL scan. Add a regression through the manager and both runner levels so auto mode with DenyRead either fails with this explicit unsupported error or, after a replacement design, proves execution of a Users-granted binary plus DenyRead and write-jail enforcement.
Unelevated auto-fallback built the same fully restricted narrow-SID token as the elevated path, so DenyRead still failed before ordinary tools could load. Reject nonempty DenyRead before setup or launch on both tiers, drop the unelevated workaround advice, and cover manager plus runner levels.
|
Addressed the latest review (DenyRead on unelevated). Changes
Test plan
|
3 participants
Summary
Does not add Users/Authenticated Users to the Windows restricted-token SID list. Preflight DenyWrite scans and non-inheriting ACEs cannot enforce a write boundary for the command's lifetime (reparse coverage gaps and post-scan group-writable children). Until access-time confinement exists (AppContainer/LPAC or equivalent), the runner keeps the narrow restricting-SID set.
This PR documents that boundary, disables SID broadening permanently in the command runner, stops planning shared system-path DenyWrite ACEs that only compensated for broadening, and retains write-root revoke cleanup so hosts that ran earlier experimental builds do not keep stale denies over configured write roots.
Changes
internal/sandbox/windows_command_runner_windows.go:broadenReadSIDsis always false; no pre-command descendant coverage gate for broadening.internal/sandbox/windows_acl.go: no new shared DenyWrite on C:, ProgramData, Windows\Temp, Public; still revoke stable read-only SID on write roots for DenyRead elevated profiles.internal/sandbox/windows_token_windows.go: document that callers must passbroadenReadSIDs=false.Test plan
go test ./internal/sandbox/ -count=1go vet ./internal/sandbox/git diff HEAD --checkZERO_SANDBOX_REAL_SMOKE=1with restricted-token runner still write-jails without Users SIDsFollow-up
A functional DenyRead + fully restricted token that can still launch ordinary Program Files binaries needs OS-enforced write confinement at open time, not a preflight path walk.