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

[Richard Haines <richard_c_haines@btinternet.com>]

On Thu, 2020-02-06 at 11:03 -0500, Stephen Smalley wrote:
> On 2/3/20 10:42 AM, Richard Haines wrote:
> > 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>
> > > > ---
> > > > 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.
> 
> Until this gets resolved, this patch must not go into mainline. 
> Otherwise we're looking at a potential userspace ABI issue when/if
> it 
> gets resolved.
> 
> > > > 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.
> 
> Already noted this elsewhere but for the sake of ensuring it is noted
> in 
> this thread too: the key_perms capability will need to be added
> after 
> the genfs_seclabel_symlinks capability since the latter is already 
> queued on selinux/next and the corresponding libsepol patch has been 
> merged upstream.

I'm still waiting for David to get back regarding the permission loop.

Anyway I'll update the patch as soon as I see
POLICYDB_CAPABILITY_GENFS_SECLABEL_SYMLINKS in Paul's 
https://github.com/SELinuxProject/selinux-kernel/tree/next