Bug#770492: [RFC PATCH RESEND] vfs: Move security_inode_killpriv() after permission checks
On Mon, Apr 13, 2015 at 11:39:01AM +1000, James Morris wrote: On Wed, 8 Apr 2015, Mateusz Guzik wrote: This is still a problem. Any feedback about the patch? I'd like to see feedback from vfs folk (Al). Ping? Are there any concerns with the patch? -- Mateusz Guzik -- To UNSUBSCRIBE, email to debian-bugs-rc-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#770492: [RFC PATCH RESEND] vfs: Move security_inode_killpriv() after permission checks
On Wed, 8 Apr 2015, Mateusz Guzik wrote: This is still a problem. Any feedback about the patch? I'd like to see feedback from vfs folk (Al). -- James Morris jmor...@namei.org -- To UNSUBSCRIBE, email to debian-bugs-rc-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#770492: [RFC PATCH RESEND] vfs: Move security_inode_killpriv() after permission checks
On Sat, Jan 17, 2015 at 11:26:46PM +, Ben Hutchings wrote: chown() and write() should clear all privilege attributes on a file - setuid, setgid, setcap and any other extended privilege attributes. However, any attributes beyond setuid and setgid are managed by the LSM and not directly by the filesystem, so they cannot be set along with the other attributes. Currently we call security_inode_killpriv() in notify_change(), but in case of a chown() this is too early - we have not called inode_change_ok() or made any filesystem-specific permission/sanity checks. Add a new function setattr_killpriv() which calls security_inode_killpriv() if necessary, and change the setattr() implementation to call this in each filesystem that supports xattrs. This assumes that extended privilege attributes are always stored in xattrs. Compile-tested only. XXX This is a silent change to the VFS API, but we should probably change something so OOT filesystems fail to compile if they aren't updated to call setattr_killpriv(). This is still a problem. Any feedback about the patch? Reported-by: Ben Harris bj...@cam.ac.uk References: https://bugs.debian.org/770492 --- drivers/staging/lustre/lustre/llite/llite_lib.c | 4 fs/9p/vfs_inode.c | 4 fs/9p/vfs_inode_dotl.c | 4 fs/attr.c | 32 + fs/btrfs/inode.c| 4 fs/ceph/inode.c | 4 fs/cifs/inode.c | 11 - fs/ext2/inode.c | 4 fs/ext3/inode.c | 4 fs/ext4/inode.c | 4 fs/f2fs/file.c | 4 fs/fuse/dir.c | 15 +++- fs/fuse/file.c | 3 ++- fs/fuse/fuse_i.h| 2 +- fs/gfs2/inode.c | 3 +++ fs/hfs/inode.c | 4 fs/hfsplus/inode.c | 4 fs/jffs2/fs.c | 4 fs/jfs/file.c | 4 fs/kernfs/inode.c | 17 + fs/libfs.c | 3 +++ fs/nfs/inode.c | 11 +++-- fs/ocfs2/file.c | 6 - fs/reiserfs/inode.c | 4 fs/ubifs/file.c | 4 fs/xfs/xfs_acl.c| 3 ++- fs/xfs/xfs_file.c | 2 +- fs/xfs/xfs_ioctl.c | 2 +- fs/xfs/xfs_iops.c | 16 ++--- fs/xfs/xfs_iops.h | 10 ++-- include/linux/fs.h | 1 + mm/shmem.c | 4 32 files changed, 176 insertions(+), 25 deletions(-) diff --git a/drivers/staging/lustre/lustre/llite/llite_lib.c b/drivers/staging/lustre/lustre/llite/llite_lib.c index a8bcc51..2a714b2 100644 --- a/drivers/staging/lustre/lustre/llite/llite_lib.c +++ b/drivers/staging/lustre/lustre/llite/llite_lib.c @@ -1434,6 +1434,10 @@ int ll_setattr_raw(struct dentry *dentry, struct iattr *attr, bool hsm_import) spin_unlock(lli-lli_lock); } + rc = setattr_killpriv(dentry, attr); + if (rc) + return rc; + /* We always do an MDS RPC, even if we're only changing the size; * only the MDS knows whether truncate() should fail with -ETXTBUSY */ diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c index 296482f..735cbf84 100644 --- a/fs/9p/vfs_inode.c +++ b/fs/9p/vfs_inode.c @@ -1130,6 +1130,10 @@ static int v9fs_vfs_setattr(struct dentry *dentry, struct iattr *iattr) if (S_ISREG(dentry-d_inode-i_mode)) filemap_write_and_wait(dentry-d_inode-i_mapping); + retval = setattr_killpriv(dentry, iattr); + if (retval) + return retval; + retval = p9_client_wstat(fid, wstat); if (retval 0) return retval; diff --git a/fs/9p/vfs_inode_dotl.c b/fs/9p/vfs_inode_dotl.c index 02b64f4..f3ca76d 100644 --- a/fs/9p/vfs_inode_dotl.c +++ b/fs/9p/vfs_inode_dotl.c @@ -583,6 +583,10 @@ int v9fs_vfs_setattr_dotl(struct dentry *dentry, struct iattr *iattr) if (S_ISREG(inode-i_mode)) filemap_write_and_wait(inode-i_mapping); + retval = setattr_killpriv(dentry, iattr); + if (retval) + return retval; + retval = p9_client_setattr(fid, p9attr); if (retval 0) return retval; diff --git a/fs/attr.c
Bug#770492: [RFC PATCH RESEND] vfs: Move security_inode_killpriv() after permission checks
On Sat, Jan 17, 2015 at 6:26 PM, Ben Hutchings b...@decadent.org.uk wrote: chown() and write() should clear all privilege attributes on a file - setuid, setgid, setcap and any other extended privilege attributes. However, any attributes beyond setuid and setgid are managed by the LSM and not directly by the filesystem, so they cannot be set along with the other attributes. Currently we call security_inode_killpriv() in notify_change(), but in case of a chown() this is too early - we have not called inode_change_ok() or made any filesystem-specific permission/sanity checks. Add a new function setattr_killpriv() which calls security_inode_killpriv() if necessary, and change the setattr() implementation to call this in each filesystem that supports xattrs. This assumes that extended privilege attributes are always stored in xattrs. Compile-tested only. XXX This is a silent change to the VFS API, but we should probably change something so OOT filesystems fail to compile if they aren't updated to call setattr_killpriv(). Reported-by: Ben Harris bj...@cam.ac.uk References: https://bugs.debian.org/770492 This seems to have stalled. I don't see it in linux-next or anywhere else I can find. The issue has a shiny CVE now, so it makes people that follow those nervous. Is there any further feedback or follow-up here? josh --- drivers/staging/lustre/lustre/llite/llite_lib.c | 4 fs/9p/vfs_inode.c | 4 fs/9p/vfs_inode_dotl.c | 4 fs/attr.c | 32 + fs/btrfs/inode.c| 4 fs/ceph/inode.c | 4 fs/cifs/inode.c | 11 - fs/ext2/inode.c | 4 fs/ext3/inode.c | 4 fs/ext4/inode.c | 4 fs/f2fs/file.c | 4 fs/fuse/dir.c | 15 +++- fs/fuse/file.c | 3 ++- fs/fuse/fuse_i.h| 2 +- fs/gfs2/inode.c | 3 +++ fs/hfs/inode.c | 4 fs/hfsplus/inode.c | 4 fs/jffs2/fs.c | 4 fs/jfs/file.c | 4 fs/kernfs/inode.c | 17 + fs/libfs.c | 3 +++ fs/nfs/inode.c | 11 +++-- fs/ocfs2/file.c | 6 - fs/reiserfs/inode.c | 4 fs/ubifs/file.c | 4 fs/xfs/xfs_acl.c| 3 ++- fs/xfs/xfs_file.c | 2 +- fs/xfs/xfs_ioctl.c | 2 +- fs/xfs/xfs_iops.c | 16 ++--- fs/xfs/xfs_iops.h | 10 ++-- include/linux/fs.h | 1 + mm/shmem.c | 4 32 files changed, 176 insertions(+), 25 deletions(-) diff --git a/drivers/staging/lustre/lustre/llite/llite_lib.c b/drivers/staging/lustre/lustre/llite/llite_lib.c index a8bcc51..2a714b2 100644 --- a/drivers/staging/lustre/lustre/llite/llite_lib.c +++ b/drivers/staging/lustre/lustre/llite/llite_lib.c @@ -1434,6 +1434,10 @@ int ll_setattr_raw(struct dentry *dentry, struct iattr *attr, bool hsm_import) spin_unlock(lli-lli_lock); } + rc = setattr_killpriv(dentry, attr); + if (rc) + return rc; + /* We always do an MDS RPC, even if we're only changing the size; * only the MDS knows whether truncate() should fail with -ETXTBUSY */ diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c index 296482f..735cbf84 100644 --- a/fs/9p/vfs_inode.c +++ b/fs/9p/vfs_inode.c @@ -1130,6 +1130,10 @@ static int v9fs_vfs_setattr(struct dentry *dentry, struct iattr *iattr) if (S_ISREG(dentry-d_inode-i_mode)) filemap_write_and_wait(dentry-d_inode-i_mapping); + retval = setattr_killpriv(dentry, iattr); + if (retval) + return retval; + retval = p9_client_wstat(fid, wstat); if (retval 0) return retval; diff --git a/fs/9p/vfs_inode_dotl.c b/fs/9p/vfs_inode_dotl.c index 02b64f4..f3ca76d 100644 --- a/fs/9p/vfs_inode_dotl.c +++ b/fs/9p/vfs_inode_dotl.c @@ -583,6 +583,10 @@ int v9fs_vfs_setattr_dotl(struct dentry *dentry, struct iattr *iattr) if (S_ISREG(inode-i_mode)) filemap_write_and_wait(inode-i_mapping); + retval =
Bug#770492: [RFC PATCH RESEND] vfs: Move security_inode_killpriv() after permission checks
On 1/21/2015 6:03 AM, Stephen Smalley wrote: On 01/20/2015 06:17 PM, James Morris wrote: On Sat, 17 Jan 2015, Ben Hutchings wrote: chown() and write() should clear all privilege attributes on a file - setuid, setgid, setcap and any other extended privilege attributes. However, any attributes beyond setuid and setgid are managed by the LSM and not directly by the filesystem, so they cannot be set along with the other attributes. Currently we call security_inode_killpriv() in notify_change(), but in case of a chown() this is too early - we have not called inode_change_ok() or made any filesystem-specific permission/sanity checks. Add a new function setattr_killpriv() which calls security_inode_killpriv() if necessary, and change the setattr() implementation to call this in each filesystem that supports xattrs. This assumes that extended privilege attributes are always stored in xattrs. It'd be useful to get some input from LSM module maintainers on this. e.g. doesn't SELinux already handle this via policy directives? There have been a couple postings of a similar patch set [1] by Jan Kara, although I don't believe that series addressed chown(). If I am reading the patches correctly, they (correctly) don't affect SELinux or Smack labels; they are just calling the existing security_inode_killpriv() hook, which is only implemented for the capability module to remove the security.capability xattr. The description of the change should say that. I can easily imagine an enthusiastic test developer reading the existing description and filing bugs because SELinux, Smack and whatever other xattr based systems might be around don't clear their attributes. If the intent wasn't clear to the first person to use xattrs for security purposes, I shouldn't expect the new and inexperienced to see it. My position softens. Document it correctly, and I'm fine with it. [1] http://marc.info/?l=linux-security-modulem=141890696325054w=2 -- To UNSUBSCRIBE, email to debian-bugs-rc-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#770492: [RFC PATCH RESEND] vfs: Move security_inode_killpriv() after permission checks
On 01/20/2015 06:17 PM, James Morris wrote: On Sat, 17 Jan 2015, Ben Hutchings wrote: chown() and write() should clear all privilege attributes on a file - setuid, setgid, setcap and any other extended privilege attributes. However, any attributes beyond setuid and setgid are managed by the LSM and not directly by the filesystem, so they cannot be set along with the other attributes. Currently we call security_inode_killpriv() in notify_change(), but in case of a chown() this is too early - we have not called inode_change_ok() or made any filesystem-specific permission/sanity checks. Add a new function setattr_killpriv() which calls security_inode_killpriv() if necessary, and change the setattr() implementation to call this in each filesystem that supports xattrs. This assumes that extended privilege attributes are always stored in xattrs. It'd be useful to get some input from LSM module maintainers on this. e.g. doesn't SELinux already handle this via policy directives? There have been a couple postings of a similar patch set [1] by Jan Kara, although I don't believe that series addressed chown(). If I am reading the patches correctly, they (correctly) don't affect SELinux or Smack labels; they are just calling the existing security_inode_killpriv() hook, which is only implemented for the capability module to remove the security.capability xattr. [1] http://marc.info/?l=linux-security-modulem=141890696325054w=2 -- To UNSUBSCRIBE, email to debian-bugs-rc-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#770492: [RFC PATCH RESEND] vfs: Move security_inode_killpriv() after permission checks
On Sat, 17 Jan 2015, Ben Hutchings wrote: chown() and write() should clear all privilege attributes on a file - setuid, setgid, setcap and any other extended privilege attributes. However, any attributes beyond setuid and setgid are managed by the LSM and not directly by the filesystem, so they cannot be set along with the other attributes. Currently we call security_inode_killpriv() in notify_change(), but in case of a chown() this is too early - we have not called inode_change_ok() or made any filesystem-specific permission/sanity checks. Add a new function setattr_killpriv() which calls security_inode_killpriv() if necessary, and change the setattr() implementation to call this in each filesystem that supports xattrs. This assumes that extended privilege attributes are always stored in xattrs. It'd be useful to get some input from LSM module maintainers on this. e.g. doesn't SELinux already handle this via policy directives? -- James Morris jmor...@namei.org -- To UNSUBSCRIBE, email to debian-bugs-rc-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#770492: [RFC PATCH RESEND] vfs: Move security_inode_killpriv() after permission checks
On 1/20/2015 3:17 PM, James Morris wrote: On Sat, 17 Jan 2015, Ben Hutchings wrote: chown() and write() should clear all privilege attributes on a file - setuid, setgid, setcap and any other extended privilege attributes. However, any attributes beyond setuid and setgid are managed by the LSM and not directly by the filesystem, so they cannot be set along with the other attributes. Currently we call security_inode_killpriv() in notify_change(), but in case of a chown() this is too early - we have not called inode_change_ok() or made any filesystem-specific permission/sanity checks. Add a new function setattr_killpriv() which calls security_inode_killpriv() if necessary, and change the setattr() implementation to call this in each filesystem that supports xattrs. This assumes that extended privilege attributes are always stored in xattrs. It'd be useful to get some input from LSM module maintainers on this. I've already chimed in. Clearing the Smack label on a file because someone writes to it makes no sense whatsoever. The same with chown. The Smack label is attached to the object, which is a container of data, not the data itself. Smack labels are Mandatory Access Control labels, not Information labels. If that doesn't mean anything to the reader, check out the P1003.1e/2c (withdrawn) DRAFT. The proposed implementation does not correctly handle either Mandatory Access Control labels or Information labels. The MAC label is *very different* from the setuid bit. e.g. doesn't SELinux already handle this via policy directives? -- To UNSUBSCRIBE, email to debian-bugs-rc-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#770492: [RFC PATCH RESEND] vfs: Move security_inode_killpriv() after permission checks
chown() and write() should clear all privilege attributes on a file - setuid, setgid, setcap and any other extended privilege attributes. However, any attributes beyond setuid and setgid are managed by the LSM and not directly by the filesystem, so they cannot be set along with the other attributes. Currently we call security_inode_killpriv() in notify_change(), but in case of a chown() this is too early - we have not called inode_change_ok() or made any filesystem-specific permission/sanity checks. Add a new function setattr_killpriv() which calls security_inode_killpriv() if necessary, and change the setattr() implementation to call this in each filesystem that supports xattrs. This assumes that extended privilege attributes are always stored in xattrs. Compile-tested only. XXX This is a silent change to the VFS API, but we should probably change something so OOT filesystems fail to compile if they aren't updated to call setattr_killpriv(). Reported-by: Ben Harris bj...@cam.ac.uk References: https://bugs.debian.org/770492 --- drivers/staging/lustre/lustre/llite/llite_lib.c | 4 fs/9p/vfs_inode.c | 4 fs/9p/vfs_inode_dotl.c | 4 fs/attr.c | 32 + fs/btrfs/inode.c| 4 fs/ceph/inode.c | 4 fs/cifs/inode.c | 11 - fs/ext2/inode.c | 4 fs/ext3/inode.c | 4 fs/ext4/inode.c | 4 fs/f2fs/file.c | 4 fs/fuse/dir.c | 15 +++- fs/fuse/file.c | 3 ++- fs/fuse/fuse_i.h| 2 +- fs/gfs2/inode.c | 3 +++ fs/hfs/inode.c | 4 fs/hfsplus/inode.c | 4 fs/jffs2/fs.c | 4 fs/jfs/file.c | 4 fs/kernfs/inode.c | 17 + fs/libfs.c | 3 +++ fs/nfs/inode.c | 11 +++-- fs/ocfs2/file.c | 6 - fs/reiserfs/inode.c | 4 fs/ubifs/file.c | 4 fs/xfs/xfs_acl.c| 3 ++- fs/xfs/xfs_file.c | 2 +- fs/xfs/xfs_ioctl.c | 2 +- fs/xfs/xfs_iops.c | 16 ++--- fs/xfs/xfs_iops.h | 10 ++-- include/linux/fs.h | 1 + mm/shmem.c | 4 32 files changed, 176 insertions(+), 25 deletions(-) diff --git a/drivers/staging/lustre/lustre/llite/llite_lib.c b/drivers/staging/lustre/lustre/llite/llite_lib.c index a8bcc51..2a714b2 100644 --- a/drivers/staging/lustre/lustre/llite/llite_lib.c +++ b/drivers/staging/lustre/lustre/llite/llite_lib.c @@ -1434,6 +1434,10 @@ int ll_setattr_raw(struct dentry *dentry, struct iattr *attr, bool hsm_import) spin_unlock(lli-lli_lock); } + rc = setattr_killpriv(dentry, attr); + if (rc) + return rc; + /* We always do an MDS RPC, even if we're only changing the size; * only the MDS knows whether truncate() should fail with -ETXTBUSY */ diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c index 296482f..735cbf84 100644 --- a/fs/9p/vfs_inode.c +++ b/fs/9p/vfs_inode.c @@ -1130,6 +1130,10 @@ static int v9fs_vfs_setattr(struct dentry *dentry, struct iattr *iattr) if (S_ISREG(dentry-d_inode-i_mode)) filemap_write_and_wait(dentry-d_inode-i_mapping); + retval = setattr_killpriv(dentry, iattr); + if (retval) + return retval; + retval = p9_client_wstat(fid, wstat); if (retval 0) return retval; diff --git a/fs/9p/vfs_inode_dotl.c b/fs/9p/vfs_inode_dotl.c index 02b64f4..f3ca76d 100644 --- a/fs/9p/vfs_inode_dotl.c +++ b/fs/9p/vfs_inode_dotl.c @@ -583,6 +583,10 @@ int v9fs_vfs_setattr_dotl(struct dentry *dentry, struct iattr *iattr) if (S_ISREG(inode-i_mode)) filemap_write_and_wait(inode-i_mapping); + retval = setattr_killpriv(dentry, iattr); + if (retval) + return retval; + retval = p9_client_setattr(fid, p9attr); if (retval 0) return retval; diff --git a/fs/attr.c b/fs/attr.c index 6530ced..184f3bf 100644 --- a/fs/attr.c +++ b/fs/attr.c @@ -168,6 +168,28 @@ void setattr_copy(struct inode *inode, const struct iattr *attr) EXPORT_SYMBOL(setattr_copy); /**
