From 864db8042dbb191028676f80addf8c35f348aee2 Mon Sep 17 00:00:00 2001 From: Aleksa Sarai Date: Wed, 15 Apr 2026 01:44:09 +1000 Subject: [PATCH] rootfs: make /dev initialisation code fd-based These codepaths are very old and operate on pure paths but before pivot_root(2), meaning that a bad image with a malicious /dev symlink could cause us to operate on host paths instead. In practice this means that we could be tricked into removing a file called "ptmx" (note that /dev/pts/ptmx and /dev/ptmx are both immune for different reasons) or creating a very restricted set of symlinks (with fixed targets and names). The scope of these bugs is thus quite limited, but we definitely need to harden against it. These codepaths were unfortunately missed during the fd-based rework in commit d40b3439a961 ("rootfs: switch to fd-based handling of mountpoint targets") -- I must've assumed they were called after pivot_root(2)... Fixes: GHSA-xjvp-4fhw-gc47 Fixes: CVE-2026-41579 Fixes: d40b3439a961 ("rootfs: switch to fd-based handling of mountpoint targets") Signed-off-by: Aleksa Sarai --- internal/pathrs/mkdirall.go | 14 ++++++++-- internal/pathrs/root_pathrslite.go | 45 ++++++++++++++++++++++++++++++ libcontainer/rootfs_linux.go | 42 ++++++++++++++-------------- 3 files changed, 77 insertions(+), 24 deletions(-) diff --git a/internal/pathrs/mkdirall.go b/internal/pathrs/mkdirall.go index 81c9a022c..31cc08579 100644 --- a/internal/pathrs/mkdirall.go +++ b/internal/pathrs/mkdirall.go @@ -24,6 +24,14 @@ import ( "path/filepath" ) +func splitPath(path string) (dirPath, filename string, err error) { + dirPath, filename = filepath.Split(path) + if filepath.Join("/", filename) == "/" { + return "", "", fmt.Errorf("root subpath %q has bad trailing component %q", path, filename) + } + return dirPath, filename, nil +} + // MkdirAllParentInRoot is like [MkdirAllInRoot] except that it only creates // the parent directory of the target path, returning the trailing component so // the caller has more flexibility around constructing the final inode. @@ -41,9 +49,9 @@ func MkdirAllParentInRoot(root *os.File, unsafePath string, mode os.FileMode) (* return nil, "", fmt.Errorf("failed to construct hallucinated target path: %w", err) } - dirPath, filename := filepath.Split(unsafePath) - if filepath.Join("/", filename) == "/" { - return nil, "", fmt.Errorf("create parent dir in root subpath %q has bad trailing component %q", unsafePath, filename) + dirPath, filename, err := splitPath(unsafePath) + if err != nil { + return nil, "", fmt.Errorf("split path %q for mkdir parent: %w", unsafePath, err) } dirFd, err := MkdirAllInRoot(root, dirPath, mode) diff --git a/internal/pathrs/root_pathrslite.go b/internal/pathrs/root_pathrslite.go index 0ddabf804..fc5114a85 100644 --- a/internal/pathrs/root_pathrslite.go +++ b/internal/pathrs/root_pathrslite.go @@ -19,7 +19,9 @@ package pathrs import ( + "fmt" "os" + "path/filepath" "github.com/cyphar/filepath-securejoin/pathrs-lite" "golang.org/x/sys/unix" @@ -65,3 +67,46 @@ func CreateInRoot(root *os.File, subpath string, flags int, fileMode uint32) (*o } return os.NewFile(uintptr(fd), root.Name()+"/"+subpath), nil } + +// UnlinkInRoot deletes the inode specified at the given subpath. If you pass +// [unix.AT_REMOVEDIR] it will remove directories, otherwise it will remove +// non-directory inodes. +func UnlinkInRoot(root *os.File, subpath string, flags int) error { + dirPath, filename, err := splitPath(subpath) + if err != nil { + return fmt.Errorf("split path %q for unlink: %w", subpath, err) + } + + dirFd := root + if filepath.Join("/", dirPath) != "/" { + newDirFd, err := OpenInRoot(root, dirPath, unix.O_DIRECTORY|unix.O_PATH) + if err != nil { + return fmt.Errorf("failed to open parent directory %q for unlink: %w", dirPath, err) + } + dirFd = newDirFd + defer dirFd.Close() + } + + err = unix.Unlinkat(int(dirFd.Fd()), filename, flags) + if err != nil { + err = &os.PathError{Op: "unlinkat", Path: dirFd.Name() + "/" + filename, Err: err} + } + return err +} + +// SymlinkInRoot creates a symlink inside a root with the given target (as well +// as creating any missing parent directories). If the subpath already exists, +// an error is returned. +func SymlinkInRoot(linktarget string, root *os.File, subpath string) error { + dirFd, filename, err := MkdirAllParentInRoot(root, subpath, 0o755) + if err != nil { + return err + } + defer dirFd.Close() + + err = unix.Symlinkat(linktarget, int(dirFd.Fd()), filename) + if err != nil { + err = &os.PathError{Op: "symlinkat", Path: dirFd.Name() + "/" + filename, Err: err} + } + return err +} diff --git a/libcontainer/rootfs_linux.go b/libcontainer/rootfs_linux.go index 8bd5d1ef8..7accf3648 100644 --- a/libcontainer/rootfs_linux.go +++ b/libcontainer/rootfs_linux.go @@ -97,6 +97,19 @@ func needsSetupDev(config *configs.Config) bool { return true } +func doSetupDev(rootFd *os.File, config *configs.Config) error { + if err := createDevices(rootFd, config); err != nil { + return fmt.Errorf("error creating device nodes: %w", err) + } + if err := setupPtmx(rootFd); err != nil { + return fmt.Errorf("error setting up ptmx: %w", err) + } + if err := setupDevSymlinks(rootFd); err != nil { + return fmt.Errorf("error setting up /dev symlinks: %w", err) + } + return nil +} + // setupAndMountToRootfs sets up the mount for a single mount point and mounts it to the rootfs. func setupAndMountToRootfs(pipe *syncSocket, config *configs.Config, mountConfig *mountConfig, m *configs.Mount) error { entry := mountEntry{Mount: m} @@ -184,14 +197,8 @@ func prepareRootfs(pipe *syncSocket, iConfig *initConfig) (err error) { setupDev := needsSetupDev(config) if setupDev { - if err := createDevices(rootFd, config); err != nil { - return fmt.Errorf("error creating device nodes: %w", err) - } - if err := setupPtmx(config); err != nil { - return fmt.Errorf("error setting up ptmx: %w", err) - } - if err := setupDevSymlinks(config.Rootfs); err != nil { - return fmt.Errorf("error setting up /dev symlinks: %w", err) + if err := doSetupDev(rootFd, config); err != nil { + return fmt.Errorf("configuring container /dev: %w", err) } } @@ -893,7 +900,7 @@ func checkProcMount(rootfs, dest string, m mountEntry) error { return fmt.Errorf("%q cannot be mounted because it is inside /proc", dest) } -func setupDevSymlinks(rootfs string) error { +func setupDevSymlinks(rootFd *os.File) error { // In theory, these should be links to /proc/thread-self, but systems // expect these to be /proc/self and this matches how most distributions // work. @@ -909,11 +916,8 @@ func setupDevSymlinks(rootfs string) error { links = append(links, [2]string{"/proc/kcore", "/dev/core"}) } for _, link := range links { - var ( - src = link[0] - dst = filepath.Join(rootfs, link[1]) - ) - if err := os.Symlink(src, dst); err != nil && !errors.Is(err, os.ErrExist) { + target, devName := link[0], link[1] + if err := pathrs.SymlinkInRoot(target, rootFd, devName); err != nil && !errors.Is(err, os.ErrExist) { return err } } @@ -1129,15 +1133,11 @@ func setReadonly() error { return mount("", "/", "", flags, "") } -func setupPtmx(config *configs.Config) error { - ptmx := filepath.Join(config.Rootfs, "dev/ptmx") - if err := os.Remove(ptmx); err != nil && !errors.Is(err, os.ErrNotExist) { +func setupPtmx(rootFd *os.File) error { + if err := pathrs.UnlinkInRoot(rootFd, "/dev/ptmx", 0); err != nil && !errors.Is(err, os.ErrNotExist) { return err } - if err := os.Symlink("pts/ptmx", ptmx); err != nil { - return err - } - return nil + return pathrs.SymlinkInRoot("pts/ptmx", rootFd, "/dev/ptmx") } // pivotRoot will call pivot_root such that rootfs becomes the new root