From 050970d923a9117c70f1b51c9947b64fde2a36a9 Mon Sep 17 00:00:00 2001 From: euxaristia <25621994+euxaristia@users.noreply.github.com> Date: Fri, 10 Jul 2026 10:42:50 -0400 Subject: [PATCH 1/7] fix(windows): add Users and Authenticated Users SIDs to restricted token SIDs --- internal/sandbox/windows_token_windows.go | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/internal/sandbox/windows_token_windows.go b/internal/sandbox/windows_token_windows.go index 5c5498e7..6807147d 100644 --- a/internal/sandbox/windows_token_windows.go +++ b/internal/sandbox/windows_token_windows.go @@ -92,14 +92,24 @@ func createWindowsRestrictedTokenFromBase(base windows.Token, capabilitySIDs []w if err != nil { return 0, fmt.Errorf("create world SID: %w", err) } + 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 := 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}) } entries = append(entries, windows.SIDAndAttributes{Sid: sidFromBytes(logonSID)}, windows.SIDAndAttributes{Sid: worldSID}, + windows.SIDAndAttributes{Sid: usersSID}, + windows.SIDAndAttributes{Sid: authUserSID}, ) var restricted windows.Token From 5669652ecedf06d0f0b32022cc0b7e84850bc832 Mon Sep 17 00:00:00 2001 From: euxaristia <25621994+euxaristia@users.noreply.github.com> Date: Sat, 11 Jul 2026 13:43:36 -0400 Subject: [PATCH 2/7] fix(sandbox): add DenyWrite ACEs for system drive, ProgramData, and Windows Temp --- .../runner_windows_integration_test.go | 16 ++++ internal/sandbox/windows_acl.go | 74 +++++++++++++++++++ internal/sandbox/windows_acl_apply_windows.go | 8 +- internal/sandbox/windows_acl_test.go | 52 ++++++++++++- 4 files changed, 142 insertions(+), 8 deletions(-) diff --git a/internal/sandbox/runner_windows_integration_test.go b/internal/sandbox/runner_windows_integration_test.go index 708f43e4..f3767e39 100644 --- a/internal/sandbox/runner_windows_integration_test.go +++ b/internal/sandbox/runner_windows_integration_test.go @@ -203,6 +203,22 @@ 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") + } + } } // TestWindowsRestrictedTokenNestedPipeCapture pins the fix in diff --git a/internal/sandbox/windows_acl.go b/internal/sandbox/windows_acl.go index 55d37d34..8778f285 100644 --- a/internal/sandbox/windows_acl.go +++ b/internal/sandbox/windows_acl.go @@ -2,6 +2,7 @@ package sandbox import ( "errors" + "os" "path/filepath" "strings" ) @@ -19,6 +20,7 @@ type WindowsACLEntry struct { Path string `json:"path"` Capability string `json:"capability"` Materialize bool `json:"materialize,omitempty"` + NoInherit bool `json:"no_inherit,omitempty"` } type WindowsACLPlan struct { @@ -76,9 +78,81 @@ func BuildWindowsACLPlan(config WindowsSandboxCommandConfig) (WindowsACLPlan, er }) } } + + // Deny write to shared Windows-writable directories (C:\, C:\ProgramData, C:\Windows\Temp) + // to prevent write-jail escape via the added Users and Authenticated Users SIDs. + 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` + } + + sharedDenyPaths := []string{ + systemDrive + `\`, + programData, + systemRoot + `\Temp`, + } + + caps, err := LoadOrCreateWindowsCapabilitySIDs(config.SandboxHome) + if err != nil { + return WindowsACLPlan{}, err + } + var allSIDs []string + for _, cap := range writeCapabilities { + allSIDs = append(allSIDs, cap.SID) + } + allSIDs = append(allSIDs, caps.ReadOnly) + + for _, denyPath := range sharedDenyPaths { + isParent := false + isEqual := false + for _, cap := range writeCapabilities { + if isParentOrEqual(denyPath, cap.Root) { + if windowsCapabilityPathKey(denyPath) == windowsCapabilityPathKey(cap.Root) { + isEqual = true + } else { + isParent = true + } + } + } + if isEqual { + continue // Do not deny write if it is exactly an allowed write root + } + for _, sid := range allSIDs { + entries = append(entries, WindowsACLEntry{ + Action: WindowsACLDenyWrite, + Path: denyPath, + Capability: sid, + NoInherit: isParent, // Disable inheritance if a write root exists inside this path + }) + } + } + return WindowsACLPlan{Entries: dedupeWindowsACLEntries(entries)}, nil } +func isParentOrEqual(parent, child string) bool { + p := windowsCapabilityPathKey(parent) + c := windowsCapabilityPathKey(child) + if p == "" || c == "" { + return false + } + if p == c { + return true + } + if !strings.HasSuffix(p, `\`) { + p += `\` + } + return strings.HasPrefix(c, p) +} + type windowsWriteRootCapability struct { Root string SID string diff --git a/internal/sandbox/windows_acl_apply_windows.go b/internal/sandbox/windows_acl_apply_windows.go index 28f852b6..b3ca49a4 100644 --- a/internal/sandbox/windows_acl_apply_windows.go +++ b/internal/sandbox/windows_acl_apply_windows.go @@ -146,10 +146,6 @@ func windowsACLGroupRequiresExistingTarget(group windowsACLPathGroup) bool { 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 - } for _, entry := range entries { sid, err := windows.StringToSid(entry.Capability) if err != nil { @@ -159,6 +155,10 @@ func windowsExplicitAccessEntries(entries []WindowsACLEntry, isDir bool) ([]wind 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, diff --git a/internal/sandbox/windows_acl_test.go b/internal/sandbox/windows_acl_test.go index 1925bd8a..13e78b4d 100644 --- a/internal/sandbox/windows_acl_test.go +++ b/internal/sandbox/windows_acl_test.go @@ -52,6 +52,29 @@ 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) + + caps, err := LoadOrCreateWindowsCapabilitySIDs(home) + if err != nil { + t.Fatalf("LoadOrCreateWindowsCapabilitySIDs: %v", err) + } + 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` + } + + for _, sid := range []string{workspaceSID, cacheSID, caps.ReadOnly} { + assertWindowsACLEntryExt(t, plan, WindowsACLDenyWrite, systemDrive+`\`, sid, false, true) + assertWindowsACLEntryExt(t, plan, WindowsACLDenyWrite, programData, sid, false, false) + assertWindowsACLEntryExt(t, plan, WindowsACLDenyWrite, systemRoot+`\Temp`, sid, false, false) + } } func TestBuildWindowsACLPlanUsesReadOnlySIDWithoutWriteRoots(t *testing.T) { @@ -73,10 +96,25 @@ 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) + if len(plan.Entries) != 4 { + t.Fatalf("ACL entries = %#v, want four entries (1 deny-read, 3 deny-write)", plan.Entries) } assertWindowsACLEntry(t, plan, WindowsACLDenyRead, `C:\workspace\secret-read`, caps.ReadOnly, true) + 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` + } + assertWindowsACLEntryExt(t, plan, WindowsACLDenyWrite, systemDrive+`\`, caps.ReadOnly, false, false) + assertWindowsACLEntryExt(t, plan, WindowsACLDenyWrite, programData, caps.ReadOnly, false, false) + assertWindowsACLEntryExt(t, plan, WindowsACLDenyWrite, systemRoot+`\Temp`, caps.ReadOnly, false, false) } func TestBuildWindowsACLPlanRejectsUnrestrictedProfiles(t *testing.T) { @@ -117,16 +155,22 @@ func TestPlanWindowsDenyReadPathsIncludesCanonicalExistingPath(t *testing.T) { } func assertWindowsACLEntry(t *testing.T, plan WindowsACLPlan, action WindowsACLAction, path string, capability string, materialize bool) { + t.Helper() + assertWindowsACLEntryExt(t, plan, action, path, capability, materialize, false) +} + +func assertWindowsACLEntryExt(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 { From 9eaba2b57bf0caf36874fa1a42d22a1bef811bfd Mon Sep 17 00:00:00 2001 From: euxaristia <25621994+euxaristia@users.noreply.github.com> Date: Mon, 13 Jul 2026 00:56:20 -0400 Subject: [PATCH 3/7] fix(sandbox): scope broadened restricted-token SIDs to the elevated tier Only the elevated restricted-token tier (zero sandbox setup, run as Administrator) now gets WinBuiltinUsersSid/WinAuthenticatedUserSid on the restricted token. That tier is also the only one with the WRITE_DAC needed to enforce BuildWindowsACLPlan's DenyWrite mitigation on shared system paths, so the unelevated tier keeps the narrower pre-widening SID set instead of aborting every command with access denied when it can't edit C:\, ProgramData, or Windows\Temp. Also add C:\Users\Public as an explicit DenyWrite target: inheriting a deny from C:\ never actually protected pre-existing children like it, since NTFS does not retroactively propagate inherited ACEs onto objects that already exist. Drop the NoInherit toggle that tried to route around this by disabling inheritance whenever a write root sat under C:\; it wasn't needed, since a write root's own explicit Allow ACE already takes precedence over anything inherited by canonical ACE ordering. --- .../runner_windows_integration_test.go | 23 ++++ internal/sandbox/windows_acl.go | 129 ++++++++++-------- internal/sandbox/windows_acl_apply_windows.go | 2 +- internal/sandbox/windows_acl_test.go | 100 +++++++++----- .../sandbox/windows_command_runner_windows.go | 6 +- internal/sandbox/windows_token_windows.go | 43 ++++-- 6 files changed, 194 insertions(+), 109 deletions(-) diff --git a/internal/sandbox/runner_windows_integration_test.go b/internal/sandbox/runner_windows_integration_test.go index f3767e39..da634e12 100644 --- a/internal/sandbox/runner_windows_integration_test.go +++ b/internal/sandbox/runner_windows_integration_test.go @@ -72,6 +72,29 @@ func TestWindowsRestrictedTokenRealSandboxSmoke(t *testing.T) { t.Fatalf("sandboxed write marker = %q, %v; want ok", bytes, err) } + // The elevated tier's restricted token carries the Users/Authenticated + // Users SIDs (added for Program Files/System32 reads), which also match + // the write grant those groups already hold on C:\Users\Public. Pin that + // BuildWindowsACLPlan's DenyWrite mitigation still blocks a write there: + // an independent shared-writable directory outside every carved-out + // system path (ProgramData, Windows\Temp), and outside every workspace + // write root. + publicDir := os.Getenv("PUBLIC") + if publicDir == "" { + t.Skip("PUBLIC is not set; cannot probe C:\\Users\\Public write jail") + } + 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) diff --git a/internal/sandbox/windows_acl.go b/internal/sandbox/windows_acl.go index 8778f285..2f623d18 100644 --- a/internal/sandbox/windows_acl.go +++ b/internal/sandbox/windows_acl.go @@ -20,7 +20,6 @@ type WindowsACLEntry struct { Path string `json:"path"` Capability string `json:"capability"` Materialize bool `json:"materialize,omitempty"` - NoInherit bool `json:"no_inherit,omitempty"` } type WindowsACLPlan struct { @@ -79,78 +78,88 @@ func BuildWindowsACLPlan(config WindowsSandboxCommandConfig) (WindowsACLPlan, er } } - // Deny write to shared Windows-writable directories (C:\, C:\ProgramData, C:\Windows\Temp) - // to prevent write-jail escape via the added Users and Authenticated Users SIDs. - 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` - } - - sharedDenyPaths := []string{ - systemDrive + `\`, - programData, - systemRoot + `\Temp`, - } + // Deny write to shared Windows-writable directories (C:\, C:\ProgramData, + // C:\Windows\Temp, C:\Users\Public) to prevent write-jail escape via the + // added Users and Authenticated Users SIDs. Only the elevated tier + // (WindowsSandboxLevelRestrictedToken, applied by `zero sandbox setup` + // running as Administrator) reaches here with those SIDs on the token in + // the first place — see createWindowsRestrictedTokenFromBase — and only + // that tier has the WRITE_DAC needed to edit these system-owned DACLs. + // The unelevated tier keeps the narrower (pre-widening) restricting-SID + // set and never needs these entries. + if config.SandboxLevel == WindowsSandboxLevelRestrictedToken { + 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` + } - caps, err := LoadOrCreateWindowsCapabilitySIDs(config.SandboxHome) - if err != nil { - return WindowsACLPlan{}, err - } - var allSIDs []string - for _, cap := range writeCapabilities { - allSIDs = append(allSIDs, cap.SID) - } - allSIDs = append(allSIDs, caps.ReadOnly) + sharedDenyPaths := []string{ + systemDrive + `\`, + programData, + systemRoot + `\Temp`, + publicDir, + } - for _, denyPath := range sharedDenyPaths { - isParent := false - isEqual := false - for _, cap := range writeCapabilities { - if isParentOrEqual(denyPath, cap.Root) { - if windowsCapabilityPathKey(denyPath) == windowsCapabilityPathKey(cap.Root) { - isEqual = true - } else { - isParent = true - } - } + caps, err := LoadOrCreateWindowsCapabilitySIDs(config.SandboxHome) + if err != nil { + return WindowsACLPlan{}, err } - if isEqual { - continue // Do not deny write if it is exactly an allowed write root + var allSIDs []string + for _, cap := range writeCapabilities { + allSIDs = append(allSIDs, cap.SID) } - for _, sid := range allSIDs { - entries = append(entries, WindowsACLEntry{ - Action: WindowsACLDenyWrite, - Path: denyPath, - Capability: sid, - NoInherit: isParent, // Disable inheritance if a write root exists inside this path - }) + allSIDs = append(allSIDs, caps.ReadOnly) + + for _, denyPath := range sharedDenyPaths { + if windowsPathEqualsAnyRoot(denyPath, writeCapabilities) { + continue // Do not deny write if it is exactly an allowed write root + } + // Inheritance is intentionally left on: the write root's own + // explicit Allow ACE (set directly on that path in its own group, + // above) is a non-inherited entry, and canonical ACE ordering + // always evaluates explicit entries before inherited ones — so an + // inherited Deny from a shared ancestor here can never shadow it. + // It also still defends newly created objects elsewhere under the + // shared path: NTFS does not retroactively propagate an + // inheritable ACE onto pre-existing children, which is exactly + // why C:\Users\Public above is listed explicitly rather than + // relied on via inheritance from C:\. + for _, sid := range allSIDs { + entries = append(entries, WindowsACLEntry{ + Action: WindowsACLDenyWrite, + Path: denyPath, + Capability: sid, + }) + } } } return WindowsACLPlan{Entries: dedupeWindowsACLEntries(entries)}, nil } -func isParentOrEqual(parent, child string) bool { - p := windowsCapabilityPathKey(parent) - c := windowsCapabilityPathKey(child) - if p == "" || c == "" { +func windowsPathEqualsAnyRoot(path string, capabilities []windowsWriteRootCapability) bool { + key := windowsCapabilityPathKey(path) + if key == "" { return false } - if p == c { - return true - } - if !strings.HasSuffix(p, `\`) { - p += `\` + for _, cap := range capabilities { + if windowsCapabilityPathKey(cap.Root) == key { + return true + } } - return strings.HasPrefix(c, p) + return false } type windowsWriteRootCapability struct { diff --git a/internal/sandbox/windows_acl_apply_windows.go b/internal/sandbox/windows_acl_apply_windows.go index b3ca49a4..2fe0301b 100644 --- a/internal/sandbox/windows_acl_apply_windows.go +++ b/internal/sandbox/windows_acl_apply_windows.go @@ -156,7 +156,7 @@ func windowsExplicitAccessEntries(entries []WindowsACLEntry, isDir bool) ([]wind return nil, err } inheritance := uint32(0) - if isDir && !entry.NoInherit { + if isDir { inheritance = windows.SUB_CONTAINERS_AND_OBJECTS_INHERIT } out = append(out, windows.EXPLICIT_ACCESS{ diff --git a/internal/sandbox/windows_acl_test.go b/internal/sandbox/windows_acl_test.go index 13e78b4d..4313be3c 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, @@ -57,24 +58,70 @@ func TestBuildWindowsACLPlanForWorkspaceWriteProfile(t *testing.T) { if err != nil { t.Fatalf("LoadOrCreateWindowsCapabilitySIDs: %v", err) } - systemDrive := os.Getenv("SystemDrive") + systemDrive, systemRoot, programData, publicDir := windowsSharedDenyPathsForTest() + + for _, sid := range []string{workspaceSID, cacheSID, caps.ReadOnly} { + assertWindowsACLEntry(t, plan, WindowsACLDenyWrite, systemDrive+`\`, sid, false) + assertWindowsACLEntry(t, plan, WindowsACLDenyWrite, programData, sid, false) + assertWindowsACLEntry(t, plan, WindowsACLDenyWrite, systemRoot+`\Temp`, sid, false) + assertWindowsACLEntry(t, plan, WindowsACLDenyWrite, publicDir, sid, false) + } +} + +// TestBuildWindowsACLPlanOmitsSharedDenyPathsWhenUnelevated pins the fix for +// the unelevated tier aborting every sandboxed command: BuildWindowsACLPlan +// must not add DenyWrite entries for C:\, C:\ProgramData, C:\Windows\Temp, or +// C:\Users\Public when SandboxLevel is WindowsSandboxLevelUnelevated, because +// SetNamedSecurityInfo on those system-owned paths requires WRITE_DAC that an +// ordinary (non-Administrator) user does not have. The unelevated tier never +// puts the Users/Authenticated Users SIDs on the token in the first place +// (see createWindowsRestrictedTokenFromBase), so it does not need these +// mitigating entries. +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`}}, + }, + Network: NetworkPolicy{Mode: NetworkDeny}, + }, + }) + if err != nil { + t.Fatalf("BuildWindowsACLPlan: %v", err) + } + systemDrive, systemRoot, programData, publicDir := windowsSharedDenyPathsForTest() + 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("unelevated ACL plan = %#v, want no DenyWrite entry for shared path %q", plan.Entries, path) + } + } + } +} + +func windowsSharedDenyPathsForTest() (systemDrive, systemRoot, programData, publicDir string) { + systemDrive = os.Getenv("SystemDrive") if systemDrive == "" { systemDrive = "C:" } - systemRoot := os.Getenv("SystemRoot") + systemRoot = os.Getenv("SystemRoot") if systemRoot == "" { systemRoot = systemDrive + `\Windows` } - programData := os.Getenv("ProgramData") + programData = os.Getenv("ProgramData") if programData == "" { programData = systemDrive + `\ProgramData` } - - for _, sid := range []string{workspaceSID, cacheSID, caps.ReadOnly} { - assertWindowsACLEntryExt(t, plan, WindowsACLDenyWrite, systemDrive+`\`, sid, false, true) - assertWindowsACLEntryExt(t, plan, WindowsACLDenyWrite, programData, sid, false, false) - assertWindowsACLEntryExt(t, plan, WindowsACLDenyWrite, systemRoot+`\Temp`, sid, false, false) + publicDir = os.Getenv("PUBLIC") + if publicDir == "" { + publicDir = systemDrive + `\Users\Public` } + return systemDrive, systemRoot, programData, publicDir } func TestBuildWindowsACLPlanUsesReadOnlySIDWithoutWriteRoots(t *testing.T) { @@ -84,7 +131,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, @@ -96,25 +144,15 @@ func TestBuildWindowsACLPlanUsesReadOnlySIDWithoutWriteRoots(t *testing.T) { if err != nil { t.Fatalf("BuildWindowsACLPlan: %v", err) } - if len(plan.Entries) != 4 { - t.Fatalf("ACL entries = %#v, want four entries (1 deny-read, 3 deny-write)", plan.Entries) + if len(plan.Entries) != 5 { + t.Fatalf("ACL entries = %#v, want five entries (1 deny-read, 4 deny-write)", plan.Entries) } assertWindowsACLEntry(t, plan, WindowsACLDenyRead, `C:\workspace\secret-read`, caps.ReadOnly, true) - 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` - } - assertWindowsACLEntryExt(t, plan, WindowsACLDenyWrite, systemDrive+`\`, caps.ReadOnly, false, false) - assertWindowsACLEntryExt(t, plan, WindowsACLDenyWrite, programData, caps.ReadOnly, false, false) - assertWindowsACLEntryExt(t, plan, WindowsACLDenyWrite, systemRoot+`\Temp`, caps.ReadOnly, false, false) + systemDrive, systemRoot, programData, publicDir := windowsSharedDenyPathsForTest() + assertWindowsACLEntry(t, plan, WindowsACLDenyWrite, systemDrive+`\`, caps.ReadOnly, false) + assertWindowsACLEntry(t, plan, WindowsACLDenyWrite, programData, caps.ReadOnly, false) + assertWindowsACLEntry(t, plan, WindowsACLDenyWrite, systemRoot+`\Temp`, caps.ReadOnly, false) + assertWindowsACLEntry(t, plan, WindowsACLDenyWrite, publicDir, caps.ReadOnly, false) } func TestBuildWindowsACLPlanRejectsUnrestrictedProfiles(t *testing.T) { @@ -155,22 +193,16 @@ func TestPlanWindowsDenyReadPathsIncludesCanonicalExistingPath(t *testing.T) { } func assertWindowsACLEntry(t *testing.T, plan WindowsACLPlan, action WindowsACLAction, path string, capability string, materialize bool) { - t.Helper() - assertWindowsACLEntryExt(t, plan, action, path, capability, materialize, false) -} - -func assertWindowsACLEntryExt(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.NoInherit == noInherit { + entry.Materialize == materialize { return } } - t.Fatalf("ACL entries = %#v, want %s %q capability %q materialize=%v noInherit=%v", plan.Entries, action, path, capability, materialize, noInherit) + t.Fatalf("ACL entries = %#v, want %s %q capability %q materialize=%v", plan.Entries, action, path, capability, materialize) } func windowsPathListContains(paths []string, want string) bool { diff --git a/internal/sandbox/windows_command_runner_windows.go b/internal/sandbox/windows_command_runner_windows.go index 51618784..c4483a61 100644 --- a/internal/sandbox/windows_command_runner_windows.go +++ b/internal/sandbox/windows_command_runner_windows.go @@ -69,7 +69,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) - token, err := createWindowsRestrictedTokenForCapabilitySIDs(tokenSIDs) + // Only the elevated restricted-token tier can enforce the DenyWrite + // mitigation BuildWindowsACLPlan adds for the broadened read SIDs (it + // requires Administrator rights); see createWindowsRestrictedTokenFromBase. + broadenReadSIDs := config.SandboxLevel == WindowsSandboxLevelRestrictedToken + token, err := createWindowsRestrictedTokenForCapabilitySIDs(tokenSIDs, broadenReadSIDs) if err != nil { fmt.Fprintln(stderr, WindowsSandboxCommandRunnerName+": "+err.Error()) return 1 diff --git a/internal/sandbox/windows_token_windows.go b/internal/sandbox/windows_token_windows.go index 6807147d..c996415d 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) (windows.Token, error) { +func createWindowsRestrictedTokenForCapabilitySIDs(capabilitySIDStrings []string, 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,23 @@ func createWindowsRestrictedTokenForCapabilitySIDs(capabilitySIDStrings []string return 0, fmt.Errorf("open process token: %w", err) } defer base.Close() - return createWindowsRestrictedTokenFromBase(base, capabilitySIDs) + return createWindowsRestrictedTokenFromBase(base, capabilitySIDs, broadenReadSIDs) } -func createWindowsRestrictedTokenFromBase(base windows.Token, capabilitySIDs []windowsLocalSID) (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. Because the restricting-SID +// check applies to writes as well as reads, this also grants write wherever +// those groups already have it — BuildWindowsACLPlan mitigates that by adding +// DenyWrite ACEs to the known shared Users/Authenticated-Users-writable +// directories, but it can only do so with Administrator rights (see +// WindowsSandboxLevelRestrictedToken). broadenReadSIDs must therefore stay +// false for WindowsSandboxLevelUnelevated, which cannot enforce that +// mitigation: it keeps the original (narrower) read scope instead of +// widening the write jail with nothing to close the gap. +func createWindowsRestrictedTokenFromBase(base windows.Token, capabilitySIDs []windowsLocalSID, broadenReadSIDs bool) (windows.Token, error) { logonSID, err := copyWindowsLogonSID(base) if err != nil { return 0, err @@ -92,14 +105,6 @@ func createWindowsRestrictedTokenFromBase(base windows.Token, capabilitySIDs []w if err != nil { return 0, fmt.Errorf("create world SID: %w", err) } - 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 := make([]windows.SIDAndAttributes, 0, len(capabilitySIDs)+4) for _, sid := range capabilitySIDs { @@ -108,9 +113,21 @@ func createWindowsRestrictedTokenFromBase(base windows.Token, capabilitySIDs []w entries = append(entries, windows.SIDAndAttributes{Sid: sidFromBytes(logonSID)}, windows.SIDAndAttributes{Sid: worldSID}, - windows.SIDAndAttributes{Sid: usersSID}, - windows.SIDAndAttributes{Sid: authUserSID}, ) + 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}, + ) + } var restricted windows.Token result, _, callErr := procCreateRestrictedToken.Call( From afa2ffa52fcab4e11b52c9444d667ab989dc2427 Mon Sep 17 00:00:00 2001 From: euxaristia <25621994+euxaristia@users.noreply.github.com> Date: Mon, 13 Jul 2026 01:19:00 -0400 Subject: [PATCH 4/7] fix(sandbox): fail loudly on unexpected ProgramData marker stat errors Align the ProgramData marker check with its outsideMarker and publicMarker siblings, which already report a fatal error if Stat fails for a reason other than the file not existing. --- internal/sandbox/runner_windows_integration_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/internal/sandbox/runner_windows_integration_test.go b/internal/sandbox/runner_windows_integration_test.go index da634e12..f2752a55 100644 --- a/internal/sandbox/runner_windows_integration_test.go +++ b/internal/sandbox/runner_windows_integration_test.go @@ -240,6 +240,8 @@ func TestWindowsUnelevatedRealSandboxSmoke(t *testing.T) { 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) } } } From 3597b620fc53903e0c0f3f7673b99d77fff40fb1 Mon Sep 17 00:00:00 2001 From: euxaristia <25621994+euxaristia@users.noreply.github.com> Date: Mon, 13 Jul 2026 23:13:36 -0400 Subject: [PATCH 5/7] fix(windows): resolve deny-paths from trusted APIs and stop ACL inheritance propagation Resolve the shared DenyWrite target paths (system drive, ProgramData, Windows\Temp, Users\Public) via GetSystemWindowsDirectory and SHGetKnownFolderPath instead of trusting the SystemDrive/SystemRoot/ ProgramData/PUBLIC environment variables, which an attacker able to influence the elevated setup process's environment could spoof to leave the real system paths unprotected. Also stop marking those four DenyWrite entries as inheritable. SetNamedSecurityInfo automatically propagates inheritable ACEs onto a target's existing descendants, so the previous entries recursively stamped a deny ACE across the entire existing subtree of the system drive rather than just the four intended directories, which was slow, polluted unrelated machine ACLs, and could shadow legitimate workspace allows for repos under the system drive. A plain, non-inherited deny directly on each of the four paths already blocks writes (including new children) at that path without touching any descendant's ACL. --- internal/sandbox/windows_acl.go | 67 ++++++++++--------- internal/sandbox/windows_acl_apply_windows.go | 2 +- internal/sandbox/windows_acl_paths_other.go | 34 ++++++++++ internal/sandbox/windows_acl_paths_windows.go | 44 ++++++++++++ internal/sandbox/windows_acl_test.go | 57 ++++++++-------- 5 files changed, 144 insertions(+), 60 deletions(-) create mode 100644 internal/sandbox/windows_acl_paths_other.go create mode 100644 internal/sandbox/windows_acl_paths_windows.go diff --git a/internal/sandbox/windows_acl.go b/internal/sandbox/windows_acl.go index 2f623d18..3a82999e 100644 --- a/internal/sandbox/windows_acl.go +++ b/internal/sandbox/windows_acl.go @@ -2,7 +2,7 @@ package sandbox import ( "errors" - "os" + "fmt" "path/filepath" "strings" ) @@ -16,10 +16,18 @@ const ( ) 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"` } type WindowsACLPlan struct { @@ -88,21 +96,13 @@ func BuildWindowsACLPlan(config WindowsSandboxCommandConfig) (WindowsACLPlan, er // The unelevated tier keeps the narrower (pre-widening) restricting-SID // set and never needs these entries. if config.SandboxLevel == WindowsSandboxLevelRestrictedToken { - 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` + // Resolved from trusted Win32 APIs, not from the + // SystemDrive/SystemRoot/ProgramData/PUBLIC environment variables: + // see resolveWindowsSharedDenyPaths for why trusting the environment + // here would be a spoofable security boundary. + systemDrive, systemRoot, programData, publicDir, err := resolveWindowsSharedDenyPaths() + if err != nil { + return WindowsACLPlan{}, fmt.Errorf("resolve shared deny paths: %w", err) } sharedDenyPaths := []string{ @@ -126,21 +126,28 @@ func BuildWindowsACLPlan(config WindowsSandboxCommandConfig) (WindowsACLPlan, er if windowsPathEqualsAnyRoot(denyPath, writeCapabilities) { continue // Do not deny write if it is exactly an allowed write root } - // Inheritance is intentionally left on: the write root's own - // explicit Allow ACE (set directly on that path in its own group, - // above) is a non-inherited entry, and canonical ACE ordering - // always evaluates explicit entries before inherited ones — so an - // inherited Deny from a shared ancestor here can never shadow it. - // It also still defends newly created objects elsewhere under the - // shared path: NTFS does not retroactively propagate an - // inheritable ACE onto pre-existing children, which is exactly - // why C:\Users\Public above is listed explicitly rather than - // relied on via inheritance from C:\. + // NoInherit: these four shared paths must NOT carry an inheritable + // ACE. SetNamedSecurityInfo automatically propagates any + // inheritable ACE down onto the target's EXISTING descendants + // (per Microsoft's documented remarks for SetNamedSecurityInfoW), + // not just ones created afterward. C:\ in particular can have an + // enormous, slow-to-walk, and largely unrelated existing subtree + // (Program Files, Users, arbitrary installed software), and + // stamping a synthetic deny ACE onto all of it would also + // permanently pollute those machine ACLs and could shadow + // legitimate workspace Allow entries for repos that happen to + // live under the system drive. Each of these four paths is + // listed explicitly (rather than relied on via inheritance from + // C:\) precisely so a plain, non-inherited Deny placed directly + // on each one is sufficient: it blocks the denied SIDs from + // writing (including creating new children) directly under that + // path without ever touching any descendant's own ACL. for _, sid := range allSIDs { entries = append(entries, WindowsACLEntry{ Action: WindowsACLDenyWrite, Path: denyPath, Capability: sid, + NoInherit: true, }) } } diff --git a/internal/sandbox/windows_acl_apply_windows.go b/internal/sandbox/windows_acl_apply_windows.go index 2fe0301b..b3ca49a4 100644 --- a/internal/sandbox/windows_acl_apply_windows.go +++ b/internal/sandbox/windows_acl_apply_windows.go @@ -156,7 +156,7 @@ func windowsExplicitAccessEntries(entries []WindowsACLEntry, isDir bool) ([]wind return nil, err } inheritance := uint32(0) - if isDir { + if isDir && !entry.NoInherit { inheritance = windows.SUB_CONTAINERS_AND_OBJECTS_INHERIT } out = append(out, windows.EXPLICIT_ACCESS{ diff --git a/internal/sandbox/windows_acl_paths_other.go b/internal/sandbox/windows_acl_paths_other.go new file mode 100644 index 00000000..f3de23b0 --- /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 00000000..ee19762b --- /dev/null +++ b/internal/sandbox/windows_acl_paths_windows.go @@ -0,0 +1,44 @@ +//go:build windows + +package sandbox + +import ( + "fmt" + "path/filepath" + + "golang.org/x/sys/windows" +) + +// resolveWindowsSharedDenyPaths resolves the canonical system paths that +// BuildWindowsACLPlan protects with shared DenyWrite entries (the system +// drive root, %SystemRoot%\Temp, ProgramData, and the Public user profile). +// +// These are resolved from trusted Win32 APIs (GetSystemWindowsDirectory, +// SHGetKnownFolderPath) rather than the SystemDrive/SystemRoot/ProgramData/ +// PUBLIC environment variables. Those variables are ordinary process +// environment state: anything able to influence the environment of the +// elevated `zero sandbox setup` process (which builds and applies this ACL +// plan) could spoof them to point the DenyWrite mitigation at the wrong +// paths, leaving the real system directories unprotected while the +// restricted token is still broadened with the Users and Authenticated +// Users SIDs (see createWindowsRestrictedTokenFromBase). The Win32 APIs used +// here are answered by the OS from its own configuration, not from the +// caller's environment block, so they are not spoofable the same way. +func resolveWindowsSharedDenyPaths() (systemDrive, systemRoot, programData, publicDir string, err error) { + windowsDir, err := windows.GetSystemWindowsDirectory() + if err != nil { + return "", "", "", "", fmt.Errorf("resolve system windows directory: %w", err) + } + systemRoot = filepath.Clean(windowsDir) + systemDrive = filepath.VolumeName(systemRoot) + if systemDrive == "" { + return "", "", "", "", fmt.Errorf("resolve system drive from windows directory %q", systemRoot) + } + if programData, err = windows.KnownFolderPath(windows.FOLDERID_ProgramData, 0); err != nil { + return "", "", "", "", fmt.Errorf("resolve ProgramData known folder: %w", err) + } + if publicDir, err = windows.KnownFolderPath(windows.FOLDERID_Public, 0); err != nil { + return "", "", "", "", fmt.Errorf("resolve Public known folder: %w", err) + } + return systemDrive, systemRoot, programData, publicDir, nil +} diff --git a/internal/sandbox/windows_acl_test.go b/internal/sandbox/windows_acl_test.go index 4313be3c..2d2e2ae9 100644 --- a/internal/sandbox/windows_acl_test.go +++ b/internal/sandbox/windows_acl_test.go @@ -58,13 +58,13 @@ func TestBuildWindowsACLPlanForWorkspaceWriteProfile(t *testing.T) { if err != nil { t.Fatalf("LoadOrCreateWindowsCapabilitySIDs: %v", err) } - systemDrive, systemRoot, programData, publicDir := windowsSharedDenyPathsForTest() + systemDrive, systemRoot, programData, publicDir := windowsSharedDenyPathsForTest(t) for _, sid := range []string{workspaceSID, cacheSID, caps.ReadOnly} { - assertWindowsACLEntry(t, plan, WindowsACLDenyWrite, systemDrive+`\`, sid, false) - assertWindowsACLEntry(t, plan, WindowsACLDenyWrite, programData, sid, false) - assertWindowsACLEntry(t, plan, WindowsACLDenyWrite, systemRoot+`\Temp`, sid, false) - assertWindowsACLEntry(t, plan, WindowsACLDenyWrite, publicDir, sid, false) + assertWindowsACLEntryInheritance(t, plan, WindowsACLDenyWrite, systemDrive+`\`, sid, false, true) + assertWindowsACLEntryInheritance(t, plan, WindowsACLDenyWrite, programData, sid, false, true) + assertWindowsACLEntryInheritance(t, plan, WindowsACLDenyWrite, systemRoot+`\Temp`, sid, false, true) + assertWindowsACLEntryInheritance(t, plan, WindowsACLDenyWrite, publicDir, sid, false, true) } } @@ -94,7 +94,7 @@ func TestBuildWindowsACLPlanOmitsSharedDenyPathsWhenUnelevated(t *testing.T) { if err != nil { t.Fatalf("BuildWindowsACLPlan: %v", err) } - systemDrive, systemRoot, programData, publicDir := windowsSharedDenyPathsForTest() + 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) { @@ -104,22 +104,15 @@ func TestBuildWindowsACLPlanOmitsSharedDenyPathsWhenUnelevated(t *testing.T) { } } -func windowsSharedDenyPathsForTest() (systemDrive, systemRoot, programData, publicDir string) { - systemDrive = os.Getenv("SystemDrive") - if systemDrive == "" { - systemDrive = "C:" - } - systemRoot = os.Getenv("SystemRoot") - if systemRoot == "" { - systemRoot = systemDrive + `\Windows` - } - programData = os.Getenv("ProgramData") - if programData == "" { - programData = systemDrive + `\ProgramData` - } - publicDir = os.Getenv("PUBLIC") - if publicDir == "" { - publicDir = systemDrive + `\Users\Public` +// 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 } @@ -148,11 +141,11 @@ func TestBuildWindowsACLPlanUsesReadOnlySIDWithoutWriteRoots(t *testing.T) { t.Fatalf("ACL entries = %#v, want five entries (1 deny-read, 4 deny-write)", plan.Entries) } assertWindowsACLEntry(t, plan, WindowsACLDenyRead, `C:\workspace\secret-read`, caps.ReadOnly, true) - systemDrive, systemRoot, programData, publicDir := windowsSharedDenyPathsForTest() - assertWindowsACLEntry(t, plan, WindowsACLDenyWrite, systemDrive+`\`, caps.ReadOnly, false) - assertWindowsACLEntry(t, plan, WindowsACLDenyWrite, programData, caps.ReadOnly, false) - assertWindowsACLEntry(t, plan, WindowsACLDenyWrite, systemRoot+`\Temp`, caps.ReadOnly, false) - assertWindowsACLEntry(t, plan, WindowsACLDenyWrite, publicDir, caps.ReadOnly, false) + systemDrive, systemRoot, programData, publicDir := windowsSharedDenyPathsForTest(t) + assertWindowsACLEntryInheritance(t, plan, WindowsACLDenyWrite, systemDrive+`\`, caps.ReadOnly, false, true) + assertWindowsACLEntryInheritance(t, plan, WindowsACLDenyWrite, programData, caps.ReadOnly, false, true) + assertWindowsACLEntryInheritance(t, plan, WindowsACLDenyWrite, systemRoot+`\Temp`, caps.ReadOnly, false, true) + assertWindowsACLEntryInheritance(t, plan, WindowsACLDenyWrite, publicDir, caps.ReadOnly, false, true) } func TestBuildWindowsACLPlanRejectsUnrestrictedProfiles(t *testing.T) { @@ -193,16 +186,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 { From 949397f33771f8fa738aabcf97e2eb730e364c44 Mon Sep 17 00:00:00 2001 From: euxaristia <25621994+euxaristia@users.noreply.github.com> Date: Wed, 15 Jul 2026 13:00:33 -0400 Subject: [PATCH 6/7] fix(sandbox): scope SID broadening to fully restricted tokens, stable deny identity - Users/Authenticated Users are only added to the restricted-SID list when the token is fully restricted (a DenyRead profile): a WRITE_RESTRICTED token already reads with its normal identity, so broadening it gained nothing for Program Files/System32 reads while letting those groups' write grants pass the restricted-SID write check. The default elevated profile therefore no longer takes the write-jail risk at all. - The shared system-path DenyWrite mitigation (C:\, ProgramData, Windows\Temp, Public) is only planned for DenyRead profiles, the ones whose tokens actually carry the broadened SIDs, and its deny ACEs name only the stable read-only capability SID, which every broadened token now carries. Machine-wide DACLs stay at a constant four entries total instead of growing by four per distinct sandboxed project. - The real-Windows smoke's Public-directory probe now pins that a non-DenyRead profile's token cannot reach the Users write grant at all, and plan-level tests pin both the stable deny identity and the absence of shared entries for non-DenyRead profiles. --- .../runner_windows_integration_test.go | 14 ++--- internal/sandbox/windows_acl.go | 45 ++++++++-------- internal/sandbox/windows_acl_test.go | 51 +++++++++++++++++-- .../sandbox/windows_command_runner_windows.go | 28 ++++++++-- internal/sandbox/windows_token_windows.go | 21 +++++--- 5 files changed, 114 insertions(+), 45 deletions(-) diff --git a/internal/sandbox/runner_windows_integration_test.go b/internal/sandbox/runner_windows_integration_test.go index f2752a55..c14bd04d 100644 --- a/internal/sandbox/runner_windows_integration_test.go +++ b/internal/sandbox/runner_windows_integration_test.go @@ -72,13 +72,13 @@ func TestWindowsRestrictedTokenRealSandboxSmoke(t *testing.T) { t.Fatalf("sandboxed write marker = %q, %v; want ok", bytes, err) } - // The elevated tier's restricted token carries the Users/Authenticated - // Users SIDs (added for Program Files/System32 reads), which also match - // the write grant those groups already hold on C:\Users\Public. Pin that - // BuildWindowsACLPlan's DenyWrite mitigation still blocks a write there: - // an independent shared-writable directory outside every carved-out - // system path (ProgramData, Windows\Temp), and outside every workspace - // write root. + // This profile has no DenyRead paths, so its WRITE_RESTRICTED token is + // never broadened with the Users/Authenticated Users SIDs (see + // createWindowsRestrictedTokenFromBase): the write grant those groups + // hold on C:\Users\Public must not be reachable through the restricted + // SID check at all. Pin that a write there fails: an independent + // shared-writable directory outside every carved-out system path + // (ProgramData, Windows\Temp), and outside every workspace write root. publicDir := os.Getenv("PUBLIC") if publicDir == "" { t.Skip("PUBLIC is not set; cannot probe C:\\Users\\Public write jail") diff --git a/internal/sandbox/windows_acl.go b/internal/sandbox/windows_acl.go index 3a82999e..9bb5cb19 100644 --- a/internal/sandbox/windows_acl.go +++ b/internal/sandbox/windows_acl.go @@ -88,14 +88,15 @@ func BuildWindowsACLPlan(config WindowsSandboxCommandConfig) (WindowsACLPlan, er // Deny write to shared Windows-writable directories (C:\, C:\ProgramData, // C:\Windows\Temp, C:\Users\Public) to prevent write-jail escape via the - // added Users and Authenticated Users SIDs. Only the elevated tier - // (WindowsSandboxLevelRestrictedToken, applied by `zero sandbox setup` - // running as Administrator) reaches here with those SIDs on the token in - // the first place — see createWindowsRestrictedTokenFromBase — and only - // that tier has the WRITE_DAC needed to edit these system-owned DACLs. - // The unelevated tier keeps the narrower (pre-widening) restricting-SID - // set and never needs these entries. - if config.SandboxLevel == WindowsSandboxLevelRestrictedToken { + // added Users and Authenticated Users SIDs. Only DenyRead profiles on the + // elevated tier (WindowsSandboxLevelRestrictedToken, applied by `zero + // sandbox setup` running as Administrator) carry those SIDs at all: a + // WRITE_RESTRICTED token reads with its normal identity and is never + // broadened, so the default profile needs no shared entries, and only + // the elevated tier has the WRITE_DAC needed to edit these system-owned + // DACLs. The unelevated tier keeps the narrower restricting-SID set and + // never needs these entries either. + if config.SandboxLevel == WindowsSandboxLevelRestrictedToken && len(config.PermissionProfile.FileSystem.DenyRead) > 0 { // Resolved from trusted Win32 APIs, not from the // SystemDrive/SystemRoot/ProgramData/PUBLIC environment variables: // see resolveWindowsSharedDenyPaths for why trusting the environment @@ -112,15 +113,19 @@ func BuildWindowsACLPlan(config WindowsSandboxCommandConfig) (WindowsACLPlan, er publicDir, } + // The deny ACEs name only the stable read-only capability SID, which + // every broadened token carries (see the runner): a deny ACE blocks + // when it matches ANY SID on the token, so one shared identity is + // sufficient, and it keeps these machine-wide DACLs at a constant + // four entries total. Naming the per-workspace/per-root capability + // SIDs here instead would append four permanent deny ACEs for every + // distinct project ever sandboxed on the machine, growing C:\, + // ProgramData, Windows\Temp, and Public's DACLs without bound. caps, err := LoadOrCreateWindowsCapabilitySIDs(config.SandboxHome) if err != nil { return WindowsACLPlan{}, err } - var allSIDs []string - for _, cap := range writeCapabilities { - allSIDs = append(allSIDs, cap.SID) - } - allSIDs = append(allSIDs, caps.ReadOnly) + denySID := caps.ReadOnly for _, denyPath := range sharedDenyPaths { if windowsPathEqualsAnyRoot(denyPath, writeCapabilities) { @@ -142,14 +147,12 @@ func BuildWindowsACLPlan(config WindowsSandboxCommandConfig) (WindowsACLPlan, er // on each one is sufficient: it blocks the denied SIDs from // writing (including creating new children) directly under that // path without ever touching any descendant's own ACL. - for _, sid := range allSIDs { - entries = append(entries, WindowsACLEntry{ - Action: WindowsACLDenyWrite, - Path: denyPath, - Capability: sid, - NoInherit: true, - }) - } + entries = append(entries, WindowsACLEntry{ + Action: WindowsACLDenyWrite, + Path: denyPath, + Capability: denySID, + NoInherit: true, + }) } } diff --git a/internal/sandbox/windows_acl_test.go b/internal/sandbox/windows_acl_test.go index 2d2e2ae9..a18b6b64 100644 --- a/internal/sandbox/windows_acl_test.go +++ b/internal/sandbox/windows_acl_test.go @@ -60,11 +60,52 @@ func TestBuildWindowsACLPlanForWorkspaceWriteProfile(t *testing.T) { } systemDrive, systemRoot, programData, publicDir := windowsSharedDenyPathsForTest(t) - for _, sid := range []string{workspaceSID, cacheSID, caps.ReadOnly} { - assertWindowsACLEntryInheritance(t, plan, WindowsACLDenyWrite, systemDrive+`\`, sid, false, true) - assertWindowsACLEntryInheritance(t, plan, WindowsACLDenyWrite, programData, sid, false, true) - assertWindowsACLEntryInheritance(t, plan, WindowsACLDenyWrite, systemRoot+`\Temp`, sid, false, true) - assertWindowsACLEntryInheritance(t, plan, WindowsACLDenyWrite, publicDir, sid, false, true) + // The shared system-path denies name only the one stable read-only SID + // that every broadened token carries. Naming the per-workspace/per-root + // SIDs here instead would append four permanent deny ACEs to these + // machine-wide DACLs for every distinct project ever sandboxed. + for _, path := range []string{systemDrive + `\`, programData, systemRoot + `\Temp`, publicDir} { + assertWindowsACLEntryInheritance(t, plan, WindowsACLDenyWrite, path, caps.ReadOnly, false, true) + for _, entry := range plan.Entries { + if entry.Action != WindowsACLDenyWrite || windowsCapabilityPathKey(entry.Path) != windowsCapabilityPathKey(path) { + continue + } + if entry.Capability == workspaceSID || entry.Capability == cacheSID { + t.Fatalf("shared deny path %q names per-root SID %q; machine DACLs must only carry the stable read-only SID", path, entry.Capability) + } + } + } +} + +// TestBuildWindowsACLPlanOmitsSharedDenyPathsWithoutDenyRead pins the +// scoping of the Users/Authenticated Users broadening: profiles without +// DenyRead run under a WRITE_RESTRICTED token, which reads with its normal +// identity and is never broadened, so their plans must not touch the shared +// system-path DACLs at all. +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) + } + 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 without DenyRead touches shared path %q: %#v", path, entry) + } + } } } diff --git a/internal/sandbox/windows_command_runner_windows.go b/internal/sandbox/windows_command_runner_windows.go index 89542e2a..43fdeeab 100644 --- a/internal/sandbox/windows_command_runner_windows.go +++ b/internal/sandbox/windows_command_runner_windows.go @@ -69,16 +69,36 @@ 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) - // Only the elevated restricted-token tier can enforce the DenyWrite - // mitigation BuildWindowsACLPlan adds for the broadened read SIDs (it - // requires Administrator rights); see createWindowsRestrictedTokenFromBase. - broadenReadSIDs := config.SandboxLevel == WindowsSandboxLevelRestrictedToken // 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. writeRestricted := len(config.PermissionProfile.FileSystem.DenyRead) == 0 + // Broadening with Users/Authenticated Users is only useful on the fully + // restricted token, where READS also require a restricted-SID match and + // system paths like Program Files and System32 grant those groups rather + // than Everyone. A WRITE_RESTRICTED token already performs reads with the + // normal token identity, so broadening there cannot improve reads at all; + // it would only let Users/Authenticated Users write grants pass the + // restricted-SID write check and weaken the default write jail for no + // benefit. It also needs the elevated tier: only that tier can enforce + // the shared-directory DenyWrite mitigation BuildWindowsACLPlan adds for + // the broadened SIDs (it requires Administrator rights); see + // createWindowsRestrictedTokenFromBase. + broadenReadSIDs := config.SandboxLevel == WindowsSandboxLevelRestrictedToken && !writeRestricted + if broadenReadSIDs { + // The shared-directory DenyWrite mitigation names the one stable + // read-only capability SID rather than the per-workspace SIDs (see + // BuildWindowsACLPlan), so every broadened token must carry it for + // those deny ACEs to bind. + caps, err := LoadOrCreateWindowsCapabilitySIDs(config.SandboxHome) + if err != nil { + fmt.Fprintln(stderr, WindowsSandboxCommandRunnerName+": "+err.Error()) + return 1 + } + tokenSIDs = append(tokenSIDs, caps.ReadOnly) + } token, err := createWindowsRestrictedTokenForCapabilitySIDs(tokenSIDs, broadenReadSIDs, writeRestricted) if err != nil { fmt.Fprintln(stderr, WindowsSandboxCommandRunnerName+": "+err.Error()) diff --git a/internal/sandbox/windows_token_windows.go b/internal/sandbox/windows_token_windows.go index 5496bfd4..8aa11016 100644 --- a/internal/sandbox/windows_token_windows.go +++ b/internal/sandbox/windows_token_windows.go @@ -87,14 +87,19 @@ func createWindowsRestrictedTokenForCapabilitySIDs(capabilitySIDStrings []string // 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. Because the restricting-SID -// check applies to writes as well as reads, this also grants write wherever -// those groups already have it — BuildWindowsACLPlan mitigates that by adding -// DenyWrite ACEs to the known shared Users/Authenticated-Users-writable -// directories, but it can only do so with Administrator rights (see -// WindowsSandboxLevelRestrictedToken). broadenReadSIDs must therefore stay -// false for WindowsSandboxLevelUnelevated, which cannot enforce that -// mitigation: it keeps the original (narrower) read scope instead of +// Users/Authenticated Users rather than Everyone. That only matters on the +// fully restricted token (writeRestricted=false), where reads also require a +// restricted-SID match; a WRITE_RESTRICTED token reads with its normal +// identity, so broadening it would gain nothing for reads while letting the +// groups' write grants pass the restricted-SID write check. Because the +// restricting-SID check applies to writes as well as reads, broadening also +// grants write wherever those groups already have it — BuildWindowsACLPlan +// mitigates that by adding DenyWrite ACEs to the known shared +// Users/Authenticated-Users-writable directories, but it can only do so +// with Administrator rights (see WindowsSandboxLevelRestrictedToken). +// broadenReadSIDs must therefore stay false both when writeRestricted is set +// and for WindowsSandboxLevelUnelevated, which cannot enforce that +// mitigation: those keep the original (narrower) SID scope instead of // widening the write jail with nothing to close the gap. // // writeRestricted requests a WRITE_RESTRICTED token, which scopes the From 821f43673b154db4f87fec180a2305b077b8e0ba Mon Sep 17 00:00:00 2001 From: euxaristia <25621994+euxaristia@users.noreply.github.com> Date: Wed, 15 Jul 2026 14:49:16 -0400 Subject: [PATCH 7/7] fix(sandbox): include NoInherit in the ACL entry dedupe key A direct-only deny and an inheritable deny on the same path and SID are different ACL shapes; 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. --- internal/sandbox/windows_acl.go | 6 +++++- internal/sandbox/windows_acl_test.go | 21 +++++++++++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/internal/sandbox/windows_acl.go b/internal/sandbox/windows_acl.go index 9bb5cb19..7d1ada8b 100644 --- a/internal/sandbox/windows_acl.go +++ b/internal/sandbox/windows_acl.go @@ -277,7 +277,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_test.go b/internal/sandbox/windows_acl_test.go index a18b6b64..5666b15b 100644 --- a/internal/sandbox/windows_acl_test.go +++ b/internal/sandbox/windows_acl_test.go @@ -254,3 +254,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) + } +}