On Wed, Sep 9, 2020 at 4:24 PM Stephen Smalley
<stephen.smalley.work@xxxxxxxxx> wrote:
>
> 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.
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
