Quoting Bogdan Purcareata (bogdan.purcare...@nxp.com):
> On 14.01.2016 01:09, Serge Hallyn wrote:
> > Quoting Bogdan Purcareata (bogdan.purcare...@nxp.com):
> >> On 11.01.2016 20:59, Serge Hallyn wrote:
> >>> 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)
> >> That's my use case - the failing function is tmp_proc_mount, thus happening
> >
> > Wait, is your rootfs NULL or not?
>
> Yep, it's NULL - plain "lxc-execute -n foo -f empty.conf -- bash".
>
> > We can handle that case specially.
> >
> > mount_proc_if_needed() is only called from tmp_proc_mount(), which is only
> > called from lxc_setup(). If rootfs is NULL, then all bets are off anyway
> > and we can just call mount(). If rootfs is not NULL, then we're mounting
> > in a separate container rootfs and we should use safe_mount(), but we
> > expect the real /proc to then exist.
>
> In that case, would the following fix be more appropriate?
>
> @@ -1747,8 +1747,14 @@ int mount_proc_if_needed(const char *rootfs)
> return 0;
>
> domount:
> - if (safe_mount("proc", path, "proc", 0, NULL, rootfs) < 0)
> + if (!strcmp(rootfs,"")) /* rootfs is NULL */
> + ret = mount("proc", path, "proc", 0, NULL);
> + else
> + ret = safe_mount("proc", path, "proc", 0, NULL, rootfs);
> +
> + if (ret < 0)
> return -1;
> +
> INFO("Mounted /proc in container for security transition");
> return 1;
> }
>
> Btw checking for the rootfs to be NULL is done this way since
> mount_proc_if_needed is called like:
>
> mount_proc_if_needed(lxc_conf->rootfs.path ? lxc_conf->rootfs.mount : "");
I think that looks right, thanks. (I reserve the right to exclaim in a
few hours that I dumbly forgot a special case :)
Please make sure to have a comment above tmp_proc_mount and
mount_proc_if_needed saying that they must not be called from inside the
container's namespace.
-serge
_______________________________________________
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel
