On Mon, 2020-02-03 at 10:29 -0500, Stephen Smalley wrote:
> On 2/3/20 9:13 AM, Richard Haines wrote:
> > Add a new 'key_perms' policy capability and support for the
> > additional
> > key permissions: inval, revoke, join, clear
>
> For future reference, subject line should have [PATCH] prefix; git
> send-email will do this for you automatically. Also, the subsystem
> prefix would conventionally be "keys,selinux:" to indicates that it
> touches the keys and selinux subsystems, no need for "security/".
>
> > Also fixes JOIN -> LINK permission translation when policy
> > capability 'keys_perms' = 0;
> >
> > The current "setattr" perm name remains and is used for
> > KEY_NEED_SETSEC.
> > This gives the following permissions for the 'key' class:
> >
> > create Create a key or keyring.
> > view View attributes.
> > read Read contents.
> > write Update or modify.
> > search Search (keyring) or find (key).
> > link Link a key into the keyring.
> > setattr kernel < 5.X Change permissions on a keyring.
> > kernel >= 5.X Set owner, group, ACL.
> > inval Invalidate key.
> > revoke Revoke key.
> > join Join keyring as session.
> > clear Clear a keyring.
> >
> > Signed-off-by: Richard Haines <richard_c_haines@xxxxxxxxxxxxxx>
> > Signed-off-by: David Howells <dhowells@xxxxxxxxxx>
> > ---
> > include/linux/key.h | 3 +-
> > security/keys/keyctl.c | 3 +-
> > security/selinux/hooks.c | 73
> > ++++++++++++++++++++++++++++++-------
> > security/selinux/include/classmap.h | 2 +-
> > security/selinux/include/security.h | 8 ++++
> > security/selinux/ss/services.c | 3 +-
> > 6 files changed, 74 insertions(+), 18 deletions(-)
> >
> > diff --git a/include/linux/key.h b/include/linux/key.h
> > index 24c69457783f..ddfc0709569b 100644
> > --- a/include/linux/key.h
> > +++ b/include/linux/key.h
> > @@ -435,7 +435,8 @@ extern void key_free_user_ns(struct
> > user_namespace *);
> > #define KEY_NEED_REVOKE 0x080 /* Require permission to
> > revoke key */
> > #define KEY_NEED_JOIN 0x100 /* Require permission to
> > join keyring as session */
> > #define KEY_NEED_CLEAR 0x200 /* Require permission to
> > clear a keyring */
> > -#define KEY_NEED_ALL 0x3ff
> > +#define KEY_NEED_PARENT_JOIN 0x400 /* Require permission to impose
> > keyring on parent */
> > +#define KEY_NEED_ALL 0x7ff
> >
> > #define OLD_KEY_NEED_SETATTR 0x20 /* Used to be Require
> > permission to change attributes */
> >
> > diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
> > index a0c97d4d8251..65e1c0c3feb1 100644
> > --- a/security/keys/keyctl.c
> > +++ b/security/keys/keyctl.c
> > @@ -1592,7 +1592,8 @@ long keyctl_session_to_parent(void)
> > struct cred *cred;
> > int ret;
> >
> > - keyring_r = lookup_user_key(KEY_SPEC_SESSION_KEYRING, 0,
> > KEY_NEED_JOIN);
> > + keyring_r = lookup_user_key(KEY_SPEC_SESSION_KEYRING, 0,
> > + KEY_NEED_PARENT_JOIN);
>
> I'm unclear on how this works with the regular key permission
> checking
> (not SELinux). There is no KEY_ACE_PARENT_JOIN permission AFAICT
> and
> the check would fail if the regular permissions were only
> KEY_ACE_JOIN.
> What we need is an additional flag that will get ignored by
> key_permission() for its permission checking but signify to SELinux
> that
> different handling is required here.
David will need to answer this.
>
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index c8db5235b01f..a499bd7d9777 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -6565,18 +6566,62 @@ static int selinux_key_permission(key_ref_t
> > key_ref,
> > if (perm == 0)
> > return 0;
> >
> > - oldstyle_perm = perm & (KEY_NEED_VIEW | KEY_NEED_READ |
> > KEY_NEED_WRITE |
> > - KEY_NEED_SEARCH | KEY_NEED_LINK);
> > - if (perm & KEY_NEED_SETSEC)
> > - oldstyle_perm |= OLD_KEY_NEED_SETATTR;
> > - if (perm & KEY_NEED_INVAL)
> > - oldstyle_perm |= KEY_NEED_SEARCH;
> > - if (perm & KEY_NEED_REVOKE && !(perm & OLD_KEY_NEED_SETATTR))
> > - oldstyle_perm |= KEY_NEED_WRITE;
> > - if (perm & KEY_NEED_JOIN)
> > - oldstyle_perm |= KEY_NEED_SEARCH;
> > - if (perm & KEY_NEED_CLEAR)
> > - oldstyle_perm |= KEY_NEED_WRITE;
> > + if (selinux_policycap_key_perms()) {
> > + while (count) {
> > + switch_perm = bit & perm;
> > + switch (switch_perm) {
> > + case KEY_NEED_VIEW:
> > + key_perm |= KEY__VIEW;
> > + break;
> > + case KEY_NEED_READ:
> > + key_perm |= KEY__READ;
> > + break;
> > + case KEY_NEED_WRITE:
> > + key_perm |= KEY__WRITE;
> > + break;
> > + case KEY_NEED_SEARCH:
> > + key_perm |= KEY__SEARCH;
> > + break;
> > + case KEY_NEED_LINK:
> > + key_perm |= KEY__LINK;
> > + break;
> > + case KEY_NEED_SETSEC:
> > + key_perm |= KEY__SETATTR;
> > + break;
> > + case KEY_NEED_INVAL:
> > + key_perm |= KEY__INVAL;
> > + break;
> > + case KEY_NEED_REVOKE:
> > + key_perm |= KEY__REVOKE;
> > + break;
> > + case KEY_NEED_JOIN:
> > + case KEY_NEED_PARENT_JOIN:
> > + key_perm |= KEY__JOIN;
> > + break;
> > + case KEY_NEED_CLEAR:
> > + key_perm |= KEY__CLEAR;
> > + break;
> > + }
> > + bit <<= 1;
> > + count >>= 1;
> > + }
>
> So I assume then that it is unreasonable to have key_permission()
> limit
> its inputs to single-permission-at-a-time checks and therefore we
> have
> to permit multiple permissions? David? If not, is there any concern
> about the performance overhead of this loop and if so should we
> optimize
> for the common case of a single permission?
I have a single perm patch as well if that is required
>
> > diff --git a/security/selinux/include/security.h
> > b/security/selinux/include/security.h
> > index ae840634e3c7..6b264b6d9d31 100644
> > --- a/security/selinux/include/security.h
> > +++ b/security/selinux/include/security.h
> > @@ -79,6 +79,7 @@ enum {
> > POLICYDB_CAPABILITY_ALWAYSNETWORK,
> > POLICYDB_CAPABILITY_CGROUPSECLABEL,
> > POLICYDB_CAPABILITY_NNP_NOSUID_TRANSITION,
> > + POLICYDB_CAPABILITY_KEYPERMS,
> > __POLICYDB_CAPABILITY_MAX
> > };
> > #define POLICYDB_CAPABILITY_MAX (__POLICYDB_CAPABILITY_MAX - 1)
> > @@ -178,6 +179,13 @@ static inline bool
> > selinux_policycap_nnp_nosuid_transition(void)
>
> This will collide with
> https://lore.kernel.org/selinux/20200128191656.111902-1-cgzones@xxxxxxxxxxxxxx/
> so we'll have to sort out which one goes first.
