On Sun, Jan 31, 2021 at 6:14 PM Nicolas Iooss <nicolas.iooss@xxxxxxx> wrote: > > When __cil_validate_constrain_expr() fails, > cil_constrain_to_policydb_helper() does not destroy the constraint > expression. This leads to a memory leak reported by OSS-Fuzz with the > following CIL policy: > > (class CLASS (PERM)) > (classorder (CLASS)) > (sid SID) > (sidorder (SID)) > (user USER) > (role ROLE) > (type TYPE) > (category CAT) > (categoryorder (CAT)) > (sensitivity SENS) > (sensitivityorder (SENS)) > (sensitivitycategory SENS (CAT)) > (allow TYPE self (CLASS (PERM))) > (roletype ROLE TYPE) > (userrole USER ROLE) > (userlevel USER (SENS)) > (userrange USER ((SENS)(SENS (CAT)))) > (sidcontext SID (USER ROLE TYPE ((SENS)(SENS)))) > > (constrain > (CLASS (PERM)) > (or > (eq t1 TYPE) > (or > (eq t1 TYPE) > (or > (eq t1 TYPE) > (or > (eq t1 TYPE) > (or > (eq t1 TYPE) > (eq t1 TYPE) > ) > ) > ) > ) > ) > ) > > Add constraint_expr_destroy(sepol_expr) to destroy the expression. > > Moreover constraint_expr_destroy() was not freeing all items of an > expression. Code in libsepol/src and checkpolicy contained while loop to > free all the items of a constraint expression, but not the one in > libsepol/cil. As freeing only the first item of an expression is > misleading, change the semantic of constraint_expr_destroy() to iterate > the list of constraint_expr_t and to free all items. > > Fixes: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=28938 > Signed-off-by: Nicolas Iooss <nicolas.iooss@xxxxxxx> Looks good. Acked-by: James Carter <jwcart2@xxxxxxxxx> > --- > checkpolicy/policy_define.c | 7 +------ > libsepol/cil/src/cil_binary.c | 1 + > libsepol/src/constraint.c | 6 +++++- > libsepol/src/policydb.c | 15 ++------------- > 4 files changed, 9 insertions(+), 20 deletions(-) > > diff --git a/checkpolicy/policy_define.c b/checkpolicy/policy_define.c > index bf6c3e68bef3..c9286f7733c5 100644 > --- a/checkpolicy/policy_define.c > +++ b/checkpolicy/policy_define.c > @@ -3479,12 +3479,7 @@ static constraint_expr_t *constraint_expr_clone(constraint_expr_t * expr) > > return h; > oom: > - e = h; > - while (e) { > - l = e; > - e = e->next; > - constraint_expr_destroy(l); > - } > + constraint_expr_destroy(h); > return NULL; > } > > diff --git a/libsepol/cil/src/cil_binary.c b/libsepol/cil/src/cil_binary.c > index 3b01ade146c5..7ba2098b61ea 100644 > --- a/libsepol/cil/src/cil_binary.c > +++ b/libsepol/cil/src/cil_binary.c > @@ -2841,6 +2841,7 @@ int cil_constrain_to_policydb_helper(policydb_t *pdb, const struct cil_db *db, s > return SEPOL_OK; > > exit: > + constraint_expr_destroy(sepol_expr); > free(sepol_constrain); > return rc; > } > diff --git a/libsepol/src/constraint.c b/libsepol/src/constraint.c > index 71540195d633..58eb6da7940f 100644 > --- a/libsepol/src/constraint.c > +++ b/libsepol/src/constraint.c > @@ -38,10 +38,14 @@ int constraint_expr_init(constraint_expr_t * expr) > > void constraint_expr_destroy(constraint_expr_t * expr) > { > - if (expr != NULL) { > + constraint_expr_t *next; > + > + while (expr != NULL) { > + next = expr->next; > ebitmap_destroy(&expr->names); > type_set_destroy(expr->type_names); > free(expr->type_names); > free(expr); > + expr = next; > } > } > diff --git a/libsepol/src/policydb.c b/libsepol/src/policydb.c > index 71ada42ca609..f45d28c764ba 100644 > --- a/libsepol/src/policydb.c > +++ b/libsepol/src/policydb.c > @@ -1359,7 +1359,6 @@ static int class_destroy(hashtab_key_t key, hashtab_datum_t datum, void *p > { > class_datum_t *cladatum; > constraint_node_t *constraint, *ctemp; > - constraint_expr_t *e, *etmp; > > if (key) > free(key); > @@ -1371,12 +1370,7 @@ static int class_destroy(hashtab_key_t key, hashtab_datum_t datum, void *p > hashtab_destroy(cladatum->permissions.table); > constraint = cladatum->constraints; > while (constraint) { > - e = constraint->expr; > - while (e) { > - etmp = e; > - e = e->next; > - constraint_expr_destroy(etmp); > - } > + constraint_expr_destroy(constraint->expr); > ctemp = constraint; > constraint = constraint->next; > free(ctemp); > @@ -1384,12 +1378,7 @@ static int class_destroy(hashtab_key_t key, hashtab_datum_t datum, void *p > > constraint = cladatum->validatetrans; > while (constraint) { > - e = constraint->expr; > - while (e) { > - etmp = e; > - e = e->next; > - constraint_expr_destroy(etmp); > - } > + constraint_expr_destroy(constraint->expr); > ctemp = constraint; > constraint = constraint->next; > free(ctemp); > -- > 2.30.0 >
