rootfs: fix 'can we mount on top of /proc' check

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: 331692b ("Only allow proc mount if it is procfs")
Signed-off-by: Aleksa Sarai <[email protected]>

libcontainer/criu_linux.go

@@ -527,6 +527,14 @@ func (c *Container) restoreNetwork(req *criurpc.CriuReq, criuOpts *CriuOpts) {
 // rootfs_linux.go.
 func (c *Container) makeCriuRestoreMountpoints(m *configs.Mount) error {
 	me := mountEntry{Mount: m}
+	dest, err := securejoin.SecureJoin(c.config.Rootfs, m.Destination)
+	if err != nil {
+		return err
+	}
+	// TODO: pass srcFD? Not sure if criu is impacted by issue #2484.
+	if err := checkProcMount(c.config.Rootfs, dest, me); err != nil {
+		return err
+	}
 	switch m.Device {
 	case "cgroup":
 		// No mount point(s) need to be created:
@@ -538,21 +546,19 @@ func (c *Container) makeCriuRestoreMountpoints(m *configs.Mount) error {
 		//   the mountpoint appears as soon as /sys is mounted
 		return nil
 	case "bind":
-		// The prepareBindMount() function checks if source
-		// exists. So it cannot be used for other filesystem types.
-		// TODO: pass srcFD? Not sure if criu is impacted by issue #2484.
-		if err := prepareBindMount(me, c.config.Rootfs); err != nil {
-			return err
-		}
-	default:
-		// for all other filesystems just create the mountpoints
-		dest, err := securejoin.SecureJoin(c.config.Rootfs, m.Destination)
+		// For bind-mounts (unlike other filesystem types), we need to check if
+		// the source exists.
+		fi, _, err := me.srcStat()
 		if err != nil {
+			// error out if the source of a bind mount does not exist as we
+			// will be unable to bind anything to it.
 			return err
 		}
-		if err := checkProcMount(c.config.Rootfs, dest, me); err != nil {
+		if err := createIfNotExists(dest, fi.IsDir()); err != nil {
 			return err
 		}
+	default:
+		// for all other filesystems just create the mountpoints
 		if err := os.MkdirAll(dest, 0o755); err != nil {
 			return err
 		}

libcontainer/rootfs_linux.go

@@ -281,27 +281,6 @@ func cleanupTmp(tmpdir string) {
 	_ = os.RemoveAll(tmpdir)
 }
 
-func prepareBindMount(m mountEntry, rootfs string) error {
-	fi, _, err := m.srcStat()
-	if err != nil {
-		// error out if the source of a bind mount does not exist as we will be
-		// unable to bind anything to it.
-		return err
-	}
-	// ensure that the destination of the bind mount is resolved of symlinks at mount time because
-	// any previous mounts can invalidate the next mount's destination.
-	// this can happen when a user specifies mounts within other mounts to cause breakouts or other
-	// evil stuff to try to escape the container's rootfs.
-	var dest string
-	if dest, err = securejoin.SecureJoin(rootfs, m.Destination); err != nil {
-		return err
-	}
-	if err := checkProcMount(rootfs, dest, m); err != nil {
-		return err
-	}
-	return createIfNotExists(dest, fi.IsDir())
-}
-
 func mountCgroupV1(m *configs.Mount, c *mountConfig) error {
 	binds, err := getCgroupMounts(m)
 	if err != nil {
@@ -520,6 +499,9 @@ func mountToRootfs(c *mountConfig, m mountEntry) error {
 			// Do not use securejoin as it resolves symlinks.
 			dest = filepath.Join(rootfs, dest)
 		}
+		if err := checkProcMount(rootfs, dest, m); err != nil {
+			return err
+		}
 		if fi, err := os.Lstat(dest); err != nil {
 			if !os.IsNotExist(err) {
 				return err
@@ -539,6 +521,9 @@ func mountToRootfs(c *mountConfig, m mountEntry) error {
 	if err != nil {
 		return err
 	}
+	if err := checkProcMount(rootfs, dest, m); err != nil {
+		return err
+	}
 
 	switch m.Device {
 	case "mqueue":
@@ -570,7 +555,13 @@ func mountToRootfs(c *mountConfig, m mountEntry) error {
 
 		return err
 	case "bind":
-		if err := prepareBindMount(m, rootfs); err != nil {
+		fi, _, err := m.srcStat()
+		if err != nil {
+			// error out if the source of a bind mount does not exist as we will be
+			// unable to bind anything to it.
+			return err
+		}
+		if err := createIfNotExists(dest, fi.IsDir()); err != nil {
 			return err
 		}
 		// open_tree()-related shenanigans are all handled in mountViaFds.
@@ -688,9 +679,6 @@ func mountToRootfs(c *mountConfig, m mountEntry) error {
 		}
 		return mountCgroupV1(m.Mount, c)
 	default:
-		if err := checkProcMount(rootfs, dest, m); err != nil {
-			return err
-		}
 		if err := os.MkdirAll(dest, 0o755); err != nil {
 			return err
 		}
@@ -735,6 +723,11 @@ func getCgroupMounts(m *configs.Mount) ([]*configs.Mount, error) {
 	return binds, nil
 }
 
+// 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.
 //
@@ -750,12 +743,28 @@ func checkProcMount(rootfs, dest string, m mountEntry) error {
 		return nil
 	}
 	if path == "." {
-		// only allow a mount on-top of proc if it's source is "proc"
-		st, err := m.srcStatfs()
-		if err != nil {
-			return err
-		}
-		if st.Type == unix.PROC_SUPER_MAGIC {
+		// Only allow bind-mounts on top of /proc, and only if the source is a
+		// procfs mount.
+		if m.IsBind() {
+			fsSt, err := m.srcStatfs()
+			if err != nil {
+				return err
+			}
+			if fsSt.Type == unix.PROC_SUPER_MAGIC {
+				if _, uSt, err := m.srcStat(); err != nil {
+					return err
+				} else 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, m.srcName(), 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)

libcontainer/rootfs_linux_test.go

@@ -35,8 +35,7 @@ func TestCheckProcMountOnProc(t *testing.T) {
 	dest := "/rootfs/proc/"
 	err := checkProcMount("/rootfs", dest, m)
 	if err != nil {
-		// TODO: This test failure is fixed in a later commit in this series.
-		t.Logf("procfs type mount on /proc should not return an error: %v", err)
+		t.Fatalf("procfs type mount on /proc should not return an error: %v", err)
 	}
 }
 
@@ -52,7 +51,41 @@ func TestCheckBindMountOnProc(t *testing.T) {
 	dest := "/rootfs/proc/"
 	err := checkProcMount("/rootfs", dest, m)
 	if err != nil {
-		t.Fatalf("bind-mount of procfs on top of /proc should not return an error: %v", err)
+		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 := mountEntry{
+		Mount: &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)
+	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 := mountEntry{
+		Mount: &configs.Mount{
+			Destination: "/proc",
+			Source:      "/sys",
+			Device:      "proc",
+			Flags:       unix.MS_BIND,
+		},
+	}
+	dest := "/rootfs/proc/"
+	err := checkProcMount("/rootfs", dest, m)
+	if err == nil {
+		t.Fatalf("dodgy bind-mount on top of /proc should return an error")
 	}
 }