>From d520929586ee2063d48359aaaef8f28807604cae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?P=C3=A1draig=20Brady?= Date: Wed, 15 Oct 2014 18:08:42 +0100 Subject: [PATCH] chroot: call chroot() unconditionally to handle bind mounted "/" * src/chroot.c (is_root): Adjust to compare canonicalized paths rather than inodes, to handle (return false in) the case where we have a tree that is constructed by first bind mounting "/" (thus having the same inode). (main): Unconditionally call chroot() because it's safer and of minimal performance benefit to avoid in this case. This will cause inconsistency with some platforms not allowing `chroot / true` for non root users. * tests/misc/chroot-fail.sh: Adjust appropriately. * NEWS: Mention the bug fixes. Fixes http://bugs.gnu.org/18736 --- NEWS | 11 ++++----- src/chroot.c | 29 +++++++++++---------------- tests/misc/chroot-fail.sh | 46 +++++++++++++++++++++++++------------------- 3 files changed, 43 insertions(+), 43 deletions(-) diff --git a/NEWS b/NEWS index 52332bd..5fbdc6a 100644 --- a/NEWS +++ b/NEWS @@ -7,6 +7,11 @@ GNU coreutils NEWS -*- outline -*- dd supports more robust SIGINFO/SIGUSR1 handling for outputting statistics. Previously those signals may have inadvertently terminated the process. + chroot again calls chroot(DIR) and chdir("/"), even if DIR is "/". + This handles separate bind mounted "/" trees, and environments + depending on the implicit chdir("/"). + [bugs introduced in coreutils-8.23] + cp no longer issues an incorrect warning about directory hardlinks when a source directory is specified multiple times. Now, consistent with other file types, a warning is issued for source directories with duplicate names, @@ -25,12 +30,6 @@ GNU coreutils NEWS -*- outline -*- dd accepts a new status=progress level to print data transfer statistics on stderr approximately every second. -** Changes in behavior - - chroot changes the current directory to "/" in again - unless the above new - --skip-chdir option is specified. - [bug introduced in coreutils-8.23] - ** Improvements cp,install,mv will convert smaller runs of NULs in the input to holes, diff --git a/src/chroot.c b/src/chroot.c index 171ced9..3aacafa 100644 --- a/src/chroot.c +++ b/src/chroot.c @@ -162,20 +162,17 @@ parse_additional_groups (char const *groups, GETGROUPS_T **pgids, return ret; } +/* Return whether the passed path is equivalent to "/". + Note we don't compare against get_root_dev_ino() as "/" + could be bind mounted to a separate location. */ + static bool is_root (const char* dir) { - struct dev_ino root_ino; - if (! get_root_dev_ino (&root_ino)) - error (EXIT_CANCELED, errno, _("failed to get attributes of %s"), - quote ("/")); - - struct stat arg_st; - if (stat (dir, &arg_st) == -1) - error (EXIT_CANCELED, errno, _("failed to get attributes of %s"), - quote (dir)); - - return SAME_INODE (root_ino, arg_st); + char *resolved = canonicalize_file_name (dir); + bool is_res_root = resolved && STREQ ("/", resolved); + free (resolved); + return is_res_root; } void @@ -291,8 +288,6 @@ main (int argc, char **argv) usage (EXIT_CANCELED); } - /* Only do chroot specific actions if actually changing root. - The main difference here is that we don't change working dir. */ if (! is_oldroot) { /* We have to look up users and groups twice. @@ -328,12 +323,12 @@ main (int argc, char **argv) n_gids = ngroups; } #endif - - if (chroot (newroot) != 0) - error (EXIT_CANCELED, errno, _("cannot change root directory to %s"), - newroot); } + if (chroot (newroot) != 0) + error (EXIT_CANCELED, errno, _("cannot change root directory to %s"), + newroot); + if (! skip_chdir && chdir ("/")) error (EXIT_CANCELED, errno, _("cannot chdir to root directory")); diff --git a/tests/misc/chroot-fail.sh b/tests/misc/chroot-fail.sh index 82ae23c..75f724a 100755 --- a/tests/misc/chroot-fail.sh +++ b/tests/misc/chroot-fail.sh @@ -29,14 +29,18 @@ test $? = 125 || fail=1 chroot --- / true # unknown option test $? = 125 || fail=1 -# Note chroot("/") succeeds for non-root users on some systems, but not all, -# however we avoid the chroot() with "/" to have common behavior. -chroot / sh -c 'exit 2' # exit status propagation -test $? = 2 || fail=1 -chroot / . # invalid command -test $? = 126 || fail=1 -chroot / no_such # no such command -test $? = 127 || fail=1 +# chroot("/") succeeds for non-root users on some systems, but not all. +if chroot / true ; then + can_chroot_root=1 + chroot / sh -c 'exit 2' # exit status propagation + test $? = 2 || fail=1 + chroot / . # invalid command + test $? = 126 || fail=1 + chroot / no_such # no such command + test $? = 127 || fail=1 +else + test $? = 125 || fail=1 +fi # Ensure that --skip-chdir fails with a non-"/" argument. cat <<\EOF > exp || framework_failure_ @@ -47,17 +51,19 @@ chroot --skip-chdir . env pwd >out 2>err && fail=1 compare /dev/null out || fail=1 compare exp err || fail=1 -# Ensure we don't chroot("/") when NEWROOT is old "/". -ln -s / isroot || framework_failure_ -for dir in '/' '/.' '/../' isroot; do - # Verify that chroot(1) succeeds and performs chdir("/") - # (chroot(1) of coreutils-8.23 failed to run the latter). - curdir=$(chroot "$dir" env pwd) || fail=1 - test "$curdir" = '/' || fail=1 - - # Test the "--skip-chdir" option. - curdir=$(chroot --skip-chdir "$dir" env pwd) || fail=1 - test "$curdir" = '/' && fail=1 -done +# Ensure we chdir("/") appropriately when NEWROOT is old "/". +if test "$can_chroot_root"; then + ln -s / isroot || framework_failure_ + for dir in '/' '/.' '/../' isroot; do + # Verify that chroot(1) succeeds and performs chdir("/") + # (chroot(1) of coreutils-8.23 failed to run the latter). + curdir=$(chroot "$dir" env pwd) || fail=1 + test "$curdir" = '/' || fail=1 + + # Test the "--skip-chdir" option. + curdir=$(chroot --skip-chdir "$dir" env pwd) || fail=1 + test "$curdir" = '/' && fail=1 + done +fi Exit $fail -- 1.7.7.6