diff --git a/internal/pathrs/mkdirall.go b/internal/pathrs/mkdirall.go new file mode 100644 index 000000000..31cc08579 --- /dev/null +++ b/internal/pathrs/mkdirall.go @@ -0,0 +1,59 @@ +// 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" +) + +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. +// +// 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 *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.Name(), unsafePath) + if err != nil { + return nil, "", fmt.Errorf("failed to construct hallucinated target path: %w", err) + } + + 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) + return dirFd, filename, err +} diff --git a/internal/pathrs/mkdirall_pathrslite.go b/internal/pathrs/mkdirall_pathrslite.go index a9a0157c6..0a6f25ebd 100644 --- a/internal/pathrs/mkdirall_pathrslite.go +++ b/internal/pathrs/mkdirall_pathrslite.go @@ -21,16 +21,14 @@ package pathrs import ( "fmt" "os" - "path/filepath" "github.com/cyphar/filepath-securejoin/pathrs-lite" "github.com/sirupsen/logrus" - "golang.org/x/sys/unix" ) -// MkdirAllInRootOpen attempts to make +// MkdirAllInRoot attempts to make // -// path, _ := securejoin.SecureJoin(root, unsafePath) +// path, _ := securejoin.SecureJoin(root.Name(), unsafePath) // os.MkdirAll(path, mode) // os.Open(path) // @@ -49,19 +47,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) { - // 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 - // 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 +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) } // Check for any silly mode bits. @@ -77,23 +66,7 @@ func MkdirAllInRootOpen(root, unsafePath string, mode os.FileMode) (*os.File, er 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) }) } - -// 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/internal/pathrs/path.go b/internal/pathrs/path.go index 1ee7c795d..77be98924 100644 --- a/internal/pathrs/path.go +++ b/internal/pathrs/path.go @@ -19,7 +19,11 @@ package pathrs import ( + "os" + "path/filepath" "strings" + + securejoin "github.com/cyphar/filepath-securejoin" ) // IsLexicallyInRoot is shorthand for strings.HasPrefix(path+"/", root+"/"), @@ -32,3 +36,81 @@ 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) +} + +// 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/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/internal/pathrs/root_pathrslite.go b/internal/pathrs/root_pathrslite.go index 899af2703..1914b5a88 100644 --- a/internal/pathrs/root_pathrslite.go +++ b/internal/pathrs/root_pathrslite.go @@ -28,11 +28,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 @@ -47,13 +47,8 @@ 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) { - 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 := MkdirAllInRootOpen(root, dir, 0o755) +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 } @@ -68,5 +63,48 @@ 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 +} + +// 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/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/criu_linux.go b/libcontainer/criu_linux.go index 53a0202ad..d0f974272 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" ) @@ -539,19 +540,31 @@ 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 { - _ = 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(rootFd, 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. @@ -570,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/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..4b76f3e0d 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" @@ -35,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 @@ -90,13 +89,26 @@ 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 } } 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. @@ -106,8 +118,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, @@ -167,14 +188,8 @@ func prepareRootfs(pipe *syncSocket, iConfig *initConfig) (err error) { setupDev := needsSetupDev(config) if setupDev { - if err := createDevices(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) } } @@ -195,7 +210,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} } @@ -210,13 +225,14 @@ 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() } 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 @@ -256,7 +272,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 } @@ -328,12 +344,13 @@ 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). - 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.MkdirAllInRoot(c.root, b.Destination, 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 @@ -361,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, m.Destination, ss)); err != nil && !os.IsExist(err) { + ssPath := filepath.Join(m.Destination, ss) + if err := pathrs.SymlinkInRoot(mc, c.root, ssPath); err != nil && !errors.Is(err, os.ErrExist) { return err } } @@ -425,15 +443,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,11 +527,10 @@ 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 := utils.StripRoot(rootfs, m.Destination) - dstFile, err := pathrs.OpenInRoot(rootfs, unsafePath, unix.O_PATH) +func (m *mountEntry) createOpenMountpoint(root *os.File) (Err error) { + rootfs := root.Name() + unsafePath := pathrs.LexicallyStripRoot(rootfs, m.Destination) + dstFile, err := pathrs.OpenInRoot(root, unsafePath, unix.O_PATH) defer func() { if dstFile != nil && Err != nil { _ = dstFile.Close() @@ -542,22 +566,10 @@ 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 = utils.StripRoot(rootfs, newUnsafePath) - 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.MkdirAllInRootOpen(rootfs, unsafePath, 0o755) + dstFile, err = pathrs.MkdirAllInRoot(root, unsafePath, 0o755) } if err != nil { return fmt.Errorf("make mountpoint %q: %w", m.Destination, err) @@ -587,7 +599,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() @@ -604,6 +615,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.Name() dest := filepath.Clean(m.Destination) if !pathrs.IsLexicallyInRoot(rootfs, dest) { // Do not use securejoin as it resolves symlinks. @@ -619,7 +631,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(c.root, dest, 0o755) if err != nil { return err } @@ -627,17 +639,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 { @@ -648,12 +660,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 } @@ -775,7 +787,7 @@ func mountToRootfs(c *mountConfig, m mountEntry) error { } return mountCgroupV1(m, c) default: - return m.mountPropagate(rootfs, mountLabel) + return m.mountPropagate(c.root, mountLabel) } } @@ -890,7 +902,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. @@ -906,11 +918,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 } } @@ -950,18 +959,18 @@ func reOpenDevNull() error { } // Create the device nodes in the container. -func createDevices(config *configs.Config) error { +func createDevices(rootFd *os.File, config *configs.Config) error { useBindMount := userns.RunningInUserNS() || config.Namespaces.Contains(configs.NEWUSER) 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 } // 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 } } @@ -981,20 +990,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 } - 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.MkdirAllInRootOpen(rootfs, destDirPath, 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) } @@ -1130,41 +1131,31 @@ 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 // 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 { @@ -1406,7 +1397,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) } @@ -1429,16 +1425,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 := utils.StripRoot(rootfs, fullPath) - reopened, err := pathrs.OpenInRoot(rootfs, unsafePath, flags) + unsafePath := pathrs.LexicallyStripRoot(rootfs, fullPath) + reopened, err := pathrs.OpenInRoot(rootFd, unsafePath, flags) if err != nil { return nil, fmt.Errorf("re-open mountpoint %q: %w", unsafePath, err) } @@ -1468,7 +1465,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 @@ -1477,7 +1474,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 } @@ -1494,16 +1491,14 @@ 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) } _ = 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/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..cda0a8841 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 ( @@ -150,9 +151,12 @@ 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 = 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) @@ -179,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() 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 +}