Message ID | 1534419492-17830-1-git-send-email-jose.pekkarinen@canonical.com |
---|---|
State | New |
Headers | show |
Series | fs: Avoid premature clearing of capabilities | expand |
Please ignore this, José. On Thu, 16 Aug 2018 at 14:37, José Pekkarinen <jose.pekkarinen@canonical.com> wrote: > > From: Jan Kara <jack@suse.cz> > > Currently, notify_change() clears capabilities or IMA attributes by > calling security_inode_killpriv() before calling into ->setattr. Thus it > happens before any other permission checks in inode_change_ok() and user > is thus allowed to trigger clearing of capabilities or IMA attributes > for any file he can look up e.g. by calling chown for that file. This is > unexpected and can lead to user DoSing a system. > > Fix the problem by calling security_inode_killpriv() at the end of > inode_change_ok() instead of from notify_change(). At that moment we are > sure user has permissions to do the requested change. > > References: CVE-2015-1350 > Reviewed-by: Christoph Hellwig <hch@lst.de> > Signed-off-by: Jan Kara <jack@suse.cz> > --- > fs/attr.c | 23 +++++++++++++++++------ > 1 file changed, 17 insertions(+), 6 deletions(-) > > diff --git a/fs/attr.c b/fs/attr.c > index df05bc1..870e4b6 100644 > --- a/fs/attr.c > +++ b/fs/attr.c > @@ -68,7 +68,7 @@ int inode_change_ok(const struct inode *inode, struct iattr *attr) > > /* If force is set do it anyway. */ > if (ia_valid & ATTR_FORCE) > - return 0; > + goto kill_priv; > > /* Make sure a caller can chown. */ > if ((ia_valid & ATTR_UID) && !chown_ok(inode, attr->ia_uid)) > @@ -95,6 +95,19 @@ int inode_change_ok(const struct inode *inode, struct iattr *attr) > return -EPERM; > } > > +kill_priv: > + /* User has permission for the change */ > + if (ia_valid & ATTR_KILL_PRIV) { > + int error; > + struct dentry * dentry = d_find_alias(inode); > + > + if (dentry) { > + error = security_inode_killpriv(dentry); > + if (error) > + return error; > + } > + } > + > return 0; > } > EXPORT_SYMBOL(inode_change_ok); > @@ -250,13 +263,11 @@ int notify_change(struct dentry * dentry, struct iattr * attr, struct inode **de > if (!(ia_valid & ATTR_MTIME_SET)) > attr->ia_mtime = now; > if (ia_valid & ATTR_KILL_PRIV) { > - attr->ia_valid &= ~ATTR_KILL_PRIV; > - ia_valid &= ~ATTR_KILL_PRIV; > error = security_inode_need_killpriv(dentry); > - if (error > 0) > - error = security_inode_killpriv(dentry); > - if (error) > + if (error < 0) > return error; > + if (error == 0) > + ia_valid = attr->ia_valid &= ~ATTR_KILL_PRIV; > } > > /* > -- > 2.7.4 >
diff --git a/fs/attr.c b/fs/attr.c index df05bc1..870e4b6 100644 --- a/fs/attr.c +++ b/fs/attr.c @@ -68,7 +68,7 @@ int inode_change_ok(const struct inode *inode, struct iattr *attr) /* If force is set do it anyway. */ if (ia_valid & ATTR_FORCE) - return 0; + goto kill_priv; /* Make sure a caller can chown. */ if ((ia_valid & ATTR_UID) && !chown_ok(inode, attr->ia_uid)) @@ -95,6 +95,19 @@ int inode_change_ok(const struct inode *inode, struct iattr *attr) return -EPERM; } +kill_priv: + /* User has permission for the change */ + if (ia_valid & ATTR_KILL_PRIV) { + int error; + struct dentry * dentry = d_find_alias(inode); + + if (dentry) { + error = security_inode_killpriv(dentry); + if (error) + return error; + } + } + return 0; } EXPORT_SYMBOL(inode_change_ok); @@ -250,13 +263,11 @@ int notify_change(struct dentry * dentry, struct iattr * attr, struct inode **de if (!(ia_valid & ATTR_MTIME_SET)) attr->ia_mtime = now; if (ia_valid & ATTR_KILL_PRIV) { - attr->ia_valid &= ~ATTR_KILL_PRIV; - ia_valid &= ~ATTR_KILL_PRIV; error = security_inode_need_killpriv(dentry); - if (error > 0) - error = security_inode_killpriv(dentry); - if (error) + if (error < 0) return error; + if (error == 0) + ia_valid = attr->ia_valid &= ~ATTR_KILL_PRIV; } /*