Ondrej Mosnacek <omosnace@xxxxxxxxxx> writes:
> On Wed, Jan 6, 2021 at 9:22 AM Nicolas Iooss <nicolas.iooss@xxxxxxx> 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@xxxxxxxxxx>
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@xxxxxxx>
>> ---
>> 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.
