* [PATCH] libsepol: destroy filename_trans list properly
@ 2021-01-06 8:19 Nicolas Iooss
2021-01-06 8:30 ` Ondrej Mosnacek
0 siblings, 1 reply; 4+ messages in thread
From: Nicolas Iooss @ 2021-01-06 8:19 UTC (permalink / raw)
To: selinux
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] libsepol: destroy filename_trans list properly
2021-01-06 8:19 [PATCH] libsepol: destroy filename_trans list properly Nicolas Iooss
@ 2021-01-06 8:30 ` Ondrej Mosnacek
2021-01-06 8:58 ` Nicolas Iooss
2021-01-13 22:30 ` Petr Lautrbach
0 siblings, 2 replies; 4+ messages in thread
From: Ondrej Mosnacek @ 2021-01-06 8:30 UTC (permalink / raw)
To: Nicolas Iooss; +Cc: SElinux list
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.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] libsepol: destroy filename_trans list properly
2021-01-06 8:30 ` Ondrej Mosnacek
@ 2021-01-06 8:58 ` Nicolas Iooss
2021-01-13 22:30 ` Petr Lautrbach
1 sibling, 0 replies; 4+ messages in thread
From: Nicolas Iooss @ 2021-01-06 8:58 UTC (permalink / raw)
To: Ondrej Mosnacek; +Cc: SElinux list
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.
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] libsepol: destroy filename_trans list properly
2021-01-06 8:30 ` Ondrej Mosnacek
2021-01-06 8:58 ` Nicolas Iooss
@ 2021-01-13 22:30 ` Petr Lautrbach
1 sibling, 0 replies; 4+ messages in thread
From: Petr Lautrbach @ 2021-01-13 22:30 UTC (permalink / raw)
To: SElinux list; +Cc: Ondrej Mosnacek, Nicolas Iooss
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.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-01-14 2:04 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-06 8:19 [PATCH] libsepol: destroy filename_trans list properly Nicolas Iooss
2021-01-06 8:30 ` Ondrej Mosnacek
2021-01-06 8:58 ` Nicolas Iooss
2021-01-13 22:30 ` Petr Lautrbach
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.