| Message ID | 20210131231255.58909-1-nicolas.iooss@m4x.org (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | libsepol/cil: fix memory leak when a constraint expression is too deep | expand |
On Sun, Jan 31, 2021 at 6:14 PM Nicolas Iooss <nicolas.iooss@m4x.org> 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@m4x.org> Looks good. Acked-by: James Carter <jwcart2@gmail.com> > --- > 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 >
James Carter <jwcart2@gmail.com> writes: > On Sun, Jan 31, 2021 at 6:14 PM Nicolas Iooss <nicolas.iooss@m4x.org> 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@m4x.org> > > Looks good. > > Acked-by: James Carter <jwcart2@gmail.com> Merged. Thanks! > >> --- >> 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 >>
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);
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@m4x.org> --- 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(-)