From 3c3a466f5136f8c22956d887bb13ea8ce2b45b9d Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Mon, 3 Aug 2026 18:14:44 +0530 Subject: [PATCH 1/2] fix(sandbox): stop the Windows write jail honouring Everyone-granted paths The restricted token put the World SID (S-1-1-0, Everyone) in its restricted-SID list unconditionally, from the original sandbox baseline and with no comment saying why. A WRITE_RESTRICTED token runs two checks for a write and needs both: the normal one against its enabled SIDs, and a second against the restricted list. The write jail rests entirely on that second check only succeeding where Zero has explicitly ACL'd a capability SID. Everyone is a SID every principal carries, so on any path whose DACL grants Everyone write, the restricted half passed for free and confinement fell back to the ordinary user's own permissions -- the exact boundary this token exists to be stricter than. Reachable with no privilege, no symlink and no race: an Everyone-writable directory is enough, and share roots opened with Everyone:F and loose third-party installer ACLs supply them. Ruled out as the common case: C:\Users\Public\Documents grants BATCH, not Everyone. The runner already stated the rule this broke -- it declines to add the user SID, Administrators or SYSTEM because "each has write access nearly everywhere" -- while listing Everyone among the restricted SIDs. Removed only from the WRITE_RESTRICTED token, because without that flag it is load-bearing rather than gratuitous. The restricted-SID check then applies to reads too, and default Windows DACLs grant BUILTIN\Users rather than anything in this list, so a token with no Everyone cannot open cmd.exe and dies at launch with STATUS_ACCESS_DENIED (0xC0000022). Dropping it there was tried first and takes TestWindowsUnelevatedRealSandboxSmoke with it. That path is taken only when a profile configures DenyRead, which is already the posture trading capability for read-deny enforcement (#612), so the bypass survives there deliberately and narrowly; closing it needs a read-side grant that is not a universal group. Verified against real tokens rather than reasoned about. The new smoke test drives the real runner and asserts three outcomes that only agree when the jail is intact: a granted root is writable, an ordinary outside path is denied, and an Everyone-writable outside path is denied too. Before the change its third case wrote the file with exit 0; the first two were already correct, which is what made the gap invisible. Restoring the SID unconditionally turns it red again. --- .../runner_windows_integration_test.go | 105 ++++++++++++++++++ internal/sandbox/windows_token_windows.go | 54 +++++++-- 2 files changed, 150 insertions(+), 9 deletions(-) diff --git a/internal/sandbox/runner_windows_integration_test.go b/internal/sandbox/runner_windows_integration_test.go index d0f715744..0fc8caf68 100644 --- a/internal/sandbox/runner_windows_integration_test.go +++ b/internal/sandbox/runner_windows_integration_test.go @@ -386,6 +386,111 @@ func runWindowsRealSmokeCommand(t *testing.T, runnerExe string, base WindowsSand } } +// The write jail must hold on a path whose DACL grants Everyone write access. +// +// A WRITE_RESTRICTED token runs TWO checks for a write and needs both to pass: +// the normal one against its enabled SIDs, and a second against its RESTRICTED +// SID list. The jail is built on the second check only succeeding where Zero has +// explicitly ACL'd a capability SID. Putting the World SID (S-1-1-0) in the +// restricted list breaks that globally — every principal is a member of +// Everyone, so on a DACL that grants Everyone write, the restricted half passes +// for free and confinement collapses back to the ordinary user's own +// permissions, which is precisely the boundary the sandbox exists to be +// stricter than. +// +// The runner already states this rule for the SIDs it refuses to add ("None of +// the granted SIDs can be added to the restricted list without collapsing the +// write jail"); Everyone was in the list anyway, since the original sandbox +// baseline. +// +// Realistic rather than theoretical: administrators open share roots with +// Everyone:F, and third-party installers ship loose ACLs. It needs no privilege, +// no symlink and no race. Checked and ruled out: C:\Users\Public\Documents +// grants BATCH, not Everyone, so the common "public folder" case is not this. +func TestWindowsRestrictedTokenDeniesWritesToEveryoneWritablePaths(t *testing.T) { + if os.Getenv("ZERO_SANDBOX_REAL_SMOKE") != "1" { + t.Skip("set ZERO_SANDBOX_REAL_SMOKE=1 to run real Windows sandbox smoke tests") + } + runnerExe := realSmokeExecutable(t, "ZERO_WINDOWS_COMMAND_RUNNER_EXE", WindowsSandboxCommandRunnerName) + + root := t.TempDir() + outside := t.TempDir() + everyoneDir := filepath.Join(outside, "everyone-writable") + if err := os.MkdirAll(everyoneDir, 0o700); err != nil { + t.Fatalf("MkdirAll everyone-writable: %v", err) + } + // Granted through the production ACL applier, so the hostile DACL is built + // the same way a real one is rather than by a test-only shortcut. + snapshot, applied, err := applyWindowsACLPathGroup(windowsACLPathGroup{ + Path: everyoneDir, + Entries: []WindowsACLEntry{{ + Action: WindowsACLAllowWrite, + Path: everyoneDir, + Capability: "S-1-1-0", + }}, + }) + if err != nil { + t.Fatalf("grant Everyone write: %v", err) + } + if !applied { + t.Fatal("precondition: the Everyone grant did not apply, so there is no bypass to test for") + } + t.Cleanup(func() { _ = rollbackWindowsACLSnapshots([]windowsACLSnapshot{snapshot}) }) + + sandboxHome := filepath.Join(root, ".zero-sandbox") + // No DenyRead on purpose: that is what makes the runner choose the + // WRITE_RESTRICTED token, which is the default posture and the one whose + // restricted-SID list this test is about. + profile := PermissionProfile{ + FileSystem: FileSystemPolicy{ + Kind: FileSystemRestricted, + ReadRoots: []string{root}, + WriteRoots: []WritableRoot{{Root: root, ProtectedMetadataNames: []string{".git", ".zero", ".agents"}}}, + IncludePlatformRoots: true, + AllowTemp: true, + }, + Network: NetworkPolicy{Mode: NetworkDeny}, + } + config := WindowsSandboxCommandArgsOptions{ + SandboxHome: sandboxHome, + CommandCWD: root, + WorkspaceRoots: []string{root}, + PermissionProfile: profile, + SandboxLevel: WindowsSandboxLevelUnelevated, + } + + // Granted root first. A jail that denies everything would satisfy the two + // assertions below while being useless, and this separates the two outcomes. + insideMarker := filepath.Join(root, "write-ok.txt") + runWindowsRealSmokeCommand(t, runnerExe, config, []string{ + "cmd.exe", "/d", "/s", "/c", "echo ok>" + insideMarker, + }, 0) + if contents, err := os.ReadFile(insideMarker); err != nil || strings.TrimSpace(string(contents)) != "ok" { + t.Fatalf("sandboxed write to a granted root = %q, %v; want ok", contents, err) + } + + // Control: an ordinary directory outside every granted root. This is the + // behaviour the Everyone case must match. + controlMarker := filepath.Join(outside, "control-denied.txt") + runWindowsRealSmokeCommand(t, runnerExe, config, []string{ + "cmd.exe", "/d", "/s", "/c", "echo leaked>" + controlMarker, + }, 1) + if _, err := os.Stat(controlMarker); err == nil { + t.Fatal("precondition: the sandbox allowed a write to an ordinary path outside every granted root, so this test cannot measure the Everyone case") + } + + // The assertion. Same as the control in every respect except the DACL. + everyoneMarker := filepath.Join(everyoneDir, "everyone-denied.txt") + runWindowsRealSmokeCommand(t, runnerExe, config, []string{ + "cmd.exe", "/d", "/s", "/c", "echo leaked>" + everyoneMarker, + }, 1) + if _, err := os.Stat(everyoneMarker); err == nil { + t.Error("the sandbox wrote outside every granted root because the path grants Everyone write; the restricted-SID list is satisfied by a SID every principal carries") + } else if !os.IsNotExist(err) { + t.Errorf("stat the Everyone-writable marker: %v", err) + } +} + func powershellSingleQuote(value string) string { out := "'" for _, r := range value { diff --git a/internal/sandbox/windows_token_windows.go b/internal/sandbox/windows_token_windows.go index a02e9b001..1cf360948 100644 --- a/internal/sandbox/windows_token_windows.go +++ b/internal/sandbox/windows_token_windows.go @@ -88,19 +88,55 @@ func createWindowsRestrictedTokenFromBase(base windows.Token, capabilitySIDs []w if err != nil { return 0, err } - worldSID, err := windows.CreateWellKnownSid(windows.WinWorldSid) - if err != nil { - return 0, fmt.Errorf("create world SID: %w", err) - } - entries := make([]windows.SIDAndAttributes, 0, len(capabilitySIDs)+2) for _, sid := range capabilitySIDs { entries = append(entries, windows.SIDAndAttributes{Sid: sid.sid}) } - entries = append(entries, - windows.SIDAndAttributes{Sid: sidFromBytes(logonSID)}, - windows.SIDAndAttributes{Sid: worldSID}, - ) + entries = append(entries, windows.SIDAndAttributes{Sid: sidFromBytes(logonSID)}) + + // The World SID (S-1-1-0, Everyone) is added ONLY to the token that does not + // carry WRITE_RESTRICTED, and putting it back unconditionally would reopen a + // write-jail bypass. + // + // A restricted SID is a key to every object whose DACL names it. That is why + // the runner refuses to add the user SID, Administrators or SYSTEM — "each + // has write access nearly everywhere". Everyone is the broadest of the lot: + // every principal carries it, so under WRITE_RESTRICTED the second + // (restricted-SID) check passes for free on any path whose DACL grants + // Everyone write, and confinement falls back to the ordinary user's own + // permissions — the exact boundary this token exists to be stricter than. It + // needs no privilege, no symlink and no race: an Everyone-writable directory + // is enough, and share roots opened with Everyone:F and loose installer ACLs + // supply them. It was present from the original sandbox baseline, + // uncommented, and under WRITE_RESTRICTED nothing depends on it, because that + // flag already exempts reads from the restricted-SID check. + // + // Without the flag it IS load-bearing and cannot simply be dropped. The + // restricted-SID check then applies to READS too, and default Windows DACLs + // grant BUILTIN\Users rather than anything in this list, so a token without + // Everyone cannot open cmd.exe — the process dies at launch with + // STATUS_ACCESS_DENIED (0xC0000022) before it runs anything. That path is + // only taken when the profile configures DenyRead, which is already the + // posture that trades capability for read-deny enforcement (#612). + // + // So the bypass survives for DenyRead profiles, deliberately and narrowly, + // rather than being traded for a sandbox that cannot start a command. Closing + // it there needs a different mechanism (a read-side grant that is not a + // universal group), tracked separately. + // + // The logon SID above stays in both modes: it is this token's own rather than + // a broad group, and broadenWindowsRestrictedTokenDefaultDacl depends on it so + // the process can use pipes and events it creates for itself. + // + // Anything added here needs the same scrutiny — Authenticated Users, Users, + // INTERACTIVE and BATCH would each produce this bypass on a DACL naming them. + if !writeRestricted { + worldSID, err := windows.CreateWellKnownSid(windows.WinWorldSid) + if err != nil { + return 0, fmt.Errorf("create world SID: %w", err) + } + entries = append(entries, windows.SIDAndAttributes{Sid: worldSID}) + } // WRITE_RESTRICTED scopes the restricted-SID check to write-type accesses: // reads use only the normal token identity, so the sandboxed process can From 6cac6c2abdf6625a7a390ae81a357d7ce0adb64d Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Mon, 3 Aug 2026 18:24:14 +0530 Subject: [PATCH 2/2] test(sandbox): make a denied write distinguishable from a runner failure The denial assertions expected exit 1, which the runner also returns for its own errors -- a failed marker validation, a bad argument, a token it could not build. Written that way the test passes both when the sandbox denies the write and when nothing ever ran, and the second case proves nothing while being indistinguishable from success. That is a poor property for any test and a bad one for this test, whose entire job is to notice a confinement regression. The leading "granted root is writable" assertion covered part of it, but said nothing about whether the two later commands launched. The denial commands now report a distinctive exit code the runner cannot produce for itself, so the code proves cmd.exe actually ran and its redirect was refused. Re-checked in both directions rather than assumed: the test passes against the fix, and restoring the World SID unconditionally fails it with "exit code = 0, want 77" -- still catching the bug it was written for. Raised by CodeRabbit on #865. --- .../runner_windows_integration_test.go | 29 +++++++++++++++---- 1 file changed, 23 insertions(+), 6 deletions(-) diff --git a/internal/sandbox/runner_windows_integration_test.go b/internal/sandbox/runner_windows_integration_test.go index 0fc8caf68..fb98f1287 100644 --- a/internal/sandbox/runner_windows_integration_test.go +++ b/internal/sandbox/runner_windows_integration_test.go @@ -9,6 +9,7 @@ import ( "os" "os/exec" "path/filepath" + "strconv" "strings" "testing" "time" @@ -472,18 +473,14 @@ func TestWindowsRestrictedTokenDeniesWritesToEveryoneWritablePaths(t *testing.T) // Control: an ordinary directory outside every granted root. This is the // behaviour the Everyone case must match. controlMarker := filepath.Join(outside, "control-denied.txt") - runWindowsRealSmokeCommand(t, runnerExe, config, []string{ - "cmd.exe", "/d", "/s", "/c", "echo leaked>" + controlMarker, - }, 1) + runWindowsRealSmokeCommand(t, runnerExe, config, deniedWriteCommand(controlMarker), deniedWriteExitCode) if _, err := os.Stat(controlMarker); err == nil { t.Fatal("precondition: the sandbox allowed a write to an ordinary path outside every granted root, so this test cannot measure the Everyone case") } // The assertion. Same as the control in every respect except the DACL. everyoneMarker := filepath.Join(everyoneDir, "everyone-denied.txt") - runWindowsRealSmokeCommand(t, runnerExe, config, []string{ - "cmd.exe", "/d", "/s", "/c", "echo leaked>" + everyoneMarker, - }, 1) + runWindowsRealSmokeCommand(t, runnerExe, config, deniedWriteCommand(everyoneMarker), deniedWriteExitCode) if _, err := os.Stat(everyoneMarker); err == nil { t.Error("the sandbox wrote outside every granted root because the path grants Everyone write; the restricted-SID list is satisfied by a SID every principal carries") } else if !os.IsNotExist(err) { @@ -491,6 +488,26 @@ func TestWindowsRestrictedTokenDeniesWritesToEveryoneWritablePaths(t *testing.T) } } +// deniedWriteExitCode is chosen so a denial cannot be confused with the runner +// failing to start the command at all. +// +// The obvious spelling of these assertions is `echo leaked>path` expecting exit +// 1, but the runner itself exits 1 for its own errors — a failed marker +// validation, a bad argument, a token it could not build. A test written that +// way passes both when the sandbox denies the write and when nothing ever ran, +// and the second case proves nothing while looking identical to success. On a +// test whose whole job is catching a confinement regression, that is a failure +// mode to design out rather than hope about. +// +// 77 is arbitrary beyond being outside the range the runner produces for itself. +const deniedWriteExitCode = 77 + +// deniedWriteCommand attempts a write and reports deniedWriteExitCode when the +// redirect is refused, so the exit code also proves cmd.exe actually ran. +func deniedWriteCommand(marker string) []string { + return []string{"cmd.exe", "/d", "/s", "/c", "echo leaked>" + marker + " || exit " + strconv.Itoa(deniedWriteExitCode)} +} + func powershellSingleQuote(value string) string { out := "'" for _, r := range value {