Re: [PATCH v3] selinux: simplify mls_context_to_sid()

[Stephen Smalley <sds@xxxxxxxxxxxxx>]

On 11/12/18 6:44 AM, Ondrej Mosnacek wrote:
> This function has only two callers, but only one of them actually needs
> the special logic at the beginning. Factoring this logic out into
> string_to_context_struct() allows us to drop the arguments 'oldc', 's',
> and 'def_sid'.
>
> Signed-off-by: Ondrej Mosnacek <omosnace@xxxxxxxxxx>
> ---
>
> Changes in v3:
>   - correct the comment about policy read lock
>
> Changes in v2:
>   - also drop unneeded #include's from mls.c
>
>   security/selinux/ss/mls.c      | 49 +++++-----------------------------
>   security/selinux/ss/mls.h      |  5 +---
>   security/selinux/ss/services.c | 32 +++++++++++++++++++---
>   3 files changed, 36 insertions(+), 50 deletions(-)
>
> diff --git a/security/selinux/ss/mls.c b/security/selinux/ss/mls.c
> index 2fe459df3c85..d1da928a7e77 100644
> --- a/security/selinux/ss/mls.c
> +++ b/security/selinux/ss/mls.c
> @@ -24,10 +24,7 @@
>   #include <linux/string.h>
>   #include <linux/errno.h>
>   #include <net/netlabel.h>
> -#include "sidtab.h"
>   #include "mls.h"
> -#include "policydb.h"
> -#include "services.h"
>   
>   /*
>    * Return the length in bytes for the MLS fields of the
> @@ -223,20 +220,12 @@ int mls_context_isvalid(struct policydb *p, struct context *c)
>    * This function modifies the string in place, inserting
>    * NULL characters to terminate the MLS fields.
>    *
> - * If a def_sid is provided and no MLS field is present,
> - * copy the MLS field of the associated default context.
> - * Used for upgraded to MLS systems where objects may lack
> - * MLS fields.
> - *
> - * Policy read-lock must be held for sidtab lookup.
> + * Policy read-lock must be held for policy data lookup.
>    *
>    */
>   int mls_context_to_sid(struct policydb *pol,
> -		       char oldc,
>   		       char *scontext,
> -		       struct context *context,
> -		       struct sidtab *s,
> -		       u32 def_sid)
> +		       struct context *context)
>   {
>   	char *sensitivity, *cur_cat, *next_cat, *rngptr;
>   	struct level_datum *levdatum;
> @@ -244,29 +233,6 @@ int mls_context_to_sid(struct policydb *pol,
>   	int l, rc, i;
>   	char *rangep[2];
>   
> -	if (!pol->mls_enabled) {
> -		if ((def_sid != SECSID_NULL && oldc) || (*scontext) == '\0')
> -			return 0;
> -		return -EINVAL;
> -	}
> -
> -	/*
> -	 * No MLS component to the security context, try and map to
> -	 * default if provided.
> -	 */
> -	if (!oldc) {
> -		struct context *defcon;
> -
> -		if (def_sid == SECSID_NULL)
> -			return -EINVAL;
> -
> -		defcon = sidtab_search(s, def_sid);
> -		if (!defcon)
> -			return -EINVAL;
> -
> -		return mls_context_cpy(context, defcon);
> -	}
> -
>   	/*
>   	 * If we're dealing with a range, figure out where the two parts
>   	 * of the range begin.
> @@ -364,14 +330,11 @@ int mls_from_string(struct policydb *p, char *str, struct context *context,
>   		return -EINVAL;
>   
>   	tmpstr = kstrdup(str, gfp_mask);
> -	if (!tmpstr) {
> -		rc = -ENOMEM;
> -	} else {
> -		rc = mls_context_to_sid(p, ':', tmpstr, context,
> -					NULL, SECSID_NULL);
> -		kfree(tmpstr);
> -	}
> +	if (!tmpstr)
> +		return -ENOMEM;
>   
> +	rc = mls_context_to_sid(p, tmpstr, context);
> +	kfree(tmpstr);
>   	return rc;
>   }
>   
> diff --git a/security/selinux/ss/mls.h b/security/selinux/ss/mls.h
> index 67093647576d..e2498f78e100 100644
> --- a/security/selinux/ss/mls.h
> +++ b/security/selinux/ss/mls.h
> @@ -33,11 +33,8 @@ int mls_range_isvalid(struct policydb *p, struct mls_range *r);
>   int mls_level_isvalid(struct policydb *p, struct mls_level *l);
>   
>   int mls_context_to_sid(struct policydb *p,
> -		       char oldc,
>   		       char *scontext,
> -		       struct context *context,
> -		       struct sidtab *s,
> -		       u32 def_sid);
> +		       struct context *context);
>   
>   int mls_from_string(struct policydb *p, char *str, struct context *context,
>   		    gfp_t gfp_mask);
> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index 12e414394530..ccad4334f99d 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -1425,9 +1425,35 @@ static int string_to_context_struct(struct policydb *pol,
>   
>   	ctx->type = typdatum->value;
>   
> -	rc = mls_context_to_sid(pol, oldc, p, ctx, sidtabp, def_sid);
> -	if (rc)
> -		goto out;
> +	if (!pol->mls_enabled) {
> +		rc = -EINVAL;
> +		if ((def_sid == SECSID_NULL || !oldc) && (*p) != '\0')
> +			goto out;

I don't think this is your bug, but unless I'm mistaken, p could be OOB 
and be dereferenced here.  Seems to have been introduced by 
95ffe194204ae3cef88d0b59be209204bbe9b3be.  Likely not caught in testing 
since both Linux distributions and Android enable MLS to use the 
category sets for isolation.

> +	} else if (!oldc) {
> +		/*
> +		 * If a def_sid is provided and no MLS field is present,
> +		 * copy the MLS field of the associated default context.
> +		 * Used for upgrading to MLS systems where objects may lack
> +		 * MLS fields.
> +		 */
> +		struct context *defcon;
> +
> +		rc = -EINVAL;
> +		if (def_sid == SECSID_NULL)
> +			goto out;
> +
> +		defcon = sidtab_search(sidtabp, def_sid);
> +		if (!defcon)
> +			goto out;
> +
> +		rc = mls_context_cpy(ctx, defcon);
> +		if (rc)
> +			goto out;
> +	} else {
> +		rc = mls_context_to_sid(pol, p, ctx);
> +		if (rc)
> +			goto out;
> +	}
>   
>   	/* Check the validity of the new context. */
>   	rc = -EINVAL;