Message ID | 1534419915-1335-1-git-send-email-jose.pekkarinen@canonical.com |
---|---|
State | New |
Headers | show |
Series | [T,CVE-2015-1350] fs: Avoid premature clearing of capabilities | expand |
On 08/16/18 13:45, José Pekkarinen wrote: > CVE-2015-1350 > > Buglink: https://launchpad.net/bugs/1415636 > > 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. Same comments as for Xenial: References: CVE-2015-1350 > Reviewed-by: Christoph Hellwig <hch@lst.de> > Signed-off-by: Jan Kara <jack@suse.cz> (backported from 030b533c4fd4d2ec3402363323de4bb2983c9cee) > Signed-off-by: José Pekkarinen <jose.pekkarinen@canonical.com> > --- > fs/attr.c | 23 +++++++++++++++++------ > 1 file changed, 17 insertions(+), 6 deletions(-) > > diff --git a/fs/attr.c b/fs/attr.c > index 6530ced..f58bbc8 100644 > --- a/fs/attr.c > +++ b/fs/attr.c > @@ -44,7 +44,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) && > @@ -77,6 +77,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); > @@ -217,13 +230,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; > } > > /* >
On 20.08.2018 08:59, Kleber Souza wrote: > On 08/16/18 13:45, José Pekkarinen wrote: >> CVE-2015-1350 >> >> Buglink: https://launchpad.net/bugs/1415636 >> >> 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. > > Same comments as for Xenial: Reasons-> xenial > > > References: CVE-2015-1350 >> Reviewed-by: Christoph Hellwig <hch@lst.de> >> Signed-off-by: Jan Kara <jack@suse.cz> > (backported from 030b533c4fd4d2ec3402363323de4bb2983c9cee) >> Signed-off-by: José Pekkarinen <jose.pekkarinen@canonical.com> >> --- >> fs/attr.c | 23 +++++++++++++++++------ >> 1 file changed, 17 insertions(+), 6 deletions(-) >> >> diff --git a/fs/attr.c b/fs/attr.c >> index 6530ced..f58bbc8 100644 >> --- a/fs/attr.c >> +++ b/fs/attr.c >> @@ -44,7 +44,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) && >> @@ -77,6 +77,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); >> @@ -217,13 +230,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; >> } >> >> /* >> > >
diff --git a/fs/attr.c b/fs/attr.c index 6530ced..f58bbc8 100644 --- a/fs/attr.c +++ b/fs/attr.c @@ -44,7 +44,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) && @@ -77,6 +77,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); @@ -217,13 +230,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; } /*