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

[Petr Lautrbach <plautrba@redhat.com>]

James Carter <jwcart2@gmail.com> writes:

> On Wed, Jan 6, 2021 at 3:13 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.
>>
>> When using "val > UINT32_MAX" with "unsigned long val;", it is expected
>> that some compilers emit a warning when the size of "unsigned long" is
>> 32 bits. In theory gcc could be such a compiler (with warning
>> -Wtype-limits, which is included in -Wextra). Nevertheless this is
>> currently broken, according to
>> https://gcc.gnu.org/pipermail/gcc-help/2021-January/139755.html and
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=89126 (this bug was
>> opened in January 2019).
>>
>> In order to prevent this warning from appearing, introduce some
>> preprocessor macros around the bound check.
>>
>> Fixes: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=28456
>> Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
>
> Acked-by: James Carter <jwcart2@gmail.com>

Merged, thanks!


>> ---
>>  libsepol/cil/src/cil_build_ast.c | 16 ++++++++++++----
>>  1 file changed, 12 insertions(+), 4 deletions(-)
>>
>> diff --git a/libsepol/cil/src/cil_build_ast.c b/libsepol/cil/src/cil_build_ast.c
>> index be10d61b1314..02481558ad11 100644
>> --- a/libsepol/cil/src/cil_build_ast.c
>> +++ b/libsepol/cil/src/cil_build_ast.c
>> @@ -5570,19 +5570,27 @@ 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);
>> +       val = strtoul(int_node->data, &endptr, base);
>>         if (errno != 0 || endptr == int_node->data || *endptr != '\0') {
>>                 rc = SEPOL_ERR;
>>                 goto exit;
>>         }
>>
>> +       /* Ensure that the value fits a 32-bit integer without triggering -Wtype-limits */
>> +#if ULONG_MAX > UINT32_MAX
>> +       if (val > UINT32_MAX) {
>> +               rc = SEPOL_ERR;
>> +               goto exit;
>> +       }
>> +#endif
>> +
>>         *integer = val;
>>
>>         return SEPOL_OK;
>> @@ -5598,7 +5606,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.30.0
>>