NACK: [T] [CVE-2015-1350] [PATCH] fs: Avoid premature clearing of capabilities

[Stefan Bader]

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 at lst.de>
>> Signed-off-by: Jan Kara <jack at suse.cz>
> (backported from 030b533c4fd4d2ec3402363323de4bb2983c9cee)
>> Signed-off-by: José Pekkarinen <jose.pekkarinen at 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;
>>  	}
>>  
>>  	/*
>>
> 
> 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20180820/7afe3bb9/attachment.sig>