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