On Fri, Feb 5, 2021 at 5:28 AM Nicolas Iooss <nicolas.iooss@xxxxxxx> wrote:
>
> On Thu, Feb 4, 2021 at 11:01 PM James Carter <jwcart2@xxxxxxxxx> wrote:
> >
> > From: James Carter <jwcart2@xxxxxxxxxxxxx>
> >
> > Nicolass Iooss reports:
>
> There is only one S in my first name ;)
>
> > OSS-Fuzz found an integer overflow when compiling the following
> > (empty) CIL policy:
> >
> > ;;*lms 2147483647 a
> > ; (empty line)
> >
> > Change hll_lineno to uint32_t which is the type of the field hll_line
> > in struct cil_tree_node where the line number will be stored eventually.
> > Read the line number into an unsigned long variable using strtoul()
> > instead of strtol(). On systems where ULONG_MAX > UINT32_MAX, set the
> > value to 0 and print a warning if it is larger than UINT32_MAX before
> > storing it in hll_lineno.
> >
> > Also change hll_expand to uint32_t, since its value will be either
> > 0 or 1 and there is no need for it to be signed.
> >
> > Reported-by: Nicolass Iooss <nicolas.iooss@xxxxxxx>
> > Signed-off-by: James Carter <jwcart2@xxxxxxxxx>
> > ---
> > libsepol/cil/src/cil_parser.c | 31 ++++++++++++++++++++-----------
> > 1 file changed, 20 insertions(+), 11 deletions(-)
> >
> > diff --git a/libsepol/cil/src/cil_parser.c b/libsepol/cil/src/cil_parser.c
> > index b62043b9..9d3bd580 100644
> >[...]
> > @@ -140,11 +141,19 @@ static int add_hll_linemark(struct cil_tree_node **current, int *hll_lineno, int
> > cil_log(CIL_ERR, "Invalid line mark syntax\n");
> > goto exit;
> > }
> > - *hll_lineno = strtol(tok.value, &end, 10);
> > +
> > + val = strtoul(tok.value, &end, 10);
> > if (errno == ERANGE || *end != '\0') {
> > cil_log(CIL_ERR, "Problem parsing line number for line mark\n");
> > goto exit;
> > }
> > +#if ULONG_MAX > UINT32_MAX
> > + if (val > UINT32_MAX) {
> > + cil_log(CIL_WARN, "Line mark line number > UINT32_MAX\n");
> > + val = 0;
> > + }
> > +#endif
> > + *hll_lineno = val;
>
> Using both cil_log(CIL_ERR, "Problem parsing line number for line
> mark\n"); and cil_log(CIL_WARN, "Line mark line number >
> UINT32_MAX\n"); is inconsistent (if a CIL file is processed on a
> 32-bit system and contains a line mark with a number greater than
> UINT32_MAX, the compilation will fail due to the first if).
>
> In my humble opinion, the compiler should output an error if val >
> UINT32_MAX here, while accepting to (silently) wrap around UINT32_MAX
> when the line number is later incremented (which is the standard
> behavior with unsigned integers in C, if I remember correctly). This
> way, numbers greater than UINT32_MAX are forbidden in CIL source
> files, both on 32-bit and 64-bit systems. If you disagree and want to
> accept line mark with any line numbers, the first if block needs to be
> changed to use "cil_log(CIL_WARN, "Problem parsing line number for
> line mark\n");val=0;".
>
I agree with your reasoning. Really if there is a number greater than
UINT32_MAX in the source files, it most likely is an error.
Yes, it is not undefined behavior for an unsigned value to wrap.
> Another issue: function add_hll_linemark() currently ends with:
>
> exit:
> cil_log(CIL_ERR, "Problem with high-level line mark at line %d of
> %s\n", tok.line, path);
> return SEPOL_ERR;
>
> The %d needs to be replaced with %u in the message. Moreover if you
> want to keep cil_log(CIL_WARN), it would be very useful to have ("...
> at line %u of %s\n", tok.line, path) in the message.
>
Good catch, I send out a revised patch.
Thanks for the review,
Jim
> Thanks,
> Nicolas
>
