On Mon, Apr 27, 2020 at 1:02 PM Stephen Smalley
<stephen.smalley.work@xxxxxxxxx> wrote:
> On Mon, Apr 27, 2020 at 10:13 AM David Howells <dhowells@xxxxxxxxxx> wrote:
...
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index 0b4e32161b77..6087955b49d8 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -6539,20 +6539,38 @@ static void selinux_key_free(struct key *k)
> > kfree(ksec);
> > }
> >
> > +static unsigned int selinux_keyperm_to_av(unsigned int need_perm)
> > +{
> > + switch (need_perm) {
> > + case KEY_NEED_VIEW: return KEY__VIEW;
> > + case KEY_NEED_READ: return KEY__READ;
> > + case KEY_NEED_WRITE: return KEY__WRITE;
> > + case KEY_NEED_SEARCH: return KEY__SEARCH;
> > + case KEY_NEED_LINK: return KEY__LINK;
> > + case KEY_NEED_SETATTR: return KEY__SETATTR;
> > + default:
>
> Possibly WARN() or BUG() here? Or BUILD_BUG_ON(KEY_NEED_ALL != 0x3f)
> to force an update here
> whenever a new key permission is defined?
My vote is for a build time check via BUILD_BUG_ON() or similar
mechanism. It's worked really well in other places, I'm thinking
specifically of the SELinux netlink mapping.
> > + return 0;
> > + }
> > +}
> > +
> > static int selinux_key_permission(key_ref_t key_ref,
> > const struct cred *cred,
> > - unsigned perm)
> > + unsigned need_perm)
> > {
> > struct key *key;
> > struct key_security_struct *ksec;
> > + unsigned int perm;
> > u32 sid;
> >
> > /* if no specific permissions are requested, we skip the
> > permission check. No serious, additional covert channels
> > appear to be created. */
> > - if (perm == 0)
> > + if (need_perm == 0)
> > return 0;
> >
> > + perm = selinux_keyperm_to_av(need_perm);
> > + if (perm == 0)
> > + return -EACCES;
>
> We should log or audit some kind of message here, whether via WARN(),
> audit_log(), or something, to avoid silent denials.
Assuming we add a build time check (see above), I think a WARN here is okay.
--
paul moore
www.paul-moore.com
