Subject: bug#18736: chroot regression - chroot avoids the chroot() call too eagerly.

From: Pádraig Brady <[email-masked]>
Date: Wed, 15 Oct 2014 18:17:12 +0100

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-masked]>
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