[<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
> >
Powered by blists - more mailing lists