GNU bug report logs - #18062
[PATCH] chroot: always change to / if not changing credentials

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 18062 in the body.
You can then email your comments to 18062 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Andreas Schwab <schwab <at> linux-m68k.org>
To: bug-coreutils <at> gnu.org
Subject: [PATCH] chroot: always change to / if not changing credentials
Date: Sun, 20 Jul 2014 14:05:18 +0200

---
 src/chroot.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/src/chroot.c b/src/chroot.c
index 6c2d63f..079759f 100644
--- a/src/chroot.c
+++ b/src/chroot.c
@@ -269,9 +269,10 @@ 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_root (argv[optind]))
+  /* Only do chroot specific actions if actually changing root or if not
+     changing credentials.  The main difference here is that we don't
+     change working dir.  */
+  if (! is_root (argv[optind]) || !(userspec || groups))
     {
       /* We have to look up users and groups twice.
         - First, outside the chroot to load potentially necessary passwd/group
-- 
2.0.2


-- 
Andreas Schwab, schwab <at> linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Message #8 received at 18062 <at> debbugs.gnu.org (full text, mbox):

From: Bernhard Voelker <mail <at> bernhard-voelker.de>
To: Andreas Schwab <schwab <at> linux-m68k.org>, 18062 <at> debbugs.gnu.org
Subject: Re: bug#18062: [PATCH] chroot: always change to / if not changing
 credentials
Date: Sun, 20 Jul 2014 16:36:29 +0200

On 07/20/2014 02:05 PM, Andreas Schwab wrote:
> diff --git a/src/chroot.c b/src/chroot.c
> index 6c2d63f..079759f 100644
> --- a/src/chroot.c
> +++ b/src/chroot.c
> @@ -269,9 +269,10 @@ 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_root (argv[optind]))
> +  /* Only do chroot specific actions if actually changing root or if not
> +     changing credentials.  The main difference here is that we don't
> +     change working dir.  */
> +  if (! is_root (argv[optind]) || !(userspec || groups))

This effectively reverts the idea behind v8.22-94-g99960ee:

    chroot: don't chdir() if not changing root

    This allows chroot to be used as a light weight tool
    to change user identification for a command,
    while not changing the current working directory.
    It also makes `chroot / true` consistently succeed on
    all platforms for non root users.

Now, with the patch:

  $ src/chroot / true
  src/chroot: cannot change root directory to /: Operation not permitted

... and some tests fail, too.

Why do you think the change is needed?
Is it about chroot(2) or chdir("/) being skipped?

Thanks & have a nice day,
Berny

Message #11 received at 18062 <at> debbugs.gnu.org (full text, mbox):

From: Andreas Schwab <schwab <at> linux-m68k.org>
To: Bernhard Voelker <mail <at> bernhard-voelker.de>
Cc: 18062 <at> debbugs.gnu.org
Subject: Re: bug#18062: [PATCH] chroot: always change to / if not changing
 credentials
Date: Sun, 20 Jul 2014 16:46:11 +0200

Bernhard Voelker <mail <at> bernhard-voelker.de> writes:

> This effectively reverts the idea behind v8.22-94-g99960ee:

This should be ripped out completely, chroot is not su.

> Why do you think the change is needed?

https://build.opensuse.org/package/live_build_log/home:AndreasSchwab/hdf/13.1/x86_64

Andreas.

-- 
Andreas Schwab, schwab <at> linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Message #14 received at 18062 <at> debbugs.gnu.org (full text, mbox):

From: Bernhard Voelker <mail <at> bernhard-voelker.de>
To: Andreas Schwab <schwab <at> linux-m68k.org>
Cc: 18062 <at> debbugs.gnu.org
Subject: Re: bug#18062: [PATCH] chroot: always change to / if not changing
 credentials
Date: Sun, 20 Jul 2014 17:30:08 +0200

On 07/20/2014 04:46 PM, Andreas Schwab wrote:
> Bernhard Voelker <mail <at> bernhard-voelker.de> writes:
>> This effectively reverts the idea behind v8.22-94-g99960ee:
> 
> This should be ripped out completely, chroot is not su.

Well, --userspec is a GNU extension - a very useful one IMO.
And why should "chroot /" invoke chroot(2) when it's a noop,
or even worse, on some systems depends on using an
administrative account?

>> Why do you think the change is needed?
> 
> https://build.opensuse.org/package/live_build_log/home:AndreasSchwab/hdf/13.1/x86_64

Your local patch [1] looks different - it also moves the chdir(1)
down to after the if-body.

[1]
https://build.opensuse.org/package/view_file/home:AndreasSchwab/coreutils/coreutils-chroot.patch?expand=1

Why's that?

Have a nice day,
Berny

Message #17 received at 18062 <at> debbugs.gnu.org (full text, mbox):

From: Andreas Schwab <schwab <at> linux-m68k.org>
To: Bernhard Voelker <mail <at> bernhard-voelker.de>
Cc: 18062 <at> debbugs.gnu.org
Subject: Re: bug#18062: [PATCH] chroot: always change to / if not changing
 credentials
Date: Sun, 20 Jul 2014 18:10:48 +0200

Bernhard Voelker <mail <at> bernhard-voelker.de> writes:

> And why should "chroot /" invoke chroot(2)

What else do you expect from a command called chroot???

> Why's that?

Don't waste your time with red herrings.

Andreas.

-- 
Andreas Schwab, schwab <at> linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Message #20 received at 18062 <at> debbugs.gnu.org (full text, mbox):

From: Bernhard Voelker <mail <at> bernhard-voelker.de>
To: Andreas Schwab <schwab <at> linux-m68k.org>
Cc: 18062 <at> debbugs.gnu.org, Jim Meyering <jim <at> meyering.net>
Subject: Re: bug#18062: [PATCH] chroot: always change to / if not changing
 credentials
Date: Mon, 21 Jul 2014 23:20:02 +0200

On 07/20/2014 06:10 PM, Andreas Schwab wrote:
> Bernhard Voelker <mail <at> bernhard-voelker.de> writes:
>> And why should "chroot /" invoke chroot(2)
> 
> What else do you expect from a command called chroot???

Let's resume:
1) The change to skip chroot() for the root directory and
synonyms was made for consistency with systems where this
is already allowed for non-root users by the kernel.
I consider this a good choice.

2) The same if-clause also skips the determination of the new
uid/gid/supplementary groups because the result would be the same
during the second determination _after_ chroot("/").
Note the functionality for changing the uid/gid/suppl. groups
had already been there and had just been improved for numeric ids.
This therefore was an optimization to omit redundant processing,
thus a good choice, too.

3) The choice for moving the chdir("/") inside the same if-clause
was made because it's cool to use things like
  chroot --user=$NON_ROOT_USERNAME / env PATH="$PATH" cp -p c c2
without the need to chdir() to the previous directory inside the
chroot jail.  Admittedly, this might break the expectations of
some previously existing use cases - as we see in your OBS log.
;-(

Now, what to do?

a) leave it as it is?
This would most probably break several scripts and cause much
unexpected work for our users.

b) revert part 1), i.e. chroot() for "/" again?
This would re-introduce previous discrepancy in behavior
on different systems.

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.

Please correct me if I overlooked something.

Jim?

Thanks & have a nice day,
Berny

Message #23 received at 18062 <at> debbugs.gnu.org (full text, mbox):

From: Bernhard Voelker <mail <at> bernhard-voelker.de>
To: Andreas Schwab <schwab <at> linux-m68k.org>
Cc: 18062 <at> debbugs.gnu.org, Jim Meyering <jim <at> meyering.net>
Subject: Re: bug#18062: [PATCH] chroot: always change to / if not changing
 credentials
Date: Wed, 23 Jul 2014 00:59:54 +0200

[Message part 1 (text/plain, inline)]

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

[0001-chroot-perform-chdir-in-any-case-again.patch (text/x-patch, attachment)]

Message #26 received at 18062 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Bernhard Voelker <mail <at> bernhard-voelker.de>
Cc: 18062 <at> debbugs.gnu.org, Andreas Schwab <schwab <at> linux-m68k.org>
Subject: Re: bug#18062: [PATCH] chroot: always change to / if not changing
 credentials
Date: Sun, 27 Jul 2014 21:32:40 +0100

On 07/21/2014 10:20 PM, Bernhard Voelker wrote:
> On 07/20/2014 06:10 PM, Andreas Schwab wrote:
>> Bernhard Voelker <mail <at> bernhard-voelker.de> writes:
>>> And why should "chroot /" invoke chroot(2)
>>
>> What else do you expect from a command called chroot???
> 
> Let's resume:
> 1) The change to skip chroot() for the root directory and
> synonyms was made for consistency with systems where this
> is already allowed for non-root users by the kernel.
> I consider this a good choice.
> 
> 2) The same if-clause also skips the determination of the new
> uid/gid/supplementary groups because the result would be the same
> during the second determination _after_ chroot("/").
> Note the functionality for changing the uid/gid/suppl. groups
> had already been there and had just been improved for numeric ids.
> This therefore was an optimization to omit redundant processing,
> thus a good choice, too.
> 
> 3) The choice for moving the chdir("/") inside the same if-clause
> was made because it's cool to use things like
>   chroot --user=$NON_ROOT_USERNAME / env PATH="$PATH" cp -p c c2
> without the need to chdir() to the previous directory inside the
> chroot jail.  Admittedly, this might break the expectations of
> some previously existing use cases - as we see in your OBS log.
> ;-(
> 
> Now, what to do?
> 
> a) leave it as it is?
> This would most probably break several scripts and cause much
> unexpected work for our users.
> 
> b) revert part 1), i.e. chroot() for "/" again?
> This would re-introduce previous discrepancy in behavior
> on different systems.
> 
> 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.

Drats. This change was initially discussed at:
http://lists.gnu.org/archive/html/coreutils/2014-05/msg00033.html
There I noted that we'd want to keep doing the chdir("/") for older
scripts that might assume the working dir = /.
I.E. when not invoking with --user we'd do the chdir("/"),
but then went ahead and fluffed the implementation.

Now on consideration it's probably best to not even key this change
on the --user option, and have a separate --chdir option?
I.E. since it's useful to maintain the current directory as seen
in the tests, we should be providing this functionality outside of tests also.

  chroot --user=$NON_ROOT_USERNAME --chdir=. / cp -p c c2

Now the syntax is getting a bit awkward for this use case,
though not too onerous I think since it gives a little extra functionality.

thanks,
Pádraig.

Message #29 received at 18062 <at> debbugs.gnu.org (full text, mbox):

From: Bernhard Voelker <mail <at> bernhard-voelker.de>
To: Pádraig Brady <P <at> draigBrady.com>
Cc: 18062 <at> debbugs.gnu.org, Andreas Schwab <schwab <at> linux-m68k.org>
Subject: Re: bug#18062: [PATCH] chroot: always change to / if not changing
 credentials
Date: Thu, 31 Jul 2014 09:19:56 +0200

On 07/27/2014 10:32 PM, Pádraig Brady wrote:
> Drats. This change was initially discussed at:
> http://lists.gnu.org/archive/html/coreutils/2014-05/msg00033.html
> There I noted that we'd want to keep doing the chdir("/") for older
> scripts that might assume the working dir = /.
> I.E. when not invoking with --user we'd do the chdir("/"),
> but then went ahead and fluffed the implementation.

At that point I wasn't that clear about the separation of the 3 tasks
in chroot(1): a) chroot(2), b) chdir(2), and c) finding/setting uid/gid
inside and outside the jail.  At least a) and b) got obviously a bit
mixed during the discussion.

> Now on consideration it's probably best to not even key this change
> on the --user option, and have a separate --chdir option?

As there is such a patch out now since more than a week:
http://lists.gnu.org/archive/html/bug-coreutils/2014-07/msg00083.html
would anyone comment on it?

Well, I took the way to add an internal "---skip-chdir" option,
but we can turn it into a publicly-visible "--skip-chdir" easily
if desired - although I don't see how such a probably-shoot-yourself-
in-the-foot option would help in real-world scripts. I think it'd
be clearer in such scripts to explicitly "cd" into the previous
directory.

Another idea was to re-introduce a 'setuidgid' tool built from
chroot.c without the chdir("/"), but that seemed even more awkward
than the ---skip-chdir solution.

Thanks & have a nice day,
Berny

Message #32 received at 18062 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Bernhard Voelker <mail <at> bernhard-voelker.de>
Cc: 18062 <at> debbugs.gnu.org, Andreas Schwab <schwab <at> linux-m68k.org>
Subject: Re: bug#18062: [PATCH] chroot: always change to / if not changing
 credentials
Date: Thu, 31 Jul 2014 10:56:15 +0100

On 07/31/2014 08:19 AM, Bernhard Voelker wrote:
> On 07/27/2014 10:32 PM, Pádraig Brady wrote:
>> Drats. This change was initially discussed at:
>> http://lists.gnu.org/archive/html/coreutils/2014-05/msg00033.html
>> There I noted that we'd want to keep doing the chdir("/") for older
>> scripts that might assume the working dir = /.
>> I.E. when not invoking with --user we'd do the chdir("/"),
>> but then went ahead and fluffed the implementation.
> 
> At that point I wasn't that clear about the separation of the 3 tasks
> in chroot(1): a) chroot(2), b) chdir(2), and c) finding/setting uid/gid
> inside and outside the jail.  At least a) and b) got obviously a bit
> mixed during the discussion.
> 
>> Now on consideration it's probably best to not even key this change
>> on the --user option, and have a separate --chdir option?
> 
> As there is such a patch out now since more than a week:
> http://lists.gnu.org/archive/html/bug-coreutils/2014-07/msg00083.html
> would anyone comment on it?
> 
> Well, I took the way to add an internal "---skip-chdir" option,
> but we can turn it into a publicly-visible "--skip-chdir" easily
> if desired

I'm leaning towards that.

> - although I don't see how such a probably-shoot-yourself-
> in-the-foot option would help in real-world scripts. I think it'd
> be clearer in such scripts to explicitly "cd" into the previous
> directory.

It might not always be run from a script,
and so would be awkward to go back to the current dir.
--skip-chdir is explicit so I don't really see the danger.
We could restrict the --skip-chdir functionality to when the
specified root dir = current root dir.

> Another idea was to re-introduce a 'setuidgid' tool built from
> chroot.c without the chdir("/"), but that seemed even more awkward
> than the ---skip-chdir solution.

There are various tools already available for this (previously itemized
on this list) so I don't think it warrants another separate tool.
Given the small extension to existing chroot(1) functionality,
I thought it was worth chroot having full support for this.

cheers,
Pádraig.

Message #35 received at 18062 <at> debbugs.gnu.org (full text, mbox):

From: Bernhard Voelker <mail <at> bernhard-voelker.de>
To: Pádraig Brady <P <at> draigBrady.com>
Cc: 18062 <at> debbugs.gnu.org, Andreas Schwab <schwab <at> linux-m68k.org>
Subject: Re: bug#18062: [PATCH] chroot: always change to / if not changing
 credentials
Date: Thu, 31 Jul 2014 15:31:32 +0200

On 07/31/2014 11:56 AM, Pádraig Brady wrote:
> On 07/31/2014 08:19 AM, Bernhard Voelker wrote:
>> Well, I took the way to add an internal "---skip-chdir" option,
>> but we can turn it into a publicly-visible "--skip-chdir" easily
>> if desired
>
> I'm leaning towards that.

Okay, I'll make it a regular option then ...

>> - although I don't see how such a probably-shoot-yourself-
>> in-the-foot option would help in real-world scripts. I think it'd
>> be clearer in such scripts to explicitly "cd" into the previous
>> directory.
>
> It might not always be run from a script,
> and so would be awkward to go back to the current dir.
> --skip-chdir is explicit so I don't really see the danger.
> We could restrict the --skip-chdir functionality to when the
> specified root dir = current root dir.

... with this restriction.
Getting back to you with the new patch soon (hopefully this night).

Thanks & have a nice day,
Berny

Message #38 received at 18062 <at> debbugs.gnu.org (full text, mbox):

From: Bernhard Voelker <mail <at> bernhard-voelker.de>
To: Pádraig Brady <P <at> draigBrady.com>
Cc: 18062 <at> debbugs.gnu.org, Andreas Schwab <schwab <at> linux-m68k.org>
Subject: Re: bug#18062: [PATCH] chroot: always change to / if not changing
 credentials
Date: Fri, 01 Aug 2014 02:12:22 +0200

[Message part 1 (text/plain, inline)]

On 07/31/2014 03:31 PM, Bernhard Voelker wrote:
> Getting back to you with the new patch soon (hopefully this night).

Here it is.

Thanks & have a nice day,
Berny

[0001-chroot-perform-chdir-again-unless-new-skip-chdir-is-.patch (text/x-patch, attachment)]

Message #41 received at 18062 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Bernhard Voelker <mail <at> bernhard-voelker.de>
Cc: 18062 <at> debbugs.gnu.org, Andreas Schwab <schwab <at> linux-m68k.org>
Subject: Re: bug#18062: [PATCH] chroot: always change to / if not changing
 credentials
Date: Fri, 01 Aug 2014 10:32:42 +0100

Looks good.

thanks!
Pádraig.

Message #44 received at 18062 <at> debbugs.gnu.org (full text, mbox):

From: Bernhard Voelker <mail <at> bernhard-voelker.de>
To: Pádraig Brady <P <at> draigBrady.com>
Cc: 18062 <at> debbugs.gnu.org, Andreas Schwab <schwab <at> linux-m68k.org>
Subject: Re: bug#18062: [PATCH] chroot: always change to / if not changing
 credentials
Date: Fri, 01 Aug 2014 14:29:22 +0200

On 08/01/2014 11:32 AM, Pádraig Brady wrote:
> Looks good.

Pushed:
https://git.sv.gnu.org/cgit/coreutils.git/commit/?id=0cf7b1d9

Thanks for the review!

Have a nice day,
Berny

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.