On Wed, Jan 6, 2021 at 9:30 AM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote:
>
> 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?
I do not know how: the issue states "Only users with Commit permission
or issue reporter may view.", and there is no owner associated to it
so I do not know how to change its visibility. This issue also has
"Labels: Disclosure-2021-04-01" so it will become public in 3 months
anyway...
By the way, as a project maintainer, you can request access by asking
to be added to the list of people in
https://github.com/google/oss-fuzz/blob/master/projects/selinux/project.yaml.
Nicolas
> > 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.
>
