[PATCH v2] selinux: tighten policydb_context_isvalid() checks

[PATCH v2] selinux: tighten policydb_context_isvalid() checks

[Stephen Smalley]

policydb_context_isvalid() bounds each of user, role, and type but
does not require the value to be defined. A crafted policy can declare
more identifiers that it defines, so an initial SID or ocontext
context whose type is not defined can be inserted into the sidtab and
later reach type_attribute_bounds_av() from
context_struct_compute_av(), thereby reaching the
BUG_ON(!type_val_to_struct[type - 1]). A user with no datum can reach
context_struct_to_string() and thereby trigger strlen(NULL). The user
and role datums are already NULL-checked further down but only when
role is not OBJECT_R_VAL, so an object context can carry an undefined
value through.

Use policydb_{role,user,type}_isvalid(), which check both the range
and that the value has a name, in place of the inline range checks.
The name and datum arrays are populated by *_index(), so a value with
a name also has a datum. This runs after policydb_index(), so the
arrays are populated. The now-redundant NULL guards on role and
usrdatum in the OBJECT_R_VAL block can then be dropped.

Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com>
---
v2 fixes a typo in the patch description.

 security/selinux/ss/policydb.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
index c4a81bd31e05..3beb3bd60e0f 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -995,13 +995,13 @@ bool policydb_context_isvalid(const struct policydb *p, const struct context *c)
 	const struct role_datum *role;
 	const struct user_datum *usrdatum;
 
-	if (!c->role || c->role > p->p_roles.nprim)
+	if (!policydb_role_isvalid(p, c->role))
 		return false;
 
-	if (!c->user || c->user > p->p_users.nprim)
+	if (!policydb_user_isvalid(p, c->user))
 		return false;
 
-	if (!c->type || c->type > p->p_types.nprim)
+	if (!policydb_simpletype_isvalid(p, c->type))
 		return false;
 
 	if (c->role != OBJECT_R_VAL) {
@@ -1009,7 +1009,7 @@ bool policydb_context_isvalid(const struct policydb *p, const struct context *c)
 		 * Role must be authorized for the type.
 		 */
 		role = p->role_val_to_struct[c->role - 1];
-		if (!role || !ebitmap_get_bit(&role->types, c->type - 1))
+		if (!ebitmap_get_bit(&role->types, c->type - 1))
 			/* role may not be associated with type */
 			return false;
 
@@ -1017,9 +1017,6 @@ bool policydb_context_isvalid(const struct policydb *p, const struct context *c)
 		 * User must be authorized for the role.
 		 */
 		usrdatum = p->user_val_to_struct[c->user - 1];
-		if (!usrdatum)
-			return false;
-
 		if (!ebitmap_get_bit(&usrdatum->roles, c->role - 1))
 			/* user may not be associated with role */
 			return false;
-- 
2.55.0

Re: [PATCH v2] selinux: tighten policydb_context_isvalid() checks

[sashiko-bot]

> policydb_context_isvalid() bounds each of user, role, and type but
> does not require the value to be defined. A crafted policy can declare
> more identifiers that it defines, so an initial SID or ocontext
> context whose type is not defined can be inserted into the sidtab and
> later reach type_attribute_bounds_av() from
> context_struct_compute_av(), thereby reaching the
> BUG_ON(!type_val_to_struct[type - 1]). A user with no datum can reach
> context_struct_to_string() and thereby trigger strlen(NULL). The user
> and role datums are already NULL-checked further down but only when
> role is not OBJECT_R_VAL, so an object context can carry an undefined
> value through.
> 
> Use policydb_{role,user,type}_isvalid(), which check both the range
> and that the value has a name, in place of the inline range checks.
> The name and datum arrays are populated by *_index(), so a value with
> a name also has a datum. This runs after policydb_index(), so the
> arrays are populated. The now-redundant NULL guards on role and
> usrdatum in the OBJECT_R_VAL block can then be dropped.
> 
> Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260807191342.19500-2-stephen.smalley.work@gmail.com?part=1