MIME-Version: 1.0
Date: Mon, 6 Jul 2015 12:20:52 -0700
Subject: Re: Group man to group root privilege escalation
From: Kees Cook
To: Andy Lutomirski
Cc: Willy Tarreau, Linus Torvalds, "security at kernel.org", halfdog, Al Viro
Content-Type: text/plain; charset=UTF-8

On Fri, Jun 26, 2015 at 2:20 PM, Andy Lutomirski wrote:
> On Thu, Jun 25, 2015 at 7:55 AM, Willy Tarreau wrote:
>> On Thu, Jun 25, 2015 at 07:47:23AM -0700, Andy Lutomirski wrote:
>>> On Thu, Jun 25, 2015 at 7:38 AM, Willy Tarreau wrote:
>>> > On Thu, Jun 25, 2015 at 07:31:49AM -0700, Andy Lutomirski wrote:
>>> >> On Wed, Jun 24, 2015 at 11:09 PM, Willy Tarreau wrote:
>>> >> > On Wed, Jun 24, 2015 at 03:06:21PM -0700, Linus Torvalds wrote:
>>> >> >> On Wed, Jun 24, 2015 at 2:45 PM, Linus Torvalds wrote:
>>> >> >> >
>>> >> >> > Ahh, yes. Ok, I agree, we should probably do both - use f_cred for the
>>> >> >> > suid removal check, *and* make sure that group-sticky directories
>>> >> >> > clear the sgid bit on file creation.
>>> >> >>
>>> >> >> Maybe something fairly simple like this for the SGID case?
>>> >> >>
>>> >> >>                     Linus
>>> >> >
>>> >> >>  fs/inode.c | 2 ++
>>> >> >>  1 file changed, 2 insertions(+)
>>> >> >>
>>> >> >> diff --git a/fs/inode.c b/fs/inode.c
>>> >> >> index e8d62688ed91..7bde0ad673e4 100644
>>> >> >> --- a/fs/inode.c
>>> >> >> +++ b/fs/inode.c
>>> >> >> @@ -1898,6 +1898,8 @@ void inode_init_owner(struct inode *inode, const struct inode *dir,
>>> >> >>               inode->i_gid = dir->i_gid;
>>> >> >>               if (S_ISDIR(mode))
>>> >> >>                       mode |= S_ISGID;
>>> >> >> +             else if ((mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP))
>>> >> >> +                     mode &= ~S_ISGID;
>>> >> >
>>> >> > Why limit it only to cases where S_IXGRP is set ? Wouldn't it be more
>>> >> > consistent to always clear it ? That's easier to document and remember
>>> >> > in my opinion, that would basically just be :
>>> >> >
>>> >> > +               else
>>> >> > +                       mode &= ~S_ISGID;
>>> >> >
>>> >> > Am I missing something ?
>>> >>
>>> >> I tend to agree.  We could also make this conditional on dir->i_gid !=
>>> >> current's gid.
>>> >
>>> > I'd be cautious here, for having seen situations where this was actually
>>> > used. That's how I discovered that it was possible to create a directory
>>> > belonging to a group you're not in (I was still young by then). I believe
>>> > it was a mail service with the setgid to group mail recursively causing
>>> > the creation of everything in group mail. So I fear that doing so could
>>> > possibly break existing applications.
>>> >
>>>
>>> I meant only clearing setgid if dir->i_gid != current's gid *and* the
>>> directory is setgid.  This should be a little more compatible with
>>> weird use cases.
>>
>> Ah OK then, but I don't see how this case can happen then, since you need
>> to have either to end up with the gid in the directory.
>
> I mean:
>
> ... else if ((mode & S_ISGID) != 0 && inode->i_gid != current_fsgid())
>
> In the problematic case, if you create a setgid file in a setgid
> directory, then the file won't be setgid.  But if your fsgid is the
> directory's gid, then the directory's setgid attribute has no effect
> and it should be safe to create the file with setgid set, right?

Which cases are required by POSIX? Can we really get away with
stripping S_ISGID?

-Kees

-- 
Kees Cook
Chrome OS Security