On Tue, Feb 2, 2021 at 2:42 PM William Roberts <bill.c.roberts@xxxxxxxxx> wrote:
>
> On Tue, Feb 2, 2021 at 1:37 AM Nicolas Iooss <nicolas.iooss@xxxxxxx> 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
> >
