Repository: opencontainers/runc
Pull request #4334 (2024-07-02 10:22:54)

[1.1] rootfs: fix 'can we mount on top of /proc' check
(Backport of a patch from #3985.)

<hr>

(This is a cherry-pick of cdff09ab875159d004035990c0d45e8bdf20ed35 but modified so that changes like 8e8b136c4923a and a60933bb24565 don't also need to be backported. Ideally we would backport the entire "remove all mount logic from nsexec" series, but that would be a bit too much.)

Our previous test for whether we can mount on top of /proc incorrectly assumed that it would only be called with bind-mount sources. This meant that having a non bind-mount entry for a pseudo-filesystem (like overlayfs) with a dummy source set to /proc on the host would let you bypass the check, which could easily lead to security issues.

In addition, the check should be applied more uniformly to all mount types, so fix that as well. And add some tests for some of the tricky cases to make sure we protect against them properly.

Fixes: 331692baa7af ("Only allow proc mount if it is procfs")
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>

kolyshkin: @cyphar this one needs a rebase to make CI green again as #4336 is now merged 

Patch:
diff --git a/libcontainer/container_linux.go b/libcontainer/container_linux.go
index 40b332f9..7eb3a99a 100644
--- a/libcontainer/container_linux.go
+++ b/libcontainer/container_linux.go
@@ -1300,7 +1300,7 @@ func (c *linuxContainer) makeCriuRestoreMountpoints(m *configs.Mount) error {
 		if err != nil {
 			return err
 		}
-		if err := checkProcMount(c.config.Rootfs, dest, ""); err != nil {
+		if err := checkProcMount(c.config.Rootfs, dest, m, ""); err != nil {
 			return err
 		}
 		if err := os.MkdirAll(dest, 0o755); err != nil {
diff --git a/libcontainer/rootfs_linux.go b/libcontainer/rootfs_linux.go
index 52ad3ba1..f6626730 100644
--- a/libcontainer/rootfs_linux.go
+++ b/libcontainer/rootfs_linux.go
@@ -244,7 +244,7 @@ func prepareBindMount(m *configs.Mount, rootfs string, mountFd *int) error {
 	if dest, err = securejoin.SecureJoin(rootfs, m.Destination); err != nil {
 		return err
 	}
-	if err := checkProcMount(rootfs, dest, source); err != nil {
+	if err := checkProcMount(rootfs, dest, m, source); err != nil {
 		return err
 	}
 	if err := createIfNotExists(dest, stat.IsDir()); err != nil {
@@ -509,7 +509,7 @@ func mountToRootfs(m *configs.Mount, c *mountConfig) error {
 		}
 		return mountCgroupV1(m, c)
 	default:
-		if err := checkProcMount(rootfs, dest, m.Source); err != nil {
+		if err := checkProcMount(rootfs, dest, m, m.Source); err != nil {
 			return err
 		}
 		if err := os.MkdirAll(dest, 0o755); err != nil {
@@ -557,11 +557,17 @@ func getCgroupMounts(m *configs.Mount) ([]*configs.Mount, error) {
 	return binds, nil
 }
 
-// checkProcMount checks to ensure that the mount destination is not over the top of /proc.
-// dest is required to be an abs path and have any symlinks resolved before calling this function.
+// Taken from <include/linux/proc_ns.h>. If a file is on a filesystem of type
+// PROC_SUPER_MAGIC, we're guaranteed that only the root of the superblock will
+// have this inode number.
+const procRootIno = 1
+
+// checkProcMount checks to ensure that the mount destination is not over the
+// top of /proc. dest is required to be an abs path and have any symlinks
+// resolved before calling this function.
 //
-// if source is nil, don't stat the filesystem.  This is used for restore of a checkpoint.
-func checkProcMount(rootfs, dest, source string) error {
+// source is "" when doing criu restores.
+func checkProcMount(rootfs, dest string, m *configs.Mount, source string) error {
 	const procPath = "/proc"
 	path, err := filepath.Rel(filepath.Join(rootfs, procPath), dest)
 	if err != nil {
@@ -572,18 +578,39 @@ func checkProcMount(rootfs, dest, source string) error {
 		return nil
 	}
 	if path == "." {
-		// an empty source is pasted on restore
+		// Skip this check for criu restores.
+		// NOTE: This is a special case kept from the original implementation,
+		// only present for the 1.1.z branch to avoid any possible breakage in
+		// a patch release. This check was removed in commit cdff09ab8751
+		// ("rootfs: fix 'can we mount on top of /proc' check") in 1.2, because
+		// it doesn't make sense with the new IsBind()-based checks.
 		if source == "" {
 			return nil
 		}
-		// only allow a mount on-top of proc if it's source is "proc"
-		isproc, err := isProc(source)
-		if err != nil {
-			return err
-		}
-		// pass if the mount is happening on top of /proc and the source of
-		// the mount is a proc filesystem
-		if isproc {
+		// Only allow bind-mounts on top of /proc, and only if the source is a
+		// procfs mount.
+		if m.IsBind() {
+			var fsSt unix.Statfs_t
+			if err := unix.Statfs(source, &fsSt); err != nil {
+				return &os.PathError{Op: "statfs", Path: source, Err: err}
+			}
+			if fsSt.Type == unix.PROC_SUPER_MAGIC {
+				var uSt unix.Stat_t
+				if err := unix.Stat(source, &uSt); err != nil {
+					return &os.PathError{Op: "stat", Path: source, Err: err}
+				}
+				if uSt.Ino != procRootIno {
+					// We cannot error out in this case, because we've
+					// supported these kinds of mounts for a long time.
+					// However, we would expect users to bind-mount the root of
+					// a real procfs on top of /proc in the container. We might
+					// want to block this in the future.
+					logrus.Warnf("bind-mount %v (source %v) is of type procfs but is not the root of a procfs (inode %d). Future versions of runc might block this configuration -- please report an issue to <https://github.com/opencontainers/runc> if you see this warning.", dest, source, uSt.Ino)
+				}
+				return nil
+			}
+		} else if m.Device == "proc" {
+			// Fresh procfs-type mounts are always safe to mount on top of /proc.
 			return nil
 		}
 		return fmt.Errorf("%q cannot be mounted because it is not of type proc", dest)
@@ -617,14 +644,6 @@ func checkProcMount(rootfs, dest, source string) error {
 	return fmt.Errorf("%q cannot be mounted because it is inside /proc", dest)
 }
 
-func isProc(path string) (bool, error) {
-	var s unix.Statfs_t
-	if err := unix.Statfs(path, &s); err != nil {
-		return false, &os.PathError{Op: "statfs", Path: path, Err: err}
-	}
-	return s.Type == unix.PROC_SUPER_MAGIC, nil
-}
-
 func setupDevSymlinks(rootfs string) error {
 	links := [][2]string{
 		{"/proc/self/fd", "/dev/fd"},


Test patch:
diff --git a/libcontainer/rootfs_linux_test.go b/libcontainer/rootfs_linux_test.go
index 223f75e8..9a5f7b1e 100644
--- a/libcontainer/rootfs_linux_test.go
+++ b/libcontainer/rootfs_linux_test.go
@@ -3,52 +3,129 @@ package libcontainer
 import (
 	"testing"
 
+	"golang.org/x/sys/unix"
+
 	"github.com/opencontainers/runc/libcontainer/configs"
 )
 
-func TestCheckMountDestOnProc(t *testing.T) {
+func TestCheckMountDestInProc(t *testing.T) {
+	m := &configs.Mount{
+		Destination: "/proc/sys",
+		Source:      "/proc/sys",
+		Device:      "bind",
+		Flags:       unix.MS_BIND,
+	}
 	dest := "/rootfs/proc/sys"
-	err := checkProcMount("/rootfs", dest, "")
+	err := checkProcMount("/rootfs", dest, m, m.Source)
 	if err == nil {
 		t.Fatal("destination inside proc should return an error")
 	}
 }
 
-func TestCheckMountDestOnProcChroot(t *testing.T) {
+func TestCheckProcMountOnProc(t *testing.T) {
+	m := &configs.Mount{
+		Destination: "/proc",
+		Source:      "foo",
+		Device:      "proc",
+	}
 	dest := "/rootfs/proc/"
-	err := checkProcMount("/rootfs", dest, "/proc")
+	err := checkProcMount("/rootfs", dest, m, m.Source)
 	if err != nil {
-		t.Fatal("destination inside proc when using chroot should not return an error")
+		t.Fatalf("procfs type mount on /proc should not return an error: %v", err)
+	}
+}
+
+func TestCheckBindMountOnProc(t *testing.T) {
+	m := &configs.Mount{
+		Destination: "/proc",
+		Source:      "/proc/self",
+		Device:      "bind",
+		Flags:       unix.MS_BIND,
+	}
+	dest := "/rootfs/proc/"
+	err := checkProcMount("/rootfs", dest, m, m.Source)
+	if err != nil {
+		t.Fatalf("bind-mount of procfs on top of /proc should not return an error (for now): %v", err)
+	}
+}
+
+func TestCheckTrickyMountOnProc(t *testing.T) {
+	// Make a non-bind mount that looks like a bit like a bind-mount.
+	m := &configs.Mount{
+		Destination: "/proc",
+		Source:      "/proc",
+		Device:      "overlay",
+		Data:        "lowerdir=/tmp/fakeproc,upperdir=/tmp/fakeproc2,workdir=/tmp/work",
+	}
+	dest := "/rootfs/proc/"
+	err := checkProcMount("/rootfs", dest, m, m.Source)
+	if err == nil {
+		t.Fatalf("dodgy overlayfs mount on top of /proc should return an error")
+	}
+}
+
+func TestCheckTrickyBindMountOnProc(t *testing.T) {
+	// Make a bind mount that looks like it might be a procfs mount.
+	m := &configs.Mount{
+		Destination: "/proc",
+		Source:      "/sys",
+		Device:      "proc",
+		Flags:       unix.MS_BIND,
+	}
+	dest := "/rootfs/proc/"
+	err := checkProcMount("/rootfs", dest, m, m.Source)
+	if err == nil {
+		t.Fatalf("dodgy bind-mount on top of /proc should return an error")
 	}
 }
 
 func TestCheckMountDestInSys(t *testing.T) {
+	m := &configs.Mount{
+		Destination: "/sys/fs/cgroup",
+		Source:      "tmpfs",
+		Device:      "tmpfs",
+	}
 	dest := "/rootfs//sys/fs/cgroup"
-	err := checkProcMount("/rootfs", dest, "")
+	err := checkProcMount("/rootfs", dest, m, m.Source)
 	if err != nil {
-		t.Fatal("destination inside /sys should not return an error")
+		t.Fatalf("destination inside /sys should not return an error: %v", err)
 	}
 }
 
 func TestCheckMountDestFalsePositive(t *testing.T) {
+	m := &configs.Mount{
+		Destination: "/sysfiles/fs/cgroup",
+		Source:      "tmpfs",
+		Device:      "tmpfs",
+	}
 	dest := "/rootfs/sysfiles/fs/cgroup"
-	err := checkProcMount("/rootfs", dest, "")
+	err := checkProcMount("/rootfs", dest, m, m.Source)
 	if err != nil {
 		t.Fatal(err)
 	}
 }
 
 func TestCheckMountDestNsLastPid(t *testing.T) {
+	m := &configs.Mount{
+		Destination: "/proc/sys/kernel/ns_last_pid",
+		Source:      "lxcfs",
+		Device:      "fuse.lxcfs",
+	}
 	dest := "/rootfs/proc/sys/kernel/ns_last_pid"
-	err := checkProcMount("/rootfs", dest, "/proc")
+	err := checkProcMount("/rootfs", dest, m, m.Source)
 	if err != nil {
-		t.Fatal("/proc/sys/kernel/ns_last_pid should not return an error")
+		t.Fatalf("/proc/sys/kernel/ns_last_pid should not return an error: %v", err)
 	}
 }
 
 func TestCheckCryptoFipsEnabled(t *testing.T) {
+	m := &configs.Mount{
+		Destination: "/proc/sys/crypto/fips_enabled",
+		Source:      "tmpfs",
+		Device:      "tmpfs",
+	}
 	dest := "/rootfs/proc/sys/crypto/fips_enabled"
-	err := checkProcMount("/rootfs", dest, "/proc")
+	err := checkProcMount("/rootfs", dest, m, m.Source)
 	if err != nil {
 		t.Fatalf("/proc/sys/crypto/fips_enabled should not return an error: %v", err)
 	}