| Message ID | 20210106081922.450743-1-nicolas.iooss@m4x.org (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | libsepol: destroy filename_trans list properly | expand |
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 >
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. >
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.
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; }
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(-)