[lxc-devel] [PATCH v2] safe_mount: Handle mounting proc and refactor
Bogdan Purcareata
bogdan.purcareata at nxp.com
Thu Jan 14 11:45:23 UTC 2016
On 14.01.2016 01:09, Serge Hallyn wrote:
> Quoting Bogdan Purcareata (bogdan.purcareata at nxp.com):
>> On 11.01.2016 20:59, Serge Hallyn wrote:
>>> 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)
>> 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 : "");
Thanks!
Bogdan P.
More information about the lxc-devel
mailing list