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.

diff --git a/CHANGES b/CHANGES
index 0e652f6e352bd64f6a6854e0da48169953f4460b..62938ef4fcf3c8c18145432daef448976cddcf6c 100644 (file)
--- 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 70d528285ca68aaa48f52972d2d66334fcc8ac30..97a914db6aa8798eb73f48ae2316d9efe7598723 100644 (file)
--- 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 7c2ebe27d52774ce2f4fc4bf98227ada161f52ca..cde8467e5543f43525fd71a6b8c7902002ebe7c4 100644 (file)
--- 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 32ed87ac3f7c95cb4b8bd3c345f8959c19d3a61d..0139043da12ed12ada5760cb55e992a192b03c0f 100644 (file)
--- 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 9fe9a552f96e34a1a8e928418c35fe1a5fcf6cce..f3165f2a36855d7ba9431fbd1e4782e43083a394 100644 (file)
--- 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.
 
 #### <a name="public_namespace"></a>Public Interface Namespace
 

diff --git a/fuzz/Makefile.am b/fuzz/Makefile.am
index d82f8c359b31494cd21613e92671086aad66b6ae..021156da95b24b70f737091fbd2ec4cc82e659e5 100644 (file)
--- 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)                \