Merge pull request #742 from mpg/cf-varpos-copy-2.7-restricted

[backport 2.7] Constant-flow copy of HMAC from variable position

include/mbedtls/config.h

@@ -447,9 +447,10 @@
  *
  * Enable testing of the constant-flow nature of some sensitive functions with
  * clang's MemorySanitizer. This causes some existing tests to also test
- * non-functional properties of the code under test.
+ * this non-functional property of the code under test.
  *
- * This setting requires compiling with clang -fsanitize=memory.
+ * This setting requires compiling with clang -fsanitize=memory. The test
+ * suites can then be run normally.
  *
  * \warning This macro is only used for extended testing; it is not considered
  * part of the library's API, so it may change or disappear at any time.
@@ -458,6 +459,25 @@
  */
 //#define MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN
 
+/**
+ * \def MBEDTLS_TEST_CONSTANT_FLOW_VALGRIND
+ *
+ * Enable testing of the constant-flow nature of some sensitive functions with
+ * valgrind's memcheck tool. This causes some existing tests to also test
+ * this non-functional property of the code under test.
+ *
+ * This setting requires valgrind headers for building, and is only useful for
+ * testing if the tests suites are run with valgrind's memcheck. This can be
+ * done for an individual test suite with 'valgrind ./test_suite_xxx', or when
+ * using CMake, this can be done for all test suites with 'make memcheck'.
+ *
+ * \warning This macro is only used for extended testing; it is not considered
+ * part of the library's API, so it may change or disappear at any time.
+ *
+ * Uncomment to enable testing of the constant-flow nature of selected code.
+ */
+//#define MBEDTLS_TEST_CONSTANT_FLOW_VALGRIND
+
 /**
  * \def MBEDTLS_TEST_NULL_ENTROPY
  *

include/mbedtls/ssl_internal.h

@@ -788,6 +788,30 @@ int mbedtls_ssl_cf_hmac(
         const unsigned char *data, size_t data_len_secret,
         size_t min_data_len, size_t max_data_len,
         unsigned char *output );
+
+/** \brief Copy data from a secret position with constant flow.
+ *
+ * This function copies \p len bytes from \p src_base + \p offset_secret to \p
+ * dst, with a code flow and memory access pattern that does not depend on \p
+ * offset_secret, but only on \p offset_min, \p offset_max and \p len.
+ *
+ * \param dst           The destination buffer. This must point to a writable
+ *                      buffer of at least \p len bytes.
+ * \param src_base      The base of the source buffer. This must point to a
+ *                      readable buffer of at least \p offset_max + \p len
+ *                      bytes.
+ * \param offset_secret The offset in the source buffer from which to copy.
+ *                      This must be no less than \p offset_min and no greater
+ *                      than \p offset_max.
+ * \param offset_min    The minimal value of \p offset_secret.
+ * \param offset_max    The maximal value of \p offset_secret.
+ * \param len           The number of bytes to copy.
+ */
+void mbedtls_ssl_cf_memcpy_offset( unsigned char *dst,
+                                   const unsigned char *src_base,
+                                   size_t offset_secret,
+                                   size_t offset_min, size_t offset_max,
+                                   size_t len );
 #endif /* MBEDTLS_SSL_SOME_SUITES_USE_TLS_CBC */
 
 #ifdef __cplusplus

library/ssl_tls.c

@@ -1303,27 +1303,6 @@ static void ssl_mac( mbedtls_md_context_t *md_ctx,
 #define SSL_SOME_MODES_USE_MAC
 #endif
 
-/* The function below is only used in the Lucky 13 counter-measure in
- * ssl_decrypt_buf(). These are the defines that guard the call site. */
-#if defined(SSL_SOME_MODES_USE_MAC) && \
-    ( defined(MBEDTLS_SSL_PROTO_TLS1) || \
-      defined(MBEDTLS_SSL_PROTO_TLS1_1) || \
-      defined(MBEDTLS_SSL_PROTO_TLS1_2) )
-/* This function makes sure every byte in the memory region is accessed
- * (in ascending addresses order) */
-static void ssl_read_memory( const unsigned char *p, size_t len )
-{
-    unsigned char acc = 0;
-    volatile unsigned char force;
-
-    for( ; len != 0; p++, len-- )
-        acc ^= *p;
-
-    force = acc;
-    (void) force;
-}
-#endif /* SSL_SOME_MODES_USE_MAC && ( TLS1 || TLS1_1 || TLS1_2 ) */
-
 /*
  * Encryption/decryption functions
  */
@@ -1785,6 +1764,26 @@ cleanup:
     mbedtls_md_free( &aux );
     return( ret );
 }
+
+/*
+ * Constant-flow memcpy from variable position in buffer.
+ * - functionally equivalent to memcpy(dst, src + offset_secret, len)
+ * - but with execution flow independent from the value of offset_secret.
+ */
+void mbedtls_ssl_cf_memcpy_offset( unsigned char *dst,
+                                   const unsigned char *src_base,
+                                   size_t offset_secret,
+                                   size_t offset_min, size_t offset_max,
+                                   size_t len )
+{
+    size_t offset;
+
+    for( offset = offset_min; offset <= offset_max; offset++ )
+    {
+        mbedtls_ssl_cf_memcpy_if_eq( dst, src_base + offset, len,
+                                     offset, offset_secret );
+    }
+}
 #endif /* MBEDTLS_SSL_SOME_SUITES_USE_TLS_CBC */
 
 static int ssl_decrypt_buf( mbedtls_ssl_context *ssl )
@@ -2142,6 +2141,7 @@ static int ssl_decrypt_buf( mbedtls_ssl_context *ssl )
     if( auth_done == 0 )
     {
         unsigned char mac_expect[MBEDTLS_SSL_MAC_ADD];
+        unsigned char mac_peer[MBEDTLS_SSL_MAC_ADD];
 
         ssl->in_msglen -= ssl->transform_in->maclen;
 
@@ -2156,6 +2156,8 @@ static int ssl_decrypt_buf( mbedtls_ssl_context *ssl )
                       ssl->in_msg, ssl->in_msglen,
                       ssl->in_ctr, ssl->in_msgtype,
                       mac_expect );
+            memcpy( mac_peer, ssl->in_msg + ssl->in_msglen,
+                              ssl->transform_in->maclen );
         }
         else
 #endif /* MBEDTLS_SSL_PROTO_SSL3 */
@@ -2194,12 +2196,10 @@ static int ssl_decrypt_buf( mbedtls_ssl_context *ssl )
                 return( ret );
             }
 
-            /* Make sure we access all the memory that could contain the MAC,
-             * before we check it in the next code block. This makes the
-             * synchronisation requirements for just-in-time Prime+Probe
-             * attacks much tighter and hopefully impractical. */
-            ssl_read_memory( ssl->in_msg + min_len,
-                                 max_len - min_len + ssl->transform_in->maclen );
+            mbedtls_ssl_cf_memcpy_offset( mac_peer, ssl->in_msg,
+                                          ssl->in_msglen,
+                                          min_len, max_len,
+                                          ssl->transform_in->maclen );
         }
         else
 #endif /* MBEDTLS_SSL_PROTO_TLS1 || MBEDTLS_SSL_PROTO_TLS1_1 || \
@@ -2211,11 +2211,10 @@ static int ssl_decrypt_buf( mbedtls_ssl_context *ssl )
 
 #if defined(MBEDTLS_SSL_DEBUG_ALL)
         MBEDTLS_SSL_DEBUG_BUF( 4, "expected mac", mac_expect, ssl->transform_in->maclen );
-        MBEDTLS_SSL_DEBUG_BUF( 4, "message  mac", ssl->in_msg + ssl->in_msglen,
-                               ssl->transform_in->maclen );
+        MBEDTLS_SSL_DEBUG_BUF( 4, "message  mac", mac_peer, ssl->transform_in->maclen );
 #endif
 
-        if( mbedtls_ssl_safer_memcmp( ssl->in_msg + ssl->in_msglen, mac_expect,
+        if( mbedtls_ssl_safer_memcmp( mac_peer, mac_expect,
                                       ssl->transform_in->maclen ) != 0 )
         {
 #if defined(MBEDTLS_SSL_DEBUG_ALL)

library/version_features.c

@@ -256,6 +256,9 @@ static const char *features[] = {
 #if defined(MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN)
     "MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN",
 #endif /* MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN */
+#if defined(MBEDTLS_TEST_CONSTANT_FLOW_VALGRIND)
+    "MBEDTLS_TEST_CONSTANT_FLOW_VALGRIND",
+#endif /* MBEDTLS_TEST_CONSTANT_FLOW_VALGRIND */
 #if defined(MBEDTLS_TEST_NULL_ENTROPY)
     "MBEDTLS_TEST_NULL_ENTROPY",
 #endif /* MBEDTLS_TEST_NULL_ENTROPY */

scripts/config.pl

@@ -125,6 +125,7 @@ MBEDTLS_REMOVE_ARC4_CIPHERSUITES
 MBEDTLS_RSA_NO_CRT
 MBEDTLS_SSL_HW_RECORD_ACCEL
 MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN
+MBEDTLS_TEST_CONSTANT_FLOW_VALGRIND
 MBEDTLS_TEST_NULL_ENTROPY
 MBEDTLS_X509_ALLOW_UNSUPPORTED_CRITICAL_EXTENSION
 MBEDTLS_ZLIB_SUPPORT

tests/scripts/all.sh

@@ -979,6 +979,28 @@ component_test_memsan_constant_flow () {
     make test
 }
 
+component_test_valgrind_constant_flow () {
+    # This tests both (1) everything that valgrind's memcheck usually checks
+    # (heap buffer overflows, use of uninitialized memory, use-after-free,
+    # etc.) and (2) branches or memory access depending on secret values,
+    # which will be reported as uninitialized memory. To distinguish between
+    # secret and actually uninitialized:
+    # - unset MBEDTLS_TEST_CONSTANT_FLOW_VALGRIND - does the failure persist?
+    # - or alternatively, build with debug info and manually run the offending
+    # test suite with valgrind --track-origins=yes, then check if the origin
+    # was TEST_CF_SECRET() or something else.
+    msg "build: cmake release GCC, full config with constant flow testing"
+    scripts/config.pl full
+    scripts/config.pl set MBEDTLS_TEST_CONSTANT_FLOW_VALGRIND
+    cmake -D CMAKE_BUILD_TYPE:String=Release .
+    make
+
+    # this only shows a summary of the results (how many of each type)
+    # details are left in Testing/<date>/DynamicAnalysis.xml
+    msg "test: main suites (valgrind + constant flow)"
+    make memcheck
+}
+
 component_test_default_no_deprecated () {
     # Test that removing the deprecated features from the default
     # configuration leaves something consistent.

tests/suites/helpers.function

@@ -38,6 +38,27 @@ typedef UINT32 uint32_t;
 #include <unistd.h>
 #endif
 
+/*
+ * Define the two macros
+ *
+ *  #define TEST_CF_SECRET(ptr, size)
+ *  #define TEST_CF_PUBLIC(ptr, size)
+ *
+ * that can be used in tests to mark a memory area as secret (no branch or
+ * memory access should depend on it) or public (default, only needs to be
+ * marked explicitly when it was derived from secret data).
+ *
+ * Arguments:
+ * - ptr: a pointer to the memory area to be marked
+ * - size: the size in bytes of the memory area
+ *
+ * Implementation:
+ * The basic idea is that of ctgrind <https://github.com/agl/ctgrind>: we can
+ * re-use tools that were designed for checking use of uninitialized memory.
+ * This file contains two implementations: one based on MemorySanitizer, the
+ * other on valgrind's memcheck. If none of them is enabled, dummy macros that
+ * do nothing are defined for convenience.
+ */
 #if defined(MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN)
 #include <sanitizer/msan_interface.h>
 
@@ -47,7 +68,16 @@ typedef UINT32 uint32_t;
 #define TEST_CF_PUBLIC  __msan_unpoison
 // void __msan_unpoison(const volatile void *a, size_t size);
 
-#else /* MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN */
+#elif defined(MBEDTLS_TEST_CONSTANT_FLOW_VALGRIND)
+#include <valgrind/memcheck.h>
+
+#define TEST_CF_SECRET  VALGRIND_MAKE_MEM_UNDEFINED
+// VALGRIND_MAKE_MEM_UNDEFINED(_qzz_addr, _qzz_len)
+#define TEST_CF_PUBLIC  VALGRIND_MAKE_MEM_DEFINED
+// VALGRIND_MAKE_MEM_DEFINED(_qzz_addr, _qzz_len)
+
+#else /* MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN ||
+         MBEDTLS_TEST_CONSTANT_FLOW_VALGRIND */
 
 #define TEST_CF_SECRET(ptr, size)
 #define TEST_CF_PUBLIC(ptr, size)

tests/suites/test_suite_ssl.data

@@ -73,3 +73,15 @@ ssl_cf_hmac:MBEDTLS_MD_SHA256
 Constant-flow HMAC: SHA384
 depends_on:MBEDTLS_SHA512_C:!MBEDTLS_SHA512_NO_SHA384
 ssl_cf_hmac:MBEDTLS_MD_SHA384
+
+# these are the numbers we'd get with an empty plaintext and truncated HMAC
+Constant-flow memcpy from offset: small
+ssl_cf_memcpy_offset:0:5:10
+
+# we could get this with 255-bytes plaintext and untruncated SHA-256
+Constant-flow memcpy from offset: medium
+ssl_cf_memcpy_offset:0:255:32
+
+# we could get this with 355-bytes plaintext and untruncated SHA-384
+Constant-flow memcpy from offset: large
+ssl_cf_memcpy_offset:100:339:48

tests/suites/test_suite_ssl.function

@@ -145,3 +145,36 @@ exit:
     mbedtls_free( out );
 }
 /* END_CASE */
+
+/* BEGIN_CASE depends_on:MBEDTLS_SSL_SOME_SUITES_USE_TLS_CBC */
+void ssl_cf_memcpy_offset( int offset_min, int offset_max, int len )
+{
+    unsigned char *dst = NULL;
+    unsigned char *src = NULL;
+    size_t src_len = offset_max + len;
+    size_t secret;
+
+    dst = mbedtls_calloc( 1, len );
+    TEST_ASSERT( dst != NULL );
+    src = mbedtls_calloc( 1, src_len );
+    TEST_ASSERT( src != NULL );
+
+    /* Fill src in a way that we can detect if we copied the right bytes */
+    rnd_std_rand( NULL, src, src_len );
+
+    for( secret = offset_min; secret <= (size_t) offset_max; secret++ )
+    {
+        TEST_CF_SECRET( &secret, sizeof( secret ) );
+        mbedtls_ssl_cf_memcpy_offset( dst, src, secret,
+                                      offset_min, offset_max, len );
+        TEST_CF_PUBLIC( &secret, sizeof( secret ) );
+        TEST_CF_PUBLIC( dst, len );
+
+        TEST_ASSERT( memcmp( dst, src + secret, len ) == 0 );
+    }
+
+exit:
+    mbedtls_free( dst );
+    mbedtls_free( src );
+}
+/* END_CASE */