Re: [PATCH] libsepol/cil: Fix class permission verification in CIL

[Christian Göttsche <cgzones@xxxxxxxxxxxxxx>]

On 4/20/23 14:58, James Carter wrote:
> Before the CIL post processing phase (where expressions are evaluated,
> various ebitmaps are set, etc) there is a pre-verification where
> checks are made to find self references or loops in bounds, attribute
> sets, and class permissions. The class permission checking is faulty
> in two ways.
>
> First, it does not check for the use of "all" in a permission expression
> for a class that has no permissions. An error will still be generated
> later and secilc will exit cleanly, but without an error message that
> explains the problem.
>
> Second, it does not properly handle lists in permission expressions.
> For example, "(C ((P)))" is a legitimate class permission. The
> permissions expression contains one item that is a list containing
> one permission. This permission expression will be properly evaluated.
> Unfortunately, the class permission verification assumes that each
> item in the permission expression is either an operator or a
> permission datum and a segmenation fault will occur.
>
> Refactor the class permission checking to give a proper error when
> "all" is used in a permission expression for a class that has no
> permissions and so that it can handle lists in permission
> expressions. Also, check for the actual flavor of each item in
> the permission expression and return an error if an unexpected
> flavor is found.
>
> The failure to properly handle lists in permission expressions was
> found by oss-fuzz (#58085).


For what it's worth:

Verified the fuzzer no longer crashes and no new issues arose after 
running for ~1h.

Successfully build DSSP5.


Tested-by: Christian Göttsche <cgzones@xxxxxxxxxxxxxx>


> Signed-off-by: James Carter <jwcart2@xxxxxxxxx>
> ---
>   libsepol/cil/src/cil_verify.c | 167 +++++++++++++++++++++++-----------
>   1 file changed, 114 insertions(+), 53 deletions(-)
>
> diff --git a/libsepol/cil/src/cil_verify.c b/libsepol/cil/src/cil_verify.c
> index 4640dc59..3f58969d 100644
> --- a/libsepol/cil/src/cil_verify.c
> +++ b/libsepol/cil/src/cil_verify.c
> @@ -1700,31 +1700,109 @@ static int __add_perm_to_list(__attribute__((unused)) hashtab_key_t k, hashtab_d
>   	return SEPOL_OK;
>   }
>   
> -static int __cil_verify_classperms(struct cil_list *classperms,
> -				   struct cil_symtab_datum *orig,
> -				   struct cil_symtab_datum *parent,
> -				   struct cil_symtab_datum *cur,
> -				   enum cil_flavor flavor,
> -				   unsigned steps, unsigned limit)
> +static int __cil_verify_classperms(struct cil_list *classperms, struct cil_symtab_datum *orig, struct cil_symtab_datum *cur, unsigned steps, unsigned limit);
> +
> +static int __cil_verify_map_perm(struct cil_class *class, struct cil_perm *perm, struct cil_symtab_datum *orig, unsigned steps, unsigned limit)
> +{
> +	int rc;
> +
> +	if (!perm->classperms) {
> +		cil_tree_log(NODE(class), CIL_ERR, "No class permissions for map class %s, permission %s", DATUM(class)->name, DATUM(perm)->name);
> +		goto exit;
> +	}
> +
> +	rc = __cil_verify_classperms(perm->classperms, orig, &perm->datum, steps, limit);
> +	if (rc != SEPOL_OK) {
> +		cil_tree_log(NODE(class), CIL_ERR, "There was an error verifying class permissions for map class %s, permission %s", DATUM(class)->name, DATUM(perm)->name);
> +		goto exit;
> +	}
> +
> +	return SEPOL_OK;
> +
> +exit:
> +	return SEPOL_ERR;
> +}
> +
> +
> +static int __cil_verify_perms(struct cil_class *class, struct cil_list *perms, struct cil_symtab_datum *orig, unsigned steps, unsigned limit)
>   {
>   	int rc = SEPOL_ERR;
> -	struct cil_list_item *curr;
> +	int count = 0;
> +	struct cil_list_item *i = NULL;
>   
> -	if (classperms == NULL) {
> -		if (flavor == CIL_MAP_PERM) {
> -			cil_tree_log(NODE(cur), CIL_ERR, "Map class %s does not have a classmapping for %s", parent->name, cur->name);
> +	if (!perms) {
> +		cil_tree_log(NODE(class), CIL_ERR, "No permissions for class %s in class permissions", DATUM(class)->name);
> +		goto exit;
> +	}
> +
> +	cil_list_for_each(i, perms) {
> +		count++;
> +		if (i->flavor == CIL_LIST) {
> +			rc = __cil_verify_perms(class, i->data, orig, steps, limit);
> +			if (rc != SEPOL_OK) {
> +				goto exit;
> +			}
> +		} else if (i->flavor == CIL_DATUM) {
> +			struct cil_perm *perm = i->data;
> +			if (FLAVOR(perm) == CIL_MAP_PERM) {
> +				rc = __cil_verify_map_perm(class, perm, orig, steps, limit);
> +				if (rc != SEPOL_OK) {
> +					goto exit;
> +				}
> +			}
> +		} else if (i->flavor == CIL_OP) {
> +			enum cil_flavor op = (enum cil_flavor)(uintptr_t)i->data;
> +			if (op == CIL_ALL) {
> +				struct cil_list *perm_list;
> +				struct cil_list_item *j = NULL;
> +				int count2 = 0;
> +				cil_list_init(&perm_list, CIL_MAP_PERM);
> +				cil_symtab_map(&class->perms, __add_perm_to_list, perm_list);
> +				cil_list_for_each(j, perm_list) {
> +					count2++;
> +					struct cil_perm *perm = j->data;
> +					if (FLAVOR(perm) == CIL_MAP_PERM) {
> +						rc = __cil_verify_map_perm(class, perm, orig, steps, limit);
> +						if (rc != SEPOL_OK) {
> +							cil_list_destroy(&perm_list, CIL_FALSE);
> +							goto exit;
> +						}
> +					}
> +				}
> +				cil_list_destroy(&perm_list, CIL_FALSE);
> +				if (count2 == 0) {
> +					cil_tree_log(NODE(class), CIL_ERR, "Operator \"all\" used for %s which has no permissions associated with it", DATUM(class)->name);
> +					goto exit;
> +				}
> +			}
>   		} else {
> -			cil_tree_log(NODE(cur), CIL_ERR, "Classpermission %s does not have a classpermissionset", cur->name);
> +			cil_tree_log(NODE(class), CIL_ERR, "Permission list for %s has an unexpected flavor: %d", DATUM(class)->name, i->flavor);
> +			goto exit;
>   		}
> +	}
> +
> +	if (count == 0) {
> +		cil_tree_log(NODE(class), CIL_ERR, "Empty permissions list for class %s in class permissions", DATUM(class)->name);
> +		goto exit;
> +	}
> +
> +	return SEPOL_OK;
> +
> +exit:
> +	return SEPOL_ERR;
> +}
> +
> +static int __cil_verify_classperms(struct cil_list *classperms, struct cil_symtab_datum *orig, struct cil_symtab_datum *cur, unsigned steps, unsigned limit)
> +{
> +	int rc;
> +	struct cil_list_item *i;
> +
> +	if (classperms == NULL) {
>   		goto exit;
>   	}
>   
>   	if (steps > 0 && orig == cur) {
> -		if (flavor == CIL_MAP_PERM) {
> -			cil_tree_log(NODE(cur), CIL_ERR, "Found circular class permissions involving the map class %s and permission %s", parent->name, cur->name);
> -		} else {
> -			cil_tree_log(NODE(cur), CIL_ERR, "Found circular class permissions involving the set %s", cur->name);
> -		}
> +		cil_tree_log(NODE(cur), CIL_ERR, "Found circular class permissions involving %s", cur->name);
>   		goto exit;
>   	} else {
>   		steps++;
> @@ -1735,44 +1813,20 @@ static int __cil_verify_classperms(struct cil_list *classperms,
>   		}
>   	}
>   
> -	cil_list_for_each(curr, classperms) {
> -		if (curr->flavor == CIL_CLASSPERMS) {
> -			struct cil_classperms *cp = curr->data;
> -			if (FLAVOR(cp->class) != CIL_CLASS) { /* MAP */
> -				struct cil_list_item *i = NULL;
> -				cil_list_for_each(i, cp->perms) {
> -					if (i->flavor != CIL_OP) {
> -						struct cil_perm *cmp = i->data;
> -						rc = __cil_verify_classperms(cmp->classperms, orig, &cp->class->datum, &cmp->datum, CIL_MAP_PERM, steps, limit);
> -						if (rc != SEPOL_OK) {
> -							goto exit;
> -						}
> -					} else {
> -						enum cil_flavor op = (enum cil_flavor)(uintptr_t)i->data;
> -						if (op == CIL_ALL) {
> -							struct cil_class *mc = cp->class;
> -							struct cil_list *perm_list;
> -							struct cil_list_item *j = NULL;
> -
> -							cil_list_init(&perm_list, CIL_MAP_PERM);
> -							cil_symtab_map(&mc->perms, __add_perm_to_list, perm_list);
> -							cil_list_for_each(j, perm_list) {
> -								struct cil_perm *cmp = j->data;
> -								rc = __cil_verify_classperms(cmp->classperms, orig, &cp->class->datum, &cmp->datum, CIL_MAP_PERM, steps, limit);
> -								if (rc != SEPOL_OK) {
> -									cil_list_destroy(&perm_list, CIL_FALSE);
> -									goto exit;
> -								}
> -							}
> -							cil_list_destroy(&perm_list, CIL_FALSE);
> -						}
> -					}
> -				}
> +	cil_list_for_each(i, classperms) {
> +		if (i->flavor == CIL_CLASSPERMS) {
> +			struct cil_classperms *cp = i->data;
> +			rc = __cil_verify_perms(cp->class, cp->perms, orig, steps, limit);
> +			if (rc != SEPOL_OK) {
> +				goto exit;
>   			}
>   		} else { /* SET */
> -			struct cil_classperms_set *cp_set = curr->data;
> +			struct cil_classperms_set *cp_set = i->data;
>   			struct cil_classpermission *cp = cp_set->set;
> -			rc = __cil_verify_classperms(cp->classperms, orig, NULL, &cp->datum, CIL_CLASSPERMISSION, steps, limit);
> +			if (!cp->classperms) {
> +				cil_tree_log(NODE(cur), CIL_ERR, "Classpermission %s does not have a classpermissionset", DATUM(cp)->name);
> +			}
> +			rc = __cil_verify_classperms(cp->classperms, orig, &cp->datum, steps, limit);
>   			if (rc != SEPOL_OK) {
>   				goto exit;
>   			}
> @@ -1787,9 +1841,15 @@ exit:
>   
>   static int __cil_verify_classpermission(struct cil_tree_node *node)
>   {
> +	int rc;
>   	struct cil_classpermission *cp = node->data;
>   
> -	return __cil_verify_classperms(cp->classperms, &cp->datum, NULL, &cp->datum, CIL_CLASSPERMISSION, 0, 2);
> +	rc = __cil_verify_classperms(cp->classperms, &cp->datum, &cp->datum, 0, 2);
> +	if (rc != SEPOL_OK) {
> +		cil_tree_log(node, CIL_ERR, "Error verifying class permissions for classpermission %s", DATUM(cp)->name);
> +	}
> +
> +	return rc;
>   }
>   
>   struct cil_verify_map_args {
> @@ -1804,8 +1864,9 @@ static int __verify_map_perm_classperms(__attribute__((unused)) hashtab_key_t k,
>   	struct cil_perm *cmp = (struct cil_perm *)d;
>   	int rc;
>   
> -	rc = __cil_verify_classperms(cmp->classperms, &cmp->datum, &map_args->class->datum, &cmp->datum, CIL_MAP_PERM, 0, 2);
> +	rc = __cil_verify_classperms(cmp->classperms, &cmp->datum, &cmp->datum, 0, 2);
>   	if (rc != SEPOL_OK) {
> +		cil_tree_log(NODE(cmp), CIL_ERR, "Error verifying class permissions for map class %s, permission %s", DATUM(map_args->class)->name, DATUM(cmp)->name);
>   		map_args->rc = rc;
>   	}
>