On Thu, 2020-02-20 at 14:28 -0500, Stephen Smalley wrote:
> On 2/20/20 1:10 PM, Richard Haines wrote:
> > Add a new 'key_perms' policy capability and support for the
> > additional
> > key permissions: inval, revoke, join, clear
> >
> > 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 < current version: Change permissions on a
> > keyring.
> > kernel >= current version: 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>
>
> What is this relative to? Didn't apply for me on any of keys-acl or
> keys-next or selinux-next.
I build this version of the patch on linux-next-next-20200214.tar.gz
>
> Regardless, we need to revert the original patch and create a new
> one
> that addresses the KEY_NEED_PARENT_JOIN issue I mentioned and that
> adds
> the key_perms capability in the right place in the first place, not
> apply a fix on top.
Yes I know. I was just making the point to David that as far as I can
see keys only ever passes one perm at a time.
>
> > ---
> > security/selinux/hooks.c | 123 ++++++++++++++++-------
> > -----
> > security/selinux/include/security.h | 10 +--
> > security/selinux/ss/services.c | 4 +-
> > 3 files changed, 76 insertions(+), 61 deletions(-)
> >
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index 46a8f3e7d..af179442c 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -6538,8 +6538,7 @@ static int selinux_key_permission(key_ref_t
> > key_ref,
> > {
> > struct key *key;
> > struct key_security_struct *ksec;
> > - unsigned int key_perm = 0, switch_perm = 0;
> > - int bit = 1, count = KEY_NEED_ALL;
> > + unsigned int key_perm = 0;
> > u32 sid;
> >
> > /* if no specific permissions are requested, we skip the
> > @@ -6549,60 +6548,73 @@ static int selinux_key_permission(key_ref_t
> > key_ref,
> > return 0;
> >
> > 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;
> > + 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;
> > + default:
> > + pr_err("BUG pcap = 1 entry_perm: 0x%04x\n",
> > perm);
> > + BUG();
> > + break;
> > }
> > } else {
> > - key_perm = perm & (KEY_NEED_VIEW | KEY_NEED_READ |
> > - KEY_NEED_WRITE | KEY_NEED_SEARCH |
> > - KEY_NEED_LINK);
> > - if (perm & KEY_NEED_PARENT_JOIN)
> > - key_perm |= KEY_NEED_LINK;
> > - if (perm & KEY_NEED_SETSEC)
> > - key_perm |= OLD_KEY_NEED_SETATTR;
> > - if (perm & KEY_NEED_INVAL)
> > - key_perm |= KEY_NEED_SEARCH;
> > - if (perm & KEY_NEED_REVOKE && !(perm &
> > OLD_KEY_NEED_SETATTR))
> > - key_perm |= KEY_NEED_WRITE;
> > - if (perm & KEY_NEED_JOIN)
> > - key_perm |= KEY_NEED_SEARCH;
> > - if (perm & KEY_NEED_CLEAR)
> > - key_perm |= KEY_NEED_WRITE;
> > + switch (perm) {
> > + case KEY_NEED_VIEW:
> > + key_perm = KEY_NEED_VIEW;
> > + break;
> > + case KEY_NEED_READ:
> > + key_perm = KEY_NEED_READ;
> > + break;
> > + case KEY_NEED_WRITE:
> > + case KEY_NEED_REVOKE:
> > + case KEY_NEED_CLEAR:
> > + key_perm = KEY_NEED_WRITE;
> > + break;
> > + case KEY_NEED_SEARCH:
> > + case KEY_NEED_INVAL:
> > + case KEY_NEED_JOIN:
> > + key_perm = KEY_NEED_SEARCH;
> > + break;
> > + case KEY_NEED_LINK:
> > + case KEY_NEED_PARENT_JOIN:
> > + key_perm = KEY_NEED_LINK;
> > + break;
> > + case KEY_NEED_SETSEC:
> > + key_perm = OLD_KEY_NEED_SETATTR;
> > + break;
> > + default:
> > + pr_err("BUG pcap = 0 entry_perm: 0x%04x\n",
> > perm);
> > + BUG();
> > + break;
> > + }
> > }
> >
> > sid = cred_sid(cred);
> > @@ -6610,6 +6622,9 @@ static int selinux_key_permission(key_ref_t
> > key_ref,
> > key = key_ref_to_ptr(key_ref);
> > ksec = key->security;
> >
> > + pr_info("entry_perm: 0x%04x exit_perm: 0x%04x\n",
> > + perm, key_perm);
> > +
> > return avc_has_perm(&selinux_state,
> > sid, ksec->sid, SECCLASS_KEY, key_perm,
> > NULL);
> > }
> > diff --git a/security/selinux/include/security.h
> > b/security/selinux/include/security.h
> > index cba9610b8..c0998e79d 100644
> > --- a/security/selinux/include/security.h
> > +++ b/security/selinux/include/security.h
> > @@ -79,8 +79,8 @@ enum {
> > POLICYDB_CAPABILITY_ALWAYSNETWORK,
> > POLICYDB_CAPABILITY_CGROUPSECLABEL,
> > POLICYDB_CAPABILITY_NNP_NOSUID_TRANSITION,
> > - POLICYDB_CAPABILITY_KEYPERMS,
> > POLICYDB_CAPABILITY_GENFS_SECLABEL_SYMLINKS,
> > + POLICYDB_CAPABILITY_KEYPERMS,
> > __POLICYDB_CAPABILITY_MAX
> > };
> > #define POLICYDB_CAPABILITY_MAX (__POLICYDB_CAPABILITY_MAX - 1)
> > @@ -215,18 +215,18 @@ static inline bool
> > selinux_policycap_nnp_nosuid_transition(void)
> > return state-
> > >policycap[POLICYDB_CAPABILITY_NNP_NOSUID_TRANSITION];
> > }
> >
> > -static inline bool selinux_policycap_key_perms(void)
> > +static inline bool selinux_policycap_genfs_seclabel_symlinks(void)
> > {
> > struct selinux_state *state = &selinux_state;
> >
> > - return state->policycap[POLICYDB_CAPABILITY_KEYPERMS];
> > + return state-
> > >policycap[POLICYDB_CAPABILITY_GENFS_SECLABEL_SYMLINKS];
> > }
> >
> > -static inline bool selinux_policycap_genfs_seclabel_symlinks(void)
> > +static inline bool selinux_policycap_key_perms(void)
> > {
> > struct selinux_state *state = &selinux_state;
> >
> > - return state-
> > >policycap[POLICYDB_CAPABILITY_GENFS_SECLABEL_SYMLINKS];
> > + return state->policycap[POLICYDB_CAPABILITY_KEYPERMS];
> > }
> >
> > int security_mls_enabled(struct selinux_state *state);
> > diff --git a/security/selinux/ss/services.c
> > b/security/selinux/ss/services.c
> > index d4a05f34d..6efc86c47 100644
> > --- a/security/selinux/ss/services.c
> > +++ b/security/selinux/ss/services.c
> > @@ -73,8 +73,8 @@ const char
> > *selinux_policycap_names[__POLICYDB_CAPABILITY_MAX] = {
> > "always_check_network",
> > "cgroup_seclabel",
> > "nnp_nosuid_transition",
> > - "key_perms",
> > - "genfs_seclabel_symlinks"
> > + "genfs_seclabel_symlinks",
> > + "key_perms"
> > };
> >
> > static struct selinux_ss selinux_ss;
> >
