Package: schroot Followup-For: Bug #728096 Users have hit this issue in Ubuntu, too. I've triaged it and have determined that Laurent is correct in Message #17 that the schroot mount helper is using the host's filesystem to resolve symlinks in the chroot.
It is simply calling realpath(3) on the mount destination in the chroot,
from a process that is not chrooted, so realpath(3) is going to use the
host filesystem to resolve symlinks such as
'/<chroot>/dev/shm -> /run/shm'. It then tries to fixup the final
resolved path but appended the chroot basepath but it's too late at that
point, the host's filesystem has already been used to resolve the
symlink target.
We're actually getting filesystems mounted in the host filesystem in
Ubuntu (LP: #1438942) due to /run/shm existing in the host and pointing
back to /dev/shm.
Here's a simple test:
# Show that schroot's mount binary is using the host fs to resolve symlinks
$ mkdir -p /tmp/bug728096/dev
$ mkdir -p /tmp/bug728096/run/shm
$ ln -s /run/shm /tmp/bug728096/dev/shm
$ ls -ld /{dev,run}/shm
drwxrwxrwt 2 root root 120 Jul 29 14:07 /dev/shm
lrwxrwxrwx 1 root root 8 Jul 29 14:07 /run/shm -> /dev/shm
$ ls -ld /tmp/bug728096/{dev,run}/shm
lrwxrwxrwx 1 tyhicks tyhicks 8 Jul 29 14:49 /tmp/bug728096/dev/shm ->
/run/shm
drwxrwxr-x 2 tyhicks tyhicks 4096 Jul 29 14:49 /tmp/bug728096/run/shm
$ echo "tmpfs /dev/shm/ tmpfs defaults 0 0" > /tmp/fstab
$ grep /dev/shm /proc/mounts
tmpfs /dev/shm tmpfs rw,nosuid,nodev 0 0
$ sudo ./libexec/mount/mount -v -f /tmp/fstab -m /tmp/bug728096
$ grep /dev/shm /proc/mounts
tmpfs /dev/shm tmpfs rw,nosuid,nodev 0 0
tmpfs /dev/shm tmpfs rw,relatime 0 0
# Now reproduce Laurent's original bug report by removing the host's /run/shm
$ sudo umount /dev/shm
$ sudo rm /run/shm
$ sudo ./libexec/mount/mount -v -f /tmp/fstab -m /tmp/bug728096
E: boost::filesystem::create_directory: File exists: "/tmp/bug728096/dev/shm"
$ echo $?
1
I've attached fixes for the master and 1.6 schroot branches. They fork
off a process that changes root to the chroot base path before calling
realpath(3) on the mount destination.
Note that I'm still pretty confused by a part of the code in
resolve_path_chrooted() (which is mostly the old resolve_path()). I
don't understand the what is happening after the
"// Try validating the parent directory." comment but I left it since I
couldn't wrap my head around it. Any insight to what it is doing and
whether we can remove it now?
Tyler
From 86a39d878bc1b6ea59b4f354f03b635014b720a1 Mon Sep 17 00:00:00 2001 From: Tyler Hicks <tyhi...@canonical.com> Date: Tue, 28 Jul 2015 02:11:07 -0500 Subject: [PATCH] libexec-mount: Resolve mount destinations while chrooted The mount binary was attempting to use realpath(3) from outside of the chroot to resolve mount destination paths inside of the chroot. It would then take the resolved path and prepend it with the path to the chroot in an attempt to enforce that symlink resolution will always end up inside of the chroot. One example of why this approach isn't sufficient is that when /<chroot>/dev/shm/ is the mount destination but it is a symlink to /run/shm and the host's /run/shm is a symlink to /dev/shm. The resolved path will end up being /<chroot>/dev/shm/ and, due to mount following symlinks, the host's /dev/shm will be mounted over. To fix the path resolution issue, this patch first resolves the path to the chroot base path, then forks and calls chroot(2) on that path, then resolves the path to the mount destination inside the chroot. Finally, the resolved chroot base path and the resolved mount destination path are combined to create the fully resolved path used for mounting. Bug-Debian: https://bugs.debian.org/728096 Bug-Ubuntu: https://launchpad.net/bugs/1438942 Signed-off-by: Tyler Hicks <tyhi...@canonical.com> --- libexec/mount/main.cc | 141 ++++++++++++++++++++++++++++++++++++++++++-------- libexec/mount/main.h | 28 ++++++++-- 2 files changed, 141 insertions(+), 28 deletions(-) diff --git a/libexec/mount/main.cc b/libexec/mount/main.cc index 9ea6e80..bbf4a18 100644 --- a/libexec/mount/main.cc +++ b/libexec/mount/main.cc @@ -30,6 +30,7 @@ #include <ctime> #include <iostream> #include <locale> +#include <poll.h> #include <sys/types.h> #include <sys/stat.h> @@ -53,8 +54,14 @@ namespace schroot { {bin::schroot_mount::main::CHILD_FORK, N_("Failed to fork child")}, {bin::schroot_mount::main::CHILD_WAIT, N_("Wait for child failed")}, + // TRANSLATORS: %1% = directory + {bin::schroot_mount::main::CHROOT, N_("Failed to change root to directory ‘%1%’")}, + {bin::schroot_mount::main::DUP, N_("Failed to duplicate file descriptor")}, // TRANSLATORS: %1% = command name {bin::schroot_mount::main::EXEC, N_("Failed to execute “%1%”")}, + {bin::schroot_mount::main::PIPE, N_("Failed to create pipe")}, + {bin::schroot_mount::main::POLL, N_("Failed to poll file descriptor")}, + {bin::schroot_mount::main::READ, N_("Failed to read file descriptor")}, {bin::schroot_mount::main::REALPATH, N_("Failed to resolve path “%1%”")} }; @@ -81,23 +88,14 @@ namespace bin } std::string - main::resolve_path (const std::string& mountpoint) + main::resolve_path_chrooted (const std::string& mountpoint) { - // Ensure entry has a leading / to prevent security hole where - // mountpoint might be outside the chroot. - std::string absmountpoint(mountpoint); - if (absmountpoint.empty() || absmountpoint[0] != '/') - absmountpoint = std::string("/") + absmountpoint; - - char *resolved_path = realpath(opts->mountpoint.c_str(), 0); - if (!resolved_path) - throw error(opts->mountpoint, REALPATH, strerror(errno)); - std::string basepath(resolved_path); - std::free(resolved_path); - - std::string directory(opts->mountpoint + absmountpoint); + std::string directory(mountpoint); + if (directory.empty() || directory[0] != '/') + directory = std::string("/") + directory; + // Canonicalise path to remove any symlinks. - resolved_path = realpath(directory.c_str(), 0); + char *resolved_path = realpath(directory.c_str(), 0); if (resolved_path == 0) { // The path is either not present or is an invalid link. If @@ -117,13 +115,13 @@ namespace bin else { // Try validating the parent directory. - schroot::string_list dirs = schroot::split_string(mountpoint, "/"); + schroot::string_list dirs = schroot::split_string(directory, "/"); if (dirs.size() > 1) // Recurse if possible, otherwise continue { std::string saveddir = *dirs.rbegin(); dirs.pop_back(); - std::string newpath(resolve_path(schroot::string_list_to_string(dirs, "/"))); + std::string newpath(resolve_path_chrooted(schroot::string_list_to_string(dirs, "/"))); directory = newpath + "/" + saveddir; } } @@ -133,16 +131,113 @@ namespace bin directory = resolved_path; std::free(resolved_path); } - // If the link was absolute (i.e. points somewhere on the host, - // outside the chroot, make sure that this is modified to be - // inside. - if (directory.size() < basepath.size() || - directory.substr(0,basepath.size()) != basepath) - directory = basepath + directory; return directory; } + std::string + main::resolve_path (const std::string& mountpoint) + { + std::string stdout_buf; + int stdout_pipe[2]; + int exit_status = 0; + pid_t pid; + + try + { + if (pipe(stdout_pipe) < 0) + throw error(PIPE, strerror(errno)); + + if ((pid = fork()) == -1) + { + throw error(CHILD_FORK, strerror(errno)); + } + else if (pid == 0) + { + try + { + // Set up pipes for stdout + if (dup2(stdout_pipe[1], STDOUT_FILENO) < 0) + throw error(DUP, strerror(errno)); + + close(stdout_pipe[0]); + close(stdout_pipe[1]); + + char *resolved_path = realpath(opts->mountpoint.c_str(), 0); + if (!resolved_path) + throw error(opts->mountpoint, REALPATH, strerror(errno)); + + std::string basepath(resolved_path); + std::free(resolved_path); + + if (chroot(basepath.c_str()) < 0) + throw error(basepath, CHROOT, strerror(errno)); + + std::cout << basepath + resolve_path_chrooted(mountpoint); + std::cout.flush(); + _exit(EXIT_SUCCESS); + } + catch (std::exception const& e) + { + schroot::log_exception_error(e); + } + catch (...) + { + schroot::log_error() + << _("An unknown exception occurred") << std::endl; + } + _exit(EXIT_FAILURE); + } + + // Log stdout + close(stdout_pipe[1]); + + struct pollfd pollfd; + pollfd.fd = stdout_pipe[0]; + pollfd.events = POLLIN; + pollfd.revents = 0; + + char buffer[BUFSIZ]; + + while (1) + { + int status; + + if ((status = poll(&pollfd, 1, -1)) < 0) + throw error(POLL, strerror(errno)); + + int outdata = 0; + + if (pollfd.revents & POLLIN) + { + if ((outdata = read(pollfd.fd, buffer, BUFSIZ)) < 0 + && errno != EINTR) + throw error(READ, strerror(errno)); + + if (outdata) + stdout_buf += std::string(&buffer[0], outdata); + } + + if (outdata == 0) // pipe closed + break; + } + + close(stdout_pipe[0]); + wait_for_child(pid, exit_status); + } + catch (error const& e) + { + close(stdout_pipe[0]); + close(stdout_pipe[1]); + throw; + } + + if (exit_status) + exit(exit_status); + + return stdout_buf; + } + void main::action_mount () { diff --git a/libexec/mount/main.h b/libexec/mount/main.h index c523bfe..790dc3a 100644 --- a/libexec/mount/main.h +++ b/libexec/mount/main.h @@ -45,7 +45,12 @@ namespace bin { CHILD_FORK, ///< Failed to fork child. CHILD_WAIT, ///< Wait for child failed. + CHROOT, ///< Failed to change root. + DUP, ///< Failed to duplicate file descriptor. EXEC, ///< Failed to execute. + PIPE, ///< Failed to create pipe. + POLL, ///< Failed to poll file descriptor. + READ, ///< Failed to read file descriptor. REALPATH ///< Failed to resolve path. }; @@ -84,16 +89,29 @@ namespace bin const schroot::environment& env); /** - * Ensure that the mountpoint is a valid absolute path inside the - * chroot. This is to avoid absolute or relative symlinks - * pointing outside the chroot causing filesystems to be mounted - * on the host. An exception will be thrown if it is not possible - * to resolve the path. + * Ensure that the mountpoint is a valid absolute path. The calling + * process must be chrooted before calling this function to avoid + * resolving absolute or relative symlinks pointing outside the chroot. + * An exception will be thrown if it is not possible to resolve the path. * * @param mountpoint the mountpoint to check, * @returns the validated path. */ + std::string + resolve_path_chrooted (const std::string& mountpoint); + /** + * Ensure that the chroot base path and the mountpoint is a valid + * absolute path inside the chroot. This function calls chroot on the + * resolved chroot base path before attempting to resolve the mountpoint + * path inside of the chroot to avoid absolute or relative symlinks + * pointing outside the chroot causing filesystems to be mounted on the + * host. An exception will be thrown if it is not possible to resolve + * the chroot base path or the mountpoint path. + * + * @param mountpoint the mountpoint to check, + * @returns the validated path. + */ std::string resolve_path (const std::string& mountpoint); -- 2.1.4
From c051ee6ef19d0592baac099c19dc485b42bd1771 Mon Sep 17 00:00:00 2001 From: Tyler Hicks <tyhi...@canonical.com> Date: Tue, 28 Jul 2015 01:01:12 -0500 Subject: [PATCH] schroot-mount: Resolve mount destinations while chrooted The schroot-mount binary was attempting to use realpath(3) from outside of the chroot to resolve mount destination paths inside of the chroot. It would then take the resolved path and prepend it with the path to the chroot in an attempt to enforce that symlink resolution will always end up inside of the chroot. One example of why this approach isn't sufficient is that when /<chroot>/dev/shm/ is the mount destination but it is a symlink to /run/shm and the host's /run/shm is a symlink to /dev/shm. The resolved path will end up being /<chroot>/dev/shm/ and, due to mount following symlinks, the host's /dev/shm will be mounted over. To fix the path resolution issue, this patch first resolves the path to the chroot base path, then forks and calls chroot(2) on that path, then resolves the path to the mount destination inside the chroot. Finally, the resolved chroot base path and the resolved mount destination path are combined to create the fully resolved path used for mounting. Bug-Debian: https://bugs.debian.org/728096 Bug-Ubuntu: https://launchpad.net/bugs/1438942 Signed-off-by: Tyler Hicks <tyhi...@canonical.com> --- bin/schroot-mount/schroot-mount-main.cc | 141 ++++++++++++++++++++++++++------ bin/schroot-mount/schroot-mount-main.h | 29 +++++-- 2 files changed, 142 insertions(+), 28 deletions(-) diff --git a/bin/schroot-mount/schroot-mount-main.cc b/bin/schroot-mount/schroot-mount-main.cc index 327f5ed..aa5004b 100644 --- a/bin/schroot-mount/schroot-mount-main.cc +++ b/bin/schroot-mount/schroot-mount-main.cc @@ -30,6 +30,7 @@ #include <ctime> #include <iostream> #include <locale> +#include <poll.h> #include <sys/types.h> #include <sys/stat.h> @@ -59,8 +60,14 @@ namespace { emap(main::CHILD_FORK, N_("Failed to fork child")), emap(main::CHILD_WAIT, N_("Wait for child failed")), + // TRANSLATORS: %1% = directory + emap(main::CHROOT, N_("Failed to change root to directory ‘%1%’")), + emap(main::DUP, N_("Failed to duplicate file descriptor")), // TRANSLATORS: %1% = command name emap(main::EXEC, N_("Failed to execute “%1%”")), + emap(main::PIPE, N_("Failed to create pipe")), + emap(main::POLL, N_("Failed to poll file descriptor")), + emap(main::READ, N_("Failed to read file descriptor")), emap(main::REALPATH, N_("Failed to resolve path “%1%”")) }; @@ -88,23 +95,14 @@ main::~main () } std::string -main::resolve_path (std::string const& mountpoint) +main::resolve_path_chrooted (std::string const& mountpoint) { - // Ensure entry has a leading / to prevent security hole where - // mountpoint might be outside the chroot. - std::string absmountpoint(mountpoint); - if (absmountpoint.empty() || absmountpoint[0] != '/') - absmountpoint = std::string("/") + absmountpoint; - - char *resolved_path = realpath(opts->mountpoint.c_str(), 0); - if (!resolved_path) - throw error(opts->mountpoint, REALPATH, strerror(errno)); - std::string basepath(resolved_path); - std::free(resolved_path); - - std::string directory(opts->mountpoint + absmountpoint); + std::string directory(mountpoint); + if (directory.empty() || directory[0] != '/') + directory = std::string("/") + directory; + // Canonicalise path to remove any symlinks. - resolved_path = realpath(directory.c_str(), 0); + char *resolved_path = realpath(directory.c_str(), 0); if (resolved_path == 0) { // The path is either not present or is an invalid link. If @@ -124,13 +122,13 @@ main::resolve_path (std::string const& mountpoint) else { // Try validating the parent directory. - sbuild::string_list dirs = sbuild::split_string(mountpoint, "/"); + sbuild::string_list dirs = sbuild::split_string(directory, "/"); if (dirs.size() > 1) // Recurse if possible, otherwise continue { std::string saveddir = *dirs.rbegin(); dirs.pop_back(); - std::string newpath(resolve_path(sbuild::string_list_to_string(dirs, "/"))); + std::string newpath(resolve_path_chrooted(sbuild::string_list_to_string(dirs, "/"))); directory = newpath + "/" + saveddir; } } @@ -140,16 +138,113 @@ main::resolve_path (std::string const& mountpoint) directory = resolved_path; std::free(resolved_path); } - // If the link was absolute (i.e. points somewhere on the host, - // outside the chroot, make sure that this is modified to be - // inside. - if (directory.size() < basepath.size() || - directory.substr(0,basepath.size()) != basepath) - directory = basepath + directory; return directory; } +std::string +main::resolve_path (std::string const& mountpoint) +{ + std::string stdout_buf; + int stdout_pipe[2]; + int exit_status = 0; + pid_t pid; + + try + { + if (pipe(stdout_pipe) < 0) + throw error(PIPE, strerror(errno)); + + if ((pid = fork()) == -1) + { + throw error(CHILD_FORK, strerror(errno)); + } + else if (pid == 0) + { + try + { + // Set up pipes for stdout + if (dup2(stdout_pipe[1], STDOUT_FILENO) < 0) + throw error(DUP, strerror(errno)); + + close(stdout_pipe[0]); + close(stdout_pipe[1]); + + char *resolved_path = realpath(opts->mountpoint.c_str(), 0); + if (!resolved_path) + throw error(opts->mountpoint, REALPATH, strerror(errno)); + + std::string basepath(resolved_path); + std::free(resolved_path); + + if (chroot(basepath.c_str()) < 0) + throw error(basepath, CHROOT, strerror(errno)); + + std::cout << basepath + resolve_path_chrooted(mountpoint); + std::cout.flush(); + _exit(EXIT_SUCCESS); + } + catch (std::exception const& e) + { + sbuild::log_exception_error(e); + } + catch (...) + { + sbuild::log_error() + << _("An unknown exception occurred") << std::endl; + } + _exit(EXIT_FAILURE); + } + + // Log stdout + close(stdout_pipe[1]); + + struct pollfd pollfd; + pollfd.fd = stdout_pipe[0]; + pollfd.events = POLLIN; + pollfd.revents = 0; + + char buffer[BUFSIZ]; + + while (1) + { + int status; + + if ((status = poll(&pollfd, 1, -1)) < 0) + throw error(POLL, strerror(errno)); + + int outdata = 0; + + if (pollfd.revents & POLLIN) + { + if ((outdata = read(pollfd.fd, buffer, BUFSIZ)) < 0 + && errno != EINTR) + throw error(READ, strerror(errno)); + + if (outdata) + stdout_buf += std::string(&buffer[0], outdata); + } + + if (outdata == 0) // pipe closed + break; + } + + close(stdout_pipe[0]); + wait_for_child(pid, exit_status); + } + catch (error const& e) + { + close(stdout_pipe[0]); + close(stdout_pipe[1]); + throw; + } + + if (exit_status) + exit(exit_status); + + return stdout_buf; +} + void main::action_mount () { diff --git a/bin/schroot-mount/schroot-mount-main.h b/bin/schroot-mount/schroot-mount-main.h index 06c0333..7b0ef24 100644 --- a/bin/schroot-mount/schroot-mount-main.h +++ b/bin/schroot-mount/schroot-mount-main.h @@ -42,7 +42,12 @@ namespace schroot_mount { CHILD_FORK, ///< Failed to fork child. CHILD_WAIT, ///< Wait for child failed. + CHROOT, ///< Failed to change root. + DUP, ///< Failed to duplicate file descriptor. EXEC, ///< Failed to execute. + PIPE, ///< Failed to create pipe. + POLL, ///< Failed to poll file descriptor. + READ, ///< Failed to read file descriptor. REALPATH ///< Failed to resolve path. }; @@ -81,17 +86,31 @@ namespace schroot_mount sbuild::environment const& env); /** - * Ensure that the mountpoint is a valid absolute path inside the - * chroot. This is to avoid absolute or relative symlinks - * pointing outside the chroot causing filesystems to be mounted - * on the host. An exception will be thrown if it is not possible - * to resolve the path. + * Ensure that the mountpoint is a valid absolute path. The calling process + * must be chrooted before calling this function to avoid resolving + * absolute or relative symlinks pointing outside the chroot. An exception + * will be thrown if it is not possible to resolve the path. * * @param mountpoint the mountpoint to check, * @returns the validated path. */ std::string + resolve_path_chrooted (std::string const& mountpoint); + + /** + * Ensure that the chroot base path and the mountpoint is a valid absolute + * path inside the chroot. This function calls chroot on the resolved + * chroot base path before attempting to resolve the mountpoint path inside + * of the chroot to avoid absolute or relative symlinks pointing outside + * the chroot causing filesystems to be mounted on the host. An exception + * will be thrown if it is not possible to resolve the chroot base path or + * the mountpoint path. + * + * @param mountpoint the mountpoint to check, + * @returns the validated path. + */ + std::string resolve_path (std::string const& mountpoint); /** -- 2.1.4
signature.asc
Description: Digital signature
