fix(sandbox): stop the Windows write jail honouring Everyone-granted paths

internal/sandbox/runner_windows_integration_test.go

@@ -9,6 +9,7 @@ import (
 	"os"
 	"os/exec"
 	"path/filepath"
+	"strconv"
 	"strings"
 	"testing"
 	"time"
@@ -386,6 +387,127 @@ 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, 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, 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) {
+		t.Errorf("stat the Everyone-writable marker: %v", err)
+	}
+}
+
+// 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 {

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