ACK(ish): [Trusty][Xenial][PATCH 1/1] v3: CVE-2015-1350 fs: Avoid premature clearing of capabilities

[Khaled Elmously]

On 2018-01-23 12:32:49 , Stefan Bader wrote:
> On 13.12.2017 03:15, Khalid Elmously wrote:
> > From: Jan Kara <jack at 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 at lst.de>
> > Signed-off-by: Jan Kara <jack at suse.cz>
> > Signed-off-by: Khalid Elmously <khalid.elmously at canonical.com>
> Acked-by: Stefan Bader <stefan.bader at 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 at lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team