fix(sandbox): keep Windows restricted-token SIDs narrow and fail closed on DenyRead - #1006
fix(sandbox): keep Windows restricted-token SIDs narrow and fail closed on DenyRead#1006euxaristia wants to merge 6 commits into
Conversation
Co-Authored-By: cairn-code <cairn-code@users.noreply.github.com>
…setup Skip inherited ACEs when preserving read deny entries, correct syntax in the command runner error message, and evaluate unsupported DenyRead profiles before the elevation check in Windows sandbox setup. Refs Gitlawb#640
…ean dead descendant machinery. Refs Gitlawb#640
Greptile SummaryThis PR rejects Windows restricted-token profiles containing DenyRead, narrows newly applied DenyWrite masks, and refreshes setup markers for legacy ACL migration.
Confidence Score: 3/5The PR should not merge until legacy descendant ACL migration and the malformed denied-write smoke probes are corrected. Setup refresh only migrates exact paths represented in the current plan, leaving previously propagated SYNCHRONIZE denies behind, while the new cmd.exe quoting can make confinement probes pass because of malformed redirects rather than enforced access denial. Files Needing Attention: internal/sandbox/windows_acl_apply_windows.go; internal/sandbox/runner_windows_integration_test.go
|
| Filename | Overview |
|---|---|
| internal/sandbox/windows_acl_apply_windows.go | Adds exact-path in-place ACL migration, but leaves legacy descendant ACEs outside the migration path. |
| internal/sandbox/runner_windows_integration_test.go | Adds broader smoke probes, but command quoting can make denied writes fail syntactically and probe cleanup does not establish ownership. |
| internal/sandbox/windows_command_runner.go | Consistently rejects unsupported restricted-token DenyRead profiles before persistent setup or command launch. |
| internal/sandbox/windows_setup.go | Bumps the setup marker schema and adds early DenyRead rejection, though the resulting refresh cannot migrate unplanned descendant ACEs. |
| internal/sandbox/windows_acl.go | Extends ACL entry identity with inheritance shape and defines migration actions without introducing a current plan-generation path for revocation. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Existing schema-v4 installation] --> B[Schema-v5 setup refresh]
B --> C[Build current ACL plan]
C --> D{Exact DenyWrite path and SID in plan?}
D -->|Yes| E[Migrate matching ACE to narrow mask]
D -->|No: legacy descendant| F[ACE is not visited]
F --> G[Legacy SYNCHRONIZE deny remains]
Reviews (1): Last reviewed commit: "Migrate legacy Windows DenyWrite ACEs, r..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if entry.Action == WindowsACLDenyWrite { | ||
| // Replace any pre-existing broader DenyWrite mask (e.g. from | ||
| // builds that included SYNCHRONIZE) with the current narrow | ||
| // mask. We patch the mask in-place within a DACL copy rather | ||
| // than filtering the old ACE and re-adding via ACLFromEntries, | ||
| // because SetEntriesInAcl merges DENY entries for the same | ||
| // SID — which would combine the new DenyWrite with any | ||
| // co-resident DenyRead into a single deny-all ACE. | ||
| if baseDACL != nil && windowsHasExplicitDenyWriteForSID(baseDACL, sid) { | ||
| migrated, err := windowsMigrateDenyWriteInDACL(baseDACL, sid) | ||
| if err != nil { | ||
| return nil, nil, err | ||
| } | ||
| baseDACL = migrated | ||
| continue | ||
| } |
There was a problem hiding this comment.
On upgraded hosts with legacy DenyWrite ACEs propagated to descendants, migration only processes exact paths represented in the current plan. Those descendant ACEs retain SYNCHRONIZE, so directory access and synchronization can continue failing after the schema refresh.
| func deniedWriteCommand(marker string) []string { | ||
| return []string{"cmd.exe", "/d", "/s", "/c", "echo leaked>" + marker + " || exit " + strconv.Itoa(deniedWriteExitCode)} | ||
| return []string{"cmd.exe", "/d", "/s", "/c", "echo leaked>" + cmdQuote(marker) + " || exit " + strconv.Itoa(deniedWriteExitCode)} | ||
| } |
There was a problem hiding this comment.
Every denied-write probe uses /d /s /c, which misses the runner's raw /d /c handling and causes syscall.EscapeArg to backslash-escape cmdQuote's inner quotes. The redirect then fails on a malformed target and returns the expected denial code even when sandbox confinement is broken, producing a false-positive smoke test.
| func deniedWriteCommand(marker string) []string { | |
| return []string{"cmd.exe", "/d", "/s", "/c", "echo leaked>" + marker + " || exit " + strconv.Itoa(deniedWriteExitCode)} | |
| return []string{"cmd.exe", "/d", "/s", "/c", "echo leaked>" + cmdQuote(marker) + " || exit " + strconv.Itoa(deniedWriteExitCode)} | |
| } | |
| func deniedWriteCommand(marker string) []string { | |
| return []string{"cmd.exe", "/d", "/c", "echo leaked>" + cmdQuote(marker) + " || exit " + strconv.Itoa(deniedWriteExitCode)} | |
| } |
| } | ||
| p := &sharedDirectoryProbe{path: probePath} | ||
| t.Cleanup(func() { | ||
| p.cleanup(t) |
There was a problem hiding this comment.
The allocator only observes that a shared-directory path is absent; cleanup later removes anything that occupies that path and ignores removal errors. This can delete another process's file created after allocation or silently leave the test's own probe behind.
Context Used: AGENTS.md (source)
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (15)
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review. WalkthroughThe Windows sandbox now rejects ChangesWindows sandbox enforcement
Estimated code review effort: 4 (Complex) | ~60 minutes Merge Risk: ⚪ Minimal · up to The Windows sandbox changes have no substantiated merge-blocking issue; the reported lint diagnostics are advisory only. Sequence Diagram(s)sequenceDiagram
participant SandboxManager
participant WindowsCommandRunner
participant WindowsSetup
participant CapabilitySIDState
SandboxManager->>WindowsCommandRunner: validate restricted-token DenyRead profile
WindowsCommandRunner-->>SandboxManager: return unsupported-profile error
WindowsSetup->>WindowsCommandRunner: validate permission profile
WindowsCommandRunner-->>WindowsSetup: return status 1 before setup
WindowsCommandRunner->>CapabilitySIDState: load or create state only after validation
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
1 participant
Summary
Windows restricted-token sandboxing cannot support
DenyReadwithout access-time confinement because omittingUsers/Authenticated Usersprevents system binaries from executing, while adding those groups reopens ambient write access. This PR rejects unsupportedDenyReadconfigurations upfront before setup/token creation, migrates legacySYNCHRONIZEDenyWrite ACEs in-place on upgraded hosts, randomizes shared-directory smoke test probes, and removes dead descendant scanning machinery.Refs #639
Changes
DenyReadprofiles upfront ininternal/sandbox/windows_command_runner.goandwindows_setup_windows.go.DenyWriteACEs containingSYNCHRONIZEin-place while preserving co-residentDenyReadininternal/sandbox/windows_acl_apply_windows.go.allocateSharedDirectoryProbeininternal/sandbox/runner_windows_integration_test.go.Test plan
go test ./internal/sandbox/... -count=1go run ./cmd/zero-release buildgo run ./cmd/zero-release smokeSummary by CodeRabbit
DenyReadconfigurations before setup or process launch, with clearer error messages.