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

[Serge Hallyn]

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.
> 
> Signed-off-by: Bogdan Purcareata <bogdan.purcareata at nxp.com>
> ---
>  src/lxc/utils.c | 53 ++++++++++++++++++++++++++++++++---------------------
>  1 file changed, 32 insertions(+), 21 deletions(-)
> 
> diff --git a/src/lxc/utils.c b/src/lxc/utils.c
> index 8fa7e6b..f080a18 100644
> --- a/src/lxc/utils.c
> +++ b/src/lxc/utils.c
> @@ -1638,56 +1638,67 @@ 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 = "";
>  
> +	/* in case we're mounting /proc w/o a rootfs path, it has previously been
> +	 * unmounted, therefore /proc/self/fd entries no longer exist */
> +	if (strlen(rootfs) == 0 && !strncmp(fstype, "proc", 4))
> +		goto do_mount;

But you're not "handling" this case, you're ignoring it.  So if the
container has /proc as a symlink (which was precisely one of the
example exploits) this will fall prey to that.

>  	/* todo - allow symlinks for relative paths if 'allowsymlinks' option is passed */
>  	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);
> +	mntdest = destbuf;
> +
> +do_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)
> +		close(destfd);
> +out_src:
> +	if (srcfd > 0)
> +		close(srcfd);
> +out:
> +	return ret;
>  }
>  
>  /*
> -- 
> 2.1.4
> 
> _______________________________________________
> lxc-devel mailing list
> lxc-devel at lists.linuxcontainers.org
> http://lists.linuxcontainers.org/listinfo/lxc-devel