lkml.org 
[lkml]   [2016]   [Jan]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
Subjectshould_remove_suid capable check is busted
On Jan 14, 2016 10:36 PM, "Konstantin Khlebnikov" <koct9i@gmail.com> wrote:
>
> On Fri, Jan 15, 2016 at 9:18 AM, Andy Lutomirski <luto@amacapital.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
> >

\
 
 \ /
  Last update: 2016-01-19 21:01    [W:0.033 / U:0.720 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site