fix(windows): add Users and Authenticated Users SIDs to restricted token SIDs

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

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
 		}

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,

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
+}

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
+}