libsepol: signed integer overflow in the HLL line counter of CIL compiler

libsepol: signed integer overflow in the HLL line counter of CIL compiler

[Nicolas Iooss]

Hello,

OSS-Fuzz found an integer overflow when compiling the following
(empty) CIL policy:

;;*lms 2147483647 a
; (empty line)

";;*lms" is a line mark which can be produced by the HLL compiler (if
I understand correctly the meaning of CIL_KEY_HLL_LMS in
libsepol/cil/src/cil_parser.c). The line number is parsed as an "int"
variable:

  *hll_lineno = strtol(tok.value, &end, 10);
  if (errno == ERANGE || *end != '\0') {
    cil_log(CIL_ERR, "Problem parsing line number for line mark\n");
    goto exit;
  }

This code has another issue which is that it silently truncates values
to 32-bit signed integers on systems where sizeof(long) is 8, because
hll_lineno is of type "int *", not "long *".

But the issue found by OSS-Fuzz is that when 2147483647 is used (which
is INT_MAX, 0x7fffffff in hexadecimal), "hll_lineno++;" overflows the
capacity of signed integers, in cil_parser(), and this is an undefined
behavior. This could be fixed by limiting the number of lines in a
source file to some sane value. Another approach consists in emitting
a warning and resetting the line counter every time it reaches
INT_MAX. Thoughts?

For reference (and for the people who have access to it), the related
OSS-Fuzz issue is
https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=28751.

Cheers,
Nicolas

Re: libsepol: signed integer overflow in the HLL line counter of CIL compiler

[William Roberts]

On Tue, Feb 2, 2021 at 1:37 AM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
>
> Hello,
>
> OSS-Fuzz found an integer overflow when compiling the following
> (empty) CIL policy:
>
> ;;*lms 2147483647 a
> ; (empty line)
>
> ";;*lms" is a line mark which can be produced by the HLL compiler (if
> I understand correctly the meaning of CIL_KEY_HLL_LMS in
> libsepol/cil/src/cil_parser.c). The line number is parsed as an "int"
> variable:
>
>   *hll_lineno = strtol(tok.value, &end, 10);
>   if (errno == ERANGE || *end != '\0') {
>     cil_log(CIL_ERR, "Problem parsing line number for line mark\n");
>     goto exit;
>   }
>
> This code has another issue which is that it silently truncates values
> to 32-bit signed integers on systems where sizeof(long) is 8, because
> hll_lineno is of type "int *", not "long *".
>
> But the issue found by OSS-Fuzz is that when 2147483647 is used (which
> is INT_MAX, 0x7fffffff in hexadecimal), "hll_lineno++;" overflows the
> capacity of signed integers, in cil_parser(), and this is an undefined
> behavior. This could be fixed by limiting the number of lines in a
> source file to some sane value. Another approach consists in emitting
> a warning and resetting the line counter every time it reaches
> INT_MAX. Thoughts?

I would lean towards using the proper type, so we get the full range depending
on architecture and warn on lineno wrap, but let it happen. IIUC,
Lineno is a helper.
I don't see it really affecting anything, so why make it a fatal error.

>
> For reference (and for the people who have access to it), the related
> OSS-Fuzz issue is
> https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=28751.
>
> Cheers,
> Nicolas
>

Re: libsepol: signed integer overflow in the HLL line counter of CIL compiler

[James Carter]

On Tue, Feb 2, 2021 at 2:42 PM William Roberts <bill.c.roberts@gmail.com> wrote:
>
> On Tue, Feb 2, 2021 at 1:37 AM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
> >
> > Hello,
> >
> > OSS-Fuzz found an integer overflow when compiling the following
> > (empty) CIL policy:
> >
> > ;;*lms 2147483647 a
> > ; (empty line)
> >
> > ";;*lms" is a line mark which can be produced by the HLL compiler (if
> > I understand correctly the meaning of CIL_KEY_HLL_LMS in
> > libsepol/cil/src/cil_parser.c). The line number is parsed as an "int"
> > variable:
> >
> >   *hll_lineno = strtol(tok.value, &end, 10);
> >   if (errno == ERANGE || *end != '\0') {
> >     cil_log(CIL_ERR, "Problem parsing line number for line mark\n");
> >     goto exit;
> >   }
> >
> > This code has another issue which is that it silently truncates values
> > to 32-bit signed integers on systems where sizeof(long) is 8, because
> > hll_lineno is of type "int *", not "long *".
> >
> > But the issue found by OSS-Fuzz is that when 2147483647 is used (which
> > is INT_MAX, 0x7fffffff in hexadecimal), "hll_lineno++;" overflows the
> > capacity of signed integers, in cil_parser(), and this is an undefined
> > behavior. This could be fixed by limiting the number of lines in a
> > source file to some sane value. Another approach consists in emitting
> > a warning and resetting the line counter every time it reaches
> > INT_MAX. Thoughts?
>
> I would lean towards using the proper type, so we get the full range depending
> on architecture and warn on lineno wrap, but let it happen. IIUC,
> Lineno is a helper.
> I don't see it really affecting anything, so why make it a fatal error.
>

I agree that it doesn't need to be a fatal error. I think it should be
read into an unsigned long and then checked using something like what
Nicolas did in cil_fill_integer() before being saved in hll_lineno.
Also, it makes sense to just make hll_lineno be a uint32_t.

I am working on a patch, but I won't be able to send it out until late
this afternoon.

Jim

> >
> > For reference (and for the people who have access to it), the related
> > OSS-Fuzz issue is
> > https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=28751.
> >
> > Cheers,
> > Nicolas
> >