feat(sandbox): Windows sandbox principals (foundation for #662, does not close it) - #808
feat(sandbox): Windows sandbox principals (foundation for #662, does not close it)#808Vasanthdev2004 wants to merge 72 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds Windows sandbox identity provisioning, protected principal-secret storage, batch logon support, runtime token selection, and principal-specific ACL planning with Windows-focused unit and integration tests. ChangesWindows sandbox principal
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/sandbox/windows_identity_logon_windows.go (2)
48-54: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsolidate the five separate
advapi32.dlllazy loads.Five independent
windows.NewLazySystemDLL("advapi32.dll")calls wherewindows_identity_windows.gouses a single sharednetapi32var for its DLL and derives procs from it. Mirroring that pattern here is cheap and keeps the two files consistent.♻️ Proposed refactor
-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") -) +var ( + advapi32 = windows.NewLazySystemDLL("advapi32.dll") + procLogonUserW = advapi32.NewProc("LogonUserW") + procLsaOpenPolicy = advapi32.NewProc("LsaOpenPolicy") + procLsaClose = advapi32.NewProc("LsaClose") + procLsaAddAccountRights = advapi32.NewProc("LsaAddAccountRights") + procLsaNtStatusToWinErr = advapi32.NewProc("LsaNtStatusToWinError") +)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/sandbox/windows_identity_logon_windows.go` around lines 48 - 54, Consolidate the five independent advapi32.dll lazy loads in the proc declarations around procLogonUserW, procLsaOpenPolicy, procLsaClose, procLsaAddAccountRights, and procLsaNtStatusToWinErr by defining one shared lazy DLL variable and deriving each procedure from it, matching the shared-DLL pattern used by the neighboring Windows identity implementation.
195-203: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRedundant/fragile "keep alive" idiom repeated across both files.
Both files independently reinvent a "keep the buffer alive after the syscall" step, but the object is already retained through the call by the compiler's special-case handling of
uintptr(unsafe.Pointer(x))appearing in the.Call()argument list (perunsafepackage docs, this also applies toLazyProc.Callon Windows), and pointer fields nested inside that object are reachable transitively via normal GC tracing. None of these five sites add real protection, and if protection were ever genuinely needed,_ = buffer[0]/_ = infois not the guaranteed primitive for it —runtime.KeepAliveis.
internal/sandbox/windows_identity_logon_windows.go#L195-L203: replace theruntimeKeepAliveUint16helper with a directruntime.KeepAlive(buffer)call at each use (or drop it, since the buffer is already protected viaentryin the.Call()argument).internal/sandbox/windows_identity_logon_windows.go#L150-L152: swapruntimeKeepAliveUint16(buffer)forruntime.KeepAlive(buffer), or remove the line.internal/sandbox/windows_identity_windows.go#L202-L204: dropdefer func(){_=info}()inensureWindowsSandboxGroup, or replace withdefer runtime.KeepAlive(&info)if you want to keep the intent explicit.internal/sandbox/windows_identity_windows.go#L239: same for theinfodefer inensureWindowsSandboxUser.internal/sandbox/windows_identity_windows.go#L262: same for theentrydefer inaddWindowsSandboxUserToGroup.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/sandbox/windows_identity_logon_windows.go` around lines 195 - 203, Remove the redundant fragile keep-alive idioms and rely on the syscall argument retention; in internal/sandbox/windows_identity_logon_windows.go:150-152 and :195-203, remove runtimeKeepAliveUint16 and its uses (or replace each with runtime.KeepAlive(buffer) if explicit intent is retained). In internal/sandbox/windows_identity_windows.go:202-204, :239, and :262, remove the defer closures referencing info or entry, or replace them with defer runtime.KeepAlive(&info) / defer runtime.KeepAlive(&entry) respectively.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/sandbox/windows_identity_acl.go`:
- Around line 85-91: Validate each value in ProtectedMetadataNames before
constructing the WindowsACLEntry, accepting only a single non-empty path
component and rejecting empty values, "."/"..", and any value containing path
separators. Do not call filepath.Join for rejected names; add tests covering
traversal and separator-containing inputs while preserving valid-name
materialization.
---
Nitpick comments:
In `@internal/sandbox/windows_identity_logon_windows.go`:
- Around line 48-54: Consolidate the five independent advapi32.dll lazy loads in
the proc declarations around procLogonUserW, procLsaOpenPolicy, procLsaClose,
procLsaAddAccountRights, and procLsaNtStatusToWinErr by defining one shared lazy
DLL variable and deriving each procedure from it, matching the shared-DLL
pattern used by the neighboring Windows identity implementation.
- Around line 195-203: Remove the redundant fragile keep-alive idioms and rely
on the syscall argument retention; in
internal/sandbox/windows_identity_logon_windows.go:150-152 and :195-203, remove
runtimeKeepAliveUint16 and its uses (or replace each with
runtime.KeepAlive(buffer) if explicit intent is retained). In
internal/sandbox/windows_identity_windows.go:202-204, :239, and :262, remove the
defer closures referencing info or entry, or replace them with defer
runtime.KeepAlive(&info) / defer runtime.KeepAlive(&entry) respectively.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c2343104-e3d2-400c-8739-a6f655821fe1
📒 Files selected for processing (6)
internal/sandbox/windows_acl_apply_windows.gointernal/sandbox/windows_identity_acl.gointernal/sandbox/windows_identity_acl_test.gointernal/sandbox/windows_identity_logon_windows.gointernal/sandbox/windows_identity_windows.gointernal/sandbox/windows_identity_windows_test.go
Zero automated PR reviewVerdict: No blockers found Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
internal/sandbox/windows_command_runner_windows.go (2)
84-88: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGive the operator an exit when the principal backend breaks.
This is the one path that hard-fails instead of falling back, and the message is a bare wrapped error. Since the whole feature is opt-in, tell the user how to opt back out — the
ensureWindowsUnelevatedSetupmessage at Line 136 is a good model for actionable runner errors.♻️ Suggested wording
principalToken, ok, err := windowsSandboxPrincipalToken(config) if err != nil { - fmt.Fprintln(stderr, WindowsSandboxCommandRunnerName+": "+err.Error()) + 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 }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/sandbox/windows_command_runner_windows.go` around lines 84 - 88, Update the error handling around windowsSandboxPrincipalToken so the stderr message explains that the Windows sandbox principal backend failed and gives the operator an actionable way to disable or opt out of the opt-in feature, following the guidance style used by ensureWindowsUnelevatedSetup. Preserve the existing immediate exit with status 1.
89-97: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHoist the principal lookup above the restricted-token SID computation.
capabilitySIDs,offlineSID,tokenSIDs, andwriteRestrictedare all computed unconditionally and discarded on the principal path. Moving thewindowsSandboxPrincipalTokencall to just after the network-policy validation makes the two backends read as a clean either/or and avoids the wasted SID resolution. (Only do this if the network-enforcement question above resolves in favor of keeping the principal path independent of those SIDs.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/sandbox/windows_command_runner_windows.go` around lines 89 - 97, Move the windowsSandboxPrincipalToken lookup and its success-path handling to immediately after network-policy validation, before computing capabilitySIDs, offlineSID, tokenSIDs, or writeRestricted. Keep the principal-token execution via runWindowsCommandAsUser unchanged, and ensure the restricted-token SID calculations run only on the fallback path.internal/sandbox/windows_identity_secret_windows.go (1)
139-166: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueConsider DPAPI for the on-disk secret. The ACL blocks other users, but the password is still stored in plaintext. If you want defense in depth against offline inspection or backup exposure, encrypt it with DPAPI before writing it.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/sandbox/windows_identity_secret_windows.go` around lines 139 - 166, Update writeWindowsSandboxSecret to protect the password with Windows DPAPI before persisting it, writing the encrypted bytes instead of plaintext while preserving the existing owner ACL and cleanup behavior. Reuse the repository’s existing DPAPI encryption helper if available; otherwise add the minimal Windows-specific encryption step and report encryption failures without writing the secret.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/sandbox/windows_command_runner_windows.go`:
- Around line 78-97: Update the principal execution branch in the Windows
command runner so deny-mode commands cannot bypass network isolation: either
make the WFP filter use the provisioned principal SID, or bypass the principal
path and continue through the restricted-token backend when NetworkDeny is
enabled. Ensure the existing windowsRuntimeTokenSIDs-based deny behavior remains
enforced.
In `@internal/sandbox/windows_identity_runtime_windows.go`:
- Around line 106-127: Update provisionWindowsSandboxPrincipalForSetup to reset
the password for existing principals before writeWindowsSandboxSecret persists
the credential. Reuse ensureWindowsSandboxUser’s existing account-handling
behavior or adjust the provisioning flow so nerrUserExists accounts receive the
newly generated password, while preserving fresh-account provisioning and
subsequent logon-rights setup.
In `@internal/sandbox/windows_identity_secret_windows_test.go`:
- Around line 181-196: Update windowsSecretACEList to inspect the generic
ACE_HEADER returned by GetAce before interpreting it as ACCESS_ALLOWED_ACE.
Accept only the supported allow-ACE type, and return a clear error for deny,
object, or any other unsupported ACE type so invalid SID offsets cannot be
decoded as trustees.
---
Nitpick comments:
In `@internal/sandbox/windows_command_runner_windows.go`:
- Around line 84-88: Update the error handling around
windowsSandboxPrincipalToken so the stderr message explains that the Windows
sandbox principal backend failed and gives the operator an actionable way to
disable or opt out of the opt-in feature, following the guidance style used by
ensureWindowsUnelevatedSetup. Preserve the existing immediate exit with status
1.
- Around line 89-97: Move the windowsSandboxPrincipalToken lookup and its
success-path handling to immediately after network-policy validation, before
computing capabilitySIDs, offlineSID, tokenSIDs, or writeRestricted. Keep the
principal-token execution via runWindowsCommandAsUser unchanged, and ensure the
restricted-token SID calculations run only on the fallback path.
In `@internal/sandbox/windows_identity_secret_windows.go`:
- Around line 139-166: Update writeWindowsSandboxSecret to protect the password
with Windows DPAPI before persisting it, writing the encrypted bytes instead of
plaintext while preserving the existing owner ACL and cleanup behavior. Reuse
the repository’s existing DPAPI encryption helper if available; otherwise add
the minimal Windows-specific encryption step and report encryption failures
without writing the secret.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 90fab087-5f05-4a9a-ae92-73e983828792
📒 Files selected for processing (4)
internal/sandbox/windows_command_runner_windows.gointernal/sandbox/windows_identity_runtime_windows.gointernal/sandbox/windows_identity_secret_windows.gointernal/sandbox/windows_identity_secret_windows_test.go
|
Validation update: the provisioning chain has now been run for real, elevated, on Windows 11. and the objects it created were really there, confirmed independently afterwards: Verified end to end: NetLocalGroupAdd, NetUserAdd, NetLocalGroupAddMembers and the SID lookup all succeed against the real APIs; a second provision returns the same username and SID, so the idempotent "already exists" handling is correct; and lookup finds what provisioning created. Notably there was no ERROR_PASSWORD_RESTRICTION, so the generated password satisfies the default complexity policy. That also means the hand-rolled USER_INFO_1, LOCALGROUP_INFO_1 and LOCALGROUP_MEMBERS_INFO_3 layouts marshal correctly, which matters because they are passed as raw buffers where a wrong field order fails or corrupts memory rather than erroring cleanly. Still not verified: that test exercises provisionWindowsSandboxIdentity only. LsaAddAccountRights (the batch-logon grant and the deny-interactive hardening) and LogonUser (minting the token) have still never executed, so the identity is proven to exist but not yet proven usable. CI cannot cover either, since it runs unelevated. Also still open: the provisioning entry points have no non-test callers yet. Keeping this a draft until the logon half is exercised too. |
|
Setup is wired now, so the feature is reachable end to end rather than inert.
Provisioning is folded into setup's existing rollback rather than each later failure path having to remember it, and the rollback revokes ACEs before deleting the account. Doing it the other way round would leave ACEs naming a SID that no longer resolves, which is the orphaned residue this model exists to avoid. Everything stays behind How to exercise it, on a machine where creating local accounts is acceptable: Validation status: provisioning (group, account, membership, SID, idempotency) is confirmed working elevated on Windows 11. The logon half now has a test, TestGrantLogonRightsAndMintPrincipalToken, which exercises LsaAddAccountRights and LogonUser and asserts the minted token's user SID is the principal rather than the caller. It has not been run yet; Smart App Control blocks freshly built unsigned binaries on the machine available to me, so it needs a box without that restriction. That is the last unproven primitive and the reason this is still a draft. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/sandbox/windows_identity_runtime_windows_test.go`:
- Around line 11-29: Make TestWindowsSandboxIdentityGating hermetic by clearing
windowsSandboxIdentityEnv from the process environment before running the table,
so the "absent" case cannot fall back to an externally set value. Restore the
original environment after the test using the standard test cleanup mechanism.
In `@internal/sandbox/windows_setup_windows.go`:
- Around line 38-64: Add coverage in the Windows sandbox setup tests for the
flow around runWindowsSandboxSetup: verify opt-out does not call
setupWindowsSandboxPrincipal, and verify an opt-in principal-setup failure still
invokes the existing ACL rollback. Use the test’s existing configuration and
rollback helpers, preserving current success and error behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bb64b652-8bb9-4259-8b0e-53533dd380cf
📒 Files selected for processing (3)
internal/sandbox/windows_identity_runtime_windows.gointernal/sandbox/windows_identity_runtime_windows_test.gointernal/sandbox/windows_setup_windows.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/sandbox/windows_identity_runtime_windows.go
|
Thanks, this was a useful pass. Went through all three. Network enforcement (the hedge on the second point) turned out to be the real finding. Chasing it down: Fixed in fb8e39b: the principal stands down whenever the network is denied and the restricted-token path runs instead. Keying the filters to the principal's own SID is the follow-up that lifts the restriction, and I would rather do that with the privileged paths validated on a clean box than bolt it on here. Worth flagging that my first regression test for this was worthless. It called Actionable error: taken. The message now names DPAPI: also taken, in deb3a98. The ACL is still the primary control and the thing that keeps the principal from reading its own credential, but you are right that it only binds while the filesystem is the one being asked, so a backup or a mounted image gives up the password in the clear. Hoisting the lookup above the SID computation: leaving it. Now that the principal path is gated on network mode, it is no longer independent of those SIDs, so the ordering earns its keep. Still unproven and called out in the description: |
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Verdict
Changes requested.
Two things drive that. The lookup path below discards a check you deliberately wrote, and it should be fixed regardless of what else happens. Separately, the privileged half of this change has never been executed by anyone, and account provisioning, logon-rights assignment and credential storage are not things I am willing to approve unrun, however sound the design reasoning is. Neither point is a criticism of the direction, which I think is right.
The design reasoning here is unusually clear, and the honesty about what has and has not been run is appreciated.
One practical note before anything else: the description opens by calling this a draft, but the pull request is not marked as a draft on GitHub, so it currently sits open for review and merge. Converting it would match your stated intent. Related, the Smoke jobs for macOS, Ubuntu and Windows, along with Zero Review, were still pending when I looked, so the CI signal you describe as the check for the wiring commit has not yet reported.
What I was able to verify. On macOS, make fmt-check, go build ./... and go vet ./... are clean, and the full suite passes at 82 packages with no failures. More usefully for a change of this shape, GOOS=windows go vet ./internal/sandbox/... exits cleanly and GOOS=windows go test -c compiles the test binary, which type-checks the roughly 1,500 lines of _windows.go that never compile on a non-Windows host. That is not execution, but it does confirm the Win32 call sites, struct definitions and build tags hold together across the whole addition.
I also mutated the ACL ordering to check the test does real work: reversing the entry order returned by buildWindowsPrincipalACLPlan fails TestPrincipalACLPlanEmitsDeniesBeforeAllows. The deny-before-allow invariant is genuinely asserted rather than only documented.
Two further things came back clean and are worth recording. Password generation draws 24 bytes from crypto/rand and encodes them with unpadded base32, giving roughly 120 bits with no modulo bias, and the fixed prefix covering the complexity classes is a reasonable approach. Account naming leaves 11 hex characters of the SHA-256 digest after the nine-character prefix, so 44 bits, which puts a birthday collision far beyond any plausible number of workspaces on one machine.
One substantive finding. lookupWindowsSandboxIdentity (internal/sandbox/windows_identity_windows.go:338-345) collapses every error from resolveWindowsSandboxSID into errWindowsSandboxIdentityUnavailable, which discards the deliberate check you wrote at lines 274-276 refusing a name that resolves to a non-user account.
The effect is that if zero-sbx-<hash> is squatted by a pre-existing local group or alias, resolveWindowsSandboxSID correctly refuses it, but the caller reads that refusal as "not provisioned" and windowsSandboxPrincipalToken (lines 73-76 of windows_identity_runtime_windows.go) falls back quietly to the restricted token. Your own description draws the line in the right place, that only a provisioned-but-unusable identity should surface an error, and this is precisely that case reaching the operator as silence. Distinguishing ERROR_NONE_MAPPED from other lookup failures would preserve the fallback for the common "setup has not run" case while surfacing the rest.
A smaller one: the comment at windows_identity_windows.go:122 refers the reader to sandboxRuntimeKey for how the workspace key is hashed, but no such symbol exists. The function is windowsSandboxWorkspaceKey in windows_identity_runtime_windows.go:44.
On the question you raised for decision. Creating real local accounts being visible to endpoint protection, enterprise policy and net user seems worth settling before this leaves draft, and I agree it is a product call rather than a design flaw. The inversion argument is persuasive on its merits: unreachable by construction is a stronger boundary than an enumerated deny list, and the trustee-keyed revocation answers a real gap.
Limitations of this review. I have no Windows host and no elevated session, so NetUserAdd, LsaAddAccountRights, NetUserDel and LogonUser are unexecuted by me as well. I did not check the raw Win32 struct layouts against the SDK, and I did not review the LSA byte-versus-rune length handling beyond confirming it compiles. Everything above rests on reading the code and on cross-compilation.
Worth flagging for coordination: this addresses the same credentialDenyReadPaths weakness on Windows that I raised on #801, where removing the sandbox HOME and XDG_CONFIG_HOME overrides makes real credential locations the resolution target. The two changes point at the same boundary from opposite sides and would benefit from being sequenced deliberately.
Merge is kevin's call per the program gate.
|
CI is green now. The Windows smoke failure was not from this branch, and it is worth saying what it actually was rather than just re-running until it passed. Three tests failed, all in Fixes are up separately rather than folded in here, since they have nothing to do with the sandbox work and one of them touches product code:
I also opened #811 for something that fell out of the reproduction and is a genuine user-facing bug rather than a test problem: the provider-command timeout is a floor, not a bound. Process creation happens before the timer is armed and the drain after Nothing on this branch changed for any of that. Once #809 and #810 land I will rebase this one. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
internal/sandbox/windows_identity_runtime_windows_test.go (1)
11-22: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTable is still not hermetic.
The
"absent"case falls through toos.Getenv, so this test fails on any machine that actually hasZERO_WINDOWS_SANDBOX_IDENTITY=1exported — precisely the machines doing the elevated validation runs for this PR. Addt.Setenv(windowsSandboxIdentityEnv, "")before the table.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/sandbox/windows_identity_runtime_windows_test.go` around lines 11 - 22, Make TestWindowsSandboxIdentityGating hermetic by setting windowsSandboxIdentityEnv to an empty value with t.Setenv before iterating over the test cases, ensuring the "absent" case cannot inherit the host environment.internal/sandbox/windows_identity_secret_windows_test.go (1)
183-198: 🎯 Functional Correctness | 🟡 Minor | 💤 Low valueStill assumes every ACE is an
ACCESS_ALLOWED_ACE.
GetAcereturns a genericACE_HEADER; a deny or object ACE would put the SID at a different offset and this helper would decode garbage, making the "unexpected trustee" assertion misleading rather than failing cleanly. Gate onace.Header.AceType != windows.ACCESS_ALLOWED_ACE_TYPEand return an error.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/sandbox/windows_identity_secret_windows_test.go` around lines 183 - 198, The windowsSecretACEList helper must validate each ACE type before interpreting its SID layout. After GetAce returns, check ace.Header.AceType and return an error for any type other than windows.ACCESS_ALLOWED_ACE_TYPE; only then cast to ACCESS_ALLOWED_ACE and copy the SID.internal/sandbox/windows_identity_acl.go (1)
85-92: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winPath traversal via
ProtectedMetadataNamesstill unaddressed.
filepath.Join(cleaned, name)accepts../separator-bearing values, so a malformedProtectedMetadataNamesentry can materialize a deny ACE outsideroot.Root. This was flagged in a prior review and is still present with no validation added.🔒 Proposed fix
for _, name := range root.ProtectedMetadataNames { + if name == "" || name == "." || name == ".." || filepath.Base(name) != name { + return WindowsACLPlan{}, fmt.Errorf( + "windows principal ACL plan: invalid protected metadata name %q", name, + ) + } entries = append(entries, WindowsACLEntry{ Action: WindowsACLDenyWrite, Path: filepath.Join(cleaned, name),Add a regression test in
windows_identity_acl_test.gocovering a traversal/separator-bearing name once this validation lands. As per coding guidelines,**/*_test.go: "add regression tests for behavior changes."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/sandbox/windows_identity_acl.go` around lines 85 - 92, Validate each entry from root.ProtectedMetadataNames before constructing the WindowsACLEntry, rejecting traversal or separator-bearing names that could escape cleaned/root.Root; only append entries for safe metadata names. Add a regression test in windows_identity_acl_test.go covering both traversal and separator-bearing input.Source: Coding guidelines
🧹 Nitpick comments (1)
internal/sandbox/windows_identity_windows.go (1)
196-205: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
runtime.KeepAliveinstead of a deferred no-op.
defer func() { _ = info }()does keepinfoalive (the closure captures it), but it reads as dead code and a future cleanup will delete it, silently reintroducing a use-after-free window. The same pattern repeats at Lines 239 and 262.♻️ Proposed change
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 }() + // Keep info (and the Go strings it points at) alive across the call. + runtime.KeepAlive(info) return netAPIStatus("NetLocalGroupAdd", status, nerrGroupExists, errorAliasExists)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/sandbox/windows_identity_windows.go` around lines 196 - 205, Replace the deferred no-op keeping info alive in the NetLocalGroupAdd call with runtime.KeepAlive(info) after the syscall returns. Apply the same change to the corresponding patterns around the related calls at Lines 239 and 262, and add the runtime import if needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/sandbox/windows_identity_logon_windows.go`:
- Around line 108-154: The native Windows calls need explicit GC liveness
guarantees for all borrowed arguments. In grantWindowsSandboxLogonRights, add
runtime.KeepAlive for attributes after procLsaOpenPolicy.Call and for entry
after procLsaAddAccountRights.Call, while retaining the buffer keep-alive; also
update the LogonUserW call site to keep the user, domain, and secret pointers
alive after the call returns.
In `@internal/sandbox/windows_identity_runtime_windows.go`:
- Around line 139-145: Update the Windows sandbox identity flow around
ensureWindowsSandboxUser and writeWindowsSandboxSecret so a pre-existing
account’s password is actually synchronized before writing the secret. Remove
the inaccurate claim that the caller resets the password, and ensure the stored
secret matches the account password for both new and existing users.
In `@internal/sandbox/windows_identity_secret_windows.go`:
- Around line 186-196: Update readWindowsSandboxSecret to map permission-denied
errors, including Windows ERROR_ACCESS_DENIED, to
errWindowsSandboxIdentityUnavailable alongside missing-file errors so callers
fall back to the restricted token. Update removeWindowsSandboxSecret to treat
the same unreadable or inaccessible-secret condition as non-fatal, allowing
principal cleanup to continue while preserving other error propagation.
In `@internal/sandbox/windows_identity_windows.go`:
- Around line 213-241: The existing-user path in ensureWindowsSandboxUser must
reset the account password via NetUserSetInfo at level 1003 using USER_INFO_1003
before returning success; update internal/sandbox/windows_identity_windows.go
lines 213-241 accordingly while preserving normal creation behavior. In
internal/sandbox/windows_identity_runtime_windows.go lines 139-145, revise the
related comment to accurately describe that ensureWindowsSandboxUser performs
the password reset.
---
Duplicate comments:
In `@internal/sandbox/windows_identity_acl.go`:
- Around line 85-92: Validate each entry from root.ProtectedMetadataNames before
constructing the WindowsACLEntry, rejecting traversal or separator-bearing names
that could escape cleaned/root.Root; only append entries for safe metadata
names. Add a regression test in windows_identity_acl_test.go covering both
traversal and separator-bearing input.
In `@internal/sandbox/windows_identity_runtime_windows_test.go`:
- Around line 11-22: Make TestWindowsSandboxIdentityGating hermetic by setting
windowsSandboxIdentityEnv to an empty value with t.Setenv before iterating over
the test cases, ensuring the "absent" case cannot inherit the host environment.
In `@internal/sandbox/windows_identity_secret_windows_test.go`:
- Around line 183-198: The windowsSecretACEList helper must validate each ACE
type before interpreting its SID layout. After GetAce returns, check
ace.Header.AceType and return an error for any type other than
windows.ACCESS_ALLOWED_ACE_TYPE; only then cast to ACCESS_ALLOWED_ACE and copy
the SID.
---
Nitpick comments:
In `@internal/sandbox/windows_identity_windows.go`:
- Around line 196-205: Replace the deferred no-op keeping info alive in the
NetLocalGroupAdd call with runtime.KeepAlive(info) after the syscall returns.
Apply the same change to the corresponding patterns around the related calls at
Lines 239 and 262, and add the runtime import if needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4be32672-966b-47b1-955b-a7e02d7e5891
📒 Files selected for processing (13)
internal/sandbox/windows_acl_apply_windows.gointernal/sandbox/windows_command_runner_windows.gointernal/sandbox/windows_identity_acl.gointernal/sandbox/windows_identity_acl_test.gointernal/sandbox/windows_identity_dpapi_windows.gointernal/sandbox/windows_identity_logon_windows.gointernal/sandbox/windows_identity_runtime_windows.gointernal/sandbox/windows_identity_runtime_windows_test.gointernal/sandbox/windows_identity_secret_windows.gointernal/sandbox/windows_identity_secret_windows_test.gointernal/sandbox/windows_identity_windows.gointernal/sandbox/windows_identity_windows_test.gointernal/sandbox/windows_setup_windows.go
|
Thanks, this is a good review, and the lookup finding is right. The squatted-name case. Fixed in 9e1e651. You are right that it lands exactly where the description says the line should sit, and I had written the check and then thrown it away one call later. It was worse than the one site you found: The decision sits in its own function rather than inline, 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 that classifier with a real error from a well-known local group, needs no privilege, and I checked it fails if the old collapse-everything behaviour is restored: The stale comment. Fixed, it is The draft framing. That was stale and I have rewritten the opening. This is not a draft: it is opt-in behind an environment variable and I would rather it be reviewed than sit hidden. The provisioning half has since been run on a real elevated session, so account and group creation are no longer unexecuted. CI. It has reported since, and is green on all nine checks. Three Windows tests did fail on the first run, none of them in code this branch touches. I reproduced two of them locally under CPU contention on a clean tree, so they were pre-existing flakes rather than anything here; they are fixed in #810 and #809, and #811 covers a genuine product bug that fell out of the reproduction. On sequencing with #801. Agreed, and worth being concrete: these do point at the same boundary from opposite sides. #801 removes the sandbox Also worth flagging for the same reason: this backend currently stands down whenever the network is denied, which is the default. WFP filters key on the offline-marker SID and a The two things you verified that I could not, the cross-compiled vet and |
|
Both taken, and the first one was a real bug rather than a documentation slip. The pre-existing account. You are right, and the effect is worse than the comment being wrong. Fixed in e33dce0. The gated provisioning test now provisions twice and logs on with the password from the second run. That is the only assertion worth having here: a stale password is indistinguishable from a correct one until something actually authenticates with it, so checking that the two runs return the same identity would have passed straight through this bug. The keep-alives. Also taken.
On the |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/sandbox/windows_identity_windows_test.go`:
- Around line 239-246: After provisioning the test principal in the gated
identity test, register a t.Cleanup callback that revokes SeBatchLogonRight and
removes the test principal, ensuring cleanup runs on every subsequent failure
path. Keep the existing grantWindowsSandboxLogonRights and
logonWindowsSandboxPrincipal flow unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d03dfa6a-7671-40c4-b4c8-5d77781ed16c
📒 Files selected for processing (4)
internal/sandbox/windows_identity_logon_windows.gointernal/sandbox/windows_identity_runtime_windows.gointernal/sandbox/windows_identity_windows.gointernal/sandbox/windows_identity_windows_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/sandbox/windows_identity_logon_windows.go
- internal/sandbox/windows_identity_runtime_windows.go
- internal/sandbox/windows_identity_windows.go
|
Taken, and it was pointing at more than the test. You are right that the round trip left residue: it granted a real batch logon right to a real local account and had no cleanup at all, so anyone running the gated suite kept both. That is on me, and it got worse when I added the logon step in the last commit. The part worth flagging is that the same hole was in the production teardown. Fixed in fbe340b:
One thing I did not want to take on trust. Treating "this account holds no rights" as success depends on
|
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Verdict
Approve.
Reviewed at fbe340b3995c, base ac50a5a840d2, re-confirmed against the live head before posting.
I withdraw both findings from my previous review. Each is fixed, and the first is fixed in the way I hoped rather than the cheapest way.
lookupWindowsSandboxIdentity no longer collapses every lookup failure into "not provisioned". classifyWindowsSandboxLookupError (internal/sandbox/windows_identity_windows.go) maps ERROR_NONE_MAPPED to errWindowsSandboxIdentityUnavailable and returns everything else unchanged, so the deliberate refusal in resolveWindowsSandboxSID for a name resolving to a non-user account now reaches the operator instead of degrading quietly to the restricted token. TestLookupWindowsSandboxIdentityRejectsNonUserAccount covers exactly that case. The sandboxRuntimeKey comment now names windowsSandboxWorkspaceKey, which exists.
On the execution question, which was my other reason for requesting changes. The position has changed materially. Account and group provisioning have now been run on a real elevated session, the description says so precisely, and all three Smoke jobs plus Zero Review are passing, including windows-latest. The logon half — LsaAddAccountRights and LogonUser — remains unexecuted, and the description says that too, in those words.
I am approving with that gap open rather than in spite of it, for two reasons. The whole surface is behind ZERO_WINDOWS_SANDBOX_IDENTITY=1 and off by default, so no existing install changes behaviour. And the disclosure is accurate and specific rather than implied, which is the standard the review protocol asks for. An unrun privileged path that nobody reaches without opting in, declared plainly, is a reasonable posture for foundation work.
On the new material in this delta. The DPAPI wrapping is well-judged. CRYPTPROTECT_UI_FORBIDDEN is the right flag for a path that may run without an interactive desktop, the LocalFree of the DPAPI-allocated output is correctly deferred, and the ciphertext is copied out rather than aliased. I checked the one thing that looked like a documentation mismatch and it was not: the comment says the principal name is the entropy, and windowsSandboxSecretEntropy derives it from the secret's own filename, which is the principal name — so read and write agree by construction, as the comment claims.
Resetting the password when the account already exists is a real bug fix rather than a refinement. NetUserAdd leaves an existing account untouched, so without NetUserSetInfo the stored secret would not have been the account's password, and the failure would have surfaced much later as an unexplained logon failure. Revoking logon rights before deleting the principal, and keeping the restricted token when the network is denied, are both correct orderings.
Two smaller things came back clean and are worth recording. Replacing defer func() { _ = info }() with runtime.KeepAlive is the correct idiom — the deferred closure did not reliably keep the pointed-to Go memory alive across the syscall, and KeepAlive does. And the KeepAlive calls were added for name and comment as well, not only the struct.
Verification. On macOS, go build ./..., go vet ./... and gofmt -l are clean and the suite passes. More usefully for this change, GOOS=windows go vet ./internal/sandbox/... exits 0 and GOOS=windows go test -c compiles, which type-checks the entire Windows surface including the new DPAPI file. That is not execution, but it confirms the Win32 call sites, struct definitions and build tags hold together across the whole addition.
Limitations. I have no Windows host and no elevated session. LsaAddAccountRights, LogonUser, CryptProtectData and NetUserSetInfo are unexecuted by me. I did not check the raw struct layouts against the SDK beyond confirming the existing layout tests still pass.
This does not clear CodeRabbit's outstanding review, and #812 is stacked on this branch, so landing order matters.
Merge is kevin's call per the program gate.
|
Both findings are correct. I checked each against the head before agreeing, and neither is a misreading. Fixed in 6ccf4cf. 1, the account takeover. Confirmed. Ownership is now read back from the comment provisioning stamps before anything is touched, and a name held by an account Zero did not create fails with a typed The irony is not lost on me. I added exactly this guard to the deletion path in the follow-up PR after CodeRabbit raised deleting-by-derived-name, and did not think to look at the adoption path, which is the more dangerous of the two. Deleting the wrong account is loud. Resetting its password and quietly running as it is not. 2, the partial-failure residue. Also confirmed, and your description of why is precise: the rollback is only constructed after Provisioning now unwinds what the run actually did, in reverse, on every failure path, tracking the four things you listed. One deliberate difference from your list, worth stating because it is a judgement rather than an oversight. Cleanup is scoped to what THIS run created. 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 the pre-existing 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 to the restricted token rather than failing. If you think that is the wrong call I will change it. 3, the unexecuted LogonUser path. Agreed, and I have said so in the description since the start rather than being talked into it. It is the central runtime path and it has not run end to end on an elevated machine. Smart App Control on my box blocks freshly built unsigned binaries, which is exactly the class of binary the gated provisioning test produces. I am not going to claim that as verified, and I do not think opt-in gating substitutes for running it. You also asked for a test with an unrelated existing account on the derived name. Added, driven against
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/sandbox/windows_identity_runtime_windows.go`:
- Around line 146-190: Update the provisioning cleanup flow around the undo
closure and grantWindowsSandboxLogonRights call: compute secretPath immediately
after identity provisioning succeeds, before granting logon rights, and remove
the secretWritten condition so undo removes any resolved secret path on
subsequent failure. Preserve the existing no-op behavior when secretPath is
empty and keep successful secret writing unchanged.
In `@internal/sandbox/windows_identity_windows.go`:
- Around line 389-432: Update setupWindowsSandboxPrincipal to remove the
existing Windows sandbox secret when provisioning succeeds in changing or
reusing an account but setup fails before writeWindowsSandboxSecret. Ensure the
rollback error path deletes the stale .secret file, while preserving the normal
secret write and unrelated provisioning error behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 24ed1d1c-eaa0-4d51-843c-1e1a7a825854
📒 Files selected for processing (3)
internal/sandbox/windows_identity_runtime_windows.gointernal/sandbox/windows_identity_windows.gointernal/sandbox/windows_identity_windows_test.go
|
Both findings are the same hole seen from two angles, and you are right: the cleanup I added missed the one window it was written for. Provisioning always sets the account's password, including resetting a pre-existing owned account's, so from the moment it returns the stored secret is already stale. My Fixed in 832f53a: the path is resolved from the account name before anything can fail, and removal is unconditional rather than gated on Worth naming the pattern, since this is twice now on this PR. The takeover fix and this one are both cases where I reasoned correctly about what should happen and then wrote a condition that did not cover the case I was reasoning about. Reading the comment I had written would have told you the intended behaviour; only reading the code shows it did not happen.
|
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Verdict
Approve.
Reviewed at 832f53a98d74, base 5d1869e, re-confirmed against the live head before posting. My earlier approval at fbe340b was dismissed by the push; this replaces it, and the new work is strictly better.
The two commits since then are both real improvements, not polish.
windowsSandboxUserIsManaged closes a hazard that was live in the version I approved. The account name is derived from a workspace hash rather than discovered, so it can be occupied by an account with nothing to do with Zero — and provisioning would previously have adopted it and reset its password. Reading back the comment stamp before adopting, and refusing with a named error otherwise, is the right shape, and the same predicate is reused on the delete path in #812. Dropping the stored secret when provisioning fails closes the matching half: a secret file that no longer corresponds to any account is worse than none, because it looks provisioned.
One substantive finding, non-blocking, on the adoption gate.
provisionWindowsSandboxIdentity proves ownership using the comment field alone. It does not inspect the adopted account's group memberships. An account named zero-sbx-<hash>, carrying Zero's comment, and also a member of Administrators would pass the gate: Zero resets its password, adds it to the sandbox group, and mints principal tokens for it. The sandboxed child then runs as an administrator, which inverts the property this whole design rests on — your description's argument is that a separate account has no access to the caller's profile by construction, and an adopted account with extra memberships is precisely the case where that stops being true by construction.
I want to be fair about reachability: planting such an account requires administrator rights already, so this is not fresh escalation. It is a persistence and laundering path — something that had admin once leaves a stamped account behind, and Zero thereafter grants it sandbox duty on every run — and it is also the shape a botched or partial earlier provisioning could leave behind on its own. Given that the model's selling point is a boundary that holds by construction, asserting the adopted account's memberships (at minimum, that it is not in Administrators) rather than only its comment would make the claim true rather than nearly true. A comment is a stamp, not a capability check.
What I verified. On macOS: gofmt, go build ./..., go vet ./... clean, suite passing. GOOS=windows go vet ./internal/sandbox/... exits 0 and GOOS=windows go test -c compiles, which type-checks the whole Windows surface including the two new netapi32 procs and the USER_INFO_1 read-back. That is type-checking, not execution.
Limitations, unchanged and still the main thing a reader should weigh. I have no Windows host and no elevated session. NetUserGetInfo, NetApiBufferFree, NetUserSetInfo, LsaAddAccountRights and LogonUser are unexecuted by me. Your description remains accurate about which halves you have run, and that accuracy is why I am comfortable approving with the logon path still unrun: the feature is behind ZERO_WINDOWS_SANDBOX_IDENTITY=1 and off by default, so nothing changes for an existing install.
CodeRabbit's changes-requested from 08:17 is still outstanding and is separate from this.
Merge is kevin's call per the program gate.
832f53a to
99fefdc
Compare
anandh8x
left a comment
anandh8x
left a comment
There was a problem hiding this comment.
Review at 99fefdc
PR #808 — Windows sandbox principals (foundation for #662). 14 files, +2559, 12 commits, all new *_windows.go files (build-constrained) except windows_identity_acl.go which is pure-Go ACL-plan logic that compiles on all platforms. Opt-in behind ZERO_WINDOWS_SANDBOX_IDENTITY=1.
Verdict: approve. The design is sound, the fail-soft contract is right, and the honest caveats are the right ones.
What this does
Gives the sandbox its own identity on Windows: a separate local account per workspace in one managed group. This inverts the read-confinement problem — instead of trying to deny the caller's own account (which locks Zero out too), a separate account has no access to the caller's profile by construction, so credential stores are unreachable without enumerating deny rules.
What's good
- The inversion is the right design. Every other Windows backend derives its token from the calling user via
CreateRestrictedToken, which is whycredentialDenyReadPathsis a no-op on Windows. A separate account makes "what to GRANT" the interesting question instead of "what to DENY," and the same SID keys write grants and firewall rules. - Fail-soft contract is correct. No provisioned account, no stored secret, or opt-in off →
ok=false, nil error, restricted-token backend runs unchanged. Only a provisioned-but-unusable identity surfaces an error (broken sandbox, not absent sandbox). The runner integration (windows_command_runner_windows.go) is a clean 25-line addition that tries the principal first and falls back. - Network-denial tradeoff is honest. A principal token from
LogonUsercan't carry the offline-marker SID that WFP filters key on, so the principal stands down when the network is denied and the restricted-token path runs instead. The PR explicitly says "trading network denial for read confinement would have been the wrong way round." Keying filters to the principal's own SID is the named follow-up. - Provisioning is idempotent. "Already exists" statuses are success. Re-running
zero sandbox setupconverges instead of accumulating accounts. Password is reset on re-provisioning so the stored secret stays in step with the account. - Squat protection.
windowsSandboxUserIsManagedreads back the comment stamp before adopting an existing account. Refuses with a named error (errWindowsSandboxNameCollision) if the name is taken by a non-Zero account. This closes the "reset a stranger's password" hazard. - Secret storage is layered. DACL naming only the invoking user + SYSTEM, applied to an empty file before the password is written (bytes never exist under inherited permissions),
SE_DACL_PROTECTEDso inherited ACEs can't reach it, plus DPAPI (CryptProtectData) encryption with the principal name as entropy so a blob copied to another path fails to decrypt. The testTestStoredSecretDACLNamesOnlyOwnerAndSystemreads the DACL back and fails if any other trustee appears; another assertsSE_DACL_PROTECTED. - ACL plan is deny-before-allow. Carve-outs survive Windows DACL evaluation order. Trustee-keyed revocation drops every ACE naming the principal without needing a record of what was granted — the cleanup path the capability-SID model lacks.
- Rollback is thorough.
provisionWindowsSandboxPrincipalForSetupcomputessecretPathearly (before anything can fail), the undo closure removes the secret unconditionally ("provisioning has already replaced the account's password by the time any of this can fail, so whatever is on disk cannot authenticate"), andsetupWindowsSandboxPrincipalcallsremovePrincipal()on ACL-plan failure, which removes secret → logon rights → account in that order. - Logon rights are least-privilege. Only
SeBatchLogonRightgranted; interactive, network, remote-interactive, and service logon explicitly denied.LogonUserpinned to"."so a same-named domain account is never picked up. - Platform separation is clean.
windows_identity_acl.go(plan logic, no build tag, compiles everywhere, testable on Linux) vs*_windows.go(syscall execution, build-constrained). Cross-compile forGOOS=windowsclean;GOOS=windows go test -ctype-checks the full Windows surface includingnetapi32procs andUSER_INFO_1layout.
Verification performed
GOOS=windows go vet ./internal/sandbox/...— cleanGOOS=windows go test -c— compiles (type-checks all Windows-specific code)go build ./internal/sandbox/...(Linux) — cleango test ./internal/sandbox/(Linux, from non-/tmppath) — pass, all 14 tests greengo vet ./internal/sandbox/...— clean
CodeRabbit's findings are addressed
CodeRabbit's latest CHANGES_REQUESTED (08:17Z) asked for (1) computing secretPath before granting logon rights and removing the secretWritten condition, and (2) removing the stale .secret file when provisioning succeeds but setup fails before writeWindowsSandboxSecret. Both are addressed by commits 99fefdc and 52f843a (pushed 09:46Z, after the review). The undo closure now computes secretPath early and removes it unconditionally; setupWindowsSandboxPrincipal's rollback calls removePrincipal() which removes the secret first.
gnanam's non-blocking finding (acknowledged, not blocking)
gnanam's APPROVED review notes that the adoption gate (windowsSandboxUserIsManaged) checks the comment field alone, not the account's group memberships. An account named zero-sbx-<hash> with Zero's comment but also in Administrators would pass the gate. gnanam correctly frames this as a persistence/laundering path (not fresh escalation, since planting requires admin already). The fix — asserting the adopted account is not in Administrators — is a reasonable follow-up but not a blocker given the opt-in gate and the admin prerequisite for exploitation.
Honest caveats (from the PR description, still accurate)
- The logon half is unproven.
NetUserAdd,LsaAddAccountRights,LogonUserneed elevation; they compile and are layout-checked but haven't run to completion (Smart App Control blocked the test binary). The provisioning round-trip test is gated behindZERO_WINDOWS_IDENTITY_PROVISION_TEST=1plus an elevation check. - Creating real local accounts is user-visible. AV/EDR commonly flag
NetUserAdd; enterprise policy often blocks local account creation; accounts appear innet userand Settings. The opt-in gate makes this a deliberate call.
These are the right caveats for a foundation PR. The feature is off by default; nothing changes for an existing install.
Verdict
Approve. The design inverts the Windows read-confinement problem correctly, the fail-soft contract is sound, the rollback paths are thorough, and the honest caveats are the right ones. gnanam's non-blocking finding (membership check on adoption) is worth a follow-up. CodeRabbit's two actionable findings are addressed by the latest commits. Ready for kevin to merge.
|
@jatmn @anandh8x @gnanam1990 @kevincodex1 ready for another look. Head is Since your last review:
Still open, and I would rather say it than have you find it:
anandh8x, that second point is your gate and it is still genuinely open. Everything you asked for on the code side is in. Happy for you to keep changes requested on the evidence alone. |
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Verdict: Approve (reaffirmed on cb9263ac)
Re-reviewed the six commits added since my approval (38cb642e..cb9263ac). They're all follow-through fixes, each with a regression test, and they strengthen the PR. All CI is now green — including Windows smoke (6m53s), so the earlier red was the internal/config process-termination flake as suspected, not this PR.
The one security-critical commit — 5fc3d7604 (read-capability SID) — is correct
A DenyRead profile drops WRITE_RESTRICTED and runs the restricted-SID check over reads, but read roots were granted only to the principal's account SID, which the write jail deliberately keeps out of the restricting set — so under the strict token every read failed, down to the executable. The fix mints a synthetic ReadAllow SID, grants it on read roots, and carries it in the strict token's restricting set. I checked the parts that could go wrong:
- Carveouts stay unreadable. Read roots begin at the filesystem root, so the broad grant could re-expose a
DenyReadcarveout — butwindowsReadDenyCapabilitySIDsnow appendsreadSIDto the deny set on everyDenyReadpath, and an explicit deny on the carveout beats the inherited allow from the read root by canonical ACL ordering.TestDenyReadCoversTheReadCapabilitySIDpins it. - Grant and restrict can't drift. Both gates decide from the identical profile expression — the grant via
windowsReadAllowCapabilitySID(len(DenyRead)==0) and the token viawriteRestricted := len(DenyRead)==0(windows_command_runner_windows.go:80). Invariant #5 satisfied: one decision, not two. - No new exposure.
ReadAllowis synthetic and held by nobody; a restricting SID can only narrow a token, so publishing the grant hands out nothing to real users. The non-principal strict path (windowsRuntimeTokenSIDsexcludesreadSID) is inert — it had no read-root grant before either, so no regression.
The runtime-root fixes are bugs found by running it for real
61cc460af— putting both runtime candidates in the capability plan added a write root nothing creates, so elevated setup failed on a missing path. Now every candidate is created setup-side before the granting plan, withTestWindowsSandboxSetupProvisionsEveryGrantedWriteRootwalking the plan so choose-vs-create can't drift.c355e303b— the marker fingerprint diverged because the runner inherits the sandbox's repointedTMP/TEMPwhile setup sees the operator's, soos.TempDir()produced two different temp roots (same count, different hash) and every command failed validation. The fingerprint is now derived only whereTEMPis still the operator's. This is precisely the class of bug you only hit by running the elevated setup + a sandboxed command end-to-end — which resolves the empirical-verification concern behind my original hold.1a365fbec— the marker-rejection error now reports the path, both counts, and both hashes instead of an undebuggable one-liner.8e02817d5— fixes a vacuous jail-aliasing test (the old assertion appended a sentinel then ranged the caller's slice, which append's new header can never grow — it passed against every implementation, including one returning the slice verbatim). Now reads the backing array.
Verified
GOOS=windows go build ./...+go vet ./internal/sandbox/clean; darwin build/vet/gofmtclean.internal/sandbox+internal/doctorsuites pass locally; all CI green (macOS/ubuntu/windows smoke, CodeRabbit, Zero Review).
Blocking: none. Non-blocking notes from my prior review still stand (8-byte runtime-root hash truncation; reliance on default BUILTIN\Users read ACLs — both documented in-code). The remaining gate is @anandh8x's outstanding review, not code.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found additional issues that need to be addressed before this is ready.
Findings
-
[P1] Do not resolve the attacker-controlled path when checking for ancestor junctions
internal/sandbox/windows_acl_reparse_windows.go:49
actualis the final path reached by the handle, butexpectedcomes fromcanonicalSandboxWorkspaceRoot(path), which callsEvalSymlinks. Withworkspace/.gitreplaced by a junction to an outside directory, both values therefore resolve to that same outside directory and the comparison succeeds. Elevated setup can still rewrite a DACL outside the approved tree, andTestOpenWindowsACLTargetRefusesAJunctionAncestorshould fail whenever itsmklinkprecondition actually runs (it can skip in CI). Compare against a no-follow/pinned component walk or another trusted lexical root instead of resolving the suspect path and treating the result as expected. -
[P1] Bind the elevated secret write to a no-follow handle
internal/sandbox/windows_identity_secret_windows.go:144
This elevated path usesMkdirAll, opens and closes the live pathname withO_TRUNC, changes its DACL by pathname, then reopens it withWriteFile. The sandbox home is owned by the invoking user, so a pre-placed directory junction or an enabled unprivileged symlink—and a leaf swap between those calls—can redirect the Administrator process outside that tree. In the symlink case it can truncate and replace the DACL of an attacker-selected Administrator-accessible file; even the junction-only case lets the caller plant the deterministic secret file and grant itself control in an otherwise inaccessible directory. Keep directory traversal and leaf creation no-follow beneath a pinned parent and perform the ACL/write through the verified handle, as the ACL applier in this PR already does. The documented missing-secret fallback is intentional, so this finding is about the elevated pathname escape, not a request to change that policy. -
[P1] Use a process-creation contract that works for the alternate account
internal/sandbox/windows_command_runner_windows.go:153
internal/sandbox/windows_process_windows.go:36
The ordinary command runner passes a different account'sLogonUsertoken toCreateProcessAsUser. Microsoft documents that this normally requiresSE_INCREASE_QUOTA_NAMEand can requireSE_ASSIGNPRIMARYTOKEN_NAME; the only documentedSE_ASSIGNPRIMARYTOKEN_NAMEexemption is for a restricted version of the caller's own primary token, not this separate account, and the runner neither establishes nor checks those privileges. The same call also forceswinsta0\default, but setup never grants the batch account access to that window station and desktop even though the API requires both DACLs to admit the target user/logon session. Thus a principal command can fail withERROR_PRIVILEGE_NOT_HELDorACCESS_DENIEDbefore its executable runs. Please choose an alternate-account launch mechanism/broker usable from the unelevated runner and an appropriate noninteractive desktop (or explicitly manage and revoke the required DACL grants), then exercise that exact path. See the CreateProcessAsUser contract. -
[P1] Build the environment and profile for the principal token
internal/sandbox/windows_runner.go:335
The explicit environment starts as the invoking user's environment and the later overrides changeHOME, temp/cache variables, and a few system variables only.USERPROFILE,APPDATA,LOCALAPPDATA,HOMEDRIVE,HOMEPATH,USERNAME, and related values still identify the caller, whose profile the separate principal is intentionally unable to access;CreateProcessAsUserneither adapts the supplied block nor loads that user's HKCU profile. Native Windows tools can consequently fail during startup/config discovery or observe an identity inconsistent with their environment even if process creation succeeds. Construct/load an environment and profile for the target token, then layer the deliberate sandbox redirects over it, and assert the resulting profile/known-folder paths are usable and do not point at the caller. -
[P1] Handle Git worktree and submodule gitfiles before materializing carve-outs
internal/sandbox/windows_identity_acl.go:110
The principal plan now makes every.git/configand.git/hookstarget mandatory and materializes it by descending through.gitas a directory. In a linked worktree or a common submodule layout,.gitis a regulargitdir:file, so the handle-relative directory walk cannot open/create a child beneath it and opted-in elevated setup aborts. Resolve the actual Git directory (or otherwise handle gitfiles explicitly) before planning/materialization and cover a real linked-worktree layout; the related open #805 addresses this same repository-layout class elsewhere but does not fix this head. -
[P2] Retain the trustee SID when revocation fails
internal/sandbox/windows_identity_runtime_windows.go:495
Teardown keeps the ACL ledger after a revoke error but still deletes the account. The ledger schema stores only paths, not the deleted account's SID, so the claimed retry is impossible: a later setup creates a new RID, attempts revocation with that new SID, and then overwrites the only record of where the old raw-SID ACEs remain. Keep the account until revocation succeeds or persist the exact retired SID with its paths and explicitly revoke that trustee before narrowing/replacing the ledger; cover revoke failure followed by reprovisioning.
A linked worktree or submodule has .git as a FILE holding a `gitdir:` pointer, not a directory. The principal plan names .git/config and .git/hooks and materializes both, and the Windows materializer gets there by descending through .git as a directory. A regular file cannot have children, so opted-in elevated setup aborted and the sandbox could not be used in a worktree at all. Zero's own development worktrees are this shape, which is how it went unnoticed. Deny the pointer file itself there instead. That is the stronger protection rather than a fallback: a principal able to rewrite `gitdir:` repoints the repository at a control directory of its choosing, which subsumes editing config or planting a hook. The real control directory sits outside the write root, so nothing is inherited there and no carveout is needed. Decided by an Lstat rather than a lexical guess, since the layout is a property of the checkout. An absent .git keeps the directory-shaped carveouts so they are still created before git first runs.
|
@jatmn worked the first two. One is fixed, one I could not reproduce and I want to put the measurement in front of you rather than argue from reading. P1 gitfile: fixed in P1 ancestor junction: I cannot reproduce this one. The Your underlying point holds for a real symlink, where both sides would resolve and agree. I could not demonstrate that either: So as written I think the severity is off, and I did not want to rewrite
The remaining four are untouched so far. On the process one, @gnanam1990 sorry, the push dismissed your approval on |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Reject redirected ancestors without resolving the requested path
internal/sandbox/windows_acl_reparse_windows.go:49
Author context: your latest test correctly shows that the static junction case is rejected on your machine. This finding is about a different case: a directory symlink (available to an unprivileged user with Developer Mode) or a path swap during setup.The check resolves the requested path before comparing it with the opened handle. In the symlink case, both resolve to the same outside directory, so the check passes and elevated setup can change ACLs outside the workspace. Walk the path with no-follow handles, and add a directory-symlink/swap regression.
-
[P1] Bind principal-secret creation to verified no-follow handles
internal/sandbox/windows_identity_secret_windows.go:144
Elevated setup creates, truncates, locks, and writes the secret through separate path-based calls. The sandbox-home path is user controlled, so a junction, symlink, or file swap between those calls can redirect the Administrator process to another file. Use retained no-follow handles for the parent, file, ACL update, and write. -
[P1] Provide a launch contract that can create a process for the batch principal
internal/sandbox/windows_process_windows.go:59
Author context: you noted that this path has not yet run end to end.The normal runner tries to start a process as the new account without the Windows privileges and desktop permissions that API requires. An opted-in, network-allowed command can therefore fail before its program starts with
ERROR_PRIVILEGE_NOT_HELDorACCESS_DENIED. Use a launch mechanism that works for an alternate account from an unelevated runner, and test that exact path. -
[P1] Build the child environment from the principal token rather than the caller
internal/sandbox/windows_runner.go:335
The child gets the caller's Windows profile variables, such asUSERPROFILE,APPDATA, andUSERNAME, even though it runs as the sandbox account. That account is deliberately blocked from the caller's profile, so tools can fail while looking up config or see the wrong identity. This remains a problem after process launch is fixed. Build an environment for the sandbox account, then apply the intended sandbox temp/cache overrides. -
[P2] Retain the SID needed to retry a failed principal-ACE revocation
internal/sandbox/windows_identity_runtime_windows.go:495
If ACL cleanup fails, teardown deletes the account but saves only the affected paths—not that account's SID. The next setup creates a different SID, so it cannot remove the old ACL entries and then overwrites the record. Keep the account until cleanup succeeds, or save the old SID with the paths and use it on retry. Add a failed-cleanup-then-reprovision test.
The elevated secret write resolved its path four separate times: MkdirAll, an O_TRUNC open, SetNamedSecurityInfo by name, then WriteFile by name. The sandbox home belongs to the invoking user, who is the party this sandbox contains, so each resolution was a place to swap a component. A symlink leaf lets an Administrator truncate a file of the caller's choosing and then rewrite its DACL; a junction alone is enough to plant the deterministic secret somewhere the caller controls. Create the leaf relative to a pinned no-follow parent handle, refuse it if it is a reparse point, and apply both the DACL and the bytes to that handle. The name is never resolved again after the create. The payload is now sealed before the file exists, so a failure there leaves nothing on disk rather than an empty file for someone to race. Cleanup on failure stays by name, which is safe in the direction that matters: at worst it misses and leaves a locked-down file, never deletes something it did not create.
CreateProcessAsUser exempts only a restricted version of the caller's own primary token from SE_ASSIGNPRIMARYTOKEN_NAME, which is precisely why the ordinary restricted-token path works while holding nothing special. A principal token comes from LogonUser against a separate local account, so the exemption does not apply and both that privilege and SE_INCREASE_QUOTA_NAME are required. Nothing enabled or checked either, and a token measured on an ordinary unelevated process holds neither, so the failure arrived as a bare "Access is denied" from inside process creation, before the command's executable was ever opened, and read as the command being rejected. Enable them where they are held, since present-but-disabled still fails the access check and that is where an elevated administrator lands, and refuse with the specific names and a way out where they are not. Detected by ENUMERATING the token. AdjustTokenPrivileges reports an unheld privilege by returning success with ERROR_NOT_ALL_ASSIGNED, which this binding does not surface: it returns nil for SeTcbPrivilege on an ordinary process. A check built on its error passed everywhere, which is worse than no check, since it would call the sandbox ready in exactly the case it cannot run. This does not make the principal launchable. It makes the reason legible while the launch mechanism itself is settled.
|
@jatmn took the next two. Head is P1 secret write: fixed in P1 process creation: confirmed, and I measured it rather than reading it. A probe on an ordinary unelevated process shows the token holding five privileges, LUIDs 19, 23, 25, 33 and 34. The part I would add to your finding is why the ordinary path is fine.
Worth flagging how nearly that shipped broken. My first version asked On the mechanism itself I would rather agree a direction than pick one blind, since each has a real catch:
I lean two stage. Before writing it I would want your read, and kevin's on whether a broker is acceptable, because your ask ends with exercising that exact path and that needs the elevated machine rather than my judgement. Also still open from your list: the environment/profile one and the ledger SID one, both untouched. |
The child environment starts as the invoking user's, and the deliberate sandbox redirects only replace HOME, the temp variables and the per-tool cache dirs. Everything identifying the account survived, so a command running as the principal read USERPROFILE, APPDATA, LOCALAPPDATA, HOMEDRIVE, HOMEPATH, USERNAME and USERDOMAIN describing the CALLER, whose profile the principal deliberately cannot open. Native tools resolve per-user state through exactly those, so they fail during startup or quietly look somewhere they have no business reading. Point them into the sandbox runtime tree, which is already granted to the principal and already holds its caches, so the paths are writable by construction. Naming the real Windows profile would need LoadUserProfile to have run, and a variable pointing at a directory that does not exist yet is a worse answer than one pointing somewhere usable. Layered under the deliberate redirects rather than over them: sandboxRuntimeEnvironment stays the single owner of HOME, TMPDIR, TMP and TEMP, and a regression pins that so the two cannot drift into setting the same variables from two places. This is the environment half of the finding. Loading the principal's profile and known folders is left until the launch mechanism is settled, since LOGON_WITH_PROFILE would do it as a side effect.
|
@jatmn environment finding, half of it:
Layered under the deliberate redirects rather than over them, so I did not do the profile and known-folders half. If the launch mechanism ends up going through Where the six stand:
So the two that need your input rather than my keyboard are the ancestor junction premise and the launch mechanism. Everything else is either landed or queued behind those. |
Both unelevated ACL failures told the reader to re-run with `--sandbox forbid`. There is no such option: SandboxPreferenceForbid is an internal engine state with no flag behind it, so acting on it produced an unknown option and left them stuck on the failure they had just been told how to clear. Advice that does not work costs more than none, because finding that out takes the reader's time. Name the real way out instead, the user config key, which is honored from global config only so a cloned repo cannot set it. The elevated-setup remedy beside it was already correct and stays. Reported by jatmn against the same string on #640. It predates this branch, having arrived with the unelevated fallback tier in #427, and the copy on #886 is fixed separately in 1b304e1. Also covers the secret write with the junction regression it was owed: the caller owns the sandbox home, so they can put a reparse point where the secret directory is expected, and the pathname version followed it in an elevated process. The test asserts the refusal names the reparse point and that nothing survives on the far side, since refusing while still creating the file would leave the caller holding it.
verifyWindowsACLTargetNotRedirected asks GetFinalPathNameByHandle where the handle landed, then compared that against canonicalSandboxWorkspaceRoot(path), which runs filepath.EvalSymlinks. That is the same resolution the kernel had just performed, so for a directory symlink the two sides agreed precisely BECAUSE the redirect happened: elevated setup went on to rewrite the DACL of an object outside the workspace while the check reported success. Junctions were rejected, but by accident rather than by design. Go reports a junction as ModeIrregular rather than ModeSymlink, so EvalSymlinks refuses it, the canonicalization falls back to the lexical path, and the mismatch surfaces. That is a property of the standard library's mode bits, not of this guard, and a Go release that resolved mount points would silently disarm the one case it was known to catch. The expected side is now normalized without resolving anything. GetLongPathName expands an 8.3 short name by reading directory entries and does not follow a link to its target, and EqualFold still covers casing, so the two spellings that legitimately name the same object still compare equal. Every failure degrades to the lexically cleaned path, which can only produce a spurious refusal, never a spurious match. A target deliberately spelled through a symlink is now refused. That is the intended direction: the question here is whether the object the handle landed on is the object that was named. On the tests, plainly: the directory-symlink regression is the one that separates the old basis from the new, and it needs Developer Mode or SeCreateSymbolicLinkPrivilege, so it skips on a machine without either and skipped on mine. The junction test beside it passes against both bases and says so in its own comment; it is an invariant test guarding the Go behaviour the old code accidentally depended on, not a regression for this change.
|
One of the five is fixed here. Rather than claim more, here is where each actually stands, checked against the current head instead of against what the commit titles suggest. [P1] Reject redirected ancestors — FIXED ( Your diagnosis was right and the mechanism is worse than "the check resolves the requested path". The two sides were computed by the SAME resolver: Junctions were rejected, but by accident. Go reports a junction as The expected side is now normalized without resolving anything: One consequence worth your sign-off: a target deliberately spelled through a symlink is now refused. I think that is correct for a check whose whole question is whether the object opened is the object named, but it is a behaviour change and entry paths are not canonicalized at plan-build time today. On the test, plainly. The directory-symlink regression is the one that separates the old basis from the new, and it needs Developer Mode or [P1] Secret creation on no-follow handles — PARTIAL, not closed. [P1] Launch contract — NOT fixed, and
Both are real designs with real costs (interactive logon right, seclogon, stdio over pre-created named pipes, exit-code and cancellation across two hops; or a permanent SYSTEM service whose IPC ACL becomes the boundary). That is an architecture decision for this PR's owner, not something I should pick unilaterally at this hour. One piece I would take regardless, and it is small: the privilege probe currently runs per command, so an operator learns the backend cannot launch AFTER an account, password, ACEs and ledger have been provisioned. Setup and [P1] Child environment — PARTIAL. [P2] Retain the SID for a failed revocation — NOT fixed. Neither file was touched since your review. The ledger is Validation for what I did push: Happy to take the launch mechanism next if you tell me which of the two shapes you want, and the environment and ledger fixes are straightforward once that is settled. |
TestACLComparablePathDoesNotResolveAReparsePoint failed on Windows CI while passing locally. The test was wrong, not the code. It asserted that windowsACLComparablePath returns a string equal to the path passed in. GetLongPathName legitimately rewrites that string: a CI runner's temp directory is an 8.3 short name, so expanding RUNNER~1 to runneradmin produces a different string naming exactly the same object, and the assertion failed on the one property the function is supposed to have. Both sides are now normalized before comparison, which states the real property: a path THROUGH the reparse point must not normalize to the target's normalization, or the guard would compare a redirected handle against a redirected expectation and match itself. A second assertion keeps the link component present so a resolution to something else entirely still fails. The test's documented limit is unchanged and still honest: a junction cannot separate the old basis from the new one, because EvalSymlinks does not resolve junctions either. It remains an invariant test.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues introduced by this PR that need to be addressed before this is ready. They arise in the new opt-in principal backend or the new sandbox exec path, rather than being baseline drift.
Findings
-
[P1] Serialize the sandbox-home capability and WFP transaction
internal/sandbox/windows_setup_lock_windows.go:80
The new setup mutex is keyed by workspace, butwindows-cap-sids.jsonis shared by every workspace under a sandbox home. Two first-time elevated setups for different workspaces can both observe no file, mint different offline-marker SIDs, and overwrite one another's file. Each setup may already have installed WFP filters keyed to its own SID; a laterNetworkDenyrunner loads the last persisted SID, and if the installed filter has the other SID it does not match, so the command is no longer network-denied. The root cause is treating machine-/sandbox-home-scoped identity state as workspace-scoped. Serialize capability-file read-modify-write and the dependent WFP installation with a sandbox-home-wide lock (or one transactional state owner), and add a concurrent two-workspace regression that proves the persisted SID and filters agree. -
[P1] Provide a principal launch design that works from the normal runner
internal/sandbox/windows_command_runner_windows.go:162
After the user completes the documented elevated setup, a normalzeroprocess obtains a different-accountLogonUsertoken and then requires its own process token to holdSeAssignPrimaryTokenPrivilegeandSeIncreaseQuotaPrivilegebefore callingCreateProcessAsUser. The helper itself explains that an ordinary unelevated process lacks those privileges, so the only mode in which the principal is eligible—opt-in plus network allow—exits before starting the requested tool. Even a caller that happened to hold those privileges is forced ontowinsta0\\default, but setup grants the batch-only principal no window-station or desktop access. This needs a launch architecture that is viable for an unelevated caller, such as a deliberately designed bootstrap/broker arrangement with secure stdio, cancellation, and token-jailing semantics; merely preflighting the absent privileges does not make the feature usable. Exercise that exact setup-to-command path on Windows. -
[P1] Bind the complete privileged secret path to no-follow handles
internal/sandbox/windows_identity_secret_windows.go:138
The new secret writer callsos.MkdirAll(filepath.Dir(path))through the user-controlled sandbox-home pathname beforecreateWindowsSecretFileNoFollowopens the final parent. A user can place or swap a junction/symlink in that directory chain, causing the elevated setup to create the directory and then create, truncate, DACL-rewrite, and write the secret below an attacker-selected target. Verifying the final parent after this has occurred only verifies the redirected location, not containment beneath the intended sandbox home. The root cause is protecting only the leaf while earlier privileged operations remain pathname-based. Create and retain every parent through a no-follow, handle-relative walk, then create, secure, publish, and clean up the secret relative to those handles; add a directory-junction/swap regression covering the complete sequence. -
[P1] Publish rotated secrets atomically instead of exposing a fallback window
internal/sandbox/windows_identity_secret_handle_windows.go:70
FILE_OVERWRITE_IFtruncates the existing live secret beforewriteWindowsSecretHandlehas completed the replacement ciphertext. A command beginning in that interval reads an empty or partial blob, maps it to “identity unavailable,” and then silently uses the weaker same-user restricted-token fallback, losing the opted-in read confinement. The setup lock does not protect command readers, so this is reachable during ordinary setup/command overlap. The root cause is treating an exclusive writer as sufficient when readers require a complete credential. Build and ACL a complete replacement file, atomically replace the live name only after it is valid, and make readers fail closed or retry around an in-progress rotation rather than downgrade isolation. -
[P1] Fingerprint every principal ACL input, including deny-write paths
internal/sandbox/windows_setup.go:431
windowsPrincipalPlanFingerprinthashes write roots, read roots, and deny-read paths, but omitsDenyWrite; the actual plan later applied inapplyWindowsPrincipalACLsdoes include it. Adding a deny-write policy path therefore leaves marker validation successful and the old inherited principal allow remains usable; removing one leaves its deny ACE in place. The root cause is duplicating the plan inputs for fingerprinting rather than making the fingerprint derive from exactly the applied plan. Hash the same complete principal ACL plan used at setup (with only the trustee replaced by the stable placeholder) and add marker-validation regressions for both adding and removingDenyWrite. -
[P1] Support linked-worktree Git metadata or reject it before setup
internal/sandbox/profile.go:147
For a linked worktree, the new logic correctly avoids treating the.gitpointer file as a directory, but it then gives the separate principal no access to thegitdir:control directory referenced outside the worktree. Unlike the original caller, a new local account has no inherited access to that directory, so routinegit status, commits, hooks, and index updates cannot traverse it after principal launch is made functional. The root cause is changing the carveout shape without tracing the pointer's runtime consumer. Resolve and validate the referenced gitdir and grant the minimal ACLs it requires while preserving the control-plane protections, or explicitly decline the principal backend for this layout; test a real Git operation in a linked worktree, not only setup andgit init. -
[P1] Construct and provision a complete principal environment
internal/sandbox/windows_principal_env_windows.go:49
The new environment shim rewrites only seven variables and points%USERPROFILE%,%APPDATA%, and%LOCALAPPDATA%atruntime.Data\\profile, but neither setup nor launch creates that tree. The child still inherits caller-specific paths such asPATH,PSModulePath,GOPATH, OneDrive, and roaming-profile settings; typical Windows PATH entries point to the caller's AppData directories, which the principal intentionally cannot read. Native tool discovery and config/cache initialization can consequently fail or consult the wrong identity. The root cause is editing a caller environment instead of constructing one for the target token. Create/load a usable principal profile and environment (including known folders), then apply only the intended sandbox redirects, and test tools that use AppData and profile-derived PATH entries. -
[P2] Retain the retired trustee SID with failed-revocation state
internal/sandbox/windows_identity_runtime_windows.go:495
On an ACE revocation failure, teardown keeps the new ledger but deletes the account. The ledger schema records only paths, not the retiring account's SID. A later setup creates a new account SID, attempts revocation with that new trustee, then narrows/replaces the ledger; it can never remove the old raw-SID ACEs and loses the only locations recorded for them. The root cause is persisting the target set without the identity required to act on it. Either retain the account until its revocation succeeds, or store an orphaned-grants record containing the old SID and paths, retry that exact trustee before normal provisioning, and add a revoke-failure-then-reprovision regression. -
[P2] Preserve raw cmd.exe
/ctext for the new sandbox-exec command
internal/sandbox/windows_process_windows.go:99
zero sandbox exec -- cmd /c ...is documented as an arbitrary command path, but only Zero's internalcmd.exe /d /cargument shape uses the raw shell-command-line builder. The documented form falls through tosyscall.EscapeArg, which inserts backslashes before embedded quotes even though cmd.exe parses/ctext differently from CommandLineToArgvW. For example, a/cscript containingpython -c "print(15 / 3)"can be misparsed before Python starts. The root cause is giving equivalent cmd invocation forms different command-line encoding semantics. Normalize cmd/cmd.exe/crequests to the raw command-text representation in one place, or generalize the runner's cmd detection, and cover nested quotes and metacharacters through the publicsandbox execcommand.
|
@jatmn one new fact that might change how you rank this, not a nudge on the findings. #881 is a user report that the Windows native sandbox blocks every This PR is what fixes it. I verified rather than assuming:
Setup fingerprinted the bare profile while every command arrived with the runtime root already appended, so a marker written seconds earlier was rejected and nothing could run. The reporter traced it to the same two functions independently. So the practical position: anyone who runs Nothing here is an argument that your findings are addressed. Two of the four are still open and one is an architecture decision I explicitly did not want to make alone. I am only flagging that the cost of this sitting is now a user-visible outage on one platform rather than an unshipped feature, in case that moves it up your queue. Happy to split the marker fix out into its own PR against |
|
@jatmn done, as offered: #901 carries the marker fix on its own against That takes the user-visible outage off this PR's critical path, so #808 can be judged on the principal work alone rather than under time pressure from #881. Which is how it should have been in the first place. #901 is 560 lines across 5 files, against 12,169 across 67 here. It contains only the runtime-root work: nothing from the principal, ledger, secret or logon machinery. Worth knowing it was not purely a lift. The marker fix depends on One thing I found doing the split, which applies here too. The composition test I brought over passes with the production call site in Either order works: if #808 lands first I close #901 as redundant; if #901 lands first, this rebases onto it and the runtime-root commits drop out. Your four findings are untouched by any of this, and I am not asking you to revisit them. |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues introduced or reintroduced by this branch that need to be addressed before this is ready. The principal-backend findings are in the new opt-in path; the ChatGPT findings are regressions from this PR's base, which already contains the corresponding working support.
Findings
-
[P1] Use a launch mechanism available to the normal principal runner
internal/sandbox/windows_command_runner_windows.go:162
The only configuration in which the principal backend is eligible—opt-in plus an allowed-network profile—mints a separate-account token and then requiresSeAssignPrimaryTokenPrivilegeandSeIncreaseQuotaPrivilegebefore callingCreateProcessAsUser. An ordinary unelevated Zero process holds neither, soenableWindowsPrincipalLaunchPrivilegesreturns an error and the requested command never starts, even after the one-time elevated setup succeeded. The root cause is using an alternate-account process-creation API whose privilege exemption applies only to a restricted version of the caller's own token. A clearer error does not make the feature usable: use a viable broker/bootstrap design or another launch contract available to the ordinary runner, preserve jailed-token/stdio/cancellation semantics, and cover the elevated-setup-to-ordinary-command path on Windows. -
[P1] Include deny-write rules in the principal setup fingerprint
internal/sandbox/windows_setup.go:431
The setup marker hashes a principal ACL plan containing write roots, read roots, andDenyRead, but omitsDenyWrite; the plan actually applied at setup includes it. Consequently, changing a deny-write policy leaves the marker valid: adding a denial does not install its ACE and the principal retains the old write grant, while removing one can leave a stale deny ACE. The root cause is duplicating the plan inputs for hashing rather than fingerprinting the actual plan. Derive the marker hash from the same complete principal plan setup applies, substituting only the stable placeholder trustee, and add regressions for both adding and removingDenyWrite. -
[P1] Publish principal-secret rotations without a readable invalid interval
internal/sandbox/windows_identity_secret_handle_windows.go:70
Rotating an existing secret opens the live name withFILE_OVERWRITE_IF, truncating it before the replacement blob has been ACLed and written. A concurrently starting command can read that empty/partial blob, classify the identity as unavailable, and silently fall back to the weaker same-user restricted-token backend. The setup mutex does not cover command readers, so this loses opted-in read confinement during routine setup. The root cause is treating an exclusive writer as sufficient even though readers require a complete credential. Build and ACL a replacement file first, atomically publish it only once valid, and make readers retry or fail closed while rotation is in progress. -
[P1] Keep privileged secret-path traversal and cleanup handle-relative
internal/sandbox/windows_identity_secret_windows.go:138
The new leaf helper protects the file after it has been opened, but elevated setup still callsMkdirAll(filepath.Dir(path))through the caller-controlled sandbox-home path before that no-follow walk, and failure cleanup later callsos.Remove(path)by name. A directory junction/symlink or swap can therefore redirect privileged directory creation, or redirect cleanup to an attacker-chosen deterministic secret leaf outside the intended home. The root cause is securing only the final handle while retaining privileged path resolution before and after it. Create and retain every parent through a no-follow, handle-relative walk; create, ACL, write, and remove the leaf relative to that verified parent handle; and add junction/symlink-swap coverage for the full failure sequence. -
[P1] Restore the complete ChatGPT OAuth model-discovery protocol
internal/tui/picker.go:409
This branch removes the base's token/account-bound discovery options and ChatGPT protocol handling: the picker now loads a stored OAuth bearer intoAPIKeybut calls discovery with empty options. That drops the refresh-capable resolver, matchingchatgpt-account-idresolver, and Codex user agent; the changed parser also no longer accepts ChatGPT'smodels[].slugresponse or appends the required client-version query. ChatGPT OAuth users consequently fall back silently to the baked-in list rather than receiving their entitled live models; this also affects picker refreshes triggered by launch/provider switching. The root cause is treating a stored bearer as interchangeable with ordinary API-key authentication and deleting the protocol-specific response path. Restore the base's token/account-bound discovery contract end-to-end, including refresh, matching account header, Codex request metadata, query shape, response parsing, and regressions that prove live entitlement data is used. -
[P1] Preserve the shipped ChatGPT priority-tier control
internal/tui/commands.go:262
This sandbox PR deletes the base's/fastcommand, capability-gated TUI state,CompletionRequest.ServiceTier, andservice_tierserialization for both OpenAI transports. A subscription user who previously selected the advertised priority tier now has no way to request it and silently receives the default tier. The root cause is unrelated removal of the complete request capability rather than a sandbox-specific change. Restore the capability-gated control and wire field with its existing provider/model checks, or move a deliberately approved removal into a separate, explicitly documented change. -
[P1] Make linked-worktree Git metadata usable by the principal
internal/sandbox/profile.go:136
For a linked worktree or submodule the new code correctly avoids materializing children under the.gitpointer file, but it grants the separate account no access to thegitdir:control directory outside the worktree. That account has no inherited access there, so after a usable principal launch, normalgit status, commits, hooks, and index/ref access fail. The root cause is treating the pointer file's protection as the complete Git contract without tracing Git's consumer path to the referenced directory. Resolve and validate thegitdir:target and grant only the required access while preserving the control-plane protections, or reject this layout before setup; cover a real Git operation in a linked worktree. -
[P1] Build and provision a complete environment for the principal
internal/sandbox/windows_principal_env_windows.go:49
This only rewrites seven variables in the invoking user's environment and pointsUSERPROFILE,APPDATA, andLOCALAPPDATAat<runtime.Data>\\profile, which setup never creates. Caller-derivedPATH,PSModulePath,GOPATH, OneDrive, and similar values still direct the separate account into the caller's profile, which it is intentionally unable to traverse. Once the launch path works, native tools can fail during discovery/config initialization or operate with the wrong identity. The root cause is editing an inherited caller environment instead of constructing one for the target token. Construct/load a usable target-token environment and profile/known-folder tree, create the redirected directories, then layer only the deliberate sandbox redirects over it; test tools that use AppData and profile-derived PATH entries. -
[P2] Persist the retiring trustee SID when ACE revocation fails
internal/sandbox/windows_identity_runtime_windows.go:495
Teardown keeps a ledger after revocation fails but deletes the account, while the ledger records only paths. The next setup creates a new SID, attempts revocation for that new trustee, and then overwrites/narrows the only record; it can never remove the old raw-SID ACEs despite reporting that they remain findable. The root cause is persisting the target set without the identity required to act on it. Retain the account until revocation succeeds, or persist orphaned grants as the retired SID plus paths and retry that trustee before normal provisioning; cover revoke failure followed by reprovisioning. -
[P2] Preserve raw
cmd /ctext in the public sandbox-exec path
internal/sandbox/windows_process_windows.go:99
The new documentedzero sandbox exec -- cmd /c <script>form falls through normalEscapeArgencoding because the raw-command-line special case recognizes onlycmd.exe /d /c.cmd.exeparses/ctext differently fromCommandLineToArgvW, so embedded quotes (for example a Python-cprogram) gain backslashes and can be misparsed before the child runs. The root cause is maintaining two semantically equivalent cmd invocation shapes with different command-line encoders. Normalize publiccmd/cmd.exe/cinvocations at one boundary to the raw-text path and cover nested quotes and metacharacters throughsandbox exec.
|
@jatmn thanks. I merged The two ChatGPT findings are a stale base, not a removalThis branch never had that code to delete. It forked from That one commit adds 90 The merge was clean, zero conflicts, and that is the confirmation rather than my say-so: a branch that had actually removed those lines would have had to touch them, and would have conflicted. Counts on this branch before and after: So neither needs work. "Restore the base's token/account-bound discovery contract end-to-end" would have been a rewrite of code that is already correct on Nothing of this branch's own was lost. The other eightNot disputing any of them, and two change how I think this should proceed. The launch-mechanism P1 matters most. You are right that The secret-rotation P1 is the one I am least comfortable having shipped. Truncating the live name before the replacement is valid, with readers outside the setup mutex, means a routine setup can silently downgrade an opted-in command to the weaker backend. Fail-closed readers plus atomic publish, as you describe. I am not going to pretend the remaining six are quick. Since the launch contract has to change first and several of the others sit on top of it, I would rather rework this deliberately than push a fast round of patches at a foundation that is about to move. One note that touches your review of #901The runtime-root work #901 carries had a real defect that this branch does not: the split dropped the production call site of |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Use a launch contract available to the normal unelevated runner
internal/sandbox/windows_command_runner_windows.go:162
The only eligible principal mode (opt-in with network allow) logs on as the separate account and then unconditionally requiresSeAssignPrimaryTokenPrivilegeandSeIncreaseQuotaPrivilegebeforeCreateProcessAsUser. The helper explicitly documents that ordinary unelevated processes lack them and returns an error; setup grants the principalSeBatchLogonRight, not launch privileges to every future Zero process. Thus the advertised sequence—elevated setup followed by ordinary tool calls—always exits before the executable starts. This is a launch-contract mismatch, not a missing diagnostic: redesign the handoff around a mechanism an unelevated caller can actually use (for example a constrained bootstrap or broker), while preserving the jailed token, stdio, cancellation, and exit-status semantics. Do not merely gate the existing error more clearly. -
[P1] Fingerprint the complete principal ACL plan
internal/sandbox/windows_setup.go:431
The marker fingerprint rebuilds the principal plan withoutDenyWrite, although setup applies it atinternal/sandbox/windows_identity_runtime_windows.go:597. Adding a deny-write rule therefore leaves the marker valid and never installs its ACE; removing one can likewise leave an old deny ACE in place. The root cause is duplicating the plan inputs for hashing instead of hashing the actual applied plan. Derive the fingerprint from the same complete principal plan setup applies, substituting only the stable placeholder trustee, and add regressions for both adding and removingDenyWrite. -
[P1] Publish rotated principal secrets atomically
internal/sandbox/windows_identity_secret_handle_windows.go:58
FILE_OVERWRITE_IFtruncates the live secret before the replacement DACL and ciphertext are complete. Command readers do not participate in the setup lock; one can read the empty or partial blob, classify the principal as unavailable, and fall back to the weaker same-user restricted-token backend atwindows_identity_runtime_windows.go:137. That loses the opted-in read boundary during ordinary setup/rotation rather than failing visibly. The root cause is treating an exclusive writer as sufficient for a reader-visible credential. Create and ACL a replacement through the protected parent, atomically publish only the complete blob, and make readers retry or fail closed during a replacement; cover a concurrent reader/rotation interleaving. -
[P1] Keep privileged secret-path setup and cleanup handle-relative
internal/sandbox/windows_identity_secret_windows.go:138
Although the leaf create is no-follow, elevated setup still runsMkdirAllthrough the caller-controlled sandbox-home path before that walk, and failure cleanup resolvesos.Remove(path)by name at lines 167 and 171. A reparse-point/ancestor swap can redirect privileged directory creation or make cleanup delete the deterministic secret filename in an attacker-selected target. Protecting only the final create handle does not secure the surrounding privileged path resolution. Create and retain every parent through a no-follow handle-relative walk; create, lock, write, and delete the leaf relative to that verified parent; and add junction/symlink-swap coverage for each failure cleanup path. -
[P1] Preserve Git access for linked worktrees and submodules
internal/sandbox/profile.go:153
For a.gitpointer file this code correctly denies rewriting the pointer, but the isolated principal receives no access to the externalgitdir:control directory. Git still needs that directory for its index, refs, objects, and hooks, sogit status, commits, and hook execution fail once the principal launch path works. The root cause is treating the pointer file as the whole Git contract while the consumer follows it outside the workspace. Resolve and validate thegitdir:target, provision only the access Git needs while retaining config/hook protections, or reject this layout before setup; add a real linked-worktree Git operation rather than only testing the planned pointer carveout. -
[P1] Build a usable target-principal environment
internal/sandbox/windows_principal_env_windows.go:49
The principal process inherits the caller environment and rewrites only seven identity variables.PATH,PSModulePath,GOPATH, OneDrive, and other profile-derived entries still name the caller's deliberately inaccessible profile; additionally, the new<runtime.Data>\\profilehierarchy is never created. Native tools can therefore fail discovery/config initialization or use the wrong account paths even thoughUSERPROFILEitself was rewritten. The root cause is patching an invoking-user environment rather than constructing one for the token being launched. Build/load a usable target-principal environment and profile/known-folder tree, then layer the intentional sandbox redirects over it; exercise tools that resolve executables and state through AppData and profile-derived PATH entries. -
[P1] Make principal ACL-ledger writes reparse-safe
internal/sandbox/windows_principal_ledger.go:88
The elevated setup writes its ledger viaMkdirAll,CreateTemp, andRenameunder caller-controlledsandboxHome, with no no-follow ancestor validation. Replacingwindows-principal-aclwith a junction during setup can redirect temporary creation and replacement outside the intended directory, allowing privileged file creation/replacement and corrupting the only cleanup record. This is the same elevated reparse-point boundary the secret work started to address, but the new persistent state bypasses it entirely. Use a pinned handle-relative create-and-replace protocol (or reject reparse ancestors) for the ledger and its removal path, and cover a concurrent junction swap. -
[P2] Retain the retiring SID when ACL revocation fails
internal/sandbox/windows_principal_ledger.go:34
The kept ledger records only paths. Teardown can report a failed revocation, then delete the account; a later setup creates a new SID, attempts revocation for that new trustee, and overwrites the only path record. The raw-SID ACEs from the retired account can no longer be removed despite the error promising they remain findable. The root cause is preserving the targets but discarding the trustee required to act on them. Persist orphaned grants as the retiring SID plus their paths and retry those before normal provisioning, or retain the account until revocation succeeds; test a revoke failure followed by reprovisioning. -
[P2] Preserve raw
cmd /ccommand text insandbox exec
internal/sandbox/windows_runner.go:148
The documentedzero sandbox exec -- cmd /c <script>form does not match the raw-command-line special case, which accepts onlycmd.exe /d /c. It falls through normal argv escaping, butcmd /cparses its remainder differently fromCommandLineToArgvW; nested quotes (such as a Python-cprogram) are changed before the child runs. The root cause is supporting equivalent public command forms while recognizing only one internal spelling. Normalizecmdandcmd.exe/cinvocations at one boundary to the raw-text path, without changing non-shell argv behavior, and cover nested quotes and metacharacters through the publicsandbox execpath.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Use a launch mechanism available to the normal principal runner
internal/sandbox/windows_command_runner_windows.go:162
The only configuration in which the principal backend is eligible—ZERO_WINDOWS_SANDBOX_IDENTITY=1with a network-allow profile—successfully obtains the separate-account token, then unconditionally callsenableWindowsPrincipalLaunchPrivilegesbeforeCreateProcessAsUser. That helper requiresSeAssignPrimaryTokenPrivilegeandSeIncreaseQuotaPrivilege, which a normal unelevated Zero process does not hold; elevated setup grants the principalSeBatchLogonRight, not process-creation privileges to every later caller. The result is deterministic: setup can provision the account, secret, and ACLs, but the first normal eligible command exits before opening the requested executable. This is a launch-contract mismatch, not a missing diagnostic. Design the handoff around an unprivileged-compatible mechanism—such as a constrained bootstrap or broker—while preserving the restricted token, inherited stdio, cancellation, and child exit status; add an elevated-setup-to-ordinary-command integration test so the supported workflow is exercised. -
[P1] Publish principal-secret rotations without a readable invalid interval
internal/sandbox/windows_identity_secret_handle_windows.go:58
Replacing an existing secret usesFILE_OVERWRITE_IF, which truncates the live name before the replacement DACL and DPAPI blob have been completed. The setup mutex does not cover command readers: a command starting during this interval reads an empty, partial, or undecryptable blob;readWindowsSandboxSecretclassifies that as identity-unavailable; and the caller silently falls back to the weaker same-user restricted-token backend. Thus a routine elevated setup/rotation can temporarily remove the read-confining boundary the operator explicitly enabled. The root cause is publishing a mutable credential at its final name before it is valid. Build the sealed blob in a separate protected file, flush and ACL it, then atomically replace the live name; readers should retry a replacement in progress or fail closed rather than downgrade. Cover the concurrent reader/rotation interleaving. -
[P1] Keep privileged secret-directory setup and cleanup handle-relative
internal/sandbox/windows_identity_secret_windows.go:138
The no-follow code protects only the final leaf, afteros.MkdirAll(filepath.Dir(path))has already resolved the caller-controlled sandbox-home path; the failure paths at lines 167 and 171 then callos.Remove(path)by name. An unprivileged workspace user can arrange a reparse ancestor before elevation or swap one between the pinned leaf creation and cleanup. The elevated process can then create directories through the redirected ancestor or remove the deterministic secret leaf at an attacker-selected target. The root cause is mixing a secure final-component open with unsecured path resolution before and after it. Create and retain every parent through a no-follow, handle-relative walk, and create, ACL, write, replace, and remove the leaf relative to that verified parent handle. Add junction/symlink-swap tests covering both setup and each error cleanup path. -
[P1] Make the principal ACL ledger's privileged writes reparse-safe
internal/sandbox/windows_principal_ledger.go:93
Elevated setup persists the only stale-ACE recovery record usingMkdirAll,CreateTemp, andRenamebelow the caller-controlled sandbox home, while teardown removes it by pathname. Replacingwindows-principal-aclwith a junction before or during this sequence redirects privileged temporary creation, replacement, or removal outside the intended state directory; it can also corrupt or destroy the sole record needed to clean up old ACL grants. The secret path's newer handle-relative protocol does not protect this independent persistent-state path. Treat the ledger as security-boundary state: create and hold verified parent handles, write the replacement through a sibling handle, atomically publish it relative to that parent, and remove it the same way. Add a concurrent junction-swap regression for write, replacement, and teardown. -
[P1] Preserve Git access for linked worktrees and submodules
internal/sandbox/profile.go:136
For a linked worktree or submodule,.gitis agitdir:pointer file. The change correctly stops the principal rewriting that pointer, but treats the external target as needing no further handling because it is outside the write root. That conflates sandbox protection with Git's runtime contract: Git follows the target for the index, refs, objects, configuration, and hooks, and the new separate local account has no corresponding grant there. Once the launch path works, normalgit status, commits, hooks, and operations that need the worktree metadata fail. Resolve and validate the pointer target, then grant only the access Git requires without reopening the protected configuration/hook surfaces; alternatively reject this layout explicitly during setup. Exercise a real linked-worktree Git operation rather than asserting only the pointer-file carveout. -
[P2] Persist the retiring trustee SID when ACE revocation fails
internal/sandbox/windows_identity_runtime_windows.go:467
On revocation failure teardown deliberately keeps the ledger, but then revokes the account's logon rights and deletes the account. The ledger stores only paths, not the retiring SID. A later setup creates a same-named account with a new SID, reads the old paths, attempts trustee-based removal using the new SID, and eventually overwrites or narrows the record. The raw-SID ACEs from the retired account consequently remain on disk forever even though the earlier error says the ledger was kept so they could be found. The root cause is persisting the target set without the identity required to act on it. Persist orphaned grants as{sid, paths}and retry those trustees before normal provisioning, or retain the account until all revocations succeed. Add a regression that forces revocation failure, reprovisions the principal, and verifies removal of the original SID's ACEs.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Overall guidance
This is not a set of independent edge cases. The PR introduces a new security
principal, durable machine state, elevated setup, and a second Windows execution
path, but several of those layers are only locally correct. The implementation
needs a smaller set of end-to-end invariants, then tests that exercise those
invariants through the actual setup and command boundaries.
-
Define and prove the supported lifecycle before extending the ACL model.
The advertised lifecycle is: an administrator performs one-time setup; an
ordinary user subsequently runs a command as the separate principal; the
command receives the requested filesystem and network policy; teardown
removes every resource that setup created. Today the launch half cannot
satisfy that lifecycle because a normal caller cannot use the selected
process-creation API with a different-account token. Do not treat the
diagnostic for missing privileges as the implementation of the lifecycle.
Choose an architecture that can actually launch from the intended caller
(for example, a deliberately constrained broker), document its trust and
cancellation/stdio model, and make an elevated-setup-to-ordinary-command
integration test the first acceptance criterion. -
Treat every file below sandbox home as privileged security state.
The secret and ACL ledger decide who can run as the principal and which
filesystem grants can later be removed. A caller-controlled path is not
safe merely because its final leaf is opened with no-follow semantics:
every parent traversal, temporary-file creation, replacement, and cleanup
must be bound to verified handles as well. Centralize this protocol instead
of maintaining one secure implementation for secret leaves and independent
pathname implementations for directories and ledger files. Its contract
should be: no attacker-controlled reparse point is followed, cleanup cannot
target a substituted name, and replacement never exposes an incomplete
credential. -
Make setup publication transactional for readers, not just serialized for
writers. A mutex around setup does not protect command processes that
read the secret or ledger without that mutex. Publish complete, protected
replacements atomically; retain enough identity information to recover from
a partially failed teardown; and choose fail-closed or explicit retry
semantics for any state transition that would otherwise downgrade the
sandbox. Every persistent record needs a schema that contains the identity
used to reverse its side effects, not only the paths affected. -
Model real subprocess contracts, not just the workspace DACL. A
separate account loses the caller's ambient access. That is the feature for
credentials, but it also affects Git worktree metadata, executable lookup,
per-user state, and other tool dependencies. Enumerate the supported
workspace layouts and required external resources, grant only the minimum
validated access, and reject layouts that cannot be made safe. ACL-plan
shape tests are useful, but they cannot replace tests that execute Git and a
representative command under the final token.
I recommend pausing further feature additions until those lifecycle and state
protocols are settled. Then split the work into reviewable pieces: first a
proven unprivileged launch mechanism, then handle-relative/atomic state
storage and teardown, then filesystem and Git compatibility. Each piece
should include a Windows integration test that proves the externally visible
contract rather than only mocks the relevant Win32 call or inspects the
planned ACEs.
Findings
-
[P1] Make the principal launcher usable by the normal unelevated command path
internal/sandbox/windows_command_runner_windows.go:162
The principal branch first successfully obtains aLogonUsertoken, then unconditionally callsenableWindowsPrincipalLaunchPrivilegesbeforeCreateProcessAsUser. That helper requires the calling Zero process to holdSeAssignPrimaryTokenPrivilegeandSeIncreaseQuotaPrivilege; its own error path says ordinary unelevated processes do not hold them. Elevated setup grants onlySeBatchLogonRightto the newly created principal, which permits that account to log on but does not grant either process-creation privilege to every later desktop process. Consequently, the documented workflow of elevated setup followed by ordinaryzerocommands fails at this check before the requested executable is opened whenever the principal path is eligible.The root cause is treating a different-account
CreateProcessAsUserlaunch as equivalent to launching a restricted version of the caller's own token. Rework the handoff around a mechanism that is valid for an unelevated caller (for example, a deliberately constrained elevated broker or bootstrap), while preserving the restricted token, stdio, cancellation, and child exit status. Add an integration test that performs elevated provisioning and then proves an ordinary-user command actually runs under the principal; a unit test that merely reports missing privileges cannot establish the supported flow. -
[P1] Do not publish a truncated principal secret during rotation
internal/sandbox/windows_identity_secret_handle_windows.go:70
Rotation opens the final, live secret withFILE_OVERWRITE_IF, which truncates/replaces that name beforelockWindowsSecretHandleToOwnerandwriteWindowsSecretHandlehave completed. The setup mutex serializes writers only; command readers callos.ReadFilewithout it. A command starting in this interval sees an empty, partial, or DPAPI-invalid payload.readWindowsSandboxSecretclassifies each of those states as identity-unavailable, andwindowsSandboxPrincipalTokenthen deliberately falls back to the same-user restricted-token backend. That fallback does not provide the opt-in principal's read confinement, so an ordinary setup/rotation transiently weakens the active sandbox boundary.The root cause is publishing a mutable credential at its final name before it is valid. Build the encrypted payload in a sibling file below a verified parent, apply and verify its protected DACL, flush it, then atomically replace the live name. Readers should retry a replacement-in-progress or fail closed while an opted-in principal is being repaired; they must not silently downgrade. Add a concurrent reader/rotation test that proves no command obtains the fallback token during publication.
-
[P1] Make all privileged secret-directory operations handle-relative
internal/sandbox/windows_identity_secret_windows.go:138
The new no-follow operation protects only the final secret leaf. Elevated setup has already resolved the caller-controlled sandbox-home path throughos.MkdirAll(filepath.Dir(path)), and its post-create error paths callos.Remove(path)by name. A user can place a junction/reparse point in an ancestor before elevation, or swap one between final-leaf creation and cleanup; the elevated process can then create directories through the redirected ancestor or remove the deterministic secret leaf at an attacker-selected location. The final-component handle does not secure the earlier or later pathname resolutions.The root cause is mixing a secure leaf operation with unsecured parent traversal and cleanup. Walk every parent with no-follow, handle-relative opens; retain the verified parent handle for create, ACL, write, replace, and cleanup; and never fall back to a named remove. Add junction/symlink-swap coverage for parent creation and each failure cleanup path, not only for the final leaf.
-
[P1] Protect the principal ACL ledger from reparse-point swaps
internal/sandbox/windows_principal_ledger.go:93
Elevated setup persists the only record used to recover stale principal ACEs withos.MkdirAll,os.CreateTemp, andos.Renamebeneath caller-controlled sandbox home; teardown usesos.Removeon the same pathname. Swappingwindows-principal-aclor an ancestor for a junction before or during this sequence redirects privileged temporary creation, replacement, or removal outside the intended state tree. It can also corrupt or destroy the sole record needed to revoke old grants. The handle-relative secret implementation does not protect this independent persistent-state path.Treat the ledger as security-boundary state rather than ordinary application metadata. Create and hold its parent through the same no-follow, handle-relative protocol; create the replacement as a sibling through that handle; atomically publish and remove relative to that verified parent. Add concurrent junction-swap regression tests for write, replacement, and teardown.
-
[P1] Support or reject linked-worktree Git metadata for principals
internal/sandbox/profile.go:157
A linked worktree or submodule has a.gitfile containing agitdir:pointer. The new code correctly avoids materializing children beneath that file and denies rewriting the pointer, but then assumes its external target needs no further handling because it lies outside the write root. That does not hold for the separate principal: Git follows the target for index, refs, objects, configuration, and hooks, and the new local account has no corresponding grant to the caller's external control directory. Once the launcher works,git status, commits, fetches, and hook-related operations in these supported layouts fail.The root cause is conflating protection of the pointer with access to the data Git must consume. Resolve and validate the
gitdir:target, then grant only the minimum access Git requires without reopening protected configuration and hook surfaces; alternatively, reject this layout explicitly during setup. Cover a real linked-worktree Git operation, not solely the.gitpointer's carveout shape. -
[P2] Retain the retired trustee SID when revocation fails
internal/sandbox/windows_identity_runtime_windows.go:467
On revocation failure teardown deliberately keeps the ledger, but then revokes the account's logon rights and deletes the account. The ledger schema stores only paths. A later setup creates the same account name with a new SID, reads those paths, and removes ACEs using the new SID. Raw ACEs for the retired SID remain permanently, after which the ledger can be narrowed or overwritten despite the earlier error claiming the residue was kept findable.The root cause is preserving the target paths without preserving the identity needed to act on them. Persist orphaned grants as at least
{ sid, paths }and retry every recorded trustee before normal provisioning, or retain the original account until all revocations succeed. Add a regression that forces revocation failure, reprovisions the principal, and verifies removal of the original SID's ACEs.
6 participants
Opt-in behind
ZERO_WINDOWS_SANDBOX_IDENTITY=1. The provisioning half has now been run on a real elevated session; the logon half has not, and that is called out below.What this does NOT do yet
Two corrections to how an earlier version of this description read, both raised in review.
This does not close #662 for a default install. The principal backend is deliberately disabled whenever the network mode is deny (
windowsSandboxPrincipalEligible), because WFP block filters key on the offline-marker SID that only a restricted token can carry. Default policy IS network-deny. So with nothing butZERO_WINDOWS_SANDBOX_IDENTITY=1set, commands keep using the restricted same-user token andcredentialDenyReadPathsremains a no-op on Windows. Principal read confinement needs elevated setup AND a network-allow command profile, until the filters are also keyed to the principal SID. This PR is the foundation for #662, not its fix.One change here is not gated by the opt-in.
WindowsACLAllowWritenow includesDELETE.FILE_DELETE_CHILDis deliberately NOT granted: it would let a sandboxed command delete a protected carveout such as.git/configthrough its parent directory and recreate it without the deny ACE. That mask is shared with the capability-SID plans, so it applies on every elevated setup re-run whether or not the env var is set. It is a fix rather than a regression (without it a sandboxed command could create files it could never delete or rename), but it is a real behaviour change for installs that never opt in, and it belongs in the release notes rather than buried in a principal PR.Why
credentialDenyReadPathsopens withif runtime.GOOS == "windows" { return nil }, so on Windows no credential path is protected (#662, and the Windows half of #675). That is not an oversight and not a one-line fix.Every Windows backend derives its token from the CALLING user via
CreateRestrictedToken. A deny-read ACE that would stop the sandboxed child reading~/.awsnames the same account Zero itself runs as, so it would lock Zero out too. The one existing escape hatch is costly: the runner dropsWRITE_RESTRICTEDwhenever any DenyRead path is configured, because the kernel skips restricted-SID deny ACEs for reads under that flag, and a fully restricted token then cannot open executables. That is the same wall #640 hit.What this does
Gives the sandbox an identity of its own: a separate local account per workspace, in one managed group.
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. The interesting direction becomes what to GRANT, and the same SID is what a write grant or a firewall rule keys to.
SeBatchLogonRight, and explicitly denies interactive, network, remote-interactive and service logon, so the account cannot be signed into even if its password leaked.LogonUseris pinned to"."so a same-named domain account is never picked up.CryptProtectData, since an ACL only binds while the filesystem is the one being asked and a backup or a mounted image would otherwise give it up in the clear. The principal name is the entropy, so a blob copied onto another principal's path fails to decrypt rather than authenticating the wrong account.Gated behind
ZERO_WINDOWS_SANDBOX_IDENTITY=1, so no existing install changes behaviour.Verification, and what is not verified
gofmt,go vet,go build ./...clean; builds for linux, darwin and windows. 29 tests, all passing when I ran them, covering name derivation and truncation, password complexity, "already exists" handling, the raw Win32 struct layouts, LSA byte-vs-rune lengths, deny-before-allow ordering, trustee scoping, root grants, metadata materialization, revocation, secret round-trip and overwrite, path traversal, and idempotent removal.Two of those matter most and do real work rather than asserting intent: one reads the stored secret's DACL back and fails if any trustee other than the owner and SYSTEM appears, and another asserts
SE_DACL_PROTECTEDso an inherited ACE cannot reach it.One deliberate restriction. Network denial is enforced by WFP filters keyed to the offline-marker SID. The restricted token carries that SID; a token from
LogonUsercannot, because it names the account rather than a synthetic capability SID. A principal would therefore have left those block filters matching nothing, anddenyis the default mode. So the principal stands down whenever the network is denied and the restricted-token path runs instead, which means this backend currently engages only for network-allowed commands. Trading network denial for read confinement would have been the wrong way round. Keying the filters to the principal's own SID is the follow-up that lifts the restriction.Honest caveats:
NetUserAdd,LsaAddAccountRights,NetUserDelandLogonUserall need administrator rights. They compile and are layout-checked, but nobody has run them. The provisioning round-trip test is gated behindZERO_WINDOWS_IDENTITY_PROVISION_TEST=1plus an elevation check. Account and group creation have since been confirmed on a real elevated session; the logon path has not.TestGrantLogonRightsAndMintPrincipalTokenhas not run to completion: Smart App Control on this machine blocks freshly built unsigned binaries, so it needs a clean elevated box. Everything that does not require elevation runs here, including the secret round-trip, which asserts the password does not appear verbatim in the stored bytes.Worth deciding before this leaves draft
Creating real local accounts is user-visible in a way the current sandbox is not: AV and EDR commonly flag
NetUserAdd, enterprise policy often blocks local account creation, and the accounts appear innet userand Settings. None of that blocks the design, but it should be a deliberate call rather than a surprise in a merged PR.Summary by CodeRabbit