From cd1460b9924de59dd6c636ed44c3b98e801effcb Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 22 Jan 2025 21:21:20 +0100 Subject: [PATCH 1/4] 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/psa/crypto_config.h | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/include/psa/crypto_config.h b/include/psa/crypto_config.h index 01d43c18a..0c61f8a6e 100644 --- a/include/psa/crypto_config.h +++ b/include/psa/crypto_config.h @@ -636,8 +636,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 bf15a648f293308dc4b50679000450cf5be49067 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 22 Jan 2025 21:32:12 +0100 Subject: [PATCH 2/4] 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 | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 63b10db5f..7f25fc3c1 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -175,7 +175,7 @@ find_package(Threads) # If this is the root project add longer list of available CMAKE_BUILD_TYPE values if(NOT TF_PSA_CRYPTO_AS_SUBPROJECT) 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() @@ -318,10 +318,23 @@ function(set_clang_base_compile_options target) set_target_properties(${target} PROPERTIES LINK_FLAGS_ASAN "-fsanitize=address -fsanitize=undefined") target_compile_options(${target} PRIVATE $<$:-fsanitize=address -fno-common -fsanitize=undefined -fno-sanitize-recover=all -O1 -g3 -fno-omit-frame-pointer -fno-optimize-sibling-calls>) set_target_properties(${target} PROPERTIES LINK_FLAGS_ASANDBG "-fsanitize=address -fsanitize=undefined") + + set(sanitize_memory_debug_flags "-O1 -g3 -fno-omit-frame-pointer -fno-optimize-sibling-calls -fsanitize-memory-track-origins=2") target_compile_options(${target} PRIVATE $<$:-fsanitize=memory>) set_target_properties(${target} PROPERTIES LINK_FLAGS_MEMSAN "-fsanitize=memory") - target_compile_options(${target} PRIVATE $<$:-fsanitize=memory -O1 -g3 -fno-omit-frame-pointer -fno-optimize-sibling-calls -fsanitize-memory-track-origins=2>) + target_compile_options(${target} PRIVATE $<$:-fsanitize=memory ${sanitize_memory_debug_flags}>) set_target_properties(${target} PROPERTIES LINK_FLAGS_MEMSANDBG "-fsanitize=memory") + + set(cf_sanitize_memory_flags -fsanitize=memory -DMBEDTLS_TEST_CONSTANT_FLOW_MEMSAN) + if(CMAKE_C_COMPILER_VERSION VERSION_GREATER 16.0.0) + set(cf_sanitize_memory_flags ${cf_sanitize_memory_flags} -fno-sanitize-memory-param-retval) + endif() + string(REPLACE ";" " " cf_sanitize_memory_flags_joined "${cf_sanitize_memory_flags}") + target_compile_options(${target} PRIVATE $<$:${cf_sanitize_memory_flags}>) + set_target_properties(${target} PROPERTIES LINK_FLAGS_CFMEMSAN "${cf_sanitize_memory_flags_joined}") + target_compile_options(${target} PRIVATE $<$:${cf_sanitize_memory_flags} ${sanitize_memory_debug_flags}>) + set_target_properties(${target} PROPERTIES LINK_FLAGS_CFMEMSANDBG "${cf_sanitize_memory_flags_joined}") + target_compile_options(${target} PRIVATE $<$:-fsanitize=thread -O3>) set_target_properties(${target} PROPERTIES LINK_FLAGS_TSAN "-fsanitize=thread") target_compile_options(${target} PRIVATE $<$:-fsanitize=thread -O1 -g3 -fno-omit-frame-pointer -fno-optimize-sibling-calls>) From a5cfdb99126de40c8f34c11e0f571812c22c24ef Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 22 Jan 2025 22:06:54 +0100 Subject: [PATCH 3/4] 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 111fef6c4..798768e07 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 64c815f43..1ef0702a4 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 f9d94932e046a5b5a49ac9f39565294eb856809b Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 22 Jan 2025 21:33:29 +0100 Subject: [PATCH 4/4] MSan for constant flow: all.sh: support modern Clang Support Clang >=16 in 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 | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/tests/scripts/components-sanitizers.sh b/tests/scripts/components-sanitizers.sh index b81b12ea0..2fc311498 100644 --- a/tests/scripts/components-sanitizers.sh +++ b/tests/scripts/components-sanitizers.sh @@ -33,8 +33,8 @@ skip_all_except_given_suite () { component_tf_psa_crypto_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" @@ -45,8 +45,7 @@ component_tf_psa_crypto_test_memsan_constant_flow_psa () { cmake -DCMAKE_C_COMPILER=clang -DGEN_FILES=ON "$TF_PSA_CRYPTO_ROOT_DIR" make - $TF_PSA_CRYPTO_ROOT_DIR/scripts/config.py set MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN - cmake -DCMAKE_C_COMPILER=clang -DGEN_FILES=OFF -DCMAKE_BUILD_TYPE:String=MemSan "$TF_PSA_CRYPTO_ROOT_DIR" + cmake -DCMAKE_C_COMPILER=clang -DGEN_FILES=OFF -DCMAKE_BUILD_TYPE:String=CFMemSan "$TF_PSA_CRYPTO_ROOT_DIR" make msg "test: main suites (Msan + constant flow)"