fix(sandbox): keep Windows restricted-token SIDs narrow and fail closed on DenyRead

internal/sandbox/manager_test.go

@@ -204,12 +204,19 @@ func TestSandboxManagerBuildsCommandPlanThroughWindowsRunner(t *testing.T) {
 	windowsSandboxInitialized = func() bool { return true }
 	backend := Backend{Name: BackendWindowsRestrictedToken, Available: true, Executable: `C:\zero\zero-windows-command-runner.exe`, Platform: "windows"}
 	policy := DefaultPolicy()
+	// Build the usual restricted FS profile, then clear DenyRead. On non-Windows
+	// hosts PermissionProfileFromPolicy injects credential-store DenyRead paths,
+	// and the Windows plan path rejects any non-empty DenyRead (PR #640). This
+	// happy-path plan must exercise a valid restricted profile without DenyRead;
+	// rejection coverage lives in TestSandboxManagerRejectsWindowsDenyReadOnBothRestrictedTokenTiers.
+	profile := PermissionProfileFromPolicy(`C:\workspace`, policy, nil)
+	profile.FileSystem.DenyRead = nil
 	manager := NewSandboxManager(SandboxManagerOptions{GOOS: "windows", Backend: backend})
 	plan, err := manager.BuildCommandPlan(SandboxManagerRequest{
 		WorkspaceRoot:     `C:\workspace`,
 		Command:           CommandSpec{Name: "cmd.exe", Args: []string{"/d", "/s", "/c", "dir"}, Dir: `C:\workspace\src`, Env: []string{"PATH=C:\\Tools", "TERM=xterm"}},
 		Policy:            policy,
-		Profile:           PermissionProfileFromPolicy(`C:\workspace`, policy, nil),
+		Profile:           profile,
 		Preference:        SandboxPreferenceAuto,
 		ValidateExecution: true,
 	})
@@ -243,6 +250,87 @@ func TestSandboxManagerBuildsCommandPlanThroughWindowsRunner(t *testing.T) {
 	}
 }
 
+// TestSandboxManagerRejectsWindowsDenyReadOnBothRestrictedTokenTiers is the
+// regression for PR #640: DenyRead cannot be launched or provisioned through
+// either the elevated restricted-token path or the unelevated auto fallback.
+// Both build the same fully restricted narrow-SID token.
+func TestSandboxManagerRejectsWindowsDenyReadOnBothRestrictedTokenTiers(t *testing.T) {
+	backend := Backend{Name: BackendWindowsRestrictedToken, Available: true, Executable: `C:\zero\zero-windows-command-runner.exe`, Platform: "windows"}
+	manager := NewSandboxManager(SandboxManagerOptions{GOOS: "windows", Backend: backend})
+	policy := DefaultPolicy()
+	profile := PermissionProfile{
+		FileSystem: FileSystemPolicy{
+			Kind:       FileSystemRestricted,
+			WriteRoots: []WritableRoot{{Root: `C:\workspace`}},
+			DenyRead:   []string{`C:\workspace\secret`},
+		},
+		Network: NetworkPolicy{Mode: NetworkDeny},
+	}
+	cmd := CommandSpec{Name: "cmd.exe", Args: []string{"/c", "dir"}, Dir: `C:\workspace`}
+
+	t.Run("elevated_restricted_token", func(t *testing.T) {
+		restore := windowsSandboxInitialized
+		t.Cleanup(func() { windowsSandboxInitialized = restore })
+		windowsSandboxInitialized = func() bool { return true }
+
+		_, err := manager.BuildCommandPlan(SandboxManagerRequest{
+			WorkspaceRoot:     `C:\workspace`,
+			Command:           cmd,
+			Policy:            policy,
+			Profile:           profile,
+			Preference:        SandboxPreferenceAuto,
+			ValidateExecution: true,
+		})
+		if err == nil {
+			t.Fatal("expected BuildCommandPlan error for elevated restricted-token DenyRead")
+		}
+		msg := err.Error()
+		for _, want := range []string{"DenyRead", "not supported", "restricted-token"} {
+			if !strings.Contains(msg, want) {
+				t.Fatalf("error %q missing %q", msg, want)
+			}
+		}
+	})
+
+	t.Run("unelevated_auto_fallback", func(t *testing.T) {
+		restore := windowsSandboxInitialized
+		t.Cleanup(func() { windowsSandboxInitialized = restore })
+		windowsSandboxInitialized = func() bool { return false }
+
+		req, err := manager.BuildExecutionRequest(SandboxManagerRequest{
+			WorkspaceRoot:     `C:\workspace`,
+			Command:           cmd,
+			Policy:            policy,
+			Profile:           profile,
+			Preference:        SandboxPreferenceAuto,
+			ValidateExecution: true,
+		})
+		if err != nil {
+			t.Fatalf("BuildExecutionRequest: %v", err)
+		}
+		if req.EnforcementLevel != EnforcementUnelevated {
+			t.Fatalf("EnforcementLevel = %v, want unelevated auto fallback before DenyRead rejection", req.EnforcementLevel)
+		}
+		_, err = manager.BuildCommandPlan(SandboxManagerRequest{
+			WorkspaceRoot:     `C:\workspace`,
+			Command:           cmd,
+			Policy:            policy,
+			Profile:           profile,
+			Preference:        SandboxPreferenceAuto,
+			ValidateExecution: true,
+		})
+		if err == nil {
+			t.Fatal("expected BuildCommandPlan error for unelevated DenyRead")
+		}
+		if !strings.Contains(err.Error(), "DenyRead") || !strings.Contains(err.Error(), "not supported") {
+			t.Fatalf("unelevated DenyRead error = %v", err)
+		}
+		if strings.Contains(err.Error(), "Use `--sandbox forbid`, the unelevated") {
+			t.Fatalf("error still recommends unelevated as a workaround: %v", err)
+		}
+	})
+}
+
 func TestSandboxManagerDegradesUnavailableCommandPlan(t *testing.T) {
 	policy := DefaultPolicy()
 	backend := Backend{Name: BackendUnavailable, Platform: "windows", Fallback: true, Message: "native sandbox unavailable"}

internal/sandbox/profile.go

@@ -224,10 +224,11 @@ type credentialDenyPaths struct {
 // the preserved caller environment and Zero's own config/token stores. Four
 // deliberate limits:
 //
-//   - Windows is skipped: a non-empty profile DenyRead switches the Windows
-//     runner onto the capability-SID/ACL deny path and away from the
-//     WRITE_RESTRICTED token, which the unelevated tier depends on. Revisit
-//     once the Windows deny-read model is settled.
+//   - Windows is skipped: a non-empty profile DenyRead is unsupported on both
+//     restricted-token runner levels under the narrow SID set (PR #640). The
+//     fully restricted token cannot load ordinary system binaries without
+//     Users/AuthUsers, and adding those groups reopens write grants outside
+//     WriteRoots. Revisit once access-time confinement exists.
 //   - A candidate nested under a user-configured AllowRead entry is dropped,
 //     so `allowRead: ["~/.aws"]` remains an explicit opt-out.
 //   - Candidates are emitted whether or not they currently exist on disk.

internal/sandbox/runner_windows_integration_test.go

@@ -73,6 +73,24 @@ func TestWindowsRestrictedTokenRealSandboxSmoke(t *testing.T) {
 		t.Fatalf("sandboxed write marker = %q, %v; want ok", bytes, err)
 	}
 
+	// SID broadening is disabled, so the restricted-SID list never includes
+	// Users/Authenticated Users. The write grant those groups hold on
+	// C:\Users\Public must not be reachable through the restricted-SID check.
+	// Pin that a write there fails: an independent shared-writable directory
+	// outside every workspace write root.
+	publicDir := os.Getenv("PUBLIC")
+	if publicDir == "" {
+		t.Log("PUBLIC is not set; skipping C:\\Users\\Public write-jail probe")
+	} else {
+		publicProbe := allocateSharedDirectoryProbe(t, publicDir, "elevated-public")
+		runWindowsRealSmokeCommand(t, runnerExe, config, deniedWriteCommand(publicProbe.Path()), deniedWriteExitCode)
+		if _, err := os.Stat(publicProbe.Path()); err == nil {
+			t.Fatalf("Windows sandbox allowed a write to the shared C:\\Users\\Public directory")
+		} else if !os.IsNotExist(err) {
+			t.Fatalf("stat public marker: %v", err)
+		}
+	}
+
 	listener, err := net.Listen("tcp4", "127.0.0.1:0")
 	if err != nil {
 		t.Fatalf("listen loopback for Windows network smoke: %v", err)
@@ -156,12 +174,14 @@ func TestWindowsUnelevatedRealSandboxSmoke(t *testing.T) {
 	}
 
 	sandboxHome := filepath.Join(root, ".zero-sandbox")
+	// Success path: restricted FS write-jail with no DenyRead. Non-empty DenyRead
+	// is unsupported on both restricted-token tiers under the narrow SID set
+	// (PR #640); the rejection probe below covers that separately.
 	profile := PermissionProfile{
 		FileSystem: FileSystemPolicy{
 			Kind:                 FileSystemRestricted,
 			ReadRoots:            []string{root},
 			WriteRoots:           []WritableRoot{{Root: root, ProtectedMetadataNames: []string{".git", ".zero", ".agents"}}},
-			DenyRead:             []string{privateDir},
 			IncludePlatformRoots: true,
 			AllowTemp:            true,
 		},
@@ -190,10 +210,18 @@ func TestWindowsUnelevatedRealSandboxSmoke(t *testing.T) {
 		t.Fatalf("expected the unelevated setup marker to be recorded: %v", err)
 	}
 
-	// DenyRead check: reading from the privateDir must be blocked (exit code 1)
-	runWindowsRealSmokeCommand(t, runnerExe, config, []string{
+	// DenyRead is unsupported on both restricted-token tiers under the narrow
+	// SID set (PR #640): the runner must reject before launch rather than
+	// attempting a fully restricted token that cannot load system tools.
+	denyReadConfig := config
+	denyReadConfig.PermissionProfile.FileSystem.DenyRead = []string{privateDir}
+	runWindowsRealSmokeCommandExpectError(t, runnerExe, denyReadConfig, []string{
 		"cmd.exe", "/d", "/s", "/c", "type " + secretFile,
-	}, 1)
+	}, "DenyRead", "not supported")
+	// The secret must remain readable from the host; the sandbox never ran.
+	if data, err := os.ReadFile(secretFile); err != nil || string(data) != "super-secret" {
+		t.Fatalf("host secret file after rejected DenyRead launch: %q, %v", data, err)
+	}
 
 	outsideMarker := filepath.Join(outside, "unelevated-write-denied.txt")
 	runWindowsRealSmokeCommand(t, runnerExe, config, []string{
@@ -204,6 +232,18 @@ func TestWindowsUnelevatedRealSandboxSmoke(t *testing.T) {
 	} else if !os.IsNotExist(err) {
 		t.Fatalf("stat outside marker: %v", err)
 	}
+
+	// Verify write to C:\ProgramData is blocked
+	programData := os.Getenv("ProgramData")
+	if programData != "" {
+		programDataProbe := allocateSharedDirectoryProbe(t, programData, "unelevated-programdata")
+		runWindowsRealSmokeCommand(t, runnerExe, config, deniedWriteCommand(programDataProbe.Path()), deniedWriteExitCode)
+		if _, err := os.Stat(programDataProbe.Path()); err == nil {
+			t.Fatalf("unelevated sandbox allowed a write to ProgramData shared directory")
+		} else if !os.IsNotExist(err) {
+			t.Fatalf("stat ProgramData marker: %v", err)
+		}
+	}
 }
 
 // TestWindowsRestrictedTokenNestedPipeCapture pins the fix in
@@ -387,6 +427,34 @@ func runWindowsRealSmokeCommand(t *testing.T, runnerExe string, base WindowsSand
 	}
 }
 
+// runWindowsRealSmokeCommandExpectError runs the command runner and requires a
+// non-zero exit whose combined output contains each want substring (used for
+// explicit unsupported-mode rejections rather than sandboxed command failures).
+func runWindowsRealSmokeCommandExpectError(t *testing.T, runnerExe string, base WindowsSandboxCommandArgsOptions, command []string, wantSubstr ...string) {
+	t.Helper()
+	base.Command = command
+	args, err := BuildWindowsSandboxCommandArgs(base)
+	if err != nil {
+		t.Fatalf("BuildWindowsSandboxCommandArgs: %v", err)
+	}
+	ctx, cancel := context.WithTimeout(context.Background(), 20*time.Second)
+	defer cancel()
+	cmd := exec.CommandContext(ctx, runnerExe, args...)
+	output, err := cmd.CombinedOutput()
+	if ctx.Err() != nil {
+		t.Fatalf("Windows sandbox command timed out: %v\n%s", ctx.Err(), output)
+	}
+	if err == nil {
+		t.Fatalf("Windows sandbox command exit code = 0, want error containing %v\n%s", wantSubstr, output)
+	}
+	text := string(output)
+	for _, want := range wantSubstr {
+		if !strings.Contains(text, want) {
+			t.Fatalf("Windows sandbox command error missing %q: %v\n%s", want, err, output)
+		}
+	}
+}
+
 // The write jail must hold on a path whose DACL grants Everyone write access.
 //
 // A WRITE_RESTRICTED token runs TWO checks for a write and needs both to pass:
@@ -505,7 +573,49 @@ const deniedWriteExitCode = 77
 // deniedWriteCommand attempts a write and reports deniedWriteExitCode when the
 // redirect is refused, so the exit code also proves cmd.exe actually ran.
 func deniedWriteCommand(marker string) []string {
-	return []string{"cmd.exe", "/d", "/s", "/c", "echo leaked>" + marker + " || exit " + strconv.Itoa(deniedWriteExitCode)}
+	return []string{"cmd.exe", "/d", "/s", "/c", "echo leaked>" + cmdQuote(marker) + " || exit " + strconv.Itoa(deniedWriteExitCode)}
+}
+
+func cmdQuote(path string) string {
+	return `"` + strings.ReplaceAll(path, `"`, `\"`) + `"`
+}
+
+type sharedDirectoryProbe struct {
+	path string
+}
+
+func allocateSharedDirectoryProbe(t testing.TB, dir, prefix string) *sharedDirectoryProbe {
+	t.Helper()
+	if dir == "" {
+		t.Skip("shared directory path is not set")
+	}
+	var probePath string
+	for attempt := 0; attempt < 50; attempt++ {
+		candidate := filepath.Join(dir, fmt.Sprintf("zero-smoke-%s-%d-%d-%d.txt", prefix, os.Getpid(), time.Now().UnixNano(), attempt))
+		if _, err := os.Lstat(candidate); os.IsNotExist(err) {
+			probePath = candidate
+			break
+		}
+	}
+	if probePath == "" {
+		t.Fatalf("allocate shared directory probe in %s: failed to find unused filename", dir)
+	}
+	p := &sharedDirectoryProbe{path: probePath}
+	t.Cleanup(func() {
+		p.cleanup(t)
+	})
+	return p
+}
+
+func (p *sharedDirectoryProbe) Path() string {
+	return p.path
+}
+
+func (p *sharedDirectoryProbe) cleanup(t testing.TB) {
+	t.Helper()
+	if _, err := os.Lstat(p.path); err == nil {
+		_ = os.Remove(p.path)
+	}
 }
 
 func powershellSingleQuote(value string) string {
@@ -519,3 +629,39 @@ func powershellSingleQuote(value string) string {
 	}
 	return out + "'"
 }
+
+func TestSharedDirectoryProbeLifecycle(t *testing.T) {
+	dir := t.TempDir()
+
+	// 1. Two simultaneous allocations produce distinct non-colliding paths.
+	probe1 := allocateSharedDirectoryProbe(t, dir, "p1")
+	probe2 := allocateSharedDirectoryProbe(t, dir, "p2")
+	if probe1.Path() == probe2.Path() {
+		t.Fatalf("expected distinct probe paths, got %q and %q", probe1.Path(), probe2.Path())
+	}
+
+	// 2. Pre-existing unrelated file is never selected or deleted.
+	unrelatedFile := filepath.Join(dir, "unrelated.txt")
+	if err := os.WriteFile(unrelatedFile, []byte("preserve me"), 0o600); err != nil {
+		t.Fatalf("write unrelated file: %v", err)
+	}
+	probe3 := allocateSharedDirectoryProbe(t, dir, "p3")
+	probe3.cleanup(t)
+	if data, err := os.ReadFile(unrelatedFile); err != nil || string(data) != "preserve me" {
+		t.Fatalf("unrelated file was modified or deleted: data=%q, err=%v", data, err)
+	}
+
+	// 3. Interrupted / no-create path: cleanup on absent file succeeds quietly.
+	probe4 := allocateSharedDirectoryProbe(t, dir, "p4")
+	probe4.cleanup(t)
+
+	// 4. Unexpected write created during test is cleaned up.
+	probe5 := allocateSharedDirectoryProbe(t, dir, "p5")
+	if err := os.WriteFile(probe5.Path(), []byte("leaked"), 0o600); err != nil {
+		t.Fatalf("write probe5 file: %v", err)
+	}
+	probe5.cleanup(t)
+	if _, err := os.Lstat(probe5.Path()); !os.IsNotExist(err) {
+		t.Fatalf("expected probe5 to be cleaned up after creation, stat err=%v", err)
+	}
+}

internal/sandbox/windows_acl.go

@@ -2,6 +2,7 @@ package sandbox
 
 import (
 	"errors"
+	"fmt"
 	"path/filepath"
 	"strings"
 )
@@ -12,13 +13,28 @@ const (
 	WindowsACLAllowWrite WindowsACLAction = "allow-write"
 	WindowsACLDenyRead   WindowsACLAction = "deny-read"
 	WindowsACLDenyWrite  WindowsACLAction = "deny-write"
+	// WindowsACLRevokeCapability removes any existing ACE (allow or deny) for
+	// Capability at Path, without itself granting or denying anything (applied
+	// via SetEntriesInAclW's SET_ACCESS mode with a zero mask, not
+	// REVOKE_ACCESS — see windowsACLAccess for why). It is consumed by
+	// applyWindowsACLPlan for migration cleanup and tests; no current
+	// plan-generation path emits this action, preserving legacy-process confinement.
+	WindowsACLRevokeCapability WindowsACLAction = "revoke-capability"
 )
 
 type WindowsACLEntry struct {
-	Action      WindowsACLAction `json:"action"`
-	Path        string           `json:"path"`
-	Capability  string           `json:"capability"`
-	Materialize bool             `json:"materialize,omitempty"`
+	Action     WindowsACLAction `json:"action"`
+	Path       string           `json:"path"`
+	Capability string           `json:"capability"`
+	// NoInherit forces the applied ACE to carry no inheritance flags, even
+	// when the target is a directory. Without it, applyWindowsACLPlan makes
+	// every directory ACE inheritable (SUB_CONTAINERS_AND_OBJECTS_INHERIT),
+	// and SetNamedSecurityInfo automatically propagates any inheritable ACE
+	// down onto the target's EXISTING descendants (not just new ones it
+	// creates going forward), which is why direct-only denies must set this
+	// flag rather than rely on inheritance.
+	NoInherit   bool `json:"noInherit,omitempty"`
+	Materialize bool `json:"materialize,omitempty"`
 }
 
 type WindowsACLPlan struct {
@@ -76,6 +92,7 @@ func BuildWindowsACLPlan(config WindowsSandboxCommandConfig) (WindowsACLPlan, er
 			})
 		}
 	}
+
 	return WindowsACLPlan{Entries: dedupeWindowsACLEntries(entries)}, nil
 }
 
@@ -184,7 +201,11 @@ func dedupeWindowsACLEntries(entries []WindowsACLEntry) []WindowsACLEntry {
 		if entry.Action == "" || strings.TrimSpace(entry.Path) == "" || strings.TrimSpace(entry.Capability) == "" {
 			continue
 		}
-		key := string(entry.Action) + "\x00" + windowsCapabilityPathKey(entry.Path) + "\x00" + strings.ToLower(entry.Capability)
+		// NoInherit is part of the identity: a direct-only deny and an
+		// inheritable one on the same path/SID are different ACL shapes, and
+		// collapsing them could silently promote a deliberately non-inherited
+		// shared-path deny into an inheritable one (or vice versa).
+		key := string(entry.Action) + "\x00" + windowsCapabilityPathKey(entry.Path) + "\x00" + strings.ToLower(entry.Capability) + "\x00" + fmt.Sprintf("%t", entry.NoInherit)
 		if _, ok := seen[key]; ok {
 			continue
 		}