Re: [PATCH] libsepol: destroy filename_trans list properly

[Petr Lautrbach <plautrba@redhat.com>]

Ondrej Mosnacek <omosnace@redhat.com> writes:

> On Wed, Jan 6, 2021 at 9:22 AM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
>> OSS-Fuzz found a direct memory leak in policydb_filetrans_insert()
>> because filenametr_destroy() does not fully destroy the list associated
>> with a typetransition.
>>
>> More precisely, let's consider this (minimized) CIL policy:
>>
>>     (class CLASS (PERM))
>>     (classorder (CLASS))
>>     (sid SID)
>>     (sidorder (SID))
>>     (user USER)
>>     (role ROLE)
>>     (type TYPE) ; "type 1" in libsepol internal structures
>>     (type TYPE2) ; "type 2" in libsepol internal structures
>>     (type TYPE3) ; "type 3" in libsepol internal structures
>>     (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))))
>>
>>     (typetransition TYPE2 TYPE CLASS "some_file" TYPE2)
>>     (typetransition TYPE3 TYPE CLASS "some_file" TYPE3)
>>
>> The two typetransition statements make policydb_filetrans_insert()
>> insert an item with key {ttype=1, tclass=1, name="some_file"} in the
>> hashmap p->filename_trans. This item contains a linked list of two
>> filename_trans_datum_t elements:
>>
>> * The first one uses {otype=2, stypes=bitmap containing 2}
>> * The second one uses {otype=3, stypes=bitmap containing 3}
>>
>> Nevertheless filenametr_destroy() (called by
>> hashtab_map(p->filename_trans, filenametr_destroy, NULL);) only frees
>> the first element. Fix this memory leak by freeing all elements.
>>
>> This issue was introduced by commit 42ae834a7428 ("libsepol,checkpolicy:
>> optimize storage of filename transitions") and was never present in the
>> kernel, as filenametr_destroy() was modified appropriately in commit
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=c3a276111ea2572399281988b3129683e2a6b60b
>
> Ouch, good catch!
>
> Acked-by: Ondrej Mosnacek <omosnace@redhat.com>

Merged, thanks!


>>
>> Fixes: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=29138
>
> I get "Permission denied" when opening this link. Any chance it could
> be made public?
>
>> Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
>> ---
>>  libsepol/src/policydb.c | 10 +++++++---
>>  1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/libsepol/src/policydb.c b/libsepol/src/policydb.c
>> index f43d5c67463e..71ada42ca609 100644
>> --- a/libsepol/src/policydb.c
>> +++ b/libsepol/src/policydb.c
>> @@ -1462,12 +1462,16 @@ static int filenametr_destroy(hashtab_key_t key, hashtab_datum_t datum,
>>                               void *p __attribute__ ((unused)))
>>  {
>>         filename_trans_key_t *ft = (filename_trans_key_t *)key;
>> -       filename_trans_datum_t *fd = datum;
>> +       filename_trans_datum_t *fd = datum, *next;
>>
>>         free(ft->name);
>>         free(key);
>> -       ebitmap_destroy(&fd->stypes);
>> -       free(datum);
>> +       do {
>> +               next = fd->next;
>> +               ebitmap_destroy(&fd->stypes);
>> +               free(fd);
>> +               fd = next;
>> +       } while (fd);
>>         return 0;
>>  }
>>
>> --
>> 2.30.0
>>
>
> -- 
> Ondrej Mosnacek
> Software Engineer, Platform Security - SELinux kernel
> Red Hat, Inc.