should_remove_suid capable check is busted

[Andy Lutomirski]

On Jan 14, 2016 10:36 PM, "Konstantin Khlebnikov" <koct9i@xxxxxxxxx> wrote:
>
> On Fri, Jan 15, 2016 at 9:18 AM, Andy Lutomirski <luto@xxxxxxxxxxxxxx> wrote:
> > While we're at it:
> >
> > int should_remove_suid(struct dentry *dentry)
> > {
> >         umode_t mode = d_inode(dentry)->i_mode;
> >         int kill = 0;
> >
> >         /* suid always must be killed */
> >         if (unlikely(mode & S_ISUID))
> >                 kill = ATTR_KILL_SUID;
> >
> >         /*
> >          * sgid without any exec bits is just a mandatory locking mark; leave
> >          * it alone.  If some exec bits are set, it's a real sgid; kill it.
> >          */
> >         if (unlikely((mode & S_ISGID) && (mode & S_IXGRP)))
> >                 kill |= ATTR_KILL_SGID;
> >
> >         if (unlikely(kill && !capable(CAP_FSETID) && S_ISREG(mode)))
> >                 return kill;
> >
> >         return 0;
> > }
> > EXPORT_SYMBOL(should_remove_suid);
> >
> > Oh wait, is that an implicit use of current_cred in vfs_write?  No, it
> > couldn't be.  Kernel developers *never* make that mistake.
> >
> > This is, of course, totally fucked because this function doesn't have
> > access to a struct file and therefore can't see f_cred.  I'm not going
> > to look in to this right now, but I swear I saw an exploit that took
> > advantage of this bug recently.  Anyone want to try to fix it?
>
> Good point. it's here since 2.3.43.
> As I see file->f_cred is reachable in all places.

Nope, vfs_truncate doesn't have f_cred reachable.  All other call sites are fine

And here's the reference:

http://www.halfdog.net/Security/2015/SetgidDirectoryPrivilegeEscalation/

Seriously, can we get away with removing the capable() check outright?
 nfs already explicitly ignores capabilities for this purpose, and in
my opinion having a security decision on write depend the FSETID
capability is just BS.  I'm a bit afraid of breaking some package
manager, though.

What a clusterfsck.

--Andy

>
> >
> > FWIW, posix says (man 3p write):
> >
> >        Upon  successful  completion,  where  nbyte  is greater than 0, write()
> >        shall mark for update the last data modification and last  file  status
> >        change  timestamps  of the file, and if the file is a regular file, the
> >        S_ISUID and S_ISGID bits of the file mode may be cleared.
> >
> > so maybe the thing to do is just drop the capable check entirely and
> > cross our fingers that nothing was relying on it.
> >
> > --Andy
> >