Messages in this thread | | | From | Andy Lutomirski <> | Date | Tue, 19 Jan 2016 11:26:50 -0800 | Subject | should_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 > >
| |