Fix TOCTOU error when bind and cache mounts use "src" values

Fix a time-of-check/time-of-use error when mounting type=bind and
type=cache directories that use a "src" flag.  A hostile writer could
use a concurrently-running stage or build to replace that "src" location
between the point when we had resolved possible symbolic links and when
runc/crun/whatever actually went to create the bind mount
(CVE-2024-11218).

Stop ignoring the "src" option for cache mounts when there's no "from"
option.

Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>

cmd/buildah/run.go

@@ -13,6 +13,7 @@ import (
 	"github.com/containers/buildah/pkg/overlay"
 	"github.com/containers/buildah/pkg/parse"
 	"github.com/containers/buildah/util"
+	"github.com/containers/storage/pkg/mount"
 	"github.com/opencontainers/runtime-spec/specs-go"
 	"github.com/sirupsen/logrus"
 	"github.com/spf13/cobra"
@@ -191,14 +192,22 @@ func runCmd(c *cobra.Command, args []string, iopts runInputOptions) error {
 	if err != nil {
 		return fmt.Errorf("building system context: %w", err)
 	}
-	mounts, mountedImages, _, targetLocks, err := volumes.GetVolumes(systemContext, store, builder.MountLabel, iopts.volumes, iopts.mounts, iopts.contextDir, iopts.workingDir, tmpDir)
+	mounts, mountedImages, intermediateMounts, _, targetLocks, err := volumes.GetVolumes(systemContext, store, builder.MountLabel, iopts.volumes, iopts.mounts, iopts.contextDir, iopts.workingDir, tmpDir)
 	if err != nil {
 		return err
 	}
 	defer func() {
 		if err := overlay.CleanupContent(tmpDir); err != nil {
 			logrus.Debugf("unmounting overlay mounts under %q: %v", tmpDir, err)
 		}
+		for _, intermediateMount := range intermediateMounts {
+			if err := mount.Unmount(intermediateMount); err != nil {
+				logrus.Debugf("unmounting mount %q: %v", intermediateMount, err)
+			}
+			if err := os.Remove(intermediateMount); err != nil {
+				logrus.Debugf("removing should-be-empty mount directory %q: %v", intermediateMount, err)
+			}
+		}
 		for _, mountedImage := range mountedImages {
 			if _, err := store.UnmountImage(mountedImage, false); err != nil {
 				logrus.Debugf("unmounting image %q: %v", mountedImage, err)

run.go

@@ -194,6 +194,8 @@ type runMountArtifacts struct {
 	SSHAuthSock string
 	// Lock files, which should have their Unlock() methods called
 	TargetLocks []*lockfile.LockFile
+	// Intermediate mount points, which should be Unmount()ed and Removed()d
+	IntermediateMounts []string
 }
 
 // RunMountInfo are the available run mounts for this run

run_common.go

@@ -44,6 +44,7 @@ import (
 	"github.com/containers/storage/pkg/idtools"
 	"github.com/containers/storage/pkg/ioutils"
 	"github.com/containers/storage/pkg/lockfile"
+	"github.com/containers/storage/pkg/mount"
 	"github.com/containers/storage/pkg/reexec"
 	"github.com/containers/storage/pkg/unshare"
 	"github.com/opencontainers/go-digest"
@@ -1533,6 +1534,7 @@ func (b *Builder) runSetupRunMounts(mountPoint, bundlePath string, mounts []stri
 	mountTargets := make([]string, 0, len(mounts))
 	tmpFiles := make([]string, 0, len(mounts))
 	mountImages := make([]string, 0, len(mounts))
+	intermediateMounts := make([]string, 0, len(mounts))
 	finalMounts := make([]specs.Mount, 0, len(mounts))
 	agents := make([]*sshagent.AgentServer, 0, len(mounts))
 	defaultSSHSock := ""
@@ -1552,6 +1554,14 @@ func (b *Builder) runSetupRunMounts(mountPoint, bundlePath string, mounts []stri
 					b.Logger.Error(err.Error())
 				}
 			}
+			for _, intermediateMount := range intermediateMounts {
+				if err := mount.Unmount(intermediateMount); err != nil {
+					b.Logger.Errorf("unmounting %q: %v", intermediateMount, err)
+				}
+				if err := os.Remove(intermediateMount); err != nil {
+					b.Logger.Errorf("removing should-be-empty directory %q: %v", intermediateMount, err)
+				}
+			}
 			for _, mountImage := range mountImages {
 				if _, err := b.store.UnmountImage(mountImage, false); err != nil {
 					b.Logger.Error(err.Error())
@@ -1568,9 +1578,10 @@ func (b *Builder) runSetupRunMounts(mountPoint, bundlePath string, mounts []stri
 	for _, mount := range mounts {
 		var mountSpec *specs.Mount
 		var err error
-		var envFile, image, bundleMountsDir, overlayDir string
+		var envFile, image, bundleMountsDir, overlayDir, intermediateMount string
 		var agent *sshagent.AgentServer
 		var tl *lockfile.LockFile
+
 		tokens := strings.Split(mount, ",")
 
 		// If `type` is not set default to TypeBind
@@ -1615,7 +1626,7 @@ func (b *Builder) runSetupRunMounts(mountPoint, bundlePath string, mounts []stri
 					return nil, nil, err
 				}
 			}
-			mountSpec, image, overlayDir, err = b.getBindMount(tokens, sources.SystemContext, sources.ContextDir, sources.StageMountPoints, idMaps, sources.WorkDir, bundleMountsDir)
+			mountSpec, image, intermediateMount, overlayDir, err = b.getBindMount(tokens, sources.SystemContext, sources.ContextDir, sources.StageMountPoints, idMaps, sources.WorkDir, bundleMountsDir)
 			if err != nil {
 				return nil, nil, err
 			}
@@ -1625,6 +1636,9 @@ func (b *Builder) runSetupRunMounts(mountPoint, bundlePath string, mounts []stri
 			if overlayDir != "" {
 				overlayDirs = append(overlayDirs, overlayDir)
 			}
+			if intermediateMount != "" {
+				intermediateMounts = append(intermediateMounts, intermediateMount)
+			}
 			finalMounts = append(finalMounts, *mountSpec)
 		case "tmpfs":
 			mountSpec, err = b.getTmpfsMount(tokens, idMaps)
@@ -1633,10 +1647,18 @@ func (b *Builder) runSetupRunMounts(mountPoint, bundlePath string, mounts []stri
 			}
 			finalMounts = append(finalMounts, *mountSpec)
 		case "cache":
-			mountSpec, tl, err = b.getCacheMount(tokens, sources.StageMountPoints, idMaps, sources.WorkDir)
+			if bundleMountsDir == "" {
+				if bundleMountsDir, err = os.MkdirTemp(bundlePath, "mounts"); err != nil {
+					return nil, nil, err
+				}
+			}
+			mountSpec, intermediateMount, tl, err = b.getCacheMount(tokens, sources.StageMountPoints, idMaps, sources.WorkDir, bundleMountsDir)
 			if err != nil {
 				return nil, nil, err
 			}
+			if intermediateMount != "" {
+				intermediateMounts = append(intermediateMounts, intermediateMount)
+			}
 			if tl != nil {
 				targetLocks = append(targetLocks, tl)
 			}
@@ -1660,32 +1682,33 @@ func (b *Builder) runSetupRunMounts(mountPoint, bundlePath string, mounts []stri
 	}
 	succeeded = true
 	artifacts := &runMountArtifacts{
-		RunMountTargets: mountTargets,
-		RunOverlayDirs:  overlayDirs,
-		TmpFiles:        tmpFiles,
-		Agents:          agents,
-		MountedImages:   mountImages,
-		SSHAuthSock:     defaultSSHSock,
-		TargetLocks:     targetLocks,
+		RunMountTargets:    mountTargets,
+		RunOverlayDirs:     overlayDirs,
+		TmpFiles:           tmpFiles,
+		Agents:             agents,
+		MountedImages:      mountImages,
+		SSHAuthSock:        defaultSSHSock,
+		TargetLocks:        targetLocks,
+		IntermediateMounts: intermediateMounts,
 	}
 	return finalMounts, artifacts, nil
 }
 
-func (b *Builder) getBindMount(tokens []string, sys *types.SystemContext, contextDir string, stageMountPoints map[string]internal.StageMountDetails, idMaps IDMaps, workDir, tmpDir string) (*specs.Mount, string, string, error) {
+func (b *Builder) getBindMount(tokens []string, sys *types.SystemContext, contextDir string, stageMountPoints map[string]internal.StageMountDetails, idMaps IDMaps, workDir, tmpDir string) (*specs.Mount, string, string, string, error) {
 	if contextDir == "" {
-		return nil, "", "", errors.New("context directory for current run invocation is not configured")
+		return nil, "", "", "", errors.New("context directory for current run invocation is not configured")
 	}
 	var optionMounts []specs.Mount
-	mount, image, overlayDir, err := volumes.GetBindMount(sys, tokens, contextDir, b.store, b.MountLabel, stageMountPoints, workDir, tmpDir)
+	mount, image, intermediateMount, overlayMount, err := volumes.GetBindMount(sys, tokens, contextDir, b.store, b.MountLabel, stageMountPoints, workDir, tmpDir)
 	if err != nil {
-		return nil, image, overlayDir, err
+		return nil, "", "", "", err
 	}
 	optionMounts = append(optionMounts, mount)
 	volumes, err := b.runSetupVolumeMounts(b.MountLabel, nil, optionMounts, idMaps)
 	if err != nil {
-		return nil, image, overlayDir, err
+		return nil, "", "", "", err
 	}
-	return &volumes[0], image, overlayDir, nil
+	return &volumes[0], image, intermediateMount, overlayMount, nil
 }
 
 func (b *Builder) getTmpfsMount(tokens []string, idMaps IDMaps) (*specs.Mount, error) {
@@ -1964,9 +1987,9 @@ func (b *Builder) cleanupTempVolumes() {
 // cleanupRunMounts cleans up run mounts so they only appear in this run.
 func (b *Builder) cleanupRunMounts(mountpoint string, artifacts *runMountArtifacts) error {
 	for _, agent := range artifacts.Agents {
-		err := agent.Shutdown()
-		if err != nil {
-			return err
+		servePath := agent.ServePath()
+		if err := agent.Shutdown(); err != nil {
+			return fmt.Errorf("shutting down SSH agent at %q: %v", servePath, err)
 		}
 	}
 	// clean up any overlays we mounted
@@ -1975,6 +1998,15 @@ func (b *Builder) cleanupRunMounts(mountpoint string, artifacts *runMountArtifac
 			return err
 		}
 	}
+	// unmount anything that needs unmounting
+	for _, intermediateMount := range artifacts.IntermediateMounts {
+		if err := mount.Unmount(intermediateMount); err != nil && !errors.Is(err, os.ErrNotExist) {
+			return fmt.Errorf("unmounting %q: %w", intermediateMount, err)
+		}
+		if err := os.Remove(intermediateMount); err != nil && !errors.Is(err, os.ErrNotExist) {
+			return fmt.Errorf("removing should-be-empty directory %q: %w", intermediateMount, err)
+		}
+	}
 	// unmount any images we mounted for this run
 	for _, image := range artifacts.MountedImages {
 		if _, err := b.store.UnmountImage(image, false); err != nil {

run_freebsd.go

@@ -351,9 +351,12 @@ func setupSpecialMountSpecChanges(spec *spec.Spec, shmSize string) ([]specs.Moun
 	return spec.Mounts, nil
 }
 
-// If this function succeeds and returns a non-nil *lockfile.LockFile, the caller must unlock it (when??).
-func (b *Builder) getCacheMount(tokens []string, stageMountPoints map[string]internal.StageMountDetails, idMaps IDMaps, workDir string) (*spec.Mount, *lockfile.LockFile, error) {
-	return nil, nil, errors.New("cache mounts not supported on freebsd")
+// If this succeeded, the caller would be expected to, after the command which
+// uses the mount exits, unmount the mounted filesystem and remove its
+// mountpoint (if we provided the path to its mountpoint), and release the lock
+// (if we took one).
+func (b *Builder) getCacheMount(tokens []string, stageMountPoints map[string]internal.StageMountDetails, idMaps IDMaps, workDir, tmpDir string) (*specs.Mount, string, *lockfile.LockFile, error) {
+	return nil, "", nil, errors.New("cache mounts not supported on freebsd")
 }
 
 func (b *Builder) runSetupVolumeMounts(mountLabel string, volumeMounts []string, optionMounts []specs.Mount, idMaps IDMaps) (mounts []specs.Mount, Err error) {

run_linux.go

@@ -38,6 +38,7 @@ import (
 	"github.com/containers/storage/pkg/idtools"
 	"github.com/containers/storage/pkg/ioutils"
 	"github.com/containers/storage/pkg/lockfile"
+	"github.com/containers/storage/pkg/mount"
 	"github.com/containers/storage/pkg/stringid"
 	"github.com/containers/storage/pkg/unshare"
 	"github.com/docker/go-units"
@@ -1379,24 +1380,39 @@ func checkIdsGreaterThan5(ids []specs.LinuxIDMapping) bool {
 	return false
 }
 
-// If this function succeeds and returns a non-nil *lockfile.LockFile, the caller must unlock it (when??).
-func (b *Builder) getCacheMount(tokens []string, stageMountPoints map[string]internal.StageMountDetails, idMaps IDMaps, workDir string) (*specs.Mount, *lockfile.LockFile, error) {
+// Returns a Mount to add to the runtime spec's list of mounts, an optional
+// path of a mounted filesystem, unmounted, and an optional lock, or an error.
+//
+// The caller is expected to, after the command which uses the mount exits,
+// unmount the mounted filesystem (if we provided the path to its mountpoint)
+// and remove its mountpoint, , and release the lock (if we took one).
+func (b *Builder) getCacheMount(tokens []string, stageMountPoints map[string]internal.StageMountDetails, idMaps IDMaps, workDir, tmpDir string) (*specs.Mount, string, *lockfile.LockFile, error) {
 	var optionMounts []specs.Mount
-	mount, targetLock, err := volumes.GetCacheMount(tokens, b.store, b.MountLabel, stageMountPoints, workDir)
+	optionMount, intermediateMount, targetLock, err := volumes.GetCacheMount(tokens, stageMountPoints, workDir, tmpDir)
 	if err != nil {
-		return nil, nil, err
+		return nil, "", nil, err
 	}
 	succeeded := false
 	defer func() {
-		if !succeeded && targetLock != nil {
-			targetLock.Unlock()
+		if !succeeded {
+			if intermediateMount != "" {
+				if err := mount.Unmount(intermediateMount); err != nil {
+					b.Logger.Debugf("unmounting %q: %v", intermediateMount, err)
+				}
+				if err := os.Remove(intermediateMount); err != nil {
+					b.Logger.Debugf("removing should-be-empty directory %q: %v", intermediateMount, err)
+				}
+			}
+			if targetLock != nil {
+				targetLock.Unlock()
+			}
 		}
 	}()
-	optionMounts = append(optionMounts, mount)
+	optionMounts = append(optionMounts, optionMount)
 	volumes, err := b.runSetupVolumeMounts(b.MountLabel, nil, optionMounts, idMaps)
 	if err != nil {
-		return nil, nil, err
+		return nil, "", nil, err
 	}
 	succeeded = true
-	return &volumes[0], targetLock, nil
+	return &volumes[0], intermediateMount, targetLock, nil
 }

tests/bud.bats

@@ -6938,8 +6938,8 @@ RUN --mount=type=cache,source=../../../../../../../../../../../$TEST_SCRATCH_DIR
 ls -l /var/tmp && cat /var/tmp/file.txt
 EOF
 
-  run_buildah 1 build --no-cache ${TEST_SCRATCH_DIR}
-  expect_output --substring "cat: can't open '/var/tmp/file.txt': No such file or directory"
+  run_buildah 125 build --no-cache ${TEST_SCRATCH_DIR}
+  expect_output --substring "no such file or directory"
 
   mkdir ${TEST_SCRATCH_DIR}/cve20249675
   cat > ${TEST_SCRATCH_DIR}/cve20249675/Containerfile <<EOF

tests/bud/buildkit-mount-from/Dockerfilebuildkitbase

@@ -1,2 +1,3 @@
 FROM scratch
-COPY hello .
+COPY hello hello1 .
+COPY hello2 /subdir/hello

tests/bud/buildkit-mount-from/hello1

@@ -0,0 +1 @@
+hello1

tests/run.bats

@@ -460,8 +460,10 @@ function configure_and_check_user() {
 	_prefetch alpine
 	run_buildah from --quiet --pull=false $WITH_POLICY_JSON alpine
 	cid=$output
-	run_buildah run --mount type=bind,source=.,from=buildkitbase,target=/test${zflag}  $cid cat /test/hello
-	expect_output --substring "hello"
+	run_buildah run --mount type=bind,source=.,from=buildkitbase,target=/test${zflag}  $cid cat /test/hello1
+	expect_output --substring "hello1"
+	run_buildah run --mount type=bind,source=subdir,from=buildkitbase,target=/test${zflag}  $cid cat /test/hello
+	expect_output --substring "hello2"
 }
 
 @test "run --mount=type=cache like buildkit" {
@@ -475,8 +477,8 @@ function configure_and_check_user() {
 	_prefetch alpine
 	run_buildah from --quiet --pull=false $WITH_POLICY_JSON alpine
 	cid=$output
-	run_buildah run --mount type=cache,target=/test${zflag}  $cid sh -c 'echo "hello" > /test/hello && cat /test/hello'
-	run_buildah run --mount type=cache,target=/test${zflag}  $cid cat /test/hello
+	run_buildah run --mount type=cache,target=/test${zflag}  $cid sh -c 'mkdir -p /test/subdir && echo "hello" > /test/subdir/h.txt && cat /test/subdir/h.txt'
+	run_buildah run --mount type=cache,src=subdir,target=/test${zflag}  $cid cat /test/h.txt
 	expect_output --substring "hello"
 }