diff --git a/internal/sandbox/runner_windows_integration_test.go b/internal/sandbox/runner_windows_integration_test.go index 708f43e4..c14bd04d 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) } + // 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") + } + 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) @@ -203,6 +226,24 @@ func TestWindowsUnelevatedRealSandboxSmoke(t *testing.T) { } else if !os.IsNotExist(err) { t.Fatalf("stat outside marker: %v", err) } + + // Verify write to C:\ProgramData is blocked + programData := os.Getenv("ProgramData") + if programData != "" { + programDataMarker := filepath.Join(programData, "zero-unelevated-write-denied.txt") + _ = os.Remove(programDataMarker) + + runWindowsRealSmokeCommand(t, runnerExe, config, []string{ + "cmd.exe", "/d", "/s", "/c", "echo leaked>" + programDataMarker, + }, 1) + + if _, err := os.Stat(programDataMarker); err == nil { + _ = os.Remove(programDataMarker) + t.Fatalf("unelevated sandbox allowed a write to ProgramData shared directory") + } else if !os.IsNotExist(err) { + t.Fatalf("stat ProgramData marker: %v", err) + } + } } // TestWindowsRestrictedTokenNestedPipeCapture pins the fix in diff --git a/internal/sandbox/windows_acl.go b/internal/sandbox/windows_acl.go index 55d37d34..7d1ada8b 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" ) @@ -15,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 { @@ -76,9 +85,93 @@ 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 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 + // 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{ + systemDrive + `\`, + programData, + systemRoot + `\Temp`, + 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 + } + denySID := caps.ReadOnly + + for _, denyPath := range sharedDenyPaths { + if windowsPathEqualsAnyRoot(denyPath, writeCapabilities) { + continue // Do not deny write if it is exactly an allowed write root + } + // 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. + entries = append(entries, WindowsACLEntry{ + Action: WindowsACLDenyWrite, + Path: denyPath, + Capability: denySID, + NoInherit: true, + }) + } + } + return WindowsACLPlan{Entries: dedupeWindowsACLEntries(entries)}, nil } +func windowsPathEqualsAnyRoot(path string, capabilities []windowsWriteRootCapability) bool { + key := windowsCapabilityPathKey(path) + if key == "" { + return false + } + for _, cap := range capabilities { + if windowsCapabilityPathKey(cap.Root) == key { + return true + } + } + return false +} + type windowsWriteRootCapability struct { Root string SID string @@ -184,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_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_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 1925bd8a..5666b15b 100644 --- a/internal/sandbox/windows_acl_test.go +++ b/internal/sandbox/windows_acl_test.go @@ -12,6 +12,7 @@ func TestBuildWindowsACLPlanForWorkspaceWriteProfile(t *testing.T) { config := WindowsSandboxCommandConfig{ SandboxHome: home, WorkspaceRoots: []string{`C:\workspace`}, + SandboxLevel: WindowsSandboxLevelRestrictedToken, PermissionProfile: PermissionProfile{ FileSystem: FileSystemPolicy{ Kind: FileSystemRestricted, @@ -52,6 +53,109 @@ 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, systemRoot, programData, publicDir := windowsSharedDenyPathsForTest(t) + + // 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) + } + } + } +} + +// 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(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("unelevated ACL plan = %#v, want no DenyWrite entry for shared path %q", plan.Entries, path) + } + } + } +} + +// windowsSharedDenyPathsForTest calls the same trusted-path resolution +// BuildWindowsACLPlan itself uses, rather than reimplementing the +// resolution logic independently, so this test cannot silently drift out of +// sync with (or mask a regression in) the production resolver. +func windowsSharedDenyPathsForTest(t *testing.T) (systemDrive, systemRoot, programData, publicDir string) { + t.Helper() + systemDrive, systemRoot, programData, publicDir, err := resolveWindowsSharedDenyPaths() + if err != nil { + t.Fatalf("resolveWindowsSharedDenyPaths: %v", err) + } + return systemDrive, systemRoot, programData, publicDir } func TestBuildWindowsACLPlanUsesReadOnlySIDWithoutWriteRoots(t *testing.T) { @@ -61,7 +165,8 @@ func TestBuildWindowsACLPlanUsesReadOnlySIDWithoutWriteRoots(t *testing.T) { t.Fatalf("LoadOrCreateWindowsCapabilitySIDs: %v", err) } plan, err := BuildWindowsACLPlan(WindowsSandboxCommandConfig{ - SandboxHome: home, + SandboxHome: home, + SandboxLevel: WindowsSandboxLevelRestrictedToken, PermissionProfile: PermissionProfile{ FileSystem: FileSystemPolicy{ Kind: FileSystemRestricted, @@ -73,10 +178,15 @@ 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) != 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, 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) { @@ -117,16 +227,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 +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) + } +} diff --git a/internal/sandbox/windows_command_runner_windows.go b/internal/sandbox/windows_command_runner_windows.go index 0b9e8f64..43fdeeab 100644 --- a/internal/sandbox/windows_command_runner_windows.go +++ b/internal/sandbox/windows_command_runner_windows.go @@ -75,7 +75,31 @@ func runWindowsSandboxCommand(config WindowsSandboxCommandConfig, stderr io.Writ // 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 - token, err := createWindowsRestrictedTokenForCapabilitySIDs(tokenSIDs, writeRestricted) + // 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()) return 1 diff --git a/internal/sandbox/windows_token_windows.go b/internal/sandbox/windows_token_windows.go index a02e9b00..8aa11016 100644 --- a/internal/sandbox/windows_token_windows.go +++ b/internal/sandbox/windows_token_windows.go @@ -48,7 +48,7 @@ func (sid windowsLocalSID) close() { } } -func createWindowsRestrictedTokenForCapabilitySIDs(capabilitySIDStrings []string, writeRestricted bool) (windows.Token, error) { +func createWindowsRestrictedTokenForCapabilitySIDs(capabilitySIDStrings []string, broadenReadSIDs bool, writeRestricted bool) (windows.Token, error) { if len(capabilitySIDStrings) == 0 { return 0, errors.New("windows restricted token requires at least one capability SID") } @@ -80,10 +80,37 @@ func createWindowsRestrictedTokenForCapabilitySIDs(capabilitySIDStrings []string return 0, fmt.Errorf("open process token: %w", err) } defer base.Close() - return createWindowsRestrictedTokenFromBase(base, capabilitySIDs, writeRestricted) + return createWindowsRestrictedTokenFromBase(base, capabilitySIDs, broadenReadSIDs, writeRestricted) } -func createWindowsRestrictedTokenFromBase(base windows.Token, capabilitySIDs []windowsLocalSID, writeRestricted bool) (windows.Token, error) { +// createWindowsRestrictedTokenFromBase builds the restricted token. When +// broadenReadSIDs is set, it also restricts to WinBuiltinUsersSid and +// WinAuthenticatedUserSid so the sandboxed process can read/execute binaries +// under paths like C:\Program Files or C:\Windows whose ACLs grant +// Users/Authenticated Users rather than Everyone. 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 +// restricted-SID check to write-type accesses only: reads use the token's +// normal (unrestricted) identity, so the sandboxed process can open +// executables, DLLs, and per-user config the signed-in user can read. The +// caller sets writeRestricted=false whenever DenyRead paths are configured, +// because a WRITE_RESTRICTED token also makes the kernel skip restricted-SID +// deny ACEs for reads (the DenyRead bypass fixed in #612), so DenyRead +// enforcement requires the fully restricted token instead. +func createWindowsRestrictedTokenFromBase(base windows.Token, capabilitySIDs []windowsLocalSID, broadenReadSIDs bool, writeRestricted bool) (windows.Token, error) { logonSID, err := copyWindowsLogonSID(base) if err != nil { return 0, err @@ -93,7 +120,7 @@ func createWindowsRestrictedTokenFromBase(base windows.Token, capabilitySIDs []w return 0, fmt.Errorf("create world SID: %w", err) } - entries := make([]windows.SIDAndAttributes, 0, len(capabilitySIDs)+2) + entries := make([]windows.SIDAndAttributes, 0, len(capabilitySIDs)+4) for _, sid := range capabilitySIDs { entries = append(entries, windows.SIDAndAttributes{Sid: sid.sid}) } @@ -101,6 +128,20 @@ func createWindowsRestrictedTokenFromBase(base windows.Token, capabilitySIDs []w windows.SIDAndAttributes{Sid: sidFromBytes(logonSID)}, windows.SIDAndAttributes{Sid: worldSID}, ) + if broadenReadSIDs { + usersSID, err := windows.CreateWellKnownSid(windows.WinBuiltinUsersSid) + if err != nil { + return 0, fmt.Errorf("create users SID: %w", err) + } + authUserSID, err := windows.CreateWellKnownSid(windows.WinAuthenticatedUserSid) + if err != nil { + return 0, fmt.Errorf("create authenticated user SID: %w", err) + } + entries = append(entries, + windows.SIDAndAttributes{Sid: usersSID}, + windows.SIDAndAttributes{Sid: authUserSID}, + ) + } // WRITE_RESTRICTED scopes the restricted-SID check to write-type accesses: // reads use only the normal token identity, so the sandboxed process can