Re: [PATCH] selinux-testsuite: Add key and key_socket tests

[Richard Haines <richard_c_haines@btinternet.com>]

On Mon, 2019-09-16 at 15:11 -0400, Stephen Smalley wrote:
> On 9/16/19 2:55 PM, Richard Haines wrote:
> > On Mon, 2019-09-16 at 13:58 -0400, Stephen Smalley wrote:
> > > On 9/9/19 9:17 AM, Richard Haines wrote:
> > > > Test all permissions associated with the key and key_socket
> > > > classes.
> > > > 
> > > > Note that kernel 5.3 commit keys: Fix request_key() lack of
> > > > Link
> > > > perm
> > > > check on found key ("504b69eb3c95180bc59f1ae9096ad4b10bbbf254")
> > > > added an additional check for link perm on request_key(). The
> > > > tests
> > > > will support earlier kernels.
> > > 
> > > I'm not sure why you coupled key and key_socket together; they
> > > don't
> > > have anything to do with each other, and were introduced in very
> > > different kernel and probably refpolicy releases.  I would
> > > recommend
> > > splitting them.  SECCLASS_KEY and its permission checks were
> > > introduced
> > > in Linux v2.6.18; SECCLASS_KEY_SOCKET was part of the original
> > > SELinux
> > > merge for Linux 2.6.0.
> > 
> > I'll split them.
> > 
> > > You only appear to be testing self access, not permission checks
> > > between
> > > a process and a keyring created by another process in a different
> > > security context.
> > 
> > Okay I'll add these tests
> > > 1 test fails for me,
> > > keys/test ................... Failed KEYCTL_SESSION_TO_PARENT:
> > > Operation
> > > not permitted
> > > keys/test ................... 1/13
> > > #   Failed test at keys/test line 38.
> > > # Looks like you failed 1 test of 13.
> > > keys/test ................... Dubious, test returned 1 (wstat
> > > 256,
> > > 0x100)
> > 
> > You must have systems that don't like my patches - I can't get this
> > fail. Using Fedora 30 and also Rawhide from a few weeks ago.
> 
> I'll have to look into it further, but it was on stock F30.
> 
> > I don't know if this is of any interest (It works on Rawhide with
> > kernel from [1]):
> > 
> > I've been building 'key' tests to add the new permissions defined
> > in
> > kernel-next [1].
> > To test these with new policy supporting the new perms + old policy
> > that does not, I added the kernel test patch below.
> > This patch handles security_key_permission() passing a single
> > permission, as checking the current keys code I only see it passing
> > a
> > single permission at a time.
> > 
> > I've also an sepol patch + selinux-testsuite tests
> > 
> > [1]
> > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/security/selinux?h=next-20190904&id=1f96e0f129eb2bea15a00c154eee8b85aa181d1a
> 
> Yes, that's probably worth submitting for real.  Be sure to include 
> David Howells on the distribution for it. I wouldn't assume that only
> a 
> single permission can ever be passed unless key_permission() itself 
> asserts that invariant.
> 
That's okay as I've also tested the patch below that handles multiple
permissions from keys:
Any view on what is best !!

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 901cc052f..78413277c 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -6502,7 +6502,8 @@ static int selinux_key_permission(key_ref_t
key_ref,
 {
 	struct key *key;
 	struct key_security_struct *ksec;
-	unsigned oldstyle_perm;
+	unsigned int key_perm = 0, switch_perm = 0;
+	int x = KEY_NEED_ALL, bit = 1;
 	u32 sid;
 
 	/* if no specific permissions are requested, we skip the
@@ -6511,18 +6512,67 @@ 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;
+	/*
+	 * selinux_key_permission() is called with only one permission
set.
+	 * However this will handle multiple bits set.
+	 */
+	while (x) {
+		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: /* Keep this as "setattr" in
policy */
+			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:
+			key_perm |= KEY__JOIN;
+			break;
+		case KEY_NEED_CLEAR:
+			key_perm |= KEY__CLEAR;
+			break;
+		}
+		bit <<= 1;
+		x >>= 1;
+	}
+
+	/* If old policy, then reset new perms to orig. */
+	if (!selinux_policycap_key_perms()) {
+		if (perm & KEY_NEED_INVAL) {
+			key_perm &= ~KEY__INVAL;
+			key_perm |= KEY__SEARCH;
+		}
+		if (perm & KEY_NEED_REVOKE && !(perm &
OLD_KEY_NEED_SETATTR)) {
+			key_perm &= ~KEY__REVOKE;
+			key_perm |= KEY__WRITE;
+		}
+		if (perm & KEY_NEED_JOIN) {
+			key_perm &= ~KEY__JOIN;
+			key_perm |= KEY__SEARCH;
+		}
+		if (perm & KEY_NEED_CLEAR) {
+			key_perm &= ~KEY__CLEAR;
+			key_perm |= KEY__WRITE;
+		}
+	}
 
 	sid = cred_sid(cred);
 
@@ -6530,7 +6580,7 @@ static int selinux_key_permission(key_ref_t
key_ref,
 	ksec = key->security;
 
 	return avc_has_perm(&selinux_state,
-			    sid, ksec->sid, SECCLASS_KEY,
oldstyle_perm, NULL);
+			    sid, ksec->sid, SECCLASS_KEY, key_perm,
NULL);
 }
 
 static int selinux_key_getsecurity(struct key *key, char **_buffer)
@@ -6555,7 +6605,7 @@ static int selinux_watch_key(struct key *key)
 	u32 sid = current_sid();
 
 	return avc_has_perm(&selinux_state,
-			    sid, ksec->sid, SECCLASS_KEY,
KEY_NEED_VIEW, NULL);
+			    sid, ksec->sid, SECCLASS_KEY, KEY__VIEW,
NULL);
 }
 #endif
 #endif
diff --git a/security/selinux/include/classmap.h
b/security/selinux/include/classmap.h
index 201f7e588..a51ab9bd9 100644
--- a/security/selinux/include/classmap.h
+++ b/security/selinux/include/classmap.h
@@ -158,7 +158,7 @@ struct security_class_mapping secclass_map[] = {
 	  { "send", "recv", "relabelto", "forward_in", "forward_out",
NULL } },
 	{ "key",
 	  { "view", "read", "write", "search", "link", "setattr",
"create",
-	    NULL } },
+	    "inval", "revoke", "join", "clear", NULL } },
 	{ "dccp_socket",
 	  { COMMON_SOCK_PERMS,
 	    "node_bind", "name_connect", NULL } },
diff --git a/security/selinux/include/security.h
b/security/selinux/include/security.h
index 111121281..a248eef75 100644
--- a/security/selinux/include/security.h
+++ b/security/selinux/include/security.h
@@ -78,6 +78,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)
@@ -177,6 +178,13 @@ 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)
+{
+	struct selinux_state *state = &selinux_state;
+
+	return state->policycap[POLICYDB_CAPABILITY_KEYPERMS];
+}
+
 int security_mls_enabled(struct selinux_state *state);
 int security_load_policy(struct selinux_state *state,
 			 void *data, size_t len);
diff --git a/security/selinux/ss/services.c
b/security/selinux/ss/services.c
index d61563a36..eb3949fc8 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -73,7 +73,8 @@ const char
*selinux_policycap_names[__POLICYDB_CAPABILITY_MAX] = {
 	"extended_socket_class",
 	"always_check_network",
 	"cgroup_seclabel",
-	"nnp_nosuid_transition"
+	"nnp_nosuid_transition",
+	"key_perms"
 };
 
 static struct selinux_ss selinux_ss;