On 07/21/2014 11:20 PM, Bernhard Voelker wrote:
> c) revert part 3), i.e. chdir("/") in any case?
> This would require some work on our tests, because we couldn't
> use commands like above as easy as this.Here's a patch implementing the alternative outlined in c). Have a nice day, Berny
>From bdb6835de64aa8b44dee87cc53fe44f663d98c54 Mon Sep 17 00:00:00 2001 From: Bernhard Voelker <[email protected]> Date: Wed, 23 Jul 2014 00:39:06 +0200 Subject: [PATCH] chroot: perform chdir("/") in any case again 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 in a chroot jail. 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 any case again. To be able to use chroot(1) in the tests, add the hidden "---skip-chdir" option - note the 3 dashes. * src/chroot.c (SKIP_CHDIR): Add enum. (long_opts): Add entry for the hidden ---skip-chdir option. (main): Accept the above new option. Move down the chdir() call after the if-clause to ensure it is run in any case - unless ---skip-chdir is specified. * 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. Change the "unknown option" test to use the really "--unknown-option", as the previous "---" would now be a valid abbreviation for the new hidden option. * NEWS (Changes in behavior): Mention the fix. * 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 --- NEWS | 5 +++++ init.cfg | 3 ++- src/chroot.c | 15 +++++++++++---- tests/cp/preserve-gid.sh | 3 ++- tests/cp/special-bits.sh | 3 ++- tests/id/setgid.sh | 8 ++++---- tests/misc/chroot-fail.sh | 36 +++++++++++++++++++++++++++-------- tests/misc/truncate-owned-by-other.sh | 2 +- tests/mv/sticky-to-xpart.sh | 5 +++-- tests/rm/fail-2eperm.sh | 6 ++++-- tests/rm/no-give-up.sh | 2 +- tests/touch/now-owned-by-other.sh | 2 +- 12 files changed, 64 insertions(+), 26 deletions(-) diff --git a/NEWS b/NEWS index 511e626..d5fefde 100644 --- a/NEWS +++ b/NEWS @@ -2,6 +2,11 @@ GNU coreutils NEWS -*- outline -*- * Noteworthy changes in release ?.? (????-??-??) [?] +** Changes in behavior + + chroot changes the current directory to "/" in any case again. + [bug introduced in coreutils-8.23] + * Noteworthy changes in release 8.23 (2014-07-18) [stable] diff --git a/init.cfg b/init.cfg index 725ee12..987b8a4 100644 --- a/init.cfg +++ b/init.cfg @@ -400,7 +400,8 @@ nonroot_has_perm_() require_built_ chroot local 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 diff --git a/src/chroot.c b/src/chroot.c index 6c2d63f..d835935 100644 --- a/src/chroot.c +++ b/src/chroot.c @@ -49,13 +49,15 @@ static inline bool gid_unset (gid_t gid) { return gid == (gid_t) -1; } enum { GROUPS = UCHAR_MAX + 1, - USERSPEC + USERSPEC, + SKIP_CHDIR }; static struct option const long_opts[] = { {"groups", required_argument, NULL, GROUPS}, {"userspec", required_argument, NULL, USERSPEC}, + {"-skip-chdir", no_argument, NULL, SKIP_CHDIR}, /* hidden option */ {GETOPT_HELP_OPTION_DECL}, {GETOPT_VERSION_OPTION_DECL}, {NULL, 0, NULL, 0} @@ -218,6 +220,7 @@ main (int argc, char **argv) char *userspec = NULL; char const *username = NULL; char const *groups = NULL; + bool skip_chdir = false; /* Parsed user and group IDs. */ uid_t uid = -1; @@ -254,6 +257,10 @@ main (int argc, char **argv) groups = optarg; break; + case SKIP_CHDIR: + skip_chdir = true; + break; + case_GETOPT_HELP_CHAR; case_GETOPT_VERSION_CHAR (PROGRAM_NAME, AUTHORS); @@ -310,11 +317,11 @@ main (int argc, char **argv) if (chroot (argv[optind]) != 0) error (EXIT_CANCELED, errno, _("cannot change root directory to %s"), argv[optind]); - - if (chdir ("/")) - error (EXIT_CANCELED, errno, _("cannot chdir to root directory")); } + if (! skip_chdir && chdir ("/")) + error (EXIT_CANCELED, errno, _("cannot chdir to root directory")); + if (argc == optind + 1) { /* No command. Run an interactive shell. */ diff --git a/tests/cp/preserve-gid.sh b/tests/cp/preserve-gid.sh index f141ac1..3af8c75 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 a55eea2..3ce52ae 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 6d9d74f..1e271cc 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 a84826f..77bf1aa 100755 --- a/tests/misc/chroot-fail.sh +++ b/tests/misc/chroot-fail.sh @@ -26,11 +26,11 @@ require_built_ chroot # them actually run a command, we don't need root privileges chroot # missing argument test $? = 125 || fail=1 -chroot --- / true # unknown option +chroot --unknown-option / true 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,11 +38,31 @@ 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 - curdir=$(chroot "$dir" env pwd) || fail=1 - test "$curdir" = '/' && fail=1 -done +# Ensure that chroot("/") succeeds - because chroot("/") should be skipped. +# This is a prerequesite for the following test. +chroot '/' env pwd || fail=1 + +# Ensure we don't chroot("/") ... also for synonyms of "/". +# Nevertheless, chdir("/") should usually be run - unless the +# hidden ---skip-chdir option is specified. +# Skip the tests on systems where chroot("/") succeeds for +# a non-root user. +run=1 +chroot . env pwd >out 2>err && run=0 +grep 'chroot: cannot change root directory to .*: Operation not permitted' err \ + || run=0 + +if test $run = 1; then + for dir in '/' '/.' '/../'; do + # Verify that chroot(1) succeeds and performs chdir("/"). + curdir=$(chroot "$dir" env pwd) || fail=1 + test "$curdir" = '/' || fail=1 + + # Test the hidden "---skip-chdir" option, too. + curdir=$(chroot ---skip-chdir "$dir" env pwd) || fail=1 + test "$curdir" = '/' && fail=1 + done + +fi Exit $fail diff --git a/tests/misc/truncate-owned-by-other.sh b/tests/misc/truncate-owned-by-other.sh index e70badb..77982fe 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 e0c99e9..7b57c22 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 6e8ce9b..ba70845 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 41070c9..276d3a0 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 d01097e..449cdf4 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 -- 1.8.4.5
