Re: [PATCH v2 1/2] selinux: reject a permission value exceeding the class permission count

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Incomplete Fix / Out-of-Bounds Heap Write in `security_get_permissions` via inherited common classes.
--

--- Patch [1]: [PATCH v2 1/2] selinux: reject a permission value exceeding the class permission count ---
commit b4fe3642552433263525065c279ba6cb8b2c045e
Author: Bryam Vargas <[email protected]>

selinux: reject a permission value exceeding the class permission count

This patch adds a bounds check in perm_read() to ensure that a permission
value does not exceed the symbol table's nprim. This is intended to prevent
out-of-bounds heap writes during policy loading when
security_get_permissions() populates an nprim-sized array.

> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> index ead504a639e36..6973b68d9782a 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -1175,6 +1175,9 @@ static int perm_read(struct policydb *p, struct symtab *s, struct policy_file *f
>  	rc = -EINVAL;
>  	if (perdatum->value < 1 || perdatum->value > SEL_VEC_MAX)
>  		goto bad;
> +	/* the value indexes an nprim-sized array in security_get_permissions() */
> +	if (perdatum->value > s->nprim)
> +		goto bad;

[Severity: High]
This is a pre-existing issue, but does this fully prevent the out-of-bounds
write when a class inherits from a common class?

The bounds check added here validates permissions against their immediate
symbol table's nprim (which is comdatum->permissions.nprim for common
permissions). It seems this misses the inheritance vector where a class's
nprim is smaller than its inherited common class.

For example, when the inheritance link is established in class_read():

security/selinux/ss/policydb.c:class_read() {
	...
	cladatum->comdatum = symtab_search(&p->p_commons, cladatum->comkey);
	...
}

There is no validation to ensure that
cladatum->permissions.nprim >= cladatum->comdatum->permissions.nprim.

Later, in security_get_permissions(), an undersized array could be allocated
using the class's nprim, and populated with the common's permissions:

security/selinux/ss/services.c:security_get_permissions() {
	...
	*nperms = match->permissions.nprim;
	*perms = kcalloc(*nperms, sizeof(**perms), GFP_ATOMIC);
	...
	if (match->comdatum) {
		rc = hashtab_map(&match->comdatum->permissions.table,
				 get_permissions_callback, *perms);
	...
}

Could a crafted policy still trigger an out-of-bounds write here if the class
declares fewer permissions than its inherited common class?

>  
>  	rc = str_read(&key, GFP_KERNEL, fp, len);
>  	if (rc)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1