Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Mime-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset=UTF-8
Date:   Tue, 23 May 2023 22:31:29 +0300
Message-Id: <CSTWO5EA6LPR.32BAX2H2GMPZ6@suppilovahvero>
From:   "Jarkko Sakkinen" <jarkko@kernel.org>
To:     =?utf-8?q?Christian_G=C3=B6ttsche?= <cgzones@googlemail.com>,
        <selinux@vger.kernel.org>
Cc:     "David Howells" <dhowells@redhat.com>,
        "Paul Moore" <paul@paul-moore.com>,
        "James Morris" <jmorris@namei.org>,
        "Serge E. Hallyn" <serge@hallyn.com>, <keyrings@vger.kernel.org>,
        <linux-security-module@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] security: keys: perform capable check only on
 privileged operations
References: <20230511123252.723185-1-cgzones@googlemail.com>
In-Reply-To: <20230511123252.723185-1-cgzones@googlemail.com>
Precedence: bulk

On Thu May 11, 2023 at 3:32 PM EEST, Christian G=C3=B6ttsche 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=C3=B6ttsche <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 =3D -EACCES;
>  	down_write(&key->sem);
> =20
> -	if (!capable(CAP_SYS_ADMIN)) {
> +	{
> +		bool is_privileged_op =3D false;
> +
>  		/* only the sysadmin can chown a key to some other UID */
>  		if (user !=3D (uid_t) -1 && !uid_eq(key->uid, uid))
> -			goto error_put;
> +			is_privileged_op =3D true;
> =20
>  		/* 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 !=3D (gid_t) -1 && !gid_eq(gid, key->gid) && !in_group_p(gid=
))
> +			is_privileged_op =3D true;
> +
> +		if (is_privileged_op && !capable(CAP_SYS_ADMIN))
>  			goto error_put;
>  	}
> =20
> @@ -1088,7 +1093,7 @@ long keyctl_setperm_key(key_serial_t id, key_perm_t=
 perm)
>  	down_write(&key->sem);
> =20
>  	/* 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 =3D perm;
>  		notify_key(key, NOTIFY_KEY_SETATTR, 0);
>  		ret =3D 0;
> --=20
> 2.40.1

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

BR, Jarkko