From 92ca672f51dbe91ae2ec55ec7c1ec11a09b22ae6 Mon Sep 17 00:00:00 2001 From: euxaristia Date: Sat, 22 Aug 2026 17:10:03 -0400 Subject: [PATCH 1/9] fix(sandbox): keep Windows restricted-token SIDs narrow and fail closed on DenyRead --- internal/sandbox/manager_test.go | 90 +- internal/sandbox/profile.go | 9 +- .../runner_windows_integration_test.go | 83 +- internal/sandbox/windows_acl.go | 68 +- internal/sandbox/windows_acl_apply_windows.go | 253 +++++- .../sandbox/windows_acl_apply_windows_test.go | 282 +++++++ internal/sandbox/windows_acl_descendants.go | 73 ++ .../sandbox/windows_acl_descendants_test.go | 95 +++ .../windows_acl_descendants_windows.go | 509 ++++++++++++ .../windows_acl_descendants_windows_test.go | 773 ++++++++++++++++++ internal/sandbox/windows_acl_paths_other.go | 34 + internal/sandbox/windows_acl_paths_windows.go | 44 + internal/sandbox/windows_acl_test.go | 185 ++++- internal/sandbox/windows_command_runner.go | 43 + .../sandbox/windows_command_runner_test.go | 60 ++ .../sandbox/windows_command_runner_windows.go | 19 +- internal/sandbox/windows_runner.go | 8 +- internal/sandbox/windows_setup_windows.go | 6 + 18 files changed, 2604 insertions(+), 30 deletions(-) create mode 100644 internal/sandbox/windows_acl_descendants.go create mode 100644 internal/sandbox/windows_acl_descendants_test.go create mode 100644 internal/sandbox/windows_acl_descendants_windows.go create mode 100644 internal/sandbox/windows_acl_descendants_windows_test.go create mode 100644 internal/sandbox/windows_acl_paths_other.go create mode 100644 internal/sandbox/windows_acl_paths_windows.go create mode 100644 internal/sandbox/windows_command_runner_test.go diff --git a/internal/sandbox/manager_test.go b/internal/sandbox/manager_test.go index b930420f4..6e2941602 100644 --- a/internal/sandbox/manager_test.go +++ b/internal/sandbox/manager_test.go @@ -204,12 +204,19 @@ func TestSandboxManagerBuildsCommandPlanThroughWindowsRunner(t *testing.T) { windowsSandboxInitialized = func() bool { return true } backend := Backend{Name: BackendWindowsRestrictedToken, Available: true, Executable: `C:\zero\zero-windows-command-runner.exe`, Platform: "windows"} policy := DefaultPolicy() + // Build the usual restricted FS profile, then clear DenyRead. On non-Windows + // hosts PermissionProfileFromPolicy injects credential-store DenyRead paths, + // and the Windows plan path rejects any non-empty DenyRead (PR #640). This + // happy-path plan must exercise a valid restricted profile without DenyRead; + // rejection coverage lives in TestSandboxManagerRejectsWindowsDenyReadOnBothRestrictedTokenTiers. + profile := PermissionProfileFromPolicy(`C:\workspace`, policy, nil) + profile.FileSystem.DenyRead = nil manager := NewSandboxManager(SandboxManagerOptions{GOOS: "windows", Backend: backend}) plan, err := manager.BuildCommandPlan(SandboxManagerRequest{ WorkspaceRoot: `C:\workspace`, Command: CommandSpec{Name: "cmd.exe", Args: []string{"/d", "/s", "/c", "dir"}, Dir: `C:\workspace\src`, Env: []string{"PATH=C:\\Tools", "TERM=xterm"}}, Policy: policy, - Profile: PermissionProfileFromPolicy(`C:\workspace`, policy, nil), + Profile: profile, Preference: SandboxPreferenceAuto, ValidateExecution: true, }) @@ -243,6 +250,87 @@ func TestSandboxManagerBuildsCommandPlanThroughWindowsRunner(t *testing.T) { } } +// TestSandboxManagerRejectsWindowsDenyReadOnBothRestrictedTokenTiers is the +// regression for PR #640: DenyRead cannot be launched or provisioned through +// either the elevated restricted-token path or the unelevated auto fallback. +// Both build the same fully restricted narrow-SID token. +func TestSandboxManagerRejectsWindowsDenyReadOnBothRestrictedTokenTiers(t *testing.T) { + backend := Backend{Name: BackendWindowsRestrictedToken, Available: true, Executable: `C:\zero\zero-windows-command-runner.exe`, Platform: "windows"} + manager := NewSandboxManager(SandboxManagerOptions{GOOS: "windows", Backend: backend}) + policy := DefaultPolicy() + profile := PermissionProfile{ + FileSystem: FileSystemPolicy{ + Kind: FileSystemRestricted, + WriteRoots: []WritableRoot{{Root: `C:\workspace`}}, + DenyRead: []string{`C:\workspace\secret`}, + }, + Network: NetworkPolicy{Mode: NetworkDeny}, + } + cmd := CommandSpec{Name: "cmd.exe", Args: []string{"/c", "dir"}, Dir: `C:\workspace`} + + t.Run("elevated_restricted_token", func(t *testing.T) { + restore := windowsSandboxInitialized + t.Cleanup(func() { windowsSandboxInitialized = restore }) + windowsSandboxInitialized = func() bool { return true } + + _, err := manager.BuildCommandPlan(SandboxManagerRequest{ + WorkspaceRoot: `C:\workspace`, + Command: cmd, + Policy: policy, + Profile: profile, + Preference: SandboxPreferenceAuto, + ValidateExecution: true, + }) + if err == nil { + t.Fatal("expected BuildCommandPlan error for elevated restricted-token DenyRead") + } + msg := err.Error() + for _, want := range []string{"DenyRead", "not supported", "restricted-token"} { + if !strings.Contains(msg, want) { + t.Fatalf("error %q missing %q", msg, want) + } + } + }) + + t.Run("unelevated_auto_fallback", func(t *testing.T) { + restore := windowsSandboxInitialized + t.Cleanup(func() { windowsSandboxInitialized = restore }) + windowsSandboxInitialized = func() bool { return false } + + req, err := manager.BuildExecutionRequest(SandboxManagerRequest{ + WorkspaceRoot: `C:\workspace`, + Command: cmd, + Policy: policy, + Profile: profile, + Preference: SandboxPreferenceAuto, + ValidateExecution: true, + }) + if err != nil { + t.Fatalf("BuildExecutionRequest: %v", err) + } + if req.EnforcementLevel != EnforcementUnelevated { + t.Fatalf("EnforcementLevel = %v, want unelevated auto fallback before DenyRead rejection", req.EnforcementLevel) + } + _, err = manager.BuildCommandPlan(SandboxManagerRequest{ + WorkspaceRoot: `C:\workspace`, + Command: cmd, + Policy: policy, + Profile: profile, + Preference: SandboxPreferenceAuto, + ValidateExecution: true, + }) + if err == nil { + t.Fatal("expected BuildCommandPlan error for unelevated DenyRead") + } + if !strings.Contains(err.Error(), "DenyRead") || !strings.Contains(err.Error(), "not supported") { + t.Fatalf("unelevated DenyRead error = %v", err) + } + if strings.Contains(err.Error(), "Use `--sandbox forbid`, the unelevated") { + t.Fatalf("error still recommends unelevated as a workaround: %v", err) + } + }) +} + func TestSandboxManagerDegradesUnavailableCommandPlan(t *testing.T) { policy := DefaultPolicy() backend := Backend{Name: BackendUnavailable, Platform: "windows", Fallback: true, Message: "native sandbox unavailable"} diff --git a/internal/sandbox/profile.go b/internal/sandbox/profile.go index 349e2b1c6..ab44721b4 100644 --- a/internal/sandbox/profile.go +++ b/internal/sandbox/profile.go @@ -224,10 +224,11 @@ type credentialDenyPaths struct { // the preserved caller environment and Zero's own config/token stores. Four // deliberate limits: // -// - Windows is skipped: a non-empty profile DenyRead switches the Windows -// runner onto the capability-SID/ACL deny path and away from the -// WRITE_RESTRICTED token, which the unelevated tier depends on. Revisit -// once the Windows deny-read model is settled. +// - Windows is skipped: a non-empty profile DenyRead is unsupported on both +// restricted-token runner levels under the narrow SID set (PR #640). The +// fully restricted token cannot load ordinary system binaries without +// Users/AuthUsers, and adding those groups reopens write grants outside +// WriteRoots. Revisit once access-time confinement exists. // - A candidate nested under a user-configured AllowRead entry is dropped, // so `allowRead: ["~/.aws"]` remains an explicit opt-out. // - Candidates are emitted whether or not they currently exist on disk. diff --git a/internal/sandbox/runner_windows_integration_test.go b/internal/sandbox/runner_windows_integration_test.go index fb98f1287..bf7697a7f 100644 --- a/internal/sandbox/runner_windows_integration_test.go +++ b/internal/sandbox/runner_windows_integration_test.go @@ -73,6 +73,28 @@ func TestWindowsRestrictedTokenRealSandboxSmoke(t *testing.T) { t.Fatalf("sandboxed write marker = %q, %v; want ok", bytes, err) } + // SID broadening is disabled, so the restricted-SID list never includes + // Users/Authenticated Users. The write grant those groups hold on + // C:\Users\Public must not be reachable through the restricted-SID check. + // Pin that a write there fails: an independent shared-writable directory + // outside every workspace write root. + publicDir := os.Getenv("PUBLIC") + if publicDir == "" { + t.Log("PUBLIC is not set; skipping C:\\Users\\Public write-jail probe") + } else { + publicMarker := filepath.Join(publicDir, "zero-elevated-write-denied.txt") + _ = os.Remove(publicMarker) + runWindowsRealSmokeCommand(t, runnerExe, config, []string{ + "cmd.exe", "/d", "/s", "/c", "echo leaked>" + publicMarker, + }, 1) + if _, err := os.Stat(publicMarker); err == nil { + _ = os.Remove(publicMarker) + t.Fatalf("Windows sandbox allowed a write to the shared C:\\Users\\Public directory") + } else if !os.IsNotExist(err) { + t.Fatalf("stat public marker: %v", err) + } + } + listener, err := net.Listen("tcp4", "127.0.0.1:0") if err != nil { t.Fatalf("listen loopback for Windows network smoke: %v", err) @@ -156,12 +178,14 @@ func TestWindowsUnelevatedRealSandboxSmoke(t *testing.T) { } sandboxHome := filepath.Join(root, ".zero-sandbox") + // Success path: restricted FS write-jail with no DenyRead. Non-empty DenyRead + // is unsupported on both restricted-token tiers under the narrow SID set + // (PR #640); the rejection probe below covers that separately. profile := PermissionProfile{ FileSystem: FileSystemPolicy{ Kind: FileSystemRestricted, ReadRoots: []string{root}, WriteRoots: []WritableRoot{{Root: root, ProtectedMetadataNames: []string{".git", ".zero", ".agents"}}}, - DenyRead: []string{privateDir}, IncludePlatformRoots: true, AllowTemp: true, }, @@ -190,10 +214,18 @@ func TestWindowsUnelevatedRealSandboxSmoke(t *testing.T) { t.Fatalf("expected the unelevated setup marker to be recorded: %v", err) } - // DenyRead check: reading from the privateDir must be blocked (exit code 1) - runWindowsRealSmokeCommand(t, runnerExe, config, []string{ + // DenyRead is unsupported on both restricted-token tiers under the narrow + // SID set (PR #640): the runner must reject before launch rather than + // attempting a fully restricted token that cannot load system tools. + denyReadConfig := config + denyReadConfig.PermissionProfile.FileSystem.DenyRead = []string{privateDir} + runWindowsRealSmokeCommandExpectError(t, runnerExe, denyReadConfig, []string{ "cmd.exe", "/d", "/s", "/c", "type " + secretFile, - }, 1) + }, "DenyRead", "not supported") + // The secret must remain readable from the host; the sandbox never ran. + if data, err := os.ReadFile(secretFile); err != nil || string(data) != "super-secret" { + t.Fatalf("host secret file after rejected DenyRead launch: %q, %v", data, err) + } outsideMarker := filepath.Join(outside, "unelevated-write-denied.txt") runWindowsRealSmokeCommand(t, runnerExe, config, []string{ @@ -204,6 +236,24 @@ func TestWindowsUnelevatedRealSandboxSmoke(t *testing.T) { } else if !os.IsNotExist(err) { t.Fatalf("stat outside marker: %v", err) } + + // Verify write to C:\ProgramData is blocked + programData := os.Getenv("ProgramData") + if programData != "" { + programDataMarker := filepath.Join(programData, "zero-unelevated-write-denied.txt") + _ = os.Remove(programDataMarker) + + runWindowsRealSmokeCommand(t, runnerExe, config, []string{ + "cmd.exe", "/d", "/s", "/c", "echo leaked>" + programDataMarker, + }, 1) + + if _, err := os.Stat(programDataMarker); err == nil { + _ = os.Remove(programDataMarker) + t.Fatalf("unelevated sandbox allowed a write to ProgramData shared directory") + } else if !os.IsNotExist(err) { + t.Fatalf("stat ProgramData marker: %v", err) + } + } } // TestWindowsRestrictedTokenNestedPipeCapture pins the fix in @@ -387,6 +437,31 @@ func runWindowsRealSmokeCommand(t *testing.T, runnerExe string, base WindowsSand } } +// runWindowsRealSmokeCommandExpectError runs the command runner and requires a +// non-zero exit whose combined output contains each want substring (used for +// explicit unsupported-mode rejections rather than sandboxed command failures). +func runWindowsRealSmokeCommandExpectError(t *testing.T, runnerExe string, base WindowsSandboxCommandArgsOptions, command []string, wantSubstr ...string) { + t.Helper() + base.Command = command + args, err := BuildWindowsSandboxCommandArgs(base) + if err != nil { + t.Fatalf("BuildWindowsSandboxCommandArgs: %v", err) + } + ctx, cancel := context.WithTimeout(context.Background(), 20*time.Second) + defer cancel() + cmd := exec.CommandContext(ctx, runnerExe, args...) + output, err := cmd.CombinedOutput() + if err == nil { + t.Fatalf("Windows sandbox command exit code = 0, want error containing %v\n%s", wantSubstr, output) + } + text := string(output) + for _, want := range wantSubstr { + if !strings.Contains(text, want) { + t.Fatalf("Windows sandbox command error missing %q: %v\n%s", want, err, output) + } + } +} + // The write jail must hold on a path whose DACL grants Everyone write access. // // A WRITE_RESTRICTED token runs TWO checks for a write and needs both to pass: diff --git a/internal/sandbox/windows_acl.go b/internal/sandbox/windows_acl.go index 55d37d347..a2a391b60 100644 --- a/internal/sandbox/windows_acl.go +++ b/internal/sandbox/windows_acl.go @@ -2,6 +2,7 @@ package sandbox import ( "errors" + "fmt" "path/filepath" "strings" ) @@ -12,13 +13,65 @@ const ( WindowsACLAllowWrite WindowsACLAction = "allow-write" WindowsACLDenyRead WindowsACLAction = "deny-read" WindowsACLDenyWrite WindowsACLAction = "deny-write" + // WindowsACLRevokeCapability removes any existing ACE (allow or deny) for + // Capability at Path, without itself granting or denying anything (applied + // via SetEntriesInAclW's SET_ACCESS mode with a zero mask, not + // REVOKE_ACCESS — see windowsACLAccess for why). It reconciles stale + // shared/descendant DenyWrite ACEs an earlier setup run applied for the + // stable read-only capability SID (see BuildWindowsACLPlan) that a later + // run no longer intends: if a path previously covered by the + // shared-root/descendant DenyWrite mitigation is later configured as an + // allowed write root, that old deny is otherwise left on disk and wins + // over the new Allow under Windows' deny-before-allow evaluation — see + // jatmn's review. Clearing a SID with no matching ACE is a safe no-op, so + // this can always be emitted unconditionally alongside every write-root + // Allow entry. + WindowsACLRevokeCapability WindowsACLAction = "revoke-capability" ) type WindowsACLEntry struct { - Action WindowsACLAction `json:"action"` - Path string `json:"path"` - Capability string `json:"capability"` - Materialize bool `json:"materialize,omitempty"` + Action WindowsACLAction `json:"action"` + Path string `json:"path"` + Capability string `json:"capability"` + // NoInherit forces the applied ACE to carry no inheritance flags, even + // when the target is a directory. Without it, applyWindowsACLPlan makes + // every directory ACE inheritable (SUB_CONTAINERS_AND_OBJECTS_INHERIT), + // and SetNamedSecurityInfo automatically propagates any inheritable ACE + // down onto the target's EXISTING descendants (not just new ones it + // creates going forward) — see the shared-deny-path entries below for + // why that is unsafe on broad system roots. + NoInherit bool `json:"noInherit,omitempty"` + Materialize bool `json:"materialize,omitempty"` + // ScanDescendants marks a shared-root DenyWrite entry whose EXISTING + // writable descendants must ALSO be denied, one direct (non-inheriting) + // deny per writable descendant, at apply time. A non-inherited deny on the + // root object alone does not cover a pre-existing child that independently + // grants Users/Authenticated Users write, because a Windows access check + // for that child never consults a non-inherited ACE on its parent. This is + // deliberately NOT serialized (json:"-"): the concrete descendant set is + // live-filesystem state that differs between the setup process and a later + // command run, so folding it into the hashed plan would make + // ValidateWindowsSandboxSetupMarker non-deterministic. The flag itself is + // derived deterministically from the same inputs on both sides, and the + // descendant enumeration/denies happen as an apply-time side effect in + // applyWindowsACLPlan (windows-only), never in the cross-platform plan hash. + ScanDescendants bool `json:"-"` + // RevokeDescendants marks a write-root's WindowsACLRevokeCapability entry + // (see the constant below) as needing the same stale-deny cleanup applied + // recursively to the root's existing descendants, not just the root path + // itself. A tree scanned and denied by an earlier setup run (either + // because it WAS one of the four shared roots, or because it was a + // writable descendant applyWindowsSharedDescendantDenies found and denied + // elsewhere in the tree) can later be promoted to an allowed write root by + // the caller configuring some ANCESTOR of it as a WriteRoot. Revoking only + // at the exact configured root leaves any stale direct, non-inheriting + // deny on that ancestor's descendants in place, and a stale deny wins over + // the newly-added inheritable Allow under Windows' deny-before-allow + // evaluation — see jatmn's review. Like ScanDescendants, this is + // deliberately NOT serialized (json:"-"): the concrete stale-deny set is + // live-filesystem state, and the actual descendant walk/revoke happens as + // an apply-time side effect in applyWindowsACLPlan (windows-only). + RevokeDescendants bool `json:"-"` } type WindowsACLPlan struct { @@ -76,6 +129,7 @@ func BuildWindowsACLPlan(config WindowsSandboxCommandConfig) (WindowsACLPlan, er }) } } + return WindowsACLPlan{Entries: dedupeWindowsACLEntries(entries)}, nil } @@ -184,7 +238,11 @@ func dedupeWindowsACLEntries(entries []WindowsACLEntry) []WindowsACLEntry { if entry.Action == "" || strings.TrimSpace(entry.Path) == "" || strings.TrimSpace(entry.Capability) == "" { continue } - key := string(entry.Action) + "\x00" + windowsCapabilityPathKey(entry.Path) + "\x00" + strings.ToLower(entry.Capability) + // NoInherit is part of the identity: a direct-only deny and an + // inheritable one on the same path/SID are different ACL shapes, and + // collapsing them could silently promote a deliberately non-inherited + // shared-path deny into an inheritable one (or vice versa). + key := string(entry.Action) + "\x00" + windowsCapabilityPathKey(entry.Path) + "\x00" + strings.ToLower(entry.Capability) + "\x00" + fmt.Sprintf("%t", entry.NoInherit) if _, ok := seen[key]; ok { continue } diff --git a/internal/sandbox/windows_acl_apply_windows.go b/internal/sandbox/windows_acl_apply_windows.go index c666aa9ef..45384f061 100644 --- a/internal/sandbox/windows_acl_apply_windows.go +++ b/internal/sandbox/windows_acl_apply_windows.go @@ -6,6 +6,7 @@ import ( "errors" "fmt" "os" + "path/filepath" "sort" "strings" @@ -28,6 +29,7 @@ type windowsACLSnapshot struct { func applyWindowsACLPlan(plan WindowsACLPlan) (func() error, error) { groups := groupWindowsACLPlanByPath(plan) + writeRoots := windowsPlanAllowWriteRoots(plan) snapshots := make([]windowsACLSnapshot, 0, len(groups)) for _, group := range groups { snapshot, applied, err := applyWindowsACLPathGroup(group) @@ -41,12 +43,150 @@ func applyWindowsACLPlan(plan WindowsACLPlan) (func() error, error) { if applied { snapshots = append(snapshots, snapshot) } + // A shared-root deny only protects the root object itself; its existing + // writable descendants each need their own direct deny (see + // windows_acl_descendants_windows.go). Only scan once the root deny + // actually applied (applied == true means the root exists). + if denySID, ok := windowsGroupScanDescendantsSID(group); ok && applied { + descendantSnapshots, err := applyWindowsSharedDescendantDenies(group.Path, denySID, writeRoots) + snapshots = append(snapshots, descendantSnapshots...) + if err != nil { + rollbackErr := rollbackWindowsACLSnapshots(snapshots) + if rollbackErr != nil { + return nil, fmt.Errorf("%w; rollback failed: %v", err, rollbackErr) + } + return nil, err + } + } + // A write root's stale-deny revoke only clears the root path itself; + // clear the same stale deny from its existing descendants too, or a + // stray direct deny an earlier run left there keeps winning over this + // root's new inheritable Allow (see windows_acl.go's RevokeDescendants + // doc and jatmn's review). Best-effort: leaving a stale deny in place + // only over-restricts an explicitly configured write root, it never + // widens access, so this never fails the whole plan apply. + if denySID, ok := windowsGroupRevokeDescendantsSID(group); ok && applied { + snapshots = append(snapshots, windowsRevokeStaleDescendantDenies(group.Path, denySID)...) + } } return func() error { return rollbackWindowsACLSnapshots(snapshots) }, nil } +// windowsPlanAllowWriteRoots collects the plan's allow-write root paths so the +// descendant scan can exclude a configured write root (and anything under it): +// a write root that happens to live under one of the shared roots must never be +// jailed by a compensating deny. +func windowsPlanAllowWriteRoots(plan WindowsACLPlan) []string { + var roots []string + for _, entry := range plan.Entries { + if entry.Action == WindowsACLAllowWrite { + if path := strings.TrimSpace(entry.Path); path != "" { + roots = append(roots, path) + } + } + } + return roots +} + +// windowsGroupScanDescendantsSID returns the deny SID of a group's shared-root +// DenyWrite entry when that entry requests descendant scanning. +func windowsGroupScanDescendantsSID(group windowsACLPathGroup) (string, bool) { + for _, entry := range group.Entries { + if entry.Action == WindowsACLDenyWrite && entry.ScanDescendants && strings.TrimSpace(entry.Capability) != "" { + return entry.Capability, true + } + } + return "", false +} + +// windowsGroupRevokeDescendantsSID returns the capability SID of a group's +// write-root stale-deny revoke entry when that entry requests clearing the +// same stale deny from the root's existing descendants too (see +// RevokeDescendants). +func windowsGroupRevokeDescendantsSID(group windowsACLPathGroup) (string, bool) { + for _, entry := range group.Entries { + if entry.Action == WindowsACLRevokeCapability && entry.RevokeDescendants && strings.TrimSpace(entry.Capability) != "" { + return entry.Capability, true + } + } + return "", false +} + +// windowsRevokeStaleDescendantDenies walks a newly-promoted write root's +// existing descendants and clears any direct DenyWrite ACE they carry for +// denySID — left over from when an earlier `zero sandbox setup` run found +// this same subtree writable by Users/Authenticated Users and applied the +// shared-root compensating deny (windows_acl_descendants_windows.go) before +// the caller configured this path as an allowed write root. That stale, +// non-inheriting deny on a descendant still wins over the root's own new, +// inheritable Allow under Windows' deny-before-allow ACE evaluation, so the +// root would otherwise remain partly unwritable — see jatmn's review. +// +// This is deliberately best-effort, not fail-closed like the writable- +// descendant scan: leaving a stray stale deny in place only over-restricts an +// explicitly configured write root (a functionality bug), it never widens +// access, so an unreadable descendant or a reparse point here is skipped +// rather than aborting the whole plan apply. Bounded by the same depth/entry +// caps as the writable-descendant scan so a pathological or cyclic tree +// cannot make this run unboundedly long. +func windowsRevokeStaleDescendantDenies(root, denySID string) []windowsACLSnapshot { + type node struct { + path string + depth int + } + var snapshots []windowsACLSnapshot + visited := 0 + queue := []node{{path: root, depth: 0}} + for len(queue) > 0 { + current := queue[0] + queue = queue[1:] + entries, err := os.ReadDir(current.path) + if err != nil { + continue + } + for _, entry := range entries { + child := filepath.Join(current.path, entry.Name()) + // Unlike the writable-descendant scan, this cleanup pass does not + // need to follow reparse points transparently: skipping one just + // means a stray deny under it might survive, which is the same + // safe-but-inconvenient outcome as any other skip here. + if windowsPathIsReparsePoint(child) { + continue + } + if visited >= windowsDescendantScanMaxDirs { + return snapshots + } + visited++ + // Revoking a SID with no matching ACE is a safe no-op. A pre-check + // keyed on complete write-deny coverage would also skip a partial + // stale deny that still blocks writes on a promoted write root. + snapshot, applied, err := applyWindowsACLPathGroup(windowsACLPathGroup{ + Path: child, + Entries: []WindowsACLEntry{{ + Action: WindowsACLRevokeCapability, + Path: child, + Capability: denySID, + NoInherit: true, + }}, + }) + if err == nil && applied { + snapshots = append(snapshots, snapshot) + } + if !entry.IsDir() { + continue + } + depth := current.depth + 1 + if depth >= windowsDescendantScanMaxDepth { + continue + } + queue = append(queue, node{path: child, depth: depth}) + } + } + return snapshots +} + func groupWindowsACLPlanByPath(plan WindowsACLPlan) []windowsACLPathGroup { byPath := map[string]*windowsACLPathGroup{} for _, entry := range dedupeWindowsACLEntries(plan.Entries) { @@ -123,7 +263,7 @@ func applyWindowsACLPathGroup(group windowsACLPathGroup) (windowsACLSnapshot, bo if err != nil { return fail(fmt.Errorf("read windows DACL for %s: %w", path, err)) } - accessEntries, err := windowsExplicitAccessEntries(group.Entries, isDir) + accessEntries, err := windowsExplicitAccessEntries(group.Entries, isDir, oldDACL) if err != nil { return fail(err) } @@ -191,21 +331,46 @@ func windowsACLGroupRequiresExistingTarget(group windowsACLPathGroup) bool { return false } -func windowsExplicitAccessEntries(entries []WindowsACLEntry, isDir bool) ([]windows.EXPLICIT_ACCESS, error) { +func windowsExplicitAccessEntries(entries []WindowsACLEntry, isDir bool, oldDACL *windows.ACL) ([]windows.EXPLICIT_ACCESS, error) { out := make([]windows.EXPLICIT_ACCESS, 0, len(entries)) - inheritance := uint32(0) - if isDir { - inheritance = windows.SUB_CONTAINERS_AND_OBJECTS_INHERIT - } for _, entry := range entries { sid, err := windows.StringToSid(entry.Capability) if err != nil { return nil, fmt.Errorf("parse windows capability SID %q: %w", entry.Capability, err) } + if entry.Action == WindowsACLRevokeCapability { + // Migration cleanup for hosts that ran experimental SID-broadening + // builds: strip the synthetic full DenyWrite ACE for this SID, but + // re-emit any co-resident DenyRead ACEs for the same stable SID so + // a concurrent profile's read boundary is not deleted (jatmn P1). + // SET_ACCESS with a zero mask clears every ACE for the trustee + // (REVOKE_ACCESS leaves DENY ACEs untouched empirically); the + // preserved read-deny entries that follow restore DenyRead only. + out = append(out, windows.EXPLICIT_ACCESS{ + AccessPermissions: 0, + AccessMode: windows.SET_ACCESS, + Inheritance: 0, + Trustee: windows.TRUSTEE{ + TrusteeForm: windows.TRUSTEE_IS_SID, + TrusteeType: windows.TRUSTEE_IS_GROUP, + TrusteeValue: windows.TrusteeValueFromSID(sid), + }, + }) + preserved, err := windowsPreservedReadDenyAccessEntries(oldDACL, sid, isDir) + if err != nil { + return nil, err + } + out = append(out, preserved...) + continue + } accessMode, permissions, err := windowsACLAccess(entry.Action) if err != nil { return nil, err } + inheritance := uint32(0) + if isDir && !entry.NoInherit { + inheritance = windows.SUB_CONTAINERS_AND_OBJECTS_INHERIT + } out = append(out, windows.EXPLICIT_ACCESS{ AccessPermissions: permissions, AccessMode: accessMode, @@ -220,6 +385,77 @@ func windowsExplicitAccessEntries(entries []WindowsACLEntry, isDir bool) ([]wind return out, nil } +// windowsPreservedReadDenyAccessEntries returns DENY_ACCESS EXPLICIT_ACCESS +// entries that re-apply any non-write-related DENY ACEs for wantSID from +// oldDACL. Write-related DENY ACEs (the experimental shared/descendant +// DenyWrite shape) are intentionally omitted so migration revoke can drop +// them without also clearing a live DenyRead for the same SID. +func windowsPreservedReadDenyAccessEntries(oldDACL *windows.ACL, wantSID *windows.SID, isDir bool) ([]windows.EXPLICIT_ACCESS, error) { + if oldDACL == nil || wantSID == nil { + return nil, nil + } + var out []windows.EXPLICIT_ACCESS + for index := uint16(0); index < oldDACL.AceCount; index++ { + var ace *windows.ACCESS_ALLOWED_ACE + if err := windows.GetAce(oldDACL, uint32(index), &ace); err != nil { + return nil, fmt.Errorf("read ACE %d while preserving read deny: %w", index, err) + } + if ace.Header.AceType != windows.ACCESS_DENIED_ACE_TYPE && ace.Header.AceType != windowsAccessDeniedObjectAceType { + continue + } + sid, ok := windowsAceSID(ace) + if !ok || !sid.Equals(wantSID) { + continue + } + if windowsIsExperimentalWriteDenyMask(ace.Mask) { + continue + } + // Preserve non-write DENY ACEs (typically DenyRead for the stable + // sandbox-home ReadOnly SID), keeping their original inheritance + // scope rather than promoting every variant to container+object or + // dropping inherit-only ACEs that SET_ACCESS zero-mask already cleared. + inheritance := uint32(0) + if isDir { + inheritance = uint32(ace.Header.AceFlags) & (windows.OBJECT_INHERIT_ACE | + windows.CONTAINER_INHERIT_ACE | + windows.NO_PROPAGATE_INHERIT_ACE | + windows.INHERIT_ONLY_ACE) + } + out = append(out, windows.EXPLICIT_ACCESS{ + AccessPermissions: ace.Mask, + AccessMode: windows.DENY_ACCESS, + Inheritance: inheritance, + Trustee: windows.TRUSTEE{ + TrusteeForm: windows.TRUSTEE_IS_SID, + TrusteeType: windows.TRUSTEE_IS_GROUP, + TrusteeValue: windows.TrusteeValueFromSID(wantSID), + }, + }) + } + return out, nil +} + +// windowsIsExperimentalWriteDenyMask reports whether mask is a synthetic +// DenyWrite (or partial write deny) from earlier broadening builds — the only +// ACEs migration revoke may drop for the stable ReadOnly SID. Pure DenyRead +// masks share some STANDARD_RIGHTS bits with FILE_GENERIC_WRITE, so this keys +// off content-write / delete / DAC bits that DenyRead never carries. +func windowsIsExperimentalWriteDenyMask(mask windows.ACCESS_MASK) bool { + _, writeMask, err := windowsACLAccess(WindowsACLDenyWrite) + if err != nil { + return false + } + if mask&writeMask == writeMask { + return true + } + // Content-write / ownership bits unique to write denies (not in DenyRead's + // FILE_GENERIC_READ|FILE_GENERIC_EXECUTE mask alone). + const writeContent = windows.FILE_WRITE_DATA | windows.FILE_APPEND_DATA | + windows.FILE_WRITE_EA | windows.FILE_WRITE_ATTRIBUTES | + windowsFileDeleteChild | windows.DELETE | windows.WRITE_DAC | windows.WRITE_OWNER + return mask&writeContent != 0 +} + func windowsACLAccess(action WindowsACLAction) (windows.ACCESS_MODE, windows.ACCESS_MASK, error) { switch action { case WindowsACLAllowWrite: @@ -227,7 +463,10 @@ func windowsACLAccess(action WindowsACLAction) (windows.ACCESS_MODE, windows.ACC case WindowsACLDenyRead: return windows.DENY_ACCESS, windows.FILE_GENERIC_READ | windows.FILE_GENERIC_EXECUTE, nil case WindowsACLDenyWrite: - return windows.DENY_ACCESS, windows.FILE_GENERIC_WRITE | windows.DELETE | windowsFileDeleteChild | windows.WRITE_DAC | windows.WRITE_OWNER, nil + return windows.DENY_ACCESS, (windows.FILE_GENERIC_WRITE | windows.DELETE | windowsFileDeleteChild | windows.WRITE_DAC | windows.WRITE_OWNER) &^ windows.SYNCHRONIZE, nil + case WindowsACLRevokeCapability: + // Handled specially in windowsExplicitAccessEntries (preserve DenyRead). + return windows.SET_ACCESS, 0, nil default: return 0, 0, fmt.Errorf("unsupported windows ACL action %q", action) } diff --git a/internal/sandbox/windows_acl_apply_windows_test.go b/internal/sandbox/windows_acl_apply_windows_test.go index f0b7675d0..31910bb47 100644 --- a/internal/sandbox/windows_acl_apply_windows_test.go +++ b/internal/sandbox/windows_acl_apply_windows_test.go @@ -48,6 +48,288 @@ func TestApplyWindowsACLPathGroupHandleBasedRoundTrip(t *testing.T) { } } +// TestApplyWindowsACLPathGroupRevokeCapabilityRemovesStaleDeny is the +// real-Windows regression for jatmn's P2 finding: promoting a path to an +// allowed write root must also remove a stale deny ACE an earlier setup +// round left there for the stable capability SID, not merely omit it from +// this plan. Without the fix, applyWindowsACLPlan's SetEntriesInAcl-based +// merge only touches trustees actually named in the new entry list, so an +// old DenyWrite ACE for a SID the new plan does not mention would survive +// and keep winning over the new Allow under deny-before-allow evaluation. +func TestApplyWindowsACLPathGroupRevokeCapabilityRemovesStaleDeny(t *testing.T) { + // The stale/allow SIDs must be synthetic identities the test process itself + // is not a member of (exactly like the real stable capability SIDs + // LoadOrCreateWindowsCapabilitySIDs mints): a WindowsACLDenyWrite mask + // includes WRITE_DAC/WRITE_OWNER/DELETE, so denying a well-known group the + // test process actually belongs to (e.g. Everyone, BUILTIN\Users) would + // lock the test out of managing — and t.TempDir() out of cleaning up — + // its own fixture. + caps, err := LoadOrCreateWindowsCapabilitySIDs(t.TempDir()) + if err != nil { + t.Fatalf("LoadOrCreateWindowsCapabilitySIDs: %v", err) + } + otherCaps, err := LoadOrCreateWindowsCapabilitySIDs(t.TempDir()) + if err != nil { + t.Fatalf("LoadOrCreateWindowsCapabilitySIDs (other): %v", err) + } + staleSID := caps.ReadOnly + allowSID := otherCaps.ReadOnly + + dir := t.TempDir() + // Simulate the stale deny an earlier setup round applied while this path + // was still covered by the shared-root/descendant DenyWrite mitigation. + if _, _, err := applyWindowsACLPathGroup(windowsACLPathGroup{ + Path: dir, + Entries: []WindowsACLEntry{{ + Action: WindowsACLDenyWrite, + Path: dir, + Capability: staleSID, + NoInherit: true, + }}, + }); err != nil { + t.Fatalf("apply stale deny: %v", err) + } + if !dirDeniesSID(t, dir, staleSID) { + t.Fatalf("test fixture bug: %q does not carry the stale deny it is supposed to", dir) + } + + // Now promote dir to a write root: the plan carries an Allow for a + // different SID plus the reconciling revoke for the stale one. + if _, _, err := applyWindowsACLPathGroup(windowsACLPathGroup{ + Path: dir, + Entries: []WindowsACLEntry{ + {Action: WindowsACLAllowWrite, Path: dir, Capability: allowSID}, + {Action: WindowsACLRevokeCapability, Path: dir, Capability: staleSID, NoInherit: true}, + }, + }); err != nil { + t.Fatalf("apply promotion to write root: %v", err) + } + if dirDeniesSID(t, dir, staleSID) { + t.Fatalf("%q still carries the stale deny for %q after promotion to a write root", dir, staleSID) + } +} + +// TestApplyWindowsACLRevokePreservesDenyRead pins that migration revoke for a +// promoted write root removes only the experimental DenyWrite ACE for the +// stable SID and leaves a co-resident DenyRead for the same SID intact — so a +// concurrent profile's read boundary is not deleted (jatmn P1). +func TestApplyWindowsACLRevokePreservesDenyRead(t *testing.T) { + // Synthetic capability SID (not a group this process is in) so DenyWrite's + // WRITE_DAC/DELETE bits do not lock the test out of its own temp dir. + caps, err := LoadOrCreateWindowsCapabilitySIDs(t.TempDir()) + if err != nil { + t.Fatalf("LoadOrCreateWindowsCapabilitySIDs: %v", err) + } + sid := caps.ReadOnly + dir := t.TempDir() + + // Profile A shape plus experimental broadening: both DenyRead and full + // DenyWrite for the same stable SID on one path (two ACEs in one apply so + // a second DENY_ACCESS merge cannot replace the first). + if _, _, err := applyWindowsACLPathGroup(windowsACLPathGroup{ + Path: dir, + Entries: []WindowsACLEntry{ + { + Action: WindowsACLDenyRead, + Path: dir, + Capability: sid, + NoInherit: true, + }, + { + Action: WindowsACLDenyWrite, + Path: dir, + Capability: sid, + NoInherit: true, + }, + }, + }); err != nil { + t.Fatalf("apply DenyRead+DenyWrite: %v", err) + } + writeDenied, err := windowsPathDeniesCapabilitySID(dir, sid) + if err != nil { + t.Fatalf("windowsPathDeniesCapabilitySID before: %v", err) + } + if !writeDenied { + t.Fatal("fixture: expected full write deny present before revoke") + } + if !dirDeniesReadSID(t, dir, sid) { + t.Fatal("fixture: expected read deny present before revoke") + } + + // Profile B promotes dir to a write root: revoke stale write deny only. + if _, _, err := applyWindowsACLPathGroup(windowsACLPathGroup{ + Path: dir, + Entries: []WindowsACLEntry{{ + Action: WindowsACLRevokeCapability, + Path: dir, + Capability: sid, + NoInherit: true, + }}, + }); err != nil { + t.Fatalf("revoke: %v", err) + } + writeDenied, err = windowsPathDeniesCapabilitySID(dir, sid) + if err != nil { + t.Fatalf("windowsPathDeniesCapabilitySID after: %v", err) + } + if writeDenied { + t.Fatal("write deny for SID still present after migration revoke") + } + if !dirDeniesReadSID(t, dir, sid) { + t.Fatal("DenyRead for same SID was removed by migration revoke; read boundary must be preserved") + } +} + +// TestApplyWindowsACLPlanRevokeDescendantsClearsChildDeny is the regression for +// the RevokeDescendants walk: promoting a root to a write root must clear a +// stale direct deny an earlier run left on an existing child, not only the root +// path itself. Also pins reparse skip and the depth cap as best-effort bounds. +func TestApplyWindowsACLPlanRevokeDescendantsClearsChildDeny(t *testing.T) { + caps, err := LoadOrCreateWindowsCapabilitySIDs(t.TempDir()) + if err != nil { + t.Fatalf("LoadOrCreateWindowsCapabilitySIDs: %v", err) + } + staleSID := caps.ReadOnly + allowSID, err := LoadOrCreateWindowsCapabilitySIDs(t.TempDir()) + if err != nil { + t.Fatalf("LoadOrCreateWindowsCapabilitySIDs (allow): %v", err) + } + + root := t.TempDir() + child := mkdir(t, filepath.Join(root, "child")) + deep := mkdir(t, filepath.Join(child, "deep")) + // Stale full DenyWrite on the direct child (the primary cleanup target). + if _, _, err := applyWindowsACLPathGroup(windowsACLPathGroup{ + Path: child, + Entries: []WindowsACLEntry{{ + Action: WindowsACLDenyWrite, + Path: child, + Capability: staleSID, + NoInherit: true, + }}, + }); err != nil { + t.Fatalf("apply stale deny on child: %v", err) + } + // Partial write deny on deep: the old complete-coverage pre-check would + // skip this; unconditional revoke must still clear it. + denyCapabilityMask(t, deep, staleSID, windows.FILE_WRITE_ATTRIBUTES) + if !dirDeniesSID(t, child, staleSID) { + t.Fatal("fixture: child missing full stale deny") + } + if denyACECountForSID(t, deep, staleSID) == 0 { + t.Fatal("fixture: deep missing partial stale deny") + } + + // Junction under root: walker must skip it (best-effort) without failing. + juncTarget := mkdir(t, filepath.Join(t.TempDir(), "junc-target")) + junc := filepath.Join(root, "junc") + if out, err := exec.Command("cmd", "/c", "mklink", "/J", junc, juncTarget).CombinedOutput(); err != nil { + t.Logf("skipping junction sub-check (mklink /J unavailable): %v %s", err, strings.TrimSpace(string(out))) + } else { + t.Cleanup(func() { _ = os.Remove(junc) }) + if !windowsPathIsReparsePoint(junc) { + t.Fatalf("fixture: %q is not a reparse point", junc) + } + } + + cleanup, err := applyWindowsACLPlan(WindowsACLPlan{Entries: []WindowsACLEntry{ + {Action: WindowsACLAllowWrite, Path: root, Capability: allowSID.ReadOnly}, + { + Action: WindowsACLRevokeCapability, + Path: root, + Capability: staleSID, + NoInherit: true, + RevokeDescendants: true, + }, + }}) + if err != nil { + t.Fatalf("applyWindowsACLPlan: %v", err) + } + t.Cleanup(func() { _ = cleanup() }) + + if dirDeniesSID(t, child, staleSID) { + t.Fatalf("%q still carries full stale deny after RevokeDescendants", child) + } + if denyACECountForSID(t, deep, staleSID) != 0 { + t.Fatalf("%q still carries partial stale deny after RevokeDescendants", deep) + } + + // Depth cap: with max depth 1 the walker revokes root's children but does + // not enqueue them, so a nested deny under a new root is left in place. + cappedRoot := t.TempDir() + cappedChild := mkdir(t, filepath.Join(cappedRoot, "level1")) + cappedDeep := mkdir(t, filepath.Join(cappedChild, "level2")) + if _, _, err := applyWindowsACLPathGroup(windowsACLPathGroup{ + Path: cappedDeep, + Entries: []WindowsACLEntry{{ + Action: WindowsACLDenyWrite, + Path: cappedDeep, + Capability: staleSID, + NoInherit: true, + }}, + }); err != nil { + t.Fatalf("apply stale deny on capped deep: %v", err) + } + oldDepth := windowsDescendantScanMaxDepth + windowsDescendantScanMaxDepth = 1 + t.Cleanup(func() { windowsDescendantScanMaxDepth = oldDepth }) + if _, err := applyWindowsACLPlan(WindowsACLPlan{Entries: []WindowsACLEntry{{ + Action: WindowsACLRevokeCapability, + Path: cappedRoot, + Capability: staleSID, + NoInherit: true, + RevokeDescendants: true, + }}}); err != nil { + t.Fatalf("applyWindowsACLPlan (depth cap): %v", err) + } + if !dirDeniesSID(t, cappedDeep, staleSID) { + t.Fatal("depth cap should leave level2 deny in place when max depth is 1") + } +} + +// dirDeniesReadSID reports whether path's DACL has a DENY ACE for wantSID whose +// mask covers FILE_GENERIC_READ (DenyRead shape) without the full write-probe +// mask of experimental DenyWrite. +func dirDeniesReadSID(t *testing.T, path, wantSID string) bool { + t.Helper() + want, err := windows.StringToSid(wantSID) + if err != nil { + t.Fatalf("StringToSid %q: %v", wantSID, err) + } + sd, err := windows.GetNamedSecurityInfo(path, windows.SE_FILE_OBJECT, windows.DACL_SECURITY_INFORMATION) + if err != nil { + t.Fatalf("GetNamedSecurityInfo %s: %v", path, err) + } + dacl, _, err := sd.DACL() + if err != nil { + t.Fatalf("DACL %s: %v", path, err) + } + if dacl == nil { + return false + } + _, readMask, err := windowsACLAccess(WindowsACLDenyRead) + if err != nil { + t.Fatalf("windowsACLAccess DenyRead: %v", err) + } + for index := uint16(0); index < dacl.AceCount; index++ { + var ace *windows.ACCESS_ALLOWED_ACE + if err := windows.GetAce(dacl, uint32(index), &ace); err != nil { + t.Fatalf("GetAce %d of %s: %v", index, path, err) + } + if ace.Header.AceType != windows.ACCESS_DENIED_ACE_TYPE && ace.Header.AceType != windowsAccessDeniedObjectAceType { + continue + } + sid, ok := windowsAceSID(ace) + if !ok || !sid.Equals(want) { + continue + } + if ace.Mask&readMask == readMask && !windowsIsExperimentalWriteDenyMask(ace.Mask) { + return true + } + } + return false +} + // A materialized target that does not exist yet is created, ACL'd through the // handle, and removed on rollback. func TestApplyWindowsACLPathGroupMaterializes(t *testing.T) { diff --git a/internal/sandbox/windows_acl_descendants.go b/internal/sandbox/windows_acl_descendants.go new file mode 100644 index 000000000..f076c285f --- /dev/null +++ b/internal/sandbox/windows_acl_descendants.go @@ -0,0 +1,73 @@ +package sandbox + +import "strings" + +// Shared basename policies and pure helpers for the Windows descendant-scan +// fail-closed rules. The Win32 walk lives in windows_acl_descendants_windows.go; +// these helpers are compiled on every GOOS so non-Windows tests can pin the +// policy without the Windows APIs. + +// windowsDescendantScanSystemLockedNames are basenames that Windows keeps +// exclusive to SYSTEM (or otherwise unreadable even to elevated Administrators +// without taking ownership). They appear under every fixed volume root. Listing +// or DACL-reading them fails on healthy machines; treating that as incomplete +// coverage would make DenyRead setup fail everywhere. They never grant +// BUILTIN\Users / Authenticated Users write in stock configuration. +var windowsDescendantScanSystemLockedNames = map[string]struct{}{ + "system volume information": {}, + "$recycle.bin": {}, + "recovery": {}, +} + +func windowsDescendantScanNameIsSystemLocked(name string) bool { + _, ok := windowsDescendantScanSystemLockedNames[strings.ToLower(strings.TrimSpace(name))] + return ok +} + +// windowsPathIsDriveRootPath reports whether path is exactly a drive letter +// root such as "C:\" or "C:" (case-insensitively), with no further path +// segments. Used to scope the windowsDescendantScanNameIsSystemLocked +// exception to the one place those basenames are ever legitimately the real, +// SYSTEM-exclusive Windows directory: directly under an actual volume root. +// A directory sharing one of those basenames anywhere else in the tree (e.g. +// nested under ProgramData or Public, whether by installer accident or +// deliberately) is not the real thing and must not be silently skipped — see +// jatmn's review. Pure string check so non-Windows tests can pin it without +// Win32 or filepath's platform-dependent volume parsing. +func windowsPathIsDriveRootPath(path string) bool { + trimmed := strings.TrimSuffix(strings.TrimSpace(path), `\`) + if len(trimmed) != 2 || trimmed[1] != ':' { + return false + } + c := trimmed[0] + return (c >= 'A' && c <= 'Z') || (c >= 'a' && c <= 'z') +} + +// windowsMountPathIsOnlySystemDrive reports whether a volume mount path is the +// system drive root (e.g. `C:\` or `C:`) rather than another letter or a +// folder mount such as `C:\mnt\data`. Used by the volume gate so a second +// fixed volume mounted only as a folder is rejected the same as one mounted +// on a drive letter. +func windowsMountPathIsOnlySystemDrive(mountPath, systemDrive string) bool { + trimmed := strings.TrimSuffix(mountPath, `\`) + return strings.EqualFold(strings.ToUpper(trimmed), strings.ToUpper(systemDrive)) +} + +// windowsMountPathsAreOnlySystemDrive reports whether mountPaths (a fixed +// volume's DOS/folder mount points from GetVolumePathNamesForVolumeName) name +// the system drive root and nothing else. A fixed volume with NO mount paths +// at all is still directly reachable through its raw "\\?\Volume{GUID}\" +// path even though it has no conventional mount point, so an empty list +// fails closed (false) instead of being read as "unreachable." Pure string +// logic so non-Windows tests can pin the fail-closed cases without Win32. +func windowsMountPathsAreOnlySystemDrive(mountPaths []string, systemDrive string) bool { + if len(mountPaths) == 0 { + return false + } + for _, mountPath := range mountPaths { + if !windowsMountPathIsOnlySystemDrive(mountPath, systemDrive) { + return false + } + } + return true +} diff --git a/internal/sandbox/windows_acl_descendants_test.go b/internal/sandbox/windows_acl_descendants_test.go new file mode 100644 index 000000000..c846594dd --- /dev/null +++ b/internal/sandbox/windows_acl_descendants_test.go @@ -0,0 +1,95 @@ +package sandbox + +import "testing" + +func TestWindowsDescendantScanNamePolicies(t *testing.T) { + for _, name := range []string{ + "System Volume Information", + "SYSTEM VOLUME INFORMATION", + "$Recycle.Bin", + "Recovery", + } { + if !windowsDescendantScanNameIsSystemLocked(name) { + t.Fatalf("windowsDescendantScanNameIsSystemLocked(%q) = false, want true", name) + } + } + for _, name := range []string{"ProgramData", "plain", "Users"} { + if windowsDescendantScanNameIsSystemLocked(name) { + t.Fatalf("windowsDescendantScanNameIsSystemLocked(%q) = true, want false", name) + } + } +} + +// TestWindowsPathIsDriveRootPath pins the canonical-root-level scoping fix +// (jatmn's review): the system-locked basename allowlist must only fire +// directly under a genuine drive letter root, never at an arbitrary nested +// path that merely shares the same parent-relative shape. +func TestWindowsPathIsDriveRootPath(t *testing.T) { + for _, path := range []string{`C:\`, `C:`, `c:\`, `Z:\`} { + if !windowsPathIsDriveRootPath(path) { + t.Fatalf("windowsPathIsDriveRootPath(%q) = false, want true", path) + } + } + for _, path := range []string{ + `C:\ProgramData`, + `C:\Users\Public`, + `C:\Windows\Temp`, + ``, + `\\?\Volume{guid}\`, + `relative`, + } { + if windowsPathIsDriveRootPath(path) { + t.Fatalf("windowsPathIsDriveRootPath(%q) = true, want false", path) + } + } +} + +func TestWindowsMountPathIsOnlySystemDrive(t *testing.T) { + cases := []struct { + mount, system string + want bool + }{ + {`C:\`, `C:`, true}, + {`C:`, `C:`, true}, + {`c:\`, `C:`, true}, + {`D:\`, `C:`, false}, + {`C:\mnt\data`, `C:`, false}, + {`C:\mnt\data\`, `C:`, false}, + {`\\?\Volume{guid}\`, `C:`, false}, + } + for _, tc := range cases { + got := windowsMountPathIsOnlySystemDrive(tc.mount, tc.system) + if got != tc.want { + t.Fatalf("windowsMountPathIsOnlySystemDrive(%q, %q) = %v, want %v", tc.mount, tc.system, got, tc.want) + } + } +} + +// TestWindowsMountPathsAreOnlySystemDrive pins the volume-gate fail-closed fix +// (jatmn's review): a fixed volume with no mount points at all must not be +// read as "unreachable" (it is still reachable via its raw +// "\\?\Volume{GUID}\" path), and any mount path other than the system drive +// root disqualifies the volume, matching the existing per-path behavior. +func TestWindowsMountPathsAreOnlySystemDrive(t *testing.T) { + cases := []struct { + name string + mountPaths []string + system string + want bool + }{ + {"only system drive", []string{`C:\`}, `C:`, true}, + {"no mount points at all", nil, `C:`, false}, + {"empty mount list", []string{}, `C:`, false}, + {"extra drive letter", []string{`C:\`, `D:\`}, `C:`, false}, + {"folder mount point", []string{`C:\mnt\data`}, `C:`, false}, + {"other drive only", []string{`D:\`}, `C:`, false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := windowsMountPathsAreOnlySystemDrive(tc.mountPaths, tc.system) + if got != tc.want { + t.Fatalf("windowsMountPathsAreOnlySystemDrive(%#v, %q) = %v, want %v", tc.mountPaths, tc.system, got, tc.want) + } + }) + } +} diff --git a/internal/sandbox/windows_acl_descendants_windows.go b/internal/sandbox/windows_acl_descendants_windows.go new file mode 100644 index 000000000..eabc3d45a --- /dev/null +++ b/internal/sandbox/windows_acl_descendants_windows.go @@ -0,0 +1,509 @@ +//go:build windows + +package sandbox + +import ( + "fmt" + "os" + "path/filepath" + "strings" + "unsafe" + + "golang.org/x/sys/windows" +) + +// Shared-root DenyWrite and Users/Authenticated Users SID broadening are no +// longer shipped: BuildWindowsACLPlan does not stamp machine-wide denies on +// C:\, %ProgramData%, %SystemRoot%\Temp, or C:\Users\Public, and the restricted +// token is never broadened with those groups. This file keeps the descendant +// walker and related helpers for (1) migration cleanup of stale denies left by +// earlier PR builds and (2) Windows regression tests of that walker. +// +// Historical coverage rules the walker still implements (fail closed when used): +// +// - Every directory under a root is considered for descent within the +// depth/entry caps, whether or not the parent itself is Users-writable. +// - Hitting windowsDescendantScanMaxDepth or windowsDescendantScanMaxDirs +// means unexamined territory remains: the scan returns an error rather than +// certifying a partial walk as clean. +// - Reparse points (junctions, symlinks, volume mount points) use a +// no-follow, identity-aware policy (see jatmn's review). The walker: +// 1. Detects reparse points via FILE_ATTRIBUTE_REPARSE_POINT (and the +// ModeSymlink/ModeIrregular bits ReadDir already reports) and never +// inspects their DACL, never applies a deny, and never descends. +// 2. Records the (volume serial, file index) identity of every real +// directory it does enter, so an alternate path to the same object is +// skipped rather than re-enumerated. +// - A directory this process cannot list, or a child whose DACL it cannot +// read, is fail-closed UNLESS the basename is a known SYSTEM-exclusive +// Windows directory (e.g. "System Volume Information") AND it sits +// directly under an actual drive letter root (windowsPathIsDriveRootPath). +// +// Basename policies live in windows_acl_descendants.go so non-Windows tests can +// pin them without Win32. Bounds are vars so Windows tests can lower them. +var ( + windowsDescendantScanMaxDepth = 48 + // windowsDescendantScanMaxDirs bounds the total files+directories the scan + // will inspect below a single root. Kept large enough for stock system + // trees while still bounding worst-case work; the walker is retained for + // migration cleanup and tests, not live elevated DenyRead setup. + windowsDescendantScanMaxDirs = 500000 +) + +// windowsBroadenedWriteProbeMask is the set of access-mask bits that let a +// principal create, delete, or modify content, attributes, or extended +// attributes in (or the security of) a directory, i.e. the bits that make a +// directory a usable write-jail escape. FILE_WRITE_DATA is FILE_ADD_FILE and +// FILE_APPEND_DATA is FILE_ADD_SUBDIRECTORY for a directory object. +const windowsBroadenedWriteProbeMask windows.ACCESS_MASK = (windows.FILE_GENERIC_WRITE | + windowsFileDeleteChild | + windows.DELETE | + windows.WRITE_DAC | + windows.WRITE_OWNER) &^ windows.SYNCHRONIZE + +// applyWindowsSharedDescendantDenies enumerates the existing writable +// descendants of a shared root and applies a direct, non-inheriting DenyWrite +// (naming denySID, the same stable read-only capability SID the root deny uses) +// to each. It returns every snapshot it applied (including on error) so the +// caller can roll the whole apply back. A descendant it identified as writable +// but could not deny is a hole it cannot close, so that failure is returned +// (fail closed). An incomplete enumeration (caps, unreadable non-reparse child) +// is also returned as an error. Reparse points are skipped by the enumerator +// (no-follow). Descendants that already carry a complete write deny for +// denySID are left untouched so setup reruns and command-time revalidation do +// not accumulate duplicate permanent ACEs. +func applyWindowsSharedDescendantDenies(root, denySID string, writeRoots []string) ([]windowsACLSnapshot, error) { + descendants, err := windowsEnumerateWritableDescendants(root, writeRoots) + if err != nil { + return nil, fmt.Errorf("enumerate writable descendants of %s: %w", root, err) + } + snapshots := make([]windowsACLSnapshot, 0, len(descendants)) + for _, dir := range descendants { + denied, err := windowsPathDeniesCapabilitySID(dir, denySID) + if err != nil { + return snapshots, fmt.Errorf("inspect existing deny on %s: %w", dir, err) + } + if denied { + continue + } + snapshot, applied, err := applyWindowsACLPathGroup(windowsACLPathGroup{ + Path: dir, + Entries: []WindowsACLEntry{{ + Action: WindowsACLDenyWrite, + Path: dir, + Capability: denySID, + NoInherit: true, + }}, + }) + if err != nil { + return snapshots, fmt.Errorf("deny writable descendant %s: %w", dir, err) + } + if applied { + snapshots = append(snapshots, snapshot) + } + } + return snapshots, nil +} + +// windowsEnumerateWritableDescendants returns the existing files and +// directories below root that grant BUILTIN\Users or Authenticated Users +// write, excluding any configured write root (and anything under it) so +// legitimate workspace writes are never jailed. Files are checked and denied +// just like directories — a writable file directly under a shared root is as +// much an escape surface as a writable directory — but only directories are +// descended into. +// +// Fail closed: exhausting the depth or entry caps, or failing to list/inspect +// a non-allowlisted, non-reparse entry returns an error rather than a partial +// success the caller could mistake for complete coverage. Reparse points are +// skipped (no-follow), not treated as incomplete coverage: their targets are +// reached through the real path when it lies under the same root. +func windowsEnumerateWritableDescendants(root string, writeRoots []string) ([]string, error) { + if windowsCapabilityPathKey(root) == "" { + return nil, nil + } + excluded := make([]string, 0, len(writeRoots)) + for _, writeRoot := range writeRoots { + if key := windowsCapabilityPathKey(writeRoot); key != "" { + excluded = append(excluded, key) + } + } + isExcluded := func(key string) bool { + for _, prefix := range excluded { + if key == prefix || strings.HasPrefix(key, prefix+`\`) { + return true + } + } + return false + } + + type node struct { + path string + depth int + } + var out []string + visited := 0 + // seenIDs records real directory object identities already entered so an + // alternate path to the same object is not re-enumerated (identity-aware + // half of the reparse policy). + seenIDs := make(map[windowsFileObjectID]struct{}) + queue := []node{{path: root, depth: 0}} + for len(queue) > 0 { + current := queue[0] + queue = queue[1:] + // Defensive: never list through a reparse path that somehow reached + // the queue (root is expected to be a real directory). + if windowsPathIsReparsePoint(current.path) { + continue + } + if id, ok := windowsFileObjectIdentity(current.path); ok { + if _, seen := seenIDs[id]; seen { + continue + } + seenIDs[id] = struct{}{} + } + entries, err := os.ReadDir(current.path) + if err != nil { + if windowsPathIsDriveRootPath(filepath.Dir(current.path)) && windowsDescendantScanNameIsSystemLocked(filepath.Base(current.path)) { + continue + } + return nil, fmt.Errorf("list descendants of %s: %w", current.path, err) + } + for _, entry := range entries { + child := filepath.Join(current.path, entry.Name()) + childKey := windowsCapabilityPathKey(child) + if isExcluded(childKey) { + continue + } + // No-follow: stock compatibility junctions (and any other reparse + // point) are never DACL-inspected, denied, or descended. Mode bits + // catch what ReadDir already classified; GetFileAttributes covers + // any reparse form those bits miss. + isReparse := (entry.Type()&os.ModeSymlink != 0) || (entry.Type()&os.ModeIrregular != 0) || windowsPathIsReparsePoint(child) + if isReparse { + if visited >= windowsDescendantScanMaxDirs { + return nil, fmt.Errorf("descendant scan exceeded %d entries below %s", windowsDescendantScanMaxDirs, root) + } + visited++ + continue + } + if visited >= windowsDescendantScanMaxDirs { + return nil, fmt.Errorf("descendant scan exceeded %d entries below %s", windowsDescendantScanMaxDirs, root) + } + visited++ + writable, err := windowsDirGrantsBroadenedWrite(child) + if err != nil { + // Same canonical-root-level scoping as the ReadDir case above: + // current.path (child's parent) must itself be a drive root for + // this to be the real, SYSTEM-exclusive directory. + if windowsPathIsDriveRootPath(current.path) && windowsDescendantScanNameIsSystemLocked(entry.Name()) { + continue + } + return nil, fmt.Errorf("inspect DACL of %s: %w", child, err) + } + if writable { + out = append(out, child) + } + if !entry.IsDir() { + continue + } + childDepth := current.depth + 1 + if childDepth >= windowsDescendantScanMaxDepth { + // A directory at the depth cap may still have unexamined + // children. Fail closed rather than pretend the subtree is clean. + // Leaf files at this depth were already inspected above. + // Only fail when we would have needed to descend further: always + // report the cap so callers cannot certify "complete". + return nil, fmt.Errorf("descendant scan exceeded depth %d at %s", windowsDescendantScanMaxDepth, child) + } + // Always descend (subject to caps), including through non-writable + // ancestors and stock system trees (Windows, Program Files, ...), so + // a deep writable child is not missed. A non-writable directory's OWN + // DACL says nothing about a descendant several levels down: an + // installer-created child with a loosened, non-inherited grant (e.g. + // C:\Users\shared) is exactly the escape this scan exists to find, and + // certifying a subtree clean from its root DACL alone would miss it + // (see jatmn's review). There is deliberately no basename-based + // shortcut here anymore — hitting windowsDescendantScanMaxDepth or + // windowsDescendantScanMaxDirs on a genuinely huge stock tree fails + // the scan closed (see the caller), which keeps the narrow SID set + // rather than certifying an unexamined subtree as safe. + queue = append(queue, node{path: child, depth: childDepth}) + } + } + return out, nil +} + +// windowsFileObjectID is the NTFS object identity used to detect that two +// paths name the same directory (volume serial + 64-bit file index). +type windowsFileObjectID struct { + volume uint32 + index uint64 +} + +// windowsFileObjectIdentity returns the on-disk identity of path when it can +// be opened as a real (non-reparse) directory. ok is false on any open/inspect +// failure so the walker falls through to path-based enumeration rather than +// treating an unreadable directory as already-seen. +func windowsFileObjectIdentity(path string) (windowsFileObjectID, bool) { + ptr, err := windows.UTF16PtrFromString(path) + if err != nil { + return windowsFileObjectID{}, false + } + handle, err := windows.CreateFile( + ptr, + windows.FILE_READ_ATTRIBUTES, + windows.FILE_SHARE_READ|windows.FILE_SHARE_WRITE|windows.FILE_SHARE_DELETE, + nil, + windows.OPEN_EXISTING, + windows.FILE_FLAG_BACKUP_SEMANTICS|windows.FILE_FLAG_OPEN_REPARSE_POINT, + 0, + ) + if err != nil { + return windowsFileObjectID{}, false + } + defer windows.CloseHandle(handle) + var info windows.ByHandleFileInformation + if err := windows.GetFileInformationByHandle(handle, &info); err != nil { + return windowsFileObjectID{}, false + } + if info.FileAttributes&windows.FILE_ATTRIBUTE_REPARSE_POINT != 0 { + return windowsFileObjectID{}, false + } + if info.FileAttributes&windows.FILE_ATTRIBUTE_DIRECTORY == 0 { + return windowsFileObjectID{}, false + } + return windowsFileObjectID{ + volume: info.VolumeSerialNumber, + index: (uint64(info.FileIndexHigh) << 32) | uint64(info.FileIndexLow), + }, true +} + +// windowsAccessAllowedObjectAceType and windowsAccessDeniedObjectAceType are +// the AceType values for ACCESS_ALLOWED_OBJECT_ACE / ACCESS_DENIED_OBJECT_ACE +// (https://learn.microsoft.com/en-us/windows/win32/api/winnt/ns-winnt-access_allowed_object_ace). +// windowsAccessAllowedCallbackAceType and windowsAccessAllowedCallbackObjectAceType +// are ACCESS_ALLOWED_CALLBACK_ACE_TYPE and ACCESS_ALLOWED_CALLBACK_OBJECT_ACE_TYPE +// (MS-DTYP 2.4.4.6 / conditional-ACE object variant): a callback ACE carries a +// conditional expression (e.g. "resource attribute matches") that gates +// whether the grant applies, appended AFTER the SID, so it does not move the +// SID's own offset relative to its non-callback sibling. x/sys/windows only +// models the plain ACCESS_ALLOWED_ACE layout (Header, Mask, SidStart) and +// exposes just ACCESS_ALLOWED_ACE_TYPE/ACCESS_DENIED_ACE_TYPE, so all four are +// declared locally. +const ( + windowsAccessAllowedObjectAceType = 0x05 + windowsAccessDeniedObjectAceType = 0x06 + windowsAccessAllowedCallbackAceType = 0x09 + windowsAccessAllowedCallbackObjectAceType = 0x0B +) + +// windowsAceSID locates the trustee SID within ace, an *ACCESS_ALLOWED_ACE +// pointer that GetAce hands back regardless of the ACE's true type — for +// object ACEs that pointer is only valid for reading Header/Mask, not +// SidStart. An object ACE (ACCESS_ALLOWED_OBJECT_ACE / ACCESS_DENIED_OBJECT_ACE) +// inserts a Flags DWORD and up to two conditionally-present 16-byte GUIDs +// (ObjectType, InheritedObjectType) between Mask and the real SID; naively +// reading &ace.SidStart for one of these — as if it had the plain ACE layout — +// reinterprets Flags/GUID bytes as SID bytes and silently computes the wrong +// trustee, both risking a false match and missing a real Users/Authenticated +// Users grant hidden inside an object ACE. +// +// ACCESS_ALLOWED_CALLBACK_ACE_TYPE and ACCESS_ALLOWED_CALLBACK_OBJECT_ACE_TYPE +// are recognized the same way as their non-callback counterparts: per MS-DTYP, +// a callback ACE's conditional expression ("ApplicationData") is appended +// AFTER the SID, not inserted before it, so the SID offset is identical. Only +// the ALLOW callback variants are recognized here, deliberately — see +// windowsDirGrantsBroadenedWrite for why a callback DENY is never trusted to +// suppress a grant. ok is false for any other ACE type (audit, alarm, +// mandatory label, compound, callback deny, ...), which either does not +// represent a trustee write grant in the sense this scan cares about, or (for +// callback deny) is not safe to rely on, and is skipped exactly as it always +// has been. +func windowsAceSID(ace *windows.ACCESS_ALLOWED_ACE) (sid *windows.SID, ok bool) { + switch ace.Header.AceType { + case windows.ACCESS_ALLOWED_ACE_TYPE, windows.ACCESS_DENIED_ACE_TYPE, windowsAccessAllowedCallbackAceType: + return (*windows.SID)(unsafe.Pointer(&ace.SidStart)), true + case windowsAccessAllowedObjectAceType, windowsAccessDeniedObjectAceType, windowsAccessAllowedCallbackObjectAceType: + // For an object ACE, the memory the Go struct calls SidStart is + // actually the ACE's Flags DWORD; the real SID sits further out, + // pushed by whichever of the two optional GUIDs Flags says are present. + // offset is plain arithmetic on a byte count, never itself derived from + // a pointer conversion, so accumulating it across statements is safe; + // only the final pointer+offset conversion below needs to happen in a + // single expression (go vet's unsafeptr rule). + flags := ace.SidStart + offset := unsafe.Sizeof(ace.SidStart) + if flags&windows.ACE_OBJECT_TYPE_PRESENT != 0 { + offset += 16 + } + if flags&windows.ACE_INHERITED_OBJECT_TYPE_PRESENT != 0 { + offset += 16 + } + return (*windows.SID)(unsafe.Pointer(uintptr(unsafe.Pointer(&ace.SidStart)) + offset)), true + default: + return nil, false + } +} + +// windowsDirGrantsBroadenedWrite reports whether path's effective DACL lets +// BUILTIN\Users or Authenticated Users write. It walks the DACL (which, as +// returned by GetNamedSecurityInfo, already contains inherited ACEs) in order, +// honoring a deny ACE that precedes an allow for the same bits, the canonical +// evaluation. A NULL DACL grants everyone full access and is treated as +// writable. +// +// Note: this is a deliberate DACL walk rather than AccessCheck. It must detect +// grants that would become usable once the restricted token is broadened with +// those groups, independent of the setup process's own token. INHERIT_ONLY ACEs +// are skipped because they do not apply to the object itself. +// +// A callback allow ACE (ACCESS_ALLOWED_CALLBACK_ACE / _OBJECT_ACE) is treated +// exactly like an unconditional allow: this static walk cannot evaluate the +// ACE's conditional expression against the sandbox token, so the only safe +// assumption is the worst case, that the condition holds and the grant +// applies (see jatmn's review). The symmetric callback DENY types are +// deliberately NOT recognized by windowsAceSID at all, so they never reach +// this switch: trusting an unproven condition to suppress deniedWrite would +// risk the opposite mistake, misclassifying a writable directory as safe. +func windowsDirGrantsBroadenedWrite(path string) (bool, error) { + // GetNamedSecurityInfo returns a self-relative descriptor copied onto the Go + // heap (it LocalFrees the Win32 allocation itself), so it must NOT be + // LocalFree'd here: doing so frees Go-managed memory and corrupts the heap. + sd, err := windows.GetNamedSecurityInfo(path, windows.SE_FILE_OBJECT, windows.DACL_SECURITY_INFORMATION) + if err != nil { + return false, err + } + dacl, _, err := sd.DACL() + if err != nil { + return false, err + } + if dacl == nil { + return true, nil + } + var deniedWrite windows.ACCESS_MASK + for index := uint16(0); index < dacl.AceCount; index++ { + var ace *windows.ACCESS_ALLOWED_ACE + if err := windows.GetAce(dacl, uint32(index), &ace); err != nil { + return false, fmt.Errorf("read ACE %d of %s: %w", index, path, err) + } + // An INHERIT_ONLY ACE does not apply to this object itself — it only + // seeds ACLs of newly created children. Counting one here could let + // an inherit-only deny suppress a later applicable allow in + // deniedWrite, misclassifying a writable directory as safe. + if ace.Header.AceFlags&windows.INHERIT_ONLY_ACE != 0 { + continue + } + sid, ok := windowsAceSID(ace) + if !ok { + continue + } + if !sid.IsWellKnown(windows.WinBuiltinUsersSid) && !sid.IsWellKnown(windows.WinAuthenticatedUserSid) { + continue + } + writeBits := ace.Mask & windowsBroadenedWriteProbeMask + if writeBits == 0 { + continue + } + switch ace.Header.AceType { + case windows.ACCESS_DENIED_ACE_TYPE, windowsAccessDeniedObjectAceType: + deniedWrite |= writeBits + case windows.ACCESS_ALLOWED_ACE_TYPE, windowsAccessAllowedObjectAceType, + windowsAccessAllowedCallbackAceType, windowsAccessAllowedCallbackObjectAceType: + if writeBits&^deniedWrite != 0 { + return true, nil + } + } + } + return false, nil +} + +// windowsPathIsReparsePoint reports whether path carries the reparse-point +// attribute (a junction, symlink, or mount point). Any error resolving the +// attributes is reported as "not a reparse point" so the caller falls through +// to its own DACL read, which surfaces a real access problem there instead. +func windowsPathIsReparsePoint(path string) bool { + ptr, err := windows.UTF16PtrFromString(path) + if err != nil { + return false + } + attrs, err := windows.GetFileAttributes(ptr) + if err != nil { + return false + } + return attrs&windows.FILE_ATTRIBUTE_REPARSE_POINT != 0 +} + +// windowsUncoveredWritableDescendants returns Users/AuthUsers-writable +// descendants of root that do not yet carry a DenyWrite ACE for denySID. +func windowsUncoveredWritableDescendants(root, denySID string, writeRoots []string) ([]string, error) { + descendants, err := windowsEnumerateWritableDescendants(root, writeRoots) + if err != nil { + return nil, fmt.Errorf("enumerate writable descendants of %s: %w", root, err) + } + var holes []string + for _, dir := range descendants { + denied, err := windowsPathDeniesCapabilitySID(dir, denySID) + if err != nil { + return nil, fmt.Errorf("inspect existing deny on %s: %w", dir, err) + } + if !denied { + holes = append(holes, dir) + } + } + return holes, nil +} + +// windowsPathDeniesCapabilitySID reports whether path's DACL already contains +// deny ACE(s) naming the given capability SID string (the synthetic identity +// used for shared-root / descendant DenyWrite entries) that together cover +// every write-relevant bit in windowsBroadenedWriteProbeMask. +// +// A partial deny is not coverage: denying only FILE_WRITE_ATTRIBUTES (or only +// read/execute via a DenyRead ACE that reuses the same stable SID) leaves +// FILE_WRITE_DATA / FILE_APPEND_DATA open for a Users/AuthUsers grant, so the +// apply and verification paths must still merge the full canonical DenyWrite +// rather than skipping the path — see jatmn's review. Accumulated deny ACEs +// for wantSID are OR'd before the completeness check so a multi-ACE full +// cover still counts. +func windowsPathDeniesCapabilitySID(path, wantSID string) (bool, error) { + want, err := windows.StringToSid(wantSID) + if err != nil { + return false, fmt.Errorf("parse capability SID %q: %w", wantSID, err) + } + sd, err := windows.GetNamedSecurityInfo(path, windows.SE_FILE_OBJECT, windows.DACL_SECURITY_INFORMATION) + if err != nil { + return false, err + } + dacl, _, err := sd.DACL() + if err != nil { + return false, err + } + if dacl == nil { + return false, nil + } + var deniedMask windows.ACCESS_MASK + for index := uint16(0); index < dacl.AceCount; index++ { + var ace *windows.ACCESS_ALLOWED_ACE + if err := windows.GetAce(dacl, uint32(index), &ace); err != nil { + return false, fmt.Errorf("read ACE %d of %s: %w", index, path, err) + } + if ace.Header.AceType != windows.ACCESS_DENIED_ACE_TYPE && ace.Header.AceType != windowsAccessDeniedObjectAceType { + continue + } + // An INHERIT_ONLY ACE does not apply to this object itself (see the + // same skip in windowsDirGrantsBroadenedWrite). Counting one here + // would report an inherited-but-inapplicable deny as "already + // denied," causing applyWindowsSharedDescendantDenies to skip + // applying the real, effective deny and leave the descendant open. + if ace.Header.AceFlags&windows.INHERIT_ONLY_ACE != 0 { + continue + } + sid, ok := windowsAceSID(ace) + if !ok || !sid.Equals(want) { + continue + } + deniedMask |= ace.Mask + } + return (deniedMask & windowsBroadenedWriteProbeMask) == windowsBroadenedWriteProbeMask, nil +} diff --git a/internal/sandbox/windows_acl_descendants_windows_test.go b/internal/sandbox/windows_acl_descendants_windows_test.go new file mode 100644 index 000000000..b4f57d3e1 --- /dev/null +++ b/internal/sandbox/windows_acl_descendants_windows_test.go @@ -0,0 +1,773 @@ +//go:build windows + +package sandbox + +import ( + "encoding/binary" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + "unsafe" + + "golang.org/x/sys/windows" +) + +// touchFile creates an empty file at path, failing the test on error. +func touchFile(t *testing.T, path string) string { + t.Helper() + if err := os.WriteFile(path, nil, 0o644); err != nil { + t.Fatalf("WriteFile(%q): %v", path, err) + } + return path +} + +// dirDeniesSID reports whether path's DACL carries a deny ACE naming the given +// string SID. It reads the same way the descendant scan applies denies, so a +// test can confirm the compensating deny actually landed (and, after rollback, +// is gone) using the real Win32 ACL APIs on a test-owned temp tree. +func dirDeniesSID(t *testing.T, path, wantSID string) bool { + t.Helper() + sd, err := windows.GetNamedSecurityInfo(path, windows.SE_FILE_OBJECT, windows.DACL_SECURITY_INFORMATION) + if err != nil { + t.Fatalf("GetNamedSecurityInfo %s: %v", path, err) + } + dacl, _, err := sd.DACL() + if err != nil { + t.Fatalf("DACL %s: %v", path, err) + } + if dacl == nil { + return false + } + for index := uint16(0); index < dacl.AceCount; index++ { + var ace *windows.ACCESS_ALLOWED_ACE + if err := windows.GetAce(dacl, uint32(index), &ace); err != nil { + t.Fatalf("GetAce %d of %s: %v", index, path, err) + } + if ace.Header.AceType != windows.ACCESS_DENIED_ACE_TYPE && ace.Header.AceType != windowsAccessDeniedObjectAceType { + continue + } + sid, ok := windowsAceSID(ace) + if !ok || sid.String() != wantSID { + continue + } + return true + } + return false +} + +// grantUsersWrite adds a direct (non-inheriting) allow-write ACE for +// BUILTIN\Users to path's DACL using the real Win32 ACL APIs. The test process +// owns the t.TempDir() tree, so this needs no elevation. +func grantUsersWrite(t *testing.T, path string) { + t.Helper() + usersSID, err := windows.CreateWellKnownSid(windows.WinBuiltinUsersSid) + if err != nil { + t.Fatalf("CreateWellKnownSid(Users): %v", err) + } + sd, err := windows.GetNamedSecurityInfo(path, windows.SE_FILE_OBJECT, windows.DACL_SECURITY_INFORMATION) + if err != nil { + t.Fatalf("GetNamedSecurityInfo %s: %v", path, err) + } + oldDACL, _, err := sd.DACL() + if err != nil { + t.Fatalf("DACL %s: %v", path, err) + } + newDACL, err := windows.ACLFromEntries([]windows.EXPLICIT_ACCESS{{ + AccessPermissions: windows.FILE_GENERIC_WRITE, + AccessMode: windows.GRANT_ACCESS, + Inheritance: windows.NO_INHERITANCE, + Trustee: windows.TRUSTEE{ + TrusteeForm: windows.TRUSTEE_IS_SID, + TrusteeType: windows.TRUSTEE_IS_GROUP, + TrusteeValue: windows.TrusteeValueFromSID(usersSID), + }, + }}, oldDACL) + if err != nil { + t.Fatalf("ACLFromEntries %s: %v", path, err) + } + if err := windows.SetNamedSecurityInfo(path, windows.SE_FILE_OBJECT, windows.DACL_SECURITY_INFORMATION, nil, nil, newDACL, nil); err != nil { + t.Fatalf("SetNamedSecurityInfo %s: %v", path, err) + } +} + +// denyUsersWrite adds a direct (non-inheriting) deny-write ACE for +// BUILTIN\Users to path's DACL, overriding any inherited write grant from the +// enclosing t.TempDir() tree. Used to construct a directory that is +// genuinely non-writable at its own DACL, independent of whatever the test +// temp tree happens to inherit. +// +// Denies the concrete (non-generic) bits of windowsBroadenedWriteProbeMask — +// the same bits windowsDirGrantsBroadenedWrite itself checks, minus +// GENERIC_WRITE/GENERIC_ALL. Two things must NOT be in this mask: +// - SYNCHRONIZE (part of windows.FILE_GENERIC_WRITE): the test process is +// normally a member of BUILTIN\Users, and denying SYNCHRONIZE also blocks +// its own later synchronous opens of path (e.g. os.ReadDir), not just +// "write" — verified directly against this code path. +// - Raw GENERIC_WRITE/GENERIC_ALL bits: stored unmapped in an ACE (as +// opposed to being resolved to their constituent FILE_* bits first), +// these were empirically observed to make Windows deny EVERY access, +// including a plain FILE_LIST_DIRECTORY open, not just generic write. +func denyUsersWrite(t *testing.T, path string) { + t.Helper() + usersSID, err := windows.CreateWellKnownSid(windows.WinBuiltinUsersSid) + if err != nil { + t.Fatalf("CreateWellKnownSid(Users): %v", err) + } + sd, err := windows.GetNamedSecurityInfo(path, windows.SE_FILE_OBJECT, windows.DACL_SECURITY_INFORMATION) + if err != nil { + t.Fatalf("GetNamedSecurityInfo %s: %v", path, err) + } + oldDACL, _, err := sd.DACL() + if err != nil { + t.Fatalf("DACL %s: %v", path, err) + } + denyMask := windowsBroadenedWriteProbeMask &^ (windows.GENERIC_WRITE | windows.GENERIC_ALL | windows.SYNCHRONIZE) + newDACL, err := windows.ACLFromEntries([]windows.EXPLICIT_ACCESS{{ + AccessPermissions: denyMask, + AccessMode: windows.DENY_ACCESS, + Inheritance: windows.NO_INHERITANCE, + Trustee: windows.TRUSTEE{ + TrusteeForm: windows.TRUSTEE_IS_SID, + TrusteeType: windows.TRUSTEE_IS_GROUP, + TrusteeValue: windows.TrusteeValueFromSID(usersSID), + }, + }}, oldDACL) + if err != nil { + t.Fatalf("ACLFromEntries %s: %v", path, err) + } + if err := windows.SetNamedSecurityInfo(path, windows.SE_FILE_OBJECT, windows.DACL_SECURITY_INFORMATION, nil, nil, newDACL, nil); err != nil { + t.Fatalf("SetNamedSecurityInfo %s: %v", path, err) + } +} + +// TestWindowsEnumerateWritableDescendantsDoesNotPruneSystemLookalikeTrees is +// the real-Windows regression for jatmn's review finding: a directory whose +// basename matched the old prune list (e.g. "Program Files") and was +// non-writable at its OWN DACL must still be descended into, because a +// non-writable root DACL says nothing about a writable descendant several +// levels down (e.g. an installer-created "Program Files\SomeApp" with a +// loosened grant). Before the fix, this subtree was silently certified clean +// from the root DACL alone and never scanned further. +func TestWindowsEnumerateWritableDescendantsDoesNotPruneSystemLookalikeTrees(t *testing.T) { + root := t.TempDir() + programFiles := mkdir(t, filepath.Join(root, "Program Files")) + // Create and grant the nested descendant BEFORE denying write on + // programFiles itself: the deny targets BUILTIN\Users, which the test + // process is normally a member of, so denying it on programFiles first + // would block the test process from creating anything under it. + someApp := mkdir(t, filepath.Join(programFiles, "SomeApp")) + grantUsersWrite(t, someApp) + denyUsersWrite(t, programFiles) + + rootWritable, err := windowsDirGrantsBroadenedWrite(programFiles) + if err != nil { + t.Fatalf("windowsDirGrantsBroadenedWrite(programFiles): %v", err) + } + if rootWritable { + t.Fatal("test fixture bug: programFiles must be non-writable at its own DACL to exercise the old prune condition") + } + + found, err := windowsEnumerateWritableDescendants(root, nil) + if err != nil { + t.Fatalf("windowsEnumerateWritableDescendants: %v", err) + } + if !windowsPathListContains(found, someApp) { + t.Fatalf("enumeration = %#v, want it to include writable descendant %q under non-writable %q (must not be pruned by basename)", found, someApp, programFiles) + } +} + +// TestWindowsDirGrantsBroadenedWriteDetectsUsersWrite pins the DACL probe the +// descendant scan relies on: a directory whose DACL grants BUILTIN\Users write +// is reported writable; one that does not is reported not writable. +func TestWindowsDirGrantsBroadenedWriteDetectsUsersWrite(t *testing.T) { + root := t.TempDir() + writable := mkdir(t, filepath.Join(root, "writable")) + grantUsersWrite(t, writable) + plain := mkdir(t, filepath.Join(root, "plain")) + + got, err := windowsDirGrantsBroadenedWrite(writable) + if err != nil { + t.Fatalf("windowsDirGrantsBroadenedWrite(writable): %v", err) + } + if !got { + t.Fatalf("windowsDirGrantsBroadenedWrite(writable) = false, want true") + } + + plainWritable, err := windowsDirGrantsBroadenedWrite(plain) + if err != nil { + t.Fatalf("windowsDirGrantsBroadenedWrite(plain): %v", err) + } + if plainWritable { + t.Skip("test temp tree grants BUILTIN\\Users write by inheritance; cannot exercise the negative case here") + } +} + +// TestWindowsEnumerateWritableDescendantsFindsExistingWritableChildren is the +// real-Windows regression for the write-jail gap the reviewer flagged: an +// existing writable descendant of a shared root (including one nested under +// another writable directory) must be discovered so it can be denied directly, +// and a configured write root must be excluded so legitimate workspace writes +// are never jailed. +func TestWindowsEnumerateWritableDescendantsFindsExistingWritableChildren(t *testing.T) { + root := t.TempDir() + outer := mkdir(t, filepath.Join(root, "outer")) + grantUsersWrite(t, outer) + inner := mkdir(t, filepath.Join(outer, "inner")) + grantUsersWrite(t, inner) + // Depth-3 writable child under non-writable ancestors: the scan must keep + // descending through non-writable parents or this escape stays open. + level1 := mkdir(t, filepath.Join(root, "locked1")) + level2 := mkdir(t, filepath.Join(level1, "locked2")) + deepWritable := mkdir(t, filepath.Join(level2, "deep-writable")) + grantUsersWrite(t, deepWritable) + plain := mkdir(t, filepath.Join(root, "plain")) + workspace := mkdir(t, filepath.Join(root, "workspace")) + grantUsersWrite(t, workspace) + writableFile := touchFile(t, filepath.Join(root, "writable.txt")) + grantUsersWrite(t, writableFile) + + found, err := windowsEnumerateWritableDescendants(root, nil) + if err != nil { + t.Fatalf("windowsEnumerateWritableDescendants: %v", err) + } + if !windowsPathListContains(found, outer) { + t.Fatalf("enumeration = %#v, want it to include writable child %q", found, outer) + } + if !windowsPathListContains(found, inner) { + t.Fatalf("enumeration = %#v, want it to include nested writable descendant %q", found, inner) + } + if !windowsPathListContains(found, deepWritable) { + t.Fatalf("enumeration = %#v, want it to include depth-3 writable child %q under non-writable ancestors", found, deepWritable) + } + if !windowsPathListContains(found, writableFile) { + t.Fatalf("enumeration = %#v, want it to include writable file %q (a file is as much an escape surface as a directory)", found, writableFile) + } + + plainWritable, err := windowsDirGrantsBroadenedWrite(plain) + if err != nil { + t.Fatalf("windowsDirGrantsBroadenedWrite(plain): %v", err) + } + if !plainWritable && windowsPathListContains(found, plain) { + t.Fatalf("enumeration = %#v, want it to exclude non-writable child %q", found, plain) + } + + // Excluding the workspace write root (and its subtree) must drop it from the + // result even though it grants Users write. + excluded, err := windowsEnumerateWritableDescendants(root, []string{workspace}) + if err != nil { + t.Fatalf("windowsEnumerateWritableDescendants(exclude): %v", err) + } + if windowsPathListContains(excluded, workspace) { + t.Fatalf("enumeration = %#v, want it to exclude the configured write root %q", excluded, workspace) + } + if !windowsPathListContains(excluded, outer) { + t.Fatalf("enumeration = %#v, want it to still include %q when a different path is excluded", excluded, outer) + } +} + +// selfUserSID returns the current process token's own user SID, used by the +// canonical-root-scoping test below to deny itself directory-listing access +// (an owner always retains READ_CONTROL/WRITE_DAC implicitly, so this cannot +// lock the test out of restoring its own change). +func selfUserSID(t *testing.T) *windows.SID { + t.Helper() + user, err := windows.GetCurrentProcessToken().GetTokenUser() + if err != nil { + t.Fatalf("GetTokenUser: %v", err) + } + return user.User.Sid +} + +// setSelfListDirectoryAccess grants or denies the current user FILE_LIST_DIRECTORY +// on path, restoring/breaking the ability to os.ReadDir it without touching +// READ_CONTROL/WRITE_DAC (which owners always retain), so the test can always +// undo its own change. +// +// deny=false restores access via SET_ACCESS with a zero mask rather than +// REVOKE_ACCESS: empirically, SetEntriesInAclW's REVOKE_ACCESS mode does not +// remove a pre-existing DENY ACE for the trustee (verified directly against +// this code path — see the same finding in BuildWindowsACLPlan's +// WindowsACLRevokeCapability, windows_acl_apply_windows.go), so relying on it +// here would leave the test process permanently denied FILE_LIST_DIRECTORY on +// its own temp fixture. +func setSelfListDirectoryAccess(t *testing.T, path string, deny bool) { + t.Helper() + mode := windows.ACCESS_MODE(windows.SET_ACCESS) + permissions := windows.ACCESS_MASK(0) + if deny { + mode = windows.DENY_ACCESS + permissions = windows.FILE_LIST_DIRECTORY + } + sd, err := windows.GetNamedSecurityInfo(path, windows.SE_FILE_OBJECT, windows.DACL_SECURITY_INFORMATION) + if err != nil { + t.Fatalf("GetNamedSecurityInfo %s: %v", path, err) + } + oldDACL, _, err := sd.DACL() + if err != nil { + t.Fatalf("DACL %s: %v", path, err) + } + targetOldDACL := oldDACL + if deny { + targetOldDACL = nil + } + newDACL, err := windows.ACLFromEntries([]windows.EXPLICIT_ACCESS{{ + AccessPermissions: permissions, + AccessMode: mode, + Inheritance: windows.NO_INHERITANCE, + Trustee: windows.TRUSTEE{ + TrusteeForm: windows.TRUSTEE_IS_SID, + TrusteeType: windows.TRUSTEE_IS_USER, + TrusteeValue: windows.TrusteeValueFromSID(selfUserSID(t)), + }, + }}, targetOldDACL) + if err != nil { + t.Fatalf("ACLFromEntries %s: %v", path, err) + } + if err := windows.SetNamedSecurityInfo(path, windows.SE_FILE_OBJECT, windows.DACL_SECURITY_INFORMATION, nil, nil, newDACL, nil); err != nil { + t.Fatalf("SetNamedSecurityInfo %s: %v", path, err) + } +} + +// TestWindowsEnumerateWritableDescendantsFailsClosedOnNonRootLookalikeSystemDir +// is the real-Windows regression for jatmn's review finding: a directory that +// shares a basename with a known SYSTEM-exclusive Windows directory (here +// "Recovery") but sits somewhere other than a genuine drive letter root must +// fail the scan closed when it cannot be listed, not be silently skipped as +// if it were the real, stock volume-root object. +func TestWindowsEnumerateWritableDescendantsFailsClosedOnNonRootLookalikeSystemDir(t *testing.T) { + root := t.TempDir() + outer := mkdir(t, filepath.Join(root, "outer")) + lookalike := mkdir(t, filepath.Join(outer, "Recovery")) + + setSelfListDirectoryAccess(t, lookalike, true) + t.Cleanup(func() { setSelfListDirectoryAccess(t, lookalike, false) }) + + if _, err := os.ReadDir(lookalike); err == nil { + t.Skip("could not deny self directory-listing access on this host; skipping fail-closed assertion") + } + + _, err := windowsEnumerateWritableDescendants(root, nil) + if err == nil { + t.Fatal("windowsEnumerateWritableDescendants: expected a fail-closed error for an unlistable non-root-level lookalike system directory, got nil") + } +} + +// TestWindowsEnumerateWritableDescendantsFailsClosedOnEntryCap pins that +// exhausting the descendant entry budget is an error, not a silent partial +// success that would still let setup broaden the restricted token. +func TestWindowsEnumerateWritableDescendantsFailsClosedOnEntryCap(t *testing.T) { + prev := windowsDescendantScanMaxDirs + windowsDescendantScanMaxDirs = 3 + t.Cleanup(func() { windowsDescendantScanMaxDirs = prev }) + + root := t.TempDir() + for _, name := range []string{"a", "b", "c", "d"} { + mkdir(t, filepath.Join(root, name)) + } + _, err := windowsEnumerateWritableDescendants(root, nil) + if err == nil { + t.Fatal("windowsEnumerateWritableDescendants: expected entry-cap error, got nil") + } +} + +// TestWindowsPathDeniesCapabilitySIDRoundTrip ensures the pre-broaden hole +// check can see a deny ACE that applyWindowsSharedDescendantDenies just wrote. +func TestWindowsPathDeniesCapabilitySIDRoundTrip(t *testing.T) { + caps, err := LoadOrCreateWindowsCapabilitySIDs(t.TempDir()) + if err != nil { + t.Fatalf("LoadOrCreateWindowsCapabilitySIDs: %v", err) + } + root := t.TempDir() + writable := mkdir(t, filepath.Join(root, "writable")) + grantUsersWrite(t, writable) + + before, err := windowsPathDeniesCapabilitySID(writable, caps.ReadOnly) + if err != nil { + t.Fatalf("windowsPathDeniesCapabilitySID before: %v", err) + } + if before { + t.Fatal("path already denies capability SID before apply") + } + if _, err := applyWindowsSharedDescendantDenies(root, caps.ReadOnly, nil); err != nil { + t.Fatalf("applyWindowsSharedDescendantDenies: %v", err) + } + after, err := windowsPathDeniesCapabilitySID(writable, caps.ReadOnly) + if err != nil { + t.Fatalf("windowsPathDeniesCapabilitySID after: %v", err) + } + if !after { + t.Fatal("path does not deny capability SID after apply") + } + holes, err := windowsUncoveredWritableDescendants(root, caps.ReadOnly, nil) + if err != nil { + t.Fatalf("windowsUncoveredWritableDescendants: %v", err) + } + if len(holes) != 0 { + t.Fatalf("holes = %#v, want none after apply", holes) + } +} + +// TestWindowsPathDeniesCapabilitySIDIgnoresReadOnlyDeny is the real-Windows +// regression for jatmn's review finding: a pre-existing deny ACE for the +// stable capability SID that only denies read/execute (the exact shape +// planWindowsDenyReadPaths applies for a DenyRead path) must NOT be read as +// "write already denied." Before the fix, any deny ACE naming the SID short- +// circuited the check regardless of its mask, so a writable descendant that +// happened to sit under a DenyRead path would be skipped by +// applyWindowsSharedDescendantDenies and never get the write deny it needs. +func TestWindowsPathDeniesCapabilitySIDIgnoresReadOnlyDeny(t *testing.T) { + caps, err := LoadOrCreateWindowsCapabilitySIDs(t.TempDir()) + if err != nil { + t.Fatalf("LoadOrCreateWindowsCapabilitySIDs: %v", err) + } + root := t.TempDir() + writable := mkdir(t, filepath.Join(root, "writable")) + grantUsersWrite(t, writable) + + // Apply the same DenyRead entry BuildWindowsACLPlan would generate for a + // DenyRead path sharing this stable capability SID. + if _, _, err := applyWindowsACLPathGroup(windowsACLPathGroup{ + Path: writable, + Entries: []WindowsACLEntry{{ + Action: WindowsACLDenyRead, + Path: writable, + Capability: caps.ReadOnly, + }}, + }); err != nil { + t.Fatalf("apply DenyRead entry: %v", err) + } + + deniesWrite, err := windowsPathDeniesCapabilitySID(writable, caps.ReadOnly) + if err != nil { + t.Fatalf("windowsPathDeniesCapabilitySID: %v", err) + } + if deniesWrite { + t.Fatal("windowsPathDeniesCapabilitySID = true for a read-only deny ACE, want false: a DenyRead ACE does not block writes") + } + + // The descendant-denial pass must therefore still add the real write deny + // rather than skipping this path as already covered. + if _, err := applyWindowsSharedDescendantDenies(root, caps.ReadOnly, nil); err != nil { + t.Fatalf("applyWindowsSharedDescendantDenies: %v", err) + } + deniesWrite, err = windowsPathDeniesCapabilitySID(writable, caps.ReadOnly) + if err != nil { + t.Fatalf("windowsPathDeniesCapabilitySID after apply: %v", err) + } + if !deniesWrite { + t.Fatal("windowsPathDeniesCapabilitySID = false after applyWindowsSharedDescendantDenies, want true: the write deny should now be present") + } +} + +// TestApplyWindowsSharedDescendantDeniesAppliesAndRollsBack proves the +// enforcement half of the fix end to end on real ACLs: a writable descendant of +// a shared root gets a direct deny ACE for the read-only capability SID (the SID +// every broadened token carries), and the returned rollback restores the DACL. +// This runs unprivileged because it operates only on the test-owned temp tree. +func TestApplyWindowsSharedDescendantDeniesAppliesAndRollsBack(t *testing.T) { + caps, err := LoadOrCreateWindowsCapabilitySIDs(t.TempDir()) + if err != nil { + t.Fatalf("LoadOrCreateWindowsCapabilitySIDs: %v", err) + } + root := t.TempDir() + writable := mkdir(t, filepath.Join(root, "writable")) + grantUsersWrite(t, writable) + + if dirDeniesSID(t, writable, caps.ReadOnly) { + t.Fatalf("descendant already denies %q before apply", caps.ReadOnly) + } + snapshots, err := applyWindowsSharedDescendantDenies(root, caps.ReadOnly, nil) + if err != nil { + t.Fatalf("applyWindowsSharedDescendantDenies: %v", err) + } + if len(snapshots) == 0 { + t.Fatalf("apply returned no snapshots; the writable descendant was never denied") + } + if !dirDeniesSID(t, writable, caps.ReadOnly) { + t.Fatalf("descendant %q does not deny %q after apply", writable, caps.ReadOnly) + } + + if err := rollbackWindowsACLSnapshots(snapshots); err != nil { + t.Fatalf("rollbackWindowsACLSnapshots: %v", err) + } + if dirDeniesSID(t, writable, caps.ReadOnly) { + t.Fatalf("descendant %q still denies %q after rollback", writable, caps.ReadOnly) + } + // Checking only that the deny disappeared would also pass a rollback that + // clobbered the original Users write ACE along with it; reassert the + // pre-existing writable state is actually restored, not just any DACL. + 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) + } +} + +// TestWindowsAceSIDLocatesSIDInObjectACE pins the offset arithmetic +// windowsAceSID relies on for ACCESS_ALLOWED_OBJECT_ACE / ACCESS_DENIED_OBJECT_ACE: +// the real SID sits past a Flags DWORD and 0, 1, or 2 conditionally-present +// 16-byte GUIDs (ObjectType, InheritedObjectType), never at the plain-ACE +// SidStart offset GetAce's *ACCESS_ALLOWED_ACE typing would naively suggest. +// This builds the raw ACE bytes directly, per Microsoft's documented layout, +// because x/sys/windows has no AddAccessAllowedObjectAce binding to create a +// real one through the OS. +func TestWindowsAceSIDLocatesSIDInObjectACE(t *testing.T) { + usersSID, err := windows.CreateWellKnownSid(windows.WinBuiltinUsersSid) + if err != nil { + t.Fatalf("CreateWellKnownSid: %v", err) + } + sidBytes := unsafe.Slice((*byte)(unsafe.Pointer(usersSID)), usersSID.Len()) + + cases := []struct { + name string + aceType byte + flags uint32 + guids int // number of 16-byte GUIDs the flags say precede the SID + }{ + {"no optional GUIDs", windowsAccessAllowedObjectAceType, 0, 0}, + {"object type GUID only", windowsAccessAllowedObjectAceType, windows.ACE_OBJECT_TYPE_PRESENT, 1}, + {"inherited type GUID only", windowsAccessDeniedObjectAceType, windows.ACE_INHERITED_OBJECT_TYPE_PRESENT, 1}, + {"both GUIDs", windowsAccessDeniedObjectAceType, windows.ACE_OBJECT_TYPE_PRESENT | windows.ACE_INHERITED_OBJECT_TYPE_PRESENT, 2}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + // Layout: ACE_HEADER(4) + Mask(4) + Flags(4) + guids*GUID(16) + SID. + buf := make([]byte, 4+4+4+16*tc.guids+len(sidBytes)) + buf[0] = tc.aceType // Header.AceType + binary.LittleEndian.PutUint32(buf[8:12], tc.flags) // Flags, at the offset SidStart occupies in the plain-ACE layout + copy(buf[12+16*tc.guids:], sidBytes) + + ace := (*windows.ACCESS_ALLOWED_ACE)(unsafe.Pointer(&buf[0])) + sid, ok := windowsAceSID(ace) + if !ok { + t.Fatal("windowsAceSID returned ok=false for a recognized object ACE type") + } + if !sid.Equals(usersSID) { + t.Fatalf("windowsAceSID = %s, want %s", sid.String(), usersSID.String()) + } + }) + } +} + +// TestWindowsAceSIDSkipsUnhandledAceTypes confirms an ACE type this scan does +// not model (audit, mandatory label, ...) is skipped rather than misread as a +// plain or object ACE — the same conservative behavior the code had before +// object-ACE support was added. +func TestWindowsAceSIDSkipsUnhandledAceTypes(t *testing.T) { + const systemMandatoryLabelAceType = 0x11 + buf := make([]byte, 32) + buf[0] = systemMandatoryLabelAceType + ace := (*windows.ACCESS_ALLOWED_ACE)(unsafe.Pointer(&buf[0])) + if _, ok := windowsAceSID(ace); ok { + t.Fatal("windowsAceSID should return ok=false for an unhandled ACE type") + } +} + +func TestWindowsPathDeniesCapabilitySIDRequiresEssentialWriteMask(t *testing.T) { + dir := t.TempDir() + sid := "S-1-1-0" + + group := windowsACLPathGroup{ + Path: dir, + Entries: []WindowsACLEntry{{ + Action: WindowsACLDenyWrite, + Path: dir, + Capability: sid, + }}, + } + if _, _, err := applyWindowsACLPathGroup(group); err != nil { + t.Fatal(err) + } + + denied, err := windowsPathDeniesCapabilitySID(dir, sid) + if err != nil { + t.Fatal(err) + } + if !denied { + t.Fatal("expected full DenyWrite to satisfy windowsPathDeniesCapabilitySID") + } +} + +// denyCapabilityMask adds a direct (non-inheriting) deny ACE for capabilitySID +// covering only mask. Used to build partial-deny fixtures that must not pass +// windowsPathDeniesCapabilitySID's complete-coverage check. +func denyCapabilityMask(t *testing.T, path, capabilitySID string, mask windows.ACCESS_MASK) { + t.Helper() + sid, err := windows.StringToSid(capabilitySID) + if err != nil { + t.Fatalf("StringToSid(%q): %v", capabilitySID, err) + } + sd, err := windows.GetNamedSecurityInfo(path, windows.SE_FILE_OBJECT, windows.DACL_SECURITY_INFORMATION) + if err != nil { + t.Fatalf("GetNamedSecurityInfo %s: %v", path, err) + } + oldDACL, _, err := sd.DACL() + if err != nil { + t.Fatalf("DACL %s: %v", path, err) + } + newDACL, err := windows.ACLFromEntries([]windows.EXPLICIT_ACCESS{{ + AccessPermissions: mask, + AccessMode: windows.DENY_ACCESS, + Inheritance: windows.NO_INHERITANCE, + Trustee: windows.TRUSTEE{ + TrusteeForm: windows.TRUSTEE_IS_SID, + TrusteeType: windows.TRUSTEE_IS_WELL_KNOWN_GROUP, + TrusteeValue: windows.TrusteeValueFromSID(sid), + }, + }}, oldDACL) + if err != nil { + t.Fatalf("ACLFromEntries %s: %v", path, err) + } + if err := windows.SetNamedSecurityInfo(path, windows.SE_FILE_OBJECT, windows.DACL_SECURITY_INFORMATION, nil, nil, newDACL, nil); err != nil { + t.Fatalf("SetNamedSecurityInfo %s: %v", path, err) + } +} + +// TestWindowsPathDeniesCapabilitySIDRejectsPartialWriteDeny is the regression +// for jatmn's complete-coverage finding: a stable-SID deny that only blocks +// FILE_WRITE_ATTRIBUTES (or any other proper subset of the write probe mask) +// must not be accepted as descendant coverage. FILE_WRITE_DATA would still +// pass through a Users/AuthUsers allow under a partial-deny check that only +// tested non-zero overlap with the probe mask. +func TestWindowsPathDeniesCapabilitySIDRejectsPartialWriteDeny(t *testing.T) { + const sidStr = "S-1-1-0" + dir := t.TempDir() + denyCapabilityMask(t, dir, sidStr, windows.FILE_WRITE_ATTRIBUTES) + + denied, err := windowsPathDeniesCapabilitySID(dir, sidStr) + if err != nil { + t.Fatalf("windowsPathDeniesCapabilitySID: %v", err) + } + if denied { + t.Fatal("windowsPathDeniesCapabilitySID = true for FILE_WRITE_ATTRIBUTES-only deny, want false: partial deny must not certify write coverage") + } + + // applyWindowsSharedDescendantDenies must still apply the full canonical + // deny when the only pre-existing ACE is this partial one. + scanRoot := t.TempDir() + child := mkdir(t, filepath.Join(scanRoot, "partial")) + grantUsersWrite(t, child) + denyCapabilityMask(t, child, sidStr, windows.FILE_WRITE_ATTRIBUTES) + if denied, err := windowsPathDeniesCapabilitySID(child, sidStr); err != nil || denied { + t.Fatalf("precondition: partial deny on child must not count as coverage (denied=%v err=%v)", denied, err) + } + if _, err := applyWindowsSharedDescendantDenies(scanRoot, sidStr, nil); err != nil { + t.Fatalf("applyWindowsSharedDescendantDenies: %v", err) + } + if denied, err := windowsPathDeniesCapabilitySID(child, sidStr); err != nil || !denied { + t.Fatalf("after apply: want full write deny on partial-deny child (denied=%v err=%v)", denied, err) + } +} + +// TestWindowsEnumerateWritableDescendantsSkipsJunctions is the real-Windows +// regression for jatmn's compatibility-junction finding: a directory junction +// under the scan root must not be followed (re-walking the target tree) or +// hard-failed when the reparse is non-listable. The real target path is still +// examined when reached without going through the reparse. +func TestWindowsEnumerateWritableDescendantsSkipsJunctions(t *testing.T) { + root := t.TempDir() + realDir := mkdir(t, filepath.Join(root, "real")) + writable := mkdir(t, filepath.Join(realDir, "writable")) + grantUsersWrite(t, writable) + junc := filepath.Join(root, "junc") + // mklink /J needs no elevation; create the junction via cmd. + out, err := exec.Command("cmd", "/c", "mklink", "/J", junc, realDir).CombinedOutput() + if err != nil { + t.Skipf("cannot create junction (mklink /J): %v %s", err, strings.TrimSpace(string(out))) + } + t.Cleanup(func() { _ = os.Remove(junc) }) + + if !windowsPathIsReparsePoint(junc) { + t.Fatalf("fixture bug: %q is not a reparse point", junc) + } + + found, err := windowsEnumerateWritableDescendants(root, nil) + if err != nil { + t.Fatalf("windowsEnumerateWritableDescendants: %v", err) + } + if !windowsPathListContains(found, writable) { + t.Fatalf("enumeration = %#v, want writable child %q via real path", found, writable) + } + // Junction path itself must not appear: no-follow skips reparse entries + // entirely (DACL inspect would follow and risk double-counting). + juncWritable := filepath.Join(junc, "writable") + if windowsPathListContains(found, junc) || windowsPathListContains(found, juncWritable) { + t.Fatalf("enumeration = %#v, must not include junction path %q or its followed child", found, junc) + } +} + +// TestApplyWindowsSharedDescendantDeniesIdempotentRootDeny pins that a second +// apply on an already-covered root does not stack another DenyWrite ACE. +func TestApplyWindowsSharedDescendantDeniesIdempotentRootDeny(t *testing.T) { + dir := t.TempDir() + // Synthetic capability SID so DenyWrite's WRITE_DAC/DELETE bits do not + // lock the test out of its own temp dir. + caps, err := LoadOrCreateWindowsCapabilitySIDs(t.TempDir()) + if err != nil { + t.Fatalf("LoadOrCreateWindowsCapabilitySIDs: %v", err) + } + sid := caps.ReadOnly + + group := windowsACLPathGroup{ + Path: dir, + Entries: []WindowsACLEntry{{ + Action: WindowsACLDenyWrite, + Path: dir, + Capability: sid, + NoInherit: true, + }}, + } + if _, _, err := applyWindowsACLPathGroup(group); err != nil { + t.Fatal(err) + } + countBefore := denyACECountForSID(t, dir, sid) + + // A second full DenyWrite merge for the same SID must leave the ACE count + // unchanged (complete coverage skips re-applying). + if _, _, err := applyWindowsACLPathGroup(group); err != nil { + t.Fatal(err) + } + countAfter := denyACECountForSID(t, dir, sid) + if countAfter != countBefore { + t.Fatalf("deny ACE count for %q changed from %d to %d after second apply", sid, countBefore, countAfter) + } +} + +// denyACECountForSID counts ACCESS_DENIED ACEs naming wantSID on path's DACL. +func denyACECountForSID(t *testing.T, path, wantSID string) int { + t.Helper() + want, err := windows.StringToSid(wantSID) + if err != nil { + t.Fatalf("StringToSid %q: %v", wantSID, err) + } + sd, err := windows.GetNamedSecurityInfo(path, windows.SE_FILE_OBJECT, windows.DACL_SECURITY_INFORMATION) + if err != nil { + t.Fatalf("GetNamedSecurityInfo %s: %v", path, err) + } + dacl, _, err := sd.DACL() + if err != nil { + t.Fatalf("DACL %s: %v", path, err) + } + if dacl == nil { + return 0 + } + count := 0 + for index := uint16(0); index < dacl.AceCount; index++ { + var ace *windows.ACCESS_ALLOWED_ACE + if err := windows.GetAce(dacl, uint32(index), &ace); err != nil { + t.Fatalf("GetAce %d of %s: %v", index, path, err) + } + if ace.Header.AceType != windows.ACCESS_DENIED_ACE_TYPE { + continue + } + sid, ok := windowsAceSID(ace) + if ok && sid.Equals(want) { + count++ + } + } + return count +} diff --git a/internal/sandbox/windows_acl_paths_other.go b/internal/sandbox/windows_acl_paths_other.go new file mode 100644 index 000000000..f3de23b0c --- /dev/null +++ b/internal/sandbox/windows_acl_paths_other.go @@ -0,0 +1,34 @@ +//go:build !windows + +package sandbox + +import "os" + +// resolveWindowsSharedDenyPaths mirrors resolveWindowsSharedDenyPaths from +// windows_acl_paths_windows.go using environment-variable fallbacks. The +// trusted-API resolution used on Windows cannot be exercised on other GOOS, +// but that carries none of the production risk it exists to close: +// BuildWindowsACLPlan's shared-deny-path logic only ever runs for real on +// Windows (applyWindowsACLPlan, which actually mutates a DACL, is itself +// windows-only), so on other platforms this is only reached from unit tests +// that inspect the plan's structure, not from an elevated setup process +// whose environment an attacker might control. +func resolveWindowsSharedDenyPaths() (systemDrive, systemRoot, programData, publicDir string, err error) { + 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, nil +} diff --git a/internal/sandbox/windows_acl_paths_windows.go b/internal/sandbox/windows_acl_paths_windows.go new file mode 100644 index 000000000..7d7a6d910 --- /dev/null +++ b/internal/sandbox/windows_acl_paths_windows.go @@ -0,0 +1,44 @@ +//go:build windows + +package sandbox + +import ( + "fmt" + "path/filepath" + + "golang.org/x/sys/windows" +) + +// resolveWindowsSharedDenyPaths resolves the canonical system paths that +// earlier SID-broadening builds protected with shared DenyWrite entries +// (the system drive root, %SystemRoot%\Temp, ProgramData, and the Public +// user profile). SID broadening is disabled, so BuildWindowsACLPlan no +// longer stamps those denies; this resolver remains for tests and for any +// future access-time design that needs the same canonical roots. +// +// Paths are resolved from trusted Win32 APIs (GetSystemWindowsDirectory, +// SHGetKnownFolderPath) rather than SystemDrive/SystemRoot/ProgramData/ +// PUBLIC environment variables, which are ordinary process environment +// state and spoofable by anything that can influence the elevated setup +// process. +func resolveWindowsSharedDenyPaths() (systemDrive, systemRoot, programData, publicDir string, err error) { + // Prefer GetSystemWindowsDirectory: it always returns the shared system + // Windows directory. GetWindowsDirectory can return a private per-session + // path on Terminal Services for non-TSAWARE processes. + windowsDir, err := windows.GetSystemWindowsDirectory() + if err != nil { + return "", "", "", "", fmt.Errorf("resolve system windows directory: %w", err) + } + systemRoot = filepath.Clean(windowsDir) + systemDrive = filepath.VolumeName(systemRoot) + if systemDrive == "" { + return "", "", "", "", fmt.Errorf("resolve system drive from windows directory %q", systemRoot) + } + if programData, err = windows.KnownFolderPath(windows.FOLDERID_ProgramData, 0); err != nil { + return "", "", "", "", fmt.Errorf("resolve ProgramData known folder: %w", err) + } + if publicDir, err = windows.KnownFolderPath(windows.FOLDERID_Public, 0); err != nil { + return "", "", "", "", fmt.Errorf("resolve Public known folder: %w", err) + } + return systemDrive, systemRoot, programData, publicDir, nil +} diff --git a/internal/sandbox/windows_acl_test.go b/internal/sandbox/windows_acl_test.go index 1925bd8a9..5af061cc0 100644 --- a/internal/sandbox/windows_acl_test.go +++ b/internal/sandbox/windows_acl_test.go @@ -12,6 +12,7 @@ func TestBuildWindowsACLPlanForWorkspaceWriteProfile(t *testing.T) { config := WindowsSandboxCommandConfig{ SandboxHome: home, WorkspaceRoots: []string{`C:\workspace`}, + SandboxLevel: WindowsSandboxLevelRestrictedToken, PermissionProfile: PermissionProfile{ FileSystem: FileSystemPolicy{ Kind: FileSystemRestricted, @@ -52,6 +53,78 @@ func TestBuildWindowsACLPlanForWorkspaceWriteProfile(t *testing.T) { assertWindowsACLEntry(t, plan, WindowsACLDenyWrite, `C:\workspace\secret-write`, cacheSID, false) assertWindowsACLEntry(t, plan, WindowsACLDenyRead, `C:\workspace\secret-read`, workspaceSID, true) assertWindowsACLEntry(t, plan, WindowsACLDenyRead, `C:\workspace\secret-read`, cacheSID, true) + + // SID broadening is disabled, so the plan must not stamp shared system-path + // DenyWrite ACEs or revoke legacy capability SIDs. Revocation could weaken + // the boundary of a command launched by an earlier build. + assertNoSharedSystemDenyWrites(t, plan) + assertNoWindowsACLRevokes(t, plan) +} + +// TestBuildWindowsACLPlanOmitsSharedDenyPathsWithoutDenyRead pins that +// profiles without DenyRead never stamp shared system-path DenyWrite ACEs or +// revoke old capability-SID guards that a running sandbox may still require. +func TestBuildWindowsACLPlanOmitsSharedDenyPathsWithoutDenyRead(t *testing.T) { + home := t.TempDir() + plan, err := BuildWindowsACLPlan(WindowsSandboxCommandConfig{ + SandboxHome: home, + WorkspaceRoots: []string{`C:\workspace`}, + SandboxLevel: WindowsSandboxLevelRestrictedToken, + PermissionProfile: PermissionProfile{ + FileSystem: FileSystemPolicy{ + Kind: FileSystemRestricted, + WriteRoots: []WritableRoot{{Root: `C:\workspace`}}, + }, + Network: NetworkPolicy{Mode: NetworkDeny}, + }, + }) + if err != nil { + t.Fatalf("BuildWindowsACLPlan: %v", err) + } + assertNoSharedSystemDenyWrites(t, plan) + assertNoWindowsACLRevokes(t, plan) +} + +// TestBuildWindowsACLPlanOmitsSharedDenyPathsWhenUnelevated pins that the +// unelevated tier never stamps shared system-path DenyWrite ACEs (it also +// never broadens the restricted-SID list). +func TestBuildWindowsACLPlanOmitsSharedDenyPathsWhenUnelevated(t *testing.T) { + home := t.TempDir() + plan, err := BuildWindowsACLPlan(WindowsSandboxCommandConfig{ + SandboxHome: home, + WorkspaceRoots: []string{`C:\workspace`}, + SandboxLevel: WindowsSandboxLevelUnelevated, + PermissionProfile: PermissionProfile{ + FileSystem: FileSystemPolicy{ + Kind: FileSystemRestricted, + WriteRoots: []WritableRoot{{Root: `C:\workspace`}}, + DenyRead: []string{`C:\workspace\secret`}, + }, + Network: NetworkPolicy{Mode: NetworkDeny}, + }, + }) + if err != nil { + t.Fatalf("BuildWindowsACLPlan: %v", err) + } + assertNoSharedSystemDenyWrites(t, plan) + for _, entry := range plan.Entries { + if entry.Action == WindowsACLRevokeCapability { + t.Fatalf("unelevated plan = %#v, want no WindowsACLRevokeCapability entry", plan.Entries) + } + } +} + +// windowsSharedDenyPathsForTest calls the same trusted-path resolution +// BuildWindowsACLPlan itself uses, rather than reimplementing the +// resolution logic independently, so this test cannot silently drift out of +// sync with (or mask a regression in) the production resolver. +func windowsSharedDenyPathsForTest(t *testing.T) (systemDrive, systemRoot, programData, publicDir string) { + t.Helper() + systemDrive, systemRoot, programData, publicDir, err := resolveWindowsSharedDenyPaths() + if err != nil { + t.Fatalf("resolveWindowsSharedDenyPaths: %v", err) + } + return systemDrive, systemRoot, programData, publicDir } func TestBuildWindowsACLPlanUsesReadOnlySIDWithoutWriteRoots(t *testing.T) { @@ -61,7 +134,8 @@ func TestBuildWindowsACLPlanUsesReadOnlySIDWithoutWriteRoots(t *testing.T) { t.Fatalf("LoadOrCreateWindowsCapabilitySIDs: %v", err) } plan, err := BuildWindowsACLPlan(WindowsSandboxCommandConfig{ - SandboxHome: home, + SandboxHome: home, + SandboxLevel: WindowsSandboxLevelRestrictedToken, PermissionProfile: PermissionProfile{ FileSystem: FileSystemPolicy{ Kind: FileSystemRestricted, @@ -73,10 +147,88 @@ func TestBuildWindowsACLPlanUsesReadOnlySIDWithoutWriteRoots(t *testing.T) { if err != nil { t.Fatalf("BuildWindowsACLPlan: %v", err) } + // Without write roots there is nothing to revoke; without SID broadening + // there are no shared system-path DenyWrite entries either. if len(plan.Entries) != 1 { t.Fatalf("ACL entries = %#v, want one deny-read entry", plan.Entries) } assertWindowsACLEntry(t, plan, WindowsACLDenyRead, `C:\workspace\secret-read`, caps.ReadOnly, true) + assertNoSharedSystemDenyWrites(t, plan) +} + +// TestBuildWindowsACLPlanDisablesSharedDenyPathDescendantScan pins that SID +// broadening is off: the plan must not request ScanDescendants or shared-root +// DenyWrite entries that only existed to compensate for Users/AuthUsers SIDs. +func TestBuildWindowsACLPlanDisablesSharedDenyPathDescendantScan(t *testing.T) { + home := t.TempDir() + plan, err := BuildWindowsACLPlan(WindowsSandboxCommandConfig{ + SandboxHome: home, + WorkspaceRoots: []string{`C:\workspace`}, + SandboxLevel: WindowsSandboxLevelRestrictedToken, + PermissionProfile: PermissionProfile{ + FileSystem: FileSystemPolicy{ + Kind: FileSystemRestricted, + WriteRoots: []WritableRoot{{Root: `C:\workspace`}}, + DenyRead: []string{`C:\workspace\secret-read`}, + DenyWrite: []string{`C:\workspace\secret-write`}, + }, + Network: NetworkPolicy{Mode: NetworkDeny}, + }, + }) + if err != nil { + t.Fatalf("BuildWindowsACLPlan: %v", err) + } + assertNoSharedSystemDenyWrites(t, plan) + for _, entry := range plan.Entries { + if entry.ScanDescendants { + t.Fatalf("plan entry %#v requests descendant scan; shared DenyWrite compensation is disabled", entry) + } + } +} + +// TestBuildWindowsACLPlanDoesNotRevokeLegacyGuards pins that a setup run does +// not remove persistent guards installed by an older build. A previously +// launched sandbox can still carry the legacy capability SID, so removing its +// deny would widen that process's access. +func TestBuildWindowsACLPlanDoesNotRevokeLegacyGuards(t *testing.T) { + home := t.TempDir() + plan, err := BuildWindowsACLPlan(WindowsSandboxCommandConfig{ + SandboxHome: home, + WorkspaceRoots: []string{`C:\workspace`}, + SandboxLevel: WindowsSandboxLevelRestrictedToken, + PermissionProfile: PermissionProfile{ + FileSystem: FileSystemPolicy{ + Kind: FileSystemRestricted, + WriteRoots: []WritableRoot{{Root: `C:\workspace`}}, + }, + Network: NetworkPolicy{Mode: NetworkDeny}, + }, + }) + if err != nil { + t.Fatalf("BuildWindowsACLPlan: %v", err) + } + assertNoWindowsACLRevokes(t, plan) +} + +func assertNoSharedSystemDenyWrites(t *testing.T, plan WindowsACLPlan) { + t.Helper() + systemDrive, systemRoot, programData, publicDir := windowsSharedDenyPathsForTest(t) + for _, path := range []string{systemDrive + `\`, programData, systemRoot + `\Temp`, publicDir} { + for _, entry := range plan.Entries { + if entry.Action == WindowsACLDenyWrite && windowsCapabilityPathKey(entry.Path) == windowsCapabilityPathKey(path) { + t.Fatalf("plan stamps shared system DenyWrite on %q = %#v; SID broadening is disabled so shared denies must not be planned", path, entry) + } + } + } +} + +func assertNoWindowsACLRevokes(t *testing.T, plan WindowsACLPlan) { + t.Helper() + for _, entry := range plan.Entries { + if entry.Action == WindowsACLRevokeCapability { + t.Fatalf("plan = %#v, want no WindowsACLRevokeCapability entries", plan.Entries) + } + } } func TestBuildWindowsACLPlanRejectsUnrestrictedProfiles(t *testing.T) { @@ -117,16 +269,22 @@ func TestPlanWindowsDenyReadPathsIncludesCanonicalExistingPath(t *testing.T) { } func assertWindowsACLEntry(t *testing.T, plan WindowsACLPlan, action WindowsACLAction, path string, capability string, materialize bool) { + t.Helper() + assertWindowsACLEntryInheritance(t, plan, action, path, capability, materialize, false) +} + +func assertWindowsACLEntryInheritance(t *testing.T, plan WindowsACLPlan, action WindowsACLAction, path string, capability string, materialize bool, noInherit bool) { t.Helper() for _, entry := range plan.Entries { if entry.Action == action && windowsCapabilityPathKey(entry.Path) == windowsCapabilityPathKey(path) && strings.EqualFold(entry.Capability, capability) && - entry.Materialize == materialize { + entry.Materialize == materialize && + entry.NoInherit == noInherit { return } } - t.Fatalf("ACL entries = %#v, want %s %q capability %q materialize=%v", plan.Entries, action, path, capability, materialize) + t.Fatalf("ACL entries = %#v, want %s %q capability %q materialize=%v noInherit=%v", plan.Entries, action, path, capability, materialize, noInherit) } func windowsPathListContains(paths []string, want string) bool { @@ -138,3 +296,24 @@ func windowsPathListContains(paths []string, want string) bool { } return false } + +// TestDedupeWindowsACLEntriesKeepsInheritanceVariants pins NoInherit as part +// of the entry identity: a direct-only deny and an inheritable deny on the +// same path and SID are different ACL shapes, and 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. +func TestDedupeWindowsACLEntriesKeepsInheritanceVariants(t *testing.T) { + entries := []WindowsACLEntry{ + {Action: WindowsACLDenyWrite, Path: `C:\shared`, Capability: "S-1-5-21-1", NoInherit: true}, + {Action: WindowsACLDenyWrite, Path: `C:\shared`, Capability: "S-1-5-21-1"}, + {Action: WindowsACLDenyWrite, Path: `C:\shared`, Capability: "S-1-5-21-1", NoInherit: true}, + } + out := dedupeWindowsACLEntries(entries) + if len(out) != 2 { + t.Fatalf("dedupe = %#v, want the NoInherit and inheritable variants kept distinct", out) + } + if !out[0].NoInherit || out[1].NoInherit { + t.Fatalf("dedupe order/shape = %#v, want first NoInherit then inheritable", out) + } +} diff --git a/internal/sandbox/windows_command_runner.go b/internal/sandbox/windows_command_runner.go index cb320f4ca..0991550a3 100644 --- a/internal/sandbox/windows_command_runner.go +++ b/internal/sandbox/windows_command_runner.go @@ -11,9 +11,52 @@ func RunWindowsSandboxCommandRunner(args []string, stderr io.Writer) int { fmt.Fprintln(stderr, WindowsSandboxCommandRunnerName+": "+err.Error()) return 2 } + // Reject unsupported DenyRead profiles before minting persistent capability + // SID state under SandboxHome (defense in depth: runWindowsSandboxCommand + // also checks, but only after LoadOrCreateWindowsCapabilitySIDs). + if err := windowsDenyReadRestrictedTokenUnsupported(config); err != nil { + fmt.Fprintln(stderr, WindowsSandboxCommandRunnerName+": "+err.Error()) + return 1 + } if _, err := LoadOrCreateWindowsCapabilitySIDs(config.SandboxHome); err != nil { fmt.Fprintln(stderr, WindowsSandboxCommandRunnerName+": "+err.Error()) return 1 } return runWindowsSandboxCommand(config, stderr) } + +// windowsDenyReadRestrictedTokenUnsupported reports that Windows restricted- +// token sandboxing (elevated restricted-token or unelevated) cannot run +// profiles with DenyRead until access-time confinement exists. Both runner +// levels build the same fully restricted narrow-SID token when DenyRead is +// set: without Users/AuthUsers it cannot load ordinary system executables; +// adding those groups reopens write grants outside WriteRoots. Prefer a clear +// rejection over a silent launch failure. Do not recommend the other tier as a +// workaround: the limitation is the token mechanism, not elevation. +func windowsDenyReadRestrictedTokenUnsupported(config WindowsSandboxCommandConfig) error { + switch config.SandboxLevel { + case WindowsSandboxLevelRestrictedToken, WindowsSandboxLevelUnelevated: + default: + return nil + } + return windowsDenyReadRestrictedTokenUnsupportedProfile(config.PermissionProfile) +} + +// windowsDenyReadRestrictedTokenUnsupportedProfile is the level-agnostic check +// used by the manager, command-plan builder, setup, and runner so DenyRead is +// rejected before any restricted-token path can provision or launch. +func windowsDenyReadRestrictedTokenUnsupportedProfile(profile PermissionProfile) error { + if len(profile.FileSystem.DenyRead) == 0 { + return nil + } + return fmt.Errorf( + "DenyRead is not supported with the Windows restricted-token sandbox "+ + "(elevated or unelevated): without Users/Authenticated Users in the "+ + "restricting SID set, ordinary system binaries under Program Files and "+ + "Windows cannot load, and adding those groups would admit their existing "+ + "write grants outside WriteRoots. "+ + "Remove DenyRead from this configuration or use the documented sandbox_permissions require_escalated approval flow; that flow cannot preserve DenyRead. "+ + "Configured DenyRead path count: %d", + len(profile.FileSystem.DenyRead), + ) +} diff --git a/internal/sandbox/windows_command_runner_test.go b/internal/sandbox/windows_command_runner_test.go new file mode 100644 index 000000000..4ca21c595 --- /dev/null +++ b/internal/sandbox/windows_command_runner_test.go @@ -0,0 +1,60 @@ +package sandbox + +import ( + "strings" + "testing" +) + +func TestWindowsDenyReadRestrictedTokenUnsupported(t *testing.T) { + // Both restricted-token runner levels reject DenyRead before launch/setup. + for _, level := range []WindowsSandboxLevel{ + WindowsSandboxLevelRestrictedToken, + WindowsSandboxLevelUnelevated, + } { + err := windowsDenyReadRestrictedTokenUnsupported(WindowsSandboxCommandConfig{ + SandboxLevel: level, + PermissionProfile: PermissionProfile{ + FileSystem: FileSystemPolicy{ + Kind: FileSystemRestricted, + DenyRead: []string{`C:\secret`, `D:\private`}, + }, + }, + }) + if err == nil { + t.Fatalf("expected unsupported error for %s DenyRead profile", level) + } + msg := err.Error() + for _, want := range []string{"DenyRead", "not supported", "restricted-token", "unelevated", "path count: 2"} { + if !strings.Contains(msg, want) { + t.Fatalf("%s error %q missing %q", level, msg, want) + } + } + // DenyRead often names credential or private-file paths; keep them out of stderr. + for _, secret := range []string{`C:\secret`, `D:\private`} { + if strings.Contains(msg, secret) { + t.Fatalf("%s error leaked DenyRead path %q: %q", level, secret, msg) + } + } + if strings.Contains(msg, "--sandbox forbid") { + t.Fatalf("%s error advertises unsupported --sandbox forbid recovery: %q", level, msg) + } + if !strings.Contains(msg, "sandbox_permissions") { + t.Fatalf("%s error should name the documented sandbox_permissions recovery flow: %q", level, msg) + } + } + + // No DenyRead: allowed (WRITE_RESTRICTED path can launch system tools). + for _, level := range []WindowsSandboxLevel{ + WindowsSandboxLevelRestrictedToken, + WindowsSandboxLevelUnelevated, + } { + if err := windowsDenyReadRestrictedTokenUnsupported(WindowsSandboxCommandConfig{ + SandboxLevel: level, + PermissionProfile: PermissionProfile{ + FileSystem: FileSystemPolicy{Kind: FileSystemRestricted}, + }, + }); err != nil { + t.Fatalf("unexpected error without DenyRead at %s: %v", level, err) + } + } +} diff --git a/internal/sandbox/windows_command_runner_windows.go b/internal/sandbox/windows_command_runner_windows.go index 0b9e8f64e..de8eebfea 100644 --- a/internal/sandbox/windows_command_runner_windows.go +++ b/internal/sandbox/windows_command_runner_windows.go @@ -8,6 +8,15 @@ import ( ) func runWindowsSandboxCommand(config WindowsSandboxCommandConfig, stderr io.Writer) int { + // Fully restricted DenyRead tokens cannot load ordinary Users-granted + // system binaries without SID broadening; broadening is permanently off + // because it admits write grants outside WriteRoots. Reject on both the + // elevated and unelevated restricted-token tiers before setup or launch + // until access-time confinement exists (PR #640). + if err := windowsDenyReadRestrictedTokenUnsupported(config); err != nil { + fmt.Fprintln(stderr, WindowsSandboxCommandRunnerName+": "+err.Error()) + return 1 + } switch config.SandboxLevel { case WindowsSandboxLevelRestrictedToken: if err := ValidateWindowsSandboxSetupMarker(WindowsSandboxSetupConfigFromCommand(config)); err != nil { @@ -69,11 +78,11 @@ func runWindowsSandboxCommand(config WindowsSandboxCommandConfig, stderr io.Writ // this has no in-token fix; preflight blocking and output hints live in // internal/tools/shell_runtime.go. tokenSIDs := windowsRuntimeTokenSIDs(capabilitySIDs, offlineSID, config.PermissionProfile.Network.Mode) - // A WRITE_RESTRICTED token keeps reads unrestricted so sandboxed commands - // can actually launch executables; it is only unsafe when DenyRead paths - // are configured, because the kernel skips restricted-SID deny ACEs for - // reads under that flag (#612). Profiles with DenyRead keep the fully - // restricted token, trading spawn capability for read-deny enforcement. + // WRITE_RESTRICTED keeps reads unrestricted so sandboxed commands can load + // Users-granted executables. It is only used when DenyRead is empty (#612: + // WRITE_RESTRICTED skips restricted-SID deny ACEs for reads). Non-empty + // DenyRead is rejected above for both runner levels rather than launching a + // fully restricted narrow-SID token that cannot execute normal tools. writeRestricted := len(config.PermissionProfile.FileSystem.DenyRead) == 0 token, err := createWindowsRestrictedTokenForCapabilitySIDs(tokenSIDs, writeRestricted) if err != nil { diff --git a/internal/sandbox/windows_runner.go b/internal/sandbox/windows_runner.go index 032f2a844..400ecad58 100644 --- a/internal/sandbox/windows_runner.go +++ b/internal/sandbox/windows_runner.go @@ -317,6 +317,12 @@ func ParseWindowsSandboxCommandArgs(args []string) (WindowsSandboxCommandConfig, } func windowsRestrictedTokenCommandPlan(execRequest SandboxExecutionRequest, policy Policy) (CommandPlan, error) { + // Reject DenyRead before provisioning either restricted-token runner level. + // Both elevated and unelevated build the same fully restricted narrow-SID + // token for DenyRead profiles, which cannot load ordinary system binaries. + if err := windowsDenyReadRestrictedTokenUnsupportedProfile(execRequest.PermissionProfile); err != nil { + return CommandPlan{}, err + } spec := execRequest.Command var sandboxHomeEnv map[string]string if spec.Env != nil { @@ -329,7 +335,7 @@ func windowsRestrictedTokenCommandPlan(execRequest SandboxExecutionRequest, poli childEnv := sandboxEnvironmentForCommandWithSensitiveEnv(spec.Env, policy, BackendWindowsRestrictedToken, execRequest.WorkspaceRoot, spec.sensitiveEnvKeys) childEnv = sandboxRuntimeEnvironment(childEnv, execRequest.PermissionProfile.Runtime) // The unelevated enforcement tier maps to the runner's unelevated level: same - // restricted token, but the runner applies the workspace ACLs itself instead + // restricted token, but the runner applies the workspace ACL plan itself instead // of requiring the elevated setup marker. level := WindowsSandboxLevelRestrictedToken if execRequest.EnforcementLevel == EnforcementUnelevated { diff --git a/internal/sandbox/windows_setup_windows.go b/internal/sandbox/windows_setup_windows.go index 888355397..d2c2d2f90 100644 --- a/internal/sandbox/windows_setup_windows.go +++ b/internal/sandbox/windows_setup_windows.go @@ -17,6 +17,12 @@ func runWindowsSandboxSetup(config WindowsSandboxSetupConfig, stderr io.Writer) fmt.Fprintln(stderr, WindowsSandboxSetupName+": Administrator rights are required. Re-run `zero sandbox setup` from an elevated (Run as administrator) terminal.") return 1 } + // Do not provision DenyRead ACLs for a token mode that cannot launch normal + // tools with DenyRead under the narrow restricting-SID set (PR #640). + if err := windowsDenyReadRestrictedTokenUnsupportedProfile(config.PermissionProfile); err != nil { + fmt.Fprintln(stderr, WindowsSandboxSetupName+": "+err.Error()) + return 1 + } plan, err := BuildWindowsACLPlan(config.commandConfig()) if err != nil { fmt.Fprintln(stderr, WindowsSandboxSetupName+": "+err.Error()) From fe07a520850ca4d76a16f350479f0725f1868583 Mon Sep 17 00:00:00 2001 From: euxaristia Date: Sat, 22 Aug 2026 18:08:27 -0400 Subject: [PATCH 2/9] fix(sandbox): address CodeRabbit review feedback on PR 640 --- .../runner_windows_integration_test.go | 3 ++ internal/sandbox/windows_acl.go | 47 ++++------------- internal/sandbox/windows_acl_descendants.go | 29 ----------- .../sandbox/windows_acl_descendants_test.go | 50 ------------------- .../windows_acl_descendants_windows_test.go | 15 +++++- 5 files changed, 27 insertions(+), 117 deletions(-) diff --git a/internal/sandbox/runner_windows_integration_test.go b/internal/sandbox/runner_windows_integration_test.go index bf7697a7f..0ea0001c3 100644 --- a/internal/sandbox/runner_windows_integration_test.go +++ b/internal/sandbox/runner_windows_integration_test.go @@ -451,6 +451,9 @@ func runWindowsRealSmokeCommandExpectError(t *testing.T, runnerExe string, base defer cancel() cmd := exec.CommandContext(ctx, runnerExe, args...) output, err := cmd.CombinedOutput() + if ctx.Err() != nil { + t.Fatalf("Windows sandbox command timed out: %v\n%s", ctx.Err(), output) + } if err == nil { t.Fatalf("Windows sandbox command exit code = 0, want error containing %v\n%s", wantSubstr, output) } diff --git a/internal/sandbox/windows_acl.go b/internal/sandbox/windows_acl.go index a2a391b60..310bc9a9a 100644 --- a/internal/sandbox/windows_acl.go +++ b/internal/sandbox/windows_acl.go @@ -16,16 +16,9 @@ const ( // WindowsACLRevokeCapability removes any existing ACE (allow or deny) for // Capability at Path, without itself granting or denying anything (applied // via SetEntriesInAclW's SET_ACCESS mode with a zero mask, not - // REVOKE_ACCESS — see windowsACLAccess for why). It reconciles stale - // shared/descendant DenyWrite ACEs an earlier setup run applied for the - // stable read-only capability SID (see BuildWindowsACLPlan) that a later - // run no longer intends: if a path previously covered by the - // shared-root/descendant DenyWrite mitigation is later configured as an - // allowed write root, that old deny is otherwise left on disk and wins - // over the new Allow under Windows' deny-before-allow evaluation — see - // jatmn's review. Clearing a SID with no matching ACE is a safe no-op, so - // this can always be emitted unconditionally alongside every write-root - // Allow entry. + // REVOKE_ACCESS — see windowsACLAccess for why). It is consumed by + // applyWindowsACLPlan for migration cleanup and tests; no current + // plan-generation path emits this action, preserving legacy-process confinement. WindowsACLRevokeCapability WindowsACLAction = "revoke-capability" ) @@ -44,33 +37,15 @@ type WindowsACLEntry struct { Materialize bool `json:"materialize,omitempty"` // ScanDescendants marks a shared-root DenyWrite entry whose EXISTING // writable descendants must ALSO be denied, one direct (non-inheriting) - // deny per writable descendant, at apply time. A non-inherited deny on the - // root object alone does not cover a pre-existing child that independently - // grants Users/Authenticated Users write, because a Windows access check - // for that child never consults a non-inherited ACE on its parent. This is - // deliberately NOT serialized (json:"-"): the concrete descendant set is - // live-filesystem state that differs between the setup process and a later - // command run, so folding it into the hashed plan would make - // ValidateWindowsSandboxSetupMarker non-deterministic. The flag itself is - // derived deterministically from the same inputs on both sides, and the - // descendant enumeration/denies happen as an apply-time side effect in - // applyWindowsACLPlan (windows-only), never in the cross-platform plan hash. + // deny per writable descendant, at apply time. This flag and its associated + // walker are consumed by applyWindowsACLPlan and tests; no current + // plan-generation path sets this flag since broad shared-root DenyWrite ACEs + // and SID broadening are no longer planned. ScanDescendants bool `json:"-"` - // RevokeDescendants marks a write-root's WindowsACLRevokeCapability entry - // (see the constant below) as needing the same stale-deny cleanup applied - // recursively to the root's existing descendants, not just the root path - // itself. A tree scanned and denied by an earlier setup run (either - // because it WAS one of the four shared roots, or because it was a - // writable descendant applyWindowsSharedDescendantDenies found and denied - // elsewhere in the tree) can later be promoted to an allowed write root by - // the caller configuring some ANCESTOR of it as a WriteRoot. Revoking only - // at the exact configured root leaves any stale direct, non-inheriting - // deny on that ancestor's descendants in place, and a stale deny wins over - // the newly-added inheritable Allow under Windows' deny-before-allow - // evaluation — see jatmn's review. Like ScanDescendants, this is - // deliberately NOT serialized (json:"-"): the concrete stale-deny set is - // live-filesystem state, and the actual descendant walk/revoke happens as - // an apply-time side effect in applyWindowsACLPlan (windows-only). + // RevokeDescendants marks a WindowsACLRevokeCapability entry as needing + // stale-deny cleanup applied recursively to existing descendants, not just + // the root path itself. This flag is consumed by applyWindowsACLPlan for + // migration cleanup and tests; no current plan-generation path sets this flag. RevokeDescendants bool `json:"-"` } diff --git a/internal/sandbox/windows_acl_descendants.go b/internal/sandbox/windows_acl_descendants.go index f076c285f..0bdd59e4b 100644 --- a/internal/sandbox/windows_acl_descendants.go +++ b/internal/sandbox/windows_acl_descendants.go @@ -42,32 +42,3 @@ func windowsPathIsDriveRootPath(path string) bool { c := trimmed[0] return (c >= 'A' && c <= 'Z') || (c >= 'a' && c <= 'z') } - -// windowsMountPathIsOnlySystemDrive reports whether a volume mount path is the -// system drive root (e.g. `C:\` or `C:`) rather than another letter or a -// folder mount such as `C:\mnt\data`. Used by the volume gate so a second -// fixed volume mounted only as a folder is rejected the same as one mounted -// on a drive letter. -func windowsMountPathIsOnlySystemDrive(mountPath, systemDrive string) bool { - trimmed := strings.TrimSuffix(mountPath, `\`) - return strings.EqualFold(strings.ToUpper(trimmed), strings.ToUpper(systemDrive)) -} - -// windowsMountPathsAreOnlySystemDrive reports whether mountPaths (a fixed -// volume's DOS/folder mount points from GetVolumePathNamesForVolumeName) name -// the system drive root and nothing else. A fixed volume with NO mount paths -// at all is still directly reachable through its raw "\\?\Volume{GUID}\" -// path even though it has no conventional mount point, so an empty list -// fails closed (false) instead of being read as "unreachable." Pure string -// logic so non-Windows tests can pin the fail-closed cases without Win32. -func windowsMountPathsAreOnlySystemDrive(mountPaths []string, systemDrive string) bool { - if len(mountPaths) == 0 { - return false - } - for _, mountPath := range mountPaths { - if !windowsMountPathIsOnlySystemDrive(mountPath, systemDrive) { - return false - } - } - return true -} diff --git a/internal/sandbox/windows_acl_descendants_test.go b/internal/sandbox/windows_acl_descendants_test.go index c846594dd..fdb800c15 100644 --- a/internal/sandbox/windows_acl_descendants_test.go +++ b/internal/sandbox/windows_acl_descendants_test.go @@ -43,53 +43,3 @@ func TestWindowsPathIsDriveRootPath(t *testing.T) { } } } - -func TestWindowsMountPathIsOnlySystemDrive(t *testing.T) { - cases := []struct { - mount, system string - want bool - }{ - {`C:\`, `C:`, true}, - {`C:`, `C:`, true}, - {`c:\`, `C:`, true}, - {`D:\`, `C:`, false}, - {`C:\mnt\data`, `C:`, false}, - {`C:\mnt\data\`, `C:`, false}, - {`\\?\Volume{guid}\`, `C:`, false}, - } - for _, tc := range cases { - got := windowsMountPathIsOnlySystemDrive(tc.mount, tc.system) - if got != tc.want { - t.Fatalf("windowsMountPathIsOnlySystemDrive(%q, %q) = %v, want %v", tc.mount, tc.system, got, tc.want) - } - } -} - -// TestWindowsMountPathsAreOnlySystemDrive pins the volume-gate fail-closed fix -// (jatmn's review): a fixed volume with no mount points at all must not be -// read as "unreachable" (it is still reachable via its raw -// "\\?\Volume{GUID}\" path), and any mount path other than the system drive -// root disqualifies the volume, matching the existing per-path behavior. -func TestWindowsMountPathsAreOnlySystemDrive(t *testing.T) { - cases := []struct { - name string - mountPaths []string - system string - want bool - }{ - {"only system drive", []string{`C:\`}, `C:`, true}, - {"no mount points at all", nil, `C:`, false}, - {"empty mount list", []string{}, `C:`, false}, - {"extra drive letter", []string{`C:\`, `D:\`}, `C:`, false}, - {"folder mount point", []string{`C:\mnt\data`}, `C:`, false}, - {"other drive only", []string{`D:\`}, `C:`, false}, - } - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - got := windowsMountPathsAreOnlySystemDrive(tc.mountPaths, tc.system) - if got != tc.want { - t.Fatalf("windowsMountPathsAreOnlySystemDrive(%#v, %q) = %v, want %v", tc.mountPaths, tc.system, got, tc.want) - } - }) - } -} diff --git a/internal/sandbox/windows_acl_descendants_windows_test.go b/internal/sandbox/windows_acl_descendants_windows_test.go index b4f57d3e1..837e81a88 100644 --- a/internal/sandbox/windows_acl_descendants_windows_test.go +++ b/internal/sandbox/windows_acl_descendants_windows_test.go @@ -568,8 +568,12 @@ func TestWindowsAceSIDSkipsUnhandledAceTypes(t *testing.T) { } func TestWindowsPathDeniesCapabilitySIDRequiresEssentialWriteMask(t *testing.T) { + caps, err := LoadOrCreateWindowsCapabilitySIDs(t.TempDir()) + if err != nil { + t.Fatalf("LoadOrCreateWindowsCapabilitySIDs: %v", err) + } + sid := caps.ReadOnly dir := t.TempDir() - sid := "S-1-1-0" group := windowsACLPathGroup{ Path: dir, @@ -634,7 +638,14 @@ func denyCapabilityMask(t *testing.T, path, capabilitySID string, mask windows.A // pass through a Users/AuthUsers allow under a partial-deny check that only // tested non-zero overlap with the probe mask. func TestWindowsPathDeniesCapabilitySIDRejectsPartialWriteDeny(t *testing.T) { - const sidStr = "S-1-1-0" + // Synthetic capability SID: a full DenyWrite for a group the test process + // belongs to (e.g. Everyone) denies DELETE/WRITE_DAC and blocks t.TempDir + // cleanup. + caps, err := LoadOrCreateWindowsCapabilitySIDs(t.TempDir()) + if err != nil { + t.Fatalf("LoadOrCreateWindowsCapabilitySIDs: %v", err) + } + sidStr := caps.ReadOnly dir := t.TempDir() denyCapabilityMask(t, dir, sidStr, windows.FILE_WRITE_ATTRIBUTES) From 5465b0f8f440f26b1c044a578d3c6257cfbe765f Mon Sep 17 00:00:00 2001 From: euxaristia Date: Mon, 24 Aug 2026 06:40:21 -0400 Subject: [PATCH 3/9] refactor(sandbox): Address CodeRabbit nitpicks on PR 640. Co-Authored-By: cairn-code --- internal/sandbox/windows_acl.go | 4 ++-- internal/sandbox/windows_acl_descendants_windows_test.go | 8 +++++--- internal/sandbox/windows_acl_test.go | 6 +----- 3 files changed, 8 insertions(+), 10 deletions(-) diff --git a/internal/sandbox/windows_acl.go b/internal/sandbox/windows_acl.go index 310bc9a9a..982f60e76 100644 --- a/internal/sandbox/windows_acl.go +++ b/internal/sandbox/windows_acl.go @@ -31,8 +31,8 @@ type WindowsACLEntry struct { // every directory ACE inheritable (SUB_CONTAINERS_AND_OBJECTS_INHERIT), // and SetNamedSecurityInfo automatically propagates any inheritable ACE // down onto the target's EXISTING descendants (not just new ones it - // creates going forward) — see the shared-deny-path entries below for - // why that is unsafe on broad system roots. + // creates going forward), which is why direct-only denies must set this + // flag rather than rely on inheritance. NoInherit bool `json:"noInherit,omitempty"` Materialize bool `json:"materialize,omitempty"` // ScanDescendants marks a shared-root DenyWrite entry whose EXISTING diff --git a/internal/sandbox/windows_acl_descendants_windows_test.go b/internal/sandbox/windows_acl_descendants_windows_test.go index 837e81a88..0567aad31 100644 --- a/internal/sandbox/windows_acl_descendants_windows_test.go +++ b/internal/sandbox/windows_acl_descendants_windows_test.go @@ -711,9 +711,11 @@ func TestWindowsEnumerateWritableDescendantsSkipsJunctions(t *testing.T) { } } -// TestApplyWindowsSharedDescendantDeniesIdempotentRootDeny pins that a second -// apply on an already-covered root does not stack another DenyWrite ACE. -func TestApplyWindowsSharedDescendantDeniesIdempotentRootDeny(t *testing.T) { +// TestApplyWindowsACLPathGroupIdempotentDenyWrite pins that a repeated +// applyWindowsACLPathGroup DenyWrite merge on an already-covered root does not +// stack another DenyWrite ACE. The descendant walker's own idempotency is +// covered by TestApplyWindowsSharedDescendantDeniesAppliesAndRollsBack. +func TestApplyWindowsACLPathGroupIdempotentDenyWrite(t *testing.T) { dir := t.TempDir() // Synthetic capability SID so DenyWrite's WRITE_DAC/DELETE bits do not // lock the test out of its own temp dir. diff --git a/internal/sandbox/windows_acl_test.go b/internal/sandbox/windows_acl_test.go index 5af061cc0..2d819bce8 100644 --- a/internal/sandbox/windows_acl_test.go +++ b/internal/sandbox/windows_acl_test.go @@ -107,11 +107,7 @@ func TestBuildWindowsACLPlanOmitsSharedDenyPathsWhenUnelevated(t *testing.T) { t.Fatalf("BuildWindowsACLPlan: %v", err) } assertNoSharedSystemDenyWrites(t, plan) - for _, entry := range plan.Entries { - if entry.Action == WindowsACLRevokeCapability { - t.Fatalf("unelevated plan = %#v, want no WindowsACLRevokeCapability entry", plan.Entries) - } - } + assertNoWindowsACLRevokes(t, plan) } // windowsSharedDenyPathsForTest calls the same trusted-path resolution From d9d72e8078f4043a023f0b7703d2894c0993a127 Mon Sep 17 00:00:00 2001 From: euxaristia Date: Mon, 31 Aug 2026 03:39:31 -0400 Subject: [PATCH 4/9] fix(sandbox): skip inherited ACEs and reject DenyRead upfront during 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 #640 --- internal/sandbox/windows_acl_apply_windows.go | 3 + .../windows_acl_descendants_windows_test.go | 63 +++++++++++++++++++ internal/sandbox/windows_command_runner.go | 2 +- internal/sandbox/windows_setup_test.go | 31 +++++++++ internal/sandbox/windows_setup_windows.go | 12 ++-- 5 files changed, 104 insertions(+), 7 deletions(-) diff --git a/internal/sandbox/windows_acl_apply_windows.go b/internal/sandbox/windows_acl_apply_windows.go index 45384f061..2ebfb25e3 100644 --- a/internal/sandbox/windows_acl_apply_windows.go +++ b/internal/sandbox/windows_acl_apply_windows.go @@ -403,6 +403,9 @@ func windowsPreservedReadDenyAccessEntries(oldDACL *windows.ACL, wantSID *window if ace.Header.AceType != windows.ACCESS_DENIED_ACE_TYPE && ace.Header.AceType != windowsAccessDeniedObjectAceType { continue } + if ace.Header.AceFlags&windows.INHERITED_ACE != 0 { + continue + } sid, ok := windowsAceSID(ace) if !ok || !sid.Equals(wantSID) { continue diff --git a/internal/sandbox/windows_acl_descendants_windows_test.go b/internal/sandbox/windows_acl_descendants_windows_test.go index 0567aad31..671bcb4ee 100644 --- a/internal/sandbox/windows_acl_descendants_windows_test.go +++ b/internal/sandbox/windows_acl_descendants_windows_test.go @@ -784,3 +784,66 @@ func denyACECountForSID(t *testing.T, path, wantSID string) int { } return count } + +func TestWindowsPreservedReadDenyAccessEntriesSkipsInheritedACEs(t *testing.T) { + caps, err := LoadOrCreateWindowsCapabilitySIDs(t.TempDir()) + if err != nil { + t.Fatalf("LoadOrCreateWindowsCapabilitySIDs: %v", err) + } + sid, err := windows.StringToSid(caps.ReadOnly) + if err != nil { + t.Fatalf("StringToSid: %v", err) + } + + parent := t.TempDir() + // Add an inheritable DenyRead on parent. + sd, err := windows.GetNamedSecurityInfo(parent, windows.SE_FILE_OBJECT, windows.DACL_SECURITY_INFORMATION) + if err != nil { + t.Fatalf("GetNamedSecurityInfo %s: %v", parent, err) + } + oldDACL, _, err := sd.DACL() + if err != nil { + t.Fatalf("DACL: %v", err) + } + parentDACL, err := windows.ACLFromEntries([]windows.EXPLICIT_ACCESS{{ + AccessPermissions: windows.FILE_GENERIC_READ | windows.FILE_EXECUTE, + AccessMode: windows.DENY_ACCESS, + Inheritance: windows.CONTAINER_INHERIT_ACE | windows.OBJECT_INHERIT_ACE, + Trustee: windows.TRUSTEE{ + TrusteeForm: windows.TRUSTEE_IS_SID, + TrusteeType: windows.TRUSTEE_IS_WELL_KNOWN_GROUP, + TrusteeValue: windows.TrusteeValueFromSID(sid), + }, + }}, oldDACL) + if err != nil { + t.Fatalf("ACLFromEntries: %v", err) + } + if err := windows.SetNamedSecurityInfo(parent, windows.SE_FILE_OBJECT, windows.DACL_SECURITY_INFORMATION, nil, nil, parentDACL, nil); err != nil { + t.Fatalf("SetNamedSecurityInfo parent: %v", err) + } + + // Create child directory — it inherits the DenyRead ACE with INHERITED_ACE set. + child := filepath.Join(parent, "child") + if err := os.Mkdir(child, 0o700); err != nil { + t.Fatalf("Mkdir child: %v", err) + } + + // Read child's DACL. + childSD, err := windows.GetNamedSecurityInfo(child, windows.SE_FILE_OBJECT, windows.DACL_SECURITY_INFORMATION) + if err != nil { + t.Fatalf("GetNamedSecurityInfo child: %v", err) + } + childDACL, _, err := childSD.DACL() + if err != nil { + t.Fatalf("child DACL: %v", err) + } + + // Preserving read deny entries on child must skip the inherited ACE. + preserved, err := windowsPreservedReadDenyAccessEntries(childDACL, sid, true) + if err != nil { + t.Fatalf("windowsPreservedReadDenyAccessEntries: %v", err) + } + if len(preserved) != 0 { + t.Fatalf("expected inherited ACE to be skipped, got %d preserved entries: %#v", len(preserved), preserved) + } +} diff --git a/internal/sandbox/windows_command_runner.go b/internal/sandbox/windows_command_runner.go index 0991550a3..cc1a66de9 100644 --- a/internal/sandbox/windows_command_runner.go +++ b/internal/sandbox/windows_command_runner.go @@ -55,7 +55,7 @@ func windowsDenyReadRestrictedTokenUnsupportedProfile(profile PermissionProfile) "restricting SID set, ordinary system binaries under Program Files and "+ "Windows cannot load, and adding those groups would admit their existing "+ "write grants outside WriteRoots. "+ - "Remove DenyRead from this configuration or use the documented sandbox_permissions require_escalated approval flow; that flow cannot preserve DenyRead. "+ + "Remove DenyRead from this configuration or use the documented sandbox_permissions: \"require_escalated\" approval flow; that flow cannot preserve DenyRead. "+ "Configured DenyRead path count: %d", len(profile.FileSystem.DenyRead), ) diff --git a/internal/sandbox/windows_setup_test.go b/internal/sandbox/windows_setup_test.go index 0a3c6f044..251098a2e 100644 --- a/internal/sandbox/windows_setup_test.go +++ b/internal/sandbox/windows_setup_test.go @@ -63,6 +63,37 @@ func TestRunWindowsSandboxSetupRejectsInvalidArgs(t *testing.T) { } } +func TestRunWindowsSandboxSetupRejectsDenyReadUpfront(t *testing.T) { + home := t.TempDir() + args, err := BuildWindowsSandboxSetupArgs(WindowsSandboxSetupArgsOptions{ + SandboxHome: home, + CommandCWD: `C:\workspace\src`, + WorkspaceRoots: []string{`C:\workspace`}, + PermissionProfile: PermissionProfile{ + FileSystem: FileSystemPolicy{ + Kind: FileSystemRestricted, + WriteRoots: []WritableRoot{{Root: `C:\workspace`}}, + DenyRead: []string{`C:\workspace\secret`}, + }, + Network: NetworkPolicy{Mode: NetworkDeny}, + }, + }) + if err != nil { + t.Fatalf("BuildWindowsSandboxSetupArgs: %v", err) + } + var stderr bytes.Buffer + code := RunWindowsSandboxSetup(args, &stderr) + if code != 1 { + t.Fatalf("exit code = %d, want 1 for unsupported DenyRead", code) + } + if !strings.Contains(stderr.String(), "DenyRead is not supported") { + t.Fatalf("stderr = %q, want DenyRead unsupported rejection before elevation check", stderr.String()) + } + if strings.Contains(stderr.String(), "Administrator rights are required") { + t.Fatalf("stderr = %q, should not claim Administrator rights when DenyRead is unsupported", stderr.String()) + } +} + func TestWindowsSandboxSetupMarkerRefreshesWhenProfileChanges(t *testing.T) { config := WindowsSandboxSetupConfig{ SandboxHome: t.TempDir(), diff --git a/internal/sandbox/windows_setup_windows.go b/internal/sandbox/windows_setup_windows.go index d2c2d2f90..a71f78cae 100644 --- a/internal/sandbox/windows_setup_windows.go +++ b/internal/sandbox/windows_setup_windows.go @@ -10,6 +10,12 @@ import ( ) func runWindowsSandboxSetup(config WindowsSandboxSetupConfig, stderr io.Writer) int { + // Do not provision DenyRead ACLs for a token mode that cannot launch normal + // tools with DenyRead under the narrow restricting-SID set (PR #640). + if err := windowsDenyReadRestrictedTokenUnsupportedProfile(config.PermissionProfile); err != nil { + fmt.Fprintln(stderr, WindowsSandboxSetupName+": "+err.Error()) + return 1 + } // Applying the WFP network filters and workspace ACLs requires Administrator // rights; without them WFP fails deep inside with a raw ACCESS_DENIED (0x5). // Check up front and return an actionable message instead. @@ -17,12 +23,6 @@ func runWindowsSandboxSetup(config WindowsSandboxSetupConfig, stderr io.Writer) fmt.Fprintln(stderr, WindowsSandboxSetupName+": Administrator rights are required. Re-run `zero sandbox setup` from an elevated (Run as administrator) terminal.") return 1 } - // Do not provision DenyRead ACLs for a token mode that cannot launch normal - // tools with DenyRead under the narrow restricting-SID set (PR #640). - if err := windowsDenyReadRestrictedTokenUnsupportedProfile(config.PermissionProfile); err != nil { - fmt.Fprintln(stderr, WindowsSandboxSetupName+": "+err.Error()) - return 1 - } plan, err := BuildWindowsACLPlan(config.commandConfig()) if err != nil { fmt.Fprintln(stderr, WindowsSandboxSetupName+": "+err.Error()) From 90d056af9dcae13596c9bc85fe270915e1cc6a25 Mon Sep 17 00:00:00 2001 From: euxaristia Date: Tue, 1 Sep 2026 16:36:36 -0400 Subject: [PATCH 5/9] Reject unsupported DenyRead profiles upfront during sandbox setup Refs #640 --- internal/sandbox/windows_setup.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/internal/sandbox/windows_setup.go b/internal/sandbox/windows_setup.go index 3fc9634e8..be4cdd6c9 100644 --- a/internal/sandbox/windows_setup.go +++ b/internal/sandbox/windows_setup.go @@ -145,6 +145,10 @@ func RunWindowsSandboxSetup(args []string, stderr io.Writer) int { fmt.Fprintln(stderr, WindowsSandboxSetupName+": "+err.Error()) return 2 } + if err := windowsDenyReadRestrictedTokenUnsupportedProfile(config.PermissionProfile); err != nil { + fmt.Fprintln(stderr, WindowsSandboxSetupName+": "+err.Error()) + return 1 + } return runWindowsSandboxSetup(config, stderr) } From a84626f45a4397a4d29d40a3a859a0e730fb8ca4 Mon Sep 17 00:00:00 2001 From: euxaristia Date: Wed, 2 Sep 2026 04:33:51 -0400 Subject: [PATCH 6/9] Migrate legacy Windows DenyWrite ACEs, randomize smoke probes, and clean dead descendant machinery. Refs #640 --- .../runner_windows_integration_test.go | 102 ++- internal/sandbox/windows_acl.go | 12 - internal/sandbox/windows_acl_apply_windows.go | 364 ++++---- .../sandbox/windows_acl_apply_windows_test.go | 391 ++++---- internal/sandbox/windows_acl_descendants.go | 44 - .../sandbox/windows_acl_descendants_test.go | 45 - .../windows_acl_descendants_windows.go | 509 ----------- .../windows_acl_descendants_windows_test.go | 849 ------------------ internal/sandbox/windows_acl_paths_other.go | 34 - internal/sandbox/windows_acl_paths_windows.go | 44 - internal/sandbox/windows_acl_test.go | 75 +- internal/sandbox/windows_command_runner.go | 2 +- .../sandbox/windows_command_runner_test.go | 7 +- internal/sandbox/windows_setup.go | 2 +- internal/sandbox/windows_setup_test.go | 8 +- internal/sandbox/windows_unelevated.go | 2 +- 16 files changed, 444 insertions(+), 2046 deletions(-) delete mode 100644 internal/sandbox/windows_acl_descendants.go delete mode 100644 internal/sandbox/windows_acl_descendants_test.go delete mode 100644 internal/sandbox/windows_acl_descendants_windows.go delete mode 100644 internal/sandbox/windows_acl_descendants_windows_test.go delete mode 100644 internal/sandbox/windows_acl_paths_other.go delete mode 100644 internal/sandbox/windows_acl_paths_windows.go diff --git a/internal/sandbox/runner_windows_integration_test.go b/internal/sandbox/runner_windows_integration_test.go index 0ea0001c3..b34da8edf 100644 --- a/internal/sandbox/runner_windows_integration_test.go +++ b/internal/sandbox/runner_windows_integration_test.go @@ -82,13 +82,9 @@ func TestWindowsRestrictedTokenRealSandboxSmoke(t *testing.T) { if publicDir == "" { t.Log("PUBLIC is not set; skipping C:\\Users\\Public write-jail probe") } else { - publicMarker := filepath.Join(publicDir, "zero-elevated-write-denied.txt") - _ = os.Remove(publicMarker) - runWindowsRealSmokeCommand(t, runnerExe, config, []string{ - "cmd.exe", "/d", "/s", "/c", "echo leaked>" + publicMarker, - }, 1) - if _, err := os.Stat(publicMarker); err == nil { - _ = os.Remove(publicMarker) + publicProbe := allocateSharedDirectoryProbe(t, publicDir, "elevated-public") + runWindowsRealSmokeCommand(t, runnerExe, config, deniedWriteCommand(publicProbe.Path()), deniedWriteExitCode) + if _, err := os.Stat(publicProbe.Path()); err == nil { t.Fatalf("Windows sandbox allowed a write to the shared C:\\Users\\Public directory") } else if !os.IsNotExist(err) { t.Fatalf("stat public marker: %v", err) @@ -240,15 +236,9 @@ func TestWindowsUnelevatedRealSandboxSmoke(t *testing.T) { // Verify write to C:\ProgramData is blocked programData := os.Getenv("ProgramData") if programData != "" { - programDataMarker := filepath.Join(programData, "zero-unelevated-write-denied.txt") - _ = os.Remove(programDataMarker) - - runWindowsRealSmokeCommand(t, runnerExe, config, []string{ - "cmd.exe", "/d", "/s", "/c", "echo leaked>" + programDataMarker, - }, 1) - - if _, err := os.Stat(programDataMarker); err == nil { - _ = os.Remove(programDataMarker) + programDataProbe := allocateSharedDirectoryProbe(t, programData, "unelevated-programdata") + runWindowsRealSmokeCommand(t, runnerExe, config, deniedWriteCommand(programDataProbe.Path()), deniedWriteExitCode) + if _, err := os.Stat(programDataProbe.Path()); err == nil { t.Fatalf("unelevated sandbox allowed a write to ProgramData shared directory") } else if !os.IsNotExist(err) { t.Fatalf("stat ProgramData marker: %v", err) @@ -583,7 +573,49 @@ const deniedWriteExitCode = 77 // deniedWriteCommand attempts a write and reports deniedWriteExitCode when the // redirect is refused, so the exit code also proves cmd.exe actually ran. 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 cmdQuote(path string) string { + return `"` + strings.ReplaceAll(path, `"`, `\"`) + `"` +} + +type sharedDirectoryProbe struct { + path string +} + +func allocateSharedDirectoryProbe(t testing.TB, dir, prefix string) *sharedDirectoryProbe { + t.Helper() + if dir == "" { + t.Skip("shared directory path is not set") + } + var probePath string + for attempt := 0; attempt < 50; attempt++ { + candidate := filepath.Join(dir, fmt.Sprintf("zero-smoke-%s-%d-%d-%d.txt", prefix, os.Getpid(), time.Now().UnixNano(), attempt)) + if _, err := os.Lstat(candidate); os.IsNotExist(err) { + probePath = candidate + break + } + } + if probePath == "" { + t.Fatalf("allocate shared directory probe in %s: failed to find unused filename", dir) + } + p := &sharedDirectoryProbe{path: probePath} + t.Cleanup(func() { + p.cleanup(t) + }) + return p +} + +func (p *sharedDirectoryProbe) Path() string { + return p.path +} + +func (p *sharedDirectoryProbe) cleanup(t testing.TB) { + t.Helper() + if _, err := os.Lstat(p.path); err == nil { + _ = os.Remove(p.path) + } } func powershellSingleQuote(value string) string { @@ -597,3 +629,39 @@ func powershellSingleQuote(value string) string { } return out + "'" } + +func TestSharedDirectoryProbeLifecycle(t *testing.T) { + dir := t.TempDir() + + // 1. Two simultaneous allocations produce distinct non-colliding paths. + probe1 := allocateSharedDirectoryProbe(t, dir, "p1") + probe2 := allocateSharedDirectoryProbe(t, dir, "p2") + if probe1.Path() == probe2.Path() { + t.Fatalf("expected distinct probe paths, got %q and %q", probe1.Path(), probe2.Path()) + } + + // 2. Pre-existing unrelated file is never selected or deleted. + unrelatedFile := filepath.Join(dir, "unrelated.txt") + if err := os.WriteFile(unrelatedFile, []byte("preserve me"), 0o600); err != nil { + t.Fatalf("write unrelated file: %v", err) + } + probe3 := allocateSharedDirectoryProbe(t, dir, "p3") + probe3.cleanup(t) + if data, err := os.ReadFile(unrelatedFile); err != nil || string(data) != "preserve me" { + t.Fatalf("unrelated file was modified or deleted: data=%q, err=%v", data, err) + } + + // 3. Interrupted / no-create path: cleanup on absent file succeeds quietly. + probe4 := allocateSharedDirectoryProbe(t, dir, "p4") + probe4.cleanup(t) + + // 4. Unexpected write created during test is cleaned up. + probe5 := allocateSharedDirectoryProbe(t, dir, "p5") + if err := os.WriteFile(probe5.Path(), []byte("leaked"), 0o600); err != nil { + t.Fatalf("write probe5 file: %v", err) + } + probe5.cleanup(t) + if _, err := os.Lstat(probe5.Path()); !os.IsNotExist(err) { + t.Fatalf("expected probe5 to be cleaned up after creation, stat err=%v", err) + } +} diff --git a/internal/sandbox/windows_acl.go b/internal/sandbox/windows_acl.go index 982f60e76..96a18004a 100644 --- a/internal/sandbox/windows_acl.go +++ b/internal/sandbox/windows_acl.go @@ -35,18 +35,6 @@ type WindowsACLEntry struct { // flag rather than rely on inheritance. NoInherit bool `json:"noInherit,omitempty"` Materialize bool `json:"materialize,omitempty"` - // ScanDescendants marks a shared-root DenyWrite entry whose EXISTING - // writable descendants must ALSO be denied, one direct (non-inheriting) - // deny per writable descendant, at apply time. This flag and its associated - // walker are consumed by applyWindowsACLPlan and tests; no current - // plan-generation path sets this flag since broad shared-root DenyWrite ACEs - // and SID broadening are no longer planned. - ScanDescendants bool `json:"-"` - // RevokeDescendants marks a WindowsACLRevokeCapability entry as needing - // stale-deny cleanup applied recursively to existing descendants, not just - // the root path itself. This flag is consumed by applyWindowsACLPlan for - // migration cleanup and tests; no current plan-generation path sets this flag. - RevokeDescendants bool `json:"-"` } type WindowsACLPlan struct { diff --git a/internal/sandbox/windows_acl_apply_windows.go b/internal/sandbox/windows_acl_apply_windows.go index 2ebfb25e3..facd166ff 100644 --- a/internal/sandbox/windows_acl_apply_windows.go +++ b/internal/sandbox/windows_acl_apply_windows.go @@ -6,15 +6,22 @@ import ( "errors" "fmt" "os" - "path/filepath" "sort" "strings" + "unsafe" "golang.org/x/sys/windows" ) const windowsFileDeleteChild windows.ACCESS_MASK = 0x00000040 +const ( + windowsAccessAllowedObjectAceType = 0x5 + windowsAccessDeniedObjectAceType = 0x6 + windowsAccessAllowedCallbackAceType = 0x9 + windowsAccessAllowedCallbackObjectAceType = 0xB +) + type windowsACLPathGroup struct { Path string Entries []WindowsACLEntry @@ -29,7 +36,6 @@ type windowsACLSnapshot struct { func applyWindowsACLPlan(plan WindowsACLPlan) (func() error, error) { groups := groupWindowsACLPlanByPath(plan) - writeRoots := windowsPlanAllowWriteRoots(plan) snapshots := make([]windowsACLSnapshot, 0, len(groups)) for _, group := range groups { snapshot, applied, err := applyWindowsACLPathGroup(group) @@ -43,150 +49,12 @@ func applyWindowsACLPlan(plan WindowsACLPlan) (func() error, error) { if applied { snapshots = append(snapshots, snapshot) } - // A shared-root deny only protects the root object itself; its existing - // writable descendants each need their own direct deny (see - // windows_acl_descendants_windows.go). Only scan once the root deny - // actually applied (applied == true means the root exists). - if denySID, ok := windowsGroupScanDescendantsSID(group); ok && applied { - descendantSnapshots, err := applyWindowsSharedDescendantDenies(group.Path, denySID, writeRoots) - snapshots = append(snapshots, descendantSnapshots...) - if err != nil { - rollbackErr := rollbackWindowsACLSnapshots(snapshots) - if rollbackErr != nil { - return nil, fmt.Errorf("%w; rollback failed: %v", err, rollbackErr) - } - return nil, err - } - } - // A write root's stale-deny revoke only clears the root path itself; - // clear the same stale deny from its existing descendants too, or a - // stray direct deny an earlier run left there keeps winning over this - // root's new inheritable Allow (see windows_acl.go's RevokeDescendants - // doc and jatmn's review). Best-effort: leaving a stale deny in place - // only over-restricts an explicitly configured write root, it never - // widens access, so this never fails the whole plan apply. - if denySID, ok := windowsGroupRevokeDescendantsSID(group); ok && applied { - snapshots = append(snapshots, windowsRevokeStaleDescendantDenies(group.Path, denySID)...) - } } return func() error { return rollbackWindowsACLSnapshots(snapshots) }, nil } -// windowsPlanAllowWriteRoots collects the plan's allow-write root paths so the -// descendant scan can exclude a configured write root (and anything under it): -// a write root that happens to live under one of the shared roots must never be -// jailed by a compensating deny. -func windowsPlanAllowWriteRoots(plan WindowsACLPlan) []string { - var roots []string - for _, entry := range plan.Entries { - if entry.Action == WindowsACLAllowWrite { - if path := strings.TrimSpace(entry.Path); path != "" { - roots = append(roots, path) - } - } - } - return roots -} - -// windowsGroupScanDescendantsSID returns the deny SID of a group's shared-root -// DenyWrite entry when that entry requests descendant scanning. -func windowsGroupScanDescendantsSID(group windowsACLPathGroup) (string, bool) { - for _, entry := range group.Entries { - if entry.Action == WindowsACLDenyWrite && entry.ScanDescendants && strings.TrimSpace(entry.Capability) != "" { - return entry.Capability, true - } - } - return "", false -} - -// windowsGroupRevokeDescendantsSID returns the capability SID of a group's -// write-root stale-deny revoke entry when that entry requests clearing the -// same stale deny from the root's existing descendants too (see -// RevokeDescendants). -func windowsGroupRevokeDescendantsSID(group windowsACLPathGroup) (string, bool) { - for _, entry := range group.Entries { - if entry.Action == WindowsACLRevokeCapability && entry.RevokeDescendants && strings.TrimSpace(entry.Capability) != "" { - return entry.Capability, true - } - } - return "", false -} - -// windowsRevokeStaleDescendantDenies walks a newly-promoted write root's -// existing descendants and clears any direct DenyWrite ACE they carry for -// denySID — left over from when an earlier `zero sandbox setup` run found -// this same subtree writable by Users/Authenticated Users and applied the -// shared-root compensating deny (windows_acl_descendants_windows.go) before -// the caller configured this path as an allowed write root. That stale, -// non-inheriting deny on a descendant still wins over the root's own new, -// inheritable Allow under Windows' deny-before-allow ACE evaluation, so the -// root would otherwise remain partly unwritable — see jatmn's review. -// -// This is deliberately best-effort, not fail-closed like the writable- -// descendant scan: leaving a stray stale deny in place only over-restricts an -// explicitly configured write root (a functionality bug), it never widens -// access, so an unreadable descendant or a reparse point here is skipped -// rather than aborting the whole plan apply. Bounded by the same depth/entry -// caps as the writable-descendant scan so a pathological or cyclic tree -// cannot make this run unboundedly long. -func windowsRevokeStaleDescendantDenies(root, denySID string) []windowsACLSnapshot { - type node struct { - path string - depth int - } - var snapshots []windowsACLSnapshot - visited := 0 - queue := []node{{path: root, depth: 0}} - for len(queue) > 0 { - current := queue[0] - queue = queue[1:] - entries, err := os.ReadDir(current.path) - if err != nil { - continue - } - for _, entry := range entries { - child := filepath.Join(current.path, entry.Name()) - // Unlike the writable-descendant scan, this cleanup pass does not - // need to follow reparse points transparently: skipping one just - // means a stray deny under it might survive, which is the same - // safe-but-inconvenient outcome as any other skip here. - if windowsPathIsReparsePoint(child) { - continue - } - if visited >= windowsDescendantScanMaxDirs { - return snapshots - } - visited++ - // Revoking a SID with no matching ACE is a safe no-op. A pre-check - // keyed on complete write-deny coverage would also skip a partial - // stale deny that still blocks writes on a promoted write root. - snapshot, applied, err := applyWindowsACLPathGroup(windowsACLPathGroup{ - Path: child, - Entries: []WindowsACLEntry{{ - Action: WindowsACLRevokeCapability, - Path: child, - Capability: denySID, - NoInherit: true, - }}, - }) - if err == nil && applied { - snapshots = append(snapshots, snapshot) - } - if !entry.IsDir() { - continue - } - depth := current.depth + 1 - if depth >= windowsDescendantScanMaxDepth { - continue - } - queue = append(queue, node{path: child, depth: depth}) - } - } - return snapshots -} - func groupWindowsACLPlanByPath(plan WindowsACLPlan) []windowsACLPathGroup { byPath := map[string]*windowsACLPathGroup{} for _, entry := range dedupeWindowsACLEntries(plan.Entries) { @@ -263,13 +131,18 @@ func applyWindowsACLPathGroup(group windowsACLPathGroup) (windowsACLSnapshot, bo if err != nil { return fail(fmt.Errorf("read windows DACL for %s: %w", path, err)) } - accessEntries, err := windowsExplicitAccessEntries(group.Entries, isDir, oldDACL) + baseDACL, accessEntries, err := prepareWindowsACLPathGroupEntries(group.Entries, isDir, oldDACL) if err != nil { return fail(err) } - nextDACL, err := windows.ACLFromEntries(accessEntries, oldDACL) - if err != nil { - return fail(fmt.Errorf("build windows ACL for %s: %w", path, err)) + var nextDACL *windows.ACL + if len(accessEntries) > 0 { + nextDACL, err = windows.ACLFromEntries(accessEntries, baseDACL) + if err != nil { + return fail(fmt.Errorf("build windows ACL for %s: %w", path, err)) + } + } else { + nextDACL = baseDACL } if err := windows.SetSecurityInfo(handle, windows.SE_FILE_OBJECT, windows.DACL_SECURITY_INFORMATION, nil, nil, nextDACL, nil); err != nil { return fail(fmt.Errorf("apply windows ACL for %s: %w", path, err)) @@ -331,41 +204,45 @@ func windowsACLGroupRequiresExistingTarget(group windowsACLPathGroup) bool { return false } -func windowsExplicitAccessEntries(entries []WindowsACLEntry, isDir bool, oldDACL *windows.ACL) ([]windows.EXPLICIT_ACCESS, error) { - out := make([]windows.EXPLICIT_ACCESS, 0, len(entries)) +func prepareWindowsACLPathGroupEntries(entries []WindowsACLEntry, isDir bool, oldDACL *windows.ACL) (*windows.ACL, []windows.EXPLICIT_ACCESS, error) { + baseDACL := oldDACL + var out []windows.EXPLICIT_ACCESS for _, entry := range entries { sid, err := windows.StringToSid(entry.Capability) if err != nil { - return nil, fmt.Errorf("parse windows capability SID %q: %w", entry.Capability, err) + return nil, nil, fmt.Errorf("parse windows capability SID %q: %w", entry.Capability, err) } if entry.Action == WindowsACLRevokeCapability { - // Migration cleanup for hosts that ran experimental SID-broadening - // builds: strip the synthetic full DenyWrite ACE for this SID, but - // re-emit any co-resident DenyRead ACEs for the same stable SID so - // a concurrent profile's read boundary is not deleted (jatmn P1). - // SET_ACCESS with a zero mask clears every ACE for the trustee - // (REVOKE_ACCESS leaves DENY ACEs untouched empirically); the - // preserved read-deny entries that follow restore DenyRead only. - out = append(out, windows.EXPLICIT_ACCESS{ - AccessPermissions: 0, - AccessMode: windows.SET_ACCESS, - Inheritance: 0, - Trustee: windows.TRUSTEE{ - TrusteeForm: windows.TRUSTEE_IS_SID, - TrusteeType: windows.TRUSTEE_IS_GROUP, - TrusteeValue: windows.TrusteeValueFromSID(sid), - }, - }) - preserved, err := windowsPreservedReadDenyAccessEntries(oldDACL, sid, isDir) - if err != nil { - return nil, err + // Clear all explicit write-deny ACEs for sid from baseDACL while preserving any DenyRead + if baseDACL != nil { + filtered, err := windowsFilterDACL(baseDACL, sid) + if err != nil { + return nil, nil, err + } + baseDACL = filtered } - out = append(out, preserved...) continue } + 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 + } + } accessMode, permissions, err := windowsACLAccess(entry.Action) if err != nil { - return nil, err + return nil, nil, err } inheritance := uint32(0) if isDir && !entry.NoInherit { @@ -382,7 +259,135 @@ func windowsExplicitAccessEntries(entries []WindowsACLEntry, isDir bool, oldDACL }, }) } - return out, nil + return baseDACL, out, nil +} + +type windowsACLHeader struct { + AclRevision byte + Sbz1 byte + AclSize uint16 + AceCount uint16 + Sbz2 uint16 +} + +func windowsFilterDACL(oldDACL *windows.ACL, removeSID *windows.SID) (*windows.ACL, error) { + if oldDACL == nil || removeSID == nil { + return oldDACL, nil + } + var keepBytes uint32 = uint32(unsafe.Sizeof(windowsACLHeader{})) + var keepCount uint16 = 0 + for i := uint32(0); i < uint32(oldDACL.AceCount); i++ { + var ace *windows.ACCESS_ALLOWED_ACE + if err := windows.GetAce(oldDACL, i, &ace); err != nil { + return nil, fmt.Errorf("read ACE %d for filter: %w", i, err) + } + if ace.Header.AceFlags&windows.INHERITED_ACE == 0 { + if ace.Header.AceType == windows.ACCESS_DENIED_ACE_TYPE || ace.Header.AceType == windowsAccessDeniedObjectAceType { + if sid, ok := windowsAceSID(ace); ok && sid.Equals(removeSID) && windowsIsExperimentalWriteDenyMask(ace.Mask) { + continue + } + } + } + keepBytes += uint32(ace.Header.AceSize) + keepCount++ + } + + buf := make([]byte, keepBytes) + hdr := (*windowsACLHeader)(unsafe.Pointer(&buf[0])) + oldHdr := (*windowsACLHeader)(unsafe.Pointer(oldDACL)) + *hdr = *oldHdr + hdr.AclSize = uint16(keepBytes) + hdr.AceCount = keepCount + + offset := unsafe.Sizeof(windowsACLHeader{}) + for i := uint32(0); i < uint32(oldDACL.AceCount); i++ { + var ace *windows.ACCESS_ALLOWED_ACE + if err := windows.GetAce(oldDACL, i, &ace); err != nil { + return nil, fmt.Errorf("read ACE %d for copy: %w", i, err) + } + if ace.Header.AceFlags&windows.INHERITED_ACE == 0 { + if ace.Header.AceType == windows.ACCESS_DENIED_ACE_TYPE || ace.Header.AceType == windowsAccessDeniedObjectAceType { + if sid, ok := windowsAceSID(ace); ok && sid.Equals(removeSID) && windowsIsExperimentalWriteDenyMask(ace.Mask) { + continue + } + } + } + aceSize := uintptr(ace.Header.AceSize) + srcSlice := unsafe.Slice((*byte)(unsafe.Pointer(ace)), aceSize) + copy(buf[offset:offset+aceSize], srcSlice) + offset += aceSize + } + + return (*windows.ACL)(unsafe.Pointer(hdr)), nil +} + +// windowsMigrateDenyWriteInDACL copies oldDACL and narrows any explicit +// deny-write ACE for targetSID to the current narrow mask, preserving all +// other ACEs (including DenyRead) in their original positions. This avoids +// SetEntriesInAcl's merging behavior that would combine separate deny ACEs +// for the same SID into a single full-deny ACE. +func windowsMigrateDenyWriteInDACL(oldDACL *windows.ACL, targetSID *windows.SID) (*windows.ACL, error) { + if oldDACL == nil || targetSID == nil { + return oldDACL, nil + } + _, narrowMask, err := windowsACLAccess(WindowsACLDenyWrite) + if err != nil { + return nil, err + } + + oldHdr := (*windowsACLHeader)(unsafe.Pointer(oldDACL)) + buf := make([]byte, oldHdr.AclSize) + src := unsafe.Slice((*byte)(unsafe.Pointer(oldDACL)), oldHdr.AclSize) + copy(buf, src) + + newDACL := (*windows.ACL)(unsafe.Pointer(&buf[0])) + for i := uint32(0); i < uint32(newDACL.AceCount); i++ { + var ace *windows.ACCESS_ALLOWED_ACE + if err := windows.GetAce(newDACL, i, &ace); err != nil { + return nil, fmt.Errorf("read ACE %d for migration: %w", i, err) + } + if ace.Header.AceFlags&windows.INHERITED_ACE != 0 { + continue + } + if ace.Header.AceType != windows.ACCESS_DENIED_ACE_TYPE && ace.Header.AceType != windowsAccessDeniedObjectAceType { + continue + } + sid, ok := windowsAceSID(ace) + if !ok || !sid.Equals(targetSID) { + continue + } + if windowsIsExperimentalWriteDenyMask(ace.Mask) { + ace.Mask = narrowMask + } + } + + return newDACL, nil +} + +func windowsHasExplicitDenyWriteForSID(oldDACL *windows.ACL, wantSID *windows.SID) bool { + if oldDACL == nil || wantSID == nil { + return false + } + for index := uint16(0); index < oldDACL.AceCount; index++ { + var ace *windows.ACCESS_ALLOWED_ACE + if err := windows.GetAce(oldDACL, uint32(index), &ace); err != nil { + continue + } + if ace.Header.AceType != windows.ACCESS_DENIED_ACE_TYPE && ace.Header.AceType != windowsAccessDeniedObjectAceType { + continue + } + if ace.Header.AceFlags&windows.INHERITED_ACE != 0 { + continue + } + sid, ok := windowsAceSID(ace) + if !ok || !sid.Equals(wantSID) { + continue + } + if windowsIsExperimentalWriteDenyMask(ace.Mask) { + return true + } + } + return false } // windowsPreservedReadDenyAccessEntries returns DENY_ACCESS EXPLICIT_ACCESS @@ -459,6 +464,25 @@ func windowsIsExperimentalWriteDenyMask(mask windows.ACCESS_MASK) bool { return mask&writeContent != 0 } +func windowsAceSID(ace *windows.ACCESS_ALLOWED_ACE) (sid *windows.SID, ok bool) { + switch ace.Header.AceType { + case windows.ACCESS_ALLOWED_ACE_TYPE, windows.ACCESS_DENIED_ACE_TYPE, windowsAccessAllowedCallbackAceType: + return (*windows.SID)(unsafe.Pointer(&ace.SidStart)), true + case windowsAccessAllowedObjectAceType, windowsAccessDeniedObjectAceType, windowsAccessAllowedCallbackObjectAceType: + flags := ace.SidStart + offset := unsafe.Sizeof(ace.SidStart) + if flags&windows.ACE_OBJECT_TYPE_PRESENT != 0 { + offset += 16 + } + if flags&windows.ACE_INHERITED_OBJECT_TYPE_PRESENT != 0 { + offset += 16 + } + return (*windows.SID)(unsafe.Pointer(uintptr(unsafe.Pointer(&ace.SidStart)) + offset)), true + default: + return nil, false + } +} + func windowsACLAccess(action WindowsACLAction) (windows.ACCESS_MODE, windows.ACCESS_MASK, error) { switch action { case WindowsACLAllowWrite: diff --git a/internal/sandbox/windows_acl_apply_windows_test.go b/internal/sandbox/windows_acl_apply_windows_test.go index 31910bb47..a772d7c2e 100644 --- a/internal/sandbox/windows_acl_apply_windows_test.go +++ b/internal/sandbox/windows_acl_apply_windows_test.go @@ -48,245 +48,6 @@ func TestApplyWindowsACLPathGroupHandleBasedRoundTrip(t *testing.T) { } } -// TestApplyWindowsACLPathGroupRevokeCapabilityRemovesStaleDeny is the -// real-Windows regression for jatmn's P2 finding: promoting a path to an -// allowed write root must also remove a stale deny ACE an earlier setup -// round left there for the stable capability SID, not merely omit it from -// this plan. Without the fix, applyWindowsACLPlan's SetEntriesInAcl-based -// merge only touches trustees actually named in the new entry list, so an -// old DenyWrite ACE for a SID the new plan does not mention would survive -// and keep winning over the new Allow under deny-before-allow evaluation. -func TestApplyWindowsACLPathGroupRevokeCapabilityRemovesStaleDeny(t *testing.T) { - // The stale/allow SIDs must be synthetic identities the test process itself - // is not a member of (exactly like the real stable capability SIDs - // LoadOrCreateWindowsCapabilitySIDs mints): a WindowsACLDenyWrite mask - // includes WRITE_DAC/WRITE_OWNER/DELETE, so denying a well-known group the - // test process actually belongs to (e.g. Everyone, BUILTIN\Users) would - // lock the test out of managing — and t.TempDir() out of cleaning up — - // its own fixture. - caps, err := LoadOrCreateWindowsCapabilitySIDs(t.TempDir()) - if err != nil { - t.Fatalf("LoadOrCreateWindowsCapabilitySIDs: %v", err) - } - otherCaps, err := LoadOrCreateWindowsCapabilitySIDs(t.TempDir()) - if err != nil { - t.Fatalf("LoadOrCreateWindowsCapabilitySIDs (other): %v", err) - } - staleSID := caps.ReadOnly - allowSID := otherCaps.ReadOnly - - dir := t.TempDir() - // Simulate the stale deny an earlier setup round applied while this path - // was still covered by the shared-root/descendant DenyWrite mitigation. - if _, _, err := applyWindowsACLPathGroup(windowsACLPathGroup{ - Path: dir, - Entries: []WindowsACLEntry{{ - Action: WindowsACLDenyWrite, - Path: dir, - Capability: staleSID, - NoInherit: true, - }}, - }); err != nil { - t.Fatalf("apply stale deny: %v", err) - } - if !dirDeniesSID(t, dir, staleSID) { - t.Fatalf("test fixture bug: %q does not carry the stale deny it is supposed to", dir) - } - - // Now promote dir to a write root: the plan carries an Allow for a - // different SID plus the reconciling revoke for the stale one. - if _, _, err := applyWindowsACLPathGroup(windowsACLPathGroup{ - Path: dir, - Entries: []WindowsACLEntry{ - {Action: WindowsACLAllowWrite, Path: dir, Capability: allowSID}, - {Action: WindowsACLRevokeCapability, Path: dir, Capability: staleSID, NoInherit: true}, - }, - }); err != nil { - t.Fatalf("apply promotion to write root: %v", err) - } - if dirDeniesSID(t, dir, staleSID) { - t.Fatalf("%q still carries the stale deny for %q after promotion to a write root", dir, staleSID) - } -} - -// TestApplyWindowsACLRevokePreservesDenyRead pins that migration revoke for a -// promoted write root removes only the experimental DenyWrite ACE for the -// stable SID and leaves a co-resident DenyRead for the same SID intact — so a -// concurrent profile's read boundary is not deleted (jatmn P1). -func TestApplyWindowsACLRevokePreservesDenyRead(t *testing.T) { - // Synthetic capability SID (not a group this process is in) so DenyWrite's - // WRITE_DAC/DELETE bits do not lock the test out of its own temp dir. - caps, err := LoadOrCreateWindowsCapabilitySIDs(t.TempDir()) - if err != nil { - t.Fatalf("LoadOrCreateWindowsCapabilitySIDs: %v", err) - } - sid := caps.ReadOnly - dir := t.TempDir() - - // Profile A shape plus experimental broadening: both DenyRead and full - // DenyWrite for the same stable SID on one path (two ACEs in one apply so - // a second DENY_ACCESS merge cannot replace the first). - if _, _, err := applyWindowsACLPathGroup(windowsACLPathGroup{ - Path: dir, - Entries: []WindowsACLEntry{ - { - Action: WindowsACLDenyRead, - Path: dir, - Capability: sid, - NoInherit: true, - }, - { - Action: WindowsACLDenyWrite, - Path: dir, - Capability: sid, - NoInherit: true, - }, - }, - }); err != nil { - t.Fatalf("apply DenyRead+DenyWrite: %v", err) - } - writeDenied, err := windowsPathDeniesCapabilitySID(dir, sid) - if err != nil { - t.Fatalf("windowsPathDeniesCapabilitySID before: %v", err) - } - if !writeDenied { - t.Fatal("fixture: expected full write deny present before revoke") - } - if !dirDeniesReadSID(t, dir, sid) { - t.Fatal("fixture: expected read deny present before revoke") - } - - // Profile B promotes dir to a write root: revoke stale write deny only. - if _, _, err := applyWindowsACLPathGroup(windowsACLPathGroup{ - Path: dir, - Entries: []WindowsACLEntry{{ - Action: WindowsACLRevokeCapability, - Path: dir, - Capability: sid, - NoInherit: true, - }}, - }); err != nil { - t.Fatalf("revoke: %v", err) - } - writeDenied, err = windowsPathDeniesCapabilitySID(dir, sid) - if err != nil { - t.Fatalf("windowsPathDeniesCapabilitySID after: %v", err) - } - if writeDenied { - t.Fatal("write deny for SID still present after migration revoke") - } - if !dirDeniesReadSID(t, dir, sid) { - t.Fatal("DenyRead for same SID was removed by migration revoke; read boundary must be preserved") - } -} - -// TestApplyWindowsACLPlanRevokeDescendantsClearsChildDeny is the regression for -// the RevokeDescendants walk: promoting a root to a write root must clear a -// stale direct deny an earlier run left on an existing child, not only the root -// path itself. Also pins reparse skip and the depth cap as best-effort bounds. -func TestApplyWindowsACLPlanRevokeDescendantsClearsChildDeny(t *testing.T) { - caps, err := LoadOrCreateWindowsCapabilitySIDs(t.TempDir()) - if err != nil { - t.Fatalf("LoadOrCreateWindowsCapabilitySIDs: %v", err) - } - staleSID := caps.ReadOnly - allowSID, err := LoadOrCreateWindowsCapabilitySIDs(t.TempDir()) - if err != nil { - t.Fatalf("LoadOrCreateWindowsCapabilitySIDs (allow): %v", err) - } - - root := t.TempDir() - child := mkdir(t, filepath.Join(root, "child")) - deep := mkdir(t, filepath.Join(child, "deep")) - // Stale full DenyWrite on the direct child (the primary cleanup target). - if _, _, err := applyWindowsACLPathGroup(windowsACLPathGroup{ - Path: child, - Entries: []WindowsACLEntry{{ - Action: WindowsACLDenyWrite, - Path: child, - Capability: staleSID, - NoInherit: true, - }}, - }); err != nil { - t.Fatalf("apply stale deny on child: %v", err) - } - // Partial write deny on deep: the old complete-coverage pre-check would - // skip this; unconditional revoke must still clear it. - denyCapabilityMask(t, deep, staleSID, windows.FILE_WRITE_ATTRIBUTES) - if !dirDeniesSID(t, child, staleSID) { - t.Fatal("fixture: child missing full stale deny") - } - if denyACECountForSID(t, deep, staleSID) == 0 { - t.Fatal("fixture: deep missing partial stale deny") - } - - // Junction under root: walker must skip it (best-effort) without failing. - juncTarget := mkdir(t, filepath.Join(t.TempDir(), "junc-target")) - junc := filepath.Join(root, "junc") - if out, err := exec.Command("cmd", "/c", "mklink", "/J", junc, juncTarget).CombinedOutput(); err != nil { - t.Logf("skipping junction sub-check (mklink /J unavailable): %v %s", err, strings.TrimSpace(string(out))) - } else { - t.Cleanup(func() { _ = os.Remove(junc) }) - if !windowsPathIsReparsePoint(junc) { - t.Fatalf("fixture: %q is not a reparse point", junc) - } - } - - cleanup, err := applyWindowsACLPlan(WindowsACLPlan{Entries: []WindowsACLEntry{ - {Action: WindowsACLAllowWrite, Path: root, Capability: allowSID.ReadOnly}, - { - Action: WindowsACLRevokeCapability, - Path: root, - Capability: staleSID, - NoInherit: true, - RevokeDescendants: true, - }, - }}) - if err != nil { - t.Fatalf("applyWindowsACLPlan: %v", err) - } - t.Cleanup(func() { _ = cleanup() }) - - if dirDeniesSID(t, child, staleSID) { - t.Fatalf("%q still carries full stale deny after RevokeDescendants", child) - } - if denyACECountForSID(t, deep, staleSID) != 0 { - t.Fatalf("%q still carries partial stale deny after RevokeDescendants", deep) - } - - // Depth cap: with max depth 1 the walker revokes root's children but does - // not enqueue them, so a nested deny under a new root is left in place. - cappedRoot := t.TempDir() - cappedChild := mkdir(t, filepath.Join(cappedRoot, "level1")) - cappedDeep := mkdir(t, filepath.Join(cappedChild, "level2")) - if _, _, err := applyWindowsACLPathGroup(windowsACLPathGroup{ - Path: cappedDeep, - Entries: []WindowsACLEntry{{ - Action: WindowsACLDenyWrite, - Path: cappedDeep, - Capability: staleSID, - NoInherit: true, - }}, - }); err != nil { - t.Fatalf("apply stale deny on capped deep: %v", err) - } - oldDepth := windowsDescendantScanMaxDepth - windowsDescendantScanMaxDepth = 1 - t.Cleanup(func() { windowsDescendantScanMaxDepth = oldDepth }) - if _, err := applyWindowsACLPlan(WindowsACLPlan{Entries: []WindowsACLEntry{{ - Action: WindowsACLRevokeCapability, - Path: cappedRoot, - Capability: staleSID, - NoInherit: true, - RevokeDescendants: true, - }}}); err != nil { - t.Fatalf("applyWindowsACLPlan (depth cap): %v", err) - } - if !dirDeniesSID(t, cappedDeep, staleSID) { - t.Fatal("depth cap should leave level2 deny in place when max depth is 1") - } -} - // dirDeniesReadSID reports whether path's DACL has a DENY ACE for wantSID whose // mask covers FILE_GENERIC_READ (DenyRead shape) without the full write-probe // mask of experimental DenyWrite. @@ -422,3 +183,155 @@ func TestOpenWindowsACLTargetReportsIsDir(t *testing.T) { t.Fatal("isDir = true for a regular file, want false") } } + +// TestWindowsACLDenyWriteMigratesLegacySynchronizeMask regression tests that an +// existing legacy DenyWrite ACE containing SYNCHRONIZE (from older PR builds) is +// replaced in-place with the narrow mask that excludes SYNCHRONIZE, preserving +// co-resident DenyRead ACEs and operating idempotently. +func TestWindowsACLDenyWriteMigratesLegacySynchronizeMask(t *testing.T) { + dir := t.TempDir() + childDir := filepath.Join(dir, "sub") + if err := os.Mkdir(childDir, 0o755); err != nil { + t.Fatalf("mkdir childDir: %v", err) + } + childFile := filepath.Join(childDir, "child.txt") + if err := os.WriteFile(childFile, []byte("data"), 0o644); err != nil { + t.Fatalf("write childFile: %v", err) + } + + caps, err := LoadOrCreateWindowsCapabilitySIDs(t.TempDir()) + if err != nil { + t.Fatalf("LoadOrCreateWindowsCapabilitySIDs: %v", err) + } + sidStr := caps.ReadOnly + sid, err := windows.StringToSid(sidStr) + if err != nil { + t.Fatalf("StringToSid: %v", err) + } + + // 1. Seed legacy DenyWrite ACE containing SYNCHRONIZE + co-resident DenyRead ACE. + legacyWriteMask := (windows.FILE_GENERIC_WRITE | windows.DELETE | windowsFileDeleteChild | windows.WRITE_DAC | windows.WRITE_OWNER | windows.SYNCHRONIZE) + _, readMask, err := windowsACLAccess(WindowsACLDenyRead) + if err != nil { + t.Fatalf("windowsACLAccess DenyRead: %v", err) + } + seedEntries := []windows.EXPLICIT_ACCESS{ + { + AccessPermissions: legacyWriteMask, + AccessMode: windows.DENY_ACCESS, + Inheritance: windows.SUB_CONTAINERS_AND_OBJECTS_INHERIT, + Trustee: windows.TRUSTEE{ + TrusteeForm: windows.TRUSTEE_IS_SID, + TrusteeType: windows.TRUSTEE_IS_GROUP, + TrusteeValue: windows.TrusteeValueFromSID(sid), + }, + }, + { + AccessPermissions: readMask, + AccessMode: windows.DENY_ACCESS, + Inheritance: windows.SUB_CONTAINERS_AND_OBJECTS_INHERIT, + Trustee: windows.TRUSTEE{ + TrusteeForm: windows.TRUSTEE_IS_SID, + TrusteeType: windows.TRUSTEE_IS_GROUP, + TrusteeValue: windows.TrusteeValueFromSID(sid), + }, + }, + } + handle, _, err := openWindowsACLTarget(dir) + if err != nil { + t.Fatalf("openWindowsACLTarget: %v", err) + } + seededDACL, err := windows.ACLFromEntries(seedEntries, nil) + if err != nil { + _ = windows.CloseHandle(handle) + t.Fatalf("ACLFromEntries: %v", err) + } + if err := windows.SetSecurityInfo(handle, windows.SE_FILE_OBJECT, windows.DACL_SECURITY_INFORMATION, nil, nil, seededDACL, nil); err != nil { + _ = windows.CloseHandle(handle) + t.Fatalf("SetSecurityInfo: %v", err) + } + _ = windows.CloseHandle(handle) + + // 2. Apply WindowsACLPlan with new narrow DenyWrite action. + plan := WindowsACLPlan{ + Entries: []WindowsACLEntry{{ + Action: WindowsACLDenyWrite, + Path: dir, + Capability: sidStr, + }}, + } + rollback, err := applyWindowsACLPlan(plan) + if err != nil { + t.Fatalf("applyWindowsACLPlan: %v", err) + } + t.Cleanup(func() { _ = rollback() }) + + // 3. Verify effective DACL: SYNCHRONIZE must NOT be denied, write rights denied, DenyRead preserved. + sd, err := windows.GetNamedSecurityInfo(dir, windows.SE_FILE_OBJECT, windows.DACL_SECURITY_INFORMATION) + if err != nil { + t.Fatalf("GetNamedSecurityInfo: %v", err) + } + dacl, _, err := sd.DACL() + if err != nil { + t.Fatalf("DACL: %v", err) + } + + var hasNarrowWriteDeny, hasSynchronizeDeny, hasReadDeny bool + _, narrowWriteMask, err := windowsACLAccess(WindowsACLDenyWrite) + if err != nil { + t.Fatalf("windowsACLAccess: %v", err) + } + for i := uint16(0); i < dacl.AceCount; i++ { + var ace *windows.ACCESS_ALLOWED_ACE + if err := windows.GetAce(dacl, uint32(i), &ace); err != nil { + t.Fatalf("GetAce: %v", err) + } + if ace.Header.AceType != windows.ACCESS_DENIED_ACE_TYPE && ace.Header.AceType != windowsAccessDeniedObjectAceType { + continue + } + aceSID, ok := windowsAceSID(ace) + if !ok || !aceSID.Equals(sid) { + continue + } + if ace.Mask&windows.SYNCHRONIZE != 0 && windowsIsExperimentalWriteDenyMask(ace.Mask) { + hasSynchronizeDeny = true + } + if ace.Mask&narrowWriteMask == narrowWriteMask { + hasNarrowWriteDeny = true + } + if ace.Mask&readMask == readMask && !windowsIsExperimentalWriteDenyMask(ace.Mask) { + hasReadDeny = true + } + } + + if hasSynchronizeDeny { + t.Fatal("resulting DACL still denies SYNCHRONIZE for trustee; migration failed to narrow mask") + } + if !hasNarrowWriteDeny { + t.Fatal("resulting DACL is missing narrow DenyWrite ACE") + } + if !hasReadDeny { + t.Fatal("resulting DACL lost co-resident DenyRead ACE during migration") + } + + // 4. Assert synchronous directory read works. + if entries, err := os.ReadDir(dir); err != nil || len(entries) == 0 { + t.Fatalf("os.ReadDir failed on migrated directory: entries=%v, err=%v", entries, err) + } + + // 5. Assert second apply is idempotent. + if _, err := applyWindowsACLPlan(plan); err != nil { + t.Fatalf("second applyWindowsACLPlan failed: %v", err) + } + sd2, err := windows.GetNamedSecurityInfo(dir, windows.SE_FILE_OBJECT, windows.DACL_SECURITY_INFORMATION) + if err != nil { + t.Fatalf("GetNamedSecurityInfo 2: %v", err) + } + dacl2, _, err := sd2.DACL() + if err != nil { + t.Fatalf("DACL 2: %v", err) + } + if dacl2.AceCount != dacl.AceCount { + t.Fatalf("second apply changed ACE count: %d vs %d", dacl2.AceCount, dacl.AceCount) + } +} diff --git a/internal/sandbox/windows_acl_descendants.go b/internal/sandbox/windows_acl_descendants.go deleted file mode 100644 index 0bdd59e4b..000000000 --- a/internal/sandbox/windows_acl_descendants.go +++ /dev/null @@ -1,44 +0,0 @@ -package sandbox - -import "strings" - -// Shared basename policies and pure helpers for the Windows descendant-scan -// fail-closed rules. The Win32 walk lives in windows_acl_descendants_windows.go; -// these helpers are compiled on every GOOS so non-Windows tests can pin the -// policy without the Windows APIs. - -// windowsDescendantScanSystemLockedNames are basenames that Windows keeps -// exclusive to SYSTEM (or otherwise unreadable even to elevated Administrators -// without taking ownership). They appear under every fixed volume root. Listing -// or DACL-reading them fails on healthy machines; treating that as incomplete -// coverage would make DenyRead setup fail everywhere. They never grant -// BUILTIN\Users / Authenticated Users write in stock configuration. -var windowsDescendantScanSystemLockedNames = map[string]struct{}{ - "system volume information": {}, - "$recycle.bin": {}, - "recovery": {}, -} - -func windowsDescendantScanNameIsSystemLocked(name string) bool { - _, ok := windowsDescendantScanSystemLockedNames[strings.ToLower(strings.TrimSpace(name))] - return ok -} - -// windowsPathIsDriveRootPath reports whether path is exactly a drive letter -// root such as "C:\" or "C:" (case-insensitively), with no further path -// segments. Used to scope the windowsDescendantScanNameIsSystemLocked -// exception to the one place those basenames are ever legitimately the real, -// SYSTEM-exclusive Windows directory: directly under an actual volume root. -// A directory sharing one of those basenames anywhere else in the tree (e.g. -// nested under ProgramData or Public, whether by installer accident or -// deliberately) is not the real thing and must not be silently skipped — see -// jatmn's review. Pure string check so non-Windows tests can pin it without -// Win32 or filepath's platform-dependent volume parsing. -func windowsPathIsDriveRootPath(path string) bool { - trimmed := strings.TrimSuffix(strings.TrimSpace(path), `\`) - if len(trimmed) != 2 || trimmed[1] != ':' { - return false - } - c := trimmed[0] - return (c >= 'A' && c <= 'Z') || (c >= 'a' && c <= 'z') -} diff --git a/internal/sandbox/windows_acl_descendants_test.go b/internal/sandbox/windows_acl_descendants_test.go deleted file mode 100644 index fdb800c15..000000000 --- a/internal/sandbox/windows_acl_descendants_test.go +++ /dev/null @@ -1,45 +0,0 @@ -package sandbox - -import "testing" - -func TestWindowsDescendantScanNamePolicies(t *testing.T) { - for _, name := range []string{ - "System Volume Information", - "SYSTEM VOLUME INFORMATION", - "$Recycle.Bin", - "Recovery", - } { - if !windowsDescendantScanNameIsSystemLocked(name) { - t.Fatalf("windowsDescendantScanNameIsSystemLocked(%q) = false, want true", name) - } - } - for _, name := range []string{"ProgramData", "plain", "Users"} { - if windowsDescendantScanNameIsSystemLocked(name) { - t.Fatalf("windowsDescendantScanNameIsSystemLocked(%q) = true, want false", name) - } - } -} - -// TestWindowsPathIsDriveRootPath pins the canonical-root-level scoping fix -// (jatmn's review): the system-locked basename allowlist must only fire -// directly under a genuine drive letter root, never at an arbitrary nested -// path that merely shares the same parent-relative shape. -func TestWindowsPathIsDriveRootPath(t *testing.T) { - for _, path := range []string{`C:\`, `C:`, `c:\`, `Z:\`} { - if !windowsPathIsDriveRootPath(path) { - t.Fatalf("windowsPathIsDriveRootPath(%q) = false, want true", path) - } - } - for _, path := range []string{ - `C:\ProgramData`, - `C:\Users\Public`, - `C:\Windows\Temp`, - ``, - `\\?\Volume{guid}\`, - `relative`, - } { - if windowsPathIsDriveRootPath(path) { - t.Fatalf("windowsPathIsDriveRootPath(%q) = true, want false", path) - } - } -} diff --git a/internal/sandbox/windows_acl_descendants_windows.go b/internal/sandbox/windows_acl_descendants_windows.go deleted file mode 100644 index eabc3d45a..000000000 --- a/internal/sandbox/windows_acl_descendants_windows.go +++ /dev/null @@ -1,509 +0,0 @@ -//go:build windows - -package sandbox - -import ( - "fmt" - "os" - "path/filepath" - "strings" - "unsafe" - - "golang.org/x/sys/windows" -) - -// Shared-root DenyWrite and Users/Authenticated Users SID broadening are no -// longer shipped: BuildWindowsACLPlan does not stamp machine-wide denies on -// C:\, %ProgramData%, %SystemRoot%\Temp, or C:\Users\Public, and the restricted -// token is never broadened with those groups. This file keeps the descendant -// walker and related helpers for (1) migration cleanup of stale denies left by -// earlier PR builds and (2) Windows regression tests of that walker. -// -// Historical coverage rules the walker still implements (fail closed when used): -// -// - Every directory under a root is considered for descent within the -// depth/entry caps, whether or not the parent itself is Users-writable. -// - Hitting windowsDescendantScanMaxDepth or windowsDescendantScanMaxDirs -// means unexamined territory remains: the scan returns an error rather than -// certifying a partial walk as clean. -// - Reparse points (junctions, symlinks, volume mount points) use a -// no-follow, identity-aware policy (see jatmn's review). The walker: -// 1. Detects reparse points via FILE_ATTRIBUTE_REPARSE_POINT (and the -// ModeSymlink/ModeIrregular bits ReadDir already reports) and never -// inspects their DACL, never applies a deny, and never descends. -// 2. Records the (volume serial, file index) identity of every real -// directory it does enter, so an alternate path to the same object is -// skipped rather than re-enumerated. -// - A directory this process cannot list, or a child whose DACL it cannot -// read, is fail-closed UNLESS the basename is a known SYSTEM-exclusive -// Windows directory (e.g. "System Volume Information") AND it sits -// directly under an actual drive letter root (windowsPathIsDriveRootPath). -// -// Basename policies live in windows_acl_descendants.go so non-Windows tests can -// pin them without Win32. Bounds are vars so Windows tests can lower them. -var ( - windowsDescendantScanMaxDepth = 48 - // windowsDescendantScanMaxDirs bounds the total files+directories the scan - // will inspect below a single root. Kept large enough for stock system - // trees while still bounding worst-case work; the walker is retained for - // migration cleanup and tests, not live elevated DenyRead setup. - windowsDescendantScanMaxDirs = 500000 -) - -// windowsBroadenedWriteProbeMask is the set of access-mask bits that let a -// principal create, delete, or modify content, attributes, or extended -// attributes in (or the security of) a directory, i.e. the bits that make a -// directory a usable write-jail escape. FILE_WRITE_DATA is FILE_ADD_FILE and -// FILE_APPEND_DATA is FILE_ADD_SUBDIRECTORY for a directory object. -const windowsBroadenedWriteProbeMask windows.ACCESS_MASK = (windows.FILE_GENERIC_WRITE | - windowsFileDeleteChild | - windows.DELETE | - windows.WRITE_DAC | - windows.WRITE_OWNER) &^ windows.SYNCHRONIZE - -// applyWindowsSharedDescendantDenies enumerates the existing writable -// descendants of a shared root and applies a direct, non-inheriting DenyWrite -// (naming denySID, the same stable read-only capability SID the root deny uses) -// to each. It returns every snapshot it applied (including on error) so the -// caller can roll the whole apply back. A descendant it identified as writable -// but could not deny is a hole it cannot close, so that failure is returned -// (fail closed). An incomplete enumeration (caps, unreadable non-reparse child) -// is also returned as an error. Reparse points are skipped by the enumerator -// (no-follow). Descendants that already carry a complete write deny for -// denySID are left untouched so setup reruns and command-time revalidation do -// not accumulate duplicate permanent ACEs. -func applyWindowsSharedDescendantDenies(root, denySID string, writeRoots []string) ([]windowsACLSnapshot, error) { - descendants, err := windowsEnumerateWritableDescendants(root, writeRoots) - if err != nil { - return nil, fmt.Errorf("enumerate writable descendants of %s: %w", root, err) - } - snapshots := make([]windowsACLSnapshot, 0, len(descendants)) - for _, dir := range descendants { - denied, err := windowsPathDeniesCapabilitySID(dir, denySID) - if err != nil { - return snapshots, fmt.Errorf("inspect existing deny on %s: %w", dir, err) - } - if denied { - continue - } - snapshot, applied, err := applyWindowsACLPathGroup(windowsACLPathGroup{ - Path: dir, - Entries: []WindowsACLEntry{{ - Action: WindowsACLDenyWrite, - Path: dir, - Capability: denySID, - NoInherit: true, - }}, - }) - if err != nil { - return snapshots, fmt.Errorf("deny writable descendant %s: %w", dir, err) - } - if applied { - snapshots = append(snapshots, snapshot) - } - } - return snapshots, nil -} - -// windowsEnumerateWritableDescendants returns the existing files and -// directories below root that grant BUILTIN\Users or Authenticated Users -// write, excluding any configured write root (and anything under it) so -// legitimate workspace writes are never jailed. Files are checked and denied -// just like directories — a writable file directly under a shared root is as -// much an escape surface as a writable directory — but only directories are -// descended into. -// -// Fail closed: exhausting the depth or entry caps, or failing to list/inspect -// a non-allowlisted, non-reparse entry returns an error rather than a partial -// success the caller could mistake for complete coverage. Reparse points are -// skipped (no-follow), not treated as incomplete coverage: their targets are -// reached through the real path when it lies under the same root. -func windowsEnumerateWritableDescendants(root string, writeRoots []string) ([]string, error) { - if windowsCapabilityPathKey(root) == "" { - return nil, nil - } - excluded := make([]string, 0, len(writeRoots)) - for _, writeRoot := range writeRoots { - if key := windowsCapabilityPathKey(writeRoot); key != "" { - excluded = append(excluded, key) - } - } - isExcluded := func(key string) bool { - for _, prefix := range excluded { - if key == prefix || strings.HasPrefix(key, prefix+`\`) { - return true - } - } - return false - } - - type node struct { - path string - depth int - } - var out []string - visited := 0 - // seenIDs records real directory object identities already entered so an - // alternate path to the same object is not re-enumerated (identity-aware - // half of the reparse policy). - seenIDs := make(map[windowsFileObjectID]struct{}) - queue := []node{{path: root, depth: 0}} - for len(queue) > 0 { - current := queue[0] - queue = queue[1:] - // Defensive: never list through a reparse path that somehow reached - // the queue (root is expected to be a real directory). - if windowsPathIsReparsePoint(current.path) { - continue - } - if id, ok := windowsFileObjectIdentity(current.path); ok { - if _, seen := seenIDs[id]; seen { - continue - } - seenIDs[id] = struct{}{} - } - entries, err := os.ReadDir(current.path) - if err != nil { - if windowsPathIsDriveRootPath(filepath.Dir(current.path)) && windowsDescendantScanNameIsSystemLocked(filepath.Base(current.path)) { - continue - } - return nil, fmt.Errorf("list descendants of %s: %w", current.path, err) - } - for _, entry := range entries { - child := filepath.Join(current.path, entry.Name()) - childKey := windowsCapabilityPathKey(child) - if isExcluded(childKey) { - continue - } - // No-follow: stock compatibility junctions (and any other reparse - // point) are never DACL-inspected, denied, or descended. Mode bits - // catch what ReadDir already classified; GetFileAttributes covers - // any reparse form those bits miss. - isReparse := (entry.Type()&os.ModeSymlink != 0) || (entry.Type()&os.ModeIrregular != 0) || windowsPathIsReparsePoint(child) - if isReparse { - if visited >= windowsDescendantScanMaxDirs { - return nil, fmt.Errorf("descendant scan exceeded %d entries below %s", windowsDescendantScanMaxDirs, root) - } - visited++ - continue - } - if visited >= windowsDescendantScanMaxDirs { - return nil, fmt.Errorf("descendant scan exceeded %d entries below %s", windowsDescendantScanMaxDirs, root) - } - visited++ - writable, err := windowsDirGrantsBroadenedWrite(child) - if err != nil { - // Same canonical-root-level scoping as the ReadDir case above: - // current.path (child's parent) must itself be a drive root for - // this to be the real, SYSTEM-exclusive directory. - if windowsPathIsDriveRootPath(current.path) && windowsDescendantScanNameIsSystemLocked(entry.Name()) { - continue - } - return nil, fmt.Errorf("inspect DACL of %s: %w", child, err) - } - if writable { - out = append(out, child) - } - if !entry.IsDir() { - continue - } - childDepth := current.depth + 1 - if childDepth >= windowsDescendantScanMaxDepth { - // A directory at the depth cap may still have unexamined - // children. Fail closed rather than pretend the subtree is clean. - // Leaf files at this depth were already inspected above. - // Only fail when we would have needed to descend further: always - // report the cap so callers cannot certify "complete". - return nil, fmt.Errorf("descendant scan exceeded depth %d at %s", windowsDescendantScanMaxDepth, child) - } - // Always descend (subject to caps), including through non-writable - // ancestors and stock system trees (Windows, Program Files, ...), so - // a deep writable child is not missed. A non-writable directory's OWN - // DACL says nothing about a descendant several levels down: an - // installer-created child with a loosened, non-inherited grant (e.g. - // C:\Users\shared) is exactly the escape this scan exists to find, and - // certifying a subtree clean from its root DACL alone would miss it - // (see jatmn's review). There is deliberately no basename-based - // shortcut here anymore — hitting windowsDescendantScanMaxDepth or - // windowsDescendantScanMaxDirs on a genuinely huge stock tree fails - // the scan closed (see the caller), which keeps the narrow SID set - // rather than certifying an unexamined subtree as safe. - queue = append(queue, node{path: child, depth: childDepth}) - } - } - return out, nil -} - -// windowsFileObjectID is the NTFS object identity used to detect that two -// paths name the same directory (volume serial + 64-bit file index). -type windowsFileObjectID struct { - volume uint32 - index uint64 -} - -// windowsFileObjectIdentity returns the on-disk identity of path when it can -// be opened as a real (non-reparse) directory. ok is false on any open/inspect -// failure so the walker falls through to path-based enumeration rather than -// treating an unreadable directory as already-seen. -func windowsFileObjectIdentity(path string) (windowsFileObjectID, bool) { - ptr, err := windows.UTF16PtrFromString(path) - if err != nil { - return windowsFileObjectID{}, false - } - handle, err := windows.CreateFile( - ptr, - windows.FILE_READ_ATTRIBUTES, - windows.FILE_SHARE_READ|windows.FILE_SHARE_WRITE|windows.FILE_SHARE_DELETE, - nil, - windows.OPEN_EXISTING, - windows.FILE_FLAG_BACKUP_SEMANTICS|windows.FILE_FLAG_OPEN_REPARSE_POINT, - 0, - ) - if err != nil { - return windowsFileObjectID{}, false - } - defer windows.CloseHandle(handle) - var info windows.ByHandleFileInformation - if err := windows.GetFileInformationByHandle(handle, &info); err != nil { - return windowsFileObjectID{}, false - } - if info.FileAttributes&windows.FILE_ATTRIBUTE_REPARSE_POINT != 0 { - return windowsFileObjectID{}, false - } - if info.FileAttributes&windows.FILE_ATTRIBUTE_DIRECTORY == 0 { - return windowsFileObjectID{}, false - } - return windowsFileObjectID{ - volume: info.VolumeSerialNumber, - index: (uint64(info.FileIndexHigh) << 32) | uint64(info.FileIndexLow), - }, true -} - -// windowsAccessAllowedObjectAceType and windowsAccessDeniedObjectAceType are -// the AceType values for ACCESS_ALLOWED_OBJECT_ACE / ACCESS_DENIED_OBJECT_ACE -// (https://learn.microsoft.com/en-us/windows/win32/api/winnt/ns-winnt-access_allowed_object_ace). -// windowsAccessAllowedCallbackAceType and windowsAccessAllowedCallbackObjectAceType -// are ACCESS_ALLOWED_CALLBACK_ACE_TYPE and ACCESS_ALLOWED_CALLBACK_OBJECT_ACE_TYPE -// (MS-DTYP 2.4.4.6 / conditional-ACE object variant): a callback ACE carries a -// conditional expression (e.g. "resource attribute matches") that gates -// whether the grant applies, appended AFTER the SID, so it does not move the -// SID's own offset relative to its non-callback sibling. x/sys/windows only -// models the plain ACCESS_ALLOWED_ACE layout (Header, Mask, SidStart) and -// exposes just ACCESS_ALLOWED_ACE_TYPE/ACCESS_DENIED_ACE_TYPE, so all four are -// declared locally. -const ( - windowsAccessAllowedObjectAceType = 0x05 - windowsAccessDeniedObjectAceType = 0x06 - windowsAccessAllowedCallbackAceType = 0x09 - windowsAccessAllowedCallbackObjectAceType = 0x0B -) - -// windowsAceSID locates the trustee SID within ace, an *ACCESS_ALLOWED_ACE -// pointer that GetAce hands back regardless of the ACE's true type — for -// object ACEs that pointer is only valid for reading Header/Mask, not -// SidStart. An object ACE (ACCESS_ALLOWED_OBJECT_ACE / ACCESS_DENIED_OBJECT_ACE) -// inserts a Flags DWORD and up to two conditionally-present 16-byte GUIDs -// (ObjectType, InheritedObjectType) between Mask and the real SID; naively -// reading &ace.SidStart for one of these — as if it had the plain ACE layout — -// reinterprets Flags/GUID bytes as SID bytes and silently computes the wrong -// trustee, both risking a false match and missing a real Users/Authenticated -// Users grant hidden inside an object ACE. -// -// ACCESS_ALLOWED_CALLBACK_ACE_TYPE and ACCESS_ALLOWED_CALLBACK_OBJECT_ACE_TYPE -// are recognized the same way as their non-callback counterparts: per MS-DTYP, -// a callback ACE's conditional expression ("ApplicationData") is appended -// AFTER the SID, not inserted before it, so the SID offset is identical. Only -// the ALLOW callback variants are recognized here, deliberately — see -// windowsDirGrantsBroadenedWrite for why a callback DENY is never trusted to -// suppress a grant. ok is false for any other ACE type (audit, alarm, -// mandatory label, compound, callback deny, ...), which either does not -// represent a trustee write grant in the sense this scan cares about, or (for -// callback deny) is not safe to rely on, and is skipped exactly as it always -// has been. -func windowsAceSID(ace *windows.ACCESS_ALLOWED_ACE) (sid *windows.SID, ok bool) { - switch ace.Header.AceType { - case windows.ACCESS_ALLOWED_ACE_TYPE, windows.ACCESS_DENIED_ACE_TYPE, windowsAccessAllowedCallbackAceType: - return (*windows.SID)(unsafe.Pointer(&ace.SidStart)), true - case windowsAccessAllowedObjectAceType, windowsAccessDeniedObjectAceType, windowsAccessAllowedCallbackObjectAceType: - // For an object ACE, the memory the Go struct calls SidStart is - // actually the ACE's Flags DWORD; the real SID sits further out, - // pushed by whichever of the two optional GUIDs Flags says are present. - // offset is plain arithmetic on a byte count, never itself derived from - // a pointer conversion, so accumulating it across statements is safe; - // only the final pointer+offset conversion below needs to happen in a - // single expression (go vet's unsafeptr rule). - flags := ace.SidStart - offset := unsafe.Sizeof(ace.SidStart) - if flags&windows.ACE_OBJECT_TYPE_PRESENT != 0 { - offset += 16 - } - if flags&windows.ACE_INHERITED_OBJECT_TYPE_PRESENT != 0 { - offset += 16 - } - return (*windows.SID)(unsafe.Pointer(uintptr(unsafe.Pointer(&ace.SidStart)) + offset)), true - default: - return nil, false - } -} - -// windowsDirGrantsBroadenedWrite reports whether path's effective DACL lets -// BUILTIN\Users or Authenticated Users write. It walks the DACL (which, as -// returned by GetNamedSecurityInfo, already contains inherited ACEs) in order, -// honoring a deny ACE that precedes an allow for the same bits, the canonical -// evaluation. A NULL DACL grants everyone full access and is treated as -// writable. -// -// Note: this is a deliberate DACL walk rather than AccessCheck. It must detect -// grants that would become usable once the restricted token is broadened with -// those groups, independent of the setup process's own token. INHERIT_ONLY ACEs -// are skipped because they do not apply to the object itself. -// -// A callback allow ACE (ACCESS_ALLOWED_CALLBACK_ACE / _OBJECT_ACE) is treated -// exactly like an unconditional allow: this static walk cannot evaluate the -// ACE's conditional expression against the sandbox token, so the only safe -// assumption is the worst case, that the condition holds and the grant -// applies (see jatmn's review). The symmetric callback DENY types are -// deliberately NOT recognized by windowsAceSID at all, so they never reach -// this switch: trusting an unproven condition to suppress deniedWrite would -// risk the opposite mistake, misclassifying a writable directory as safe. -func windowsDirGrantsBroadenedWrite(path string) (bool, error) { - // GetNamedSecurityInfo returns a self-relative descriptor copied onto the Go - // heap (it LocalFrees the Win32 allocation itself), so it must NOT be - // LocalFree'd here: doing so frees Go-managed memory and corrupts the heap. - sd, err := windows.GetNamedSecurityInfo(path, windows.SE_FILE_OBJECT, windows.DACL_SECURITY_INFORMATION) - if err != nil { - return false, err - } - dacl, _, err := sd.DACL() - if err != nil { - return false, err - } - if dacl == nil { - return true, nil - } - var deniedWrite windows.ACCESS_MASK - for index := uint16(0); index < dacl.AceCount; index++ { - var ace *windows.ACCESS_ALLOWED_ACE - if err := windows.GetAce(dacl, uint32(index), &ace); err != nil { - return false, fmt.Errorf("read ACE %d of %s: %w", index, path, err) - } - // An INHERIT_ONLY ACE does not apply to this object itself — it only - // seeds ACLs of newly created children. Counting one here could let - // an inherit-only deny suppress a later applicable allow in - // deniedWrite, misclassifying a writable directory as safe. - if ace.Header.AceFlags&windows.INHERIT_ONLY_ACE != 0 { - continue - } - sid, ok := windowsAceSID(ace) - if !ok { - continue - } - if !sid.IsWellKnown(windows.WinBuiltinUsersSid) && !sid.IsWellKnown(windows.WinAuthenticatedUserSid) { - continue - } - writeBits := ace.Mask & windowsBroadenedWriteProbeMask - if writeBits == 0 { - continue - } - switch ace.Header.AceType { - case windows.ACCESS_DENIED_ACE_TYPE, windowsAccessDeniedObjectAceType: - deniedWrite |= writeBits - case windows.ACCESS_ALLOWED_ACE_TYPE, windowsAccessAllowedObjectAceType, - windowsAccessAllowedCallbackAceType, windowsAccessAllowedCallbackObjectAceType: - if writeBits&^deniedWrite != 0 { - return true, nil - } - } - } - return false, nil -} - -// windowsPathIsReparsePoint reports whether path carries the reparse-point -// attribute (a junction, symlink, or mount point). Any error resolving the -// attributes is reported as "not a reparse point" so the caller falls through -// to its own DACL read, which surfaces a real access problem there instead. -func windowsPathIsReparsePoint(path string) bool { - ptr, err := windows.UTF16PtrFromString(path) - if err != nil { - return false - } - attrs, err := windows.GetFileAttributes(ptr) - if err != nil { - return false - } - return attrs&windows.FILE_ATTRIBUTE_REPARSE_POINT != 0 -} - -// windowsUncoveredWritableDescendants returns Users/AuthUsers-writable -// descendants of root that do not yet carry a DenyWrite ACE for denySID. -func windowsUncoveredWritableDescendants(root, denySID string, writeRoots []string) ([]string, error) { - descendants, err := windowsEnumerateWritableDescendants(root, writeRoots) - if err != nil { - return nil, fmt.Errorf("enumerate writable descendants of %s: %w", root, err) - } - var holes []string - for _, dir := range descendants { - denied, err := windowsPathDeniesCapabilitySID(dir, denySID) - if err != nil { - return nil, fmt.Errorf("inspect existing deny on %s: %w", dir, err) - } - if !denied { - holes = append(holes, dir) - } - } - return holes, nil -} - -// windowsPathDeniesCapabilitySID reports whether path's DACL already contains -// deny ACE(s) naming the given capability SID string (the synthetic identity -// used for shared-root / descendant DenyWrite entries) that together cover -// every write-relevant bit in windowsBroadenedWriteProbeMask. -// -// A partial deny is not coverage: denying only FILE_WRITE_ATTRIBUTES (or only -// read/execute via a DenyRead ACE that reuses the same stable SID) leaves -// FILE_WRITE_DATA / FILE_APPEND_DATA open for a Users/AuthUsers grant, so the -// apply and verification paths must still merge the full canonical DenyWrite -// rather than skipping the path — see jatmn's review. Accumulated deny ACEs -// for wantSID are OR'd before the completeness check so a multi-ACE full -// cover still counts. -func windowsPathDeniesCapabilitySID(path, wantSID string) (bool, error) { - want, err := windows.StringToSid(wantSID) - if err != nil { - return false, fmt.Errorf("parse capability SID %q: %w", wantSID, err) - } - sd, err := windows.GetNamedSecurityInfo(path, windows.SE_FILE_OBJECT, windows.DACL_SECURITY_INFORMATION) - if err != nil { - return false, err - } - dacl, _, err := sd.DACL() - if err != nil { - return false, err - } - if dacl == nil { - return false, nil - } - var deniedMask windows.ACCESS_MASK - for index := uint16(0); index < dacl.AceCount; index++ { - var ace *windows.ACCESS_ALLOWED_ACE - if err := windows.GetAce(dacl, uint32(index), &ace); err != nil { - return false, fmt.Errorf("read ACE %d of %s: %w", index, path, err) - } - if ace.Header.AceType != windows.ACCESS_DENIED_ACE_TYPE && ace.Header.AceType != windowsAccessDeniedObjectAceType { - continue - } - // An INHERIT_ONLY ACE does not apply to this object itself (see the - // same skip in windowsDirGrantsBroadenedWrite). Counting one here - // would report an inherited-but-inapplicable deny as "already - // denied," causing applyWindowsSharedDescendantDenies to skip - // applying the real, effective deny and leave the descendant open. - if ace.Header.AceFlags&windows.INHERIT_ONLY_ACE != 0 { - continue - } - sid, ok := windowsAceSID(ace) - if !ok || !sid.Equals(want) { - continue - } - deniedMask |= ace.Mask - } - return (deniedMask & windowsBroadenedWriteProbeMask) == windowsBroadenedWriteProbeMask, nil -} diff --git a/internal/sandbox/windows_acl_descendants_windows_test.go b/internal/sandbox/windows_acl_descendants_windows_test.go deleted file mode 100644 index 671bcb4ee..000000000 --- a/internal/sandbox/windows_acl_descendants_windows_test.go +++ /dev/null @@ -1,849 +0,0 @@ -//go:build windows - -package sandbox - -import ( - "encoding/binary" - "os" - "os/exec" - "path/filepath" - "strings" - "testing" - "unsafe" - - "golang.org/x/sys/windows" -) - -// touchFile creates an empty file at path, failing the test on error. -func touchFile(t *testing.T, path string) string { - t.Helper() - if err := os.WriteFile(path, nil, 0o644); err != nil { - t.Fatalf("WriteFile(%q): %v", path, err) - } - return path -} - -// dirDeniesSID reports whether path's DACL carries a deny ACE naming the given -// string SID. It reads the same way the descendant scan applies denies, so a -// test can confirm the compensating deny actually landed (and, after rollback, -// is gone) using the real Win32 ACL APIs on a test-owned temp tree. -func dirDeniesSID(t *testing.T, path, wantSID string) bool { - t.Helper() - sd, err := windows.GetNamedSecurityInfo(path, windows.SE_FILE_OBJECT, windows.DACL_SECURITY_INFORMATION) - if err != nil { - t.Fatalf("GetNamedSecurityInfo %s: %v", path, err) - } - dacl, _, err := sd.DACL() - if err != nil { - t.Fatalf("DACL %s: %v", path, err) - } - if dacl == nil { - return false - } - for index := uint16(0); index < dacl.AceCount; index++ { - var ace *windows.ACCESS_ALLOWED_ACE - if err := windows.GetAce(dacl, uint32(index), &ace); err != nil { - t.Fatalf("GetAce %d of %s: %v", index, path, err) - } - if ace.Header.AceType != windows.ACCESS_DENIED_ACE_TYPE && ace.Header.AceType != windowsAccessDeniedObjectAceType { - continue - } - sid, ok := windowsAceSID(ace) - if !ok || sid.String() != wantSID { - continue - } - return true - } - return false -} - -// grantUsersWrite adds a direct (non-inheriting) allow-write ACE for -// BUILTIN\Users to path's DACL using the real Win32 ACL APIs. The test process -// owns the t.TempDir() tree, so this needs no elevation. -func grantUsersWrite(t *testing.T, path string) { - t.Helper() - usersSID, err := windows.CreateWellKnownSid(windows.WinBuiltinUsersSid) - if err != nil { - t.Fatalf("CreateWellKnownSid(Users): %v", err) - } - sd, err := windows.GetNamedSecurityInfo(path, windows.SE_FILE_OBJECT, windows.DACL_SECURITY_INFORMATION) - if err != nil { - t.Fatalf("GetNamedSecurityInfo %s: %v", path, err) - } - oldDACL, _, err := sd.DACL() - if err != nil { - t.Fatalf("DACL %s: %v", path, err) - } - newDACL, err := windows.ACLFromEntries([]windows.EXPLICIT_ACCESS{{ - AccessPermissions: windows.FILE_GENERIC_WRITE, - AccessMode: windows.GRANT_ACCESS, - Inheritance: windows.NO_INHERITANCE, - Trustee: windows.TRUSTEE{ - TrusteeForm: windows.TRUSTEE_IS_SID, - TrusteeType: windows.TRUSTEE_IS_GROUP, - TrusteeValue: windows.TrusteeValueFromSID(usersSID), - }, - }}, oldDACL) - if err != nil { - t.Fatalf("ACLFromEntries %s: %v", path, err) - } - if err := windows.SetNamedSecurityInfo(path, windows.SE_FILE_OBJECT, windows.DACL_SECURITY_INFORMATION, nil, nil, newDACL, nil); err != nil { - t.Fatalf("SetNamedSecurityInfo %s: %v", path, err) - } -} - -// denyUsersWrite adds a direct (non-inheriting) deny-write ACE for -// BUILTIN\Users to path's DACL, overriding any inherited write grant from the -// enclosing t.TempDir() tree. Used to construct a directory that is -// genuinely non-writable at its own DACL, independent of whatever the test -// temp tree happens to inherit. -// -// Denies the concrete (non-generic) bits of windowsBroadenedWriteProbeMask — -// the same bits windowsDirGrantsBroadenedWrite itself checks, minus -// GENERIC_WRITE/GENERIC_ALL. Two things must NOT be in this mask: -// - SYNCHRONIZE (part of windows.FILE_GENERIC_WRITE): the test process is -// normally a member of BUILTIN\Users, and denying SYNCHRONIZE also blocks -// its own later synchronous opens of path (e.g. os.ReadDir), not just -// "write" — verified directly against this code path. -// - Raw GENERIC_WRITE/GENERIC_ALL bits: stored unmapped in an ACE (as -// opposed to being resolved to their constituent FILE_* bits first), -// these were empirically observed to make Windows deny EVERY access, -// including a plain FILE_LIST_DIRECTORY open, not just generic write. -func denyUsersWrite(t *testing.T, path string) { - t.Helper() - usersSID, err := windows.CreateWellKnownSid(windows.WinBuiltinUsersSid) - if err != nil { - t.Fatalf("CreateWellKnownSid(Users): %v", err) - } - sd, err := windows.GetNamedSecurityInfo(path, windows.SE_FILE_OBJECT, windows.DACL_SECURITY_INFORMATION) - if err != nil { - t.Fatalf("GetNamedSecurityInfo %s: %v", path, err) - } - oldDACL, _, err := sd.DACL() - if err != nil { - t.Fatalf("DACL %s: %v", path, err) - } - denyMask := windowsBroadenedWriteProbeMask &^ (windows.GENERIC_WRITE | windows.GENERIC_ALL | windows.SYNCHRONIZE) - newDACL, err := windows.ACLFromEntries([]windows.EXPLICIT_ACCESS{{ - AccessPermissions: denyMask, - AccessMode: windows.DENY_ACCESS, - Inheritance: windows.NO_INHERITANCE, - Trustee: windows.TRUSTEE{ - TrusteeForm: windows.TRUSTEE_IS_SID, - TrusteeType: windows.TRUSTEE_IS_GROUP, - TrusteeValue: windows.TrusteeValueFromSID(usersSID), - }, - }}, oldDACL) - if err != nil { - t.Fatalf("ACLFromEntries %s: %v", path, err) - } - if err := windows.SetNamedSecurityInfo(path, windows.SE_FILE_OBJECT, windows.DACL_SECURITY_INFORMATION, nil, nil, newDACL, nil); err != nil { - t.Fatalf("SetNamedSecurityInfo %s: %v", path, err) - } -} - -// TestWindowsEnumerateWritableDescendantsDoesNotPruneSystemLookalikeTrees is -// the real-Windows regression for jatmn's review finding: a directory whose -// basename matched the old prune list (e.g. "Program Files") and was -// non-writable at its OWN DACL must still be descended into, because a -// non-writable root DACL says nothing about a writable descendant several -// levels down (e.g. an installer-created "Program Files\SomeApp" with a -// loosened grant). Before the fix, this subtree was silently certified clean -// from the root DACL alone and never scanned further. -func TestWindowsEnumerateWritableDescendantsDoesNotPruneSystemLookalikeTrees(t *testing.T) { - root := t.TempDir() - programFiles := mkdir(t, filepath.Join(root, "Program Files")) - // Create and grant the nested descendant BEFORE denying write on - // programFiles itself: the deny targets BUILTIN\Users, which the test - // process is normally a member of, so denying it on programFiles first - // would block the test process from creating anything under it. - someApp := mkdir(t, filepath.Join(programFiles, "SomeApp")) - grantUsersWrite(t, someApp) - denyUsersWrite(t, programFiles) - - rootWritable, err := windowsDirGrantsBroadenedWrite(programFiles) - if err != nil { - t.Fatalf("windowsDirGrantsBroadenedWrite(programFiles): %v", err) - } - if rootWritable { - t.Fatal("test fixture bug: programFiles must be non-writable at its own DACL to exercise the old prune condition") - } - - found, err := windowsEnumerateWritableDescendants(root, nil) - if err != nil { - t.Fatalf("windowsEnumerateWritableDescendants: %v", err) - } - if !windowsPathListContains(found, someApp) { - t.Fatalf("enumeration = %#v, want it to include writable descendant %q under non-writable %q (must not be pruned by basename)", found, someApp, programFiles) - } -} - -// TestWindowsDirGrantsBroadenedWriteDetectsUsersWrite pins the DACL probe the -// descendant scan relies on: a directory whose DACL grants BUILTIN\Users write -// is reported writable; one that does not is reported not writable. -func TestWindowsDirGrantsBroadenedWriteDetectsUsersWrite(t *testing.T) { - root := t.TempDir() - writable := mkdir(t, filepath.Join(root, "writable")) - grantUsersWrite(t, writable) - plain := mkdir(t, filepath.Join(root, "plain")) - - got, err := windowsDirGrantsBroadenedWrite(writable) - if err != nil { - t.Fatalf("windowsDirGrantsBroadenedWrite(writable): %v", err) - } - if !got { - t.Fatalf("windowsDirGrantsBroadenedWrite(writable) = false, want true") - } - - plainWritable, err := windowsDirGrantsBroadenedWrite(plain) - if err != nil { - t.Fatalf("windowsDirGrantsBroadenedWrite(plain): %v", err) - } - if plainWritable { - t.Skip("test temp tree grants BUILTIN\\Users write by inheritance; cannot exercise the negative case here") - } -} - -// TestWindowsEnumerateWritableDescendantsFindsExistingWritableChildren is the -// real-Windows regression for the write-jail gap the reviewer flagged: an -// existing writable descendant of a shared root (including one nested under -// another writable directory) must be discovered so it can be denied directly, -// and a configured write root must be excluded so legitimate workspace writes -// are never jailed. -func TestWindowsEnumerateWritableDescendantsFindsExistingWritableChildren(t *testing.T) { - root := t.TempDir() - outer := mkdir(t, filepath.Join(root, "outer")) - grantUsersWrite(t, outer) - inner := mkdir(t, filepath.Join(outer, "inner")) - grantUsersWrite(t, inner) - // Depth-3 writable child under non-writable ancestors: the scan must keep - // descending through non-writable parents or this escape stays open. - level1 := mkdir(t, filepath.Join(root, "locked1")) - level2 := mkdir(t, filepath.Join(level1, "locked2")) - deepWritable := mkdir(t, filepath.Join(level2, "deep-writable")) - grantUsersWrite(t, deepWritable) - plain := mkdir(t, filepath.Join(root, "plain")) - workspace := mkdir(t, filepath.Join(root, "workspace")) - grantUsersWrite(t, workspace) - writableFile := touchFile(t, filepath.Join(root, "writable.txt")) - grantUsersWrite(t, writableFile) - - found, err := windowsEnumerateWritableDescendants(root, nil) - if err != nil { - t.Fatalf("windowsEnumerateWritableDescendants: %v", err) - } - if !windowsPathListContains(found, outer) { - t.Fatalf("enumeration = %#v, want it to include writable child %q", found, outer) - } - if !windowsPathListContains(found, inner) { - t.Fatalf("enumeration = %#v, want it to include nested writable descendant %q", found, inner) - } - if !windowsPathListContains(found, deepWritable) { - t.Fatalf("enumeration = %#v, want it to include depth-3 writable child %q under non-writable ancestors", found, deepWritable) - } - if !windowsPathListContains(found, writableFile) { - t.Fatalf("enumeration = %#v, want it to include writable file %q (a file is as much an escape surface as a directory)", found, writableFile) - } - - plainWritable, err := windowsDirGrantsBroadenedWrite(plain) - if err != nil { - t.Fatalf("windowsDirGrantsBroadenedWrite(plain): %v", err) - } - if !plainWritable && windowsPathListContains(found, plain) { - t.Fatalf("enumeration = %#v, want it to exclude non-writable child %q", found, plain) - } - - // Excluding the workspace write root (and its subtree) must drop it from the - // result even though it grants Users write. - excluded, err := windowsEnumerateWritableDescendants(root, []string{workspace}) - if err != nil { - t.Fatalf("windowsEnumerateWritableDescendants(exclude): %v", err) - } - if windowsPathListContains(excluded, workspace) { - t.Fatalf("enumeration = %#v, want it to exclude the configured write root %q", excluded, workspace) - } - if !windowsPathListContains(excluded, outer) { - t.Fatalf("enumeration = %#v, want it to still include %q when a different path is excluded", excluded, outer) - } -} - -// selfUserSID returns the current process token's own user SID, used by the -// canonical-root-scoping test below to deny itself directory-listing access -// (an owner always retains READ_CONTROL/WRITE_DAC implicitly, so this cannot -// lock the test out of restoring its own change). -func selfUserSID(t *testing.T) *windows.SID { - t.Helper() - user, err := windows.GetCurrentProcessToken().GetTokenUser() - if err != nil { - t.Fatalf("GetTokenUser: %v", err) - } - return user.User.Sid -} - -// setSelfListDirectoryAccess grants or denies the current user FILE_LIST_DIRECTORY -// on path, restoring/breaking the ability to os.ReadDir it without touching -// READ_CONTROL/WRITE_DAC (which owners always retain), so the test can always -// undo its own change. -// -// deny=false restores access via SET_ACCESS with a zero mask rather than -// REVOKE_ACCESS: empirically, SetEntriesInAclW's REVOKE_ACCESS mode does not -// remove a pre-existing DENY ACE for the trustee (verified directly against -// this code path — see the same finding in BuildWindowsACLPlan's -// WindowsACLRevokeCapability, windows_acl_apply_windows.go), so relying on it -// here would leave the test process permanently denied FILE_LIST_DIRECTORY on -// its own temp fixture. -func setSelfListDirectoryAccess(t *testing.T, path string, deny bool) { - t.Helper() - mode := windows.ACCESS_MODE(windows.SET_ACCESS) - permissions := windows.ACCESS_MASK(0) - if deny { - mode = windows.DENY_ACCESS - permissions = windows.FILE_LIST_DIRECTORY - } - sd, err := windows.GetNamedSecurityInfo(path, windows.SE_FILE_OBJECT, windows.DACL_SECURITY_INFORMATION) - if err != nil { - t.Fatalf("GetNamedSecurityInfo %s: %v", path, err) - } - oldDACL, _, err := sd.DACL() - if err != nil { - t.Fatalf("DACL %s: %v", path, err) - } - targetOldDACL := oldDACL - if deny { - targetOldDACL = nil - } - newDACL, err := windows.ACLFromEntries([]windows.EXPLICIT_ACCESS{{ - AccessPermissions: permissions, - AccessMode: mode, - Inheritance: windows.NO_INHERITANCE, - Trustee: windows.TRUSTEE{ - TrusteeForm: windows.TRUSTEE_IS_SID, - TrusteeType: windows.TRUSTEE_IS_USER, - TrusteeValue: windows.TrusteeValueFromSID(selfUserSID(t)), - }, - }}, targetOldDACL) - if err != nil { - t.Fatalf("ACLFromEntries %s: %v", path, err) - } - if err := windows.SetNamedSecurityInfo(path, windows.SE_FILE_OBJECT, windows.DACL_SECURITY_INFORMATION, nil, nil, newDACL, nil); err != nil { - t.Fatalf("SetNamedSecurityInfo %s: %v", path, err) - } -} - -// TestWindowsEnumerateWritableDescendantsFailsClosedOnNonRootLookalikeSystemDir -// is the real-Windows regression for jatmn's review finding: a directory that -// shares a basename with a known SYSTEM-exclusive Windows directory (here -// "Recovery") but sits somewhere other than a genuine drive letter root must -// fail the scan closed when it cannot be listed, not be silently skipped as -// if it were the real, stock volume-root object. -func TestWindowsEnumerateWritableDescendantsFailsClosedOnNonRootLookalikeSystemDir(t *testing.T) { - root := t.TempDir() - outer := mkdir(t, filepath.Join(root, "outer")) - lookalike := mkdir(t, filepath.Join(outer, "Recovery")) - - setSelfListDirectoryAccess(t, lookalike, true) - t.Cleanup(func() { setSelfListDirectoryAccess(t, lookalike, false) }) - - if _, err := os.ReadDir(lookalike); err == nil { - t.Skip("could not deny self directory-listing access on this host; skipping fail-closed assertion") - } - - _, err := windowsEnumerateWritableDescendants(root, nil) - if err == nil { - t.Fatal("windowsEnumerateWritableDescendants: expected a fail-closed error for an unlistable non-root-level lookalike system directory, got nil") - } -} - -// TestWindowsEnumerateWritableDescendantsFailsClosedOnEntryCap pins that -// exhausting the descendant entry budget is an error, not a silent partial -// success that would still let setup broaden the restricted token. -func TestWindowsEnumerateWritableDescendantsFailsClosedOnEntryCap(t *testing.T) { - prev := windowsDescendantScanMaxDirs - windowsDescendantScanMaxDirs = 3 - t.Cleanup(func() { windowsDescendantScanMaxDirs = prev }) - - root := t.TempDir() - for _, name := range []string{"a", "b", "c", "d"} { - mkdir(t, filepath.Join(root, name)) - } - _, err := windowsEnumerateWritableDescendants(root, nil) - if err == nil { - t.Fatal("windowsEnumerateWritableDescendants: expected entry-cap error, got nil") - } -} - -// TestWindowsPathDeniesCapabilitySIDRoundTrip ensures the pre-broaden hole -// check can see a deny ACE that applyWindowsSharedDescendantDenies just wrote. -func TestWindowsPathDeniesCapabilitySIDRoundTrip(t *testing.T) { - caps, err := LoadOrCreateWindowsCapabilitySIDs(t.TempDir()) - if err != nil { - t.Fatalf("LoadOrCreateWindowsCapabilitySIDs: %v", err) - } - root := t.TempDir() - writable := mkdir(t, filepath.Join(root, "writable")) - grantUsersWrite(t, writable) - - before, err := windowsPathDeniesCapabilitySID(writable, caps.ReadOnly) - if err != nil { - t.Fatalf("windowsPathDeniesCapabilitySID before: %v", err) - } - if before { - t.Fatal("path already denies capability SID before apply") - } - if _, err := applyWindowsSharedDescendantDenies(root, caps.ReadOnly, nil); err != nil { - t.Fatalf("applyWindowsSharedDescendantDenies: %v", err) - } - after, err := windowsPathDeniesCapabilitySID(writable, caps.ReadOnly) - if err != nil { - t.Fatalf("windowsPathDeniesCapabilitySID after: %v", err) - } - if !after { - t.Fatal("path does not deny capability SID after apply") - } - holes, err := windowsUncoveredWritableDescendants(root, caps.ReadOnly, nil) - if err != nil { - t.Fatalf("windowsUncoveredWritableDescendants: %v", err) - } - if len(holes) != 0 { - t.Fatalf("holes = %#v, want none after apply", holes) - } -} - -// TestWindowsPathDeniesCapabilitySIDIgnoresReadOnlyDeny is the real-Windows -// regression for jatmn's review finding: a pre-existing deny ACE for the -// stable capability SID that only denies read/execute (the exact shape -// planWindowsDenyReadPaths applies for a DenyRead path) must NOT be read as -// "write already denied." Before the fix, any deny ACE naming the SID short- -// circuited the check regardless of its mask, so a writable descendant that -// happened to sit under a DenyRead path would be skipped by -// applyWindowsSharedDescendantDenies and never get the write deny it needs. -func TestWindowsPathDeniesCapabilitySIDIgnoresReadOnlyDeny(t *testing.T) { - caps, err := LoadOrCreateWindowsCapabilitySIDs(t.TempDir()) - if err != nil { - t.Fatalf("LoadOrCreateWindowsCapabilitySIDs: %v", err) - } - root := t.TempDir() - writable := mkdir(t, filepath.Join(root, "writable")) - grantUsersWrite(t, writable) - - // Apply the same DenyRead entry BuildWindowsACLPlan would generate for a - // DenyRead path sharing this stable capability SID. - if _, _, err := applyWindowsACLPathGroup(windowsACLPathGroup{ - Path: writable, - Entries: []WindowsACLEntry{{ - Action: WindowsACLDenyRead, - Path: writable, - Capability: caps.ReadOnly, - }}, - }); err != nil { - t.Fatalf("apply DenyRead entry: %v", err) - } - - deniesWrite, err := windowsPathDeniesCapabilitySID(writable, caps.ReadOnly) - if err != nil { - t.Fatalf("windowsPathDeniesCapabilitySID: %v", err) - } - if deniesWrite { - t.Fatal("windowsPathDeniesCapabilitySID = true for a read-only deny ACE, want false: a DenyRead ACE does not block writes") - } - - // The descendant-denial pass must therefore still add the real write deny - // rather than skipping this path as already covered. - if _, err := applyWindowsSharedDescendantDenies(root, caps.ReadOnly, nil); err != nil { - t.Fatalf("applyWindowsSharedDescendantDenies: %v", err) - } - deniesWrite, err = windowsPathDeniesCapabilitySID(writable, caps.ReadOnly) - if err != nil { - t.Fatalf("windowsPathDeniesCapabilitySID after apply: %v", err) - } - if !deniesWrite { - t.Fatal("windowsPathDeniesCapabilitySID = false after applyWindowsSharedDescendantDenies, want true: the write deny should now be present") - } -} - -// TestApplyWindowsSharedDescendantDeniesAppliesAndRollsBack proves the -// enforcement half of the fix end to end on real ACLs: a writable descendant of -// a shared root gets a direct deny ACE for the read-only capability SID (the SID -// every broadened token carries), and the returned rollback restores the DACL. -// This runs unprivileged because it operates only on the test-owned temp tree. -func TestApplyWindowsSharedDescendantDeniesAppliesAndRollsBack(t *testing.T) { - caps, err := LoadOrCreateWindowsCapabilitySIDs(t.TempDir()) - if err != nil { - t.Fatalf("LoadOrCreateWindowsCapabilitySIDs: %v", err) - } - root := t.TempDir() - writable := mkdir(t, filepath.Join(root, "writable")) - grantUsersWrite(t, writable) - - if dirDeniesSID(t, writable, caps.ReadOnly) { - t.Fatalf("descendant already denies %q before apply", caps.ReadOnly) - } - snapshots, err := applyWindowsSharedDescendantDenies(root, caps.ReadOnly, nil) - if err != nil { - t.Fatalf("applyWindowsSharedDescendantDenies: %v", err) - } - if len(snapshots) == 0 { - t.Fatalf("apply returned no snapshots; the writable descendant was never denied") - } - if !dirDeniesSID(t, writable, caps.ReadOnly) { - t.Fatalf("descendant %q does not deny %q after apply", writable, caps.ReadOnly) - } - - if err := rollbackWindowsACLSnapshots(snapshots); err != nil { - t.Fatalf("rollbackWindowsACLSnapshots: %v", err) - } - if dirDeniesSID(t, writable, caps.ReadOnly) { - t.Fatalf("descendant %q still denies %q after rollback", writable, caps.ReadOnly) - } - // Checking only that the deny disappeared would also pass a rollback that - // clobbered the original Users write ACE along with it; reassert the - // pre-existing writable state is actually restored, not just any DACL. - 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) - } -} - -// TestWindowsAceSIDLocatesSIDInObjectACE pins the offset arithmetic -// windowsAceSID relies on for ACCESS_ALLOWED_OBJECT_ACE / ACCESS_DENIED_OBJECT_ACE: -// the real SID sits past a Flags DWORD and 0, 1, or 2 conditionally-present -// 16-byte GUIDs (ObjectType, InheritedObjectType), never at the plain-ACE -// SidStart offset GetAce's *ACCESS_ALLOWED_ACE typing would naively suggest. -// This builds the raw ACE bytes directly, per Microsoft's documented layout, -// because x/sys/windows has no AddAccessAllowedObjectAce binding to create a -// real one through the OS. -func TestWindowsAceSIDLocatesSIDInObjectACE(t *testing.T) { - usersSID, err := windows.CreateWellKnownSid(windows.WinBuiltinUsersSid) - if err != nil { - t.Fatalf("CreateWellKnownSid: %v", err) - } - sidBytes := unsafe.Slice((*byte)(unsafe.Pointer(usersSID)), usersSID.Len()) - - cases := []struct { - name string - aceType byte - flags uint32 - guids int // number of 16-byte GUIDs the flags say precede the SID - }{ - {"no optional GUIDs", windowsAccessAllowedObjectAceType, 0, 0}, - {"object type GUID only", windowsAccessAllowedObjectAceType, windows.ACE_OBJECT_TYPE_PRESENT, 1}, - {"inherited type GUID only", windowsAccessDeniedObjectAceType, windows.ACE_INHERITED_OBJECT_TYPE_PRESENT, 1}, - {"both GUIDs", windowsAccessDeniedObjectAceType, windows.ACE_OBJECT_TYPE_PRESENT | windows.ACE_INHERITED_OBJECT_TYPE_PRESENT, 2}, - } - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - // Layout: ACE_HEADER(4) + Mask(4) + Flags(4) + guids*GUID(16) + SID. - buf := make([]byte, 4+4+4+16*tc.guids+len(sidBytes)) - buf[0] = tc.aceType // Header.AceType - binary.LittleEndian.PutUint32(buf[8:12], tc.flags) // Flags, at the offset SidStart occupies in the plain-ACE layout - copy(buf[12+16*tc.guids:], sidBytes) - - ace := (*windows.ACCESS_ALLOWED_ACE)(unsafe.Pointer(&buf[0])) - sid, ok := windowsAceSID(ace) - if !ok { - t.Fatal("windowsAceSID returned ok=false for a recognized object ACE type") - } - if !sid.Equals(usersSID) { - t.Fatalf("windowsAceSID = %s, want %s", sid.String(), usersSID.String()) - } - }) - } -} - -// TestWindowsAceSIDSkipsUnhandledAceTypes confirms an ACE type this scan does -// not model (audit, mandatory label, ...) is skipped rather than misread as a -// plain or object ACE — the same conservative behavior the code had before -// object-ACE support was added. -func TestWindowsAceSIDSkipsUnhandledAceTypes(t *testing.T) { - const systemMandatoryLabelAceType = 0x11 - buf := make([]byte, 32) - buf[0] = systemMandatoryLabelAceType - ace := (*windows.ACCESS_ALLOWED_ACE)(unsafe.Pointer(&buf[0])) - if _, ok := windowsAceSID(ace); ok { - t.Fatal("windowsAceSID should return ok=false for an unhandled ACE type") - } -} - -func TestWindowsPathDeniesCapabilitySIDRequiresEssentialWriteMask(t *testing.T) { - caps, err := LoadOrCreateWindowsCapabilitySIDs(t.TempDir()) - if err != nil { - t.Fatalf("LoadOrCreateWindowsCapabilitySIDs: %v", err) - } - sid := caps.ReadOnly - dir := t.TempDir() - - group := windowsACLPathGroup{ - Path: dir, - Entries: []WindowsACLEntry{{ - Action: WindowsACLDenyWrite, - Path: dir, - Capability: sid, - }}, - } - if _, _, err := applyWindowsACLPathGroup(group); err != nil { - t.Fatal(err) - } - - denied, err := windowsPathDeniesCapabilitySID(dir, sid) - if err != nil { - t.Fatal(err) - } - if !denied { - t.Fatal("expected full DenyWrite to satisfy windowsPathDeniesCapabilitySID") - } -} - -// denyCapabilityMask adds a direct (non-inheriting) deny ACE for capabilitySID -// covering only mask. Used to build partial-deny fixtures that must not pass -// windowsPathDeniesCapabilitySID's complete-coverage check. -func denyCapabilityMask(t *testing.T, path, capabilitySID string, mask windows.ACCESS_MASK) { - t.Helper() - sid, err := windows.StringToSid(capabilitySID) - if err != nil { - t.Fatalf("StringToSid(%q): %v", capabilitySID, err) - } - sd, err := windows.GetNamedSecurityInfo(path, windows.SE_FILE_OBJECT, windows.DACL_SECURITY_INFORMATION) - if err != nil { - t.Fatalf("GetNamedSecurityInfo %s: %v", path, err) - } - oldDACL, _, err := sd.DACL() - if err != nil { - t.Fatalf("DACL %s: %v", path, err) - } - newDACL, err := windows.ACLFromEntries([]windows.EXPLICIT_ACCESS{{ - AccessPermissions: mask, - AccessMode: windows.DENY_ACCESS, - Inheritance: windows.NO_INHERITANCE, - Trustee: windows.TRUSTEE{ - TrusteeForm: windows.TRUSTEE_IS_SID, - TrusteeType: windows.TRUSTEE_IS_WELL_KNOWN_GROUP, - TrusteeValue: windows.TrusteeValueFromSID(sid), - }, - }}, oldDACL) - if err != nil { - t.Fatalf("ACLFromEntries %s: %v", path, err) - } - if err := windows.SetNamedSecurityInfo(path, windows.SE_FILE_OBJECT, windows.DACL_SECURITY_INFORMATION, nil, nil, newDACL, nil); err != nil { - t.Fatalf("SetNamedSecurityInfo %s: %v", path, err) - } -} - -// TestWindowsPathDeniesCapabilitySIDRejectsPartialWriteDeny is the regression -// for jatmn's complete-coverage finding: a stable-SID deny that only blocks -// FILE_WRITE_ATTRIBUTES (or any other proper subset of the write probe mask) -// must not be accepted as descendant coverage. FILE_WRITE_DATA would still -// pass through a Users/AuthUsers allow under a partial-deny check that only -// tested non-zero overlap with the probe mask. -func TestWindowsPathDeniesCapabilitySIDRejectsPartialWriteDeny(t *testing.T) { - // Synthetic capability SID: a full DenyWrite for a group the test process - // belongs to (e.g. Everyone) denies DELETE/WRITE_DAC and blocks t.TempDir - // cleanup. - caps, err := LoadOrCreateWindowsCapabilitySIDs(t.TempDir()) - if err != nil { - t.Fatalf("LoadOrCreateWindowsCapabilitySIDs: %v", err) - } - sidStr := caps.ReadOnly - dir := t.TempDir() - denyCapabilityMask(t, dir, sidStr, windows.FILE_WRITE_ATTRIBUTES) - - denied, err := windowsPathDeniesCapabilitySID(dir, sidStr) - if err != nil { - t.Fatalf("windowsPathDeniesCapabilitySID: %v", err) - } - if denied { - t.Fatal("windowsPathDeniesCapabilitySID = true for FILE_WRITE_ATTRIBUTES-only deny, want false: partial deny must not certify write coverage") - } - - // applyWindowsSharedDescendantDenies must still apply the full canonical - // deny when the only pre-existing ACE is this partial one. - scanRoot := t.TempDir() - child := mkdir(t, filepath.Join(scanRoot, "partial")) - grantUsersWrite(t, child) - denyCapabilityMask(t, child, sidStr, windows.FILE_WRITE_ATTRIBUTES) - if denied, err := windowsPathDeniesCapabilitySID(child, sidStr); err != nil || denied { - t.Fatalf("precondition: partial deny on child must not count as coverage (denied=%v err=%v)", denied, err) - } - if _, err := applyWindowsSharedDescendantDenies(scanRoot, sidStr, nil); err != nil { - t.Fatalf("applyWindowsSharedDescendantDenies: %v", err) - } - if denied, err := windowsPathDeniesCapabilitySID(child, sidStr); err != nil || !denied { - t.Fatalf("after apply: want full write deny on partial-deny child (denied=%v err=%v)", denied, err) - } -} - -// TestWindowsEnumerateWritableDescendantsSkipsJunctions is the real-Windows -// regression for jatmn's compatibility-junction finding: a directory junction -// under the scan root must not be followed (re-walking the target tree) or -// hard-failed when the reparse is non-listable. The real target path is still -// examined when reached without going through the reparse. -func TestWindowsEnumerateWritableDescendantsSkipsJunctions(t *testing.T) { - root := t.TempDir() - realDir := mkdir(t, filepath.Join(root, "real")) - writable := mkdir(t, filepath.Join(realDir, "writable")) - grantUsersWrite(t, writable) - junc := filepath.Join(root, "junc") - // mklink /J needs no elevation; create the junction via cmd. - out, err := exec.Command("cmd", "/c", "mklink", "/J", junc, realDir).CombinedOutput() - if err != nil { - t.Skipf("cannot create junction (mklink /J): %v %s", err, strings.TrimSpace(string(out))) - } - t.Cleanup(func() { _ = os.Remove(junc) }) - - if !windowsPathIsReparsePoint(junc) { - t.Fatalf("fixture bug: %q is not a reparse point", junc) - } - - found, err := windowsEnumerateWritableDescendants(root, nil) - if err != nil { - t.Fatalf("windowsEnumerateWritableDescendants: %v", err) - } - if !windowsPathListContains(found, writable) { - t.Fatalf("enumeration = %#v, want writable child %q via real path", found, writable) - } - // Junction path itself must not appear: no-follow skips reparse entries - // entirely (DACL inspect would follow and risk double-counting). - juncWritable := filepath.Join(junc, "writable") - if windowsPathListContains(found, junc) || windowsPathListContains(found, juncWritable) { - t.Fatalf("enumeration = %#v, must not include junction path %q or its followed child", found, junc) - } -} - -// TestApplyWindowsACLPathGroupIdempotentDenyWrite pins that a repeated -// applyWindowsACLPathGroup DenyWrite merge on an already-covered root does not -// stack another DenyWrite ACE. The descendant walker's own idempotency is -// covered by TestApplyWindowsSharedDescendantDeniesAppliesAndRollsBack. -func TestApplyWindowsACLPathGroupIdempotentDenyWrite(t *testing.T) { - dir := t.TempDir() - // Synthetic capability SID so DenyWrite's WRITE_DAC/DELETE bits do not - // lock the test out of its own temp dir. - caps, err := LoadOrCreateWindowsCapabilitySIDs(t.TempDir()) - if err != nil { - t.Fatalf("LoadOrCreateWindowsCapabilitySIDs: %v", err) - } - sid := caps.ReadOnly - - group := windowsACLPathGroup{ - Path: dir, - Entries: []WindowsACLEntry{{ - Action: WindowsACLDenyWrite, - Path: dir, - Capability: sid, - NoInherit: true, - }}, - } - if _, _, err := applyWindowsACLPathGroup(group); err != nil { - t.Fatal(err) - } - countBefore := denyACECountForSID(t, dir, sid) - - // A second full DenyWrite merge for the same SID must leave the ACE count - // unchanged (complete coverage skips re-applying). - if _, _, err := applyWindowsACLPathGroup(group); err != nil { - t.Fatal(err) - } - countAfter := denyACECountForSID(t, dir, sid) - if countAfter != countBefore { - t.Fatalf("deny ACE count for %q changed from %d to %d after second apply", sid, countBefore, countAfter) - } -} - -// denyACECountForSID counts ACCESS_DENIED ACEs naming wantSID on path's DACL. -func denyACECountForSID(t *testing.T, path, wantSID string) int { - t.Helper() - want, err := windows.StringToSid(wantSID) - if err != nil { - t.Fatalf("StringToSid %q: %v", wantSID, err) - } - sd, err := windows.GetNamedSecurityInfo(path, windows.SE_FILE_OBJECT, windows.DACL_SECURITY_INFORMATION) - if err != nil { - t.Fatalf("GetNamedSecurityInfo %s: %v", path, err) - } - dacl, _, err := sd.DACL() - if err != nil { - t.Fatalf("DACL %s: %v", path, err) - } - if dacl == nil { - return 0 - } - count := 0 - for index := uint16(0); index < dacl.AceCount; index++ { - var ace *windows.ACCESS_ALLOWED_ACE - if err := windows.GetAce(dacl, uint32(index), &ace); err != nil { - t.Fatalf("GetAce %d of %s: %v", index, path, err) - } - if ace.Header.AceType != windows.ACCESS_DENIED_ACE_TYPE { - continue - } - sid, ok := windowsAceSID(ace) - if ok && sid.Equals(want) { - count++ - } - } - return count -} - -func TestWindowsPreservedReadDenyAccessEntriesSkipsInheritedACEs(t *testing.T) { - caps, err := LoadOrCreateWindowsCapabilitySIDs(t.TempDir()) - if err != nil { - t.Fatalf("LoadOrCreateWindowsCapabilitySIDs: %v", err) - } - sid, err := windows.StringToSid(caps.ReadOnly) - if err != nil { - t.Fatalf("StringToSid: %v", err) - } - - parent := t.TempDir() - // Add an inheritable DenyRead on parent. - sd, err := windows.GetNamedSecurityInfo(parent, windows.SE_FILE_OBJECT, windows.DACL_SECURITY_INFORMATION) - if err != nil { - t.Fatalf("GetNamedSecurityInfo %s: %v", parent, err) - } - oldDACL, _, err := sd.DACL() - if err != nil { - t.Fatalf("DACL: %v", err) - } - parentDACL, err := windows.ACLFromEntries([]windows.EXPLICIT_ACCESS{{ - AccessPermissions: windows.FILE_GENERIC_READ | windows.FILE_EXECUTE, - AccessMode: windows.DENY_ACCESS, - Inheritance: windows.CONTAINER_INHERIT_ACE | windows.OBJECT_INHERIT_ACE, - Trustee: windows.TRUSTEE{ - TrusteeForm: windows.TRUSTEE_IS_SID, - TrusteeType: windows.TRUSTEE_IS_WELL_KNOWN_GROUP, - TrusteeValue: windows.TrusteeValueFromSID(sid), - }, - }}, oldDACL) - if err != nil { - t.Fatalf("ACLFromEntries: %v", err) - } - if err := windows.SetNamedSecurityInfo(parent, windows.SE_FILE_OBJECT, windows.DACL_SECURITY_INFORMATION, nil, nil, parentDACL, nil); err != nil { - t.Fatalf("SetNamedSecurityInfo parent: %v", err) - } - - // Create child directory — it inherits the DenyRead ACE with INHERITED_ACE set. - child := filepath.Join(parent, "child") - if err := os.Mkdir(child, 0o700); err != nil { - t.Fatalf("Mkdir child: %v", err) - } - - // Read child's DACL. - childSD, err := windows.GetNamedSecurityInfo(child, windows.SE_FILE_OBJECT, windows.DACL_SECURITY_INFORMATION) - if err != nil { - t.Fatalf("GetNamedSecurityInfo child: %v", err) - } - childDACL, _, err := childSD.DACL() - if err != nil { - t.Fatalf("child DACL: %v", err) - } - - // Preserving read deny entries on child must skip the inherited ACE. - preserved, err := windowsPreservedReadDenyAccessEntries(childDACL, sid, true) - if err != nil { - t.Fatalf("windowsPreservedReadDenyAccessEntries: %v", err) - } - if len(preserved) != 0 { - t.Fatalf("expected inherited ACE to be skipped, got %d preserved entries: %#v", len(preserved), preserved) - } -} diff --git a/internal/sandbox/windows_acl_paths_other.go b/internal/sandbox/windows_acl_paths_other.go deleted file mode 100644 index f3de23b0c..000000000 --- a/internal/sandbox/windows_acl_paths_other.go +++ /dev/null @@ -1,34 +0,0 @@ -//go:build !windows - -package sandbox - -import "os" - -// resolveWindowsSharedDenyPaths mirrors resolveWindowsSharedDenyPaths from -// windows_acl_paths_windows.go using environment-variable fallbacks. The -// trusted-API resolution used on Windows cannot be exercised on other GOOS, -// but that carries none of the production risk it exists to close: -// BuildWindowsACLPlan's shared-deny-path logic only ever runs for real on -// Windows (applyWindowsACLPlan, which actually mutates a DACL, is itself -// windows-only), so on other platforms this is only reached from unit tests -// that inspect the plan's structure, not from an elevated setup process -// whose environment an attacker might control. -func resolveWindowsSharedDenyPaths() (systemDrive, systemRoot, programData, publicDir string, err error) { - 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, nil -} diff --git a/internal/sandbox/windows_acl_paths_windows.go b/internal/sandbox/windows_acl_paths_windows.go deleted file mode 100644 index 7d7a6d910..000000000 --- a/internal/sandbox/windows_acl_paths_windows.go +++ /dev/null @@ -1,44 +0,0 @@ -//go:build windows - -package sandbox - -import ( - "fmt" - "path/filepath" - - "golang.org/x/sys/windows" -) - -// resolveWindowsSharedDenyPaths resolves the canonical system paths that -// earlier SID-broadening builds protected with shared DenyWrite entries -// (the system drive root, %SystemRoot%\Temp, ProgramData, and the Public -// user profile). SID broadening is disabled, so BuildWindowsACLPlan no -// longer stamps those denies; this resolver remains for tests and for any -// future access-time design that needs the same canonical roots. -// -// Paths are resolved from trusted Win32 APIs (GetSystemWindowsDirectory, -// SHGetKnownFolderPath) rather than SystemDrive/SystemRoot/ProgramData/ -// PUBLIC environment variables, which are ordinary process environment -// state and spoofable by anything that can influence the elevated setup -// process. -func resolveWindowsSharedDenyPaths() (systemDrive, systemRoot, programData, publicDir string, err error) { - // Prefer GetSystemWindowsDirectory: it always returns the shared system - // Windows directory. GetWindowsDirectory can return a private per-session - // path on Terminal Services for non-TSAWARE processes. - windowsDir, err := windows.GetSystemWindowsDirectory() - if err != nil { - return "", "", "", "", fmt.Errorf("resolve system windows directory: %w", err) - } - systemRoot = filepath.Clean(windowsDir) - systemDrive = filepath.VolumeName(systemRoot) - if systemDrive == "" { - return "", "", "", "", fmt.Errorf("resolve system drive from windows directory %q", systemRoot) - } - if programData, err = windows.KnownFolderPath(windows.FOLDERID_ProgramData, 0); err != nil { - return "", "", "", "", fmt.Errorf("resolve ProgramData known folder: %w", err) - } - if publicDir, err = windows.KnownFolderPath(windows.FOLDERID_Public, 0); err != nil { - return "", "", "", "", fmt.Errorf("resolve Public known folder: %w", err) - } - return systemDrive, systemRoot, programData, publicDir, nil -} diff --git a/internal/sandbox/windows_acl_test.go b/internal/sandbox/windows_acl_test.go index 2d819bce8..257198cd6 100644 --- a/internal/sandbox/windows_acl_test.go +++ b/internal/sandbox/windows_acl_test.go @@ -110,78 +110,6 @@ func TestBuildWindowsACLPlanOmitsSharedDenyPathsWhenUnelevated(t *testing.T) { assertNoWindowsACLRevokes(t, plan) } -// windowsSharedDenyPathsForTest calls the same trusted-path resolution -// BuildWindowsACLPlan itself uses, rather than reimplementing the -// resolution logic independently, so this test cannot silently drift out of -// sync with (or mask a regression in) the production resolver. -func windowsSharedDenyPathsForTest(t *testing.T) (systemDrive, systemRoot, programData, publicDir string) { - t.Helper() - systemDrive, systemRoot, programData, publicDir, err := resolveWindowsSharedDenyPaths() - if err != nil { - t.Fatalf("resolveWindowsSharedDenyPaths: %v", err) - } - return systemDrive, systemRoot, programData, publicDir -} - -func TestBuildWindowsACLPlanUsesReadOnlySIDWithoutWriteRoots(t *testing.T) { - home := t.TempDir() - caps, err := LoadOrCreateWindowsCapabilitySIDs(home) - if err != nil { - t.Fatalf("LoadOrCreateWindowsCapabilitySIDs: %v", err) - } - plan, err := BuildWindowsACLPlan(WindowsSandboxCommandConfig{ - SandboxHome: home, - SandboxLevel: WindowsSandboxLevelRestrictedToken, - PermissionProfile: PermissionProfile{ - FileSystem: FileSystemPolicy{ - Kind: FileSystemRestricted, - DenyRead: []string{`C:\workspace\secret-read`}, - }, - Network: NetworkPolicy{Mode: NetworkDeny}, - }, - }) - if err != nil { - t.Fatalf("BuildWindowsACLPlan: %v", err) - } - // Without write roots there is nothing to revoke; without SID broadening - // there are no shared system-path DenyWrite entries either. - if len(plan.Entries) != 1 { - t.Fatalf("ACL entries = %#v, want one deny-read entry", plan.Entries) - } - assertWindowsACLEntry(t, plan, WindowsACLDenyRead, `C:\workspace\secret-read`, caps.ReadOnly, true) - assertNoSharedSystemDenyWrites(t, plan) -} - -// TestBuildWindowsACLPlanDisablesSharedDenyPathDescendantScan pins that SID -// broadening is off: the plan must not request ScanDescendants or shared-root -// DenyWrite entries that only existed to compensate for Users/AuthUsers SIDs. -func TestBuildWindowsACLPlanDisablesSharedDenyPathDescendantScan(t *testing.T) { - home := t.TempDir() - plan, err := BuildWindowsACLPlan(WindowsSandboxCommandConfig{ - SandboxHome: home, - WorkspaceRoots: []string{`C:\workspace`}, - SandboxLevel: WindowsSandboxLevelRestrictedToken, - PermissionProfile: PermissionProfile{ - FileSystem: FileSystemPolicy{ - Kind: FileSystemRestricted, - WriteRoots: []WritableRoot{{Root: `C:\workspace`}}, - DenyRead: []string{`C:\workspace\secret-read`}, - DenyWrite: []string{`C:\workspace\secret-write`}, - }, - Network: NetworkPolicy{Mode: NetworkDeny}, - }, - }) - if err != nil { - t.Fatalf("BuildWindowsACLPlan: %v", err) - } - assertNoSharedSystemDenyWrites(t, plan) - for _, entry := range plan.Entries { - if entry.ScanDescendants { - t.Fatalf("plan entry %#v requests descendant scan; shared DenyWrite compensation is disabled", entry) - } - } -} - // TestBuildWindowsACLPlanDoesNotRevokeLegacyGuards pins that a setup run does // not remove persistent guards installed by an older build. A previously // launched sandbox can still carry the legacy capability SID, so removing its @@ -208,8 +136,7 @@ func TestBuildWindowsACLPlanDoesNotRevokeLegacyGuards(t *testing.T) { func assertNoSharedSystemDenyWrites(t *testing.T, plan WindowsACLPlan) { t.Helper() - systemDrive, systemRoot, programData, publicDir := windowsSharedDenyPathsForTest(t) - for _, path := range []string{systemDrive + `\`, programData, systemRoot + `\Temp`, publicDir} { + for _, path := range []string{`C:\`, `C:\ProgramData`, `C:\Windows\Temp`, `C:\Users\Public`} { for _, entry := range plan.Entries { if entry.Action == WindowsACLDenyWrite && windowsCapabilityPathKey(entry.Path) == windowsCapabilityPathKey(path) { t.Fatalf("plan stamps shared system DenyWrite on %q = %#v; SID broadening is disabled so shared denies must not be planned", path, entry) diff --git a/internal/sandbox/windows_command_runner.go b/internal/sandbox/windows_command_runner.go index cc1a66de9..21397040c 100644 --- a/internal/sandbox/windows_command_runner.go +++ b/internal/sandbox/windows_command_runner.go @@ -55,7 +55,7 @@ func windowsDenyReadRestrictedTokenUnsupportedProfile(profile PermissionProfile) "restricting SID set, ordinary system binaries under Program Files and "+ "Windows cannot load, and adding those groups would admit their existing "+ "write grants outside WriteRoots. "+ - "Remove DenyRead from this configuration or use the documented sandbox_permissions: \"require_escalated\" approval flow; that flow cannot preserve DenyRead. "+ + "Remove DenyRead from this configuration to use the sandbox on Windows. "+ "Configured DenyRead path count: %d", len(profile.FileSystem.DenyRead), ) diff --git a/internal/sandbox/windows_command_runner_test.go b/internal/sandbox/windows_command_runner_test.go index 4ca21c595..b6cec3645 100644 --- a/internal/sandbox/windows_command_runner_test.go +++ b/internal/sandbox/windows_command_runner_test.go @@ -38,8 +38,11 @@ func TestWindowsDenyReadRestrictedTokenUnsupported(t *testing.T) { if strings.Contains(msg, "--sandbox forbid") { t.Fatalf("%s error advertises unsupported --sandbox forbid recovery: %q", level, msg) } - if !strings.Contains(msg, "sandbox_permissions") { - t.Fatalf("%s error should name the documented sandbox_permissions recovery flow: %q", level, msg) + if strings.Contains(msg, "sandbox_permissions") || strings.Contains(msg, "require_escalated") { + t.Fatalf("%s error advertises unusable escalation recovery: %q", level, msg) + } + if !strings.Contains(msg, "Remove DenyRead") { + t.Fatalf("%s error should advise removing DenyRead: %q", level, msg) } } diff --git a/internal/sandbox/windows_setup.go b/internal/sandbox/windows_setup.go index be4cdd6c9..ac7366a7c 100644 --- a/internal/sandbox/windows_setup.go +++ b/internal/sandbox/windows_setup.go @@ -15,7 +15,7 @@ import ( const WindowsSandboxSetupName = "zero-windows-sandbox-setup.exe" -const windowsSandboxSetupMarkerSchemaVersion = 4 +const windowsSandboxSetupMarkerSchemaVersion = 5 type WindowsSandboxSetupArgsOptions struct { SandboxHome string diff --git a/internal/sandbox/windows_setup_test.go b/internal/sandbox/windows_setup_test.go index 251098a2e..ecb76831a 100644 --- a/internal/sandbox/windows_setup_test.go +++ b/internal/sandbox/windows_setup_test.go @@ -157,8 +157,8 @@ func TestWindowsSandboxSetupMarkerValidatesBothNetworkModes(t *testing.T) { } } -// A pre-v4 marker on disk must be rejected as out of date so the schema bump -// forces a clean re-setup (old markers scoped the filter to write SIDs). +// A pre-v5 marker on disk must be rejected as out of date so the schema bump +// forces a clean re-setup (old markers had legacy DenyWrite ACEs). func TestWindowsSandboxSetupMarkerRejectsOldSchema(t *testing.T) { config := WindowsSandboxSetupConfig{ SandboxHome: t.TempDir(), @@ -173,7 +173,7 @@ func TestWindowsSandboxSetupMarkerRejectsOldSchema(t *testing.T) { if err != nil { t.Fatalf("BuildWindowsSandboxSetupMarker: %v", err) } - marker.SchemaVersion = 3 + marker.SchemaVersion = 4 bytes, err := json.Marshal(marker) if err != nil { t.Fatalf("marshal: %v", err) @@ -183,7 +183,7 @@ func TestWindowsSandboxSetupMarkerRejectsOldSchema(t *testing.T) { } err = ValidateWindowsSandboxSetupMarker(config) if err == nil || !strings.Contains(err.Error(), "out of date") { - t.Fatalf("schema-3 marker must be out of date, got: %v", err) + t.Fatalf("schema-4 marker must be out of date, got: %v", err) } } diff --git a/internal/sandbox/windows_unelevated.go b/internal/sandbox/windows_unelevated.go index 980664d6a..b7cb05f53 100644 --- a/internal/sandbox/windows_unelevated.go +++ b/internal/sandbox/windows_unelevated.go @@ -11,7 +11,7 @@ import ( "github.com/Gitlawb/zero/internal/fsutil" ) -const windowsUnelevatedSetupMarkerSchemaVersion = 1 +const windowsUnelevatedSetupMarkerSchemaVersion = 2 // windowsUnelevatedSetupMarkerMaxPlans bounds the applied-plan history so the // marker cannot grow without limit when a user hops between many workspaces. From 6c4d6c39411b7570cdfe0f2143d859677e10fce5 Mon Sep 17 00:00:00 2001 From: euxaristia Date: Fri, 4 Sep 2026 15:00:02 -0400 Subject: [PATCH 7/9] Use /d /c for denied-write probe command and verify ownership on cleanup. Refs #640 --- internal/sandbox/runner_windows_integration_test.go | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/internal/sandbox/runner_windows_integration_test.go b/internal/sandbox/runner_windows_integration_test.go index b34da8edf..e9ac60eb4 100644 --- a/internal/sandbox/runner_windows_integration_test.go +++ b/internal/sandbox/runner_windows_integration_test.go @@ -4,6 +4,7 @@ package sandbox import ( "context" + "errors" "fmt" "net" "os" @@ -573,7 +574,7 @@ const deniedWriteExitCode = 77 // deniedWriteCommand attempts a write and reports deniedWriteExitCode when the // redirect is refused, so the exit code also proves cmd.exe actually ran. func deniedWriteCommand(marker string) []string { - return []string{"cmd.exe", "/d", "/s", "/c", "echo leaked>" + cmdQuote(marker) + " || exit " + strconv.Itoa(deniedWriteExitCode)} + return []string{"cmd.exe", "/d", "/c", "echo leaked>" + cmdQuote(marker) + " || exit " + strconv.Itoa(deniedWriteExitCode)} } func cmdQuote(path string) string { @@ -613,8 +614,13 @@ func (p *sharedDirectoryProbe) Path() string { func (p *sharedDirectoryProbe) cleanup(t testing.TB) { t.Helper() - if _, err := os.Lstat(p.path); err == nil { - _ = os.Remove(p.path) + data, err := os.ReadFile(p.path) + if err == nil { + if strings.HasPrefix(string(data), "leaked") { + if err := os.Remove(p.path); err != nil && !errors.Is(err, os.ErrNotExist) { + t.Errorf("cleanup probe %s: %v", p.path, err) + } + } } } From 92bdafab5ae002b22cc6b4d3c4ed2e55b29a7da3 Mon Sep 17 00:00:00 2001 From: euxaristia Date: Fri, 4 Sep 2026 16:27:14 -0400 Subject: [PATCH 8/9] Honor NoInherit in legacy ACL migration and verify probe ownership. Refs #639 --- .../runner_windows_integration_test.go | 52 +++++++--- internal/sandbox/windows_acl_apply_windows.go | 87 +++++++++++----- .../sandbox/windows_acl_apply_windows_test.go | 98 +++++++++++++++++++ 3 files changed, 200 insertions(+), 37 deletions(-) diff --git a/internal/sandbox/runner_windows_integration_test.go b/internal/sandbox/runner_windows_integration_test.go index e9ac60eb4..cd6d0388e 100644 --- a/internal/sandbox/runner_windows_integration_test.go +++ b/internal/sandbox/runner_windows_integration_test.go @@ -84,7 +84,7 @@ func TestWindowsRestrictedTokenRealSandboxSmoke(t *testing.T) { t.Log("PUBLIC is not set; skipping C:\\Users\\Public write-jail probe") } else { publicProbe := allocateSharedDirectoryProbe(t, publicDir, "elevated-public") - runWindowsRealSmokeCommand(t, runnerExe, config, deniedWriteCommand(publicProbe.Path()), deniedWriteExitCode) + runWindowsRealSmokeCommand(t, runnerExe, config, publicProbe.DeniedWriteCommand(), deniedWriteExitCode) if _, err := os.Stat(publicProbe.Path()); err == nil { t.Fatalf("Windows sandbox allowed a write to the shared C:\\Users\\Public directory") } else if !os.IsNotExist(err) { @@ -238,7 +238,7 @@ func TestWindowsUnelevatedRealSandboxSmoke(t *testing.T) { programData := os.Getenv("ProgramData") if programData != "" { programDataProbe := allocateSharedDirectoryProbe(t, programData, "unelevated-programdata") - runWindowsRealSmokeCommand(t, runnerExe, config, deniedWriteCommand(programDataProbe.Path()), deniedWriteExitCode) + runWindowsRealSmokeCommand(t, runnerExe, config, programDataProbe.DeniedWriteCommand(), deniedWriteExitCode) if _, err := os.Stat(programDataProbe.Path()); err == nil { t.Fatalf("unelevated sandbox allowed a write to ProgramData shared directory") } else if !os.IsNotExist(err) { @@ -582,7 +582,8 @@ func cmdQuote(path string) string { } type sharedDirectoryProbe struct { - path string + path string + marker string } func allocateSharedDirectoryProbe(t testing.TB, dir, prefix string) *sharedDirectoryProbe { @@ -601,7 +602,8 @@ func allocateSharedDirectoryProbe(t testing.TB, dir, prefix string) *sharedDirec if probePath == "" { t.Fatalf("allocate shared directory probe in %s: failed to find unused filename", dir) } - p := &sharedDirectoryProbe{path: probePath} + marker := fmt.Sprintf("leaked-%s-%d-%d", prefix, os.Getpid(), time.Now().UnixNano()) + p := &sharedDirectoryProbe{path: probePath, marker: marker} t.Cleanup(func() { p.cleanup(t) }) @@ -612,14 +614,26 @@ func (p *sharedDirectoryProbe) Path() string { return p.path } +func (p *sharedDirectoryProbe) Marker() string { + return p.marker +} + +func (p *sharedDirectoryProbe) DeniedWriteCommand() []string { + return []string{"cmd.exe", "/d", "/c", "echo " + p.marker + ">" + cmdQuote(p.path) + " || exit " + strconv.Itoa(deniedWriteExitCode)} +} + func (p *sharedDirectoryProbe) cleanup(t testing.TB) { t.Helper() data, err := os.ReadFile(p.path) - if err == nil { - if strings.HasPrefix(string(data), "leaked") { - if err := os.Remove(p.path); err != nil && !errors.Is(err, os.ErrNotExist) { - t.Errorf("cleanup probe %s: %v", p.path, err) - } + if err != nil { + if !errors.Is(err, os.ErrNotExist) { + t.Errorf("cleanup probe read %s: %v", p.path, err) + } + return + } + if strings.TrimSpace(string(data)) == p.marker { + if err := os.Remove(p.path); err != nil && !errors.Is(err, os.ErrNotExist) { + t.Errorf("cleanup probe remove %s: %v", p.path, err) } } } @@ -639,12 +653,15 @@ func powershellSingleQuote(value string) string { func TestSharedDirectoryProbeLifecycle(t *testing.T) { dir := t.TempDir() - // 1. Two simultaneous allocations produce distinct non-colliding paths. + // 1. Two simultaneous allocations produce distinct non-colliding paths and markers. probe1 := allocateSharedDirectoryProbe(t, dir, "p1") probe2 := allocateSharedDirectoryProbe(t, dir, "p2") if probe1.Path() == probe2.Path() { t.Fatalf("expected distinct probe paths, got %q and %q", probe1.Path(), probe2.Path()) } + if probe1.Marker() == probe2.Marker() { + t.Fatalf("expected distinct probe markers, got %q and %q", probe1.Marker(), probe2.Marker()) + } // 2. Pre-existing unrelated file is never selected or deleted. unrelatedFile := filepath.Join(dir, "unrelated.txt") @@ -661,13 +678,24 @@ func TestSharedDirectoryProbeLifecycle(t *testing.T) { probe4 := allocateSharedDirectoryProbe(t, dir, "p4") probe4.cleanup(t) - // 4. Unexpected write created during test is cleaned up. + // 4. Unexpected write created during test with matching marker is cleaned up. probe5 := allocateSharedDirectoryProbe(t, dir, "p5") - if err := os.WriteFile(probe5.Path(), []byte("leaked"), 0o600); err != nil { + if err := os.WriteFile(probe5.Path(), []byte(probe5.Marker()), 0o600); err != nil { t.Fatalf("write probe5 file: %v", err) } probe5.cleanup(t) if _, err := os.Lstat(probe5.Path()); !os.IsNotExist(err) { t.Fatalf("expected probe5 to be cleaned up after creation, stat err=%v", err) } + + // 5. File at probe path with foreign/unmatched content is NOT removed. + probe6 := allocateSharedDirectoryProbe(t, dir, "p6") + if err := os.WriteFile(probe6.Path(), []byte("unrelated-foreign-content"), 0o600); err != nil { + t.Fatalf("write probe6 file: %v", err) + } + probe6.cleanup(t) + if _, err := os.Lstat(probe6.Path()); err != nil { + t.Fatalf("expected probe6 with foreign content to be preserved, stat err=%v", err) + } + _ = os.Remove(probe6.Path()) } diff --git a/internal/sandbox/windows_acl_apply_windows.go b/internal/sandbox/windows_acl_apply_windows.go index facd166ff..2a6f71461 100644 --- a/internal/sandbox/windows_acl_apply_windows.go +++ b/internal/sandbox/windows_acl_apply_windows.go @@ -232,7 +232,7 @@ func prepareWindowsACLPathGroupEntries(entries []WindowsACLEntry, isDir bool, ol // 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) + migrated, err := windowsMigrateDenyWriteInDACL(baseDACL, sid, isDir, entry.NoInherit) if err != nil { return nil, nil, err } @@ -322,11 +322,11 @@ func windowsFilterDACL(oldDACL *windows.ACL, removeSID *windows.SID) (*windows.A } // windowsMigrateDenyWriteInDACL copies oldDACL and narrows any explicit -// deny-write ACE for targetSID to the current narrow mask, preserving all -// other ACEs (including DenyRead) in their original positions. This avoids -// SetEntriesInAcl's merging behavior that would combine separate deny ACEs -// for the same SID into a single full-deny ACE. -func windowsMigrateDenyWriteInDACL(oldDACL *windows.ACL, targetSID *windows.SID) (*windows.ACL, error) { +// deny-write ACE for targetSID to the current narrow mask, carrying requested +// inheritance (isDir, noInherit) into the migrated ACE. It drops inherit-only +// ACEs when no inheritance is requested, avoiding propagation to children. +// All other ACEs (including DenyRead) are preserved in their original positions. +func windowsMigrateDenyWriteInDACL(oldDACL *windows.ACL, targetSID *windows.SID, isDir bool, noInherit bool) (*windows.ACL, error) { if oldDACL == nil || targetSID == nil { return oldDACL, nil } @@ -335,33 +335,70 @@ func windowsMigrateDenyWriteInDACL(oldDACL *windows.ACL, targetSID *windows.SID) return nil, err } - oldHdr := (*windowsACLHeader)(unsafe.Pointer(oldDACL)) - buf := make([]byte, oldHdr.AclSize) - src := unsafe.Slice((*byte)(unsafe.Pointer(oldDACL)), oldHdr.AclSize) - copy(buf, src) + const inheritFlags = byte(windows.OBJECT_INHERIT_ACE | windows.CONTAINER_INHERIT_ACE | windows.INHERIT_ONLY_ACE | windows.NO_PROPAGATE_INHERIT_ACE) + shouldDropInheritOnly := noInherit || !isDir - newDACL := (*windows.ACL)(unsafe.Pointer(&buf[0])) - for i := uint32(0); i < uint32(newDACL.AceCount); i++ { + var keepBytes uint32 = uint32(unsafe.Sizeof(windowsACLHeader{})) + var keepCount uint16 = 0 + for i := uint32(0); i < uint32(oldDACL.AceCount); i++ { var ace *windows.ACCESS_ALLOWED_ACE - if err := windows.GetAce(newDACL, i, &ace); err != nil { - return nil, fmt.Errorf("read ACE %d for migration: %w", i, err) - } - if ace.Header.AceFlags&windows.INHERITED_ACE != 0 { - continue + if err := windows.GetAce(oldDACL, i, &ace); err != nil { + return nil, fmt.Errorf("read ACE %d for migration size: %w", i, err) } - if ace.Header.AceType != windows.ACCESS_DENIED_ACE_TYPE && ace.Header.AceType != windowsAccessDeniedObjectAceType { - continue + if ace.Header.AceFlags&windows.INHERITED_ACE == 0 && + (ace.Header.AceType == windows.ACCESS_DENIED_ACE_TYPE || ace.Header.AceType == windowsAccessDeniedObjectAceType) { + if sid, ok := windowsAceSID(ace); ok && sid.Equals(targetSID) && windowsIsExperimentalWriteDenyMask(ace.Mask) { + if shouldDropInheritOnly && (ace.Header.AceFlags&windows.INHERIT_ONLY_ACE != 0) { + // Drop inherit-only ACE when no inheritance is requested on the target + continue + } + } } - sid, ok := windowsAceSID(ace) - if !ok || !sid.Equals(targetSID) { - continue + keepBytes += uint32(ace.Header.AceSize) + keepCount++ + } + + buf := make([]byte, keepBytes) + hdr := (*windowsACLHeader)(unsafe.Pointer(&buf[0])) + oldHdr := (*windowsACLHeader)(unsafe.Pointer(oldDACL)) + *hdr = *oldHdr + hdr.AclSize = uint16(keepBytes) + hdr.AceCount = keepCount + + offset := unsafe.Sizeof(windowsACLHeader{}) + for i := uint32(0); i < uint32(oldDACL.AceCount); i++ { + var ace *windows.ACCESS_ALLOWED_ACE + if err := windows.GetAce(oldDACL, i, &ace); err != nil { + return nil, fmt.Errorf("read ACE %d for migration copy: %w", i, err) } - if windowsIsExperimentalWriteDenyMask(ace.Mask) { - ace.Mask = narrowMask + aceSize := uintptr(ace.Header.AceSize) + if ace.Header.AceFlags&windows.INHERITED_ACE == 0 && + (ace.Header.AceType == windows.ACCESS_DENIED_ACE_TYPE || ace.Header.AceType == windowsAccessDeniedObjectAceType) { + if sid, ok := windowsAceSID(ace); ok && sid.Equals(targetSID) && windowsIsExperimentalWriteDenyMask(ace.Mask) { + if shouldDropInheritOnly && (ace.Header.AceFlags&windows.INHERIT_ONLY_ACE != 0) { + continue + } + dest := buf[offset : offset+aceSize] + srcSlice := unsafe.Slice((*byte)(unsafe.Pointer(ace)), aceSize) + copy(dest, srcSlice) + migratedAce := (*windows.ACCESS_ALLOWED_ACE)(unsafe.Pointer(&dest[0])) + migratedAce.Mask = narrowMask + if noInherit || !isDir { + migratedAce.Header.AceFlags &^= inheritFlags + } else if isDir { + migratedAce.Header.AceFlags |= byte(windows.OBJECT_INHERIT_ACE | windows.CONTAINER_INHERIT_ACE) + } + offset += aceSize + continue + } } + + srcSlice := unsafe.Slice((*byte)(unsafe.Pointer(ace)), aceSize) + copy(buf[offset:offset+aceSize], srcSlice) + offset += aceSize } - return newDACL, nil + return (*windows.ACL)(unsafe.Pointer(hdr)), nil } func windowsHasExplicitDenyWriteForSID(oldDACL *windows.ACL, wantSID *windows.SID) bool { diff --git a/internal/sandbox/windows_acl_apply_windows_test.go b/internal/sandbox/windows_acl_apply_windows_test.go index a772d7c2e..25dd01c14 100644 --- a/internal/sandbox/windows_acl_apply_windows_test.go +++ b/internal/sandbox/windows_acl_apply_windows_test.go @@ -335,3 +335,101 @@ func TestWindowsACLDenyWriteMigratesLegacySynchronizeMask(t *testing.T) { t.Fatalf("second apply changed ACE count: %d vs %d", dacl2.AceCount, dacl.AceCount) } } + +// TestWindowsACLDenyWriteMigratesLegacyNoInherit regression tests that when an +// existing directory carries a legacy inheritable deny-write ACE, migrating it +// with NoInherit: true removes any inherit-only ACEs and clears inheritance flags +// on the direct ACE so child files/directories do not inherit the deny. +func TestWindowsACLDenyWriteMigratesLegacyNoInherit(t *testing.T) { + dir := t.TempDir() + childDir := filepath.Join(dir, "sub") + if err := os.Mkdir(childDir, 0o755); err != nil { + t.Fatalf("mkdir childDir: %v", err) + } + childFile := filepath.Join(childDir, "child.txt") + if err := os.WriteFile(childFile, []byte("data"), 0o644); err != nil { + t.Fatalf("write childFile: %v", err) + } + + caps, err := LoadOrCreateWindowsCapabilitySIDs(t.TempDir()) + if err != nil { + t.Fatalf("LoadOrCreateWindowsCapabilitySIDs: %v", err) + } + sidStr := caps.ReadOnly + sid, err := windows.StringToSid(sidStr) + if err != nil { + t.Fatalf("StringToSid: %v", err) + } + + // 1. Seed legacy inheritable DenyWrite ACEs: direct + inherit-only. + legacyWriteMask := (windows.FILE_GENERIC_WRITE | windows.DELETE | windowsFileDeleteChild | windows.WRITE_DAC | windows.WRITE_OWNER | windows.SYNCHRONIZE) + seedEntries := []windows.EXPLICIT_ACCESS{ + { + AccessPermissions: legacyWriteMask, + AccessMode: windows.DENY_ACCESS, + Inheritance: windows.SUB_CONTAINERS_AND_OBJECTS_INHERIT, + Trustee: windows.TRUSTEE{ + TrusteeForm: windows.TRUSTEE_IS_SID, + TrusteeType: windows.TRUSTEE_IS_GROUP, + TrusteeValue: windows.TrusteeValueFromSID(sid), + }, + }, + } + handle, _, err := openWindowsACLTarget(dir) + if err != nil { + t.Fatalf("openWindowsACLTarget: %v", err) + } + seededDACL, err := windows.ACLFromEntries(seedEntries, nil) + if err != nil { + _ = windows.CloseHandle(handle) + t.Fatalf("ACLFromEntries: %v", err) + } + if err := windows.SetSecurityInfo(handle, windows.SE_FILE_OBJECT, windows.DACL_SECURITY_INFORMATION, nil, nil, seededDACL, nil); err != nil { + _ = windows.CloseHandle(handle) + t.Fatalf("SetSecurityInfo: %v", err) + } + _ = windows.CloseHandle(handle) + + // 2. Apply WindowsACLPlan with NoInherit: true. + plan := WindowsACLPlan{ + Entries: []WindowsACLEntry{{ + Action: WindowsACLDenyWrite, + Path: dir, + Capability: sidStr, + NoInherit: true, + }}, + } + rollback, err := applyWindowsACLPlan(plan) + if err != nil { + t.Fatalf("applyWindowsACLPlan: %v", err) + } + t.Cleanup(func() { _ = rollback() }) + + // 3. Verify effective DACL on dir: must have NO inheritance flags and no inherit-only ACE. + sd, err := windows.GetNamedSecurityInfo(dir, windows.SE_FILE_OBJECT, windows.DACL_SECURITY_INFORMATION) + if err != nil { + t.Fatalf("GetNamedSecurityInfo: %v", err) + } + dacl, _, err := sd.DACL() + if err != nil { + t.Fatalf("DACL: %v", err) + } + + const inheritFlags = windows.OBJECT_INHERIT_ACE | windows.CONTAINER_INHERIT_ACE | windows.INHERIT_ONLY_ACE + for i := uint16(0); i < dacl.AceCount; i++ { + var ace *windows.ACCESS_ALLOWED_ACE + if err := windows.GetAce(dacl, uint32(i), &ace); err != nil { + t.Fatalf("GetAce: %v", err) + } + if ace.Header.AceType != windows.ACCESS_DENIED_ACE_TYPE && ace.Header.AceType != windowsAccessDeniedObjectAceType { + continue + } + aceSID, ok := windowsAceSID(ace) + if !ok || !aceSID.Equals(sid) { + continue + } + if ace.Header.AceFlags&inheritFlags != 0 { + t.Fatalf("migrated ACE for NoInherit entry retained inheritance flags: 0x%x", ace.Header.AceFlags) + } + } +} From 5c46b75c273d53cf38f7d16410ef945d35a85fb9 Mon Sep 17 00:00:00 2001 From: euxaristia Date: Fri, 4 Sep 2026 16:54:07 -0400 Subject: [PATCH 9/9] Preserve DenyRead bits when migrating combined legacy deny ACEs. Refs #639 --- internal/sandbox/windows_acl_apply_windows.go | 79 ++++++++--- .../sandbox/windows_acl_apply_windows_test.go | 128 ++++++++++++++++++ 2 files changed, 186 insertions(+), 21 deletions(-) diff --git a/internal/sandbox/windows_acl_apply_windows.go b/internal/sandbox/windows_acl_apply_windows.go index 2a6f71461..dcacf68e9 100644 --- a/internal/sandbox/windows_acl_apply_windows.go +++ b/internal/sandbox/windows_acl_apply_windows.go @@ -283,8 +283,10 @@ func windowsFilterDACL(oldDACL *windows.ACL, removeSID *windows.SID) (*windows.A } if ace.Header.AceFlags&windows.INHERITED_ACE == 0 { if ace.Header.AceType == windows.ACCESS_DENIED_ACE_TYPE || ace.Header.AceType == windowsAccessDeniedObjectAceType { - if sid, ok := windowsAceSID(ace); ok && sid.Equals(removeSID) && windowsIsExperimentalWriteDenyMask(ace.Mask) { - continue + if sid, ok := windowsAceSID(ace); ok && sid.Equals(removeSID) { + if windowsIsExperimentalWriteDenyMask(ace.Mask) { + continue + } } } } @@ -305,14 +307,28 @@ func windowsFilterDACL(oldDACL *windows.ACL, removeSID *windows.SID) (*windows.A if err := windows.GetAce(oldDACL, i, &ace); err != nil { return nil, fmt.Errorf("read ACE %d for copy: %w", i, err) } + aceSize := uintptr(ace.Header.AceSize) if ace.Header.AceFlags&windows.INHERITED_ACE == 0 { if ace.Header.AceType == windows.ACCESS_DENIED_ACE_TYPE || ace.Header.AceType == windowsAccessDeniedObjectAceType { - if sid, ok := windowsAceSID(ace); ok && sid.Equals(removeSID) && windowsIsExperimentalWriteDenyMask(ace.Mask) { - continue + if sid, ok := windowsAceSID(ace); ok && sid.Equals(removeSID) { + if windowsIsExperimentalWriteDenyMask(ace.Mask) { + continue + } + // If this is a combined read/write deny ACE, preserve its DenyRead + // bits by stripping the write-denial bits rather than dropping the ACE. + if ace.Mask&windowsReadContentBits != 0 && windowsHasWriteDenyBits(ace.Mask) { + dest := buf[offset : offset+aceSize] + srcSlice := unsafe.Slice((*byte)(unsafe.Pointer(ace)), aceSize) + copy(dest, srcSlice) + migratedAce := (*windows.ACCESS_ALLOWED_ACE)(unsafe.Pointer(&dest[0])) + const legacyWriteMask = windows.FILE_GENERIC_WRITE | windows.DELETE | windowsFileDeleteChild | windows.WRITE_DAC | windows.WRITE_OWNER + migratedAce.Mask &^= legacyWriteMask + offset += aceSize + continue + } } } } - aceSize := uintptr(ace.Header.AceSize) srcSlice := unsafe.Slice((*byte)(unsafe.Pointer(ace)), aceSize) copy(buf[offset:offset+aceSize], srcSlice) offset += aceSize @@ -347,7 +363,7 @@ func windowsMigrateDenyWriteInDACL(oldDACL *windows.ACL, targetSID *windows.SID, } if ace.Header.AceFlags&windows.INHERITED_ACE == 0 && (ace.Header.AceType == windows.ACCESS_DENIED_ACE_TYPE || ace.Header.AceType == windowsAccessDeniedObjectAceType) { - if sid, ok := windowsAceSID(ace); ok && sid.Equals(targetSID) && windowsIsExperimentalWriteDenyMask(ace.Mask) { + if sid, ok := windowsAceSID(ace); ok && sid.Equals(targetSID) && windowsHasWriteDenyBits(ace.Mask) { if shouldDropInheritOnly && (ace.Header.AceFlags&windows.INHERIT_ONLY_ACE != 0) { // Drop inherit-only ACE when no inheritance is requested on the target continue @@ -374,7 +390,7 @@ func windowsMigrateDenyWriteInDACL(oldDACL *windows.ACL, targetSID *windows.SID, aceSize := uintptr(ace.Header.AceSize) if ace.Header.AceFlags&windows.INHERITED_ACE == 0 && (ace.Header.AceType == windows.ACCESS_DENIED_ACE_TYPE || ace.Header.AceType == windowsAccessDeniedObjectAceType) { - if sid, ok := windowsAceSID(ace); ok && sid.Equals(targetSID) && windowsIsExperimentalWriteDenyMask(ace.Mask) { + if sid, ok := windowsAceSID(ace); ok && sid.Equals(targetSID) && windowsHasWriteDenyBits(ace.Mask) { if shouldDropInheritOnly && (ace.Header.AceFlags&windows.INHERIT_ONLY_ACE != 0) { continue } @@ -383,6 +399,12 @@ func windowsMigrateDenyWriteInDACL(oldDACL *windows.ACL, targetSID *windows.SID, copy(dest, srcSlice) migratedAce := (*windows.ACCESS_ALLOWED_ACE)(unsafe.Pointer(&dest[0])) migratedAce.Mask = narrowMask + if ace.Mask&windowsReadContentBits != 0 { + _, readMask, _ := windowsACLAccess(WindowsACLDenyRead) + migratedAce.Mask |= (ace.Mask & readMask) + } + const legacyWriteMask = windows.FILE_GENERIC_WRITE | windows.DELETE | windowsFileDeleteChild | windows.WRITE_DAC | windows.WRITE_OWNER + migratedAce.Mask |= (ace.Mask &^ legacyWriteMask) if noInherit || !isDir { migratedAce.Header.AceFlags &^= inheritFlags } else if isDir { @@ -420,7 +442,7 @@ func windowsHasExplicitDenyWriteForSID(oldDACL *windows.ACL, wantSID *windows.SI if !ok || !sid.Equals(wantSID) { continue } - if windowsIsExperimentalWriteDenyMask(ace.Mask) { + if windowsHasWriteDenyBits(ace.Mask) { return true } } @@ -466,8 +488,11 @@ func windowsPreservedReadDenyAccessEntries(oldDACL *windows.ACL, wantSID *window windows.NO_PROPAGATE_INHERIT_ACE | windows.INHERIT_ONLY_ACE) } + mask := ace.Mask + const legacyWriteMask = windows.FILE_GENERIC_WRITE | windows.DELETE | windowsFileDeleteChild | windows.WRITE_DAC | windows.WRITE_OWNER + mask &^= legacyWriteMask out = append(out, windows.EXPLICIT_ACCESS{ - AccessPermissions: ace.Mask, + AccessPermissions: mask, AccessMode: windows.DENY_ACCESS, Inheritance: inheritance, Trustee: windows.TRUSTEE{ @@ -480,12 +505,17 @@ func windowsPreservedReadDenyAccessEntries(oldDACL *windows.ACL, wantSID *window return out, nil } -// windowsIsExperimentalWriteDenyMask reports whether mask is a synthetic -// DenyWrite (or partial write deny) from earlier broadening builds — the only -// ACEs migration revoke may drop for the stable ReadOnly SID. Pure DenyRead -// masks share some STANDARD_RIGHTS bits with FILE_GENERIC_WRITE, so this keys -// off content-write / delete / DAC bits that DenyRead never carries. -func windowsIsExperimentalWriteDenyMask(mask windows.ACCESS_MASK) bool { +const ( + windowsWriteContentBits = windows.FILE_WRITE_DATA | windows.FILE_APPEND_DATA | + windows.FILE_WRITE_EA | windows.FILE_WRITE_ATTRIBUTES | + windowsFileDeleteChild | windows.DELETE | windows.WRITE_DAC | windows.WRITE_OWNER + windowsReadContentBits = windows.FILE_READ_DATA | windows.FILE_READ_EA | windows.FILE_EXECUTE +) + +// windowsHasWriteDenyBits reports whether mask contains content-write, delete, +// or ownership deny bits that indicate write denial (whether pure or combined +// with read denial). +func windowsHasWriteDenyBits(mask windows.ACCESS_MASK) bool { _, writeMask, err := windowsACLAccess(WindowsACLDenyWrite) if err != nil { return false @@ -493,12 +523,19 @@ func windowsIsExperimentalWriteDenyMask(mask windows.ACCESS_MASK) bool { if mask&writeMask == writeMask { return true } - // Content-write / ownership bits unique to write denies (not in DenyRead's - // FILE_GENERIC_READ|FILE_GENERIC_EXECUTE mask alone). - const writeContent = windows.FILE_WRITE_DATA | windows.FILE_APPEND_DATA | - windows.FILE_WRITE_EA | windows.FILE_WRITE_ATTRIBUTES | - windowsFileDeleteChild | windows.DELETE | windows.WRITE_DAC | windows.WRITE_OWNER - return mask&writeContent != 0 + return mask&windowsWriteContentBits != 0 +} + +// windowsIsExperimentalWriteDenyMask reports whether mask is a synthetic +// DenyWrite (or partial write deny) from earlier broadening builds without any +// co-resident DenyRead bits — the only ACEs migration revoke may drop for the +// stable ReadOnly SID. If the mask also denies read-content bits, it is a combined +// read/write deny rather than a pure experimental write deny. +func windowsIsExperimentalWriteDenyMask(mask windows.ACCESS_MASK) bool { + if !windowsHasWriteDenyBits(mask) { + return false + } + return mask&windowsReadContentBits == 0 } func windowsAceSID(ace *windows.ACCESS_ALLOWED_ACE) (sid *windows.SID, ok bool) { diff --git a/internal/sandbox/windows_acl_apply_windows_test.go b/internal/sandbox/windows_acl_apply_windows_test.go index 25dd01c14..1d57b2350 100644 --- a/internal/sandbox/windows_acl_apply_windows_test.go +++ b/internal/sandbox/windows_acl_apply_windows_test.go @@ -433,3 +433,131 @@ func TestWindowsACLDenyWriteMigratesLegacyNoInherit(t *testing.T) { } } } + +// TestWindowsACLDenyWriteMigratesCombinedLegacyReadWriteDeny regression tests +// that migrating a pre-existing DACL with a single combined legacy read/write +// deny ACE preserves the DenyRead bits while narrowing only the write denial bits. +func TestWindowsACLDenyWriteMigratesCombinedLegacyReadWriteDeny(t *testing.T) { + dir := t.TempDir() + probeFile := filepath.Join(dir, "probe.txt") + if err := os.WriteFile(probeFile, []byte("content"), 0o644); err != nil { + t.Fatalf("write probeFile: %v", err) + } + + caps, err := LoadOrCreateWindowsCapabilitySIDs(t.TempDir()) + if err != nil { + t.Fatalf("LoadOrCreateWindowsCapabilitySIDs: %v", err) + } + sidStr := caps.ReadOnly + sid, err := windows.StringToSid(sidStr) + if err != nil { + t.Fatalf("StringToSid: %v", err) + } + + _, readMask, err := windowsACLAccess(WindowsACLDenyRead) + if err != nil { + t.Fatalf("windowsACLAccess DenyRead: %v", err) + } + _, narrowWriteMask, err := windowsACLAccess(WindowsACLDenyWrite) + if err != nil { + t.Fatalf("windowsACLAccess DenyWrite: %v", err) + } + + // 1. Seed a single combined legacy read/write deny ACE. + legacyWriteMask := windows.FILE_GENERIC_WRITE | windows.DELETE | windowsFileDeleteChild | windows.WRITE_DAC | windows.WRITE_OWNER | windows.SYNCHRONIZE + combinedMask := readMask | legacyWriteMask + seedEntries := []windows.EXPLICIT_ACCESS{ + { + AccessPermissions: combinedMask, + AccessMode: windows.DENY_ACCESS, + Inheritance: windows.SUB_CONTAINERS_AND_OBJECTS_INHERIT, + Trustee: windows.TRUSTEE{ + TrusteeForm: windows.TRUSTEE_IS_SID, + TrusteeType: windows.TRUSTEE_IS_GROUP, + TrusteeValue: windows.TrusteeValueFromSID(sid), + }, + }, + } + handle, _, err := openWindowsACLTarget(dir) + if err != nil { + t.Fatalf("openWindowsACLTarget: %v", err) + } + seededDACL, err := windows.ACLFromEntries(seedEntries, nil) + if err != nil { + _ = windows.CloseHandle(handle) + t.Fatalf("ACLFromEntries: %v", err) + } + if err := windows.SetSecurityInfo(handle, windows.SE_FILE_OBJECT, windows.DACL_SECURITY_INFORMATION, nil, nil, seededDACL, nil); err != nil { + _ = windows.CloseHandle(handle) + t.Fatalf("SetSecurityInfo: %v", err) + } + _ = windows.CloseHandle(handle) + + // 2. Apply WindowsACLPlan with WindowsACLDenyWrite. + plan := WindowsACLPlan{ + Entries: []WindowsACLEntry{{ + Action: WindowsACLDenyWrite, + Path: dir, + Capability: sidStr, + }}, + } + rollback, err := applyWindowsACLPlan(plan) + if err != nil { + t.Fatalf("applyWindowsACLPlan: %v", err) + } + t.Cleanup(func() { _ = rollback() }) + + // 3. Inspect DACL on dir. + sd, err := windows.GetNamedSecurityInfo(dir, windows.SE_FILE_OBJECT, windows.DACL_SECURITY_INFORMATION) + if err != nil { + t.Fatalf("GetNamedSecurityInfo: %v", err) + } + dacl, _, err := sd.DACL() + if err != nil { + t.Fatalf("DACL: %v", err) + } + + foundCombinedMigrated := false + for i := uint16(0); i < dacl.AceCount; i++ { + var ace *windows.ACCESS_ALLOWED_ACE + if err := windows.GetAce(dacl, uint32(i), &ace); err != nil { + t.Fatalf("GetAce: %v", err) + } + if ace.Header.AceType != windows.ACCESS_DENIED_ACE_TYPE && ace.Header.AceType != windowsAccessDeniedObjectAceType { + continue + } + aceSID, ok := windowsAceSID(ace) + if !ok || !aceSID.Equals(sid) { + continue + } + // The migrated ACE must retain read denial bits and have narrowed write denial bits. + if ace.Mask&readMask == readMask && ace.Mask&narrowWriteMask == narrowWriteMask { + foundCombinedMigrated = true + } + } + + if !foundCombinedMigrated { + t.Fatal("resulting DACL lost DenyRead bits or failed to narrow DenyWrite in combined ACE") + } + + // 4. Verify dirDeniesReadSID recognizes the DenyRead on the migrated combined ACE. + if !dirDeniesReadSID(t, dir, sidStr) { + t.Fatal("dirDeniesReadSID failed to recognize DenyRead on migrated combined ACE") + } + + // 5. Verify idempotency on second apply. + if _, err := applyWindowsACLPlan(plan); err != nil { + t.Fatalf("second applyWindowsACLPlan failed: %v", err) + } + sd2, err := windows.GetNamedSecurityInfo(dir, windows.SE_FILE_OBJECT, windows.DACL_SECURITY_INFORMATION) + if err != nil { + t.Fatalf("GetNamedSecurityInfo 2: %v", err) + } + dacl2, _, err := sd2.DACL() + if err != nil { + t.Fatalf("DACL 2: %v", err) + } + if dacl2.AceCount != dacl.AceCount { + t.Fatalf("second apply changed ACE count: %d vs %d", dacl2.AceCount, dacl.AceCount) + } +}