Re: [PATCH 4/6] libsepol/cil: fix NULL pointer dereference when parsing an improper integer

[William Roberts <bill.c.roberts@gmail.com>]

On Wed, Dec 30, 2020 at 4:09 AM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
>
> OSS-Fuzz found a NULL pointer dereference when the CIL compiler tries to
> compile a policy with an invalid integer:
>
>     $ echo '(ioportcon(2())n)' > tmp.cil
>     $ secilc tmp.cil
>     Segmentation fault (core dumped)
>
> This is because strtol() is called with a NULL pointer, in
> cil_fill_integer().
>
> Fix this by checking that int_node->data is not NULL. While at it, use
> strtoul() instead of strtol() to parse an unsigned integer.
>
> Fixes: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=28456
> Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
> ---
>  libsepol/cil/src/cil_build_ast.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/libsepol/cil/src/cil_build_ast.c b/libsepol/cil/src/cil_build_ast.c
> index 67801def0dc0..0c9015cef578 100644
> --- a/libsepol/cil/src/cil_build_ast.c
> +++ b/libsepol/cil/src/cil_build_ast.c
> @@ -5566,15 +5566,15 @@ int cil_fill_integer(struct cil_tree_node *int_node, uint32_t *integer, int base
>  {
>         int rc = SEPOL_ERR;
>         char *endptr = NULL;
> -       int val;
> +       unsigned long val;
>
> -       if (int_node == NULL || integer == NULL) {
> +       if (int_node == NULL || int_node->data == NULL || integer == NULL) {
>                 goto exit;
>         }
>
>         errno = 0;
> -       val = strtol(int_node->data, &endptr, base);
> -       if (errno != 0 || endptr == int_node->data || *endptr != '\0') {
> +       val = strtoul(int_node->data, &endptr, base);
> +       if (errno != 0 || endptr == int_node->data || *endptr != '\0' || val > UINT32_MAX) {

I wonder if compilers/static analysis tools will balk on this as
strtoul's return, an unsigned long,
on a 32 bit machine will be 32 bits, so this could have a dead
expression as val > UINT32_MAX
will always be false. Perhaps use the strtoull variant to always have 64 bits?

>                 rc = SEPOL_ERR;
>                 goto exit;
>         }
> @@ -5594,7 +5594,7 @@ int cil_fill_integer64(struct cil_tree_node *int_node, uint64_t *integer, int ba
>         char *endptr = NULL;
>         uint64_t val;
>
> -       if (int_node == NULL || integer == NULL) {
> +       if (int_node == NULL || int_node->data == NULL || integer == NULL) {
>                 goto exit;
>         }
>
> --
> 2.29.2
>