| Message ID | 20230511123252.723185-1-cgzones@googlemail.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | security: keys: perform capable check only on privileged operations | expand |
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.
On Fri, 19 May 2023 at 23:08, Paul Moore <paul@paul-moore.com> wrote: > > 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. This complication I exactly wanted to avoid. For me the inner scope encapsulates the all the logic around the capability check just fine and is quite readable. An alternative would be to create a new function performing the checks and call it via if (!chown_key_capable(key, user, uid, group, gid)) got error_put; A minor inconvenience is the number of needed arguments (and the actual code after inlining should be the same to the inner scope in the end). > > -- > paul-moore.com
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
On Tue, May 23, 2023 at 2:33 PM Christian Göttsche <cgzones@googlemail.com> wrote: > On Fri, 19 May 2023 at 23:08, Paul Moore <paul@paul-moore.com> wrote: > > 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. > > This complication I exactly wanted to avoid. For me the inner scope > encapsulates the all the logic around the capability check just fine > and is quite readable. An alternative would be to create a new > function performing the checks and call it via > > if (!chown_key_capable(key, user, uid, group, gid)) > got error_put; > > A minor inconvenience is the number of needed arguments (and the > actual code after inlining should be the same to the inner scope in > the end). Well, lucky for you, Jarkko and David maintain the keys code, not me, and Jarkko seems to like your patch just fine :) Jarkko, I assume you'll be taking this via the keys tree?
On Thu, 2023-05-25 at 17:25 -0400, Paul Moore wrote: > > A minor inconvenience is the number of needed arguments (and the > > actual code after inlining should be the same to the inner scope in > > the end). > > Well, lucky for you, Jarkko and David maintain the keys code, not me, > and Jarkko seems to like your patch just fine :) > > Jarkko, I assume you'll be taking this via the keys tree? I just picked it and mirrored to linux-next. I think it is super important change because it tones down the human error (a little bit at least). You could say improves user experience kind of I guess :-) BR, Jarkko
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;
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(-)