[PATCH 0/5] Fix bugs identified by the secilc-fuzzer

[PATCH 0/5] Fix bugs identified by the secilc-fuzzer

[James Carter]

These patches are all related by the fact that the secilc-fuzzer
identified the bugs that led to them. Wherever possible I have
tried to fix all of the issues related to the specific one found.

James Carter (5):
  libsepol/cil: Fix instances where an error returns SEPOL_OK
  libsepol/cil: Detect degenerate inheritance and exit with an error
  libsepol/cil: Check datum in ordered list for expected flavor
  libsepol/cil: Check for self-referential loops in sets
  libsepol/cil: Return an error if a call argument fails to resolve

 libsepol/cil/src/cil_build_ast.c   |   3 +
 libsepol/cil/src/cil_internal.h    |   2 +
 libsepol/cil/src/cil_resolve_ast.c | 107 ++++++++++++++++++++---------
 libsepol/cil/src/cil_verify.c      |  97 ++++++++++++++++++--------
 libsepol/cil/src/cil_verify.h      |   1 -
 5 files changed, 151 insertions(+), 59 deletions(-)

-- 
2.26.3

[PATCH 1/5] libsepol/cil: Fix instances where an error returns SEPOL_OK

[James Carter]

There are six instances when the CIL policy is being built or
resolved where an error can be detected, but SEPOL_OK is returned
instead of SEPOL_ERR. This causes the policy compiler to continue
when it should exit with an error.

Return SEPOL_ERR in these cases, so the compiler exits with an
error.

Two of the instances were found by the secilc-fuzzer.

Signed-off-by: James Carter <jwcart2@gmail.com>
---
 libsepol/cil/src/cil_build_ast.c   | 3 +++
 libsepol/cil/src/cil_resolve_ast.c | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/libsepol/cil/src/cil_build_ast.c b/libsepol/cil/src/cil_build_ast.c
index 5b1e2824..87043a8f 100644
--- a/libsepol/cil/src/cil_build_ast.c
+++ b/libsepol/cil/src/cil_build_ast.c
@@ -444,6 +444,7 @@ int cil_gen_class(struct cil_db *db, struct cil_tree_node *parse_current, struct
 		}
 		if (class->num_perms > CIL_PERMS_PER_CLASS) {
 			cil_tree_log(parse_current, CIL_ERR, "Too many permissions in class '%s'", class->datum.name);
+			rc = SEPOL_ERR;
 			goto exit;
 		}
 
@@ -1018,6 +1019,7 @@ int cil_gen_common(struct cil_db *db, struct cil_tree_node *parse_current, struc
 	}
 	if (common->num_perms > CIL_PERMS_PER_CLASS) {
 		cil_tree_log(parse_current, CIL_ERR, "Too many permissions in common '%s'", common->datum.name);
+		rc = SEPOL_ERR;
 		goto exit;
 	}
 
@@ -3209,6 +3211,7 @@ int cil_gen_expandtypeattribute(struct cil_db *db, struct cil_tree_node *parse_c
 		expandattr->expand = CIL_FALSE;
 	} else {
 		cil_log(CIL_ERR, "Value must be either \'true\' or \'false\'");
+		rc = SEPOL_ERR;
 		goto exit;
 	}
 
diff --git a/libsepol/cil/src/cil_resolve_ast.c b/libsepol/cil/src/cil_resolve_ast.c
index 872b6799..5389df43 100644
--- a/libsepol/cil/src/cil_resolve_ast.c
+++ b/libsepol/cil/src/cil_resolve_ast.c
@@ -772,6 +772,7 @@ int cil_resolve_classcommon(struct cil_tree_node *current, void *extra_args)
 	class->num_perms += common->num_perms;
 	if (class->num_perms > CIL_PERMS_PER_CLASS) {
 		cil_tree_log(current, CIL_ERR, "Too many permissions in class '%s' when including common permissions", class->datum.name);
+		rc = SEPOL_ERR;
 		goto exit;
 	}
 
@@ -1484,6 +1485,7 @@ int cil_resolve_classorder(struct cil_tree_node *current, void *extra_args)
 		rc = cil_resolve_name(current, (char *)curr->data, CIL_SYM_CLASSES, extra_args, &datum);
 		if (rc != SEPOL_OK) {
 			cil_log(CIL_ERR, "Failed to resolve class %s in classorder\n", (char *)curr->data);
+			rc = SEPOL_ERR;
 			goto exit;
 		}
 		cil_list_append(new, CIL_CLASS, datum);
@@ -2464,6 +2466,7 @@ int cil_resolve_blockabstract(struct cil_tree_node *current, void *extra_args)
 	block_node = NODE(block_datum);
 	if (block_node->flavor != CIL_BLOCK) {
 		cil_log(CIL_ERR, "Failed to resolve blockabstract to a block, rc: %d\n", rc);
+		rc = SEPOL_ERR;
 		goto exit;
 	}
 
-- 
2.26.3

[PATCH 2/5] libsepol/cil: Detect degenerate inheritance and exit with an error

[James Carter]

A CIL policy with inheritance of the form
...
(blockinherit ba)
(block ba
  (block b1
    (blockinherit bb)
  )
  (block bb
    (block b2
      (blockinherit bc)
    )
    (block bc
      (block b3
        (blockinherit bd)
      )
      (block bd
        (block b4
          (blockinherit be)
        )
        (block be
        ...
will require creating 2^depth copies of the block at the bottom of
the inheritance chain. This pattern can quickly consume all the
memory of the system compiling this policy.

The depth of the inheritance chain can be found be walking the
tree up through the parents and noting how many of the parent
blocks have been inherited. The number of times a block will be
copied is found by counting the list of nodes in the "bi_nodes"
list of the block. To minimize legitimate policies from being
falsely detected as being degenerate, both the depth and breadth
(number of copies) are checked and an error is given only if both
exceed the limits (depth >= 12 and breadth >= 4096).

This problem was found by the secilc-fuzzer.

Signed-off-by: James Carter <jwcart2@gmail.com>
---
 libsepol/cil/src/cil_internal.h    |  2 ++
 libsepol/cil/src/cil_resolve_ast.c | 54 ++++++++++++++++++++++++++++++
 2 files changed, 56 insertions(+)

diff --git a/libsepol/cil/src/cil_internal.h b/libsepol/cil/src/cil_internal.h
index 9bdcbdd0..74e0b34d 100644
--- a/libsepol/cil/src/cil_internal.h
+++ b/libsepol/cil/src/cil_internal.h
@@ -48,6 +48,8 @@
 
 #define CIL_MAX_NAME_LENGTH 2048
 
+#define CIL_DEGENERATE_INHERITANCE_DEPTH 12
+#define CIL_DEGENERATE_INHERITANCE_BREADTH (0x1 << CIL_DEGENERATE_INHERITANCE_DEPTH)
 
 enum cil_pass {
 	CIL_PASS_INIT = 0,
diff --git a/libsepol/cil/src/cil_resolve_ast.c b/libsepol/cil/src/cil_resolve_ast.c
index 5389df43..68909647 100644
--- a/libsepol/cil/src/cil_resolve_ast.c
+++ b/libsepol/cil/src/cil_resolve_ast.c
@@ -2410,6 +2410,55 @@ exit:
 	return rc;
 }
 
+/*
+ * Detect degenerate inheritance of the form:
+ * ...
+ * (blockinherit ba)
+ * (block ba
+ *    (block b1
+ *      (blockinherit bb)
+ *    )
+ *    (block bb
+ *      (block b2
+ *        (blockinherit bc)
+ *      )
+ *      (block bc
+ *      ...
+ */
+static int cil_check_for_degenerate_inheritance(struct cil_tree_node *current)
+{
+	struct cil_block *block = current->data;
+	struct cil_tree_node *node;
+	struct cil_list_item *item;
+	unsigned depth;
+	unsigned breadth = 0;
+
+	cil_list_for_each(item, block->bi_nodes) {
+		breadth++;
+	}
+
+	if (breadth >= CIL_DEGENERATE_INHERITANCE_BREADTH) {
+		node = current->parent;
+		depth = 0;
+		while (node && node->flavor != CIL_ROOT) {
+			if (node->flavor == CIL_BLOCK) {
+				block = node->data;
+				if (block->bi_nodes != NULL) {
+					depth++;
+				}
+			}
+			node = node->parent;
+		}
+
+		if (depth >= CIL_DEGENERATE_INHERITANCE_DEPTH) {
+			cil_tree_log(current, CIL_ERR, "Degenerate inheritance detected (depth=%u, breadth=%u)", depth, breadth);
+			return SEPOL_ERR;
+		}
+	}
+
+	return SEPOL_OK;
+}
+
 int cil_resolve_blockinherit_copy(struct cil_tree_node *current, void *extra_args)
 {
 	struct cil_block *block = current->data;
@@ -2426,6 +2475,11 @@ int cil_resolve_blockinherit_copy(struct cil_tree_node *current, void *extra_arg
 
 	db = args->db;
 
+	rc = cil_check_for_degenerate_inheritance(current);
+	if (rc != SEPOL_OK) {
+		goto exit;
+	}
+
 	// Make sure this is the original block and not a merged block from a blockinherit
 	if (current != block->datum.nodes->head->data) {
 		rc = SEPOL_OK;
-- 
2.26.3

[PATCH 3/5] libsepol/cil: Check datum in ordered list for expected flavor

[James Carter]

The secilc-fuzzer found an out of bounds memory access occurs
when building the binary policy if a map class is included in a
classorder statement.

The order statements in CIL (sidorder, classorder, categoryorder,
and sensitivityorder) are used to specify an ordering for sids,
classes, categories, and sensitivities. When the order statments
are resolved and merged, only in the case of the category order
list is the datum resolved checked to see if it is the expected
flavor.

When resolving the sid, class, and sensitivity order statements,
check that each name resolved to a datum of the expected flavor
and return an error if it does not.

Signed-off-by: James Carter <jwcart2@gmail.com>
---
 libsepol/cil/src/cil_resolve_ast.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/libsepol/cil/src/cil_resolve_ast.c b/libsepol/cil/src/cil_resolve_ast.c
index 68909647..b081d45d 100644
--- a/libsepol/cil/src/cil_resolve_ast.c
+++ b/libsepol/cil/src/cil_resolve_ast.c
@@ -1488,6 +1488,11 @@ int cil_resolve_classorder(struct cil_tree_node *current, void *extra_args)
 			rc = SEPOL_ERR;
 			goto exit;
 		}
+		if (FLAVOR(datum) != CIL_CLASS) {
+			cil_log(CIL_ERR, "%s is not a class. Only classes are allowed in classorder statements\n", datum->name);
+			rc = SEPOL_ERR;
+			goto exit;
+		}
 		cil_list_append(new, CIL_CLASS, datum);
 	}
 
@@ -1526,6 +1531,12 @@ int cil_resolve_sidorder(struct cil_tree_node *current, void *extra_args)
 			cil_log(CIL_ERR, "Failed to resolve sid %s in sidorder\n", (char *)curr->data);
 			goto exit;
 		}
+		if (FLAVOR(datum) != CIL_SID) {
+			cil_log(CIL_ERR, "%s is not a sid. Only sids are allowed in sidorder statements\n", datum->name);
+			rc = SEPOL_ERR;
+			goto exit;
+		}
+
 		cil_list_append(new, CIL_SID, datum);
 	}
 
@@ -1617,6 +1628,11 @@ int cil_resolve_sensitivityorder(struct cil_tree_node *current, void *extra_args
 			cil_log(CIL_ERR, "Failed to resolve sensitivty %s in sensitivityorder\n", (char *)curr->data);
 			goto exit;
 		}
+		if (FLAVOR(datum) != CIL_SENS) {
+			cil_log(CIL_ERR, "%s is not a sensitivity. Only sensitivities are allowed in sensitivityorder statements\n", datum->name);
+			rc = SEPOL_ERR;
+			goto exit;
+		}
 		cil_list_append(new, CIL_SENS, datum);
 	}
 
-- 
2.26.3

[PATCH 4/5] libsepol/cil: Check for self-referential loops in sets

[James Carter]

The secilc-fuzzer found a self-referential loop using category sets.
Any set declaration in CIL that allows sets in it is susceptible to
the creation of a self-referential loop. There is a check, but only
for the name of the set being declared being used in the set
declaration.

Check for self-refential loops in user, role, and type attributes
and in category sets. Since all of the sets need to be declared,
this check has to be done when verifying the CIL db before doing
the post phase.

Signed-off-by: James Carter <jwcart2@gmail.com>
---
 libsepol/cil/src/cil_resolve_ast.c | 31 +---------
 libsepol/cil/src/cil_verify.c      | 97 +++++++++++++++++++++---------
 libsepol/cil/src/cil_verify.h      |  1 -
 3 files changed, 71 insertions(+), 58 deletions(-)

diff --git a/libsepol/cil/src/cil_resolve_ast.c b/libsepol/cil/src/cil_resolve_ast.c
index b081d45d..34c28fc6 100644
--- a/libsepol/cil/src/cil_resolve_ast.c
+++ b/libsepol/cil/src/cil_resolve_ast.c
@@ -438,11 +438,6 @@ int cil_resolve_typeattributeset(struct cil_tree_node *current, void *extra_args
 		goto exit;
 	}
 
-	rc = cil_verify_no_self_reference(attr_datum, attrtypes->datum_expr);
-	if (rc != SEPOL_OK) {
-		goto exit;
-	}
-
 	if (attr->expr_list == NULL) {
 		cil_list_init(&attr->expr_list, CIL_TYPEATTRIBUTE);
 	}
@@ -1151,11 +1146,6 @@ int cil_resolve_roleattributeset(struct cil_tree_node *current, void *extra_args
 		goto exit;
 	}
 
-	rc = cil_verify_no_self_reference(attr_datum, attrroles->datum_expr);
-	if (rc != SEPOL_OK) {
-		goto exit;
-	}
-
 	if (attr->expr_list == NULL) {
 		cil_list_init(&attr->expr_list, CIL_ROLEATTRIBUTE);
 	}
@@ -1666,21 +1656,7 @@ exit:
 
 int cil_resolve_catset(struct cil_tree_node *current, struct cil_catset *catset, void *extra_args)
 {
-	int rc = SEPOL_ERR;
-
-	rc = cil_resolve_cats(current, catset->cats, extra_args);
-	if (rc != SEPOL_OK) {
-		goto exit;
-	}
-
-	rc = cil_verify_no_self_reference((struct cil_symtab_datum *)catset, catset->cats->datum_expr);
-	if (rc != SEPOL_OK) {
-		cil_list_destroy(&catset->cats->datum_expr, CIL_FALSE);
-		goto exit;
-	}
-
-exit:
-	return rc;
+	return cil_resolve_cats(current, catset->cats, extra_args);
 }
 
 int cil_resolve_senscat(struct cil_tree_node *current, void *extra_args)
@@ -3544,11 +3520,6 @@ int cil_resolve_userattributeset(struct cil_tree_node *current, void *extra_args
 		goto exit;
 	}
 
-	rc = cil_verify_no_self_reference(attr_datum, attrusers->datum_expr);
-	if (rc != SEPOL_OK) {
-		goto exit;
-	}
-
 	if (attr->expr_list == NULL) {
 		cil_list_init(&attr->expr_list, CIL_USERATTRIBUTE);
 	}
diff --git a/libsepol/cil/src/cil_verify.c b/libsepol/cil/src/cil_verify.c
index 5a37dd2f..4c860a21 100644
--- a/libsepol/cil/src/cil_verify.c
+++ b/libsepol/cil/src/cil_verify.c
@@ -430,28 +430,71 @@ int cil_verify_decl_does_not_shadow_macro_parameter(struct cil_macro *macro, str
 	return SEPOL_OK;
 }
 
-int cil_verify_no_self_reference(struct cil_symtab_datum *datum, struct cil_list *datum_list)
+int cil_verify_no_self_reference(enum cil_flavor flavor, struct cil_symtab_datum *datum, struct cil_symtab_datum *orig);
+
+static int __verify_no_self_reference_in_expr(struct cil_list *expr, struct cil_symtab_datum *orig)
 {
-	struct cil_list_item *i;
+	struct cil_list_item *item;
+	int rc = SEPOL_OK;
 
-	cil_list_for_each(i, datum_list) {
-		if (i->flavor == CIL_DATUM) {
-			struct cil_symtab_datum *d = i->data;
-			if (d == datum) {
-				cil_log(CIL_ERR,"Self-reference found for %s\n",datum->name);
-				return SEPOL_ERR;
-			}
-		} else if (i->flavor == CIL_LIST) {
-			int rc = cil_verify_no_self_reference(datum, i->data);
-			if (rc != SEPOL_OK) {
-				return SEPOL_ERR;
-			}
+	if (!expr) {
+		return SEPOL_OK;
+	}
+
+	cil_list_for_each(item, expr) {
+		if (item->flavor == CIL_DATUM) {
+			struct cil_symtab_datum* datum = item->data;
+			rc = cil_verify_no_self_reference(FLAVOR(datum), datum, orig);
+		} else if (item->flavor == CIL_LIST) {
+			rc = __verify_no_self_reference_in_expr(item->data, orig);
+		}
+		if (rc != SEPOL_OK) {
+			return SEPOL_ERR;
 		}
 	}
 
 	return SEPOL_OK;
 }
 
+int cil_verify_no_self_reference(enum cil_flavor flavor, struct cil_symtab_datum *datum, struct cil_symtab_datum *orig)
+{
+	int rc = SEPOL_OK;
+
+	if (datum == orig) {
+		cil_tree_log(NODE(orig), CIL_ERR, "Self-reference found for %s", orig->name);
+		return SEPOL_ERR;
+	} else if (orig == NULL) {
+		orig = datum;
+	}
+
+	switch (flavor) {
+	case CIL_USERATTRIBUTE: {
+		struct cil_userattribute *attr = (struct cil_userattribute *)datum;
+		rc = __verify_no_self_reference_in_expr(attr->expr_list, orig);
+		break;
+	}
+	case CIL_ROLEATTRIBUTE: {
+		struct cil_roleattribute *attr = (struct cil_roleattribute *)datum;
+		rc = __verify_no_self_reference_in_expr(attr->expr_list, orig);
+		break;
+	}
+	case CIL_TYPEATTRIBUTE: {
+		struct cil_typeattribute *attr = (struct cil_typeattribute *)datum;
+		rc = __verify_no_self_reference_in_expr(attr->expr_list, orig);
+		break;
+	}
+	case CIL_CATSET: {
+		struct cil_catset *set = (struct cil_catset *)datum;
+		rc = __verify_no_self_reference_in_expr(set->cats->datum_expr, orig);
+		break;
+	}
+	default:
+		break;
+	}
+
+	return rc;
+}
+
 int __cil_verify_ranges(struct cil_list *list)
 {
 	int rc = SEPOL_ERR;
@@ -1757,27 +1800,22 @@ static int __cil_verify_map_class(struct cil_tree_node *node)
 
 int __cil_pre_verify_helper(struct cil_tree_node *node, uint32_t *finished, __attribute__((unused)) void *extra_args)
 {
-	int rc = SEPOL_ERR;
+	int rc = SEPOL_OK;
 
-	if (node->flavor == CIL_MACRO) {
+	switch (node->flavor) {
+	case CIL_MACRO: {
 		*finished = CIL_TREE_SKIP_HEAD;
-		rc = SEPOL_OK;
-		goto exit;
-	} else if (node->flavor == CIL_BLOCK) {
+		break;
+	}
+	case CIL_BLOCK: {
 		struct cil_block *blk = node->data;
 		if (blk->is_abstract == CIL_TRUE) {
 			*finished = CIL_TREE_SKIP_HEAD;
 		}
-		rc = SEPOL_OK;
-		goto exit;
+		break;
 	}
-
-	switch (node->flavor) {
 	case CIL_USER:
 		rc = __cil_verify_user_pre_eval(node);
-		if (rc != SEPOL_OK) {
-			goto exit;
-		}
 		break;
 	case CIL_MAP_CLASS:
 		rc = __cil_verify_map_class(node);
@@ -1785,11 +1823,16 @@ int __cil_pre_verify_helper(struct cil_tree_node *node, uint32_t *finished, __at
 	case CIL_CLASSPERMISSION:
 		rc = __cil_verify_classpermission(node);
 		break;
+	case CIL_USERATTRIBUTE:
+	case CIL_ROLEATTRIBUTE:
+	case CIL_TYPEATTRIBUTE:
+	case CIL_CATSET:
+		rc = cil_verify_no_self_reference(node->flavor, node->data, NULL);
+		break;
 	default:
 		rc = SEPOL_OK;
 		break;
 	}
 
-exit:
 	return rc;
 }
diff --git a/libsepol/cil/src/cil_verify.h b/libsepol/cil/src/cil_verify.h
index c497018f..4ea14f5b 100644
--- a/libsepol/cil/src/cil_verify.h
+++ b/libsepol/cil/src/cil_verify.h
@@ -63,7 +63,6 @@ int cil_verify_constraint_leaf_expr_syntax(enum cil_flavor l_flavor, enum cil_fl
 int cil_verify_constraint_expr_syntax(struct cil_tree_node *current, enum cil_flavor op);
 int cil_verify_conditional_blocks(struct cil_tree_node *current);
 int cil_verify_decl_does_not_shadow_macro_parameter(struct cil_macro *macro, struct cil_tree_node *node, const char *name);
-int cil_verify_no_self_reference(struct cil_symtab_datum *datum, struct cil_list *datum_list);
 int __cil_verify_ranges(struct cil_list *list);
 int __cil_verify_ordered_node_helper(struct cil_tree_node *node, uint32_t *finished, void *extra_args);
 int __cil_verify_ordered(struct cil_tree_node *current, enum cil_flavor flavor);
-- 
2.26.3

Re: [PATCH 4/5] libsepol/cil: Check for self-referential loops in sets

[Nicolas Iooss]

On Wed, Apr 28, 2021 at 10:18 PM James Carter <jwcart2@gmail.com> wrote:
>
> The secilc-fuzzer found a self-referential loop using category sets.
> Any set declaration in CIL that allows sets in it is susceptible to
> the creation of a self-referential loop. There is a check, but only
> for the name of the set being declared being used in the set
> declaration.
>
> Check for self-refential loops in user, role, and type attributes
> and in category sets. Since all of the sets need to be declared,
> this check has to be done when verifying the CIL db before doing
> the post phase.
>
> Signed-off-by: James Carter <jwcart2@gmail.com>
> ---
>  libsepol/cil/src/cil_resolve_ast.c | 31 +---------
>  libsepol/cil/src/cil_verify.c      | 97 +++++++++++++++++++++---------
>  libsepol/cil/src/cil_verify.h      |  1 -
>  3 files changed, 71 insertions(+), 58 deletions(-)
>
[...]
> diff --git a/libsepol/cil/src/cil_verify.c b/libsepol/cil/src/cil_verify.c
> index 5a37dd2f..4c860a21 100644
> --- a/libsepol/cil/src/cil_verify.c
> +++ b/libsepol/cil/src/cil_verify.c
> @@ -430,28 +430,71 @@ int cil_verify_decl_does_not_shadow_macro_parameter(struct cil_macro *macro, str
>         return SEPOL_OK;
>  }
>
> -int cil_verify_no_self_reference(struct cil_symtab_datum *datum, struct cil_list *datum_list)
> +int cil_verify_no_self_reference(enum cil_flavor flavor, struct cil_symtab_datum *datum, struct cil_symtab_datum *orig);

Hello,
Your patches look fine. Nevertheless it would be cleaner if this
function was declared "static", as it is not used outside of
cil_verify.c. This is a suggestion which is not blocking any merge, so
if you prefer to merge the patches directly, feel free to do so.

Acked-by: Nicolas Iooss <nicolas.iooss@m4x.org>

And thanks for working on fixing issues identified by OSS-Fuzz!
Nicolas

Re: [PATCH 4/5] libsepol/cil: Check for self-referential loops in sets

[James Carter]

On Sat, May 1, 2021 at 10:33 AM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
>
> On Wed, Apr 28, 2021 at 10:18 PM James Carter <jwcart2@gmail.com> wrote:
> >
> > The secilc-fuzzer found a self-referential loop using category sets.
> > Any set declaration in CIL that allows sets in it is susceptible to
> > the creation of a self-referential loop. There is a check, but only
> > for the name of the set being declared being used in the set
> > declaration.
> >
> > Check for self-refential loops in user, role, and type attributes
> > and in category sets. Since all of the sets need to be declared,
> > this check has to be done when verifying the CIL db before doing
> > the post phase.
> >
> > Signed-off-by: James Carter <jwcart2@gmail.com>
> > ---
> >  libsepol/cil/src/cil_resolve_ast.c | 31 +---------
> >  libsepol/cil/src/cil_verify.c      | 97 +++++++++++++++++++++---------
> >  libsepol/cil/src/cil_verify.h      |  1 -
> >  3 files changed, 71 insertions(+), 58 deletions(-)
> >
> [...]
> > diff --git a/libsepol/cil/src/cil_verify.c b/libsepol/cil/src/cil_verify.c
> > index 5a37dd2f..4c860a21 100644
> > --- a/libsepol/cil/src/cil_verify.c
> > +++ b/libsepol/cil/src/cil_verify.c
> > @@ -430,28 +430,71 @@ int cil_verify_decl_does_not_shadow_macro_parameter(struct cil_macro *macro, str
> >         return SEPOL_OK;
> >  }
> >
> > -int cil_verify_no_self_reference(struct cil_symtab_datum *datum, struct cil_list *datum_list)
> > +int cil_verify_no_self_reference(enum cil_flavor flavor, struct cil_symtab_datum *datum, struct cil_symtab_datum *orig);
>
> Hello,
> Your patches look fine. Nevertheless it would be cleaner if this
> function was declared "static", as it is not used outside of
> cil_verify.c. This is a suggestion which is not blocking any merge, so
> if you prefer to merge the patches directly, feel free to do so.
>
> Acked-by: Nicolas Iooss <nicolas.iooss@m4x.org>
>

I like your suggestion better.
Thanks,
Jim

> And thanks for working on fixing issues identified by OSS-Fuzz!
> Nicolas
>

[PATCH 5/5] libsepol/cil: Return an error if a call argument fails to resolve

[James Carter]

Return an error if a call argument fails to resolve so that
the resolution phase stops and returns an error.

This problem was found by the secilc-fuzzer.

Signed-off-by: James Carter <jwcart2@gmail.com>
---
 libsepol/cil/src/cil_resolve_ast.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/libsepol/cil/src/cil_resolve_ast.c b/libsepol/cil/src/cil_resolve_ast.c
index 34c28fc6..5368ae80 100644
--- a/libsepol/cil/src/cil_resolve_ast.c
+++ b/libsepol/cil/src/cil_resolve_ast.c
@@ -3253,6 +3253,7 @@ int cil_resolve_call2(struct cil_tree_node *current, void *extra_args)
 		if (sym_index != CIL_SYM_UNKNOWN) {
 			rc = cil_resolve_name(current, arg->arg_str, sym_index, extra_args, &(arg->arg));
 			if (rc != SEPOL_OK) {
+				cil_tree_log(current, CIL_ERR, "Failed to resolve %s in call argument list", arg->arg_str);
 				goto exit;
 			}
 		}
@@ -3284,7 +3285,7 @@ int cil_resolve_name_call_args(struct cil_call *call, char *name, enum cil_sym_i
 		if (param_index == sym_index) {
 			if (name == arg->param_str) {
 				*datum = arg->arg;
-				rc = SEPOL_OK;
+				rc = *datum ? SEPOL_OK : SEPOL_ERR;
 				goto exit;
 			}
 		}
-- 
2.26.3

Re: [PATCH 0/5] Fix bugs identified by the secilc-fuzzer

[James Carter]

On Wed, Apr 28, 2021 at 4:17 PM James Carter <jwcart2@gmail.com> wrote:
>
> These patches are all related by the fact that the secilc-fuzzer
> identified the bugs that led to them. Wherever possible I have
> tried to fix all of the issues related to the specific one found.
>
> James Carter (5):
>   libsepol/cil: Fix instances where an error returns SEPOL_OK
>   libsepol/cil: Detect degenerate inheritance and exit with an error
>   libsepol/cil: Check datum in ordered list for expected flavor
>   libsepol/cil: Check for self-referential loops in sets
>   libsepol/cil: Return an error if a call argument fails to resolve
>
>  libsepol/cil/src/cil_build_ast.c   |   3 +
>  libsepol/cil/src/cil_internal.h    |   2 +
>  libsepol/cil/src/cil_resolve_ast.c | 107 ++++++++++++++++++++---------
>  libsepol/cil/src/cil_verify.c      |  97 ++++++++++++++++++--------
>  libsepol/cil/src/cil_verify.h      |   1 -
>  5 files changed, 151 insertions(+), 59 deletions(-)
>
> --
> 2.26.3
>

All but patch 4 has been applied. I will send a v2 for patch 4 based
on the suggested by Nicolas.
Jim