Skip to content

fix(sandbox): fix nested pipe creation under the Windows restricted token#456

Merged
kevincodex1 merged 2 commits into
Gitlawb:mainfrom
euxaristia:fix/windows-restricted-token-pipe-access
Jul 5, 2026
Merged

fix(sandbox): fix nested pipe creation under the Windows restricted token#456
kevincodex1 merged 2 commits into
Gitlawb:mainfrom
euxaristia:fix/windows-restricted-token-pipe-access

Conversation

@euxaristia

@euxaristia euxaristia commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

This touches the sandbox's security boundary. Please review the DACL change carefully.

I want to flag that up front rather than bury it: this modifies the default DACL of the restricted token every sandboxed Windows command runs under. I have verified it does not weaken the filesystem write-jail (details and repro below), but this is exactly the kind of change that deserves real scrutiny before merge, not a rubber stamp because the tests pass.

What

Any subprocess spawned from inside a process already running under the Windows sandbox's write-restricted token, that captures the child's output via a pipe, fails with ERROR_ACCESS_DENIED. This is not tool-specific: it reproduces with gh pr list (gh shells out to git and captures its output), with a bare Go program doing exec.Command(...).Output(), and with cmd.exe's own FOR /F ('...') construct, which does the identical CreatePipe+CreateProcess internally with zero external dependencies. All three fail identically before this fix and succeed after it.

Root cause

A WRITE_RESTRICTED token requires a WRITE-type access check to match both the normal enabled-SID grant and a separate grant to one of the token's restricted SIDs. CreateRestrictedToken does not modify the default DACL it inherits from the base token, so a pipe created with a default security descriptor (the common case: every language runtime's "spawn a subprocess and capture output" primitive does this) only carries ACEs for the base identity, none of which are in the restricted-SID list, so the second check fails.

This is the same failure family already documented in this file for Schannel (SEC_E_NO_CREDENTIALS), but broader: it affects any pipe, event, mutex, or other kernel object created with default security by a process running under the token, not just TLS.

This is not specific to the unelevated tier from #427. The affected token-construction code (createWindowsRestrictedTokenFromBase) is shared by both the fully elevated and unelevated restricted-token paths. The elevated setup only changes whether WFP filters and ACLs are pre-provisioned, not the shape of the token processes actually run under. This bug predates that work and, as far as I can tell, has been present since the restricted-token sandbox was first built.

The fix, and why I believe it's safe

Add the token's own logon SID (already unconditionally present in its restricted-SID list, see the entries construction in createWindowsRestrictedTokenFromBase) to the token's default DACL, so objects it creates without an explicit security descriptor automatically satisfy the extra check.

Why this shouldn't weaken anything:

  • Filesystem write-jailing is untouched. It's enforced by explicit ACL grants applied to workspace paths (windows_acl.go), created WITH an explicit security descriptor. The token's default DACL is only consulted when object creation supplies none. I verified this directly (see below).
  • The exposure is bounded to the same logon session. Only other processes running as the same signed-in user could, in principle, open a named kernel object the sandboxed process creates with default security. Anonymous pipes (the actual object type this fixes, and what gh/Go's os/exec/FOR /F all use) have no name and are reachable only via an inherited handle, so this is a no-op for the specific objects motivating the fix.
  • The logon SID was already trusted. It's already part of the token's own restricted-SID list; this doesn't introduce a new principal, it makes an existing one usable for object creation the same way it's already usable for the file-write jail.

I'm very open to a narrower or different mechanism here if you have one in mind. This is the standard fix for this exact WRITE_RESTRICTED-token gotcha (I believe it matches how Chromium's Windows sandbox handles the equivalent problem), but I don't want to presume it's the only acceptable approach for Zero.

Verification (Windows 11, before/after using a build without this fix for comparison)

Before:

  • FOR /F ('C:\Windows\System32\whoami.exe') fails with Win32 error 5 (ERROR_ACCESS_DENIED), surfaced by cmd.exe as the generic "is not recognized as an internal or external command".
  • gh pr list fails with failed to run git: pipe: Access is denied.
  • A bare Go exec.Command("cmd.exe", "/c", "echo", ...).Output() fails with pipe: Access is denied.

After: all three succeed. gh pr list returns real data.

Write-jail regression check (the part I'd most want a second pair of eyes on): a write attempted outside every granted root is still denied by the OS after the fix, identical to before. Verified both via a standalone write-attempt binary and via the existing TestWindowsUnelevatedRealSandboxSmoke integration test, unmodified, still passing.

Testing

  • New TestWindowsRestrictedTokenNestedPipeCapture (gated by ZERO_SANDBOX_REAL_SMOKE=1, like the existing real-hardware tests) pins the FOR /F case using only cmd.exe, no external dependency, so CI-adjacent environments can run it without gh/git present.
  • go vet ./internal/sandbox/... clean.
  • Full internal/sandbox suite: passing, excluding failures that are pre-existing and identical on unmodified main in my environment (they assume paths that don't hold under a deeply nested working directory, unrelated to this change; happy to point at specific test names if useful).
  • go build ./... clean.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features
    • Added a Windows real-sandbox smoke test that verifies nested command execution and reliably capturing subprocess output under highly restrictive settings (runs when real smoke mode is enabled).
  • Bug Fixes
    • Improved Windows restricted-token sandbox behavior by broadening default access so self-created kernel objects work correctly, preventing failures and empty captured outputs in nested scenarios.

…oken

Any subprocess spawned FROM WITHIN a process already running under Zero's
write-restricted token, that captures the child's output via a pipe, failed
with ERROR_ACCESS_DENIED. Reproduces with `gh pr list` (gh shells out to git
and captures its output), with a bare Go program doing exec.Command(...).
Output(), and with cmd.exe's own FOR /F ('...') construct, which uses the
identical CreatePipe+CreateProcess pattern internally with no external tools
involved.

Root cause: a WRITE_RESTRICTED token requires a WRITE-type access check to
match BOTH the normal enabled-SID grant and a separate grant to one of the
token's restricted SIDs. CreateRestrictedToken does not modify the default
DACL it inherits from the base token, so a pipe created with a default
security descriptor (the common case; every language runtime's "spawn a
subprocess and capture output" primitive does this) only carries ACEs for
the base identity, none of which are in the restricted-SID list, so the
second check fails. This is the same failure family already documented for
Schannel/SEC_E_NO_CREDENTIALS, but broader in scope: it affects any pipe,
event, mutex, or other kernel object created with default security by a
process running under the token.

Fix: add the token's own logon SID (already unconditionally present in its
restricted-SID list) to the token's default DACL, so objects it creates
without an explicit security descriptor automatically satisfy the extra
check. This does not touch filesystem access: NTFS write-jailing is
enforced by the explicit ACL grants applied to workspace paths, created
WITH an explicit security descriptor, and the token's default DACL is only
consulted when none is supplied. Exposure is bounded to the same logon
session (only other processes running as the same signed-in user could, in
principle, open a NAMED object the sandboxed process creates with default
security; anonymous pipes have no name and are reachable only via an
inherited handle, so this is a no-op for the actual objects this fixes).

This is not specific to the unelevated tier added in Gitlawb#427 - the affected
token-construction code is shared by both the fully elevated and unelevated
restricted-token paths, so this predates that work.

Verified on Windows 11 (before/after, using an unfixed build for
comparison):
- Before: FOR /F fails with Win32 error 5 (ERROR_ACCESS_DENIED), masked
  behind cmd.exe's generic "not recognized" message; `gh pr list` fails
  with "failed to run git: pipe: Access is denied."; a bare Go nested
  exec.Command(...).Output() fails with "pipe: Access is denied."
- After: all three succeed; `gh pr list` returns real PR data.
- Write-jail regression check: a write outside every granted root is still
  denied by the OS after the fix (unchanged from before).

New TestWindowsRestrictedTokenNestedPipeCapture (ZERO_SANDBOX_REAL_SMOKE=1)
pins the FOR /F case using only cmd.exe, no external dependency. go vet and
the full internal/sandbox suite are clean (excluding pre-existing failures
already present identically on unmodified main, unrelated to this change).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jul 3, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 9a37fb79-e5f0-4fbd-b1e5-226cdc2a262f

📥 Commits

Reviewing files that changed from the base of the PR and between 8efe76b and b9d1286.

📒 Files selected for processing (1)
  • internal/sandbox/windows_token_windows.go

Walkthrough

Adds a Windows restricted-token DACL broadening step using SetEntriesInAclW, then wires it into token creation. Also adds a gated Windows smoke test that runs cmd.exe with FOR /F to capture whoami.exe output into a marker file.

Changes

Windows Restricted Token DACL and Smoke Test

Layer / File(s) Summary
SetEntriesInAclW binding and DACL update
internal/sandbox/windows_token_windows.go
Declares procSetEntriesInAclW, adds helpers to read and write a token default DACL, and wraps SetEntriesInAclW to merge explicit access entries into the existing ACL with native allocation cleanup.
Restricted token creation and smoke test
internal/sandbox/windows_token_windows.go, internal/sandbox/runner_windows_integration_test.go
Calls the DACL broadening step after privilege enablement during restricted token creation, then adds a gated Windows smoke test that runs cmd.exe with FOR /F to capture whoami.exe output into a marker file and checks for non-empty output.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant createRestrictedToken
  participant broadenWindowsRestrictedTokenDefaultDacl
  participant windowsTokenDefaultDacl
  participant setEntriesInACL
  participant SetEntriesInAclW
  participant windowsSetTokenDefaultDacl
  participant TestWindowsRestrictedTokenNestedPipeCapture
  participant cmd.exe
  participant whoami.exe
  participant MarkerFile

  createRestrictedToken->>broadenWindowsRestrictedTokenDefaultDacl: broaden default DACL
  broadenWindowsRestrictedTokenDefaultDacl->>windowsTokenDefaultDacl: read current TokenDefaultDacl
  broadenWindowsRestrictedTokenDefaultDacl->>setEntriesInACL: merge explicit access for logon SID
  setEntriesInACL->>SetEntriesInAclW: call native ACL merge
  SetEntriesInAclW-->>setEntriesInACL: merged ACL
  broadenWindowsRestrictedTokenDefaultDacl->>windowsSetTokenDefaultDacl: write updated TokenDefaultDacl
  TestWindowsRestrictedTokenNestedPipeCapture->>cmd.exe: run FOR /F batch script
  cmd.exe->>whoami.exe: execute whoami.exe
  whoami.exe-->>cmd.exe: identity output
  cmd.exe->>MarkerFile: write captured output
  TestWindowsRestrictedTokenNestedPipeCapture->>MarkerFile: read and assert non-empty
Loading

Suggested reviewers: jatmn, gnanam1990, kevincodex1, anandh8x

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main fix: nested pipe creation under the Windows restricted-token sandbox.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_token_windows.go`:
- Around line 229-241: The error handling in SetEntriesInAclW is using callErr
when the API already returns the Win32 failure code in ret. Update the failure
path in the SetEntriesInAclW call site to treat any nonzero ret as the actual
error and return syscall.Errno(ret) instead of relying on callErr, keeping the
existing success path and runtime.KeepAlive(entries) 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: 3d11f9d1-9760-4368-95b8-5abf6fdadd73

📥 Commits

Reviewing files that changed from the base of the PR and between bdb1b0e and 8efe76b.

📒 Files selected for processing (2)
  • internal/sandbox/runner_windows_integration_test.go
  • internal/sandbox/windows_token_windows.go

Comment thread internal/sandbox/windows_token_windows.go Outdated

@Vasanthdev2004 Vasanthdev2004 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Request changes — the fix is right and worth shipping, but there's a real memory-safety defect to close first

Worth it: yes, clearly. This fixes a serious, already-shipped Windows bug: under Zero's restricted-token sandbox, any tool that shells out and captures a child's output — gh invoking git, Go's os/exec … .Output(), even cmd.exe's FOR /F — fails with ERROR_ACCESS_DENIED because the sandboxed process can't create the anonymous pipe it needs. That silently breaks a broad class of agent tool calls on Windows. The root-cause writeup (a WRITE_RESTRICTED token requires a restricted-SID match on the second access check, and CreateRestrictedToken leaves the inherited default DACL without a restricted-SID ACE) is accurate, and adding the token's own already-trusted logon SID to the default DACL is the standard, minimal remedy — it matches how Chromium's sandbox handles the same gotcha. Nice work @euxaristia.

Security is sound — the write-jail is not weakened. I checked: the filesystem jail is enforced by explicit ACLs on workspace paths (windows_acl.go), created with an explicit security descriptor; the token default DACL is only consulted for objects created without one. The logon SID is already unconditionally in the token's restricted-SID list, so no new principal is introduced, and setup fails closed (token Close()d on any error). Agreed on your assessment.

🔴 Blocker — dangling buffer fed into SetEntriesInAclW (I confirmed this by reading the code)

windowsTokenDefaultDacl returns dacl (= info.DefaultDacl), which points into the local buf filled by GetTokenInformation. But runtime.KeepAlive(buf) is inside that function, ending at its return — so once it returns, buf is unreferenced. In broadenWindowsRestrictedTokenDefaultDacl, oldDacl is then dereferenced by SetEntriesInAclW after intervening allocations (the EXPLICIT_ACCESS literal, TrusteeValueFromSID) that are GC-safe points — nothing keeps buf alive across that window. Go's current GC is non-moving so the address is stable and it didn't manifest in testing, but a collected/reused buf could feed a corrupted DACL into SetEntriesInAclW in security-boundary code. (Notably you did runtime.KeepAlive the SID and other buffers elsewhere — this one spot was missed.)

Fix: return buf alongside the ACL and runtime.KeepAlive(buf) at the call site until after SetEntriesInAclW returns (or deep-copy the ACL before returning).

Minor (the outstanding CodeRabbit item)

SetEntriesInAclW returns its error code in the return value and doesn't set thread last-error, so the callErr branch in setEntriesInACL is misleading. Prefer return nil, fmt.Errorf("SetEntriesInAclW: %w", syscall.Errno(ret)) and drop the callErr check. Cosmetic (still fail-closed), but it's the open review item.

Nit: GENERIC_ALL is wider than the WRITE-type access the second check needs; a narrower grant would be marginally more principled (delta is negligible given the same-session/same-SID bounding).

Verified on Windows: go build ./..., go vet, go test ./internal/sandbox/... all clean/green; the new TestWindowsRestrictedTokenNestedPipeCapture is env-gated (ZERO_SANDBOX_REAL_SMOKE=1) and skips cleanly, so no #414-class break. With the dangling-buffer fix in, I'm happy to approve — this is a valuable fix.

@gnanam1990 gnanam1990 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: needs-changes

REGRESSION RISK: high

This PR modifies the default DACL of the Windows restricted token that every sandboxed command runs under. The fix itself (adding the logon SID to the default DACL so WRITE_RESTRICTED token processes can create anonymous pipes) is correct and security-sound — the filesystem write-jail is enforced by explicit ACLs, not the default DACL, and the logon SID is already in the token's restricted-SID list. However, there is an unresolved memory-safety defect in the implementation that must be fixed before merge.

BUILD / TEST

  • go build ./... — pass (Windows-only files compile via build constraints on macOS)
  • go vet ./internal/sandbox/... — pass
  • gofmt -l on changed files — clean
  • TestWindowsRestrictedTokenNestedPipeCapture is env-gated (ZERO_SANDBOX_REAL_SMOKE=1) and skips cleanly on non-Windows
  • CI all green including Windows Smoke — but the memory-safety defect below does not manifest in testing because Go's current GC is non-moving; it is a latent correctness issue, not a test-detectable one
  • Could not verify the Windows sandbox behavior locally (macOS host); relied on CI Windows job and code inspection

CONTRIBUTING

No linked issue. The author is a community contributor (euxaristia). Per CONTRIBUTING.md, community PRs require an approved parent issue with the issue-approved label. No such issue is linked. Process violation, but the change addresses a real shipped Windows bug. Maintainer's call.

FINDINGS

  1. Blocker — internal/sandbox/windows_token_windows.go:199-213 — dangling buffer fed into SetEntriesInAclW. windowsTokenDefaultDacl returns dacl (which is info.DefaultDacl), a pointer into the local buf filled by GetTokenInformation. The runtime.KeepAlive(buf) call is inside the function and ends at its return, so once the function returns, buf is unreferenced. In broadenWindowsRestrictedTokenDefaultDacl, oldDacl is then passed to setEntriesInACLSetEntriesInAclW after intervening allocations (EXPLICIT_ACCESS literal, TrusteeValueFromSID) that are GC-safe points. Nothing keeps buf alive across that window. A collected/reused buf could feed a corrupted DACL into SetEntriesInAclW in security-boundary code. Go's current non-moving GC means the address is stable and this hasn't manifested, but it is a real defect.

    Fix: return buf alongside the ACL and runtime.KeepAlive(buf) at the call site until after SetEntriesInAclW returns, or deep-copy the ACL before returning.

  2. Minor — internal/sandbox/windows_token_windows.go:237-245SetEntriesInAclW error handling. SetEntriesInAclW returns its error code in the return value, not via thread last-error. The callErr branch in setEntriesInACL is misleading. Prefer return nil, fmt.Errorf("SetEntriesInAclW: %w", syscall.Errno(ret)) and drop the callErr check. Still fail-closed, but the error attribution is incorrect.

  3. Nit — internal/sandbox/windows_token_windows.go:175GENERIC_ALL is wider than needed. The WRITE_RESTRICTED token's second access check needs a WRITE-type match; a narrower grant would be marginally more principled. Delta is negligible given the same-session/same-SID bounding.

EXISTING REVIEWS

  • Vasanthdev2004 (CHANGES_REQUESTED): Identified the dangling-buffer blocker (finding #1 above), the SetEntriesInAclW error handling (finding #2), and the GENERIC_ALL nit (finding #3). Confirmed the security analysis is sound and the fix approach is correct. All findings are valid on the current head (8efe76b) — no new commits have been pushed since this review. Nothing was missed.
  • coderabbitai (CHANGES_REQUESTED): Flagged the same SetEntriesInAclW error handling issue. Valid on the current head.

All reviews are on the current head (8efe76b). No stale reviews. The blockers have not been addressed.

BOTTOM LINE

The fix is correct and valuable, but the dangling-buffer memory-safety defect in windowsTokenDefaultDacl must be resolved before this can merge; the other items are minor.

Addresses the blocker from review on this PR (Vasanthdev2004, gnanam1990,
coderabbitai): windowsTokenDefaultDacl returned a *windows.ACL that points
INTO the local buf byte slice backing TOKEN_DEFAULT_DACL - Windows embeds
the ACL data in the same allocation GetTokenInformation fills. The
function's own runtime.KeepAlive(buf) only covered buf up to that
function's own return; nothing kept it alive across the caller's later use.
In broadenWindowsRestrictedTokenDefaultDacl, the returned ACL pointer was
then dereferenced by native code in setEntriesInACL after intervening
allocations (the EXPLICIT_ACCESS literal, TrusteeValueFromSID) that are
GC-safepoint-eligible - a real use-after-free window in code that
constructs part of the sandbox's own restricted token, even though Go's
current non-moving GC meant it didn't manifest in testing.

windowsTokenDefaultDacl now returns the backing buffer alongside the ACL;
the caller holds runtime.KeepAlive(oldDaclBuf) until after
SetEntriesInAclW (the last point that dereferences it) returns.

Also from review: SetEntriesInAclW reports its Win32 error directly in the
return value, not via GetLastError, so setEntriesInACL now uses that
return value directly instead of the (potentially stale/unrelated) syscall
error. And narrowed the default-DACL grant from GENERIC_ALL to GENERIC_READ
| GENERIC_WRITE - the second WRITE_RESTRICTED access check only needs a
read/write match; GENERIC_ALL also implied DELETE/WRITE_DAC/WRITE_OWNER
that the pipe/event/mutex use case never needed.

Re-verified on Windows 11 with the fixed runner: nested pipe capture still
succeeds (pure Go exec.Command(...).Output(), and TestWindowsRestrictedTokenNestedPipeCapture
via go test itself), and the write-jail regression check (write denied
outside every granted root, allowed inside) is unchanged. Full
internal/sandbox suite green.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@euxaristia

Copy link
Copy Markdown
Contributor Author

Pushed a fix for the dangling-buffer blocker, thank you both for catching it (and for the precise root-cause writeup, that made it a fast fix rather than a hunt).

The fix: windowsTokenDefaultDacl now returns the backing buffer alongside the ACL pointer, and broadenWindowsRestrictedTokenDefaultDacl holds runtime.KeepAlive(oldDaclBuf) until after SetEntriesInAclW (the last point that actually dereferences it) returns, instead of the KeepAlive being scoped to the wrong function entirely.

Also took the two smaller items while I was in there:

  • setEntriesInACL now uses SetEntriesInAclW's return value directly for the error, per the CodeRabbit finding, since that API reports failure in its return value rather than via GetLastError.
  • Narrowed the default-DACL grant from GENERIC_ALL to GENERIC_READ | GENERIC_WRITE, per the nit. The second access check only needs a read/write match; GENERIC_ALL was granting more than the pipe/event/mutex case needs.

Re-verified on real Windows 11 with the fixed build: nested pipe capture still succeeds and the write-jail regression check is unchanged (write denied outside every granted root, allowed inside), same as the original verification. Full internal/sandbox suite green, including TestWindowsRestrictedTokenNestedPipeCapture run for real via go test with ZERO_SANDBOX_REAL_SMOKE=1.

@euxaristia

Copy link
Copy Markdown
Contributor Author

The dangling-buffer memory-safety issue flagged in review is fixed in b9d1286: the ACL buffer's KeepAlive is now scoped past the SetEntriesInAclW call site instead of ending before it. CodeRabbit re-reviewed after the fix and approved.

I don't have permission to formally re-request review from non-collaborators, so flagging here directly. @Vasanthdev2004 @gnanam1990 if either of you has time to take another look, I'd appreciate it. All CI checks are currently green.

@euxaristia

Copy link
Copy Markdown
Contributor Author

All CodeRabbit and reviewer blockers addressed in commit b9d1286 (dangling buf fixed with KeepAlive, error handling updated, narrowed permissions).

Please re-review @Vasanthdev2004 @gnanam1990

Thanks!

@euxaristia

Copy link
Copy Markdown
Contributor Author

Re-review requested on head b9d1286 — this commit addresses the memory-safety blocker from Vasanthdev2004 and gnanam1990 (reviews were on 8efe76b).

What changed in b9d1286:

  1. Dangling buffer (blocker): windowsTokenDefaultDacl now returns the backing buf alongside the ACL; broadenWindowsRestrictedTokenDefaultDacl holds runtime.KeepAlive(oldDaclBuf) until after SetEntriesInAclW returns.
  2. SetEntriesInAclW errors: setEntriesInACL now uses the API return value (syscall.Errno(ret)) instead of thread last-error.
  3. Permissions narrowed: default-DACL grant is GENERIC_READ | GENERIC_WRITE instead of GENERIC_ALL.

Verification: CI all green (including Windows smoke). CodeRabbit approved on b9d1286. Write-jail behavior unchanged per PR description.

Please re-review when you have a moment. If anything is still off, I will fix promptly.

@Vasanthdev2004 Vasanthdev2004 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approve — updating my earlier review; every blocker I raised is fixed in HEAD

Re-reviewed as part of the Windows-sandbox cluster. My earlier CHANGES_REQUESTED (and @gnanam1990's) were filed against the old commit 8efe76bthey're now stale. Current HEAD b9d1286 addresses every item and CodeRabbit re-approved; superseding my review with this approval.

Worth merging: yes. This fixes a serious already-shipped Windows bug — under the WRITE_RESTRICTED sandbox token the process can't create the anonymous pipe that nested output-capture needs (ghgit, os/exec .Output(), cmd FOR /F), so those silently fail. The fix adds the token's own logon SID to its default DACL; I confirmed the write-jail is unaffected because it rides on explicit per-file security descriptors, not the token DACL. Isolated to windows_token_windows.go — zero rebase interaction with the rest of the cluster.

Two minor, non-blocking notes for a fast-follow:

  • Reuse the existing wrapper. procSetEntriesInAclW is declared manually with a comment saying SetEntriesInAclW is "not wrapped by x/sys/windows" — but windows.ACLFromEntries is exactly that wrapper and is already used in this package (windows_acl_apply_windows.go:122). The manual version is correct but duplicative, and the comment is inaccurate.
  • CI doesn't actually exercise the fix. TestWindowsRestrictedTokenNestedPipeCapture is gated on ZERO_SANDBOX_REAL_SMOKE, which no workflow sets, so it's skipped in CI — the end-to-end behavior is validated only by your manual Windows run. The token-construction path is still exercised by the windows-latest smoke (so no regression risk), but the fix's payoff isn't automatically gated.

Governance: still no linked issue-approved issue per CONTRIBUTING — maintainer's call. Approving on the merits; @gnanam1990's stale CHANGES_REQUESTED should be dismissed so this isn't blocked on resolved concerns.

@euxaristia

Copy link
Copy Markdown
Contributor Author

The dangling-buffer blocker and the SetEntriesInAclW error-handling nit were both fixed in b9d1286: windowsTokenDefaultDacl now returns the backing buffer alongside the ACL, and the caller holds runtime.KeepAlive(oldDaclBuf) until after SetEntriesInAclW returns. Vasanthdev2004 and CodeRabbit already re-reviewed and approved on that commit.

@gnanam1990 your CHANGES_REQUESTED is the only one still outstanding, could you take another look?

@euxaristia

Copy link
Copy Markdown
Contributor Author

@gnanam1990 Flagging since I can't re-request your review directly: the dangling-buffer blocker, the SetEntriesInAclW error-handling issue, and the GENERIC_ALL scope you and Vasanth both flagged are fixed in b9d1286 (windowsTokenDefaultDacl now returns the backing buffer and keeps it alive across the native call, the ACL merge uses the syscall's own return code, and the grant is GENERIC_READ|GENERIC_WRITE instead of GENERIC_ALL). Both coderabbitai and Vasanthdev2004 re-reviewed and approved that commit. Would appreciate another look when you have a chance.

@gnanam1990 gnanam1990 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approve — superseding my earlier CHANGES_REQUESTED (it was on 8efe76b; b9d1286 resolves every item)

Re-reviewed on HEAD b9d1286. All three findings from my prior review are correctly addressed:

  • Blocker (dangling buffer → use-after-scope): fixed exactly right. windowsTokenDefaultDacl now returns the backing []byte alongside the ACL, and the caller runtime.KeepAlive(oldDaclBuf)s it after setEntriesInACL returns — so the buffer stays reachable across the native SetEntriesInAclW dereference. No use-after-scope window remains.
  • Minor (error attribution): now fmt.Errorf("SetEntriesInAclW: %w", syscall.Errno(ret)) on ret != 0, with an accurate comment that the API reports via its return value, not thread last-error.
  • Nit (GENERIC_ALL too wide): narrowed to GENERIC_READ | GENERIC_WRITE (drops DELETE/WRITE_DAC/WRITE_OWNER) — tighter than the original target.

What I verified this pass:

  • Cross-compiled the Windows-only code (a native build excludes it): GOOS=windows go build ./... and GOOS=windows go vet ./internal/sandbox/... both clean; gofmt clean.
  • Memory safety: both runtime.KeepAlives (oldDaclBuf past setEntriesInACL, entries past the syscall) close the use-after-scope windows; logonSID stays live as a broaden parameter through the call; defer LocalFree(newDacl) runs after windowsSetTokenDefaultDacl (copy-then-free is safe).
  • Security boundary: the change only adds the token's already-trusted logon SID to its default DACL; the filesystem write-jail rides on explicit per-file security descriptors, which the default DACL doesn't touch — so the jail is not weakened, and narrowing to READ|WRITE tightens it. Confirms the earlier analysis.
  • CI green, including Smoke (windows-latest).

Non-blocking fast-follows (agreeing with Vasanthdev2004):

  1. windows.ACLFromEntries already wraps SetEntriesInAclW and is used in-package (windows_acl_apply_windows.go:122); the manual procSetEntriesInAclW is redundant and its "not wrapped by x/sys/windows" comment is inaccurate — worth a cleanup later.
  2. The new TestWindowsRestrictedTokenNestedPipeCapture is gated on ZERO_SANDBOX_REAL_SMOKE=1, which no workflow sets, so the fix's end-to-end behavior isn't exercised in CI (token construction is still covered by the windows smoke, so no regression risk).

Solid fix for a real, already-shipped Windows bug. Nice work @euxaristia. Governance note stands (no linked issue-approved issue per CONTRIBUTING) — maintainer's call, not a technical blocker.

@kevincodex1 kevincodex1 merged commit 563a6db into Gitlawb:main Jul 5, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants