path: root/main/lxc/CVE-2015-1335.patch

From 3c62cae308e8e66dcc616c5bd34671e1d2eea5a6 Mon Sep 17 00:00:00 2001
From: Serge Hallyn <serge.hallyn@ubuntu.com>
Date: Mon, 31 Aug 2015 12:57:20 -0500
Subject: [PATCH 1/1] 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:

This patch causes lxc to check /proc/self/mountinfo after each
mount into a container rootfs (that is, where we are not chrooted
into the container), making sure that the mount target wasn't a
symlink.

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.

lxc-test-symlink: background lxc-start

CVE-2015-1335

Signed-off-by: Serge Hallyn <serge.hallyn@ubuntu.com>
---
 doc/lxc.container.conf.sgml.in | 12 ++++++
 src/lxc/cgfs.c                 |  5 ++-
 src/lxc/cgmanager.c            |  4 +-
 src/lxc/conf.c                 | 30 +++++++-------
 src/lxc/utils.c                | 90 ++++++++++++++++++++++++++++++++++++++++++
 src/lxc/utils.h                |  2 +
 src/tests/Makefile.am          |  3 +-
 src/tests/lxc-test-symlink     | 88 +++++++++++++++++++++++++++++++++++++++++
 8 files changed, 216 insertions(+), 18 deletions(-)
 create mode 100644 src/tests/lxc-test-symlink

diff --git a/doc/lxc.container.conf.sgml.in b/doc/lxc.container.conf.sgml.in
index 9cd0c57..6f06762 100644
--- a/doc/lxc.container.conf.sgml.in
+++ b/doc/lxc.container.conf.sgml.in
@@ -676,6 +676,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>
diff --git a/src/lxc/cgfs.c b/src/lxc/cgfs.c
index 15346dc..df4ad46 100644
--- a/src/lxc/cgfs.c
+++ b/src/lxc/cgfs.c
@@ -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;
diff --git a/src/lxc/cgmanager.c b/src/lxc/cgmanager.c
index 1872f03..79af751 100644
--- a/src/lxc/cgmanager.c
+++ b/src/lxc/cgmanager.c
@@ -1332,7 +1332,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;
 	}
@@ -1343,7 +1343,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;
 	}
diff --git a/src/lxc/conf.c b/src/lxc/conf.c
index 320b6c9..8acee5a 100644
--- a/src/lxc/conf.c
+++ b/src/lxc/conf.c
@@ -795,7 +795,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)
 				SYSERROR("error mounting %s on %s flags %lu", source, destination, mflags);
@@ -989,7 +989,8 @@ static int setup_tty(const struct lxc_rootfs *rootfs,
 				return -1;
 			}
 
-			if (mount(pty_info->name, lxcpath, "none", MS_BIND, 0)) {
+			if (safe_mount(pty_info->name, lxcpath, "none", MS_BIND, 0,
+					rootfs->mount)) {
 				WARN("failed to mount '%s'->'%s'",
 				     pty_info->name, path);
 				continue;
@@ -1016,7 +1017,8 @@ static int setup_tty(const struct lxc_rootfs *rootfs,
 					close(ret);
 				}
 			}
-			if (mount(pty_info->name, path, "none", MS_BIND, 0)) {
+			if (safe_mount(pty_info->name, path, "none", MS_BIND, 0,
+					rootfs->mount)) {
 				WARN("failed to mount '%s'->'%s'",
 						pty_info->name, path);
 				continue;
@@ -1442,16 +1444,16 @@ static int mount_autodev(const char *name, char *root, const char *lxcpath)
 			SYSERROR("WARNING: Failed to create symlink '%s'->'%s'", host_path, devtmpfs_path);
 		}
 		DEBUG("Bind mounting %s to %s", devtmpfs_path , path );
-		ret = mount(devtmpfs_path, path, NULL, MS_BIND, 0 );
+		ret = safe_mount(devtmpfs_path, path, NULL, MS_BIND, 0, root );
 	} else {
 		/* Only mount a tmpfs on here if we don't already a mount */
 		if ( ! mount_check_fs( host_path, NULL ) ) {
 			DEBUG("Mounting tmpfs to %s", host_path );
-			ret = mount("none", path, "tmpfs", 0, "size=100000,mode=755");
+			ret = safe_mount("none", path, "tmpfs", 0, "size=100000,mode=755", root);
 		} else {
 			/* This allows someone to manually set up a mount */
 			DEBUG("Bind mounting %s to %s", host_path, path );
-			ret = mount(host_path , path, NULL, MS_BIND, 0 );
+			ret = safe_mount(host_path , path, NULL, MS_BIND, 0, root );
 		}
 	}
 	if (ret) {
@@ -1828,7 +1830,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;
 	}
@@ -1883,7 +1885,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;
 	}
@@ -2033,13 +2035,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));
@@ -2172,7 +2174,7 @@ static inline int mount_entry_on_systemfs(struct mntent *mntent)
 	}
 
 	ret = mount_entry(mntent->mnt_fsname, mntent->mnt_dir,
-			  mntent->mnt_type, mntflags, mntdata, optional);
+			  mntent->mnt_type, mntflags, mntdata, optional, NULL);
 
 	free(pathdirname);
 	free(mntdata);
@@ -2259,7 +2261,7 @@ skipabs:
 	}
 
 	ret = mount_entry(mntent->mnt_fsname, path, mntent->mnt_type,
-			  mntflags, mntdata, optional);
+			  mntflags, mntdata, optional, rootfs->mount);
 
 	free(mntdata);
 
@@ -2315,7 +2317,7 @@ static int mount_entry_on_relative_rootfs(struct mntent *mntent,
 	}
 
 	ret = mount_entry(mntent->mnt_fsname, path, mntent->mnt_type,
-			  mntflags, mntdata, optional);
+			  mntflags, mntdata, optional, rootfs);
 
 	free(pathdirname);
 	free(mntdata);
@@ -3981,7 +3983,7 @@ static int do_tmp_proc_mount(const char *rootfs)
 	return 0;
 
 domount:
-	if (mount("proc", path, "proc", 0, NULL))
+	if (safe_mount("proc", path, "proc", 0, NULL, rootfs))
 		return -1;
 	INFO("Mounted /proc in container for security transition");
 	return 1;
diff --git a/src/lxc/utils.c b/src/lxc/utils.c
index 5ef04fc..f84af33 100644
--- a/src/lxc/utils.c
+++ b/src/lxc/utils.c
@@ -1322,3 +1322,93 @@ next_loop:
 	free(path);
 	return NULL;
 }
+
+/*
+ * ws points into an array of \0-separate path elements.
+ * ws should be pointing to one of the path elements or
+ * the next \0.  It will return the first character of the
+ * next path element.
+ */
+static char *next_word(char *ws) {
+	while (*ws && *ws != ' ') ws++;
+	while (*ws && *ws == ' ') ws++;
+	return ws;
+}
+
+/*
+ * This is only used during container startup.  So we know we won't race
+ * with anyone else mounting.  Check the last line in /proc/self/mountinfo
+ * to make sure the target is under the container root.
+ */
+static bool ensure_not_symlink(const char *target, const char *croot)
+{
+	FILE *f = fopen("/proc/self/mountinfo", "r");
+	char *line = NULL, *ws = NULL, *we = NULL;
+	size_t len = 0, i;
+	bool ret = false;
+
+	if (!croot || croot[0] == '\0')
+		return true;
+
+	if (!f) {
+		ERROR("Cannot open /proc/self/mountinfo");
+		return false;
+	}
+
+	while (getline(&line, &len, f) != -1) {
+	}
+	fclose(f);
+
+	if (!line)
+		return false;
+	ws = line;
+	for (i = 0; i < 4; i++)
+		ws = next_word(ws);
+	if (!*ws)
+		goto out;
+	we = ws;
+	while (*we && *we != ' ')
+		we++;
+	if (!*we)
+		goto out;
+	*we = '\0';
+
+	/* now make sure that ws starts with croot and ends with rest of target */
+	if (croot && strncmp(ws, croot, strlen(croot)) != 0) {
+		ERROR("Mount onto %s resulted in %s\n", target, ws);
+		goto out;
+	}
+
+	size_t start = croot ? strlen(croot) : 0;
+	if (strcmp(ws + start, target + start) != 0) {
+		ERROR("Mount onto %s resulted in %s\n", target, ws);
+		goto out;
+	}
+
+	ret = true;
+
+out:
+	free(line);
+	return ret;
+}
+/*
+ * Safely mount a path into a container, ensuring that the mount target
+ * is under the container's @rootfs.  (If @rootfs is NULL, then the container
+ * uses the host's /)
+ */
+int safe_mount(const char *src, const char *dest, const char *fstype,
+		unsigned long flags, const void *data, const char *rootfs)
+{
+	int ret;
+	ret = mount(src, dest, fstype, flags, data);
+	if (ret < 0) {
+		SYSERROR("Mount of '%s' onto '%s' failed", src, dest);
+		return ret;
+	}
+	if (!ensure_not_symlink(dest, rootfs)) {
+		ERROR("Mount of '%s' onto '%s' was onto a symlink!", src, dest);
+		umount(dest);
+		return -1;
+	}
+	return 0;
+}
diff --git a/src/lxc/utils.h b/src/lxc/utils.h
index f48f403..30e8a98 100644
--- a/src/lxc/utils.h
+++ b/src/lxc/utils.h
@@ -280,3 +280,5 @@ uint64_t fnv_64a_buf(void *buf, size_t len, uint64_t hval);
 int detect_shared_rootfs(void);
 int detect_ramfs_rootfs(void);
 char *on_path(char *cmd);
+int safe_mount(const char *src, const char *dest, const char *fstype,
+		unsigned long flags, const void *data, const char *rootfs);
diff --git a/src/tests/Makefile.am b/src/tests/Makefile.am
index 19a4205..edb62d6 100644
--- a/src/tests/Makefile.am
+++ b/src/tests/Makefile.am
@@ -48,7 +48,7 @@ bin_PROGRAMS = lxc-test-containertests lxc-test-locktests lxc-test-startone \
 	lxc-test-reboot lxc-test-list lxc-test-attach lxc-test-device-add-remove \
 	lxc-test-apparmor
 
-bin_SCRIPTS = lxc-test-autostart
+bin_SCRIPTS = lxc-test-autostart lxc-test-symlink
 
 if DISTRO_UBUNTU
 bin_SCRIPTS += lxc-test-usernic lxc-test-ubuntu lxc-test-unpriv
@@ -71,6 +71,7 @@ EXTRA_DIST = \
 	locktests.c \
 	lxcpath.c \
 	lxc-test-autostart \
+	lxc-test-symlink \
 	lxc-test-ubuntu \
 	lxc-test-unpriv \
 	may_control.c \
diff --git a/src/tests/lxc-test-symlink b/src/tests/lxc-test-symlink
new file mode 100644
index 0000000..b014a66
--- /dev/null
+++ b/src/tests/lxc-test-symlink
@@ -0,0 +1,88 @@
+#!/bin/bash
+
+set -ex
+
+# lxc: linux Container library
+
+# Authors:
+# Serge Hallyn <serge.hallyn@ubuntu.com>
+#
+# This is a regression test for symbolic links
+
+dirname=`mktemp -d`
+fname=`mktemp`
+fname2=`mktemp`
+
+lxcpath=/var/lib/lxcsym1
+
+cleanup() {
+	lxc-destroy -P $lxcpath -f -n symtest1 || true
+	rm -f $lxcpath
+	rmdir $dirname || true
+	rm -f $fname || true
+	rm -f $fname2 || true
+}
+
+trap cleanup EXIT SIGHUP SIGINT SIGTERM
+
+testrun() {
+	expected=$1
+	run=$2
+	pass="pass"
+	lxc-start -d -P $lxcpath -n symtest1 -l trace -o $lxcpath/log || pass="fail"
+	[ $pass = "pass" ] && lxc-wait -P $lxcpath -n symtest1 -t 10 -s RUNNING || pass="fail"
+	if [ "$pass" != "$expected" ]; then
+		echo "Test $run: expected $expected but container did not.  Start log:"
+		cat $lxcpath/log
+		echo "FAIL: Test $run: expected $expected but container did not."
+		false
+	fi
+	lxc-stop -P $lxcpath -n symtest1 -k || true
+}
+
+# make lxcpath a symlink - this should NOT cause failure
+ln -s /var/lib/lxc $lxcpath
+
+lxc-destroy -P $lxcpath -f -n symtest1 || true
+lxc-create -P $lxcpath -t busybox -n symtest1
+
+cat >> /var/lib/lxc/symtest1/config << EOF
+lxc.mount.entry = $dirname opt/xxx/dir none bind,create=dir
+lxc.mount.entry = $fname opt/xxx/file none bind,create=file
+lxc.mount.entry = $fname2 opt/xxx/file2 none bind
+EOF
+
+# Regular - should succeed
+mkdir -p /var/lib/lxc/symtest1/rootfs/opt/xxx
+touch /var/lib/lxc/symtest1/rootfs/opt/xxx/file2
+testrun pass 1
+
+# symlink - should fail
+rm -rf /var/lib/lxc/symtest1/rootfs/opt/xxx
+mkdir -p /var/lib/lxc/symtest1/rootfs/opt/xxx2
+ln -s /var/lib/lxc/symtest1/rootfs/opt/xxx2 /var/lib/lxc/symtest1/rootfs/opt/xxx
+touch /var/lib/lxc/symtest1/rootfs/opt/xxx/file2
+testrun fail 2
+
+# final final symlink - should fail
+rm -rf $lxcpath/symtest1/rootfs/opt/xxx
+mkdir -p $lxcpath/symtest1/rootfs/opt/xxx
+mkdir -p $lxcpath/symtest1/rootfs/opt/xxx/dir
+touch $lxcpath/symtest1/rootfs/opt/xxx/file
+touch $lxcpath/symtest1/rootfs/opt/xxx/file2src
+ln -s $lxcpath/symtest1/rootfs/opt/xxx/file2src $lxcpath/symtest1/rootfs/opt/xxx/file2
+testrun fail 3
+
+# Ideally we'd also try a loop device, but that won't work in nested containers
+# anyway - TODO
+
+# what about /proc itself
+
+rm -rf $lxcpath/symtest1/rootfs/opt/xxx
+mkdir -p $lxcpath/symtest1/rootfs/opt/xxx
+touch $lxcpath/symtest1/rootfs/opt/xxx/file2
+mv $lxcpath/symtest1/rootfs/proc $lxcpath/symtest1/rootfs/proc1
+ln -s $lxcpath/symtest1/rootfs/proc1 $lxcpath/symtest1/rootfs/proc
+testrun fail 4
+
+echo "all tests passed"
-- 
2.5.0