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()