Messages in this thread | | | From | Paul Moore <> | Date | Fri, 19 May 2023 17:07:51 -0400 | Subject | Re: [PATCH] security: keys: perform capable check only on privileged operations |
| |
On Thu, May 11, 2023 at 8:33 AM Christian Göttsche <cgzones@googlemail.com> 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; > }
Hmm. Using braces just to create a new scope is a bit hacky; I'll admit to using it to quickly create new local variables, but I only do so in debug/test situations, not production code.
What if you move the CAP_SYS_ADMIN check down into the if-conditional where the code checks to see if CAP_SYS_ADMIN is needed when changing the UID? It should be possible to structure the CAP_SYS_ADMIN check such that it is only executed if needed. It's a little more complicated in the GID case, but I believe it should be doable.
-- paul-moore.com
| |