public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
From: Josh Triplett <josh@joshtriplett.org>
To: "Theodore Ts'o" <tytso@mit.edu>,
	"Eric W. Biederman" <ebiederm@xmission.com>,
	Andy Lutomirski <luto@amacapital.net>,
	Andrew Morton <akpm@linux-foundation.org>,
	Kees Cook <keescook@chromium.org>,
	Michael Kerrisk-manpages <mtk.manpages@gmail.com>,
	Linux API <linux-api@vger.kernel.org>,
	linux-man <linux-man@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups
Date: Sun, 16 Nov 2014 11:09:38 -0800	[thread overview]
Message-ID: <20141116190938.GC5032@thin> (raw)
In-Reply-To: <20141116133230.GA32030@thunk.org>

On Sun, Nov 16, 2014 at 08:32:30AM -0500, Theodore Ts'o wrote:
> On Sat, Nov 15, 2014 at 09:08:07PM -0600, Eric W. Biederman wrote:
> > That may be a bug with the user namespace permission check.  Perhaps we
> > shouldn't allow dropping groups that aren't mapped in the user
> > namespace.
> 
> I'm not saying that we can't change the behavior of whether or not a
> user can drop a group permission.  I'm just saying that we need to do
> so consciously.

Agreed.

> The setgroups()/getgroups() ABI isn't part of
> POSIX/SuSv3 so we wouldn't be breaking POSIX compatibility, for those
> people who care about that.

POSIX.1-2001 actually specifies getgroups, but not setgroups.  In any
case, yes, POSIX doesn't say anything about this behavior.

> The bigger deal is that it's very different from how BSD 4.x has
> handled things, which means there is two decades of history that we're
> looking at here.  And there are times when taking away permissions in
> an expected fashion can cause security problems.  (As a silly example;
> some architect at Digital wrote a spec that said that setuid must
> return EINVAL for values greater than 32k --- back in the days when
> uid's were a signed short.  The junior programmer who implemented this
> for Ultrix made the check for 32,000 decimal.  Guess what happened
> when /bin/login got a failure with setuid when it wasn't expecting one
> --- since root could never get an error with that system call, right?

Ignored it and kept going, starting the user's shell as root?

I'd guess that a similar story motivated the note in the Linux manpages
for setuid, setresuid, and similar, saying "Note: there are cases where
setuid() can fail even when the caller is UID 0; it is a grave security
error to omit checking for a failure return from setuid().".

(Also, these days, glibc marks setuid and similar with the
warn_unused_result attribute.)

> And MIT Project Athena started ran out of lower numbered uid's and
> froshlings started getting assigned uid's > 32,000....)
> 
> In this particular case, the change is probably a little less likely
> to cause serious problems, although the fact that sudo does allow
> negative group assignments is an example of another potential
> breakage.
> 
> OTOH, I'm aware of how this could cause major problems to the concept
> of allowing an untrusted user to set up their own containers to
> constrain what program with a possibly untrusted provinance might be
> able to do.  I can see times when I might want to run in a container
> where the user didn't have access to groups that I have access to by
> default --- including groups such as disk, sudo, lpadmin, etc.
> 
> If we do want to make such a change, my suggestion is to keep things
> *very* simple.  Let it be a boot-time option whether or not users are
> allowed to drop group permissions, and let it affect all possible ways
> that users can drop groups.  And we can create a shell script that
> will search for the obvious ways that a user could get screwed by
> enabling this, which we can encourage distributions to package up for
> their end users.  And then we document the heck out of the fact that
> this option exists, and when/if we want to make it the default, so
> it's perfectly clear and transparent to all what is happening.

An option sounds sensible to me.  I think a sysctl makes more sense,
though.  I'll add one in v4.

What did you have in mind about the shell script? Something like:
grep -r !% /etc/sudoers /etc/sudoers.d
?

- Josh Triplett

  parent reply	other threads:[~2014-11-16 19:09 UTC|newest]

Thread overview: 37+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-11-15  9:00 [PATCH 1/2] groups: Factor out a function to set a pre-sorted group list Josh Triplett
2014-11-15  9:01 ` [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups Josh Triplett
2014-11-15 15:37   ` Eric W. Biederman
2014-11-15 19:29     ` Josh Triplett
2014-11-15 20:06       ` Andy Lutomirski
2014-11-15 20:20         ` Josh Triplett
2014-11-16  2:05           ` Theodore Ts'o
2014-11-16  2:35             ` Josh Triplett
2014-11-16  3:08               ` Eric W. Biederman
2014-11-16  5:07                 ` Josh Triplett
2014-11-16 13:32                 ` Theodore Ts'o
2014-11-16 15:42                   ` Andy Lutomirski
2014-11-16 19:12                     ` Josh Triplett
2014-11-16 19:09                   ` Josh Triplett [this message]
2014-11-16  3:40               ` Theodore Ts'o
2014-11-16  4:52                 ` Josh Triplett
2014-11-17 11:37                   ` One Thousand Gnomes
2014-11-17 18:07                     ` Andy Lutomirski
2014-11-17 22:11                       ` Eric W.Biederman
2014-11-17 22:22                         ` Andy Lutomirski
2014-11-17 22:37                           ` josh
2014-11-18  0:56                             ` Casey Schaufler
2014-11-17 18:06   ` Casey Schaufler
2014-11-17 18:31     ` Andy Lutomirski
2014-11-17 18:46       ` Andy Lutomirski
2014-11-17 18:51         ` Casey Schaufler
2014-11-27 16:59           ` [CFT][PATCH] userns: Avoid problems with negative groups Eric W. Biederman
2014-11-27 20:52             ` Andy Lutomirski
2014-11-28  5:21               ` Eric W. Biederman
2014-11-28  5:22                 ` [CFT][PATCH v2] " Eric W. Biederman
2014-11-28 15:11                 ` [CFT][PATCH] " Andy Lutomirski
2014-11-28 16:34                   ` Eric W. Biederman
2014-11-28 17:11                     ` Andy Lutomirski
2014-11-17 22:41         ` [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups Eric W.Biederman
2014-11-17 22:50           ` Andy Lutomirski
2014-11-17 23:13             ` josh
2014-11-15  9:01 ` [PATCH manpages] getgroups.2: Document unprivileged setgroups calls Josh Triplett

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20141116190938.GC5032@thin \
    --to=josh@joshtriplett.org \
    --cc=akpm@linux-foundation.org \
    --cc=ebiederm@xmission.com \
    --cc=keescook@chromium.org \
    --cc=linux-api@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-man@vger.kernel.org \
    --cc=luto@amacapital.net \
    --cc=mtk.manpages@gmail.com \
    --cc=tytso@mit.edu \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox