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_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..25210b76d 100644 --- a/internal/sandbox/windows_identity_rollback_windows_test.go +++ b/internal/sandbox/windows_identity_rollback_windows_test.go @@ -13,23 +13,39 @@ 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 + 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 } + 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 } + // 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 } 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 +62,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 +88,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 +99,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 e03d0daa0..53e75e15f 100644 --- a/internal/sandbox/windows_identity_runtime_windows.go +++ b/internal/sandbox/windows_identity_runtime_windows.go @@ -55,17 +55,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 @@ -96,7 +105,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 := lookupWindowsSandboxPrincipalForCommandFn(key, role) if err != nil { if errors.Is(err, errWindowsSandboxIdentityUnavailable) { // Not provisioned. On the restricted-token tier the marker check has @@ -113,11 +126,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 @@ -182,6 +215,19 @@ var warnWindowsSandboxPrincipalNotUsed = func(reason string) { var windowsSandboxPrincipalNotUsedWarnOnce 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`. @@ -189,9 +235,9 @@ var windowsSandboxPrincipalNotUsedWarnOnce 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 @@ -207,7 +253,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, @@ -256,7 +302,7 @@ func provisionWindowsSandboxPrincipalForSetup(config WindowsSandboxCommandConfig _ = revokeWindowsSandboxLogonRightsFn(identity.SID) } if created { - _ = removeWindowsSandboxIdentity(identity.Username) + _ = removeWindowsSandboxIdentity(identity.Username, key) } return cleanupErr } @@ -305,110 +351,145 @@ 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) { - username := windowsSandboxUserName(windowsSandboxWorkspaceKey(config.WorkspaceRoots)) - // Retire a principal whose grants were never recorded, BEFORE provisioning - // adopts it. - // - // This is the one case where the prior grant set is not empty but unknowable: - // an account from an earlier setup exists, and nothing on Windows can - // enumerate the paths whose DACL names its SID. Carrying on would revoke only - // what the new plan happens to name and leave the rest — the fail-open this - // record exists to close, reached on the single path where it cannot be ruled - // out. - // - // Retiring is a real fix rather than a gesture because Windows never reuses a - // deleted local account's RID: whatever ACEs survive name a principal that no - // longer exists and grant access to nobody, and the SID minted below is one - // no DACL on this machine can already carry. It also needs no new operator - // action, which matters — there is no `zero sandbox teardown` to send anyone - // to, so refusing here would strand the workspace instead of fixing it. - if _, recorded := readWindowsPrincipalACLLedger(config.SandboxHome, username); !recorded { - if err := retireUnrecordedWindowsSandboxPrincipal(config); err != nil { - return nil, err - } - } - 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 + key := windowsSandboxWorkspaceKey(config.WorkspaceRoots) + // 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(config.SandboxHome, username, 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 { + username := windowsSandboxUserName(key, role) + // Retire a principal whose grants were never recorded, BEFORE provisioning + // adopts it. + // + // This is the one case where the prior grant set is not empty but unknowable: + // an account from an earlier setup exists, and nothing on Windows can + // enumerate the paths whose DACL names its SID. Carrying on would revoke only + // what the new plan happens to name and leave the rest — the fail-open the + // record exists to close, reached on the single path where it cannot be ruled + // out. + // + // Retiring is a real fix rather than a gesture because Windows never reuses a + // deleted local account's RID: whatever ACEs survive name a principal that no + // longer exists and grant access to nobody, and the SID minted below is one + // no DACL on this machine can already carry. It also needs no new operator + // action, which matters — there is no `zero sandbox teardown` to send anyone + // to, so refusing here would strand the workspace instead of fixing it. + // + // Per role, like everything else in this loop: the two principals are + // separate accounts with separate records, and one having lost its record + // says nothing about the other. + if _, recorded := readWindowsPrincipalACLLedger(config.SandboxHome, username); !recorded { + if err := retireUnrecordedWindowsSandboxPrincipal(config, role); err != nil { + _ = rollback() + return nil, err + } } - return removeErr - }, nil + identity, created, err := provisionWindowsSandboxPrincipalForSetupFn(config, role) + if err != nil { + _ = rollback() + return nil, err + } + // 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 removeWindowsSandboxPrincipalForSetupFn(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 AND + // on the paths this role's record names, 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 with separate records, and + // revoking one must not disturb the other. + revertACL, err := applyWindowsPrincipalACLs(config.SandboxHome, username, 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 } -// retireUnrecordedWindowsSandboxPrincipal removes this workspace's principal -// when one exists, and does nothing when one does not. +// retireUnrecordedWindowsSandboxPrincipal removes this role's principal when one +// exists, and does nothing when one does not. // // The absent case is the ordinary one and is not a problem: with no account // there is nothing that could be holding an ACE, so a missing record is simply // a machine where setup has not run yet. -func retireUnrecordedWindowsSandboxPrincipal(config WindowsSandboxCommandConfig) error { - _, err := lookupWindowsSandboxIdentityFn(windowsSandboxWorkspaceKey(config.WorkspaceRoots)) +// +// Scoped to one role. The other role is a separate account holding its own ACEs +// under its own record, so retiring both because one lost its record would +// destroy a principal there is nothing wrong with. +func retireUnrecordedWindowsSandboxPrincipal(config WindowsSandboxCommandConfig, role windowsSandboxRole) error { + _, err := lookupWindowsSandboxIdentityFn(windowsSandboxWorkspaceKey(config.WorkspaceRoots), role) if err != nil { if errors.Is(err, errWindowsSandboxIdentityUnavailable) { return nil } return err } - return removeWindowsSandboxPrincipalForSetupFn(config) + return removeWindowsSandboxPrincipalForSetupFn(config, role) } -// 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 @@ -421,12 +502,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 @@ -436,7 +519,7 @@ func removeWindowsSandboxPrincipalForSetup(config WindowsSandboxCommandConfig) e // The rollback is discarded here on purpose, unlike at setup: this is // teardown, the account is about to be deleted, and putting its ACEs back // is the opposite of what the caller asked for. - if paths, pathsErr := windowsPrincipalRevocationPaths(config, identity.SID.String()); pathsErr == nil { + if paths, pathsErr := windowsPrincipalRevocationPaths(config, identity.SID.String(), role); pathsErr == nil { _, _ = revokeWindowsPrincipalACEs(identity.SID.String(), paths) } if err := revokeWindowsSandboxLogonRights(identity.SID); err != nil { @@ -445,11 +528,23 @@ func removeWindowsSandboxPrincipalForSetup(config WindowsSandboxCommandConfig) e } else if !errors.Is(err, errWindowsSandboxIdentityUnavailable) { return err } - if err := removeWindowsSandboxIdentity(username); err != nil { - return err - } - // Last, and only once the account is actually gone, so a failure anywhere - // above leaves the record describing a principal that still exists. + // 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, key); err != nil { + if !errors.Is(err, errWindowsSandboxForeignAccountRetained) { + return err + } + // The record is KEPT in that case. It names paths this role's principal + // was granted, and a foreign account holding the name is not evidence + // those grants are gone — dropping the record would leave them + // unrevokable, which is the whole failure it exists to prevent. A record + // with no principal is revoked harmlessly by the next setup. + return nil + } + // Otherwise last, and only once the account is actually gone, so a failure + // anywhere above leaves the record describing a principal that still exists. // // It describes grants for a SID that no longer resolves, and leaving it would // have the next setup revoke those paths on behalf of a freshly minted SID @@ -486,13 +581,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 @@ -709,13 +822,15 @@ func windowsPrincipalTeardownPaths(config WindowsSandboxCommandConfig, principal // plan, so retiring the principal revoked every ACE except the one that was // widening the sandbox — and then deleted the account, leaving that ACE naming a // SID nothing could resolve to clean it up later. -func windowsPrincipalRevocationPaths(config WindowsSandboxCommandConfig, principalSID string) ([]string, error) { +func windowsPrincipalRevocationPaths(config WindowsSandboxCommandConfig, principalSID string, role windowsSandboxRole) ([]string, error) { current, err := windowsPrincipalTeardownPaths(config, principalSID) if err != nil { return nil, err } + // This role's record, not the workspace's: the other role is a separate + // trustee whose recorded paths are none of this revocation's business. recorded, _ := readWindowsPrincipalACLLedger( - config.SandboxHome, windowsSandboxUserName(windowsSandboxWorkspaceKey(config.WorkspaceRoots))) + config.SandboxHome, windowsSandboxUserName(windowsSandboxWorkspaceKey(config.WorkspaceRoots), role)) return unionWindowsPrincipalACLPaths(recorded, current), nil } @@ -727,11 +842,41 @@ var ( writeWindowsSandboxSecretFn = writeWindowsSandboxSecret ) -// Seams for the two elevated calls the unrecorded-principal retirement depends -// on, so the decision to retire is observable in a test without a provisioned -// machine — on which the lookup declines for its own reasons and would report -// success whether or not the guard existed. -var ( - lookupWindowsSandboxIdentityFn = lookupWindowsSandboxIdentity - removeWindowsSandboxPrincipalForSetupFn = removeWindowsSandboxPrincipalForSetup -) +// 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 + +// Seam for the lookup the unrecorded-principal retirement decides on, so that +// decision is observable in a test without a provisioned machine — on which the +// lookup declines for its own reasons and would report success whether or not +// the guard existed. +// +// The retirement's other elevated call, removeWindowsSandboxPrincipalForSetup, +// is seamed with the dual-role rollback seams above rather than here, for the +// reason recorded there: two declarations of one seam let a test stub one while +// production uses the other. +var lookupWindowsSandboxIdentityFn = lookupWindowsSandboxIdentity diff --git a/internal/sandbox/windows_identity_runtime_windows_test.go b/internal/sandbox/windows_identity_runtime_windows_test.go index 9b7bc5ae5..01b5e8cee 100644 --- a/internal/sandbox/windows_identity_runtime_windows_test.go +++ b/internal/sandbox/windows_identity_runtime_windows_test.go @@ -206,10 +206,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(), @@ -220,15 +220,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") - } - // 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") + 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) + } } - if eligible(NetworkAllow, "0") { - t.Fatal("principal backend eligible without the opt-in") +} + +// 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) + } + if got := windowsSandboxRoleForNetwork(NetworkAllow); got != windowsSandboxRoleOnline { + t.Fatalf("allow selected %q, want the online principal", got) + } + // 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..25f5d672a 100644 --- a/internal/sandbox/windows_identity_windows.go +++ b/internal/sandbox/windows_identity_windows.go @@ -54,13 +54,54 @@ 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 ( nerrSuccess = 0 nerrGroupExists = 2223 + nerrGroupNotFound = 2220 nerrUserExists = 2224 errorAliasExists = 1379 errorMemberInAlias = 1378 @@ -81,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") @@ -136,12 +178,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 +202,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,14 +255,91 @@ 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 { - return err + 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) } - comment, err := windows.UTF16PtrFromString(windowsSandboxGroupComment) + return sid.String(), nil +} + +// ensureWindowsLocalGroup creates a local group, or leaves it alone when it +// already exists. +func ensureWindowsLocalGroup(groupName string, groupComment string) error { + 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 0, err + } info := localGroupInfo1{Name: name, Comment: comment} status, _, _ := procNetLocalGroupAdd.Call( 0, // local machine @@ -228,7 +352,42 @@ func ensureWindowsSandboxGroup() 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. @@ -311,7 +470,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 +673,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 +701,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 +776,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 @@ -611,15 +804,42 @@ func provisionWindowsSandboxIdentity(workspaceKey string) (windowsSandboxIdentit // // 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. + // 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 + } + if !managed { + // 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 { return err } status, _, _ := procNetUserDel.Call(0, uintptr(unsafe.Pointer(name))) + runtime.KeepAlive(name) 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. @@ -629,21 +849,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 +885,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 } @@ -715,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_identity_windows_test.go b/internal/sandbox/windows_identity_windows_test.go index 11d64c741..9c8cdde0f 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), "ziptest01") - identity, password, _, err := provisionWindowsSandboxIdentity("ziptest01") + identity, password, _, err := provisionWindowsSandboxIdentity("ziptest01", windowsSandboxRoleOffline) if err != nil { t.Fatalf("provision: %v", err) } @@ -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, "ziptest01"); err != nil && !errors.Is(err, errWindowsSandboxForeignAccountRetained) { t.Errorf("cleanup: remove principal: %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) } @@ -284,10 +284,10 @@ 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") + 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), key) - identity, password, _, err := provisionWindowsSandboxIdentity(key) + identity, password, _, err := provisionWindowsSandboxIdentity(key, windowsSandboxRoleOffline) if err != nil { t.Fatalf("provision: %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, key); err != nil && !errors.Is(err, errWindowsSandboxForeignAccountRetained) { t.Errorf("cleanup: remove principal: %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..72f289a8d 100644 --- a/internal/sandbox/windows_network.go +++ b/internal/sandbox/windows_network.go @@ -62,15 +62,119 @@ 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 trimmed := strings.TrimSpace(groupSID); trimmed != "" { + identitySIDs = append(identitySIDs, trimmed) + } + } return WindowsNetworkPlan{ Mode: NetworkDeny, ProviderKey: windowsWFPProviderKey, SubLayerKey: windowsWFPSubLayerKey, - IdentitySIDs: []string{offlineSID}, + IdentitySIDs: identitySIDs, Filters: windowsDenyWFPFilterSpecs(), }, 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 +} + +// assertWindowsNetworkPlanCoversOfflineGroup is the post-provisioning check that +// setup runs before it installs filters and reports success. +// +// It fails closed on every answer that is not a definite yes, because the thing +// it guards is invisible when it goes wrong: filters that do not name the +// offline group leave every offline principal with an open network on a machine +// whose setup marker says it is protected. Nobody sees an error, and the sandbox +// looks correctly installed. +// +// Two answers other than "the plan omits the group" also mean the check did not +// happen and must not be treated as a pass: +// +// - The lookup failed. Whether the group is covered is then unknown, and the +// Win32 reason is worth surfacing: an operator seeing "access is denied" +// knows to re-run elevated. +// - The lookup succeeded and found nothing. ("", nil) means the group does not +// exist, which is the ordinary state BEFORE provisioning and an impossible +// one after it, so here it means the group vanished or was never created. +// WindowsNetworkPlanCoversPrincipals answers true for an empty SID, correctly +// for the pre-provisioning callers that ask it, so the emptiness has to be +// rejected here rather than delegated to it. +// +// provisioned says whether principals were actually provisioned on this run. +// When they were not there is no offline group, no principal carrying it, and +// nothing for the filters to miss, so there is nothing to assert. Running the +// check anyway turns the ordinary opt-out setup into a hard failure, because a +// machine with no group resolves to ("", nil) and the empty-SID rejection below +// is correct only after provisioning. +func assertWindowsNetworkPlanCoversOfflineGroup(plan WindowsNetworkPlan, resolve func() (string, error), provisioned bool) error { + if !provisioned { + return nil + } + if resolve == nil { + return errors.New("sandbox offline group resolver is not wired up, so filter coverage cannot be verified") + } + groupSID, err := resolve() + if err != nil { + return fmt.Errorf("resolve the sandbox offline group after provisioning, so filter coverage cannot be verified: %w", err) + } + if strings.TrimSpace(groupSID) == "" { + return errors.New("the sandbox offline group does not exist after provisioning, so the block filters would not apply to any sandbox principal") + } + if !WindowsNetworkPlanCoversPrincipals(plan, groupSID) { + return 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") + } + return 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_coverage_assert_test.go b/internal/sandbox/windows_network_coverage_assert_test.go new file mode 100644 index 000000000..7b07176b9 --- /dev/null +++ b/internal/sandbox/windows_network_coverage_assert_test.go @@ -0,0 +1,129 @@ +package sandbox + +import ( + "errors" + "strings" + "testing" +) + +// The post-provisioning coverage assertion is the last thing standing between a +// mis-ordered plan and a machine that reports a successful setup while every +// offline principal has an open network. It has to fail closed on every way of +// not knowing the answer, not just on a definite negative. +func TestAssertWindowsNetworkPlanCoversOfflineGroupFailsClosed(t *testing.T) { + const groupSID = "S-1-5-32-9999" + covering := WindowsNetworkPlan{IdentitySIDs: []string{"S-1-5-21-marker", groupSID}} + notCovering := WindowsNetworkPlan{IdentitySIDs: []string{"S-1-5-21-marker"}} + lookupFailed := errors.New("look up zero-sandbox-offline: access is denied") + + tests := []struct { + name string + plan WindowsNetworkPlan + resolve func() (string, error) + wantErr bool + wantText string + }{ + { + name: "plan names the group", + plan: covering, + resolve: func() (string, error) { return groupSID, nil }, + }, + { + // The finding anandh8x raised: a failed lookup skipped the assertion + // entirely and setup carried on to write a success marker. + name: "lookup fails", + plan: covering, + resolve: func() (string, error) { return "", lookupFailed }, + wantErr: true, + wantText: "access is denied", + }, + { + // Empty-and-no-error means "the group does not exist", which is a + // legitimate answer BEFORE provisioning and an impossible one after it. + // WindowsNetworkPlanCoversPrincipals answers true for an empty SID, so + // without an explicit check the assertion passes vacuously. + name: "group missing after provisioning", + plan: covering, + resolve: func() (string, error) { return "", nil }, + wantErr: true, + wantText: "does not exist", + }, + { + name: "whitespace-only SID", + plan: covering, + resolve: func() (string, error) { return " ", nil }, + wantErr: true, + wantText: "does not exist", + }, + { + name: "plan omits the group", + plan: notCovering, + resolve: func() (string, error) { return groupSID, nil }, + wantErr: true, + wantText: "do not name the sandbox offline group", + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + err := assertWindowsNetworkPlanCoversOfflineGroup(test.plan, test.resolve, true) + if test.wantErr && err == nil { + t.Fatalf("assertWindowsNetworkPlanCoversOfflineGroup() = nil, want an error so setup rolls back") + } + if !test.wantErr { + if err != nil { + t.Fatalf("assertWindowsNetworkPlanCoversOfflineGroup() = %v, want nil", err) + } + return + } + if !strings.Contains(err.Error(), test.wantText) { + t.Fatalf("error = %q, want it to mention %q so the operator can tell the cases apart", err, test.wantText) + } + }) + } + + // The lookup failure has to stay unwrapped-comparable: setup logs it and an + // operator needs the underlying Win32 reason, not a flattened string. + err := assertWindowsNetworkPlanCoversOfflineGroup(covering, func() (string, error) { return "", lookupFailed }, true) + if !errors.Is(err, lookupFailed) { + t.Fatalf("errors.Is(err, lookupFailed) = false; the cause was dropped: %v", err) + } +} + +// The assert only means something AFTER principals were provisioned. The offline +// group is created inside provisionWindowsSandboxIdentity, which runs only under +// the ZERO_WINDOWS_SANDBOX_IDENTITY opt-in, so on a default machine there is no +// group, no principal, and nothing for the filters to miss. +// +// Rejecting an empty SID is right once provisioning ran (that is the vacuous +// pass anandh8x found) and wrong before it: it turned the ordinary opt-out setup +// into a hard failure, which is the regression jatmn caught on #812. +func TestAssertWindowsNetworkPlanCoversOfflineGroupSkipsWhenNothingWasProvisioned(t *testing.T) { + plan := WindowsNetworkPlan{IdentitySIDs: []string{"S-1-5-21-marker"}} + + // The default machine: no group exists, so the resolver reports ("", nil). + if err := assertWindowsNetworkPlanCoversOfflineGroup(plan, func() (string, error) { return "", nil }, false); err != nil { + t.Fatalf("opt-out setup was refused: %v", err) + } + // Same state WITH provisioning claimed: now the missing group is a real fault. + if err := assertWindowsNetworkPlanCoversOfflineGroup(plan, func() (string, error) { return "", nil }, true); err == nil { + t.Fatal("a missing group after provisioning must still fail closed") + } + // Not provisioned must not become a licence to skip a lookup failure either: + // the resolver is never consulted, so a broken lookup cannot matter here. + if err := assertWindowsNetworkPlanCoversOfflineGroup(plan, func() (string, error) { + t.Fatal("the resolver must not be consulted when nothing was provisioned") + return "", nil + }, false); err != nil { + t.Fatalf("opt-out setup was refused: %v", err) + } +} + +// A nil resolver is a wiring mistake, and guessing that coverage is fine is the +// one thing it must not do. +func TestAssertWindowsNetworkPlanCoversOfflineGroupRejectsNilResolver(t *testing.T) { + plan := WindowsNetworkPlan{IdentitySIDs: []string{"S-1-5-32-9999"}} + if err := assertWindowsNetworkPlanCoversOfflineGroup(plan, nil, true); err == nil { + t.Fatal("assertWindowsNetworkPlanCoversOfflineGroup(nil resolver) = nil, want an error") + } +} diff --git a/internal/sandbox/windows_network_test.go b/internal/sandbox/windows_network_test.go index 4253916f8..50f66d960 100644 --- a/internal/sandbox/windows_network_test.go +++ b/internal/sandbox/windows_network_test.go @@ -118,3 +118,108 @@ 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") + } +} + +// 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_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) + } +} 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) + } + }) + } +} diff --git a/internal/sandbox/windows_online_offline_test.go b/internal/sandbox/windows_online_offline_test.go index 370abee73..f49d9258c 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,40 @@ 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) + } + // 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) + 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: diff --git a/internal/sandbox/windows_principal_ledger_windows_test.go b/internal/sandbox/windows_principal_ledger_windows_test.go index b71118cec..049e4ac5b 100644 --- a/internal/sandbox/windows_principal_ledger_windows_test.go +++ b/internal/sandbox/windows_principal_ledger_windows_test.go @@ -126,56 +126,84 @@ func TestPrincipalACLRecordCoversTheGrantBeforeItIsMade(t *testing.T) { // it is the fail-open: revocation would then cover only what the new plan // happens to name. Retiring the account instead makes every ACE that cannot be // found name a SID Windows never reuses. +// +// The record is per role because the principals are: seeding one role's record +// must retire the other and only the other. Retiring both would destroy an +// account there is nothing wrong with, and retiring neither is the fail-open. func TestSetupRetiresAPrincipalWithNoRecordOfItsGrants(t *testing.T) { for name, testCase := range map[string]struct { - seedRecord bool + seedRoles []windowsSandboxRole identityFound bool - wantRetired int + wantRetired []windowsSandboxRole }{ - "no record and a principal from an earlier setup": {identityFound: true, wantRetired: 1}, - "no record and nothing provisioned": {identityFound: false, wantRetired: 0}, - "a record to reconcile against": {seedRecord: true, identityFound: true, wantRetired: 0}, + "no record and principals from an earlier setup": { + identityFound: true, + wantRetired: []windowsSandboxRole{windowsSandboxRoleOffline, windowsSandboxRoleOnline}, + }, + "no record and nothing provisioned": {identityFound: false}, + "only the offline role has a record": { + seedRoles: []windowsSandboxRole{windowsSandboxRoleOffline}, + identityFound: true, + wantRetired: []windowsSandboxRole{windowsSandboxRoleOnline}, + }, + "both roles have a record to reconcile against": { + seedRoles: []windowsSandboxRole{windowsSandboxRoleOffline, windowsSandboxRoleOnline}, + identityFound: true, + }, } { t.Run(name, func(t *testing.T) { config := stubWindowsPrincipalSetup(t) - username := windowsSandboxUserName(windowsSandboxWorkspaceKey(config.WorkspaceRoots)) - if testCase.seedRecord { + key := windowsSandboxWorkspaceKey(config.WorkspaceRoots) + for _, role := range testCase.seedRoles { + username := windowsSandboxUserName(key, role) if err := writeWindowsPrincipalACLLedger(config.SandboxHome, username, []string{`C:\ws\recorded`}); err != nil { - t.Fatalf("seed the record: %v", err) + t.Fatalf("seed the %s record: %v", role, err) } } - lookupWindowsSandboxIdentityFn = func(string) (windowsSandboxIdentity, error) { + lookupWindowsSandboxIdentityFn = func(_ string, role windowsSandboxRole) (windowsSandboxIdentity, error) { if testCase.identityFound { - return windowsSandboxIdentity{Username: username, SID: guestsSID(t)}, nil + return windowsSandboxIdentity{Username: windowsSandboxUserName(key, role), SID: guestsSID(t)}, nil } return windowsSandboxIdentity{}, errWindowsSandboxIdentityUnavailable } - retired := 0 - removeWindowsSandboxPrincipalForSetupFn = func(WindowsSandboxCommandConfig) error { - retired++ + var retired []windowsSandboxRole + removeWindowsSandboxPrincipalForSetupFn = func(_ WindowsSandboxCommandConfig, role windowsSandboxRole) error { + retired = append(retired, role) return nil } if _, err := setupWindowsSandboxPrincipal(config); err != nil { t.Fatalf("setupWindowsSandboxPrincipal: %v", err) } - if retired != testCase.wantRetired { - t.Errorf("retired the principal %d times, want %d", retired, testCase.wantRetired) + if !sameRoles(retired, testCase.wantRetired) { + t.Errorf("retired %v, want %v", retired, testCase.wantRetired) } }) } } +func sameRoles(got []windowsSandboxRole, want []windowsSandboxRole) bool { + if len(got) != len(want) { + return false + } + for index := range got { + if got[index] != want[index] { + return false + } + } + return true +} + // A failure to retire has to fail the setup. Reporting success would leave the // operator believing the sandbox is provisioned while a principal whose grants // nobody can enumerate is still holding them. func TestSetupFailsWhenAnUnrecordedPrincipalCannotBeRetired(t *testing.T) { config := stubWindowsPrincipalSetup(t) - lookupWindowsSandboxIdentityFn = func(string) (windowsSandboxIdentity, error) { + lookupWindowsSandboxIdentityFn = func(string, windowsSandboxRole) (windowsSandboxIdentity, error) { return windowsSandboxIdentity{Username: "zerosbx", SID: guestsSID(t)}, nil } - removeWindowsSandboxPrincipalForSetupFn = func(WindowsSandboxCommandConfig) error { + removeWindowsSandboxPrincipalForSetupFn = func(WindowsSandboxCommandConfig, windowsSandboxRole) error { return errors.New("account is in use") } if _, err := setupWindowsSandboxPrincipal(config); err == nil { @@ -202,15 +230,26 @@ func TestTeardownRevokesRecordedPathsTheCurrentPolicyNoLongerNames(t *testing.T) }, }, } - username := windowsSandboxUserName(windowsSandboxWorkspaceKey(config.WorkspaceRoots)) + role := windowsSandboxRoleOffline + username := windowsSandboxUserName(windowsSandboxWorkspaceKey(config.WorkspaceRoots), role) if err := writeWindowsPrincipalACLLedger(home, username, []string{dropped}); err != nil { t.Fatalf("seed the record: %v", err) } - paths, err := windowsPrincipalRevocationPaths(config, "S-1-5-32-546") + paths, err := windowsPrincipalRevocationPaths(config, "S-1-5-32-546", role) if err != nil { t.Fatalf("windowsPrincipalRevocationPaths: %v", err) } + + // The other role's revocation must not pick this up: separate accounts, + // separate records, and retiring one must leave the other's ACEs alone. + other, err := windowsPrincipalRevocationPaths(config, "S-1-5-32-546", windowsSandboxRoleOnline) + if err != nil { + t.Fatalf("windowsPrincipalRevocationPaths(online): %v", err) + } + if containsPathFold(other, dropped) { + t.Errorf("the online role would revoke %q, which only the offline role's record names", dropped) + } if !containsPathFold(paths, dropped) { t.Errorf("teardown would revoke %v, missing the recorded root %q the policy no longer names", paths, dropped) } @@ -246,8 +285,8 @@ func stubWindowsPrincipalSetup(t *testing.T) WindowsSandboxCommandConfig { sandboxUserCacheDir = prevCache }) - provisionWindowsSandboxIdentityFn = func(key string) (windowsSandboxIdentity, string, bool, error) { - return windowsSandboxIdentity{Username: windowsSandboxUserName(key), SID: guestsSID(t)}, "pw", true, nil + provisionWindowsSandboxIdentityFn = func(key string, role windowsSandboxRole) (windowsSandboxIdentity, string, bool, error) { + return windowsSandboxIdentity{Username: windowsSandboxUserName(key, role), SID: guestsSID(t)}, "pw", true, nil } grantWindowsSandboxLogonRightsFn = func(*windows.SID) error { return nil } resetWindowsSandboxUserPasswordFn = func(string, string) error { return nil } diff --git a/internal/sandbox/windows_setup_windows.go b/internal/sandbox/windows_setup_windows.go index ac8dc1243..01eaa8dee 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,41 @@ 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 + } + // 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. + // Gated on the SAME opt-in that decides whether principals were provisioned + // at all (line 36). The offline group is created inside provisioning, so on + // an opt-out machine it legitimately does not exist, and asserting there + // would refuse a setup that has nothing to get wrong. + if err := assertWindowsNetworkPlanCoversOfflineGroup(networkPlan, resolveWindowsSandboxOfflineGroupSID, + windowsSandboxIdentityEnabled(config.commandConfig().Env)); 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) 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) } }