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
> >
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>