Re: [PATCH] selinux: reject a permission value exceeding the class permission count

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

Pre-existing issues:
- [High] The patch prevents an out-of-bounds write for class-specific permissions but fails to address the identical vulnerability when a class inherits permissions from a common. If a malicious policy declares a common with a large `nprim` and a class that inherits it but specifies a small `nprim`, the common permissions will bypass the patch's check and map to out-of-bounds indices in `security_get_permissions()`.
- [High] NULL pointer dereference in selinuxfs when a policy contains gaps in permission or class values. If a policy defines a sparse set of values (e.g., `nprim` is 3, but only values 1 and 3 are populated), `security_get_permissions()` leaves gaps in its allocated string array as NULL pointers. When `sel_make_perm_files()` iterates through these arrays, it passes the NULL pointers to `sel_attach_file()`, causing a kernel panic.
--

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

selinux: reject a permission value exceeding the class permission count

This commit adds a bounds check in perm_read() to ensure a permission value
does not exceed the nprim value of its owning class or common. It attempts
to prevent an out-of-bounds heap write when security_get_permissions()
allocates an array sized by nprim and populates it.

> 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 patch fully prevent the
out-of-bounds write if a class inherits permissions from a common?

While this check bounds the permission value against its immediate
symbol table's nprim, it seems a malicious policy could declare a
common with a large nprim and a class that inherits it with a small
nprim.

The parser sets the class's nprim without validating it against the
inherited common:

security/selinux/ss/policydb.c:class_read() {
    ...
    cladatum->permissions.nprim = le32_to_cpu(buf[3]);
    ...
}

Then security_get_permissions() allocates an array based solely on the
class's nprim:

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);
    ...
}

Since the common permissions bypass this patch's symtab-specific bounds
check and are mapped into the undersized perms array, could this still
lead to a slab-out-of-bounds write in get_permissions_callback()?

[Severity: High]
This is a pre-existing issue, but could this sparse array allocation
lead to a null pointer dereference later on?

If a policy defines a sparse set of values (for instance, if nprim is 3
but only values 1 and 3 are populated), get_permissions_callback() maps
the present permissions:

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

This leaves intermediate unpopulated indices as NULL in the array. When
sel_make_perm_files() iterates through this array:

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

It passes the NULL pointer to sel_attach_file(), which passes it to
d_alloc_name(), which requires a valid string for hashlen_string().

Can this cause a kernel panic when a process loads such a crafted policy?

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

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