From e63a12c8444c8e93b7c18c0e5e695728d84782a6 Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Mon, 27 Jul 2026 10:18:33 +0530 Subject: [PATCH 1/8] feat(sandbox): give each workspace an offline and an online principal The principal backend stood down whenever the network was denied, which is the default, so opting into it left the restricted-token path doing all the work in normal use. The reason was that network denial is enforced by block filters keyed to the offline-marker SID, and a principal token cannot carry it: LogonUser builds a token from an account's real group memberships, and the marker is a synthetic capability SID. A real local group closes that gap. ZeroSandboxOffline is created by setup, the block filters name its SID alongside the marker, and a principal is denied the network by being a member. Each workspace therefore gets two accounts that differ only in that membership, and the command's network mode selects between them. A group rather than each principal's own SID because principals are per workspace: one filter set covers every offline principal on the machine instead of needing a filter per workspace. Both principals are provisioned together even though a given setup run sees one profile, because setup needs elevation and commands do not. Provisioning lazily would mean an unelevated command discovering it needs an account it cannot create. They also get identical filesystem access, so an approved network command sees the same filesystem as an ordinary one. Two orderings are load bearing. The network plan is now built AFTER provisioning, because the group it keys to is created there; planning first installed filters naming only the marker and left every offline principal with an open network while looking correctly set up. And the role tag sits before the workspace hash in the account name, so truncation at the 20 character limit eats hash characters rather than the tag, which would otherwise collide the two roles onto one account on exactly the workspaces most likely to truncate. Anything that is not an explicit allow maps to the offline principal, so an unrecognised mode loses the network rather than keeping it. The filter identity set is resolved rather than assumed, and stays absent until the group exists, so a machine that never provisions principals computes the same plan as before. That matters because the plan is hashed into the setup marker and re-derived on every command; an identity set that differed between setup and the command path would fail every command as out of date. Cost worth stating: this doubles the sandbox accounts on a machine, to two per workspace. --- .../windows_identity_policy_windows_test.go | 22 +- ...identity_privilege_recheck_windows_test.go | 6 +- .../windows_identity_rollback_windows_test.go | 30 ++- .../windows_identity_runtime_windows.go | 173 ++++++++++------ .../windows_identity_runtime_windows_test.go | 42 ++-- internal/sandbox/windows_identity_windows.go | 191 ++++++++++++++---- .../sandbox/windows_identity_windows_test.go | 65 ++++-- internal/sandbox/windows_network.go | 33 ++- internal/sandbox/windows_network_test.go | 68 +++++++ internal/sandbox/windows_setup_windows.go | 25 ++- .../windows_stale_secret_windows_test.go | 4 +- 11 files changed, 490 insertions(+), 169 deletions(-) diff --git a/internal/sandbox/windows_identity_policy_windows_test.go b/internal/sandbox/windows_identity_policy_windows_test.go index f1137cebd..3a7dcd42a 100644 --- a/internal/sandbox/windows_identity_policy_windows_test.go +++ b/internal/sandbox/windows_identity_policy_windows_test.go @@ -115,7 +115,7 @@ func findPrincipalACLEntry(plan WindowsACLPlan, action WindowsACLAction, path st // read the absent secret as "not provisioned" and quietly fell back to the // weaker backend. func TestProvisionWindowsSandboxIdentityDefersPasswordRotation(t *testing.T) { - stubWindowsProvisioning(t, true, nil, nil) + stubWindowsProvisioning(t, true, nil, nil, nil) rotated := false previous := resetWindowsSandboxUserPasswordFn @@ -125,7 +125,7 @@ func TestProvisionWindowsSandboxIdentityDefersPasswordRotation(t *testing.T) { return nil } - if _, _, _, err := provisionWindowsSandboxIdentity("workspacekey"); err != nil { + if _, _, _, err := provisionWindowsSandboxIdentity("workspacekey", windowsSandboxRoleOffline); err != nil { t.Fatalf("provisioning an adopted account: %v", err) } if rotated { @@ -147,7 +147,7 @@ func TestWindowsSandboxUserCommentDistinguishesWorkspaces(t *testing.T) { } // The names DO collide, which is the whole reason the comment has to carry // the key. If this stops being true the test is no longer covering anything. - if windowsSandboxUserName("aaaaaaaaaaaabbbbbbbb") != windowsSandboxUserName("aaaaaaaaaaaacccccccc") { + if windowsSandboxUserName("aaaaaaaaaaaabbbbbbbb", windowsSandboxRoleOffline) != windowsSandboxUserName("aaaaaaaaaaaacccccccc", windowsSandboxRoleOffline) { t.Skip("account names no longer collide for these keys; revisit what this test is for") } } @@ -168,7 +168,7 @@ func TestSetupRollbackRevokesRightsOnlyForCreatedPrincipals(t *testing.T) { "created principal": {existed: false, wantRevoked: true}, } { t.Run(name, func(t *testing.T) { - stubWindowsProvisioning(t, testCase.existed, nil, nil) + stubWindowsProvisioning(t, testCase.existed, nil, nil, nil) revoked := false prevGrant, prevRevoke := grantWindowsSandboxLogonRightsFn, revokeWindowsSandboxLogonRightsFn @@ -189,7 +189,7 @@ func TestSetupRollbackRevokesRightsOnlyForCreatedPrincipals(t *testing.T) { SandboxHome: t.TempDir(), WorkspaceRoots: []string{`C:\ws`}, } - if _, _, err := provisionWindowsSandboxPrincipalForSetup(config); err == nil { + if _, _, err := provisionWindowsSandboxPrincipalForSetup(config, windowsSandboxRoleOffline); err == nil { t.Fatal("provisioning reported success despite an injected grant failure") } if revoked != testCase.wantRevoked { @@ -246,7 +246,7 @@ func TestLookupWindowsSandboxIdentityRejectsForeignWorkspace(t *testing.T) { askedKey = workspaceKey return false, nil } - _, err := lookupWindowsSandboxIdentity("workspacekey") + _, err := lookupWindowsSandboxIdentity("workspacekey", windowsSandboxRoleOffline) if err == nil { t.Fatal("lookup accepted an account belonging to another workspace") } @@ -267,7 +267,7 @@ func TestLookupWindowsSandboxIdentityAbsentAccountIsUnavailable(t *testing.T) { t.Fatal("ownership must not be consulted for an account that does not resolve") return false, nil } - if _, err := lookupWindowsSandboxIdentity("nosuchworkspacekey"); !errors.Is(err, errWindowsSandboxIdentityUnavailable) { + if _, err := lookupWindowsSandboxIdentity("nosuchworkspacekey", windowsSandboxRoleOffline); !errors.Is(err, errWindowsSandboxIdentityUnavailable) { t.Fatalf("absent account returned %v, want errWindowsSandboxIdentityUnavailable", err) } } @@ -362,13 +362,13 @@ func TestSetupRuntimeRootWithoutWorkspaceIsNotAnError(t *testing.T) { // exists to withhold: rewriting the ACLs confining it, reading the secret locked // to the invoking user, and stopping Zero. func TestProvisionWindowsSandboxIdentityRefusesPrivilegedAccount(t *testing.T) { - stubWindowsProvisioning(t, true, nil, nil) + stubWindowsProvisioning(t, true, nil, nil, nil) previous := windowsSandboxUserIsPrivilegedFn t.Cleanup(func() { windowsSandboxUserIsPrivilegedFn = previous }) windowsSandboxUserIsPrivilegedFn = func(string) (bool, error) { return true, nil } - _, _, created, err := provisionWindowsSandboxIdentity("workspacekey") + _, _, created, err := provisionWindowsSandboxIdentity("workspacekey", windowsSandboxRoleOffline) if !errors.Is(err, errWindowsSandboxPrivilegedAccount) { t.Fatalf("provisioning adopted a privileged account, err = %v", err) } @@ -379,13 +379,13 @@ func TestProvisionWindowsSandboxIdentityRefusesPrivilegedAccount(t *testing.T) { // The ordinary adopted account is unaffected. func TestProvisionWindowsSandboxIdentityAdoptsUnprivilegedAccount(t *testing.T) { - stubWindowsProvisioning(t, true, nil, nil) + stubWindowsProvisioning(t, true, nil, nil, nil) previous := windowsSandboxUserIsPrivilegedFn t.Cleanup(func() { windowsSandboxUserIsPrivilegedFn = previous }) windowsSandboxUserIsPrivilegedFn = func(string) (bool, error) { return false, nil } - if _, _, _, err := provisionWindowsSandboxIdentity("workspacekey"); err != nil { + if _, _, _, err := provisionWindowsSandboxIdentity("workspacekey", windowsSandboxRoleOffline); err != nil { t.Fatalf("an unprivileged managed account must still be adopted: %v", err) } } diff --git a/internal/sandbox/windows_identity_privilege_recheck_windows_test.go b/internal/sandbox/windows_identity_privilege_recheck_windows_test.go index 9544b867a..796c80831 100644 --- a/internal/sandbox/windows_identity_privilege_recheck_windows_test.go +++ b/internal/sandbox/windows_identity_privilege_recheck_windows_test.go @@ -26,7 +26,7 @@ func TestLookupPrincipalForCommandRefusesAnAccountThatBecamePrivileged(t *testin return true, nil } - _, err = lookupWindowsSandboxPrincipalForCommand("workspace-key") + _, err = lookupWindowsSandboxPrincipalForCommand("workspace-key", windowsSandboxRoleOnline) if !errors.Is(err, errWindowsSandboxPrivilegedAccount) { t.Fatalf("err = %v, want errWindowsSandboxPrivilegedAccount", err) } @@ -49,7 +49,7 @@ func TestLookupPrincipalForCommandAcceptsAnUnprivilegedAccount(t *testing.T) { restoreLookupSeams(t, sid) windowsSandboxUserIsPrivilegedFn = func(string) (bool, error) { return false, nil } - identity, err := lookupWindowsSandboxPrincipalForCommand("workspace-key") + identity, err := lookupWindowsSandboxPrincipalForCommand("workspace-key", windowsSandboxRoleOnline) if err != nil { t.Fatalf("lookupWindowsSandboxPrincipalForCommand: %v", err) } @@ -72,7 +72,7 @@ func TestLookupIdentityItselfDoesNotConsultPrivilege(t *testing.T) { return true, nil } - if _, err := lookupWindowsSandboxIdentity("workspace-key"); err != nil { + if _, err := lookupWindowsSandboxIdentity("workspace-key", windowsSandboxRoleOnline); err != nil { t.Fatalf("lookupWindowsSandboxIdentity: %v", err) } } diff --git a/internal/sandbox/windows_identity_rollback_windows_test.go b/internal/sandbox/windows_identity_rollback_windows_test.go index e8593b816..68eff5e98 100644 --- a/internal/sandbox/windows_identity_rollback_windows_test.go +++ b/internal/sandbox/windows_identity_rollback_windows_test.go @@ -13,23 +13,30 @@ import ( // function can run on an ordinary machine. Every one of them needs an elevated // caller and a real local account, so without this the test would stop at the // first call and never reach the behaviour it is named for. -func stubWindowsProvisioning(t *testing.T, existed bool, groupErr error, sidErr error) { +func stubWindowsProvisioning(t *testing.T, existed bool, groupErr error, offlineErr error, sidErr error) { t.Helper() prevGroup, prevUser := ensureWindowsSandboxGroupFn, ensureWindowsSandboxUserFn + prevOfflineGroup := ensureWindowsSandboxOfflineGroupFn prevAdd, prevSID := addWindowsSandboxUserToGroupFn, resolveWindowsSandboxSIDFn prevManaged := windowsSandboxUserIsManagedFn + prevOffline := addWindowsSandboxUserToOfflineGroupFn t.Cleanup(func() { ensureWindowsSandboxGroupFn, ensureWindowsSandboxUserFn = prevGroup, prevUser + ensureWindowsSandboxOfflineGroupFn = prevOfflineGroup addWindowsSandboxUserToGroupFn, resolveWindowsSandboxSIDFn = prevAdd, prevSID windowsSandboxUserIsManagedFn = prevManaged + addWindowsSandboxUserToOfflineGroupFn = prevOffline }) ensureWindowsSandboxGroupFn = func() error { return nil } + ensureWindowsSandboxOfflineGroupFn = func() error { return nil } // Adopted accounts are ours in these tests; the ownership check is a real // syscall and would otherwise refuse before the code under test runs. windowsSandboxUserIsManagedFn = func(string, string) (bool, error) { return true, nil } ensureWindowsSandboxUserFn = func(string, string, string) (bool, error) { return existed, nil } + addWindowsSandboxUserToOfflineGroupFn = func(string) error { return offlineErr } addWindowsSandboxUserToGroupFn = func(string) error { return groupErr } + addWindowsSandboxUserToOfflineGroupFn = func(string) error { return offlineErr } resolveWindowsSandboxSIDFn = func(username string) (*windows.SID, error) { if sidErr != nil { return nil, sidErr @@ -46,19 +53,22 @@ func stubWindowsProvisioning(t *testing.T, existed bool, groupErr error, sidErr // most: it is the enforcement boundary and it can fail under local policy. func TestProvisionWindowsSandboxIdentityReturnsNameForRollback(t *testing.T) { groupFailure := errors.New("group attachment refused by policy") + offlineFailure := errors.New("offline group attachment refused by policy") sidFailure := errors.New("sid lookup failed") for name, testCase := range map[string]struct { - groupErr error - sidErr error + groupErr error + offlineErr error + sidErr error }{ - "group attachment fails": {groupErr: groupFailure}, - "sid resolution fails": {sidErr: sidFailure}, + "group attachment fails": {groupErr: groupFailure}, + "offline group attachment fails": {offlineErr: offlineFailure}, + "sid resolution fails": {sidErr: sidFailure}, } { t.Run(name, func(t *testing.T) { - stubWindowsProvisioning(t, false, testCase.groupErr, testCase.sidErr) + stubWindowsProvisioning(t, false, testCase.groupErr, testCase.offlineErr, testCase.sidErr) - identity, _, created, err := provisionWindowsSandboxIdentity("workspacekey") + identity, _, created, err := provisionWindowsSandboxIdentity("workspacekey", windowsSandboxRoleOffline) if err == nil { t.Fatal("provisioning reported success despite an injected failure") } @@ -69,7 +79,7 @@ func TestProvisionWindowsSandboxIdentityReturnsNameForRollback(t *testing.T) { if identity.Username == "" { t.Fatal("identity carries no username, so the rollback deletes \"\" and strands the account") } - if want := windowsSandboxUserName("workspacekey"); identity.Username != want { + if want := windowsSandboxUserName("workspacekey", windowsSandboxRoleOffline); identity.Username != want { t.Fatalf("username = %q, want %q", identity.Username, want) } }) @@ -80,9 +90,9 @@ func TestProvisionWindowsSandboxIdentityReturnsNameForRollback(t *testing.T) { // failed. created=false is what stops the rollback turning a partial failure // into the loss of a working principal from an earlier setup. func TestProvisionWindowsSandboxIdentityDoesNotClaimPreexistingAccount(t *testing.T) { - stubWindowsProvisioning(t, true, errors.New("group attachment refused"), nil) + stubWindowsProvisioning(t, true, errors.New("group attachment refused"), nil, nil) - _, _, created, err := provisionWindowsSandboxIdentity("workspacekey") + _, _, created, err := provisionWindowsSandboxIdentity("workspacekey", windowsSandboxRoleOffline) if err == nil { t.Fatal("provisioning reported success despite an injected failure") } diff --git a/internal/sandbox/windows_identity_runtime_windows.go b/internal/sandbox/windows_identity_runtime_windows.go index 7b1ca58a4..cbfea5aed 100644 --- a/internal/sandbox/windows_identity_runtime_windows.go +++ b/internal/sandbox/windows_identity_runtime_windows.go @@ -65,17 +65,26 @@ func windowsSandboxWorkspaceKey(workspaceRoots []string) string { // machine with nothing provisioned the lookup declines anyway, which would let a // missing guard here pass unnoticed. func windowsSandboxPrincipalEligible(config WindowsSandboxCommandConfig) bool { - if !windowsSandboxIdentityEnabled(config.Env) { - return false - } - // Network denial is enforced by WFP filters keyed to the offline-marker SID, - // which the restricted token carries and a principal token cannot: LogonUser - // mints a token for the account, not for a synthetic capability SID. Using a - // principal here would leave those filters matching nothing and silently drop - // network enforcement, which is a worse trade than the read confinement it - // buys. Fall back to the restricted token, which still enforces the network, - // until the filters are also keyed to the principal's own SID. - return config.PermissionProfile.Network.Mode != NetworkDeny + return windowsSandboxIdentityEnabled(config.Env) +} + +// windowsSandboxRoleForNetwork maps a command's network mode onto the principal +// that enforces it. +// +// The two roles differ only in membership of the offline group, which the block +// filters are keyed to. That indirection exists because network denial cannot be +// expressed on a logon token the way it is on a restricted token: the restricted +// token carries the offline-marker SID as a restricting SID, and LogonUser has +// no equivalent, since it builds a token from the account's real memberships. +// +// Defaulting anything that is not an explicit allow to the offline principal is +// deliberate. A mode this does not recognise should lose the network, not keep +// it. +func windowsSandboxRoleForNetwork(mode NetworkMode) windowsSandboxRole { + if mode == NetworkAllow { + return windowsSandboxRoleOnline + } + return windowsSandboxRoleOffline } // windowsSandboxPrincipalToken returns a token for this workspace's sandbox @@ -91,7 +100,11 @@ func windowsSandboxPrincipalToken(config WindowsSandboxCommandConfig) (windows.T return 0, false, nil } key := windowsSandboxWorkspaceKey(config.WorkspaceRoots) - identity, err := lookupWindowsSandboxPrincipalForCommand(key) + // The network mode picks the principal. Both are provisioned by setup; the + // offline one is in the group the block filters match, so choosing here is + // what enforces the mode. + role := windowsSandboxRoleForNetwork(config.PermissionProfile.Network.Mode) + identity, err := lookupWindowsSandboxPrincipalForCommand(key, role) if err != nil { if errors.Is(err, errWindowsSandboxIdentityUnavailable) { // Not provisioned: fall back quietly, this is the default state. @@ -158,9 +171,9 @@ var windowsSandboxPrincipalWarnOnce sync.Once // The password is written BEFORE the caller applies any ACL plan, so a setup // that fails partway leaves a principal that can at least be logged on and // therefore cleaned up, rather than an account nothing holds the secret for. -func provisionWindowsSandboxPrincipalForSetup(config WindowsSandboxCommandConfig) (windowsSandboxIdentity, bool, error) { +func provisionWindowsSandboxPrincipalForSetup(config WindowsSandboxCommandConfig, role windowsSandboxRole) (windowsSandboxIdentity, bool, error) { key := windowsSandboxWorkspaceKey(config.WorkspaceRoots) - identity, password, created, err := provisionWindowsSandboxIdentityFn(key) + identity, password, created, err := provisionWindowsSandboxIdentityFn(key, role) // Undo whatever this run actually did, in reverse, on any failure after the // account exists. Without it a failure between creating the account and @@ -176,7 +189,7 @@ func provisionWindowsSandboxPrincipalForSetup(config WindowsSandboxCommandConfig rotated := false // Resolved from the account name rather than the identity, so it is known // before anything can fail. - secretPath, secretPathErr := windowsSandboxSecretPath(config.SandboxHome, windowsSandboxUserName(key)) + secretPath, secretPathErr := windowsSandboxSecretPath(config.SandboxHome, windowsSandboxUserName(key, role)) undo := func() error { // Only when this run invalidated it. The secret is removed if this run // created the account, or if it rotated an existing account's password, @@ -274,71 +287,95 @@ func provisionWindowsSandboxPrincipalForSetup(config WindowsSandboxCommandConfig // naming a SID that no longer resolves, which is the orphaned-entry residue this // model exists to avoid. func setupWindowsSandboxPrincipal(config WindowsSandboxCommandConfig) (func() error, error) { - identity, created, err := provisionWindowsSandboxPrincipalForSetup(config) - if err != nil { - return nil, err - } - // Scoped to what this run created, the same contract provisioning already - // applies to its own rollback. - // - // Unconditional removal here meant a transient ACL failure during a re-run of - // elevated setup deleted a principal that was working before the run started, - // taking its secret and logon rights with it. Provisioning was careful not to - // do that and then this undid the care one level up. A pre-existing principal - // is left alone: its ACEs are still reverted, since this run applied them, - // but the account itself is not this run's to destroy. - removePrincipal := func() error { - if !created { - return nil + // Both roles are provisioned regardless of the profile this setup ran with. + // Setup needs elevation and a command does not, so a command whose network + // mode differs from the one setup happened to see must still find a principal + // waiting; provisioning lazily would mean an unelevated command discovering it + // needs an account it cannot create. + roles := []windowsSandboxRole{windowsSandboxRoleOffline, windowsSandboxRoleOnline} + + var undo []func() error + rollback := func() error { + // Unwind in reverse, and keep going after a failure so one broken step + // cannot strand everything behind it. The first error is reported. + var firstErr error + for i := len(undo) - 1; i >= 0; i-- { + if err := undo[i](); err != nil && firstErr == nil { + firstErr = err + } } - return removeWindowsSandboxPrincipalForSetup(config) + return firstErr } filesystem := config.PermissionProfile.FileSystem - writeRoots := filesystem.WriteRoots - // The runtime tree has to be granted here, at setup, because nothing grants it - // later. + // Resolved once, before the loop: both principals need write access to the + // same runtime tree. // - // permissionProfileWithRuntime appends the per-workspace runtime root to - // WriteRoots on every COMMAND, and redirects HOME, GOCACHE, npm_config_cache - // and friends into it. That root lives under the user cache, not the - // workspace, so the profile setup sees never contains it. On the - // restricted-token path that costs nothing, since the child still runs as the - // caller and already has rights there. A principal is a separate local account - // with none, so without this every npm install, go build or pip install fails - // on a cache write with a bare ACCESS_DENIED and nothing pointing at the - // sandbox as the cause. - if runtimeRoot, err := setupWindowsSandboxRuntimeRoot(config); err != nil { - _ = removePrincipal() - return nil, err - } else if runtimeRoot != "" { - writeRoots = append(append([]WritableRoot{}, writeRoots...), WritableRoot{Root: runtimeRoot}) - } - revertACL, err := applyWindowsPrincipalACLs(identity.SID.String(), filesystem, writeRoots) + // permissionProfileWithRuntime appends this root to WriteRoots on every + // COMMAND and redirects HOME, GOCACHE, npm_config_cache and friends into it, + // but it lives under the user cache rather than the workspace, so the profile + // setup sees never contains it. On the restricted-token path that costs + // nothing, since the child still runs as the caller. A principal is a separate + // local account with none of those rights, so without this every npm install, + // go build or pip install fails on a cache write with a bare ACCESS_DENIED and + // nothing pointing at the sandbox as the cause. + runtimeRoot, err := setupWindowsSandboxRuntimeRoot(config) if err != nil { - _ = removePrincipal() return nil, err } - return func() error { - aclErr := revertACL() - // Remove the principal even when the ACL revert failed, so a broken - // rollback does not also strand an account; report the ACL error since it - // is the one that leaves state behind. - removeErr := removePrincipal() - if aclErr != nil { - return aclErr + + for _, role := range roles { + identity, created, err := provisionWindowsSandboxPrincipalForSetup(config, role) + if err != nil { + _ = rollback() + return nil, err } - return removeErr - }, nil + // Only for a principal this run created. + // + // Appending unconditionally meant that if the offline role adopted an + // account that already existed and the online role, or any later step, + // then failed, the outer rollback deleted a principal that was working + // before setup started. Dual-role provisioning makes that likely rather + // than unlucky: the first role usually succeeds, so there is almost + // always something for a later failure to destroy. This mirrors the + // contract provisioning already applies to its own inner rollback. + if created { + undo = append(undo, func() error { return removeWindowsSandboxPrincipalForSetup(config, role) }) + } + + // Both principals get the same filesystem access. They differ only in + // network reach, so granting the offline one less would make an approved + // network command see a different filesystem than an ordinary one. + writeRoots := filesystem.WriteRoots + if runtimeRoot != "" { + writeRoots = append(append([]WritableRoot{}, writeRoots...), WritableRoot{Root: runtimeRoot}) + } + // Revokes this trustee's existing ACEs on the paths the plan touches + // before applying it, so a re-run after narrowing a root does not leave + // the wider grant behind. Per role: the two principals are separate + // trustees and revoking one must not disturb the other. + revertACL, err := applyWindowsPrincipalACLs(identity.SID.String(), filesystem, writeRoots) + if err != nil { + _ = rollback() + return nil, err + } + // ACEs are reverted before the account they name is deleted, because + // removing the account first leaves ACEs naming a SID that no longer + // resolves, which is the orphaned residue this model exists to avoid. + // Appending after the removal closure puts it earlier in the reverse + // unwind, which is what gets that ordering. + undo = append(undo, revertACL) + } + return rollback, nil } -// removeWindowsSandboxPrincipalForSetup retires a workspace's principal in the +// removeWindowsSandboxPrincipalForSetup retires one role's principal in the // order that leaves nothing behind: secret, then ACEs, then LSA logon rights, // then the account itself. Everything keyed to the SID has to go while the SID // still resolves. -func removeWindowsSandboxPrincipalForSetup(config WindowsSandboxCommandConfig) error { +func removeWindowsSandboxPrincipalForSetup(config WindowsSandboxCommandConfig, role windowsSandboxRole) error { key := windowsSandboxWorkspaceKey(config.WorkspaceRoots) - username := windowsSandboxUserName(key) + username := windowsSandboxUserName(key, role) secretPath, err := windowsSandboxSecretPath(config.SandboxHome, username) if err != nil { return err @@ -351,12 +388,14 @@ func removeWindowsSandboxPrincipalForSetup(config WindowsSandboxCommandConfig) e // which is the same orphaned residue the trustee-keyed ACE revocation exists // to avoid. A principal that was never provisioned has no SID to resolve and // nothing to revoke, so that case is not an error. - if identity, err := lookupWindowsSandboxIdentity(windowsSandboxWorkspaceKey(config.WorkspaceRoots)); err == nil { + if identity, err := lookupWindowsSandboxIdentity(key, role); err == nil { // ACEs first, for the same reason: once the account is gone its SID stops // resolving and every ACE naming it becomes an orphaned raw-SID entry on // the user's own tree, which is precisely the residue the capability-SID // model left behind and this one exists to avoid. Revocation is by - // trustee, so it clears grants written by older versions too. + // trustee, so it clears grants written by older versions too, and it is + // scoped to THIS role's principal — the other role is a separate trustee + // whose ACEs must survive one role being retired. // // Failing to revoke is not fatal. A path the user has since deleted or // renamed cannot be cleaned, and refusing to remove the account over it diff --git a/internal/sandbox/windows_identity_runtime_windows_test.go b/internal/sandbox/windows_identity_runtime_windows_test.go index 26f2852a8..c88f797e0 100644 --- a/internal/sandbox/windows_identity_runtime_windows_test.go +++ b/internal/sandbox/windows_identity_runtime_windows_test.go @@ -108,10 +108,10 @@ func TestWindowsSandboxWorkspaceKeyIsStableAndDistinct(t *testing.T) { // filters matching nothing, so the principal backend must stand down whenever // the network is denied rather than silently trading network enforcement for // read confinement. -// The eligibility predicate is asserted rather than the token lookup, because on -// a machine with no principal provisioned the lookup declines for its own reasons -// and would report success here whether or not the guard existed. -func TestPrincipalBackendDefersToRestrictedTokenWhenNetworkDenied(t *testing.T) { +// The backend now runs under both network modes, so the opt-in is the only thing +// eligibility turns on. The mode selects which principal is used, not whether one +// is used at all. +func TestPrincipalBackendEligibilityTurnsOnlyOnTheOptIn(t *testing.T) { eligible := func(mode NetworkMode, optIn string) bool { config := WindowsSandboxCommandConfig{ SandboxHome: t.TempDir(), @@ -122,15 +122,33 @@ func TestPrincipalBackendDefersToRestrictedTokenWhenNetworkDenied(t *testing.T) return windowsSandboxPrincipalEligible(config) } - if eligible(NetworkDeny, "1") { - t.Fatal("principal backend eligible with the network denied; the WFP filters key on the offline-marker SID, which a logon token does not carry, so egress would be unenforced") + for _, mode := range []NetworkMode{NetworkDeny, NetworkAllow} { + if !eligible(mode, "1") { + t.Fatalf("principal backend refused with network %q; the mode should choose a principal, not disable the backend", mode) + } + if eligible(mode, "0") { + t.Fatalf("principal backend eligible without the opt-in for network %q", mode) + } + } +} + +// The network mode is enforced by WHICH principal runs, because the offline one +// is a member of the group the block filters match. Picking the wrong role is +// therefore a silent loss of network enforcement, so the mapping is asserted +// directly rather than inferred from a token that only exists on a provisioned +// machine. +func TestPrincipalRoleFollowsNetworkMode(t *testing.T) { + if got := windowsSandboxRoleForNetwork(NetworkDeny); got != windowsSandboxRoleOffline { + t.Fatalf("deny selected %q, want the offline principal; the online one is not in the blocked group and would have the network", got) } - // The guard must be specific to denial, not a blanket disable that would make - // the whole backend dead code. - if !eligible(NetworkAllow, "1") { - t.Fatal("principal backend refused with the network allowed; the guard is over-broad and disables the backend entirely") + if got := windowsSandboxRoleForNetwork(NetworkAllow); got != windowsSandboxRoleOnline { + t.Fatalf("allow selected %q, want the online principal", got) } - if eligible(NetworkAllow, "0") { - t.Fatal("principal backend eligible without the opt-in") + // Fail closed. An unrecognised mode must lose the network rather than keep it, + // so anything that is not an explicit allow maps to the offline principal. + for _, mode := range []NetworkMode{NetworkMode(""), NetworkMode("bogus"), NetworkMode("ALLOW")} { + if got := windowsSandboxRoleForNetwork(mode); got != windowsSandboxRoleOffline { + t.Fatalf("unrecognised mode %q selected %q, want the offline principal so an unknown mode fails closed", mode, got) + } } } diff --git a/internal/sandbox/windows_identity_windows.go b/internal/sandbox/windows_identity_windows.go index 03504b135..174c15480 100644 --- a/internal/sandbox/windows_identity_windows.go +++ b/internal/sandbox/windows_identity_windows.go @@ -54,8 +54,48 @@ const ( windowsSandboxUserComment = "Zero sandbox principal (managed)" windowsSandboxUserCommentKey = windowsSandboxUserComment + " key=" windowsSandboxUserNameMax = 20 + + // windowsSandboxOfflineGroupName is what the network block filters are keyed + // to. Network denial cannot be expressed on a principal's own token the way + // it is on a restricted token: the block filters match the offline-marker + // SID, which is a synthetic capability SID, and LogonUser mints a token from + // an account's real group memberships rather than from arbitrary SIDs. A + // principal is therefore denied the network by being a MEMBER of this group, + // whose SID the filters also match. + // + // A group rather than the offline principal's own SID because principals are + // per workspace: one filter set covers every offline principal on the machine + // instead of needing a filter per workspace. + windowsSandboxOfflineGroupName = "ZeroSandboxOffline" + windowsSandboxOfflineGroupComment = "Zero sandbox principals denied network access (managed)" ) +// windowsSandboxRole distinguishes the two principals a workspace gets. They are +// separate accounts rather than one account reconfigured per command, because +// the network decision is baked into group membership at setup time and setup +// needs elevation; flipping it per command would need an elevated hop on every +// command. +type windowsSandboxRole string + +const ( + // windowsSandboxRoleOffline is a member of the offline group, so the block + // filters match its token and it has no network. + windowsSandboxRoleOffline windowsSandboxRole = "offline" + // windowsSandboxRoleOnline is not, so an approved network command reaches the + // network while still being read-confined by having its own identity. + windowsSandboxRoleOnline windowsSandboxRole = "online" +) + +// roleTag is the single character that distinguishes the two accounts for a +// workspace. One character because the 20-character account-name limit is +// already tight and every character spent here is a bit of workspace hash lost. +func (role windowsSandboxRole) roleTag() string { + if role == windowsSandboxRoleOnline { + return "n" + } + return "d" +} + // Win32 status codes that mean "already there". Treated as success so // provisioning converges instead of failing on a second run. const ( @@ -136,12 +176,17 @@ func (identity windowsSandboxIdentity) String() string { return identity.Username + " (" + identity.SID.String() + ")" } -// windowsSandboxUserName derives a stable account name for a workspace key. The -// key is hashed by the caller (see windowsSandboxWorkspaceKey) so the name reveals no -// path, and it is truncated to the 20-character local-account limit. The same -// workspace always maps to the same account, so re-running setup reuses the -// principal instead of accumulating accounts. -func windowsSandboxUserName(workspaceKey string) string { +// windowsSandboxUserName derives a stable account name for a workspace key and +// role. The key is hashed by the caller (see windowsSandboxWorkspaceKey) so the +// name reveals no path, and it is truncated to the 20-character local-account +// limit. The same workspace and role always map to the same account, so +// re-running setup reuses the principals instead of accumulating accounts. +// +// The role tag sits before the hash rather than after it, so truncation eats +// hash characters and never the tag. A name that lost its tag would collide the +// two roles onto one account, which would silently put an online principal in +// the offline group or the reverse. +func windowsSandboxUserName(workspaceKey string, role windowsSandboxRole) string { cleaned := strings.Map(func(r rune) rune { switch { case r >= 'a' && r <= 'z', r >= '0' && r <= '9': @@ -155,7 +200,7 @@ func windowsSandboxUserName(workspaceKey string) string { if cleaned == "" { cleaned = "default" } - name := windowsSandboxUserPrefix + cleaned + name := windowsSandboxUserPrefix + role.roleTag() + cleaned if len(name) > windowsSandboxUserNameMax { name = name[:windowsSandboxUserNameMax] } @@ -208,11 +253,54 @@ func netAPIStatus(call string, status uintptr, okStatuses ...uintptr) error { // ensureWindowsSandboxGroup creates the managed local group, or leaves it alone // when it already exists. func ensureWindowsSandboxGroup() error { - name, err := windows.UTF16PtrFromString(windowsSandboxGroupName) + return ensureWindowsLocalGroup(windowsSandboxGroupName, windowsSandboxGroupComment) +} + +// ensureWindowsSandboxOfflineGroup creates the group the network block filters +// are keyed to. Membership of it is what denies a principal the network, so it +// has to exist before any offline principal is created. +func ensureWindowsSandboxOfflineGroup() error { + return ensureWindowsLocalGroup(windowsSandboxOfflineGroupName, windowsSandboxOfflineGroupComment) +} + +// Wires the portable network planner to the Windows group lookup. Done here so +// windows_network.go can stay free of Win32 calls while still folding the group +// into the filter identity set. +func init() { + resolveWindowsSandboxOfflineGroupSIDHook = resolveWindowsSandboxOfflineGroupSID +} + +// resolveWindowsSandboxOfflineGroupSID returns the SID the block filters key to. +// +// Reports ("", nil) when the group does not exist, which is the state before +// setup has ever run. Callers fold that into "no extra identity", so the plan +// they compute matches what an un-provisioned machine would produce rather than +// failing to build at all. +func resolveWindowsSandboxOfflineGroupSID() (string, error) { + sid, _, accountType, err := windows.LookupSID("", windowsSandboxOfflineGroupName) + if err != nil { + if errors.Is(err, windows.ERROR_NONE_MAPPED) { + return "", nil + } + return "", fmt.Errorf("look up %s: %w", windowsSandboxOfflineGroupName, err) + } + // A local group is an alias. Anything else means the name has been taken by + // something that is not ours, and keying network filters to it would be both + // wrong and a way to affect unrelated accounts. + if accountType != windows.SidTypeAlias && accountType != windows.SidTypeGroup { + return "", fmt.Errorf("%s resolves to a non-group account (type %d)", windowsSandboxOfflineGroupName, accountType) + } + return sid.String(), nil +} + +// ensureWindowsLocalGroup creates a local group, or leaves it alone when it +// already exists. +func ensureWindowsLocalGroup(groupName string, groupComment string) error { + name, err := windows.UTF16PtrFromString(groupName) if err != nil { return err } - comment, err := windows.UTF16PtrFromString(windowsSandboxGroupComment) + comment, err := windows.UTF16PtrFromString(groupComment) if err != nil { return err } @@ -311,7 +399,20 @@ func resetWindowsSandboxUserPassword(username string, password string) error { // addWindowsSandboxUserToGroup puts a principal in the managed group, ignoring // the status that means it is already a member. func addWindowsSandboxUserToGroup(username string) error { - group, err := windows.UTF16PtrFromString(windowsSandboxGroupName) + return addWindowsUserToLocalGroup(windowsSandboxGroupName, username) +} + +// addWindowsSandboxUserToOfflineGroup is what actually denies a principal the +// network: the block filters match this group's SID, and LogonUser puts the +// group into the token it mints for a member. +func addWindowsSandboxUserToOfflineGroup(username string) error { + return addWindowsUserToLocalGroup(windowsSandboxOfflineGroupName, username) +} + +// addWindowsUserToLocalGroup puts an account in a local group, ignoring the +// status that means it is already a member. +func addWindowsUserToLocalGroup(groupName string, username string) error { + group, err := windows.UTF16PtrFromString(groupName) if err != nil { return err } @@ -501,19 +602,21 @@ func resolveWindowsSandboxSID(username string) (*windows.SID, error) { // post-creation pair would never get past ensureWindowsSandboxGroup on an // ordinary machine and would pass without reaching the code it names. var ( - ensureWindowsSandboxGroupFn = ensureWindowsSandboxGroup - ensureWindowsSandboxUserFn = ensureWindowsSandboxUser - addWindowsSandboxUserToGroupFn = addWindowsSandboxUserToGroup - resolveWindowsSandboxSIDFn = resolveWindowsSandboxSID - resetWindowsSandboxUserPasswordFn = resetWindowsSandboxUserPassword - windowsSandboxUserIsManagedFn = windowsSandboxUserIsManaged - windowsSandboxUserIsPrivilegedFn = windowsSandboxUserIsPrivileged - grantWindowsSandboxLogonRightsFn = grantWindowsSandboxLogonRights - revokeWindowsSandboxLogonRightsFn = revokeWindowsSandboxLogonRights + ensureWindowsSandboxGroupFn = ensureWindowsSandboxGroup + ensureWindowsSandboxOfflineGroupFn = ensureWindowsSandboxOfflineGroup + ensureWindowsSandboxUserFn = ensureWindowsSandboxUser + addWindowsSandboxUserToGroupFn = addWindowsSandboxUserToGroup + addWindowsSandboxUserToOfflineGroupFn = addWindowsSandboxUserToOfflineGroup + resolveWindowsSandboxSIDFn = resolveWindowsSandboxSID + resetWindowsSandboxUserPasswordFn = resetWindowsSandboxUserPassword + windowsSandboxUserIsManagedFn = windowsSandboxUserIsManaged + windowsSandboxUserIsPrivilegedFn = windowsSandboxUserIsPrivileged + grantWindowsSandboxLogonRightsFn = grantWindowsSandboxLogonRights + revokeWindowsSandboxLogonRightsFn = revokeWindowsSandboxLogonRights // applyWindowsACLPlanFn is a seam so a test can pin the ORDER of setup's ACL - // work. The revocation below only prevents a stale grant if it runs before - // the plan that re-adds the current one; a test that exercised the revoke - // helper on its own would pass just as happily with the call site deleted. + // work. The revocation only prevents a stale grant if it runs before the plan + // that re-adds the current one; a test that exercised the revoke helper on its + // own would pass just as happily with the call site deleted. applyWindowsACLPlanFn = applyWindowsACLPlan ) @@ -527,11 +630,17 @@ var ( // live. The returned value is always the account's actual password, including // when the account already existed, because that case is reset explicitly // below. -func provisionWindowsSandboxIdentity(workspaceKey string) (windowsSandboxIdentity, string, bool, error) { +func provisionWindowsSandboxIdentity(workspaceKey string, role windowsSandboxRole) (windowsSandboxIdentity, string, bool, error) { if err := ensureWindowsSandboxGroupFn(); err != nil { return windowsSandboxIdentity{}, "", false, err } - username := windowsSandboxUserName(workspaceKey) + // The offline group has to exist before an offline principal joins it, and it + // is created unconditionally so the network filters can be keyed to its SID + // whether or not an offline principal has been provisioned yet. + if err := ensureWindowsSandboxOfflineGroupFn(); err != nil { + return windowsSandboxIdentity{}, "", false, err + } + username := windowsSandboxUserName(workspaceKey, role) password, err := newWindowsSandboxPassword() if err != nil { return windowsSandboxIdentity{}, "", false, err @@ -596,6 +705,19 @@ func provisionWindowsSandboxIdentity(workspaceKey string) (windowsSandboxIdentit if err := addWindowsSandboxUserToGroupFn(username); err != nil { return windowsSandboxIdentity{Username: username}, "", !existed, err } + // Membership of the offline group IS the network denial, so it is applied + // here rather than left to a later step: a principal that reached the runner + // without it would look correctly provisioned and quietly have the network. + // + // This is the failure most worth surviving cleanly. It happens after the + // account exists, and local policy can refuse a group join, so the name has + // to come back with the error or the rollback deletes "" and strands a + // principal that has network access and no offline membership. + if role == windowsSandboxRoleOffline { + if err := addWindowsSandboxUserToOfflineGroupFn(username); err != nil { + return windowsSandboxIdentity{Username: username}, "", !existed, err + } + } sid, err := resolveWindowsSandboxSIDFn(username) if err != nil { return windowsSandboxIdentity{Username: username}, "", !existed, err @@ -629,21 +751,8 @@ var errWindowsSandboxIdentityUnavailable = errors.New("no Zero sandbox principal // creating anything, so the unelevated command path can discover whether an // identity exists. It returns errWindowsSandboxIdentityUnavailable when setup // has not run. -func lookupWindowsSandboxIdentity(workspaceKey string) (windowsSandboxIdentity, error) { - username := windowsSandboxUserName(workspaceKey) - // Ownership is checked here as well as at provisioning, because the account - // NAME cannot carry the whole workspace key. - // - // The name keeps 11 characters of the digest; the comment holds all of it. - // Provisioning refuses a name whose comment names a different workspace, and - // without the same check here the workspace that LOST that race would still - // resolve the name to a SID and quietly use the other workspace's principal, - // its secret and its ACL identity. Setup would have failed for it, so this is - // the path that decides whether the refusal actually holds. - // - // A collision is very unlikely with real keys, roughly 2^-44 per pair, but the - // cost of being wrong is one workspace running as another's identity, and the - // check is one syscall on a path that is already doing several. +func lookupWindowsSandboxIdentity(workspaceKey string, role windowsSandboxRole) (windowsSandboxIdentity, error) { + username := windowsSandboxUserName(workspaceKey, role) // SID resolution runs FIRST so "no such account" stays the unavailable // sentinel. windowsSandboxUserIsManaged answers false for both an absent // account and one belonging to someone else, so checking it before this would @@ -678,8 +787,8 @@ func lookupWindowsSandboxIdentity(workspaceKey string) (windowsSandboxIdentity, // stay able to clean up an account that has become privileged rather than // refusing to touch it. Refusing there would leave the very account this guards // against permanently undeletable by Zero. -func lookupWindowsSandboxPrincipalForCommand(workspaceKey string) (windowsSandboxIdentity, error) { - identity, err := lookupWindowsSandboxIdentity(workspaceKey) +func lookupWindowsSandboxPrincipalForCommand(workspaceKey string, role windowsSandboxRole) (windowsSandboxIdentity, error) { + identity, err := lookupWindowsSandboxIdentity(workspaceKey, role) if err != nil { return windowsSandboxIdentity{}, err } diff --git a/internal/sandbox/windows_identity_windows_test.go b/internal/sandbox/windows_identity_windows_test.go index 11d64c741..da743035b 100644 --- a/internal/sandbox/windows_identity_windows_test.go +++ b/internal/sandbox/windows_identity_windows_test.go @@ -16,7 +16,7 @@ import ( // A local Windows account name is capped at 20 characters, so the derived name // must truncate rather than produce a name NetUserAdd rejects. func TestWindowsSandboxUserNameRespectsLengthLimit(t *testing.T) { - name := windowsSandboxUserName(strings.Repeat("a", 64)) + name := windowsSandboxUserName(strings.Repeat("a", 64), windowsSandboxRoleOffline) if len(name) > windowsSandboxUserNameMax { t.Fatalf("name %q is %d chars, want at most %d", name, len(name), windowsSandboxUserNameMax) } @@ -28,12 +28,12 @@ func TestWindowsSandboxUserNameRespectsLengthLimit(t *testing.T) { // The same workspace must map to the same principal, otherwise re-running setup // would accumulate a new local account every time. func TestWindowsSandboxUserNameIsStable(t *testing.T) { - first := windowsSandboxUserName("abc123") - second := windowsSandboxUserName("abc123") + first := windowsSandboxUserName("abc123", windowsSandboxRoleOffline) + second := windowsSandboxUserName("abc123", windowsSandboxRoleOffline) if first != second { t.Fatalf("name is not stable: %q vs %q", first, second) } - if other := windowsSandboxUserName("def456"); other == first { + if other := windowsSandboxUserName("def456", windowsSandboxRoleOffline); other == first { t.Fatalf("different workspaces produced the same principal %q", first) } } @@ -41,7 +41,7 @@ func TestWindowsSandboxUserNameIsStable(t *testing.T) { // The key is sanitised to characters a local account name accepts, so a hash or // path fragment cannot smuggle a separator or a space into the name. func TestWindowsSandboxUserNameRejectsUnsafeCharacters(t *testing.T) { - name := windowsSandboxUserName(`C:\Users\me\proj ect`) + name := windowsSandboxUserName(`C:\Users\me\proj ect`, windowsSandboxRoleOffline) for _, r := range strings.TrimPrefix(name, windowsSandboxUserPrefix) { isLower := r >= 'a' && r <= 'z' isDigit := r >= '0' && r <= '9' @@ -58,7 +58,7 @@ func TestWindowsSandboxUserNameRejectsUnsafeCharacters(t *testing.T) { // than the bare prefix. func TestWindowsSandboxUserNameHandlesEmptyKey(t *testing.T) { for _, key := range []string{"", "///", " "} { - if got := windowsSandboxUserName(key); got == windowsSandboxUserPrefix { + if got := windowsSandboxUserName(key, windowsSandboxRoleOffline); got == windowsSandboxUserPrefix { t.Fatalf("key %q produced a bare prefix", key) } } @@ -213,9 +213,9 @@ func TestProvisionWindowsSandboxIdentityRoundTrip(t *testing.T) { // previous run. Provisioning no longer resets an adopted account's password, // so a leftover account would otherwise be adopted with a password this test // never learns. - _ = removeWindowsSandboxIdentity(windowsSandboxUserName("ziptest01")) + _ = removeWindowsSandboxIdentity(windowsSandboxUserName("ziptest01", windowsSandboxRoleOffline)) - identity, password, _, err := provisionWindowsSandboxIdentity("ziptest01") + identity, password, _, err := provisionWindowsSandboxIdentity("ziptest01", windowsSandboxRoleOffline) if err != nil { t.Fatalf("provision: %v", err) } @@ -240,7 +240,7 @@ func TestProvisionWindowsSandboxIdentityRoundTrip(t *testing.T) { } // Re-running must converge on the same principal rather than failing or // creating a second account. - again, secondPassword, _, err := provisionWindowsSandboxIdentity("ziptest01") + again, secondPassword, _, err := provisionWindowsSandboxIdentity("ziptest01", windowsSandboxRoleOffline) if err != nil { t.Fatalf("second provision: %v", err) } @@ -265,7 +265,7 @@ func TestProvisionWindowsSandboxIdentityRoundTrip(t *testing.T) { SandboxHome: t.TempDir(), WorkspaceRoots: []string{`C:\ziptest01`}, } - setupIdentity, _, err := provisionWindowsSandboxPrincipalForSetup(config) + setupIdentity, _, err := provisionWindowsSandboxPrincipalForSetup(config, windowsSandboxRoleOffline) if err != nil { t.Fatalf("setup provision: %v", err) } @@ -287,7 +287,7 @@ func TestProvisionWindowsSandboxIdentityRoundTrip(t *testing.T) { _ = removeWindowsSandboxIdentity(setupIdentity.Username) }) // Lookup must find what provisioning created. - found, err := lookupWindowsSandboxIdentity("ziptest01") + found, err := lookupWindowsSandboxIdentity("ziptest01", windowsSandboxRoleOffline) if err != nil { t.Fatalf("lookup after provision: %v", err) } @@ -318,9 +318,9 @@ func TestGrantLogonRightsAndMintPrincipalToken(t *testing.T) { const key = "ziplogon01" // A leftover account from an interrupted run would keep its old password, // which the freshly generated one will not match, so start from a clean slate. - _ = removeWindowsSandboxIdentity(windowsSandboxUserName(key)) + _ = removeWindowsSandboxIdentity(windowsSandboxUserName(key, windowsSandboxRoleOffline)) - identity, password, _, err := provisionWindowsSandboxIdentity(key) + identity, password, _, err := provisionWindowsSandboxIdentity(key, windowsSandboxRoleOffline) if err != nil { t.Fatalf("provision: %v", err) } @@ -367,7 +367,7 @@ func TestGrantLogonRightsAndMintPrincipalToken(t *testing.T) { // "run setup" error rather than a raw lookup failure, so the command path can // fall back instead of surfacing a Win32 code. func TestLookupWindowsSandboxIdentityUnprovisioned(t *testing.T) { - _, err := lookupWindowsSandboxIdentity("nosuchworkspacekey9z") + _, err := lookupWindowsSandboxIdentity("nosuchworkspacekey9z", windowsSandboxRoleOffline) if err == nil { t.Skip("a principal for this key unexpectedly exists on this machine") } @@ -459,3 +459,40 @@ func TestWindowsSandboxNameCollisionIsTyped(t *testing.T) { t.Fatalf("collision message = %q, want it to say the account is not ours", wrapped.Error()) } } + +// The two roles must never collide onto one account. If they did, the online +// principal would be whatever the offline one is, which means either an approved +// network command silently has no network, or an ordinary command silently has +// one. The second is the dangerous direction. +func TestWindowsSandboxUserNameSeparatesRoles(t *testing.T) { + const key = "abc123" + offline := windowsSandboxUserName(key, windowsSandboxRoleOffline) + online := windowsSandboxUserName(key, windowsSandboxRoleOnline) + if offline == online { + t.Fatalf("both roles produced %q", offline) + } + // Still stable per role, or setup would create a new account every run. + if again := windowsSandboxUserName(key, windowsSandboxRoleOnline); again != online { + t.Fatalf("online name is not stable: %q then %q", online, again) + } + + // The role tag has to survive truncation. A very long key is exactly when the + // name is cut to the 20-character limit, and losing the tag there is what + // would collide the roles on precisely the workspaces most likely to hit it. + longKey := strings.Repeat("f", 200) + longOffline := windowsSandboxUserName(longKey, windowsSandboxRoleOffline) + longOnline := windowsSandboxUserName(longKey, windowsSandboxRoleOnline) + if longOffline == longOnline { + t.Fatalf("truncation collided the roles onto %q", longOffline) + } + for _, name := range []string{longOffline, longOnline} { + if len(name) > windowsSandboxUserNameMax { + t.Fatalf("name %q is %d chars, want at most %d", name, len(name), windowsSandboxUserNameMax) + } + } + + // Different workspaces still get different accounts within a role. + if other := windowsSandboxUserName("def456", windowsSandboxRoleOffline); other == offline { + t.Fatalf("two workspaces produced the same offline account %q", other) + } +} diff --git a/internal/sandbox/windows_network.go b/internal/sandbox/windows_network.go index 0d614a0fc..febcf4ad6 100644 --- a/internal/sandbox/windows_network.go +++ b/internal/sandbox/windows_network.go @@ -62,15 +62,46 @@ func BuildWindowsNetworkInfraPlan(config WindowsSandboxCommandConfig) (WindowsNe if err != nil { return WindowsNetworkPlan{}, err } + // The offline-marker SID stays first: the setup marker records + // IdentitySIDs[0] as the offline filter identity. + identitySIDs := []string{offlineSID} + // A sandbox principal cannot carry the offline-marker SID, because LogonUser + // builds a token from an account's real group memberships and the marker is a + // synthetic capability SID. So the same filters additionally match a real + // local group that network-denied principals belong to. + // + // Resolved rather than assumed, and absent until the group exists, so a + // machine that has never provisioned principals computes exactly the plan it + // computed before this existed. That matters because the plan is hashed into + // the setup marker and compared on every command: an identity set that + // differed between setup and the command path would fail every command with + // "setup is out of date". + if resolveWindowsSandboxOfflineGroupSIDHook != nil { + groupSID, err := resolveWindowsSandboxOfflineGroupSIDHook() + if err != nil { + return WindowsNetworkPlan{}, err + } + if strings.TrimSpace(groupSID) != "" { + identitySIDs = append(identitySIDs, groupSID) + } + } return WindowsNetworkPlan{ Mode: NetworkDeny, ProviderKey: windowsWFPProviderKey, SubLayerKey: windowsWFPSubLayerKey, - IdentitySIDs: []string{offlineSID}, + IdentitySIDs: identitySIDs, Filters: windowsDenyWFPFilterSpecs(), }, nil } +// resolveWindowsSandboxOfflineGroupSIDHook resolves the local group that +// network-denied principals belong to. It is wired up on Windows only, so this +// file stays free of Win32 calls and the plan on other platforms is unchanged. +// +// Returning ("", nil) means the group does not exist yet, which is the state +// before principals have ever been provisioned. +var resolveWindowsSandboxOfflineGroupSIDHook func() (string, error) + // WindowsNetworkInfraHash fingerprints the provisioned (mode-independent) network // infrastructure so the setup marker validates against the same setup for BOTH // command modes. It never folds in the per-command network mode. diff --git a/internal/sandbox/windows_network_test.go b/internal/sandbox/windows_network_test.go index 4253916f8..d49a5b53e 100644 --- a/internal/sandbox/windows_network_test.go +++ b/internal/sandbox/windows_network_test.go @@ -118,3 +118,71 @@ func assertWindowsWFPCommonFilter(t *testing.T, specs map[string]WindowsWFPFilte // Coverage for the network infra plan + hash and the per-mode token-SID // composition lives in windows_online_offline_test.go. + +// The block filters have to name the offline group as well as the offline-marker +// SID, or a sandbox principal is not covered by them at all: LogonUser builds a +// token from real group memberships and cannot carry the synthetic marker. This +// is the single property that makes network denial work for the principal +// backend, so assert it on the plan rather than trusting the wiring. +func TestNetworkInfraPlanIncludesOfflineGroupIdentity(t *testing.T) { + previous := resolveWindowsSandboxOfflineGroupSIDHook + t.Cleanup(func() { resolveWindowsSandboxOfflineGroupSIDHook = previous }) + + config := WindowsSandboxCommandConfig{SandboxHome: t.TempDir()} + + // Before principals have ever been provisioned the group does not exist, and + // the plan must be exactly what it was before this feature. The plan is hashed + // into the setup marker and re-derived on every command, so an identity set + // that appeared out of nowhere would fail every command with "setup is out of + // date". + resolveWindowsSandboxOfflineGroupSIDHook = func() (string, error) { return "", nil } + base, err := BuildWindowsNetworkInfraPlan(config) + if err != nil { + t.Fatalf("build plan without the group: %v", err) + } + if len(base.IdentitySIDs) != 1 { + t.Fatalf("identity SIDs = %v, want only the offline marker when the group is absent", base.IdentitySIDs) + } + + const groupSID = "S-1-5-32-9999" + resolveWindowsSandboxOfflineGroupSIDHook = func() (string, error) { return groupSID, nil } + withGroup, err := BuildWindowsNetworkInfraPlan(config) + if err != nil { + t.Fatalf("build plan with the group: %v", err) + } + if len(withGroup.IdentitySIDs) != 2 || withGroup.IdentitySIDs[1] != groupSID { + t.Fatalf("identity SIDs = %v, want the offline marker plus %s", withGroup.IdentitySIDs, groupSID) + } + // The marker records IdentitySIDs[0] as the offline filter identity, so the + // marker SID has to stay first. + if withGroup.IdentitySIDs[0] != base.IdentitySIDs[0] { + t.Fatalf("offline marker moved from position 0: %v", withGroup.IdentitySIDs) + } + // Adding the group must change the fingerprint, or setup and the command path + // could disagree about the installed filters without anything noticing. + baseHash, err := WindowsNetworkInfraHash(base) + if err != nil { + t.Fatalf("hash base: %v", err) + } + groupHash, err := WindowsNetworkInfraHash(withGroup) + if err != nil { + t.Fatalf("hash with group: %v", err) + } + if baseHash == groupHash { + t.Fatal("the infra hash ignored the offline group identity") + } +} + +// A lookup failure must not be swallowed into "no group", because that silently +// produces a plan whose filters do not cover sandbox principals. +func TestNetworkInfraPlanPropagatesOfflineGroupLookupFailure(t *testing.T) { + previous := resolveWindowsSandboxOfflineGroupSIDHook + t.Cleanup(func() { resolveWindowsSandboxOfflineGroupSIDHook = previous }) + + resolveWindowsSandboxOfflineGroupSIDHook = func() (string, error) { + return "", errors.New("boom") + } + if _, err := BuildWindowsNetworkInfraPlan(WindowsSandboxCommandConfig{SandboxHome: t.TempDir()}); err == nil { + t.Fatal("a failed offline-group lookup produced a plan; the filters would not cover any principal") + } +} diff --git a/internal/sandbox/windows_setup_windows.go b/internal/sandbox/windows_setup_windows.go index ac8dc1243..9d46bf6e0 100644 --- a/internal/sandbox/windows_setup_windows.go +++ b/internal/sandbox/windows_setup_windows.go @@ -22,14 +22,6 @@ func runWindowsSandboxSetup(config WindowsSandboxSetupConfig, stderr io.Writer) fmt.Fprintln(stderr, WindowsSandboxSetupName+": "+err.Error()) return 1 } - // Always provision the mode-INDEPENDENT infrastructure: the outbound block - // filters scoped to the offline-marker SID. Runtime gates network per command - // by whether the token carries that SID, so one setup serves both modes. - networkPlan, err := BuildWindowsNetworkInfraPlan(config.commandConfig()) - if err != nil { - fmt.Fprintln(stderr, WindowsSandboxSetupName+": "+err.Error()) - return 1 - } rollback, err := applyWindowsACLPlan(plan) if err != nil { fmt.Fprintln(stderr, WindowsSandboxSetupName+": "+err.Error()) @@ -62,6 +54,23 @@ func runWindowsSandboxSetup(config WindowsSandboxSetupConfig, stderr io.Writer) return aclErr } } + // Built AFTER any principal provisioning, not before. The block filters are + // keyed to the offline group as well as the offline-marker SID, and that group + // is created as part of provisioning: planning first would install filters + // that name only the marker, leaving every offline principal with an open + // network while looking correctly set up. + // + // Mode-INDEPENDENT by design. Which identity a command runs under is what + // selects allow or deny, so one setup serves both modes. + networkPlan, err := BuildWindowsNetworkInfraPlan(config.commandConfig()) + if err != nil { + if rollbackErr := rollback(); rollbackErr != nil { + fmt.Fprintf(stderr, "%s: %v; rollback failed: %v\n", WindowsSandboxSetupName, err, rollbackErr) + return 1 + } + fmt.Fprintln(stderr, WindowsSandboxSetupName+": "+err.Error()) + return 1 + } if err := applyWindowsNetworkPlan(networkPlan); err != nil { if rollbackErr := rollback(); rollbackErr != nil { fmt.Fprintf(stderr, "%s: %v; rollback failed: %v\n", WindowsSandboxSetupName, err, rollbackErr) diff --git a/internal/sandbox/windows_stale_secret_windows_test.go b/internal/sandbox/windows_stale_secret_windows_test.go index 7e4e3ebef..222b8fde3 100644 --- a/internal/sandbox/windows_stale_secret_windows_test.go +++ b/internal/sandbox/windows_stale_secret_windows_test.go @@ -52,7 +52,7 @@ func TestProvisionSurfacesFailedStaleSecretCleanup(t *testing.T) { t.Fatal(err) } // created=false so the run ADOPTS an account and rotation applies. - provisionWindowsSandboxIdentityFn = func(string) (windowsSandboxIdentity, string, bool, error) { + provisionWindowsSandboxIdentityFn = func(string, windowsSandboxRole) (windowsSandboxIdentity, string, bool, error) { return windowsSandboxIdentity{Username: "zero-sbx-test", SID: sid}, "pw", false, nil } grantWindowsSandboxLogonRightsFn = func(*windows.SID) error { return nil } @@ -80,7 +80,7 @@ func TestProvisionSurfacesFailedStaleSecretCleanup(t *testing.T) { CommandCWD: `C:\ws`, WorkspaceRoots: []string{`C:\ws`}, } - _, _, err = provisionWindowsSandboxPrincipalForSetup(config) + _, _, err = provisionWindowsSandboxPrincipalForSetup(config, windowsSandboxRoleOffline) if removed != testCase.wantRemove { t.Fatalf("stale secret removal attempted = %v, want %v", removed, testCase.wantRemove) From b95bfe631e10692798e50a1b2ecf5d614721a4ec Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Mon, 27 Jul 2026 10:39:12 +0530 Subject: [PATCH 2/8] fix(sandbox): prove ownership before deleting a sandbox account removeWindowsSandboxIdentity is called with a DERIVED name, so it could be pointed at a name that happens to belong to somebody else's local account. Deleting a user is not a recoverable mistake, and the only thing standing between the two cases was the name matching a pattern we generate ourselves. Raised by CodeRabbit against the test fixtures, but the production teardown path had the same hazard, so the guard belongs there rather than in the tests. The ownership check itself now lives on the base branch, which grew the same helper to stop provisioning ADOPTING a squatted account. This applies it to the other end: an account that is not ours is left alone rather than deleted. The gated tests get the protection for free, since their pre-clean goes through the same helper. Also appends the trimmed offline-group SID rather than the raw one. Worth noting the reported consequence does not hold: newWindowsWFPUserCondition canonicalises before converting, so a padded value would have been trimmed before reaching StringToSid. The resolver returns SID.String(), which never carries whitespace, so this is defensive tidying rather than a fix. --- internal/sandbox/windows_identity_windows.go | 12 ++++++++++++ internal/sandbox/windows_network.go | 4 ++-- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/internal/sandbox/windows_identity_windows.go b/internal/sandbox/windows_identity_windows.go index 174c15480..6ff4cccf9 100644 --- a/internal/sandbox/windows_identity_windows.go +++ b/internal/sandbox/windows_identity_windows.go @@ -734,11 +734,23 @@ func provisionWindowsSandboxIdentity(workspaceKey string, role windowsSandboxRol // A missing account is success, so teardown converges the same way provisioning // does. Requires an elevated caller. func removeWindowsSandboxIdentity(username string) error { + // Account names here are derived, not discovered, so this could be pointed at + // a name that happens to belong to somebody else's local account. Deleting a + // user is not a recoverable mistake, so ownership is proven before deleting + // rather than inferred from the name matching a pattern we generate. + managed, err := windowsSandboxUserIsManaged(username) + if err != nil { + return err + } + if !managed { + return nil + } name, err := windows.UTF16PtrFromString(username) if err != nil { return err } status, _, _ := procNetUserDel.Call(0, uintptr(unsafe.Pointer(name))) + runtime.KeepAlive(name) return netAPIStatus("NetUserDel", status, nerrUserNotFound) } diff --git a/internal/sandbox/windows_network.go b/internal/sandbox/windows_network.go index febcf4ad6..62ab69824 100644 --- a/internal/sandbox/windows_network.go +++ b/internal/sandbox/windows_network.go @@ -81,8 +81,8 @@ func BuildWindowsNetworkInfraPlan(config WindowsSandboxCommandConfig) (WindowsNe if err != nil { return WindowsNetworkPlan{}, err } - if strings.TrimSpace(groupSID) != "" { - identitySIDs = append(identitySIDs, groupSID) + if trimmed := strings.TrimSpace(groupSID); trimmed != "" { + identitySIDs = append(identitySIDs, trimmed) } } return WindowsNetworkPlan{ From 746eed363dc0ba822c56646ac2defc8c449b826f Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Mon, 27 Jul 2026 15:46:58 +0530 Subject: [PATCH 3/8] fix(sandbox): assert the filters cover principals, and report a retained account Two follow-ups from review, both on the same theme: a control that quietly does nothing looks identical to one that works. The network plan must be built AFTER provisioning, because provisioning creates the group the block filters name. Built first, the filters name only the offline marker and every offline principal has an open network while setup reports success. That ordering is invisible at the call site, so setup now checks the plan actually names the offline group before installing anything and refuses if it does not. A later refactor that moves the plan build back fails loudly instead of producing a security control that enforces nothing. The predicate is separate so it can be asserted directly: a plan carrying only the marker must read as uncovered, one carrying the group as covered, case differences must not read as missing, and a host with no group provisioned has no principal to miss and must not be refused. Making coverage always report true fails that test. Removal also reported plain success when it declined to delete an account Zero did not create. Leaving it alone is right, but telling an operator cleanup completed when a name they may care about was deliberately retained is not. That case is now a distinguishable sentinel, and teardown treats it as success, since "no principal of ours under this name" is the goal state either way. --- .../windows_identity_runtime_windows.go | 9 ++++- internal/sandbox/windows_identity_windows.go | 14 ++++++- .../sandbox/windows_identity_windows_test.go | 4 +- internal/sandbox/windows_network.go | 25 +++++++++++++ internal/sandbox/windows_network_test.go | 37 +++++++++++++++++++ internal/sandbox/windows_setup_windows.go | 17 +++++++++ 6 files changed, 102 insertions(+), 4 deletions(-) diff --git a/internal/sandbox/windows_identity_runtime_windows.go b/internal/sandbox/windows_identity_runtime_windows.go index cbfea5aed..a5496755c 100644 --- a/internal/sandbox/windows_identity_runtime_windows.go +++ b/internal/sandbox/windows_identity_runtime_windows.go @@ -414,7 +414,14 @@ func removeWindowsSandboxPrincipalForSetup(config WindowsSandboxCommandConfig, r } else if !errors.Is(err, errWindowsSandboxIdentityUnavailable) { return err } - return removeWindowsSandboxIdentity(username) + // A foreign account under our derived name is left in place deliberately, and + // that is the goal state for teardown: no principal of ours exists under it. + // Surfacing it as a teardown failure would make setup rollback report an + // error for having correctly declined to delete somebody else's account. + if err := removeWindowsSandboxIdentity(username); err != nil && !errors.Is(err, errWindowsSandboxForeignAccountRetained) { + return err + } + return nil } // setupWindowsSandboxRuntimeRoot resolves this workspace's runtime root and diff --git a/internal/sandbox/windows_identity_windows.go b/internal/sandbox/windows_identity_windows.go index 6ff4cccf9..9c07b3a26 100644 --- a/internal/sandbox/windows_identity_windows.go +++ b/internal/sandbox/windows_identity_windows.go @@ -743,7 +743,13 @@ func removeWindowsSandboxIdentity(username string) error { return err } if !managed { - return nil + // Distinguishable rather than a bare nil. Leaving the account alone is + // right, and callers that only want the goal state ("no principal of ours + // under this name") can treat it as success by matching this sentinel. But + // reporting a plain success would tell an operator that cleanup completed + // when a name they may care about was deliberately left in place, which is + // the one detail worth surfacing here. + return fmt.Errorf("%w: %q", errWindowsSandboxForeignAccountRetained, username) } name, err := windows.UTF16PtrFromString(username) if err != nil { @@ -754,6 +760,12 @@ func removeWindowsSandboxIdentity(username string) error { return netAPIStatus("NetUserDel", status, nerrUserNotFound) } +// errWindowsSandboxForeignAccountRetained reports that removal left an account +// in place because it is not one Zero created. It is not a failure to clean up, +// it is a refusal to delete somebody else's account, and teardown paths treat it +// as success. +var errWindowsSandboxForeignAccountRetained = errors.New("left a local account in place because Zero did not create it") + // errWindowsSandboxIdentityUnavailable reports that no sandbox principal has // been provisioned yet, so callers can fall back to the restricted-token // backend instead of failing the command. diff --git a/internal/sandbox/windows_identity_windows_test.go b/internal/sandbox/windows_identity_windows_test.go index da743035b..1823b0b6d 100644 --- a/internal/sandbox/windows_identity_windows_test.go +++ b/internal/sandbox/windows_identity_windows_test.go @@ -228,7 +228,7 @@ func TestProvisionWindowsSandboxIdentityRoundTrip(t *testing.T) { if err := revokeWindowsSandboxLogonRights(identity.SID); err != nil { t.Errorf("cleanup: revoke logon rights: %v", err) } - if err := removeWindowsSandboxIdentity(identity.Username); err != nil { + if err := removeWindowsSandboxIdentity(identity.Username); err != nil && !errors.Is(err, errWindowsSandboxForeignAccountRetained) { t.Errorf("cleanup: remove principal: %v", err) } }) @@ -330,7 +330,7 @@ func TestGrantLogonRightsAndMintPrincipalToken(t *testing.T) { if err := revokeWindowsSandboxLogonRights(identity.SID); err != nil { t.Errorf("cleanup: revoke logon rights: %v", err) } - if err := removeWindowsSandboxIdentity(identity.Username); err != nil { + if err := removeWindowsSandboxIdentity(identity.Username); err != nil && !errors.Is(err, errWindowsSandboxForeignAccountRetained) { t.Errorf("cleanup: remove principal: %v", err) } }) diff --git a/internal/sandbox/windows_network.go b/internal/sandbox/windows_network.go index 62ab69824..fbf62341f 100644 --- a/internal/sandbox/windows_network.go +++ b/internal/sandbox/windows_network.go @@ -94,6 +94,31 @@ func BuildWindowsNetworkInfraPlan(config WindowsSandboxCommandConfig) (WindowsNe }, nil } +// WindowsNetworkPlanCoversPrincipals reports whether a plan's block filters name +// the offline group, which is the only thing that makes them apply to a sandbox +// principal. +// +// This exists to be asserted, not consulted. The plan must be built AFTER +// provisioning, because provisioning is what creates the group it resolves; a +// plan built first names only the offline marker and leaves every offline +// principal with an open network while setup reports success. That is a control +// that enforces nothing while claiming to work, and the ordering which prevents +// it is invisible at the call site, so a later refactor can undo it silently. +func WindowsNetworkPlanCoversPrincipals(plan WindowsNetworkPlan, offlineGroupSID string) bool { + offlineGroupSID = strings.TrimSpace(offlineGroupSID) + if offlineGroupSID == "" { + // No group provisioned on this host, so there is no principal for the + // filters to miss and nothing to assert. + return true + } + for _, sid := range plan.IdentitySIDs { + if strings.EqualFold(strings.TrimSpace(sid), offlineGroupSID) { + return true + } + } + return false +} + // resolveWindowsSandboxOfflineGroupSIDHook resolves the local group that // network-denied principals belong to. It is wired up on Windows only, so this // file stays free of Win32 calls and the plan on other platforms is unchanged. diff --git a/internal/sandbox/windows_network_test.go b/internal/sandbox/windows_network_test.go index d49a5b53e..50f66d960 100644 --- a/internal/sandbox/windows_network_test.go +++ b/internal/sandbox/windows_network_test.go @@ -186,3 +186,40 @@ func TestNetworkInfraPlanPropagatesOfflineGroupLookupFailure(t *testing.T) { t.Fatal("a failed offline-group lookup produced a plan; the filters would not cover any principal") } } + +// The ordering that makes the block filters apply to sandbox principals is +// invisible at the call site: the plan must be built AFTER provisioning, +// because provisioning creates the group the filters name. A plan built first +// names only the offline marker, and a machine would then report a successful +// setup while every offline principal had an open network. +// +// Asserted on the predicate setup checks before installing anything, so moving +// the plan build back ahead of provisioning fails loudly instead of silently +// producing a control that enforces nothing. +func TestNetworkPlanCoverageDetectsPrincipalsLeftUncovered(t *testing.T) { + const groupSID = "S-1-5-32-4242" + + // What a plan built BEFORE provisioning looks like: marker only. + tooEarly := WindowsNetworkPlan{IdentitySIDs: []string{"S-1-15-3-1111"}} + if WindowsNetworkPlanCoversPrincipals(tooEarly, groupSID) { + t.Fatal("a plan naming only the offline marker was reported as covering principals; the ordering guard would not fire") + } + + // And after: marker plus the group. + correct := WindowsNetworkPlan{IdentitySIDs: []string{"S-1-15-3-1111", groupSID}} + if !WindowsNetworkPlanCoversPrincipals(correct, groupSID) { + t.Fatal("a plan naming the offline group was reported as not covering principals; setup would refuse a correct plan") + } + // SID comparison is case-insensitive, so a differently-cased resolve does not + // read as a missing group and block a valid setup. + mixed := WindowsNetworkPlan{IdentitySIDs: []string{strings.ToLower(groupSID)}} + if !WindowsNetworkPlanCoversPrincipals(mixed, groupSID) { + t.Fatal("case difference read as a missing group") + } + + // A host with no group provisioned has no principal to miss, so there is + // nothing to assert and setup must not refuse. + if !WindowsNetworkPlanCoversPrincipals(tooEarly, "") { + t.Fatal("an unprovisioned host was treated as a coverage failure") + } +} diff --git a/internal/sandbox/windows_setup_windows.go b/internal/sandbox/windows_setup_windows.go index 9d46bf6e0..2d895a1f2 100644 --- a/internal/sandbox/windows_setup_windows.go +++ b/internal/sandbox/windows_setup_windows.go @@ -3,6 +3,7 @@ package sandbox import ( + "errors" "fmt" "io" @@ -71,6 +72,22 @@ func runWindowsSandboxSetup(config WindowsSandboxSetupConfig, stderr io.Writer) fmt.Fprintln(stderr, WindowsSandboxSetupName+": "+err.Error()) return 1 } + // Refuse to install filters that would not cover the principals just + // provisioned. The ordering above is what makes them cover it, and nothing at + // this call site shows that, so assert it rather than trusting it to survive + // a later refactor. Installing anyway would leave a machine reporting a + // successful setup while every offline principal had an open network. + if groupSID, groupErr := resolveWindowsSandboxOfflineGroupSID(); groupErr == nil { + if !WindowsNetworkPlanCoversPrincipals(networkPlan, groupSID) { + err := errors.New("network block filters do not name the sandbox offline group, so they would not apply to any sandbox principal; the network plan must be built after principals are provisioned") + if rollbackErr := rollback(); rollbackErr != nil { + fmt.Fprintf(stderr, "%s: %v; rollback failed: %v\n", WindowsSandboxSetupName, err, rollbackErr) + return 1 + } + fmt.Fprintln(stderr, WindowsSandboxSetupName+": "+err.Error()) + return 1 + } + } if err := applyWindowsNetworkPlan(networkPlan); err != nil { if rollbackErr := rollback(); rollbackErr != nil { fmt.Fprintf(stderr, "%s: %v; rollback failed: %v\n", WindowsSandboxSetupName, err, rollbackErr) From 1b3df0c04a43e0c35b2faa05e8ae0c7578a8abbc Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Wed, 29 Jul 2026 14:11:46 +0530 Subject: [PATCH 4/8] fix(sandbox): spare adopted principals when dual-role setup rolls back Provisioning already declined to delete an account it had adopted, and the outer setup rollback then appended an unconditional removal for every role that got that far. With two roles that is the common case rather than an unlucky one: the offline role usually succeeds, so a failure in the online role or in ACL application destroyed a principal that was working before the run started. The removal closure is now only appended for a principal this run created. Threads the workspace key into the delete path as well. Ownership was proven from the account comment alone, which on a name collision belongs to a DIFFERENT workspace, so deleting it would have been the same unrecoverable mistake the check exists to prevent. Policy DenyWrite now reaches the principal ACL plan here too, matching the single-principal path. Fixes the mode-independence test, which required exactly one identity SID and so failed on any Windows host that already had ZeroSandboxOffline, where the plan legitimately carries two. CI never saw it because the Linux and macOS jobs leave the hook nil and a fresh Windows runner has no group. The hook is now pinned, and the test additionally asserts the property it is named for in the group-present case, including that the infra hash changes when the group appears, which is the cross-workspace coupling raised for a maintainer decision. --- .../windows_dualrole_rollback_windows_test.go | 161 ++++++++++++++++++ .../windows_identity_runtime_windows.go | 21 ++- internal/sandbox/windows_identity_windows.go | 7 +- .../sandbox/windows_identity_windows_test.go | 10 +- .../sandbox/windows_online_offline_test.go | 41 +++++ 5 files changed, 229 insertions(+), 11 deletions(-) create mode 100644 internal/sandbox/windows_dualrole_rollback_windows_test.go diff --git a/internal/sandbox/windows_dualrole_rollback_windows_test.go b/internal/sandbox/windows_dualrole_rollback_windows_test.go new file mode 100644 index 000000000..381c06364 --- /dev/null +++ b/internal/sandbox/windows_dualrole_rollback_windows_test.go @@ -0,0 +1,161 @@ +//go:build windows + +package sandbox + +import ( + "errors" + "testing" + + "golang.org/x/sys/windows" +) + +// The outer rollback must only delete principals this run created. +// +// Dual-role setup provisions offline then online. If the offline role adopts an +// account that already existed and anything afterwards fails, the outer rollback +// used to delete it anyway, turning a partial re-setup failure into the loss of +// a principal that was working before the run started. Two roles make that the +// common case rather than an unlucky one: the first role usually succeeds, so +// there is nearly always something for a later failure to destroy. +func TestDualRoleSetupRollbackSparesAdoptedPrincipals(t *testing.T) { + for name, testCase := range map[string]struct { + createdByRole map[windowsSandboxRole]bool + wantRemoved []windowsSandboxRole + }{ + "offline adopted, online created": { + createdByRole: map[windowsSandboxRole]bool{ + windowsSandboxRoleOffline: false, + windowsSandboxRoleOnline: true, + }, + // Only the one this run made. Deleting the adopted offline principal + // is the data loss this guards against. + wantRemoved: []windowsSandboxRole{windowsSandboxRoleOnline}, + }, + "both adopted": { + createdByRole: map[windowsSandboxRole]bool{ + windowsSandboxRoleOffline: false, + windowsSandboxRoleOnline: false, + }, + wantRemoved: nil, + }, + "both created": { + createdByRole: map[windowsSandboxRole]bool{ + windowsSandboxRoleOffline: true, + windowsSandboxRoleOnline: true, + }, + wantRemoved: []windowsSandboxRole{windowsSandboxRoleOffline, windowsSandboxRoleOnline}, + }, + } { + t.Run(name, func(t *testing.T) { + var removed []windowsSandboxRole + restoreDualRoleSeams(t, testCase.createdByRole, &removed) + + // Fail on the SECOND role, not the first. ACL application happens per + // role inside the loop, so failing the first aborts before the second + // is provisioned at all and the rollback never has more than one + // principal to consider. The case worth testing is the one review + // described: the first role succeeds, the second fails, and the + // question is whether the first gets destroyed on the way out. + // Count GRANT plans, not ACL calls. applyWindowsPrincipalACLs revokes + // this trustee's existing ACEs before applying the new plan, so there + // are two calls per role now. The contract under test is "the second + // ROLE fails"; counting raw calls would fail the first role's grant + // instead and prove something else entirely. + grants := 0 + applyWindowsACLPlanFn = func(plan WindowsACLPlan) (func() error, error) { + if len(plan.Entries) > 0 && plan.Entries[0].Action == windowsACLRevoke { + return func() error { return nil }, nil + } + grants++ + if grants < 2 { + return func() error { return nil }, nil + } + return nil, errors.New("ACL apply refused") + } + + if _, err := setupWindowsSandboxPrincipal(windowsSandboxTestConfig()); err == nil { + t.Fatal("setup reported success despite an injected ACL failure") + } + assertRolesEqual(t, removed, testCase.wantRemoved) + }) + } +} + +// The same contract on the success path: the rollback handed back to the caller +// for a LATER setup step to invoke must be just as reluctant. +func TestDualRoleSetupReturnedRollbackSparesAdoptedPrincipals(t *testing.T) { + var removed []windowsSandboxRole + restoreDualRoleSeams(t, map[windowsSandboxRole]bool{ + windowsSandboxRoleOffline: false, + windowsSandboxRoleOnline: true, + }, &removed) + + rollback, err := setupWindowsSandboxPrincipal(windowsSandboxTestConfig()) + if err != nil { + t.Fatalf("setup: %v", err) + } + if len(removed) != 0 { + t.Fatalf("setup removed principals before anything failed: %v", removed) + } + if err := rollback(); err != nil { + t.Fatalf("rollback: %v", err) + } + assertRolesEqual(t, removed, []windowsSandboxRole{windowsSandboxRoleOnline}) +} + +func restoreDualRoleSeams(t *testing.T, createdByRole map[windowsSandboxRole]bool, removed *[]windowsSandboxRole) { + t.Helper() + prevProvision := provisionWindowsSandboxPrincipalForSetupFn + prevApply := applyWindowsACLPlanFn + prevRemove := removeWindowsSandboxPrincipalForSetupFn + t.Cleanup(func() { + provisionWindowsSandboxPrincipalForSetupFn = prevProvision + applyWindowsACLPlanFn = prevApply + removeWindowsSandboxPrincipalForSetupFn = prevRemove + }) + + provisionWindowsSandboxPrincipalForSetupFn = func(_ WindowsSandboxCommandConfig, role windowsSandboxRole) (windowsSandboxIdentity, bool, error) { + sid, err := windows.CreateWellKnownSid(windows.WinLocalSystemSid) + if err != nil { + return windowsSandboxIdentity{}, false, err + } + return windowsSandboxIdentity{Username: "zero-sbx-test", SID: sid}, createdByRole[role], nil + } + applyWindowsACLPlanFn = func(WindowsACLPlan) (func() error, error) { + return func() error { return nil }, nil + } + removeWindowsSandboxPrincipalForSetupFn = func(_ WindowsSandboxCommandConfig, role windowsSandboxRole) error { + *removed = append(*removed, role) + return nil + } +} + +func windowsSandboxTestConfig() WindowsSandboxCommandConfig { + return WindowsSandboxCommandConfig{ + SandboxHome: `C:\sandboxhome`, + CommandCWD: `C:\ws`, + WorkspaceRoots: []string{`C:\ws`}, + PermissionProfile: PermissionProfile{ + FileSystem: FileSystemPolicy{ + Kind: FileSystemRestricted, + WriteRoots: []WritableRoot{{Root: `C:\ws`}}, + }, + }, + } +} + +func assertRolesEqual(t *testing.T, got []windowsSandboxRole, want []windowsSandboxRole) { + t.Helper() + if len(got) != len(want) { + t.Fatalf("removed %v, want %v", got, want) + } + seen := map[windowsSandboxRole]bool{} + for _, role := range got { + seen[role] = true + } + for _, role := range want { + if !seen[role] { + t.Fatalf("removed %v, want %v", got, want) + } + } +} diff --git a/internal/sandbox/windows_identity_runtime_windows.go b/internal/sandbox/windows_identity_runtime_windows.go index a5496755c..e9569991e 100644 --- a/internal/sandbox/windows_identity_runtime_windows.go +++ b/internal/sandbox/windows_identity_runtime_windows.go @@ -164,6 +164,19 @@ var warnWindowsSandboxPrincipalUnavailable = func(username string) { var windowsSandboxPrincipalWarnOnce sync.Once +// Seamed so the dual-role setup rollback can be exercised without an elevated +// machine. The contract under test is which principals the outer rollback is +// willing to delete, which is a data-loss decision and the one thing here worth +// proving rather than reasoning about. +var ( + provisionWindowsSandboxPrincipalForSetupFn = provisionWindowsSandboxPrincipalForSetup + removeWindowsSandboxPrincipalForSetupFn = removeWindowsSandboxPrincipalForSetup + // applyWindowsACLPlanFn lives with the other identity seams in + // windows_identity_windows.go — the ACL-ordering guarantee it exists for + // predates the dual-role split, and two declarations of the same seam would + // let a test stub one while production used the other. +) + // provisionWindowsSandboxPrincipalForSetup does the elevated half: create the // account, grant it the batch logon right, and store its password locked to the // invoking user. Called from `zero sandbox setup`. @@ -238,7 +251,7 @@ func provisionWindowsSandboxPrincipalForSetup(config WindowsSandboxCommandConfig _ = revokeWindowsSandboxLogonRightsFn(identity.SID) } if created { - _ = removeWindowsSandboxIdentity(identity.Username) + _ = removeWindowsSandboxIdentity(identity.Username, key) } return cleanupErr } @@ -325,7 +338,7 @@ func setupWindowsSandboxPrincipal(config WindowsSandboxCommandConfig) (func() er } for _, role := range roles { - identity, created, err := provisionWindowsSandboxPrincipalForSetup(config, role) + identity, created, err := provisionWindowsSandboxPrincipalForSetupFn(config, role) if err != nil { _ = rollback() return nil, err @@ -340,7 +353,7 @@ func setupWindowsSandboxPrincipal(config WindowsSandboxCommandConfig) (func() er // always something for a later failure to destroy. This mirrors the // contract provisioning already applies to its own inner rollback. if created { - undo = append(undo, func() error { return removeWindowsSandboxPrincipalForSetup(config, role) }) + undo = append(undo, func() error { return removeWindowsSandboxPrincipalForSetupFn(config, role) }) } // Both principals get the same filesystem access. They differ only in @@ -418,7 +431,7 @@ func removeWindowsSandboxPrincipalForSetup(config WindowsSandboxCommandConfig, r // that is the goal state for teardown: no principal of ours exists under it. // Surfacing it as a teardown failure would make setup rollback report an // error for having correctly declined to delete somebody else's account. - if err := removeWindowsSandboxIdentity(username); err != nil && !errors.Is(err, errWindowsSandboxForeignAccountRetained) { + if err := removeWindowsSandboxIdentity(username, key); err != nil && !errors.Is(err, errWindowsSandboxForeignAccountRetained) { return err } return nil diff --git a/internal/sandbox/windows_identity_windows.go b/internal/sandbox/windows_identity_windows.go index 9c07b3a26..9a588ead2 100644 --- a/internal/sandbox/windows_identity_windows.go +++ b/internal/sandbox/windows_identity_windows.go @@ -733,12 +733,15 @@ func provisionWindowsSandboxIdentity(workspaceKey string, role windowsSandboxRol // // A missing account is success, so teardown converges the same way provisioning // does. Requires an elevated caller. -func removeWindowsSandboxIdentity(username string) error { +func removeWindowsSandboxIdentity(username string, workspaceKey string) error { // Account names here are derived, not discovered, so this could be pointed at // a name that happens to belong to somebody else's local account. Deleting a // user is not a recoverable mistake, so ownership is proven before deleting // rather than inferred from the name matching a pattern we generate. - managed, err := windowsSandboxUserIsManaged(username) + // Keyed to the workspace as well as to the marker: on a name collision the + // account belongs to a DIFFERENT workspace, and deleting it would be the + // same unrecoverable mistake as deleting a stranger's account. + managed, err := windowsSandboxUserIsManagedFn(username, workspaceKey) if err != nil { return err } diff --git a/internal/sandbox/windows_identity_windows_test.go b/internal/sandbox/windows_identity_windows_test.go index 1823b0b6d..9c8cdde0f 100644 --- a/internal/sandbox/windows_identity_windows_test.go +++ b/internal/sandbox/windows_identity_windows_test.go @@ -213,7 +213,7 @@ func TestProvisionWindowsSandboxIdentityRoundTrip(t *testing.T) { // previous run. Provisioning no longer resets an adopted account's password, // so a leftover account would otherwise be adopted with a password this test // never learns. - _ = removeWindowsSandboxIdentity(windowsSandboxUserName("ziptest01", windowsSandboxRoleOffline)) + _ = removeWindowsSandboxIdentity(windowsSandboxUserName("ziptest01", windowsSandboxRoleOffline), "ziptest01") identity, password, _, err := provisionWindowsSandboxIdentity("ziptest01", windowsSandboxRoleOffline) if err != nil { @@ -228,7 +228,7 @@ func TestProvisionWindowsSandboxIdentityRoundTrip(t *testing.T) { if err := revokeWindowsSandboxLogonRights(identity.SID); err != nil { t.Errorf("cleanup: revoke logon rights: %v", err) } - if err := removeWindowsSandboxIdentity(identity.Username); err != nil && !errors.Is(err, errWindowsSandboxForeignAccountRetained) { + if err := removeWindowsSandboxIdentity(identity.Username, "ziptest01"); err != nil && !errors.Is(err, errWindowsSandboxForeignAccountRetained) { t.Errorf("cleanup: remove principal: %v", err) } }) @@ -284,7 +284,7 @@ func TestProvisionWindowsSandboxIdentityRoundTrip(t *testing.T) { _ = token.Close() t.Cleanup(func() { _ = revokeWindowsSandboxLogonRights(setupIdentity.SID) - _ = removeWindowsSandboxIdentity(setupIdentity.Username) + _ = removeWindowsSandboxIdentity(setupIdentity.Username, "ziptest01") }) // Lookup must find what provisioning created. found, err := lookupWindowsSandboxIdentity("ziptest01", windowsSandboxRoleOffline) @@ -318,7 +318,7 @@ func TestGrantLogonRightsAndMintPrincipalToken(t *testing.T) { const key = "ziplogon01" // A leftover account from an interrupted run would keep its old password, // which the freshly generated one will not match, so start from a clean slate. - _ = removeWindowsSandboxIdentity(windowsSandboxUserName(key, windowsSandboxRoleOffline)) + _ = removeWindowsSandboxIdentity(windowsSandboxUserName(key, windowsSandboxRoleOffline), key) identity, password, _, err := provisionWindowsSandboxIdentity(key, windowsSandboxRoleOffline) if err != nil { @@ -330,7 +330,7 @@ func TestGrantLogonRightsAndMintPrincipalToken(t *testing.T) { if err := revokeWindowsSandboxLogonRights(identity.SID); err != nil { t.Errorf("cleanup: revoke logon rights: %v", err) } - if err := removeWindowsSandboxIdentity(identity.Username); err != nil && !errors.Is(err, errWindowsSandboxForeignAccountRetained) { + if err := removeWindowsSandboxIdentity(identity.Username, key); err != nil && !errors.Is(err, errWindowsSandboxForeignAccountRetained) { t.Errorf("cleanup: remove principal: %v", err) } }) diff --git a/internal/sandbox/windows_online_offline_test.go b/internal/sandbox/windows_online_offline_test.go index 370abee73..f9e32ed71 100644 --- a/internal/sandbox/windows_online_offline_test.go +++ b/internal/sandbox/windows_online_offline_test.go @@ -37,6 +37,18 @@ func TestWindowsRuntimeTokenSIDs(t *testing.T) { // setup serves both modes (and its fingerprint is stable across modes). func TestBuildWindowsNetworkInfraPlanIsModeIndependent(t *testing.T) { home := t.TempDir() + // Pinned, because the count below depends on whether this machine happens to + // have the offline group already. BuildWindowsNetworkInfraPlan folds that + // group's SID in when it resolves, so on a Windows host where an earlier + // elevated setup created ZeroSandboxOffline the plan legitimately carries two + // identity SIDs and this test failed on a correct plan. CI never saw it: the + // Linux and macOS jobs leave the hook nil, and a fresh Windows runner has no + // group yet. Stubbing it makes the assertion about the plan rather than about + // the machine it runs on. + previousHook := resolveWindowsSandboxOfflineGroupSIDHook + t.Cleanup(func() { resolveWindowsSandboxOfflineGroupSIDHook = previousHook }) + resolveWindowsSandboxOfflineGroupSIDHook = nil + mk := func(mode NetworkMode) WindowsSandboxCommandConfig { return WindowsSandboxCommandConfig{ SandboxHome: home, @@ -72,6 +84,35 @@ func TestBuildWindowsNetworkInfraPlanIsModeIndependent(t *testing.T) { if denyPlan.IdentitySIDs[0] != offline { t.Errorf("infra plan SID = %q, want offline-marker %q", denyPlan.IdentitySIDs[0], offline) } + + // Mode independence has to hold on a machine that already has the offline + // group too, which is where the pinned counts above would otherwise be + // asserting something about the host rather than the plan. The group SID is + // folded in for both modes, so the hashes must still agree; only the count + // changes. + resolveWindowsSandboxOfflineGroupSIDHook = func() (string, error) { return "S-1-5-32-9999", nil } + withGroupDeny, err := BuildWindowsNetworkInfraPlan(mk(NetworkDeny)) + if err != nil { + t.Fatalf("deny infra plan with the group present: %v", err) + } + withGroupAllow, err := BuildWindowsNetworkInfraPlan(mk(NetworkAllow)) + if err != nil { + t.Fatalf("allow infra plan with the group present: %v", err) + } + if len(withGroupDeny.IdentitySIDs) != 2 { + t.Fatalf("group present should add its SID, got %v", withGroupDeny.IdentitySIDs) + } + groupDenyHash, _ := WindowsNetworkInfraHash(withGroupDeny) + groupAllowHash, _ := WindowsNetworkInfraHash(withGroupAllow) + if groupDenyHash != groupAllowHash || groupDenyHash == "" { + t.Fatalf("infra hash must stay mode-independent with the group present: deny=%q allow=%q", groupDenyHash, groupAllowHash) + } + // And it must differ from the no-group hash, which is the cross-workspace + // coupling called out for a maintainer decision: one workspace creating the + // group changes every other sandbox home's expected fingerprint. + if groupDenyHash == denyHash { + t.Error("group presence did not change the infra hash; the marker staleness this causes is a deliberate property and should be visible here") + } } // A pre-existing schema-1 capability file (no Offline SID) is upgraded in place: From 30a0f9ac5a51e8b0fd543cf94bc0251ef22f46e5 Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Wed, 29 Jul 2026 17:07:51 +0530 Subject: [PATCH 5/8] test(sandbox): stub the password reset and pin the resolved group SIDs Two review points on the tests added in the previous commit. The provisioning stub left resetWindowsSandboxUserPassword as a real call. Nothing under test reaches it any more, because rotation moved to the caller, but a test that resets a real managed account's password if the code ever moves back is not a risk worth carrying. It is now stubbed to fail the test instead, which also states the contract. The group-present assertion checked only that two identity SIDs were present. A duplicated offline marker or an unrelated SID would satisfy that while meaning something quite different, so it now pins both positions. Also drops a duplicated stub assignment left by the rebase. --- .../sandbox/windows_identity_rollback_windows_test.go | 11 ++++++++++- internal/sandbox/windows_online_offline_test.go | 9 +++++++-- 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/internal/sandbox/windows_identity_rollback_windows_test.go b/internal/sandbox/windows_identity_rollback_windows_test.go index 68eff5e98..25210b76d 100644 --- a/internal/sandbox/windows_identity_rollback_windows_test.go +++ b/internal/sandbox/windows_identity_rollback_windows_test.go @@ -20,12 +20,14 @@ func stubWindowsProvisioning(t *testing.T, existed bool, groupErr error, offline prevAdd, prevSID := addWindowsSandboxUserToGroupFn, resolveWindowsSandboxSIDFn prevManaged := windowsSandboxUserIsManagedFn prevOffline := addWindowsSandboxUserToOfflineGroupFn + prevReset := resetWindowsSandboxUserPasswordFn t.Cleanup(func() { ensureWindowsSandboxGroupFn, ensureWindowsSandboxUserFn = prevGroup, prevUser ensureWindowsSandboxOfflineGroupFn = prevOfflineGroup addWindowsSandboxUserToGroupFn, resolveWindowsSandboxSIDFn = prevAdd, prevSID windowsSandboxUserIsManagedFn = prevManaged addWindowsSandboxUserToOfflineGroupFn = prevOffline + resetWindowsSandboxUserPasswordFn = prevReset }) ensureWindowsSandboxGroupFn = func() error { return nil } @@ -33,8 +35,15 @@ func stubWindowsProvisioning(t *testing.T, existed bool, groupErr error, offline // Adopted accounts are ours in these tests; the ownership check is a real // syscall and would otherwise refuse before the code under test runs. windowsSandboxUserIsManagedFn = func(string, string) (bool, error) { return true, nil } + // Stubbed even though provisioning no longer rotates: rotation moved to the + // caller, so nothing under test reaches this today. Left in so that if it + // ever moves back, these tests fail rather than resetting the password of a + // real managed account that happens to exist on the machine running them. + resetWindowsSandboxUserPasswordFn = func(string, string) error { + t.Fatal("provisioning must not rotate an account password; rotation belongs to the caller, immediately before the secret is written") + return nil + } ensureWindowsSandboxUserFn = func(string, string, string) (bool, error) { return existed, nil } - addWindowsSandboxUserToOfflineGroupFn = func(string) error { return offlineErr } addWindowsSandboxUserToGroupFn = func(string) error { return groupErr } addWindowsSandboxUserToOfflineGroupFn = func(string) error { return offlineErr } resolveWindowsSandboxSIDFn = func(username string) (*windows.SID, error) { diff --git a/internal/sandbox/windows_online_offline_test.go b/internal/sandbox/windows_online_offline_test.go index f9e32ed71..f49d9258c 100644 --- a/internal/sandbox/windows_online_offline_test.go +++ b/internal/sandbox/windows_online_offline_test.go @@ -99,8 +99,13 @@ func TestBuildWindowsNetworkInfraPlanIsModeIndependent(t *testing.T) { if err != nil { t.Fatalf("allow infra plan with the group present: %v", err) } - if len(withGroupDeny.IdentitySIDs) != 2 { - t.Fatalf("group present should add its SID, got %v", withGroupDeny.IdentitySIDs) + // Assert the SIDs themselves, not the count. A duplicate offline marker or an + // unrelated SID would satisfy a length check while meaning something quite + // different. + if len(withGroupDeny.IdentitySIDs) != 2 || + withGroupDeny.IdentitySIDs[0] != offline || + withGroupDeny.IdentitySIDs[1] != "S-1-5-32-9999" { + t.Fatalf("group present should add its SID after the offline marker, got %v (offline marker %q)", withGroupDeny.IdentitySIDs, offline) } groupDenyHash, _ := WindowsNetworkInfraHash(withGroupDeny) groupAllowHash, _ := WindowsNetworkInfraHash(withGroupAllow) From c82b3cc997f8bf04067f64198f6b8bed08037098 Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Fri, 31 Jul 2026 23:07:19 +0530 Subject: [PATCH 6/8] fix(sandbox): refuse an offline group zero does not own MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ensureWindowsLocalGroup accepted NERR_GroupExists and ERROR_ALIAS_EXISTS as success without inspecting the group it was about to reuse. Setup then resolved that group's SID and installed it on the persistent WFP deny filters, and made the sandbox principal a member of it. If anything else on the machine already owns a group named ZeroSandboxOffline — another tool, a policy, a prior unrelated convention — that is not a no-op. Every existing member abruptly loses outbound access, because the filters now name their group. In the other direction the sandbox principal inherits whatever permissions that group carries, which is the opposite of what an offline principal is for. The add now reports its raw status and the already-exists branch verifies the group carries this setup's managed marker before adopting it, failing with an actionable message otherwise. A lookup error fails closed rather than adopting. NetLocalGroupAdd and the ownership lookup sit behind seams so the branch is reachable in tests without an elevated machine; the marker compared is the group's own, so the principals group and the offline group cannot be confused. Reported by jatmn on #812. --- internal/sandbox/windows_identity_windows.go | 77 ++++++++++++++- ...ws_offline_group_ownership_windows_test.go | 96 +++++++++++++++++++ 2 files changed, 170 insertions(+), 3 deletions(-) create mode 100644 internal/sandbox/windows_offline_group_ownership_windows_test.go diff --git a/internal/sandbox/windows_identity_windows.go b/internal/sandbox/windows_identity_windows.go index 9a588ead2..38509abfe 100644 --- a/internal/sandbox/windows_identity_windows.go +++ b/internal/sandbox/windows_identity_windows.go @@ -101,6 +101,7 @@ func (role windowsSandboxRole) roleTag() string { const ( nerrSuccess = 0 nerrGroupExists = 2223 + nerrGroupNotFound = 2220 nerrUserExists = 2224 errorAliasExists = 1379 errorMemberInAlias = 1378 @@ -121,6 +122,7 @@ var ( netapi32 = windows.NewLazySystemDLL("netapi32.dll") procNetUserAdd = netapi32.NewProc("NetUserAdd") procNetLocalGroupAdd = netapi32.NewProc("NetLocalGroupAdd") + procNetLocalGroupGetInfo = netapi32.NewProc("NetLocalGroupGetInfo") procNetLocalGroupAddMembers = netapi32.NewProc("NetLocalGroupAddMembers") procNetUserDel = netapi32.NewProc("NetUserDel") procNetUserSetInfo = netapi32.NewProc("NetUserSetInfo") @@ -296,13 +298,47 @@ func resolveWindowsSandboxOfflineGroupSID() (string, error) { // ensureWindowsLocalGroup creates a local group, or leaves it alone when it // already exists. func ensureWindowsLocalGroup(groupName string, groupComment string) error { - name, err := windows.UTF16PtrFromString(groupName) + status, err := addWindowsLocalGroupFn(groupName, groupComment) if err != nil { return err } + if status == nerrGroupExists || status == errorAliasExists { + // A group with this name already exists, which is the normal re-run case + // — but only if it is OURS. The offline group's SID is installed on the + // persistent WFP deny filters and the sandbox principal is made a member + // of it, so silently adopting a same-named group created by some other + // tool or by policy would cut off every existing member's outbound + // traffic and hand our principal that group's permissions. + owned, err := windowsLocalGroupOwnedByZeroFn(groupName, groupComment) + if err != nil { + return err + } + if !owned { + return fmt.Errorf("local group %q already exists and is not managed by zero; "+ + "rename or remove it before running sandbox setup", groupName) + } + return nil + } + return netAPIStatus("NetLocalGroupAdd", status) +} + +// Seams for the two Win32 calls behind group creation, so the already-exists +// branch is reachable in tests without an elevated machine. +var ( + addWindowsLocalGroupFn = addWindowsLocalGroup + windowsLocalGroupOwnedByZeroFn = windowsLocalGroupOwnedByZero +) + +// addWindowsLocalGroup issues NetLocalGroupAdd and hands back its raw status so +// the caller can distinguish "already exists" from a real failure. +func addWindowsLocalGroup(groupName string, groupComment string) (uintptr, error) { + name, err := windows.UTF16PtrFromString(groupName) + if err != nil { + return 0, err + } comment, err := windows.UTF16PtrFromString(groupComment) if err != nil { - return err + return 0, err } info := localGroupInfo1{Name: name, Comment: comment} status, _, _ := procNetLocalGroupAdd.Call( @@ -316,7 +352,42 @@ func ensureWindowsLocalGroup(groupName string, groupComment string) error { runtime.KeepAlive(info) runtime.KeepAlive(name) runtime.KeepAlive(comment) - return netAPIStatus("NetLocalGroupAdd", status, nerrGroupExists, errorAliasExists) + return status, nil +} + +// windowsLocalGroupOwnedByZero reports whether an existing local group carries +// the managed-group marker this setup writes, so a foreign group that merely +// shares the name is never adopted. +func windowsLocalGroupOwnedByZero(groupName string, wantComment string) (bool, error) { + name, err := windows.UTF16PtrFromString(groupName) + if err != nil { + return false, err + } + var buffer *byte + status, _, _ := procNetLocalGroupGetInfo.Call( + 0, // local machine + uintptr(unsafe.Pointer(name)), + 1, // level: LOCALGROUP_INFO_1 + uintptr(unsafe.Pointer(&buffer)), + ) + runtime.KeepAlive(name) + if status == nerrGroupNotFound { + // It existed a moment ago and does not now. Treat that as not-ours + // rather than guessing; the next setup run recreates it cleanly. + return false, nil + } + if err := netAPIStatus("NetLocalGroupGetInfo", status); err != nil { + return false, err + } + if buffer == nil { + return false, nil + } + defer procNetApiBufferFree.Call(uintptr(unsafe.Pointer(buffer))) + info := (*localGroupInfo1)(unsafe.Pointer(buffer)) + if info.Comment == nil { + return false, nil + } + return windows.UTF16PtrToString(info.Comment) == wantComment, nil } // ensureWindowsSandboxUser creates a sandbox account with the supplied password. diff --git a/internal/sandbox/windows_offline_group_ownership_windows_test.go b/internal/sandbox/windows_offline_group_ownership_windows_test.go new file mode 100644 index 000000000..4ca357ba7 --- /dev/null +++ b/internal/sandbox/windows_offline_group_ownership_windows_test.go @@ -0,0 +1,96 @@ +//go:build windows + +package sandbox + +import ( + "errors" + "strings" + "testing" +) + +// The offline group's SID goes onto the persistent WFP deny filters and the +// sandbox principal is made a member of it. Adopting a same-named group that +// something else owns therefore cuts off every existing member's outbound +// traffic and hands our principal that group's permissions, so an unmarked +// group has to be refused rather than reused. +func TestEnsureWindowsLocalGroupRefusesForeignSameNameGroup(t *testing.T) { + for name, testCase := range map[string]struct { + status uintptr + owned bool + ownedErr error + wantError string + }{ + "foreign group with our name": { + status: nerrGroupExists, owned: false, + wantError: "not managed by zero", + }, + "foreign group reported via ERROR_ALIAS_EXISTS": { + status: errorAliasExists, owned: false, + wantError: "not managed by zero", + }, + "our own group on a re-run": { + status: nerrGroupExists, owned: true, + }, + "ownership lookup fails": { + status: nerrGroupExists, ownedErr: errors.New("lookup refused"), + wantError: "lookup refused", + }, + "group did not exist": { + status: nerrSuccess, + }, + } { + t.Run(name, func(t *testing.T) { + prevAdd, prevOwned := addWindowsLocalGroupFn, windowsLocalGroupOwnedByZeroFn + t.Cleanup(func() { + addWindowsLocalGroupFn = prevAdd + windowsLocalGroupOwnedByZeroFn = prevOwned + }) + addWindowsLocalGroupFn = func(string, string) (uintptr, error) { return testCase.status, nil } + lookups := 0 + windowsLocalGroupOwnedByZeroFn = func(string, string) (bool, error) { + lookups++ + return testCase.owned, testCase.ownedErr + } + + err := ensureWindowsSandboxOfflineGroup() + if testCase.wantError == "" { + if err != nil { + t.Fatalf("ensureWindowsSandboxOfflineGroup: %v", err) + } + return + } + if err == nil { + t.Fatalf("adopted a group it should have refused (lookups=%d)", lookups) + } + if !strings.Contains(err.Error(), testCase.wantError) { + t.Fatalf("error = %q, want it to mention %q", err, testCase.wantError) + } + }) + } +} + +// The marker compared against is the offline group's own comment, not the +// principals group's: two managed groups with different markers must not be +// mistaken for each other. +func TestEnsureWindowsLocalGroupChecksTheGroupsOwnMarker(t *testing.T) { + prevAdd, prevOwned := addWindowsLocalGroupFn, windowsLocalGroupOwnedByZeroFn + t.Cleanup(func() { + addWindowsLocalGroupFn = prevAdd + windowsLocalGroupOwnedByZeroFn = prevOwned + }) + addWindowsLocalGroupFn = func(string, string) (uintptr, error) { return nerrGroupExists, nil } + var gotName, gotComment string + windowsLocalGroupOwnedByZeroFn = func(name, comment string) (bool, error) { + gotName, gotComment = name, comment + return true, nil + } + if err := ensureWindowsSandboxOfflineGroup(); err != nil { + t.Fatalf("ensureWindowsSandboxOfflineGroup: %v", err) + } + if gotName != windowsSandboxOfflineGroupName { + t.Fatalf("checked group %q, want %q", gotName, windowsSandboxOfflineGroupName) + } + if gotComment != windowsSandboxOfflineGroupComment { + t.Fatalf("compared marker %q, want %q", gotComment, windowsSandboxOfflineGroupComment) + } +} From 90e2556c59e21af876158fb4de364918a514b957 Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Fri, 31 Jul 2026 23:10:18 +0530 Subject: [PATCH 7/8] fix(sandbox): recheck offline group membership before minting a token MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Network denial does not follow from picking the offline account. The WFP block filters match the offline GROUP'S SID, and LogonUser builds a token from the account's real memberships — so membership is the whole enforcement, and the command path never revalidated it. An account that drifts out of ZeroSandboxOffline through local policy, an administrator, or a re-setup that could not re-add it still resolves, still has its stored secret, and still logs on. Its token no longer satisfies the filter condition, so a NetworkDeny command gets full egress under a profile that asked for none. The stale setup marker keeps the whole path looking healthy. The offline role now confirms the membership its mode depends on before the secret is read, and falls back to the restricted token when it is absent. That direction is deliberate: the restricted token carries the offline marker the same filters match, so egress stays blocked, and only read confinement is lost. A failed lookup surfaces rather than downgrading silently. The online role is not checked, since it is not in that group by design. Reported by jatmn on #812. --- .../windows_identity_runtime_windows.go | 52 ++++++++- internal/sandbox/windows_identity_windows.go | 53 +++++++++ ...windows_offline_membership_windows_test.go | 102 ++++++++++++++++++ 3 files changed, 205 insertions(+), 2 deletions(-) create mode 100644 internal/sandbox/windows_offline_membership_windows_test.go diff --git a/internal/sandbox/windows_identity_runtime_windows.go b/internal/sandbox/windows_identity_runtime_windows.go index e9569991e..092745848 100644 --- a/internal/sandbox/windows_identity_runtime_windows.go +++ b/internal/sandbox/windows_identity_runtime_windows.go @@ -104,7 +104,7 @@ func windowsSandboxPrincipalToken(config WindowsSandboxCommandConfig) (windows.T // offline one is in the group the block filters match, so choosing here is // what enforces the mode. role := windowsSandboxRoleForNetwork(config.PermissionProfile.Network.Mode) - identity, err := lookupWindowsSandboxPrincipalForCommand(key, role) + identity, err := lookupWindowsSandboxPrincipalForCommandFn(key, role) if err != nil { if errors.Is(err, errWindowsSandboxIdentityUnavailable) { // Not provisioned: fall back quietly, this is the default state. @@ -115,11 +115,31 @@ func windowsSandboxPrincipalToken(config WindowsSandboxCommandConfig) (windows.T // operator has to see, not a reason to pretend setup never ran. return 0, false, err } + // Selecting the offline account is only half of what denies it the network. + // The WFP filters match the OFFLINE GROUP'S SID, so an account that has + // drifted out of that group — local policy, an administrator, a re-setup that + // could not re-add it — still logs on from a stored secret and its token no + // longer satisfies the filter condition. It would get full egress under a + // profile that asked for none. Re-check the membership the mode depends on + // rather than trusting the marker written when setup last succeeded. + if role == windowsSandboxRoleOffline { + member, err := windowsSandboxUserInLocalGroupFn(identity.Username, windowsSandboxOfflineGroupName) + if err != nil { + return 0, false, err + } + if !member { + // Fail closed toward the weaker-but-still-denied path: the restricted + // token carries the offline marker the same filters match, so egress + // stays blocked. Handing back this principal token would not. + warnWindowsSandboxOfflineMembershipMissing(identity.Username) + return 0, false, nil + } + } secretPath, err := windowsSandboxSecretPath(config.SandboxHome, identity.Username) if err != nil { return 0, false, err } - password, err := readWindowsSandboxSecret(secretPath) + password, err := readWindowsSandboxSecretFn(secretPath) if err != nil { if errors.Is(err, errWindowsSandboxIdentityUnavailable) { // The account exists but its password does not. Setup was interrupted @@ -643,3 +663,31 @@ var ( removeWindowsSandboxSecretFn = removeWindowsSandboxSecret writeWindowsSandboxSecretFn = writeWindowsSandboxSecret ) + +// warnWindowsSandboxOfflineMembershipMissing reports the one drift that would +// otherwise silently hand a no-network profile full egress. +// +// Separate from the missing-secret warning because the remedy differs and +// because this one is a containment failure rather than a setup gap: the +// account is fine, its group membership is not. Once per process, for the same +// reason as its sibling — this sits on the command path. +var warnWindowsSandboxOfflineMembershipMissing = func(username string) { + windowsSandboxOfflineMembershipWarnOnce.Do(func() { + fmt.Fprintf(os.Stderr, + "[zero] sandbox principal %q is no longer a member of %q, which is the group the network "+ + "block filters match. Falling back to the restricted-token sandbox, which still denies "+ + "the network but does not confine reads. "+ + "Re-run `zero sandbox setup` from an elevated terminal to restore it.\n", + username, windowsSandboxOfflineGroupName) + }) +} + +var windowsSandboxOfflineMembershipWarnOnce sync.Once + +// Seam for the principal lookup, so the command path's mode-enforcement checks +// are reachable in tests without a provisioned machine. +var lookupWindowsSandboxPrincipalForCommandFn = lookupWindowsSandboxPrincipalForCommand + +// Seam for the secret read on the command path, so the mode-enforcement gates +// ahead of it can be exercised without a provisioned secret on disk. +var readWindowsSandboxSecretFn = readWindowsSandboxSecret diff --git a/internal/sandbox/windows_identity_windows.go b/internal/sandbox/windows_identity_windows.go index 38509abfe..25f5d672a 100644 --- a/internal/sandbox/windows_identity_windows.go +++ b/internal/sandbox/windows_identity_windows.go @@ -922,3 +922,56 @@ func classifyWindowsSandboxLookupError(err error) error { } return err } + +// windowsSandboxUserInLocalGroup reports direct membership of one named local +// group. Network denial is keyed to the offline group's SID on the WFP filters, +// so the command path uses this to confirm the account it is about to log on +// still carries the membership that makes those filters apply to it. +// +// Indirected through a var so the command path can be tested without an +// elevated machine. +var windowsSandboxUserInLocalGroupFn = windowsSandboxUserInLocalGroup + +func windowsSandboxUserInLocalGroup(username string, groupName string) (bool, error) { + name, err := windows.UTF16PtrFromString(username) + if err != nil { + return false, err + } + var ( + buffer *byte + entries uint32 + total uint32 + ) + status, _, _ := procNetUserGetLocalGroups.Call( + 0, // local machine + uintptr(unsafe.Pointer(name)), + 0, // level: LOCALGROUP_USERS_INFO_0 + 0, // flags: direct membership only + uintptr(unsafe.Pointer(&buffer)), + uintptr(^uint32(0)), // MAX_PREFERRED_LENGTH + uintptr(unsafe.Pointer(&entries)), + uintptr(unsafe.Pointer(&total)), + ) + runtime.KeepAlive(name) + if status == nerrUserNotFound { + return false, nil + } + if err := netAPIStatus("NetUserGetLocalGroups", status); err != nil { + return false, err + } + if buffer == nil || entries == 0 { + return false, nil + } + defer procNetApiBufferFree.Call(uintptr(unsafe.Pointer(buffer))) + want := strings.ToLower(groupName) + groups := unsafe.Slice((*localGroupUsersInfo0)(unsafe.Pointer(buffer)), entries) + for _, group := range groups { + if group.Name == nil { + continue + } + if strings.ToLower(windows.UTF16PtrToString(group.Name)) == want { + return true, nil + } + } + return false, nil +} diff --git a/internal/sandbox/windows_offline_membership_windows_test.go b/internal/sandbox/windows_offline_membership_windows_test.go new file mode 100644 index 000000000..e41fe09c1 --- /dev/null +++ b/internal/sandbox/windows_offline_membership_windows_test.go @@ -0,0 +1,102 @@ +//go:build windows + +package sandbox + +import ( + "errors" + "testing" + + "golang.org/x/sys/windows" +) + +// Choosing the offline account is only half of what denies it the network: the +// WFP filters match the offline GROUP'S SID. An account that has drifted out of +// that group still logs on from its stored secret, and its token no longer +// satisfies the filter condition — so a no-network profile would get full +// egress. The command path has to re-check the membership, not trust the marker +// setup wrote when it last succeeded. +func TestPrincipalTokenRechecksOfflineGroupMembership(t *testing.T) { + for name, testCase := range map[string]struct { + mode NetworkMode + member bool + memberErr error + wantChecked bool + wantReached bool + wantErr bool + }{ + "offline principal drifted out of the group": { + mode: NetworkDeny, member: false, + wantChecked: true, wantReached: false, + }, + "offline principal still a member": { + mode: NetworkDeny, member: true, + wantChecked: true, wantReached: true, + }, + "membership lookup fails": { + mode: NetworkDeny, memberErr: errors.New("group lookup refused"), + wantChecked: true, wantReached: false, wantErr: true, + }, + // An allow-network command uses the online principal, which is not in the + // offline group by design. Checking it there would refuse every approved + // network command. + "online principal is not checked": { + mode: NetworkAllow, member: false, + wantChecked: false, wantReached: true, + }, + } { + t.Run(name, func(t *testing.T) { + prevLookup := lookupWindowsSandboxPrincipalForCommandFn + prevMember := windowsSandboxUserInLocalGroupFn + prevSecret := readWindowsSandboxSecretFn + prevWarn := warnWindowsSandboxOfflineMembershipMissing + t.Cleanup(func() { + lookupWindowsSandboxPrincipalForCommandFn = prevLookup + windowsSandboxUserInLocalGroupFn = prevMember + readWindowsSandboxSecretFn = prevSecret + warnWindowsSandboxOfflineMembershipMissing = prevWarn + }) + + sid, err := windows.CreateWellKnownSid(windows.WinLocalSystemSid) + if err != nil { + t.Fatal(err) + } + lookupWindowsSandboxPrincipalForCommandFn = func(string, windowsSandboxRole) (windowsSandboxIdentity, error) { + return windowsSandboxIdentity{Username: "zero-sbx-test", SID: sid}, nil + } + checkedGroup := "" + windowsSandboxUserInLocalGroupFn = func(_ string, group string) (bool, error) { + checkedGroup = group + return testCase.member, testCase.memberErr + } + secretRead := false + readWindowsSandboxSecretFn = func(string) (string, error) { + secretRead = true + return "", errWindowsSandboxIdentityUnavailable + } + warnWindowsSandboxOfflineMembershipMissing = func(string) {} + + config := windowsSandboxTestConfig() + config.Env = map[string]string{windowsSandboxIdentityEnv: "1"} + config.PermissionProfile.Network.Mode = testCase.mode + + _, ok, err := windowsSandboxPrincipalToken(config) + if testCase.wantErr { + if err == nil { + t.Fatal("a failed membership lookup was swallowed") + } + } else if err != nil { + t.Fatalf("windowsSandboxPrincipalToken: %v", err) + } + _ = ok + if secretRead != testCase.wantReached { + t.Fatalf("reached the secret read = %v, want %v (gate must short-circuit before it)", secretRead, testCase.wantReached) + } + if checked := checkedGroup != ""; checked != testCase.wantChecked { + t.Fatalf("membership checked = %v, want %v", checked, testCase.wantChecked) + } + if testCase.wantChecked && checkedGroup != windowsSandboxOfflineGroupName { + t.Fatalf("checked group %q, want %q", checkedGroup, windowsSandboxOfflineGroupName) + } + }) + } +} From b88ac4693839deebfc06e082fb766dc298dcef4e Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Sat, 1 Aug 2026 20:53:59 +0530 Subject: [PATCH 8/8] fix(sandbox): derive setup's runtime root deterministically or not at all MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit windowsSandboxRuntimeRootPath resolved through sandboxRuntimeRootFor, which falls back to os.MkdirTemp when the user cache lives inside the workspace and memoizes that only in-process. Elevated setup is its own process. It granted the principals an ACE on temp root A; the next command, being a new process, derived temp root B, where the principal has no ACE, and failed ordinary cache writes with a bare ACCESS_DENIED and nothing pointing at the sandbox. Teardown, a third process, cleaned a third directory. The three callers that have to agree exactly could not agree at all. Setup now uses the same side-effect-free derivation teardown already used, and reports no runtime root when that derivation is unusable rather than inventing one. A root only the granting process can name is worse than no root: the principal loses the runtime tree, which is a degraded sandbox, instead of the sandbox appearing provisioned while every command fails. TestTeardownPathDerivationCreatesNothing asserted the opposite — that setup "should still fall back to a usable tree" — so it is inverted here, with the reasoning recorded in the test. That assertion encoded the assumption this finding overturns: a per-process temp tree is not usable. Restoring the fallback fails it with the invented path in the message, and a new TestSetupAndTeardownDeriveTheSameRuntimeRoot pins the ordinary case, so "report none" cannot quietly become the answer everywhere. Reported by jatmn on #812. --- .../windows_identity_runtime_windows.go | 24 +++++++-- ...indows_workspace_canonical_windows_test.go | 50 +++++++++++++++++-- 2 files changed, 67 insertions(+), 7 deletions(-) diff --git a/internal/sandbox/windows_identity_runtime_windows.go b/internal/sandbox/windows_identity_runtime_windows.go index 092745848..644961a25 100644 --- a/internal/sandbox/windows_identity_runtime_windows.go +++ b/internal/sandbox/windows_identity_runtime_windows.go @@ -484,13 +484,31 @@ func windowsSandboxRuntimeRootPath(config WindowsSandboxCommandConfig) (string, if err != nil { return "", fmt.Errorf("resolve user cache directory for sandbox runtime: %w", err) } - // Same canonicalization as the workspace root above: sandboxRuntimeRootFor - // compares them, so they have to be the same spelling of the same path. + // Same canonicalization as the workspace root above: the derivation compares + // them, so they have to be the same spelling of the same path. cacheRoot = canonicalSandboxWorkspaceRoot(cacheRoot) if cacheRoot == "" || cacheRoot == "." { return "", errors.New("user cache directory is unavailable for sandbox runtime") } - return sandboxRuntimeRootFor(workspaceRoot, cacheRoot) + // Deliberately the deterministic derivation and NOT sandboxRuntimeRootFor. + // + // sandboxRuntimeRootFor falls back to os.MkdirTemp when the cache lives + // inside the workspace, and memoizes that only in-process. Elevated setup is + // its own process, so it would grant the principals an ACE on temp root A, + // and the next command — a new process — would derive temp root B and hit + // ACCESS_DENIED on ordinary cache writes with nothing pointing at the cause. + // Teardown, being a third process, would clean a third directory. + // + // When the deterministic root is unusable there is no name both sides can + // agree on, so this reports "no runtime root" rather than inventing one. The + // principal then simply gets no ACE here: it loses the runtime tree, which is + // a degraded sandbox rather than a broken one, and it is the same answer + // teardown already gives. + root, ok := deterministicSandboxRuntimeRoot(workspaceRoot, cacheRoot) + if !ok { + return "", nil + } + return root, nil } // windowsSandboxDeterministicRuntimeRootPath names the cache-derived runtime diff --git a/internal/sandbox/windows_workspace_canonical_windows_test.go b/internal/sandbox/windows_workspace_canonical_windows_test.go index 0997c3113..e986e2f31 100644 --- a/internal/sandbox/windows_workspace_canonical_windows_test.go +++ b/internal/sandbox/windows_workspace_canonical_windows_test.go @@ -202,16 +202,58 @@ func TestTeardownPathDerivationCreatesNothing(t *testing.T) { t.Errorf("temp directory gained %d entries; naming the paths must not create one", after-before) } - // And the setup resolver, which is allowed to create, still does. - created, err := windowsSandboxRuntimeRootPath(WindowsSandboxCommandConfig{ + // Setup's resolver must agree with teardown's, and create nothing either. + // + // This used to assert the opposite — that setup falls back to a usable tree — + // on the reasoning that some runtime root beats none. That was wrong: the + // fallback is os.MkdirTemp memoized only in-process, so elevated setup would + // grant the principal an ACE on temp root A, the next command would derive + // root B and fail ACCESS_DENIED on ordinary cache writes, and teardown would + // clean a third. A root only setup can name is worse than no root, because + // the sandbox looks provisioned and is not. + beforeSetup := tempDirEntryCount(t) + setupRoot, err := windowsSandboxRuntimeRootPath(WindowsSandboxCommandConfig{ WorkspaceRoots: []string{workspace}, CommandCWD: workspace, }) if err != nil { t.Fatalf("windowsSandboxRuntimeRootPath: %v", err) } - if created == "" { - t.Error("setup's resolver should still fall back to a usable tree") + if setupRoot != "" { + t.Errorf("setup named %q for a workspace whose runtime root is underivable; it must report none rather than invent one", setupRoot) + } + if after := tempDirEntryCount(t); after != beforeSetup { + t.Errorf("setup's resolver created %d temp entries; it must create nothing", after-beforeSetup) + } +} + +// Setup and teardown must derive the SAME root in the ordinary case, since one +// grants the ACE the other revokes. This is the case the fix above must not +// break: reporting "no root" is only correct when the root is underivable. +func TestSetupAndTeardownDeriveTheSameRuntimeRoot(t *testing.T) { + workspace := t.TempDir() + cacheRoot := t.TempDir() // outside the workspace, so the derivation is usable + previous := sandboxUserCacheDir + sandboxUserCacheDir = func() (string, error) { return cacheRoot, nil } + t.Cleanup(func() { sandboxUserCacheDir = previous }) + + setupRoot, err := windowsSandboxRuntimeRootPath(WindowsSandboxCommandConfig{ + WorkspaceRoots: []string{workspace}, + CommandCWD: workspace, + }) + if err != nil { + t.Fatalf("windowsSandboxRuntimeRootPath: %v", err) + } + if setupRoot == "" { + t.Fatal("setup named no runtime root for a derivable workspace") + } + teardownRoot, ok := deterministicSandboxRuntimeRoot( + canonicalSandboxWorkspaceRoot(workspace), canonicalSandboxWorkspaceRoot(cacheRoot)) + if !ok { + t.Fatal("precondition: the deterministic root should be usable here") + } + if setupRoot != teardownRoot { + t.Errorf("setup grants on %q but teardown revokes %q", setupRoot, teardownRoot) } }