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

[Stefan Bader]

On 08.08.2018 10:29, José Pekkarinen wrote:

The way you submitted this patch, you claim authorship of the patch, which is
untrue. This is either a cherry-pick or backport of the following upstream patch:

commit 030b533c4fd4d2ec3402363323de4bb2983c9cee
Author: Jan Kara <jack at suse.cz>
Date:   Thu May 26 17:21:32 2016 +0200

> CVE-2015-1350
> 
> Buglink: https://launchpad.net/bugs/1415636

For CVE related patches we stopped using additional tracking bugs. Tracking is
done via the references to upstream SHA1 in the cherry pick/backport line. Which
makes it vital to exist.
> 
> 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 at lst.de>
Signed-off-by: Jan Kara <jack at suse.cz>

(backported from commit 030b533c4fd4d2ec3402363323de4bb2983c9cee)
 ^ or cherry picked ... (if patch applied unmodified)
> Signed-off-by: José Pekkarinen <jose.pekkarinen at canonical.com>

Upstream SHA1 reference is missing and applying upstream patches one picks the
*complete* commit message, including the signed-off lines.

-Stefan
> ---
>  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;
>  	}
>  
>  	/*
> 


-------------- 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/20180816/1d0becb3/attachment.sig>