linux-kernel - should_remove_suid capable check is busted

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-ID: <CALCETrXvyguKpRsyBC_6AGOxzSdMZ43Q5w_3zzAThsm+R91LSg@mail.gmail.com>
Date:	Tue, 19 Jan 2016 11:26:50 -0800
From:	Andy Lutomirski <luto@...nel.org>
To:	Konstantin Khlebnikov <koct9i@...il.com>
Cc:	Willy Tarreau <w@....eu>, Kees Cook <keescook@...omium.org>,
	Alexander Viro <viro@...iv.linux.org.uk>,
	"linux-mm@...ck.org" <linux-mm@...ck.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	yalin wang <yalin.wang2010@...il.com>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Jan Kara <jack@...e.cz>
Subject: should_remove_suid capable check is busted

On Jan 14, 2016 10:36 PM, "Konstantin Khlebnikov" <koct9i@...il.com> wrote:
>
> On Fri, Jan 15, 2016 at 9:18 AM, Andy Lutomirski <luto@...capital.net> 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
> >