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

[Stephen Smalley <sds@tycho.nsa.gov>]

On 9/16/19 3:23 PM, Richard Haines wrote:
> 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 !!

Unless key_task_permission() prohibits passing multiple permissions or 
David Howells tells you it will never happen, I'd support it in the 
security hook.  selinux_inode_permission() and selinux_ipc_permission() 
support it for their checks.  I don't think I'd do the work of mapping 
to the new permissions unless the policycap is set.

> 
> 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;
> 
>