From 7104ab977231d918361b1390a8d4badabaaeca15 Mon Sep 17 00:00:00 2001 From: ferguss Date: Mon, 19 Oct 2026 13:43:22 +1100 Subject: [PATCH] Assert exact Windows token and job handle cleanup --- .github/workflows/v2.yml | 2 + internal/platform/process_windows.go | 13 ++-- internal/platform/process_windows_test.go | 80 +++++++++++++++++++---- 3 files changed, 76 insertions(+), 19 deletions(-) diff --git a/.github/workflows/v2.yml b/.github/workflows/v2.yml index 4e4806f..26068dd 100644 --- a/.github/workflows/v2.yml +++ b/.github/workflows/v2.yml @@ -73,6 +73,8 @@ jobs: # missing GOCOVERDIR warning would contaminate psql probe output. go build -o $env:EP_SUPERVISOR ./cmd/embedded-postgres if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE } + go test -count=50 -timeout=2m -run '^TestWindowsNativeFailuresReleaseHandles$' ./internal/platform + if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE } go test -v -count=1 -p=1 -timeout=4m -run '^(TestWindowsRestrictedChildCanOpenItsToken|TestWindowsNativeFailuresReleaseHandles|TestWindowsUserDACLRejectsInvalidToken|TestParallelLifecycle|TestLifetimeCancellation|TestPersistentDataNeverErased|TestCLIParentPipe|TestCLIBackground|TestCLIExec|TestWindowsExecCancellation)$' . ./internal/platform ./internal/cli if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE } } diff --git a/internal/platform/process_windows.go b/internal/platform/process_windows.go index ea2fe54..2487fb6 100644 --- a/internal/platform/process_windows.go +++ b/internal/platform/process_windows.go @@ -28,12 +28,13 @@ type windowsCalls struct { sid func(string) (*syscall.SID, error) restrict func(syscall.Token, []syscall.SIDAndAttributes, *syscall.Token) (uintptr, error) secure func(syscall.Token) error + close func(syscall.Handle) error create func() (uintptr, error) limit func(uintptr, *extendedLimits) (uintptr, error) assign func(uintptr, syscall.Handle) (uintptr, error) } -var nativeWindows = windowsCalls{syscall.OpenProcessToken, syscall.StringToSid, nativeRestrict, nativeUserDACL, nativeCreate, nativeLimit, nativeAssign} +var nativeWindows = windowsCalls{syscall.OpenProcessToken, syscall.StringToSid, nativeRestrict, nativeUserDACL, syscall.CloseHandle, nativeCreate, nativeLimit, nativeAssign} // A token created by an elevated PowerShell process can inherit an // administrators-only DACL. Once that SID is disabled, the child cannot reopen @@ -121,7 +122,7 @@ func configureWindows(cmd *exec.Cmd, id *Identity, calls windowsCalls) (func(), if err = calls.openToken(current, syscall.TOKEN_DUPLICATE|syscall.TOKEN_ASSIGN_PRIMARY|syscall.TOKEN_QUERY|syscall.TOKEN_ADJUST_DEFAULT|writeDACL, &original); err != nil { return nil, err } - defer original.Close() + defer calls.close(syscall.Handle(original)) admin, err := calls.sid("S-1-5-32-544") if err != nil { return nil, err @@ -137,11 +138,11 @@ func configureWindows(cmd *exec.Cmd, id *Identity, calls windowsCalls) (func(), return nil, fmt.Errorf("create restricted token: %w", callErr) } if err := calls.secure(token); err != nil { - token.Close() + calls.close(syscall.Handle(token)) return nil, fmt.Errorf("secure restricted token: %w", err) } cmd.SysProcAttr = &syscall.SysProcAttr{Token: token, CreationFlags: syscall.CREATE_NEW_PROCESS_GROUP, HideWindow: true} - return func() { _ = token.Close() }, nil + return func() { _ = calls.close(syscall.Handle(token)) }, nil } func Kill(p *os.Process) error { @@ -180,13 +181,13 @@ func guardWindows(calls windowsCalls) error { limits.Basic.LimitFlags = 0x2000 ok, err := calls.limit(job, &limits) if ok == 0 { - syscall.CloseHandle(syscall.Handle(job)) + calls.close(syscall.Handle(job)) return fmt.Errorf("configure job: %w", err) } current, _ := syscall.GetCurrentProcess() ok, err = calls.assign(job, current) if ok == 0 { - syscall.CloseHandle(syscall.Handle(job)) + calls.close(syscall.Handle(job)) return fmt.Errorf("assign job: %w", err) } return nil diff --git a/internal/platform/process_windows_test.go b/internal/platform/process_windows_test.go index 29d90a3..3699be9 100644 --- a/internal/platform/process_windows_test.go +++ b/internal/platform/process_windows_test.go @@ -56,19 +56,64 @@ func TestWindowsUserDACLRejectsInvalidToken(t *testing.T) { func TestWindowsNativeFailuresReleaseHandles(t *testing.T) { failure := syscall.ERROR_ACCESS_DENIED - handleCount := func() uint32 { - t.Helper() - current, _ := syscall.GetCurrentProcess() - var count uint32 - ok, _, e := kernel.NewProc("GetProcessHandleCount").Call(uintptr(current), uintptr(unsafe.Pointer(&count))) - if ok == 0 { - t.Fatal(e) + // Process-wide handle counts include runtime threads and unrelated activity. + // Audit the exact resources acquired by this call instead, and require each + // native CloseHandle to succeed. No unrelated-handle allowance is needed. + audit := func(calls windowsCalls) (windowsCalls, func()) { + live := make(map[syscall.Handle]bool) + track := func(handle syscall.Handle) { + t.Helper() + if live[handle] { + t.Fatal("handle acquired twice without closing", handle) + } + live[handle] = true + } + open := calls.openToken + calls.openToken = func(process syscall.Handle, access uint32, token *syscall.Token) error { + err := open(process, access, token) + if err == nil { + track(syscall.Handle(*token)) + } + return err + } + restrict := calls.restrict + calls.restrict = func(original syscall.Token, denied []syscall.SIDAndAttributes, token *syscall.Token) (uintptr, error) { + ok, err := restrict(original, denied, token) + if ok != 0 { + track(syscall.Handle(*token)) + } + return ok, err + } + create := calls.create + calls.create = func() (uintptr, error) { + handle, err := create() + if handle != 0 { + track(syscall.Handle(handle)) + } + return handle, err + } + closeHandle := calls.close + calls.close = func(handle syscall.Handle) error { + t.Helper() + if !live[handle] { + t.Fatal("closing unowned or already closed handle", handle) + } + if err := closeHandle(handle); err != nil { + t.Fatal("native handle close failed", handle, err) + } + delete(live, handle) + return nil + } + return calls, func() { + t.Helper() + if len(live) != 0 { + t.Fatal("native handles left open", live) + } } - return count } - before := handleCount() + for range 3 { - for _, kind := range []string{"open", "admin-sid", "power-sid", "restrict", "secure"} { + for _, kind := range []string{"open", "admin-sid", "power-sid", "restrict", "secure", "success"} { calls := nativeWindows switch kind { case "open": @@ -85,11 +130,21 @@ func TestWindowsNativeFailuresReleaseHandles(t *testing.T) { case "secure": calls.secure = func(syscall.Token) error { return failure } } + calls, assertClosed := audit(calls) release, e := configureWindows(exec.Command("unused"), nil, calls) + if kind == "success" { + if e != nil { + t.Fatal("native token creation failed", e) + } + release() + assertClosed() + continue + } if e == nil { release() t.Fatal("native token failure ignored", kind) } + assertClosed() if !errors.Is(e, failure) { t.Fatal("native failure cause lost", kind, e) } @@ -104,13 +159,12 @@ func TestWindowsNativeFailuresReleaseHandles(t *testing.T) { case "assign": calls.assign = func(uintptr, syscall.Handle) (uintptr, error) { return 0, failure } } + calls, assertClosed := audit(calls) // Every case fails before any process can be assigned to the job. if e := guardWindows(calls); !errors.Is(e, failure) { t.Fatal("job failure cause lost", kind, e) } + assertClosed() } } - if after := handleCount(); after > before+2 { - t.Fatalf("native failure paths leaked handles: before=%d after=%d", before, after) - } }