| Message ID | 20201230100746.2549568-4-nicolas.iooss@m4x.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/6] libsepol: do not decode out-of-bound rolebounds | expand |
On Wed, Dec 30, 2020 at 4:09 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. > > 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 | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/libsepol/cil/src/cil_build_ast.c b/libsepol/cil/src/cil_build_ast.c > index 67801def0dc0..0c9015cef578 100644 > --- a/libsepol/cil/src/cil_build_ast.c > +++ b/libsepol/cil/src/cil_build_ast.c > @@ -5566,15 +5566,15 @@ 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); > - if (errno != 0 || endptr == int_node->data || *endptr != '\0') { > + val = strtoul(int_node->data, &endptr, base); > + if (errno != 0 || endptr == int_node->data || *endptr != '\0' || val > UINT32_MAX) { I wonder if compilers/static analysis tools will balk on this as strtoul's return, an unsigned long, on a 32 bit machine will be 32 bits, so this could have a dead expression as val > UINT32_MAX will always be false. Perhaps use the strtoull variant to always have 64 bits? > rc = SEPOL_ERR; > goto exit; > } > @@ -5594,7 +5594,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.29.2 >
On Thu, Dec 31, 2020 at 4:04 PM William Roberts <bill.c.roberts@gmail.com> wrote: > > On Wed, Dec 30, 2020 at 4:09 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. > > > > 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 | 10 +++++----- > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/libsepol/cil/src/cil_build_ast.c b/libsepol/cil/src/cil_build_ast.c > > index 67801def0dc0..0c9015cef578 100644 > > --- a/libsepol/cil/src/cil_build_ast.c > > +++ b/libsepol/cil/src/cil_build_ast.c > > @@ -5566,15 +5566,15 @@ 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); > > - if (errno != 0 || endptr == int_node->data || *endptr != '\0') { > > + val = strtoul(int_node->data, &endptr, base); > > + if (errno != 0 || endptr == int_node->data || *endptr != '\0' || val > UINT32_MAX) { > > I wonder if compilers/static analysis tools will balk on this as > strtoul's return, an unsigned long, > on a 32 bit machine will be 32 bits, so this could have a dead > expression as val > UINT32_MAX > will always be false. Perhaps use the strtoull variant to always have 64 bits? In my humble opinion, a compiler or a static analyzer which warn about the fact that "comparing an unsigned long value to UINT32_MAX is always true" have an issue, because this seems to be the most natural way of checking that a potentially-64-bit number can be safely downcasted to 32 bits. I find the suggestion of using strtoull to get a 32-bit integer to be very hackish, considering that on 32-bit systems, strtoul does the job fine (returning with errno = ERANGE when the value is too large) and 64-bit integers are using pairs of registers to be stored. If this code ever causes issues with some compilers, some preprocessor logic (such as "#if ULONG_MAX > UINT32_MAX") could be added to hide "val > UINT32_MAX" from 32-bit compilers. Nevertheless in an effort to keep the amount of preprocessor code as low as possible, I do not want to include such logic right now. In short, I am not willing to change this patch unless someone reports a regression due to "val > UINT32_MAX". Thanks for your review! Nicolas > > rc = SEPOL_ERR; > > goto exit; > > } > > @@ -5594,7 +5594,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.29.2 > >
On Sat, Jan 2, 2021 at 5:13 AM Nicolas Iooss <nicolas.iooss@m4x.org> wrote: > > On Thu, Dec 31, 2020 at 4:04 PM William Roberts > <bill.c.roberts@gmail.com> wrote: > > > > On Wed, Dec 30, 2020 at 4:09 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. > > > > > > 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 | 10 +++++----- > > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > > > diff --git a/libsepol/cil/src/cil_build_ast.c b/libsepol/cil/src/cil_build_ast.c > > > index 67801def0dc0..0c9015cef578 100644 > > > --- a/libsepol/cil/src/cil_build_ast.c > > > +++ b/libsepol/cil/src/cil_build_ast.c > > > @@ -5566,15 +5566,15 @@ 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); > > > - if (errno != 0 || endptr == int_node->data || *endptr != '\0') { > > > + val = strtoul(int_node->data, &endptr, base); > > > + if (errno != 0 || endptr == int_node->data || *endptr != '\0' || val > UINT32_MAX) { > > > > I wonder if compilers/static analysis tools will balk on this as > > strtoul's return, an unsigned long, > > on a 32 bit machine will be 32 bits, so this could have a dead > > expression as val > UINT32_MAX > > will always be false. Perhaps use the strtoull variant to always have 64 bits? > > In my humble opinion, a compiler or a static analyzer which warn about > the fact that "comparing an unsigned long value to UINT32_MAX is > always true" have an issue, because this seems to be the most natural > way of checking that a potentially-64-bit number can be safely > downcasted to 32 bits. > > I find the suggestion of using strtoull to get a 32-bit integer to be > very hackish, considering that on 32-bit systems, strtoul does the job > fine (returning with errno = ERANGE when the value is too large) and > 64-bit integers are using pairs of registers to be stored. If this > code ever causes issues with some compilers, some preprocessor logic > (such as "#if ULONG_MAX > UINT32_MAX") could be added to hide "val > > UINT32_MAX" from 32-bit compilers. Nevertheless in an effort to keep > the amount of preprocessor code as low as possible, I do not want to > include such logic right now. The reason I bring this up, is that I've personally encountered this on Android when porting libraries before. But these we're with much older GCC/CLANG variants. When Android's build went from gcc to clang, I remember clang being extra noisy and these -Wtype-limits warnings being a constant battle when trying to enable -Wextra. t really boils down to the warning (per the man page): -Wtype-limits Warn if a comparison is always true or always false due to the limited range of the data type, but do not warn for constant expressions. For example, warn if an unsigned variable is compared against zero with < or >=. This warning is also enabled by -Wextra. But I tested this with an arm32v7 Docker Container running Ubuntu 20.04 using: - GCC 9.3.0 - CLANG 10.0 as well as the NDK clang version Android (6454773 based on r365631c2) clang version 9.0.8 cross compiling from x86_64 machine and it doesn't reproduce. I wonder if they changed the behavior a bit because of having to deal with this error all the time, and it was no fun. What's more perplexing is that the uint8_t and uint16_t cases trigger the warning and the uint32_t and uint64_t cases do not. I can understand skipping the warning on the unsigned long because of the word length changing from 8 to 4 bytes, and the 4 byte check ending up being always false and harmless, but in the fixed width case... I wonder why the compiler gods chose so. uint8_t y = (uint8_t)argc; if(y > UINT8_MAX) { printf("foo: %u\n", y); } // always false uint16_t w = (uint16_t)argc; if(w > UINT16_MAX) { printf("foo: %u\n", w); } // always false uint32_t q = (uint32_t)argc; if(q > UINT32_MAX) { printf("foo: %u\n", q); } // always false uint64_t z = (uint64_t)argc; if(z > UINT64_MAX) { printf("foo: %llu\n", z); } // always false a.c:23:7: warning: comparison is always false due to limited range of data type [-Wtype-limits] 23 | if(y > UINT8_MAX) { printf("foo: %u\n", y); } // always false > > In short, I am not willing to change this patch unless someone reports > a regression due to "val > UINT32_MAX". Based on my tests with modern versions of toolchains, I think we're fine. I don't know why we're fine, but we appear so. > > Thanks for your review! > Nicolas ack for the whole series since this was my only worry. > > > > rc = SEPOL_ERR; > > > goto exit; > > > } > > > @@ -5594,7 +5594,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.29.2 > > > >
On Wed, Dec 30, 2020 at 5:09 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. > > 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 | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/libsepol/cil/src/cil_build_ast.c b/libsepol/cil/src/cil_build_ast.c > index 67801def0dc0..0c9015cef578 100644 > --- a/libsepol/cil/src/cil_build_ast.c > +++ b/libsepol/cil/src/cil_build_ast.c > @@ -5566,15 +5566,15 @@ 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); > - if (errno != 0 || endptr == int_node->data || *endptr != '\0') { > + val = strtoul(int_node->data, &endptr, base); > + if (errno != 0 || endptr == int_node->data || *endptr != '\0' || val > UINT32_MAX) { > rc = SEPOL_ERR; > goto exit; > } > @@ -5594,7 +5594,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.29.2 >
On Mon, Jan 4, 2021 at 11:03 AM James Carter <jwcart2@gmail.com> wrote: > > On Wed, Dec 30, 2020 at 5:09 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. > > > > 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 | 10 +++++----- > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/libsepol/cil/src/cil_build_ast.c b/libsepol/cil/src/cil_build_ast.c > > index 67801def0dc0..0c9015cef578 100644 > > --- a/libsepol/cil/src/cil_build_ast.c > > +++ b/libsepol/cil/src/cil_build_ast.c > > @@ -5566,15 +5566,15 @@ 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); > > - if (errno != 0 || endptr == int_node->data || *endptr != '\0') { > > + val = strtoul(int_node->data, &endptr, base); > > + if (errno != 0 || endptr == int_node->data || *endptr != '\0' || val > UINT32_MAX) { > > rc = SEPOL_ERR; > > goto exit; > > } > > @@ -5594,7 +5594,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.29.2 > > It turns out when GCC fixes a bug with -Wtype-limits, this will cause a regression. The current top-level Makefile includes exporting CFLAGS -Wextra which will enable this warning. I find it surprising this has been a known gcc issue for some time and that clang has the same bug. See: - https://gcc.gnu.org/pipermail/gcc-help/2021-January/139755.html I would fix it now. Bill
diff --git a/libsepol/cil/src/cil_build_ast.c b/libsepol/cil/src/cil_build_ast.c index 67801def0dc0..0c9015cef578 100644 --- a/libsepol/cil/src/cil_build_ast.c +++ b/libsepol/cil/src/cil_build_ast.c @@ -5566,15 +5566,15 @@ 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); - if (errno != 0 || endptr == int_node->data || *endptr != '\0') { + val = strtoul(int_node->data, &endptr, base); + if (errno != 0 || endptr == int_node->data || *endptr != '\0' || val > UINT32_MAX) { rc = SEPOL_ERR; goto exit; } @@ -5594,7 +5594,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; }
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. 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 | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)