From 4a56e6c7d6abc086ee55d14f62fecd84f38fe06e Mon Sep 17 00:00:00 2001 From: Thales Maciel Date: Tue, 28 Apr 2026 15:26:56 -0300 Subject: [PATCH] roothelper: walk validateManagedPath components, reject symlinks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit validateManagedPath was textual-only: filepath.Clean + dest-prefix match. That stopped `..` escapes but not the symlink-bypass attack that motivated this fix — a daemon-UID attacker can write into StateDir/RuntimeDir (it's their UID), so they can plant `/redirect -> /etc` and any helper RPC that then operates on `/redirect/...` resolves through the symlink at the kernel and lands at /etc/... on the host. Concretely the leaks this closed: * priv.create_dm_snapshot: rootfs/cow paths fed to losetup — losetup follows the symlink and attaches a host block device. * priv.launch_firecracker: kernel/initrd paths hard-linked into the chroot via `ln -f` — link(2) on Linux follows source symlinks, hard-linking host files into the jail. * priv.read_ext4_file / priv.write_ext4_files: image paths fed to debugfs / e2cp as root. * validateLaunchDrivePath: drive paths mknod'd or hard-linked. * validateJailerOpts: chroot base. Fix: after the existing prefix match, walk every component below the matched root with Lstat. Any existing symlink — leaf or intermediate — fails the validator. ENOENT is tolerated because several callers pass paths firecracker/the helper materialise later (sockets, log files, kernel hard-link targets); whoever materialises them goes through the same validation when the helper-side primitive runs. Subsumes most of validateNotSymlink's coverage but the explicit call sites (methodEnsureSocketAccess, methodCleanupJailerChroot) keep their belt-and-braces check — those paths must EXIST and not be symlinks, which validateNotSymlink enforces strictly while the broadened validateManagedPath tolerates ENOENT. Race-free in practice: helper RPCs are short and the validator fires on the same kernel state the next syscall sees. The helper loop processes RPCs serially per-connection, and the validator plus the syscall both run as root within microseconds of each other. Four new tests cover symlink leaf, symlink intermediate, missing leaf (must pass), and the plain happy path. Smoke at JOBS=4 still green — every legitimate daemon-supplied path passes the walk. Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/roothelper/roothelper.go | 40 ++++++++++++- internal/roothelper/roothelper_test.go | 77 ++++++++++++++++++++++++++ 2 files changed, 115 insertions(+), 2 deletions(-) diff --git a/internal/roothelper/roothelper.go b/internal/roothelper/roothelper.go index 4310aca..32c625f 100644 --- a/internal/roothelper/roothelper.go +++ b/internal/roothelper/roothelper.go @@ -1037,6 +1037,7 @@ func (s *Server) validateManagedPath(path string, roots ...string) error { return fmt.Errorf("path %q must be absolute", path) } cleaned := filepath.Clean(path) + var matched string for _, root := range roots { root = strings.TrimSpace(root) if root == "" { @@ -1044,10 +1045,45 @@ func (s *Server) validateManagedPath(path string, roots ...string) error { } root = filepath.Clean(root) if cleaned == root || strings.HasPrefix(cleaned, root+string(os.PathSeparator)) { - return nil + matched = root + break } } - return fmt.Errorf("path %q is outside banger-managed directories", path) + if matched == "" { + return fmt.Errorf("path %q is outside banger-managed directories", path) + } + // Walk each component below the matched root with Lstat and refuse + // symlinks. Without this, validation was textual-only: a daemon-UID + // attacker could plant a symlink under StateDir/RuntimeDir and get + // the helper to drive losetup, ln -f, debugfs, e2cp, fsck, etc. at + // the dereferenced target (host devices, /etc/shadow, …). + // + // ENOENT is tolerated: some callers pass paths that firecracker + // creates after this check (sockets, log files). Anything missing + // can't be a symlink at this instant; whoever materialises it later + // goes through the helper's create primitives, which validate again. + if cleaned == matched { + return nil + } + suffix := strings.TrimPrefix(cleaned, matched+string(os.PathSeparator)) + cur := matched + for _, seg := range strings.Split(suffix, string(os.PathSeparator)) { + if seg == "" { + continue + } + cur = filepath.Join(cur, seg) + info, err := os.Lstat(cur) + if err != nil { + if os.IsNotExist(err) { + return nil + } + return fmt.Errorf("inspect %q: %w", cur, err) + } + if info.Mode()&os.ModeSymlink != 0 { + return fmt.Errorf("path %q has a symlink at %q", path, cur) + } + } + return nil } // validateExt4ImagePath accepts a path that is either inside the diff --git a/internal/roothelper/roothelper_test.go b/internal/roothelper/roothelper_test.go index a5ce078..6f8fb8b 100644 --- a/internal/roothelper/roothelper_test.go +++ b/internal/roothelper/roothelper_test.go @@ -237,6 +237,83 @@ func TestValidateDMSnapshotHandles(t *testing.T) { } } +// TestValidateManagedPathRejectsSymlinkLeaf pins the leaf-symlink +// rejection: even when the path string sits inside a managed root, a +// symlink at the final component must be refused. Otherwise a +// daemon-UID attacker could plant `/foo -> /etc/shadow` and +// get the helper to drive privileged tooling against host files. +func TestValidateManagedPathRejectsSymlinkLeaf(t *testing.T) { + t.Parallel() + srv := &Server{} + root := t.TempDir() + target := filepath.Join(t.TempDir(), "outside") + if err := os.WriteFile(target, []byte("secret"), 0o600); err != nil { + t.Fatalf("write target: %v", err) + } + link := filepath.Join(root, "leak") + if err := os.Symlink(target, link); err != nil { + t.Fatalf("symlink: %v", err) + } + err := srv.validateManagedPath(link, root) + if err == nil { + t.Fatal("validateManagedPath(symlink leaf) succeeded, want error") + } +} + +// TestValidateManagedPathRejectsSymlinkIntermediate pins ancestor +// symlink rejection. Without the walk, an attacker plants +// `/dir -> /etc` and a path like `/dir/passwd` +// passes the textual prefix check but resolves to /etc/passwd at op +// time. +func TestValidateManagedPathRejectsSymlinkIntermediate(t *testing.T) { + t.Parallel() + srv := &Server{} + root := t.TempDir() + target := t.TempDir() + link := filepath.Join(root, "redirect") + if err := os.Symlink(target, link); err != nil { + t.Fatalf("symlink: %v", err) + } + err := srv.validateManagedPath(filepath.Join(link, "passwd"), root) + if err == nil { + t.Fatal("validateManagedPath(symlink intermediate) succeeded, want error") + } +} + +// TestValidateManagedPathToleratesMissingLeaf confirms ENOENT does +// not flip the validator into a fail. Several callers pass paths +// firecracker (or the helper's own staging) creates AFTER validation +// — sockets, log files, kernel hard-link targets — and a strict +// existence check would break those flows. +func TestValidateManagedPathToleratesMissingLeaf(t *testing.T) { + t.Parallel() + srv := &Server{} + root := t.TempDir() + missing := filepath.Join(root, "deeper", "not-yet") + if err := srv.validateManagedPath(missing, root); err != nil { + t.Fatalf("validateManagedPath(missing leaf) = %v, want nil", err) + } +} + +// TestValidateManagedPathPassesPlainSubpath is the happy path: a +// regular file inside a real subdir should sail through the new walk. +func TestValidateManagedPathPassesPlainSubpath(t *testing.T) { + t.Parallel() + srv := &Server{} + root := t.TempDir() + subdir := filepath.Join(root, "vms", "abc") + if err := os.MkdirAll(subdir, 0o755); err != nil { + t.Fatalf("mkdir: %v", err) + } + leaf := filepath.Join(subdir, "rootfs.ext4") + if err := os.WriteFile(leaf, []byte("data"), 0o644); err != nil { + t.Fatalf("write leaf: %v", err) + } + if err := srv.validateManagedPath(leaf, root); err != nil { + t.Fatalf("validateManagedPath(plain subpath) = %v, want nil", err) + } +} + func TestValidateLinuxIfaceName(t *testing.T) { t.Parallel()