fix(sandbox): prove ownership before deleting a sandbox account

removeWindowsSandboxIdentity is called with a DERIVED name, so it could be
pointed at a name that happens to belong to somebody else's local account.
Deleting a user is not a recoverable mistake, and the only thing standing
between the two cases was the name matching a pattern we generate ourselves.
Raised by CodeRabbit against the test fixtures, but the production teardown path
had the same hazard, so the guard belongs there rather than in the tests.

Ownership is now read back from the comment provisioning stamps on the account,
and an account that is not ours is left alone rather than deleted. The gated
tests get the protection for free, since their pre-clean goes through the same
helper.

The test asserts against real accounts every Windows install carries, which are
definitively not ours, and needs no privilege because it only has to establish
that they are not classified as managed. Classifying everything as managed makes
it fail.

Also appends the trimmed offline-group SID rather than the raw one. Worth noting
the reported consequence does not hold: newWindowsWFPUserCondition canonicalises
before converting, so a padded value would have been trimmed before reaching
StringToSid. The resolver returns SID.String(), which never carries whitespace,
so this is defensive tidying rather than a fix.

Author: Vasanthdev2004

internal/sandbox/windows_identity_windows.go

@@ -117,6 +117,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
@@ -497,14 +499,61 @@ func provisionWindowsSandboxIdentity(workspaceKey string, role windowsSandboxRol
 // A missing account is success, so teardown converges the same way provisioning
 // does. Requires an elevated caller.
 func removeWindowsSandboxIdentity(username string) error {
+	// Account names here are derived, not discovered, so this could be pointed at
+	// a name that happens to belong to somebody else's local account. Deleting a
+	// user is not a recoverable mistake, so ownership is proven before deleting
+	// rather than inferred from the name matching a pattern we generate.
+	managed, err := windowsSandboxUserIsManaged(username)
+	if err != nil {
+		return err
+	}
+	if !managed {
+		return nil
+	}
 	name, err := windows.UTF16PtrFromString(username)
 	if err != nil {
 		return err
 	}
 	status, _, _ := procNetUserDel.Call(0, uintptr(unsafe.Pointer(name)))
+	runtime.KeepAlive(name)
 	return netAPIStatus("NetUserDel", status, nerrUserNotFound)
 }
 
+// windowsSandboxUserIsManaged reports whether a local account is one Zero
+// created, by reading back the comment provisioning stamps on it.
+//
+// A missing account is not managed rather than an error, since the callers are
+// all teardown paths converging on "this account should not exist".
+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
+}
+
 // 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.

internal/sandbox/windows_identity_windows_test.go

@@ -425,3 +425,38 @@ func TestWindowsSandboxUserNameSeparatesRoles(t *testing.T) {
 		t.Fatalf("two workspaces produced the same offline account %q", other)
 	}
 }
+
+// removeWindowsSandboxIdentity is called with a DERIVED name, so it can be
+// pointed at a name that happens to belong to somebody else's local account.
+// Deleting a user is not recoverable, so ownership has to be proven rather than
+// inferred from the name matching a pattern we generate ourselves.
+//
+// Asserted against real accounts that exist on every Windows install and are
+// definitively not ours. Needs no privilege, since it only has to establish that
+// they are not classified as managed; NetUserDel is never reached for them.
+func TestWindowsSandboxUserIsManagedRejectsForeignAccounts(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; removal would delete a real account", name)
+		}
+	}
+	if checked == 0 {
+		t.Skip("none of the well-known local accounts could be queried on this machine")
+	}
+	// A name nothing owns must also come back false rather than erroring, since
+	// teardown calls this on accounts that may never have existed.
+	managed, err := windowsSandboxUserIsManaged("zero-sbx-dnosuchacct")
+	if err != nil {
+		t.Fatalf("querying a missing account: %v", err)
+	}
+	if managed {
+		t.Fatal("a missing account was classified as managed")
+	}
+}

internal/sandbox/windows_network.go

@@ -81,8 +81,8 @@ func BuildWindowsNetworkInfraPlan(config WindowsSandboxCommandConfig) (WindowsNe
 		if err != nil {
 			return WindowsNetworkPlan{}, err
 		}
-		if strings.TrimSpace(groupSID) != "" {
-			identitySIDs = append(identitySIDs, groupSID)
+		if trimmed := strings.TrimSpace(groupSID); trimmed != "" {
+			identitySIDs = append(identitySIDs, trimmed)
 		}
 	}
 	return WindowsNetworkPlan{