Re: [PATCH] security: keys: perform capable check only on privileged operations

["Jarkko Sakkinen" <jarkko@kernel.org>]

On Thu May 11, 2023 at 3:32 PM EEST, Christian Göttsche wrote:
> If the current task fails the check for the queried capability via
> `capable(CAP_SYS_ADMIN)` LSMs like SELinux generate a denial message.
> Issuing such denial messages unnecessarily can lead to a policy author
> granting more privileges to a subject than needed to silence them.
>
> Reorder CAP_SYS_ADMIN checks after the check whether the operation is
> actually privileged.
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> ---
>  security/keys/keyctl.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
> index d54f73c558f7..19be69fa4d05 100644
> --- a/security/keys/keyctl.c
> +++ b/security/keys/keyctl.c
> @@ -980,14 +980,19 @@ long keyctl_chown_key(key_serial_t id, uid_t user, gid_t group)
>  	ret = -EACCES;
>  	down_write(&key->sem);
>  
> -	if (!capable(CAP_SYS_ADMIN)) {
> +	{
> +		bool is_privileged_op = false;
> +
>  		/* only the sysadmin can chown a key to some other UID */
>  		if (user != (uid_t) -1 && !uid_eq(key->uid, uid))
> -			goto error_put;
> +			is_privileged_op = true;
>  
>  		/* only the sysadmin can set the key's GID to a group other
>  		 * than one of those that the current process subscribes to */
>  		if (group != (gid_t) -1 && !gid_eq(gid, key->gid) && !in_group_p(gid))
> +			is_privileged_op = true;
> +
> +		if (is_privileged_op && !capable(CAP_SYS_ADMIN))
>  			goto error_put;
>  	}
>  
> @@ -1088,7 +1093,7 @@ long keyctl_setperm_key(key_serial_t id, key_perm_t perm)
>  	down_write(&key->sem);
>  
>  	/* if we're not the sysadmin, we can only change a key that we own */
> -	if (capable(CAP_SYS_ADMIN) || uid_eq(key->uid, current_fsuid())) {
> +	if (uid_eq(key->uid, current_fsuid()) || capable(CAP_SYS_ADMIN)) {
>  		key->perm = perm;
>  		notify_key(key, NOTIFY_KEY_SETATTR, 0);
>  		ret = 0;
> -- 
> 2.40.1

Acked-by: Jarkko Sakkinen <jarkko@kernel.org>

BR, Jarkko