| Message ID | 1474299059-14806-3-git-send-email-jack@suse.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 2016-09-19 at 17:30 +0200, Jan Kara wrote: > To avoid clearing of capabilities or security related extended > attributes too early, inode_change_ok() will need to take dentry instead > of inode. ceph_setattr() has the dentry easily available but > __ceph_setattr() is also called from ceph_set_acl() where dentry is not > easily available. Luckily that call path does not need inode_change_ok() > to be called anyway. So reorganize functions a bit so that > inode_change_ok() is called only from paths where dentry is available. > > > Signed-off-by: Jan Kara <jack@suse.cz> > --- > fs/ceph/acl.c | 5 +++++ > fs/ceph/inode.c | 19 +++++++++++-------- > 2 files changed, 16 insertions(+), 8 deletions(-) > > diff --git a/fs/ceph/acl.c b/fs/ceph/acl.c > index 013151d50069..a2cedfde75eb 100644 > --- a/fs/ceph/acl.c > +++ b/fs/ceph/acl.c > @@ -127,6 +127,11 @@ int ceph_set_acl(struct inode *inode, struct posix_acl *acl, int type) > > goto out_free; > > } > > > + if (ceph_snap(inode) != CEPH_NOSNAP) { > > + ret = -EROFS; > > + goto out_free; > > + } > + So to make sure I understand: What's the expected behavior when someone changes the ACL in such a way that the mode gets changed? Should security_inode_killpriv be getting called in that case? If so, where does that happen, as I don't see notify_change being called in this codepath? > if (new_mode != old_mode) { > > newattrs.ia_mode = new_mode; > > newattrs.ia_valid = ATTR_MODE; > diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c > index dd3a6dbf71eb..2aa3c0bcf3a5 100644 > --- a/fs/ceph/inode.c > +++ b/fs/ceph/inode.c > @@ -1905,13 +1905,6 @@ int __ceph_setattr(struct inode *inode, struct iattr *attr) > > int inode_dirty_flags = 0; > > bool lock_snap_rwsem = false; > > > - if (ceph_snap(inode) != CEPH_NOSNAP) > > - return -EROFS; > - > > - err = inode_change_ok(inode, attr); > > - if (err != 0) > > - return err; > - > > prealloc_cf = ceph_alloc_cap_flush(); > > if (!prealloc_cf) > > return -ENOMEM; > @@ -2124,7 +2117,17 @@ out_put: > */ > int ceph_setattr(struct dentry *dentry, struct iattr *attr) > { > > - return __ceph_setattr(d_inode(dentry), attr); > > + struct inode *inode = d_inode(dentry); > > + int err; > + > > + if (ceph_snap(inode) != CEPH_NOSNAP) > > + return -EROFS; > + > > + err = inode_change_ok(inode, attr); > > + if (err != 0) > > + return err; > + > > + return __ceph_setattr(inode, attr); > } > > /* Looks reasonable though: Acked-by: Jeff Layton <jlayton@redhat.com> -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon 19-09-16 14:57:02, Jeff Layton wrote: > On Mon, 2016-09-19 at 17:30 +0200, Jan Kara wrote: > > To avoid clearing of capabilities or security related extended > > attributes too early, inode_change_ok() will need to take dentry instead > > of inode. ceph_setattr() has the dentry easily available but > > __ceph_setattr() is also called from ceph_set_acl() where dentry is not > > easily available. Luckily that call path does not need inode_change_ok() > > to be called anyway. So reorganize functions a bit so that > > inode_change_ok() is called only from paths where dentry is available. > > > > > Signed-off-by: Jan Kara <jack@suse.cz> > > --- > > fs/ceph/acl.c | 5 +++++ > > fs/ceph/inode.c | 19 +++++++++++-------- > > 2 files changed, 16 insertions(+), 8 deletions(-) > > > > diff --git a/fs/ceph/acl.c b/fs/ceph/acl.c > > index 013151d50069..a2cedfde75eb 100644 > > --- a/fs/ceph/acl.c > > +++ b/fs/ceph/acl.c > > @@ -127,6 +127,11 @@ int ceph_set_acl(struct inode *inode, struct posix_acl *acl, int type) > > > goto out_free; > > > } > > > > > + if (ceph_snap(inode) != CEPH_NOSNAP) { > > > + ret = -EROFS; > > > + goto out_free; > > > + } > > + > > So to make sure I understand: What's the expected behavior when someone > changes the ACL in such a way that the mode gets changed? Should > security_inode_killpriv be getting called in that case? If so, where > does that happen, as I don't see notify_change being called in this > codepath? No. security_inode_killpriv() is supposed to be called when either contents of the file changes (truncate, write) or when owner of the file changes. It is not called for permission changes. ... > Looks reasonable though: > > Acked-by: Jeff Layton <jlayton@redhat.com> Thanks for review! Honza
On Thu, 2016-09-22 at 10:50 +0200, Jan Kara wrote: > On Mon 19-09-16 14:57:02, Jeff Layton wrote: > > > > On Mon, 2016-09-19 at 17:30 +0200, Jan Kara wrote: > > > > > > To avoid clearing of capabilities or security related extended > > > attributes too early, inode_change_ok() will need to take dentry instead > > > of inode. ceph_setattr() has the dentry easily available but > > > __ceph_setattr() is also called from ceph_set_acl() where dentry is not > > > easily available. Luckily that call path does not need inode_change_ok() > > > to be called anyway. So reorganize functions a bit so that > > > inode_change_ok() is called only from paths where dentry is available. > > > > > > > > > > > Signed-off-by: Jan Kara <jack@suse.cz> > > > --- > > > fs/ceph/acl.c | 5 +++++ > > > fs/ceph/inode.c | 19 +++++++++++-------- > > > 2 files changed, 16 insertions(+), 8 deletions(-) > > > > > > diff --git a/fs/ceph/acl.c b/fs/ceph/acl.c > > > index 013151d50069..a2cedfde75eb 100644 > > > --- a/fs/ceph/acl.c > > > +++ b/fs/ceph/acl.c > > > @@ -127,6 +127,11 @@ int ceph_set_acl(struct inode *inode, struct posix_acl *acl, int type) > > > > > > > > goto out_free; > > > > } > > > > > > > > > > > + if (ceph_snap(inode) != CEPH_NOSNAP) { > > > > + ret = -EROFS; > > > > + goto out_free; > > > > + } > > > + > > > > So to make sure I understand: What's the expected behavior when someone > > changes the ACL in such a way that the mode gets changed? Should > > security_inode_killpriv be getting called in that case? If so, where > > does that happen, as I don't see notify_change being called in this > > codepath? > > No. security_inode_killpriv() is supposed to be called when either contents > of the file changes (truncate, write) or when owner of the file changes. It > is not called for permission changes. > > ... Huh, ok... I would have thought that changing the mode of the file should remove capabilities though. Consider: Suppose we have a binary with extra file capabilities that is only executable by group owner. Attacker is able to change the permissions such that it's executable by world, and now anyone can get that capability since it wasn't revoked. IOW, I wonder if we may be trying to follow suit a little too closely with how the setuid bit works. When you call chmod(), you have to reinstate the setuid bit anyway if you want to keep it so not revoking it is ok there. Capabilities are stored separately though, so you don't need to make the same conscious decision to keep them there. Maybe the kernel should be revoking them when the mode changes? > > > > Looks reasonable though: > > > > Acked-by: Jeff Layton <jlayton@redhat.com> > > Thanks for review! > > Honza
> On 19 Sep 2016, at 23:30, Jan Kara <jack@suse.cz> wrote: > > To avoid clearing of capabilities or security related extended > attributes too early, inode_change_ok() will need to take dentry instead > of inode. ceph_setattr() has the dentry easily available but > __ceph_setattr() is also called from ceph_set_acl() where dentry is not > easily available. Luckily that call path does not need inode_change_ok() > to be called anyway. So reorganize functions a bit so that > inode_change_ok() is called only from paths where dentry is available. > > Signed-off-by: Jan Kara <jack@suse.cz> > --- > fs/ceph/acl.c | 5 +++++ > fs/ceph/inode.c | 19 +++++++++++-------- > 2 files changed, 16 insertions(+), 8 deletions(-) > > diff --git a/fs/ceph/acl.c b/fs/ceph/acl.c > index 013151d50069..a2cedfde75eb 100644 > --- a/fs/ceph/acl.c > +++ b/fs/ceph/acl.c > @@ -127,6 +127,11 @@ int ceph_set_acl(struct inode *inode, struct posix_acl *acl, int type) > goto out_free; > } > > + if (ceph_snap(inode) != CEPH_NOSNAP) { > + ret = -EROFS; > + goto out_free; > + } > + > if (new_mode != old_mode) { > newattrs.ia_mode = new_mode; > newattrs.ia_valid = ATTR_MODE; > diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c > index dd3a6dbf71eb..2aa3c0bcf3a5 100644 > --- a/fs/ceph/inode.c > +++ b/fs/ceph/inode.c > @@ -1905,13 +1905,6 @@ int __ceph_setattr(struct inode *inode, struct iattr *attr) > int inode_dirty_flags = 0; > bool lock_snap_rwsem = false; > > - if (ceph_snap(inode) != CEPH_NOSNAP) > - return -EROFS; > - > - err = inode_change_ok(inode, attr); > - if (err != 0) > - return err; > - > prealloc_cf = ceph_alloc_cap_flush(); > if (!prealloc_cf) > return -ENOMEM; > @@ -2124,7 +2117,17 @@ out_put: > */ > int ceph_setattr(struct dentry *dentry, struct iattr *attr) > { > - return __ceph_setattr(d_inode(dentry), attr); > + struct inode *inode = d_inode(dentry); > + int err; > + > + if (ceph_snap(inode) != CEPH_NOSNAP) > + return -EROFS; > + > + err = inode_change_ok(inode, attr); > + if (err != 0) > + return err; > + > + return __ceph_setattr(inode, attr); > } Acked-by: 'Yan, Zheng’ <zyan@redhat.com> > > /* > -- > 2.6.6 > -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu 22-09-16 13:20:48, Jeff Layton wrote: > On Thu, 2016-09-22 at 10:50 +0200, Jan Kara wrote: > > On Mon 19-09-16 14:57:02, Jeff Layton wrote: > > > > > > On Mon, 2016-09-19 at 17:30 +0200, Jan Kara wrote: > > > > > > > > To avoid clearing of capabilities or security related extended > > > > attributes too early, inode_change_ok() will need to take dentry instead > > > > of inode. ceph_setattr() has the dentry easily available but > > > > __ceph_setattr() is also called from ceph_set_acl() where dentry is not > > > > easily available. Luckily that call path does not need inode_change_ok() > > > > to be called anyway. So reorganize functions a bit so that > > > > inode_change_ok() is called only from paths where dentry is available. > > > > > > > > > > > > > > Signed-off-by: Jan Kara <jack@suse.cz> > > > > --- > > > > fs/ceph/acl.c | 5 +++++ > > > > fs/ceph/inode.c | 19 +++++++++++-------- > > > > 2 files changed, 16 insertions(+), 8 deletions(-) > > > > > > > > diff --git a/fs/ceph/acl.c b/fs/ceph/acl.c > > > > index 013151d50069..a2cedfde75eb 100644 > > > > --- a/fs/ceph/acl.c > > > > +++ b/fs/ceph/acl.c > > > > @@ -127,6 +127,11 @@ int ceph_set_acl(struct inode *inode, struct posix_acl *acl, int type) > > > > > > > > > > goto out_free; > > > > > } > > > > > > > > > > > > > > + if (ceph_snap(inode) != CEPH_NOSNAP) { > > > > > + ret = -EROFS; > > > > > + goto out_free; > > > > > + } > > > > + > > > > > > So to make sure I understand: What's the expected behavior when someone > > > changes the ACL in such a way that the mode gets changed? Should > > > security_inode_killpriv be getting called in that case? If so, where > > > does that happen, as I don't see notify_change being called in this > > > codepath? > > > > No. security_inode_killpriv() is supposed to be called when either contents > > of the file changes (truncate, write) or when owner of the file changes. It > > is not called for permission changes. > > > > ... > > Huh, ok... > > I would have thought that changing the mode of the file should remove > capabilities though. Consider: > > Suppose we have a binary with extra file capabilities that is only > executable by group owner. Attacker is able to change the permissions > such that it's executable by world, and now anyone can get that > capability since it wasn't revoked. IOW, I wonder if we may be trying > to follow suit a little too closely with how the setuid bit works. > > When you call chmod(), you have to reinstate the setuid bit anyway if > you want to keep it so not revoking it is ok there. Capabilities are > stored separately though, so you don't need to make the same conscious > decision to keep them there. Maybe the kernel should be revoking them > when the mode changes? So this is certainly an orthogonal issue and I don't want to complicate this patch set with it. But yeah, your arguments make some sense - inadvertedly exposing an excutable with additional capabilities may be an issue. However we behave like this for a long time so I'm not sure we have the liberty of changing the behavior. Anyway, I've added linux-security-module to CC if they have anything to say to this.
diff --git a/fs/ceph/acl.c b/fs/ceph/acl.c index 013151d50069..a2cedfde75eb 100644 --- a/fs/ceph/acl.c +++ b/fs/ceph/acl.c @@ -127,6 +127,11 @@ int ceph_set_acl(struct inode *inode, struct posix_acl *acl, int type) goto out_free; } + if (ceph_snap(inode) != CEPH_NOSNAP) { + ret = -EROFS; + goto out_free; + } + if (new_mode != old_mode) { newattrs.ia_mode = new_mode; newattrs.ia_valid = ATTR_MODE; diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c index dd3a6dbf71eb..2aa3c0bcf3a5 100644 --- a/fs/ceph/inode.c +++ b/fs/ceph/inode.c @@ -1905,13 +1905,6 @@ int __ceph_setattr(struct inode *inode, struct iattr *attr) int inode_dirty_flags = 0; bool lock_snap_rwsem = false; - if (ceph_snap(inode) != CEPH_NOSNAP) - return -EROFS; - - err = inode_change_ok(inode, attr); - if (err != 0) - return err; - prealloc_cf = ceph_alloc_cap_flush(); if (!prealloc_cf) return -ENOMEM; @@ -2124,7 +2117,17 @@ out_put: */ int ceph_setattr(struct dentry *dentry, struct iattr *attr) { - return __ceph_setattr(d_inode(dentry), attr); + struct inode *inode = d_inode(dentry); + int err; + + if (ceph_snap(inode) != CEPH_NOSNAP) + return -EROFS; + + err = inode_change_ok(inode, attr); + if (err != 0) + return err; + + return __ceph_setattr(inode, attr); } /*
To avoid clearing of capabilities or security related extended attributes too early, inode_change_ok() will need to take dentry instead of inode. ceph_setattr() has the dentry easily available but __ceph_setattr() is also called from ceph_set_acl() where dentry is not easily available. Luckily that call path does not need inode_change_ok() to be called anyway. So reorganize functions a bit so that inode_change_ok() is called only from paths where dentry is available. Signed-off-by: Jan Kara <jack@suse.cz> --- fs/ceph/acl.c | 5 +++++ fs/ceph/inode.c | 19 +++++++++++-------- 2 files changed, 16 insertions(+), 8 deletions(-)