| Message ID | 20200220181031.156674-2-richard_c_haines@btinternet.com (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
| Series | selinux: Add support for new key permissions | expand |
On Thu, Feb 20, 2020 at 2:26 PM Stephen Smalley <sds@tycho.nsa.gov> 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@btinternet.com> > > Signed-off-by: David Howells <dhowells@redhat.com> > > What is this relative to? Didn't apply for me on any of keys-acl or > keys-next or selinux-next. > > 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, you really need to revert this patch David, I mentioned this some time ago when the linux-next conflict appeared. Also, future patches like this *really* need to go in via the SELinux tree, not the keys tree, as they affect the SELinux kernel ABI and if they aren't merged via the same tree lots of bad things can happen if we aren't careful.
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@btinternet.com> > > Signed-off-by: David Howells <dhowells@redhat.com> > > 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; > >
Paul Moore <paul@paul-moore.com> wrote: > Yes, you really need to revert this patch David, I mentioned this some > time ago when the linux-next conflict appeared. It is reverted. > Also, future patches like this *really* need to go in via the SELinux tree, > not the keys tree, as they affect the SELinux kernel ABI and if they aren't > merged via the same tree lots of bad things can happen if we aren't careful. Are you're willing to take the matching keyring changes with it? The SELinux patch won't build without them - but they have to go in at the same time otherwise the keyrings part will malfunction. David
Stephen Smalley <sds@tycho.nsa.gov> wrote: > 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. I think the problem is that selinux_key_permission() is munging the new perm set into the old perm set and then passing that to avc_has_perm(). Really, we need to work backwards if the SELinux policy is described in terms of the old perm set. Is there any way to make that possible? David
On Fri, Feb 28, 2020 at 10:52 AM David Howells <dhowells@redhat.com> wrote: > Paul Moore <paul@paul-moore.com> wrote: > > > Yes, you really need to revert this patch David, I mentioned this some > > time ago when the linux-next conflict appeared. > > It is reverted. Thank you. [sorry, I forgot the reply-all] > > Also, future patches like this *really* need to go in via the SELinux tree, > > not the keys tree, as they affect the SELinux kernel ABI and if they aren't > > merged via the same tree lots of bad things can happen if we aren't careful. > > Are you're willing to take the matching keyring changes with it? The SELinux > patch won't build without them - but they have to go in at the same time > otherwise the keyrings part will malfunction. Assuming they've got the right ACKs from any subsystems that are affected, sure.
On Fri, Feb 28, 2020 at 10:56 AM David Howells <dhowells@redhat.com> wrote: > > Stephen Smalley <sds@tycho.nsa.gov> wrote: > > > 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. > > I think the problem is that selinux_key_permission() is munging the new perm > set into the old perm set and then passing that to avc_has_perm(). Really, we > need to work backwards if the SELinux policy is described in terms of the old > perm set. > > Is there any way to make that possible? That's not the problem. The problem is that security_key_permission() needs to be passed something (an additional flag or a different permission) in order to differentiate the two different callers so that SELinux can support both the old logic (when the new key_perms capability is disabled) and the new logic (when it is enabled). The old patch tried to do this by introducing a new KEY_NEED_PARENT_JOIN permission. But it didn't expose this as a KEY_ACE value and therefore creates a conflict/inconsistency between the ACE permissions and the internal permissions. Either it needs to be exposed as a legitimate ACE value too, or it needs to be handled differently, e.g. as an additional kernel-internal flag that gets passed down so SELinux can distinguish them.
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;