X-Received: by 10.152.1.40 with SMTP id 8mr46179906laj.56.1435243663405; Thu,
 25 Jun 2015 07:47:43 -0700 (PDT)
MIME-Version: 1.0
Received: by 10.152.170.233 with HTTP; Thu, 25 Jun 2015 07:47:23 -0700 (PDT)
From: Andy Lutomirski
Date: Thu, 25 Jun 2015 07:47:23 -0700
Subject: Re: Group man to group root privilege escalation
To: Willy Tarreau
Cc: Linus Torvalds, "security at kernel.org", halfdog, Kees Cook, Al Viro
Content-Type: text/plain; charset=UTF-8

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.

--Andy