Re: [PATCH v3] selinux: Only apply bounds checking to source types

[Paul Moore <pmoore@redhat.com>]

On Tuesday, May 03, 2016 01:48:20 PM Stephen Smalley wrote:
> The current bounds checking of both source and target types
> requires allowing any domain that has access to the child
> domain to also have the same permissions to the parent, which
> is undesirable.  Drop the target bounds checking ...

Thanks for doing this, especially the detailed patch description.  It all 
looks reasonable to me and the code looks sane as well, a few comments below.

> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index 89df646..48babec 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -553,33 +553,23 @@ static void type_attribute_bounds_av(struct context
> *scontext, scontext->type - 1);
>  	BUG_ON(!source);
> 
> +	if (!source->bounds)
> +		return;
> +
>  	target = flex_array_get_ptr(policydb.type_val_to_struct_array,
>  				    tcontext->type - 1);
>  	BUG_ON(!target);
> 
> -	if (source->bounds) {
> -		memset(&lo_avd, 0, sizeof(lo_avd));
> -
> -		memcpy(&lo_scontext, scontext, sizeof(lo_scontext));
> -		lo_scontext.type = source->bounds;
> +	memset(&lo_avd, 0, sizeof(lo_avd));
> 
> -		context_struct_compute_av(&lo_scontext,
> -					  tcontext,
> -					  tclass,
> -					  &lo_avd,
> -					  NULL);
> -		if ((lo_avd.allowed & avd->allowed) == avd->allowed)
> -			return;		/* no masked permission */
> -		masked = ~lo_avd.allowed & avd->allowed;
> -	}
> +	memcpy(&lo_scontext, scontext, sizeof(lo_scontext));
> +	lo_scontext.type = source->bounds;
> 
>  	if (target->bounds) {
> -		memset(&lo_avd, 0, sizeof(lo_avd));
> -
>  		memcpy(&lo_tcontext, tcontext, sizeof(lo_tcontext));
>  		lo_tcontext.type = target->bounds;
> 
> -		context_struct_compute_av(scontext,
> +		context_struct_compute_av(&lo_scontext,
>  					  &lo_tcontext,
>  					  tclass,
>  					  &lo_avd,
> @@ -587,17 +577,9 @@ static void type_attribute_bounds_av(struct context
> *scontext, if ((lo_avd.allowed & avd->allowed) == avd->allowed)
>  			return;		/* no masked permission */
>  		masked = ~lo_avd.allowed & avd->allowed;
> -	}
> -
> -	if (source->bounds && target->bounds) {
> -		memset(&lo_avd, 0, sizeof(lo_avd));
> -		/*
> -		 * lo_scontext and lo_tcontext are already
> -		 * set up.
> -		 */
> -
> +	} else {
>  		context_struct_compute_av(&lo_scontext,
> -					  &lo_tcontext,
> +					  tcontext,
>  					  tclass,
>  					  &lo_avd,
>  					  NULL);

Now that we are ignoring the unbounded subject/source, it seems like we could 
even further simplify the code; no change to the logic, just tidy the 
implementation a bit.

I think we could move the lo_avd/avd comparison and masked calculation outside 
the if statement as it is common to both the true and false conditions.

We could also likely move the compute_av() call outside the if statement too 
if we abstract the target/object behind a pointer which could either reference 
tcontext or lo_tcontext depending on the target.

I might be missing some small detail, but I don't believe the "if (masked)" 
check is ever going to be false; if correct we can toss the conditional check.

-- 
paul moore
security @ redhat