diff mbox series

[v2] libsepol/cil: Give an error when constraint expressions exceed max depth

Message ID 20200909163245.249011-1-jwcart2@gmail.com (mailing list archive)
State Superseded
Headers show
Series [v2] libsepol/cil: Give an error when constraint expressions exceed max depth | expand

Commit Message

James Carter Sept. 9, 2020, 4:32 p.m. UTC 
CIL was not correctly determining the depth of constraint expressions
which prevented it from giving an error when the max depth was exceeded.
This allowed invalid policy binaries with constraint expressions exceeding
the max depth to be created.

Check the depth of a constraint expression after converting it to the
postfix form used in the binary policy and give an error when the max
depth is exceeded.

Reported-by: Jonathan Hettwer <j2468h@gmail.com>
Signed-off-by: James Carter <jwcart2@gmail.com>
---
 libsepol/cil/src/cil_binary.c    | 42 ++++++++++++++++++++++++++++++++
 libsepol/cil/src/cil_build_ast.c | 20 ++++-----------
 2 files changed, 47 insertions(+), 15 deletions(-)

Comments

James Carter Sept. 9, 2020, 6:04 p.m. UTC | #1 
I just realized that I copied more than just the depth checking. I am
going to keep the extra checks, but I need more error messages and a
new function name.

Jim

On Wed, Sep 9, 2020 at 12:32 PM James Carter <jwcart2@gmail.com> wrote:
>
> CIL was not correctly determining the depth of constraint expressions
> which prevented it from giving an error when the max depth was exceeded.
> This allowed invalid policy binaries with constraint expressions exceeding
> the max depth to be created.
>
> Check the depth of a constraint expression after converting it to the
> postfix form used in the binary policy and give an error when the max
> depth is exceeded.
>
> Reported-by: Jonathan Hettwer <j2468h@gmail.com>
> Signed-off-by: James Carter <jwcart2@gmail.com>
> ---
>  libsepol/cil/src/cil_binary.c    | 42 ++++++++++++++++++++++++++++++++
>  libsepol/cil/src/cil_build_ast.c | 20 ++++-----------
>  2 files changed, 47 insertions(+), 15 deletions(-)
>
> diff --git a/libsepol/cil/src/cil_binary.c b/libsepol/cil/src/cil_binary.c
> index 77266858..3131a63e 100644
> --- a/libsepol/cil/src/cil_binary.c
> +++ b/libsepol/cil/src/cil_binary.c
> @@ -2713,6 +2713,42 @@ int __cil_constrain_expr_to_sepol_expr(policydb_t *pdb, const struct cil_db *db,
>         return SEPOL_OK;
>  }
>
> +int __cil_constrain_expr_check_depth(constraint_expr_t *sepol_expr)
> +{
> +       constraint_expr_t *e = sepol_expr;
> +       int depth = -1;
> +
> +       while (e) {
> +               switch (e->expr_type) {
> +               case CEXPR_NOT:
> +                       if (depth < 0)
> +                               return SEPOL_ERR;
> +                       break;
> +               case CEXPR_AND:
> +               case CEXPR_OR:
> +                       if (depth < 1)
> +                               return SEPOL_ERR;
> +                       depth--;
> +                       break;
> +               case CEXPR_ATTR:
> +                       if (depth == (CEXPR_MAXDEPTH - 1))
> +                               return SEPOL_ERR;
> +                       depth++;
> +                       break;
> +               case CEXPR_NAMES:
> +                       if (depth == (CEXPR_MAXDEPTH - 1))
> +                               return SEPOL_ERR;
> +                       depth++;
> +                       break;
> +               default:
> +                       return SEPOL_ERR;
> +               }
> +               e = e->next;
> +       }
> +
> +       return SEPOL_OK;
> +}
> +
>  int cil_constrain_to_policydb_helper(policydb_t *pdb, const struct cil_db *db, struct cil_symtab_datum *class, struct cil_list *perms, struct cil_list *expr)
>  {
>         int rc = SEPOL_ERR;
> @@ -2736,6 +2772,12 @@ int cil_constrain_to_policydb_helper(policydb_t *pdb, const struct cil_db *db, s
>                 goto exit;
>         }
>
> +       rc = __cil_constrain_expr_check_depth(sepol_expr);
> +       if (rc != SEPOL_OK) {
> +               cil_log(CIL_ERR,"Constraint expression exceeded max allowable depth\n");
> +               goto exit;
> +       }
> +
>         sepol_constrain->expr = sepol_expr;
>         sepol_constrain->next = sepol_class->constraints;
>         sepol_class->constraints = sepol_constrain;
> diff --git a/libsepol/cil/src/cil_build_ast.c b/libsepol/cil/src/cil_build_ast.c
> index 60ecaaff..870c6923 100644
> --- a/libsepol/cil/src/cil_build_ast.c
> +++ b/libsepol/cil/src/cil_build_ast.c
> @@ -2738,7 +2738,7 @@ exit:
>         return SEPOL_ERR;
>  }
>
> -static int __cil_fill_constraint_expr(struct cil_tree_node *current, enum cil_flavor flavor, struct cil_list **expr, int *depth)
> +static int __cil_fill_constraint_expr(struct cil_tree_node *current, enum cil_flavor flavor, struct cil_list **expr)
>  {
>         int rc = SEPOL_ERR;
>         enum cil_flavor op;
> @@ -2750,12 +2750,6 @@ static int __cil_fill_constraint_expr(struct cil_tree_node *current, enum cil_fl
>                 goto exit;
>         }
>
> -       if (*depth > CEXPR_MAXDEPTH) {
> -               cil_log(CIL_ERR, "Max depth of %d exceeded for constraint expression\n", CEXPR_MAXDEPTH);
> -               rc = SEPOL_ERR;
> -               goto exit;
> -       }
> -
>         op = __cil_get_constraint_operator_flavor(current->data);
>
>         rc = cil_verify_constraint_expr_syntax(current, op);
> @@ -2769,14 +2763,13 @@ static int __cil_fill_constraint_expr(struct cil_tree_node *current, enum cil_fl
>         case CIL_CONS_DOM:
>         case CIL_CONS_DOMBY:
>         case CIL_CONS_INCOMP:
> -               (*depth)++;
>                 rc = __cil_fill_constraint_leaf_expr(current, flavor, op, expr);
>                 if (rc != SEPOL_OK) {
>                         goto exit;
>                 }
>                 break;
>         case CIL_NOT:
> -               rc = __cil_fill_constraint_expr(current->next->cl_head, flavor, &lexpr, depth);
> +               rc = __cil_fill_constraint_expr(current->next->cl_head, flavor, &lexpr);
>                 if (rc != SEPOL_OK) {
>                         goto exit;
>                 }
> @@ -2785,11 +2778,11 @@ static int __cil_fill_constraint_expr(struct cil_tree_node *current, enum cil_fl
>                 cil_list_append(*expr, CIL_LIST, lexpr);
>                 break;
>         default:
> -               rc = __cil_fill_constraint_expr(current->next->cl_head, flavor, &lexpr, depth);
> +               rc = __cil_fill_constraint_expr(current->next->cl_head, flavor, &lexpr);
>                 if (rc != SEPOL_OK) {
>                         goto exit;
>                 }
> -               rc = __cil_fill_constraint_expr(current->next->next->cl_head, flavor, &rexpr, depth);
> +               rc = __cil_fill_constraint_expr(current->next->next->cl_head, flavor, &rexpr);
>                 if (rc != SEPOL_OK) {
>                         cil_list_destroy(&lexpr, CIL_TRUE);
>                         goto exit;
> @@ -2801,8 +2794,6 @@ static int __cil_fill_constraint_expr(struct cil_tree_node *current, enum cil_fl
>                 break;
>         }
>
> -       (*depth)--;
> -
>         return SEPOL_OK;
>  exit:
>
> @@ -2812,13 +2803,12 @@ exit:
>  int cil_gen_constraint_expr(struct cil_tree_node *current, enum cil_flavor flavor, struct cil_list **expr)
>  {
>         int rc = SEPOL_ERR;
> -       int depth = 0;
>
>         if (current->cl_head == NULL) {
>                 goto exit;
>         }
>
> -       rc = __cil_fill_constraint_expr(current->cl_head, flavor, expr, &depth);
> +       rc = __cil_fill_constraint_expr(current->cl_head, flavor, expr);
>         if (rc != SEPOL_OK) {
>                 goto exit;
>         }
> --
> 2.25.4
>
diff mbox series

Patch

diff --git a/libsepol/cil/src/cil_binary.c b/libsepol/cil/src/cil_binary.c
index 77266858..3131a63e 100644
--- a/libsepol/cil/src/cil_binary.c
+++ b/libsepol/cil/src/cil_binary.c
@@ -2713,6 +2713,42 @@  int __cil_constrain_expr_to_sepol_expr(policydb_t *pdb, const struct cil_db *db,
 	return SEPOL_OK;
 }
 
+int __cil_constrain_expr_check_depth(constraint_expr_t *sepol_expr)
+{
+	constraint_expr_t *e = sepol_expr;
+	int depth = -1;
+
+	while (e) {
+		switch (e->expr_type) {
+		case CEXPR_NOT:
+			if (depth < 0)
+				return SEPOL_ERR;
+			break;
+		case CEXPR_AND:
+		case CEXPR_OR:
+			if (depth < 1)
+				return SEPOL_ERR;
+			depth--;
+			break;
+		case CEXPR_ATTR:
+			if (depth == (CEXPR_MAXDEPTH - 1))
+				return SEPOL_ERR;
+			depth++;
+			break;
+		case CEXPR_NAMES:
+			if (depth == (CEXPR_MAXDEPTH - 1))
+				return SEPOL_ERR;
+			depth++;
+			break;
+		default:
+			return SEPOL_ERR;
+		}
+		e = e->next;
+	}
+
+	return SEPOL_OK;
+}
+
 int cil_constrain_to_policydb_helper(policydb_t *pdb, const struct cil_db *db, struct cil_symtab_datum *class, struct cil_list *perms, struct cil_list *expr)
 {
 	int rc = SEPOL_ERR;
@@ -2736,6 +2772,12 @@  int cil_constrain_to_policydb_helper(policydb_t *pdb, const struct cil_db *db, s
 		goto exit;
 	}
 
+	rc = __cil_constrain_expr_check_depth(sepol_expr);
+	if (rc != SEPOL_OK) {
+		cil_log(CIL_ERR,"Constraint expression exceeded max allowable depth\n");
+		goto exit;
+	}
+
 	sepol_constrain->expr = sepol_expr;
 	sepol_constrain->next = sepol_class->constraints;
 	sepol_class->constraints = sepol_constrain;
diff --git a/libsepol/cil/src/cil_build_ast.c b/libsepol/cil/src/cil_build_ast.c
index 60ecaaff..870c6923 100644
--- a/libsepol/cil/src/cil_build_ast.c
+++ b/libsepol/cil/src/cil_build_ast.c
@@ -2738,7 +2738,7 @@  exit:
 	return SEPOL_ERR;
 }
 
-static int __cil_fill_constraint_expr(struct cil_tree_node *current, enum cil_flavor flavor, struct cil_list **expr, int *depth)
+static int __cil_fill_constraint_expr(struct cil_tree_node *current, enum cil_flavor flavor, struct cil_list **expr)
 {
 	int rc = SEPOL_ERR;
 	enum cil_flavor op;
@@ -2750,12 +2750,6 @@  static int __cil_fill_constraint_expr(struct cil_tree_node *current, enum cil_fl
 		goto exit;
 	}
 
-	if (*depth > CEXPR_MAXDEPTH) {
-		cil_log(CIL_ERR, "Max depth of %d exceeded for constraint expression\n", CEXPR_MAXDEPTH);
-		rc = SEPOL_ERR;
-		goto exit;
-	}
-
 	op = __cil_get_constraint_operator_flavor(current->data);
 
 	rc = cil_verify_constraint_expr_syntax(current, op);
@@ -2769,14 +2763,13 @@  static int __cil_fill_constraint_expr(struct cil_tree_node *current, enum cil_fl
 	case CIL_CONS_DOM:
 	case CIL_CONS_DOMBY:
 	case CIL_CONS_INCOMP:
-		(*depth)++;
 		rc = __cil_fill_constraint_leaf_expr(current, flavor, op, expr);
 		if (rc != SEPOL_OK) {
 			goto exit;
 		}
 		break;
 	case CIL_NOT:
-		rc = __cil_fill_constraint_expr(current->next->cl_head, flavor, &lexpr, depth);
+		rc = __cil_fill_constraint_expr(current->next->cl_head, flavor, &lexpr);
 		if (rc != SEPOL_OK) {
 			goto exit;
 		}
@@ -2785,11 +2778,11 @@  static int __cil_fill_constraint_expr(struct cil_tree_node *current, enum cil_fl
 		cil_list_append(*expr, CIL_LIST, lexpr);
 		break;
 	default:
-		rc = __cil_fill_constraint_expr(current->next->cl_head, flavor, &lexpr, depth);
+		rc = __cil_fill_constraint_expr(current->next->cl_head, flavor, &lexpr);
 		if (rc != SEPOL_OK) {
 			goto exit;
 		}
-		rc = __cil_fill_constraint_expr(current->next->next->cl_head, flavor, &rexpr, depth);
+		rc = __cil_fill_constraint_expr(current->next->next->cl_head, flavor, &rexpr);
 		if (rc != SEPOL_OK) {
 			cil_list_destroy(&lexpr, CIL_TRUE);
 			goto exit;
@@ -2801,8 +2794,6 @@  static int __cil_fill_constraint_expr(struct cil_tree_node *current, enum cil_fl
 		break;
 	}
 
-	(*depth)--;
-
 	return SEPOL_OK;
 exit:
 
@@ -2812,13 +2803,12 @@  exit:
 int cil_gen_constraint_expr(struct cil_tree_node *current, enum cil_flavor flavor, struct cil_list **expr)
 {
 	int rc = SEPOL_ERR;
-	int depth = 0;
 
 	if (current->cl_head == NULL) {
 		goto exit;
 	}
 
-	rc = __cil_fill_constraint_expr(current->cl_head, flavor, expr, &depth);
+	rc = __cil_fill_constraint_expr(current->cl_head, flavor, expr);
 	if (rc != SEPOL_OK) {
 		goto exit;
 	}