diff --git a/internal/sandbox/manager_test.go b/internal/sandbox/manager_test.go index b930420f4..6e2941602 100644 --- a/internal/sandbox/manager_test.go +++ b/internal/sandbox/manager_test.go @@ -204,12 +204,19 @@ func TestSandboxManagerBuildsCommandPlanThroughWindowsRunner(t *testing.T) { windowsSandboxInitialized = func() bool { return true } backend := Backend{Name: BackendWindowsRestrictedToken, Available: true, Executable: `C:\zero\zero-windows-command-runner.exe`, Platform: "windows"} policy := DefaultPolicy() + // Build the usual restricted FS profile, then clear DenyRead. On non-Windows + // hosts PermissionProfileFromPolicy injects credential-store DenyRead paths, + // and the Windows plan path rejects any non-empty DenyRead (PR #640). This + // happy-path plan must exercise a valid restricted profile without DenyRead; + // rejection coverage lives in TestSandboxManagerRejectsWindowsDenyReadOnBothRestrictedTokenTiers. + profile := PermissionProfileFromPolicy(`C:\workspace`, policy, nil) + profile.FileSystem.DenyRead = nil manager := NewSandboxManager(SandboxManagerOptions{GOOS: "windows", Backend: backend}) plan, err := manager.BuildCommandPlan(SandboxManagerRequest{ WorkspaceRoot: `C:\workspace`, Command: CommandSpec{Name: "cmd.exe", Args: []string{"/d", "/s", "/c", "dir"}, Dir: `C:\workspace\src`, Env: []string{"PATH=C:\\Tools", "TERM=xterm"}}, Policy: policy, - Profile: PermissionProfileFromPolicy(`C:\workspace`, policy, nil), + Profile: profile, Preference: SandboxPreferenceAuto, ValidateExecution: true, }) @@ -243,6 +250,87 @@ func TestSandboxManagerBuildsCommandPlanThroughWindowsRunner(t *testing.T) { } } +// TestSandboxManagerRejectsWindowsDenyReadOnBothRestrictedTokenTiers is the +// regression for PR #640: DenyRead cannot be launched or provisioned through +// either the elevated restricted-token path or the unelevated auto fallback. +// Both build the same fully restricted narrow-SID token. +func TestSandboxManagerRejectsWindowsDenyReadOnBothRestrictedTokenTiers(t *testing.T) { + backend := Backend{Name: BackendWindowsRestrictedToken, Available: true, Executable: `C:\zero\zero-windows-command-runner.exe`, Platform: "windows"} + manager := NewSandboxManager(SandboxManagerOptions{GOOS: "windows", Backend: backend}) + policy := DefaultPolicy() + profile := PermissionProfile{ + FileSystem: FileSystemPolicy{ + Kind: FileSystemRestricted, + WriteRoots: []WritableRoot{{Root: `C:\workspace`}}, + DenyRead: []string{`C:\workspace\secret`}, + }, + Network: NetworkPolicy{Mode: NetworkDeny}, + } + cmd := CommandSpec{Name: "cmd.exe", Args: []string{"/c", "dir"}, Dir: `C:\workspace`} + + t.Run("elevated_restricted_token", func(t *testing.T) { + restore := windowsSandboxInitialized + t.Cleanup(func() { windowsSandboxInitialized = restore }) + windowsSandboxInitialized = func() bool { return true } + + _, err := manager.BuildCommandPlan(SandboxManagerRequest{ + WorkspaceRoot: `C:\workspace`, + Command: cmd, + Policy: policy, + Profile: profile, + Preference: SandboxPreferenceAuto, + ValidateExecution: true, + }) + if err == nil { + t.Fatal("expected BuildCommandPlan error for elevated restricted-token DenyRead") + } + msg := err.Error() + for _, want := range []string{"DenyRead", "not supported", "restricted-token"} { + if !strings.Contains(msg, want) { + t.Fatalf("error %q missing %q", msg, want) + } + } + }) + + t.Run("unelevated_auto_fallback", func(t *testing.T) { + restore := windowsSandboxInitialized + t.Cleanup(func() { windowsSandboxInitialized = restore }) + windowsSandboxInitialized = func() bool { return false } + + req, err := manager.BuildExecutionRequest(SandboxManagerRequest{ + WorkspaceRoot: `C:\workspace`, + Command: cmd, + Policy: policy, + Profile: profile, + Preference: SandboxPreferenceAuto, + ValidateExecution: true, + }) + if err != nil { + t.Fatalf("BuildExecutionRequest: %v", err) + } + if req.EnforcementLevel != EnforcementUnelevated { + t.Fatalf("EnforcementLevel = %v, want unelevated auto fallback before DenyRead rejection", req.EnforcementLevel) + } + _, err = manager.BuildCommandPlan(SandboxManagerRequest{ + WorkspaceRoot: `C:\workspace`, + Command: cmd, + Policy: policy, + Profile: profile, + Preference: SandboxPreferenceAuto, + ValidateExecution: true, + }) + if err == nil { + t.Fatal("expected BuildCommandPlan error for unelevated DenyRead") + } + if !strings.Contains(err.Error(), "DenyRead") || !strings.Contains(err.Error(), "not supported") { + t.Fatalf("unelevated DenyRead error = %v", err) + } + if strings.Contains(err.Error(), "Use `--sandbox forbid`, the unelevated") { + t.Fatalf("error still recommends unelevated as a workaround: %v", err) + } + }) +} + func TestSandboxManagerDegradesUnavailableCommandPlan(t *testing.T) { policy := DefaultPolicy() backend := Backend{Name: BackendUnavailable, Platform: "windows", Fallback: true, Message: "native sandbox unavailable"} diff --git a/internal/sandbox/profile.go b/internal/sandbox/profile.go index 349e2b1c6..ab44721b4 100644 --- a/internal/sandbox/profile.go +++ b/internal/sandbox/profile.go @@ -224,10 +224,11 @@ type credentialDenyPaths struct { // the preserved caller environment and Zero's own config/token stores. Four // deliberate limits: // -// - Windows is skipped: a non-empty profile DenyRead switches the Windows -// runner onto the capability-SID/ACL deny path and away from the -// WRITE_RESTRICTED token, which the unelevated tier depends on. Revisit -// once the Windows deny-read model is settled. +// - Windows is skipped: a non-empty profile DenyRead is unsupported on both +// restricted-token runner levels under the narrow SID set (PR #640). The +// fully restricted token cannot load ordinary system binaries without +// Users/AuthUsers, and adding those groups reopens write grants outside +// WriteRoots. Revisit once access-time confinement exists. // - A candidate nested under a user-configured AllowRead entry is dropped, // so `allowRead: ["~/.aws"]` remains an explicit opt-out. // - Candidates are emitted whether or not they currently exist on disk. diff --git a/internal/sandbox/runner_windows_integration_test.go b/internal/sandbox/runner_windows_integration_test.go index fb98f1287..b34da8edf 100644 --- a/internal/sandbox/runner_windows_integration_test.go +++ b/internal/sandbox/runner_windows_integration_test.go @@ -73,6 +73,24 @@ 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 { + publicProbe := allocateSharedDirectoryProbe(t, publicDir, "elevated-public") + runWindowsRealSmokeCommand(t, runnerExe, config, deniedWriteCommand(publicProbe.Path()), deniedWriteExitCode) + if _, err := os.Stat(publicProbe.Path()); err == nil { + t.Fatalf("Windows sandbox allowed a write to the shared C:\\Users\\Public directory") + } else if !os.IsNotExist(err) { + t.Fatalf("stat public marker: %v", err) + } + } + listener, err := net.Listen("tcp4", "127.0.0.1:0") if err != nil { t.Fatalf("listen loopback for Windows network smoke: %v", err) @@ -156,12 +174,14 @@ func TestWindowsUnelevatedRealSandboxSmoke(t *testing.T) { } sandboxHome := filepath.Join(root, ".zero-sandbox") + // Success path: restricted FS write-jail with no DenyRead. Non-empty DenyRead + // is unsupported on both restricted-token tiers under the narrow SID set + // (PR #640); the rejection probe below covers that separately. profile := PermissionProfile{ FileSystem: FileSystemPolicy{ Kind: FileSystemRestricted, ReadRoots: []string{root}, WriteRoots: []WritableRoot{{Root: root, ProtectedMetadataNames: []string{".git", ".zero", ".agents"}}}, - DenyRead: []string{privateDir}, IncludePlatformRoots: true, AllowTemp: true, }, @@ -190,10 +210,18 @@ func TestWindowsUnelevatedRealSandboxSmoke(t *testing.T) { t.Fatalf("expected the unelevated setup marker to be recorded: %v", err) } - // DenyRead check: reading from the privateDir must be blocked (exit code 1) - runWindowsRealSmokeCommand(t, runnerExe, config, []string{ + // DenyRead is unsupported on both restricted-token tiers under the narrow + // SID set (PR #640): the runner must reject before launch rather than + // attempting a fully restricted token that cannot load system tools. + denyReadConfig := config + denyReadConfig.PermissionProfile.FileSystem.DenyRead = []string{privateDir} + runWindowsRealSmokeCommandExpectError(t, runnerExe, denyReadConfig, []string{ "cmd.exe", "/d", "/s", "/c", "type " + secretFile, - }, 1) + }, "DenyRead", "not supported") + // The secret must remain readable from the host; the sandbox never ran. + if data, err := os.ReadFile(secretFile); err != nil || string(data) != "super-secret" { + t.Fatalf("host secret file after rejected DenyRead launch: %q, %v", data, err) + } outsideMarker := filepath.Join(outside, "unelevated-write-denied.txt") runWindowsRealSmokeCommand(t, runnerExe, config, []string{ @@ -204,6 +232,18 @@ 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 != "" { + programDataProbe := allocateSharedDirectoryProbe(t, programData, "unelevated-programdata") + runWindowsRealSmokeCommand(t, runnerExe, config, deniedWriteCommand(programDataProbe.Path()), deniedWriteExitCode) + if _, err := os.Stat(programDataProbe.Path()); err == nil { + t.Fatalf("unelevated sandbox allowed a write to ProgramData shared directory") + } else if !os.IsNotExist(err) { + t.Fatalf("stat ProgramData marker: %v", err) + } + } } // TestWindowsRestrictedTokenNestedPipeCapture pins the fix in @@ -387,6 +427,34 @@ 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 ctx.Err() != nil { + t.Fatalf("Windows sandbox command timed out: %v\n%s", ctx.Err(), output) + } + if err == nil { + t.Fatalf("Windows sandbox command exit code = 0, want error containing %v\n%s", wantSubstr, output) + } + text := string(output) + for _, want := range wantSubstr { + if !strings.Contains(text, want) { + t.Fatalf("Windows sandbox command error missing %q: %v\n%s", want, err, output) + } + } +} + // The write jail must hold on a path whose DACL grants Everyone write access. // // A WRITE_RESTRICTED token runs TWO checks for a write and needs both to pass: @@ -505,7 +573,49 @@ const deniedWriteExitCode = 77 // deniedWriteCommand attempts a write and reports deniedWriteExitCode when the // redirect is refused, so the exit code also proves cmd.exe actually ran. func deniedWriteCommand(marker string) []string { - return []string{"cmd.exe", "/d", "/s", "/c", "echo leaked>" + marker + " || exit " + strconv.Itoa(deniedWriteExitCode)} + return []string{"cmd.exe", "/d", "/s", "/c", "echo leaked>" + cmdQuote(marker) + " || exit " + strconv.Itoa(deniedWriteExitCode)} +} + +func cmdQuote(path string) string { + return `"` + strings.ReplaceAll(path, `"`, `\"`) + `"` +} + +type sharedDirectoryProbe struct { + path string +} + +func allocateSharedDirectoryProbe(t testing.TB, dir, prefix string) *sharedDirectoryProbe { + t.Helper() + if dir == "" { + t.Skip("shared directory path is not set") + } + var probePath string + for attempt := 0; attempt < 50; attempt++ { + candidate := filepath.Join(dir, fmt.Sprintf("zero-smoke-%s-%d-%d-%d.txt", prefix, os.Getpid(), time.Now().UnixNano(), attempt)) + if _, err := os.Lstat(candidate); os.IsNotExist(err) { + probePath = candidate + break + } + } + if probePath == "" { + t.Fatalf("allocate shared directory probe in %s: failed to find unused filename", dir) + } + p := &sharedDirectoryProbe{path: probePath} + t.Cleanup(func() { + p.cleanup(t) + }) + return p +} + +func (p *sharedDirectoryProbe) Path() string { + return p.path +} + +func (p *sharedDirectoryProbe) cleanup(t testing.TB) { + t.Helper() + if _, err := os.Lstat(p.path); err == nil { + _ = os.Remove(p.path) + } } func powershellSingleQuote(value string) string { @@ -519,3 +629,39 @@ func powershellSingleQuote(value string) string { } return out + "'" } + +func TestSharedDirectoryProbeLifecycle(t *testing.T) { + dir := t.TempDir() + + // 1. Two simultaneous allocations produce distinct non-colliding paths. + probe1 := allocateSharedDirectoryProbe(t, dir, "p1") + probe2 := allocateSharedDirectoryProbe(t, dir, "p2") + if probe1.Path() == probe2.Path() { + t.Fatalf("expected distinct probe paths, got %q and %q", probe1.Path(), probe2.Path()) + } + + // 2. Pre-existing unrelated file is never selected or deleted. + unrelatedFile := filepath.Join(dir, "unrelated.txt") + if err := os.WriteFile(unrelatedFile, []byte("preserve me"), 0o600); err != nil { + t.Fatalf("write unrelated file: %v", err) + } + probe3 := allocateSharedDirectoryProbe(t, dir, "p3") + probe3.cleanup(t) + if data, err := os.ReadFile(unrelatedFile); err != nil || string(data) != "preserve me" { + t.Fatalf("unrelated file was modified or deleted: data=%q, err=%v", data, err) + } + + // 3. Interrupted / no-create path: cleanup on absent file succeeds quietly. + probe4 := allocateSharedDirectoryProbe(t, dir, "p4") + probe4.cleanup(t) + + // 4. Unexpected write created during test is cleaned up. + probe5 := allocateSharedDirectoryProbe(t, dir, "p5") + if err := os.WriteFile(probe5.Path(), []byte("leaked"), 0o600); err != nil { + t.Fatalf("write probe5 file: %v", err) + } + probe5.cleanup(t) + if _, err := os.Lstat(probe5.Path()); !os.IsNotExist(err) { + t.Fatalf("expected probe5 to be cleaned up after creation, stat err=%v", err) + } +} diff --git a/internal/sandbox/windows_acl.go b/internal/sandbox/windows_acl.go index 55d37d347..96a18004a 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,28 @@ 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 is consumed by + // applyWindowsACLPlan for migration cleanup and tests; no current + // plan-generation path emits this action, preserving legacy-process confinement. + WindowsACLRevokeCapability WindowsACLAction = "revoke-capability" ) 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), which is why direct-only denies must set this + // flag rather than rely on inheritance. + NoInherit bool `json:"noInherit,omitempty"` + Materialize bool `json:"materialize,omitempty"` } type WindowsACLPlan struct { @@ -76,6 +92,7 @@ func BuildWindowsACLPlan(config WindowsSandboxCommandConfig) (WindowsACLPlan, er }) } } + return WindowsACLPlan{Entries: dedupeWindowsACLEntries(entries)}, nil } @@ -184,7 +201,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..facd166ff 100644 --- a/internal/sandbox/windows_acl_apply_windows.go +++ b/internal/sandbox/windows_acl_apply_windows.go @@ -8,12 +8,20 @@ import ( "os" "sort" "strings" + "unsafe" "golang.org/x/sys/windows" ) const windowsFileDeleteChild windows.ACCESS_MASK = 0x00000040 +const ( + windowsAccessAllowedObjectAceType = 0x5 + windowsAccessDeniedObjectAceType = 0x6 + windowsAccessAllowedCallbackAceType = 0x9 + windowsAccessAllowedCallbackObjectAceType = 0xB +) + type windowsACLPathGroup struct { Path string Entries []WindowsACLEntry @@ -123,13 +131,18 @@ func applyWindowsACLPathGroup(group windowsACLPathGroup) (windowsACLSnapshot, bo if err != nil { return fail(fmt.Errorf("read windows DACL for %s: %w", path, err)) } - accessEntries, err := windowsExplicitAccessEntries(group.Entries, isDir) + baseDACL, accessEntries, err := prepareWindowsACLPathGroupEntries(group.Entries, isDir, oldDACL) if err != nil { return fail(err) } - nextDACL, err := windows.ACLFromEntries(accessEntries, oldDACL) - if err != nil { - return fail(fmt.Errorf("build windows ACL for %s: %w", path, err)) + var nextDACL *windows.ACL + if len(accessEntries) > 0 { + nextDACL, err = windows.ACLFromEntries(accessEntries, baseDACL) + if err != nil { + return fail(fmt.Errorf("build windows ACL for %s: %w", path, err)) + } + } else { + nextDACL = baseDACL } if err := windows.SetSecurityInfo(handle, windows.SE_FILE_OBJECT, windows.DACL_SECURITY_INFORMATION, nil, nil, nextDACL, nil); err != nil { return fail(fmt.Errorf("apply windows ACL for %s: %w", path, err)) @@ -191,20 +204,49 @@ func windowsACLGroupRequiresExistingTarget(group windowsACLPathGroup) bool { return false } -func windowsExplicitAccessEntries(entries []WindowsACLEntry, isDir bool) ([]windows.EXPLICIT_ACCESS, error) { - out := make([]windows.EXPLICIT_ACCESS, 0, len(entries)) - inheritance := uint32(0) - if isDir { - inheritance = windows.SUB_CONTAINERS_AND_OBJECTS_INHERIT - } +func prepareWindowsACLPathGroupEntries(entries []WindowsACLEntry, isDir bool, oldDACL *windows.ACL) (*windows.ACL, []windows.EXPLICIT_ACCESS, error) { + baseDACL := oldDACL + var out []windows.EXPLICIT_ACCESS for _, entry := range entries { sid, err := windows.StringToSid(entry.Capability) if err != nil { - return nil, fmt.Errorf("parse windows capability SID %q: %w", entry.Capability, err) + return nil, nil, fmt.Errorf("parse windows capability SID %q: %w", entry.Capability, err) + } + if entry.Action == WindowsACLRevokeCapability { + // Clear all explicit write-deny ACEs for sid from baseDACL while preserving any DenyRead + if baseDACL != nil { + filtered, err := windowsFilterDACL(baseDACL, sid) + if err != nil { + return nil, nil, err + } + baseDACL = filtered + } + continue + } + if entry.Action == WindowsACLDenyWrite { + // Replace any pre-existing broader DenyWrite mask (e.g. from + // builds that included SYNCHRONIZE) with the current narrow + // mask. We patch the mask in-place within a DACL copy rather + // than filtering the old ACE and re-adding via ACLFromEntries, + // because SetEntriesInAcl merges DENY entries for the same + // SID — which would combine the new DenyWrite with any + // co-resident DenyRead into a single deny-all ACE. + if baseDACL != nil && windowsHasExplicitDenyWriteForSID(baseDACL, sid) { + migrated, err := windowsMigrateDenyWriteInDACL(baseDACL, sid) + if err != nil { + return nil, nil, err + } + baseDACL = migrated + continue + } } accessMode, permissions, err := windowsACLAccess(entry.Action) if err != nil { - return nil, err + return nil, nil, err + } + inheritance := uint32(0) + if isDir && !entry.NoInherit { + inheritance = windows.SUB_CONTAINERS_AND_OBJECTS_INHERIT } out = append(out, windows.EXPLICIT_ACCESS{ AccessPermissions: permissions, @@ -217,9 +259,230 @@ func windowsExplicitAccessEntries(entries []WindowsACLEntry, isDir bool) ([]wind }, }) } + return baseDACL, out, nil +} + +type windowsACLHeader struct { + AclRevision byte + Sbz1 byte + AclSize uint16 + AceCount uint16 + Sbz2 uint16 +} + +func windowsFilterDACL(oldDACL *windows.ACL, removeSID *windows.SID) (*windows.ACL, error) { + if oldDACL == nil || removeSID == nil { + return oldDACL, nil + } + var keepBytes uint32 = uint32(unsafe.Sizeof(windowsACLHeader{})) + var keepCount uint16 = 0 + for i := uint32(0); i < uint32(oldDACL.AceCount); i++ { + var ace *windows.ACCESS_ALLOWED_ACE + if err := windows.GetAce(oldDACL, i, &ace); err != nil { + return nil, fmt.Errorf("read ACE %d for filter: %w", i, err) + } + if ace.Header.AceFlags&windows.INHERITED_ACE == 0 { + if ace.Header.AceType == windows.ACCESS_DENIED_ACE_TYPE || ace.Header.AceType == windowsAccessDeniedObjectAceType { + if sid, ok := windowsAceSID(ace); ok && sid.Equals(removeSID) && windowsIsExperimentalWriteDenyMask(ace.Mask) { + continue + } + } + } + keepBytes += uint32(ace.Header.AceSize) + keepCount++ + } + + buf := make([]byte, keepBytes) + hdr := (*windowsACLHeader)(unsafe.Pointer(&buf[0])) + oldHdr := (*windowsACLHeader)(unsafe.Pointer(oldDACL)) + *hdr = *oldHdr + hdr.AclSize = uint16(keepBytes) + hdr.AceCount = keepCount + + offset := unsafe.Sizeof(windowsACLHeader{}) + for i := uint32(0); i < uint32(oldDACL.AceCount); i++ { + var ace *windows.ACCESS_ALLOWED_ACE + if err := windows.GetAce(oldDACL, i, &ace); err != nil { + return nil, fmt.Errorf("read ACE %d for copy: %w", i, err) + } + if ace.Header.AceFlags&windows.INHERITED_ACE == 0 { + if ace.Header.AceType == windows.ACCESS_DENIED_ACE_TYPE || ace.Header.AceType == windowsAccessDeniedObjectAceType { + if sid, ok := windowsAceSID(ace); ok && sid.Equals(removeSID) && windowsIsExperimentalWriteDenyMask(ace.Mask) { + continue + } + } + } + aceSize := uintptr(ace.Header.AceSize) + srcSlice := unsafe.Slice((*byte)(unsafe.Pointer(ace)), aceSize) + copy(buf[offset:offset+aceSize], srcSlice) + offset += aceSize + } + + return (*windows.ACL)(unsafe.Pointer(hdr)), nil +} + +// windowsMigrateDenyWriteInDACL copies oldDACL and narrows any explicit +// deny-write ACE for targetSID to the current narrow mask, preserving all +// other ACEs (including DenyRead) in their original positions. This avoids +// SetEntriesInAcl's merging behavior that would combine separate deny ACEs +// for the same SID into a single full-deny ACE. +func windowsMigrateDenyWriteInDACL(oldDACL *windows.ACL, targetSID *windows.SID) (*windows.ACL, error) { + if oldDACL == nil || targetSID == nil { + return oldDACL, nil + } + _, narrowMask, err := windowsACLAccess(WindowsACLDenyWrite) + if err != nil { + return nil, err + } + + oldHdr := (*windowsACLHeader)(unsafe.Pointer(oldDACL)) + buf := make([]byte, oldHdr.AclSize) + src := unsafe.Slice((*byte)(unsafe.Pointer(oldDACL)), oldHdr.AclSize) + copy(buf, src) + + newDACL := (*windows.ACL)(unsafe.Pointer(&buf[0])) + for i := uint32(0); i < uint32(newDACL.AceCount); i++ { + var ace *windows.ACCESS_ALLOWED_ACE + if err := windows.GetAce(newDACL, i, &ace); err != nil { + return nil, fmt.Errorf("read ACE %d for migration: %w", i, err) + } + if ace.Header.AceFlags&windows.INHERITED_ACE != 0 { + continue + } + if ace.Header.AceType != windows.ACCESS_DENIED_ACE_TYPE && ace.Header.AceType != windowsAccessDeniedObjectAceType { + continue + } + sid, ok := windowsAceSID(ace) + if !ok || !sid.Equals(targetSID) { + continue + } + if windowsIsExperimentalWriteDenyMask(ace.Mask) { + ace.Mask = narrowMask + } + } + + return newDACL, nil +} + +func windowsHasExplicitDenyWriteForSID(oldDACL *windows.ACL, wantSID *windows.SID) bool { + if oldDACL == nil || wantSID == nil { + return false + } + for index := uint16(0); index < oldDACL.AceCount; index++ { + var ace *windows.ACCESS_ALLOWED_ACE + if err := windows.GetAce(oldDACL, uint32(index), &ace); err != nil { + continue + } + if ace.Header.AceType != windows.ACCESS_DENIED_ACE_TYPE && ace.Header.AceType != windowsAccessDeniedObjectAceType { + continue + } + if ace.Header.AceFlags&windows.INHERITED_ACE != 0 { + continue + } + sid, ok := windowsAceSID(ace) + if !ok || !sid.Equals(wantSID) { + continue + } + if windowsIsExperimentalWriteDenyMask(ace.Mask) { + return true + } + } + return false +} + +// windowsPreservedReadDenyAccessEntries returns DENY_ACCESS EXPLICIT_ACCESS +// 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.INHERITED_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), keeping their original inheritance + // scope rather than promoting every variant to container+object or + // dropping inherit-only ACEs that SET_ACCESS zero-mask already cleared. + inheritance := uint32(0) + if isDir { + inheritance = uint32(ace.Header.AceFlags) & (windows.OBJECT_INHERIT_ACE | + windows.CONTAINER_INHERIT_ACE | + windows.NO_PROPAGATE_INHERIT_ACE | + windows.INHERIT_ONLY_ACE) + } + out = append(out, windows.EXPLICIT_ACCESS{ + AccessPermissions: ace.Mask, + AccessMode: windows.DENY_ACCESS, + Inheritance: inheritance, + Trustee: windows.TRUSTEE{ + TrusteeForm: windows.TRUSTEE_IS_SID, + TrusteeType: windows.TRUSTEE_IS_GROUP, + TrusteeValue: windows.TrusteeValueFromSID(wantSID), + }, + }) + } return out, nil } +// windowsIsExperimentalWriteDenyMask reports whether mask is a synthetic +// DenyWrite (or partial write deny) from earlier broadening builds — the only +// ACEs migration revoke may drop for the stable ReadOnly SID. Pure DenyRead +// masks share some STANDARD_RIGHTS bits with FILE_GENERIC_WRITE, so this keys +// off content-write / delete / DAC bits that DenyRead never carries. +func windowsIsExperimentalWriteDenyMask(mask windows.ACCESS_MASK) bool { + _, writeMask, err := windowsACLAccess(WindowsACLDenyWrite) + if err != nil { + return false + } + if mask&writeMask == writeMask { + return true + } + // Content-write / ownership bits unique to write denies (not in DenyRead's + // FILE_GENERIC_READ|FILE_GENERIC_EXECUTE mask alone). + const writeContent = windows.FILE_WRITE_DATA | windows.FILE_APPEND_DATA | + windows.FILE_WRITE_EA | windows.FILE_WRITE_ATTRIBUTES | + windowsFileDeleteChild | windows.DELETE | windows.WRITE_DAC | windows.WRITE_OWNER + return mask&writeContent != 0 +} + +func windowsAceSID(ace *windows.ACCESS_ALLOWED_ACE) (sid *windows.SID, ok bool) { + switch ace.Header.AceType { + case windows.ACCESS_ALLOWED_ACE_TYPE, windows.ACCESS_DENIED_ACE_TYPE, windowsAccessAllowedCallbackAceType: + return (*windows.SID)(unsafe.Pointer(&ace.SidStart)), true + case windowsAccessAllowedObjectAceType, windowsAccessDeniedObjectAceType, windowsAccessAllowedCallbackObjectAceType: + flags := ace.SidStart + offset := unsafe.Sizeof(ace.SidStart) + if flags&windows.ACE_OBJECT_TYPE_PRESENT != 0 { + offset += 16 + } + if flags&windows.ACE_INHERITED_OBJECT_TYPE_PRESENT != 0 { + offset += 16 + } + return (*windows.SID)(unsafe.Pointer(uintptr(unsafe.Pointer(&ace.SidStart)) + offset)), true + default: + return nil, false + } +} + func windowsACLAccess(action WindowsACLAction) (windows.ACCESS_MODE, windows.ACCESS_MASK, error) { switch action { case WindowsACLAllowWrite: @@ -227,7 +490,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..a772d7c2e 100644 --- a/internal/sandbox/windows_acl_apply_windows_test.go +++ b/internal/sandbox/windows_acl_apply_windows_test.go @@ -48,6 +48,49 @@ func TestApplyWindowsACLPathGroupHandleBasedRoundTrip(t *testing.T) { } } +// dirDeniesReadSID reports whether path's DACL has a DENY ACE for wantSID whose +// mask covers FILE_GENERIC_READ (DenyRead shape) without the full write-probe +// mask of experimental DenyWrite. +func dirDeniesReadSID(t *testing.T, path, wantSID string) bool { + t.Helper() + want, err := windows.StringToSid(wantSID) + if err != nil { + t.Fatalf("StringToSid %q: %v", wantSID, err) + } + sd, err := windows.GetNamedSecurityInfo(path, windows.SE_FILE_OBJECT, windows.DACL_SECURITY_INFORMATION) + if err != nil { + t.Fatalf("GetNamedSecurityInfo %s: %v", path, err) + } + dacl, _, err := sd.DACL() + if err != nil { + t.Fatalf("DACL %s: %v", path, err) + } + if dacl == nil { + return false + } + _, readMask, err := windowsACLAccess(WindowsACLDenyRead) + if err != nil { + t.Fatalf("windowsACLAccess DenyRead: %v", err) + } + for index := uint16(0); index < dacl.AceCount; index++ { + var ace *windows.ACCESS_ALLOWED_ACE + if err := windows.GetAce(dacl, uint32(index), &ace); err != nil { + t.Fatalf("GetAce %d of %s: %v", index, path, err) + } + if ace.Header.AceType != windows.ACCESS_DENIED_ACE_TYPE && ace.Header.AceType != windowsAccessDeniedObjectAceType { + continue + } + sid, ok := windowsAceSID(ace) + if !ok || !sid.Equals(want) { + continue + } + if ace.Mask&readMask == readMask && !windowsIsExperimentalWriteDenyMask(ace.Mask) { + return true + } + } + return false +} + // A materialized target that does not exist yet is created, ACL'd through the // handle, and removed on rollback. func TestApplyWindowsACLPathGroupMaterializes(t *testing.T) { @@ -140,3 +183,155 @@ func TestOpenWindowsACLTargetReportsIsDir(t *testing.T) { t.Fatal("isDir = true for a regular file, want false") } } + +// TestWindowsACLDenyWriteMigratesLegacySynchronizeMask regression tests that an +// existing legacy DenyWrite ACE containing SYNCHRONIZE (from older PR builds) is +// replaced in-place with the narrow mask that excludes SYNCHRONIZE, preserving +// co-resident DenyRead ACEs and operating idempotently. +func TestWindowsACLDenyWriteMigratesLegacySynchronizeMask(t *testing.T) { + dir := t.TempDir() + childDir := filepath.Join(dir, "sub") + if err := os.Mkdir(childDir, 0o755); err != nil { + t.Fatalf("mkdir childDir: %v", err) + } + childFile := filepath.Join(childDir, "child.txt") + if err := os.WriteFile(childFile, []byte("data"), 0o644); err != nil { + t.Fatalf("write childFile: %v", err) + } + + caps, err := LoadOrCreateWindowsCapabilitySIDs(t.TempDir()) + if err != nil { + t.Fatalf("LoadOrCreateWindowsCapabilitySIDs: %v", err) + } + sidStr := caps.ReadOnly + sid, err := windows.StringToSid(sidStr) + if err != nil { + t.Fatalf("StringToSid: %v", err) + } + + // 1. Seed legacy DenyWrite ACE containing SYNCHRONIZE + co-resident DenyRead ACE. + legacyWriteMask := (windows.FILE_GENERIC_WRITE | windows.DELETE | windowsFileDeleteChild | windows.WRITE_DAC | windows.WRITE_OWNER | windows.SYNCHRONIZE) + _, readMask, err := windowsACLAccess(WindowsACLDenyRead) + if err != nil { + t.Fatalf("windowsACLAccess DenyRead: %v", err) + } + seedEntries := []windows.EXPLICIT_ACCESS{ + { + AccessPermissions: legacyWriteMask, + AccessMode: windows.DENY_ACCESS, + Inheritance: windows.SUB_CONTAINERS_AND_OBJECTS_INHERIT, + Trustee: windows.TRUSTEE{ + TrusteeForm: windows.TRUSTEE_IS_SID, + TrusteeType: windows.TRUSTEE_IS_GROUP, + TrusteeValue: windows.TrusteeValueFromSID(sid), + }, + }, + { + AccessPermissions: readMask, + AccessMode: windows.DENY_ACCESS, + Inheritance: windows.SUB_CONTAINERS_AND_OBJECTS_INHERIT, + Trustee: windows.TRUSTEE{ + TrusteeForm: windows.TRUSTEE_IS_SID, + TrusteeType: windows.TRUSTEE_IS_GROUP, + TrusteeValue: windows.TrusteeValueFromSID(sid), + }, + }, + } + handle, _, err := openWindowsACLTarget(dir) + if err != nil { + t.Fatalf("openWindowsACLTarget: %v", err) + } + seededDACL, err := windows.ACLFromEntries(seedEntries, nil) + if err != nil { + _ = windows.CloseHandle(handle) + t.Fatalf("ACLFromEntries: %v", err) + } + if err := windows.SetSecurityInfo(handle, windows.SE_FILE_OBJECT, windows.DACL_SECURITY_INFORMATION, nil, nil, seededDACL, nil); err != nil { + _ = windows.CloseHandle(handle) + t.Fatalf("SetSecurityInfo: %v", err) + } + _ = windows.CloseHandle(handle) + + // 2. Apply WindowsACLPlan with new narrow DenyWrite action. + plan := WindowsACLPlan{ + Entries: []WindowsACLEntry{{ + Action: WindowsACLDenyWrite, + Path: dir, + Capability: sidStr, + }}, + } + rollback, err := applyWindowsACLPlan(plan) + if err != nil { + t.Fatalf("applyWindowsACLPlan: %v", err) + } + t.Cleanup(func() { _ = rollback() }) + + // 3. Verify effective DACL: SYNCHRONIZE must NOT be denied, write rights denied, DenyRead preserved. + sd, err := windows.GetNamedSecurityInfo(dir, windows.SE_FILE_OBJECT, windows.DACL_SECURITY_INFORMATION) + if err != nil { + t.Fatalf("GetNamedSecurityInfo: %v", err) + } + dacl, _, err := sd.DACL() + if err != nil { + t.Fatalf("DACL: %v", err) + } + + var hasNarrowWriteDeny, hasSynchronizeDeny, hasReadDeny bool + _, narrowWriteMask, err := windowsACLAccess(WindowsACLDenyWrite) + if err != nil { + t.Fatalf("windowsACLAccess: %v", err) + } + for i := uint16(0); i < dacl.AceCount; i++ { + var ace *windows.ACCESS_ALLOWED_ACE + if err := windows.GetAce(dacl, uint32(i), &ace); err != nil { + t.Fatalf("GetAce: %v", err) + } + if ace.Header.AceType != windows.ACCESS_DENIED_ACE_TYPE && ace.Header.AceType != windowsAccessDeniedObjectAceType { + continue + } + aceSID, ok := windowsAceSID(ace) + if !ok || !aceSID.Equals(sid) { + continue + } + if ace.Mask&windows.SYNCHRONIZE != 0 && windowsIsExperimentalWriteDenyMask(ace.Mask) { + hasSynchronizeDeny = true + } + if ace.Mask&narrowWriteMask == narrowWriteMask { + hasNarrowWriteDeny = true + } + if ace.Mask&readMask == readMask && !windowsIsExperimentalWriteDenyMask(ace.Mask) { + hasReadDeny = true + } + } + + if hasSynchronizeDeny { + t.Fatal("resulting DACL still denies SYNCHRONIZE for trustee; migration failed to narrow mask") + } + if !hasNarrowWriteDeny { + t.Fatal("resulting DACL is missing narrow DenyWrite ACE") + } + if !hasReadDeny { + t.Fatal("resulting DACL lost co-resident DenyRead ACE during migration") + } + + // 4. Assert synchronous directory read works. + if entries, err := os.ReadDir(dir); err != nil || len(entries) == 0 { + t.Fatalf("os.ReadDir failed on migrated directory: entries=%v, err=%v", entries, err) + } + + // 5. Assert second apply is idempotent. + if _, err := applyWindowsACLPlan(plan); err != nil { + t.Fatalf("second applyWindowsACLPlan failed: %v", err) + } + sd2, err := windows.GetNamedSecurityInfo(dir, windows.SE_FILE_OBJECT, windows.DACL_SECURITY_INFORMATION) + if err != nil { + t.Fatalf("GetNamedSecurityInfo 2: %v", err) + } + dacl2, _, err := sd2.DACL() + if err != nil { + t.Fatalf("DACL 2: %v", err) + } + if dacl2.AceCount != dacl.AceCount { + t.Fatalf("second apply changed ACE count: %d vs %d", dacl2.AceCount, dacl.AceCount) + } +} diff --git a/internal/sandbox/windows_acl_test.go b/internal/sandbox/windows_acl_test.go index 1925bd8a9..257198cd6 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,20 +53,52 @@ func TestBuildWindowsACLPlanForWorkspaceWriteProfile(t *testing.T) { assertWindowsACLEntry(t, plan, WindowsACLDenyWrite, `C:\workspace\secret-write`, cacheSID, false) assertWindowsACLEntry(t, plan, WindowsACLDenyRead, `C:\workspace\secret-read`, workspaceSID, true) assertWindowsACLEntry(t, plan, WindowsACLDenyRead, `C:\workspace\secret-read`, cacheSID, true) + + // SID broadening is disabled, so the plan must not stamp shared system-path + // DenyWrite ACEs or revoke legacy capability SIDs. Revocation could weaken + // the boundary of a command launched by an earlier build. + assertNoSharedSystemDenyWrites(t, plan) + assertNoWindowsACLRevokes(t, plan) } -func TestBuildWindowsACLPlanUsesReadOnlySIDWithoutWriteRoots(t *testing.T) { +// TestBuildWindowsACLPlanOmitsSharedDenyPathsWithoutDenyRead pins that +// profiles without DenyRead never stamp shared system-path DenyWrite ACEs or +// revoke old capability-SID guards that a running sandbox may still require. +func TestBuildWindowsACLPlanOmitsSharedDenyPathsWithoutDenyRead(t *testing.T) { home := t.TempDir() - caps, err := LoadOrCreateWindowsCapabilitySIDs(home) + 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("LoadOrCreateWindowsCapabilitySIDs: %v", err) + t.Fatalf("BuildWindowsACLPlan: %v", err) } + assertNoSharedSystemDenyWrites(t, plan) + assertNoWindowsACLRevokes(t, plan) +} + +// TestBuildWindowsACLPlanOmitsSharedDenyPathsWhenUnelevated pins that the +// unelevated tier never stamps shared system-path DenyWrite ACEs (it also +// never broadens the restricted-SID list). +func TestBuildWindowsACLPlanOmitsSharedDenyPathsWhenUnelevated(t *testing.T) { + home := t.TempDir() plan, err := BuildWindowsACLPlan(WindowsSandboxCommandConfig{ - SandboxHome: home, + SandboxHome: home, + WorkspaceRoots: []string{`C:\workspace`}, + SandboxLevel: WindowsSandboxLevelUnelevated, PermissionProfile: PermissionProfile{ FileSystem: FileSystemPolicy{ - Kind: FileSystemRestricted, - DenyRead: []string{`C:\workspace\secret-read`}, + Kind: FileSystemRestricted, + WriteRoots: []WritableRoot{{Root: `C:\workspace`}}, + DenyRead: []string{`C:\workspace\secret`}, }, Network: NetworkPolicy{Mode: NetworkDeny}, }, @@ -73,10 +106,52 @@ func TestBuildWindowsACLPlanUsesReadOnlySIDWithoutWriteRoots(t *testing.T) { if err != nil { t.Fatalf("BuildWindowsACLPlan: %v", err) } - if len(plan.Entries) != 1 { - t.Fatalf("ACL entries = %#v, want one deny-read entry", plan.Entries) + assertNoSharedSystemDenyWrites(t, plan) + assertNoWindowsACLRevokes(t, plan) +} + +// TestBuildWindowsACLPlanDoesNotRevokeLegacyGuards pins that a setup run does +// not remove persistent guards installed by an older build. A previously +// launched sandbox can still carry the legacy capability SID, so removing its +// deny would widen that process's access. +func TestBuildWindowsACLPlanDoesNotRevokeLegacyGuards(t *testing.T) { + home := t.TempDir() + plan, err := BuildWindowsACLPlan(WindowsSandboxCommandConfig{ + SandboxHome: home, + WorkspaceRoots: []string{`C:\workspace`}, + SandboxLevel: WindowsSandboxLevelRestrictedToken, + PermissionProfile: PermissionProfile{ + FileSystem: FileSystemPolicy{ + Kind: FileSystemRestricted, + WriteRoots: []WritableRoot{{Root: `C:\workspace`}}, + }, + Network: NetworkPolicy{Mode: NetworkDeny}, + }, + }) + if err != nil { + t.Fatalf("BuildWindowsACLPlan: %v", err) + } + assertNoWindowsACLRevokes(t, plan) +} + +func assertNoSharedSystemDenyWrites(t *testing.T, plan WindowsACLPlan) { + t.Helper() + for _, path := range []string{`C:\`, `C:\ProgramData`, `C:\Windows\Temp`, `C:\Users\Public`} { + for _, entry := range plan.Entries { + if entry.Action == WindowsACLDenyWrite && windowsCapabilityPathKey(entry.Path) == windowsCapabilityPathKey(path) { + t.Fatalf("plan stamps shared system DenyWrite on %q = %#v; SID broadening is disabled so shared denies must not be planned", path, entry) + } + } + } +} + +func assertNoWindowsACLRevokes(t *testing.T, plan WindowsACLPlan) { + t.Helper() + for _, entry := range plan.Entries { + if entry.Action == WindowsACLRevokeCapability { + t.Fatalf("plan = %#v, want no WindowsACLRevokeCapability entries", plan.Entries) + } } - assertWindowsACLEntry(t, plan, WindowsACLDenyRead, `C:\workspace\secret-read`, caps.ReadOnly, true) } func TestBuildWindowsACLPlanRejectsUnrestrictedProfiles(t *testing.T) { @@ -117,16 +192,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 +219,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..21397040c 100644 --- a/internal/sandbox/windows_command_runner.go +++ b/internal/sandbox/windows_command_runner.go @@ -11,9 +11,52 @@ func RunWindowsSandboxCommandRunner(args []string, stderr io.Writer) int { fmt.Fprintln(stderr, WindowsSandboxCommandRunnerName+": "+err.Error()) return 2 } + // Reject unsupported DenyRead profiles before minting persistent capability + // SID state under SandboxHome (defense in depth: runWindowsSandboxCommand + // also checks, but only after LoadOrCreateWindowsCapabilitySIDs). + if err := windowsDenyReadRestrictedTokenUnsupported(config); err != nil { + fmt.Fprintln(stderr, WindowsSandboxCommandRunnerName+": "+err.Error()) + return 1 + } if _, err := LoadOrCreateWindowsCapabilitySIDs(config.SandboxHome); err != nil { fmt.Fprintln(stderr, WindowsSandboxCommandRunnerName+": "+err.Error()) return 1 } return runWindowsSandboxCommand(config, stderr) } + +// windowsDenyReadRestrictedTokenUnsupported reports that Windows restricted- +// token sandboxing (elevated restricted-token or unelevated) cannot run +// profiles with DenyRead until access-time confinement exists. Both runner +// levels build the same fully restricted narrow-SID token when DenyRead is +// set: without Users/AuthUsers it cannot load ordinary system executables; +// adding those groups reopens write grants outside WriteRoots. Prefer a clear +// rejection over a silent launch failure. Do not recommend the other tier as a +// workaround: the limitation is the token mechanism, not elevation. +func windowsDenyReadRestrictedTokenUnsupported(config WindowsSandboxCommandConfig) error { + switch config.SandboxLevel { + case WindowsSandboxLevelRestrictedToken, WindowsSandboxLevelUnelevated: + default: + return nil + } + return windowsDenyReadRestrictedTokenUnsupportedProfile(config.PermissionProfile) +} + +// windowsDenyReadRestrictedTokenUnsupportedProfile is the level-agnostic check +// used by the manager, command-plan builder, setup, and runner so DenyRead is +// rejected before any restricted-token path can provision or launch. +func windowsDenyReadRestrictedTokenUnsupportedProfile(profile PermissionProfile) error { + if len(profile.FileSystem.DenyRead) == 0 { + return nil + } + return fmt.Errorf( + "DenyRead is not supported with the Windows restricted-token sandbox "+ + "(elevated or unelevated): without Users/Authenticated Users in the "+ + "restricting SID set, ordinary system binaries under Program Files and "+ + "Windows cannot load, and adding those groups would admit their existing "+ + "write grants outside WriteRoots. "+ + "Remove DenyRead from this configuration to use the sandbox on Windows. "+ + "Configured DenyRead path count: %d", + len(profile.FileSystem.DenyRead), + ) +} diff --git a/internal/sandbox/windows_command_runner_test.go b/internal/sandbox/windows_command_runner_test.go new file mode 100644 index 000000000..b6cec3645 --- /dev/null +++ b/internal/sandbox/windows_command_runner_test.go @@ -0,0 +1,63 @@ +package sandbox + +import ( + "strings" + "testing" +) + +func TestWindowsDenyReadRestrictedTokenUnsupported(t *testing.T) { + // Both restricted-token runner levels reject DenyRead before launch/setup. + for _, level := range []WindowsSandboxLevel{ + WindowsSandboxLevelRestrictedToken, + WindowsSandboxLevelUnelevated, + } { + err := windowsDenyReadRestrictedTokenUnsupported(WindowsSandboxCommandConfig{ + SandboxLevel: level, + PermissionProfile: PermissionProfile{ + FileSystem: FileSystemPolicy{ + Kind: FileSystemRestricted, + DenyRead: []string{`C:\secret`, `D:\private`}, + }, + }, + }) + if err == nil { + t.Fatalf("expected unsupported error for %s DenyRead profile", level) + } + msg := err.Error() + for _, want := range []string{"DenyRead", "not supported", "restricted-token", "unelevated", "path count: 2"} { + if !strings.Contains(msg, want) { + t.Fatalf("%s error %q missing %q", level, msg, want) + } + } + // DenyRead often names credential or private-file paths; keep them out of stderr. + for _, secret := range []string{`C:\secret`, `D:\private`} { + if strings.Contains(msg, secret) { + t.Fatalf("%s error leaked DenyRead path %q: %q", level, secret, msg) + } + } + if strings.Contains(msg, "--sandbox forbid") { + t.Fatalf("%s error advertises unsupported --sandbox forbid recovery: %q", level, msg) + } + if strings.Contains(msg, "sandbox_permissions") || strings.Contains(msg, "require_escalated") { + t.Fatalf("%s error advertises unusable escalation recovery: %q", level, msg) + } + if !strings.Contains(msg, "Remove DenyRead") { + t.Fatalf("%s error should advise removing DenyRead: %q", level, msg) + } + } + + // No DenyRead: allowed (WRITE_RESTRICTED path can launch system tools). + for _, level := range []WindowsSandboxLevel{ + WindowsSandboxLevelRestrictedToken, + WindowsSandboxLevelUnelevated, + } { + if err := windowsDenyReadRestrictedTokenUnsupported(WindowsSandboxCommandConfig{ + SandboxLevel: level, + PermissionProfile: PermissionProfile{ + FileSystem: FileSystemPolicy{Kind: FileSystemRestricted}, + }, + }); err != nil { + t.Fatalf("unexpected error without DenyRead at %s: %v", level, err) + } + } +} diff --git a/internal/sandbox/windows_command_runner_windows.go b/internal/sandbox/windows_command_runner_windows.go index 0b9e8f64e..de8eebfea 100644 --- a/internal/sandbox/windows_command_runner_windows.go +++ b/internal/sandbox/windows_command_runner_windows.go @@ -8,6 +8,15 @@ import ( ) func runWindowsSandboxCommand(config WindowsSandboxCommandConfig, stderr io.Writer) int { + // Fully restricted DenyRead tokens cannot load ordinary Users-granted + // system binaries without SID broadening; broadening is permanently off + // because it admits write grants outside WriteRoots. Reject on both the + // elevated and unelevated restricted-token tiers before setup or launch + // until access-time confinement exists (PR #640). + if err := windowsDenyReadRestrictedTokenUnsupported(config); err != nil { + fmt.Fprintln(stderr, WindowsSandboxCommandRunnerName+": "+err.Error()) + return 1 + } switch config.SandboxLevel { case WindowsSandboxLevelRestrictedToken: if err := ValidateWindowsSandboxSetupMarker(WindowsSandboxSetupConfigFromCommand(config)); err != nil { @@ -69,11 +78,11 @@ func runWindowsSandboxCommand(config WindowsSandboxCommandConfig, stderr io.Writ // this has no in-token fix; preflight blocking and output hints live in // internal/tools/shell_runtime.go. tokenSIDs := windowsRuntimeTokenSIDs(capabilitySIDs, offlineSID, config.PermissionProfile.Network.Mode) - // A WRITE_RESTRICTED token keeps reads unrestricted so sandboxed commands - // can actually launch executables; it is only unsafe when DenyRead paths - // are configured, because the kernel skips restricted-SID deny ACEs for - // reads under that flag (#612). Profiles with DenyRead keep the fully - // restricted token, trading spawn capability for read-deny enforcement. + // WRITE_RESTRICTED keeps reads unrestricted so sandboxed commands can load + // Users-granted executables. It is only used when DenyRead is empty (#612: + // WRITE_RESTRICTED skips restricted-SID deny ACEs for reads). Non-empty + // DenyRead is rejected above for both runner levels rather than launching a + // fully restricted narrow-SID token that cannot execute normal tools. writeRestricted := len(config.PermissionProfile.FileSystem.DenyRead) == 0 token, err := createWindowsRestrictedTokenForCapabilitySIDs(tokenSIDs, writeRestricted) if err != nil { diff --git a/internal/sandbox/windows_runner.go b/internal/sandbox/windows_runner.go index 032f2a844..400ecad58 100644 --- a/internal/sandbox/windows_runner.go +++ b/internal/sandbox/windows_runner.go @@ -317,6 +317,12 @@ func ParseWindowsSandboxCommandArgs(args []string) (WindowsSandboxCommandConfig, } func windowsRestrictedTokenCommandPlan(execRequest SandboxExecutionRequest, policy Policy) (CommandPlan, error) { + // Reject DenyRead before provisioning either restricted-token runner level. + // Both elevated and unelevated build the same fully restricted narrow-SID + // token for DenyRead profiles, which cannot load ordinary system binaries. + if err := windowsDenyReadRestrictedTokenUnsupportedProfile(execRequest.PermissionProfile); err != nil { + return CommandPlan{}, err + } spec := execRequest.Command var sandboxHomeEnv map[string]string if spec.Env != nil { @@ -329,7 +335,7 @@ func windowsRestrictedTokenCommandPlan(execRequest SandboxExecutionRequest, poli childEnv := sandboxEnvironmentForCommandWithSensitiveEnv(spec.Env, policy, BackendWindowsRestrictedToken, execRequest.WorkspaceRoot, spec.sensitiveEnvKeys) childEnv = sandboxRuntimeEnvironment(childEnv, execRequest.PermissionProfile.Runtime) // The unelevated enforcement tier maps to the runner's unelevated level: same - // restricted token, but the runner applies the workspace ACLs itself instead + // restricted token, but the runner applies the workspace ACL plan itself instead // of requiring the elevated setup marker. level := WindowsSandboxLevelRestrictedToken if execRequest.EnforcementLevel == EnforcementUnelevated { diff --git a/internal/sandbox/windows_setup.go b/internal/sandbox/windows_setup.go index 3fc9634e8..ac7366a7c 100644 --- a/internal/sandbox/windows_setup.go +++ b/internal/sandbox/windows_setup.go @@ -15,7 +15,7 @@ import ( const WindowsSandboxSetupName = "zero-windows-sandbox-setup.exe" -const windowsSandboxSetupMarkerSchemaVersion = 4 +const windowsSandboxSetupMarkerSchemaVersion = 5 type WindowsSandboxSetupArgsOptions struct { SandboxHome string @@ -145,6 +145,10 @@ func RunWindowsSandboxSetup(args []string, stderr io.Writer) int { fmt.Fprintln(stderr, WindowsSandboxSetupName+": "+err.Error()) return 2 } + if err := windowsDenyReadRestrictedTokenUnsupportedProfile(config.PermissionProfile); err != nil { + fmt.Fprintln(stderr, WindowsSandboxSetupName+": "+err.Error()) + return 1 + } return runWindowsSandboxSetup(config, stderr) } diff --git a/internal/sandbox/windows_setup_test.go b/internal/sandbox/windows_setup_test.go index 0a3c6f044..ecb76831a 100644 --- a/internal/sandbox/windows_setup_test.go +++ b/internal/sandbox/windows_setup_test.go @@ -63,6 +63,37 @@ func TestRunWindowsSandboxSetupRejectsInvalidArgs(t *testing.T) { } } +func TestRunWindowsSandboxSetupRejectsDenyReadUpfront(t *testing.T) { + home := t.TempDir() + args, err := BuildWindowsSandboxSetupArgs(WindowsSandboxSetupArgsOptions{ + SandboxHome: home, + CommandCWD: `C:\workspace\src`, + WorkspaceRoots: []string{`C:\workspace`}, + PermissionProfile: PermissionProfile{ + FileSystem: FileSystemPolicy{ + Kind: FileSystemRestricted, + WriteRoots: []WritableRoot{{Root: `C:\workspace`}}, + DenyRead: []string{`C:\workspace\secret`}, + }, + Network: NetworkPolicy{Mode: NetworkDeny}, + }, + }) + if err != nil { + t.Fatalf("BuildWindowsSandboxSetupArgs: %v", err) + } + var stderr bytes.Buffer + code := RunWindowsSandboxSetup(args, &stderr) + if code != 1 { + t.Fatalf("exit code = %d, want 1 for unsupported DenyRead", code) + } + if !strings.Contains(stderr.String(), "DenyRead is not supported") { + t.Fatalf("stderr = %q, want DenyRead unsupported rejection before elevation check", stderr.String()) + } + if strings.Contains(stderr.String(), "Administrator rights are required") { + t.Fatalf("stderr = %q, should not claim Administrator rights when DenyRead is unsupported", stderr.String()) + } +} + func TestWindowsSandboxSetupMarkerRefreshesWhenProfileChanges(t *testing.T) { config := WindowsSandboxSetupConfig{ SandboxHome: t.TempDir(), @@ -126,8 +157,8 @@ func TestWindowsSandboxSetupMarkerValidatesBothNetworkModes(t *testing.T) { } } -// A pre-v4 marker on disk must be rejected as out of date so the schema bump -// forces a clean re-setup (old markers scoped the filter to write SIDs). +// A pre-v5 marker on disk must be rejected as out of date so the schema bump +// forces a clean re-setup (old markers had legacy DenyWrite ACEs). func TestWindowsSandboxSetupMarkerRejectsOldSchema(t *testing.T) { config := WindowsSandboxSetupConfig{ SandboxHome: t.TempDir(), @@ -142,7 +173,7 @@ func TestWindowsSandboxSetupMarkerRejectsOldSchema(t *testing.T) { if err != nil { t.Fatalf("BuildWindowsSandboxSetupMarker: %v", err) } - marker.SchemaVersion = 3 + marker.SchemaVersion = 4 bytes, err := json.Marshal(marker) if err != nil { t.Fatalf("marshal: %v", err) @@ -152,7 +183,7 @@ func TestWindowsSandboxSetupMarkerRejectsOldSchema(t *testing.T) { } err = ValidateWindowsSandboxSetupMarker(config) if err == nil || !strings.Contains(err.Error(), "out of date") { - t.Fatalf("schema-3 marker must be out of date, got: %v", err) + t.Fatalf("schema-4 marker must be out of date, got: %v", err) } } diff --git a/internal/sandbox/windows_setup_windows.go b/internal/sandbox/windows_setup_windows.go index 888355397..a71f78cae 100644 --- a/internal/sandbox/windows_setup_windows.go +++ b/internal/sandbox/windows_setup_windows.go @@ -10,6 +10,12 @@ import ( ) func runWindowsSandboxSetup(config WindowsSandboxSetupConfig, stderr io.Writer) int { + // Do not provision DenyRead ACLs for a token mode that cannot launch normal + // tools with DenyRead under the narrow restricting-SID set (PR #640). + if err := windowsDenyReadRestrictedTokenUnsupportedProfile(config.PermissionProfile); err != nil { + fmt.Fprintln(stderr, WindowsSandboxSetupName+": "+err.Error()) + return 1 + } // Applying the WFP network filters and workspace ACLs requires Administrator // rights; without them WFP fails deep inside with a raw ACCESS_DENIED (0x5). // Check up front and return an actionable message instead. diff --git a/internal/sandbox/windows_unelevated.go b/internal/sandbox/windows_unelevated.go index 980664d6a..b7cb05f53 100644 --- a/internal/sandbox/windows_unelevated.go +++ b/internal/sandbox/windows_unelevated.go @@ -11,7 +11,7 @@ import ( "github.com/Gitlawb/zero/internal/fsutil" ) -const windowsUnelevatedSetupMarkerSchemaVersion = 1 +const windowsUnelevatedSetupMarkerSchemaVersion = 2 // windowsUnelevatedSetupMarkerMaxPlans bounds the applied-plan history so the // marker cannot grow without limit when a user hops between many workspaces.