[lxc-devel] [PATCH v2] safe_mount: Handle mounting proc and refactor

Serge Hallyn serge.hallyn at ubuntu.com
Mon Jan 11 18:59:23 UTC 2016


Quoting Bogdan Purcareata (bogdan.purcareata at 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.purcareata at 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 at lists.linuxcontainers.org
> http://lists.linuxcontainers.org/listinfo/lxc-devel


More information about the lxc-devel mailing list