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@xxxxxxxxxxxxxx> > --- > 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@xxxxxxxxxx> BR, Jarkko