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 31 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
| for _, name := range root.ProtectedMetadataNames { | ||
| entries = append(entries, WindowsACLEntry{ | ||
| Action: WindowsACLDenyWrite, | ||
| Path: filepath.Join(cleaned, name), | ||
| Capability: input.PrincipalSID, | ||
| Materialize: true, | ||
| }) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Reject protected metadata names that escape the write root.
ProtectedMetadataNames is documented as names, but filepath.Join accepts .. and separator-containing values. A malformed value can materialize a deny ACE outside root.Root. Require one non-empty path component and add rejection tests.
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),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for _, name := range root.ProtectedMetadataNames { | |
| entries = append(entries, WindowsACLEntry{ | |
| Action: WindowsACLDenyWrite, | |
| Path: filepath.Join(cleaned, name), | |
| Capability: input.PrincipalSID, | |
| Materialize: true, | |
| }) | |
| 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), | |
| Capability: input.PrincipalSID, | |
| Materialize: true, | |
| }) |
🤖 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 - 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.
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.
Network denial is enforced by WFP filters keyed to the offline-marker SID. The restricted token carries that SID; a token from LogonUser cannot, because it names the account rather than a synthetic capability SID. Routing a denied-network command through a sandbox principal therefore left the block filters matching nothing and dropped egress enforcement altogether, and deny is the default mode. The principal now stands down whenever the network is denied and the restricted-token backend runs instead, so read confinement is never traded for a silent loss of network denial. Keying the filters to the principal's own SID is the follow-up that lifts the restriction. The decision sits in its own predicate rather than inline: on a machine with nothing provisioned the lookup declines for its own reasons, so a test that called through it would have passed with the guard removed. Also names the opt-out variable when a provisioned principal cannot be used, since the backend is opt-in and the operator needs a way back.
…user The file ACL stays the primary control and is what keeps the sandbox principal from reading its own credential. It only binds while the filesystem is the one being asked, though, so a backup or a mounted image hands over the password in the clear. CryptProtectData ties the ciphertext to the invoking user's logon secret, which covers exactly that gap. The principal name is passed as entropy, so a blob copied onto another principal's path fails to decrypt rather than authenticating the wrong account. A secret written by an older build reads as unavailable and falls back to the restricted token; the next elevated setup rewrites it. The round-trip test needs no privilege, so it runs everywhere rather than joining the gated set, and it asserts the password does not appear verbatim in the stored bytes.
lookupWindowsSandboxIdentity collapsed every SID-resolution failure into the "no principal is provisioned" sentinel, which threw away the check resolveWindowsSandboxSID deliberately makes: a name that resolves to a group or alias rather than a user account. The command path treats that sentinel as permission to fall back quietly, so an account name squatted by something that is not a user reached the operator as silence and a downgrade to the restricted token. Caught by gnanam in review. Only ERROR_NONE_MAPPED now means setup has not run. Anything else is a principal that exists but cannot be used, and the runtime path propagates it rather than swallowing it, which is where the description already said the line should sit. The decision lives in its own function because the lookup derives its account name from a workspace key, so a test cannot hand it a name that resolves to a group. The test drives the classifier with a real error from a well-known local group, needs no privilege, and fails if the old collapse-everything behaviour is restored. Also corrects a comment pointing at sandboxRuntimeKey, which does not exist. The function is windowsSandboxWorkspaceKey.
NetUserAdd leaves a pre-existing account completely untouched, password included, and ensureWindowsSandboxUser treated that status as success. So a second setup run generated a fresh random password, stored it as the secret, and left the account still authenticating with the old one. Every later command then failed to log on with a principal that looked correctly provisioned. Two comments claimed the caller reset the password in that case; nothing did. Caught by CodeRabbit. ensureWindowsSandboxUser now reports whether the account already existed, and provisioning resets the password through NetUserSetInfo when it did, so the value it returns is always the account's real password. The comments now describe what the code does. The gated provisioning test provisions twice and then logs on with the password from the SECOND run, which is the only honest assertion here: a stale password is indistinguishable from a correct one until something tries to authenticate with it. Also makes the syscall keep-alives explicit. The LSA and LogonUser call sites borrow Go memory that was either not kept alive at all (the policy attributes, the rights descriptor, the three logon strings) or kept alive only after the error check, so the failure path returned with it already collectable. The two netapi32 sites that used a deferred no-op closure now use runtime.KeepAlive as well, so one idiom is used throughout.
Retiring a principal deleted the account but left its LSA account rights behind, keyed to a SID that no longer resolves. That is the orphaned residue this model is supposed to avoid, and the reason ACE revocation is keyed to the trustee rather than to a record of what was granted; the logon-rights half was simply missing. CodeRabbit spotted it as a test-cleanup gap, but the production teardown path had the same hole. revokeWindowsSandboxLogonRights drops every right held by the principal and removes its LSA entry, and setup teardown now calls it BEFORE deleting the account, while the SID still resolves. Removing all rights rather than naming them is deliberate: the principal is being retired, so rights granted by an older setup that this one no longer knows about should go too. An account that holds no rights is not an error, since that is the state teardown wants. That tolerance depends on STATUS_OBJECT_NAME_NOT_FOUND surviving LsaNtStatusToWinError as something errors.Is can still match, which is the sort of Windows errno assumption that is often wrong, so there is now an unprivileged test asserting it, including that the tolerance does not also swallow access-denied. Both gated tests now clean up rights and account, in that order. The provisioning round trip had no cleanup at all and, since it started granting a batch logon right, was leaving both behind on whatever machine ran it.
…visioning Two problems in the provisioning path, both raised in review. The account name is derived from a workspace hash rather than discovered, so it can be occupied by a local account that has nothing to do with Zero, whether by coincidence or because somebody put it there. Provisioning treated "NetUserAdd says it exists" as "this is ours", reset the account's password, added it to the managed group and adopted it. That is a stranger's account taken over during an elevated setup, on the strength of a name matching a pattern we generate ourselves. Ownership is now proven from the comment provisioning stamps before anything is touched, and a name held by an account Zero did not create fails with a typed collision error instead of being adopted. Second, a failure anywhere after the account existed left it behind. The rollback the setup path installs is only built once provisioning has returned successfully, so nothing could undo a failure between creating the account and storing its secret; the account, and possibly its granted logon rights, simply stayed. Provisioning now unwinds what the run actually did, in reverse, on every failure path. Scoped to what THIS run created, deliberately. An account that already existed and belongs to Zero is a working principal from an earlier setup, so deleting it because a later run failed would turn a partial failure into a total one. For that case the repair is dropping the stored secret instead: this run reset the password, so the secret no longer matches, and absent beats stale because the command path treats a missing secret as "not provisioned" and falls back rather than failing. The ownership gate is asserted against real accounts every Windows install carries, which needs no privilege because it only has to establish that they are not ours. Classifying everything as managed makes it fail.
The cleanup added for partial provisioning left the window it existed for uncovered. It only removed the on-disk secret when this run had written one, and it derived the secret path after the logon-rights grant, so a failure before that point had nothing to remove. That is exactly the case that matters. Provisioning ALWAYS sets the account's password, including resetting a pre-existing account's, so from the moment it returns the stored secret is already stale. A failure in the rights grant then left that stale secret on disk against a password that had just changed, and the next command failed the logon and reported a broken sandbox instead of falling back. The path is now resolved from the account name before anything can fail, and removal is unconditional rather than gated on having written one. Absent beats stale: the command path treats a missing secret as "not provisioned" and falls back to the restricted token, which is the outcome a failed setup should leave behind. Raised by CodeRabbit, twice from different angles, on the commit that added the cleanup.
GetAce returns a generic ACE_HEADER and the helper reinterprets it as an ACCESS_ALLOWED_ACE. That holds for the fixed-layout types, but an object ACE carries Flags and two GUIDs ahead of the trustee, so SidStart would land mid-structure and Copy would read whatever bytes follow. The caller asserts that no unexpected trustee appears in the DACL, and on such an ACE it would print a nonsense SID rather than name the entry that does not belong. Nothing under test builds anything but allowed ACEs today, so this changes no current outcome. It keeps the failure legible if that ever changes.
… find it Two findings from review, both consequences of the principal being a separate account rather than the calling user. WindowsACLAllowWrite granted FILE_GENERIC_WRITE, which covers creating and modifying but not removing or renaming, and a rename needs delete on the source. Under the old same-user token this was invisible because the caller already held inherited rights on its own tree. A principal inherits nothing, so it could write files it could never delete, which fails ordinary editing and most git operations rather than an edge case. DELETE and FILE_DELETE_CHILD are now part of the grant, matching WindowsACLDenyWrite, which already treats delete as part of write. WRITE_DAC and WRITE_OWNER stay out: they are denied so the principal cannot rewrite its own restrictions. provisionWindowsSandboxIdentity returned a zero identity alongside created=true when group attachment or SID resolution failed after NetUserAdd had already created the account. The caller's rollback deletes by identity.Username, so it was asked to delete the empty string and left the account behind. Group attachment is the case that matters, being both the enforcement boundary and something local policy can refuse. The name now comes back with the error. The four provisioning calls are indirected so the failure paths are reachable in a test. Seaming only the post-creation pair would not have been enough: every step needs an elevated caller, so the test would have stopped at the group check and passed without reaching what it names. Also seeds the empty-secret test with a genuinely empty file. The previous whitespace seed was several bytes, so it never reached the length check and failed later in DPAPI instead, which another test already covers.
Six findings from review, all on the elevated setup path. Teardown was not scoped to what the run created. provisionWindowsSandbox PrincipalForSetup was careful never to delete an account it had adopted, and then setupWindowsSandboxPrincipal called removePrincipal on any ACL failure with no such guard. Re-running elevated setup on a working machine and hitting one transient ACL error therefore deleted the local account, its secret and its logon rights. It now returns whether it created the principal and the outer teardown honours it; ACEs are still reverted, since this run applied them. Password rotation moved to immediately before the secret is committed. Resetting an adopted account's password at the top of provisioning meant every later step ran against an account whose password had been replaced with no copy stored. Any failure there left a live account authenticated by a password nothing on disk knew, and since the account pre-existed the rollback correctly declined to delete it, so the command path read the absent secret as "not provisioned" and fell back to the weaker backend for good. The two operations are now adjacent. The rollback also stops removing the secret when this run neither created the account nor rotated it, because that secret still works. Policy DenyWrite now reaches the principal ACL plan. The capability plan has always emitted these; the principal plan denied write only on protected metadata and read-only subpaths, so once the runner used a principal token a policy deny elsewhere was not enforced at all. Principal deny-read entries are materialized, matching the capability plan, so a path created after setup still gets a deny ACE. Logon-right revocation is keyed to the attempt rather than to success. Rights are added one at a time and the grant returns on first failure, so a partial grant left LSA entries behind pointing at a SID that deleting the account then made unresolvable. The ownership comment now carries the full workspace key. The account name holds only 11 characters of the digest, so two workspaces could derive one name and silently share an account, a secret and an ACL identity; a mismatch is now refused. Accounts provisioned before the key was recorded are still adopted. Also warns once on stderr when the opt-in is set and a provisioned principal cannot be used, rather than downgrading in silence.
Second round of review findings, both on the elevated setup path. The rollback revoked logon rights whenever they had been attempted, without regard to whether this run created the account. revokeWindowsSandboxLogonRights passes AllRights, which drops every right the account holds and deletes its LSA object outright. On an adopted principal that is not a rollback but destruction: a transient grant, secret-path or secret-write failure during a re-run stripped the SeBatchLogonRight and deny-logon rights an earlier setup had established, leaving exactly the broken-but-present principal this path exists to avoid. Revocation is now scoped to accounts this run created. The rights granted to an adopted account are the ones it is supposed to hold, so leaving them is the safe direction. A secret the current user cannot read now falls back instead of failing the command. The secret's DACL names whoever ran setup, so an operator who elevated with a separate administrative account, through runas or an over-the-shoulder UAC prompt, leaves a secret their ordinary account cannot open. That is the documented fail-soft case, and treating it as a hard error made every sandboxed command fail on a machine that was merely set up by a different admin. Permission errors from the removal path are deliberately still reported, since incomplete teardown is worth knowing about. Both are covered by injected-failure tests and fail if the guard is removed. The secret read is seamed to inject the permission error, because producing a real ERROR_ACCESS_DENIED needs DACL surgery and would test the platform rather than the mapping.
Four review findings on the elevated setup path. The principal had no access to the sandbox runtime root. permissionProfileWithRuntime appends that root to WriteRoots on every command and redirects HOME, GOCACHE, npm_config_cache and similar into it, but it lives under the user cache rather than the workspace, so the profile setup builds its ACL plan from never contains it. On the restricted-token path that costs nothing, since the child still runs as the caller. A principal is a separate local account with none of those rights, so every npm install, go build or pip install would have failed on a cache write with a bare ACCESS_DENIED and nothing naming the sandbox as the cause. Setup now resolves the same root and grants it. The derivation is extracted so both callers share it. If setup and prepareSandboxRuntime ever disagreed, the ACE would land on one directory while commands used another, which is the same failure with a harder diagnosis, so a test asserts the two agree. The git control-plane carveouts are materialized. .git/config and .git/hooks arrive as ReadOnlySubpaths, and applyWindowsACLPlan skips an absent target, so on a workspace where git had not run yet the deny ACEs were never written and the principal kept inherited write access once git created them. Command-time lookup verifies workspace ownership. The account name carries only 11 characters of the workspace digest; the comment carries all of it. Provisioning already refused a foreign account, but the command path resolved the name straight to a SID, so the workspace that lost a collision would have run as the other one's principal. SID resolution still runs first, so an absent account stays the unavailable sentinel rather than becoming a collision error. The gated round-trip test asserted a logon with the password from a second provisioning call. Rotation moved to the setup path, so that value is a fresh string the account never held. It now exercises the guarantee the setup path actually makes: the stored secret logs the principal on.
Adoption takes over an account whose name and ownership comment match, resets its password and hands it to the sandbox. An account that is also in Administrators, Power Users or Backup Operators would give the sandbox the rights it exists to withhold: rewriting the ACLs confining it, reading the secret locked to the invoking user, and stopping Zero. The name is derived rather than discovered, so an account can match without anyone intending it to. Membership is resolved by well-known SID rather than by group name, so a localised install where the group is Administratoren or Administrateurs is still recognised. Raised as a non-blocking follow-up in review; it is cheap enough to do now rather than track.
Materializing the git control-plane carveouts creates a missing target so
the deny-write ACE is in place before git first runs. It did that with
os.MkdirAll on the full path, which is right for .git/hooks and wrong for
.git/config: git wants a file there.
The consequence is worse than a mis-ACL'd path. On a fresh workspace
neither carveout exists — which is exactly the case materialization was
added for, so this is the common path rather than a corner — and elevated
setup would leave a directory where git's config file belongs:
warning: unable to access '<ws>/.git/config': Permission denied
fatal: unknown error occurred while reading the configuration files
git init then fails outright and the workspace is unusable.
Materialization now takes the shape from the carveout definition:
gitMetadataWriteCarveoutSpecs is the single source of truth and
gitMetadataWriteCarveouts derives its list from it, so a carveout cannot be
added in one place and have its shape forgotten in the other. A file target
gets its parent chain created and then an empty file; a directory target is
unchanged. A racing creator winning the O_EXCL is treated as success, since
the target existing is all materialization needed.
The regression test runs a real `git init` over the applied plan. It names
Guests as the principal rather than Everyone — with Everyone the deny ACE
also denies the test process and git fails for an unrelated reason, which
would have made the test pass for the wrong reason once the shape was fixed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Provisioning refuses an account that is already in Administrators, Power
Users or Backup Operators, but group membership is not frozen at setup. An
account provisioned clean can be added afterwards — by an operator, or by
an attacker who already has that access and would like the sandbox to hand
it back. Every command after that minted a token for a privileged account.
The re-check goes on the path that mints the token, not inside
lookupWindowsSandboxIdentity. Teardown resolves the same identity to revoke
its logon rights before deleting the account, so refusing there would leave
the very account this guards against permanently undeletable by Zero. The
command path already propagates anything that is not the not-provisioned
sentinel, so this surfaces to the operator instead of silently dropping
back to the restricted token.
Also make the gating test hermetic. Its "absent" case passed an empty map,
which falls through to os.Getenv, so a developer with the opt-in exported
saw a different result from CI:
ZERO_WINDOWS_SANDBOX_IDENTITY=1 go test ./internal/sandbox/
--- FAIL: TestWindowsSandboxIdentityGating/absent
enabled = true, want false for ""
Every case there supplies an explicit map entry, so the process variable is
now pinned to prove none of them consult it. The os.Getenv fallback is what
elevated setup actually runs on — it passes no Env — so it gets its own
table rather than riding on a case that also has a map entry.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
windowsPrincipalRevokePlan was implemented and tested but had no production caller, so nothing ever used it. applyWindowsACLPlan merges into the existing DACL, which means a re-run after narrowing a write root or shortening a deny list left the previous, wider ACEs sitting beside the new ones: the principal kept access the current policy no longer granted, and the sandbox silently widened as a result of being tightened. Setup does get the chance to notice — marker validation already refuses commands with "permission roots or deny lists changed" until setup runs again — so the re-apply is exactly where this belongs. The ACL step is extracted into applyWindowsPrincipalACLs: build the plan, revoke every ACE naming this trustee on the paths it touches, then apply. Revocation is by trustee rather than by remembered path, so it also clears grants written by an older version of Zero. Its rollback is discarded on purpose — the only failure path from here removes the principal outright, and restoring stale ACEs for an account about to be deleted is the residue this exists to prevent. Extracting it also makes the ordering testable without new provisioning seams, which #812 already adds with a different signature; adding them here would have collided on its rebase. Three tests: revocation actually drops a grant on a root that left the policy while keeping the one that stayed (asserted against the real DACL, counting deny ACEs as well as allow, since trustee revocation drops both); revoking a path that was never created is a no-op rather than an error; and the production path revokes BEFORE it applies. That last one is the one that matters — the first two pass just as happily with the call site deleted, and deleting it kills only the third. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…root
Two ways setup and the command path disagreed.
Teardown removed the secret, the LSA rights and the account, but never the
ACEs. Once the account is gone its SID stops resolving and every ACE naming
it becomes an orphaned raw-SID entry on the user's own tree — precisely the
residue the capability-SID model left behind and this one exists to avoid.
Revocation now runs while the SID still resolves, by trustee so it also
clears grants written by older versions. A revoke failure is deliberately
not fatal: a path the user has since deleted cannot be cleaned, and
refusing to remove the account over it would strand the principal and its
logon rights permanently, which is worse than a leftover ACE.
The runtime root was derived from filepath.Clean(WorkspaceRoots[0]) at
setup while Engine.resolveCommandDir cleans, absolutizes and then
EvalSymlinks it. That needs no symlink to diverge — Windows opens a path in
any casing and EvalSymlinks canonicalizes it:
setup sees c:\users\me\myworkspace
command sees C:\Users\me\MyWorkspace
so setup granted the principal one runtime tree and every command used
another. The grant that exists to make npm/go/pip caches writable landed
where nothing reads, surfacing as a bare ACCESS_DENIED on a cache write.
Both now go through canonicalWindowsSandboxWorkspaceRoot. An unresolvable
root falls back to the cleaned absolute path, matching the command path
rather than failing.
setupWindowsSandboxRuntimeRoot is split into derivation and creation so
teardown can name the tree without making directories on its way out.
The first version of the divergence test called the canonicalization helper
directly. It passed, and reverting setup to filepath.Clean — the actual bug
— left it passing. It now drives windowsSandboxRuntimeRootPath, and that
mutation fails it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… setup
Windows CI caught two faults in the previous commit. Both came from
canonicalizing one side of a pair.
setupWindowsSandboxRuntimeRoot resolved the workspace root while
prepareSandboxRuntime still only cleaned it, so the two disagreed exactly
where they had to agree. It passed locally because my temp paths were
already canonical; a Windows runner's TEMP is an 8.3 short path that
resolution expands:
setup granted ...\runtime\v1\5e2d212300ccdfba
commands use ...\runtime\v1\92c31f8cf536dfde
The canonicalization moves to canonicalSandboxWorkspaceRoot in
runtime_state.go and both sides call it, which is what the original fix
should have done.
The carveout shape was rebuilt from the RESOLVED write root and compared
against subpaths that cannot resolve, since .git/config does not exist at
setup and normalizeProfilePath falls back to Clean when EvalSymlinks fails.
Two spellings of the same path therefore missed the lookup and .git/config
went back to being created as a directory — the original bug, reintroduced
quietly by its own fix. gitMetadataCarveoutIsFile now matches on the
trailing segments, derived from the spec list so it cannot drift from it,
and no reconstructed absolute path is compared at all.
Both failures now have regression tests that reproduce the non-canonical
root by lowercasing, which needs no short name and no privilege. Reverting
either fix fails them.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
sandboxRuntimeRootFor compares the workspace root against the runtime root
it derives from the cache root, and falls back to a private temp tree when
the derived root would land inside the workspace. The previous commit
canonicalized only the workspace root, so that comparison ran on two
different spellings of the same path and the containment check missed:
macOS: /var/folders/... vs /private/var/folders/...
Windows: C:\Users\RUNNER~1\... vs C:\Users\runneradmin\...
The fallback never fired and the runtime tree was placed inside the
workspace it exists to stay out of. Both CI runners caught it; my box did
not, because its temp paths are already canonical and 8.3 alias creation is
disabled on the volume, so I could not reproduce either spelling locally.
Both inputs now go through canonicalSandboxWorkspaceRoot, on the
cross-platform path and the Windows setup path.
The regression test uses a symlink, which is the portable way to produce a
spelling only resolution reconciles — Clean cannot see through one. It
skips on Windows, where creating one needs privilege, and runs on the
platforms that caught the bug.
Two things about that test are deliberate. My first version used a
redundant-segment path, which Clean already normalizes, so reverting the
fix left it passing. My second resolved nothing before asserting, and
through the link the runtime root shares no textual prefix with the
workspace — it would have called a root sitting physically inside the
workspace "outside" and passed against the exact bug it exists for. It now
resolves before comparing.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The previous fix normalized both the workspace root and the cache root, and
macOS CI still failed the same way. EvalSymlinks fails outright when the
LEAF does not exist, and a cache root has not been created at the point it
is first normalized — so the workspace resolved (/var to /private/var)
while the cache root did not, and the containment check that decides
whether the runtime tree must move out of the workspace compared the two
anyway.
canonicalSandboxWorkspaceRoot now resolves the longest existing ancestor
and re-appends the remainder, so a path normalizes the same way whether or
not its final segments exist:
/var/.../001/.cache leaf missing, walk up
/var/.../001 resolves
/private/var/.../001/.cache
Terminates at the filesystem root, where it falls back to the cleaned
absolute path, and a path with no symlink anywhere along it is unchanged.
The regression test needs a symlink to produce a spelling only resolution
reconciles, so it skips on Windows — where creating one needs privilege —
and runs on the platforms that caught this. I could not reproduce either CI
spelling locally: this box's temp paths are already canonical and 8.3 alias
creation is disabled on the volume.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ontract
TestCanonicalWorkspaceRootFallsBackWhenResolutionFails asserted that a path
with missing segments came back as the plain cleaned path. That was the
behaviour before the ancestor walk, and Windows CI failed it correctly:
canonical("C:\Users\RUNNER~1\...\001\never-created\deeper")
= "C:\Users\runneradmin\...\001\never-created\deeper", want the cleaned path
The existing ancestor resolved and the missing remainder was re-appended,
which is precisely what the walk exists to do. The assertion now says that:
the result equals the canonical parent joined with the segments that do not
exist, and those segments survive rather than collapsing to the ancestor.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…rollback gaps Three findings from jatmn's review, all reachable only under the opt-in principal backend but all real. FILE_DELETE_CHILD is no longer granted. On a parent it authorises deleting a child whatever the child's own DACL says, so granting it on a write root handed back the carve-outs underneath: delete .git/config, recreate it, and the replacement inherits the grant with no deny of its own — restoring the credential.helper and core.hooksPath control the carve-out exists to prevent. It was granted to keep the mask symmetric with the deny mask, which is the wrong instinct: denying a capability is not a reason to grant it. The comment two lines up already made that argument for WRITE_DAC and WRITE_OWNER. DELETE alone still covers removing and renaming files inside the roots, which is what the grant is actually for — verified before removing it. Note this does NOT close the second route jatmn described: .git itself carries no ACE, so renaming the whole directory aside needs only DELETE. That needs a guard on .git and is not in this commit. ACL targets are now rejected when a PARENT is a reparse point. CreateFile resolves ancestors even with FILE_FLAG_OPEN_REPARSE_POINT, so the final-component check passed while elevated setup rewrote the DACL of an object outside the workspace. Junctions need no privilege to create, unlike symlinks, so this was reachable by exactly the unprivileged user the sandbox contains. GetFinalPathNameByHandle answers where the handle really landed, covering every component in one call instead of walking the path and racing between checks. The comparison is against the path's own resolved form, so a differently-cased or 8.3 spelling is still accepted. The revocation's rollback is returned instead of discarded. Discarding it was justified on the grounds that the only failure path removes the principal outright — true for a principal this run CREATED, false for one it ADOPTED, which #812 keeps alive on failure rather than destroying someone else's working account. The account survived with its previous ACEs stripped and the new ones rolled back: logged on, and unable to reach its own workspace. Teardown still discards it deliberately, since putting ACEs back on an account about to be deleted is the opposite of the point. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
windowsPrincipalTeardownPaths said the runtime root was "resolved without creating it, since teardown has no business making directories on its way out". That was my comment and it was false: it went through sandboxRuntimeRootFor, whose fallback calls os.MkdirTemp when the cache-derived root would land inside the workspace. So cleanup created a fresh temp directory, and a useless one — the fallback root is random per process and could never match the tree the commands actually used. sandboxRuntimeRootFor is split: deterministicSandboxRuntimeRoot computes the cache-derived path and says whether it is usable, creating nothing, and the existing resolver keeps the fallback on top of it. Teardown takes the pure one and simply has no runtime tree to revoke when it reports unusable, which is correct — there is no way to name the random root from here anyway. The first version of the test called the pure resolver directly. It passed, and reverting the call site to the creating one left it passing. It now drives windowsPrincipalTeardownPaths and counts temp-directory entries across the call, and that mutation fails it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The provisioning rollback removes the stored secret when this run created the account or rotated an adopted one's password, because in both cases what is on disk cannot authenticate and absent beats stale: the command path treats a missing secret as "not provisioned" and falls back, while a stale one fails the logon and reports a broken sandbox. That removal ignored its own error. When it failed, the invariant it exists to keep was not restored — a credential for a password that no longer works stayed on disk — and setup said nothing. The operator met a provisioned-but-unusable principal on the next command instead of hearing it from the run that broke it. undo now returns that one error and the five failure paths join it onto the error they were already returning, so the original cause and the cleanup failure both surface. The message names the file and what to do about it. The other undo steps still swallow: they leave residue, while this one leaves a credential. Reported by jatmn on #808.
2040a3c to
d84bc54
Compare
FILE_FLAG_OPEN_REPARSE_POINT only stops the FINAL path component being followed. materializeWindowsACLTarget built its target with os.MkdirAll and os.OpenFile on the pathname, and both resolve ancestors — so the ancestor reparse check on the subsequent no-follow re-open ran only after the objects already existed. An ordinary workspace owner needs no privilege to create a junction. Turning .git into one before elevated setup runs, with config absent, had setup create the target at a location of the attacker's choosing as Administrator. Rejecting it afterwards does not undo that, and the failure path removes only the final component, so every intermediate directory MkdirAll created outside the workspace survived permanently. Creation now goes through makeWindowsACLDirChainNoFollow, which walks up to the deepest existing ancestor and verifies it no-follow first. One check suffices for the whole chain above it because GetFinalPathNameByHandle answers for the entire resolved path. Missing components are then created one at a time, each re-verified immediately after creation, so a component swapped for a junction mid-walk is caught before anything lands underneath it. Taken over the relative-handle NtCreateFile route because x/sys/windows offers no ergonomic relative-create primitive, and this leaves a window of one component with an immediate post-create check rather than create-everything-then-verify. Reported by jatmn on #808.
|
@jatmn two things on the token-jail P1, one of which is separate from this PR. Your finding is confirmed, and your fix direction is right. I went to check whether restricting the principal token would actually close it, because I suspected it might not, and it does. One implementation note for when I do it: the ACL plan grants the workspace to The separate thing — I think there's a pre-existing gap in the restricted-token path on The control shows the jail works normally; the first shows an This is not caused by the principal work — it applies to the shipped restricted-token backend, so it predates this stack. Impact looks moderate rather than urgent: a default Windows install does not hand out many Everyone-writable directories, and you need to already be running a sandboxed command. But it does mean the fallback is not as fully jailed as the P1 assumes, so it seemed worth telling you rather than quietly folding into this PR. Happy to open it as its own issue, or take it to the security channel first if you'd rather not have the repro sitting in a PR thread — your call, you know the project's convention better than I do. Separately, the reparse-before-materialize P1 is fixed and pushed in |
The principal branch handed the raw LogonUser token to CreateProcessAsUser. That token is a full token for the account, so the sandboxed child kept every write its ambient memberships grant. The ACL plan can add grants and denies at named paths, but it cannot revoke what BUILTIN\Users, Authenticated Users or NT AUTHORITY\BATCH already allow elsewhere — so an opted-in command whose profile permitted writes only to the workspace and runtime roots could still write C:\Users\Public\Documents, which grants BATCH modify and which a batch logon therefore satisfies. The principal now gets its own identity AND the restricted token, not one or the other: reads stay confined by its ACEs, writes by the restricted-SID check. The principal's own SID joins the capability SIDs deliberately. applyWindowsPrincipalACLs grants the workspace to identity.SID rather than to a capability SID, so omitting it would leave the workspace grant matching nothing in the restricted list — a jail that locks out the inmate and no one else. The SID is read back from the token itself rather than threaded through the call, so it cannot drift from the identity actually running. Not fixed here, and separate from this finding: worldSID is unconditionally in the restricted-SID list, so any path whose DACL grants Everyone still satisfies the restricted check on both this path and the pre-existing fallback. That predates the principal work and is raised with the maintainers separately. Reported by jatmn on #808.
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] Revoke principal ACEs from roots removed by a policy change
internal/sandbox/windows_identity_runtime_windows.go:521
Re-setup buildsplanfrom the replacement profile and revokes the trustee only fromwindowsACLPlanPaths(plan). If a prior setup granted an extra write/read root and that root is later removed, it is absent from this plan and its inherited principal-SID ACE survives. The runner deliberately adds that SID to the restricted-token SID set, so the old grant continues to satisfy the write jail after the policy was narrowed. Persist/recover the previously applied target paths (and use their union with the new plan for setup and teardown) before accepting the updated marker; the new stale-ACE test currently supplies the old paths by hand rather than exercising this production path. -
[P1] Make materialization and rollback safe against replacement races
internal/sandbox/windows_acl_apply_windows.go:369
makeWindowsACLDirChainNoFollowcloses the checked handle before its pathnameos.Mkdir, and the file path similarly verifies the parent beforeos.OpenFileat line 322. A workspace owner can replace that parent with a junction in this interval, so elevated setup creates the next component outside the workspace before a later check notices. The failure cleanup also uses pathnameRemoveAll, which can follow the swapped junction and recursively delete its external target. Create relative to retained no-follow handles (and bind cleanup to those handles) rather than checking one pathname and creating/deleting through it later. -
[P1] Preserve the Git carve-outs when
.gitis replaced
internal/sandbox/windows_acl_apply_windows.go:257
The new inheritableDELETEgrant applies to the workspace root, while the deny ACEs are attached only to.git/configand.git/hooks. The principal can rename the whole.gitdirectory, create a replacement, and then write a fresh config/hooks tree that inherited the root allow but has no carve-out denies. That bypasses the protection forcredential.helper,core.hooksPath, and hooks; the author’s current comment acknowledges this route. Deny deletion/renaming of.gititself or otherwise ensure replacement metadata receives the deny entries, with an end-to-end ACL test. -
[P2] Carry the explicit principal opt-in through the elevated setup protocol
internal/sandbox/windows_setup.go:20
Commands honor an explicitZERO_WINDOWS_SANDBOX_IDENTITYfrom their serialized environment, but setup args/config carry no environment andcommandConfigsuppliesEnv == nil. The elevated helper therefore consults only its own process environment; UAC/runas can omit the variable, leaving setup to write the normal marker without provisioning a principal while a later command believes it opted in and silently falls back to the weaker same-user token. Serialize the intended opt-in in setup args/config, or reject/report a mismatch instead of treating it as a successful setup.
4 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 includesDELETEandFILE_DELETE_CHILD. 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