amadan
instancefs: refuse a symlinked dir itself, and read every mountinfo line
openDir used a plain O_NOFOLLOW open, which only refuses a symlink at the final component of dir; a same-uid process that swaps the instance directory itself for a symlink before the mount option lands would sail through. Switch to Openat2 with RESOLVE_NO_SYMLINKS|RESOLVE_NO_MAGICLINKS against AT_FDCWD, which refuses a symlink at any component of dir. RESOLVE_BENEATH stays off: dir is an absolute path handed in by the caller, not a name resolved beneath some other fd. MountNoSymfollow returned on the first mountinfo line that matched both the mountpoint and the nosymfollow option, but stacked mounts at one mountpoint list oldest first, so the LAST line is the one actually in effect. Scan every line and keep the last matching line's verdict instead. Split the scan into a testable mountNoSymfollow(io.Reader, ...) so a fixture can exercise the ordering directly.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YZ1ZvWj7naf73GjTX8MBv5
2 files changed,
+86
−10
internal/instancefs/instancefs_linux.go+49 −10internal/instancefs/instancefs_linux_test.go+37 −0
diff --git a/internal/instancefs/instancefs_linux.go b/internal/instancefs/instancefs_linux.go| index 4c6ca657a..468a5510c 100644 |
| --- a/internal/instancefs/instancefs_linux.go |
| +++ b/internal/instancefs/instancefs_linux.go |
| @@ -8,6 +8,7 @@ import ( |
| "encoding/hex" |
| "errors" |
| "fmt" |
| + "io" |
| "os" |
| "path/filepath" |
| "strings" |
| @@ -17,17 +18,26 @@ import ( |
| const resolveFlags = unix.RESOLVE_BENEATH | unix.RESOLVE_NO_SYMLINKS | unix.RESOLVE_NO_MAGICLINKS |
| -// openDir opens dir itself as a directory fd, also refusing symlinks in |
| -// dir's own components: the instance directory's parent is the agent's, |
| -// but the instance directory itself could have been swapped for a |
| -// symlink by a same-uid process before the mount option landed. |
| +// openDir opens dir itself as a directory fd, refusing a symlink at ANY |
| +// component of dir, not only its final one: the instance directory's |
| +// parent is the agent's, but a same-uid process could have swapped an |
| +// intermediate component for a symlink before the mount option landed, |
| +// and openat2's RESOLVE_NO_SYMLINKS catches that the way a plain |
| +// O_NOFOLLOW open (which only refuses a symlink at the leaf) would not. |
| +// RESOLVE_BENEATH is deliberately absent here: dir is an absolute path |
| +// handed in by the caller, not a name resolved beneath some other fd, so |
| +// there is no "beneath" for it to stay under. |
| func openDir(dir string) (int, error) { |
| - fd, err := unix.Open(dir, unix.O_PATH|unix.O_DIRECTORY|unix.O_NOFOLLOW|unix.O_CLOEXEC, 0) |
| + how := unix.OpenHow{ |
| + Flags: unix.O_PATH | unix.O_DIRECTORY | unix.O_CLOEXEC, |
| + Resolve: unix.RESOLVE_NO_SYMLINKS | unix.RESOLVE_NO_MAGICLINKS, |
| + } |
| + fd, err := unix.Openat2(unix.AT_FDCWD, dir, &how) |
| if err != nil { |
| if errors.Is(err, unix.ELOOP) { |
| return -1, fmt.Errorf("%w: %s", ErrSymlink, dir) |
| } |
| - return -1, &os.PathError{Op: "open", Path: dir, Err: err} |
| + return -1, &os.PathError{Op: "openat2", Path: dir, Err: err} |
| } |
| return fd, nil |
| } |
| @@ -124,14 +134,43 @@ func MountNoSymfollow(mountpoint string) (bool, error) { |
| return false, err |
| } |
| defer f.Close() |
| + return mountNoSymfollow(f, mountpoint) |
| +} |
| + |
| +// mountNoSymfollow is MountNoSymfollow's body, taking its mountinfo source |
| +// as an io.Reader so a test can feed it a fixture with stacked mount lines |
| +// instead of the live /proc/self/mountinfo. |
| +// |
| +// Stacked mounts at one mountpoint (a bind mount, then a later remount) |
| +// list oldest first in mountinfo, and the LAST line for a mountpoint is |
| +// the one actually in effect. Every line is therefore scanned to the end |
| +// and the last matching line's verdict remembered, rather than returning |
| +// on the first match: an earlier stacked mount carrying nosymfollow must |
| +// not outvote a later one that dropped it, or vice versa. |
| +func mountNoSymfollow(r io.Reader, mountpoint string) (bool, error) { |
| want := filepath.Clean(mountpoint) |
| - sc := bufio.NewScanner(f) |
| + sc := bufio.NewScanner(r) |
| + verdict := false |
| for sc.Scan() { |
| - if mountinfoLineHas(sc.Text(), want, "nosymfollow") { |
| - return true, nil |
| + line := sc.Text() |
| + mp, ok := mountinfoLineMountpoint(line) |
| + if !ok || mp != want { |
| + continue |
| } |
| + verdict = mountinfoLineHas(line, want, "nosymfollow") |
| + } |
| + return verdict, sc.Err() |
| +} |
| + |
| +// mountinfoLineMountpoint returns field 5 of a mountinfo line (the |
| +// mountpoint, octal-escaped), unescaped, and whether the line has enough |
| +// fields to have one at all. |
| +func mountinfoLineMountpoint(line string) (string, bool) { |
| + fields := strings.Fields(line) |
| + if len(fields) < 6 { |
| + return "", false |
| } |
| - return false, sc.Err() |
| + return unescapeMountinfo(fields[4]), true |
| } |
| // mountinfoLineHas parses one mountinfo line: field 5 is the mountpoint |
diff --git a/internal/instancefs/instancefs_linux_test.go b/internal/instancefs/instancefs_linux_test.go| index aad349adf..1c7a3d679 100644 |
| --- a/internal/instancefs/instancefs_linux_test.go |
| +++ b/internal/instancefs/instancefs_linux_test.go |
| @@ -54,6 +54,26 @@ func TestOpenBeneathRefusesASymlinkedDirectory(t *testing.T) { |
| } |
| } |
| +// TestOpenBeneathRefusesASymlinkedDir pins the review fix: openDir must |
| +// refuse a symlink at dir itself, not only at an intermediate component |
| +// under it. A same-uid process that swaps the instance directory for a |
| +// symlink before the mount option lands must be caught the same way a |
| +// symlink deeper in the path already is. |
| +func TestOpenBeneathRefusesASymlinkedDir(t *testing.T) { |
| + real := t.TempDir() |
| + if err := os.WriteFile(filepath.Join(real, "x"), []byte("x"), 0o600); err != nil { |
| + t.Fatal(err) |
| + } |
| + link := filepath.Join(t.TempDir(), "linked-dir") |
| + if err := os.Symlink(real, link); err != nil { |
| + t.Fatal(err) |
| + } |
| + _, err := ReadBeneath(link, "x") |
| + if !errors.Is(err, ErrSymlink) { |
| + t.Fatalf("ReadBeneath(%q, \"x\"): err = %v, want ErrSymlink", link, err) |
| + } |
| +} |
| + |
| func TestOpenBeneathRefusesDotDotAndAbsolute(t *testing.T) { |
| dir := t.TempDir() |
| for _, name := range []string{"../x", "a/../../x", "/etc/passwd"} { |
| @@ -134,4 +154,21 @@ func TestMountNoSymfollowReadsMountinfo(t *testing.T) { |
| if mountinfoLineHas(line, "/data", "nosymfollow") { |
| t.Error("parser matched the wrong mountpoint") |
| } |
| + |
| + // Stacked mounts at the same mountpoint list oldest first; the LAST |
| + // line is the one actually in effect, so the verdict must follow it |
| + // even when an earlier line at the same mountpoint disagrees. |
| + first := "36 35 98:0 /.instances /data/instances rw,nosuid,nodev,noexec,relatime - ext4 /dev/root rw" |
| + last := "37 35 98:0 /.instances /data/instances rw,nosuid,nodev,noexec,nosymfollow,relatime - ext4 /dev/root rw" |
| + stacked := strings.NewReader(first + "\n" + last + "\n") |
| + got, err = mountNoSymfollow(stacked, "/data/instances") |
| + if err != nil || !got { |
| + t.Errorf("stacked mounts, last with nosymfollow: got %v, %v, want true, nil", got, err) |
| + } |
| + |
| + stackedReversed := strings.NewReader(last + "\n" + first + "\n") |
| + got, err = mountNoSymfollow(stackedReversed, "/data/instances") |
| + if err != nil || got { |
| + t.Errorf("stacked mounts, last WITHOUT nosymfollow: got %v, %v, want false, nil (the earlier nosymfollow line must not outvote the effective one)", got, err) |
| + } |
| } |