[PATCH 0/6] More secilc-fuzzer problems fixed

[PATCH 0/6] More secilc-fuzzer problems fixed

[James Carter]

Patches 1-4 fix bug found directly by the secilc-fuzzer.
Patch 6 is the same class of bug as that fixed with patch 4.
Patch 5 fixes problems found while investigating patch 4 and 6.

James Carter (6):
  libsepol/cil: Handle disabled optional blocks in earlier passes
  libsepol/cil: Destroy the permission nodes when exiting with an error
  libsepol/cil: Limit the number of open parenthesis allowed
  libsepol/cil: Resolve anonymous class permission sets only once
  libsepol/cil: Pointers to datums should be set to NULL when resetting
  libsepol/cil: Resolve anonymous levels only once

 libsepol/cil/src/cil_build_ast.c   |  2 +
 libsepol/cil/src/cil_parser.c      |  7 +++-
 libsepol/cil/src/cil_reset_ast.c   | 56 +++++++++++++++++++++++++--
 libsepol/cil/src/cil_resolve_ast.c | 62 +++++++++++++++++-------------
 4 files changed, 97 insertions(+), 30 deletions(-)

-- 
2.26.3

[PATCH 1/6] libsepol/cil: Handle disabled optional blocks in earlier passes

[James Carter]

A failed tunable resolution in a tunableif can cause an optional
to be disabled before the CIL_PASS_CALL1 phase. If this occurs, the
optional block and its subtree should be destroyed, but no reset
will be required since tunables are not allowed inside an optional
block.

Anytime there are optional blocks in the disabled_optionals list
(changed == 1), destroy the optional block and its subtree even if
in a pass before CIL_PASS_CALL1.

This bug was found by the secilc-fuzzer.

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

diff --git a/libsepol/cil/src/cil_resolve_ast.c b/libsepol/cil/src/cil_resolve_ast.c
index 5368ae80..242aea4a 100644
--- a/libsepol/cil/src/cil_resolve_ast.c
+++ b/libsepol/cil/src/cil_resolve_ast.c
@@ -4099,35 +4099,37 @@ int cil_resolve_ast(struct cil_db *db, struct cil_tree_node *current)
 			}
 		}
 
-		if (changed && (pass > CIL_PASS_CALL1)) {
+		if (changed) {
 			struct cil_list_item *item;
-			/* Need to re-resolve because an optional was disabled that contained
-			 * one or more declarations. We only need to reset to the call1 pass 
-			 * because things done in the preceding passes aren't allowed in 
-			 * optionals, and thus can't be disabled.
-			 * Note: set pass to CIL_PASS_CALL1 because the pass++ will increment 
-			 * it to CIL_PASS_CALL2
-			 */
-			cil_log(CIL_INFO, "Resetting declarations\n");
-
-			if (pass >= CIL_PASS_MISC1) {
-				__cil_ordered_lists_reset(&extra_args.sidorder_lists);
-				__cil_ordered_lists_reset(&extra_args.classorder_lists);
-				__cil_ordered_lists_reset(&extra_args.unordered_classorder_lists);
-				__cil_ordered_lists_reset(&extra_args.catorder_lists);
-				__cil_ordered_lists_reset(&extra_args.sensitivityorder_lists);
-				cil_list_destroy(&db->sidorder, CIL_FALSE);
-				cil_list_destroy(&db->classorder, CIL_FALSE);
-				cil_list_destroy(&db->catorder, CIL_FALSE);
-				cil_list_destroy(&db->sensitivityorder, CIL_FALSE);
-			}
+			if (pass > CIL_PASS_CALL1) {
+				/* Need to re-resolve because an optional was disabled that contained
+				 * one or more declarations. We only need to reset to the call1 pass
+				 * because things done in the preceding passes aren't allowed in
+				 * optionals, and thus can't be disabled.
+				 * Note: set pass to CIL_PASS_CALL1 because the pass++ will increment
+				 * it to CIL_PASS_CALL2
+				 */
+				cil_log(CIL_INFO, "Resetting declarations\n");
+
+				if (pass >= CIL_PASS_MISC1) {
+					__cil_ordered_lists_reset(&extra_args.sidorder_lists);
+					__cil_ordered_lists_reset(&extra_args.classorder_lists);
+					__cil_ordered_lists_reset(&extra_args.unordered_classorder_lists);
+					__cil_ordered_lists_reset(&extra_args.catorder_lists);
+					__cil_ordered_lists_reset(&extra_args.sensitivityorder_lists);
+					cil_list_destroy(&db->sidorder, CIL_FALSE);
+					cil_list_destroy(&db->classorder, CIL_FALSE);
+					cil_list_destroy(&db->catorder, CIL_FALSE);
+					cil_list_destroy(&db->sensitivityorder, CIL_FALSE);
+				}
 
-			pass = CIL_PASS_CALL1;
+				pass = CIL_PASS_CALL1;
 
-			rc = cil_reset_ast(current);
-			if (rc != SEPOL_OK) {
-				cil_log(CIL_ERR, "Failed to reset declarations\n");
-				goto exit;
+				rc = cil_reset_ast(current);
+				if (rc != SEPOL_OK) {
+					cil_log(CIL_ERR, "Failed to reset declarations\n");
+					goto exit;
+				}
 			}
 			cil_list_for_each(item, extra_args.disabled_optionals) {
 				cil_tree_children_destroy(item->data);
-- 
2.26.3

[PATCH 2/6] libsepol/cil: Destroy the permission nodes when exiting with an error

[James Carter]

When exiting with an error because a class or common has too many
permissions, destroy the permission nodes.

This bug was found by the secilc-fuzzer.

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

diff --git a/libsepol/cil/src/cil_build_ast.c b/libsepol/cil/src/cil_build_ast.c
index 87043a8f..71f14e20 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);
+			cil_tree_children_destroy(ast_node);
 			rc = SEPOL_ERR;
 			goto exit;
 		}
@@ -1019,6 +1020,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);
+		cil_tree_children_destroy(ast_node);
 		rc = SEPOL_ERR;
 		goto exit;
 	}
-- 
2.26.3

[PATCH 3/6] libsepol/cil: Limit the number of open parenthesis allowed

[James Carter]

When parsing a CIL policy, the number of open parenthesis is tracked
to verify that each has a matching close parenthesis. If there are
too many open parenthesis, a stack overflow could occur during later
processing.

Exit with an error if the number of open parenthesis exceeds 4096
(which should be enough for any policy.)

This bug was found by the secilc-fuzzer.

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

diff --git a/libsepol/cil/src/cil_parser.c b/libsepol/cil/src/cil_parser.c
index a9306218..fb95f401 100644
--- a/libsepol/cil/src/cil_parser.c
+++ b/libsepol/cil/src/cil_parser.c
@@ -42,6 +42,8 @@
 #include "cil_strpool.h"
 #include "cil_stack.h"
 
+#define CIL_PARSER_MAX_EXPR_DEPTH (0x1 << 12)
+
 char *CIL_KEY_HLL_LMS;
 char *CIL_KEY_HLL_LMX;
 char *CIL_KEY_HLL_LME;
@@ -245,7 +247,10 @@ int cil_parser(const char *_path, char *buffer, uint32_t size, struct cil_tree *
 			break;
 		case OPAREN:
 			paren_count++;
-
+			if (paren_count > CIL_PARSER_MAX_EXPR_DEPTH) {
+				cil_log(CIL_ERR, "Number of open parenthesis exceeds limit of %d at line %d of %s\n", CIL_PARSER_MAX_EXPR_DEPTH, tok.line, path);
+				goto exit;
+			}
 			create_node(&node, current, tok.line, hll_lineno, NULL);
 			insert_node(node, current);
 			current = node;
-- 
2.26.3

[PATCH 4/6] libsepol/cil: Resolve anonymous class permission sets only once

[James Carter]

Anonymous class permission sets can be passed as call arguments.
Anonymous call arguments are resolved when they are used in a
rule. [This is because all the information might not be present
(like common permissions being added to a class) when the call
itself is resolved.] If there is more than one rule using an
anonymous class permission set, then a memory leak will occur
when a new list for the permission datum expression is created
without destroying the old one.

When resolving the class and permissions, check if the class has
already been resolved. If it has, then the permissions have been
as well.

This bug was found by the secilc-fuzzer.

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

diff --git a/libsepol/cil/src/cil_resolve_ast.c b/libsepol/cil/src/cil_resolve_ast.c
index 242aea4a..865297fb 100644
--- a/libsepol/cil/src/cil_resolve_ast.c
+++ b/libsepol/cil/src/cil_resolve_ast.c
@@ -158,6 +158,10 @@ int cil_resolve_classperms(struct cil_tree_node *current, struct cil_classperms
 	symtab_t *common_symtab = NULL;
 	struct cil_class *class;
 
+	if (cp->class) {
+		return SEPOL_OK;
+	}
+
 	rc = cil_resolve_name(current, cp->class_str, CIL_SYM_CLASSES, extra_args, &datum);
 	if (rc != SEPOL_OK) {
 		goto exit;
-- 
2.26.3

[PATCH 5/6] libsepol/cil: Pointers to datums should be set to NULL when resetting

[James Carter]

Set the pointer to the sensitivity in levels, the pointers to the low
and high levels in levelranges, the pointer to the level in userlevels,
the pointer to the range in userranges, and the pointers to contexts
in ocontexts to NULL.

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

diff --git a/libsepol/cil/src/cil_reset_ast.c b/libsepol/cil/src/cil_reset_ast.c
index d24d4f81..6d1d2da7 100644
--- a/libsepol/cil/src/cil_reset_ast.c
+++ b/libsepol/cil/src/cil_reset_ast.c
@@ -140,8 +140,11 @@ static void cil_reset_userattributeset(struct cil_userattributeset *uas)
 
 static void cil_reset_selinuxuser(struct cil_selinuxuser *selinuxuser)
 {
+	selinuxuser->user = NULL;
 	if (selinuxuser->range_str == NULL) {
 		cil_reset_levelrange(selinuxuser->range);
+	} else {
+		selinuxuser->range = NULL;
 	}
 }
 
@@ -214,6 +217,8 @@ static void cil_reset_rangetransition(struct cil_rangetransition *rangetrans)
 {
 	if (rangetrans->range_str == NULL) {
 		cil_reset_levelrange(rangetrans->range);
+	} else {
+		rangetrans->range = NULL;
 	}
 }
 
@@ -251,6 +256,7 @@ static void cil_reset_catset(struct cil_catset *catset)
 
 static inline void cil_reset_level(struct cil_level *level)
 {
+	level->sens = NULL;
 	cil_reset_cats(level->cats);
 }
 
@@ -258,10 +264,14 @@ static inline void cil_reset_levelrange(struct cil_levelrange *levelrange)
 {
 	if (levelrange->low_str == NULL) {
 		cil_reset_level(levelrange->low);
+	} else {
+		levelrange->low = NULL;
 	}
 
 	if (levelrange->high_str == NULL) {
 		cil_reset_level(levelrange->high);
+	} else {
+		levelrange->high = NULL;
 	}
 }
 
@@ -269,6 +279,8 @@ static inline void cil_reset_userlevel(struct cil_userlevel *userlevel)
 {
 	if (userlevel->level_str == NULL) {
 		cil_reset_level(userlevel->level);
+	} else {
+		userlevel->level = NULL;
 	}
 }
 
@@ -276,13 +288,20 @@ static inline void cil_reset_userrange(struct cil_userrange *userrange)
 {
 	if (userrange->range_str == NULL) {
 		cil_reset_levelrange(userrange->range);
+	} else {
+		userrange->range = NULL;
 	}
 }
 
 static inline void cil_reset_context(struct cil_context *context)
 {
+	if (!context) {
+		return;
+	}
 	if (context->range_str == NULL) {
 		cil_reset_levelrange(context->range);
+	} else {
+		context->range = NULL;
 	}
 }
 
@@ -290,26 +309,35 @@ static void cil_reset_sidcontext(struct cil_sidcontext *sidcontext)
 {
 	if (sidcontext->context_str == NULL) {
 		cil_reset_context(sidcontext->context);
+	} else {
+		sidcontext->context = NULL;
 	}
 }
 
 static void cil_reset_filecon(struct cil_filecon *filecon)
 {
-	if (filecon->context_str == NULL && filecon->context != NULL) {
+	if (filecon->context_str == NULL) {
 		cil_reset_context(filecon->context);
+	} else {
+		filecon->context = NULL;
 	}
 }
 
 static void cil_reset_ibpkeycon(struct cil_ibpkeycon *ibpkeycon)
 {
-	if (!ibpkeycon->context_str)
+	if (ibpkeycon->context_str == NULL) {
 		cil_reset_context(ibpkeycon->context);
+	} else {
+		ibpkeycon->context = NULL;
+	}
 }
 
 static void cil_reset_portcon(struct cil_portcon *portcon)
 {
 	if (portcon->context_str == NULL) {
 		cil_reset_context(portcon->context);
+	} else {
+		portcon->context = NULL;
 	}
 }
 
@@ -317,6 +345,8 @@ static void cil_reset_nodecon(struct cil_nodecon *nodecon)
 {
 	if (nodecon->context_str == NULL) {
 		cil_reset_context(nodecon->context);
+	} else {
+		nodecon->context = NULL;
 	}
 }
 
@@ -324,6 +354,8 @@ static void cil_reset_genfscon(struct cil_genfscon *genfscon)
 {
 	if (genfscon->context_str == NULL) {
 		cil_reset_context(genfscon->context);
+	} else {
+		genfscon->context = NULL;
 	}
 }
 
@@ -331,17 +363,23 @@ static void cil_reset_netifcon(struct cil_netifcon *netifcon)
 {
 	if (netifcon->if_context_str == NULL) {
 		cil_reset_context(netifcon->if_context);
+	} else {
+		netifcon->if_context = NULL;
 	}
 
 	if (netifcon->packet_context_str == NULL) {
 		cil_reset_context(netifcon->packet_context);
+	} else {
+		netifcon->packet_context = NULL;
 	}
 }
 
 static void cil_reset_ibendportcon(struct cil_ibendportcon *ibendportcon)
 {
-	if (!ibendportcon->context_str) {
+	if (ibendportcon->context_str == NULL) {
 		cil_reset_context(ibendportcon->context);
+	} else {
+		ibendportcon->context = NULL;
 	}
 }
 
@@ -349,6 +387,8 @@ static void cil_reset_pirqcon(struct cil_pirqcon *pirqcon)
 {
 	if (pirqcon->context_str == NULL) {
 		cil_reset_context(pirqcon->context);
+	} else {
+		pirqcon->context = NULL;
 	}
 }
 
@@ -356,6 +396,8 @@ static void cil_reset_iomemcon(struct cil_iomemcon *iomemcon)
 {
 	if (iomemcon->context_str == NULL) {
 		cil_reset_context(iomemcon->context);
+	} else {
+		iomemcon->context = NULL;
 	}
 }
 
@@ -363,6 +405,8 @@ static void cil_reset_ioportcon(struct cil_ioportcon *ioportcon)
 {
 	if (ioportcon->context_str == NULL) {
 		cil_reset_context(ioportcon->context);
+	} else {
+		ioportcon->context = NULL;
 	}
 }
 
@@ -370,6 +414,8 @@ static void cil_reset_pcidevicecon(struct cil_pcidevicecon *pcidevicecon)
 {
 	if (pcidevicecon->context_str == NULL) {
 		cil_reset_context(pcidevicecon->context);
+	} else {
+		pcidevicecon->context = NULL;
 	}
 }
 
@@ -377,6 +423,8 @@ static void cil_reset_devicetreecon(struct cil_devicetreecon *devicetreecon)
 {
 	if (devicetreecon->context_str == NULL) {
 		cil_reset_context(devicetreecon->context);
+	} else {
+		devicetreecon->context = NULL;
 	}
 }
 
@@ -384,6 +432,8 @@ static void cil_reset_fsuse(struct cil_fsuse *fsuse)
 {
 	if (fsuse->context_str == NULL) {
 		cil_reset_context(fsuse->context);
+	} else {
+		fsuse->context = NULL;
 	}
 }
 
-- 
2.26.3

[PATCH 6/6] libsepol/cil: Resolve anonymous levels only once

[James Carter]

Anonymous levels can be passed as call arguments and they can
appear in anonymous levelranges as well.

Anonymous call arguments are resolved when they are used in a rule.
If more than one rule uses the anonymous level, then a memory leak
will occur when a new list for the category datum expression is
created without destroying the old one.

When resolving a level, check if the sensitivity datum has already
been resolved. If it has, then the categories have been as well.

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

diff --git a/libsepol/cil/src/cil_resolve_ast.c b/libsepol/cil/src/cil_resolve_ast.c
index 865297fb..9c95f2a0 100644
--- a/libsepol/cil/src/cil_resolve_ast.c
+++ b/libsepol/cil/src/cil_resolve_ast.c
@@ -1700,6 +1700,10 @@ int cil_resolve_level(struct cil_tree_node *current, struct cil_level *level, vo
 	struct cil_symtab_datum *sens_datum = NULL;
 	int rc = SEPOL_ERR;
 
+	if (level->sens) {
+		return SEPOL_OK;
+	}
+
 	rc = cil_resolve_name(current, (char*)level->sens_str, CIL_SYM_SENS, extra_args, &sens_datum);
 	if (rc != SEPOL_OK) {
 		cil_log(CIL_ERR, "Failed to find sensitivity\n");
-- 
2.26.3

Re: [PATCH 0/6] More secilc-fuzzer problems fixed

[James Carter]

On Thu, May 13, 2021 at 2:53 PM James Carter <jwcart2@gmail.com> wrote:
>
> Patches 1-4 fix bug found directly by the secilc-fuzzer.
> Patch 6 is the same class of bug as that fixed with patch 4.
> Patch 5 fixes problems found while investigating patch 4 and 6.
>
> James Carter (6):
>   libsepol/cil: Handle disabled optional blocks in earlier passes
>   libsepol/cil: Destroy the permission nodes when exiting with an error
>   libsepol/cil: Limit the number of open parenthesis allowed
>   libsepol/cil: Resolve anonymous class permission sets only once
>   libsepol/cil: Pointers to datums should be set to NULL when resetting
>   libsepol/cil: Resolve anonymous levels only once
>
>  libsepol/cil/src/cil_build_ast.c   |  2 +
>  libsepol/cil/src/cil_parser.c      |  7 +++-
>  libsepol/cil/src/cil_reset_ast.c   | 56 +++++++++++++++++++++++++--
>  libsepol/cil/src/cil_resolve_ast.c | 62 +++++++++++++++++-------------
>  4 files changed, 97 insertions(+), 30 deletions(-)
>
> --
> 2.26.3
>

There haven't been any comments on this series, and it has been three
weeks, so I plan on merging it tomorrow.
Jim

Re: [PATCH 0/6] More secilc-fuzzer problems fixed

[James Carter]

On Thu, Jun 3, 2021 at 1:05 PM James Carter <jwcart2@gmail.com> wrote:
>
> On Thu, May 13, 2021 at 2:53 PM James Carter <jwcart2@gmail.com> wrote:
> >
> > Patches 1-4 fix bug found directly by the secilc-fuzzer.
> > Patch 6 is the same class of bug as that fixed with patch 4.
> > Patch 5 fixes problems found while investigating patch 4 and 6.
> >
> > James Carter (6):
> >   libsepol/cil: Handle disabled optional blocks in earlier passes
> >   libsepol/cil: Destroy the permission nodes when exiting with an error
> >   libsepol/cil: Limit the number of open parenthesis allowed
> >   libsepol/cil: Resolve anonymous class permission sets only once
> >   libsepol/cil: Pointers to datums should be set to NULL when resetting
> >   libsepol/cil: Resolve anonymous levels only once
> >
> >  libsepol/cil/src/cil_build_ast.c   |  2 +
> >  libsepol/cil/src/cil_parser.c      |  7 +++-
> >  libsepol/cil/src/cil_reset_ast.c   | 56 +++++++++++++++++++++++++--
> >  libsepol/cil/src/cil_resolve_ast.c | 62 +++++++++++++++++-------------
> >  4 files changed, 97 insertions(+), 30 deletions(-)
> >
> > --
> > 2.26.3
> >
>
> There haven't been any comments on this series, and it has been three
> weeks, so I plan on merging it tomorrow.
> Jim

This series has been merged.
Jim