* [PATCH v2] libsepol/cil: fix NULL pointer dereference when parsing an improper integer
@ 2021-01-06 8:13 Nicolas Iooss
2021-01-08 19:35 ` James Carter
0 siblings, 1 reply; 3+ messages in thread
From: Nicolas Iooss @ 2021-01-06 8:13 UTC (permalink / raw)
To: selinux; +Cc: William Roberts, James Carter
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>
---
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
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH v2] libsepol/cil: fix NULL pointer dereference when parsing an improper integer
2021-01-06 8:13 [PATCH v2] libsepol/cil: fix NULL pointer dereference when parsing an improper integer Nicolas Iooss
@ 2021-01-08 19:35 ` James Carter
2021-01-13 22:28 ` Petr Lautrbach
0 siblings, 1 reply; 3+ messages in thread
From: James Carter @ 2021-01-08 19:35 UTC (permalink / raw)
To: Nicolas Iooss; +Cc: SElinux list, William Roberts
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>
> ---
> 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
>
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH v2] libsepol/cil: fix NULL pointer dereference when parsing an improper integer
2021-01-08 19:35 ` James Carter
@ 2021-01-13 22:28 ` Petr Lautrbach
0 siblings, 0 replies; 3+ messages in thread
From: Petr Lautrbach @ 2021-01-13 22:28 UTC (permalink / raw)
To: James Carter, Nicolas Iooss; +Cc: SElinux list, William Roberts
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
>>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-01-14 2:04 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-01-06 8:13 [PATCH v2] libsepol/cil: fix NULL pointer dereference when parsing an improper integer Nicolas Iooss
2021-01-08 19:35 ` James Carter
2021-01-13 22:28 ` Petr Lautrbach
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.