From 2133c7bf3e003b237ed7e5b7b89038aca60f36ad Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Sun, 26 Jul 2026 23:07:09 +0530 Subject: [PATCH 01/18] feat(sandbox): add Windows sandbox principals Groundwork for closing the Windows half of #662 and #675, where credentialDenyReadPaths is a no-op today. Every Windows backend currently derives its token from the calling user via CreateRestrictedToken, so the sandbox can constrain writes but not reads: a deny ACE that would stop the sandboxed child reading a credential store names the same account Zero runs as, and would lock Zero out too. That is why deny-read is skipped on Windows rather than merely unimplemented. This adds a separate local account per workspace, held in one managed group, so the sandbox has an identity of its own: - provisioning: managed group, stable per-workspace account name inside the 20-character limit, crypto/rand password meeting complexity policy, SID resolution, idempotent so setup re-runs converge - logon rights: grants only SeBatchLogonRight and explicitly denies interactive, network, remote-interactive and service logon, then mints a token with LogonUser pinned to the local machine - ACLs keyed to the principal: denies emitted before allows so carve-outs survive, workspace granted read+write, read roots granted read, protected metadata denied write and materialized - removal: revocation by trustee, so retiring a principal drops every ACE naming it without needing a record of what was granted The inversion is the point. A separate account has no access to the caller's profile at all, so credential stores are unreachable by construction rather than by enumerating deny rules, and the same SID is what a write grant or a firewall rule can be keyed to. Nothing is wired into command execution yet: these paths are additive and no existing behavior changes. See the pull request for the open question about where the principal's password lives. --- internal/sandbox/windows_acl_apply_windows.go | 11 + internal/sandbox/windows_identity_acl.go | 144 ++++++++ internal/sandbox/windows_identity_acl_test.go | 182 +++++++++ .../sandbox/windows_identity_logon_windows.go | 203 +++++++++++ internal/sandbox/windows_identity_windows.go | 345 ++++++++++++++++++ .../sandbox/windows_identity_windows_test.go | 247 +++++++++++++ 6 files changed, 1132 insertions(+) create mode 100644 internal/sandbox/windows_identity_acl.go create mode 100644 internal/sandbox/windows_identity_acl_test.go create mode 100644 internal/sandbox/windows_identity_logon_windows.go create mode 100644 internal/sandbox/windows_identity_windows.go create mode 100644 internal/sandbox/windows_identity_windows_test.go diff --git a/internal/sandbox/windows_acl_apply_windows.go b/internal/sandbox/windows_acl_apply_windows.go index c666aa9ef..a38789f4a 100644 --- a/internal/sandbox/windows_acl_apply_windows.go +++ b/internal/sandbox/windows_acl_apply_windows.go @@ -224,6 +224,17 @@ func windowsACLAccess(action WindowsACLAction) (windows.ACCESS_MODE, windows.ACC switch action { case WindowsACLAllowWrite: return windows.GRANT_ACCESS, windows.FILE_GENERIC_READ | windows.FILE_GENERIC_WRITE | windows.FILE_GENERIC_EXECUTE, nil + case WindowsACLAllowRead: + // Read and traverse without write. A sandbox principal is a separate + // account with no inherent access to the caller's tree, so a read-only + // root has to be granted rather than assumed. Deliberately omits + // FILE_GENERIC_WRITE, DELETE and WRITE_DAC. + return windows.GRANT_ACCESS, windows.FILE_GENERIC_READ | windows.FILE_GENERIC_EXECUTE, nil + case windowsACLRevoke: + // REVOKE_ACCESS drops every ACE naming the trustee regardless of the mask, + // so the mask is ignored here. Used to retire a principal without having + // to remember which access each path was granted. + return windows.REVOKE_ACCESS, 0, nil case WindowsACLDenyRead: return windows.DENY_ACCESS, windows.FILE_GENERIC_READ | windows.FILE_GENERIC_EXECUTE, nil case WindowsACLDenyWrite: diff --git a/internal/sandbox/windows_identity_acl.go b/internal/sandbox/windows_identity_acl.go new file mode 100644 index 000000000..c37393b71 --- /dev/null +++ b/internal/sandbox/windows_identity_acl.go @@ -0,0 +1,144 @@ +package sandbox + +// ACLs for a sandbox principal. +// +// The capability-SID model this sits beside starts from "the caller can already +// read everything" and narrows writes, because the sandboxed child runs as the +// caller. A principal inverts that: a separate local account has no access to +// the caller's profile at all, so the interesting direction is what to GRANT. +// +// That inversion is the point. Credential stores under the user's profile are +// unreachable because the principal is a different account, not because a deny +// rule enumerated them, which is what makes this able to close #662 and #675 on +// Windows where a deny-read ACE against the caller's own SID never could. Deny +// rules stay useful only for objects that are readable by everyone. +// +// Grants are explicit and narrow: the workspace and any extra write roots get +// read+write, declared read-only roots get read, and the protected metadata +// carve-outs the profile already defines stay denied so .git internals and +// .zero/.agents cannot be rewritten from inside the sandbox. + +import ( + "errors" + "fmt" + "path/filepath" +) + +// WindowsACLAllowRead grants read and execute without write. It exists for the +// principal model, where a read root must be granted rather than assumed. +const WindowsACLAllowRead WindowsACLAction = "allow-read" + +// windowsPrincipalACLInput is everything needed to describe a principal's +// access. It is deliberately a plain struct rather than the full command config +// so the plan can be built and tested without a live sandbox. +type windowsPrincipalACLInput struct { + // PrincipalSID is the string SID of the sandbox account every ACE names. + PrincipalSID string + // WriteRoots receive read+write+execute. The workspace lives here. + WriteRoots []WritableRoot + // ReadRoots receive read+execute only. + ReadRoots []string + // DenyRead covers objects a principal could otherwise reach because they are + // world-readable; per-user secrets need no entry. + DenyRead []string +} + +// buildWindowsPrincipalACLPlan turns a principal's access into ACL entries. +// +// Ordering matters at apply time: deny entries are emitted before allows so a +// carve-out inside a granted root survives, which mirrors how Windows evaluates +// an explicit DACL (deny ACEs first). +func buildWindowsPrincipalACLPlan(input windowsPrincipalACLInput) (WindowsACLPlan, error) { + if input.PrincipalSID == "" { + return WindowsACLPlan{}, errors.New("windows principal ACL plan requires a principal SID") + } + if len(input.WriteRoots) == 0 && len(input.ReadRoots) == 0 { + return WindowsACLPlan{}, errors.New("windows principal ACL plan requires at least one root") + } + + entries := make([]WindowsACLEntry, 0, len(input.WriteRoots)*2+len(input.ReadRoots)+len(input.DenyRead)) + + // Deny first. A deny ACE inside a write root (protected metadata, git + // internals) has to win over the grant that follows it. + for _, path := range normalizeProfilePaths(input.DenyRead) { + entries = append(entries, WindowsACLEntry{ + Action: WindowsACLDenyRead, + Path: path, + Capability: input.PrincipalSID, + }) + } + for _, root := range input.WriteRoots { + // Normalized the same way as read and deny paths: a write root may arrive + // with "~" or as a relative path, and an ACE has to name the same absolute, + // symlink-resolved object the deny entries do or the two disagree. + cleaned := normalizeProfilePath(root.Root) + if cleaned == "" { + return WindowsACLPlan{}, fmt.Errorf("windows principal ACL plan: unusable write root %q", root.Root) + } + for _, subpath := range normalizeProfilePaths(root.ReadOnlySubpaths) { + entries = append(entries, WindowsACLEntry{ + Action: WindowsACLDenyWrite, + Path: subpath, + Capability: input.PrincipalSID, + }) + } + for _, name := range root.ProtectedMetadataNames { + entries = append(entries, WindowsACLEntry{ + Action: WindowsACLDenyWrite, + Path: filepath.Join(cleaned, name), + Capability: input.PrincipalSID, + Materialize: true, + }) + } + } + + // Then the grants the principal cannot work without. + for _, root := range input.WriteRoots { + cleaned := normalizeProfilePath(root.Root) + if cleaned == "" { + continue + } + entries = append(entries, WindowsACLEntry{ + Action: WindowsACLAllowWrite, + Path: cleaned, + Capability: input.PrincipalSID, + }) + } + for _, path := range normalizeProfilePaths(input.ReadRoots) { + entries = append(entries, WindowsACLEntry{ + Action: WindowsACLAllowRead, + Path: path, + Capability: input.PrincipalSID, + }) + } + + return WindowsACLPlan{Entries: dedupeWindowsACLEntries(entries)}, nil +} + +// windowsPrincipalRevokePlan returns the entries whose ACEs should be removed +// when a principal is retired. Revocation is by TRUSTEE rather than by path: +// every ACE naming this principal is dropped, so cleanup does not depend on +// remembering which paths were granted, and a grant added by an older version +// is still removed. +// +// This is the removal path the capability-SID model never had, where a synthetic +// SID left ACEs behind on the user's tree with nothing to match them against. +func windowsPrincipalRevokePlan(principalSID string, paths []string) (WindowsACLPlan, error) { + if principalSID == "" { + return WindowsACLPlan{}, errors.New("windows principal revoke plan requires a principal SID") + } + cleaned := normalizeProfilePaths(paths) + entries := make([]WindowsACLEntry, 0, len(cleaned)) + for _, path := range cleaned { + entries = append(entries, WindowsACLEntry{ + Action: windowsACLRevoke, + Path: path, + Capability: principalSID, + }) + } + return WindowsACLPlan{Entries: entries}, nil +} + +// windowsACLRevoke removes every ACE naming the trustee on a path, whatever +// access it granted or denied. +const windowsACLRevoke WindowsACLAction = "revoke" diff --git a/internal/sandbox/windows_identity_acl_test.go b/internal/sandbox/windows_identity_acl_test.go new file mode 100644 index 000000000..b9026337a --- /dev/null +++ b/internal/sandbox/windows_identity_acl_test.go @@ -0,0 +1,182 @@ +package sandbox + +import ( + "path/filepath" + "strings" + "testing" +) + +const testPrincipalSID = "S-1-5-21-1111111111-2222222222-3333333333-1005" + +func testPrincipalInput() windowsPrincipalACLInput { + return windowsPrincipalACLInput{ + PrincipalSID: testPrincipalSID, + WriteRoots: []WritableRoot{{ + Root: filepath.FromSlash("/ws/project"), + ReadOnlySubpaths: []string{filepath.FromSlash("/ws/project/.git/config")}, + ProtectedMetadataNames: []string{".zero", ".agents"}, + }}, + ReadRoots: []string{filepath.FromSlash("/usr/lib")}, + DenyRead: []string{filepath.FromSlash("/shared/secrets")}, + } +} + +// Windows evaluates an explicit DACL deny-before-allow, so a carve-out inside a +// granted root only survives if its deny ACE is written first. If the grant on +// the workspace landed before the deny on .zero, the protected metadata would be +// writable from inside the sandbox. +func TestPrincipalACLPlanEmitsDeniesBeforeAllows(t *testing.T) { + plan, err := buildWindowsPrincipalACLPlan(testPrincipalInput()) + if err != nil { + t.Fatalf("build: %v", err) + } + lastDeny, firstAllow := -1, -1 + for index, entry := range plan.Entries { + switch entry.Action { + case WindowsACLDenyRead, WindowsACLDenyWrite: + lastDeny = index + case WindowsACLAllowWrite, WindowsACLAllowRead: + if firstAllow == -1 { + firstAllow = index + } + } + } + if firstAllow == -1 || lastDeny == -1 { + t.Fatalf("plan is missing a deny or an allow: %+v", plan.Entries) + } + if lastDeny > firstAllow { + t.Fatalf("deny at %d comes after allow at %d; carve-outs would be overridden", lastDeny, firstAllow) + } +} + +// Every ACE must name the sandbox principal. An entry with any other trustee +// would change access for a real user. +func TestPrincipalACLPlanNamesOnlyThePrincipal(t *testing.T) { + plan, err := buildWindowsPrincipalACLPlan(testPrincipalInput()) + if err != nil { + t.Fatalf("build: %v", err) + } + for _, entry := range plan.Entries { + if entry.Capability != testPrincipalSID { + t.Fatalf("entry %+v names %q, want the principal SID", entry, entry.Capability) + } + } +} + +// A principal is a separate account with no inherent access, so a write root +// must be granted read+write and a read root granted read. Without the grant the +// sandbox cannot open its own workspace. +func TestPrincipalACLPlanGrantsRoots(t *testing.T) { + plan, err := buildWindowsPrincipalACLPlan(testPrincipalInput()) + if err != nil { + t.Fatalf("build: %v", err) + } + var grantedWrite, grantedRead bool + for _, entry := range plan.Entries { + if entry.Action == WindowsACLAllowWrite && entry.Path == normalizeProfilePath(filepath.FromSlash("/ws/project")) { + grantedWrite = true + } + if entry.Action == WindowsACLAllowRead && entry.Path == normalizeProfilePath(filepath.FromSlash("/usr/lib")) { + grantedRead = true + } + } + if !grantedWrite { + t.Fatal("write root was not granted; the sandbox could not write its workspace") + } + if !grantedRead { + t.Fatal("read root was not granted; the sandbox could not read it") + } +} + +// Protected metadata is denied write and marked Materialize so the ACE is +// created even when the directory does not exist yet, closing the window where +// a sandboxed command creates .zero before the deny lands. +func TestPrincipalACLPlanProtectsMetadata(t *testing.T) { + plan, err := buildWindowsPrincipalACLPlan(testPrincipalInput()) + if err != nil { + t.Fatalf("build: %v", err) + } + found := map[string]WindowsACLEntry{} + for _, entry := range plan.Entries { + found[entry.Path] = entry + } + for _, name := range []string{".zero", ".agents"} { + path := filepath.Join(normalizeProfilePath(filepath.FromSlash("/ws/project")), name) + entry, ok := found[path] + if !ok { + t.Fatalf("no entry protecting %s", path) + } + if entry.Action != WindowsACLDenyWrite { + t.Fatalf("%s has action %q, want deny-write", path, entry.Action) + } + if !entry.Materialize { + t.Fatalf("%s must be materialized so the deny exists before the directory does", path) + } + } +} + +// A missing principal SID must be a hard error: an empty trustee would either +// fail at apply time or, worse, be interpreted as some other account. +func TestPrincipalACLPlanRequiresSID(t *testing.T) { + input := testPrincipalInput() + input.PrincipalSID = "" + if _, err := buildWindowsPrincipalACLPlan(input); err == nil { + t.Fatal("an empty principal SID must be rejected") + } +} + +// A plan with no roots at all is a caller mistake rather than a valid empty +// grant, since the resulting sandbox could not run anything. +func TestPrincipalACLPlanRequiresRoots(t *testing.T) { + input := windowsPrincipalACLInput{PrincipalSID: testPrincipalSID} + if _, err := buildWindowsPrincipalACLPlan(input); err == nil { + t.Fatal("a plan with no roots must be rejected") + } +} + +// Revocation is keyed to the trustee, so retiring a principal removes every ACE +// naming it without having to remember what was granted. This is the cleanup +// path the capability-SID model lacks. +func TestPrincipalRevokePlanTargetsTrustee(t *testing.T) { + paths := []string{filepath.FromSlash("/ws/project"), filepath.FromSlash("/usr/lib")} + plan, err := windowsPrincipalRevokePlan(testPrincipalSID, paths) + if err != nil { + t.Fatalf("revoke plan: %v", err) + } + if len(plan.Entries) != len(paths) { + t.Fatalf("got %d entries, want %d", len(plan.Entries), len(paths)) + } + for _, entry := range plan.Entries { + if entry.Action != windowsACLRevoke { + t.Fatalf("entry %+v is not a revoke", entry) + } + if entry.Capability != testPrincipalSID { + t.Fatalf("revoke names %q, want the principal", entry.Capability) + } + } +} + +func TestPrincipalRevokePlanRequiresSID(t *testing.T) { + if _, err := windowsPrincipalRevokePlan("", []string{"/ws"}); err == nil { + t.Fatal("revoking without a principal SID must be rejected") + } +} + +// The action strings end up in a serialized plan consumed by the elevated +// helper, so they must stay stable and distinct from the existing actions. +func TestPrincipalACLActionsAreDistinct(t *testing.T) { + actions := []WindowsACLAction{ + WindowsACLAllowWrite, WindowsACLAllowRead, + WindowsACLDenyRead, WindowsACLDenyWrite, windowsACLRevoke, + } + seen := map[WindowsACLAction]bool{} + for _, action := range actions { + if strings.TrimSpace(string(action)) == "" { + t.Fatal("an action string is empty") + } + if seen[action] { + t.Fatalf("duplicate action %q", action) + } + seen[action] = true + } +} diff --git a/internal/sandbox/windows_identity_logon_windows.go b/internal/sandbox/windows_identity_logon_windows.go new file mode 100644 index 000000000..6f9d689fb --- /dev/null +++ b/internal/sandbox/windows_identity_logon_windows.go @@ -0,0 +1,203 @@ +//go:build windows + +package sandbox + +// Minting a token for a sandbox principal. +// +// A provisioned account is inert until something can log on as it. Windows +// gates that behind account rights held in the local security policy, so setup +// grants the principal exactly one: the right to be logged on as a batch job, +// which is what a non-interactive service-style logon needs. It is deliberately +// NOT granted interactive, network or remote-interactive logon, and those three +// are explicitly DENIED, so the account cannot be used to sign in at the +// console, over SMB, or through RDP even if its password leaked. The password +// exists only so LogonUser can mint a token; nobody is meant to type it. +// +// Rights are granted at setup (elevated) because LsaAddAccountRights requires +// administrator privileges. The per-command path only calls LogonUser, which +// needs no special privilege once the batch right is in place. + +import ( + "fmt" + "unsafe" + + "golang.org/x/sys/windows" +) + +const ( + // Logon type/provider for a non-interactive token. Batch is the closest + // match for "run this command as a service-like principal": it produces a + // full token without a desktop or network-credential footprint. + logon32LogonBatch = 4 + logon32ProviderDefault = 0 + + // LSA policy access rights needed to add account rights. + policyCreateAccount = 0x00000010 + policyLookupNames = 0x00000800 + + // Account rights. The sandbox principal gets the batch right and is denied + // every interactive path. + seBatchLogonRight = "SeBatchLogonRight" + seDenyInteractiveLogonRight = "SeDenyInteractiveLogonRight" + seDenyNetworkLogonRight = "SeDenyNetworkLogonRight" + seDenyRemoteInteractiveRight = "SeDenyRemoteInteractiveLogonRight" + seDenyServiceLogonRightName = "SeDenyServiceLogonRight" + windowsIdentityLogonRightsNote = "granted by `zero sandbox setup`" +) + +var ( + procLogonUserW = windows.NewLazySystemDLL("advapi32.dll").NewProc("LogonUserW") + procLsaOpenPolicy = windows.NewLazySystemDLL("advapi32.dll").NewProc("LsaOpenPolicy") + procLsaClose = windows.NewLazySystemDLL("advapi32.dll").NewProc("LsaClose") + procLsaAddAccountRights = windows.NewLazySystemDLL("advapi32.dll").NewProc("LsaAddAccountRights") + procLsaNtStatusToWinErr = windows.NewLazySystemDLL("advapi32.dll").NewProc("LsaNtStatusToWinError") +) + +// lsaUnicodeString mirrors LSA_UNICODE_STRING. Length and MaximumLength are +// BYTE counts, not rune counts, which is the usual source of bugs here. +type lsaUnicodeString struct { + Length uint16 + MaximumLength uint16 + Buffer *uint16 +} + +// lsaObjectAttributes mirrors LSA_OBJECT_ATTRIBUTES. Every field except Length +// is unused for LsaOpenPolicy, but the struct must still be the right size. +type lsaObjectAttributes struct { + Length uint32 + RootDirectory windows.Handle + ObjectName *lsaUnicodeString + Attributes uint32 + SecurityDescriptor unsafe.Pointer + SecurityQualityOfService unsafe.Pointer +} + +// newLSAString builds an LSA_UNICODE_STRING over a UTF-16 buffer the caller +// keeps alive. The returned value borrows that buffer, so the buffer must +// outlive every use of the string. +func newLSAString(buffer []uint16) lsaUnicodeString { + if len(buffer) == 0 { + return lsaUnicodeString{} + } + // The buffer is NUL-terminated; the LSA length counts bytes WITHOUT the + // terminator, while MaximumLength counts bytes WITH it. + runes := len(buffer) - 1 + return lsaUnicodeString{ + Length: uint16(runes * 2), + MaximumLength: uint16(len(buffer) * 2), + Buffer: &buffer[0], + } +} + +// lsaStatusError converts an NTSTATUS from an Lsa* call into a Go error, going +// through LsaNtStatusToWinError so the message is the familiar Win32 one rather +// than a raw NTSTATUS. +func lsaStatusError(call string, status uintptr) error { + if status == 0 { + return nil + } + winErr, _, _ := procLsaNtStatusToWinErr.Call(status) + return fmt.Errorf("%s: %w", call, windows.Errno(winErr)) +} + +// grantWindowsSandboxLogonRights gives the principal the batch logon right and +// denies every interactive logon path. Idempotent: LsaAddAccountRights silently +// succeeds when the account already holds a right, so setup can re-run. +// +// Requires an elevated caller. +func grantWindowsSandboxLogonRights(sid *windows.SID) error { + if sid == nil { + return fmt.Errorf("grant sandbox logon rights: nil SID") + } + var attributes lsaObjectAttributes + attributes.Length = uint32(unsafe.Sizeof(attributes)) + var policy windows.Handle + status, _, _ := procLsaOpenPolicy.Call( + 0, // local system + uintptr(unsafe.Pointer(&attributes)), + uintptr(policyCreateAccount|policyLookupNames), + uintptr(unsafe.Pointer(&policy)), + ) + if err := lsaStatusError("LsaOpenPolicy", status); err != nil { + return err + } + defer procLsaClose.Call(uintptr(policy)) + + rights := []string{ + seBatchLogonRight, + seDenyInteractiveLogonRight, + seDenyNetworkLogonRight, + seDenyRemoteInteractiveRight, + seDenyServiceLogonRightName, + } + // Each right is added on its own call so one unsupported name on an odd SKU + // cannot silently drop the others. + for _, right := range rights { + buffer, err := windows.UTF16FromString(right) + if err != nil { + return err + } + entry := newLSAString(buffer) + status, _, _ := procLsaAddAccountRights.Call( + uintptr(policy), + uintptr(unsafe.Pointer(sid)), + uintptr(unsafe.Pointer(&entry)), + 1, + ) + if err := lsaStatusError("LsaAddAccountRights("+right+")", status); err != nil { + return err + } + // Keep the backing buffer alive until the call has returned. + runtimeKeepAliveUint16(buffer) + } + return nil +} + +// logonWindowsSandboxPrincipal mints a primary token for the sandbox account. +// The caller owns the returned token and must Close it. +// +// This needs no elevation: the batch logon right granted at setup is what makes +// it work, which is why the per-command path can run unelevated once setup has +// been done once. +func logonWindowsSandboxPrincipal(username string, password string) (windows.Token, error) { + user, err := windows.UTF16PtrFromString(username) + if err != nil { + return 0, err + } + // "." is the local machine, so the lookup never leaves this host even if the + // machine is domain-joined and a same-named domain account exists. + domain, err := windows.UTF16PtrFromString(".") + if err != nil { + return 0, err + } + secret, err := windows.UTF16PtrFromString(password) + if err != nil { + return 0, err + } + var token windows.Token + result, _, callErr := procLogonUserW.Call( + uintptr(unsafe.Pointer(user)), + uintptr(unsafe.Pointer(domain)), + uintptr(unsafe.Pointer(secret)), + logon32LogonBatch, + logon32ProviderDefault, + uintptr(unsafe.Pointer(&token)), + ) + if result == 0 { + if callErr != nil && callErr != windows.ERROR_SUCCESS { + return 0, fmt.Errorf("LogonUser(%s): %w", username, callErr) + } + return 0, fmt.Errorf("LogonUser(%s) failed", username) + } + return token, nil +} + +// runtimeKeepAliveUint16 keeps a UTF-16 buffer reachable across a syscall that +// borrows it. Declared rather than inlined so the intent is explicit at each +// call site; the compiler must not free the slice while LSA holds the pointer. +func runtimeKeepAliveUint16(buffer []uint16) { + if len(buffer) == 0 { + return + } + _ = buffer[0] +} diff --git a/internal/sandbox/windows_identity_windows.go b/internal/sandbox/windows_identity_windows.go new file mode 100644 index 000000000..f8711d053 --- /dev/null +++ b/internal/sandbox/windows_identity_windows.go @@ -0,0 +1,345 @@ +//go:build windows + +package sandbox + +// Windows sandbox principals. +// +// Every other Windows backend here derives its token from the CALLING user via +// CreateRestrictedToken, which is why the sandbox can constrain writes but not +// reads: a deny ACE that would stop the sandboxed child reading a credential +// store names the same account Zero itself runs as, so it would lock Zero out +// too. Reads therefore stay on the caller's identity and +// credentialDenyReadPaths is a no-op on Windows (#662, #675). +// +// This file provisions a SEPARATE local account per workspace, held in one +// managed local group, so the sandbox has an identity of its own. A deny-read +// ACE naming that principal denies the sandboxed child and nothing else, and +// the same SID is what a firewall rule or a write grant can be keyed to. The +// accounts are created by the elevated `zero sandbox setup` path because +// NetUserAdd requires administrator rights; nothing here runs unelevated. +// +// Provisioning is idempotent: the "already exists" status from each API is a +// success, so setup can be re-run safely and a partially provisioned machine +// converges. + +import ( + "crypto/rand" + "encoding/base32" + "errors" + "fmt" + "strings" + "unsafe" + + "golang.org/x/sys/windows" +) + +const ( + // windowsSandboxGroupName holds every sandbox principal. Grouping them means + // an ACE can name the group once instead of enumerating accounts, and it + // gives setup a single place to find what it previously created. + windowsSandboxGroupName = "ZeroSandboxUsers" + windowsSandboxGroupComment = "Zero sandbox principals (managed by zero sandbox setup)" + + // windowsSandboxUserPrefix keeps the accounts recognisable in `net user` and + // lets cleanup identify what belongs to Zero. Windows caps a local account + // name at 20 characters, which windowsSandboxUserName respects. + windowsSandboxUserPrefix = "zero-sbx-" + windowsSandboxUserComment = "Zero sandbox principal (managed)" + windowsSandboxUserNameMax = 20 +) + +// 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 + nerrUserExists = 2224 + errorAliasExists = 1379 + errorMemberInAlias = 1378 + errorAccessDenied32 = 5 + nerrUserNotFound = 2221 +) + +// USER_INFO_1 privilege and flag values. +const ( + usrPrivUser = 1 + ufScript = 0x0001 + ufNormalAccount = 0x0200 + ufDontExpirePasswd = 0x10000 + windowsPasswordLength = 24 +) + +var ( + netapi32 = windows.NewLazySystemDLL("netapi32.dll") + procNetUserAdd = netapi32.NewProc("NetUserAdd") + procNetLocalGroupAdd = netapi32.NewProc("NetLocalGroupAdd") + procNetLocalGroupAddMembers = netapi32.NewProc("NetLocalGroupAddMembers") + procNetUserDel = netapi32.NewProc("NetUserDel") +) + +// userInfo1 mirrors USER_INFO_1. Field order and widths must match the Win32 +// struct exactly; it is passed to NetUserAdd as a raw buffer. +type userInfo1 struct { + Name *uint16 + Password *uint16 + PasswordAge uint32 + Priv uint32 + HomeDir *uint16 + Comment *uint16 + Flags uint32 + ScriptPath *uint16 +} + +// localGroupInfo1 mirrors LOCALGROUP_INFO_1. +type localGroupInfo1 struct { + Name *uint16 + Comment *uint16 +} + +// localGroupMembersInfo3 mirrors LOCALGROUP_MEMBERS_INFO_3, which identifies a +// member by name rather than SID. +type localGroupMembersInfo3 struct { + DomainAndName *uint16 +} + +// windowsSandboxIdentity is a provisioned sandbox principal: the account name +// and the SID that ACEs, tokens and firewall rules are keyed to. +type windowsSandboxIdentity struct { + Username string + SID *windows.SID +} + +// String renders the identity for logs without exposing the password, which is +// never stored on this struct. +func (identity windowsSandboxIdentity) String() string { + if identity.SID == nil { + return identity.Username + } + return identity.Username + " (" + identity.SID.String() + ")" +} + +// windowsSandboxUserName derives a stable account name for a workspace key. The +// key is hashed by the caller (see sandboxRuntimeKey) 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 { + cleaned := strings.Map(func(r rune) rune { + switch { + case r >= 'a' && r <= 'z', r >= '0' && r <= '9': + return r + case r >= 'A' && r <= 'Z': + return r + ('a' - 'A') + default: + return -1 + } + }, workspaceKey) + if cleaned == "" { + cleaned = "default" + } + name := windowsSandboxUserPrefix + cleaned + if len(name) > windowsSandboxUserNameMax { + name = name[:windowsSandboxUserNameMax] + } + return name +} + +// newWindowsSandboxPassword returns a random password for a sandbox principal. +// The account is never signed into interactively: the password exists only so +// LogonUser can mint a token for it, so it is generated per provisioning run, +// handed straight to the caller, and never persisted by this file. Base32 of +// crypto/rand bytes keeps it alphanumeric, which satisfies complexity policies +// that reject unusual punctuation, and a fixed suffix guarantees the mixed-case +// and digit classes even if the random draw happens to omit one. +func newWindowsSandboxPassword() (string, error) { + raw := make([]byte, windowsPasswordLength) + if _, err := rand.Read(raw); err != nil { + return "", fmt.Errorf("generate sandbox password: %w", err) + } + encoded := base32.StdEncoding.WithPadding(base32.NoPadding).EncodeToString(raw) + if len(encoded) > windowsPasswordLength { + encoded = encoded[:windowsPasswordLength] + } + return "Zs1!" + encoded, nil +} + +// netAPIStatus converts a netapi32 return value into an error, treating the +// supplied status codes as success so callers can spell out which "already +// exists" results are expected. +func netAPIStatus(call string, status uintptr, okStatuses ...uintptr) error { + if status == nerrSuccess { + return nil + } + for _, ok := range okStatuses { + if status == ok { + return nil + } + } + if status == errorAccessDenied32 { + return fmt.Errorf("%s: access denied (run `zero sandbox setup` from an elevated terminal)", call) + } + return fmt.Errorf("%s: status %d", call, status) +} + +// ensureWindowsSandboxGroup creates the managed local group, or leaves it alone +// when it already exists. +func ensureWindowsSandboxGroup() error { + name, err := windows.UTF16PtrFromString(windowsSandboxGroupName) + if err != nil { + return err + } + comment, err := windows.UTF16PtrFromString(windowsSandboxGroupComment) + if err != nil { + return err + } + info := localGroupInfo1{Name: name, Comment: comment} + status, _, _ := procNetLocalGroupAdd.Call( + 0, // local machine + 1, // level: LOCALGROUP_INFO_1 + uintptr(unsafe.Pointer(&info)), + 0, + ) + // Keep info alive across the call: the struct holds pointers into Go memory + // that the syscall dereferences. + defer func() { _ = info }() + return netAPIStatus("NetLocalGroupAdd", status, nerrGroupExists, errorAliasExists) +} + +// ensureWindowsSandboxUser creates a sandbox account with the supplied password, +// or leaves an existing account alone. The account is a plain local user with no +// home directory or logon script, flagged so its password never expires (nobody +// is there to rotate it) and so it is a normal, enabled account LogonUser can +// authenticate. +func ensureWindowsSandboxUser(username string, password string) error { + name, err := windows.UTF16PtrFromString(username) + if err != nil { + return err + } + secret, err := windows.UTF16PtrFromString(password) + if err != nil { + return err + } + comment, err := windows.UTF16PtrFromString(windowsSandboxUserComment) + if err != nil { + return err + } + info := userInfo1{ + Name: name, + Password: secret, + Priv: usrPrivUser, + Comment: comment, + Flags: ufScript | ufNormalAccount | ufDontExpirePasswd, + } + status, _, _ := procNetUserAdd.Call( + 0, // local machine + 1, // level: USER_INFO_1 + uintptr(unsafe.Pointer(&info)), + 0, + ) + defer func() { _ = info }() + return netAPIStatus("NetUserAdd", status, nerrUserExists) +} + +// 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) + if err != nil { + return err + } + member, err := windows.UTF16PtrFromString(username) + if err != nil { + return err + } + entry := localGroupMembersInfo3{DomainAndName: member} + status, _, _ := procNetLocalGroupAddMembers.Call( + 0, // local machine + uintptr(unsafe.Pointer(group)), + 3, // level: LOCALGROUP_MEMBERS_INFO_3 + uintptr(unsafe.Pointer(&entry)), + 1, // one member + ) + defer func() { _ = entry }() + return netAPIStatus("NetLocalGroupAddMembers", status, errorMemberInAlias) +} + +// 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. +func resolveWindowsSandboxSID(username string) (*windows.SID, error) { + sid, _, accountType, err := windows.LookupSID("", username) + if err != nil { + return nil, fmt.Errorf("look up sandbox principal %q: %w", username, err) + } + if accountType != windows.SidTypeUser { + return nil, fmt.Errorf("sandbox principal %q resolves to a non-user account (type %d)", username, accountType) + } + return sid, nil +} + +// provisionWindowsSandboxIdentity ensures the managed group and one sandbox +// principal for workspaceKey exist, and returns the identity plus the password +// the caller needs to mint a token with LogonUser. It is idempotent, so setup +// can run repeatedly. +// +// The password is returned rather than stored: on an account that already +// existed the returned value is the NEW password only if the caller resets it, +// so callers that need to log in must treat a pre-existing account as requiring +// a reset. That is handled a layer up, where the secret has somewhere safe to +// live; keeping it out of this file means no credential is written to disk here. +func provisionWindowsSandboxIdentity(workspaceKey string) (windowsSandboxIdentity, string, error) { + if err := ensureWindowsSandboxGroup(); err != nil { + return windowsSandboxIdentity{}, "", err + } + username := windowsSandboxUserName(workspaceKey) + password, err := newWindowsSandboxPassword() + if err != nil { + return windowsSandboxIdentity{}, "", err + } + if err := ensureWindowsSandboxUser(username, password); err != nil { + return windowsSandboxIdentity{}, "", err + } + if err := addWindowsSandboxUserToGroup(username); err != nil { + return windowsSandboxIdentity{}, "", err + } + sid, err := resolveWindowsSandboxSID(username) + if err != nil { + return windowsSandboxIdentity{}, "", err + } + return windowsSandboxIdentity{Username: username, SID: sid}, password, nil +} + +// removeWindowsSandboxIdentity deletes a provisioned principal. Callers must +// revoke the principal's ACEs FIRST (see windowsPrincipalRevokePlan): deleting +// the account leaves any surviving ACE naming an unresolvable SID, which is what +// shows up in Explorer as an orphaned entry and is exactly the residue this +// model is meant to avoid. +// +// A missing account is success, so teardown converges the same way provisioning +// does. Requires an elevated caller. +func removeWindowsSandboxIdentity(username string) error { + name, err := windows.UTF16PtrFromString(username) + if err != nil { + return err + } + status, _, _ := procNetUserDel.Call(0, uintptr(unsafe.Pointer(name))) + return netAPIStatus("NetUserDel", status, nerrUserNotFound) +} + +// 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. +var errWindowsSandboxIdentityUnavailable = errors.New("no Zero sandbox principal is provisioned; run `zero sandbox setup` from an elevated terminal") + +// lookupWindowsSandboxIdentity resolves an already-provisioned principal without +// 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) + sid, err := resolveWindowsSandboxSID(username) + if err != nil { + return windowsSandboxIdentity{}, errWindowsSandboxIdentityUnavailable + } + return windowsSandboxIdentity{Username: username, SID: sid}, nil +} diff --git a/internal/sandbox/windows_identity_windows_test.go b/internal/sandbox/windows_identity_windows_test.go new file mode 100644 index 000000000..e98ef85d9 --- /dev/null +++ b/internal/sandbox/windows_identity_windows_test.go @@ -0,0 +1,247 @@ +//go:build windows + +package sandbox + +import ( + "os" + "strings" + "testing" + "unsafe" + + "golang.org/x/sys/windows" +) + +// 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)) + if len(name) > windowsSandboxUserNameMax { + t.Fatalf("name %q is %d chars, want at most %d", name, len(name), windowsSandboxUserNameMax) + } + if !strings.HasPrefix(name, windowsSandboxUserPrefix) { + t.Fatalf("name %q lost the managed prefix", name) + } +} + +// 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") + if first != second { + t.Fatalf("name is not stable: %q vs %q", first, second) + } + if other := windowsSandboxUserName("def456"); other == first { + t.Fatalf("different workspaces produced the same principal %q", first) + } +} + +// 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`) + for _, r := range strings.TrimPrefix(name, windowsSandboxUserPrefix) { + isLower := r >= 'a' && r <= 'z' + isDigit := r >= '0' && r <= '9' + if !isLower && !isDigit { + t.Fatalf("name %q contains unsafe rune %q", name, r) + } + } + if name == windowsSandboxUserPrefix { + t.Fatal("sanitising removed every character, leaving a bare prefix") + } +} + +// An empty or fully-sanitised-away key must still yield a usable name rather +// than the bare prefix. +func TestWindowsSandboxUserNameHandlesEmptyKey(t *testing.T) { + for _, key := range []string{"", "///", " "} { + if got := windowsSandboxUserName(key); got == windowsSandboxUserPrefix { + t.Fatalf("key %q produced a bare prefix", key) + } + } +} + +// The password must be fresh per call and carry the character classes a default +// Windows complexity policy demands, or NetUserAdd fails with ERROR_PASSWORD_RESTRICTION. +func TestNewWindowsSandboxPasswordIsRandomAndComplex(t *testing.T) { + first, err := newWindowsSandboxPassword() + if err != nil { + t.Fatalf("generate: %v", err) + } + second, err := newWindowsSandboxPassword() + if err != nil { + t.Fatalf("generate: %v", err) + } + if first == second { + t.Fatal("two generated passwords are identical, so they are not random") + } + if len(first) < 12 { + t.Fatalf("password is only %d chars", len(first)) + } + var hasUpper, hasLower, hasDigit bool + for _, r := range first { + switch { + case r >= 'A' && r <= 'Z': + hasUpper = true + case r >= 'a' && r <= 'z': + hasLower = true + case r >= '0' && r <= '9': + hasDigit = true + } + } + if !hasUpper || !hasLower || !hasDigit { + t.Fatalf("password %q lacks a required character class", first) + } +} + +// "Already exists" is the normal result of re-running setup and must not surface +// as an error, while a genuine failure must. +func TestNetAPIStatusTreatsExistingAsSuccess(t *testing.T) { + if err := netAPIStatus("NetUserAdd", nerrSuccess); err != nil { + t.Fatalf("success status returned %v", err) + } + if err := netAPIStatus("NetUserAdd", nerrUserExists, nerrUserExists); err != nil { + t.Fatalf("existing user must be success, got %v", err) + } + if err := netAPIStatus("NetLocalGroupAdd", nerrGroupExists, nerrGroupExists, errorAliasExists); err != nil { + t.Fatalf("existing group must be success, got %v", err) + } + if err := netAPIStatus("NetUserAdd", 2245); err == nil { + t.Fatal("an unexpected status must surface as an error") + } +} + +// Access-denied is the status an unelevated run gets, and it must say so rather +// than reporting a bare number the user cannot act on. +func TestNetAPIStatusExplainsAccessDenied(t *testing.T) { + err := netAPIStatus("NetUserAdd", errorAccessDenied32) + if err == nil { + t.Fatal("access denied must be an error") + } + if !strings.Contains(err.Error(), "elevated") { + t.Fatalf("error %q should point at elevation", err) + } +} + +// The Win32 structs are passed to netapi32 as raw buffers, so their layout must +// match what the API expects. A wrong size means silent memory corruption. +func TestWindowsIdentityStructLayouts(t *testing.T) { + ptr := unsafe.Sizeof(uintptr(0)) + if got, want := unsafe.Sizeof(localGroupMembersInfo3{}), ptr; got != want { + t.Fatalf("LOCALGROUP_MEMBERS_INFO_3 size = %d, want %d", got, want) + } + if got, want := unsafe.Sizeof(localGroupInfo1{}), 2*ptr; got != want { + t.Fatalf("LOCALGROUP_INFO_1 size = %d, want %d", got, want) + } + // USER_INFO_1 is four pointers plus three DWORDs, with the compiler padding + // each DWORD pair up to pointer alignment on amd64. + if got := unsafe.Sizeof(userInfo1{}); got < 4*ptr { + t.Fatalf("USER_INFO_1 size = %d, smaller than its four pointer fields", got) + } + if unsafe.Offsetof(userInfo1{}.Password) != ptr { + t.Fatal("USER_INFO_1.Password must directly follow Name") + } +} + +// LSA_UNICODE_STRING counts BYTES, not runes, and excludes the NUL terminator +// from Length while including it in MaximumLength. Getting either wrong makes +// LsaAddAccountRights read past the buffer or silently match no right, so pin it. +func TestNewLSAStringUsesByteLengths(t *testing.T) { + buffer, err := windows.UTF16FromString("SeBatchLogonRight") + if err != nil { + t.Fatalf("encode: %v", err) + } + entry := newLSAString(buffer) + const runes uint16 = uint16(len("SeBatchLogonRight")) + if entry.Length != runes*2 { + t.Fatalf("Length = %d, want %d (bytes, excluding NUL)", entry.Length, runes*2) + } + if entry.MaximumLength != (runes+1)*2 { + t.Fatalf("MaximumLength = %d, want %d (bytes, including NUL)", entry.MaximumLength, (runes+1)*2) + } + if entry.Buffer == nil { + t.Fatal("Buffer must point at the encoded string") + } +} + +// An empty buffer must not produce a struct pointing at nothing with a nonzero +// length, which would hand LSA a wild pointer. +func TestNewLSAStringHandlesEmptyBuffer(t *testing.T) { + entry := newLSAString(nil) + if entry.Buffer != nil || entry.Length != 0 || entry.MaximumLength != 0 { + t.Fatalf("empty buffer produced %+v, want a zero value", entry) + } +} + +// The LSA structs are passed to advapi32 as raw buffers, so their sizes must +// match the Win32 definitions. +func TestLSAStructLayouts(t *testing.T) { + ptr := unsafe.Sizeof(uintptr(0)) + // LSA_UNICODE_STRING: two uint16 then a pointer, padded to pointer alignment. + if got, want := unsafe.Sizeof(lsaUnicodeString{}), 2*ptr; got != want { + t.Fatalf("LSA_UNICODE_STRING size = %d, want %d", got, want) + } + if unsafe.Offsetof(lsaUnicodeString{}.Buffer) != ptr { + t.Fatal("LSA_UNICODE_STRING.Buffer must sit at the second pointer slot") + } + var attributes lsaObjectAttributes + if unsafe.Sizeof(attributes) < 6*ptr-ptr { + t.Fatalf("LSA_OBJECT_ATTRIBUTES size = %d, smaller than its fields", unsafe.Sizeof(attributes)) + } + if unsafe.Offsetof(attributes.ObjectName) == 0 { + t.Fatal("LSA_OBJECT_ATTRIBUTES.ObjectName must not alias Length") + } +} + +// Provisioning creates real local accounts, so it only runs when explicitly +// opted into on an elevated machine. Everything above covers the logic that can +// be exercised without touching the account database. +func TestProvisionWindowsSandboxIdentityRoundTrip(t *testing.T) { + if os.Getenv("ZERO_WINDOWS_IDENTITY_PROVISION_TEST") != "1" { + t.Skip("set ZERO_WINDOWS_IDENTITY_PROVISION_TEST=1 on an elevated machine to exercise real provisioning") + } + if !windowsProcessIsElevated() { + t.Skip("provisioning requires an elevated process") + } + identity, password, err := provisionWindowsSandboxIdentity("ziptest01") + if err != nil { + t.Fatalf("provision: %v", err) + } + if identity.SID == nil { + t.Fatal("provisioned identity has no SID") + } + if password == "" { + t.Fatal("provisioning returned an empty password") + } + // Re-running must converge on the same principal rather than failing or + // creating a second account. + again, _, err := provisionWindowsSandboxIdentity("ziptest01") + if err != nil { + t.Fatalf("second provision: %v", err) + } + if again.Username != identity.Username || !again.SID.Equals(identity.SID) { + t.Fatalf("provisioning is not idempotent: %s then %s", identity, again) + } + // Lookup must find what provisioning created. + found, err := lookupWindowsSandboxIdentity("ziptest01") + if err != nil { + t.Fatalf("lookup after provision: %v", err) + } + if !found.SID.Equals(identity.SID) { + t.Fatalf("lookup returned %s, want %s", found, identity) + } +} + +// A workspace with no provisioned principal must report the actionable +// "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") + if err == nil { + t.Skip("a principal for this key unexpectedly exists on this machine") + } + if err != errWindowsSandboxIdentityUnavailable { + t.Fatalf("error = %v, want errWindowsSandboxIdentityUnavailable", err) + } +} From 29df0878e701b97cf444605d6a1fc21aa67ba6cf Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Sun, 26 Jul 2026 23:13:04 +0530 Subject: [PATCH 02/18] feat(sandbox): use a sandbox principal for Windows command execution Wires the principal model into the runner and settles where the account's password lives. The secret is stored under the sandbox home with an explicit, inheritance-protected DACL naming only the invoking user and SYSTEM. The sandbox principal is deliberately absent from it: a principal that could read the file could mint its own token and the identity boundary would be decorative. The ACL is applied to an empty file before the password is written, so the bytes never exist under the config directory's inherited permissions, and PROTECTED drops any inherited ACE outright. At command time the runner asks for a principal token first and uses it in place of the restricted token, because a separate account has reads denied by the filesystem rather than left open the way a same-user restricted token must leave them. The lookup is fail-soft: opt-out, no provisioned account, or no stored secret all report "not available" and the existing restricted-token path runs unchanged. Only a provisioned-but-unusable identity surfaces an error, since that means setup ran and the sandbox is broken rather than absent. The backend stays behind ZERO_WINDOWS_SANDBOX_IDENTITY=1 while the privileged paths are unvalidated, so no existing install changes behaviour. --- .../sandbox/windows_command_runner_windows.go | 21 ++ .../windows_identity_runtime_windows.go | 143 +++++++++++++ .../windows_identity_secret_windows.go | 194 +++++++++++++++++ .../windows_identity_secret_windows_test.go | 196 ++++++++++++++++++ 4 files changed, 554 insertions(+) create mode 100644 internal/sandbox/windows_identity_runtime_windows.go create mode 100644 internal/sandbox/windows_identity_secret_windows.go create mode 100644 internal/sandbox/windows_identity_secret_windows_test.go diff --git a/internal/sandbox/windows_command_runner_windows.go b/internal/sandbox/windows_command_runner_windows.go index 0b9e8f64e..413e41899 100644 --- a/internal/sandbox/windows_command_runner_windows.go +++ b/internal/sandbox/windows_command_runner_windows.go @@ -75,6 +75,27 @@ func runWindowsSandboxCommand(config WindowsSandboxCommandConfig, stderr io.Writ // reads under that flag (#612). Profiles with DenyRead keep the fully // restricted token, trading spawn capability for read-deny enforcement. writeRestricted := len(config.PermissionProfile.FileSystem.DenyRead) == 0 + + // A provisioned sandbox principal replaces the restricted token entirely: it + // is a separate account, so reads outside its granted roots are denied by the + // filesystem rather than left open the way a same-user restricted token has + // to leave them (#662). Absent, unprovisioned or opted-out, ok is false and + // the restricted-token backend below runs exactly as before. + principalToken, ok, err := windowsSandboxPrincipalToken(config) + if err != nil { + fmt.Fprintln(stderr, WindowsSandboxCommandRunnerName+": "+err.Error()) + return 1 + } + if ok { + defer principalToken.Close() + exitCode, err := runWindowsCommandAsUser(principalToken, config) + if err != nil { + fmt.Fprintln(stderr, WindowsSandboxCommandRunnerName+": "+err.Error()) + return 1 + } + return exitCode + } + token, err := createWindowsRestrictedTokenForCapabilitySIDs(tokenSIDs, writeRestricted) if err != nil { fmt.Fprintln(stderr, WindowsSandboxCommandRunnerName+": "+err.Error()) diff --git a/internal/sandbox/windows_identity_runtime_windows.go b/internal/sandbox/windows_identity_runtime_windows.go new file mode 100644 index 000000000..f7a4cef34 --- /dev/null +++ b/internal/sandbox/windows_identity_runtime_windows.go @@ -0,0 +1,143 @@ +//go:build windows + +package sandbox + +// Using a sandbox principal at command time. +// +// This is the seam between the identity model and the existing runner. It is +// deliberately fail-soft: when no principal is provisioned, when the secret is +// missing, or when the opt-in is off, it reports "not available" and the caller +// keeps using today's restricted-token backend. Only an outright failure to log +// on with a principal that IS provisioned surfaces as an error, because that +// means setup ran but the identity is broken, and silently downgrading the +// sandbox in that case would be the wrong kind of quiet. + +import ( + "crypto/sha256" + "encoding/hex" + "errors" + "os" + "strings" + + "golang.org/x/sys/windows" +) + +// windowsSandboxIdentityEnv opts a machine into the principal backend while it +// is still experimental. Provisioning is inert without it, so an existing +// install keeps the restricted-token behaviour until someone turns this on. +const windowsSandboxIdentityEnv = "ZERO_WINDOWS_SANDBOX_IDENTITY" + +// windowsSandboxIdentityEnabled reports whether the principal backend is opted +// into. Kept as a function so the check reads the environment at call time, +// which is what lets a test or an elevated setup run flip it. +func windowsSandboxIdentityEnabled(env map[string]string) bool { + if value, ok := env[windowsSandboxIdentityEnv]; ok { + return strings.TrimSpace(value) == "1" + } + return strings.TrimSpace(os.Getenv(windowsSandboxIdentityEnv)) == "1" +} + +// windowsSandboxWorkspaceKey derives the per-workspace key a principal is named +// after. It hashes the workspace root the same way the sandbox runtime keys its +// own state, so the account name leaks no path and one workspace always maps to +// one principal. +func windowsSandboxWorkspaceKey(workspaceRoots []string) string { + root := "" + for _, candidate := range workspaceRoots { + if trimmed := strings.TrimSpace(candidate); trimmed != "" { + root = normalizeProfilePath(trimmed) + break + } + } + if root == "" { + root = "default" + } + digest := sha256.Sum256([]byte(strings.ToLower(root))) + return hex.EncodeToString(digest[:]) +} + +// windowsSandboxPrincipalToken returns a token for this workspace's sandbox +// principal. +// +// ok is false, with a nil error, whenever the principal backend simply is not in +// play: the opt-in is off, setup has not provisioned an account, or no secret is +// stored. The caller falls back to the restricted token in those cases. An error +// means the identity exists but could not be used, which is worth surfacing +// rather than downgrading around. +func windowsSandboxPrincipalToken(config WindowsSandboxCommandConfig) (windows.Token, bool, error) { + if !windowsSandboxIdentityEnabled(config.Env) { + return 0, false, nil + } + key := windowsSandboxWorkspaceKey(config.WorkspaceRoots) + identity, err := lookupWindowsSandboxIdentity(key) + if err != nil { + // Not provisioned: fall back quietly, this is the default state. + return 0, false, nil + } + secretPath, err := windowsSandboxSecretPath(config.SandboxHome, identity.Username) + if err != nil { + return 0, false, err + } + password, err := readWindowsSandboxSecret(secretPath) + if err != nil { + if errors.Is(err, errWindowsSandboxIdentityUnavailable) { + // The account exists but its password does not. Setup was interrupted + // or the secret was removed; fall back rather than fail the command. + return 0, false, nil + } + return 0, false, err + } + token, err := logonWindowsSandboxPrincipal(identity.Username, password) + if err != nil { + // Provisioned but unusable. Surface it: a wrong password or a revoked + // batch-logon right is a broken sandbox, not an absent one. + return 0, false, err + } + return token, true, nil +} + +// 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`. +// +// 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, error) { + key := windowsSandboxWorkspaceKey(config.WorkspaceRoots) + identity, password, err := provisionWindowsSandboxIdentity(key) + if err != nil { + return windowsSandboxIdentity{}, err + } + if err := grantWindowsSandboxLogonRights(identity.SID); err != nil { + return windowsSandboxIdentity{}, err + } + secretPath, err := windowsSandboxSecretPath(config.SandboxHome, identity.Username) + if err != nil { + return windowsSandboxIdentity{}, err + } + // A pre-existing account keeps its old password, which this new one does not + // match, so the secret is rewritten every run to stay in step with whatever + // NetUserAdd left in place. On a fresh account the two agree by construction; + // on an existing one the caller resets it via ensureWindowsSandboxUser. + if err := writeWindowsSandboxSecret(secretPath, password); err != nil { + return windowsSandboxIdentity{}, err + } + return identity, nil +} + +// removeWindowsSandboxPrincipalForSetup retires a workspace's principal: secret +// first, then the account. ACE revocation is the caller's job and must happen +// before this, or ACEs naming a deleted SID are left behind. +func removeWindowsSandboxPrincipalForSetup(config WindowsSandboxCommandConfig) error { + key := windowsSandboxWorkspaceKey(config.WorkspaceRoots) + username := windowsSandboxUserName(key) + secretPath, err := windowsSandboxSecretPath(config.SandboxHome, username) + if err != nil { + return err + } + if err := removeWindowsSandboxSecret(secretPath); err != nil { + return err + } + return removeWindowsSandboxIdentity(username) +} diff --git a/internal/sandbox/windows_identity_secret_windows.go b/internal/sandbox/windows_identity_secret_windows.go new file mode 100644 index 000000000..37781032b --- /dev/null +++ b/internal/sandbox/windows_identity_secret_windows.go @@ -0,0 +1,194 @@ +//go:build windows + +package sandbox + +// Storing a sandbox principal's password. +// +// The elevated setup path provisions the account, but the per-command path runs +// UNELEVATED and needs the password to call LogonUser. So the secret has to +// cross that boundary on disk, and the only thing standing between it and the +// sandboxed child is the file's ACL. +// +// The file is locked to the invoking user: an explicit, INHERITANCE-PROTECTED +// DACL granting that user and SYSTEM, and nobody else. The sandbox principal is +// deliberately absent from it, which is the property that matters, because a +// principal that could read this file could mint its own token and the whole +// identity boundary would be decorative. Administrators are not added either; +// an admin can already take ownership, so naming them buys nothing and widens +// the visible grant. +// +// Ordering is load-bearing: the ACL is applied to an EMPTY file before the +// password is written, so the bytes never exist under the directory's inherited +// permissions even briefly. + +import ( + "errors" + "fmt" + "os" + "path/filepath" + "strings" + + "golang.org/x/sys/windows" +) + +// windowsSandboxSecretDirName holds per-principal secrets under the Zero config +// directory. Kept in its own directory so the whole set can be removed when the +// sandbox is torn down. +const windowsSandboxSecretDirName = "windows-sandbox" + +// windowsSandboxSecretPath returns where a principal's password lives. The +// account name is already sanitised to [a-z0-9-] by windowsSandboxUserName, so +// it cannot escape the directory. +func windowsSandboxSecretPath(configDir string, username string) (string, error) { + if strings.TrimSpace(configDir) == "" { + return "", errors.New("windows sandbox secret: empty config directory") + } + if strings.TrimSpace(username) == "" { + return "", errors.New("windows sandbox secret: empty principal name") + } + // Defence in depth against a caller passing something windowsSandboxUserName + // did not produce: refuse anything with a separator or a parent reference. + if strings.ContainsAny(username, `\/:`) || strings.Contains(username, "..") { + return "", fmt.Errorf("windows sandbox secret: unsafe principal name %q", username) + } + return filepath.Join(configDir, windowsSandboxSecretDirName, username+".secret"), nil +} + +// currentTokenUserSID returns the SID of the user this process runs as. Under +// UAC the elevated token keeps the same user SID as the desktop session, so +// setup and the later unelevated command path agree on the owner, which is what +// makes an owner-scoped ACL usable across the elevation boundary. +func currentTokenUserSID() (*windows.SID, error) { + var token windows.Token + if err := windows.OpenProcessToken(windows.CurrentProcess(), windows.TOKEN_QUERY, &token); err != nil { + return nil, fmt.Errorf("open process token: %w", err) + } + defer token.Close() + user, err := token.GetTokenUser() + if err != nil { + return nil, fmt.Errorf("get token user: %w", err) + } + // The SID points into a buffer owned by the Tokenuser, so copy it out before + // that buffer goes away. + copied, err := user.User.Sid.Copy() + if err != nil { + return nil, fmt.Errorf("copy token user SID: %w", err) + } + return copied, nil +} + +// lockWindowsSecretToOwner replaces a file's DACL with an explicit, +// inheritance-protected one granting only owner and SYSTEM. PROTECTED is what +// drops any ACE inherited from the config directory; without it a permissive +// parent would still grant access to whoever it names. +func lockWindowsSecretToOwner(path string, owner *windows.SID) error { + if owner == nil { + return errors.New("windows sandbox secret: nil owner SID") + } + system, err := windows.CreateWellKnownSid(windows.WinLocalSystemSid) + if err != nil { + return fmt.Errorf("resolve SYSTEM SID: %w", err) + } + entries := []windows.EXPLICIT_ACCESS{ + { + AccessPermissions: windows.GENERIC_ALL, + AccessMode: windows.GRANT_ACCESS, + Inheritance: windows.NO_INHERITANCE, + Trustee: windows.TRUSTEE{ + TrusteeForm: windows.TRUSTEE_IS_SID, + TrusteeType: windows.TRUSTEE_IS_USER, + TrusteeValue: windows.TrusteeValueFromSID(owner), + }, + }, + { + AccessPermissions: windows.GENERIC_ALL, + AccessMode: windows.GRANT_ACCESS, + Inheritance: windows.NO_INHERITANCE, + Trustee: windows.TRUSTEE{ + TrusteeForm: windows.TRUSTEE_IS_SID, + TrusteeType: windows.TRUSTEE_IS_USER, + TrusteeValue: windows.TrusteeValueFromSID(system), + }, + }, + } + acl, err := windows.ACLFromEntries(entries, nil) + if err != nil { + return fmt.Errorf("build secret ACL: %w", err) + } + if err := windows.SetNamedSecurityInfo( + path, + windows.SE_FILE_OBJECT, + windows.DACL_SECURITY_INFORMATION|windows.PROTECTED_DACL_SECURITY_INFORMATION, + nil, + nil, + acl, + nil, + ); err != nil { + return fmt.Errorf("lock secret to owner: %w", err) + } + return nil +} + +// writeWindowsSandboxSecret stores a principal's password readable only by the +// invoking user. +// +// The file is created empty, locked down, and only then written, so the secret +// is never on disk under the directory's inherited ACL. An existing file is +// replaced rather than appended, since a stale password would make LogonUser +// fail in a way that looks like a sandbox bug. +func writeWindowsSandboxSecret(path string, password string) error { + owner, err := currentTokenUserSID() + if err != nil { + return err + } + if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { + return fmt.Errorf("create secret directory: %w", err) + } + // Truncate any previous secret first: the ACL below is applied to whatever + // inode ends up at this path, so create it before locking it. + file, err := os.OpenFile(path, os.O_CREATE|os.O_TRUNC|os.O_WRONLY, 0o600) + if err != nil { + return fmt.Errorf("create secret file: %w", err) + } + if err := file.Close(); err != nil { + return fmt.Errorf("close secret file: %w", err) + } + if err := lockWindowsSecretToOwner(path, owner); err != nil { + // Do not leave an unprotected empty file behind. + _ = os.Remove(path) + return err + } + if err := os.WriteFile(path, []byte(password), 0o600); err != nil { + _ = os.Remove(path) + return fmt.Errorf("write secret: %w", err) + } + return nil +} + +// readWindowsSandboxSecret loads a principal's password. A missing file means +// setup has not run for this workspace, which the caller turns into a fallback +// rather than a hard failure. +func readWindowsSandboxSecret(path string) (string, error) { + data, err := os.ReadFile(path) + if err != nil { + if os.IsNotExist(err) { + return "", errWindowsSandboxIdentityUnavailable + } + return "", fmt.Errorf("read sandbox secret: %w", err) + } + secret := strings.TrimSpace(string(data)) + if secret == "" { + return "", errWindowsSandboxIdentityUnavailable + } + return secret, nil +} + +// removeWindowsSandboxSecret deletes a stored password. Called before the +// account itself is removed so a secret never outlives the principal it +// authenticates. +func removeWindowsSandboxSecret(path string) error { + if err := os.Remove(path); err != nil && !os.IsNotExist(err) { + return fmt.Errorf("remove sandbox secret: %w", err) + } + return nil +} diff --git a/internal/sandbox/windows_identity_secret_windows_test.go b/internal/sandbox/windows_identity_secret_windows_test.go new file mode 100644 index 000000000..d1ebdfee8 --- /dev/null +++ b/internal/sandbox/windows_identity_secret_windows_test.go @@ -0,0 +1,196 @@ +//go:build windows + +package sandbox + +import ( + "os" + "path/filepath" + "strings" + "testing" + "unsafe" + + "golang.org/x/sys/windows" +) + +func TestWindowsSandboxSecretRoundTrip(t *testing.T) { + path := filepath.Join(t.TempDir(), "cfg", "zero-sbx-test.secret") + const password = "Zs1!EXAMPLEPASSWORDVALUE" + + if err := writeWindowsSandboxSecret(path, password); err != nil { + t.Fatalf("write: %v", err) + } + got, err := readWindowsSandboxSecret(path) + if err != nil { + t.Fatalf("read: %v", err) + } + if got != password { + t.Fatalf("read %q, want the stored password", got) + } +} + +// THE security property: the stored password must be readable only by the user +// who owns it. If any other trustee appears in the DACL, and in particular the +// sandbox principal, that account could mint its own token and the identity +// boundary would be worthless. +func TestWindowsSandboxSecretIsLockedToOwner(t *testing.T) { + path := filepath.Join(t.TempDir(), "locked.secret") + if err := writeWindowsSandboxSecret(path, "Zs1!SECRET"); err != nil { + t.Fatalf("write: %v", err) + } + + descriptor, err := windows.GetNamedSecurityInfo( + path, + windows.SE_FILE_OBJECT, + windows.DACL_SECURITY_INFORMATION, + ) + if err != nil { + t.Fatalf("read back security info: %v", err) + } + dacl, _, err := descriptor.DACL() + if err != nil { + t.Fatalf("read DACL: %v", err) + } + if dacl == nil { + t.Fatal("secret has a nil DACL, which grants everyone access") + } + + owner, err := currentTokenUserSID() + if err != nil { + t.Fatalf("owner SID: %v", err) + } + system, err := windows.CreateWellKnownSid(windows.WinLocalSystemSid) + if err != nil { + t.Fatalf("SYSTEM SID: %v", err) + } + + entries, err := windowsSecretACEList(dacl) + if err != nil { + t.Fatalf("enumerate ACEs: %v", err) + } + if len(entries) == 0 { + t.Fatal("secret DACL has no ACEs") + } + for _, sid := range entries { + if sid.Equals(owner) || sid.Equals(system) { + continue + } + t.Fatalf("secret DACL grants an unexpected trustee %s; only the owner and SYSTEM may appear", sid) + } +} + +// The DACL must be inheritance-protected, otherwise a permissive ACE on the +// config directory would still reach the secret. +func TestWindowsSandboxSecretDaclIsProtected(t *testing.T) { + path := filepath.Join(t.TempDir(), "protected.secret") + if err := writeWindowsSandboxSecret(path, "Zs1!SECRET"); err != nil { + t.Fatalf("write: %v", err) + } + descriptor, err := windows.GetNamedSecurityInfo(path, windows.SE_FILE_OBJECT, windows.DACL_SECURITY_INFORMATION) + if err != nil { + t.Fatalf("read back security info: %v", err) + } + control, _, err := descriptor.Control() + if err != nil { + t.Fatalf("read control bits: %v", err) + } + if control&windows.SE_DACL_PROTECTED == 0 { + t.Fatal("secret DACL is not protected, so inherited ACEs still apply") + } +} + +// Rewriting must replace the previous secret rather than append to it, or +// LogonUser would be handed two concatenated passwords. +func TestWindowsSandboxSecretOverwrites(t *testing.T) { + path := filepath.Join(t.TempDir(), "rewrite.secret") + if err := writeWindowsSandboxSecret(path, "Zs1!FIRST"); err != nil { + t.Fatalf("first write: %v", err) + } + if err := writeWindowsSandboxSecret(path, "Zs1!SECOND"); err != nil { + t.Fatalf("second write: %v", err) + } + got, err := readWindowsSandboxSecret(path) + if err != nil { + t.Fatalf("read: %v", err) + } + if got != "Zs1!SECOND" { + t.Fatalf("read %q, want only the newest password", got) + } +} + +// A workspace whose setup has not run must report the actionable sentinel so the +// command path falls back to the restricted-token backend instead of failing. +func TestWindowsSandboxSecretMissingIsSentinel(t *testing.T) { + path := filepath.Join(t.TempDir(), "absent.secret") + if _, err := readWindowsSandboxSecret(path); err != errWindowsSandboxIdentityUnavailable { + t.Fatalf("missing secret returned %v, want errWindowsSandboxIdentityUnavailable", err) + } +} + +// An empty file is a half-written secret, not a valid empty password. +func TestWindowsSandboxSecretEmptyIsSentinel(t *testing.T) { + path := filepath.Join(t.TempDir(), "empty.secret") + if err := os.WriteFile(path, []byte(" \r\n"), 0o600); err != nil { + t.Fatalf("seed: %v", err) + } + if _, err := readWindowsSandboxSecret(path); err != errWindowsSandboxIdentityUnavailable { + t.Fatalf("empty secret returned %v, want the unavailable sentinel", err) + } +} + +// The principal name lands in a filename, so anything that could escape the +// directory has to be refused even though windowsSandboxUserName already +// sanitises its output. +func TestWindowsSandboxSecretPathRejectsTraversal(t *testing.T) { + for _, name := range []string{`..\evil`, "sub/dir", `C:\abs`, "..", ""} { + if _, err := windowsSandboxSecretPath(`C:\cfg`, name); err == nil { + t.Fatalf("principal name %q was accepted", name) + } + } + if _, err := windowsSandboxSecretPath("", "zero-sbx-a"); err == nil { + t.Fatal("empty config directory was accepted") + } + path, err := windowsSandboxSecretPath(`C:\cfg`, "zero-sbx-abc") + if err != nil { + t.Fatalf("valid name rejected: %v", err) + } + if !strings.HasSuffix(path, `zero-sbx-abc.secret`) { + t.Fatalf("unexpected secret path %q", path) + } +} + +// Removal must be idempotent so teardown converges the same way provisioning +// does, and must actually delete the secret. +func TestWindowsSandboxSecretRemoveIsIdempotent(t *testing.T) { + path := filepath.Join(t.TempDir(), "gone.secret") + if err := writeWindowsSandboxSecret(path, "Zs1!SECRET"); err != nil { + t.Fatalf("write: %v", err) + } + if err := removeWindowsSandboxSecret(path); err != nil { + t.Fatalf("first remove: %v", err) + } + if _, err := os.Stat(path); !os.IsNotExist(err) { + t.Fatalf("secret still present after removal (stat err %v)", err) + } + if err := removeWindowsSandboxSecret(path); err != nil { + t.Fatalf("removing an absent secret must succeed, got %v", err) + } +} + +// windowsSecretACEList returns the trustee SID of every ACE in a DACL so a test +// can assert exactly who is named. +func windowsSecretACEList(dacl *windows.ACL) ([]*windows.SID, error) { + var out []*windows.SID + for index := uint32(0); index < uint32(dacl.AceCount); index++ { + var ace *windows.ACCESS_ALLOWED_ACE + if err := windows.GetAce(dacl, index, &ace); err != nil { + return nil, err + } + sid := (*windows.SID)(unsafe.Pointer(&ace.SidStart)) + copied, err := sid.Copy() + if err != nil { + return nil, err + } + out = append(out, copied) + } + return out, nil +} From 93c4974d124e099f2edaf2f860f554326c50ef5e Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Sun, 26 Jul 2026 23:24:58 +0530 Subject: [PATCH 03/18] test(sandbox): name the per-shell env syntax in the provisioning skip The skip fired on an unset environment variable but read as though elevation was missing. `set VAR=1` is cmd syntax and sets a shell variable rather than an environment variable in PowerShell, so the test skipped silently after the operator believed they had enabled it. Spell out all three shells. --- internal/sandbox/windows_identity_windows_test.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/internal/sandbox/windows_identity_windows_test.go b/internal/sandbox/windows_identity_windows_test.go index e98ef85d9..9449562d4 100644 --- a/internal/sandbox/windows_identity_windows_test.go +++ b/internal/sandbox/windows_identity_windows_test.go @@ -199,7 +199,10 @@ func TestLSAStructLayouts(t *testing.T) { // be exercised without touching the account database. func TestProvisionWindowsSandboxIdentityRoundTrip(t *testing.T) { if os.Getenv("ZERO_WINDOWS_IDENTITY_PROVISION_TEST") != "1" { - t.Skip("set ZERO_WINDOWS_IDENTITY_PROVISION_TEST=1 on an elevated machine to exercise real provisioning") + // Spelled out per shell because `set VAR=1` is cmd syntax and silently + // sets a shell variable rather than an environment variable in + // PowerShell, which makes this skip look like the elevation check failing. + t.Skip("provisioning test not enabled: PowerShell `$env:ZERO_WINDOWS_IDENTITY_PROVISION_TEST = \"1\"`, cmd `set ZERO_WINDOWS_IDENTITY_PROVISION_TEST=1`, bash `export ZERO_WINDOWS_IDENTITY_PROVISION_TEST=1` (also needs an elevated terminal)") } if !windowsProcessIsElevated() { t.Skip("provisioning requires an elevated process") From 7b75a156ea35d22bc1f6e42afa2f4a68c88bdda6 Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Sun, 26 Jul 2026 23:33:08 +0530 Subject: [PATCH 04/18] test(sandbox): cover the logon-rights and token-minting half Provisioning is now validated on real Windows, but LsaAddAccountRights and LogonUser had still never executed, so the principal was proven to exist without being proven usable. The batch logon doubles as the assertion that the rights grant worked: a LOGON32_LOGON_BATCH logon fails with ERROR_LOGON_TYPE_NOT_GRANTED unless SeBatchLogonRight is actually held, so a token coming back is evidence the grant landed rather than merely that the call returned success. Granting twice is exercised too, since setup re-runs must not fail on rights already held. The token's user SID is compared against the principal's. If a token came back belonging to the caller the identity boundary would be an illusion and reads would still run as the user, which is the whole thing this model exists to stop. Removes any leftover account first and cleans up after itself, because an interrupted earlier run would leave an account whose password no longer matches a freshly generated one. --- .../sandbox/windows_identity_windows_test.go | 62 +++++++++++++++++++ 1 file changed, 62 insertions(+) diff --git a/internal/sandbox/windows_identity_windows_test.go b/internal/sandbox/windows_identity_windows_test.go index 9449562d4..480c61fd2 100644 --- a/internal/sandbox/windows_identity_windows_test.go +++ b/internal/sandbox/windows_identity_windows_test.go @@ -236,6 +236,68 @@ func TestProvisionWindowsSandboxIdentityRoundTrip(t *testing.T) { } } +// The other half of the privileged chain: granting logon rights and actually +// minting a token. Provisioning proves the account exists; this proves it is +// USABLE, which is the part the runner depends on. +// +// The batch logon doubles as the assertion that LsaAddAccountRights worked. A +// LOGON32_LOGON_BATCH logon fails with ERROR_LOGON_TYPE_NOT_GRANTED (1385) +// unless SeBatchLogonRight is actually held, so a token coming back is proof the +// grant landed rather than merely that the call returned success. +// +// Creates a real local account and removes it again, so it is gated the same way +// as the provisioning round-trip. +func TestGrantLogonRightsAndMintPrincipalToken(t *testing.T) { + if os.Getenv("ZERO_WINDOWS_IDENTITY_PROVISION_TEST") != "1" { + t.Skip("provisioning test not enabled: PowerShell `$env:ZERO_WINDOWS_IDENTITY_PROVISION_TEST = \"1\"`, cmd `set ZERO_WINDOWS_IDENTITY_PROVISION_TEST=1`, bash `export ZERO_WINDOWS_IDENTITY_PROVISION_TEST=1` (also needs an elevated terminal)") + } + if !windowsProcessIsElevated() { + t.Skip("granting logon rights requires an elevated process") + } + + 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)) + + identity, password, err := provisionWindowsSandboxIdentity(key) + if err != nil { + t.Fatalf("provision: %v", err) + } + t.Cleanup(func() { + if err := removeWindowsSandboxIdentity(identity.Username); err != nil { + t.Errorf("cleanup: %v", err) + } + }) + + // Exercises LsaAddAccountRights, including the LSA_UNICODE_STRING byte-length + // handling that nothing else has run. + if err := grantWindowsSandboxLogonRights(identity.SID); err != nil { + t.Fatalf("grant logon rights: %v", err) + } + // Idempotent: setup re-runs must not fail on rights the account already holds. + if err := grantWindowsSandboxLogonRights(identity.SID); err != nil { + t.Fatalf("granting logon rights twice must succeed: %v", err) + } + + token, err := logonWindowsSandboxPrincipal(identity.Username, password) + if err != nil { + t.Fatalf("logon as principal: %v", err) + } + defer token.Close() + + // The token must BE the principal. If this came back as the caller, the whole + // identity boundary would be an illusion and reads would still run as the user. + user, err := token.GetTokenUser() + if err != nil { + t.Fatalf("token user: %v", err) + } + if !user.User.Sid.Equals(identity.SID) { + t.Fatalf("token belongs to %s, want the principal %s", user.User.Sid, identity.SID) + } + t.Logf("minted a token for %s", identity) +} + // A workspace with no provisioned principal must report the actionable // "run setup" error rather than a raw lookup failure, so the command path can // fall back instead of surfacing a Win32 code. From 4d91a0eec2a4a959efa8a8d7ce5a1a78a9f1cb7d Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Sun, 26 Jul 2026 23:35:41 +0530 Subject: [PATCH 05/18] feat(sandbox): provision a sandbox principal during elevated setup Completes the chain. Until now the principal entry points had no non-test callers, so `zero sandbox setup` created no account and the runner seam always fell back: the feature was inert end to end. Setup now provisions this workspace's principal, grants it the batch logon right, stores its password locked to the invoking user, and applies the ACL plan that gives it read+write on the workspace and read on the declared read roots. A principal is a separate account with no inherent access to the caller's tree, so those grants are what make the sandbox able to run at all, and their absence elsewhere is what puts credential stores out of reach. Provisioning is folded into the existing rollback rather than each later failure path having to remember it, and the rollback revokes ACEs before deleting the account: removing the account first would leave ACEs naming a SID that no longer resolves, which is the orphaned residue this model exists to avoid. Gated on the same opt-in as the runner. Account creation is visible in `net user` and is exactly what endpoint protection and enterprise policy object to, so it happens only when asked for; without the opt-in the capability-SID backend remains the whole of setup, unchanged. --- .../windows_identity_runtime_windows.go | 45 +++++++++++++ .../windows_identity_runtime_windows_test.go | 63 +++++++++++++++++++ internal/sandbox/windows_setup_windows.go | 27 ++++++++ 3 files changed, 135 insertions(+) create mode 100644 internal/sandbox/windows_identity_runtime_windows_test.go diff --git a/internal/sandbox/windows_identity_runtime_windows.go b/internal/sandbox/windows_identity_runtime_windows.go index f7a4cef34..6e885b684 100644 --- a/internal/sandbox/windows_identity_runtime_windows.go +++ b/internal/sandbox/windows_identity_runtime_windows.go @@ -126,6 +126,51 @@ func provisionWindowsSandboxPrincipalForSetup(config WindowsSandboxCommandConfig return identity, nil } +// setupWindowsSandboxPrincipal provisions this workspace's principal and grants +// it the filesystem access its permission profile describes. It returns a +// rollback that undoes everything it created, so a later setup step failing does +// not leave a half-provisioned account behind. +// +// Rollback order is the inverse of creation and matters: ACEs are revoked BEFORE +// the account is deleted, because removing the account first would leave ACEs +// naming a SID that no longer resolves, which is the orphaned-entry residue this +// model exists to avoid. +func setupWindowsSandboxPrincipal(config WindowsSandboxCommandConfig) (func() error, error) { + identity, err := provisionWindowsSandboxPrincipalForSetup(config) + if err != nil { + return nil, err + } + removePrincipal := func() error { return removeWindowsSandboxPrincipalForSetup(config) } + + filesystem := config.PermissionProfile.FileSystem + plan, err := buildWindowsPrincipalACLPlan(windowsPrincipalACLInput{ + PrincipalSID: identity.SID.String(), + WriteRoots: filesystem.WriteRoots, + ReadRoots: filesystem.ReadRoots, + DenyRead: filesystem.DenyRead, + }) + if err != nil { + _ = removePrincipal() + return nil, err + } + revertACL, err := applyWindowsACLPlan(plan) + 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 + } + return removeErr + }, nil +} + // removeWindowsSandboxPrincipalForSetup retires a workspace's principal: secret // first, then the account. ACE revocation is the caller's job and must happen // before this, or ACEs naming a deleted SID are left behind. diff --git a/internal/sandbox/windows_identity_runtime_windows_test.go b/internal/sandbox/windows_identity_runtime_windows_test.go new file mode 100644 index 000000000..536cefea0 --- /dev/null +++ b/internal/sandbox/windows_identity_runtime_windows_test.go @@ -0,0 +1,63 @@ +//go:build windows + +package sandbox + +import "testing" + +// Setup must stay inert unless the principal backend is explicitly opted into. +// This is the property that makes the branch safe to merge while the privileged +// paths are still being validated: without the opt-in, `zero sandbox setup` +// creates no local account and the capability-SID backend is the whole of setup. +func TestWindowsSandboxIdentityGating(t *testing.T) { + for name, testCase := range map[string]struct { + env map[string]string + want bool + }{ + "absent": {env: map[string]string{}, want: false}, + "empty": {env: map[string]string{windowsSandboxIdentityEnv: ""}, want: false}, + "zero": {env: map[string]string{windowsSandboxIdentityEnv: "0"}, want: false}, + "true not one": {env: map[string]string{windowsSandboxIdentityEnv: "true"}, want: false}, + "one": {env: map[string]string{windowsSandboxIdentityEnv: "1"}, want: true}, + "one with space": {env: map[string]string{windowsSandboxIdentityEnv: " 1 "}, want: true}, + } { + t.Run(name, func(t *testing.T) { + if got := windowsSandboxIdentityEnabled(testCase.env); got != testCase.want { + t.Fatalf("enabled = %v, want %v for %q", got, testCase.want, testCase.env[windowsSandboxIdentityEnv]) + } + }) + } +} + +// The command environment wins over the process environment, so a run can opt in +// or out without depending on how the parent shell was launched. +func TestWindowsSandboxIdentityEnvOverridesProcess(t *testing.T) { + t.Setenv(windowsSandboxIdentityEnv, "1") + if windowsSandboxIdentityEnabled(map[string]string{windowsSandboxIdentityEnv: "0"}) { + t.Fatal("command env set to 0 must override a process env of 1") + } + if !windowsSandboxIdentityEnabled(map[string]string{}) { + t.Fatal("with no command-env entry the process env should apply") + } +} + +// One workspace maps to one principal, and different workspaces must not share +// an account, or two projects would run under the same identity and could reach +// each other's granted roots. +func TestWindowsSandboxWorkspaceKeyIsStableAndDistinct(t *testing.T) { + first := windowsSandboxWorkspaceKey([]string{`C:\ws\alpha`}) + again := windowsSandboxWorkspaceKey([]string{`C:\ws\alpha`}) + other := windowsSandboxWorkspaceKey([]string{`C:\ws\beta`}) + if first != again { + t.Fatalf("key is not stable: %q vs %q", first, again) + } + if first == other { + t.Fatal("two different workspaces produced the same principal key") + } + if first == "" { + t.Fatal("empty key") + } + // An empty root list still has to yield a usable key rather than a blank one. + if windowsSandboxWorkspaceKey(nil) == "" { + t.Fatal("no workspace roots produced an empty key") + } +} diff --git a/internal/sandbox/windows_setup_windows.go b/internal/sandbox/windows_setup_windows.go index 888355397..ac8dc1243 100644 --- a/internal/sandbox/windows_setup_windows.go +++ b/internal/sandbox/windows_setup_windows.go @@ -35,6 +35,33 @@ func runWindowsSandboxSetup(config WindowsSandboxSetupConfig, stderr io.Writer) fmt.Fprintln(stderr, WindowsSandboxSetupName+": "+err.Error()) return 1 } + // Provision this workspace's sandbox principal, when opted in. A principal is + // a separate local account, so it is created only on an explicit opt-in: it + // is visible in `net user`, and account creation is exactly the kind of thing + // endpoint protection and enterprise policy object to. Without the opt-in the + // capability-SID backend above is the whole of setup, unchanged. + if windowsSandboxIdentityEnabled(config.commandConfig().Env) { + principalRollback, err := setupWindowsSandboxPrincipal(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 + } + // Fold the principal into the existing rollback so every later failure + // path undoes it too, rather than each one having to remember. + aclRollback := rollback + rollback = func() error { + principalErr := principalRollback() + aclErr := aclRollback() + if principalErr != nil { + return principalErr + } + return aclErr + } + } if err := applyWindowsNetworkPlan(networkPlan); err != nil { if rollbackErr := rollback(); rollbackErr != nil { fmt.Fprintf(stderr, "%s: %v; rollback failed: %v\n", WindowsSandboxSetupName, err, rollbackErr) From 57421b290a5c7043c5e972b5644c452426796305 Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Sun, 26 Jul 2026 23:43:35 +0530 Subject: [PATCH 06/18] fix(sandbox): keep the restricted token when the network is denied Network denial is enforced by WFP filters keyed to the offline-marker SID. The restricted token carries that SID; a token from LogonUser cannot, because it names the account rather than a synthetic capability SID. Routing a denied-network command through a sandbox principal therefore left the block filters matching nothing and dropped egress enforcement altogether, and deny is the default mode. The principal now stands down whenever the network is denied and the restricted-token backend runs instead, so read confinement is never traded for a silent loss of network denial. Keying the filters to the principal's own SID is the follow-up that lifts the restriction. The decision sits in its own predicate rather than inline: on a machine with nothing provisioned the lookup declines for its own reasons, so a test that called through it would have passed with the guard removed. Also names the opt-out variable when a provisioned principal cannot be used, since the backend is opt-in and the operator needs a way back. --- .../sandbox/windows_command_runner_windows.go | 6 +++- .../windows_identity_runtime_windows.go | 22 ++++++++++++- .../windows_identity_runtime_windows_test.go | 32 +++++++++++++++++++ 3 files changed, 58 insertions(+), 2 deletions(-) diff --git a/internal/sandbox/windows_command_runner_windows.go b/internal/sandbox/windows_command_runner_windows.go index 413e41899..22d290d3e 100644 --- a/internal/sandbox/windows_command_runner_windows.go +++ b/internal/sandbox/windows_command_runner_windows.go @@ -83,7 +83,11 @@ func runWindowsSandboxCommand(config WindowsSandboxCommandConfig, stderr io.Writ // the restricted-token backend below runs exactly as before. principalToken, ok, err := windowsSandboxPrincipalToken(config) if err != nil { - fmt.Fprintln(stderr, WindowsSandboxCommandRunnerName+": "+err.Error()) + // The one path here that does not fall back, because a provisioned but + // unusable principal means the sandbox is broken rather than absent. Say + // how to get out of it, since the whole backend is opt-in. + fmt.Fprintf(stderr, "%s: sandbox principal is provisioned but unusable: %v. Re-run `zero sandbox setup` from an elevated terminal, or unset %s to fall back to the restricted-token sandbox.\n", + WindowsSandboxCommandRunnerName, err, windowsSandboxIdentityEnv) return 1 } if ok { diff --git a/internal/sandbox/windows_identity_runtime_windows.go b/internal/sandbox/windows_identity_runtime_windows.go index 6e885b684..bc850b48f 100644 --- a/internal/sandbox/windows_identity_runtime_windows.go +++ b/internal/sandbox/windows_identity_runtime_windows.go @@ -56,6 +56,26 @@ func windowsSandboxWorkspaceKey(workspaceRoots []string) string { return hex.EncodeToString(digest[:]) } +// windowsSandboxPrincipalEligible reports whether the principal backend may be +// used for this command at all, before any account or secret is consulted. +// +// Kept separate from the lookup so the decision is observable on its own: on a +// 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 +} + // windowsSandboxPrincipalToken returns a token for this workspace's sandbox // principal. // @@ -65,7 +85,7 @@ func windowsSandboxWorkspaceKey(workspaceRoots []string) string { // means the identity exists but could not be used, which is worth surfacing // rather than downgrading around. func windowsSandboxPrincipalToken(config WindowsSandboxCommandConfig) (windows.Token, bool, error) { - if !windowsSandboxIdentityEnabled(config.Env) { + if !windowsSandboxPrincipalEligible(config) { return 0, false, nil } key := windowsSandboxWorkspaceKey(config.WorkspaceRoots) diff --git a/internal/sandbox/windows_identity_runtime_windows_test.go b/internal/sandbox/windows_identity_runtime_windows_test.go index 536cefea0..d8658a144 100644 --- a/internal/sandbox/windows_identity_runtime_windows_test.go +++ b/internal/sandbox/windows_identity_runtime_windows_test.go @@ -61,3 +61,35 @@ func TestWindowsSandboxWorkspaceKeyIsStableAndDistinct(t *testing.T) { t.Fatal("no workspace roots produced an empty key") } } + +// Network denial is enforced by WFP filters keyed to the offline-marker SID, +// which only the restricted token carries. A principal token would leave those +// 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) { + eligible := func(mode NetworkMode, optIn string) bool { + config := WindowsSandboxCommandConfig{ + SandboxHome: t.TempDir(), + WorkspaceRoots: []string{t.TempDir()}, + Env: map[string]string{windowsSandboxIdentityEnv: optIn}, + } + config.PermissionProfile.Network.Mode = mode + 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") + } + if eligible(NetworkAllow, "0") { + t.Fatal("principal backend eligible without the opt-in") + } +} From 655c87231001d12699956370e1bb1c9c2e3ef344 Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Sun, 26 Jul 2026 23:43:43 +0530 Subject: [PATCH 07/18] feat(sandbox): encrypt the stored principal password to the invoking user The file ACL stays the primary control and is what keeps the sandbox principal from reading its own credential. It only binds while the filesystem is the one being asked, though, so a backup or a mounted image hands over the password in the clear. CryptProtectData ties the ciphertext to the invoking user's logon secret, which covers exactly that gap. The principal name is passed as entropy, so a blob copied onto another principal's path fails to decrypt rather than authenticating the wrong account. A secret written by an older build reads as unavailable and falls back to the restricted token; the next elevated setup rewrites it. The round-trip test needs no privilege, so it runs everywhere rather than joining the gated set, and it asserts the password does not appear verbatim in the stored bytes. --- .../sandbox/windows_identity_dpapi_windows.go | 76 +++++++++++++++++++ .../windows_identity_secret_windows.go | 31 +++++++- .../windows_identity_secret_windows_test.go | 76 +++++++++++++++++++ 3 files changed, 180 insertions(+), 3 deletions(-) create mode 100644 internal/sandbox/windows_identity_dpapi_windows.go diff --git a/internal/sandbox/windows_identity_dpapi_windows.go b/internal/sandbox/windows_identity_dpapi_windows.go new file mode 100644 index 000000000..d9a0bbf1d --- /dev/null +++ b/internal/sandbox/windows_identity_dpapi_windows.go @@ -0,0 +1,76 @@ +//go:build windows + +package sandbox + +// DPAPI wrapping for the stored sandbox principal password. +// +// The file ACL is the primary control and remains the thing that keeps the +// sandbox principal itself from reading its own credential. This adds the layer +// the ACL cannot: an ACL is only meaningful while the filesystem is being asked +// to enforce it, so a backup, a mounted disk image, or a copy taken by anyone +// who can bypass the DACL yields the password in the clear. CryptProtectData +// binds the ciphertext to the invoking user's logon secret, so an offline copy +// is inert without that user's credentials. +// +// The principal name is passed as optional entropy, which makes a blob usable +// only for the account it was minted for; moving one secret file over another +// then fails to decrypt instead of silently authenticating the wrong principal. + +import ( + "errors" + "fmt" + "unsafe" + + "golang.org/x/sys/windows" +) + +// protectWindowsSecret encrypts a password to the current user. +// +// CRYPTPROTECT_UI_FORBIDDEN matters here: this runs inside a CLI and, on the +// setup path, potentially without an interactive desktop, so DPAPI must fail +// rather than try to prompt. +func protectWindowsSecret(plaintext string, entropy string) ([]byte, error) { + if plaintext == "" { + return nil, errors.New("windows sandbox secret: refusing to protect an empty password") + } + in := windows.DataBlob{ + Size: uint32(len(plaintext)), + Data: &[]byte(plaintext)[0], + } + entropyBytes := []byte(entropy) + var entropyBlob *windows.DataBlob + if len(entropyBytes) > 0 { + entropyBlob = &windows.DataBlob{Size: uint32(len(entropyBytes)), Data: &entropyBytes[0]} + } + var out windows.DataBlob + if err := windows.CryptProtectData(&in, nil, entropyBlob, 0, nil, windows.CRYPTPROTECT_UI_FORBIDDEN, &out); err != nil { + return nil, fmt.Errorf("protect sandbox secret: %w", err) + } + // DPAPI allocates the output with LocalAlloc; copy it out and hand it back. + defer windows.LocalFree(windows.Handle(unsafe.Pointer(out.Data))) + return append([]byte(nil), unsafe.Slice(out.Data, out.Size)...), nil +} + +// unprotectWindowsSecret reverses protectWindowsSecret. It fails for any user +// other than the one that wrote the blob, and for a blob minted with a different +// principal name as entropy. +func unprotectWindowsSecret(ciphertext []byte, entropy string) (string, error) { + if len(ciphertext) == 0 { + return "", errWindowsSandboxIdentityUnavailable + } + in := windows.DataBlob{ + Size: uint32(len(ciphertext)), + Data: &ciphertext[0], + } + entropyBytes := []byte(entropy) + var entropyBlob *windows.DataBlob + if len(entropyBytes) > 0 { + entropyBlob = &windows.DataBlob{Size: uint32(len(entropyBytes)), Data: &entropyBytes[0]} + } + var out windows.DataBlob + if err := windows.CryptUnprotectData(&in, nil, entropyBlob, 0, nil, windows.CRYPTPROTECT_UI_FORBIDDEN, &out); err != nil { + return "", fmt.Errorf("unprotect sandbox secret: %w", err) + } + defer windows.LocalFree(windows.Handle(unsafe.Pointer(out.Data))) + return string(unsafe.Slice(out.Data, out.Size)), nil +} diff --git a/internal/sandbox/windows_identity_secret_windows.go b/internal/sandbox/windows_identity_secret_windows.go index 37781032b..96cee6305 100644 --- a/internal/sandbox/windows_identity_secret_windows.go +++ b/internal/sandbox/windows_identity_secret_windows.go @@ -158,13 +158,28 @@ func writeWindowsSandboxSecret(path string, password string) error { _ = os.Remove(path) return err } - if err := os.WriteFile(path, []byte(password), 0o600); err != nil { + // Encrypt to the invoking user on top of the ACL, so a copy taken outside the + // filesystem's enforcement (backup, disk image) is inert. The principal name is + // the entropy, which keeps one principal's blob from authenticating another. + sealed, err := protectWindowsSecret(password, windowsSandboxSecretEntropy(path)) + if err != nil { + _ = os.Remove(path) + return err + } + if err := os.WriteFile(path, sealed, 0o600); err != nil { _ = os.Remove(path) return fmt.Errorf("write secret: %w", err) } return nil } +// windowsSandboxSecretEntropy derives the DPAPI entropy from the secret's own +// filename, which is the principal name. Deriving it rather than threading the +// name through keeps read and write agreeing by construction. +func windowsSandboxSecretEntropy(path string) string { + return strings.TrimSuffix(filepath.Base(path), ".secret") +} + // readWindowsSandboxSecret loads a principal's password. A missing file means // setup has not run for this workspace, which the caller turns into a fallback // rather than a hard failure. @@ -176,8 +191,18 @@ func readWindowsSandboxSecret(path string) (string, error) { } return "", fmt.Errorf("read sandbox secret: %w", err) } - secret := strings.TrimSpace(string(data)) - if secret == "" { + if len(data) == 0 { + return "", errWindowsSandboxIdentityUnavailable + } + secret, err := unprotectWindowsSecret(data, windowsSandboxSecretEntropy(path)) + if err != nil { + // A blob written by another user, for another principal, or by an older + // build that stored the password in the clear. Report it as unavailable so + // the caller falls back to the restricted token; the next elevated setup + // rewrites the secret in the current format. + return "", errWindowsSandboxIdentityUnavailable + } + if strings.TrimSpace(secret) == "" { return "", errWindowsSandboxIdentityUnavailable } return secret, nil diff --git a/internal/sandbox/windows_identity_secret_windows_test.go b/internal/sandbox/windows_identity_secret_windows_test.go index d1ebdfee8..b77d0f5a0 100644 --- a/internal/sandbox/windows_identity_secret_windows_test.go +++ b/internal/sandbox/windows_identity_secret_windows_test.go @@ -3,6 +3,8 @@ package sandbox import ( + "bytes" + "errors" "os" "path/filepath" "strings" @@ -194,3 +196,77 @@ func windowsSecretACEList(dacl *windows.ACL) ([]*windows.SID, error) { } return out, nil } + +// DPAPI round-trip through the real store, which needs no privilege and so is +// genuine coverage rather than a gated stub. +func TestWindowsSandboxSecretRoundTripsThroughDPAPI(t *testing.T) { + path, err := windowsSandboxSecretPath(t.TempDir(), "zero-sbx-roundtrip") + if err != nil { + t.Fatalf("secret path: %v", err) + } + const password = "S0me-Sandbox-P@ssw0rd-value" + if err := writeWindowsSandboxSecret(path, password); err != nil { + t.Fatalf("write secret: %v", err) + } + got, err := readWindowsSandboxSecret(path) + if err != nil { + t.Fatalf("read secret: %v", err) + } + if got != password { + t.Fatalf("round-trip returned %q, want %q", got, password) + } + // The point of the exercise: the password must not be recoverable by reading + // the file, or the encryption layer is decorative. + raw, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read raw secret file: %v", err) + } + if bytes.Contains(raw, []byte(password)) { + t.Fatal("the password appears verbatim in the stored file; it was not encrypted") + } +} + +// Entropy is the principal name, so a blob moved onto another principal's secret +// path must fail to decrypt rather than authenticate the wrong account. +func TestWindowsSandboxSecretDoesNotTransferBetweenPrincipals(t *testing.T) { + home := t.TempDir() + minePath, err := windowsSandboxSecretPath(home, "zero-sbx-mine") + if err != nil { + t.Fatalf("secret path: %v", err) + } + theirsPath, err := windowsSandboxSecretPath(home, "zero-sbx-theirs") + if err != nil { + t.Fatalf("secret path: %v", err) + } + if err := writeWindowsSandboxSecret(minePath, "a-password-for-mine"); err != nil { + t.Fatalf("write secret: %v", err) + } + blob, err := os.ReadFile(minePath) + if err != nil { + t.Fatalf("read blob: %v", err) + } + if err := os.WriteFile(theirsPath, blob, 0o600); err != nil { + t.Fatalf("plant blob: %v", err) + } + if _, err := readWindowsSandboxSecret(theirsPath); !errors.Is(err, errWindowsSandboxIdentityUnavailable) { + t.Fatalf("a blob planted at another principal's path decrypted; got err = %v", err) + } +} + +// An older plaintext secret must degrade to a fallback rather than being handed +// to LogonUser as if it were a password. +func TestWindowsSandboxSecretRejectsLegacyPlaintext(t *testing.T) { + path, err := windowsSandboxSecretPath(t.TempDir(), "zero-sbx-legacy") + if err != nil { + t.Fatalf("secret path: %v", err) + } + if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { + t.Fatalf("mkdir: %v", err) + } + if err := os.WriteFile(path, []byte("plaintext-password"), 0o600); err != nil { + t.Fatalf("write legacy secret: %v", err) + } + if _, err := readWindowsSandboxSecret(path); !errors.Is(err, errWindowsSandboxIdentityUnavailable) { + t.Fatalf("legacy plaintext secret was accepted; got err = %v", err) + } +} From da835b554846274793e27c692f8eee00c91a95e5 Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Mon, 27 Jul 2026 09:03:13 +0530 Subject: [PATCH 08/18] fix(sandbox): surface a squatted principal name instead of falling back lookupWindowsSandboxIdentity collapsed every SID-resolution failure into the "no principal is provisioned" sentinel, which threw away the check resolveWindowsSandboxSID deliberately makes: a name that resolves to a group or alias rather than a user account. The command path treats that sentinel as permission to fall back quietly, so an account name squatted by something that is not a user reached the operator as silence and a downgrade to the restricted token. Caught by gnanam in review. Only ERROR_NONE_MAPPED now means setup has not run. Anything else is a principal that exists but cannot be used, and the runtime path propagates it rather than swallowing it, which is where the description already said the line should sit. The decision lives in its own function because the lookup derives its account name from a workspace key, so a test cannot hand it a name that resolves to a group. The test drives the classifier with a real error from a well-known local group, needs no privilege, and fails if the old collapse-everything behaviour is restored. Also corrects a comment pointing at sandboxRuntimeKey, which does not exist. The function is windowsSandboxWorkspaceKey. --- .../windows_identity_runtime_windows.go | 10 ++++- internal/sandbox/windows_identity_windows.go | 27 ++++++++++++- .../sandbox/windows_identity_windows_test.go | 38 +++++++++++++++++++ 3 files changed, 71 insertions(+), 4 deletions(-) diff --git a/internal/sandbox/windows_identity_runtime_windows.go b/internal/sandbox/windows_identity_runtime_windows.go index bc850b48f..8ec6a43d3 100644 --- a/internal/sandbox/windows_identity_runtime_windows.go +++ b/internal/sandbox/windows_identity_runtime_windows.go @@ -91,8 +91,14 @@ func windowsSandboxPrincipalToken(config WindowsSandboxCommandConfig) (windows.T key := windowsSandboxWorkspaceKey(config.WorkspaceRoots) identity, err := lookupWindowsSandboxIdentity(key) if err != nil { - // Not provisioned: fall back quietly, this is the default state. - return 0, false, nil + if errors.Is(err, errWindowsSandboxIdentityUnavailable) { + // Not provisioned: fall back quietly, this is the default state. + return 0, false, nil + } + // The name resolves to something that is not a usable principal, most + // likely squatted by a local group or alias. That is a conflict an + // operator has to see, not a reason to pretend setup never ran. + return 0, false, err } secretPath, err := windowsSandboxSecretPath(config.SandboxHome, identity.Username) if err != nil { diff --git a/internal/sandbox/windows_identity_windows.go b/internal/sandbox/windows_identity_windows.go index f8711d053..2fad0ecbd 100644 --- a/internal/sandbox/windows_identity_windows.go +++ b/internal/sandbox/windows_identity_windows.go @@ -119,7 +119,7 @@ func (identity windowsSandboxIdentity) String() string { } // windowsSandboxUserName derives a stable account name for a workspace key. The -// key is hashed by the caller (see sandboxRuntimeKey) so the name reveals no +// 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. @@ -339,7 +339,30 @@ func lookupWindowsSandboxIdentity(workspaceKey string) (windowsSandboxIdentity, username := windowsSandboxUserName(workspaceKey) sid, err := resolveWindowsSandboxSID(username) if err != nil { - return windowsSandboxIdentity{}, errWindowsSandboxIdentityUnavailable + return windowsSandboxIdentity{}, classifyWindowsSandboxLookupError(err) } return windowsSandboxIdentity{Username: username, SID: sid}, nil } + +// classifyWindowsSandboxLookupError decides whether a failed SID resolution +// means "setup has not run" or "this principal exists but is unusable". +// +// Only "no such account" is the former. Every other failure is a principal the +// caller must not paper over, including the deliberate refusal in +// resolveWindowsSandboxSID of a name squatted by a group or alias. Collapsing +// those into the unavailable sentinel would turn a real conflict into a silent +// fall back to the restricted token, which is exactly the case that should +// reach the operator rather than be absorbed. +// +// Split out from the lookup so the decision can be asserted on its own: the +// lookup derives its account name from a workspace key, so a test cannot hand +// it a name that resolves to a group. +func classifyWindowsSandboxLookupError(err error) error { + if err == nil { + return nil + } + if errors.Is(err, windows.ERROR_NONE_MAPPED) { + return errWindowsSandboxIdentityUnavailable + } + return err +} diff --git a/internal/sandbox/windows_identity_windows_test.go b/internal/sandbox/windows_identity_windows_test.go index 480c61fd2..754952f66 100644 --- a/internal/sandbox/windows_identity_windows_test.go +++ b/internal/sandbox/windows_identity_windows_test.go @@ -3,6 +3,7 @@ package sandbox import ( + "errors" "os" "strings" "testing" @@ -310,3 +311,40 @@ func TestLookupWindowsSandboxIdentityUnprovisioned(t *testing.T) { t.Fatalf("error = %v, want errWindowsSandboxIdentityUnavailable", err) } } + +// A name that resolves to something other than a user account is a conflict, +// not an absent principal, and must not be reported as "setup has not run": the +// command path treats that sentinel as permission to fall back silently, so +// collapsing the two would hide a squatted account behind a quiet downgrade to +// the restricted token. +// +// Every machine already has well-known non-user names to test against, so this +// needs no privilege and no provisioning. +func TestLookupWindowsSandboxIdentityRejectsNonUserAccount(t *testing.T) { + // Groups that exist on any Windows install. Whichever resolves first is + // enough; localized machines may not carry the English name. + for _, group := range []string{"Administrators", "Users", "Guests"} { + sid, _, accountType, err := windows.LookupSID("", group) + if err != nil || sid == nil { + continue + } + if accountType == windows.SidTypeUser { + continue + } + resolveErr := func() error { + _, err := resolveWindowsSandboxSID(group) + return err + }() + if resolveErr == nil { + t.Fatalf("resolveWindowsSandboxSID(%q) accepted a non-user account (type %d)", group, accountType) + } + // The classification is the part that matters: the sentinel is what + // licenses the command path to fall back silently, so this refusal must + // survive it rather than be folded into it. + if classified := classifyWindowsSandboxLookupError(resolveErr); errors.Is(classified, errWindowsSandboxIdentityUnavailable) { + t.Fatalf("non-user account %q classified as unprovisioned, which would silently downgrade to the restricted token: %v", group, classified) + } + return + } + t.Skip("no well-known non-user account resolved on this machine") +} From a594bbbf2fd9708301657322d5d385028474df56 Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Mon, 27 Jul 2026 09:16:24 +0530 Subject: [PATCH 09/18] fix(sandbox): reset the password when the account already exists NetUserAdd leaves a pre-existing account completely untouched, password included, and ensureWindowsSandboxUser treated that status as success. So a second setup run generated a fresh random password, stored it as the secret, and left the account still authenticating with the old one. Every later command then failed to log on with a principal that looked correctly provisioned. Two comments claimed the caller reset the password in that case; nothing did. Caught by CodeRabbit. ensureWindowsSandboxUser now reports whether the account already existed, and provisioning resets the password through NetUserSetInfo when it did, so the value it returns is always the account's real password. The comments now describe what the code does. The gated provisioning test provisions twice and then logs on with the password from the SECOND run, which is the only honest assertion here: a stale password is indistinguishable from a correct one until something tries to authenticate with it. Also makes the syscall keep-alives explicit. The LSA and LogonUser call sites borrow Go memory that was either not kept alive at all (the policy attributes, the rights descriptor, the three logon strings) or kept alive only after the error check, so the failure path returned with it already collectable. The two netapi32 sites that used a deferred no-op closure now use runtime.KeepAlive as well, so one idiom is used throughout. --- .../sandbox/windows_identity_logon_windows.go | 15 ++- .../windows_identity_runtime_windows.go | 8 +- internal/sandbox/windows_identity_windows.go | 106 ++++++++++++++---- .../sandbox/windows_identity_windows_test.go | 19 +++- 4 files changed, 120 insertions(+), 28 deletions(-) diff --git a/internal/sandbox/windows_identity_logon_windows.go b/internal/sandbox/windows_identity_logon_windows.go index 6f9d689fb..6c7acd0df 100644 --- a/internal/sandbox/windows_identity_logon_windows.go +++ b/internal/sandbox/windows_identity_logon_windows.go @@ -19,6 +19,7 @@ package sandbox import ( "fmt" + "runtime" "unsafe" "golang.org/x/sys/windows" @@ -118,6 +119,9 @@ func grantWindowsSandboxLogonRights(sid *windows.SID) error { uintptr(policyCreateAccount|policyLookupNames), uintptr(unsafe.Pointer(&policy)), ) + // LsaOpenPolicy borrows the attributes struct by address, so it has to stay + // reachable until the call has returned. + runtime.KeepAlive(attributes) if err := lsaStatusError("LsaOpenPolicy", status); err != nil { return err } @@ -144,11 +148,14 @@ func grantWindowsSandboxLogonRights(sid *windows.SID) error { uintptr(unsafe.Pointer(&entry)), 1, ) + // Both the descriptor and the buffer it points at are borrowed by the + // call. Kept alive before the error check, not after, so the failure path + // does not return with them already collectable. + runtime.KeepAlive(entry) + runtimeKeepAliveUint16(buffer) if err := lsaStatusError("LsaAddAccountRights("+right+")", status); err != nil { return err } - // Keep the backing buffer alive until the call has returned. - runtimeKeepAliveUint16(buffer) } return nil } @@ -183,6 +190,10 @@ func logonWindowsSandboxPrincipal(username string, password string) (windows.Tok logon32ProviderDefault, uintptr(unsafe.Pointer(&token)), ) + // The three strings are borrowed for the duration of the call. + runtime.KeepAlive(user) + runtime.KeepAlive(domain) + runtime.KeepAlive(secret) if result == 0 { if callErr != nil && callErr != windows.ERROR_SUCCESS { return 0, fmt.Errorf("LogonUser(%s): %w", username, callErr) diff --git a/internal/sandbox/windows_identity_runtime_windows.go b/internal/sandbox/windows_identity_runtime_windows.go index 8ec6a43d3..a3a201799 100644 --- a/internal/sandbox/windows_identity_runtime_windows.go +++ b/internal/sandbox/windows_identity_runtime_windows.go @@ -142,10 +142,10 @@ func provisionWindowsSandboxPrincipalForSetup(config WindowsSandboxCommandConfig if err != nil { return windowsSandboxIdentity{}, err } - // A pre-existing account keeps its old password, which this new one does not - // match, so the secret is rewritten every run to stay in step with whatever - // NetUserAdd left in place. On a fresh account the two agree by construction; - // on an existing one the caller resets it via ensureWindowsSandboxUser. + // 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. if err := writeWindowsSandboxSecret(secretPath, password); err != nil { return windowsSandboxIdentity{}, err } diff --git a/internal/sandbox/windows_identity_windows.go b/internal/sandbox/windows_identity_windows.go index 2fad0ecbd..2e1169dbe 100644 --- a/internal/sandbox/windows_identity_windows.go +++ b/internal/sandbox/windows_identity_windows.go @@ -27,6 +27,7 @@ import ( "encoding/base32" "errors" "fmt" + "runtime" "strings" "unsafe" @@ -75,6 +76,7 @@ var ( procNetLocalGroupAdd = netapi32.NewProc("NetLocalGroupAdd") procNetLocalGroupAddMembers = netapi32.NewProc("NetLocalGroupAddMembers") procNetUserDel = netapi32.NewProc("NetUserDel") + procNetUserSetInfo = netapi32.NewProc("NetUserSetInfo") ) // userInfo1 mirrors USER_INFO_1. Field order and widths must match the Win32 @@ -90,6 +92,12 @@ type userInfo1 struct { ScriptPath *uint16 } +// userInfo1003 mirrors USER_INFO_1003, the password-only form NetUserSetInfo +// takes when nothing else about the account should change. +type userInfo1003 struct { + Password *uint16 +} + // localGroupInfo1 mirrors LOCALGROUP_INFO_1. type localGroupInfo1 struct { Name *uint16 @@ -199,29 +207,34 @@ func ensureWindowsSandboxGroup() error { uintptr(unsafe.Pointer(&info)), 0, ) - // Keep info alive across the call: the struct holds pointers into Go memory - // that the syscall dereferences. - defer func() { _ = info }() + // The struct holds pointers into Go memory that the syscall dereferences, so + // it has to stay reachable until the call has returned. + runtime.KeepAlive(info) + runtime.KeepAlive(name) + runtime.KeepAlive(comment) return netAPIStatus("NetLocalGroupAdd", status, nerrGroupExists, errorAliasExists) } -// ensureWindowsSandboxUser creates a sandbox account with the supplied password, -// or leaves an existing account alone. The account is a plain local user with no -// home directory or logon script, flagged so its password never expires (nobody -// is there to rotate it) and so it is a normal, enabled account LogonUser can -// authenticate. -func ensureWindowsSandboxUser(username string, password string) error { +// ensureWindowsSandboxUser creates a sandbox account with the supplied password. +// The account is a plain local user with no home directory or logon script, +// flagged so its password never expires (nobody is there to rotate it) and so it +// is a normal, enabled account LogonUser can authenticate. +// +// It reports whether the account already existed, because NetUserAdd leaves such +// an account completely untouched, password included. The caller has to reset it +// or the secret it goes on to store would not be the account's password at all. +func ensureWindowsSandboxUser(username string, password string) (bool, error) { name, err := windows.UTF16PtrFromString(username) if err != nil { - return err + return false, err } secret, err := windows.UTF16PtrFromString(password) if err != nil { - return err + return false, err } comment, err := windows.UTF16PtrFromString(windowsSandboxUserComment) if err != nil { - return err + return false, err } info := userInfo1{ Name: name, @@ -236,8 +249,47 @@ func ensureWindowsSandboxUser(username string, password string) error { uintptr(unsafe.Pointer(&info)), 0, ) - defer func() { _ = info }() - return netAPIStatus("NetUserAdd", status, nerrUserExists) + // The struct holds pointers into Go memory that the call dereferences, so + // everything it borrows has to outlive the call. + runtime.KeepAlive(info) + runtime.KeepAlive(name) + runtime.KeepAlive(secret) + runtime.KeepAlive(comment) + if status == nerrUserExists { + return true, nil + } + return false, netAPIStatus("NetUserAdd", status) +} + +// resetWindowsSandboxUserPassword sets the password on an account that already +// existed, so the secret the caller stores is actually the account's password. +// +// Without this, re-running setup produced a fresh random password, wrote it to +// disk, and left the account authenticating with the old one, so every later +// command failed to log on with a principal that looked correctly provisioned. +func resetWindowsSandboxUserPassword(username string, password string) error { + name, err := windows.UTF16PtrFromString(username) + if err != nil { + return err + } + secret, err := windows.UTF16PtrFromString(password) + if err != nil { + return err + } + // USER_INFO_1003 is a password-only update, so nothing else about the + // account is disturbed. + info := userInfo1003{Password: secret} + status, _, _ := procNetUserSetInfo.Call( + 0, // local machine + uintptr(unsafe.Pointer(name)), + 1003, // level: USER_INFO_1003 + uintptr(unsafe.Pointer(&info)), + 0, + ) + runtime.KeepAlive(info) + runtime.KeepAlive(name) + runtime.KeepAlive(secret) + return netAPIStatus("NetUserSetInfo", status) } // addWindowsSandboxUserToGroup puts a principal in the managed group, ignoring @@ -259,7 +311,9 @@ func addWindowsSandboxUserToGroup(username string) error { uintptr(unsafe.Pointer(&entry)), 1, // one member ) - defer func() { _ = entry }() + runtime.KeepAlive(entry) + runtime.KeepAlive(group) + runtime.KeepAlive(member) return netAPIStatus("NetLocalGroupAddMembers", status, errorMemberInAlias) } @@ -282,11 +336,11 @@ func resolveWindowsSandboxSID(username string) (*windows.SID, error) { // the caller needs to mint a token with LogonUser. It is idempotent, so setup // can run repeatedly. // -// The password is returned rather than stored: on an account that already -// existed the returned value is the NEW password only if the caller resets it, -// so callers that need to log in must treat a pre-existing account as requiring -// a reset. That is handled a layer up, where the secret has somewhere safe to -// live; keeping it out of this file means no credential is written to disk here. +// The password is returned rather than stored, so no credential is written to +// disk here; that happens a layer up where the secret has somewhere safe to +// 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) { if err := ensureWindowsSandboxGroup(); err != nil { return windowsSandboxIdentity{}, "", err @@ -296,9 +350,19 @@ func provisionWindowsSandboxIdentity(workspaceKey string) (windowsSandboxIdentit if err != nil { return windowsSandboxIdentity{}, "", err } - if err := ensureWindowsSandboxUser(username, password); err != nil { + existed, err := ensureWindowsSandboxUser(username, password) + if err != nil { return windowsSandboxIdentity{}, "", err } + if existed { + // NetUserAdd left the account 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 + } + } if err := addWindowsSandboxUserToGroup(username); err != nil { return windowsSandboxIdentity{}, "", err } diff --git a/internal/sandbox/windows_identity_windows_test.go b/internal/sandbox/windows_identity_windows_test.go index 754952f66..60c1fef4d 100644 --- a/internal/sandbox/windows_identity_windows_test.go +++ b/internal/sandbox/windows_identity_windows_test.go @@ -220,13 +220,30 @@ func TestProvisionWindowsSandboxIdentityRoundTrip(t *testing.T) { } // Re-running must converge on the same principal rather than failing or // creating a second account. - again, _, err := provisionWindowsSandboxIdentity("ziptest01") + again, secondPassword, err := provisionWindowsSandboxIdentity("ziptest01") if err != nil { t.Fatalf("second provision: %v", err) } if again.Username != identity.Username || !again.SID.Equals(identity.SID) { t.Fatalf("provisioning is not idempotent: %s then %s", identity, again) } + // The password returned for an account that already existed has to BE that + // account's password. NetUserAdd leaves an existing account entirely alone, + // so without an explicit reset this second value is a fresh random string + // that never authenticates, and the caller would store it as the secret and + // leave every later command failing to log on with a principal that looks + // correctly provisioned. Logging on is the only honest way to assert it. + if secondPassword == "" { + t.Fatal("second provision returned an empty password") + } + if err := grantWindowsSandboxLogonRights(again.SID); err != nil { + t.Fatalf("grant logon rights: %v", err) + } + token, err := logonWindowsSandboxPrincipal(again.Username, secondPassword) + if err != nil { + t.Fatalf("logon with the password from the second provision: %v", err) + } + _ = token.Close() // Lookup must find what provisioning created. found, err := lookupWindowsSandboxIdentity("ziptest01") if err != nil { From 12b02218ff68ac28949f962987b4c14845f8dedb Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Mon, 27 Jul 2026 09:29:28 +0530 Subject: [PATCH 10/18] fix(sandbox): revoke logon rights before deleting a principal Retiring a principal deleted the account but left its LSA account rights behind, keyed to a SID that no longer resolves. That is the orphaned residue this model is supposed to avoid, and the reason ACE revocation is keyed to the trustee rather than to a record of what was granted; the logon-rights half was simply missing. CodeRabbit spotted it as a test-cleanup gap, but the production teardown path had the same hole. revokeWindowsSandboxLogonRights drops every right held by the principal and removes its LSA entry, and setup teardown now calls it BEFORE deleting the account, while the SID still resolves. Removing all rights rather than naming them is deliberate: the principal is being retired, so rights granted by an older setup that this one no longer knows about should go too. An account that holds no rights is not an error, since that is the state teardown wants. That tolerance depends on STATUS_OBJECT_NAME_NOT_FOUND surviving LsaNtStatusToWinError as something errors.Is can still match, which is the sort of Windows errno assumption that is often wrong, so there is now an unprivileged test asserting it, including that the tolerance does not also swallow access-denied. Both gated tests now clean up rights and account, in that order. The provisioning round trip had no cleanup at all and, since it started granting a batch logon right, was leaving both behind on whatever machine ran it. --- .../sandbox/windows_identity_logon_windows.go | 58 ++++++++++++++++++- .../windows_identity_logon_windows_test.go | 41 +++++++++++++ .../windows_identity_runtime_windows.go | 12 ++++ .../sandbox/windows_identity_windows_test.go | 25 +++++++- 4 files changed, 134 insertions(+), 2 deletions(-) create mode 100644 internal/sandbox/windows_identity_logon_windows_test.go diff --git a/internal/sandbox/windows_identity_logon_windows.go b/internal/sandbox/windows_identity_logon_windows.go index 6c7acd0df..45ebdaa5f 100644 --- a/internal/sandbox/windows_identity_logon_windows.go +++ b/internal/sandbox/windows_identity_logon_windows.go @@ -18,6 +18,7 @@ package sandbox // needs no special privilege once the batch right is in place. import ( + "errors" "fmt" "runtime" "unsafe" @@ -51,7 +52,10 @@ var ( procLsaOpenPolicy = windows.NewLazySystemDLL("advapi32.dll").NewProc("LsaOpenPolicy") procLsaClose = windows.NewLazySystemDLL("advapi32.dll").NewProc("LsaClose") procLsaAddAccountRights = windows.NewLazySystemDLL("advapi32.dll").NewProc("LsaAddAccountRights") - procLsaNtStatusToWinErr = windows.NewLazySystemDLL("advapi32.dll").NewProc("LsaNtStatusToWinError") + // Retiring a principal has to drop its rights as well as its account, or the + // LSA policy database keeps an entry keyed to a SID that no longer resolves. + procLsaRemoveAccountRights = windows.NewLazySystemDLL("advapi32.dll").NewProc("LsaRemoveAccountRights") + procLsaNtStatusToWinErr = windows.NewLazySystemDLL("advapi32.dll").NewProc("LsaNtStatusToWinError") ) // lsaUnicodeString mirrors LSA_UNICODE_STRING. Length and MaximumLength are @@ -160,6 +164,58 @@ func grantWindowsSandboxLogonRights(sid *windows.SID) error { return nil } +// revokeWindowsSandboxLogonRights drops every account right held by a principal +// and removes its entry from the LSA policy database. +// +// This is the logon-rights counterpart to revoking ACEs by trustee, and it has +// the same reason to exist: deleting the account on its own leaves the rights +// behind, keyed to a SID that no longer resolves, which is the orphaned residue +// this model is supposed to avoid. It must therefore run BEFORE the account is +// deleted, while the SID is still resolvable. +// +// Removing all rights rather than naming them is deliberate. The principal is +// being retired, so anything keyed to it should go, including rights a previous +// version of setup granted and this one no longer knows about. +// +// Requires an elevated caller. A principal that holds no rights is not an error: +// LsaRemoveAccountRights reports ERROR_FILE_NOT_FOUND for an account with no LSA +// entry, which is the state teardown is trying to reach anyway. +func revokeWindowsSandboxLogonRights(sid *windows.SID) error { + if sid == nil { + return fmt.Errorf("revoke sandbox logon rights: nil SID") + } + var attributes lsaObjectAttributes + attributes.Length = uint32(unsafe.Sizeof(attributes)) + var policy windows.Handle + status, _, _ := procLsaOpenPolicy.Call( + 0, // local system + uintptr(unsafe.Pointer(&attributes)), + uintptr(policyCreateAccount|policyLookupNames), + uintptr(unsafe.Pointer(&policy)), + ) + runtime.KeepAlive(attributes) + if err := lsaStatusError("LsaOpenPolicy", status); err != nil { + return err + } + defer procLsaClose.Call(uintptr(policy)) + + status, _, _ = procLsaRemoveAccountRights.Call( + uintptr(policy), + uintptr(unsafe.Pointer(sid)), + 1, // AllRights: drop everything and delete the LSA account object + 0, // UserRights ignored when AllRights is set + 0, // CountOfRights likewise + ) + runtime.KeepAlive(sid) + if err := lsaStatusError("LsaRemoveAccountRights", status); err != nil { + if errors.Is(err, windows.ERROR_FILE_NOT_FOUND) { + return nil + } + return err + } + return nil +} + // logonWindowsSandboxPrincipal mints a primary token for the sandbox account. // The caller owns the returned token and must Close it. // diff --git a/internal/sandbox/windows_identity_logon_windows_test.go b/internal/sandbox/windows_identity_logon_windows_test.go new file mode 100644 index 000000000..503d12922 --- /dev/null +++ b/internal/sandbox/windows_identity_logon_windows_test.go @@ -0,0 +1,41 @@ +//go:build windows + +package sandbox + +import ( + "errors" + "testing" + + "golang.org/x/sys/windows" +) + +// revokeWindowsSandboxLogonRights treats "this account holds no rights" as +// success, because that is the state teardown is trying to reach anyway. That +// relies on the NTSTATUS for it surviving the trip through LsaNtStatusToWinError +// as an error errors.Is can still match, which is exactly the kind of Windows +// errno assumption that quietly turns out to be false. Assert it rather than +// trust it. +// +// Needs no privilege: LsaNtStatusToWinError is a pure status translation, so +// this runs everywhere rather than joining the gated set. +func TestLsaStatusErrorMapsObjectNameNotFound(t *testing.T) { + // STATUS_OBJECT_NAME_NOT_FOUND, what LsaRemoveAccountRights reports for an + // account that has no LSA entry. + const statusObjectNameNotFound = 0xC0000034 + + err := lsaStatusError("LsaRemoveAccountRights", statusObjectNameNotFound) + if err == nil { + t.Fatal("a nonzero NTSTATUS produced no error") + } + if !errors.Is(err, windows.ERROR_FILE_NOT_FOUND) { + t.Fatalf("error = %v, want one errors.Is matches against ERROR_FILE_NOT_FOUND; "+ + "without that, revoking a principal that simply holds no rights fails teardown", err) + } + + // The tolerance must be specific. If any failure matched it, revoke would + // swallow a real one and teardown would report success having done nothing. + const statusAccessDenied = 0xC0000022 + if other := lsaStatusError("LsaRemoveAccountRights", statusAccessDenied); errors.Is(other, windows.ERROR_FILE_NOT_FOUND) { + t.Fatalf("access denied matched the not-found tolerance: %v", other) + } +} diff --git a/internal/sandbox/windows_identity_runtime_windows.go b/internal/sandbox/windows_identity_runtime_windows.go index a3a201799..54bc03dd1 100644 --- a/internal/sandbox/windows_identity_runtime_windows.go +++ b/internal/sandbox/windows_identity_runtime_windows.go @@ -210,5 +210,17 @@ func removeWindowsSandboxPrincipalForSetup(config WindowsSandboxCommandConfig) e if err := removeWindowsSandboxSecret(secretPath); err != nil { return err } + // Drop the LSA account rights before the account itself. Deleting the account + // first would leave its rights behind keyed to a SID that no longer resolves, + // 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 err := revokeWindowsSandboxLogonRights(identity.SID); err != nil { + return err + } + } else if !errors.Is(err, errWindowsSandboxIdentityUnavailable) { + return err + } return removeWindowsSandboxIdentity(username) } diff --git a/internal/sandbox/windows_identity_windows_test.go b/internal/sandbox/windows_identity_windows_test.go index 60c1fef4d..61d2d9d36 100644 --- a/internal/sandbox/windows_identity_windows_test.go +++ b/internal/sandbox/windows_identity_windows_test.go @@ -208,10 +208,28 @@ func TestProvisionWindowsSandboxIdentityRoundTrip(t *testing.T) { if !windowsProcessIsElevated() { t.Skip("provisioning requires an elevated process") } + // A leftover account from an interrupted run is harmless now that + // provisioning resets the password, but starting clean keeps a failure here + // from being explained by residue from a previous one. + _ = removeWindowsSandboxIdentity(windowsSandboxUserName("ziptest01")) + identity, password, err := provisionWindowsSandboxIdentity("ziptest01") if err != nil { t.Fatalf("provision: %v", err) } + // Registered immediately after provisioning so every failure path below is + // covered. This test grants a real batch-logon right to a real local account; + // leaving either behind on a developer machine is not acceptable residue, and + // rights are revoked before the account so nothing is left keyed to a SID that + // no longer resolves. + t.Cleanup(func() { + if err := revokeWindowsSandboxLogonRights(identity.SID); err != nil { + t.Errorf("cleanup: revoke logon rights: %v", err) + } + if err := removeWindowsSandboxIdentity(identity.Username); err != nil { + t.Errorf("cleanup: remove principal: %v", err) + } + }) if identity.SID == nil { t.Fatal("provisioned identity has no SID") } @@ -283,8 +301,13 @@ func TestGrantLogonRightsAndMintPrincipalToken(t *testing.T) { t.Fatalf("provision: %v", err) } t.Cleanup(func() { + // Rights first, then the account: the reverse order strands an LSA entry + // keyed to a SID that no longer resolves. + if err := revokeWindowsSandboxLogonRights(identity.SID); err != nil { + t.Errorf("cleanup: revoke logon rights: %v", err) + } if err := removeWindowsSandboxIdentity(identity.Username); err != nil { - t.Errorf("cleanup: %v", err) + t.Errorf("cleanup: remove principal: %v", err) } }) From 52f843a50702f4c2181371a70f7f0e24a8647fc9 Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Mon, 27 Jul 2026 13:35:30 +0530 Subject: [PATCH 11/18] 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. --- .../windows_identity_runtime_windows.go | 45 ++++++++++- internal/sandbox/windows_identity_windows.go | 76 ++++++++++++++++--- .../sandbox/windows_identity_windows_test.go | 54 ++++++++++++- 3 files changed, 160 insertions(+), 15 deletions(-) diff --git a/internal/sandbox/windows_identity_runtime_windows.go b/internal/sandbox/windows_identity_runtime_windows.go index 54bc03dd1..0876a2050 100644 --- a/internal/sandbox/windows_identity_runtime_windows.go +++ b/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 } diff --git a/internal/sandbox/windows_identity_windows.go b/internal/sandbox/windows_identity_windows.go index 2e1169dbe..a6b663313 100644 --- a/internal/sandbox/windows_identity_windows.go +++ b/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 diff --git a/internal/sandbox/windows_identity_windows_test.go b/internal/sandbox/windows_identity_windows_test.go index 61d2d9d36..f477b518f 100644 --- a/internal/sandbox/windows_identity_windows_test.go +++ b/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()) + } +} From 99fefdc4bb04bb28ed6a54e3c525c50ea1b19690 Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Mon, 27 Jul 2026 14:24:30 +0530 Subject: [PATCH 12/18] fix(sandbox): drop the stored secret whenever provisioning fails The cleanup added for partial provisioning left the window it existed for uncovered. It only removed the on-disk secret when this run had written one, and it derived the secret path after the logon-rights grant, so a failure before that point had nothing to remove. That is exactly the case that matters. Provisioning ALWAYS sets the account's password, including resetting a pre-existing account's, so from the moment it returns the stored secret is already stale. A failure in the rights grant then left that stale secret on disk against a password that had just changed, and the next command failed the logon and reported a broken sandbox instead of falling back. The path is now resolved from the account name before anything can fail, and removal is unconditional rather than gated on having written one. Absent beats stale: the command path treats a missing secret as "not provisioned" and falls back to the restricted token, which is the outcome a failed setup should leave behind. Raised by CodeRabbit, twice from different angles, on the commit that added the cleanup. --- .../windows_identity_runtime_windows.go | 26 +++++++++++-------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/internal/sandbox/windows_identity_runtime_windows.go b/internal/sandbox/windows_identity_runtime_windows.go index 0876a2050..2c18a42cd 100644 --- a/internal/sandbox/windows_identity_runtime_windows.go +++ b/internal/sandbox/windows_identity_runtime_windows.go @@ -144,14 +144,20 @@ func provisionWindowsSandboxPrincipalForSetup(config WindowsSandboxCommandConfig // deleting it because a later run failed would turn a partial failure into a // total one. rightsGranted := false - secretWritten := false - secretPath := "" + // Resolved from the account name rather than the identity, so it is known + // before anything can fail. Deriving it later, after the rights grant, left + // the one window this cleanup exists for uncovered: provisioning ALWAYS sets + // the password, including resetting a pre-existing account's, so from the + // moment it returns the stored secret is already stale. A failure before the + // path was computed then had nothing to remove. + secretPath, secretPathErr := windowsSandboxSecretPath(config.SandboxHome, windowsSandboxUserName(key)) 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. + // Unconditionally, not only when this run wrote one. Provisioning has + // already replaced the account's password by the time any of this can + // fail, so whatever is on disk cannot authenticate. Absent beats stale: + // the command path treats a missing secret as "not provisioned" and falls + // back, while a stale one fails the logon and reports a broken sandbox. + if secretPath != "" { _ = removeWindowsSandboxSecret(secretPath) } if identity.SID != nil && rightsGranted { @@ -173,10 +179,9 @@ func provisionWindowsSandboxPrincipalForSetup(config WindowsSandboxCommandConfig return windowsSandboxIdentity{}, err } rightsGranted = true - secretPath, err = windowsSandboxSecretPath(config.SandboxHome, identity.Username) - if err != nil { + if secretPathErr != nil { undo() - return windowsSandboxIdentity{}, err + return windowsSandboxIdentity{}, secretPathErr } // The secret is rewritten every run so it stays in step with the account. // provisionWindowsSandboxIdentity guarantees the password it returns is the @@ -187,7 +192,6 @@ func provisionWindowsSandboxPrincipalForSetup(config WindowsSandboxCommandConfig undo() return windowsSandboxIdentity{}, err } - secretWritten = true return identity, nil } From dbdbda11dc252d44a18b62fe7005446d4f1965ab Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Tue, 28 Jul 2026 12:24:24 +0530 Subject: [PATCH 13/18] test(sandbox): check the ACE type before decoding its trustee GetAce returns a generic ACE_HEADER and the helper reinterprets it as an ACCESS_ALLOWED_ACE. That holds for the fixed-layout types, but an object ACE carries Flags and two GUIDs ahead of the trustee, so SidStart would land mid-structure and Copy would read whatever bytes follow. The caller asserts that no unexpected trustee appears in the DACL, and on such an ACE it would print a nonsense SID rather than name the entry that does not belong. Nothing under test builds anything but allowed ACEs today, so this changes no current outcome. It keeps the failure legible if that ever changes. --- .../sandbox/windows_identity_secret_windows_test.go | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/internal/sandbox/windows_identity_secret_windows_test.go b/internal/sandbox/windows_identity_secret_windows_test.go index b77d0f5a0..5fce6d461 100644 --- a/internal/sandbox/windows_identity_secret_windows_test.go +++ b/internal/sandbox/windows_identity_secret_windows_test.go @@ -5,6 +5,7 @@ package sandbox import ( "bytes" "errors" + "fmt" "os" "path/filepath" "strings" @@ -187,6 +188,18 @@ func windowsSecretACEList(dacl *windows.ACL) ([]*windows.SID, error) { if err := windows.GetAce(dacl, index, &ace); err != nil { return nil, err } + // GetAce hands back a generic ACE_HEADER and we reinterpret it. That is + // only sound for the fixed-layout types: an object ACE carries Flags and + // two GUIDs ahead of the trustee, so SidStart would land mid-structure + // and Copy would read whatever bytes happen to be there. The caller's + // "unexpected trustee" assertion would then print a nonsense SID instead + // of naming the ACE that does not belong, which is the opposite of what + // a failing test should do. Nothing under test builds anything but + // allowed ACEs today, so this exists to keep the failure legible if that + // ever changes. + if ace.Header.AceType != windows.ACCESS_ALLOWED_ACE_TYPE { + return nil, fmt.Errorf("ACE %d has type %d, want ACCESS_ALLOWED_ACE_TYPE (%d); refusing to decode its trustee", index, ace.Header.AceType, windows.ACCESS_ALLOWED_ACE_TYPE) + } sid := (*windows.SID)(unsafe.Pointer(&ace.SidStart)) copied, err := sid.Copy() if err != nil { From 3d49e20fc2d39a726312ab10bc1ae962e04d35c5 Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Tue, 28 Jul 2026 21:31:50 +0530 Subject: [PATCH 14/18] fix(sandbox): grant delete to the principal and keep rollback able to find it Two findings from review, both consequences of the principal being a separate account rather than the calling user. WindowsACLAllowWrite granted FILE_GENERIC_WRITE, which covers creating and modifying but not removing or renaming, and a rename needs delete on the source. Under the old same-user token this was invisible because the caller already held inherited rights on its own tree. A principal inherits nothing, so it could write files it could never delete, which fails ordinary editing and most git operations rather than an edge case. DELETE and FILE_DELETE_CHILD are now part of the grant, matching WindowsACLDenyWrite, which already treats delete as part of write. WRITE_DAC and WRITE_OWNER stay out: they are denied so the principal cannot rewrite its own restrictions. provisionWindowsSandboxIdentity returned a zero identity alongside created=true when group attachment or SID resolution failed after NetUserAdd had already created the account. The caller's rollback deletes by identity.Username, so it was asked to delete the empty string and left the account behind. Group attachment is the case that matters, being both the enforcement boundary and something local policy can refuse. The name now comes back with the error. The four provisioning calls are indirected so the failure paths are reachable in a test. Seaming only the post-creation pair would not have been enough: every step needs an elevated caller, so the test would have stopped at the group check and passed without reaching what it names. Also seeds the empty-secret test with a genuinely empty file. The previous whitespace seed was several bytes, so it never reached the length check and failed later in DPAPI instead, which another test already covers. --- internal/sandbox/windows_acl_apply_windows.go | 17 +- .../windows_identity_rollback_windows_test.go | 155 ++++++++++++++++++ .../windows_identity_secret_windows_test.go | 27 ++- internal/sandbox/windows_identity_windows.go | 38 ++++- 4 files changed, 223 insertions(+), 14 deletions(-) create mode 100644 internal/sandbox/windows_identity_rollback_windows_test.go diff --git a/internal/sandbox/windows_acl_apply_windows.go b/internal/sandbox/windows_acl_apply_windows.go index a38789f4a..d53927a08 100644 --- a/internal/sandbox/windows_acl_apply_windows.go +++ b/internal/sandbox/windows_acl_apply_windows.go @@ -223,7 +223,22 @@ func windowsExplicitAccessEntries(entries []WindowsACLEntry, isDir bool) ([]wind func windowsACLAccess(action WindowsACLAction) (windows.ACCESS_MODE, windows.ACCESS_MASK, error) { switch action { case WindowsACLAllowWrite: - return windows.GRANT_ACCESS, windows.FILE_GENERIC_READ | windows.FILE_GENERIC_WRITE | windows.FILE_GENERIC_EXECUTE, nil + // DELETE and FILE_DELETE_CHILD are part of the grant, not extras. + // FILE_GENERIC_WRITE covers creating and modifying but not removing or + // renaming, and a rename needs delete access on the source. Under the + // old same-user token that gap was invisible, because the caller already + // held inherited rights on its own tree; a sandbox principal is a + // separate account with no such inheritance, so without these it can + // write a file it can never delete. Ordinary editing and most git + // operations rewrite files by replacing them, so the omission fails + // normal work rather than an edge case. + // + // WindowsACLDenyWrite below already treats delete as part of write. This + // keeps the grant symmetric with the deny instead of covering less. + // WRITE_DAC and WRITE_OWNER stay out on purpose: they are in the deny + // mask to stop the principal rewriting its own restrictions, and + // granting them here would hand back exactly that. + return windows.GRANT_ACCESS, windows.FILE_GENERIC_READ | windows.FILE_GENERIC_WRITE | windows.FILE_GENERIC_EXECUTE | windows.DELETE | windowsFileDeleteChild, nil case WindowsACLAllowRead: // Read and traverse without write. A sandbox principal is a separate // account with no inherent access to the caller's tree, so a read-only diff --git a/internal/sandbox/windows_identity_rollback_windows_test.go b/internal/sandbox/windows_identity_rollback_windows_test.go new file mode 100644 index 000000000..42128073c --- /dev/null +++ b/internal/sandbox/windows_identity_rollback_windows_test.go @@ -0,0 +1,155 @@ +//go:build windows + +package sandbox + +import ( + "errors" + "testing" + + "golang.org/x/sys/windows" +) + +// stubWindowsProvisioning replaces the four provisioning syscalls so the +// 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) { + t.Helper() + prevGroup, prevUser := ensureWindowsSandboxGroupFn, ensureWindowsSandboxUserFn + prevAdd, prevSID := addWindowsSandboxUserToGroupFn, resolveWindowsSandboxSIDFn + t.Cleanup(func() { + ensureWindowsSandboxGroupFn, ensureWindowsSandboxUserFn = prevGroup, prevUser + addWindowsSandboxUserToGroupFn, resolveWindowsSandboxSIDFn = prevAdd, prevSID + }) + + ensureWindowsSandboxGroupFn = func() error { return nil } + ensureWindowsSandboxUserFn = func(string, string) (bool, error) { return existed, nil } + addWindowsSandboxUserToGroupFn = func(string) error { return groupErr } + resolveWindowsSandboxSIDFn = func(username string) (*windows.SID, error) { + if sidErr != nil { + return nil, sidErr + } + return windows.CreateWellKnownSid(windows.WinLocalSystemSid) + } +} + +// A failure after the account has been created must still hand back the name. +// +// The caller's rollback deletes by identity.Username, so returning a zero +// identity alongside created=true asked it to delete "" and quietly left the +// account this run had just made. Group attachment is the case that matters +// 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") + sidFailure := errors.New("sid lookup failed") + + for name, testCase := range map[string]struct { + groupErr error + sidErr error + }{ + "group attachment fails": {groupErr: groupFailure}, + "sid resolution fails": {sidErr: sidFailure}, + } { + t.Run(name, func(t *testing.T) { + stubWindowsProvisioning(t, false, testCase.groupErr, testCase.sidErr) + + identity, _, created, err := provisionWindowsSandboxIdentity("workspacekey") + if err == nil { + t.Fatal("provisioning reported success despite an injected failure") + } + if !created { + t.Fatal("created = false, so the rollback would skip an account this run made") + } + // The whole point: without a name there is nothing to delete. + if identity.Username == "" { + t.Fatal("identity carries no username, so the rollback deletes \"\" and strands the account") + } + if want := windowsSandboxUserName("workspacekey"); identity.Username != want { + t.Fatalf("username = %q, want %q", identity.Username, want) + } + }) + } +} + +// An account that already existed must not be deleted because a later step +// 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) + + _, _, created, err := provisionWindowsSandboxIdentity("workspacekey") + if err == nil { + t.Fatal("provisioning reported success despite an injected failure") + } + // created is the whole assertion. It is what stops the rollback deleting an + // account it did not make, and it must stay false however provisioning + // fails. The identity is deliberately not checked: an adopted account exits + // early at the ownership check, which is a real syscall and not stubbed + // here, so asserting on the name would be testing the stub rather than the + // contract. + if created { + t.Fatal("created = true for an account this run did not create; rollback would delete a working principal") + } +} + +// The write grant has to include delete. +// +// FILE_GENERIC_WRITE covers creating and modifying but not removing or +// renaming, and a rename needs delete on the source. The old same-user token hid +// this because the caller already held inherited rights on its own tree; a +// principal is a separate account with none, so without these it can write files +// it can never remove, which fails ordinary editing and most git operations +// rather than an edge case. +func TestWindowsACLAllowWriteGrantsDelete(t *testing.T) { + mode, mask, err := windowsACLAccess(WindowsACLAllowWrite) + if err != nil { + t.Fatalf("windowsACLAccess: %v", err) + } + if mode != windows.GRANT_ACCESS { + t.Fatalf("mode = %v, want GRANT_ACCESS", mode) + } + // Atomic bits only. FILE_GENERIC_READ and FILE_GENERIC_WRITE both carry + // READ_CONTROL and SYNCHRONIZE, so testing a composite constant with & is + // satisfied by any grant at all and proves nothing. + for label, bit := range map[string]windows.ACCESS_MASK{ + "DELETE": windows.DELETE, + "FILE_DELETE_CHILD": windowsFileDeleteChild, + "FILE_WRITE_DATA": windows.FILE_WRITE_DATA, + } { + if mask&bit == 0 { + t.Errorf("write grant is missing %s", label) + } + } + // Granting these would let the principal rewrite the very restrictions + // placed on it. They are in the deny mask for that reason and must not + // appear here. + for label, bit := range map[string]windows.ACCESS_MASK{ + "WRITE_DAC": windows.WRITE_DAC, + "WRITE_OWNER": windows.WRITE_OWNER, + } { + if mask&bit != 0 { + t.Errorf("write grant unexpectedly includes %s", label) + } + } +} + +// The read grant must stay read-only. Widening the write mask above is only +// safe if this one did not move with it. +func TestWindowsACLAllowReadGrantsNoDelete(t *testing.T) { + _, mask, err := windowsACLAccess(WindowsACLAllowRead) + if err != nil { + t.Fatalf("windowsACLAccess: %v", err) + } + // Atomic bits, for the same reason as above: the read and write composites + // overlap on the standard rights, so a composite check here would report a + // failure that is not real. + for label, bit := range map[string]windows.ACCESS_MASK{ + "DELETE": windows.DELETE, + "FILE_DELETE_CHILD": windowsFileDeleteChild, + "FILE_WRITE_DATA": windows.FILE_WRITE_DATA, + } { + if mask&bit != 0 { + t.Errorf("read grant unexpectedly includes %s", label) + } + } +} diff --git a/internal/sandbox/windows_identity_secret_windows_test.go b/internal/sandbox/windows_identity_secret_windows_test.go index 5fce6d461..0293ec573 100644 --- a/internal/sandbox/windows_identity_secret_windows_test.go +++ b/internal/sandbox/windows_identity_secret_windows_test.go @@ -130,13 +130,28 @@ func TestWindowsSandboxSecretMissingIsSentinel(t *testing.T) { } // An empty file is a half-written secret, not a valid empty password. +// +// Both seeds matter and only one of them tests what the name says. A +// zero-length file is the truncated-write case, and it is the only one that +// reaches the length check. Whitespace is several bytes, so it travels on to +// DPAPI and fails to unprotect instead, which is the path +// TestWindowsSandboxSecretRejectsLegacyPlaintext already covers. Seeding only +// the whitespace, as this did, left the branch in the test's own name +// unexercised. func TestWindowsSandboxSecretEmptyIsSentinel(t *testing.T) { - path := filepath.Join(t.TempDir(), "empty.secret") - if err := os.WriteFile(path, []byte(" \r\n"), 0o600); err != nil { - t.Fatalf("seed: %v", err) - } - if _, err := readWindowsSandboxSecret(path); err != errWindowsSandboxIdentityUnavailable { - t.Fatalf("empty secret returned %v, want the unavailable sentinel", err) + for name, seed := range map[string][]byte{ + "truncated write": {}, + "whitespace only": []byte(" \r\n"), + } { + t.Run(name, func(t *testing.T) { + path := filepath.Join(t.TempDir(), "empty.secret") + if err := os.WriteFile(path, seed, 0o600); err != nil { + t.Fatalf("seed: %v", err) + } + if _, err := readWindowsSandboxSecret(path); err != errWindowsSandboxIdentityUnavailable { + t.Fatalf("secret of %d bytes returned %v, want the unavailable sentinel", len(seed), err) + } + }) } } diff --git a/internal/sandbox/windows_identity_windows.go b/internal/sandbox/windows_identity_windows.go index a6b663313..e2d7b46eb 100644 --- a/internal/sandbox/windows_identity_windows.go +++ b/internal/sandbox/windows_identity_windows.go @@ -376,6 +376,21 @@ func resolveWindowsSandboxSID(username string) (*windows.SID, error) { return sid, nil } +// Indirected so a test can drive provisioning end to end and inject a failure +// at the two points that occur AFTER the account exists. Those are the paths +// whose return value the caller's rollback depends on. +// +// All four are seamed rather than just the last two: every step here needs an +// elevated caller and a real local account, so a test that only replaced the +// 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 +) + // provisionWindowsSandboxIdentity ensures the managed group and one sandbox // principal for workspaceKey exist, and returns the identity plus the password // the caller needs to mint a token with LogonUser. It is idempotent, so setup @@ -387,7 +402,7 @@ func resolveWindowsSandboxSID(username string) (*windows.SID, error) { // when the account already existed, because that case is reset explicitly // below. func provisionWindowsSandboxIdentity(workspaceKey string) (windowsSandboxIdentity, string, bool, error) { - if err := ensureWindowsSandboxGroup(); err != nil { + if err := ensureWindowsSandboxGroupFn(); err != nil { return windowsSandboxIdentity{}, "", false, err } username := windowsSandboxUserName(workspaceKey) @@ -395,7 +410,7 @@ func provisionWindowsSandboxIdentity(workspaceKey string) (windowsSandboxIdentit if err != nil { return windowsSandboxIdentity{}, "", false, err } - existed, err := ensureWindowsSandboxUser(username, password) + existed, err := ensureWindowsSandboxUserFn(username, password) if err != nil { return windowsSandboxIdentity{}, "", false, err } @@ -421,12 +436,21 @@ func provisionWindowsSandboxIdentity(workspaceKey string) (windowsSandboxIdentit return windowsSandboxIdentity{}, "", false, err } } - if err := addWindowsSandboxUserToGroup(username); err != nil { - return windowsSandboxIdentity{}, "", !existed, err - } - sid, err := resolveWindowsSandboxSID(username) + // Both failures below can happen AFTER NetUserAdd created the account, so the + // name has to come back with them. The caller's rollback deletes by + // identity.Username, and returning a zero identity alongside created=true + // asked it to delete "", which silently stranded the account this run had + // just made. Group attachment in particular is not a formality: it can fail + // under local policy, and it is the enforcement boundary, so a half-created + // principal is exactly the state worth not leaving behind. The SID is absent + // here, which the rollback already tolerates, since nothing has been granted + // to it yet. + if err := addWindowsSandboxUserToGroupFn(username); err != nil { + return windowsSandboxIdentity{Username: username}, "", !existed, err + } + sid, err := resolveWindowsSandboxSIDFn(username) if err != nil { - return windowsSandboxIdentity{}, "", !existed, err + return windowsSandboxIdentity{Username: username}, "", !existed, err } return windowsSandboxIdentity{Username: username, SID: sid}, password, !existed, nil } From d67267f8c02bf680da2b3e88dd84deb37800ea5b Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Wed, 29 Jul 2026 13:50:34 +0530 Subject: [PATCH 15/18] fix(sandbox): stop setup destroying a principal it did not create Six findings from review, all on the elevated setup path. Teardown was not scoped to what the run created. provisionWindowsSandbox PrincipalForSetup was careful never to delete an account it had adopted, and then setupWindowsSandboxPrincipal called removePrincipal on any ACL failure with no such guard. Re-running elevated setup on a working machine and hitting one transient ACL error therefore deleted the local account, its secret and its logon rights. It now returns whether it created the principal and the outer teardown honours it; ACEs are still reverted, since this run applied them. Password rotation moved to immediately before the secret is committed. Resetting an adopted account's password at the top of provisioning meant every later step ran against an account whose password had been replaced with no copy stored. Any failure there left a live account authenticated by a password nothing on disk knew, and since the account pre-existed the rollback correctly declined to delete it, so the command path read the absent secret as "not provisioned" and fell back to the weaker backend for good. The two operations are now adjacent. The rollback also stops removing the secret when this run neither created the account nor rotated it, because that secret still works. Policy DenyWrite now reaches the principal ACL plan. The capability plan has always emitted these; the principal plan denied write only on protected metadata and read-only subpaths, so once the runner used a principal token a policy deny elsewhere was not enforced at all. Principal deny-read entries are materialized, matching the capability plan, so a path created after setup still gets a deny ACE. Logon-right revocation is keyed to the attempt rather than to success. Rights are added one at a time and the grant returns on first failure, so a partial grant left LSA entries behind pointing at a SID that deleting the account then made unresolvable. The ownership comment now carries the full workspace key. The account name holds only 11 characters of the digest, so two workspaces could derive one name and silently share an account, a secret and an ACL identity; a mismatch is now refused. Accounts provisioned before the key was recorded are still adopted. Also warns once on stderr when the opt-in is set and a provisioned principal cannot be used, rather than downgrading in silence. --- internal/sandbox/windows_identity_acl.go | 26 ++- .../windows_identity_policy_windows_test.go | 149 ++++++++++++++++++ .../windows_identity_rollback_windows_test.go | 7 +- .../windows_identity_runtime_windows.go | 118 ++++++++++---- internal/sandbox/windows_identity_windows.go | 71 ++++++--- .../sandbox/windows_identity_windows_test.go | 4 +- 6 files changed, 322 insertions(+), 53 deletions(-) create mode 100644 internal/sandbox/windows_identity_policy_windows_test.go diff --git a/internal/sandbox/windows_identity_acl.go b/internal/sandbox/windows_identity_acl.go index c37393b71..9dced2fbb 100644 --- a/internal/sandbox/windows_identity_acl.go +++ b/internal/sandbox/windows_identity_acl.go @@ -41,6 +41,13 @@ type windowsPrincipalACLInput struct { // DenyRead covers objects a principal could otherwise reach because they are // world-readable; per-user secrets need no entry. DenyRead []string + // DenyWrite carries the policy's own deny-write paths. The capability plan + // has always emitted these; the principal plan denied write only on + // protected metadata and read-only subpaths inside write roots, so a policy + // deny sitting anywhere else was simply not enforced once the runner used a + // principal token, and a shell child could write where the restricted-token + // backend would have blocked it. + DenyWrite []string } // buildWindowsPrincipalACLPlan turns a principal's access into ACL entries. @@ -60,11 +67,24 @@ func buildWindowsPrincipalACLPlan(input windowsPrincipalACLInput) (WindowsACLPla // Deny first. A deny ACE inside a write root (protected metadata, git // internals) has to win over the grant that follows it. + // Materialized, matching the capability plan. applyWindowsACLPlan skips a + // target that does not exist, so without this a deny-read path created after + // setup ran never got an ACE at all and the principal could read it. The + // deny has to be in place before the object is. for _, path := range normalizeProfilePaths(input.DenyRead) { entries = append(entries, WindowsACLEntry{ - Action: WindowsACLDenyRead, - Path: path, - Capability: input.PrincipalSID, + Action: WindowsACLDenyRead, + Path: path, + Capability: input.PrincipalSID, + Materialize: true, + }) + } + for _, path := range normalizeProfilePaths(input.DenyWrite) { + entries = append(entries, WindowsACLEntry{ + Action: WindowsACLDenyWrite, + Path: path, + Capability: input.PrincipalSID, + Materialize: true, }) } for _, root := range input.WriteRoots { diff --git a/internal/sandbox/windows_identity_policy_windows_test.go b/internal/sandbox/windows_identity_policy_windows_test.go new file mode 100644 index 000000000..a5bd7b90e --- /dev/null +++ b/internal/sandbox/windows_identity_policy_windows_test.go @@ -0,0 +1,149 @@ +//go:build windows + +package sandbox + +import ( + "path/filepath" + "strings" + "testing" +) + +// Policy deny-write has to reach the principal plan. +// +// The capability plan has always emitted these. The principal plan denied write +// only on protected metadata and read-only subpaths inside write roots, so once +// the runner used a principal token a policy deny sitting anywhere else was not +// enforced at the OS layer at all, and a shell child could write where the +// restricted-token backend would have stopped it. +func TestPrincipalACLPlanCarriesPolicyDenyWrite(t *testing.T) { + root := filepath.Join(t.TempDir(), "workspace") + denied := filepath.Join(root, "protected", "keep-out") + + plan, err := buildWindowsPrincipalACLPlan(windowsPrincipalACLInput{ + PrincipalSID: "S-1-5-21-0-0-0-1001", + WriteRoots: []WritableRoot{{Root: root}}, + DenyWrite: []string{denied}, + }) + if err != nil { + t.Fatalf("buildWindowsPrincipalACLPlan: %v", err) + } + + entry, ok := findPrincipalACLEntry(plan, WindowsACLDenyWrite, denied) + if !ok { + t.Fatalf("no deny-write ACE for the policy path; plan = %+v", plan.Entries) + } + // Materialized for the same reason the capability plan does it: the applier + // skips targets that do not exist, so a deny on a path created after setup + // would never be written. + if !entry.Materialize { + t.Error("policy deny-write ACE is not materialized, so it is skipped when the path does not exist yet") + } +} + +// Deny-read has to be materialized too, which it was not. +func TestPrincipalACLPlanMaterializesDenyRead(t *testing.T) { + root := filepath.Join(t.TempDir(), "workspace") + secret := filepath.Join(t.TempDir(), "elsewhere", "creds") + + plan, err := buildWindowsPrincipalACLPlan(windowsPrincipalACLInput{ + PrincipalSID: "S-1-5-21-0-0-0-1001", + WriteRoots: []WritableRoot{{Root: root}}, + DenyRead: []string{secret}, + }) + if err != nil { + t.Fatalf("buildWindowsPrincipalACLPlan: %v", err) + } + entry, ok := findPrincipalACLEntry(plan, WindowsACLDenyRead, secret) + if !ok { + t.Fatalf("no deny-read ACE emitted; plan = %+v", plan.Entries) + } + if !entry.Materialize { + t.Fatal("deny-read ACE is not materialized, so a path created after setup never gets one") + } +} + +// Deny entries must still precede the grants they carve out of, which is what +// makes them win under Windows DACL evaluation. Adding deny-write to the plan is +// only safe if it did not disturb that ordering. +func TestPrincipalACLPlanKeepsDeniesBeforeGrants(t *testing.T) { + root := filepath.Join(t.TempDir(), "workspace") + plan, err := buildWindowsPrincipalACLPlan(windowsPrincipalACLInput{ + PrincipalSID: "S-1-5-21-0-0-0-1001", + WriteRoots: []WritableRoot{{Root: root}}, + DenyWrite: []string{filepath.Join(root, "nope")}, + DenyRead: []string{filepath.Join(root, "secret")}, + }) + if err != nil { + t.Fatalf("buildWindowsPrincipalACLPlan: %v", err) + } + firstGrant := -1 + for i, entry := range plan.Entries { + switch entry.Action { + case WindowsACLAllowWrite, WindowsACLAllowRead: + if firstGrant == -1 { + firstGrant = i + } + case WindowsACLDenyRead, WindowsACLDenyWrite: + if firstGrant != -1 { + t.Fatalf("deny entry at %d follows a grant at %d; the grant would win", i, firstGrant) + } + } + } +} + +func findPrincipalACLEntry(plan WindowsACLPlan, action WindowsACLAction, path string) (WindowsACLEntry, bool) { + want := normalizeProfilePath(path) + for _, entry := range plan.Entries { + if entry.Action == action && entry.Path == want { + return entry, true + } + } + return WindowsACLEntry{}, false +} + +// An adopted account must not have its password rotated during provisioning. +// +// Rotating there left every later step running against an account whose password +// had already been replaced with nothing on disk holding it. Any failure in +// between stranded a working principal: the rollback correctly declined to +// delete an account it had not created, so what remained was a live account +// authenticated by a password no longer stored anywhere, and the command path +// 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) + + rotated := false + previous := resetWindowsSandboxUserPasswordFn + t.Cleanup(func() { resetWindowsSandboxUserPasswordFn = previous }) + resetWindowsSandboxUserPasswordFn = func(string, string) error { + rotated = true + return nil + } + + if _, _, _, err := provisionWindowsSandboxIdentity("workspacekey"); err != nil { + t.Fatalf("provisioning an adopted account: %v", err) + } + if rotated { + t.Fatal("provisioning rotated the password; the window this closes lasts until the secret is committed") + } +} + +// The ownership comment carries the full workspace key, so two workspaces whose +// digests collide in the 11 characters the account name can hold are refused +// rather than silently sharing one account, one secret and one ACL identity. +func TestWindowsSandboxUserCommentDistinguishesWorkspaces(t *testing.T) { + first := windowsSandboxUserCommentFor("aaaaaaaaaaaabbbbbbbb") + second := windowsSandboxUserCommentFor("aaaaaaaaaaaacccccccc") + if first == second { + t.Fatal("two workspaces produced the same ownership comment, so a name collision would be adopted") + } + if !strings.HasPrefix(first, windowsSandboxUserComment) { + t.Fatalf("comment %q lost the marker prefix that identifies it as ours", first) + } + // 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") { + t.Skip("account names no longer collide for these keys; revisit what this test is for") + } +} diff --git a/internal/sandbox/windows_identity_rollback_windows_test.go b/internal/sandbox/windows_identity_rollback_windows_test.go index 42128073c..e4713d8e4 100644 --- a/internal/sandbox/windows_identity_rollback_windows_test.go +++ b/internal/sandbox/windows_identity_rollback_windows_test.go @@ -17,13 +17,18 @@ func stubWindowsProvisioning(t *testing.T, existed bool, groupErr error, sidErr t.Helper() prevGroup, prevUser := ensureWindowsSandboxGroupFn, ensureWindowsSandboxUserFn prevAdd, prevSID := addWindowsSandboxUserToGroupFn, resolveWindowsSandboxSIDFn + prevManaged := windowsSandboxUserIsManagedFn t.Cleanup(func() { ensureWindowsSandboxGroupFn, ensureWindowsSandboxUserFn = prevGroup, prevUser addWindowsSandboxUserToGroupFn, resolveWindowsSandboxSIDFn = prevAdd, prevSID + windowsSandboxUserIsManagedFn = prevManaged }) ensureWindowsSandboxGroupFn = func() error { return nil } - ensureWindowsSandboxUserFn = func(string, string) (bool, error) { return existed, nil } + // Adopted accounts are ours in these tests; the ownership check is a real + // syscall and would otherwise refuse before the code under test runs. + windowsSandboxUserIsManagedFn = func(string, string) (bool, error) { return true, nil } + ensureWindowsSandboxUserFn = func(string, string, string) (bool, error) { return existed, nil } addWindowsSandboxUserToGroupFn = func(string) error { return groupErr } resolveWindowsSandboxSIDFn = func(username string) (*windows.SID, error) { if sidErr != nil { diff --git a/internal/sandbox/windows_identity_runtime_windows.go b/internal/sandbox/windows_identity_runtime_windows.go index 2c18a42cd..7c0389800 100644 --- a/internal/sandbox/windows_identity_runtime_windows.go +++ b/internal/sandbox/windows_identity_runtime_windows.go @@ -16,8 +16,10 @@ import ( "crypto/sha256" "encoding/hex" "errors" + "fmt" "os" "strings" + "sync" "golang.org/x/sys/windows" ) @@ -109,6 +111,14 @@ func windowsSandboxPrincipalToken(config WindowsSandboxCommandConfig) (windows.T if errors.Is(err, errWindowsSandboxIdentityUnavailable) { // The account exists but its password does not. Setup was interrupted // or the secret was removed; fall back rather than fail the command. + // + // Falling back is right, staying quiet about it was not. The opt-in is + // set and an account IS provisioned, so the operator asked for + // principal isolation and is silently getting the weaker same-user + // restricted token instead. That is the one fail-soft case worth + // announcing: the others mean the backend was never set up, while this + // one means it was and has broken since. + warnWindowsSandboxPrincipalUnavailable(identity.Username) return 0, false, nil } return 0, false, err @@ -122,6 +132,25 @@ func windowsSandboxPrincipalToken(config WindowsSandboxCommandConfig) (windows.T return token, true, nil } +// warnWindowsSandboxPrincipalUnavailable tells the operator once per process +// that the backend they opted into is not the one running. +// +// Once, because this sits on the command path: a warning per command would be +// noise on every tool call for the whole session, and noise that repeats gets +// filtered out by the reader rather than acted on. Indirected through a var so a +// test can observe it without capturing stderr. +var warnWindowsSandboxPrincipalUnavailable = func(username string) { + windowsSandboxPrincipalWarnOnce.Do(func() { + fmt.Fprintf(os.Stderr, + "[zero] %s is set and sandbox principal %q is provisioned, but its stored password is missing or unreadable. "+ + "Falling back to the restricted-token sandbox, which does not confine reads. "+ + "Re-run `zero sandbox setup` from an elevated terminal to restore it.\n", + windowsSandboxIdentityEnv, username) + }) +} + +var windowsSandboxPrincipalWarnOnce sync.Once + // 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`. @@ -129,7 +158,7 @@ func windowsSandboxPrincipalToken(config WindowsSandboxCommandConfig) (windows.T // 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, error) { +func provisionWindowsSandboxPrincipalForSetup(config WindowsSandboxCommandConfig) (windowsSandboxIdentity, bool, error) { key := windowsSandboxWorkspaceKey(config.WorkspaceRoots) identity, password, created, err := provisionWindowsSandboxIdentity(key) @@ -143,24 +172,32 @@ func provisionWindowsSandboxPrincipalForSetup(config WindowsSandboxCommandConfig // 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 + rightsAttempted := false + rotated := false // Resolved from the account name rather than the identity, so it is known - // before anything can fail. Deriving it later, after the rights grant, left - // the one window this cleanup exists for uncovered: provisioning ALWAYS sets - // the password, including resetting a pre-existing account's, so from the - // moment it returns the stored secret is already stale. A failure before the - // path was computed then had nothing to remove. + // before anything can fail. secretPath, secretPathErr := windowsSandboxSecretPath(config.SandboxHome, windowsSandboxUserName(key)) undo := func() { - // Unconditionally, not only when this run wrote one. Provisioning has - // already replaced the account's password by the time any of this can - // fail, so whatever is on disk cannot authenticate. Absent beats stale: - // the command path treats a missing secret as "not provisioned" and falls - // back, while a stale one fails the logon and reports a broken sandbox. - if secretPath != "" { + // 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, + // because in both cases what is on disk cannot authenticate and absent + // beats stale: the command path treats a missing secret as "not + // provisioned" and falls back, while a stale one fails the logon and + // reports a broken sandbox. + // + // Removing it unconditionally, as this used to, destroyed a WORKING + // secret whenever setup failed before rotation on a machine that was + // already provisioned. The account kept its old password, the only copy + // of it was deleted, and the sandbox silently degraded. + if secretPath != "" && (created || rotated) { _ = removeWindowsSandboxSecret(secretPath) } - if identity.SID != nil && rightsGranted { + // Attempted rather than completed. grantWindowsSandboxLogonRights adds + // rights one at a time and returns on the first failure, so a partial + // grant is possible; gating revocation on success left those entries + // behind, keyed to a SID that deleting the account then made + // unresolvable. Revoking a right that was never granted is harmless. + if identity.SID != nil && rightsAttempted { _ = revokeWindowsSandboxLogonRights(identity.SID) } if created { @@ -172,27 +209,39 @@ func provisionWindowsSandboxPrincipalForSetup(config WindowsSandboxCommandConfig // provisionWindowsSandboxIdentity can fail after creating the account, so // this path needs the same cleanup even though nothing below ran. undo() - return windowsSandboxIdentity{}, err + return windowsSandboxIdentity{}, false, err } + rightsAttempted = true if err := grantWindowsSandboxLogonRights(identity.SID); err != nil { undo() - return windowsSandboxIdentity{}, err + return windowsSandboxIdentity{}, false, err } - rightsGranted = true if secretPathErr != nil { undo() - return windowsSandboxIdentity{}, secretPathErr + return windowsSandboxIdentity{}, false, secretPathErr + } + // Rotation happens HERE, immediately before the secret is committed, rather + // than inside provisioning where it used to. + // + // A new account already has this password from NetUserAdd, so only an + // adopted one needs setting. Doing it at the top of provisioning meant every + // step in between ran with the account's password already replaced and no + // copy of it stored, so any failure there stranded a working principal. The + // two operations are now adjacent, which is the smallest window this can + // have without a way to restore the previous password, which Windows does + // not offer. + if !created { + if err := resetWindowsSandboxUserPasswordFn(identity.Username, password); err != nil { + undo() + return windowsSandboxIdentity{}, false, err + } + rotated = true } - // 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 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 + return windowsSandboxIdentity{}, false, err } - return identity, nil + return identity, created, nil } // setupWindowsSandboxPrincipal provisions this workspace's principal and grants @@ -205,11 +254,25 @@ func provisionWindowsSandboxPrincipalForSetup(config WindowsSandboxCommandConfig // naming a SID that no longer resolves, which is the orphaned-entry residue this // model exists to avoid. func setupWindowsSandboxPrincipal(config WindowsSandboxCommandConfig) (func() error, error) { - identity, err := provisionWindowsSandboxPrincipalForSetup(config) + identity, created, err := provisionWindowsSandboxPrincipalForSetup(config) if err != nil { return nil, err } - removePrincipal := func() error { return removeWindowsSandboxPrincipalForSetup(config) } + // 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 + } + return removeWindowsSandboxPrincipalForSetup(config) + } filesystem := config.PermissionProfile.FileSystem plan, err := buildWindowsPrincipalACLPlan(windowsPrincipalACLInput{ @@ -217,6 +280,7 @@ func setupWindowsSandboxPrincipal(config WindowsSandboxCommandConfig) (func() er WriteRoots: filesystem.WriteRoots, ReadRoots: filesystem.ReadRoots, DenyRead: filesystem.DenyRead, + DenyWrite: filesystem.DenyWrite, }) if err != nil { _ = removePrincipal() diff --git a/internal/sandbox/windows_identity_windows.go b/internal/sandbox/windows_identity_windows.go index e2d7b46eb..fbda8fecb 100644 --- a/internal/sandbox/windows_identity_windows.go +++ b/internal/sandbox/windows_identity_windows.go @@ -44,9 +44,16 @@ const ( // windowsSandboxUserPrefix keeps the accounts recognisable in `net user` and // lets cleanup identify what belongs to Zero. Windows caps a local account // name at 20 characters, which windowsSandboxUserName respects. - windowsSandboxUserPrefix = "zero-sbx-" - windowsSandboxUserComment = "Zero sandbox principal (managed)" - windowsSandboxUserNameMax = 20 + windowsSandboxUserPrefix = "zero-sbx-" + // The comment doubles as the ownership marker AND records which workspace + // the account belongs to. The account NAME can only carry 11 characters of + // the workspace digest because of the 20-character local-account limit, so + // two workspaces whose digests share that prefix derive the same name. The + // full key here turns that from a silent share of one account, one secret + // and one ACL identity into a refusal. + windowsSandboxUserComment = "Zero sandbox principal (managed)" + windowsSandboxUserCommentKey = windowsSandboxUserComment + " key=" + windowsSandboxUserNameMax = 20 ) // Win32 status codes that mean "already there". Treated as success so @@ -154,6 +161,12 @@ func windowsSandboxUserName(workspaceKey string) string { return name } +// windowsSandboxUserCommentFor returns the ownership comment for a workspace, +// carrying the full key the account name could only hold 11 characters of. +func windowsSandboxUserCommentFor(workspaceKey string) string { + return windowsSandboxUserCommentKey + workspaceKey +} + // newWindowsSandboxPassword returns a random password for a sandbox principal. // The account is never signed into interactively: the password exists only so // LogonUser can mint a token for it, so it is generated per provisioning run, @@ -225,7 +238,7 @@ func ensureWindowsSandboxGroup() error { // It reports whether the account already existed, because NetUserAdd leaves such // an account completely untouched, password included. The caller has to reset it // or the secret it goes on to store would not be the account's password at all. -func ensureWindowsSandboxUser(username string, password string) (bool, error) { +func ensureWindowsSandboxUser(username string, password string, workspaceKey string) (bool, error) { name, err := windows.UTF16PtrFromString(username) if err != nil { return false, err @@ -234,7 +247,7 @@ func ensureWindowsSandboxUser(username string, password string) (bool, error) { if err != nil { return false, err } - comment, err := windows.UTF16PtrFromString(windowsSandboxUserComment) + comment, err := windows.UTF16PtrFromString(windowsSandboxUserCommentFor(workspaceKey)) if err != nil { return false, err } @@ -332,7 +345,7 @@ var errWindowsSandboxNameCollision = errors.New("a local account with Zero's der // // 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) { +func windowsSandboxUserIsManaged(username string, workspaceKey string) (bool, error) { name, err := windows.UTF16PtrFromString(username) if err != nil { return false, err @@ -359,7 +372,14 @@ func windowsSandboxUserIsManaged(username string) (bool, error) { if info.Comment == nil { return false, nil } - return windows.UTF16PtrToString(info.Comment) == windowsSandboxUserComment, nil + comment := windows.UTF16PtrToString(info.Comment) + // An account provisioned before the key was recorded is still ours; it + // predates this check and cannot be attributed to a workspace, so it is + // adopted and its comment rewritten on the way through. + if comment == windowsSandboxUserComment { + return true, nil + } + return comment == windowsSandboxUserCommentFor(workspaceKey), nil } // resolveWindowsSandboxSID looks up the SID for a provisioned principal. The SID @@ -385,10 +405,12 @@ 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 + ensureWindowsSandboxGroupFn = ensureWindowsSandboxGroup + ensureWindowsSandboxUserFn = ensureWindowsSandboxUser + addWindowsSandboxUserToGroupFn = addWindowsSandboxUserToGroup + resolveWindowsSandboxSIDFn = resolveWindowsSandboxSID + resetWindowsSandboxUserPasswordFn = resetWindowsSandboxUserPassword + windowsSandboxUserIsManagedFn = windowsSandboxUserIsManaged ) // provisionWindowsSandboxIdentity ensures the managed group and one sandbox @@ -410,7 +432,7 @@ func provisionWindowsSandboxIdentity(workspaceKey string) (windowsSandboxIdentit if err != nil { return windowsSandboxIdentity{}, "", false, err } - existed, err := ensureWindowsSandboxUserFn(username, password) + existed, err := ensureWindowsSandboxUserFn(username, password, workspaceKey) if err != nil { return windowsSandboxIdentity{}, "", false, err } @@ -421,20 +443,29 @@ func provisionWindowsSandboxIdentity(workspaceKey string) (windowsSandboxIdentit // 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) + managed, err := windowsSandboxUserIsManagedFn(username, workspaceKey) 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{}, "", false, err - } + // Deliberately NOT resetting the password here. + // + // NetUserAdd left an existing account untouched, so the password above is + // not yet its password and something has to set it. Doing that here, at + // the top of provisioning, opened a window that lasted until the secret + // was written several steps later: a failure anywhere in between left a + // live account whose password nothing on disk knew, and because the + // account already existed the rollback correctly declined to delete it. + // The command path then read the absent secret as "not provisioned" and + // quietly fell back to the weaker backend, so the sandbox was downgraded + // for good with nothing to show for it. + // + // The caller rotates instead, immediately before committing the secret, + // which narrows that window to a single operation. Until it does, the + // account keeps its old password and the old secret on disk still + // authenticates, so a failure before that point costs nothing. } // Both failures below can happen AFTER NetUserAdd created the account, so the // name has to come back with them. The caller's rollback deletes by diff --git a/internal/sandbox/windows_identity_windows_test.go b/internal/sandbox/windows_identity_windows_test.go index f477b518f..f8320bcd5 100644 --- a/internal/sandbox/windows_identity_windows_test.go +++ b/internal/sandbox/windows_identity_windows_test.go @@ -401,7 +401,7 @@ func TestLookupWindowsSandboxIdentityRejectsNonUserAccount(t *testing.T) { func TestWindowsSandboxUserIsManagedRefusesForeignAccounts(t *testing.T) { checked := 0 for _, name := range []string{"Administrator", "Guest", "DefaultAccount"} { - managed, err := windowsSandboxUserIsManaged(name) + managed, err := windowsSandboxUserIsManaged(name, "workspacekey") if err != nil { // Localized or disabled installs may not carry every one of these. continue @@ -416,7 +416,7 @@ func TestWindowsSandboxUserIsManagedRefusesForeignAccounts(t *testing.T) { } // 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") + managed, err := windowsSandboxUserIsManaged("zero-sbx-nosuchacct", "workspacekey") if err != nil { t.Fatalf("querying a missing account: %v", err) } From a1a0edc51ceaf24724cd9d09e5888124409808a1 Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Wed, 29 Jul 2026 20:05:17 +0530 Subject: [PATCH 16/18] fix(sandbox): keep an adopted principal's logon rights on rollback Second round of review findings, both on the elevated setup path. The rollback revoked logon rights whenever they had been attempted, without regard to whether this run created the account. revokeWindowsSandboxLogonRights passes AllRights, which drops every right the account holds and deletes its LSA object outright. On an adopted principal that is not a rollback but destruction: a transient grant, secret-path or secret-write failure during a re-run stripped the SeBatchLogonRight and deny-logon rights an earlier setup had established, leaving exactly the broken-but-present principal this path exists to avoid. Revocation is now scoped to accounts this run created. The rights granted to an adopted account are the ones it is supposed to hold, so leaving them is the safe direction. A secret the current user cannot read now falls back instead of failing the command. The secret's DACL names whoever ran setup, so an operator who elevated with a separate administrative account, through runas or an over-the-shoulder UAC prompt, leaves a secret their ordinary account cannot open. That is the documented fail-soft case, and treating it as a hard error made every sandboxed command fail on a machine that was merely set up by a different admin. Permission errors from the removal path are deliberately still reported, since incomplete teardown is worth knowing about. Both are covered by injected-failure tests and fail if the guard is removed. The secret read is seamed to inject the permission error, because producing a real ERROR_ACCESS_DENIED needs DACL surgery and would test the platform rather than the mapping. --- .../windows_identity_policy_windows_test.go | 74 +++++++++++++++++++ .../windows_identity_runtime_windows.go | 27 +++++-- .../windows_identity_secret_windows.go | 20 ++++- internal/sandbox/windows_identity_windows.go | 2 + 4 files changed, 114 insertions(+), 9 deletions(-) diff --git a/internal/sandbox/windows_identity_policy_windows_test.go b/internal/sandbox/windows_identity_policy_windows_test.go index a5bd7b90e..94e8da063 100644 --- a/internal/sandbox/windows_identity_policy_windows_test.go +++ b/internal/sandbox/windows_identity_policy_windows_test.go @@ -3,9 +3,13 @@ package sandbox import ( + "errors" + "os" "path/filepath" "strings" "testing" + + "golang.org/x/sys/windows" ) // Policy deny-write has to reach the principal plan. @@ -147,3 +151,73 @@ func TestWindowsSandboxUserCommentDistinguishesWorkspaces(t *testing.T) { t.Skip("account names no longer collide for these keys; revisit what this test is for") } } + +// Rollback must not strip an adopted principal's logon rights. +// +// revokeWindowsSandboxLogonRights passes AllRights, which drops every right the +// account holds and deletes its LSA object. On an account this run created that +// is a rollback; on one it adopted it destroys the SeBatchLogonRight and +// deny-logon rights an earlier setup established, which is the working +// principal this path exists to preserve. +func TestSetupRollbackRevokesRightsOnlyForCreatedPrincipals(t *testing.T) { + for name, testCase := range map[string]struct { + existed bool + wantRevoked bool + }{ + "adopted principal": {existed: true, wantRevoked: false}, + "created principal": {existed: false, wantRevoked: true}, + } { + t.Run(name, func(t *testing.T) { + stubWindowsProvisioning(t, testCase.existed, nil, nil) + + revoked := false + prevGrant, prevRevoke := grantWindowsSandboxLogonRightsFn, revokeWindowsSandboxLogonRightsFn + t.Cleanup(func() { + grantWindowsSandboxLogonRightsFn, revokeWindowsSandboxLogonRightsFn = prevGrant, prevRevoke + }) + // Fail the grant so the undo path runs with rights already attempted, + // which is the state that used to revoke unconditionally. + grantWindowsSandboxLogonRightsFn = func(*windows.SID) error { + return errors.New("LSA grant refused by policy") + } + revokeWindowsSandboxLogonRightsFn = func(*windows.SID) error { + revoked = true + return nil + } + + config := WindowsSandboxCommandConfig{ + SandboxHome: t.TempDir(), + WorkspaceRoots: []string{`C:\ws`}, + } + if _, _, err := provisionWindowsSandboxPrincipalForSetup(config); err == nil { + t.Fatal("provisioning reported success despite an injected grant failure") + } + if revoked != testCase.wantRevoked { + if testCase.wantRevoked { + t.Fatal("rights were not revoked for an account this run created, leaving LSA entries keyed to a SID about to be deleted") + } + t.Fatal("rights were revoked for an adopted account; AllRights drops its pre-existing rights and deletes the LSA object") + } + }) + } +} + +// A secret the current user cannot read is unavailability, not breakage. +// +// The secret's DACL names whoever ran setup. An operator who elevated with a +// separate administrative account, via runas or an over-the-shoulder UAC +// prompt, leaves a secret their ordinary account cannot open. Treating that as a +// hard error made every sandboxed command fail on a machine that was merely set +// up by a different admin; it belongs in the same fail-soft path as a missing +// secret, so the warning fires and the restricted token takes over. +func TestReadWindowsSandboxSecretTreatsPermissionDeniedAsUnavailable(t *testing.T) { + previous := readWindowsSandboxSecretFile + t.Cleanup(func() { readWindowsSandboxSecretFile = previous }) + readWindowsSandboxSecretFile = func(string) ([]byte, error) { + return nil, &os.PathError{Op: "open", Path: "secret", Err: windows.ERROR_ACCESS_DENIED} + } + + if _, err := readWindowsSandboxSecret(`C:\anything.secret`); err != errWindowsSandboxIdentityUnavailable { + t.Fatalf("permission-denied read returned %v, want errWindowsSandboxIdentityUnavailable so the command falls back", err) + } +} diff --git a/internal/sandbox/windows_identity_runtime_windows.go b/internal/sandbox/windows_identity_runtime_windows.go index 7c0389800..d6f414e08 100644 --- a/internal/sandbox/windows_identity_runtime_windows.go +++ b/internal/sandbox/windows_identity_runtime_windows.go @@ -192,13 +192,24 @@ func provisionWindowsSandboxPrincipalForSetup(config WindowsSandboxCommandConfig if secretPath != "" && (created || rotated) { _ = removeWindowsSandboxSecret(secretPath) } - // Attempted rather than completed. grantWindowsSandboxLogonRights adds - // rights one at a time and returns on the first failure, so a partial - // grant is possible; gating revocation on success left those entries - // behind, keyed to a SID that deleting the account then made - // unresolvable. Revoking a right that was never granted is harmless. - if identity.SID != nil && rightsAttempted { - _ = revokeWindowsSandboxLogonRights(identity.SID) + // Only for an account this run created, and attempted rather than + // completed. + // + // Attempted, because grantWindowsSandboxLogonRights adds rights one at a + // time and returns on the first failure, so a partial grant is possible + // and gating on success left those entries behind, keyed to a SID that + // deleting the account then made unresolvable. + // + // Created, because revokeWindowsSandboxLogonRights passes AllRights, which + // drops every right the account holds and deletes its LSA object outright. + // On an adopted principal that is not a rollback, it is destruction: a + // transient failure anywhere below would strip the SeBatchLogonRight and + // deny-logon rights a previous setup established, leaving exactly the + // broken-but-present principal this whole function exists to avoid. The + // rights this run granted are the ones the account is supposed to have, so + // leaving them in place on an adopted account is the safe direction. + if identity.SID != nil && rightsAttempted && created { + _ = revokeWindowsSandboxLogonRightsFn(identity.SID) } if created { _ = removeWindowsSandboxIdentity(identity.Username) @@ -212,7 +223,7 @@ func provisionWindowsSandboxPrincipalForSetup(config WindowsSandboxCommandConfig return windowsSandboxIdentity{}, false, err } rightsAttempted = true - if err := grantWindowsSandboxLogonRights(identity.SID); err != nil { + if err := grantWindowsSandboxLogonRightsFn(identity.SID); err != nil { undo() return windowsSandboxIdentity{}, false, err } diff --git a/internal/sandbox/windows_identity_secret_windows.go b/internal/sandbox/windows_identity_secret_windows.go index 96cee6305..87463940b 100644 --- a/internal/sandbox/windows_identity_secret_windows.go +++ b/internal/sandbox/windows_identity_secret_windows.go @@ -180,15 +180,33 @@ func windowsSandboxSecretEntropy(path string) string { return strings.TrimSuffix(filepath.Base(path), ".secret") } +// Seamed so the permission-denied mapping in readWindowsSandboxSecret is +// testable. Producing a real ERROR_ACCESS_DENIED needs DACL surgery on Windows, +// since a 0000 file is still readable and reading a directory reports +// "Incorrect function", so a test built that way would exercise the platform +// rather than the mapping. +var readWindowsSandboxSecretFile = os.ReadFile + // readWindowsSandboxSecret loads a principal's password. A missing file means // setup has not run for this workspace, which the caller turns into a fallback // rather than a hard failure. func readWindowsSandboxSecret(path string) (string, error) { - data, err := os.ReadFile(path) + data, err := readWindowsSandboxSecretFile(path) if err != nil { if os.IsNotExist(err) { return "", errWindowsSandboxIdentityUnavailable } + // Permission denied is unavailability, not breakage. The secret's DACL + // names whoever ran setup, so an operator who elevated with a separate + // administrative account, through runas or an over-the-shoulder UAC + // prompt, ends up with a secret their ordinary account cannot open. That + // is the documented fail-soft case: fall back to the restricted token and + // let the warning say so. Treating it as a hard error instead made every + // sandboxed command fail on a machine that was merely set up by a + // different admin, which is a common way to run an elevated setup. + if os.IsPermission(err) { + return "", errWindowsSandboxIdentityUnavailable + } return "", fmt.Errorf("read sandbox secret: %w", err) } if len(data) == 0 { diff --git a/internal/sandbox/windows_identity_windows.go b/internal/sandbox/windows_identity_windows.go index fbda8fecb..cf8b690f8 100644 --- a/internal/sandbox/windows_identity_windows.go +++ b/internal/sandbox/windows_identity_windows.go @@ -411,6 +411,8 @@ var ( resolveWindowsSandboxSIDFn = resolveWindowsSandboxSID resetWindowsSandboxUserPasswordFn = resetWindowsSandboxUserPassword windowsSandboxUserIsManagedFn = windowsSandboxUserIsManaged + grantWindowsSandboxLogonRightsFn = grantWindowsSandboxLogonRights + revokeWindowsSandboxLogonRightsFn = revokeWindowsSandboxLogonRights ) // provisionWindowsSandboxIdentity ensures the managed group and one sandbox From f4a2b9beb09b808657f0732fe8a4b29cdb2c8785 Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Thu, 30 Jul 2026 11:08:58 +0530 Subject: [PATCH 17/18] fix(sandbox): grant the principal the runtime tree commands write to Four review findings on the elevated setup path. The principal had no access to the sandbox runtime root. permissionProfileWithRuntime appends that root to WriteRoots on every command and redirects HOME, GOCACHE, npm_config_cache and similar into it, but it lives under the user cache rather than the workspace, so the profile setup builds its ACL plan from 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 every npm install, go build or pip install would have failed on a cache write with a bare ACCESS_DENIED and nothing naming the sandbox as the cause. Setup now resolves the same root and grants it. The derivation is extracted so both callers share it. If setup and prepareSandboxRuntime ever disagreed, the ACE would land on one directory while commands used another, which is the same failure with a harder diagnosis, so a test asserts the two agree. The git control-plane carveouts are materialized. .git/config and .git/hooks arrive as ReadOnlySubpaths, and applyWindowsACLPlan skips an absent target, so on a workspace where git had not run yet the deny ACEs were never written and the principal kept inherited write access once git created them. Command-time lookup verifies workspace ownership. The account name carries only 11 characters of the workspace digest; the comment carries all of it. Provisioning already refused a foreign account, but the command path resolved the name straight to a SID, so the workspace that lost a collision would have run as the other one's principal. SID resolution still runs first, so an absent account stays the unavailable sentinel rather than becoming a collision error. The gated round-trip test asserted a logon with the password from a second provisioning call. Rotation moved to the setup path, so that value is a fresh string the account never held. It now exercises the guarantee the setup path actually makes: the stored secret logs the principal on. --- internal/sandbox/runtime_state.go | 28 +++- internal/sandbox/windows_identity_acl.go | 16 ++- .../windows_identity_policy_windows_test.go | 132 ++++++++++++++++++ .../windows_identity_runtime_windows.go | 63 ++++++++- internal/sandbox/windows_identity_windows.go | 27 +++- .../sandbox/windows_identity_windows_test.go | 49 +++++-- 6 files changed, 290 insertions(+), 25 deletions(-) diff --git a/internal/sandbox/runtime_state.go b/internal/sandbox/runtime_state.go index a738396fc..8f71f0db1 100644 --- a/internal/sandbox/runtime_state.go +++ b/internal/sandbox/runtime_state.go @@ -36,6 +36,24 @@ type SandboxRuntime struct { Temp string `json:"temp,omitempty"` } +// sandboxRuntimeRootFor derives the per-workspace runtime root. It is separated +// from prepareSandboxRuntime because the elevated Windows setup path needs the +// same answer WITHOUT taking a lease or creating anything: a sandbox principal +// is a separate account with no inherited rights under the user cache, so setup +// has to grant it write access to this tree before any command runs. +// +// Both callers must agree exactly. If they ever drift, setup grants the ACE on +// one directory while commands write to another, and the failure is a bare +// ACCESS_DENIED from npm or go build with nothing pointing at the sandbox. +func sandboxRuntimeRootFor(workspaceRoot string, cacheRoot string) (string, error) { + digest := sha256.Sum256([]byte(workspaceRoot)) + root := filepath.Join(cacheRoot, "zero", "runtime", "v1", hex.EncodeToString(digest[:8])) + if !pathWithinRoot(workspaceRoot, root) { + return root, nil + } + return fallbackSandboxRuntimeRoot(workspaceRoot) +} + func prepareSandboxRuntime(workspaceRoot string) (SandboxRuntime, func(), error) { workspaceRoot = filepath.Clean(strings.TrimSpace(workspaceRoot)) if workspaceRoot == "" || workspaceRoot == "." { @@ -49,13 +67,9 @@ func prepareSandboxRuntime(workspaceRoot string) (SandboxRuntime, func(), error) if cacheRoot == "" || cacheRoot == "." { return SandboxRuntime{}, nil, errors.New("user cache directory is unavailable") } - digest := sha256.Sum256([]byte(workspaceRoot)) - root := filepath.Join(cacheRoot, "zero", "runtime", "v1", hex.EncodeToString(digest[:8])) - if pathWithinRoot(workspaceRoot, root) { - root, err = fallbackSandboxRuntimeRoot(workspaceRoot) - if err != nil { - return SandboxRuntime{}, nil, err - } + root, err := sandboxRuntimeRootFor(workspaceRoot, cacheRoot) + if err != nil { + return SandboxRuntime{}, nil, err } lease, err := prepareSandboxRuntimeLease(root) if err != nil { diff --git a/internal/sandbox/windows_identity_acl.go b/internal/sandbox/windows_identity_acl.go index 9dced2fbb..bdbeba608 100644 --- a/internal/sandbox/windows_identity_acl.go +++ b/internal/sandbox/windows_identity_acl.go @@ -95,11 +95,21 @@ func buildWindowsPrincipalACLPlan(input windowsPrincipalACLInput) (WindowsACLPla if cleaned == "" { return WindowsACLPlan{}, fmt.Errorf("windows principal ACL plan: unusable write root %q", root.Root) } + // Materialized, like the metadata and policy denies below and above. + // + // These are the git control-plane carveouts (.git/config, .git/hooks). On a + // workspace where git has not run yet they do not exist at setup time, and + // applyWindowsACLPlan skips a target that is absent, so the ACEs were never + // written. Once git created those paths the principal still held inherited + // write access to the workspace and could install a hook or rewrite + // credential.helper. The capability plan gets away without this because its + // child runs as the caller; a separate principal account does not. for _, subpath := range normalizeProfilePaths(root.ReadOnlySubpaths) { entries = append(entries, WindowsACLEntry{ - Action: WindowsACLDenyWrite, - Path: subpath, - Capability: input.PrincipalSID, + Action: WindowsACLDenyWrite, + Path: subpath, + Capability: input.PrincipalSID, + Materialize: true, }) } for _, name := range root.ProtectedMetadataNames { diff --git a/internal/sandbox/windows_identity_policy_windows_test.go b/internal/sandbox/windows_identity_policy_windows_test.go index 94e8da063..145d17a6c 100644 --- a/internal/sandbox/windows_identity_policy_windows_test.go +++ b/internal/sandbox/windows_identity_policy_windows_test.go @@ -221,3 +221,135 @@ func TestReadWindowsSandboxSecretTreatsPermissionDeniedAsUnavailable(t *testing. t.Fatalf("permission-denied read returned %v, want errWindowsSandboxIdentityUnavailable so the command falls back", err) } } + +// A workspace must not bind to another workspace's principal. +// +// The account name carries only 11 characters of the workspace digest, so two +// workspaces can derive the same name. Provisioning refuses that case by +// checking the full key in the account comment, but the command path resolved +// the name straight to a SID. The workspace that lost the race would have failed +// setup and then quietly run as the other one's principal, using its secret and +// its ACL identity. +func TestLookupWindowsSandboxIdentityRejectsForeignWorkspace(t *testing.T) { + previous := windowsSandboxUserIsManagedFn + t.Cleanup(func() { windowsSandboxUserIsManagedFn = previous }) + + prevSID := resolveWindowsSandboxSIDFn + t.Cleanup(func() { resolveWindowsSandboxSIDFn = prevSID }) + // The account resolves; whether it BELONGS to this workspace is the question. + resolveWindowsSandboxSIDFn = func(string) (*windows.SID, error) { + return windows.CreateWellKnownSid(windows.WinLocalSystemSid) + } + + var askedKey string + windowsSandboxUserIsManagedFn = func(_ string, workspaceKey string) (bool, error) { + askedKey = workspaceKey + return false, nil + } + _, err := lookupWindowsSandboxIdentity("workspacekey") + if err == nil { + t.Fatal("lookup accepted an account belonging to another workspace") + } + if errors.Is(err, errWindowsSandboxIdentityUnavailable) { + t.Fatal("a foreign account must not read as unprovisioned; that would silently fall back instead of reporting the conflict") + } + if askedKey != "" && askedKey != "workspacekey" { + t.Fatalf("ownership was checked against %q, want the caller's workspace key", askedKey) + } +} + +// An account that does not exist must stay the unavailable sentinel rather than +// becoming a collision error, since that is the ordinary not-set-up state. +func TestLookupWindowsSandboxIdentityAbsentAccountIsUnavailable(t *testing.T) { + previous := windowsSandboxUserIsManagedFn + t.Cleanup(func() { windowsSandboxUserIsManagedFn = previous }) + windowsSandboxUserIsManagedFn = func(string, string) (bool, error) { + 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) { + t.Fatalf("absent account returned %v, want errWindowsSandboxIdentityUnavailable", err) + } +} + +// The git control-plane carveouts must be materialized. +// +// gitMetadataWriteCarveouts supplies .git/config and .git/hooks as +// ReadOnlySubpaths of the workspace. On a workspace where git has not run yet +// they do not exist when setup applies the plan, and applyWindowsACLPlan skips +// an absent target, so the deny ACEs were never written. Once git created those +// paths the principal still held inherited write access and could install a +// hook or rewrite credential.helper. +func TestPrincipalACLPlanMaterializesReadOnlySubpaths(t *testing.T) { + root := filepath.Join(t.TempDir(), "workspace") + carveout := filepath.Join(root, ".git", "config") + + plan, err := buildWindowsPrincipalACLPlan(windowsPrincipalACLInput{ + PrincipalSID: "S-1-5-21-0-0-0-1001", + WriteRoots: []WritableRoot{{Root: root, ReadOnlySubpaths: []string{carveout}}}, + }) + if err != nil { + t.Fatalf("buildWindowsPrincipalACLPlan: %v", err) + } + entry, ok := findPrincipalACLEntry(plan, WindowsACLDenyWrite, carveout) + if !ok { + t.Fatalf("no deny-write ACE for the git carveout; plan = %+v", plan.Entries) + } + if !entry.Materialize { + t.Fatal("git carveout deny-write is not materialized, so it is skipped on a workspace where .git does not exist yet") + } +} + +// Setup must grant the principal the same runtime root that commands write to. +// +// permissionProfileWithRuntime appends this root to WriteRoots on every command +// and redirects HOME, GOCACHE and npm_config_cache into it, but it lives under +// the user cache rather than the workspace, so the profile setup sees never +// contains it. A principal is a separate account with no rights there, so +// without a grant every npm install or go build fails on a cache write. +// +// The assertion that matters is that the two derivations agree. If they drift, +// setup writes the ACE on one directory while commands use another, and the +// symptom is a bare ACCESS_DENIED with nothing pointing at the sandbox. +func TestSetupGrantsTheRuntimeRootCommandsActuallyUse(t *testing.T) { + workspace := filepath.Join(t.TempDir(), "ws") + if err := os.MkdirAll(workspace, 0o700); err != nil { + t.Fatal(err) + } + + granted, err := setupWindowsSandboxRuntimeRoot(WindowsSandboxCommandConfig{ + WorkspaceRoots: []string{workspace}, + }) + if err != nil { + t.Fatalf("setupWindowsSandboxRuntimeRoot: %v", err) + } + if granted == "" { + t.Fatal("no runtime root resolved for a configured workspace") + } + if info, err := os.Stat(granted); err != nil || !info.IsDir() { + t.Fatalf("runtime root %q was not created; applyWindowsACLPlan skips absent targets so the grant would no-op (stat err %v)", granted, err) + } + + // What a command would actually use. + runtimeState, release, err := prepareSandboxRuntime(workspace) + if err != nil { + t.Fatalf("prepareSandboxRuntime: %v", err) + } + if release != nil { + defer release() + } + if filepath.Clean(runtimeState.Root) != filepath.Clean(granted) { + t.Fatalf("setup granted %q but commands write to %q", granted, runtimeState.Root) + } +} + +// No workspace root means nothing to grant, which is not an error. +func TestSetupRuntimeRootWithoutWorkspaceIsNotAnError(t *testing.T) { + granted, err := setupWindowsSandboxRuntimeRoot(WindowsSandboxCommandConfig{}) + if err != nil { + t.Fatalf("no workspace root should not error: %v", err) + } + if granted != "" { + t.Fatalf("granted %q with no workspace configured", granted) + } +} diff --git a/internal/sandbox/windows_identity_runtime_windows.go b/internal/sandbox/windows_identity_runtime_windows.go index d6f414e08..916cf27f0 100644 --- a/internal/sandbox/windows_identity_runtime_windows.go +++ b/internal/sandbox/windows_identity_runtime_windows.go @@ -18,6 +18,7 @@ import ( "errors" "fmt" "os" + "path/filepath" "strings" "sync" @@ -286,9 +287,28 @@ func setupWindowsSandboxPrincipal(config WindowsSandboxCommandConfig) (func() er } filesystem := config.PermissionProfile.FileSystem + writeRoots := filesystem.WriteRoots + // The runtime tree has to be granted here, at setup, because nothing grants it + // later. + // + // 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}) + } plan, err := buildWindowsPrincipalACLPlan(windowsPrincipalACLInput{ PrincipalSID: identity.SID.String(), - WriteRoots: filesystem.WriteRoots, + WriteRoots: writeRoots, ReadRoots: filesystem.ReadRoots, DenyRead: filesystem.DenyRead, DenyWrite: filesystem.DenyWrite, @@ -342,3 +362,44 @@ func removeWindowsSandboxPrincipalForSetup(config WindowsSandboxCommandConfig) e } return removeWindowsSandboxIdentity(username) } + +// setupWindowsSandboxRuntimeRoot resolves this workspace's runtime root and +// makes sure it exists, so the principal ACL plan can name it. +// +// It is created here rather than left to the first command because +// applyWindowsACLPlan skips a target that does not exist: granting write on a +// directory that setup never made would silently no-op, and the failure would +// only show up later as a denied cache write. Creating it under the elevated +// setup process is safe, since it lives under the invoking user's own cache +// directory and prepareSandboxRuntime would create it on the same path anyway. +// +// An empty return means there is no runtime root to grant (no workspace root +// configured), which is not an error: the caller simply grants nothing extra. +func setupWindowsSandboxRuntimeRoot(config WindowsSandboxCommandConfig) (string, error) { + workspaceRoot := "" + for _, candidate := range config.WorkspaceRoots { + if trimmed := strings.TrimSpace(candidate); trimmed != "" { + workspaceRoot = filepath.Clean(trimmed) + break + } + } + if workspaceRoot == "" { + return "", nil + } + cacheRoot, err := sandboxUserCacheDir() + if err != nil { + return "", fmt.Errorf("resolve user cache directory for sandbox runtime: %w", err) + } + cacheRoot = filepath.Clean(strings.TrimSpace(cacheRoot)) + if cacheRoot == "" || cacheRoot == "." { + return "", errors.New("user cache directory is unavailable for sandbox runtime") + } + root, err := sandboxRuntimeRootFor(workspaceRoot, cacheRoot) + if err != nil { + return "", err + } + if err := os.MkdirAll(root, 0o700); err != nil { + return "", fmt.Errorf("create sandbox runtime root: %w", err) + } + return root, nil +} diff --git a/internal/sandbox/windows_identity_windows.go b/internal/sandbox/windows_identity_windows.go index cf8b690f8..c9152512b 100644 --- a/internal/sandbox/windows_identity_windows.go +++ b/internal/sandbox/windows_identity_windows.go @@ -516,10 +516,35 @@ var errWindowsSandboxIdentityUnavailable = errors.New("no Zero sandbox principal // has not run. func lookupWindowsSandboxIdentity(workspaceKey string) (windowsSandboxIdentity, error) { username := windowsSandboxUserName(workspaceKey) - sid, err := resolveWindowsSandboxSID(username) + // 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. + // 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 + // report an unprovisioned workspace as a name collision and turn the ordinary + // not-set-up case into an error the operator has to interpret. + sid, err := resolveWindowsSandboxSIDFn(username) if err != nil { return windowsSandboxIdentity{}, classifyWindowsSandboxLookupError(err) } + managed, err := windowsSandboxUserIsManagedFn(username, workspaceKey) + if err != nil { + return windowsSandboxIdentity{}, err + } + if !managed { + return windowsSandboxIdentity{}, fmt.Errorf("%w: %q", errWindowsSandboxNameCollision, username) + } return windowsSandboxIdentity{Username: username, SID: sid}, nil } diff --git a/internal/sandbox/windows_identity_windows_test.go b/internal/sandbox/windows_identity_windows_test.go index f8320bcd5..11d64c741 100644 --- a/internal/sandbox/windows_identity_windows_test.go +++ b/internal/sandbox/windows_identity_windows_test.go @@ -209,9 +209,10 @@ func TestProvisionWindowsSandboxIdentityRoundTrip(t *testing.T) { if !windowsProcessIsElevated() { t.Skip("provisioning requires an elevated process") } - // A leftover account from an interrupted run is harmless now that - // provisioning resets the password, but starting clean keeps a failure here - // from being explained by residue from a previous one. + // Starting clean keeps a failure here from being explained by residue from a + // 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")) identity, password, _, err := provisionWindowsSandboxIdentity("ziptest01") @@ -246,23 +247,45 @@ func TestProvisionWindowsSandboxIdentityRoundTrip(t *testing.T) { if again.Username != identity.Username || !again.SID.Equals(identity.SID) { t.Fatalf("provisioning is not idempotent: %s then %s", identity, again) } - // The password returned for an account that already existed has to BE that - // account's password. NetUserAdd leaves an existing account entirely alone, - // so without an explicit reset this second value is a fresh random string - // that never authenticates, and the caller would store it as the secret and - // leave every later command failing to log on with a principal that looks - // correctly provisioned. Logging on is the only honest way to assert it. if secondPassword == "" { t.Fatal("second provision returned an empty password") } - if err := grantWindowsSandboxLogonRights(again.SID); err != nil { - t.Fatalf("grant logon rights: %v", err) + // Deliberately NOT logging on with secondPassword. Provisioning does not + // rotate an adopted account any more, so that value is a fresh random string + // the account does not hold; rotation happens in + // provisionWindowsSandboxPrincipalForSetup, immediately before the secret is + // written, to keep the window where no stored password authenticates as small + // as possible. + // + // The guarantee worth asserting is therefore the one the setup path makes: + // after it returns, the stored secret logs the principal on. That covers + // rotation, the secret write and the logon right in one assertion, and it is + // the thing a broken re-setup would actually break. + config := WindowsSandboxCommandConfig{ + SandboxHome: t.TempDir(), + WorkspaceRoots: []string{`C:\ziptest01`}, + } + setupIdentity, _, err := provisionWindowsSandboxPrincipalForSetup(config) + if err != nil { + t.Fatalf("setup provision: %v", err) + } + secretPath, err := windowsSandboxSecretPath(config.SandboxHome, setupIdentity.Username) + if err != nil { + t.Fatalf("secret path: %v", err) } - token, err := logonWindowsSandboxPrincipal(again.Username, secondPassword) + storedPassword, err := readWindowsSandboxSecret(secretPath) if err != nil { - t.Fatalf("logon with the password from the second provision: %v", err) + t.Fatalf("read stored secret: %v", err) + } + token, err := logonWindowsSandboxPrincipal(setupIdentity.Username, storedPassword) + if err != nil { + t.Fatalf("logon with the secret the setup path stored: %v", err) } _ = token.Close() + t.Cleanup(func() { + _ = revokeWindowsSandboxLogonRights(setupIdentity.SID) + _ = removeWindowsSandboxIdentity(setupIdentity.Username) + }) // Lookup must find what provisioning created. found, err := lookupWindowsSandboxIdentity("ziptest01") if err != nil { From 71ad0f182aef03407b3de25ce0f13fb5ea974324 Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Thu, 30 Jul 2026 11:24:49 +0530 Subject: [PATCH 18/18] fix(sandbox): refuse to adopt a principal in a privileged group Adoption takes over an account whose name and ownership comment match, resets its password and hands it to the sandbox. An account that is also in Administrators, Power Users or Backup Operators would give the sandbox the rights it exists to withhold: rewriting the ACLs confining it, reading the secret locked to the invoking user, and stopping Zero. The name is derived rather than discovered, so an account can match without anyone intending it to. Membership is resolved by well-known SID rather than by group name, so a localised install where the group is Administratoren or Administrateurs is still recognised. Raised as a non-blocking follow-up in review; it is cheap enough to do now rather than track. --- .../windows_identity_policy_windows_test.go | 36 ++++++ internal/sandbox/windows_identity_windows.go | 110 ++++++++++++++++++ 2 files changed, 146 insertions(+) diff --git a/internal/sandbox/windows_identity_policy_windows_test.go b/internal/sandbox/windows_identity_policy_windows_test.go index 145d17a6c..f1137cebd 100644 --- a/internal/sandbox/windows_identity_policy_windows_test.go +++ b/internal/sandbox/windows_identity_policy_windows_test.go @@ -353,3 +353,39 @@ func TestSetupRuntimeRootWithoutWorkspaceIsNotAnError(t *testing.T) { t.Fatalf("granted %q with no workspace configured", granted) } } + +// An account in a privileged group must not be adopted, even when name and +// ownership comment say it is ours. +// +// Adoption resets the password and hands the account to the sandbox. If that +// account is also in Administrators, the sandbox gains the rights the sandbox +// 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) + + previous := windowsSandboxUserIsPrivilegedFn + t.Cleanup(func() { windowsSandboxUserIsPrivilegedFn = previous }) + windowsSandboxUserIsPrivilegedFn = func(string) (bool, error) { return true, nil } + + _, _, created, err := provisionWindowsSandboxIdentity("workspacekey") + if !errors.Is(err, errWindowsSandboxPrivilegedAccount) { + t.Fatalf("provisioning adopted a privileged account, err = %v", err) + } + if created { + t.Fatal("created must stay false for an account this run refused to adopt") + } +} + +// The ordinary adopted account is unaffected. +func TestProvisionWindowsSandboxIdentityAdoptsUnprivilegedAccount(t *testing.T) { + stubWindowsProvisioning(t, true, nil, nil) + + previous := windowsSandboxUserIsPrivilegedFn + t.Cleanup(func() { windowsSandboxUserIsPrivilegedFn = previous }) + windowsSandboxUserIsPrivilegedFn = func(string) (bool, error) { return false, nil } + + if _, _, _, err := provisionWindowsSandboxIdentity("workspacekey"); err != nil { + t.Fatalf("an unprivileged managed account must still be adopted: %v", err) + } +} diff --git a/internal/sandbox/windows_identity_windows.go b/internal/sandbox/windows_identity_windows.go index c9152512b..819ef94f3 100644 --- a/internal/sandbox/windows_identity_windows.go +++ b/internal/sandbox/windows_identity_windows.go @@ -86,6 +86,7 @@ var ( procNetUserSetInfo = netapi32.NewProc("NetUserSetInfo") procNetUserGetInfo = netapi32.NewProc("NetUserGetInfo") procNetApiBufferFree = netapi32.NewProc("NetApiBufferFree") + procNetUserGetLocalGroups = netapi32.NewProc("NetUserGetLocalGroups") ) // userInfo1 mirrors USER_INFO_1. Field order and widths must match the Win32 @@ -334,6 +335,8 @@ func addWindowsSandboxUserToGroup(username string) error { // errWindowsSandboxNameCollision reports that the derived account name is taken // by a local account Zero did not create. Setup refuses rather than adopting it. +var errWindowsSandboxPrivilegedAccount = errors.New("the local account matching Zero's derived sandbox name belongs to a privileged group (Administrators, Power Users or Backup Operators); refusing to adopt it as a sandbox principal") + 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 @@ -382,6 +385,99 @@ func windowsSandboxUserIsManaged(username string, workspaceKey string) (bool, er return comment == windowsSandboxUserCommentFor(workspaceKey), nil } +// localGroupUsersInfo0 mirrors LOCALGROUP_USERS_INFO_0: one group name pointer. +type localGroupUsersInfo0 struct { + Name *uint16 +} + +// windowsSandboxUserIsPrivileged reports whether an account belongs to a local +// group that would make it a poor sandbox principal. +// +// Adoption is the reason this exists. Provisioning will take over an account +// whose name and ownership comment match, and an account that is also in +// Administrators would hand the sandbox exactly the rights the sandbox is meant +// to withhold: it could rewrite the ACLs confining it, read the secret locked to +// the invoking user, and terminate Zero. The name is derived rather than +// discovered, so an account can end up matching without anyone intending it. +// +// Membership is resolved by SID rather than by name so a localised install, where +// the group is called Administrateurs or Administratoren, is still recognised. +func windowsSandboxUserIsPrivileged(username 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))) + + privileged, err := privilegedLocalGroupNames() + if err != nil { + return false, err + } + groups := unsafe.Slice((*localGroupUsersInfo0)(unsafe.Pointer(buffer)), entries) + for _, group := range groups { + if group.Name == nil { + continue + } + if privileged[strings.ToLower(windows.UTF16PtrToString(group.Name))] { + return true, nil + } + } + return false, nil +} + +// privilegedLocalGroupNames resolves the local names of the groups a sandbox +// principal must not belong to. Resolved from well-known SIDs so the comparison +// survives a localised Windows install. +func privilegedLocalGroupNames() (map[string]bool, error) { + out := map[string]bool{} + for _, wellKnown := range []windows.WELL_KNOWN_SID_TYPE{ + windows.WinBuiltinAdministratorsSid, + windows.WinBuiltinPowerUsersSid, + windows.WinBuiltinBackupOperatorsSid, + } { + sid, err := windows.CreateWellKnownSid(wellKnown) + if err != nil { + // A group this build of Windows does not define is not a membership + // anyone can hold, so it cannot make an account privileged. + continue + } + account, _, _, err := sid.LookupAccount("") + if err != nil { + continue + } + out[strings.ToLower(account)] = true + } + if len(out) == 0 { + return nil, errors.New("could not resolve any privileged local group name") + } + return out, 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. @@ -411,6 +507,7 @@ var ( resolveWindowsSandboxSIDFn = resolveWindowsSandboxSID resetWindowsSandboxUserPasswordFn = resetWindowsSandboxUserPassword windowsSandboxUserIsManagedFn = windowsSandboxUserIsManaged + windowsSandboxUserIsPrivilegedFn = windowsSandboxUserIsPrivileged grantWindowsSandboxLogonRightsFn = grantWindowsSandboxLogonRights revokeWindowsSandboxLogonRightsFn = revokeWindowsSandboxLogonRights ) @@ -452,6 +549,19 @@ func provisionWindowsSandboxIdentity(workspaceKey string) (windowsSandboxIdentit if !managed { return windowsSandboxIdentity{}, "", false, fmt.Errorf("%w: %q", errWindowsSandboxNameCollision, username) } + // Ours by name and comment is not enough to adopt it. An account that also + // sits in Administrators (or Power Users, or Backup Operators) would give + // the sandbox the rights the sandbox exists to withhold: it could rewrite + // the ACLs confining it, read the secret locked to the invoking user, and + // stop Zero. Refuse rather than quietly take it over, and say which account + // so an operator can look at it. + privileged, err := windowsSandboxUserIsPrivilegedFn(username) + if err != nil { + return windowsSandboxIdentity{}, "", false, err + } + if privileged { + return windowsSandboxIdentity{}, "", false, fmt.Errorf("%w: %q", errWindowsSandboxPrivilegedAccount, username) + } // Deliberately NOT resetting the password here. // // NetUserAdd left an existing account untouched, so the password above is