Quoting Bogdan Purcareata (bogdan.purcare...@nxp.com): > The safe_mount primitive will mount the fs in the new container > environment by using file descriptors referred in /proc/self/fd. > However, when the mounted filesystem is proc itself, it will have > been previously unmounted, therefore resulting in an error when > searching for these file descriptors. This only happens when there's > no container rootfs prefix (commonly with lxc-execute). > > Implement the support for this use case as well, by doing the mount > based on the full path. > > Refactor the whole function in order to remove duplicated code checks > and improve readability. > > Changes since v1: > - In order to address CVE-2015-1335, still check if the destination is > not a symlink. Do the mount only if the destination file descriptor > exists. > > Signed-off-by: Bogdan Purcareata <bogdan.purcare...@nxp.com> > --- > src/lxc/utils.c | 49 ++++++++++++++++++++++++++++--------------------- > 1 file changed, 28 insertions(+), 21 deletions(-) > > diff --git a/src/lxc/utils.c b/src/lxc/utils.c > index d9e769d..c53711a 100644 > --- a/src/lxc/utils.c > +++ b/src/lxc/utils.c > @@ -1644,9 +1644,9 @@ out: > int safe_mount(const char *src, const char *dest, const char *fstype, > unsigned long flags, const void *data, const char *rootfs) > { > - int srcfd = -1, destfd, ret, saved_errno; > + int srcfd = -1, destfd = -1, ret = 0; > char srcbuf[50], destbuf[50]; // only needs enough for > /proc/self/fd/<fd> > - const char *mntsrc = src; > + const char *mntsrc = src, *mntdest = dest; > > if (!rootfs) > rootfs = ""; > @@ -1655,45 +1655,52 @@ int safe_mount(const char *src, const char *dest, > const char *fstype, > if (flags & MS_BIND && src && src[0] != '/') { > INFO("this is a relative bind mount"); > srcfd = open_without_symlink(src, NULL); > - if (srcfd < 0) > - return srcfd; > + if (srcfd < 0) { > + ret = srcfd; > + goto out; > + } > ret = snprintf(srcbuf, 50, "/proc/self/fd/%d", srcfd); > if (ret < 0 || ret > 50) { > - close(srcfd); > ERROR("Out of memory"); > - return -EINVAL; > + ret = -EINVAL; > + goto out_src; > } > mntsrc = srcbuf; > } > > destfd = open_without_symlink(dest, rootfs); > if (destfd < 0) { > - if (srcfd != -1) > - close(srcfd); > - return destfd; > + ret = destfd; > + goto out_src; > } > > ret = snprintf(destbuf, 50, "/proc/self/fd/%d", destfd); > if (ret < 0 || ret > 50) { > - if (srcfd != -1) > - close(srcfd); > - close(destfd); > ERROR("Out of memory"); > - return -EINVAL; > + ret = -EINVAL; > + goto out_dest; > } > > - ret = mount(mntsrc, destbuf, fstype, flags, data); > - saved_errno = errno; > - if (srcfd != -1) > - close(srcfd); > - close(destfd); > + /* make sure the destination descriptor exists */ > + if (access(destbuf, F_OK) == 0) > + mntdest = destbuf;
First, if we're going to shortcut I'd prefer to say "if /proc/self does not exist then skip this check" fo rnow. But can we think of any way to still do this check? What exactly are the cases? 1. lxc-execute, lxc-init tries to mount /proc. We should be able to simply have lxc always mount /proc before the pivot_root, so we can properly do this check. what use-cases will break if we demand /proc to exist in the container? (We can add an option to umount /proc in lxc-init, but the directory would have to exist) 2. lxc.rootfs unset. In this case we're trusting the *host* admin to not have messed with /proc to make it a symlink, if we can't trust that we're out of luck. Other paths are not the same (since parts of the rootfs could be bind-mounted from container-owned dirs) so we should check those. But so for this check we should simply explicitly check for "/proc". Doing a more roundabout check may leave us open to subtle different attacks. In particular I imagine there are other ways to make /proc/self/fd/N not access-able, and you are, in that case, re-introducing the TOCTTOU - the attacker could try to quickliy insert the symlink after our checks and before the real mount(). > + ret = mount(mntsrc, mntdest, fstype, flags, data); > if (ret < 0) { > - errno = saved_errno; > SYSERROR("Failed to mount %s onto %s", src, dest); > - return ret; > + goto out_dest; > } > > - return 0; > + ret = 0; > + > +out_dest: > + if (destfd > 0) These should be >= 0 (here and below) right? > + close(destfd); > +out_src: > + if (srcfd > 0) > + close(srcfd); > +out: > + return ret; > } > > /* > -- > 1.9.1 > > _______________________________________________ > lxc-devel mailing list > lxc-devel@lists.linuxcontainers.org > http://lists.linuxcontainers.org/listinfo/lxc-devel _______________________________________________ lxc-devel mailing list lxc-devel@lists.linuxcontainers.org http://lists.linuxcontainers.org/listinfo/lxc-devel