Re: [PATCH v2 2/2] selinux: reject a class permission count below its inherited common

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

Pre-existing issues:
- [High] Loading a crafted SELinux policy with sparse permission/class values or an inflated `nprim` leaves NULL pointers in symbol arrays, causing a kernel panic in selinuxfs directory creation.
- [Medium] A crafted SELinux policy with duplicate permission or class values causes unbounded memory leaks in `security_get_permissions()` and `security_get_classes()`.
--

--- Patch [2]: [PATCH v2 2/2] selinux: reject a class permission count below its inherited common ---
commit 0b87262f147a0083f6daa265c846c87ddbbf878b
Author: Bryam Vargas <[email protected]>

selinux: reject a class permission count below its inherited common

security_get_permissions() maps an inherited common's permissions into
an array sized by the class's own permissions.nprim, but class_read()
takes that nprim verbatim from the policy image and never checks that it
covers the common. A class that inherits a common of N permissions while
declaring a smaller nprim is accepted, and on load the common's
permissions are written past the class-sized array -- an out-of-bounds
heap write.

> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> index 6973b68d9782a..c6f55f0b4cb55 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -1422,6 +1422,18 @@ static int class_read(struct policydb *p, struct symtab *s, struct policy_file *
>  			       cladatum->comkey);
>  			goto bad;
>  		}
> +
> +		/*
> +		 * security_get_permissions() maps the common's permissions
> +		 * into an array sized by this class's nprim, so a class must
> +		 * declare at least as many as the common it inherits.
> +		 */
> +		if (cladatum->permissions.nprim <
> +		    cladatum->comdatum->permissions.nprim) {

[Severity: High]
This isn't a bug introduced by this patch, but does the kernel handle a policy
with sparse permission or class values correctly?

If a policy is loaded with an nprim value larger than the number of
contiguous, defined classes or permissions, security_get_classes() and
security_get_permissions() will allocate arrays of size nprim but only
populate the defined elements, leaving the rest as NULL.

Later, sel_make_classes() and sel_make_perm_files() pass these NULL pointers:

security/selinux/selinuxfs.c:sel_make_perm_files() {
    ...
    rc = sel_attach_file(dir, perms[i], inode);
    ...
}

Can this lead to a NULL pointer dereference in d_alloc_name() ->
hashlen_string() during selinuxfs directory creation?

> +			pr_err("SELinux:  class %s has fewer permissions than common %s\n",
> +			       key, cladatum->comkey);
> +			goto bad;
> +		}
>  	}
>  	for (i = 0; i < nel; i++) {
>  		rc = perm_read(p, &cladatum->permissions, fp);

[Severity: Medium]
This is a pre-existing issue, but does perm_read() enforce uniqueness of
permission and class values?

If an SELinux policy is loaded with multiple classes or permissions sharing the
same integer value, symtab_insert() only enforces uniqueness on the string
name.

During selinuxfs initialization, could get_permissions_callback() and
get_classes_callback() repeatedly execute kstrdup() to the same array index:

security/selinux/ss/services.c:get_permissions_callback() {
    ...
    perms[value] = kstrdup(name, GFP_ATOMIC);
    ...
}

Does this overwrite the previous pointer and leak the memory allocation?

>  		if (rc)
>  			goto bad;
>  	}

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