| Message ID | 20200909182444.254828-1-jwcart2@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | libsepol/cil: Validate constraint expressions before adding to binary policy | expand |
On Wed, Sep 9, 2020 at 2:25 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. > > Validate the constraint expression using the same logic that is used > when reading the binary policy. This includes checking the depth of the > the expression. > > Reported-by: Jonathan Hettwer <j2468h@gmail.com> > Signed-off-by: James Carter <jwcart2@gmail.com> > --- > libsepol/cil/src/cil_binary.c | 50 ++++++++++++++++++++++++++++++++ > libsepol/cil/src/cil_build_ast.c | 20 ++++--------- > 2 files changed, 55 insertions(+), 15 deletions(-) > > diff --git a/libsepol/cil/src/cil_binary.c b/libsepol/cil/src/cil_binary.c > index 77266858..0604db3c 100644 > --- a/libsepol/cil/src/cil_binary.c > +++ b/libsepol/cil/src/cil_binary.c > @@ -2713,6 +2713,51 @@ int __cil_constrain_expr_to_sepol_expr(policydb_t *pdb, const struct cil_db *db, > return SEPOL_OK; > } > > +int __cil_validate_constrain_expr(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) { > + cil_log(CIL_ERR,"Invalid constraint expression\n"); > + return SEPOL_ERR; > + } > + break; > + case CEXPR_AND: > + case CEXPR_OR: > + if (depth < 1) { > + cil_log(CIL_ERR,"Invalid constraint expression\n"); > + return SEPOL_ERR; > + } > + depth--; > + break; > + case CEXPR_ATTR: > + if (depth == (CEXPR_MAXDEPTH - 1)) { > + cil_log(CIL_ERR,"Constraint expression exceeded max allowable depth\n"); > + return SEPOL_ERR; > + } > + depth++; > + break; > + case CEXPR_NAMES: You don't like sharing a single block for multiple cases? Up to you. As long as it gets annotated with the magic fallthrough indicator, modern compilers shouldn't care. > + if (depth == (CEXPR_MAXDEPTH - 1)) { > + cil_log(CIL_ERR,"Constraint expression exceeded max allowable depth\n"); > + return SEPOL_ERR; > + } > + depth++; > + break; > + default: > + cil_log(CIL_ERR,"Invalid constraint expression\n"); > + return SEPOL_ERR; > + } > + e = e->next; > + } Only difference I noticed from checkpolicy logic here is that you don't do a final check that depth was 0 at the end. Don't know if that is impossible here for other reasons.
On Wed, Sep 9, 2020 at 4:24 PM Stephen Smalley <stephen.smalley.work@gmail.com> wrote: > > On Wed, Sep 9, 2020 at 2:25 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. > > > > Validate the constraint expression using the same logic that is used > > when reading the binary policy. This includes checking the depth of the > > the expression. > > > > Reported-by: Jonathan Hettwer <j2468h@gmail.com> > > Signed-off-by: James Carter <jwcart2@gmail.com> > > --- > > libsepol/cil/src/cil_binary.c | 50 ++++++++++++++++++++++++++++++++ > > libsepol/cil/src/cil_build_ast.c | 20 ++++--------- > > 2 files changed, 55 insertions(+), 15 deletions(-) > > > > diff --git a/libsepol/cil/src/cil_binary.c b/libsepol/cil/src/cil_binary.c > > index 77266858..0604db3c 100644 > > --- a/libsepol/cil/src/cil_binary.c > > +++ b/libsepol/cil/src/cil_binary.c > > @@ -2713,6 +2713,51 @@ int __cil_constrain_expr_to_sepol_expr(policydb_t *pdb, const struct cil_db *db, > > return SEPOL_OK; > > } > > > > +int __cil_validate_constrain_expr(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) { > > + cil_log(CIL_ERR,"Invalid constraint expression\n"); > > + return SEPOL_ERR; > > + } > > + break; > > + case CEXPR_AND: > > + case CEXPR_OR: > > + if (depth < 1) { > > + cil_log(CIL_ERR,"Invalid constraint expression\n"); > > + return SEPOL_ERR; > > + } > > + depth--; > > + break; > > + case CEXPR_ATTR: > > + if (depth == (CEXPR_MAXDEPTH - 1)) { > > + cil_log(CIL_ERR,"Constraint expression exceeded max allowable depth\n"); > > + return SEPOL_ERR; > > + } > > + depth++; > > + break; > > + case CEXPR_NAMES: > > You don't like sharing a single block for multiple cases? Up to you. > As long as it gets annotated with the magic fallthrough indicator, > modern compilers shouldn't care. I noticed that after that I sent it out. Since I should add the final check that you mention below, I'll change this as well. > > > + if (depth == (CEXPR_MAXDEPTH - 1)) { > > + cil_log(CIL_ERR,"Constraint expression exceeded max allowable depth\n"); > > + return SEPOL_ERR; > > + } > > + depth++; > > + break; > > + default: > > + cil_log(CIL_ERR,"Invalid constraint expression\n"); > > + return SEPOL_ERR; > > + } > > + e = e->next; > > + } > > Only difference I noticed from checkpolicy logic here is that you > don't do a final check that depth was 0 at the end. Don't know if > that is impossible here for other reasons. No, I am just sloppy today. I had removed it, because I was just going to check the depth, but didn't add it back in when I decided to keep the other validation stuff. Jim
diff --git a/libsepol/cil/src/cil_binary.c b/libsepol/cil/src/cil_binary.c index 77266858..0604db3c 100644 --- a/libsepol/cil/src/cil_binary.c +++ b/libsepol/cil/src/cil_binary.c @@ -2713,6 +2713,51 @@ int __cil_constrain_expr_to_sepol_expr(policydb_t *pdb, const struct cil_db *db, return SEPOL_OK; } +int __cil_validate_constrain_expr(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) { + cil_log(CIL_ERR,"Invalid constraint expression\n"); + return SEPOL_ERR; + } + break; + case CEXPR_AND: + case CEXPR_OR: + if (depth < 1) { + cil_log(CIL_ERR,"Invalid constraint expression\n"); + return SEPOL_ERR; + } + depth--; + break; + case CEXPR_ATTR: + if (depth == (CEXPR_MAXDEPTH - 1)) { + cil_log(CIL_ERR,"Constraint expression exceeded max allowable depth\n"); + return SEPOL_ERR; + } + depth++; + break; + case CEXPR_NAMES: + if (depth == (CEXPR_MAXDEPTH - 1)) { + cil_log(CIL_ERR,"Constraint expression exceeded max allowable depth\n"); + return SEPOL_ERR; + } + depth++; + break; + default: + cil_log(CIL_ERR,"Invalid constraint expression\n"); + 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 +2781,11 @@ int cil_constrain_to_policydb_helper(policydb_t *pdb, const struct cil_db *db, s goto exit; } + rc = __cil_validate_constrain_expr(sepol_expr); + if (rc != SEPOL_OK) { + 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; }
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. Validate the constraint expression using the same logic that is used when reading the binary policy. This includes checking the depth of the the expression. Reported-by: Jonathan Hettwer <j2468h@gmail.com> Signed-off-by: James Carter <jwcart2@gmail.com> --- libsepol/cil/src/cil_binary.c | 50 ++++++++++++++++++++++++++++++++ libsepol/cil/src/cil_build_ast.c | 20 ++++--------- 2 files changed, 55 insertions(+), 15 deletions(-)