[PATCH] fs: Avoid premature clearing of capabilities

[Jose Pekkarinen]

Please ignore this,

José.
On Thu, 16 Aug 2018 at 14:37, José Pekkarinen
<jose.pekkarinen at canonical.com> 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.
>
> References: CVE-2015-1350
> Reviewed-by: Christoph Hellwig <hch at lst.de>
> Signed-off-by: Jan Kara <jack at suse.cz>
> ---
>  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;
>         }
>
>         /*
> --
> 2.7.4
>