diff --git a/internal/sandbox/manager_test.go b/internal/sandbox/manager_test.go index 566bdf7c7..e5deefdf5 100644 --- a/internal/sandbox/manager_test.go +++ b/internal/sandbox/manager_test.go @@ -230,6 +230,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 47947316c..c702f5b45 100644 --- a/internal/sandbox/profile.go +++ b/internal/sandbox/profile.go @@ -150,10 +150,11 @@ func permissionProfileReadRoots(workspaceRoot string, policy Policy, scope *Scop // carry credentials and are now discoverable through the preserved caller // environment. Two 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. // diff --git a/internal/sandbox/runner_windows_integration_test.go b/internal/sandbox/runner_windows_integration_test.go index d0f715744..617f7cd96 100644 --- a/internal/sandbox/runner_windows_integration_test.go +++ b/internal/sandbox/runner_windows_integration_test.go @@ -72,6 +72,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) @@ -189,10 +211,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{ @@ -203,6 +233,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 @@ -386,6 +434,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) + } + } +} + func powershellSingleQuote(value string) string { out := "'" for _, r := range value { diff --git a/internal/sandbox/windows_acl.go b/internal/sandbox/windows_acl.go index 55d37d347..c94194d2b 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,9 +129,62 @@ func BuildWindowsACLPlan(config WindowsSandboxCommandConfig) (WindowsACLPlan, er }) } } + + // Shared-path DenyWrite mitigations (C:\, ProgramData, Windows\Temp, + // Users\Public) existed only to compensate for Users/Authenticated Users + // SID broadening on fully restricted DenyRead tokens. That broadening is + // permanently disabled (see runWindowsSandboxCommand): preflight DACL + // snapshots cannot enforce a write boundary for the command's lifetime. + // Do not stamp new machine-wide DenyWrite ACEs for a feature that never + // enables. Still revoke the stable read-only capability SID on configured + // write roots when DenyRead is set on the elevated tier, so hosts that + // ran earlier PR builds (which did apply shared/descendant denies) do not + // keep a stale deny that wins over the write root's Allow. + if config.SandboxLevel == WindowsSandboxLevelRestrictedToken && len(config.PermissionProfile.FileSystem.DenyRead) > 0 && len(writeCapabilities) > 0 { + caps, err := LoadOrCreateWindowsCapabilitySIDs(config.SandboxHome) + if err != nil { + return WindowsACLPlan{}, err + } + denySID := caps.ReadOnly + for _, capability := range writeCapabilities { + entries = append(entries, WindowsACLEntry{ + Action: WindowsACLRevokeCapability, + Path: capability.Root, + Capability: denySID, + NoInherit: true, + RevokeDescendants: true, + }) + } + } + return WindowsACLPlan{Entries: dedupeWindowsACLEntries(entries)}, nil } +// windowsPathUnderAnyRoot reports whether path is exactly, or nested under, one +// of the configured write-root capabilities. A shared-path deny must skip both +// cases: denying a root that IS a write root would block the workspace outright, +// and denying a root that merely CONTAINS one (e.g. a shared path of +// C:\Users\Public when C:\Users itself is a configured write root) would place +// an explicit Deny ahead of that root's Allow for every broadened token, +// winning under Windows' deny-before-allow evaluation and jailing a directory +// the user explicitly configured as writable. +func windowsPathUnderAnyRoot(path string, capabilities []windowsWriteRootCapability) bool { + key := windowsCapabilityPathKey(path) + if key == "" { + return false + } + for _, cap := range capabilities { + rootKey := windowsCapabilityPathKey(cap.Root) + if rootKey == "" { + continue + } + if key == rootKey || strings.HasPrefix(key, rootKey+`\`) { + return true + } + } + return false +} + type windowsWriteRootCapability struct { Root string SID string @@ -184,7 +290,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..092b5880d 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,149 @@ 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++ + if denied, err := windowsPathDeniesCapabilitySID(child, denySID); err == nil && denied { + 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 +262,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 +330,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 +384,75 @@ 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 + } + if ace.Header.AceFlags&windows.INHERIT_ONLY_ACE != 0 { + 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). + inheritance := uint32(0) + if isDir && ace.Header.AceFlags&(windows.OBJECT_INHERIT_ACE|windows.CONTAINER_INHERIT_ACE) != 0 { + inheritance = windows.SUB_CONTAINERS_AND_OBJECTS_INHERIT + } + 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 +460,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..6a7743ef1 100644 --- a/internal/sandbox/windows_acl_apply_windows_test.go +++ b/internal/sandbox/windows_acl_apply_windows_test.go @@ -9,6 +9,7 @@ import ( "path/filepath" "strings" "testing" + "unsafe" "golang.org/x/sys/windows" ) @@ -48,6 +49,181 @@ 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") + } +} + +// 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 { + continue + } + sid := (*windows.SID)(unsafe.Pointer(&ace.SidStart)) + if !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..c68b12728 --- /dev/null +++ b/internal/sandbox/windows_acl_descendants_windows.go @@ -0,0 +1,622 @@ +//go:build windows + +package sandbox + +import ( + "fmt" + "os" + "path/filepath" + "strings" + "unsafe" + + "golang.org/x/sys/windows" +) + +// The shared-root compensating deny (windows_acl.go) puts a direct, +// non-inheriting DenyWrite on each of C:\, %ProgramData%, %SystemRoot%\Temp, +// and C:\Users\Public. That blocks new writes directly under those objects, but +// a Windows access check for an EXISTING child never evaluates a non-inherited +// ACE on the child's parent, so a pre-existing descendant that independently +// grants BUILTIN\Users or Authenticated Users write stays writable once the +// elevated fully-restricted (DenyRead) token is broadened with those two groups, +// a write outside every configured write root. This file enumerates those +// existing writable descendants and denies each one directly. +// +// Coverage rules (fail closed): +// +// - Every directory under a shared root is considered for descent within the +// depth/entry caps, whether or not the parent itself is Users-writable. A +// depth-N writable child under non-writable ancestors is a real escape and +// must be found (see CodeRabbit/jatmn review). +// - Hitting windowsDescendantScanMaxDepth or windowsDescendantScanMaxDirs +// means unexamined territory remains: the scan returns an error so setup +// and pre-broaden revalidation cannot certify a partial walk as clean. +// These bounds must be large enough that a stock C:\ (with its Windows, +// Program Files, and WinSxS trees) actually completes — see the comment on +// the vars below for the reasoning and the honest limits of that estimate. +// - Reparse points (junctions, symlinks, volume mount points) use a +// no-follow, identity-aware policy (see jatmn's review). Path APIs without +// FILE_FLAG_OPEN_REPARSE_POINT transparently resolve through a directory +// junction or symlink, so treating a reparse as an ordinary directory +// would either hard-fail on deliberately non-listable compatibility +// junctions (C:\Documents and Settings, ProgramData\Application Data, …) +// or re-walk an already-scanned tree through the target and blow the +// depth/entry cap. The walker therefore: +// 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 +// (short name, case variant, or any residual reparse resolution) is +// skipped rather than re-enumerated. +// Stock compatibility junctions always have a real path sibling that the +// walk reaches separately (Documents and Settings -> Users); skipping the +// reparse does not leave that tree unexamined. +// - 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 at the +// one place that basename is ever legitimately the real thing: directly +// under an actual drive letter root (windowsPathIsDriveRootPath). Without +// that allowlist, elevated setup would fail on every real machine; without +// the root-level scoping, a same-named directory anywhere else in the +// tree (nested under ProgramData or Public, whether by installer accident +// or deliberately) would be silently skipped instead of failing closed — +// see jatmn's review. +// - Stock system trees under the drive root (Windows, Program Files, ...) +// are NOT pruned by basename: a directory's own DACL being non-writable +// says nothing about whether an installer-created descendant several +// levels down independently grants Users/AuthUsers write, so certifying +// a subtree clean from its root DACL alone would miss exactly that +// escape (see jatmn's review). Every directory is descended subject only +// to the depth/entry caps above; exhausting those caps on a genuinely +// huge stock tree is a fail-closed error, not a silent partial pass. +// +// Staleness: setup alone is not enough. Non-inheriting denies only cover the +// filesystem state at apply time. The elevated command runner revalidates and +// reapplies this scan immediately before broadening the restricted token +// (windowsEnsureSharedDescendantCoverage); if coverage cannot be re-established, +// the token stays on the narrow SID set. +// +// 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 shared root. A stock Windows install can + // easily have tens to hundreds of thousands of objects under C:\Windows + // alone (WinSxS in particular), so the previous 8,192 cap made every + // elevated DenyRead setup fail on a normal system drive (jatmn's review). + // This is raised to a size intended to comfortably cover a typical stock + // C:\Windows + Program Files + Program Files (x86) tree while still + // bounding worst-case work to a finite number rather than removing the + // cap outright. It is a reasoned estimate, not a measurement: this fix was + // written and cross-compiled without access to a real Windows machine, so + // the actual object count on any given box (and the wall-clock cost of + // walking it, since windowsEnsureSharedDescendantCoverage repeats this + // scan before every DenyRead command) could not be verified directly. + // Failing closed here only costs functionality (the narrow SID set), never + // safety, so an unusually large tree is a safe, if inconvenient, failure + // mode rather than a security regression. + 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 +} + +// windowsEnsureSharedDescendantCoverage re-enumerates the shared deny roots and +// ensures every currently Users/AuthUsers-writable descendant carries a direct +// DenyWrite for the stable read-only capability SID. It is called immediately +// before a command broadens the restricted token so a child created after +// `zero sandbox setup` cannot remain an uncovered write-jail escape. +// +// It also revalidates the direct deny on each shared ROOT itself (C:\, +// ProgramData, Windows\Temp, Public), not just its descendants: this was +// previously left unchecked, so an installer or service that removed the root +// deny after setup would leave the descendant walk reporting no holes while +// the root itself had silently reopened, and the runner would still broaden +// the token (see jatmn's review). +// +// Prefer reapplying denies (same permanent posture as elevated setup) for both +// the root and its descendants. When reapply fails — typically because the +// command process lacks WRITE_DAC on a system path — fall back to a +// read-only check: if the root's own deny is missing, or any writable +// descendant still lacks the synthetic deny, coverage is incomplete and the +// caller must not broaden. Fail closed on enumeration errors either way. +func windowsEnsureSharedDescendantCoverage(config WindowsSandboxCommandConfig) error { + plan, err := BuildWindowsACLPlan(config) + if err != nil { + return err + } + writeRoots := windowsPlanAllowWriteRoots(plan) + for _, group := range groupWindowsACLPlanByPath(plan) { + denySID, ok := windowsGroupScanDescendantsSID(group) + if !ok { + continue + } + if _, err := os.Stat(group.Path); err != nil { + if os.IsNotExist(err) { + continue + } + return fmt.Errorf("stat shared deny root %s: %w", group.Path, err) + } + rootDenied, checkErr := windowsPathDeniesCapabilitySID(group.Path, denySID) + if checkErr != nil { + return checkErr + } + if !rootDenied { + if _, _, err := applyWindowsACLPathGroup(group); err != nil { + denied, denyErr := windowsPathDeniesCapabilitySID(group.Path, denySID) + if denyErr != nil || !denied { + return fmt.Errorf("shared root write deny missing on %s: %w", group.Path, err) + } + // Reapply failed but the root's own deny is still effectively in + // place, so the write jail continues to hold for this root. + } + } + if _, err := applyWindowsSharedDescendantDenies(group.Path, denySID, writeRoots); err == nil { + continue + } else if holes, holeErr := windowsUncoveredWritableDescendants(group.Path, denySID, writeRoots); holeErr != nil { + return holeErr + } else if len(holes) > 0 { + return fmt.Errorf("shared descendant write coverage incomplete under %s (e.g. %s): %w", group.Path, holes[0], err) + } + // Reapply failed but every currently writable descendant already carries + // the synthetic deny, so the write jail still holds for this root. + } + return nil +} + +// 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..9271d4bc2 --- /dev/null +++ b/internal/sandbox/windows_acl_descendants_windows_test.go @@ -0,0 +1,729 @@ +//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 { + continue + } + sid := (*windows.SID)(unsafe.Pointer(&ace.SidStart)) + if sid.String() == wantSID { + 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) + } + var targetOldDACL *windows.ACL = 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) + } +} + +func TestWindowsEnsureSharedDescendantCoverageDeduplicatesRootDeny(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 || !denied { + t.Fatalf("expected root to be denied before re-check, denied=%v, err=%v", denied, err) + } + // Second check must still report complete coverage so command-time + // windowsEnsureSharedDescendantCoverage skips SetEntriesInAclW rather + // than stacking another permanent DENY_ACCESS ACE on the root. + deniedAgain, err := windowsPathDeniesCapabilitySID(dir, sid) + if err != nil || !deniedAgain { + t.Fatalf("expected idempotent complete deny on re-check, denied=%v, err=%v", deniedAgain, err) + } +} 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..def8f1e33 --- /dev/null +++ b/internal/sandbox/windows_acl_paths_windows.go @@ -0,0 +1,43 @@ +//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) { + // GetWindowsDirectory is the long-standing x/sys export; GetSystemWindowsDirectory + // is not available on every pinned golang.org/x/sys revision CI may use. + windowsDir, err := windows.GetWindowsDirectory() + 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..bf7d20af4 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,88 @@ 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. Write roots still get a revoke of the stable read-only + // SID so earlier PR builds' stale denies cannot shadow new Allows. + caps, err := LoadOrCreateWindowsCapabilitySIDs(home) + if err != nil { + t.Fatalf("LoadOrCreateWindowsCapabilitySIDs: %v", err) + } + assertNoSharedSystemDenyWrites(t, plan) + for _, root := range []string{`C:\workspace`, `D:\cache`} { + assertWindowsACLRevoke(t, plan, root, caps.ReadOnly, true) + } +} + +// TestBuildWindowsACLPlanOmitsSharedDenyPathsWithoutDenyRead pins that +// profiles without DenyRead never touch shared system-path DACLs or the +// stable read-only SID revoke path. +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) + for _, entry := range plan.Entries { + if entry.Action == WindowsACLRevokeCapability { + t.Fatalf("plan without DenyRead = %#v, want no WindowsACLRevokeCapability entry", plan.Entries) + } + } +} + +// 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 +144,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 +157,131 @@ 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) + } + } +} + +// TestBuildWindowsACLPlanRevokesStaleSharedDenyOnWriteRoot pins cleanup for +// hosts that ran earlier PR builds which stamped shared/descendant DenyWrite +// ACEs: write roots still get an unconditional revoke of the stable read-only +// SID (with RevokeDescendants) when DenyRead is configured on the elevated tier. +func TestBuildWindowsACLPlanRevokesStaleSharedDenyOnWriteRoot(t *testing.T) { + home := t.TempDir() + caps, err := LoadOrCreateWindowsCapabilitySIDs(home) + if err != nil { + t.Fatalf("LoadOrCreateWindowsCapabilitySIDs: %v", err) + } + systemDrive, _, _, publicDir := windowsSharedDenyPathsForTest(t) + promotedDescendant := systemDrive + `\Users\shared` + + plan, err := BuildWindowsACLPlan(WindowsSandboxCommandConfig{ + SandboxHome: home, + WorkspaceRoots: []string{publicDir, promotedDescendant}, + SandboxLevel: WindowsSandboxLevelRestrictedToken, + PermissionProfile: PermissionProfile{ + FileSystem: FileSystemPolicy{ + Kind: FileSystemRestricted, + WriteRoots: []WritableRoot{{Root: publicDir}, {Root: promotedDescendant}}, + DenyRead: []string{`C:\workspace\secret-read`}, + }, + Network: NetworkPolicy{Mode: NetworkDeny}, + }, + }) + if err != nil { + t.Fatalf("BuildWindowsACLPlan: %v", err) + } + assertNoSharedSystemDenyWrites(t, plan) + for _, root := range []string{publicDir, promotedDescendant} { + assertWindowsACLRevoke(t, plan, root, caps.ReadOnly, true) + } +} + +// TestBuildWindowsACLPlanOmitsRevokeCapabilityWithoutDenyRead pins that the +// stale-deny reconciliation entry is scoped to DenyRead elevated profiles. +func TestBuildWindowsACLPlanOmitsRevokeCapabilityWithoutDenyRead(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) + } + for _, entry := range plan.Entries { + if entry.Action == WindowsACLRevokeCapability { + t.Fatalf("plan without DenyRead = %#v, want no WindowsACLRevokeCapability entry", plan.Entries) + } + } +} + +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 assertWindowsACLRevoke(t *testing.T, plan WindowsACLPlan, path, capability string, revokeDescendants bool) { + t.Helper() + for _, entry := range plan.Entries { + if entry.Action == WindowsACLRevokeCapability && + windowsCapabilityPathKey(entry.Path) == windowsCapabilityPathKey(path) && + strings.EqualFold(entry.Capability, capability) { + if entry.RevokeDescendants != revokeDescendants { + t.Fatalf("revoke on %q RevokeDescendants=%v, want %v", path, entry.RevokeDescendants, revokeDescendants) + } + return + } + } + t.Fatalf("plan = %#v, want WindowsACLRevokeCapability on %q for %q", plan.Entries, path, capability) } func TestBuildWindowsACLPlanRejectsUnrestrictedProfiles(t *testing.T) { @@ -117,16 +322,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 +349,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..911e7c2da 100644 --- a/internal/sandbox/windows_command_runner.go +++ b/internal/sandbox/windows_command_runner.go @@ -3,6 +3,7 @@ package sandbox import ( "fmt" "io" + "strings" ) func RunWindowsSandboxCommandRunner(args []string, stderr io.Writer) int { @@ -17,3 +18,39 @@ func RunWindowsSandboxCommandRunner(args []string, stderr io.Writer) int { } 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. "+ + "Use `--sandbox forbid`, omit DenyRead, or wait for access-time confinement "+ + "(AppContainer/LPAC-style). Configured DenyRead paths: %s", + strings.Join(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..1873e5692 --- /dev/null +++ b/internal/sandbox/windows_command_runner_test.go @@ -0,0 +1,52 @@ +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", `C:\secret`} { + if !strings.Contains(msg, want) { + t.Fatalf("%s error %q missing %q", level, msg, want) + } + } + // Must not point users at the other restricted-token tier as a workaround. + if strings.Contains(msg, "Use `--sandbox forbid`, the unelevated") { + t.Fatalf("%s error still recommends unelevated as a DenyRead workaround: %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..0b1c67876 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,13 +78,17 @@ 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) + // Users/Authenticated Users SID broadening is permanently disabled: those + // groups unlock write grants outside WriteRoots, and preflight DenyWrite + // compensation cannot enforce an access-time write boundary. See PR #640. + const broadenReadSIDs = false + token, err := createWindowsRestrictedTokenForCapabilitySIDs(tokenSIDs, writeRestricted, broadenReadSIDs) if err != nil { fmt.Fprintln(stderr, WindowsSandboxCommandRunnerName+": "+err.Error()) return 1 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()) diff --git a/internal/sandbox/windows_token_windows.go b/internal/sandbox/windows_token_windows.go index a02e9b001..0c8e77559 100644 --- a/internal/sandbox/windows_token_windows.go +++ b/internal/sandbox/windows_token_windows.go @@ -48,7 +48,7 @@ func (sid windowsLocalSID) close() { } } -func createWindowsRestrictedTokenForCapabilitySIDs(capabilitySIDStrings []string, writeRestricted bool) (windows.Token, error) { +func createWindowsRestrictedTokenForCapabilitySIDs(capabilitySIDStrings []string, writeRestricted, broadenReadSIDs bool) (windows.Token, error) { if len(capabilitySIDStrings) == 0 { return 0, errors.New("windows restricted token requires at least one capability SID") } @@ -80,10 +80,32 @@ func createWindowsRestrictedTokenForCapabilitySIDs(capabilitySIDStrings []string return 0, fmt.Errorf("open process token: %w", err) } defer base.Close() - return createWindowsRestrictedTokenFromBase(base, capabilitySIDs, writeRestricted) + return createWindowsRestrictedTokenFromBase(base, capabilitySIDs, writeRestricted, broadenReadSIDs) } -func createWindowsRestrictedTokenFromBase(base windows.Token, capabilitySIDs []windowsLocalSID, writeRestricted bool) (windows.Token, error) { +// createWindowsRestrictedTokenFromBase builds the restricted token. When +// broadenReadSIDs is set, it also restricts to WinBuiltinUsersSid and +// WinAuthenticatedUserSid so the sandboxed process can read/execute binaries +// under paths like C:\Program Files or C:\Windows whose ACLs grant +// Users/Authenticated Users rather than Everyone. +// +// Callers must pass broadenReadSIDs=false. Preflight DenyWrite compensation +// cannot enforce a write boundary for the command's lifetime (reparse +// coverage gaps and post-scan group-writable children), so the runner keeps +// broadening disabled until access-time confinement (AppContainer/LPAC or +// equivalent) exists. The parameter remains only so the token builder can +// refuse the unsafe broadenReadSIDs+writeRestricted combination and so a +// future access-time design can re-enable a controlled form without another +// signature change. +func createWindowsRestrictedTokenFromBase(base windows.Token, capabilitySIDs []windowsLocalSID, writeRestricted, broadenReadSIDs bool) (windows.Token, error) { + // Defensive guardrail for the invariant documented above: combining the + // two would silently widen the write jail (broadenReadSIDs's write grants) + // with no compensating DenyWrite mitigation (writeRestricted's caller + // never applies one), so refuse to build the token rather than let a + // future caller-side refactor combine them by accident. + if broadenReadSIDs && writeRestricted { + return 0, errors.New("broadenReadSIDs cannot be combined with writeRestricted") + } logonSID, err := copyWindowsLogonSID(base) if err != nil { return 0, err @@ -93,7 +115,7 @@ func createWindowsRestrictedTokenFromBase(base windows.Token, capabilitySIDs []w return 0, fmt.Errorf("create world SID: %w", err) } - entries := make([]windows.SIDAndAttributes, 0, len(capabilitySIDs)+2) + entries := make([]windows.SIDAndAttributes, 0, len(capabilitySIDs)+4) for _, sid := range capabilitySIDs { entries = append(entries, windows.SIDAndAttributes{Sid: sid.sid}) } @@ -101,6 +123,20 @@ func createWindowsRestrictedTokenFromBase(base windows.Token, capabilitySIDs []w windows.SIDAndAttributes{Sid: sidFromBytes(logonSID)}, windows.SIDAndAttributes{Sid: worldSID}, ) + if broadenReadSIDs { + usersSID, err := windows.CreateWellKnownSid(windows.WinBuiltinUsersSid) + if err != nil { + return 0, fmt.Errorf("create users SID: %w", err) + } + authUserSID, err := windows.CreateWellKnownSid(windows.WinAuthenticatedUserSid) + if err != nil { + return 0, fmt.Errorf("create authenticated user SID: %w", err) + } + entries = append(entries, + windows.SIDAndAttributes{Sid: usersSID}, + windows.SIDAndAttributes{Sid: authUserSID}, + ) + } // WRITE_RESTRICTED scopes the restricted-SID check to write-type accesses: // reads use only the normal token identity, so the sandboxed process can