[lxc-devel] [lxc/stable-1.1] Fix regressions in lxc 1.1.x for containers without rootfs
dpward on Github
lxc-bot at linuxcontainers.org
Thu Apr 28 17:31:36 UTC 2016
A non-text attachment was scrubbed...
Name: not available
Type: text/x-mailbox
Size: 620 bytes
Desc: not available
URL: <http://lists.linuxcontainers.org/pipermail/lxc-devel/attachments/20160428/1decb33d/attachment.bin>
-------------- next part --------------
From 0048c7faed1fe5f9037ac9fab4d2452abd41d753 Mon Sep 17 00:00:00 2001
From: Bogdan Purcareata <bogdan.purcareata at nxp.com>
Date: Fri, 8 Jan 2016 15:38:35 +0000
Subject: [PATCH 1/6] open_without_symlink: Account when prefix is empty string
In the current implementation, the open_without_symlink function
will default to opening the root mount only if the passed rootfs
prefix is null. It doesn't account for the case where this prefix
is passed as an empty string.
Properly handle this second case as well.
Signed-off-by: Bogdan Purcareata <bogdan.purcareata at nxp.com>
Acked-by: Serge E. Hallyn <serge.hallyn at ubuntu.com>
---
src/lxc/utils.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/lxc/utils.c b/src/lxc/utils.c
index dac6418..db6e4c8 100644
--- a/src/lxc/utils.c
+++ b/src/lxc/utils.c
@@ -1576,7 +1576,7 @@ static int open_without_symlink(const char *target, const char *prefix_skip)
fulllen = strlen(target);
/* make sure prefix-skip makes sense */
- if (prefix_skip) {
+ if (prefix_skip && strlen(prefix_skip) > 0) {
curlen = strlen(prefix_skip);
if (!is_subdir(target, prefix_skip, curlen)) {
ERROR("WHOA there - target '%s' didn't start with prefix '%s'",
From 731c02b4af650edf5f58bea7cb13db3805678723 Mon Sep 17 00:00:00 2001
From: Bogdan Purcareata <bogdan.purcareata at nxp.com>
Date: Wed, 20 Jan 2016 10:53:57 +0000
Subject: [PATCH 2/6] mount_proc_if_needed: only safe mount when rootfs is
defined
The safe_mount function was introduced in order to address CVE-2015-1335,
one of the vulnerabilities being a mount with a symlink for the
destination path. In scenarios such as lxc-execute with no rootfs, the
destination path is the host /proc, which is previously mounted by the
host, and is unmounted and mounted again in a new set of namespaces,
therefore eliminating the need to check for it being a symlink.
Mount the rootfs normally if the rootfs is NULL, keep the safe mount
only for scenarios where a different rootfs is defined.
Signed-off-by: Bogdan Purcareata <bogdan.purcareata at nxp.com>
Acked-by: Serge E. Hallyn <serge.hallyn at ubuntu.com>
---
src/lxc/conf.c | 1 +
src/lxc/utils.c | 10 +++++++++-
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/src/lxc/conf.c b/src/lxc/conf.c
index 55c2fae..034bdff 100644
--- a/src/lxc/conf.c
+++ b/src/lxc/conf.c
@@ -3669,6 +3669,7 @@ int ttys_shift_ids(struct lxc_conf *c)
return 0;
}
+/* NOTE: not to be called from inside the container namespace! */
int tmp_proc_mount(struct lxc_conf *lxc_conf)
{
int mounted;
diff --git a/src/lxc/utils.c b/src/lxc/utils.c
index db6e4c8..5c383d9 100644
--- a/src/lxc/utils.c
+++ b/src/lxc/utils.c
@@ -1705,6 +1705,8 @@ int safe_mount(const char *src, const char *dest, const char *fstype,
*
* Returns < 0 on failure, 0 if the correct proc was already mounted
* and 1 if a new proc was mounted.
+ *
+ * NOTE: not to be called from inside the container namespace!
*/
int mount_proc_if_needed(const char *rootfs)
{
@@ -1738,8 +1740,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;
}
From ac390be799594408133389b3e72f4187870e5cc4 Mon Sep 17 00:00:00 2001
From: Christian Brauner <christian.brauner at mailbox.org>
Date: Tue, 2 Feb 2016 14:43:33 +0100
Subject: [PATCH 3/6] Fix NULL-ptr derefs for container without rootfs
Since we allow containers to be created without a rootfs most checks in conf.c
are not sane anymore. Instead of just checking if rootfs->path != NULL we need
to check whether rootfs != NULL.
Minor fixes:
- Have mount_autodev() always return -1 on failure: mount_autodev() returns 0
on success and -1 on failure. But when the return value of safe_mount() was
checked in mount_autodev() we returned false (instead of -1) which caused
mount_autodev() to return 0 (success) instead of the correct -1 (failure).
Signed-off-by: Christian Brauner <christian.brauner at mailbox.org>
[David Ward: resolved trivial merge conflicts with stable-1.1]
Signed-off-by: David Ward <david.ward at ll.mit.edu>
---
src/lxc/conf.c | 48 ++++++++++++++++++++++++++++++++++--------------
1 file changed, 34 insertions(+), 14 deletions(-)
diff --git a/src/lxc/conf.c b/src/lxc/conf.c
index 034bdff..b4dd70b 100644
--- a/src/lxc/conf.c
+++ b/src/lxc/conf.c
@@ -948,10 +948,15 @@ static int setup_dev_symlinks(const struct lxc_rootfs *rootfs)
int ret,i;
struct stat s;
+ /* rootfs struct will be empty when container is created without rootfs. */
+ char *rootfs_path = NULL;
+ if (rootfs && rootfs->path)
+ rootfs_path = rootfs->mount;
+
for (i = 0; i < sizeof(dev_symlinks) / sizeof(dev_symlinks[0]); i++) {
const struct dev_symlinks *d = &dev_symlinks[i];
- ret = snprintf(path, sizeof(path), "%s/dev/%s", rootfs->path ? rootfs->mount : "", d->name);
+ ret = snprintf(path, sizeof(path), "%s/dev/%s", rootfs_path ? rootfs_path : "", d->name);
if (ret < 0 || ret >= MAXPATHLEN)
return -1;
@@ -1154,13 +1159,18 @@ static int mount_autodev(const char *name, const struct lxc_rootfs *rootfs, cons
size_t clen;
char *path;
+ /* rootfs struct will be empty when container is created without rootfs. */
+ char *rootfs_path = NULL;
+ if (rootfs && rootfs->path)
+ rootfs_path = rootfs->mount;
+
INFO("Mounting container /dev");
/* $(rootfs->mount) + "/dev/pts" + '\0' */
- clen = (rootfs->path ? strlen(rootfs->mount) : 0) + 9;
+ clen = (rootfs_path ? strlen(rootfs_path) : 0) + 9;
path = alloca(clen);
- ret = snprintf(path, clen, "%s/dev", rootfs->path ? rootfs->mount : "");
+ ret = snprintf(path, clen, "%s/dev", rootfs_path ? rootfs_path : "");
if (ret < 0 || ret >= clen)
return -1;
@@ -1170,15 +1180,16 @@ static int mount_autodev(const char *name, const struct lxc_rootfs *rootfs, cons
return 0;
}
- if (safe_mount("none", path, "tmpfs", 0, "size=100000,mode=755",
- rootfs->path ? rootfs->mount : NULL)) {
+ ret = safe_mount("none", path, "tmpfs", 0, "size=100000,mode=755",
+ rootfs_path);
+ if (ret != 0) {
SYSERROR("Failed mounting tmpfs onto %s\n", path);
- return false;
+ return -1;
}
INFO("Mounted tmpfs onto %s", path);
- ret = snprintf(path, clen, "%s/dev/pts", rootfs->path ? rootfs->mount : "");
+ ret = snprintf(path, clen, "%s/dev/pts", rootfs_path ? rootfs_path : "");
if (ret < 0 || ret >= clen)
return -1;
@@ -1222,9 +1233,14 @@ static int fill_autodev(const struct lxc_rootfs *rootfs)
int i;
mode_t cmask;
+ /* rootfs struct will be empty when container is created without rootfs. */
+ char *rootfs_path = NULL;
+ if (rootfs && rootfs->path)
+ rootfs_path = rootfs->mount;
+
INFO("Creating initial consoles under container /dev");
- ret = snprintf(path, MAXPATHLEN, "%s/dev", rootfs->path ? rootfs->mount : "");
+ ret = snprintf(path, MAXPATHLEN, "%s/dev", rootfs_path ? rootfs_path : "");
if (ret < 0 || ret >= MAXPATHLEN) {
ERROR("Error calculating container /dev location");
return -1;
@@ -1237,7 +1253,7 @@ static int fill_autodev(const struct lxc_rootfs *rootfs)
cmask = umask(S_IXUSR | S_IXGRP | S_IXOTH);
for (i = 0; i < sizeof(lxc_devs) / sizeof(lxc_devs[0]); i++) {
const struct lxc_devs *d = &lxc_devs[i];
- ret = snprintf(path, MAXPATHLEN, "%s/dev/%s", rootfs->path ? rootfs->mount : "", d->name);
+ ret = snprintf(path, MAXPATHLEN, "%s/dev/%s", rootfs_path ? rootfs_path : "", d->name);
if (ret < 0 || ret >= MAXPATHLEN)
return -1;
ret = mknod(path, d->mode, makedev(d->maj, d->min));
@@ -1257,7 +1273,7 @@ static int fill_autodev(const struct lxc_rootfs *rootfs)
}
fclose(pathfile);
if (safe_mount(hostpath, path, 0, MS_BIND, NULL,
- rootfs->path ? rootfs->mount : NULL) != 0) {
+ rootfs_path ? rootfs_path : NULL) != 0) {
SYSERROR("Failed bind mounting device %s from host into container",
d->name);
return -1;
@@ -1868,7 +1884,7 @@ static int mount_entry_create_overlay_dirs(const struct mntent *mntent,
size_t len = 0;
size_t rootfslen = 0;
- if (!rootfs->path || !lxc_name || !lxc_path)
+ if (!rootfs || !rootfs->path || !lxc_name || !lxc_path)
goto err;
opts = lxc_string_split(mntent->mnt_opts, ',');
@@ -1934,7 +1950,7 @@ static int mount_entry_create_aufs_dirs(const struct mntent *mntent,
size_t len = 0;
size_t rootfslen = 0;
- if (!rootfs->path || !lxc_name || !lxc_path)
+ if (!rootfs || !rootfs->path || !lxc_name || !lxc_path)
goto err;
opts = lxc_string_split(mntent->mnt_opts, ',');
@@ -2040,9 +2056,13 @@ static inline int mount_entry_on_generic(struct mntent *mntent,
return -1;
}
+ /* rootfs struct will be empty when container is created without rootfs. */
+ char *rootfs_path = NULL;
+ if (rootfs && rootfs->path)
+ rootfs_path = rootfs->mount;
+
ret = mount_entry(mntent->mnt_fsname, path, mntent->mnt_type, mntflags,
- mntdata, optional,
- rootfs->path ? rootfs->mount : NULL);
+ mntdata, optional, rootfs_path);
free(mntdata);
return ret;
From 00a8ae1c9a26c0555bd4be1c53dead7400dd0921 Mon Sep 17 00:00:00 2001
From: Christian Brauner <christian.brauner at mailbox.org>
Date: Tue, 2 Feb 2016 22:13:07 +0100
Subject: [PATCH 4/6] Fix mount_entry_on_generic()
In mount_entry_on_generic() we dereferenced a NULL pointer whenever a container
without a rootfs was created. (Since mount_entry_on_systemfs() passes them with
NULL.) We have mount_entry_on_generic() check whether rootfs != NULL.
We also check whether rootfs != NULL in the functions ovl_mkdir() and
mount_entry_create_aufs_dirs() and bail immediately. Rationale: For overlay and
aufs lxc.mount.entry entries users give us absolute paths to e.g. workdir and
upperdir which we create for them. We currently use rootfs->path and the
lxcpath for the container to check that users give us a sane path to create
those directories under and refuse if they do not. If we want to allow overlay
mounts for containers without a rootfs they can easily be reworked.
Signed-off-by: Christian Brauner <christian.brauner at mailbox.org>
[David Ward: resolved trivial merge conflicts with stable-1.1]
Signed-off-by: David Ward <david.ward at ll.mit.edu>
---
src/lxc/conf.c | 53 ++++++++++++++++++++++-------------------------------
1 file changed, 22 insertions(+), 31 deletions(-)
diff --git a/src/lxc/conf.c b/src/lxc/conf.c
index b4dd70b..0358f43 100644
--- a/src/lxc/conf.c
+++ b/src/lxc/conf.c
@@ -948,15 +948,10 @@ static int setup_dev_symlinks(const struct lxc_rootfs *rootfs)
int ret,i;
struct stat s;
- /* rootfs struct will be empty when container is created without rootfs. */
- char *rootfs_path = NULL;
- if (rootfs && rootfs->path)
- rootfs_path = rootfs->mount;
-
for (i = 0; i < sizeof(dev_symlinks) / sizeof(dev_symlinks[0]); i++) {
const struct dev_symlinks *d = &dev_symlinks[i];
- ret = snprintf(path, sizeof(path), "%s/dev/%s", rootfs_path ? rootfs_path : "", d->name);
+ ret = snprintf(path, sizeof(path), "%s/dev/%s", rootfs->path ? rootfs->mount : "", d->name);
if (ret < 0 || ret >= MAXPATHLEN)
return -1;
@@ -1159,18 +1154,13 @@ static int mount_autodev(const char *name, const struct lxc_rootfs *rootfs, cons
size_t clen;
char *path;
- /* rootfs struct will be empty when container is created without rootfs. */
- char *rootfs_path = NULL;
- if (rootfs && rootfs->path)
- rootfs_path = rootfs->mount;
-
INFO("Mounting container /dev");
/* $(rootfs->mount) + "/dev/pts" + '\0' */
- clen = (rootfs_path ? strlen(rootfs_path) : 0) + 9;
+ clen = (rootfs->path ? strlen(rootfs->mount) : 0) + 9;
path = alloca(clen);
- ret = snprintf(path, clen, "%s/dev", rootfs_path ? rootfs_path : "");
+ ret = snprintf(path, clen, "%s/dev", rootfs->path ? rootfs->mount : "");
if (ret < 0 || ret >= clen)
return -1;
@@ -1181,7 +1171,7 @@ static int mount_autodev(const char *name, const struct lxc_rootfs *rootfs, cons
}
ret = safe_mount("none", path, "tmpfs", 0, "size=100000,mode=755",
- rootfs_path);
+ rootfs->path ? rootfs->mount : NULL);
if (ret != 0) {
SYSERROR("Failed mounting tmpfs onto %s\n", path);
return -1;
@@ -1189,7 +1179,7 @@ static int mount_autodev(const char *name, const struct lxc_rootfs *rootfs, cons
INFO("Mounted tmpfs onto %s", path);
- ret = snprintf(path, clen, "%s/dev/pts", rootfs_path ? rootfs_path : "");
+ ret = snprintf(path, clen, "%s/dev/pts", rootfs->path ? rootfs->mount : "");
if (ret < 0 || ret >= clen)
return -1;
@@ -1233,14 +1223,9 @@ static int fill_autodev(const struct lxc_rootfs *rootfs)
int i;
mode_t cmask;
- /* rootfs struct will be empty when container is created without rootfs. */
- char *rootfs_path = NULL;
- if (rootfs && rootfs->path)
- rootfs_path = rootfs->mount;
-
INFO("Creating initial consoles under container /dev");
- ret = snprintf(path, MAXPATHLEN, "%s/dev", rootfs_path ? rootfs_path : "");
+ ret = snprintf(path, MAXPATHLEN, "%s/dev", rootfs->path ? rootfs->mount : "");
if (ret < 0 || ret >= MAXPATHLEN) {
ERROR("Error calculating container /dev location");
return -1;
@@ -1253,7 +1238,7 @@ static int fill_autodev(const struct lxc_rootfs *rootfs)
cmask = umask(S_IXUSR | S_IXGRP | S_IXOTH);
for (i = 0; i < sizeof(lxc_devs) / sizeof(lxc_devs[0]); i++) {
const struct lxc_devs *d = &lxc_devs[i];
- ret = snprintf(path, MAXPATHLEN, "%s/dev/%s", rootfs_path ? rootfs_path : "", d->name);
+ ret = snprintf(path, MAXPATHLEN, "%s/dev/%s", rootfs->path ? rootfs->mount : "", d->name);
if (ret < 0 || ret >= MAXPATHLEN)
return -1;
ret = mknod(path, d->mode, makedev(d->maj, d->min));
@@ -1273,7 +1258,7 @@ static int fill_autodev(const struct lxc_rootfs *rootfs)
}
fclose(pathfile);
if (safe_mount(hostpath, path, 0, MS_BIND, NULL,
- rootfs_path ? rootfs_path : NULL) != 0) {
+ rootfs->path ? rootfs->mount : NULL) != 0) {
SYSERROR("Failed bind mounting device %s from host into container",
d->name);
return -1;
@@ -1884,6 +1869,9 @@ static int mount_entry_create_overlay_dirs(const struct mntent *mntent,
size_t len = 0;
size_t rootfslen = 0;
+ /* Since we use all of these to check whether the user has given us a
+ * sane absolute path to create the directories needed for overlay
+ * lxc.mount.entry entries we consider any of these missing fatal. */
if (!rootfs || !rootfs->path || !lxc_name || !lxc_path)
goto err;
@@ -1950,6 +1938,9 @@ static int mount_entry_create_aufs_dirs(const struct mntent *mntent,
size_t len = 0;
size_t rootfslen = 0;
+ /* Since we use all of these to check whether the user has given us a
+ * sane absolute path to create the directories needed for overlay
+ * lxc.mount.entry entries we consider any of these missing fatal. */
if (!rootfs || !rootfs->path || !lxc_name || !lxc_path)
goto err;
@@ -1993,7 +1984,6 @@ static int mount_entry_create_aufs_dirs(const struct mntent *mntent,
return fret;
}
-
static int mount_entry_create_dir_file(const struct mntent *mntent,
const char* path, const struct lxc_rootfs *rootfs,
const char *lxc_name, const char *lxc_path)
@@ -2035,6 +2025,8 @@ static int mount_entry_create_dir_file(const struct mntent *mntent,
return ret;
}
+/* rootfs, lxc_name, and lxc_path can be NULL when the container is created
+ * without a rootfs. */
static inline int mount_entry_on_generic(struct mntent *mntent,
const char* path, const struct lxc_rootfs *rootfs,
const char *lxc_name, const char *lxc_path)
@@ -2044,6 +2036,10 @@ static inline int mount_entry_on_generic(struct mntent *mntent,
int ret;
bool optional = hasmntopt(mntent, "optional") != NULL;
+ char *rootfs_path = NULL;
+ if (rootfs && rootfs->path)
+ rootfs_path = rootfs->mount;
+
ret = mount_entry_create_dir_file(mntent, path, rootfs, lxc_name, lxc_path);
if (ret < 0)
@@ -2056,13 +2052,8 @@ static inline int mount_entry_on_generic(struct mntent *mntent,
return -1;
}
- /* rootfs struct will be empty when container is created without rootfs. */
- char *rootfs_path = NULL;
- if (rootfs && rootfs->path)
- rootfs_path = rootfs->mount;
-
ret = mount_entry(mntent->mnt_fsname, path, mntent->mnt_type, mntflags,
- mntdata, optional, rootfs_path);
+ mntdata, optional, rootfs_path);
free(mntdata);
return ret;
@@ -2070,7 +2061,7 @@ static inline int mount_entry_on_generic(struct mntent *mntent,
static inline int mount_entry_on_systemfs(struct mntent *mntent)
{
- return mount_entry_on_generic(mntent, mntent->mnt_dir, NULL, NULL, NULL);
+ return mount_entry_on_generic(mntent, mntent->mnt_dir, NULL, NULL, NULL);
}
static int mount_entry_on_absolute_rootfs(struct mntent *mntent,
From 326599846379d590ca738a6ff6521ee630fe970c Mon Sep 17 00:00:00 2001
From: Christian Brauner <christian.brauner at mailbox.org>
Date: Wed, 3 Feb 2016 13:17:51 +0100
Subject: [PATCH 5/6] no rootfs => mounts are always relative to hosts /
All lxc.mount.entry entries will be relative to the hosts / when a container
does not specify a lxc.rootfs.
Signed-off-by: Christian Brauner <christian.brauner at mailbox.org>
---
src/lxc/conf.c | 19 +++++++++++++++++--
1 file changed, 17 insertions(+), 2 deletions(-)
diff --git a/src/lxc/conf.c b/src/lxc/conf.c
index 0358f43..d3ac28a 100644
--- a/src/lxc/conf.c
+++ b/src/lxc/conf.c
@@ -2061,7 +2061,22 @@ static inline int mount_entry_on_generic(struct mntent *mntent,
static inline int mount_entry_on_systemfs(struct mntent *mntent)
{
- return mount_entry_on_generic(mntent, mntent->mnt_dir, NULL, NULL, NULL);
+ char path[MAXPATHLEN];
+ int ret;
+
+ /* For containers created without a rootfs all mounts are treated as
+ * absolute paths starting at / on the host. */
+ if (mntent->mnt_dir[0] != '/')
+ ret = snprintf(path, sizeof(path), "/%s", mntent->mnt_dir);
+ else
+ ret = snprintf(path, sizeof(path), "%s", mntent->mnt_dir);
+
+ if (ret < 0 || ret >= sizeof(path)) {
+ ERROR("path name too long");
+ return -1;
+ }
+
+ return mount_entry_on_generic(mntent, path, NULL, NULL, NULL);
}
static int mount_entry_on_absolute_rootfs(struct mntent *mntent,
@@ -2122,7 +2137,7 @@ static int mount_entry_on_relative_rootfs(struct mntent *mntent,
/* relative to root mount point */
ret = snprintf(path, sizeof(path), "%s/%s", rootfs->mount, mntent->mnt_dir);
- if (ret >= sizeof(path)) {
+ if (ret < 0 || ret >= sizeof(path)) {
ERROR("path name too long");
return -1;
}
From db924c3132efc8846d4c0b99483e5acd6a97cf5e Mon Sep 17 00:00:00 2001
From: Christian Brauner <christian.brauner at mailbox.org>
Date: Wed, 23 Mar 2016 16:37:09 +0100
Subject: [PATCH 6/6] open_without_symlink: Don't SYSERROR on something else
than ELOOP
The open_without_symlink routine has been specifically created to prevent
mounts with synlinks as source or destination. Keep SYSERROR'ing in that
particular scenario, but leave error handling to calling functions for the
other ones - e.g. optional bind mount when the source dir doesn't exist
throws a nasty error.
Signed-off-by: Bogdan Purcareata <bogdan.purcareata at nxp.com>
---
src/lxc/utils.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/src/lxc/utils.c b/src/lxc/utils.c
index 5c383d9..7b61c54 100644
--- a/src/lxc/utils.c
+++ b/src/lxc/utils.c
@@ -1622,8 +1622,6 @@ static int open_without_symlink(const char *target, const char *prefix_skip)
errno = saved_errno;
if (errno == ELOOP)
SYSERROR("%s in %s was a symbolic link!", nextpath, target);
- else
- SYSERROR("Error examining %s in %s", nextpath, target);
goto out;
}
}
@@ -1668,8 +1666,11 @@ int safe_mount(const char *src, const char *dest, const char *fstype,
destfd = open_without_symlink(dest, rootfs);
if (destfd < 0) {
- if (srcfd != -1)
+ if (srcfd != -1) {
+ saved_errno = errno;
close(srcfd);
+ errno = saved_errno;
+ }
return destfd;
}
More information about the lxc-devel
mailing list