fix(sandbox): take Everyone out of the restricted-SID list (does not work yet)

internal/sandbox/windows_command_runner_windows.go

@@ -71,7 +71,7 @@ func runWindowsSandboxCommand(config WindowsSandboxCommandConfig, stderr io.Writ
 	// granting only the user, Administrators, and SYSTEM (msys2-runtime
 	// sigproc.cc sigproc_init -> sec_user_nih -> __sec_user), and a
 	// WRITE_RESTRICTED write check must ALSO match one of the token's
-	// restricted SIDs (logon SID, Everyone, capability SIDs). None of the
+	// restricted SIDs (logon SID and capability SIDs). None of the
 	// granted SIDs can be added to the restricted list without collapsing the
 	// write jail (each has write access nearly everywhere), so MSYS startup
 	// dies with "couldn't create signal pipe" or "CreateFileMapping <SID>.1",
@@ -88,6 +88,15 @@ func runWindowsSandboxCommand(config WindowsSandboxCommandConfig, stderr io.Writ
 	// restricted token, trading spawn capability for read-deny enforcement.
 	writeRestricted := len(config.PermissionProfile.FileSystem.DenyRead) == 0
 
+	// That strict token then needs the read capability in its SID list, because
+	// the restricted-SID check covers reads once WRITE_RESTRICTED is gone. See
+	// windowsRestrictedTokenSIDsForProfile.
+	tokenSIDs, err = windowsRestrictedTokenSIDsForProfile(tokenSIDs, config.SandboxHome, writeRestricted)
+	if err != nil {
+		fmt.Fprintln(stderr, WindowsSandboxCommandRunnerName+": "+err.Error())
+		return 1
+	}
+
 	// A provisioned sandbox principal replaces the restricted token entirely: it
 	// is a separate account, so reads outside its granted roots are denied by the
 	// filesystem rather than left open the way a same-user restricted token has

internal/sandbox/windows_restricted_sid_read_test.go

@@ -0,0 +1,134 @@
+package sandbox
+
+import (
+	"path/filepath"
+	"strings"
+	"testing"
+)
+
+// THE STRICT TOKEN MUST CARRY THE GRANT ITS READ ROOTS NAME.
+//
+// A profile with DenyRead drops WRITE_RESTRICTED, which puts reads under the
+// restricted-SID check as well. Everyone used to satisfy that check, and taking
+// it away without putting the read capability in its place leaves a token that
+// cannot open cmd.exe: the command dies at launch with an access denial that
+// says nothing about the profile.
+//
+// The ACL plan grants this same capability on every read root, so the two halves
+// have to be decided together. This pins the token half; the plan half is pinned
+// by the plan tests, and the kernel behaviour by the impersonation test beside
+// this one.
+func TestTheStrictTokenCarriesTheReadCapability(t *testing.T) {
+	home := t.TempDir()
+	want, err := WindowsReadAllowSID(home)
+	if err != nil {
+		t.Fatalf("SETUP INVALID: no read capability for %s: %v", home, err)
+	}
+
+	base := []string{"S-1-15-3-1024-workspace"}
+	got, err := windowsRestrictedTokenSIDsForProfile(base, home, false)
+	if err != nil {
+		t.Fatalf("windowsRestrictedTokenSIDsForProfile: %v", err)
+	}
+	if !carriesSID(got, want) {
+		t.Fatalf("the strict token's restricted SIDs %v omit the read capability %s, so the command cannot open its own executable", got, want)
+	}
+	if !carriesSID(got, base[0]) {
+		t.Fatalf("the workspace capability was dropped: %v", got)
+	}
+}
+
+// And the WRITE_RESTRICTED token does not get it: reads are already unrestricted
+// there, so adding a SID would widen the write jail for nothing.
+func TestTheWriteRestrictedTokenDoesNotCarryTheReadCapability(t *testing.T) {
+	home := t.TempDir()
+	readSID, err := WindowsReadAllowSID(home)
+	if err != nil {
+		t.Fatalf("SETUP INVALID: no read capability for %s: %v", home, err)
+	}
+	got, err := windowsRestrictedTokenSIDsForProfile([]string{"S-1-15-3-1024-workspace"}, home, true)
+	if err != nil {
+		t.Fatalf("windowsRestrictedTokenSIDsForProfile: %v", err)
+	}
+	if carriesSID(got, readSID) {
+		t.Fatalf("the WRITE_RESTRICTED token carries the read capability %s, widening the write jail to every read root", readSID)
+	}
+}
+
+// Not containsSID: #886 adds a package-level helper by that name, and the two
+// branches would stop compiling the moment both land.
+func carriesSID(values []string, want string) bool {
+	for _, value := range values {
+		if strings.EqualFold(value, want) {
+			return true
+		}
+	}
+	return false
+}
+
+// THE PLAN AND THE TOKEN DECIDE THE READ CAPABILITY SEPARATELY.
+//
+// The ACL plan asks whether the profile configures DenyRead; the runner asks
+// whether the token keeps WRITE_RESTRICTED. Both are read off the same field
+// today, in two files, with nothing tying them together. Drift either way is
+// silent and bad: a token carrying a SID no DACL names cannot open its own
+// executable, and a plan granting a SID no token carries leaves reads confined
+// by nothing.
+//
+// So this pins the equivalence rather than either half.
+func TestThePlanAndTheTokenAgreeOnTheReadCapability(t *testing.T) {
+	for _, testCase := range []struct {
+		name     string
+		denyRead []string
+	}{
+		{name: "with denyRead", denyRead: []string{filepath.Join("secrets")}},
+		{name: "without denyRead"},
+	} {
+		t.Run(testCase.name, func(t *testing.T) {
+			workspace := t.TempDir()
+			config := WindowsSandboxCommandConfig{
+				SandboxHome:    t.TempDir(),
+				CommandCWD:     workspace,
+				WorkspaceRoots: []string{workspace},
+				PermissionProfile: PermissionProfile{
+					FileSystem: FileSystemPolicy{
+						Kind:       FileSystemRestricted,
+						WriteRoots: []WritableRoot{{Root: workspace}},
+						ReadRoots:  []string{workspace},
+						DenyRead:   prefixEach(workspace, testCase.denyRead),
+					},
+				},
+			}
+
+			planned, err := windowsReadAllowCapabilitySID(config)
+			if err != nil {
+				t.Fatalf("windowsReadAllowCapabilitySID: %v", err)
+			}
+			writeRestricted := len(config.PermissionProfile.FileSystem.DenyRead) == 0
+			tokenSIDs, err := windowsRestrictedTokenSIDsForProfile(nil, config.SandboxHome, writeRestricted)
+			if err != nil {
+				t.Fatalf("windowsRestrictedTokenSIDsForProfile: %v", err)
+			}
+
+			inPlan := planned != ""
+			inToken := len(tokenSIDs) > 0
+			if inPlan != inToken {
+				t.Fatalf("the plan grants the read capability = %t but the token carries it = %t; one side confines reads the other side never checks", inPlan, inToken)
+			}
+			if inPlan && !carriesSID(tokenSIDs, planned) {
+				t.Fatalf("the plan grants %s but the token carries %v, so the command cannot read what setup allowed", planned, tokenSIDs)
+			}
+		})
+	}
+}
+
+func prefixEach(root string, relatives []string) []string {
+	if len(relatives) == 0 {
+		return nil
+	}
+	out := make([]string, 0, len(relatives))
+	for _, relative := range relatives {
+		out = append(out, filepath.Join(root, relative))
+	}
+	return out
+}

internal/sandbox/windows_runner.go

@@ -727,6 +727,31 @@ func randomWindowsCapabilitySID() string {
 	return fmt.Sprintf("S-1-5-21-%d-%d-%d-%d", words[0], words[1], words[2], words[3])
 }
 
+// windowsRestrictedTokenSIDsForProfile adds the read capability to a restricted
+// token's SID list when the profile selected the strict token.
+//
+// WRITE_RESTRICTED scopes the restricted-SID check to writes. Without it the
+// check applies to reads as well, so the token needs a restricted SID that the
+// read roots' DACLs name or the command cannot open its own executable. It used
+// to be Everyone, which is a key to every object whose DACL grants Everyone
+// write and so handed back the write jail (#869). BuildWindowsACLPlan grants
+// this capability on every read root and denies it on every DenyRead path, so
+// the read allowance and the restriction are the same decision.
+//
+// Only elevated setup can grant it on the volume root the production profile
+// seeds; the unelevated tier refuses a DenyRead profile up front for that
+// reason, so nothing reaches here expecting a grant nobody applied.
+func windowsRestrictedTokenSIDsForProfile(tokenSIDs []string, sandboxHome string, writeRestricted bool) ([]string, error) {
+	if writeRestricted {
+		return tokenSIDs, nil
+	}
+	readSID, err := WindowsReadAllowSID(sandboxHome)
+	if err != nil {
+		return nil, fmt.Errorf("resolve the sandbox read capability: %w", err)
+	}
+	return append(tokenSIDs, readSID), nil
+}
+
 // WindowsReadAllowSID returns the sandbox home's read-capability SID, minting and
 // persisting it on first use. Both halves of the setup protocol ask for it: the
 // capability ACL plan grants it on every read root, and the principal's strict

internal/sandbox/windows_token_windows.go

@@ -142,50 +142,35 @@ func createWindowsRestrictedTokenFromBase(base windows.Token, capabilitySIDs []w
 	}
 	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.
+	// NO UNIVERSAL GROUP IS EVER A RESTRICTED SID HERE (#869).
 	//
-	// 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.
+	// A restricted SID is a key to every object whose DACL names it, which is why
+	// the runner refuses to add the user SID, Administrators or SYSTEM: each has
+	// write access nearly everywhere. The World SID (S-1-1-0, Everyone) was the
+	// broadest of the lot and used to be added to the token that does NOT carry
+	// WRITE_RESTRICTED. Every principal carries Everyone, so the restricted-SID
+	// check passed for free on any path whose DACL grants Everyone write, and the
+	// workspace write jail fell back to the ordinary user's own permissions. No
+	// privilege, no symlink and no race was needed: an Everyone-writable directory
+	// was enough, and share roots opened with Everyone:F and loose installer ACLs
+	// supply them.
 	//
-	// 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.
+	// It could not simply be dropped while it was load-bearing. Without
+	// WRITE_RESTRICTED the restricted-SID check applies to reads as well, and
+	// default Windows DACLs grant BUILTIN\Users rather than anything in this
+	// list, so a token without Everyone could not open cmd.exe and died at launch
+	// with STATUS_ACCESS_DENIED. The caller now supplies the read capability for
+	// exactly that case, which names only the roots setup granted, so the read
+	// side is satisfied without handing out a key to every Everyone-writable
+	// object on the machine.
 	//
 	// 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})
-	}
-
+	// Anything added here needs the same scrutiny. Authenticated Users, Users,
+	// INTERACTIVE and BATCH would each reintroduce this bypass on a DACL naming
+	// them.
 	// WRITE_RESTRICTED scopes the restricted-SID check to write-type accesses:
 	// reads use only the normal token identity, so the sandboxed process can
 	// open executables, DLLs, and per-user config the user can read, while