From da06f3a7fe1a173796364d1a2bbb44c828794b76 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 22 Jan 2025 21:21:20 +0100 Subject: [PATCH 1/5] MSan for constant flow: update documentation for Clang 16 In the documentation of `MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN`, note that since Clang 16, an extra command line option `-fsanitize-memory-param-retval` is required. As documented in the release notes https://releases.llvm.org/16.0.0/tools/clang/docs/ReleaseNotes.html#sanitizers since Clang 16, MSan forbids passing "uninitialized" values in and out of functions. In constant-flow testing, "uninitialized" values are actually secrets that must be manipulated with a data-independent flow, and it's perfectly fine to pass these in and out of functions. Fix #9921 Signed-off-by: Gilles Peskine --- include/mbedtls/mbedtls_config.h | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/include/mbedtls/mbedtls_config.h b/include/mbedtls/mbedtls_config.h index ebc9276d20..d0b0184079 100644 --- a/include/mbedtls/mbedtls_config.h +++ b/include/mbedtls/mbedtls_config.h @@ -2039,8 +2039,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. From 3c47565db1f94552f271a17ed8ea8f762488c397 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 22 Jan 2025 21:32:12 +0100 Subject: [PATCH 2/5] MSan for constant flow: New build types CFMemSan, CFMemSanDbg New CMake build types CFMemSan, CFMemSanDbg to take care of differing compiler command lines with Clang <15 and Clang >=16. Signed-off-by: Gilles Peskine --- CMakeLists.txt | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 99e216945b..1ed06775cb 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -138,7 +138,7 @@ find_package(Threads) # 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 TSan TSanDbg" + CACHE STRING "Choose the type of build: None Debug Release Coverage ASan ASanDbg CFMemSan CFMemSanDbg MemSan MemSanDbg Check CheckFull TSan TSanDbg" FORCE) endif() @@ -263,6 +263,14 @@ if(CMAKE_COMPILER_IS_CLANG) set(CMAKE_C_FLAGS_TSAN "-fsanitize=thread -O3") set(CMAKE_C_FLAGS_TSANDBG "-fsanitize=thread -O1 -g3 -fno-omit-frame-pointer -fno-optimize-sibling-calls") 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_CFMEMSANDBG} -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) From 5dcb0f043fd85faae9107696d722345d7498e2dc Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 22 Jan 2025 21:33:29 +0100 Subject: [PATCH 3/5] MSan for constant flow: all.sh: support modern Clang Support Clang >=16 in component_test_memsan_constant_flow and component_test_memsan_constant_flow_psa. Modern versions of Clang require an extra compiler option to avoid false positives. Fixes #9921. Signed-off-by: Gilles Peskine --- tests/scripts/components-sanitizers.sh | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/tests/scripts/components-sanitizers.sh b/tests/scripts/components-sanitizers.sh index 6f97096efb..1f34b575a3 100644 --- a/tests/scripts/components-sanitizers.sh +++ b/tests/scripts/components-sanitizers.sh @@ -33,17 +33,16 @@ skip_all_except_given_suite () { 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 minus MBEDTLS_USE_PSA_CRYPTO with constant flow testing" scripts/config.py full - scripts/config.py set MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN scripts/config.py unset MBEDTLS_USE_PSA_CRYPTO scripts/config.py unset MBEDTLS_AESNI_C # memsan doesn't grok asm scripts/config.py unset MBEDTLS_HAVE_ASM - CC=clang cmake -D CMAKE_BUILD_TYPE:String=MemSan . + CC=clang cmake -D CMAKE_BUILD_TYPE:String=CFMemSan . make msg "test: main suites (full minus MBEDTLS_USE_PSA_CRYPTO, Msan + constant flow)" @@ -53,16 +52,15 @@ component_test_memsan_constant_flow () { component_test_memsan_constant_flow_psa () { # 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 scripts/config.py unset MBEDTLS_HAVE_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)" From a95097f04617581d6d754cc4ae511291e30e995b Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 22 Jan 2025 22:06:54 +0100 Subject: [PATCH 4/5] Add constant flow sanitizer sanity checks Add some basic checks of constant flow sanitizers. In particular, detect the specific way in which Clang 16 broke our constant-flow testing (by default, "uninitialized" values may not be passed to or returned from functions). Signed-off-by: Gilles Peskine --- tests/suites/test_suite_constant_time.data | 27 ++++++ .../suites/test_suite_constant_time.function | 90 +++++++++++++++++++ 2 files changed, 117 insertions(+) diff --git a/tests/suites/test_suite_constant_time.data b/tests/suites/test_suite_constant_time.data index 111fef6c4e..798768e072 100644 --- a/tests/suites/test_suite_constant_time.data +++ b/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 mbedtls_ct_memcpy_offset:0:5:10 diff --git a/tests/suites/test_suite_constant_time.function b/tests/suites/test_suite_constant_time.function index 2f985ba94b..d19eda21ed 100644 --- a/tests/suites/test_suite_constant_time.function +++ b/tests/suites/test_suite_constant_time.function @@ -19,8 +19,98 @@ #include #include + +/* 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 */ void mbedtls_ct_memcmp_null() { From cb96ae0b68d4f8233b9232f42c440a09ee5667a8 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 29 Jan 2025 11:19:17 +0100 Subject: [PATCH 5/5] Fix copypasta Signed-off-by: Gilles Peskine --- CMakeLists.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 1ed06775cb..b46d8d01af 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -265,7 +265,7 @@ if(CMAKE_COMPILER_IS_CLANG) 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_CFMEMSANDBG} -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")