mbox series

[00/12] Update checks for invalid rules in blocks

Message ID 20210330173920.281531-1-jwcart2@gmail.com (mailing list archive)
Headers show
Series Update checks for invalid rules in blocks | expand

Message

James Carter March 30, 2021, 5:39 p.m. UTC
Looking into a bug that OSS-Fuzz found led to patch 7, Check for
statements not allowed in optional blocks, which is the most important
patch in this series. Working on patch 7 led to fixing some other
problems with the checks for invalid rules, cleaning up some of the code,
and improving the CIL documentation.

Patches 1, 2, 4, 5, and 10 are doing various cleanups.
Patch 3 fixes a bug that prevents the first rule in a block from being checked.
Patches 6, 7, 8, and 9 update the checks for invalid rules.
Patch 11 fixes a bug that prevented some error messages from being displayed.
Patch 12 updates the CIL documentation.

There is still work to do in this area. I am not sure why sensitivity and
category statements are not allowed in blocks, but everything else is. That
is why I didn't add those checks when building the AST. It is not clear if
mls, handleunknown, defaultuser, defaultrole, defaulttype, defaultrange,
and policycap should be restricted to the global namespace.

James Carter (12):
  libsepol/cil: Reorder checks for invalid rules when building AST
  libsepol/cil: Cleanup build AST helper functions
  libsepol/cil: Create new first child helper function for building AST
  libsepol/cil: Use AST to track blocks and optionals when resolving
  libsepol/cil: Reorder checks for invalid rules when resolving AST
  libsepol/cil: Sync checks for invalid rules in booleanifs
  libsepol/cil: Check for statements not allowed in optional blocks
  libsepol/cil: Sync checks for invalid rules in macros
  libsepol/cil: Do not allow tunable declarations in in-statements
  libsepol/cil: Make invalid statement error messages consistent
  libsepol/cil: Use CIL_ERR for error messages in cil_compile()
  secilc/docs: Update the CIL documentation for various blocks

 libsepol/cil/src/cil.c                    |   8 +-
 libsepol/cil/src/cil_build_ast.c          | 193 ++++++++++++----------
 libsepol/cil/src/cil_resolve_ast.c        | 174 ++++++++-----------
 secilc/docs/cil_call_macro_statements.md  |   2 +
 secilc/docs/cil_conditional_statements.md |   6 +
 secilc/docs/cil_container_statements.md   |  28 ++--
 6 files changed, 205 insertions(+), 206 deletions(-)

Comments

James Carter April 15, 2021, 8:43 p.m. UTC | #1
On Tue, Mar 30, 2021 at 1:39 PM James Carter <jwcart2@gmail.com> wrote:
>
> Looking into a bug that OSS-Fuzz found led to patch 7, Check for
> statements not allowed in optional blocks, which is the most important
> patch in this series. Working on patch 7 led to fixing some other
> problems with the checks for invalid rules, cleaning up some of the code,
> and improving the CIL documentation.
>
> Patches 1, 2, 4, 5, and 10 are doing various cleanups.
> Patch 3 fixes a bug that prevents the first rule in a block from being checked.
> Patches 6, 7, 8, and 9 update the checks for invalid rules.
> Patch 11 fixes a bug that prevented some error messages from being displayed.
> Patch 12 updates the CIL documentation.
>
> There is still work to do in this area. I am not sure why sensitivity and
> category statements are not allowed in blocks, but everything else is. That
> is why I didn't add those checks when building the AST. It is not clear if
> mls, handleunknown, defaultuser, defaultrole, defaulttype, defaultrange,
> and policycap should be restricted to the global namespace.
>
> James Carter (12):
>   libsepol/cil: Reorder checks for invalid rules when building AST
>   libsepol/cil: Cleanup build AST helper functions
>   libsepol/cil: Create new first child helper function for building AST
>   libsepol/cil: Use AST to track blocks and optionals when resolving
>   libsepol/cil: Reorder checks for invalid rules when resolving AST
>   libsepol/cil: Sync checks for invalid rules in booleanifs
>   libsepol/cil: Check for statements not allowed in optional blocks
>   libsepol/cil: Sync checks for invalid rules in macros
>   libsepol/cil: Do not allow tunable declarations in in-statements
>   libsepol/cil: Make invalid statement error messages consistent
>   libsepol/cil: Use CIL_ERR for error messages in cil_compile()
>   secilc/docs: Update the CIL documentation for various blocks
>
>  libsepol/cil/src/cil.c                    |   8 +-
>  libsepol/cil/src/cil_build_ast.c          | 193 ++++++++++++----------
>  libsepol/cil/src/cil_resolve_ast.c        | 174 ++++++++-----------
>  secilc/docs/cil_call_macro_statements.md  |   2 +
>  secilc/docs/cil_conditional_statements.md |   6 +
>  secilc/docs/cil_container_statements.md   |  28 ++--
>  6 files changed, 205 insertions(+), 206 deletions(-)
>
> --
> 2.26.3
>

There hasn't been any comments on this patch set. I am planning on
merging it next week.

Jim
James Carter April 19, 2021, 6:26 p.m. UTC | #2
On Tue, Mar 30, 2021 at 1:39 PM James Carter <jwcart2@gmail.com> wrote:
>
> Looking into a bug that OSS-Fuzz found led to patch 7, Check for
> statements not allowed in optional blocks, which is the most important
> patch in this series. Working on patch 7 led to fixing some other
> problems with the checks for invalid rules, cleaning up some of the code,
> and improving the CIL documentation.
>
> Patches 1, 2, 4, 5, and 10 are doing various cleanups.
> Patch 3 fixes a bug that prevents the first rule in a block from being checked.
> Patches 6, 7, 8, and 9 update the checks for invalid rules.
> Patch 11 fixes a bug that prevented some error messages from being displayed.
> Patch 12 updates the CIL documentation.
>
> There is still work to do in this area. I am not sure why sensitivity and
> category statements are not allowed in blocks, but everything else is. That
> is why I didn't add those checks when building the AST. It is not clear if
> mls, handleunknown, defaultuser, defaultrole, defaulttype, defaultrange,
> and policycap should be restricted to the global namespace.
>
> James Carter (12):
>   libsepol/cil: Reorder checks for invalid rules when building AST
>   libsepol/cil: Cleanup build AST helper functions
>   libsepol/cil: Create new first child helper function for building AST
>   libsepol/cil: Use AST to track blocks and optionals when resolving
>   libsepol/cil: Reorder checks for invalid rules when resolving AST
>   libsepol/cil: Sync checks for invalid rules in booleanifs
>   libsepol/cil: Check for statements not allowed in optional blocks
>   libsepol/cil: Sync checks for invalid rules in macros
>   libsepol/cil: Do not allow tunable declarations in in-statements
>   libsepol/cil: Make invalid statement error messages consistent
>   libsepol/cil: Use CIL_ERR for error messages in cil_compile()
>   secilc/docs: Update the CIL documentation for various blocks
>
>  libsepol/cil/src/cil.c                    |   8 +-
>  libsepol/cil/src/cil_build_ast.c          | 193 ++++++++++++----------
>  libsepol/cil/src/cil_resolve_ast.c        | 174 ++++++++-----------
>  secilc/docs/cil_call_macro_statements.md  |   2 +
>  secilc/docs/cil_conditional_statements.md |   6 +
>  secilc/docs/cil_container_statements.md   |  28 ++--
>  6 files changed, 205 insertions(+), 206 deletions(-)
>
> --
> 2.26.3
>

This has been applied (with the whitespace error in the last patch fixed).
Jim
Nicolas Iooss April 20, 2021, 7:53 a.m. UTC | #3
On Mon, Apr 19, 2021 at 8:26 PM James Carter <jwcart2@gmail.com> wrote:
>
> On Tue, Mar 30, 2021 at 1:39 PM James Carter <jwcart2@gmail.com> wrote:
> >
> > Looking into a bug that OSS-Fuzz found led to patch 7, Check for
> > statements not allowed in optional blocks, which is the most important
> > patch in this series. Working on patch 7 led to fixing some other
> > problems with the checks for invalid rules, cleaning up some of the code,
> > and improving the CIL documentation.
> >
> > Patches 1, 2, 4, 5, and 10 are doing various cleanups.
> > Patch 3 fixes a bug that prevents the first rule in a block from being checked.
> > Patches 6, 7, 8, and 9 update the checks for invalid rules.
> > Patch 11 fixes a bug that prevented some error messages from being displayed.
> > Patch 12 updates the CIL documentation.
> >
> > There is still work to do in this area. I am not sure why sensitivity and
> > category statements are not allowed in blocks, but everything else is. That
> > is why I didn't add those checks when building the AST. It is not clear if
> > mls, handleunknown, defaultuser, defaultrole, defaulttype, defaultrange,
> > and policycap should be restricted to the global namespace.
> >
> > James Carter (12):
> >   libsepol/cil: Reorder checks for invalid rules when building AST
> >   libsepol/cil: Cleanup build AST helper functions
> >   libsepol/cil: Create new first child helper function for building AST
> >   libsepol/cil: Use AST to track blocks and optionals when resolving
> >   libsepol/cil: Reorder checks for invalid rules when resolving AST
> >   libsepol/cil: Sync checks for invalid rules in booleanifs
> >   libsepol/cil: Check for statements not allowed in optional blocks
> >   libsepol/cil: Sync checks for invalid rules in macros
> >   libsepol/cil: Do not allow tunable declarations in in-statements
> >   libsepol/cil: Make invalid statement error messages consistent
> >   libsepol/cil: Use CIL_ERR for error messages in cil_compile()
> >   secilc/docs: Update the CIL documentation for various blocks
> >
> >  libsepol/cil/src/cil.c                    |   8 +-
> >  libsepol/cil/src/cil_build_ast.c          | 193 ++++++++++++----------
> >  libsepol/cil/src/cil_resolve_ast.c        | 174 ++++++++-----------
> >  secilc/docs/cil_call_macro_statements.md  |   2 +
> >  secilc/docs/cil_conditional_statements.md |   6 +
> >  secilc/docs/cil_container_statements.md   |  28 ++--
> >  6 files changed, 205 insertions(+), 206 deletions(-)
> >
> > --
> > 2.26.3
> >
>
> This has been applied (with the whitespace error in the last patch fixed).
> Jim

Hi,
Thanks for these patches! I was on holiday in the past few weeks and
just got back. The applied patches look good to me. Moreover when I
run secilc on all the OSSFuzz reproducers I have studied so far, there
is no longer any crash :)

Nicolas