diff --git a/libcontainer/container_linux.go b/libcontainer/container_linux.go index 40b332f9810..7eb3a99a9d6 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 52ad3ba121f..f66267307e5 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 . 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 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"}, diff --git a/libcontainer/rootfs_linux_test.go b/libcontainer/rootfs_linux_test.go index 223f75e8266..9a5f7b1e6d7 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) }