From b8bf57281298e53ac24fe9850d860154aebeb6a7 Mon Sep 17 00:00:00 2001 From: Aleksa Sarai Date: Tue, 13 Oct 2020 15:03:29 +1100 Subject: [PATCH] rootfs: handle nested procfs mounts for MS_MOVE In a case where the host /proc mount has already been overmounted, the MS_MOVE handling would get ENOENT when trying to hide (for instance) "/proc/bus" because it had already hidden away "/proc". This revealed two issues in the previous implementation of this hardening feaure: 1. No checks were done to make sure the mount was a "full" mount (it is a mount of the root of the filesystem), but the kernel doesn't permit a non-full mount to be converted to a full mount (for reference, see mnt_already_visible). This just removes extra busy-work during setup. 2. ENOENT was treated as a critical error, even though it actually indicates the mount doesn't exist and thus isn't a problem. A more theoretically pure solution would be to store the set of mountpoints to be hidden and only ignore the error if an ancestor directory of the current mountpoint was already hidden, but that would just add complexity with little justification. In addition, better document the reasoning behind this logic so that folks aren't confused when looking at it. Fixes: 28a697cce3e4 ("rootfs: umount all procfs and sysfs with --no-pivot") Signed-off-by: Aleksa Sarai --- libcontainer/rootfs_linux.go | 36 ++++++++++++++++++++++++++++++------ 1 file changed, 30 insertions(+), 6 deletions(-) diff --git a/libcontainer/rootfs_linux.go b/libcontainer/rootfs_linux.go index dd52713da..87afe8e57 100644 --- a/libcontainer/rootfs_linux.go +++ b/libcontainer/rootfs_linux.go @@ -809,24 +809,46 @@ func pivotRoot(rootfs string) error { } func msMoveRoot(rootfs string) error { + // Before we move the root and chroot we have to mask all "full" sysfs and + // procfs mounts which exist on the host. This is because while the kernel + // has protections against mounting procfs if it has masks, when using + // chroot(2) the *host* procfs mount is still reachable in the mount + // namespace and the kernel permits procfs mounts inside --no-pivot + // containers. + // + // Users shouldn't be using --no-pivot except in exceptional circumstances, + // but to avoid such a trivial security flaw we apply a best-effort + // protection here. The kernel only allows a mount of a pseudo-filesystem + // like procfs or sysfs if there is a *full* mount (the root of the + // filesystem is mounted) without any other locked mount points covering a + // subtree of the mount. + // + // So we try to unmount (or mount tmpfs on top of) any mountpoint which is + // a full mount of either sysfs or procfs (since those are the most + // concerning filesystems to us). mountinfos, err := mountinfo.GetMounts(func(info *mountinfo.Info) (skip, stop bool) { - skip = false - stop = false - // Collect every sysfs and proc file systems, except those under the container rootfs - if (info.FSType != "proc" && info.FSType != "sysfs") || strings.HasPrefix(info.Mountpoint, rootfs) { + // Collect every sysfs and procfs filesystem, except for those which + // are non-full mounts or are inside the rootfs of the container. + if info.Root != "/" || + (info.FSType != "proc" && info.FSType != "sysfs") || + strings.HasPrefix(info.Mountpoint, rootfs) { skip = true - return } return }) if err != nil { return err } - for _, info := range mountinfos { p := info.Mountpoint // Be sure umount events are not propagated to the host. if err := unix.Mount("", p, "", unix.MS_SLAVE|unix.MS_REC, ""); err != nil { + if err == unix.ENOENT { + // If the mountpoint doesn't exist that means that we've + // already blasted away some parent directory of the mountpoint + // and so we don't care about this error. + continue + } return err } if err := unix.Unmount(p, unix.MNT_DETACH); err != nil { @@ -841,6 +863,8 @@ func msMoveRoot(rootfs string) error { } } } + + // Move the rootfs on top of "/" in our mount namespace. if err := unix.Mount(rootfs, "/", "", unix.MS_MOVE, ""); err != nil { return err }