Merge pull request #5556 from kolyshkin/mount-dst-rootfs-deprecate

libct: don't strip rootfs prefix from mount destinations

CHANGELOG.md

@@ -6,6 +6,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### Breaking ###
+- A mount destination prefixed with the container rootfs path is no longer
+  treated specially (previously, the prefix was stripped). As the runtime spec
+  says, the destination is a path inside the container, and it is now used
+  as such. (#5556)
+
 ### Deprecated ###
 - `libcontainer.Stats.Interfaces` and `types.Stats.NetworkInterfaces` (shown
   as `network_interfaces` in `runc events --stats` output) are deprecated, as

internal/pathrs/mkdirall_pathrslite.go

@@ -37,16 +37,7 @@ import (
 // try to detect any symlink components in the path while we are doing the
 // MkdirAll.
 //
-// NOTE: If unsafePath is a subpath of root, we assume that you have already
-// called SecureJoin and so we use the provided path verbatim without resolving
-// any symlinks (this is done in a way that avoids symlink-exchange races).
-// This means that the path also must not contain ".." elements, otherwise an
-// error will occur.
-//
-// This uses (pathrs-lite).MkdirAllHandle under the hood, but it has special
-// 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).
+// This uses (pathrs-lite).MkdirAllHandle under the hood.
 func MkdirAllInRoot(root *os.File, unsafePath string, mode os.FileMode) (*os.File, error) {
 	unsafePath, err := hallucinateUnsafePath(root.Name(), unsafePath)
 	if err != nil {

internal/pathrs/path.go

@@ -101,12 +101,7 @@ func LexicallyStripRoot(root, path string) string {
 // 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

libcontainer/criu_linux.go

@@ -109,7 +109,7 @@ func (c *Container) checkCriuVersion(minVersion int) error {
 const descriptorsFilename = "descriptors.json"
 
 func (c *Container) addCriuDumpMount(req *criurpc.CriuReq, m *configs.Mount) {
-	mountDest := strings.TrimPrefix(m.Destination, c.config.Rootfs)
+	mountDest := m.Destination
 	if dest, err := securejoin.SecureJoin(c.config.Rootfs, mountDest); err == nil {
 		mountDest = dest[len(c.config.Rootfs):]
 	}
@@ -505,7 +505,7 @@ func (c *Container) Checkpoint(criuOpts *CriuOpts) error {
 }
 
 func (c *Container) addCriuRestoreMount(req *criurpc.CriuReq, m *configs.Mount) {
-	mountDest := strings.TrimPrefix(m.Destination, c.config.Rootfs)
+	mountDest := m.Destination
 	if dest, err := securejoin.SecureJoin(c.config.Rootfs, mountDest); err == nil {
 		mountDest = dest[len(c.config.Rootfs):]
 	}

libcontainer/rootfs_linux.go

@@ -530,7 +530,7 @@ func statfsToMountFlags(st unix.Statfs_t) int {
 
 func (m *mountEntry) createOpenMountpoint(root *os.File) (Err error) {
 	rootfs := root.Name()
-	unsafePath := pathrs.LexicallyStripRoot(rootfs, m.Destination)
+	unsafePath := pathrs.LexicallyCleanPath("/" + m.Destination)
 	dstFile, err := pathrs.OpenInRoot(root, unsafePath, unix.O_PATH)
 	defer func() {
 		if dstFile != nil && Err != nil {
@@ -617,11 +617,8 @@ func mountToRootfs(c *mountConfig, m mountEntry) error {
 		// 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.
-			dest = filepath.Join(rootfs, dest)
-		}
+		// Do not use securejoin as it resolves symlinks.
+		dest := filepath.Join(rootfs, m.Destination)
 		if err := checkProcMount(rootfs, dest, m); err != nil {
 			return err
 		}
@@ -632,7 +629,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(c.root, dest, 0o755)
+		dstFile, err := pathrs.MkdirAllInRoot(c.root, m.Destination, 0o755)
 		if err != nil {
 			return err
 		}

libcontainer/specconv/spec_linux.go

@@ -639,12 +639,6 @@ func toConfigIDMap(specMaps []specs.LinuxIDMapping) []configs.IDMap {
 }
 
 func createLibcontainerMount(cwd string, m specs.Mount) (*configs.Mount, error) {
-	if !filepath.IsAbs(m.Destination) {
-		// Relax validation for backward compatibility
-		// TODO (runc v1.x.x): change warning to an error
-		// return nil, fmt.Errorf("mount destination %s is not absolute", m.Destination)
-		logrus.Warnf("mount destination %s is not absolute. Support for non-absolute mount destinations will be removed in a future release.", m.Destination)
-	}
 	mnt := parseMountOptions(m.Options)
 
 	mnt.Destination = m.Destination

tests/integration/mounts.bats

@@ -188,6 +188,23 @@ test_mount_target() {
 	assert_line --index 0 --partial '/tmp/bind/config.json'
 }
 
+@test "runc run [bind mount with rootfs-prefixed destination]" {
+	# Mount destination is a path inside the container,
+	# even if it starts with the host path to rootfs.
+	dest="$(pwd -P)/rootfs/tmp/bind"
+	update_config '	  .mounts += [{
+					source: ".",
+					destination: "'"$dest"'",
+					options: ["bind"]
+				}]
+			| .process.args |= ["ls", "'"$dest"'/config.json"]'
+
+	run -0 runc run test_busybox
+	assert_line --index 0 --partial "$dest/config.json"
+	# The prefix is not stripped.
+	[ ! -e rootfs/tmp/bind ]
+}
+
 # https://github.com/opencontainers/runc/issues/2246
 @test "runc run [ro tmpfs mount]" {
 	update_config '	  .mounts += [{