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

@@ -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.

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\"	\

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)

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],

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
+VLAs are unsafe when it is important to handle allocation failure in a
-than allocate it using `isc_mem_get()` from the heap.  Usually, a short lived
+controlled manner rather than an uncontrolled crash. They are safer if the
-arrays local to that particular functions would be good fit for using VLAs.
+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.
 
 #### <a name="public_namespace"></a>Public Interface Namespace
 

fuzz/Makefile.am

@@ -1,5 +1,8 @@
 include $(top_srcdir)/Makefile.top
 
+AM_CFLAGS +=				\
+	$(TEST_CFLAGS)
+
 AM_CPPFLAGS +=				\
 	$(LIBISC_CFLAGS)		\
 	$(LIBDNS_CFLAGS)		\