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>
---
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.
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?
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.
