From 089ad82736b7de040b7931040b397cd2891121c0 Mon Sep 17 00:00:00 2001 From: Serge Hallyn Date: Mon, 31 Aug 2015 12:57:20 -0500 Subject: [PATCH 1/1] Protect container mounts against symlinks 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/. 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 I believe I've covered every other case here. An extra pair of eyes would not be remiss! Finally, add a testcase to test some symbolic link possibilities. Changelog (Sep 2 2015): no need to safe_mount the second mount to target in mount_entry Changelog (Sep 16 2015): large update to fix regressions Signed-off-by: Serge Hallyn --- src/lxc/cgfs.c | 5 +- src/lxc/cgmanager.c | 4 +- src/lxc/conf.c | 35 ++++--- src/lxc/utils.c | 235 ++++++++++++++++++++++++++++++++++++++++++++- src/lxc/utils.h | 2 + src/tests/Makefile.am | 2 + src/tests/lxc-test-symlink | 90 +++++++++++++++++ 7 files changed, 353 insertions(+), 20 deletions(-) create mode 100755 src/tests/lxc-test-symlink diff --git a/src/lxc/cgfs.c b/src/lxc/cgfs.c index fcb3cde..df2e6b2 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 c143bea..779a1d8 100644 --- a/src/lxc/cgmanager.c +++ b/src/lxc/cgmanager.c @@ -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; } @@ -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; } diff --git a/src/lxc/conf.c b/src/lxc/conf.c index 6bc9ecb..6b746d2 100644 --- a/src/lxc/conf.c +++ b/src/lxc/conf.c @@ -763,10 +763,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 }, @@ -809,7 +810,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); @@ -1161,7 +1162,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; } @@ -1246,7 +1248,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; @@ -1499,7 +1502,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; } @@ -1554,7 +1557,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; } @@ -1704,13 +1707,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)); @@ -1757,7 +1760,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)); @@ -1837,7 +1840,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; @@ -1860,7 +1863,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); @@ -1869,7 +1872,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, @@ -1916,7 +1919,7 @@ skipabs: 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, @@ -1932,7 +1935,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, @@ -3599,7 +3602,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); diff --git a/src/lxc/utils.c b/src/lxc/utils.c index 7ced314..98160ba 100644 --- a/src/lxc/utils.c +++ b/src/lxc/utils.c @@ -1403,6 +1403,239 @@ int setproctitle(char *title) } /* + * @path: a pathname where / replaced with '\0'. + * @offsetp: pointer to int showing which path segment was last seen. + * Updated on return to reflect the next segment. + * @fulllen: full original path length. + * Returns a pointer to the next path segment, or NULL if done. + */ +static char *get_nextpath(char *path, int *offsetp, int fulllen) +{ + int offset = *offsetp; + + if (offset >= fulllen) + return NULL; + + while (path[offset] != '\0' && offset < fulllen) + offset++; + while (path[offset] == '\0' && offset < fulllen) + offset++; + + *offsetp = offset; + return (offset < fulllen) ? &path[offset] : NULL; +} + +/* + * Check that @subdir is a subdir of @dir. @len is the length of + * @dir (to avoid having to recalculate it). + */ +static bool is_subdir(const char *subdir, const char *dir, size_t len) +{ + size_t subdirlen = strlen(subdir); + + if (subdirlen < len) + return false; + if (strncmp(subdir, dir, len) != 0) + return false; + if (dir[len-1] == '/') + return true; + if (subdir[len] == '/' || subdirlen == len) + return true; + return false; +} + +int validate_symlink(int dirfd, const char *nextpath, const char *croot, + size_t croot_len) +{ + char destbuf[PATH_MAX], buf[PATH_MAX]; + int ret = -1; + size_t len; + + // it's a symlink, so take a look + len = readlinkat(dirfd, nextpath, buf, PATH_MAX); + if (len < 0) { + SYSERROR("Failed readlinkat for %s", nextpath); + if (errno == ENOENT) + return -ENOENT; + return -1; + } + + if (!realpath(buf, destbuf)) { + SYSERROR("Failed realpath on %s", buf); + if (errno == ENOENT) + return -ENOENT; + return -1; + } + + errno = EPERM; + if (!is_subdir(destbuf, croot, croot_len)) { + ERROR("is_subdir test failed for %s", destbuf); + goto out; + } + + ret = openat(dirfd, nextpath, O_RDONLY); + +out: + if (ret < 0) + ERROR("Mount target is a bad symbolic link: '%s'", nextpath); + return ret; +} + +/* + * Check if the open fd is a symlink. Return -ELOOP if it is. Return + * -ENOENT if we couldn't fstat. Return 0 if the fd is ok. + */ +static int check_symlink(int fd) +{ + struct stat sb; + int ret = fstat(fd, &sb); + if (ret < 0) + return -ENOENT; + if (S_ISLNK(sb.st_mode)) + return -ELOOP; + return 0; +} + +static int open_if_safe(int dirfd, const char *nextpath, const char *croot, + size_t croot_len) +{ + int newfd = openat(dirfd, nextpath, O_RDONLY | O_NOFOLLOW); + if (newfd >= 0) // was not a symlink, all good + return newfd; + + if (errno == ELOOP) + return validate_symlink(dirfd, nextpath, croot, croot_len); + + if (errno == EPERM || errno == EACCES) { + /* we're not root (cause we got EPERM) so + try opening with O_PATH */ + newfd = openat(dirfd, nextpath, O_PATH | O_NOFOLLOW); + if (newfd >= 0) { + /* O_PATH will return an fd for symlinks. We know + * nextpath wasn't a symlink at last openat, so if fd + * is now a link, then something * fishy is going on + */ + int ret = check_symlink(newfd); + if (ret < 0) { + close(newfd); + newfd = ret; + } + } + } + + return newfd; +} + +/* + * Open a path intending for mounting, ensuring there are no symbolic + * links in the path anywhere after 'prefix_skip' (if set). + * + * @target: path to be opened + * @prefix_skip: a part of @target in which to ignore symbolic links. This + * would be the container's rootfs. + * If there is an error opening it, return < 0 + * Otherwise returns an open fd for /a/b/c/d + */ +static int open_without_symlink(const char *target, const char *prefix_skip) +{ + int curlen = 0, dirfd, fulllen, i; + size_t plen = 0; + char *dup = NULL; + + fulllen = strlen(target); + + /* make sure prefix-skip makes sense */ + if (prefix_skip) { + curlen = strlen(prefix_skip); + if (!is_subdir(target, prefix_skip, curlen)) { + ERROR("WHOA there - target '%s' didn't start with prefix '%s'", + target, prefix_skip); + return -EINVAL; + } + /* + * get_nextpath() expects the curlen argument to be + * on a (turned into \0) / or before it, so decrement + * curlen to make sure that happens + */ + plen = curlen; + if (curlen) + curlen--; + } else { + prefix_skip = "/"; + plen = 1; + curlen = 0; + } + + /* Make a copy of target which we can hack up, and tokenize it */ + if ((dup = strdup(target)) == NULL) { + SYSERROR("Out of memory checking for symbolic link"); + return -ENOMEM; + } + for (i = 0; i < fulllen; i++) { + if (dup[i] == '/') + dup[i] = '\0'; + } + + dirfd = open(prefix_skip, O_RDONLY); + if (dirfd < 0) + goto out; + while (1) { + int newfd, saved_errno; + char *nextpath; + struct stat sb; + + if ((nextpath = get_nextpath(dup, &curlen, fulllen)) == NULL) + goto out; + newfd = open_if_safe(dirfd, nextpath, prefix_skip, plen); + saved_errno = errno; + close(dirfd); + dirfd = newfd; + if (newfd < 0) { + 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; + } + } + +out: + free(dup); + return dirfd; +} + +int safe_mount(const char *src, const char *dest, const char *fstype, + unsigned long flags, const void *data, const char *rootfs) +{ + int fd, ret, saved_errno; + char pathbuf[50]; // only needs enough for /proc/self/fd/ + + if (!rootfs) + rootfs = ""; + + fd = open_without_symlink(dest, rootfs); + if (fd < 0) { + return fd; + } + ret = snprintf(pathbuf, 50, "/proc/self/fd/%d", fd); + if (ret < 0 || ret > 50) { + close(fd); + return -EINVAL; + } + ret = mount(src, pathbuf, fstype, flags, data); + saved_errno = errno; + close(fd); + if (ret < 0) { + errno = saved_errno; + SYSERROR("Failed to mount %s onto %s", src, dest); + return ret; + } + + return 0; +} + +/* * Mount a proc under @rootfs if proc self points to a pid other than * my own. This is needed to have a known-good proc mount for setting * up LSMs both at container startup and attach. @@ -1440,7 +1673,7 @@ int mount_proc_if_needed(const char *rootfs) return 0; domount: - if (mount("proc", path, "proc", 0, NULL)) + if (safe_mount("proc", path, "proc", 0, NULL, rootfs) < 0) return -1; INFO("Mounted /proc in container for security transition"); return 1; diff --git a/src/lxc/utils.h b/src/lxc/utils.h index ee12dde..059026f 100644 --- a/src/lxc/utils.h +++ b/src/lxc/utils.h @@ -279,6 +279,8 @@ bool switch_to_ns(pid_t pid, const char *ns); int is_dir(const char *path); char *get_template_path(const char *t); int setproctitle(char *title); +int safe_mount(const char *src, const char *dest, const char *fstype, + unsigned long flags, const void *data, const char *rootfs); int mount_proc_if_needed(const char *rootfs); int null_stdfds(void); #endif /* __LXC_UTILS_H */ diff --git a/src/tests/Makefile.am b/src/tests/Makefile.am index 461d869..8af9baa 100644 --- a/src/tests/Makefile.am +++ b/src/tests/Makefile.am @@ -54,6 +54,7 @@ if DISTRO_UBUNTU bin_SCRIPTS += \ lxc-test-apparmor-mount \ lxc-test-checkpoint-restore \ + lxc-test-symlink \ lxc-test-ubuntu \ lxc-test-unpriv \ lxc-test-usernic @@ -80,6 +81,7 @@ EXTRA_DIST = \ lxc-test-checkpoint-restore \ lxc-test-cloneconfig \ lxc-test-createconfig \ + 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 100755 index 0000000..7d49c33 --- /dev/null +++ b/src/tests/lxc-test-symlink @@ -0,0 +1,90 @@ +#!/bin/bash + +set -ex + +# lxc: linux Container library + +# Authors: +# Serge Hallyn +# +# 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 -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 +#lxc.mount.entry = $lname opt/xxx/ld none bind,create=dir +#lxc.mount.entry = $lname opt/xxx/lf none bind,create=file +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