Re: [RFC PATCH 22/22] selinux: restrict policy strings

[Daniel Burgener <dburgener@linux.microsoft.com>]

On 11/15/2024 8:35 AM, Christian Göttsche wrote:
> From: Christian Göttsche <cgzones@googlemail.com>
> 
> Validate the characters and the lengths of strings parsed from binary
> policies.
> 
>    * Disallow control characters
>    * Limit characters of identifiers to alphanumeric, underscore, dash,
>      and dot
>    * Limit identifiers in length to 128, expect types to 1024 and
>      categories to 32, characters (excluding NUL-terminator)
> 
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> ---
>   security/selinux/ss/conditional.c |  2 +-
>   security/selinux/ss/policydb.c    | 60 ++++++++++++++++++++-----------
>   security/selinux/ss/policydb.h    |  5 ++-
>   3 files changed, 44 insertions(+), 23 deletions(-)
> 
> diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c
> index d37b4bdf6ba9..346102417cbf 100644
> --- a/security/selinux/ss/conditional.c
> +++ b/security/selinux/ss/conditional.c
> @@ -280,7 +280,7 @@ int cond_read_bool(struct policydb *p, struct symtab *s, struct policy_file *fp)
>   
>   	len = le32_to_cpu(buf[2]);
>   
> -	rc = str_read(&key, GFP_KERNEL, fp, len);
> +	rc = str_read(&key, GFP_KERNEL, fp, len, STR_IDENTIFIER, 128);
>   	if (rc)
>   		goto err;
>

It would be nice if these limits were named constants instead of magic 
numbers.  Right now it's hard to tell if all the "128"s are essentially 
the same limit referenced in different places, or if they could (in 
theory) be changed independently.

> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> index 917b468c5144..d98dfa6c3f30 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -1221,8 +1221,9 @@ static int context_read_and_validate(struct context *c, struct policydb *p,
>    * binary representation file.
>    */
>   
> -int str_read(char **strp, gfp_t flags, struct policy_file *fp, u32 len)
> +int str_read(char **strp, gfp_t flags, struct policy_file *fp, u32 len, int kind, u32 max_len)
>   {
> +	u32 i;
>   	int rc;
>   	char *str;
>   
> @@ -1232,19 +1233,35 @@ int str_read(char **strp, gfp_t flags, struct policy_file *fp, u32 len)
>   	if (oom_check(sizeof(char), len, fp))
>   		return -EINVAL;
>   
> +	if (max_len != 0 && len > max_len)
> +		return -EINVAL;
> +
>   	str = kmalloc(len + 1, flags | __GFP_NOWARN);
>   	if (!str)
>   		return -ENOMEM;
>   
>   	rc = next_entry(str, fp, len);
> -	if (rc) {
> -		kfree(str);
> -		return rc;
> +	if (rc)
> +		goto bad_str;
> +
> +	rc = -EINVAL;
> +	for (i = 0; i < len; i++) {
> +		if (iscntrl(str[i]))
> +			goto bad_str;
> +
> +		if (kind == STR_IDENTIFIER &&
> +		    !(isalnum(str[i]) || str[i] == '_' || str[i] == '-' || str[i] == '.'))
> +			goto bad_str;
> +
>   	}
>   
>   	str[len] = '\0';
>   	*strp = str;
>   	return 0;
> +
> +bad_str:
> +	kfree(str);
> +	return rc;
>   }
>   
>   static int perm_read(struct policydb *p, struct symtab *s, struct policy_file *fp)
> @@ -1269,7 +1286,7 @@ static int perm_read(struct policydb *p, struct symtab *s, struct policy_file *f
>   	if (perdatum->value < 1 || perdatum->value > 32)
>   		goto bad;
>   
> -	rc = str_read(&key, GFP_KERNEL, fp, len);
> +	rc = str_read(&key, GFP_KERNEL, fp, len, STR_IDENTIFIER, 128);
>   	if (rc)
>   		goto bad;
>   
> @@ -1315,7 +1332,7 @@ static int common_read(struct policydb *p, struct symtab *s, struct policy_file
>   		goto bad;
>   	comdatum->permissions.nprim = le32_to_cpu(buf[2]);
>   
> -	rc = str_read(&key, GFP_KERNEL, fp, len);
> +	rc = str_read(&key, GFP_KERNEL, fp, len, STR_IDENTIFIER, 128);
>   	if (rc)
>   		goto bad;
>   
> @@ -1552,12 +1569,12 @@ static int class_read(struct policydb *p, struct symtab *s, struct policy_file *
>   
>   	ncons = le32_to_cpu(buf[5]);
>   
> -	rc = str_read(&key, GFP_KERNEL, fp, len);
> +	rc = str_read(&key, GFP_KERNEL, fp, len, STR_IDENTIFIER, 128);
>   	if (rc)
>   		goto bad;
>   
>   	if (len2) {
> -		rc = str_read(&cladatum->comkey, GFP_KERNEL, fp, len2);
> +		rc = str_read(&cladatum->comkey, GFP_KERNEL, fp, len2, STR_IDENTIFIER, 128);
>   		if (rc)
>   			goto bad;
>   
> @@ -1691,7 +1708,7 @@ static int role_read(struct policydb *p, struct symtab *s, struct policy_file *f
>   	if (p->policyvers >= POLICYDB_VERSION_BOUNDARY)
>   		role->bounds = le32_to_cpu(buf[2]);
>   
> -	rc = str_read(&key, GFP_KERNEL, fp, len);
> +	rc = str_read(&key, GFP_KERNEL, fp, len, STR_IDENTIFIER, 128);
>   	if (rc)
>   		goto bad;
>   
> @@ -1758,7 +1775,7 @@ static int type_read(struct policydb *p, struct symtab *s, struct policy_file *f
>   		typdatum->primary = le32_to_cpu(buf[2]);
>   	}
>   
> -	rc = str_read(&key, GFP_KERNEL, fp, len);
> +	rc = str_read(&key, GFP_KERNEL, fp, len, STR_IDENTIFIER, 1024);
>   	if (rc)
>   		goto bad;
>   
> @@ -1822,7 +1839,7 @@ static int user_read(struct policydb *p, struct symtab *s, struct policy_file *f
>   	if (p->policyvers >= POLICYDB_VERSION_BOUNDARY)
>   		usrdatum->bounds = le32_to_cpu(buf[2]);
>   
> -	rc = str_read(&key, GFP_KERNEL, fp, len);
> +	rc = str_read(&key, GFP_KERNEL, fp, len, STR_IDENTIFIER, 128);
>   	if (rc)
>   		goto bad;
>   
> @@ -1871,7 +1888,7 @@ static int sens_read(struct policydb *p, struct symtab *s, struct policy_file *f
>   		goto bad;
>   	levdatum->isalias = val;
>   
> -	rc = str_read(&key, GFP_KERNEL, fp, len);
> +	rc = str_read(&key, GFP_KERNEL, fp, len, STR_IDENTIFIER, 128);
>   	if (rc)
>   		goto bad;
>   
> @@ -1914,7 +1931,7 @@ static int cat_read(struct policydb *p, struct symtab *s, struct policy_file *fp
>   		goto bad;
>   	catdatum->isalias = val;
>   
> -	rc = str_read(&key, GFP_KERNEL, fp, len);
> +	rc = str_read(&key, GFP_KERNEL, fp, len, STR_IDENTIFIER, 32);
>   	if (rc)
>   		goto bad;

The category restriction is more tight than the sensitivity one because 
a context may have many categories?  I guess that makes sense, but it 
feels counterintuitive from a user perspective, because I feel like 
users tend to think of categories and sensitivities as essentially the 
same thing.  Would dropping the sensitivity limit to 32 to match the 
category limit make sense?

Is there a more strict limit on the number of categories a context can 
have than the U32_MAX from symtab.nprim?  Because that will allow 
exceeding the page size using too many categories regardless of length 
distinctions, which is a concern if the motivation here is about 
potential future untrusted policy loaders in a namespaced environment.

-Daniel