| Message ID | cover.1512634012.git.khalid.elmously@canonical.com |
|---|---|
| Headers | show |
| Series | Fix for CVE-2015-1350 | expand |
On Thu, Dec 07, 2017 at 03:37:48AM -0500, Khalid Elmously wrote: > The VFS subsystem in the Linux kernel 3.x provides an incomplete set of > requirements for setattr operations that underspecifies removing extended > privilege attributes, which allows local users to cause a denial of service > (capability stripping) via a failed invocation of a system call, as > demonstrated by using chown to remove a capability from the ping or > Wireshark dumpcap program. > > > > Al Viro (1): > ->getxattr(): pass dentry and inode as separate arguments > > Andreas Gruenbacher (1): > ceph: Get rid of d_find_alias in ceph_set_acl > > Jan Kara (5): > xfs: Propagate dentry down to inode_change_ok() > ceph: Propagate dentry down to inode_change_ok() > fuse: Propagate dentry down to inode_change_ok() > fs: Give dentry to inode_change_ok() instead of inode > fs: Avoid premature clearing of capabilities > > Khalid Elmously (2): > wrappers for ->i_mutex access > xattr_handler: pass dentry and inode as separate arguments of ->get() The whole point of the fix is patch 9 ("fs: Avoid premature clearing of capabilities"). I see how picking up patch 8 ("fs: Give dentry to inode_change_ok() instead of inode") will make it a clean cherry-pick. But that makes you bring all those changes to all filesystems and all those other patches, then, are needed. Why not just backport patch 9 and make the change to inode_change_ok? I see how it uses the dentry instead of the inode, but why not just call d_obtain_alias (with corresponding dput after dentry use)? That would simplify the whole patchset. Though after some review of the other patches, they seem to be fine aside from the usual style that we have for backports (using -x from cherry-pick, instead of Upstream commit style). Regards. Cascardo.
On Thu, Dec 07, 2017 at 03:37:48AM -0500, Khalid Elmously wrote: > The VFS subsystem in the Linux kernel 3.x provides an incomplete set of > requirements for setattr operations that underspecifies removing extended > privilege attributes, which allows local users to cause a denial of service > (capability stripping) via a failed invocation of a system call, as > demonstrated by using chown to remove a capability from the ping or > Wireshark dumpcap program. One other thing: we stopped using bugs for CVEs. So, instead of a BugLink, you should have a line as the one below all by itself. I usually put it after the cherry-pick/backport line, right before my SOB. CVE-2015-1350
Thanks for reviewing. On 2017-12-07 09:56:55 , Thadeu Lima de Souza Cascardo wrote: > On Thu, Dec 07, 2017 at 03:37:48AM -0500, Khalid Elmously wrote: > > The VFS subsystem in the Linux kernel 3.x provides an incomplete set of > > requirements for setattr operations that underspecifies removing extended > > privilege attributes, which allows local users to cause a denial of service > > (capability stripping) via a failed invocation of a system call, as > > demonstrated by using chown to remove a capability from the ping or > > Wireshark dumpcap program. > > > > > > > > Al Viro (1): > > ->getxattr(): pass dentry and inode as separate arguments > > > > Andreas Gruenbacher (1): > > ceph: Get rid of d_find_alias in ceph_set_acl > > > > Jan Kara (5): > > xfs: Propagate dentry down to inode_change_ok() > > ceph: Propagate dentry down to inode_change_ok() > > fuse: Propagate dentry down to inode_change_ok() > > fs: Give dentry to inode_change_ok() instead of inode > > fs: Avoid premature clearing of capabilities > > > > Khalid Elmously (2): > > wrappers for ->i_mutex access > > xattr_handler: pass dentry and inode as separate arguments of ->get() > > The whole point of the fix is patch 9 ("fs: Avoid premature clearing of > capabilities"). I see how picking up patch 8 ("fs: Give dentry to > inode_change_ok() instead of inode") will make it a clean cherry-pick. > But that makes you bring all those changes to all filesystems and all > those other patches, then, are needed. > > Why not just backport patch 9 and make the change to inode_change_ok? I > see how it uses the dentry instead of the inode, but why not just call > d_obtain_alias (with corresponding dput after dentry use)? That would > simplify the whole patchset. The main reason is that I didn't want to come up with a solution myself since I don't really know if would be "correct" or not. I tested the solution you propose here and it seems to work fine, though to be honest I still don't know if it's correct or not. I was surprised that your proposal works because: a) Much of the refactoring that was done just before 030b533c4fd4d2ec3402363323de4bb2983c9cee (which is the patch we want) was done specifically for 030b533c4fd4d2ec3402363323de4bb2983c9cee, so I expected it to be necessary for a proper fix. b) My understanding was that there's a 1-to-many relationship between inodes and dentries (i.e hardlinks), thus calling security_inode_killpriv(d_obtain_alias(inode)) won't necessarily operate on the right dentry. However, since testing your suggestion I've realized that capabailities are set on the inode not the dentry so it doesn't even seem to matter which dentry is specified for killpriv(). The bottom line is that your proposal seems correct and testing shows that it fixes the problem, though I don't actually _know_ if it's the correct solution. I will go with your approach unless I receive objections. Thanks for the feedback. > Though after some review of the other > patches, they seem to be fine aside from the usual style that we have > for backports (using -x from cherry-pick, instead of Upstream commit > style). > > Regards. > Cascardo. Khaled