Re: security/selinux: Add support for new key permissions

[Richard Haines <richard_c_haines@btinternet.com>]

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@btinternet.com>
> > Signed-off-by: David Howells <dhowells@redhat.com>
> > ---
> >   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@googlemail.com/ 
> so we'll have to sort out which one goes first.