Skip to content

Commit 563a6db

Browse files
euxaristiaclaude
andauthored
fix(sandbox): fix nested pipe creation under the Windows restricted token (#456)
* fix(sandbox): fix nested pipe creation under the Windows restricted token 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 #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> * fix(sandbox): fix dangling-buffer use-after-scope in default DACL merge 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> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 parent 2312abe commit 563a6db

2 files changed

Lines changed: 196 additions & 0 deletions

File tree

‎internal/sandbox/runner_windows_integration_test.go‎

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -190,6 +190,67 @@ func TestWindowsUnelevatedRealSandboxSmoke(t *testing.T) {
190190
}
191191
}
192192

193+
// TestWindowsRestrictedTokenNestedPipeCapture pins the fix in
194+
// broadenWindowsRestrictedTokenDefaultDacl (windows_token_windows.go): a
195+
// nested subprocess spawned FROM WITHIN the sandboxed process, with its
196+
// output captured via a pipe, must succeed. Before the fix, the sandboxed
197+
// process's own attempt to create a pipe for such a subprocess failed with
198+
// ERROR_ACCESS_DENIED (Win32 error 5) — the WRITE_RESTRICTED token's extra
199+
// access check has no restricted-SID match against a freshly created pipe's
200+
// default security descriptor. This is exactly the pattern any tool that
201+
// shells out internally and captures output hits (`gh` invoking `git`, for
202+
// one; cmd.exe's own FOR /F does the identical CreatePipe+CreateProcess
203+
// dance internally, so it reproduces the bug with no external dependency
204+
// beyond cmd.exe itself).
205+
func TestWindowsRestrictedTokenNestedPipeCapture(t *testing.T) {
206+
if os.Getenv("ZERO_SANDBOX_REAL_SMOKE") != "1" {
207+
t.Skip("set ZERO_SANDBOX_REAL_SMOKE=1 to run real Windows sandbox smoke tests")
208+
}
209+
runnerExe := realSmokeExecutable(t, "ZERO_WINDOWS_COMMAND_RUNNER_EXE", WindowsSandboxCommandRunnerName)
210+
211+
root := t.TempDir()
212+
sandboxHome := filepath.Join(root, ".zero-sandbox")
213+
profile := PermissionProfile{
214+
FileSystem: FileSystemPolicy{
215+
Kind: FileSystemRestricted,
216+
ReadRoots: []string{root},
217+
WriteRoots: []WritableRoot{{Root: root, ProtectedMetadataNames: []string{".git", ".zero", ".agents"}}},
218+
IncludePlatformRoots: true,
219+
AllowTemp: true,
220+
},
221+
Network: NetworkPolicy{Mode: NetworkDeny},
222+
}
223+
config := WindowsSandboxCommandArgsOptions{
224+
SandboxHome: sandboxHome,
225+
CommandCWD: root,
226+
WorkspaceRoots: []string{root},
227+
PermissionProfile: profile,
228+
SandboxLevel: WindowsSandboxLevelUnelevated,
229+
}
230+
231+
marker := filepath.Join(root, "nested-pipe-marker.txt")
232+
script := filepath.Join(root, "nested-pipe.cmd")
233+
// FOR /F drives cmd.exe's own internal CreatePipe+CreateProcess capture
234+
// of the quoted command's output — the exact mechanism this fix targets.
235+
// The full path to whoami.exe sidesteps PATH resolution inside the
236+
// sandboxed process's minimal environment.
237+
scriptBody := "for /F %%i in ('C:\\Windows\\System32\\whoami.exe') do echo %%i> " + marker + "\r\n"
238+
if err := os.WriteFile(script, []byte(scriptBody), 0o644); err != nil {
239+
t.Fatalf("write nested-pipe script: %v", err)
240+
}
241+
242+
runWindowsRealSmokeCommand(t, runnerExe, config, []string{
243+
"cmd.exe", "/d", "/s", "/c", script,
244+
}, 0)
245+
captured, err := os.ReadFile(marker)
246+
if err != nil {
247+
t.Fatalf("read nested-pipe marker: %v", err)
248+
}
249+
if strings.TrimSpace(string(captured)) == "" {
250+
t.Fatalf("nested-pipe marker is empty; FOR /F failed to capture the subprocess's output")
251+
}
252+
}
253+
193254
func realSmokeExecutable(t *testing.T, envKey string, fallbackName string) string {
194255
t.Helper()
195256
if path := os.Getenv(envKey); path != "" {

‎internal/sandbox/windows_token_windows.go‎

Lines changed: 135 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,11 @@ const (
2121

2222
var procCreateRestrictedToken = windows.NewLazySystemDLL("advapi32.dll").NewProc("CreateRestrictedToken")
2323

24+
// procSetEntriesInAclW merges new ACEs into an existing ACL. Not wrapped by
25+
// x/sys/windows (its internal setEntriesInAcl is unexported), so it is
26+
// declared directly the same way procCreateRestrictedToken is.
27+
var procSetEntriesInAclW = windows.NewLazySystemDLL("advapi32.dll").NewProc("SetEntriesInAclW")
28+
2429
type windowsLocalSID struct {
2530
sid *windows.SID
2631
}
@@ -122,9 +127,139 @@ func createWindowsRestrictedTokenFromBase(base windows.Token, capabilitySIDs []w
122127
_ = restricted.Close()
123128
return 0, err
124129
}
130+
if err := broadenWindowsRestrictedTokenDefaultDacl(restricted, sidFromBytes(logonSID)); err != nil {
131+
_ = restricted.Close()
132+
return 0, err
133+
}
134+
runtime.KeepAlive(logonSID)
125135
return restricted, nil
126136
}
127137

138+
// broadenWindowsRestrictedTokenDefaultDacl adds the token's own logon SID to
139+
// its default DACL (the DACL applied to any kernel object the token's process
140+
// creates WITHOUT an explicit security descriptor, e.g. anonymous pipes,
141+
// events, mutexes).
142+
//
143+
// A WRITE_RESTRICTED token requires a WRITE-type access check to match BOTH
144+
// the normal enabled-SID grant AND a SEPARATE grant to one of the token's
145+
// restricted SIDs. CreateRestrictedToken does not touch the default DACL it
146+
// inherits from the base token, so newly created objects only carry ACEs for
147+
// the base token's normal identity (owner, SYSTEM, Administrators) — none of
148+
// which are in the restricted-SID list — so the second check fails. This
149+
// breaks anything that creates a pipe/handle for itself with default
150+
// security, which is exactly what any tool using a language runtime's
151+
// "spawn a subprocess and capture its output" primitive does (Go's os/exec,
152+
// and by extension `gh`, and many other CLI tools that shell out
153+
// internally): CreatePipe() succeeds, but the creating process's own token
154+
// then fails to actually read/write the handle it just made.
155+
//
156+
// The logon SID is already unconditionally present in this token's
157+
// restricted-SID list (see createWindowsRestrictedTokenFromBase), so adding
158+
// it here — and ONLY it — to the default DACL is the minimal change that
159+
// satisfies the WRITE_RESTRICTED extra check for self-created objects. It
160+
// does not touch file-system access: NTFS write-jailing is enforced by the
161+
// explicit ACL grants applied to workspace paths (windows_acl.go), which are
162+
// created WITH an explicit security descriptor and never consult the token's
163+
// default DACL. The exposure is bounded to the same logon session (other
164+
// processes running as the same signed-in user could, in principle, open a
165+
// NAMED kernel object the sandboxed process creates with default security;
166+
// anonymous pipes — the actual object type this fixes — have no name and are
167+
// only reachable via an inherited handle, so this is a no-op for them).
168+
func broadenWindowsRestrictedTokenDefaultDacl(token windows.Token, logonSID *windows.SID) error {
169+
oldDacl, oldDaclBuf, err := windowsTokenDefaultDacl(token)
170+
if err != nil {
171+
return fmt.Errorf("read token default DACL: %w", err)
172+
}
173+
// The second (WRITE-type) access check only needs a read/write match; a
174+
// full GENERIC_ALL grant (which also implies DELETE, WRITE_DAC, and
175+
// WRITE_OWNER) is broader than the pipe/event/mutex use case this exists
176+
// for actually requires.
177+
access := []windows.EXPLICIT_ACCESS{{
178+
AccessPermissions: windows.GENERIC_READ | windows.GENERIC_WRITE,
179+
AccessMode: windows.GRANT_ACCESS,
180+
Inheritance: windows.NO_INHERITANCE,
181+
Trustee: windows.TRUSTEE{
182+
MultipleTrusteeOperation: windows.NO_MULTIPLE_TRUSTEE,
183+
TrusteeForm: windows.TRUSTEE_IS_SID,
184+
TrusteeType: windows.TRUSTEE_IS_USER,
185+
TrusteeValue: windows.TrusteeValueFromSID(logonSID),
186+
},
187+
}}
188+
newDacl, err := setEntriesInACL(access, oldDacl)
189+
// oldDacl points INTO oldDaclBuf (see windowsTokenDefaultDacl) and is
190+
// dereferenced by native code inside setEntriesInACL, above. Nothing in Go
191+
// references oldDaclBuf once windowsTokenDefaultDacl returned, so without
192+
// this KeepAlive it is eligible for GC the moment an allocation in this
193+
// function (the EXPLICIT_ACCESS literal, TrusteeValueFromSID) hits a
194+
// safepoint, before the native call ever reads it - a real, if narrow,
195+
// use-after-free window in security-boundary code.
196+
runtime.KeepAlive(oldDaclBuf)
197+
if err != nil {
198+
return fmt.Errorf("merge token default DACL: %w", err)
199+
}
200+
defer func() { _, _ = windows.LocalFree(windows.Handle(unsafe.Pointer(newDacl))) }()
201+
if err := windowsSetTokenDefaultDacl(token, newDacl); err != nil {
202+
return fmt.Errorf("set token default DACL: %w", err)
203+
}
204+
return nil
205+
}
206+
207+
type windowsTokenDefaultDaclInfo struct {
208+
DefaultDacl *windows.ACL
209+
}
210+
211+
// windowsTokenDefaultDacl returns the token's current default DACL along with
212+
// the buffer backing it. TOKEN_DEFAULT_DACL embeds the ACL data inside the
213+
// same allocation GetTokenInformation fills, so the returned *windows.ACL is
214+
// only valid for as long as the returned buffer stays reachable: the caller
215+
// must runtime.KeepAlive(buf) until every native call that dereferences the
216+
// ACL has completed. A KeepAlive inside this function, ending at its own
217+
// return, would not protect any use by the caller after that.
218+
func windowsTokenDefaultDacl(token windows.Token) (*windows.ACL, []byte, error) {
219+
var size uint32
220+
err := windows.GetTokenInformation(token, windows.TokenDefaultDacl, nil, 0, &size)
221+
if err == nil || err != windows.ERROR_INSUFFICIENT_BUFFER {
222+
return nil, nil, fmt.Errorf("size TokenDefaultDacl: %w", err)
223+
}
224+
buf := make([]byte, size)
225+
if err := windows.GetTokenInformation(token, windows.TokenDefaultDacl, &buf[0], size, &size); err != nil {
226+
return nil, nil, fmt.Errorf("get TokenDefaultDacl: %w", err)
227+
}
228+
info := (*windowsTokenDefaultDaclInfo)(unsafe.Pointer(&buf[0]))
229+
return info.DefaultDacl, buf, nil
230+
}
231+
232+
func windowsSetTokenDefaultDacl(token windows.Token, dacl *windows.ACL) error {
233+
info := windowsTokenDefaultDaclInfo{DefaultDacl: dacl}
234+
err := windows.SetTokenInformation(token, windows.TokenDefaultDacl, (*byte)(unsafe.Pointer(&info)), uint32(unsafe.Sizeof(info)))
235+
runtime.KeepAlive(info)
236+
return err
237+
}
238+
239+
// setEntriesInACL merges entries into oldACL via Win32 SetEntriesInAclW,
240+
// returning a newly allocated ACL the caller must free with windows.LocalFree.
241+
func setEntriesInACL(entries []windows.EXPLICIT_ACCESS, oldACL *windows.ACL) (*windows.ACL, error) {
242+
if len(entries) == 0 {
243+
return nil, errors.New("setEntriesInACL requires at least one entry")
244+
}
245+
var newACL *windows.ACL
246+
// SetEntriesInAclW reports failure directly in its return value (ERROR_SUCCESS
247+
// on success, a Win32 error code otherwise); unlike many Win32 APIs it does not
248+
// set the thread's last-error, so the syscall trio's second/third return
249+
// values (which surface GetLastError) are not meaningful here.
250+
ret, _, _ := procSetEntriesInAclW.Call(
251+
uintptr(len(entries)),
252+
uintptr(unsafe.Pointer(&entries[0])),
253+
uintptr(unsafe.Pointer(oldACL)),
254+
uintptr(unsafe.Pointer(&newACL)),
255+
)
256+
runtime.KeepAlive(entries)
257+
if ret != 0 {
258+
return nil, fmt.Errorf("SetEntriesInAclW: %w", syscall.Errno(ret))
259+
}
260+
return newACL, nil
261+
}
262+
128263
func copyWindowsLogonSID(token windows.Token) ([]byte, error) {
129264
groups, err := token.GetTokenGroups()
130265
if err == nil {

0 commit comments

Comments
 (0)