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;".
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.
Thanks,
Nicolas
