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@xxxxxxxxxxxxxx>
> > > > Signed-off-by: David Howells <dhowells@xxxxxxxxxx>
> > > > ---
> > > > 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@xxxxxxxxxxxxxx/
> > > 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
