Re: [PATCH] libsepol/cil: fix memory leak when a constraint expression is too deep

[Petr Lautrbach <plautrba@redhat.com>]

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
>>