Skip to content
Permalink
Browse files Browse the repository at this point in the history
CVE-2015-1335: Protect container mounts against symlinks
When a container starts up, lxc sets up the container's inital fstree
by doing a bunch of mounting, guided by the container configuration
file.  The container config is owned by the admin or user on the host,
so we do not try to guard against bad entries.  However, since the
mount target is in the container, it's possible that the container admin
could divert the mount with symbolic links.  This could bypass proper
container startup (i.e. confinement of a root-owned container by the
restrictive apparmor policy, by diverting the required write to
/proc/self/attr/current), or bypass the (path-based) apparmor policy
by diverting, say, /proc to /mnt in the container.

To prevent this,

1. do not allow mounts to paths containing symbolic links

2. do not allow bind mounts from relative paths containing symbolic
links.

Details:

Define safe_mount which ensures that the container has not inserted any
symbolic links into any mount targets for mounts to be done during
container setup.

The host's mount path may contain symbolic links.  As it is under the
control of the administrator, that's ok.  So safe_mount begins the check
for symbolic links after the rootfs->mount, by opening that directory.

It opens each directory along the path using openat() relative to the
parent directory using O_NOFOLLOW.  When the target is reached, it
mounts onto /proc/self/fd/<targetfd>.

Use safe_mount() in mount_entry(), when mounting container proc,
and when needed.  In particular, safe_mount() need not be used in
any case where:

1. the mount is done in the container's namespace
2. the mount is for the container's rootfs
3. the mount is relative to a tmpfs or proc/sysfs which we have
   just safe_mount()ed ourselves

Since we were using proc/net as a temporary placeholder for /proc/sys/net
during container startup, and proc/net is a symbolic link, use proc/tty
instead.

Update the lxc.container.conf manpage with details about the new
restrictions.

Finally, add a testcase to test some symbolic link possibilities.

Reported-by: Roman Fiedler
Signed-off-by: Serge Hallyn <serge.hallyn@ubuntu.com>
Acked-by: Stéphane Graber <stgraber@ubuntu.com>
  • Loading branch information
hallyn authored and stgraber committed Sep 29, 2015
1 parent f2e4ddd commit 592fd47
Show file tree
Hide file tree
Showing 8 changed files with 363 additions and 20 deletions.
12 changes: 12 additions & 0 deletions doc/lxc.container.conf.sgml.in
Expand Up @@ -760,6 +760,18 @@ Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
container. This is useful to mount /etc, /var or /home for
examples.
</para>
<para>
NOTE - LXC will generally ensure that mount targets and relative
bind-mount sources are properly confined under the container
root, to avoid attacks involving over-mounting host directories
and files. (Symbolic links in absolute mount sources are ignored)
However, if the container configuration first mounts a directory which
is under the control of the container user, such as /home/joe, into
the container at some <filename>path</filename>, and then mounts
under <filename>path</filename>, then a TOCTTOU attack would be
possible where the container user modifies a symbolic link under
his home directory at just the right time.
</para>
<variablelist>
<varlistentry>
<term>
Expand Down
5 changes: 4 additions & 1 deletion src/lxc/cgfs.c
Expand Up @@ -1363,7 +1363,10 @@ static bool cgroupfs_mount_cgroup(void *hdata, const char *root, int type)
if (!path)
return false;
snprintf(path, bufsz, "%s/sys/fs/cgroup", root);
r = mount("cgroup_root", path, "tmpfs", MS_NOSUID|MS_NODEV|MS_NOEXEC|MS_RELATIME, "size=10240k,mode=755");
r = safe_mount("cgroup_root", path, "tmpfs",
MS_NOSUID|MS_NODEV|MS_NOEXEC|MS_RELATIME,
"size=10240k,mode=755",
root);
if (r < 0) {
SYSERROR("could not mount tmpfs to /sys/fs/cgroup in the container");
return false;
Expand Down
4 changes: 2 additions & 2 deletions src/lxc/cgmanager.c
Expand Up @@ -1477,7 +1477,7 @@ static bool cgm_bind_dir(const char *root, const char *dirname)
}

/* mount a tmpfs there so we can create subdirs */
if (mount("cgroup", cgpath, "tmpfs", 0, "size=10000,mode=755")) {
if (safe_mount("cgroup", cgpath, "tmpfs", 0, "size=10000,mode=755", root)) {
SYSERROR("Failed to mount tmpfs at %s", cgpath);
return false;
}
Expand All @@ -1488,7 +1488,7 @@ static bool cgm_bind_dir(const char *root, const char *dirname)
return false;
}

if (mount(dirname, cgpath, "none", MS_BIND, 0)) {
if (safe_mount(dirname, cgpath, "none", MS_BIND, 0, root)) {
SYSERROR("Failed to bind mount %s to %s", dirname, cgpath);
return false;
}
Expand Down
35 changes: 19 additions & 16 deletions src/lxc/conf.c
Expand Up @@ -769,10 +769,11 @@ static int lxc_mount_auto_mounts(struct lxc_conf *conf, int flags, struct lxc_ha
* 2.6.32...
*/
{ LXC_AUTO_PROC_MASK, LXC_AUTO_PROC_MIXED, "proc", "%r/proc", "proc", MS_NODEV|MS_NOEXEC|MS_NOSUID, NULL },
{ LXC_AUTO_PROC_MASK, LXC_AUTO_PROC_MIXED, "%r/proc/sys/net", "%r/proc/net", NULL, MS_BIND, NULL },
/* proc/tty is used as a temporary placeholder for proc/sys/net which we'll move back in a few steps */
{ LXC_AUTO_PROC_MASK, LXC_AUTO_PROC_MIXED, "%r/proc/sys/net", "%r/proc/tty", NULL, MS_BIND, NULL },
{ LXC_AUTO_PROC_MASK, LXC_AUTO_PROC_MIXED, "%r/proc/sys", "%r/proc/sys", NULL, MS_BIND, NULL },
{ LXC_AUTO_PROC_MASK, LXC_AUTO_PROC_MIXED, NULL, "%r/proc/sys", NULL, MS_REMOUNT|MS_BIND|MS_RDONLY, NULL },
{ LXC_AUTO_PROC_MASK, LXC_AUTO_PROC_MIXED, "%r/proc/net", "%r/proc/sys/net", NULL, MS_MOVE, NULL },
{ LXC_AUTO_PROC_MASK, LXC_AUTO_PROC_MIXED, "%r/proc/tty", "%r/proc/sys/net", NULL, MS_MOVE, NULL },
{ LXC_AUTO_PROC_MASK, LXC_AUTO_PROC_MIXED, "%r/proc/sysrq-trigger", "%r/proc/sysrq-trigger", NULL, MS_BIND, NULL },
{ LXC_AUTO_PROC_MASK, LXC_AUTO_PROC_MIXED, NULL, "%r/proc/sysrq-trigger", NULL, MS_REMOUNT|MS_BIND|MS_RDONLY, NULL },
{ LXC_AUTO_PROC_MASK, LXC_AUTO_PROC_RW, "proc", "%r/proc", "proc", MS_NODEV|MS_NOEXEC|MS_NOSUID, NULL },
Expand Down Expand Up @@ -815,7 +816,7 @@ static int lxc_mount_auto_mounts(struct lxc_conf *conf, int flags, struct lxc_ha
}
mflags = add_required_remount_flags(source, destination,
default_mounts[i].flags);
r = mount(source, destination, default_mounts[i].fstype, mflags, default_mounts[i].options);
r = safe_mount(source, destination, default_mounts[i].fstype, mflags, default_mounts[i].options, conf->rootfs.path ? conf->rootfs.mount : NULL);
saved_errno = errno;
if (r < 0 && errno == ENOENT) {
INFO("Mount source or target for %s on %s doesn't exist. Skipping.", source, destination);
Expand Down Expand Up @@ -1167,7 +1168,8 @@ static int mount_autodev(const char *name, const struct lxc_rootfs *rootfs, cons
return 0;
}

if (mount("none", path, "tmpfs", 0, "size=100000,mode=755")) {
if (safe_mount("none", path, "tmpfs", 0, "size=100000,mode=755",
rootfs->path ? rootfs->mount : NULL)) {
SYSERROR("Failed mounting tmpfs onto %s\n", path);
return false;
}
Expand Down Expand Up @@ -1252,7 +1254,8 @@ static int fill_autodev(const struct lxc_rootfs *rootfs)
return -1;
}
fclose(pathfile);
if (mount(hostpath, path, 0, MS_BIND, NULL) != 0) {
if (safe_mount(hostpath, path, 0, MS_BIND, NULL,
rootfs->path ? rootfs->mount : NULL) != 0) {
SYSERROR("Failed bind mounting device %s from host into container",
d->name);
return -1;
Expand Down Expand Up @@ -1505,7 +1508,7 @@ static int setup_dev_console(const struct lxc_rootfs *rootfs,
return -1;
}

if (mount(console->name, path, "none", MS_BIND, 0)) {
if (safe_mount(console->name, path, "none", MS_BIND, 0, rootfs->mount)) {
ERROR("failed to mount '%s' on '%s'", console->name, path);
return -1;
}
Expand Down Expand Up @@ -1560,7 +1563,7 @@ static int setup_ttydir_console(const struct lxc_rootfs *rootfs,
return 0;
}

if (mount(console->name, lxcpath, "none", MS_BIND, 0)) {
if (safe_mount(console->name, lxcpath, "none", MS_BIND, 0, rootfs->mount)) {
ERROR("failed to mount '%s' on '%s'", console->name, lxcpath);
return -1;
}
Expand Down Expand Up @@ -1710,13 +1713,13 @@ static char *get_field(char *src, int nfields)

static int mount_entry(const char *fsname, const char *target,
const char *fstype, unsigned long mountflags,
const char *data, int optional)
const char *data, int optional, const char *rootfs)
{
#ifdef HAVE_STATVFS
struct statvfs sb;
#endif

if (mount(fsname, target, fstype, mountflags & ~MS_REMOUNT, data)) {
if (safe_mount(fsname, target, fstype, mountflags & ~MS_REMOUNT, data, rootfs)) {
if (optional) {
INFO("failed to mount '%s' on '%s' (optional): %s", fsname,
target, strerror(errno));
Expand Down Expand Up @@ -1763,7 +1766,7 @@ static int mount_entry(const char *fsname, const char *target,
#endif

if (mount(fsname, target, fstype,
mountflags | MS_REMOUNT, data)) {
mountflags | MS_REMOUNT, data) < 0) {
if (optional) {
INFO("failed to mount '%s' on '%s' (optional): %s",
fsname, target, strerror(errno));
Expand Down Expand Up @@ -1843,7 +1846,7 @@ static int mount_entry_create_dir_file(const struct mntent *mntent,
}

static inline int mount_entry_on_generic(struct mntent *mntent,
const char* path)
const char* path, const char *rootfs)
{
unsigned long mntflags;
char *mntdata;
Expand All @@ -1863,7 +1866,7 @@ static inline int mount_entry_on_generic(struct mntent *mntent,
}

ret = mount_entry(mntent->mnt_fsname, path, mntent->mnt_type,
mntflags, mntdata, optional);
mntflags, mntdata, optional, rootfs);

free(mntdata);

Expand All @@ -1872,7 +1875,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);
return mount_entry_on_generic(mntent, mntent->mnt_dir, NULL);
}

static int mount_entry_on_absolute_rootfs(struct mntent *mntent,
Expand Down Expand Up @@ -1919,7 +1922,7 @@ static int mount_entry_on_absolute_rootfs(struct mntent *mntent,
return -1;
}

return mount_entry_on_generic(mntent, path);
return mount_entry_on_generic(mntent, path, rootfs->mount);
}

static int mount_entry_on_relative_rootfs(struct mntent *mntent,
Expand All @@ -1935,7 +1938,7 @@ static int mount_entry_on_relative_rootfs(struct mntent *mntent,
return -1;
}

return mount_entry_on_generic(mntent, path);
return mount_entry_on_generic(mntent, path, rootfs);
}

static int mount_file_entries(const struct lxc_rootfs *rootfs, FILE *file,
Expand Down Expand Up @@ -3602,7 +3605,7 @@ void lxc_execute_bind_init(struct lxc_conf *conf)
fclose(pathfile);
}

ret = mount(path, destpath, "none", MS_BIND, NULL);
ret = safe_mount(path, destpath, "none", MS_BIND, NULL, conf->rootfs.mount);
if (ret < 0)
SYSERROR("Failed to bind lxc.init.static into container");
INFO("lxc.init.static bound into container at %s", path);
Expand Down

0 comments on commit 592fd47

Please sign in to comment.