[PATCH] libsepol: destroy filename_trans list properly

[PATCH] libsepol: destroy filename_trans list properly

[Nicolas Iooss]

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

Fixes: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=29138
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

Re: [PATCH] libsepol: destroy filename_trans list properly

[Ondrej Mosnacek]

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>

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

Re: [PATCH] libsepol: destroy filename_trans list properly

[Nicolas Iooss]

On Wed, Jan 6, 2021 at 9:30 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> 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>
>
> >
> > 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@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.
>

Re: [PATCH] libsepol: destroy filename_trans list properly

[Petr Lautrbach]

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.