fix(sandbox): bind Windows elevated ACL setup to one no-follow handle

Closes #728.

The Windows ACL setup resolved each target's pathname independently across
os.Stat, GetNamedSecurityInfo, and SetNamedSecurityInfo (and again on rollback),
so during elevated setup a lower-privileged local user could swap a target for a
symlink/junction between operations and redirect the ACL change onto a system
object the process never validated. That is a TOCTOU across a privilege boundary.

Open each target once with a no-follow handle (FILE_FLAG_OPEN_REPARSE_POINT plus
FILE_FLAG_BACKUP_SEMANTICS, READ_CONTROL|WRITE_DAC) and drive both the read
(GetSecurityInfo) and the write (SetSecurityInfo) through that one handle, so they
hit the same kernel object. isDir is read from the same handle rather than a
separate Stat. A target that resolves to a reparse point is refused outright,
since a setup target that is a symlink/junction during elevated setup is the
swap-attack signature. Rollback re-opens no-follow rather than restoring by
pathname; the residual window is small because the target is ACL-restricted by
then, and one caller discards the rollback closure so handles are not retained for
the whole sandbox lifetime.

Tests (windows-tagged, no elevation needed): the handle-based apply plus rollback
round-trip on a user-owned temp dir, the materialize-and-remove path, reparse
rejection via a junction (needs no admin, so it runs in CI), and the not-exist
mapping. The full privilege-boundary race can't be reproduced in CI, so the guard
is verified structurally (one handle for read and write) and by junction rejection.

internal/sandbox/windows_acl_apply_windows.go

@@ -77,11 +77,18 @@ func applyWindowsACLPathGroup(group windowsACLPathGroup) (windowsACLSnapshot, bo
 	if path == "" || len(group.Entries) == 0 {
 		return windowsACLSnapshot{}, false, nil
 	}
+	// Open ONE no-follow handle to the target and drive every ACL operation
+	// (read + write) through it, so the read and the write hit the same kernel
+	// object. The previous pathname-based Stat/GetNamedSecurityInfo/
+	// SetNamedSecurityInfo each re-resolved the path independently, so during
+	// elevated setup a lower-privileged local user could swap the target for a
+	// symlink/junction between operations and redirect the ACL change onto a
+	// system object it never validated (issue #728, a TOCTOU privilege boundary).
 	materialized := false
-	info, err := os.Stat(path)
+	handle, isDir, err := openWindowsACLTarget(path)
 	if err != nil {
 		if !errors.Is(err, os.ErrNotExist) {
-			return windowsACLSnapshot{}, false, fmt.Errorf("stat windows ACL target %s: %w", path, err)
+			return windowsACLSnapshot{}, false, err
 		}
 		if !group.Materialize {
 			if windowsACLGroupRequiresExistingTarget(group) {
@@ -93,48 +100,88 @@ func applyWindowsACLPathGroup(group windowsACLPathGroup) (windowsACLSnapshot, bo
 			return windowsACLSnapshot{}, false, fmt.Errorf("materialize windows ACL target %s: %w", path, err)
 		}
 		materialized = true
-		info, err = os.Stat(path)
+		handle, isDir, err = openWindowsACLTarget(path)
 		if err != nil {
-			return windowsACLSnapshot{}, false, fmt.Errorf("stat materialized windows ACL target %s: %w", path, err)
-		}
-	}
-	descriptor, err := windows.GetNamedSecurityInfo(path, windows.SE_FILE_OBJECT, windows.DACL_SECURITY_INFORMATION)
-	if err != nil {
-		if materialized {
 			_ = os.RemoveAll(path)
+			return windowsACLSnapshot{}, false, fmt.Errorf("open materialized windows ACL target %s: %w", path, err)
 		}
-		return windowsACLSnapshot{}, false, fmt.Errorf("read windows ACL for %s: %w", path, err)
 	}
-	oldDACL, _, err := descriptor.DACL()
-	if err != nil {
-		if materialized {
-			_ = os.RemoveAll(path)
-		}
-		return windowsACLSnapshot{}, false, fmt.Errorf("read windows DACL for %s: %w", path, err)
-	}
-	accessEntries, err := windowsExplicitAccessEntries(group.Entries, info.IsDir())
-	if err != nil {
+	// From here the handle is open; every early return must close it first (and
+	// remove a freshly materialized target) so a failure leaks neither.
+	fail := func(err error) (windowsACLSnapshot, bool, error) {
+		_ = windows.CloseHandle(handle)
 		if materialized {
 			_ = os.RemoveAll(path)
 		}
 		return windowsACLSnapshot{}, false, err
 	}
+	descriptor, err := windows.GetSecurityInfo(handle, windows.SE_FILE_OBJECT, windows.DACL_SECURITY_INFORMATION)
+	if err != nil {
+		return fail(fmt.Errorf("read windows ACL for %s: %w", path, err))
+	}
+	oldDACL, _, err := descriptor.DACL()
+	if err != nil {
+		return fail(fmt.Errorf("read windows DACL for %s: %w", path, err))
+	}
+	accessEntries, err := windowsExplicitAccessEntries(group.Entries, isDir)
+	if err != nil {
+		return fail(err)
+	}
 	nextDACL, err := windows.ACLFromEntries(accessEntries, oldDACL)
 	if err != nil {
-		if materialized {
-			_ = os.RemoveAll(path)
-		}
-		return windowsACLSnapshot{}, false, fmt.Errorf("build windows ACL for %s: %w", path, err)
+		return fail(fmt.Errorf("build windows ACL for %s: %w", path, err))
 	}
-	if err := windows.SetNamedSecurityInfo(path, windows.SE_FILE_OBJECT, windows.DACL_SECURITY_INFORMATION, nil, nil, nextDACL, nil); err != nil {
-		if materialized {
-			_ = os.RemoveAll(path)
-		}
-		return windowsACLSnapshot{}, false, fmt.Errorf("apply windows ACL for %s: %w", path, err)
+	if err := windows.SetSecurityInfo(handle, windows.SE_FILE_OBJECT, windows.DACL_SECURITY_INFORMATION, nil, nil, nextDACL, nil); err != nil {
+		return fail(fmt.Errorf("apply windows ACL for %s: %w", path, err))
 	}
+	// The apply is committed; the retained descriptor is the rollback baseline.
+	// The handle has served its purpose (read+write bound to one object) and is
+	// closed now — rollback re-opens no-follow rather than holding a handle for
+	// the whole sandbox lifetime, since one caller discards the rollback closure.
+	_ = windows.CloseHandle(handle)
 	return windowsACLSnapshot{Path: path, Descriptor: descriptor, Materialized: materialized}, true, nil
 }
 
+// openWindowsACLTarget opens path for reading and rewriting its DACL without
+// following a final-component reparse point (FILE_FLAG_OPEN_REPARSE_POINT), and
+// with FILE_FLAG_BACKUP_SEMANTICS so a directory can be opened. It returns the
+// handle and whether the target is a directory. A reparse-point target is
+// rejected outright: a sandbox setup target that resolves to a symlink/junction
+// during elevated setup is the signature of a path-swap attack, and following it
+// is exactly the redirection this guard exists to prevent. A missing target is
+// surfaced as os.ErrNotExist so the caller's materialize path still fires.
+func openWindowsACLTarget(path string) (windows.Handle, bool, error) {
+	utf16Path, err := windows.UTF16PtrFromString(path)
+	if err != nil {
+		return 0, false, fmt.Errorf("encode windows ACL target %s: %w", path, err)
+	}
+	handle, err := windows.CreateFile(
+		utf16Path,
+		windows.READ_CONTROL|windows.WRITE_DAC,
+		windows.FILE_SHARE_READ|windows.FILE_SHARE_WRITE|windows.FILE_SHARE_DELETE,
+		nil,
+		windows.OPEN_EXISTING,
+		windows.FILE_FLAG_BACKUP_SEMANTICS|windows.FILE_FLAG_OPEN_REPARSE_POINT,
+		0,
+	)
+	if err != nil {
+		// syscall.Errno.Is maps ERROR_FILE_NOT_FOUND/ERROR_PATH_NOT_FOUND to
+		// os.ErrNotExist, so the %w keeps the caller's errors.Is check working.
+		return 0, false, fmt.Errorf("open windows ACL target %s: %w", path, err)
+	}
+	var info windows.ByHandleFileInformation
+	if err := windows.GetFileInformationByHandle(handle, &info); err != nil {
+		_ = windows.CloseHandle(handle)
+		return 0, false, fmt.Errorf("inspect windows ACL target %s: %w", path, err)
+	}
+	if info.FileAttributes&windows.FILE_ATTRIBUTE_REPARSE_POINT != 0 {
+		_ = windows.CloseHandle(handle)
+		return 0, false, fmt.Errorf("refusing to apply ACL to reparse-point target %s: possible path swap during elevated setup", path)
+	}
+	isDir := info.FileAttributes&windows.FILE_ATTRIBUTE_DIRECTORY != 0
+	return handle, isDir, nil
+}
+
 func windowsACLGroupRequiresExistingTarget(group windowsACLPathGroup) bool {
 	for _, entry := range group.Entries {
 		if entry.Action == WindowsACLAllowWrite {
@@ -201,9 +248,20 @@ func rollbackWindowsACLSnapshots(snapshots []windowsACLSnapshot) error {
 			errs = append(errs, fmt.Errorf("read rollback windows DACL for %s: %w", snapshot.Path, err))
 			continue
 		}
-		if err := windows.SetNamedSecurityInfo(snapshot.Path, windows.SE_FILE_OBJECT, windows.DACL_SECURITY_INFORMATION, nil, nil, dacl, nil); err != nil {
+		// Re-open no-follow rather than restoring by pathname: the restore must
+		// land on the real object, not a reparse point swapped in since apply. The
+		// residual window is small because the target is ACL-restricted by now, but
+		// a handle keeps the restore honest. On a materialized-target rollback we
+		// remove it above, so only the restore-existing path opens here.
+		handle, _, err := openWindowsACLTarget(snapshot.Path)
+		if err != nil {
+			errs = append(errs, fmt.Errorf("re-open windows ACL target %s for rollback: %w", snapshot.Path, err))
+			continue
+		}
+		if err := windows.SetSecurityInfo(handle, windows.SE_FILE_OBJECT, windows.DACL_SECURITY_INFORMATION, nil, nil, dacl, nil); err != nil {
 			errs = append(errs, fmt.Errorf("rollback windows ACL for %s: %w", snapshot.Path, err))
 		}
+		_ = windows.CloseHandle(handle)
 	}
 	return errors.Join(errs...)
 }

internal/sandbox/windows_acl_apply_windows_test.go

@@ -0,0 +1,142 @@
+//go:build windows
+
+package sandbox
+
+import (
+	"errors"
+	"os"
+	"os/exec"
+	"path/filepath"
+	"strings"
+	"testing"
+
+	"golang.org/x/sys/windows"
+)
+
+// A workspace-owned target (no elevation needed: the current user holds WRITE_DAC
+// on its own temp dir) must apply and roll back through the handle-based path.
+// This exercises the whole open -> GetSecurityInfo -> SetSecurityInfo -> close
+// sequence and the re-open rollback that replaced the pathname-based calls in
+// #728, so a regression that reintroduced GetNamedSecurityInfo/SetNamedSecurityInfo
+// (or broke the handle plumbing) would fail here.
+func TestApplyWindowsACLPathGroupHandleBasedRoundTrip(t *testing.T) {
+	dir := t.TempDir()
+	group := windowsACLPathGroup{
+		Path: dir,
+		Entries: []WindowsACLEntry{{
+			Action:     WindowsACLAllowWrite,
+			Path:       dir,
+			Capability: "S-1-1-0", // Everyone: a well-known, StringToSid-parseable group SID
+		}},
+	}
+
+	snapshot, applied, err := applyWindowsACLPathGroup(group)
+	if err != nil {
+		t.Fatalf("applyWindowsACLPathGroup: %v", err)
+	}
+	if !applied {
+		t.Fatal("applied = false, want true for an existing directory target")
+	}
+	if snapshot.Path != dir || snapshot.Materialized {
+		t.Fatalf("snapshot = %#v, want Path=%q Materialized=false", snapshot, dir)
+	}
+	if snapshot.Descriptor == nil {
+		t.Fatal("snapshot has no captured descriptor to roll back to")
+	}
+	if err := rollbackWindowsACLSnapshots([]windowsACLSnapshot{snapshot}); err != nil {
+		t.Fatalf("rollbackWindowsACLSnapshots: %v", err)
+	}
+}
+
+// A materialized target that does not exist yet is created, ACL'd through the
+// handle, and removed on rollback.
+func TestApplyWindowsACLPathGroupMaterializes(t *testing.T) {
+	target := filepath.Join(t.TempDir(), "created")
+	group := windowsACLPathGroup{
+		Path:        target,
+		Materialize: true,
+		Entries: []WindowsACLEntry{{
+			Action:      WindowsACLDenyRead,
+			Path:        target,
+			Capability:  "S-1-1-0",
+			Materialize: true,
+		}},
+	}
+
+	snapshot, applied, err := applyWindowsACLPathGroup(group)
+	if err != nil {
+		t.Fatalf("applyWindowsACLPathGroup: %v", err)
+	}
+	if !applied || !snapshot.Materialized {
+		t.Fatalf("applied=%v materialized=%v, want both true", applied, snapshot.Materialized)
+	}
+	if _, err := os.Stat(target); err != nil {
+		t.Fatalf("materialized target not created: %v", err)
+	}
+	if err := rollbackWindowsACLSnapshots([]windowsACLSnapshot{snapshot}); err != nil {
+		t.Fatalf("rollbackWindowsACLSnapshots: %v", err)
+	}
+	if _, err := os.Stat(target); !errors.Is(err, os.ErrNotExist) {
+		t.Fatalf("materialized target still present after rollback: stat err = %v", err)
+	}
+}
+
+// The core #728 guard: a target that resolves to a reparse point (symlink /
+// junction) is refused rather than followed, so a swapped-in link during elevated
+// setup cannot redirect the ACL change onto a system object.
+func TestOpenWindowsACLTargetRejectsReparsePoint(t *testing.T) {
+	realDir := t.TempDir()
+	link := filepath.Join(t.TempDir(), "link")
+	// Prefer a junction: unlike a symlink it needs no admin/Developer Mode, so
+	// this guard actually runs in CI. A junction is a directory reparse point,
+	// exactly the swap vector openWindowsACLTarget must refuse to follow. Fall
+	// back to a symlink and skip only if neither reparse form can be created.
+	if out, err := exec.Command("cmd", "/c", "mklink", "/J", link, realDir).CombinedOutput(); err != nil {
+		if serr := os.Symlink(realDir, link); serr != nil {
+			t.Skipf("cannot create a reparse point (junction: %v %q; symlink: %v)", err, strings.TrimSpace(string(out)), serr)
+		}
+	}
+	handle, _, err := openWindowsACLTarget(link)
+	if err == nil {
+		_ = windows.CloseHandle(handle)
+		t.Fatal("openWindowsACLTarget followed a reparse point, want rejection")
+	}
+	if !strings.Contains(err.Error(), "reparse-point") {
+		t.Fatalf("openWindowsACLTarget(symlink) err = %v, want a reparse-point rejection", err)
+	}
+}
+
+// A missing target surfaces as os.ErrNotExist so the caller's materialize path
+// still fires (a real open error, e.g. reparse rejection, must NOT look missing).
+func TestOpenWindowsACLTargetMissingIsNotExist(t *testing.T) {
+	_, _, err := openWindowsACLTarget(filepath.Join(t.TempDir(), "does-not-exist"))
+	if !errors.Is(err, os.ErrNotExist) {
+		t.Fatalf("openWindowsACLTarget(missing) err = %v, want os.ErrNotExist", err)
+	}
+}
+
+// isDir is read from the same handle used for the ACL ops, not a separate Stat.
+func TestOpenWindowsACLTargetReportsIsDir(t *testing.T) {
+	dir := t.TempDir()
+	handle, isDir, err := openWindowsACLTarget(dir)
+	if err != nil {
+		t.Fatalf("openWindowsACLTarget(dir): %v", err)
+	}
+	_ = windows.CloseHandle(handle)
+	if !isDir {
+		t.Fatal("isDir = false for a directory target, want true")
+	}
+
+	file := filepath.Join(dir, "f.txt")
+	if err := os.WriteFile(file, []byte("x"), 0o644); err != nil {
+		t.Fatalf("write file: %v", err)
+	}
+	handle, isDir, err = openWindowsACLTarget(file)
+	if err != nil {
+		t.Fatalf("openWindowsACLTarget(file): %v", err)
+	}
+	_ = windows.CloseHandle(handle)
+	if isDir {
+		t.Fatal("isDir = true for a regular file, want false")
+	}
+}