Backport 2.28: Fix false positives in constant time tests using MSan with Clang 16

CMakeLists.txt

@@ -133,7 +133,7 @@ endif()
 # If this is the root project add longer list of available CMAKE_BUILD_TYPE values
 if(CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR)
     set(CMAKE_BUILD_TYPE ${CMAKE_BUILD_TYPE}
-        CACHE STRING "Choose the type of build: None Debug Release Coverage ASan ASanDbg MemSan MemSanDbg Check CheckFull"
+        CACHE STRING "Choose the type of build: None Debug Release Coverage ASan ASanDbg CFMemSan CFMemSanDbg MemSan MemSanDbg Check CheckFull"
         FORCE)
 endif()
 
@@ -222,6 +222,14 @@ if(CMAKE_COMPILER_IS_CLANG)
     set(CMAKE_C_FLAGS_MEMSAN      "-fsanitize=memory -O3")
     set(CMAKE_C_FLAGS_MEMSANDBG   "-fsanitize=memory -O1 -g3 -fno-omit-frame-pointer -fno-optimize-sibling-calls -fsanitize-memory-track-origins=2")
     set(CMAKE_C_FLAGS_CHECK       "-Os")
+
+    set(CMAKE_C_FLAGS_CFMEMSAN   "${CMAKE_C_FLAGS_MEMSAN} -DMBEDTLS_TEST_CONSTANT_FLOW_MEMSAN")
+    set(CMAKE_C_FLAGS_CFMEMSANDBG "${CMAKE_C_FLAGS_MEMSANDBG} -DMBEDTLS_TEST_CONSTANT_FLOW_MEMSAN")
+    if(CMAKE_C_COMPILER_VERSION VERSION_GREATER 16.0.0)
+        set(CMAKE_C_FLAGS_CFMEMSAN "${CMAKE_C_FLAGS_CFMEMSAN} -fno-sanitize-memory-param-retval")
+        set(CMAKE_C_FLAGS_CFMEMSANDBG "${CMAKE_C_FLAGS_CFMEMSANDBG} -fno-sanitize-memory-param-retval")
+    endif()
+
 endif(CMAKE_COMPILER_IS_CLANG)
 
 if(CMAKE_COMPILER_IS_IAR)

include/mbedtls/config.h

@@ -2200,8 +2200,10 @@
  * clang's MemorySanitizer. This causes some existing tests to also test
  * this non-functional property of the code under test.
  *
- * This setting requires compiling with clang -fsanitize=memory. The test
- * suites can then be run normally.
+ * This setting requires compiling with
+ * `clang -fsanitize=memory -fno-sanitize-memory-param-retval` with Clang 16
+ * and above, or `clang -fsanitize=memory` with older Clang.
+ * 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.

tests/scripts/all.sh

@@ -1724,15 +1724,14 @@ skip_suites_without_constant_flow () {
 component_test_memsan_constant_flow () {
     # This tests both (1) accesses to undefined memory, and (2) branches or
     # memory access depending on secret values. To distinguish between those:
-    # - unset MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN - does the failure persist?
-    # - or alternatively, change the build type to MemSanDbg, which enables
+    # - change the build type to MemSan - does the failure persist?
+    # - or alternatively, change the build type to CFMemSanDbg, which enables
     # origin tracking and nicer stack traces (which are useful for debugging
     # anyway), and check if the origin was TEST_CF_SECRET() or something else.
     msg "build: cmake MSan (clang), full config with constant flow testing"
     scripts/config.py full
-    scripts/config.py set MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN
     scripts/config.py unset MBEDTLS_AESNI_C # memsan doesn't grok asm
-    CC=clang cmake -D CMAKE_BUILD_TYPE:String=MemSan .
+    CC=clang cmake -D CMAKE_BUILD_TYPE:String=CFMemSan .
     make
 
     msg "test: main suites (Msan + constant flow)"

tests/suites/test_suite_constant_time.data

@@ -1,3 +1,30 @@
+Constant-flow instrumentation: NOP
+cf_instrumentation:CF_INSTRUMENTATION_NOP
+
+Constant-flow instrumentation: memcpy
+cf_instrumentation:CF_INSTRUMENTATION_MEMCPY
+
+Constant-flow instrumentation: memmove
+cf_instrumentation:CF_INSTRUMENTATION_MEMMOVE
+
+Constant-flow instrumentation: copy with for loop
+cf_instrumentation:CF_INSTRUMENTATION_COPY_LOOP
+
+# Detect sanitizers where a secret value may not be passed to a function.
+# Such sanitizers cause false positives in most of the other constant-flow
+# tests.
+# This is the case with MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN when building
+# with Clang >=16 with just -fsanitize=memory. The fix is to pass the
+# additional option -fno-sanitize-memory-param-retval to clang.
+Constant-flow instrumentation: copy through function
+cf_instrumentation:CF_INSTRUMENTATION_COPY_THROUGH_FUNCTION
+
+Constant-flow instrumentation: memset
+cf_instrumentation:CF_INSTRUMENTATION_MEMSET
+
+Constant-flow instrumentation: mbedtls_platform_zeroize
+cf_instrumentation:CF_INSTRUMENTATION_ZEROIZE
+
 # 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

tests/suites/test_suite_constant_time.function

@@ -13,8 +13,100 @@
 #include <constant_time_invasive.h>
 
 #include <test/constant_flow.h>
+
+#include <mbedtls/platform_util.h>
+
+/* Which kind of smoke test cf_instrumentation will do. */
+typedef enum {
+    CF_INSTRUMENTATION_NOP,
+    CF_INSTRUMENTATION_MEMCPY,
+    CF_INSTRUMENTATION_MEMMOVE,
+    CF_INSTRUMENTATION_COPY_LOOP,
+    CF_INSTRUMENTATION_COPY_THROUGH_FUNCTION,
+    CF_INSTRUMENTATION_MEMSET,
+    CF_INSTRUMENTATION_ZEROIZE,
+} cf_instrumentation_method_t;
+
+/* A function that is non-inlined on GCC-like compilers, including Clang.
+ * Use this to implement CF_INSTRUMENTATION_COPY_THROUGH_FUNCTION.
+ */
+#if defined(__has_attribute)
+#if __has_attribute(noinline)
+__attribute__((noinline))
+#endif
+#endif
+static unsigned char u8_identity_function(unsigned char c)
+{
+    return c;
+}
+
 /* END_HEADER */
 
+/* BEGIN_CASE */
+/* Test the constant-flow instrumentation. If this test function fails,
+ * something is wrong with the constant-flow instrumentation. */
+void cf_instrumentation(int method)
+{
+    unsigned char a[42] = { 0 };
+    unsigned char *p = NULL;
+
+    /* Set the buffer at p to some not-all-zero pattern,
+     * unless using the NOP method. */
+    TEST_CALLOC(p, sizeof(a));
+    if (method != CF_INSTRUMENTATION_NOP) {
+        for (size_t i = 0; i < sizeof(a); i++) {
+            p[i] = i & 0xff;
+        }
+    }
+
+    TEST_CF_SECRET(&a, sizeof(a));
+    TEST_CF_SECRET(p, sizeof(a));
+
+    /* Set the buffer at p to all-zero, possibly using a, in constant time. */
+    switch (method) {
+        case CF_INSTRUMENTATION_NOP:
+            /* p is already zero per above. */
+            break;
+        case CF_INSTRUMENTATION_MEMCPY:
+            memcpy(p, a, sizeof(a));
+            break;
+        case CF_INSTRUMENTATION_MEMMOVE:
+            memmove(p, a, sizeof(a));
+            break;
+        case CF_INSTRUMENTATION_COPY_LOOP:
+            for (size_t i = 0; i < sizeof(a); i++) {
+                p[i] = a[i];
+            }
+            break;
+        case CF_INSTRUMENTATION_COPY_THROUGH_FUNCTION:
+            for (size_t i = 0; i < sizeof(a); i++) {
+                p[i] = u8_identity_function(a[i]);
+            }
+            break;
+        case CF_INSTRUMENTATION_MEMSET:
+            memset(p, 0, sizeof(a));
+            break;
+        case CF_INSTRUMENTATION_ZEROIZE:
+            mbedtls_platform_zeroize(p, sizeof(a));
+            break;
+        default:
+            TEST_CF_PUBLIC(&a, sizeof(a));
+            TEST_CF_PUBLIC(p, sizeof(a));
+            TEST_FAIL("Unknown instrumentation method");
+    }
+
+    /* Check that the buffer at p is now all-zero, not in constant time.
+     * If this triggers a constant-flow violation, TEST_CF_PUBLIC isn't
+     * doing its job. */
+    TEST_CF_PUBLIC(&a, sizeof(a));
+    TEST_CF_PUBLIC(p, sizeof(a));
+    ASSERT_COMPARE(a, sizeof(a), p, sizeof(a));
+
+exit:
+    mbedtls_free(p);
+}
+/* END_CASE */
+
 /* BEGIN_CASE depends_on:MBEDTLS_SSL_SOME_SUITES_USE_TLS_CBC:MBEDTLS_TEST_HOOKS */
 void ssl_cf_memcpy_offset(int offset_min, int offset_max, int len)
 {