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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 23 May 2023 22:31:29 +0300
From:   "Jarkko Sakkinen" <jarkko@...nel.org>
To:     Christian Göttsche <cgzones@...glemail.com>,
        <selinux@...r.kernel.org>
Cc:     "David Howells" <dhowells@...hat.com>,
        "Paul Moore" <paul@...l-moore.com>,
        "James Morris" <jmorris@...ei.org>,
        "Serge E. Hallyn" <serge@...lyn.com>, <keyrings@...r.kernel.org>,
        <linux-security-module@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] security: keys: perform capable check only on
 privileged operations

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@...glemail.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@...nel.org>

BR, Jarkko