diff mbox series

libsepol/cil: Destroy classperms list when resetting classpermission

Message ID 20210317190002.81465-1-jwcart2@gmail.com (mailing list archive)
State Rejected
Headers show
Series libsepol/cil: Destroy classperms list when resetting classpermission | expand

Commit Message

James Carter March 17, 2021, 7 p.m. UTC 
Nicolas Iooss reports:
  A few months ago, OSS-Fuzz found a crash in the CIL compiler, which
  got reported as
  https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=28648 (the title
  is misleading, or is caused by another issue that conflicts with the
  one I report in this message). Here is a minimized CIL policy which
  reproduces the issue:

  (class CLASS (PERM))
  (classorder (CLASS))
  (sid SID)
  (sidorder (SID))
  (user USER)
  (role ROLE)
  (type TYPE)
  (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))))

  (classpermission CLAPERM)

  (optional OPT
      (roletype nonexistingrole nonexistingtype)
      (classpermissionset CLAPERM (CLASS (PERM)))
  )

  The CIL policy fuzzer (which mimics secilc built with clang Address
  Sanitizer) reports:

  ==36541==ERROR: AddressSanitizer: heap-use-after-free on address
  0x603000004f98 at pc 0x56445134c842 bp 0x7ffe2a256590 sp
  0x7ffe2a256588
  READ of size 8 at 0x603000004f98 thread T0
      #0 0x56445134c841 in __cil_verify_classperms
  /selinux/libsepol/src/../cil/src/cil_verify.c:1620:8
      #1 0x56445134a43e in __cil_verify_classpermission
  /selinux/libsepol/src/../cil/src/cil_verify.c:1650:9
      #2 0x56445134a43e in __cil_pre_verify_helper
  /selinux/libsepol/src/../cil/src/cil_verify.c:1715:8
      #3 0x5644513225ac in cil_tree_walk_core
  /selinux/libsepol/src/../cil/src/cil_tree.c:272:9
      #4 0x564451322ab1 in cil_tree_walk
  /selinux/libsepol/src/../cil/src/cil_tree.c:316:7
      #5 0x5644513226af in cil_tree_walk_core
  /selinux/libsepol/src/../cil/src/cil_tree.c:284:9
      #6 0x564451322ab1 in cil_tree_walk
  /selinux/libsepol/src/../cil/src/cil_tree.c:316:7
      #7 0x5644512b88fd in cil_pre_verify
  /selinux/libsepol/src/../cil/src/cil_post.c:2510:7
      #8 0x5644512b88fd in cil_post_process
  /selinux/libsepol/src/../cil/src/cil_post.c:2524:7
      #9 0x5644511856ff in cil_compile
  /selinux/libsepol/src/../cil/src/cil.c:564:7

The classperms list of a classpermission rule is created and filled
in when classpermissionset rules are processed, so it doesn't own any
part of the list and shouldn't retain any of it when it is reset.

Destroy the classperms list (without destroying the data in it)  when
resetting a classpermission rule.

Reported-by: Nicolas Iooss <nicolas.iooss@m4x.org>
Signed-off-by: James Carter <jwcart2@gmail.com>
---
 libsepol/cil/src/cil_reset_ast.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Nicolas Iooss March 18, 2021, 10:03 p.m. UTC | #1 
On Wed, Mar 17, 2021 at 8:00 PM James Carter <jwcart2@gmail.com> wrote:
>
> Nicolas Iooss reports:
>   A few months ago, OSS-Fuzz found a crash in the CIL compiler, which
>   got reported as
>   https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=28648 (the title
>   is misleading, or is caused by another issue that conflicts with the
>   one I report in this message). Here is a minimized CIL policy which
>   reproduces the issue:
>
>   (class CLASS (PERM))
>   (classorder (CLASS))
>   (sid SID)
>   (sidorder (SID))
>   (user USER)
>   (role ROLE)
>   (type TYPE)
>   (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))))
>
>   (classpermission CLAPERM)
>
>   (optional OPT
>       (roletype nonexistingrole nonexistingtype)
>       (classpermissionset CLAPERM (CLASS (PERM)))
>   )
>
>   The CIL policy fuzzer (which mimics secilc built with clang Address
>   Sanitizer) reports:
>
>   ==36541==ERROR: AddressSanitizer: heap-use-after-free on address
>   0x603000004f98 at pc 0x56445134c842 bp 0x7ffe2a256590 sp
>   0x7ffe2a256588
>   READ of size 8 at 0x603000004f98 thread T0
>       #0 0x56445134c841 in __cil_verify_classperms
>   /selinux/libsepol/src/../cil/src/cil_verify.c:1620:8
>       #1 0x56445134a43e in __cil_verify_classpermission
>   /selinux/libsepol/src/../cil/src/cil_verify.c:1650:9
>       #2 0x56445134a43e in __cil_pre_verify_helper
>   /selinux/libsepol/src/../cil/src/cil_verify.c:1715:8
>       #3 0x5644513225ac in cil_tree_walk_core
>   /selinux/libsepol/src/../cil/src/cil_tree.c:272:9
>       #4 0x564451322ab1 in cil_tree_walk
>   /selinux/libsepol/src/../cil/src/cil_tree.c:316:7
>       #5 0x5644513226af in cil_tree_walk_core
>   /selinux/libsepol/src/../cil/src/cil_tree.c:284:9
>       #6 0x564451322ab1 in cil_tree_walk
>   /selinux/libsepol/src/../cil/src/cil_tree.c:316:7
>       #7 0x5644512b88fd in cil_pre_verify
>   /selinux/libsepol/src/../cil/src/cil_post.c:2510:7
>       #8 0x5644512b88fd in cil_post_process
>   /selinux/libsepol/src/../cil/src/cil_post.c:2524:7
>       #9 0x5644511856ff in cil_compile
>   /selinux/libsepol/src/../cil/src/cil.c:564:7
>
> The classperms list of a classpermission rule is created and filled
> in when classpermissionset rules are processed, so it doesn't own any
> part of the list and shouldn't retain any of it when it is reset.
>
> Destroy the classperms list (without destroying the data in it)  when
> resetting a classpermission rule.
>
> Reported-by: Nicolas Iooss <nicolas.iooss@m4x.org>
> Signed-off-by: James Carter <jwcart2@gmail.com>
> ---
>  libsepol/cil/src/cil_reset_ast.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/libsepol/cil/src/cil_reset_ast.c b/libsepol/cil/src/cil_reset_ast.c
> index 3da1b9a6..db70a535 100644
> --- a/libsepol/cil/src/cil_reset_ast.c
> +++ b/libsepol/cil/src/cil_reset_ast.c
> @@ -54,7 +54,7 @@ static void cil_reset_classpermission(struct cil_classpermission *cp)
>                 return;
>         }
>
> -       cil_reset_classperms_list(cp->classperms);
> +       cil_list_destroy(&cp->classperms, CIL_FALSE);
>  }
>
>  static void cil_reset_classperms_set(struct cil_classperms_set *cp_set)

Hello,
This patch seems to make secilc segfault on a test policy, for example
on GitHub Actions:

make[1]: Entering directory '/home/runner/work/selinux/selinux/secilc'
./secilc test/policy.cil
make[1]: *** [Makefile:32: test] Segmentation fault (core dumped)

(from https://github.com/fishilico/selinux/runs/2135040809?check_suite_focus=true#step:9:2645).
It also produces a segmentation fault on my Arch Linux development
system (building with gcc or clang and the default compilation flags
of the project).

Is this segfault fixed by the other patches you sent?

Thanks,
Nicolas
James Carter March 19, 2021, 1:37 p.m. UTC | #2 
On Thu, Mar 18, 2021 at 6:03 PM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
>
> On Wed, Mar 17, 2021 at 8:00 PM James Carter <jwcart2@gmail.com> wrote:
> >
> > Nicolas Iooss reports:
> >   A few months ago, OSS-Fuzz found a crash in the CIL compiler, which
> >   got reported as
> >   https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=28648 (the title
> >   is misleading, or is caused by another issue that conflicts with the
> >   one I report in this message). Here is a minimized CIL policy which
> >   reproduces the issue:
> >
> >   (class CLASS (PERM))
> >   (classorder (CLASS))
> >   (sid SID)
> >   (sidorder (SID))
> >   (user USER)
> >   (role ROLE)
> >   (type TYPE)
> >   (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))))
> >
> >   (classpermission CLAPERM)
> >
> >   (optional OPT
> >       (roletype nonexistingrole nonexistingtype)
> >       (classpermissionset CLAPERM (CLASS (PERM)))
> >   )
> >
> >   The CIL policy fuzzer (which mimics secilc built with clang Address
> >   Sanitizer) reports:
> >
> >   ==36541==ERROR: AddressSanitizer: heap-use-after-free on address
> >   0x603000004f98 at pc 0x56445134c842 bp 0x7ffe2a256590 sp
> >   0x7ffe2a256588
> >   READ of size 8 at 0x603000004f98 thread T0
> >       #0 0x56445134c841 in __cil_verify_classperms
> >   /selinux/libsepol/src/../cil/src/cil_verify.c:1620:8
> >       #1 0x56445134a43e in __cil_verify_classpermission
> >   /selinux/libsepol/src/../cil/src/cil_verify.c:1650:9
> >       #2 0x56445134a43e in __cil_pre_verify_helper
> >   /selinux/libsepol/src/../cil/src/cil_verify.c:1715:8
> >       #3 0x5644513225ac in cil_tree_walk_core
> >   /selinux/libsepol/src/../cil/src/cil_tree.c:272:9
> >       #4 0x564451322ab1 in cil_tree_walk
> >   /selinux/libsepol/src/../cil/src/cil_tree.c:316:7
> >       #5 0x5644513226af in cil_tree_walk_core
> >   /selinux/libsepol/src/../cil/src/cil_tree.c:284:9
> >       #6 0x564451322ab1 in cil_tree_walk
> >   /selinux/libsepol/src/../cil/src/cil_tree.c:316:7
> >       #7 0x5644512b88fd in cil_pre_verify
> >   /selinux/libsepol/src/../cil/src/cil_post.c:2510:7
> >       #8 0x5644512b88fd in cil_post_process
> >   /selinux/libsepol/src/../cil/src/cil_post.c:2524:7
> >       #9 0x5644511856ff in cil_compile
> >   /selinux/libsepol/src/../cil/src/cil.c:564:7
> >
> > The classperms list of a classpermission rule is created and filled
> > in when classpermissionset rules are processed, so it doesn't own any
> > part of the list and shouldn't retain any of it when it is reset.
> >
> > Destroy the classperms list (without destroying the data in it)  when
> > resetting a classpermission rule.
> >
> > Reported-by: Nicolas Iooss <nicolas.iooss@m4x.org>
> > Signed-off-by: James Carter <jwcart2@gmail.com>
> > ---
> >  libsepol/cil/src/cil_reset_ast.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/libsepol/cil/src/cil_reset_ast.c b/libsepol/cil/src/cil_reset_ast.c
> > index 3da1b9a6..db70a535 100644
> > --- a/libsepol/cil/src/cil_reset_ast.c
> > +++ b/libsepol/cil/src/cil_reset_ast.c
> > @@ -54,7 +54,7 @@ static void cil_reset_classpermission(struct cil_classpermission *cp)
> >                 return;
> >         }
> >
> > -       cil_reset_classperms_list(cp->classperms);
> > +       cil_list_destroy(&cp->classperms, CIL_FALSE);
> >  }
> >
> >  static void cil_reset_classperms_set(struct cil_classperms_set *cp_set)
>
> Hello,
> This patch seems to make secilc segfault on a test policy, for example
> on GitHub Actions:
>
> make[1]: Entering directory '/home/runner/work/selinux/selinux/secilc'
> ./secilc test/policy.cil
> make[1]: *** [Makefile:32: test] Segmentation fault (core dumped)
>
> (from https://github.com/fishilico/selinux/runs/2135040809?check_suite_focus=true#step:9:2645).
> It also produces a segmentation fault on my Arch Linux development
> system (building with gcc or clang and the default compilation flags
> of the project).
>
> Is this segfault fixed by the other patches you sent?
>

I believe this one does:
libsepol/cil: cil_reset_classperms_set() should not reset classpermission

I hadn't realized the connection when I sent the patches.

Thanks,
Jim

> Thanks,
> Nicolas
>
diff mbox series

Patch

diff --git a/libsepol/cil/src/cil_reset_ast.c b/libsepol/cil/src/cil_reset_ast.c
index 3da1b9a6..db70a535 100644
--- a/libsepol/cil/src/cil_reset_ast.c
+++ b/libsepol/cil/src/cil_reset_ast.c
@@ -54,7 +54,7 @@  static void cil_reset_classpermission(struct cil_classpermission *cp)
 		return;
 	}
 
-	cil_reset_classperms_list(cp->classperms);
+	cil_list_destroy(&cp->classperms, CIL_FALSE);
 }
 
 static void cil_reset_classperms_set(struct cil_classperms_set *cp_set)