On Wed, Sep 9, 2020 at 2:25 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.
>
> 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@xxxxxxxxx>
> Signed-off-by: James Carter <jwcart2@xxxxxxxxx>
> ---
> 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.
