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@xxxxxxxxx> 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@xxxxxxxxx>
> Signed-off-by: James Carter <jwcart2@xxxxxxxxx>
> ---
> 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
>
