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

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.

Signed-off-by: José Pekkarinen <jose.pekkarinen@canonical.com>
---
 fs/attr.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

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@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@lst.de>
Signed-off-by: Jan Kara <jack@suse.cz>

(backported from commit 030b533c4fd4d2ec3402363323de4bb2983c9cee)
 ^ or cherry picked ... (if patch applied unmodified)
> Signed-off-by: José Pekkarinen <jose.pekkarinen@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;
>  	}
>  
>  	/*
>

On Thu, 16 Aug 2018 at 12:31, Stefan Bader <stefan.bader@canonical.com> wrote:
>
> 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:

    Hi,

    It's not really what I was looking for, the patch is slightly
modified, which is
not a reason to take ownership on it. I was just trying not to spam Jan on the
submission of the patch, but it seems git send-email doesn't allow it without
removing the reference.

    Best regards.

    José.

On 16.08.2018 13:26, Jose Pekkarinen wrote:
> On Thu, 16 Aug 2018 at 12:31, Stefan Bader <stefan.bader@canonical.com> wrote:
>>
>> 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:
> 
>     Hi,
> 
>     It's not really what I was looking for, the patch is slightly
> modified, which is
> not a reason to take ownership on it. I was just trying not to spam Jan on the
> submission of the patch, but it seems git send-email doesn't allow it without
> removing the reference.

This is what "--suppress-cc=<category>" is for.

-Stefan

> 
>     Best regards.
> 
>     José.
>

On Thu, 16 Aug 2018 at 14:32, Stefan Bader <stefan.bader@canonical.com> wrote:
> This is what "--suppress-cc=<category>" is for.

    Just happened to hit it on the man page, resending soon.

    Thanks for the hint and sorry for the noise!

    José.