From eeead1cfe7c5e967237f09eb9b7f9a18dd4fc8c4 Mon Sep 17 00:00:00 2001 From: Tony Finch Date: Thu, 10 Mar 2022 13:04:08 +0000 Subject: [PATCH 1/2] Remove a redundant variable-length array In the GSS-TSIG verification code there was an alarming variable-length array whose size came off the network, from the signature in the request. It turned out to be safe, because the caller had previously checked that the signature had a reasonable size. However, the safety checks are in the generic TSIG implementation, and the risky VLA usage was in the GSS-specific code, and they are separated by the DST indirection layer, so it wasn't immediately obvious that the risky VLA was in fact safe. In fact this risky VLA was completely unnecessary, because the GSS signature can be verified in place without being copied to the stack, like the message covered by the signature. The `REGION_TO_GBUFFER()` macro backwardly assigns the region in its left argument to the GSS buffer in its right argument; this is just a pointer and length conversion, without copying any data. The `gss_verify_mic()` call uses both message and signature GSS buffers in a read-only manner. --- lib/dns/gssapi_link.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/lib/dns/gssapi_link.c b/lib/dns/gssapi_link.c index b04bfd813a..a3fbb6e7e1 100644 --- a/lib/dns/gssapi_link.c +++ b/lib/dns/gssapi_link.c @@ -189,11 +189,10 @@ gssapi_sign(dst_context_t *dctx, isc_buffer_t *sig) { static isc_result_t gssapi_verify(dst_context_t *dctx, const isc_region_t *sig) { dst_gssapi_signverifyctx_t *ctx = dctx->ctxdata.gssctx; - isc_region_t message, r; + isc_region_t message; gss_buffer_desc gmessage, gsig; OM_uint32 minor, gret; gss_ctx_id_t gssctx = dctx->key->keydata.gssctx; - unsigned char buf[sig->length]; char err[1024]; /* @@ -202,11 +201,7 @@ gssapi_verify(dst_context_t *dctx, const isc_region_t *sig) { */ isc_buffer_usedregion(ctx->buffer, &message); REGION_TO_GBUFFER(message, gmessage); - - memmove(buf, sig->base, sig->length); - r.base = buf; - r.length = sig->length; - REGION_TO_GBUFFER(r, gsig); + REGION_TO_GBUFFER(*sig, gsig); /* * Verify the data. From 599c1d2a6b4539bb1f70f5b8a7321311b4bcb60a Mon Sep 17 00:00:00 2001 From: Tony Finch Date: Fri, 18 Mar 2022 14:50:36 +0000 Subject: [PATCH 2/2] Avoid using C99 variable length arrays From an attacker's point of view, a VLA declaration is essentially a primitive for performing arbitrary arithmetic on the stack pointer. If the attacker can control the size of a VLA they have a very powerful tool for causing memory corruption. To mitigate this kind of attack, and the more general class of stack clash vulnerabilities, C compilers insert extra code when allocating a VLA to probe the growing stack one page at a time. If these probes hit the stack guard page, the program will crash. From the point of view of a C programmer, there are a few things to consider about VLAs: * If it is important to handle allocation failures in a controlled manner, don't use VLAs. You can use VLAs if it is OK for unreasonable inputs to cause an uncontrolled crash. * If the VLA is known to be smaller than some known fixed size, use a fixed size array and a run-time check to ensure it is large enough. This will be more efficient than the compiler's stack probes that need to cope with arbitrary-size VLAs. * If the VLA might be large, allocate it on the heap. The heap allocator can allocate multiple pages in one shot, whereas the stack clash probes work one page at a time. Most of the existing uses of VLAs in BIND are in test code where they are benign, but there was one instance in `named`, in the GSS-TSIG verification code, which has now been removed. This commit adjusts the style guide and the C compiler flags to allow VLAs in test code but not elsewhere. --- CHANGES | 3 +++ Makefile.tests | 3 +++ bin/tests/Makefile.am | 3 +++ configure.ac | 6 +++++- doc/dev/style.md | 11 ++++++++--- fuzz/Makefile.am | 3 +++ 6 files changed, 25 insertions(+), 4 deletions(-) diff --git a/CHANGES b/CHANGES index 0e652f6e35..62938ef4fc 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,6 @@ +5834. [cleanup] C99 variable-length arrays are difficult to use safely, + so avoid them except in test code. [GL #3201] + 5833. [bug] When encountering socket error while trying to initiate a TCP connection to a server, dig could hang indefinitely, when there were more servers to try. diff --git a/Makefile.tests b/Makefile.tests index 70d528285c..97a914db6a 100644 --- a/Makefile.tests +++ b/Makefile.tests @@ -7,6 +7,9 @@ TESTS = $(check_PROGRAMS) LOG_COMPILER = $(builddir)/../../unit-test-driver.sh +AM_CFLAGS += \ + $(TEST_CFLAGS) + AM_CPPFLAGS += \ $(CMOCKA_CFLAGS) \ -DNAMED_PLUGINDIR=\"$(libdir)/named\" \ diff --git a/bin/tests/Makefile.am b/bin/tests/Makefile.am index 7c2ebe27d5..cde8467e55 100644 --- a/bin/tests/Makefile.am +++ b/bin/tests/Makefile.am @@ -7,6 +7,9 @@ noinst_PROGRAMS = \ test_server \ wire_test +AM_CFLAGS += \ + $(TEST_CFLAGS) + test_client_CPPFLAGS = \ $(AM_CPPFLAGS) \ $(LIBISC_CFLAGS) diff --git a/configure.ac b/configure.ac index 32ed87ac3f..0139043da1 100644 --- a/configure.ac +++ b/configure.ac @@ -117,7 +117,10 @@ AS_IF([test "$enable_static" != "no" && test "$enable_developer" != "yes"], STD_CFLAGS="-Wall -Wextra -Wwrite-strings -Wpointer-arith -Wno-missing-field-initializers -Wformat -Wshadow" # These should be always errors -STD_CFLAGS="$STD_CFLAGS -Werror=implicit-function-declaration -Werror=missing-prototypes -Werror=format-security -Werror=parentheses -Werror=implicit -Werror=strict-prototypes" +STD_CFLAGS="$STD_CFLAGS -Werror=implicit-function-declaration -Werror=missing-prototypes -Werror=format-security -Werror=parentheses -Werror=implicit -Werror=strict-prototypes -Werror=vla" + +# ... except in test code +TEST_CFLAGS="-Wno-vla" # Fortify the sources by default STD_CPPFLAGS="-D_FORTIFY_SOURCE=2" @@ -159,6 +162,7 @@ AS_IF([test "$enable_developer" = "yes"], AC_SUBST([DEVELOPER_MODE]) AC_SUBST([STD_CFLAGS]) AC_SUBST([STD_CPPFLAGS]) +AC_SUBST([TEST_CFLAGS]) # [pairwise: --enable-warn-error, --disable-warn-error] AC_ARG_ENABLE([warn_error], diff --git a/doc/dev/style.md b/doc/dev/style.md index 9fe9a552f9..f3165f2a36 100644 --- a/doc/dev/style.md +++ b/doc/dev/style.md @@ -683,9 +683,14 @@ Declare variables as constant if they are not to be modified. #### Variable-Length Arrays -Use VLAs where it is more appropriate to allocate the memory on the stack rather -than allocate it using `isc_mem_get()` from the heap. Usually, a short lived -arrays local to that particular functions would be good fit for using VLAs. +VLAs are unsafe when it is important to handle allocation failure in a +controlled manner rather than an uncontrolled crash. They are safer if the +array size is checked first, but then you lose a lot of their simplicity +and readability. + +VLAs should not be used in most code in BIND. VLAs are OK in test code +where the lack of safety doesn't matter. The default compiler flags enforce +this rule. #### Public Interface Namespace diff --git a/fuzz/Makefile.am b/fuzz/Makefile.am index d82f8c359b..021156da95 100644 --- a/fuzz/Makefile.am +++ b/fuzz/Makefile.am @@ -1,5 +1,8 @@ include $(top_srcdir)/Makefile.top +AM_CFLAGS += \ + $(TEST_CFLAGS) + AM_CPPFLAGS += \ $(LIBISC_CFLAGS) \ $(LIBDNS_CFLAGS) \