fix(sandbox): refuse a squatted account name and clean up partial provisioning

Two problems in the provisioning path, both raised in review.

The account name is derived from a workspace hash rather than discovered, so it
can be occupied by a local account that has nothing to do with Zero, whether by
coincidence or because somebody put it there. Provisioning treated "NetUserAdd
says it exists" as "this is ours", reset the account's password, added it to the
managed group and adopted it. That is a stranger's account taken over during an
elevated setup, on the strength of a name matching a pattern we generate
ourselves.

Ownership is now proven from the comment provisioning stamps before anything is
touched, and a name held by an account Zero did not create fails with a typed
collision error instead of being adopted.

Second, a failure anywhere after the account existed left it behind. The
rollback the setup path installs is only built once provisioning has returned
successfully, so nothing could undo a failure between creating the account and
storing its secret; the account, and possibly its granted logon rights, simply
stayed. Provisioning now unwinds what the run actually did, in reverse, on every
failure path.

Scoped to what THIS run created, deliberately. An account that already existed
and belongs to Zero is a working principal from an earlier setup, so deleting it
because a later run failed would turn a partial failure into a total one. For
that case the repair is dropping the stored secret instead: this run reset the
password, so the secret no longer matches, and absent beats stale because the
command path treats a missing secret as "not provisioned" and falls back rather
than failing.

The ownership gate is asserted against real accounts every Windows install
carries, which needs no privilege because it only has to establish that they are
not ours. Classifying everything as managed makes it fail.

Author: Vasanthdev2004

internal/sandbox/windows_identity_runtime_windows.go

@@ -131,24 +131,63 @@ func windowsSandboxPrincipalToken(config WindowsSandboxCommandConfig) (windows.T
 // therefore cleaned up, rather than an account nothing holds the secret for.
 func provisionWindowsSandboxPrincipalForSetup(config WindowsSandboxCommandConfig) (windowsSandboxIdentity, error) {
 	key := windowsSandboxWorkspaceKey(config.WorkspaceRoots)
-	identity, password, err := provisionWindowsSandboxIdentity(key)
+	identity, password, created, err := provisionWindowsSandboxIdentity(key)
+
+	// Undo whatever this run actually did, in reverse, on any failure after the
+	// account exists. Without it a failure between creating the account and
+	// storing its secret left the account behind with no caller able to remove
+	// it: the rollback the setup path installs is only built once this function
+	// has returned successfully.
+	//
+	// Scoped to what THIS run created on purpose. An account that already existed
+	// and belongs to Zero is a working principal from an earlier setup, and
+	// deleting it because a later run failed would turn a partial failure into a
+	// total one.
+	rightsGranted := false
+	secretWritten := false
+	secretPath := ""
+	undo := func() {
+		if secretWritten && secretPath != "" {
+			// Dropping the secret is also the repair for a pre-existing account
+			// whose password this run reset: the stored secret no longer matches,
+			// and absent beats stale, since the command path treats a missing
+			// secret as "not provisioned" and falls back rather than failing.
+			_ = removeWindowsSandboxSecret(secretPath)
+		}
+		if identity.SID != nil && rightsGranted {
+			_ = revokeWindowsSandboxLogonRights(identity.SID)
+		}
+		if created {
+			_ = removeWindowsSandboxIdentity(identity.Username)
+		}
+	}
+
 	if err != nil {
+		// provisionWindowsSandboxIdentity can fail after creating the account, so
+		// this path needs the same cleanup even though nothing below ran.
+		undo()
 		return windowsSandboxIdentity{}, err
 	}
 	if err := grantWindowsSandboxLogonRights(identity.SID); err != nil {
+		undo()
 		return windowsSandboxIdentity{}, err
 	}
-	secretPath, err := windowsSandboxSecretPath(config.SandboxHome, identity.Username)
+	rightsGranted = true
+	secretPath, err = windowsSandboxSecretPath(config.SandboxHome, identity.Username)
 	if err != nil {
+		undo()
 		return windowsSandboxIdentity{}, err
 	}
 	// The secret is rewritten every run so it stays in step with the account.
 	// provisionWindowsSandboxIdentity guarantees the password it returns is the
 	// account's real one, resetting it explicitly when the account already
-	// existed, so this write is always storing something that can log on.
+	// existed and belongs to Zero, so this write is always storing something that
+	// can log on.
 	if err := writeWindowsSandboxSecret(secretPath, password); err != nil {
+		undo()
 		return windowsSandboxIdentity{}, err
 	}
+	secretWritten = true
 	return identity, nil
 }
 

internal/sandbox/windows_identity_windows.go

@@ -77,6 +77,8 @@ var (
 	procNetLocalGroupAddMembers = netapi32.NewProc("NetLocalGroupAddMembers")
 	procNetUserDel              = netapi32.NewProc("NetUserDel")
 	procNetUserSetInfo          = netapi32.NewProc("NetUserSetInfo")
+	procNetUserGetInfo          = netapi32.NewProc("NetUserGetInfo")
+	procNetApiBufferFree        = netapi32.NewProc("NetApiBufferFree")
 )
 
 // userInfo1 mirrors USER_INFO_1. Field order and widths must match the Win32
@@ -317,6 +319,49 @@ func addWindowsSandboxUserToGroup(username string) error {
 	return netAPIStatus("NetLocalGroupAddMembers", status, errorMemberInAlias)
 }
 
+// errWindowsSandboxNameCollision reports that the derived account name is taken
+// by a local account Zero did not create. Setup refuses rather than adopting it.
+var errWindowsSandboxNameCollision = errors.New("a local account with Zero's derived sandbox name already exists and was not created by Zero")
+
+// windowsSandboxUserIsManaged reports whether a local account is one Zero
+// created, by reading back the comment provisioning stamps on it.
+//
+// This is the gate on adopting an existing account. The name is derived, not
+// discovered, so an account can be sitting on it for reasons that have nothing
+// to do with Zero, and taking it over means resetting a stranger's password.
+//
+// A missing account is not managed rather than an error, so callers can use this
+// as a plain question without special-casing absence.
+func windowsSandboxUserIsManaged(username string) (bool, error) {
+	name, err := windows.UTF16PtrFromString(username)
+	if err != nil {
+		return false, err
+	}
+	var buffer *byte
+	status, _, _ := procNetUserGetInfo.Call(
+		0, // local machine
+		uintptr(unsafe.Pointer(name)),
+		1, // level: USER_INFO_1
+		uintptr(unsafe.Pointer(&buffer)),
+	)
+	runtime.KeepAlive(name)
+	if status == nerrUserNotFound {
+		return false, nil
+	}
+	if err := netAPIStatus("NetUserGetInfo", status); err != nil {
+		return false, err
+	}
+	if buffer == nil {
+		return false, nil
+	}
+	defer procNetApiBufferFree.Call(uintptr(unsafe.Pointer(buffer)))
+	info := (*userInfo1)(unsafe.Pointer(buffer))
+	if info.Comment == nil {
+		return false, nil
+	}
+	return windows.UTF16PtrToString(info.Comment) == windowsSandboxUserComment, nil
+}
+
 // resolveWindowsSandboxSID looks up the SID for a provisioned principal. The SID
 // is the durable handle: account names can collide with a pre-existing local
 // user, so every ACE and firewall rule is keyed to the SID rather than the name.
@@ -341,36 +386,49 @@ func resolveWindowsSandboxSID(username string) (*windows.SID, error) {
 // 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, error) {
+func provisionWindowsSandboxIdentity(workspaceKey string) (windowsSandboxIdentity, string, bool, error) {
 	if err := ensureWindowsSandboxGroup(); err != nil {
-		return windowsSandboxIdentity{}, "", err
+		return windowsSandboxIdentity{}, "", false, err
 	}
 	username := windowsSandboxUserName(workspaceKey)
 	password, err := newWindowsSandboxPassword()
 	if err != nil {
-		return windowsSandboxIdentity{}, "", err
+		return windowsSandboxIdentity{}, "", false, err
 	}
 	existed, err := ensureWindowsSandboxUser(username, password)
 	if err != nil {
-		return windowsSandboxIdentity{}, "", err
+		return windowsSandboxIdentity{}, "", false, err
 	}
 	if existed {
-		// NetUserAdd left the account untouched, so the password above is not yet
+		// Prove the account is ours before touching it. The name is derived from a
+		// workspace hash rather than discovered, so it can be occupied by an
+		// account that has nothing to do with Zero, whether by coincidence or
+		// because somebody created it deliberately. Adopting one means resetting
+		// its password, which is not something to do on the strength of a name
+		// matching a pattern we generate ourselves.
+		managed, err := windowsSandboxUserIsManaged(username)
+		if err != nil {
+			return windowsSandboxIdentity{}, "", false, err
+		}
+		if !managed {
+			return windowsSandboxIdentity{}, "", false, fmt.Errorf("%w: %q", errWindowsSandboxNameCollision, username)
+		}
+		// Ours, and NetUserAdd left it untouched, so the password above is not yet
 		// its password. Set it, or the secret stored by the caller would never
 		// authenticate and every command would fail to log on with a principal
 		// that looks perfectly provisioned.
 		if err := resetWindowsSandboxUserPassword(username, password); err != nil {
-			return windowsSandboxIdentity{}, "", err
+			return windowsSandboxIdentity{}, "", false, err
 		}
 	}
 	if err := addWindowsSandboxUserToGroup(username); err != nil {
-		return windowsSandboxIdentity{}, "", err
+		return windowsSandboxIdentity{}, "", !existed, err
 	}
 	sid, err := resolveWindowsSandboxSID(username)
 	if err != nil {
-		return windowsSandboxIdentity{}, "", err
+		return windowsSandboxIdentity{}, "", !existed, err
 	}
-	return windowsSandboxIdentity{Username: username, SID: sid}, password, nil
+	return windowsSandboxIdentity{Username: username, SID: sid}, password, !existed, nil
 }
 
 // removeWindowsSandboxIdentity deletes a provisioned principal. Callers must

internal/sandbox/windows_identity_windows_test.go

@@ -4,6 +4,7 @@ package sandbox
 
 import (
 	"errors"
+	"fmt"
 	"os"
 	"strings"
 	"testing"
@@ -213,7 +214,7 @@ func TestProvisionWindowsSandboxIdentityRoundTrip(t *testing.T) {
 	// from being explained by residue from a previous one.
 	_ = removeWindowsSandboxIdentity(windowsSandboxUserName("ziptest01"))
 
-	identity, password, err := provisionWindowsSandboxIdentity("ziptest01")
+	identity, password, _, err := provisionWindowsSandboxIdentity("ziptest01")
 	if err != nil {
 		t.Fatalf("provision: %v", err)
 	}
@@ -238,7 +239,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")
 	if err != nil {
 		t.Fatalf("second provision: %v", err)
 	}
@@ -296,7 +297,7 @@ func TestGrantLogonRightsAndMintPrincipalToken(t *testing.T) {
 	// which the freshly generated one will not match, so start from a clean slate.
 	_ = removeWindowsSandboxIdentity(windowsSandboxUserName(key))
 
-	identity, password, err := provisionWindowsSandboxIdentity(key)
+	identity, password, _, err := provisionWindowsSandboxIdentity(key)
 	if err != nil {
 		t.Fatalf("provision: %v", err)
 	}
@@ -388,3 +389,50 @@ func TestLookupWindowsSandboxIdentityRejectsNonUserAccount(t *testing.T) {
 	}
 	t.Skip("no well-known non-user account resolved on this machine")
 }
+
+// The account name is derived from a workspace hash, not discovered, so it can
+// be occupied by a local account that has nothing to do with Zero. Adopting one
+// means resetting a stranger's password, so provisioning has to prove ownership
+// first and refuse otherwise.
+//
+// Driven against real accounts every Windows install carries, which are
+// definitively not ours. Unprivileged: it only has to establish that they are
+// not classified as managed, so nothing is ever created or modified.
+func TestWindowsSandboxUserIsManagedRefusesForeignAccounts(t *testing.T) {
+	checked := 0
+	for _, name := range []string{"Administrator", "Guest", "DefaultAccount"} {
+		managed, err := windowsSandboxUserIsManaged(name)
+		if err != nil {
+			// Localized or disabled installs may not carry every one of these.
+			continue
+		}
+		checked++
+		if managed {
+			t.Fatalf("%q classified as a Zero sandbox principal; provisioning would reset its password", name)
+		}
+	}
+	if checked == 0 {
+		t.Skip("no well-known local account could be queried on this machine")
+	}
+	// An absent account must answer false rather than error, since provisioning
+	// asks this question about names that usually do not exist yet.
+	managed, err := windowsSandboxUserIsManaged("zero-sbx-nosuchacct")
+	if err != nil {
+		t.Fatalf("querying a missing account: %v", err)
+	}
+	if managed {
+		t.Fatal("a missing account was classified as managed")
+	}
+}
+
+// The refusal has to be a typed, recognisable collision rather than a generic
+// failure, so setup can say what is wrong instead of reporting a Win32 status.
+func TestWindowsSandboxNameCollisionIsTyped(t *testing.T) {
+	wrapped := fmt.Errorf("%w: %q", errWindowsSandboxNameCollision, "zero-sbx-dexample")
+	if !errors.Is(wrapped, errWindowsSandboxNameCollision) {
+		t.Fatal("collision error does not match its sentinel")
+	}
+	if !strings.Contains(wrapped.Error(), "not created by Zero") {
+		t.Fatalf("collision message = %q, want it to say the account is not ours", wrapped.Error())
+	}
+}