From a9aa37e267a2bd47d0a8869d4673b1df298a2c72 Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Wed, 9 Sep 2026 15:18:43 +0530 Subject: [PATCH 1/5] fix(sandbox): keep a derived ACL target inside the write root it came from FILE_FLAG_OPEN_REPARSE_POINT refuses a reparse point at the final path component and resolves every component above it, which is right for a path the operator named and not enough for one this package derived. The write-root carveouts are derived: /.git/hooks and /.git/config are constructed from a root, and /.git is a name an unprivileged workspace writer can create before setup runs. mklink /J needs no privilege, so a junction there had the apply open /hooks, an ordinary directory with nothing wrong about its final component, and zero sandbox setup wrote a deny ACE on it as Administrator, outside the workspace. A derived entry now carries the root it came from, and the apply requires the object it finally holds to still live under it. The check is on the handle rather than the name: GetFinalPathNameByHandle answers where the open object actually is, and both sides go through it so the spelling normalizes the same way. Materialization is checked before it creates, since os.MkdirAll follows the same reparse points. Deliberately not a refusal of every reparse point on the path. Above the write root the path is the operator's, who may keep a workspace under a junction or a mapped directory; only the tail the sandbox appended is held strict. Closes #1024 --- internal/sandbox/windows_acl.go | 12 +- internal/sandbox/windows_acl_apply_windows.go | 27 ++- .../windows_acl_containment_windows.go | 212 ++++++++++++++++++ .../windows_acl_containment_windows_test.go | 167 ++++++++++++++ 4 files changed, 415 insertions(+), 3 deletions(-) create mode 100644 internal/sandbox/windows_acl_containment_windows.go create mode 100644 internal/sandbox/windows_acl_containment_windows_test.go diff --git a/internal/sandbox/windows_acl.go b/internal/sandbox/windows_acl.go index 96a18004a..53376eb10 100644 --- a/internal/sandbox/windows_acl.go +++ b/internal/sandbox/windows_acl.go @@ -33,8 +33,14 @@ type WindowsACLEntry struct { // down onto the target's EXISTING descendants (not just new ones it // creates going forward), which is why direct-only denies must set this // flag rather than rely on inheritance. - NoInherit bool `json:"noInherit,omitempty"` - Materialize bool `json:"materialize,omitempty"` + NoInherit bool `json:"noInherit,omitempty"` + // Anchor is the write root Path was DERIVED from, for the paths this + // package constructs rather than the operator naming. The apply requires + // the object it finally opens to still live under it, so a reparse point + // planted on the derived tail cannot walk an elevated ACL write out of the + // sandbox. Empty for an operator-named path, which has no owned tail. + Anchor string `json:"anchor,omitempty"` + Materialize bool `json:"materialize,omitempty"` } type WindowsACLPlan struct { @@ -61,6 +67,8 @@ func BuildWindowsACLPlan(config WindowsSandboxCommandConfig) (WindowsACLPlan, er Action: WindowsACLDenyWrite, Path: path, Capability: capability.SID, + // Derived from this root, so the apply holds it inside it. + Anchor: capability.Root, }) } } diff --git a/internal/sandbox/windows_acl_apply_windows.go b/internal/sandbox/windows_acl_apply_windows.go index dcacf68e9..e8ecb008c 100644 --- a/internal/sandbox/windows_acl_apply_windows.go +++ b/internal/sandbox/windows_acl_apply_windows.go @@ -23,7 +23,10 @@ const ( ) type windowsACLPathGroup struct { - Path string + Path string + // Anchor is the write root Path was derived from, empty when the operator + // named the path. See verifyWindowsACLHandleUnderAnchor. + Anchor string Entries []WindowsACLEntry Materialize bool } @@ -69,6 +72,12 @@ func groupWindowsACLPlanByPath(plan WindowsACLPlan) []windowsACLPathGroup { } group.Entries = append(group.Entries, entry) group.Materialize = group.Materialize || entry.Materialize + // One path is derived from at most one root, so the first anchor seen is + // the anchor. Taking it rather than overwriting keeps a later + // operator-named duplicate of the same path from clearing it. + if group.Anchor == "" { + group.Anchor = entry.Anchor + } } out := make([]windowsACLPathGroup, 0, len(byPath)) for _, group := range byPath { @@ -104,6 +113,13 @@ func applyWindowsACLPathGroup(group windowsACLPathGroup) (windowsACLSnapshot, bo } return windowsACLSnapshot{}, false, nil } + // Before creating anything: os.MkdirAll walks a pathname and follows every + // reparse point on it, so a junction on the derived tail would have this + // elevated setup create the directory outside the write root and only the + // containment check below would notice, after the fact. + if err := verifyWindowsACLPathUnderAnchor(group.Anchor, path); err != nil { + return windowsACLSnapshot{}, false, err + } if err := os.MkdirAll(path, 0o700); err != nil { return windowsACLSnapshot{}, false, fmt.Errorf("materialize windows ACL target %s: %w", path, err) } @@ -123,6 +139,15 @@ func applyWindowsACLPathGroup(group windowsACLPathGroup) (windowsACLSnapshot, bo } return windowsACLSnapshot{}, false, err } + // THE OBJECT HAS TO BE WHERE THE PLAN SAID IT WOULD BE. The open above + // refuses a reparse point at the final component and resolves every one + // above it, which is right for a path the operator named and not enough for + // one this package derived from a write root: a junction on the derived tail + // redirects the handle out of the sandbox with nothing about the final + // object looking wrong. + if err := verifyWindowsACLHandleUnderAnchor(handle, group.Anchor, path); err != nil { + return fail(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)) diff --git a/internal/sandbox/windows_acl_containment_windows.go b/internal/sandbox/windows_acl_containment_windows.go new file mode 100644 index 000000000..c9595b333 --- /dev/null +++ b/internal/sandbox/windows_acl_containment_windows.go @@ -0,0 +1,212 @@ +//go:build windows + +package sandbox + +import ( + "errors" + "fmt" + "os" + "path/filepath" + "strings" + + "golang.org/x/sys/windows" +) + +// Keeping an elevated ACL write inside the tree it was planned for. +// +// FILE_FLAG_OPEN_REPARSE_POINT GUARDS ONE COMPONENT, AND A PATH HAS MANY. The +// apply opens its target with that flag, which stops the FINAL component from +// being followed and refuses the handle when it is a reparse point. Every +// component above it is resolved normally, as it must be for any absolute path +// to work at all. +// +// That is enough for a target the operator named, and not enough for one the +// sandbox derived. The write-root carveouts are derived: /.git/hooks and +// /.git/config are constructed from a root, and /.git is a name an +// unprivileged workspace writer can create. `mklink /J` needs no privilege, so +// a junction there makes the apply open /hooks instead, and +// `zero sandbox setup` writes a deny-write ACE, as Administrator, on an object +// outside the workspace. The final-component flag never fires: the object at +// the end of the walk is an ordinary directory. +// +// So the derived targets carry the root they were derived FROM, and the object +// finally opened has to still be inside it. The check is on the handle, not on +// the name: GetFinalPathNameByHandle answers where the object this process is +// holding actually lives, and both sides of the comparison go through it so the +// spelling is normalized the same way (\\?\ prefix, long names, drive letter). +// A junction anywhere along the derived tail moves the answer out of the root +// and the apply refuses. +// +// The rule is deliberately not "no reparse point anywhere in the path". Above +// the write root the path belongs to the operator, who may legitimately keep a +// workspace under a junction or a mapped directory, and refusing that would +// break setups this has nothing to say about. Only the tail the sandbox itself +// appended is held strict. + +// windowsFinalPathByHandle reports where an open handle's object actually lives. +func windowsFinalPathByHandle(handle windows.Handle) (string, error) { + buffer := make([]uint16, windows.MAX_LONG_PATH) + n, err := windows.GetFinalPathNameByHandle(handle, &buffer[0], uint32(len(buffer)), 0) + if err != nil { + return "", err + } + if int(n) > len(buffer) { + buffer = make([]uint16, n) + n, err = windows.GetFinalPathNameByHandle(handle, &buffer[0], uint32(len(buffer)), 0) + if err != nil { + return "", err + } + } + return windows.UTF16ToString(buffer[:n]), nil +} + +// windowsFinalPathOf opens path without following a final-component reparse +// point and reports where the object lives. A missing path is surfaced as +// os.ErrNotExist so callers can walk up to a parent that exists. +func windowsFinalPathOf(path string) (string, error) { + utf16Path, err := windows.UTF16PtrFromString(path) + if err != nil { + return "", fmt.Errorf("encode windows path %s: %w", path, err) + } + handle, err := windows.CreateFile( + utf16Path, + windows.READ_CONTROL, + 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 { + return "", fmt.Errorf("open windows path %s: %w", path, err) + } + defer func() { _ = windows.CloseHandle(handle) }() + final, err := windowsFinalPathByHandle(handle) + if err != nil { + return "", fmt.Errorf("resolve windows path %s: %w", path, err) + } + return final, nil +} + +// windowsACLTailUnderAnchor returns the components target adds to anchor, and +// whether target is under anchor lexically at all. +func windowsACLTailUnderAnchor(anchor, target string) (string, bool) { + relative, err := filepath.Rel(filepath.Clean(anchor), filepath.Clean(target)) + if err != nil { + return "", false + } + if relative == "." { + return "", true + } + if relative == ".." || strings.HasPrefix(relative, ".."+string(filepath.Separator)) { + return "", false + } + return relative, true +} + +// windowsACLContainmentError reports a derived target that resolved outside the +// root it was derived from. +type windowsACLContainmentError struct { + Anchor string + Target string + Actual string +} + +func (err windowsACLContainmentError) Error() string { + return fmt.Sprintf( + "refusing to apply ACL to %s: it resolves to %s, outside the write root %s; "+ + "a reparse point on the way down redirects an elevated ACL write out of the sandbox", + err.Target, err.Actual, err.Anchor) +} + +// verifyWindowsACLHandleUnderAnchor refuses a handle whose object does not live +// where the plan said it would. +// +// anchor empty means the target was named rather than derived, and there is no +// owned tail to hold strict; the final-component check the caller already did is +// the whole guard for those. +func verifyWindowsACLHandleUnderAnchor(handle windows.Handle, anchor, target string) error { + if strings.TrimSpace(anchor) == "" { + return nil + } + tail, ok := windowsACLTailUnderAnchor(anchor, target) + if !ok { + return windowsACLContainmentError{Anchor: anchor, Target: target, Actual: target} + } + anchorFinal, err := windowsFinalPathOf(anchor) + if err != nil { + return fmt.Errorf("resolve write root %s: %w", anchor, err) + } + targetFinal, err := windowsFinalPathByHandle(handle) + if err != nil { + return fmt.Errorf("resolve windows ACL target %s: %w", target, err) + } + expected := anchorFinal + if tail != "" { + expected = filepath.Join(anchorFinal, tail) + } + if !strings.EqualFold(filepath.Clean(targetFinal), filepath.Clean(expected)) { + return windowsACLContainmentError{Anchor: anchor, Target: target, Actual: targetFinal} + } + return nil +} + +// verifyWindowsACLPathUnderAnchor makes the same check against the deepest +// component of target that exists, before anything is created. +// +// Materialization creates a missing target with os.MkdirAll, which walks a +// pathname and follows every reparse point on it, so without this an elevated +// setup creates the directory on the far side of a junction and only the +// containment check afterwards notices. The window between this check and that +// create is not closed here; closing it needs the components created relative to +// retained handles, which is the rooted descent tracked in #808. +func verifyWindowsACLPathUnderAnchor(anchor, target string) error { + if strings.TrimSpace(anchor) == "" { + return nil + } + if _, ok := windowsACLTailUnderAnchor(anchor, target); !ok { + return windowsACLContainmentError{Anchor: anchor, Target: target, Actual: target} + } + existing := filepath.Clean(target) + for { + if _, err := os.Lstat(existing); err == nil { + break + } else if !errors.Is(err, os.ErrNotExist) { + return fmt.Errorf("inspect windows ACL target %s: %w", existing, err) + } + parent := filepath.Dir(existing) + if parent == existing { + return nil + } + existing = parent + } + if _, ok := windowsACLTailUnderAnchor(anchor, existing); !ok { + return nil + } + handle, err := windowsACLOpenForContainment(existing) + if err != nil { + return err + } + defer func() { _ = windows.CloseHandle(handle) }() + return verifyWindowsACLHandleUnderAnchor(handle, anchor, existing) +} + +func windowsACLOpenForContainment(path string) (windows.Handle, error) { + utf16Path, err := windows.UTF16PtrFromString(path) + if err != nil { + return 0, fmt.Errorf("encode windows ACL target %s: %w", path, err) + } + handle, err := windows.CreateFile( + utf16Path, + windows.READ_CONTROL, + 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 { + return 0, fmt.Errorf("open windows ACL target %s: %w", path, err) + } + return handle, nil +} diff --git a/internal/sandbox/windows_acl_containment_windows_test.go b/internal/sandbox/windows_acl_containment_windows_test.go new file mode 100644 index 000000000..2e02e134d --- /dev/null +++ b/internal/sandbox/windows_acl_containment_windows_test.go @@ -0,0 +1,167 @@ +//go:build windows + +package sandbox + +import ( + "errors" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" +) + +// makeACLJunction plants a directory junction at link pointing at target. +// +// A junction rather than a symlink on purpose: mklink /J needs no privilege, so +// it is reachable by exactly the unprivileged workspace writer this guards +// against, and the test runs on an ordinary developer or CI account. +func makeACLJunction(t *testing.T, link, target string) { + t.Helper() + output, err := exec.Command("cmd.exe", "/c", "mklink", "/J", link, target).CombinedOutput() + if err != nil { + t.Skipf("mklink /J unavailable here: %v (%s)", err, output) + } +} + +// AN ELEVATED ACL WRITE MUST NOT LEAVE THE WRITE ROOT THROUGH A JUNCTION. +// +// The apply opens its target with FILE_FLAG_OPEN_REPARSE_POINT, which refuses a +// reparse point at the FINAL component and resolves every component above it. +// The write-root carveouts are derived rather than named: /.git/hooks and +// /.git/config are constructed from the root, and /.git is a name an +// unprivileged workspace writer can create before setup runs. A junction there +// makes the apply open /hooks, an ordinary directory with +// nothing wrong about its final component, and `zero sandbox setup` writes a +// deny-write ACE on it as Administrator, outside the workspace. +func TestApplyWindowsACLRefusesACarveoutRedirectedByAJunction(t *testing.T) { + root := t.TempDir() + outside := filepath.Join(t.TempDir(), "elsewhere") + if err := os.MkdirAll(filepath.Join(outside, "hooks"), 0o700); err != nil { + t.Fatal(err) + } + makeACLJunction(t, filepath.Join(root, ".git"), outside) + + carveout := filepath.Join(root, ".git", "hooks") + group := windowsACLPathGroup{ + Path: carveout, + Anchor: root, + Entries: []WindowsACLEntry{{ + Action: WindowsACLDenyWrite, + Path: carveout, + Capability: "S-1-5-32-545", + Anchor: root, + }}, + } + + _, applied, err := applyWindowsACLPathGroup(group) + if applied { + t.Error("applied a deny-write ACE through a junction, so an elevated setup rewrote the DACL of an object outside the workspace") + } + var containment windowsACLContainmentError + if !errors.As(err, &containment) { + t.Fatalf("err = %v, want a containment refusal", err) + } + if !strings.Contains(containment.Actual, "elsewhere") { + t.Errorf("the refusal does not name where the target actually resolved: %v", containment) + } +} + +// The same carveout inside an ordinary workspace still gets its ACE, or the +// refusal above would prove only that the apply fails. +func TestApplyWindowsACLStillAppliesAnOrdinaryCarveout(t *testing.T) { + root := t.TempDir() + carveout := filepath.Join(root, ".git", "hooks") + if err := os.MkdirAll(carveout, 0o700); err != nil { + t.Fatal(err) + } + group := windowsACLPathGroup{ + Path: carveout, + Anchor: root, + Entries: []WindowsACLEntry{{ + Action: WindowsACLDenyWrite, + Path: carveout, + Capability: "S-1-5-32-545", + Anchor: root, + }}, + } + + snapshot, applied, err := applyWindowsACLPathGroup(group) + if err != nil { + t.Fatalf("apply: %v", err) + } + if !applied { + t.Fatal("an ordinary carveout inside the write root was not applied") + } + if snapshot.Path != carveout { + t.Fatalf("snapshot path = %q, want %q", snapshot.Path, carveout) + } +} + +// MATERIALIZATION MUST NOT CREATE THROUGH THE JUNCTION EITHER. os.MkdirAll +// walks a pathname and follows every reparse point on it, so without the +// pre-create check an elevated setup makes the directory on the far side and +// only notices afterwards. +func TestApplyWindowsACLRefusesToMaterializeThroughAJunction(t *testing.T) { + root := t.TempDir() + outside := filepath.Join(t.TempDir(), "elsewhere") + if err := os.MkdirAll(outside, 0o700); err != nil { + t.Fatal(err) + } + makeACLJunction(t, filepath.Join(root, ".git"), outside) + + carveout := filepath.Join(root, ".git", "hooks") + if _, err := os.Lstat(filepath.Join(outside, "hooks")); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("SETUP INVALID: the target already holds hooks: %v", err) + } + group := windowsACLPathGroup{ + Path: carveout, + Anchor: root, + Materialize: true, + Entries: []WindowsACLEntry{{ + Action: WindowsACLDenyRead, + Path: carveout, + Capability: "S-1-5-32-545", + Anchor: root, + Materialize: true, + }}, + } + + if _, applied, err := applyWindowsACLPathGroup(group); applied || err == nil { + t.Errorf("materialized through a junction: applied=%v err=%v", applied, err) + } + if _, err := os.Lstat(filepath.Join(outside, "hooks")); !errors.Is(err, os.ErrNotExist) { + t.Errorf("an elevated create landed outside the write root: %v", err) + } +} + +// An operator-named path carries no anchor and keeps the behaviour it had: the +// final component is still refused when it is a reparse point, and nothing above +// it is second-guessed, because above the write root the path belongs to the +// operator, who may keep a workspace under a junction. +func TestApplyWindowsACLLeavesAnUnanchoredPathAlone(t *testing.T) { + base := t.TempDir() + outside := filepath.Join(t.TempDir(), "elsewhere") + if err := os.MkdirAll(filepath.Join(outside, "leaf"), 0o700); err != nil { + t.Fatal(err) + } + makeACLJunction(t, filepath.Join(base, "link"), outside) + + named := filepath.Join(base, "link", "leaf") + group := windowsACLPathGroup{ + Path: named, + Entries: []WindowsACLEntry{{ + Action: WindowsACLDenyWrite, + Path: named, + Capability: "S-1-5-32-545", + }}, + } + + _, applied, err := applyWindowsACLPathGroup(group) + if err != nil { + t.Fatalf("an operator-named path below a junction was refused: %v", err) + } + if !applied { + t.Fatal("an operator-named path below a junction was not applied") + } +} From ad78725568a4c9a743694f259efcb0c5f4d8361a Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Wed, 9 Sep 2026 15:21:23 +0530 Subject: [PATCH 2/5] fix(sandbox): reject the reparse ancestor itself before materializing The pre-create check asked where the deepest existing ancestor lives, and a junction answers with its own path: the open does not follow a final-component reparse point, so /.git came back as /.git and matched. os.MkdirAll does follow it. What disqualifies that ancestor is that it IS a reparse point, not where it reports living. Found by reverting the check and watching the test still pass, which said the guard was doing nothing rather than that the test was weak. Pinned against the function now, because through the whole apply the create is made and then removed on the failure path, so the filesystem afterwards looks identical either way. The plan wiring gets its own test for the same reason: every apply-level case here hands the group an anchor directly, so none of them would notice the builder never setting one. --- .../windows_acl_containment_windows.go | 18 +++- .../windows_acl_containment_windows_test.go | 92 +++++++++++++++++++ 2 files changed, 109 insertions(+), 1 deletion(-) diff --git a/internal/sandbox/windows_acl_containment_windows.go b/internal/sandbox/windows_acl_containment_windows.go index c9595b333..704d46885 100644 --- a/internal/sandbox/windows_acl_containment_windows.go +++ b/internal/sandbox/windows_acl_containment_windows.go @@ -180,7 +180,9 @@ func verifyWindowsACLPathUnderAnchor(anchor, target string) error { } existing = parent } - if _, ok := windowsACLTailUnderAnchor(anchor, existing); !ok { + tail, ok := windowsACLTailUnderAnchor(anchor, existing) + if !ok || tail == "" { + // Above or at the write root, which is the operator's to arrange. return nil } handle, err := windowsACLOpenForContainment(existing) @@ -188,6 +190,20 @@ func verifyWindowsACLPathUnderAnchor(anchor, target string) error { return err } defer func() { _ = windows.CloseHandle(handle) }() + // THE ANCESTOR ITSELF IS THE REDIRECTION, WHICH IS WHY ITS NAME LOOKS RIGHT. + // + // Asking only where this object lives answers "exactly where you asked": the + // open above does not follow a final-component reparse point, so a junction + // at /.git comes back as /.git. os.MkdirAll does follow it, and + // creates the missing components on the far side. So what disqualifies this + // ancestor is that it IS a reparse point, not where it reports living. + var info windows.ByHandleFileInformation + if err := windows.GetFileInformationByHandle(handle, &info); err != nil { + return fmt.Errorf("inspect windows ACL target %s: %w", existing, err) + } + if info.FileAttributes&windows.FILE_ATTRIBUTE_REPARSE_POINT != 0 { + return windowsACLContainmentError{Anchor: anchor, Target: target, Actual: existing + " (a reparse point)"} + } return verifyWindowsACLHandleUnderAnchor(handle, anchor, existing) } diff --git a/internal/sandbox/windows_acl_containment_windows_test.go b/internal/sandbox/windows_acl_containment_windows_test.go index 2e02e134d..81c61be49 100644 --- a/internal/sandbox/windows_acl_containment_windows_test.go +++ b/internal/sandbox/windows_acl_containment_windows_test.go @@ -165,3 +165,95 @@ func TestApplyWindowsACLLeavesAnUnanchoredPathAlone(t *testing.T) { t.Fatal("an operator-named path below a junction was not applied") } } + +// AND THE GUARD ON THE CREATE HAS TO BE PINNED WHERE IT CAN BE SEEN. +// +// Through the whole apply, dropping the pre-create check is invisible: the +// directory is made on the far side of the junction, the containment check on +// the handle then refuses, and the failure path removes what it made, so the +// filesystem afterwards looks the same either way. The transient elevated create +// outside the write root is the thing being prevented, so it is checked here, +// against the function that prevents it. +func TestVerifyWindowsACLPathUnderAnchorRefusesAMissingTargetBehindAJunction(t *testing.T) { + root := t.TempDir() + outside := filepath.Join(t.TempDir(), "elsewhere") + if err := os.MkdirAll(outside, 0o700); err != nil { + t.Fatal(err) + } + makeACLJunction(t, filepath.Join(root, ".git"), outside) + + carveout := filepath.Join(root, ".git", "hooks") + if _, err := os.Lstat(carveout); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("SETUP INVALID: the carveout already exists, so nothing would be created: %v", err) + } + + err := verifyWindowsACLPathUnderAnchor(root, carveout) + var containment windowsACLContainmentError + if !errors.As(err, &containment) { + t.Fatalf("err = %v, want a containment refusal before anything is created", err) + } + + // The ordinary case still says yes, or the refusal proves only that the + // function rejects things. + ordinary := t.TempDir() + if err := verifyWindowsACLPathUnderAnchor(ordinary, filepath.Join(ordinary, ".git", "hooks")); err != nil { + t.Errorf("an ordinary missing carveout was refused: %v", err) + } + // And an unanchored path is not this function's business. + if err := verifyWindowsACLPathUnderAnchor("", carveout); err != nil { + t.Errorf("an unanchored path was refused: %v", err) + } +} + +// THE PLAN HAS TO CARRY THE ANCHOR, OR THE APPLY HAS NOTHING TO ENFORCE. +// +// Every test above hands applyWindowsACLPathGroup an anchor directly, so they +// pass whether or not anything ever sets one. This drives the real builder and +// checks which entries come out anchored: the carveouts derived from a write +// root, and not the paths the operator named, which have no owned tail and whose +// intermediates are the operator's own business. +func TestBuildWindowsACLPlanAnchorsDerivedCarveouts(t *testing.T) { + home := t.TempDir() + config := WindowsSandboxCommandConfig{ + SandboxHome: home, + WorkspaceRoots: []string{`C:\workspace`}, + PermissionProfile: PermissionProfile{ + FileSystem: FileSystemPolicy{ + Kind: FileSystemRestricted, + WriteRoots: []WritableRoot{{ + Root: `C:\workspace`, + ReadOnlySubpaths: []string{`C:\workspace\.git\hooks`}, + ProtectedMetadataNames: []string{".zero"}, + }}, + DenyWrite: []string{`C:\elsewhere\named-by-the-operator`}, + }, + Network: NetworkPolicy{Mode: NetworkDeny}, + }, + } + + plan, err := BuildWindowsACLPlan(config) + if err != nil { + t.Fatalf("BuildWindowsACLPlan: %v", err) + } + + anchors := map[string]string{} + for _, entry := range plan.Entries { + anchors[strings.ToLower(entry.Path)] = entry.Anchor + } + for _, derived := range []string{`C:\workspace\.git\hooks`, `C:\workspace\.zero`} { + anchor, present := anchors[strings.ToLower(derived)] + if !present { + t.Fatalf("SETUP INVALID: the plan has no entry for the derived carveout %s", derived) + } + if anchor != `C:\workspace` { + t.Errorf("derived carveout %s carries anchor %q, want the write root it came from", derived, anchor) + } + } + named, present := anchors[strings.ToLower(`C:\elsewhere\named-by-the-operator`)] + if !present { + t.Fatal("SETUP INVALID: the plan has no entry for the operator-named deny path") + } + if named != "" { + t.Errorf("operator-named path carries anchor %q, want none: its intermediates are not the sandbox's to police", named) + } +} From 28360a2dff3dee561d991c686db22d0ffecf3ad6 Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Wed, 9 Sep 2026 21:54:07 +0530 Subject: [PATCH 3/5] fix(sandbox): anchor only the carveouts that are under the root, and put the DACL back Two from review, both right. ReadOnlySubpaths is a profile field an operator can set to any path, and one placed outside the write root is a configuration that works today; anchoring it unconditionally turned that into a containment refusal. Only paths lexically under the root are anchored now, which is where the derived carveouts are anyway, and anything else keeps the final-component guard it always had. The successful-apply tests left their deny ACE in place, so t.TempDir could fail to remove the tree. Restoring the snapshot exposed the sharper half: the ACE denied the group the test runs as, which revoked its own WRITE_DAC and left the rollback unable to reopen the target. The tests deny a group this process is not a member of instead, which is what a capability SID is in production, and roll back afterwards. --- internal/sandbox/windows_acl.go | 14 ++++++-- .../windows_acl_containment_windows_test.go | 35 ++++++++++++++++--- 2 files changed, 42 insertions(+), 7 deletions(-) diff --git a/internal/sandbox/windows_acl.go b/internal/sandbox/windows_acl.go index 53376eb10..c7d47956e 100644 --- a/internal/sandbox/windows_acl.go +++ b/internal/sandbox/windows_acl.go @@ -63,12 +63,22 @@ func BuildWindowsACLPlan(config WindowsSandboxCommandConfig) (WindowsACLPlan, er Capability: capability.SID, }) for _, path := range capability.ProtectedWriteDenyPaths { + // ANCHORED ONLY WHERE THE ANCHOR IS TRUE. Most of these are derived + // from the root and sit under it, and holding those inside it is the + // point. But ReadOnlySubpaths is a profile field an operator can set to + // any path, and one deliberately placed outside the root is a + // configuration that works today; anchoring it would turn that into a + // containment refusal. A path that is not under the root gets no + // anchor and keeps the final-component guard it always had. + anchor := "" + if pathWithinRoot(capability.Root, path) { + anchor = capability.Root + } entries = append(entries, WindowsACLEntry{ Action: WindowsACLDenyWrite, Path: path, Capability: capability.SID, - // Derived from this root, so the apply holds it inside it. - Anchor: capability.Root, + Anchor: anchor, }) } } diff --git a/internal/sandbox/windows_acl_containment_windows_test.go b/internal/sandbox/windows_acl_containment_windows_test.go index 81c61be49..b746f8b0f 100644 --- a/internal/sandbox/windows_acl_containment_windows_test.go +++ b/internal/sandbox/windows_acl_containment_windows_test.go @@ -11,6 +11,14 @@ import ( "testing" ) +// windowsACLTestDenySID is a well-known group this process is not a member of. +// +// A deny ACE is what the carveouts actually are, and denying a group the test +// runs as revokes its own WRITE_DAC: the apply succeeds and then nothing can +// put the DACL back, so the temporary tree cannot be removed. Guests keeps the +// deny semantics without locking the test out of its own fixture. +const windowsACLTestDenySID = "S-1-5-32-546" + // makeACLJunction plants a directory junction at link pointing at target. // // A junction rather than a symlink on purpose: mklink /J needs no privilege, so @@ -49,7 +57,7 @@ func TestApplyWindowsACLRefusesACarveoutRedirectedByAJunction(t *testing.T) { Entries: []WindowsACLEntry{{ Action: WindowsACLDenyWrite, Path: carveout, - Capability: "S-1-5-32-545", + Capability: windowsACLTestDenySID, Anchor: root, }}, } @@ -81,7 +89,7 @@ func TestApplyWindowsACLStillAppliesAnOrdinaryCarveout(t *testing.T) { Entries: []WindowsACLEntry{{ Action: WindowsACLDenyWrite, Path: carveout, - Capability: "S-1-5-32-545", + Capability: windowsACLTestDenySID, Anchor: root, }}, } @@ -90,6 +98,16 @@ func TestApplyWindowsACLStillAppliesAnOrdinaryCarveout(t *testing.T) { if err != nil { t.Fatalf("apply: %v", err) } + // PUT IT BACK, OR THE TEMP TREE CANNOT BE REMOVED. The ACE just written + // denies write to the group this test runs as, and t.TempDir removes the + // tree when the test ends. + if applied { + t.Cleanup(func() { + if err := rollbackWindowsACLSnapshots([]windowsACLSnapshot{snapshot}); err != nil { + t.Errorf("rollback: %v", err) + } + }) + } if !applied { t.Fatal("an ordinary carveout inside the write root was not applied") } @@ -121,7 +139,7 @@ func TestApplyWindowsACLRefusesToMaterializeThroughAJunction(t *testing.T) { Entries: []WindowsACLEntry{{ Action: WindowsACLDenyRead, Path: carveout, - Capability: "S-1-5-32-545", + Capability: windowsACLTestDenySID, Anchor: root, Materialize: true, }}, @@ -153,14 +171,21 @@ func TestApplyWindowsACLLeavesAnUnanchoredPathAlone(t *testing.T) { Entries: []WindowsACLEntry{{ Action: WindowsACLDenyWrite, Path: named, - Capability: "S-1-5-32-545", + Capability: windowsACLTestDenySID, }}, } - _, applied, err := applyWindowsACLPathGroup(group) + snapshot, applied, err := applyWindowsACLPathGroup(group) if err != nil { t.Fatalf("an operator-named path below a junction was refused: %v", err) } + if applied { + t.Cleanup(func() { + if err := rollbackWindowsACLSnapshots([]windowsACLSnapshot{snapshot}); err != nil { + t.Errorf("rollback: %v", err) + } + }) + } if !applied { t.Fatal("an operator-named path below a junction was not applied") } From 38c1e333b1c124d951d948689cb7ab6390e15c7f Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Wed, 9 Sep 2026 21:55:31 +0530 Subject: [PATCH 4/5] test(sandbox): check the plan anchoring on every platform, out-of-root case included The plan builder is cross-platform and its anchor tests were in a Windows-only file, so nothing checked the wiring on Linux or macOS. Moved beside the other BuildWindowsACLPlan tests, with the case review raised: an out-of-root ReadOnlySubpath stays unanchored, and an in-root one is still held to its write root. --- .../windows_acl_containment_windows_test.go | 53 ---------- internal/sandbox/windows_acl_test.go | 100 ++++++++++++++++++ 2 files changed, 100 insertions(+), 53 deletions(-) diff --git a/internal/sandbox/windows_acl_containment_windows_test.go b/internal/sandbox/windows_acl_containment_windows_test.go index b746f8b0f..b5d3a15aa 100644 --- a/internal/sandbox/windows_acl_containment_windows_test.go +++ b/internal/sandbox/windows_acl_containment_windows_test.go @@ -229,56 +229,3 @@ func TestVerifyWindowsACLPathUnderAnchorRefusesAMissingTargetBehindAJunction(t * t.Errorf("an unanchored path was refused: %v", err) } } - -// THE PLAN HAS TO CARRY THE ANCHOR, OR THE APPLY HAS NOTHING TO ENFORCE. -// -// Every test above hands applyWindowsACLPathGroup an anchor directly, so they -// pass whether or not anything ever sets one. This drives the real builder and -// checks which entries come out anchored: the carveouts derived from a write -// root, and not the paths the operator named, which have no owned tail and whose -// intermediates are the operator's own business. -func TestBuildWindowsACLPlanAnchorsDerivedCarveouts(t *testing.T) { - home := t.TempDir() - config := WindowsSandboxCommandConfig{ - SandboxHome: home, - WorkspaceRoots: []string{`C:\workspace`}, - PermissionProfile: PermissionProfile{ - FileSystem: FileSystemPolicy{ - Kind: FileSystemRestricted, - WriteRoots: []WritableRoot{{ - Root: `C:\workspace`, - ReadOnlySubpaths: []string{`C:\workspace\.git\hooks`}, - ProtectedMetadataNames: []string{".zero"}, - }}, - DenyWrite: []string{`C:\elsewhere\named-by-the-operator`}, - }, - Network: NetworkPolicy{Mode: NetworkDeny}, - }, - } - - plan, err := BuildWindowsACLPlan(config) - if err != nil { - t.Fatalf("BuildWindowsACLPlan: %v", err) - } - - anchors := map[string]string{} - for _, entry := range plan.Entries { - anchors[strings.ToLower(entry.Path)] = entry.Anchor - } - for _, derived := range []string{`C:\workspace\.git\hooks`, `C:\workspace\.zero`} { - anchor, present := anchors[strings.ToLower(derived)] - if !present { - t.Fatalf("SETUP INVALID: the plan has no entry for the derived carveout %s", derived) - } - if anchor != `C:\workspace` { - t.Errorf("derived carveout %s carries anchor %q, want the write root it came from", derived, anchor) - } - } - named, present := anchors[strings.ToLower(`C:\elsewhere\named-by-the-operator`)] - if !present { - t.Fatal("SETUP INVALID: the plan has no entry for the operator-named deny path") - } - if named != "" { - t.Errorf("operator-named path carries anchor %q, want none: its intermediates are not the sandbox's to police", named) - } -} diff --git a/internal/sandbox/windows_acl_test.go b/internal/sandbox/windows_acl_test.go index 257198cd6..2271b2cd5 100644 --- a/internal/sandbox/windows_acl_test.go +++ b/internal/sandbox/windows_acl_test.go @@ -240,3 +240,103 @@ func TestDedupeWindowsACLEntriesKeepsInheritanceVariants(t *testing.T) { t.Fatalf("dedupe order/shape = %#v, want first NoInherit then inheritable", out) } } + +// THE PLAN HAS TO CARRY THE ANCHOR, OR THE APPLY HAS NOTHING TO ENFORCE. +// +// Every test above hands applyWindowsACLPathGroup an anchor directly, so they +// pass whether or not anything ever sets one. This drives the real builder and +// checks which entries come out anchored: the carveouts derived from a write +// root, and not the paths the operator named, which have no owned tail and whose +// intermediates are the operator's own business. +func TestBuildWindowsACLPlanAnchorsDerivedCarveouts(t *testing.T) { + home := t.TempDir() + config := WindowsSandboxCommandConfig{ + SandboxHome: home, + WorkspaceRoots: []string{`C:\workspace`}, + PermissionProfile: PermissionProfile{ + FileSystem: FileSystemPolicy{ + Kind: FileSystemRestricted, + WriteRoots: []WritableRoot{{ + Root: `C:\workspace`, + ReadOnlySubpaths: []string{`C:\workspace\.git\hooks`}, + ProtectedMetadataNames: []string{".zero"}, + }}, + DenyWrite: []string{`C:\elsewhere\named-by-the-operator`}, + }, + Network: NetworkPolicy{Mode: NetworkDeny}, + }, + } + + plan, err := BuildWindowsACLPlan(config) + if err != nil { + t.Fatalf("BuildWindowsACLPlan: %v", err) + } + + anchors := map[string]string{} + for _, entry := range plan.Entries { + anchors[strings.ToLower(entry.Path)] = entry.Anchor + } + for _, derived := range []string{`C:\workspace\.git\hooks`, `C:\workspace\.zero`} { + anchor, present := anchors[strings.ToLower(derived)] + if !present { + t.Fatalf("SETUP INVALID: the plan has no entry for the derived carveout %s", derived) + } + if anchor != `C:\workspace` { + t.Errorf("derived carveout %s carries anchor %q, want the write root it came from", derived, anchor) + } + } + named, present := anchors[strings.ToLower(`C:\elsewhere\named-by-the-operator`)] + if !present { + t.Fatal("SETUP INVALID: the plan has no entry for the operator-named deny path") + } + if named != "" { + t.Errorf("operator-named path carries anchor %q, want none: its intermediates are not the sandbox's to police", named) + } +} + +// AND AN OUT-OF-ROOT READ-ONLY SUBPATH IS NOT ANCHORED AT ALL. +// +// ReadOnlySubpaths is a profile field an operator can set to any path. One +// placed outside the write root is a configuration that works today, and +// anchoring it to that root would turn it into a containment refusal: the +// object is not under the root and never was. Only the paths that are actually +// under it are held to it. +func TestBuildWindowsACLPlanLeavesAnOutOfRootSubpathUnanchored(t *testing.T) { + home := t.TempDir() + plan, err := BuildWindowsACLPlan(WindowsSandboxCommandConfig{ + SandboxHome: home, + WorkspaceRoots: []string{`C:\workspace`}, + PermissionProfile: PermissionProfile{ + FileSystem: FileSystemPolicy{ + Kind: FileSystemRestricted, + WriteRoots: []WritableRoot{{ + Root: `C:\workspace`, + ReadOnlySubpaths: []string{`C:\workspace\vendor`, `D:\somewhere-else\shared`}, + }}, + }, + Network: NetworkPolicy{Mode: NetworkDeny}, + }, + }) + if err != nil { + t.Fatalf("BuildWindowsACLPlan: %v", err) + } + + seen := map[string]string{} + for _, entry := range plan.Entries { + seen[strings.ToLower(entry.Path)] = entry.Anchor + } + inside, present := seen[strings.ToLower(`C:\workspace\vendor`)] + if !present { + t.Fatal("SETUP INVALID: the plan has no entry for the in-root subpath") + } + if inside != `C:\workspace` { + t.Errorf("in-root subpath carries anchor %q, want the write root", inside) + } + outside, present := seen[strings.ToLower(`D:\somewhere-else\shared`)] + if !present { + t.Fatal("SETUP INVALID: the plan has no entry for the out-of-root subpath") + } + if outside != "" { + t.Errorf("out-of-root subpath carries anchor %q, want none: the apply would refuse a config that works today", outside) + } +} From c4fff2b1deb64693794eb4904b47d486267961cf Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Wed, 9 Sep 2026 22:18:09 +0530 Subject: [PATCH 5/5] test(sandbox): give the cross-platform anchor tests native paths Moving these beside the other BuildWindowsACLPlan tests made them run on Linux and macOS, where their Windows path literals stop meaning what they say: pathWithinRoot is filepath.Rel underneath, a backslash is an ordinary character off Windows, and C:\workspace\.git\hooks is then one component that is not under C:\workspace, so every anchor came back empty. The neighbouring tests get away with such literals because they only compare strings they built the same way and never ask whether one contains another. Paths are built with filepath.Join from temp roots now, so the containment question is asked in the separator the running platform actually uses. --- internal/sandbox/windows_acl_test.go | 52 ++++++++++++++++++---------- 1 file changed, 33 insertions(+), 19 deletions(-) diff --git a/internal/sandbox/windows_acl_test.go b/internal/sandbox/windows_acl_test.go index 2271b2cd5..bcabedbdb 100644 --- a/internal/sandbox/windows_acl_test.go +++ b/internal/sandbox/windows_acl_test.go @@ -243,25 +243,36 @@ func TestDedupeWindowsACLEntriesKeepsInheritanceVariants(t *testing.T) { // THE PLAN HAS TO CARRY THE ANCHOR, OR THE APPLY HAS NOTHING TO ENFORCE. // -// Every test above hands applyWindowsACLPathGroup an anchor directly, so they +// The apply-side tests hand applyWindowsACLPathGroup an anchor directly, so they // pass whether or not anything ever sets one. This drives the real builder and // checks which entries come out anchored: the carveouts derived from a write // root, and not the paths the operator named, which have no owned tail and whose // intermediates are the operator's own business. +// +// NATIVE PATHS, NOT WINDOWS LITERALS. The builder is cross-platform and so is +// this test, and the anchoring decision runs through pathWithinRoot, which is +// filepath.Rel underneath. On Linux a backslash is an ordinary character, so +// `C:\workspace\.git\hooks` is one component and is not under `C:\workspace`; +// the neighbouring tests get away with such literals because they only compare +// strings they built the same way and never ask whether one contains another. func TestBuildWindowsACLPlanAnchorsDerivedCarveouts(t *testing.T) { home := t.TempDir() + workspace := t.TempDir() + carveout := filepath.Join(workspace, ".git", "hooks") + metadata := filepath.Join(workspace, ".zero") + named := filepath.Join(t.TempDir(), "named-by-the-operator") config := WindowsSandboxCommandConfig{ SandboxHome: home, - WorkspaceRoots: []string{`C:\workspace`}, + WorkspaceRoots: []string{workspace}, PermissionProfile: PermissionProfile{ FileSystem: FileSystemPolicy{ Kind: FileSystemRestricted, WriteRoots: []WritableRoot{{ - Root: `C:\workspace`, - ReadOnlySubpaths: []string{`C:\workspace\.git\hooks`}, + Root: workspace, + ReadOnlySubpaths: []string{carveout}, ProtectedMetadataNames: []string{".zero"}, }}, - DenyWrite: []string{`C:\elsewhere\named-by-the-operator`}, + DenyWrite: []string{named}, }, Network: NetworkPolicy{Mode: NetworkDeny}, }, @@ -276,21 +287,21 @@ func TestBuildWindowsACLPlanAnchorsDerivedCarveouts(t *testing.T) { for _, entry := range plan.Entries { anchors[strings.ToLower(entry.Path)] = entry.Anchor } - for _, derived := range []string{`C:\workspace\.git\hooks`, `C:\workspace\.zero`} { + for _, derived := range []string{carveout, metadata} { anchor, present := anchors[strings.ToLower(derived)] if !present { t.Fatalf("SETUP INVALID: the plan has no entry for the derived carveout %s", derived) } - if anchor != `C:\workspace` { + if anchor != workspace { t.Errorf("derived carveout %s carries anchor %q, want the write root it came from", derived, anchor) } } - named, present := anchors[strings.ToLower(`C:\elsewhere\named-by-the-operator`)] + operatorAnchor, present := anchors[strings.ToLower(named)] if !present { t.Fatal("SETUP INVALID: the plan has no entry for the operator-named deny path") } - if named != "" { - t.Errorf("operator-named path carries anchor %q, want none: its intermediates are not the sandbox's to police", named) + if operatorAnchor != "" { + t.Errorf("operator-named path carries anchor %q, want none: its intermediates are not the sandbox's to police", operatorAnchor) } } @@ -303,15 +314,18 @@ func TestBuildWindowsACLPlanAnchorsDerivedCarveouts(t *testing.T) { // under it are held to it. func TestBuildWindowsACLPlanLeavesAnOutOfRootSubpathUnanchored(t *testing.T) { home := t.TempDir() + workspace := t.TempDir() + inside := filepath.Join(workspace, "vendor") + outside := filepath.Join(t.TempDir(), "shared") plan, err := BuildWindowsACLPlan(WindowsSandboxCommandConfig{ SandboxHome: home, - WorkspaceRoots: []string{`C:\workspace`}, + WorkspaceRoots: []string{workspace}, PermissionProfile: PermissionProfile{ FileSystem: FileSystemPolicy{ Kind: FileSystemRestricted, WriteRoots: []WritableRoot{{ - Root: `C:\workspace`, - ReadOnlySubpaths: []string{`C:\workspace\vendor`, `D:\somewhere-else\shared`}, + Root: workspace, + ReadOnlySubpaths: []string{inside, outside}, }}, }, Network: NetworkPolicy{Mode: NetworkDeny}, @@ -325,18 +339,18 @@ func TestBuildWindowsACLPlanLeavesAnOutOfRootSubpathUnanchored(t *testing.T) { for _, entry := range plan.Entries { seen[strings.ToLower(entry.Path)] = entry.Anchor } - inside, present := seen[strings.ToLower(`C:\workspace\vendor`)] + insideAnchor, present := seen[strings.ToLower(inside)] if !present { t.Fatal("SETUP INVALID: the plan has no entry for the in-root subpath") } - if inside != `C:\workspace` { - t.Errorf("in-root subpath carries anchor %q, want the write root", inside) + if insideAnchor != workspace { + t.Errorf("in-root subpath carries anchor %q, want the write root", insideAnchor) } - outside, present := seen[strings.ToLower(`D:\somewhere-else\shared`)] + outsideAnchor, present := seen[strings.ToLower(outside)] if !present { t.Fatal("SETUP INVALID: the plan has no entry for the out-of-root subpath") } - if outside != "" { - t.Errorf("out-of-root subpath carries anchor %q, want none: the apply would refuse a config that works today", outside) + if outsideAnchor != "" { + t.Errorf("out-of-root subpath carries anchor %q, want none: the apply would refuse a config that works today", outsideAnchor) } }