On 10/15/2014 10:40 AM, Rogier wrote: > Hi, > > Since a few months, it seems that chroot has started avoiding the > chroot call if it can be determined to be idempotent. > It looks like the new check is based on inode comparison - if the > inode is the same, the chroot() call is considered idempotent, and not > performed. > > However, while two directories being in the same file system and having > the same inode number implies that they are the same directory, it > does not necessarily imply that they have same file system path (i.e. > use the same mountpoint), so there is no guarantee that the entire > directory trees rooted at the two directories are also identical even > though the directories are.This means that chroot will fail to > chroot() in cases when the call would in fact not be idempotent. > > On my system (debian testing), I have / bind-mounted elsewhere, with a > slightly different directory tree mounted beneath it, as is mounted > beneath /. In my case, I need this so that I can make partial backups > of the system - including some file systems but leaving out others. > Undoubtly, there are other use-cases as well - I wouldn't be surprised > if users of libpam-chroot can be affected by this, depending on exactly > how they configure their chroot environments. > > The new chroot behavior no longer allows chrooting into such alternate > trees. In my case, that means that my backup fails.
I didn't consider this unusual case when avoiding the chroot() call, for efficiency (especially in the --userspec case), and to add consistency between platforms in the handling of `chroot / true` for non root users. I agree with your analysis and that we should revert to the previous behavior here, which is done in the attached patch. thanks! Pádraig.
>From a51ed4e892b74aa400633292f1413138ccb4da62 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?P=C3=A1draig=20Brady?= <[email protected]> Date: Wed, 15 Oct 2014 18:08:42 +0100 Subject: [PATCH] chroot: call chroot() unconditionally to handle bind mounted "/" * src/chroot.c (main): To handle a separate tree to "/" which is constructed by first bind mounting "/", we need to unconditionally call chroot(). This will cause inconsistency with some platforms not allowing `chroot / true` for non root users, but that is a lesser issue than not calling chroot() on the potentially different tree. * tests/misc/chroot-fail.sh: Adjust appropriately. * NEWS: Mention the bug fixes. Fixes http://bugs.gnu.org/18736 --- NEWS | 11 +++---- src/chroot.c | 67 +++++++++++++++++++++----------------------- tests/misc/chroot-fail.sh | 46 +++++++++++++++++------------- 3 files changed, 63 insertions(+), 61 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..bea2e9c 100644 --- a/src/chroot.c +++ b/src/chroot.c @@ -291,48 +291,45 @@ 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. + - First, outside the chroot to load potentially necessary passwd/group + parsing plugins (e.g. NSS); + - Second, inside chroot to redo parsing in case IDs are different. + Within chroot lookup is the main justification for having + the --user option supported by the chroot command itself. + We do this even if is_oldroot, as we could potentially + still have a different tree in this case with a bind mounted '/'. */ + if (userspec) + ignore_value (parse_user_spec (userspec, &uid, &gid, NULL, NULL)); + + /* If no gid is supplied or looked up, do so now. + Also lookup the username for use with getgroups. */ + if (uid_set (uid) && (! groups || gid_unset (gid))) { - /* We have to look up users and groups twice. - - First, outside the chroot to load potentially necessary passwd/group - parsing plugins (e.g. NSS); - - Second, inside chroot to redo parsing in case IDs are different. - Within chroot lookup is the main justification for having - the --user option supported by the chroot command itself. */ - if (userspec) - ignore_value (parse_user_spec (userspec, &uid, &gid, NULL, NULL)); - - /* If no gid is supplied or looked up, do so now. - Also lookup the username for use with getgroups. */ - if (uid_set (uid) && (! groups || gid_unset (gid))) + const struct passwd *pwd; + if ((pwd = getpwuid (uid))) { - const struct passwd *pwd; - if ((pwd = getpwuid (uid))) - { - if (gid_unset (gid)) - gid = pwd->pw_gid; - username = pwd->pw_name; - } + if (gid_unset (gid)) + gid = pwd->pw_gid; + username = pwd->pw_name; } + } - if (groups && *groups) - ignore_value (parse_additional_groups (groups, &out_gids, &n_gids, - false)); + if (groups && *groups) + ignore_value (parse_additional_groups (groups, &out_gids, &n_gids, + false)); #if HAVE_SETGROUPS - else if (! groups && gid_set (gid) && username) - { - int ngroups = xgetgroups (username, gid, &out_gids); - if (0 < ngroups) - n_gids = ngroups; - } + else if (! groups && gid_set (gid) && username) + { + int ngroups = xgetgroups (username, gid, &out_gids); + if (0 < ngroups) + 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
