chroot: perform chdir("/") again unless new --skip-chdir is specified

Since commit v8.22-94-g99960ee, chroot(1) skips the chroot(2) syscall
for "/" arguments (and synonyms).  The problem is that it also skips
the following chdir("/") call in that case.  The latter breaks existing
scripts which expect "/" to be the working directory inside the chroot.
While the first part of the change - i.e., skipping chroot("/") - is
okay for consistency with systems where it might succeed for a non-root
user, the second part might be malicious, e.g.

  cd /home/user && chroot '/' bin/foo

In the "best" case, chroot(1) could not execute 'bin/foo' with ENOENT,
but in the worst case, chroot(1) would execute '/home/user/bin/foo' in
the case that exists - instead of '/bin/foo'.

Revert that second part of the patch, i.e., perform the chdir("/)
in the common case again - unless the new --skip-chdir option is
specified.  Restrict this new option to the case of "/" arguments.

* src/chroot.c (SKIP_CHDIR): Add enum.
(long_opts): Add entry for the new --skip-chdir option.
(usage): Add --skip-chdir option, and while at it, move the other
to options into alphabetical order.
(main): Accept the above new option, allowing it only in the case
when NEWROOT is the old "/".
Move down the chdir() call after the if-clause to ensure it is
run in any case - unless --skip-chdir is specified.
Add a 'newroot' variable for the new root directory as it is used
in a couple of places now.
* tests/misc/chroot-fail.sh: Invert the last tests which check the
working directory of the execvp()ed program when a "/"-like
argument was passed: now expect it to be "/" - unless --skip-chdir
is given.
* doc/coreutils.texi (chroot invocation): Document the new option.
Document that chroot(1) usually calls chdir("/") unless the new
--skip-chdir option is specified.  Sort options.
* NEWS (Changes in behavior): Mention the fix.
(New features): Mention the new option.
* init.cfg (nonroot_has_perm_): Add chroot's new --skip-chdir option.
* tests/cp/preserve-gid.sh (t1): Likewise.
* tests/cp/special-bits.sh: Likewise.
* tests/id/setgid.sh: Likewise.
* tests/misc/truncate-owned-by-other.sh: Likewise.
* tests/mv/sticky-to-xpart.sh: Likewise.
* tests/rm/fail-2eperm.sh: Likewise.
* tests/rm/no-give-up.sh: Likewise.
* tests/touch/now-owned-by-other.sh: Likewise.

Reported by Andreas Schwab in http://bugs.gnu.org/18062

9 files changed, 37 insertions, 17 deletions

diff --git a/tests/cp/preserve-gid.sh b/tests/cp/preserve-gid.sh
index f141ac141..5499c2e5d 100755
--- a/tests/cp/preserve-gid.sh
+++ b/tests/cp/preserve-gid.sh
@@ -117,7 +117,8 @@ t1() {
   u=$1; shift
   g=$1; shift
   t0 "$f" "$u" "$g" \
-      chroot --user=+$nameless_uid:+$nameless_gid1 \
+      chroot --skip-chdir \
+             --user=+$nameless_uid:+$nameless_gid1 \
              --groups="+$nameless_gid1,+$nameless_gid2" \
         / env PATH="$tmp_path" "$@"
 }
diff --git a/tests/cp/special-bits.sh b/tests/cp/special-bits.sh
index a55eea21f..1402819a4 100755
--- a/tests/cp/special-bits.sh
+++ b/tests/cp/special-bits.sh
@@ -42,7 +42,8 @@ set _ $(ls -l b); shift; p1=$1
 set _ $(ls -l b2); shift; p2=$1
 test $p1 = $p2 || fail=1
 
-chroot --user=$NON_ROOT_USERNAME / env PATH="$PATH" cp -p c c2 || fail=1
+chroot --skip-chdir --user=$NON_ROOT_USERNAME / env PATH="$PATH" cp -p c c2 \
+  || fail=1
 set _ $(ls -l c); shift; p1=$1
 set _ $(ls -l c2); shift; p2=$1
 test $p1 = $p2 && fail=1
diff --git a/tests/id/setgid.sh b/tests/id/setgid.sh
index 6d9d74f43..019418a3d 100755
--- a/tests/id/setgid.sh
+++ b/tests/id/setgid.sh
@@ -27,14 +27,14 @@ echo $gp1 > exp || framework_failure_
 
 # With coreutils-8.16 and earlier, id -G would print both:
 #  $gp1 $NON_ROOT_GID
-chroot --user=$NON_ROOT_USERNAME:+$gp1 --groups='' / env PATH="$PATH" \
-  id -G > out || fail=1
+chroot --skip-chdir --user=$NON_ROOT_USERNAME:+$gp1 --groups='' / \
+  env PATH="$PATH" id -G > out || fail=1
 compare exp out || fail=1
 
 # With coreutils-8.22 and earlier, id would erroneously print
 #  groups=$NON_ROOT_GID
-chroot --user=$NON_ROOT_USERNAME:+$gp1 --groups='' / env PATH="$PATH" \
-  id > out || fail=1
+chroot --skip-chdir --user=$NON_ROOT_USERNAME:+$gp1 --groups='' / \
+  env PATH="$PATH" id > out || fail=1
 grep -F "groups=$gp1" out || { cat out; fail=1; }
 
 Exit $fail
diff --git a/tests/misc/chroot-fail.sh b/tests/misc/chroot-fail.sh
index a84826fd7..82ae23cdb 100755
--- a/tests/misc/chroot-fail.sh
+++ b/tests/misc/chroot-fail.sh
@@ -30,7 +30,7 @@ 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 behvavior.
+# 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
@@ -38,10 +38,25 @@ test $? = 126 || fail=1
 chroot / no_such # no such command
 test $? = 127 || fail=1
 
-# Ensure we don't chdir("/") when not changing root
-# to allow only changing user ids for a command.
-for dir in '/' '/.' '/../'; do
+# Ensure that --skip-chdir fails with a non-"/" argument.
+cat <<\EOF > exp || framework_failure_
+chroot: option --skip-chdir only permitted if NEWROOT is old '/'
+Try 'chroot --help' for more information.
+EOF
+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
 
diff --git a/tests/misc/truncate-owned-by-other.sh b/tests/misc/truncate-owned-by-other.sh
index e70badb6b..f65439ea1 100755
--- a/tests/misc/truncate-owned-by-other.sh
+++ b/tests/misc/truncate-owned-by-other.sh
@@ -29,7 +29,7 @@ chmod g+w root-owned
 # Ensure that the current directory is searchable by $NON_ROOT_USERNAME.
 chmod g+x .
 
-chroot --user=$NON_ROOT_USERNAME / env PATH="$PATH" \
+chroot --skip-chdir --user=$NON_ROOT_USERNAME / env PATH="$PATH" \
   truncate -s0 root-owned || fail=1
 
 Exit $fail
diff --git a/tests/mv/sticky-to-xpart.sh b/tests/mv/sticky-to-xpart.sh
index e0c99e946..6c1f6e813 100755
--- a/tests/mv/sticky-to-xpart.sh
+++ b/tests/mv/sticky-to-xpart.sh
@@ -42,7 +42,8 @@ chmod go+x . || framework_failure_
 
 # Ensure that $NON_ROOT_USERNAME can access the required version of mv.
 version=$(
-  chroot --user=$NON_ROOT_USERNAME / env PATH="$PATH" mv --version |
+  chroot --skip-chdir --user=$NON_ROOT_USERNAME / env PATH="$PATH" \
+    mv --version |
   sed -n '1s/.* //p'
 )
 case $version in
@@ -50,7 +51,7 @@ case $version in
   *) skip_ "cannot access just-built mv as user $NON_ROOT_USERNAME";;
 esac
 
-chroot --user=$NON_ROOT_USERNAME / env PATH="$PATH" \
+chroot --skip-chdir --user=$NON_ROOT_USERNAME / env PATH="$PATH" \
   mv t/root-owned "$other_partition_tmpdir" 2> out-t && fail=1
 
 # On some systems, we get 'Not owner'.  Convert it.
diff --git a/tests/rm/fail-2eperm.sh b/tests/rm/fail-2eperm.sh
index 6e8ce9ba3..c3240372d 100755
--- a/tests/rm/fail-2eperm.sh
+++ b/tests/rm/fail-2eperm.sh
@@ -32,14 +32,16 @@ touch a/b || framework_failure_
 # Try to ensure that $NON_ROOT_USERNAME can access
 # the required version of rm.
 rm_version=$(
-  chroot --user=$NON_ROOT_USERNAME / env PATH="$PATH" rm --version |
+  chroot --skip-chdir --user=$NON_ROOT_USERNAME / env PATH="$PATH" \
+    rm --version |
   sed -n '1s/.* //p'
 )
 case $rm_version in
   $PACKAGE_VERSION) ;;
   *) skip_ "cannot access just-built rm as user $NON_ROOT_USERNAME";;
 esac
-chroot --user=$NON_ROOT_USERNAME / env PATH="$PATH" rm -rf a 2> out-t && fail=1
+chroot --skip-chdir --user=$NON_ROOT_USERNAME / \
+  env PATH="$PATH" rm -rf a 2> out-t && fail=1
 
 # On some systems, we get 'Not owner'.  Convert it.
 # On other systems (HPUX), we get 'Permission denied'.  Convert it, too.
diff --git a/tests/rm/no-give-up.sh b/tests/rm/no-give-up.sh
index 41070c9de..958f9e882 100755
--- a/tests/rm/no-give-up.sh
+++ b/tests/rm/no-give-up.sh
@@ -30,7 +30,7 @@ chmod go=x . || framework_failure_
 
 
 # This must fail, since '.' is not writable by $NON_ROOT_USERNAME.
-chroot --user=$NON_ROOT_USERNAME / env PATH="$PATH" \
+chroot --skip-chdir --user=$NON_ROOT_USERNAME / env PATH="$PATH" \
   rm -rf d 2>/dev/null && fail=1
 
 # d must remain.
diff --git a/tests/touch/now-owned-by-other.sh b/tests/touch/now-owned-by-other.sh
index d01097edb..018ef118b 100755
--- a/tests/touch/now-owned-by-other.sh
+++ b/tests/touch/now-owned-by-other.sh
@@ -28,7 +28,7 @@ chmod g+w root-owned
 # Ensure that the current directory is searchable by $NON_ROOT_USERNAME.
 chmod g+x .
 
-chroot --user=$NON_ROOT_USERNAME / env PATH="$PATH" \
+chroot --skip-chdir --user=$NON_ROOT_USERNAME / env PATH="$PATH" \
   touch -d now root-owned || fail=1
 
 Exit $fail