From 4c89edc65c34e9a8b0ce3f571f5d82947ee66755 Mon Sep 17 00:00:00 2001 From: Aleksa Sarai Date: Sun, 9 Nov 2025 01:40:57 +1100 Subject: [PATCH 01/13] libcontainer: move CleanPath and StripRoot to internal/pathrs These helpers will be needed for the compatibility code added in future patches in this series, but because "internal/pathrs" is imported by "libcontainer/utils" we need to move them so that we can avoid circular dependencies. Because the old functions were in a non-internal package it is possible some downstreams use them, so add some wrappers but mark them as deprecated. Signed-off-by: Aleksa Sarai (cherry picked from commit 42a1e19d6788fde798fa960f047afbffbc319f8e) Signed-off-by: Aleksa Sarai --- internal/pathrs/path.go | 50 +++++++++++++++++ internal/pathrs/path_test.go | 67 +++++++++++++++++++++++ libcontainer/apparmor/apparmor_linux.go | 3 +- libcontainer/factory_linux.go | 4 +- libcontainer/rootfs_linux.go | 14 ++--- libcontainer/specconv/spec_linux.go | 12 ++--- libcontainer/utils/utils.go | 71 ++++++++----------------- libcontainer/utils/utils_test.go | 67 ----------------------- libcontainer/utils/utils_unix.go | 5 +- 9 files changed, 158 insertions(+), 135 deletions(-) diff --git a/internal/pathrs/path.go b/internal/pathrs/path.go index 1ee7c795d..709c3ae3f 100644 --- a/internal/pathrs/path.go +++ b/internal/pathrs/path.go @@ -19,6 +19,8 @@ package pathrs import ( + "os" + "path/filepath" "strings" ) @@ -32,3 +34,51 @@ func IsLexicallyInRoot(root, path string) bool { path = strings.TrimRight(path, "/") return strings.HasPrefix(path+"/", root+"/") } + +// LexicallyCleanPath makes a path safe for use with filepath.Join. This is +// done by not only cleaning the path, but also (if the path is relative) +// adding a leading '/' and cleaning it (then removing the leading '/'). This +// ensures that a path resulting from prepending another path will always +// resolve to lexically be a subdirectory of the prefixed path. This is all +// done lexically, so paths that include symlinks won't be safe as a result of +// using CleanPath. +func LexicallyCleanPath(path string) string { + // Deal with empty strings nicely. + if path == "" { + return "" + } + + // Ensure that all paths are cleaned (especially problematic ones like + // "/../../../../../" which can cause lots of issues). + + if filepath.IsAbs(path) { + return filepath.Clean(path) + } + + // If the path isn't absolute, we need to do more processing to fix paths + // such as "../../../..//some/path". We also shouldn't convert absolute + // paths to relative ones. + path = filepath.Clean(string(os.PathSeparator) + path) + // This can't fail, as (by definition) all paths are relative to root. + path, _ = filepath.Rel(string(os.PathSeparator), path) + + return path +} + +// LexicallyStripRoot returns the passed path, stripping the root path if it +// was (lexicially) inside it. Note that both passed paths will always be +// treated as absolute, and the returned path will also always be absolute. In +// addition, the paths are cleaned before stripping the root. +func LexicallyStripRoot(root, path string) string { + // Make the paths clean and absolute. + root, path = LexicallyCleanPath("/"+root), LexicallyCleanPath("/"+path) + switch { + case path == root: + path = "/" + case root == "/": + // do nothing + default: + path = strings.TrimPrefix(path, root+"/") + } + return LexicallyCleanPath("/" + path) +} diff --git a/internal/pathrs/path_test.go b/internal/pathrs/path_test.go index 19d577fba..b7fdde6b9 100644 --- a/internal/pathrs/path_test.go +++ b/internal/pathrs/path_test.go @@ -51,3 +51,70 @@ func TestIsLexicallyInRoot(t *testing.T) { }) } } + +func TestLexicallyCleanPath(t *testing.T) { + path := LexicallyCleanPath("") + if path != "" { + t.Errorf("expected to receive empty string and received %s", path) + } + + path = LexicallyCleanPath("rootfs") + if path != "rootfs" { + t.Errorf("expected to receive 'rootfs' and received %s", path) + } + + path = LexicallyCleanPath("../../../var") + if path != "var" { + t.Errorf("expected to receive 'var' and received %s", path) + } + + path = LexicallyCleanPath("/../../../var") + if path != "/var" { + t.Errorf("expected to receive '/var' and received %s", path) + } + + path = LexicallyCleanPath("/foo/bar/") + if path != "/foo/bar" { + t.Errorf("expected to receive '/foo/bar' and received %s", path) + } + + path = LexicallyCleanPath("/foo/bar/../") + if path != "/foo" { + t.Errorf("expected to receive '/foo' and received %s", path) + } +} + +func TestLexicallyStripRoot(t *testing.T) { + for _, test := range []struct { + root, path, out string + }{ + // Works with multiple components. + {"/a/b", "/a/b/c", "/c"}, + {"/hello/world", "/hello/world/the/quick-brown/fox", "/the/quick-brown/fox"}, + // '/' must be a no-op. + {"/", "/a/b/c", "/a/b/c"}, + // Must be the correct order. + {"/a/b", "/a/c/b", "/a/c/b"}, + // Must be at start. + {"/abc/def", "/foo/abc/def/bar", "/foo/abc/def/bar"}, + // Must be a lexical parent. + {"/foo/bar", "/foo/barSAMECOMPONENT", "/foo/barSAMECOMPONENT"}, + // Must only strip the root once. + {"/foo/bar", "/foo/bar/foo/bar/baz", "/foo/bar/baz"}, + // Deal with .. in a fairly sane way. + {"/foo/bar", "/foo/bar/../baz", "/foo/baz"}, + {"/foo/bar", "../../../../../../foo/bar/baz", "/baz"}, + {"/foo/bar", "/../../../../../../foo/bar/baz", "/baz"}, + {"/foo/bar/../baz", "/foo/baz/bar", "/bar"}, + {"/foo/bar/../baz", "/foo/baz/../bar/../baz/./foo", "/foo"}, + // All paths are made absolute before stripping. + {"foo/bar", "/foo/bar/baz/bee", "/baz/bee"}, + {"/foo/bar", "foo/bar/baz/beef", "/baz/beef"}, + {"foo/bar", "foo/bar/baz/beets", "/baz/beets"}, + } { + got := LexicallyStripRoot(test.root, test.path) + if got != test.out { + t.Errorf("LexicallyStripRoot(%q, %q) -- got %q, expected %q", test.root, test.path, got, test.out) + } + } +} diff --git a/libcontainer/apparmor/apparmor_linux.go b/libcontainer/apparmor/apparmor_linux.go index a3a8e9325..79a3e29cb 100644 --- a/libcontainer/apparmor/apparmor_linux.go +++ b/libcontainer/apparmor/apparmor_linux.go @@ -9,7 +9,6 @@ import ( "golang.org/x/sys/unix" "github.com/opencontainers/runc/internal/pathrs" - "github.com/opencontainers/runc/libcontainer/utils" ) var ( @@ -29,7 +28,7 @@ func isEnabled() bool { } func setProcAttr(attr, value string) error { - attr = utils.CleanPath(attr) + attr = pathrs.LexicallyCleanPath(attr) attrSubPath := "attr/apparmor/" + attr if _, err := os.Stat("/proc/self/" + attrSubPath); errors.Is(err, os.ErrNotExist) { // fall back to the old convention diff --git a/libcontainer/factory_linux.go b/libcontainer/factory_linux.go index 94b55eaa8..bc3349ab8 100644 --- a/libcontainer/factory_linux.go +++ b/libcontainer/factory_linux.go @@ -11,10 +11,10 @@ import ( "github.com/opencontainers/cgroups" "github.com/opencontainers/cgroups/manager" + "github.com/opencontainers/runc/internal/pathrs" "github.com/opencontainers/runc/libcontainer/configs" "github.com/opencontainers/runc/libcontainer/configs/validate" "github.com/opencontainers/runc/libcontainer/intelrdt" - "github.com/opencontainers/runc/libcontainer/utils" ) const ( @@ -211,7 +211,7 @@ func validateID(id string) error { } - if string(os.PathSeparator)+id != utils.CleanPath(string(os.PathSeparator)+id) { + if string(os.PathSeparator)+id != pathrs.LexicallyCleanPath(string(os.PathSeparator)+id) { return ErrInvalidID } diff --git a/libcontainer/rootfs_linux.go b/libcontainer/rootfs_linux.go index 0bd49c4d1..b9e3a04c4 100644 --- a/libcontainer/rootfs_linux.go +++ b/libcontainer/rootfs_linux.go @@ -90,7 +90,7 @@ func (m mountEntry) srcStatfs() (*unix.Statfs_t, error) { // needsSetupDev returns true if /dev needs to be set up. func needsSetupDev(config *configs.Config) bool { for _, m := range config.Mounts { - if m.Device == "bind" && utils.CleanPath(m.Destination) == "/dev" { + if m.Device == "bind" && pathrs.LexicallyCleanPath(m.Destination) == "/dev" { return false } } @@ -256,7 +256,7 @@ func finalizeRootfs(config *configs.Config) (err error) { if m.Flags&unix.MS_RDONLY != unix.MS_RDONLY { continue } - if m.Device == "tmpfs" || utils.CleanPath(m.Destination) == "/dev" { + if m.Device == "tmpfs" || pathrs.LexicallyCleanPath(m.Destination) == "/dev" { if err := remountReadonly(m); err != nil { return err } @@ -505,7 +505,7 @@ func statfsToMountFlags(st unix.Statfs_t) int { var errRootfsToFile = errors.New("config tries to change rootfs to file") func (m *mountEntry) createOpenMountpoint(rootfs string) (Err error) { - unsafePath := utils.StripRoot(rootfs, m.Destination) + unsafePath := pathrs.LexicallyStripRoot(rootfs, m.Destination) dstFile, err := pathrs.OpenInRoot(rootfs, unsafePath, unix.O_PATH) defer func() { if dstFile != nil && Err != nil { @@ -552,7 +552,7 @@ func (m *mountEntry) createOpenMountpoint(rootfs string) (Err error) { if err != nil { return err } - unsafePath = utils.StripRoot(rootfs, newUnsafePath) + unsafePath = pathrs.LexicallyStripRoot(rootfs, newUnsafePath) if dstIsFile { dstFile, err = pathrs.CreateInRoot(rootfs, unsafePath, unix.O_CREAT|unix.O_EXCL|unix.O_NOFOLLOW, 0o644) @@ -955,7 +955,7 @@ func createDevices(config *configs.Config) error { for _, node := range config.Devices { // The /dev/ptmx device is setup by setupPtmx() - if utils.CleanPath(node.Path) == "/dev/ptmx" { + if pathrs.LexicallyCleanPath(node.Path) == "/dev/ptmx" { continue } @@ -1437,7 +1437,7 @@ func reopenAfterMount(rootfs string, f *os.File, flags int) (_ *os.File, Err err if !pathrs.IsLexicallyInRoot(rootfs, fullPath) { return nil, fmt.Errorf("mountpoint %q is outside of rootfs %q", fullPath, rootfs) } - unsafePath := utils.StripRoot(rootfs, fullPath) + unsafePath := pathrs.LexicallyStripRoot(rootfs, fullPath) reopened, err := pathrs.OpenInRoot(rootfs, unsafePath, flags) if err != nil { return nil, fmt.Errorf("re-open mountpoint %q: %w", unsafePath, err) @@ -1477,7 +1477,7 @@ func (m *mountEntry) mountPropagate(rootfs string, mountLabel string) error { // operations on it. We need to set up files in "/dev", and other tmpfs // mounts may need to be chmod-ed after mounting. These mounts will be // remounted ro later in finalizeRootfs(), if necessary. - if m.Device == "tmpfs" || utils.CleanPath(m.Destination) == "/dev" { + if m.Device == "tmpfs" || pathrs.LexicallyCleanPath(m.Destination) == "/dev" { flags &= ^unix.MS_RDONLY } diff --git a/libcontainer/specconv/spec_linux.go b/libcontainer/specconv/spec_linux.go index 7f410345e..8a3374973 100644 --- a/libcontainer/specconv/spec_linux.go +++ b/libcontainer/specconv/spec_linux.go @@ -14,16 +14,16 @@ import ( systemdDbus "github.com/coreos/go-systemd/v22/dbus" dbus "github.com/godbus/dbus/v5" + "github.com/opencontainers/runtime-spec/specs-go" + "github.com/sirupsen/logrus" + "golang.org/x/sys/unix" + "github.com/opencontainers/cgroups" devices "github.com/opencontainers/cgroups/devices/config" + "github.com/opencontainers/runc/internal/pathrs" "github.com/opencontainers/runc/libcontainer/configs" "github.com/opencontainers/runc/libcontainer/internal/userns" "github.com/opencontainers/runc/libcontainer/seccomp" - libcontainerUtils "github.com/opencontainers/runc/libcontainer/utils" - "github.com/opencontainers/runtime-spec/specs-go" - "github.com/sirupsen/logrus" - - "golang.org/x/sys/unix" ) var ( @@ -746,7 +746,7 @@ func CreateCgroupConfig(opts *CreateOpts, defaultDevs []*devices.Device) (*cgrou if useSystemdCgroup { myCgroupPath = spec.Linux.CgroupsPath } else { - myCgroupPath = libcontainerUtils.CleanPath(spec.Linux.CgroupsPath) + myCgroupPath = pathrs.LexicallyCleanPath(spec.Linux.CgroupsPath) } } diff --git a/libcontainer/utils/utils.go b/libcontainer/utils/utils.go index 17259de98..e6390fad0 100644 --- a/libcontainer/utils/utils.go +++ b/libcontainer/utils/utils.go @@ -3,11 +3,11 @@ package utils import ( "encoding/json" "io" - "os" - "path/filepath" "strings" "golang.org/x/sys/unix" + + "github.com/opencontainers/runc/internal/pathrs" ) const ( @@ -36,53 +36,6 @@ func WriteJSON(w io.Writer, v interface{}) error { return err } -// CleanPath makes a path safe for use with filepath.Join. This is done by not -// only cleaning the path, but also (if the path is relative) adding a leading -// '/' and cleaning it (then removing the leading '/'). This ensures that a -// path resulting from prepending another path will always resolve to lexically -// be a subdirectory of the prefixed path. This is all done lexically, so paths -// that include symlinks won't be safe as a result of using CleanPath. -func CleanPath(path string) string { - // Deal with empty strings nicely. - if path == "" { - return "" - } - - // Ensure that all paths are cleaned (especially problematic ones like - // "/../../../../../" which can cause lots of issues). - - if filepath.IsAbs(path) { - return filepath.Clean(path) - } - - // If the path isn't absolute, we need to do more processing to fix paths - // such as "../../../..//some/path". We also shouldn't convert absolute - // paths to relative ones. - path = filepath.Clean(string(os.PathSeparator) + path) - // This can't fail, as (by definition) all paths are relative to root. - path, _ = filepath.Rel(string(os.PathSeparator), path) - - return path -} - -// StripRoot returns the passed path, stripping the root path if it was -// (lexicially) inside it. Note that both passed paths will always be treated -// as absolute, and the returned path will also always be absolute. In -// addition, the paths are cleaned before stripping the root. -func StripRoot(root, path string) string { - // Make the paths clean and absolute. - root, path = CleanPath("/"+root), CleanPath("/"+path) - switch { - case path == root: - path = "/" - case root == "/": - // do nothing - default: - path = strings.TrimPrefix(path, root+"/") - } - return CleanPath("/" + path) -} - // SearchLabels searches through a list of key=value pairs for a given key, // returning its value, and the binary flag telling whether the key exist. func SearchLabels(labels []string, key string) (string, bool) { @@ -113,3 +66,23 @@ func Annotations(labels []string) (bundle string, userAnnotations map[string]str } return bundle, userAnnotations } + +// CleanPath makes a path safe for use with filepath.Join. This is done by not +// only cleaning the path, but also (if the path is relative) adding a leading +// '/' and cleaning it (then removing the leading '/'). This ensures that a +// path resulting from prepending another path will always resolve to lexically +// be a subdirectory of the prefixed path. This is all done lexically, so paths +// that include symlinks won't be safe as a result of using CleanPath. +// +// Deprecated: This function has been moved to internal/pathrs and this wrapper +// will be removed in runc 1.5. +var CleanPath = pathrs.LexicallyCleanPath + +// StripRoot returns the passed path, stripping the root path if it was +// (lexicially) inside it. Note that both passed paths will always be treated +// as absolute, and the returned path will also always be absolute. In +// addition, the paths are cleaned before stripping the root. +// +// Deprecated: This function has been moved to internal/pathrs and this wrapper +// will be removed in runc 1.5. +var StripRoot = pathrs.LexicallyStripRoot diff --git a/libcontainer/utils/utils_test.go b/libcontainer/utils/utils_test.go index 4b5fd833c..5953118ef 100644 --- a/libcontainer/utils/utils_test.go +++ b/libcontainer/utils/utils_test.go @@ -70,70 +70,3 @@ func TestWriteJSON(t *testing.T) { t.Errorf("expected to write %s but was %s", expected, b.String()) } } - -func TestCleanPath(t *testing.T) { - path := CleanPath("") - if path != "" { - t.Errorf("expected to receive empty string and received %s", path) - } - - path = CleanPath("rootfs") - if path != "rootfs" { - t.Errorf("expected to receive 'rootfs' and received %s", path) - } - - path = CleanPath("../../../var") - if path != "var" { - t.Errorf("expected to receive 'var' and received %s", path) - } - - path = CleanPath("/../../../var") - if path != "/var" { - t.Errorf("expected to receive '/var' and received %s", path) - } - - path = CleanPath("/foo/bar/") - if path != "/foo/bar" { - t.Errorf("expected to receive '/foo/bar' and received %s", path) - } - - path = CleanPath("/foo/bar/../") - if path != "/foo" { - t.Errorf("expected to receive '/foo' and received %s", path) - } -} - -func TestStripRoot(t *testing.T) { - for _, test := range []struct { - root, path, out string - }{ - // Works with multiple components. - {"/a/b", "/a/b/c", "/c"}, - {"/hello/world", "/hello/world/the/quick-brown/fox", "/the/quick-brown/fox"}, - // '/' must be a no-op. - {"/", "/a/b/c", "/a/b/c"}, - // Must be the correct order. - {"/a/b", "/a/c/b", "/a/c/b"}, - // Must be at start. - {"/abc/def", "/foo/abc/def/bar", "/foo/abc/def/bar"}, - // Must be a lexical parent. - {"/foo/bar", "/foo/barSAMECOMPONENT", "/foo/barSAMECOMPONENT"}, - // Must only strip the root once. - {"/foo/bar", "/foo/bar/foo/bar/baz", "/foo/bar/baz"}, - // Deal with .. in a fairly sane way. - {"/foo/bar", "/foo/bar/../baz", "/foo/baz"}, - {"/foo/bar", "../../../../../../foo/bar/baz", "/baz"}, - {"/foo/bar", "/../../../../../../foo/bar/baz", "/baz"}, - {"/foo/bar/../baz", "/foo/baz/bar", "/bar"}, - {"/foo/bar/../baz", "/foo/baz/../bar/../baz/./foo", "/foo"}, - // All paths are made absolute before stripping. - {"foo/bar", "/foo/bar/baz/bee", "/baz/bee"}, - {"/foo/bar", "foo/bar/baz/beef", "/baz/beef"}, - {"foo/bar", "foo/bar/baz/beets", "/baz/beets"}, - } { - got := StripRoot(test.root, test.path) - if got != test.out { - t.Errorf("StripRoot(%q, %q) -- got %q, expected %q", test.root, test.path, got, test.out) - } - } -} diff --git a/libcontainer/utils/utils_unix.go b/libcontainer/utils/utils_unix.go index 7dbec54dc..0d5c4e628 100644 --- a/libcontainer/utils/utils_unix.go +++ b/libcontainer/utils/utils_unix.go @@ -13,9 +13,10 @@ import ( _ "unsafe" // for go:linkname securejoin "github.com/cyphar/filepath-securejoin" - "github.com/opencontainers/runc/internal/pathrs" "github.com/sirupsen/logrus" "golang.org/x/sys/unix" + + "github.com/opencontainers/runc/internal/pathrs" ) var ( @@ -152,7 +153,7 @@ func NewSockPair(name string) (parent, child *os.File, err error) { // the passed closure (the file handle will be freed once the closure returns). func WithProcfd(root, unsafePath string, fn func(procfd string) error) error { // Remove the root then forcefully resolve inside the root. - unsafePath = StripRoot(root, unsafePath) + unsafePath = pathrs.LexicallyStripRoot(root, unsafePath) fullPath, err := securejoin.SecureJoin(root, unsafePath) if err != nil { return fmt.Errorf("resolving path inside rootfs failed: %w", err) From ec44abc7102238eb9d5e7e1d6803f72e6951e7d0 Mon Sep 17 00:00:00 2001 From: Aleksa Sarai Date: Sun, 9 Nov 2025 02:03:42 +1100 Subject: [PATCH 02/13] libct: switch final WithProcfd users to WithProcfdFile This probably should've been done as part of commit d40b3439a961 ("rootfs: switch to fd-based handling of mountpoint targets") but it seems I missed them when doing the rest of the conversions. This also lets us remove utils.WithProcfd entirely, as well as pathrs.MkdirAllInRoot. Unfortunately, WithProcfd was exposed in the externally-importable "libcontainer/utils" package and so we need to have a deprecation notice to remove it in runc 1.5. Signed-off-by: Aleksa Sarai (cherry picked from commit 9dbd37e06f8124be6e496308e1735d18f27bb730) Signed-off-by: Aleksa Sarai --- internal/pathrs/mkdirall_pathrslite.go | 10 ---------- libcontainer/criu_linux.go | 15 +++++++++++---- libcontainer/rootfs_linux.go | 11 ++++++----- libcontainer/utils/utils_unix.go | 12 ++++++++++-- 4 files changed, 27 insertions(+), 21 deletions(-) diff --git a/internal/pathrs/mkdirall_pathrslite.go b/internal/pathrs/mkdirall_pathrslite.go index a9a0157c6..3fd476fbf 100644 --- a/internal/pathrs/mkdirall_pathrslite.go +++ b/internal/pathrs/mkdirall_pathrslite.go @@ -87,13 +87,3 @@ func MkdirAllInRootOpen(root, unsafePath string, mode os.FileMode) (*os.File, er return pathrs.MkdirAllHandle(rootDir, unsafePath, mode) }) } - -// MkdirAllInRoot is a wrapper around MkdirAllInRootOpen which closes the -// returned handle, for callers that don't need to use it. -func MkdirAllInRoot(root, unsafePath string, mode os.FileMode) error { - f, err := MkdirAllInRootOpen(root, unsafePath, mode) - if err == nil { - _ = f.Close() - } - return err -} diff --git a/libcontainer/criu_linux.go b/libcontainer/criu_linux.go index 53a0202ad..94e951ba7 100644 --- a/libcontainer/criu_linux.go +++ b/libcontainer/criu_linux.go @@ -24,6 +24,7 @@ import ( "google.golang.org/protobuf/proto" "github.com/opencontainers/cgroups" + "github.com/opencontainers/runc/internal/pathrs" "github.com/opencontainers/runc/libcontainer/configs" "github.com/opencontainers/runc/libcontainer/utils" ) @@ -542,16 +543,22 @@ func (c *Container) prepareCriuRestoreMounts(mounts []*configs.Mount) error { umounts := []string{} defer func() { for _, u := range umounts { - _ = utils.WithProcfd(c.config.Rootfs, u, func(procfd string) error { - if e := unix.Unmount(procfd, unix.MNT_DETACH); e != nil { - if e != unix.EINVAL { + mntFile, err := pathrs.OpenInRoot(c.config.Rootfs, u, unix.O_PATH) + if err != nil { + logrus.Warnf("Error during cleanup unmounting %s: open handle: %v", u, err) + continue + } + _ = utils.WithProcfdFile(mntFile, func(procfd string) error { + if err := unix.Unmount(procfd, unix.MNT_DETACH); err != nil { + if err != unix.EINVAL { // Ignore EINVAL as it means 'target is not a mount point.' // It probably has already been unmounted. - logrus.Warnf("Error during cleanup unmounting of %s (%s): %v", procfd, u, e) + logrus.Warnf("Error during cleanup unmounting of %s (%s): %v", procfd, u, err) } } return nil }) + _ = mntFile.Close() } }() // Now go through all mounts and create the required mountpoints. diff --git a/libcontainer/rootfs_linux.go b/libcontainer/rootfs_linux.go index b9e3a04c4..1e7c78043 100644 --- a/libcontainer/rootfs_linux.go +++ b/libcontainer/rootfs_linux.go @@ -328,12 +328,15 @@ func mountCgroupV1(m mountEntry, c *mountConfig) error { // We just created the tmpfs, and so we can just use filepath.Join // here (not to mention we want to make sure we create the path // inside the tmpfs, so we don't want to resolve symlinks). + // TODO: Why not just use b.Destination (c.root is the root here)? subsystemPath := filepath.Join(c.root, b.Destination) subsystemName := filepath.Base(b.Destination) - if err := pathrs.MkdirAllInRoot(c.root, subsystemPath, 0o755); err != nil { + subsystemDir, err := pathrs.MkdirAllInRootOpen(c.root, subsystemPath, 0o755) + if err != nil { return err } - if err := utils.WithProcfd(c.root, b.Destination, func(dstFd string) error { + defer subsystemDir.Close() + if err := utils.WithProcfdFile(subsystemDir, func(dstFd string) error { flags := defaultMountFlags if m.Flags&unix.MS_RDONLY != 0 { flags = flags | unix.MS_RDONLY @@ -1501,9 +1504,7 @@ func (m *mountEntry) mountPropagate(rootfs string, mountLabel string) error { _ = m.dstFile.Close() m.dstFile = newDstFile - // We have to apply mount propagation flags in a separate WithProcfd() call - // because the previous call invalidates the passed procfd -- the mount - // target needs to be re-opened. + // Apply the propagation flags on the new mount. if err := utils.WithProcfdFile(m.dstFile, func(dstFd string) error { for _, pflag := range m.PropagationFlags { if err := mountViaFds("", nil, m.Destination, dstFd, "", uintptr(pflag), ""); err != nil { diff --git a/libcontainer/utils/utils_unix.go b/libcontainer/utils/utils_unix.go index 0d5c4e628..cda0a8841 100644 --- a/libcontainer/utils/utils_unix.go +++ b/libcontainer/utils/utils_unix.go @@ -151,6 +151,9 @@ func NewSockPair(name string) (parent, child *os.File, err error) { // through the passed fdpath should be safe. Do not access this path through // the original path strings, and do not attempt to use the pathname outside of // the passed closure (the file handle will be freed once the closure returns). +// +// Deprecated: This function is an internal implementation detail of runc and +// is no longer used. It will be removed in runc 1.5. func WithProcfd(root, unsafePath string, fn func(procfd string) error) error { // Remove the root then forcefully resolve inside the root. unsafePath = pathrs.LexicallyStripRoot(root, unsafePath) @@ -180,10 +183,15 @@ func WithProcfd(root, unsafePath string, fn func(procfd string) error) error { return fn(procfd) } -// WithProcfdFile is a very minimal wrapper around [ProcThreadSelfFd], intended -// to make migrating from [WithProcfd] and [WithProcfdPath] usage easier. The +// WithProcfdFile is a very minimal wrapper around [ProcThreadSelfFd]. The // caller is responsible for making sure that the provided file handle is // actually safe to operate on. +// +// NOTE: THIS FUNCTION IS INTERNAL TO RUNC, DO NOT USE IT. +// +// TODO: Migrate the mount logic towards a more move_mount(2)-friendly design +// where this is kind of /proc/self/... tomfoolery is only done in a fallback +// path for old kernels. func WithProcfdFile(file *os.File, fn func(procfd string) error) error { fdpath, closer := ProcThreadSelfFd(file.Fd()) defer closer() From b60f248d7148ba547e91e6a167f8d58a93948cae Mon Sep 17 00:00:00 2001 From: Aleksa Sarai Date: Sun, 9 Nov 2025 02:26:57 +1100 Subject: [PATCH 03/13] pathrs: rename MkdirAllInRootOpen -> MkdirAllInRoot Now that MkdirAllInRoot has been removed, we can make MkdirAllInRootOpen less wordy by renaming it to MkdirAllInRoot. This is a non-functional change. Signed-off-by: Aleksa Sarai (cherry picked from commit 20c5a8ec4a16b4ad8d10f3b9748f5ba66698bb18) Signed-off-by: Aleksa Sarai --- internal/pathrs/mkdirall_pathrslite.go | 6 +++--- internal/pathrs/root_pathrslite.go | 2 +- libcontainer/rootfs_linux.go | 8 ++++---- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/internal/pathrs/mkdirall_pathrslite.go b/internal/pathrs/mkdirall_pathrslite.go index 3fd476fbf..0661273b1 100644 --- a/internal/pathrs/mkdirall_pathrslite.go +++ b/internal/pathrs/mkdirall_pathrslite.go @@ -28,7 +28,7 @@ import ( "golang.org/x/sys/unix" ) -// MkdirAllInRootOpen attempts to make +// MkdirAllInRoot attempts to make // // path, _ := securejoin.SecureJoin(root, unsafePath) // os.MkdirAll(path, mode) @@ -49,10 +49,10 @@ import ( // handling if unsafePath has already been scoped within the rootfs (this is // needed for a lot of runc callers and fixing this would require reworking a // lot of path logic). -func MkdirAllInRootOpen(root, unsafePath string, mode os.FileMode) (*os.File, error) { +func MkdirAllInRoot(root, unsafePath string, mode os.FileMode) (*os.File, error) { // If the path is already "within" the root, get the path relative to the // root and use that as the unsafe path. This is necessary because a lot of - // MkdirAllInRootOpen callers have already done SecureJoin, and refactoring + // MkdirAllInRoot callers have already done SecureJoin, and refactoring // all of them to stop using these SecureJoin'd paths would require a fair // amount of work. // TODO(cyphar): Do the refactor to libpathrs once it's ready. diff --git a/internal/pathrs/root_pathrslite.go b/internal/pathrs/root_pathrslite.go index 899af2703..e90ac3114 100644 --- a/internal/pathrs/root_pathrslite.go +++ b/internal/pathrs/root_pathrslite.go @@ -53,7 +53,7 @@ func CreateInRoot(root, subpath string, flags int, fileMode uint32) (*os.File, e return nil, fmt.Errorf("create in root subpath %q has bad trailing component %q", subpath, filename) } - dirFd, err := MkdirAllInRootOpen(root, dir, 0o755) + dirFd, err := MkdirAllInRoot(root, dir, 0o755) if err != nil { return nil, err } diff --git a/libcontainer/rootfs_linux.go b/libcontainer/rootfs_linux.go index 1e7c78043..deb209c18 100644 --- a/libcontainer/rootfs_linux.go +++ b/libcontainer/rootfs_linux.go @@ -331,7 +331,7 @@ func mountCgroupV1(m mountEntry, c *mountConfig) error { // TODO: Why not just use b.Destination (c.root is the root here)? subsystemPath := filepath.Join(c.root, b.Destination) subsystemName := filepath.Base(b.Destination) - subsystemDir, err := pathrs.MkdirAllInRootOpen(c.root, subsystemPath, 0o755) + subsystemDir, err := pathrs.MkdirAllInRoot(c.root, subsystemPath, 0o755) if err != nil { return err } @@ -560,7 +560,7 @@ func (m *mountEntry) createOpenMountpoint(rootfs string) (Err error) { if dstIsFile { dstFile, err = pathrs.CreateInRoot(rootfs, unsafePath, unix.O_CREAT|unix.O_EXCL|unix.O_NOFOLLOW, 0o644) } else { - dstFile, err = pathrs.MkdirAllInRootOpen(rootfs, unsafePath, 0o755) + dstFile, err = pathrs.MkdirAllInRoot(rootfs, unsafePath, 0o755) } if err != nil { return fmt.Errorf("make mountpoint %q: %w", m.Destination, err) @@ -622,7 +622,7 @@ func mountToRootfs(c *mountConfig, m mountEntry) error { } else if !fi.IsDir() { return fmt.Errorf("filesystem %q must be mounted on ordinary directory", m.Device) } - dstFile, err := pathrs.MkdirAllInRootOpen(rootfs, dest, 0o755) + dstFile, err := pathrs.MkdirAllInRoot(rootfs, dest, 0o755) if err != nil { return err } @@ -997,7 +997,7 @@ func createDeviceNode(rootfs string, node *devices.Device, bind bool) error { return fmt.Errorf("%w: mknod over rootfs", errRootfsToFile) } destDirPath, destName := filepath.Split(destPath) - destDir, err := pathrs.MkdirAllInRootOpen(rootfs, destDirPath, 0o755) + destDir, err := pathrs.MkdirAllInRoot(rootfs, destDirPath, 0o755) if err != nil { return fmt.Errorf("mkdir parent of device inode %q: %w", node.Path, err) } From d0ff22b5746a95909e4dee02fa7ba26acc6f9a47 Mon Sep 17 00:00:00 2001 From: Aleksa Sarai Date: Sun, 9 Nov 2025 01:41:10 +1100 Subject: [PATCH 04/13] pathrs: add "hallucination" helpers for SecureJoin magic In order to maintain compatibility with previous releases of runc (which permitted dangling symlinks as path components by permitting non-existent path components to be treated like real directories) we have to first do SecureJoin to construct a target path that is compatible with the old behaviour but has all dangling symlinks (or other invalid paths like ".." components after non-existent directories) removed. This is effectively a more generic verison of commit 3f925525b44d ("rootfs: re-allow dangling symlinks in mount targets") and will let us remove the need for open-coding SecureJoin workarounds. Signed-off-by: Aleksa Sarai (cherry picked from commit cfb74326be5865b13bd91c4374b2fb6a70838c59) Signed-off-by: Aleksa Sarai --- internal/pathrs/mkdirall_pathrslite.go | 16 +++---------- internal/pathrs/path.go | 32 ++++++++++++++++++++++++++ internal/pathrs/root_pathrslite.go | 5 ++++ libcontainer/rootfs_linux.go | 12 ---------- 4 files changed, 40 insertions(+), 25 deletions(-) diff --git a/internal/pathrs/mkdirall_pathrslite.go b/internal/pathrs/mkdirall_pathrslite.go index 0661273b1..c2578e051 100644 --- a/internal/pathrs/mkdirall_pathrslite.go +++ b/internal/pathrs/mkdirall_pathrslite.go @@ -21,7 +21,6 @@ package pathrs import ( "fmt" "os" - "path/filepath" "github.com/cyphar/filepath-securejoin/pathrs-lite" "github.com/sirupsen/logrus" @@ -50,18 +49,9 @@ import ( // needed for a lot of runc callers and fixing this would require reworking a // lot of path logic). func MkdirAllInRoot(root, unsafePath string, mode os.FileMode) (*os.File, error) { - // If the path is already "within" the root, get the path relative to the - // root and use that as the unsafe path. This is necessary because a lot of - // MkdirAllInRoot callers have already done SecureJoin, and refactoring - // all of them to stop using these SecureJoin'd paths would require a fair - // amount of work. - // TODO(cyphar): Do the refactor to libpathrs once it's ready. - if IsLexicallyInRoot(root, unsafePath) { - subPath, err := filepath.Rel(root, unsafePath) - if err != nil { - return nil, err - } - unsafePath = subPath + unsafePath, err := hallucinateUnsafePath(root, unsafePath) + if err != nil { + return nil, fmt.Errorf("failed to construct hallucinated target path: %w", err) } // Check for any silly mode bits. diff --git a/internal/pathrs/path.go b/internal/pathrs/path.go index 709c3ae3f..77be98924 100644 --- a/internal/pathrs/path.go +++ b/internal/pathrs/path.go @@ -22,6 +22,8 @@ import ( "os" "path/filepath" "strings" + + securejoin "github.com/cyphar/filepath-securejoin" ) // IsLexicallyInRoot is shorthand for strings.HasPrefix(path+"/", root+"/"), @@ -82,3 +84,33 @@ func LexicallyStripRoot(root, path string) string { } return LexicallyCleanPath("/" + path) } + +// hallucinateUnsafePath creates a new unsafePath which has all symlinks +// (including dangling symlinks) fully resolved and any non-existent components +// treated as though they are real. This is effectively just a wrapper around +// [securejoin.SecureJoin] that strips the root. This path *IS NOT* safe to use +// as-is, you *MUST* operate on the returned path with pathrs-lite. +// +// The reason for this methods is that in previous runc versions, we would +// tolerate nonsense paths with dangling symlinks as path components. +// pathrs-lite does not support this, so instead we have to emulate this +// behaviour by doing SecureJoin *purely to get a semi-reasonable path to use* +// and then we use pathrs-lite to operate on the path safely. +// +// It would be quite difficult to emulate this in a race-free way in +// pathrs-lite, so instead we use [securejoin.SecureJoin] to simply produce a +// new candidate path for operations like [MkdirAllInRoot] so they can then +// operate on the new unsafePath as if it was what the user requested. +// +// If unsafePath is already lexically inside root, it is stripped before +// re-resolving it (this is done to ensure compatibility with legacy callers +// within runc that call SecureJoin before calling into pathrs). +func hallucinateUnsafePath(root, unsafePath string) (string, error) { + unsafePath = LexicallyStripRoot(root, unsafePath) + weirdPath, err := securejoin.SecureJoin(root, unsafePath) + if err != nil { + return "", err + } + unsafePath = LexicallyStripRoot(root, weirdPath) + return unsafePath, nil +} diff --git a/internal/pathrs/root_pathrslite.go b/internal/pathrs/root_pathrslite.go index e90ac3114..051f9236b 100644 --- a/internal/pathrs/root_pathrslite.go +++ b/internal/pathrs/root_pathrslite.go @@ -48,6 +48,11 @@ func OpenInRoot(root, subpath string, flags int) (*os.File, error) { // include it in the passed flags. The fileMode argument uses unix.* mode bits, // *not* os.FileMode. func CreateInRoot(root, subpath string, flags int, fileMode uint32) (*os.File, error) { + subpath, err := hallucinateUnsafePath(root, subpath) + if err != nil { + return nil, fmt.Errorf("failed to construct hallucinated target path: %w", err) + } + dir, filename := filepath.Split(subpath) if filepath.Join("/", filename) == "/" { return nil, fmt.Errorf("create in root subpath %q has bad trailing component %q", subpath, filename) diff --git a/libcontainer/rootfs_linux.go b/libcontainer/rootfs_linux.go index deb209c18..e05a0dc53 100644 --- a/libcontainer/rootfs_linux.go +++ b/libcontainer/rootfs_linux.go @@ -545,18 +545,6 @@ func (m *mountEntry) createOpenMountpoint(rootfs string) (Err error) { } dstIsFile = !fi.IsDir() } - - // In previous runc versions, we would tolerate nonsense paths with - // dangling symlinks as path components. pathrs-lite does not support - // this, so instead we have to emulate this behaviour by doing - // SecureJoin *purely to get a semi-reasonable path to use* and then we - // use pathrs-lite to operate on the path safely. - newUnsafePath, err := securejoin.SecureJoin(rootfs, unsafePath) - if err != nil { - return err - } - unsafePath = pathrs.LexicallyStripRoot(rootfs, newUnsafePath) - if dstIsFile { dstFile, err = pathrs.CreateInRoot(rootfs, unsafePath, unix.O_CREAT|unix.O_EXCL|unix.O_NOFOLLOW, 0o644) } else { From fd76e504ed3912a90dda52bc115f01a33804da59 Mon Sep 17 00:00:00 2001 From: Aleksa Sarai Date: Sun, 9 Nov 2025 04:36:49 +1100 Subject: [PATCH 05/13] pathrs: add MkdirAllParentInRoot helper While CreateInRoot supports hallucinating the target path, we do not use it directly when constructing device inode targets because we need to have different handling for mknod and bind-mounts. The solution is to simply have a more generic MkdirAllParentInRoot helper that MkdirAll's the parent directory of the target path and then allows the caller to create the trailing component however they like. (This can be used by CreateInRoot internally as well!) Signed-off-by: Aleksa Sarai (cherry picked from commit 195e9551e4200b9f6c59d8ee51ccd1fc92ae137e) Signed-off-by: Aleksa Sarai --- internal/pathrs/mkdirall.go | 51 ++++++++++++++++++++++++++++++ internal/pathrs/root_pathrslite.go | 14 +------- libcontainer/rootfs_linux.go | 13 +------- 3 files changed, 53 insertions(+), 25 deletions(-) create mode 100644 internal/pathrs/mkdirall.go diff --git a/internal/pathrs/mkdirall.go b/internal/pathrs/mkdirall.go new file mode 100644 index 000000000..3a896f484 --- /dev/null +++ b/internal/pathrs/mkdirall.go @@ -0,0 +1,51 @@ +// SPDX-License-Identifier: Apache-2.0 +/* + * Copyright (C) 2024-2025 Aleksa Sarai + * Copyright (C) 2024-2025 SUSE LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package pathrs + +import ( + "fmt" + "os" + "path/filepath" +) + +// 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. +// +// Callers need to be very careful operating on the trailing path, as trivial +// mistakes like following symlinks can cause security bugs. Most people +// should probably just use [MkdirAllInRoot] or [CreateInRoot]. +func MkdirAllParentInRoot(root, unsafePath string, mode os.FileMode) (*os.File, string, error) { + // MkdirAllInRoot also does hallucinateUnsafePath, but we need to do it + // here first because when we split unsafePath into (dir, file) components + // we want to be doing so with the hallucinated path (so that trailing + // dangling symlinks are treated correctly). + unsafePath, err := hallucinateUnsafePath(root, unsafePath) + if err != nil { + 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) + } + + dirFd, err := MkdirAllInRoot(root, dirPath, mode) + return dirFd, filename, err +} diff --git a/internal/pathrs/root_pathrslite.go b/internal/pathrs/root_pathrslite.go index 051f9236b..aac73877e 100644 --- a/internal/pathrs/root_pathrslite.go +++ b/internal/pathrs/root_pathrslite.go @@ -19,9 +19,7 @@ package pathrs import ( - "fmt" "os" - "path/filepath" "github.com/cyphar/filepath-securejoin/pathrs-lite" "golang.org/x/sys/unix" @@ -48,17 +46,7 @@ func OpenInRoot(root, subpath string, flags int) (*os.File, error) { // include it in the passed flags. The fileMode argument uses unix.* mode bits, // *not* os.FileMode. func CreateInRoot(root, subpath string, flags int, fileMode uint32) (*os.File, error) { - subpath, err := hallucinateUnsafePath(root, subpath) - if err != nil { - return nil, fmt.Errorf("failed to construct hallucinated target path: %w", err) - } - - dir, filename := filepath.Split(subpath) - if filepath.Join("/", filename) == "/" { - return nil, fmt.Errorf("create in root subpath %q has bad trailing component %q", subpath, filename) - } - - dirFd, err := MkdirAllInRoot(root, dir, 0o755) + dirFd, filename, err := MkdirAllParentInRoot(root, subpath, 0o755) if err != nil { return nil, err } diff --git a/libcontainer/rootfs_linux.go b/libcontainer/rootfs_linux.go index e05a0dc53..5690bd311 100644 --- a/libcontainer/rootfs_linux.go +++ b/libcontainer/rootfs_linux.go @@ -12,7 +12,6 @@ import ( "syscall" "time" - securejoin "github.com/cyphar/filepath-securejoin" "github.com/cyphar/filepath-securejoin/pathrs-lite/procfs" "github.com/moby/sys/mountinfo" "github.com/moby/sys/userns" @@ -505,8 +504,6 @@ func statfsToMountFlags(st unix.Statfs_t) int { return flags } -var errRootfsToFile = errors.New("config tries to change rootfs to file") - func (m *mountEntry) createOpenMountpoint(rootfs string) (Err error) { unsafePath := pathrs.LexicallyStripRoot(rootfs, m.Destination) dstFile, err := pathrs.OpenInRoot(rootfs, unsafePath, unix.O_PATH) @@ -977,15 +974,7 @@ func createDeviceNode(rootfs string, node *devices.Device, bind bool) error { // The node only exists for cgroup reasons, ignore it here. return nil } - destPath, err := securejoin.SecureJoin(rootfs, node.Path) - if err != nil { - return err - } - if destPath == rootfs { - return fmt.Errorf("%w: mknod over rootfs", errRootfsToFile) - } - destDirPath, destName := filepath.Split(destPath) - destDir, err := pathrs.MkdirAllInRoot(rootfs, destDirPath, 0o755) + destDir, destName, err := pathrs.MkdirAllParentInRoot(rootfs, node.Path, 0o755) if err != nil { return fmt.Errorf("mkdir parent of device inode %q: %w", node.Path, err) } From 3661a9d4b36568db23784275c115ec0db8af5e2e Mon Sep 17 00:00:00 2001 From: lifubang Date: Mon, 17 Nov 2025 15:23:23 +0000 Subject: [PATCH 06/13] integration: add some tests for bind mount through dangling symlinks We intentionally broke this in commit d40b3439a961 ("rootfs: switch to fd-based handling of mountpoint targets") under the assumption that most users do not need this feature. Sadly it turns out they do, and so commit 3f925525b44d ("rootfs: re-allow dangling symlinks in mount targets") added a hotfix to re-add this functionality. This patch adds some much-needed tests for this behaviour, since it seems we are going to need to keep this for compatibility reasons (at least until runc v2...). Co-developed-by: lifubang Signed-off-by: Aleksa Sarai (cherry picked from commit 15d7c214cde94884c3dc875974d7cbc231a726f0) Signed-off-by: Aleksa Sarai --- tests/integration/mounts.bats | 58 +++++++++++++++++++++++++++++++++++ 1 file changed, 58 insertions(+) diff --git a/tests/integration/mounts.bats b/tests/integration/mounts.bats index b60c88ae2..6bab15727 100644 --- a/tests/integration/mounts.bats +++ b/tests/integration/mounts.bats @@ -127,6 +127,42 @@ function test_mount_order() { [[ "$output" == *"a/x"* ]] # the final "file" was from a/x. } +# This needs to be placed at the top of the bats file to work around +# a shellcheck bug. See . +test_mount_target() { + src="$1" + dst="$2" + real_dst="${3:-$dst}" + + echo "== $src -> $dst (=> $real_dst) ==" + + old_config="$(mktemp ./config.json.bak.XXXXXX)" + cp ./config.json "$old_config" + + update_config '.mounts += [{ + source: "'"$src"'", + destination: "'"$dst"'", + options: ["bind"] + }]' + + # Make sure the target path is at the right spot and is actually a + # bind-mount of the correct inode. + update_config '.process.args = ["stat", "-c", "%n %d:%i", "--", "'"$real_dst"'"]' + runc run test_busybox + [ "$status" -eq 0 ] + [[ "$output" == "$real_dst $(stat -c "%d:%i" -- "$src")" ]] + + # Make sure there is a mount entry for the target path. + # shellcheck disable=SC2016 + update_config '.process.args = ["awk", "-F", "PATH='"$real_dst"'", "$2 == PATH", "/proc/self/mounts"]' + runc run test_busybox + [ "$status" -eq 0 ] + [[ "$output" == *"$real_dst"* ]] + + # Switch back the old config so this function can be called multiple times. + mv "$old_config" "./config.json" +} + # https://github.com/opencontainers/runc/issues/3991 @test "runc run [tmpcopyup]" { mkdir -p rootfs/dir1/dir2 @@ -343,3 +379,25 @@ function test_mount_order() { @test "runc run [mount order, container idmap source] (userns)" { test_mount_order userns,idmap } + +@test "runc run [bind mount through a dangling symlink component]" { + rm -rf rootfs/etc/hosts + ln -s /tmp/foo/bar rootfs/jump + ln -s /jump/baz/hosts rootfs/etc/hosts + + rm -rf rootfs/tmp/foo + test_mount_target ./config.json /etc/hosts /tmp/foo/bar/baz/hosts +} + +@test "runc run [bind mount through a trailing dangling symlink]" { + rm -rf rootfs/etc/hosts + ln -s /tmp/hosts rootfs/etc/hosts + + # File. + rm -rf rootfs/tmp/hosts + test_mount_target ./config.json /etc/hosts /tmp/hosts + + # Directory. + rm -rf rootfs/tmp/hosts + test_mount_target . /etc/hosts /tmp/hosts +} From edf5328b9b91190e763c64c900fe441b8df17ea0 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Wed, 18 Mar 2026 17:17:27 -0700 Subject: [PATCH 07/13] libct: mountCgroupV1: address TODO Indeed, it does not make sense to prepend c.root once we started using MkdirAllInRoot in commit 63c29081. Signed-off-by: Kir Kolyshkin (cherry picked from commit 60352524d346721b6a8640772e9b6e23e01a6ffa) Signed-off-by: Aleksa Sarai --- libcontainer/rootfs_linux.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/libcontainer/rootfs_linux.go b/libcontainer/rootfs_linux.go index 5690bd311..574faed99 100644 --- a/libcontainer/rootfs_linux.go +++ b/libcontainer/rootfs_linux.go @@ -327,10 +327,8 @@ func mountCgroupV1(m mountEntry, c *mountConfig) error { // We just created the tmpfs, and so we can just use filepath.Join // here (not to mention we want to make sure we create the path // inside the tmpfs, so we don't want to resolve symlinks). - // TODO: Why not just use b.Destination (c.root is the root here)? - subsystemPath := filepath.Join(c.root, b.Destination) subsystemName := filepath.Base(b.Destination) - subsystemDir, err := pathrs.MkdirAllInRoot(c.root, subsystemPath, 0o755) + subsystemDir, err := pathrs.MkdirAllInRoot(c.root, b.Destination, 0o755) if err != nil { return err } From 2e9b6a8a16767f09feb341918669bf2a16758e68 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Fri, 20 Mar 2026 14:29:23 -0700 Subject: [PATCH 08/13] libct: minor refactor in mountToRootfs No change in functionality, just a preparation for the next patch. Signed-off-by: Kir Kolyshkin (cherry picked from commit 78b80677f6713372207a2d65b0bd22d17f432f22) Signed-off-by: Aleksa Sarai --- libcontainer/rootfs_linux.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/libcontainer/rootfs_linux.go b/libcontainer/rootfs_linux.go index 574faed99..45d510892 100644 --- a/libcontainer/rootfs_linux.go +++ b/libcontainer/rootfs_linux.go @@ -573,7 +573,6 @@ func (m *mountEntry) createOpenMountpoint(rootfs string) (Err error) { } func mountToRootfs(c *mountConfig, m mountEntry) error { - rootfs := c.root defer func() { if m.dstFile != nil { _ = m.dstFile.Close() @@ -590,6 +589,7 @@ func mountToRootfs(c *mountConfig, m mountEntry) error { // has been a "fun" attack scenario in the past. // TODO: This won't be necessary once we switch to libpathrs and we can // stop all of these symlink-exchange attacks. + rootfs := c.root dest := filepath.Clean(m.Destination) if !pathrs.IsLexicallyInRoot(rootfs, dest) { // Do not use securejoin as it resolves symlinks. @@ -605,7 +605,7 @@ func mountToRootfs(c *mountConfig, m mountEntry) error { } else if !fi.IsDir() { return fmt.Errorf("filesystem %q must be mounted on ordinary directory", m.Device) } - dstFile, err := pathrs.MkdirAllInRoot(rootfs, dest, 0o755) + dstFile, err := pathrs.MkdirAllInRoot(c.root, dest, 0o755) if err != nil { return err } @@ -613,17 +613,17 @@ func mountToRootfs(c *mountConfig, m mountEntry) error { // "proc" and "sys" mounts need special handling (without resolving the // destination) to avoid attacks. m.dstFile = dstFile - return m.mountPropagate(rootfs, "") + return m.mountPropagate(c.root, "") } mountLabel := c.label - if err := m.createOpenMountpoint(rootfs); err != nil { + if err := m.createOpenMountpoint(c.root); err != nil { return fmt.Errorf("create mountpoint for %s mount: %w", m.Destination, err) } switch m.Device { case "mqueue": - if err := m.mountPropagate(rootfs, ""); err != nil { + if err := m.mountPropagate(c.root, ""); err != nil { return err } return utils.WithProcfdFile(m.dstFile, func(dstFd string) error { @@ -634,12 +634,12 @@ func mountToRootfs(c *mountConfig, m mountEntry) error { if m.Extensions&configs.EXT_COPYUP == configs.EXT_COPYUP { err = doTmpfsCopyUp(m, mountLabel) } else { - err = m.mountPropagate(rootfs, mountLabel) + err = m.mountPropagate(c.root, mountLabel) } return err case "bind": // open_tree()-related shenanigans are all handled in mountViaFds. - if err := m.mountPropagate(rootfs, mountLabel); err != nil { + if err := m.mountPropagate(c.root, mountLabel); err != nil { return err } @@ -761,7 +761,7 @@ func mountToRootfs(c *mountConfig, m mountEntry) error { } return mountCgroupV1(m, c) default: - return m.mountPropagate(rootfs, mountLabel) + return m.mountPropagate(c.root, mountLabel) } } From 42cfcbe4533a094cbffd23d17dc355bbb25938a1 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Wed, 18 Mar 2026 17:21:55 -0700 Subject: [PATCH 09/13] Pre-open container root directory A lot of filesystem-related stuff happens inside the container root directory, and we have used its name before. It makes sense to pre-open it and use a *os.File handle instead. Function names in internal/pathrs are kept as is for simplicity (and it is an internal package), but they now accept root as *os.File. Signed-off-by: Kir Kolyshkin (cherry picked from commit 28cb3218873f7b073fba69c74663c9c1ffdfbe6b) It was also necessary to cherry-pick a missing hunk from commit b88635e57ec5 ("libct: close rootFd ASAP in maskPaths") that was not backported to 1.3 in the backported patch commit 9a2fec0a9a26 ("libct: close rootFd ASAP in maskPaths"). Signed-off-by: Aleksa Sarai --- internal/pathrs/mkdirall.go | 4 +- internal/pathrs/mkdirall_pathrslite.go | 15 ++----- internal/pathrs/root_pathrslite.go | 10 ++--- libcontainer/criu_linux.go | 10 ++++- libcontainer/rootfs_linux.go | 62 ++++++++++++++++++-------- 5 files changed, 62 insertions(+), 39 deletions(-) diff --git a/internal/pathrs/mkdirall.go b/internal/pathrs/mkdirall.go index 3a896f484..81c9a022c 100644 --- a/internal/pathrs/mkdirall.go +++ b/internal/pathrs/mkdirall.go @@ -31,12 +31,12 @@ import ( // Callers need to be very careful operating on the trailing path, as trivial // mistakes like following symlinks can cause security bugs. Most people // should probably just use [MkdirAllInRoot] or [CreateInRoot]. -func MkdirAllParentInRoot(root, unsafePath string, mode os.FileMode) (*os.File, string, error) { +func MkdirAllParentInRoot(root *os.File, unsafePath string, mode os.FileMode) (*os.File, string, error) { // MkdirAllInRoot also does hallucinateUnsafePath, but we need to do it // here first because when we split unsafePath into (dir, file) components // we want to be doing so with the hallucinated path (so that trailing // dangling symlinks are treated correctly). - unsafePath, err := hallucinateUnsafePath(root, unsafePath) + unsafePath, err := hallucinateUnsafePath(root.Name(), unsafePath) if err != nil { return nil, "", fmt.Errorf("failed to construct hallucinated target path: %w", err) } diff --git a/internal/pathrs/mkdirall_pathrslite.go b/internal/pathrs/mkdirall_pathrslite.go index c2578e051..0a6f25ebd 100644 --- a/internal/pathrs/mkdirall_pathrslite.go +++ b/internal/pathrs/mkdirall_pathrslite.go @@ -24,12 +24,11 @@ import ( "github.com/cyphar/filepath-securejoin/pathrs-lite" "github.com/sirupsen/logrus" - "golang.org/x/sys/unix" ) // MkdirAllInRoot attempts to make // -// path, _ := securejoin.SecureJoin(root, unsafePath) +// path, _ := securejoin.SecureJoin(root.Name(), unsafePath) // os.MkdirAll(path, mode) // os.Open(path) // @@ -48,8 +47,8 @@ import ( // handling if unsafePath has already been scoped within the rootfs (this is // needed for a lot of runc callers and fixing this would require reworking a // lot of path logic). -func MkdirAllInRoot(root, unsafePath string, mode os.FileMode) (*os.File, error) { - unsafePath, err := hallucinateUnsafePath(root, unsafePath) +func MkdirAllInRoot(root *os.File, unsafePath string, mode os.FileMode) (*os.File, error) { + unsafePath, err := hallucinateUnsafePath(root.Name(), unsafePath) if err != nil { return nil, fmt.Errorf("failed to construct hallucinated target path: %w", err) } @@ -67,13 +66,7 @@ func MkdirAllInRoot(root, unsafePath string, mode os.FileMode) (*os.File, error) mode &= 0o1777 } - rootDir, err := os.OpenFile(root, unix.O_DIRECTORY|unix.O_CLOEXEC, 0) - if err != nil { - return nil, fmt.Errorf("open root handle: %w", err) - } - defer rootDir.Close() - return retryEAGAIN(func() (*os.File, error) { - return pathrs.MkdirAllHandle(rootDir, unsafePath, mode) + return pathrs.MkdirAllHandle(root, unsafePath, mode) }) } diff --git a/internal/pathrs/root_pathrslite.go b/internal/pathrs/root_pathrslite.go index aac73877e..149ac7655 100644 --- a/internal/pathrs/root_pathrslite.go +++ b/internal/pathrs/root_pathrslite.go @@ -26,11 +26,11 @@ import ( ) // OpenInRoot opens the given path inside the root with the provided flags. It -// is effectively shorthand for [securejoin.OpenInRoot] followed by +// is effectively shorthand for [securejoin.OpenatInRoot] followed by // [securejoin.Reopen]. -func OpenInRoot(root, subpath string, flags int) (*os.File, error) { +func OpenInRoot(root *os.File, subpath string, flags int) (*os.File, error) { handle, err := retryEAGAIN(func() (*os.File, error) { - return pathrs.OpenInRoot(root, subpath) + return pathrs.OpenatInRoot(root, subpath) }) if err != nil { return nil, err @@ -45,7 +45,7 @@ func OpenInRoot(root, subpath string, flags int) (*os.File, error) { // open(O_CREAT|O_NOFOLLOW) semantics. If you want the creation to use O_EXCL, // include it in the passed flags. The fileMode argument uses unix.* mode bits, // *not* os.FileMode. -func CreateInRoot(root, subpath string, flags int, fileMode uint32) (*os.File, error) { +func CreateInRoot(root *os.File, subpath string, flags int, fileMode uint32) (*os.File, error) { dirFd, filename, err := MkdirAllParentInRoot(root, subpath, 0o755) if err != nil { return nil, err @@ -61,5 +61,5 @@ func CreateInRoot(root, subpath string, flags int, fileMode uint32) (*os.File, e if err != nil { return nil, err } - return os.NewFile(uintptr(fd), root+"/"+subpath), nil + return os.NewFile(uintptr(fd), root.Name()+"/"+subpath), nil } diff --git a/libcontainer/criu_linux.go b/libcontainer/criu_linux.go index 94e951ba7..d0f974272 100644 --- a/libcontainer/criu_linux.go +++ b/libcontainer/criu_linux.go @@ -540,10 +540,16 @@ func isOnTmpfs(path string, mounts []*configs.Mount) bool { // This function also creates missing mountpoints as long as they // are not on top of a tmpfs, as CRIU will restore tmpfs content anyway. func (c *Container) prepareCriuRestoreMounts(mounts []*configs.Mount) error { + rootFd, err := os.OpenFile(c.config.Rootfs, unix.O_DIRECTORY|unix.O_CLOEXEC|unix.O_PATH, 0) + if err != nil { + return fmt.Errorf("open rootfs handle: %w", err) + } + defer rootFd.Close() + umounts := []string{} defer func() { for _, u := range umounts { - mntFile, err := pathrs.OpenInRoot(c.config.Rootfs, u, unix.O_PATH) + mntFile, err := pathrs.OpenInRoot(rootFd, u, unix.O_PATH) if err != nil { logrus.Warnf("Error during cleanup unmounting %s: open handle: %v", u, err) continue @@ -577,7 +583,7 @@ func (c *Container) prepareCriuRestoreMounts(mounts []*configs.Mount) error { continue } me := mountEntry{Mount: m} - if err := me.createOpenMountpoint(c.config.Rootfs); err != nil { + if err := me.createOpenMountpoint(rootFd); err != nil { return fmt.Errorf("create criu restore mountpoint for %s mount: %w", me.Destination, err) } if me.dstFile != nil { diff --git a/libcontainer/rootfs_linux.go b/libcontainer/rootfs_linux.go index 45d510892..7d6e2931b 100644 --- a/libcontainer/rootfs_linux.go +++ b/libcontainer/rootfs_linux.go @@ -34,7 +34,7 @@ const defaultMountFlags = unix.MS_NOEXEC | unix.MS_NOSUID | unix.MS_NODEV // mountConfig contains mount data not specific to a mount point. type mountConfig struct { - root string + root *os.File label string cgroup2Path string rootlessCgroups bool @@ -105,8 +105,17 @@ func prepareRootfs(pipe *syncSocket, iConfig *initConfig) (err error) { return fmt.Errorf("error preparing rootfs: %w", err) } + // Pre-open rootfs. NOTE that if we need to re-enable support for mounting + // on top of container root (see issue 5070), we will need to reopen rootFd + // after such mounts. + rootFd, err := os.OpenFile(config.Rootfs, unix.O_DIRECTORY|unix.O_CLOEXEC|unix.O_PATH, 0) + if err != nil { + return fmt.Errorf("open rootfs handle: %w", err) + } + defer rootFd.Close() + mountConfig := &mountConfig{ - root: config.Rootfs, + root: rootFd, label: config.MountLabel, cgroup2Path: iConfig.Cgroup2Path, rootlessCgroups: config.RootlessCgroups, @@ -166,7 +175,7 @@ func prepareRootfs(pipe *syncSocket, iConfig *initConfig) (err error) { setupDev := needsSetupDev(config) if setupDev { - if err := createDevices(config); err != nil { + if err := createDevices(config, rootFd); err != nil { return fmt.Errorf("error creating device nodes: %w", err) } if err := setupPtmx(config); err != nil { @@ -216,6 +225,7 @@ func prepareRootfs(pipe *syncSocket, iConfig *initConfig) (err error) { if err != nil { return fmt.Errorf("error jailing process inside rootfs: %w", err) } + rootFd.Close() // Apply root mount propagation flags. // This must be done after pivot_root/chroot because the mount propagation flag is applied @@ -361,7 +371,7 @@ func mountCgroupV1(m mountEntry, c *mountConfig) error { // symlink(2) is very dumb, it will just shove the path into // the link and doesn't do any checks or relative path // conversion. Also, don't error out if the cgroup already exists. - if err := os.Symlink(mc, filepath.Join(c.root, m.Destination, ss)); err != nil && !os.IsExist(err) { + if err := os.Symlink(mc, filepath.Join(c.root.Name(), m.Destination, ss)); err != nil && !errors.Is(err, os.ErrExist) { return err } } @@ -425,15 +435,22 @@ func doTmpfsCopyUp(m mountEntry, mountLabel string) (Err error) { } defer tmpDirFile.Close() + hostRootFd, err := os.OpenFile("/", unix.O_DIRECTORY|unix.O_CLOEXEC|unix.O_PATH, 0) + if err != nil { + return fmt.Errorf("tmpcopyup: open host root: %w", err) + } + defer hostRootFd.Close() + // Configure the *host* tmpdir as if it's the container mount. We change // m.dstFile since we are going to mount *on the host*. hostMount := mountEntry{ Mount: m.Mount, dstFile: tmpDirFile, } - if err := hostMount.mountPropagate("/", mountLabel); err != nil { + if err := hostMount.mountPropagate(hostRootFd, mountLabel); err != nil { return err } + hostRootFd.Close() defer func() { if Err != nil { if err := unmount(tmpDir, unix.MNT_DETACH); err != nil { @@ -502,9 +519,10 @@ func statfsToMountFlags(st unix.Statfs_t) int { return flags } -func (m *mountEntry) createOpenMountpoint(rootfs string) (Err error) { +func (m *mountEntry) createOpenMountpoint(root *os.File) (Err error) { + rootfs := root.Name() unsafePath := pathrs.LexicallyStripRoot(rootfs, m.Destination) - dstFile, err := pathrs.OpenInRoot(rootfs, unsafePath, unix.O_PATH) + dstFile, err := pathrs.OpenInRoot(root, unsafePath, unix.O_PATH) defer func() { if dstFile != nil && Err != nil { _ = dstFile.Close() @@ -541,9 +559,9 @@ func (m *mountEntry) createOpenMountpoint(rootfs string) (Err error) { dstIsFile = !fi.IsDir() } if dstIsFile { - dstFile, err = pathrs.CreateInRoot(rootfs, unsafePath, unix.O_CREAT|unix.O_EXCL|unix.O_NOFOLLOW, 0o644) + dstFile, err = pathrs.CreateInRoot(root, unsafePath, unix.O_CREAT|unix.O_EXCL|unix.O_NOFOLLOW, 0o644) } else { - dstFile, err = pathrs.MkdirAllInRoot(rootfs, unsafePath, 0o755) + dstFile, err = pathrs.MkdirAllInRoot(root, unsafePath, 0o755) } if err != nil { return fmt.Errorf("make mountpoint %q: %w", m.Destination, err) @@ -589,7 +607,7 @@ func mountToRootfs(c *mountConfig, m mountEntry) error { // has been a "fun" attack scenario in the past. // TODO: This won't be necessary once we switch to libpathrs and we can // stop all of these symlink-exchange attacks. - rootfs := c.root + rootfs := c.root.Name() dest := filepath.Clean(m.Destination) if !pathrs.IsLexicallyInRoot(rootfs, dest) { // Do not use securejoin as it resolves symlinks. @@ -936,7 +954,7 @@ func reOpenDevNull() error { } // Create the device nodes in the container. -func createDevices(config *configs.Config) error { +func createDevices(config *configs.Config, rootFd *os.File) error { useBindMount := userns.RunningInUserNS() || config.Namespaces.Contains(configs.NEWUSER) for _, node := range config.Devices { @@ -947,7 +965,7 @@ func createDevices(config *configs.Config) error { // containers running in a user namespace are not allowed to mknod // devices so we can just bind mount it from the host. - if err := createDeviceNode(config.Rootfs, node, useBindMount); err != nil { + if err := createDeviceNode(rootFd, node, useBindMount); err != nil { return err } } @@ -967,12 +985,12 @@ func bindMountDeviceNode(destDir *os.File, destName string, node *devices.Device } // Creates the device node in the rootfs of the container. -func createDeviceNode(rootfs string, node *devices.Device, bind bool) error { +func createDeviceNode(rootFd *os.File, node *devices.Device, bind bool) error { if node.Path == "" { // The node only exists for cgroup reasons, ignore it here. return nil } - destDir, destName, err := pathrs.MkdirAllParentInRoot(rootfs, node.Path, 0o755) + destDir, destName, err := pathrs.MkdirAllParentInRoot(rootFd, node.Path, 0o755) if err != nil { return fmt.Errorf("mkdir parent of device inode %q: %w", node.Path, err) } @@ -1384,7 +1402,12 @@ func maskPaths(rootFs string, paths []string, mountLabel string) error { // resolves the underlying inode via procfs and re-opens it through // rootFd, so the resulting fd is anchored to the real path inside the // container rootfs even if path was a /proc/self/fd/N alias. - reopened, err := reopenAfterMount(rootFs, dstFh, unix.O_PATH|unix.O_CLOEXEC) + rootFd, err := os.OpenFile(rootFs, unix.O_DIRECTORY|unix.O_CLOEXEC|unix.O_PATH, 0) + if err != nil { + return fmt.Errorf("open rootfs handle for masked paths: %w", err) + } + reopened, err := reopenAfterMount(rootFd, dstFh, unix.O_PATH|unix.O_CLOEXEC) + rootFd.Close() if err != nil { return fmt.Errorf("can't reopen shared directory mask: %w", err) } @@ -1407,16 +1430,17 @@ func maskPaths(rootFs string, paths []string, mountLabel string) error { return nil } -func reopenAfterMount(rootfs string, f *os.File, flags int) (_ *os.File, Err error) { +func reopenAfterMount(rootFd, f *os.File, flags int) (_ *os.File, Err error) { fullPath, err := procfs.ProcSelfFdReadlink(f) if err != nil { return nil, fmt.Errorf("get full path: %w", err) } + rootfs := rootFd.Name() if !pathrs.IsLexicallyInRoot(rootfs, fullPath) { return nil, fmt.Errorf("mountpoint %q is outside of rootfs %q", fullPath, rootfs) } unsafePath := pathrs.LexicallyStripRoot(rootfs, fullPath) - reopened, err := pathrs.OpenInRoot(rootfs, unsafePath, flags) + reopened, err := pathrs.OpenInRoot(rootFd, unsafePath, flags) if err != nil { return nil, fmt.Errorf("re-open mountpoint %q: %w", unsafePath, err) } @@ -1446,7 +1470,7 @@ func reopenAfterMount(rootfs string, f *os.File, flags int) (_ *os.File, Err err // Do the mount operation followed by additional mounts required to take care // of propagation flags. This will always be scoped inside the container rootfs. -func (m *mountEntry) mountPropagate(rootfs string, mountLabel string) error { +func (m *mountEntry) mountPropagate(rootFd *os.File, mountLabel string) error { var ( data = label.FormatMountLabel(m.Data, mountLabel) flags = m.Flags @@ -1472,7 +1496,7 @@ func (m *mountEntry) mountPropagate(rootfs string, mountLabel string) error { // // TODO: Use move_mount(2) on newer kernels so that this is no longer // necessary on modern systems. - newDstFile, err := reopenAfterMount(rootfs, m.dstFile, unix.O_PATH) + newDstFile, err := reopenAfterMount(rootFd, m.dstFile, unix.O_PATH) if err != nil { return fmt.Errorf("reopen mountpoint after mount: %w", err) } From 083e21ed0f58ad38fbe0a298783a6f25556ddfa6 Mon Sep 17 00:00:00 2001 From: lfbzhm Date: Fri, 27 Mar 2026 13:27:43 -0700 Subject: [PATCH 10/13] libct: use preopened rootfs more This uses preopened rootfs in Chdir and pivotRoot. While at it, add O_PATH when opening oldroot in pivotRoot. Co-authored-by: Kir Kolyshkin Signed-off-by: lfbzhm Signed-off-by: Kir Kolyshkin (cherry picked from commit 5b094ed1ac281d0fd218590d02fa0714cc6953ba) Signed-off-by: Aleksa Sarai --- libcontainer/rootfs_linux.go | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/libcontainer/rootfs_linux.go b/libcontainer/rootfs_linux.go index 7d6e2931b..e50af0d5b 100644 --- a/libcontainer/rootfs_linux.go +++ b/libcontainer/rootfs_linux.go @@ -203,7 +203,7 @@ func prepareRootfs(pipe *syncSocket, iConfig *initConfig) (err error) { // container. It's just cleaner to do this here (at the expense of the // operation not being perfectly split). - if err := unix.Chdir(config.Rootfs); err != nil { + if err := unix.Fchdir(int(rootFd.Fd())); err != nil { return &os.PathError{Op: "chdir", Path: config.Rootfs, Err: err} } @@ -218,7 +218,7 @@ func prepareRootfs(pipe *syncSocket, iConfig *initConfig) (err error) { if config.NoPivotRoot { err = msMoveRoot(config.Rootfs) } else if config.Namespaces.Contains(configs.NEWNS) { - err = pivotRoot(config.Rootfs) + err = pivotRoot(rootFd) } else { err = chroot() } @@ -1139,28 +1139,22 @@ func setupPtmx(config *configs.Config) error { // pivotRoot will call pivot_root such that rootfs becomes the new root // filesystem, and everything else is cleaned up. -func pivotRoot(rootfs string) error { +func pivotRoot(root *os.File) error { // While the documentation may claim otherwise, pivot_root(".", ".") is // actually valid. What this results in is / being the new root but // /proc/self/cwd being the old root. Since we can play around with the cwd // with pivot_root this allows us to pivot without creating directories in // the rootfs. Shout-outs to the LXC developers for giving us this idea. - oldroot, err := unix.Open("/", unix.O_DIRECTORY|unix.O_RDONLY, 0) + oldroot, err := unix.Open("/", unix.O_DIRECTORY|unix.O_RDONLY|unix.O_PATH, 0) if err != nil { return &os.PathError{Op: "open", Path: "/", Err: err} } defer unix.Close(oldroot) //nolint: errcheck - newroot, err := unix.Open(rootfs, unix.O_DIRECTORY|unix.O_RDONLY, 0) - if err != nil { - return &os.PathError{Op: "open", Path: rootfs, Err: err} - } - defer unix.Close(newroot) //nolint: errcheck - // Change to the new root so that the pivot_root actually acts on it. - if err := unix.Fchdir(newroot); err != nil { - return &os.PathError{Op: "fchdir", Path: "fd " + strconv.Itoa(newroot), Err: err} + if err := unix.Fchdir(int(root.Fd())); err != nil { + return &os.PathError{Op: "chdir", Path: root.Name(), Err: err} } if err := unix.PivotRoot(".", "."); err != nil { From 78c50d4a5f31758ada2645cb45cfbc1a3c5bb4a8 Mon Sep 17 00:00:00 2001 From: Aleksa Sarai Date: Wed, 15 Apr 2026 01:12:31 +1000 Subject: [PATCH 11/13] rootfs: switch createDevices argument order This argument order matches most other helpers we have and will also match the changes we are about to make to setupPtmx and setupDevSymlinks. Signed-off-by: Aleksa Sarai (cherry picked from commit fcf04eb41b0e3c74a6ba24c75bf50875e01240f5) Signed-off-by: Aleksa Sarai --- libcontainer/rootfs_linux.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/libcontainer/rootfs_linux.go b/libcontainer/rootfs_linux.go index e50af0d5b..c7c41773d 100644 --- a/libcontainer/rootfs_linux.go +++ b/libcontainer/rootfs_linux.go @@ -175,7 +175,7 @@ func prepareRootfs(pipe *syncSocket, iConfig *initConfig) (err error) { setupDev := needsSetupDev(config) if setupDev { - if err := createDevices(config, rootFd); err != nil { + if err := createDevices(rootFd, config); err != nil { return fmt.Errorf("error creating device nodes: %w", err) } if err := setupPtmx(config); err != nil { @@ -954,7 +954,7 @@ func reOpenDevNull() error { } // Create the device nodes in the container. -func createDevices(config *configs.Config, rootFd *os.File) error { +func createDevices(rootFd *os.File, config *configs.Config) error { useBindMount := userns.RunningInUserNS() || config.Namespaces.Contains(configs.NEWUSER) for _, node := range config.Devices { From a8e53f2c6d6d25cb3dd643cc514f118aab44b097 Mon Sep 17 00:00:00 2001 From: Aleksa Sarai Date: Wed, 15 Apr 2026 01:44:09 +1000 Subject: [PATCH 12/13] 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 (cherry picked from commit 864db8042dbb191028676f80addf8c35f348aee2) 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 149ac7655..1914b5a88 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" @@ -63,3 +65,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 c7c41773d..c13eb7c52 100644 --- a/libcontainer/rootfs_linux.go +++ b/libcontainer/rootfs_linux.go @@ -96,6 +96,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 +} + // prepareRootfs sets up the devices, mount points, and filesystems for use // inside a new mount namespace. It doesn't set anything as ro. You must call // finalizeRootfs after this function to finish setting up the rootfs. @@ -175,14 +188,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) } } @@ -894,7 +901,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. @@ -910,11 +917,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 && !os.IsExist(err) { + target, devName := link[0], link[1] + if err := pathrs.SymlinkInRoot(target, rootFd, devName); err != nil && !errors.Is(err, os.ErrExist) { return err } } @@ -1126,15 +1130,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 && !os.IsNotExist(err) { +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 From 9432ad3a96f5fe99e5d2805f9b6807d43af639a1 Mon Sep 17 00:00:00 2001 From: Aleksa Sarai Date: Wed, 15 Apr 2026 01:44:21 +1000 Subject: [PATCH 13/13] rootfs: make cgroupv1 subsystem symlinks fd-based As with /dev symlinks, this was missed in commit d40b3439a961 ("rootfs: switch to fd-based handling of mountpoint targets"). It's not really clear to what extent this was exploitable (/sys/fs/cgroup is a tmpfs we create) but it's better to just fix this anyway. Fixes: d40b3439a961 ("rootfs: switch to fd-based handling of mountpoint targets") Signed-off-by: Aleksa Sarai (cherry picked from commit 66acd48f9d423e1a5551aa4cc6a8fab91ddec945) Signed-off-by: Aleksa Sarai --- libcontainer/rootfs_linux.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/libcontainer/rootfs_linux.go b/libcontainer/rootfs_linux.go index c13eb7c52..4b76f3e0d 100644 --- a/libcontainer/rootfs_linux.go +++ b/libcontainer/rootfs_linux.go @@ -378,7 +378,8 @@ func mountCgroupV1(m mountEntry, c *mountConfig) error { // symlink(2) is very dumb, it will just shove the path into // the link and doesn't do any checks or relative path // conversion. Also, don't error out if the cgroup already exists. - if err := os.Symlink(mc, filepath.Join(c.root.Name(), m.Destination, ss)); err != nil && !errors.Is(err, os.ErrExist) { + ssPath := filepath.Join(m.Destination, ss) + if err := pathrs.SymlinkInRoot(mc, c.root, ssPath); err != nil && !errors.Is(err, os.ErrExist) { return err } }