Message ID | bf8c202514709ea09c20ea9932e1e5b91be07310.1513103024.git.khalid.elmously@canonical.com |
---|---|
State | New |
Headers | show |
Series | [1/1] v2: CVE-2015-1350 fs: Avoid premature clearing of capabilities | expand |
On 13.12.2017 03:15, Khalid Elmously 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. > > > (backported from commit 030b533c4fd4d2ec3402363323de4bb2983c9cee) > [kmously: This is a re-implementation of the upstream commit that doesn't include all the refactoring that was done in upstream) > > CVE-2015-1350 > > > References: CVE-2015-1350 > Reviewed-by: Christoph Hellwig <hch@lst.de> > Signed-off-by: Jan Kara <jack@suse.cz> > Signed-off-by: Khalid Elmously <khalid.elmously@canonical.com> Acked-by: Stefan Bader <stefan.bader@canonical.com> > > > > --- Based on the suggestions from Thadeu this looks ok. Personally I am not 100% sure whether the usage of d_obtain_alias() is valid or not. -Stefan > fs/attr.c | 24 ++++++++++++++++++------ > 1 file changed, 18 insertions(+), 6 deletions(-) > > diff --git a/fs/attr.c b/fs/attr.c > index df05bc167360..870d45103f29 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,20 @@ 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; > + > + dentry = d_obtain_alias(inode); > + if (!IS_ERR(dentry)) { > + error = security_inode_killpriv(dentry); > + dput(dentry); > + return error; > + } > + } > + > return 0; > } > EXPORT_SYMBOL(inode_change_ok); > @@ -250,13 +264,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 2018-01-23 12:32:49 , Stefan Bader wrote: > On 13.12.2017 03:15, Khalid Elmously 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. > > > > > > (backported from commit 030b533c4fd4d2ec3402363323de4bb2983c9cee) > > [kmously: This is a re-implementation of the upstream commit that doesn't include all the refactoring that was done in upstream) > > > > CVE-2015-1350 > > > > > > References: CVE-2015-1350 > > Reviewed-by: Christoph Hellwig <hch@lst.de> > > Signed-off-by: Jan Kara <jack@suse.cz> > > Signed-off-by: Khalid Elmously <khalid.elmously@canonical.com> > Acked-by: Stefan Bader <stefan.bader@canonical.com> > > > > > > > > > --- > > Based on the suggestions from Thadeu this looks ok. Personally I am not 100% > sure whether the usage of d_obtain_alias() is valid or not. Yes, I thought as much, which I stated in my response to Thadeu on 2017-12-11. For what it's worth, I had also sent the complete set of patches (9 in total) needed for this CVE on 2017-12-07 - so you can review/accept that instead, if you prefer. > > -Stefan > > > fs/attr.c | 24 ++++++++++++++++++------ > > 1 file changed, 18 insertions(+), 6 deletions(-) > > > > diff --git a/fs/attr.c b/fs/attr.c > > index df05bc167360..870d45103f29 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,20 @@ 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; > > + > > + dentry = d_obtain_alias(inode); > > + if (!IS_ERR(dentry)) { > > + error = security_inode_killpriv(dentry); > > + dput(dentry); > > + return error; > > + } > > + } > > + > > return 0; > > } > > EXPORT_SYMBOL(inode_change_ok); > > @@ -250,13 +264,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; > > } > > > > /* > > > > Khaled > -- > kernel-team mailing list > kernel-team@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team
On 23.01.2018 12:32, Stefan Bader wrote: > On 13.12.2017 03:15, Khalid Elmously 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. >> >> >> (backported from commit 030b533c4fd4d2ec3402363323de4bb2983c9cee) >> [kmously: This is a re-implementation of the upstream commit that doesn't include all the refactoring that was done in upstream) >> >> CVE-2015-1350 >> >> >> References: CVE-2015-1350 >> Reviewed-by: Christoph Hellwig <hch@lst.de> >> Signed-off-by: Jan Kara <jack@suse.cz> >> Signed-off-by: Khalid Elmously <khalid.elmously@canonical.com> > Acked-by: Stefan Bader <stefan.bader@canonical.com> > >> >> >> >> --- > > Based on the suggestions from Thadeu this looks ok. Personally I am not 100% > sure whether the usage of d_obtain_alias() is valid or not. I did not want to object to this submission. Just to state that I am looking for someone else (possibly with better understanding of fs code) to give some feedback. Anybody? Mueller? > > -Stefan > >> fs/attr.c | 24 ++++++++++++++++++------ >> 1 file changed, 18 insertions(+), 6 deletions(-) >> >> diff --git a/fs/attr.c b/fs/attr.c >> index df05bc167360..870d45103f29 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,20 @@ 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; >> + >> + dentry = d_obtain_alias(inode); >> + if (!IS_ERR(dentry)) { >> + error = security_inode_killpriv(dentry); >> + dput(dentry); >> + return error; >> + } >> + } >> + >> return 0; >> } >> EXPORT_SYMBOL(inode_change_ok); >> @@ -250,13 +264,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 df05bc167360..870d45103f29 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,20 @@ 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; + + dentry = d_obtain_alias(inode); + if (!IS_ERR(dentry)) { + error = security_inode_killpriv(dentry); + dput(dentry); + return error; + } + } + return 0; } EXPORT_SYMBOL(inode_change_ok); @@ -250,13 +264,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; } /*