diff --git a/include/mbedtls/config.h b/include/mbedtls/config.h index ffd4b5b6b..1e6e05275 100644 --- a/include/mbedtls/config.h +++ b/include/mbedtls/config.h @@ -1911,9 +1911,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. @@ -1922,6 +1923,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_HOOKS * diff --git a/library/ssl_invasive.h b/library/ssl_invasive.h index cfe9b8d44..babbc2768 100644 --- a/library/ssl_invasive.h +++ b/library/ssl_invasive.h @@ -71,6 +71,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_TEST_HOOKS && MBEDTLS_SSL_SOME_SUITES_USE_TLS_CBC */ #endif /* MBEDTLS_SSL_INVASIVE_H */ diff --git a/library/ssl_msg.c b/library/ssl_msg.c index c1552e794..357826509 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -312,27 +312,6 @@ int (*mbedtls_ssl_hw_record_read)( mbedtls_ssl_context *ssl ) = NULL; int (*mbedtls_ssl_hw_record_finish)( mbedtls_ssl_context *ssl ) = NULL; #endif /* MBEDTLS_SSL_HW_RECORD_ACCEL */ -/* The function below is only used in the Lucky 13 counter-measure in - * mbedtls_ssl_decrypt_buf(). These are the defines that guard the call site. */ -#if defined(MBEDTLS_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 */ @@ -1191,6 +1170,27 @@ 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. + */ +MBEDTLS_STATIC_TESTABLE 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 */ int mbedtls_ssl_decrypt_buf( mbedtls_ssl_context const *ssl, @@ -1635,6 +1635,7 @@ int mbedtls_ssl_decrypt_buf( mbedtls_ssl_context const *ssl, if( auth_done == 0 ) { unsigned char mac_expect[MBEDTLS_SSL_MAC_ADD]; + unsigned char mac_peer[MBEDTLS_SSL_MAC_ADD]; /* If the initial value of padlen was such that * data_len < maclen + padlen + 1, then padlen @@ -1661,6 +1662,7 @@ int mbedtls_ssl_decrypt_buf( mbedtls_ssl_context const *ssl, data, rec->data_len, rec->ctr, rec->type, mac_expect ); + memcpy( mac_peer, data + rec->data_len, transform->maclen ); } else #endif /* MBEDTLS_SSL_PROTO_SSL3 */ @@ -1670,7 +1672,7 @@ int mbedtls_ssl_decrypt_buf( mbedtls_ssl_context const *ssl, { /* * The next two sizes are the minimum and maximum values of - * in_msglen over all padlen values. + * data_len over all padlen values. * * They're independent of padlen, since we previously did * data_len -= padlen. @@ -1691,12 +1693,10 @@ int mbedtls_ssl_decrypt_buf( mbedtls_ssl_context const *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( data + min_len, - max_len - min_len + transform->maclen ); + mbedtls_ssl_cf_memcpy_offset( mac_peer, data, + rec->data_len, + min_len, max_len, + transform->maclen ); } else #endif /* MBEDTLS_SSL_PROTO_TLS1 || MBEDTLS_SSL_PROTO_TLS1_1 || \ @@ -1708,10 +1708,10 @@ int mbedtls_ssl_decrypt_buf( mbedtls_ssl_context const *ssl, #if defined(MBEDTLS_SSL_DEBUG_ALL) MBEDTLS_SSL_DEBUG_BUF( 4, "expected mac", mac_expect, transform->maclen ); - MBEDTLS_SSL_DEBUG_BUF( 4, "message mac", data + rec->data_len, transform->maclen ); + MBEDTLS_SSL_DEBUG_BUF( 4, "message mac", mac_peer, transform->maclen ); #endif - if( mbedtls_ssl_safer_memcmp( data + rec->data_len, mac_expect, + if( mbedtls_ssl_safer_memcmp( mac_peer, mac_expect, transform->maclen ) != 0 ) { #if defined(MBEDTLS_SSL_DEBUG_ALL) diff --git a/library/version_features.c b/library/version_features.c index 36ae2f2df..d2840fa3c 100644 --- a/library/version_features.c +++ b/library/version_features.c @@ -558,6 +558,9 @@ static const char * const 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_HOOKS) "MBEDTLS_TEST_HOOKS", #endif /* MBEDTLS_TEST_HOOKS */ diff --git a/programs/test/query_config.c b/programs/test/query_config.c index cabf68b93..f4c14d6cb 100644 --- a/programs/test/query_config.c +++ b/programs/test/query_config.c @@ -1544,6 +1544,14 @@ int query_config( const char *config ) } #endif /* MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN */ +#if defined(MBEDTLS_TEST_CONSTANT_FLOW_VALGRIND) + if( strcmp( "MBEDTLS_TEST_CONSTANT_FLOW_VALGRIND", config ) == 0 ) + { + MACRO_EXPANSION_TO_STR( MBEDTLS_TEST_CONSTANT_FLOW_VALGRIND ); + return( 0 ); + } +#endif /* MBEDTLS_TEST_CONSTANT_FLOW_VALGRIND */ + #if defined(MBEDTLS_TEST_HOOKS) if( strcmp( "MBEDTLS_TEST_HOOKS", config ) == 0 ) { diff --git a/scripts/config.py b/scripts/config.py index 0a186476e..703e6e906 100755 --- a/scripts/config.py +++ b/scripts/config.py @@ -193,6 +193,7 @@ EXCLUDE_FROM_FULL = frozenset([ 'MBEDTLS_SHA512_NO_SHA384', # removes a feature 'MBEDTLS_SSL_HW_RECORD_ACCEL', # build dependency (hook functions) 'MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN', # build dependency (clang+memsan) + 'MBEDTLS_TEST_CONSTANT_FLOW_VALGRIND', # build dependency (valgrind headers) 'MBEDTLS_TEST_NULL_ENTROPY', # removes a feature 'MBEDTLS_X509_ALLOW_UNSUPPORTED_CRITICAL_EXTENSION', # influences the use of X.509 in TLS 'MBEDTLS_ZLIB_SUPPORT', # build dependency (libz) diff --git a/tests/include/test/constant_flow.h b/tests/include/test/constant_flow.h index 18daa4cc1..af6401166 100644 --- a/tests/include/test/constant_flow.h +++ b/tests/include/test/constant_flow.h @@ -30,6 +30,28 @@ #include MBEDTLS_CONFIG_FILE #endif +/* + * This file defines 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 : 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 @@ -39,11 +61,21 @@ #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 + +#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) -#endif /* MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN */ +#endif /* MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN || + MBEDTLS_TEST_CONSTANT_FLOW_VALGRIND */ #endif /* TEST_CONSTANT_FLOW_H */ diff --git a/tests/scripts/all.sh b/tests/scripts/all.sh index 082640aa1..0356923e2 100755 --- a/tests/scripts/all.sh +++ b/tests/scripts/all.sh @@ -1109,6 +1109,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.py full + scripts/config.py 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//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. diff --git a/tests/suites/test_suite_ssl.data b/tests/suites/test_suite_ssl.data index 3afa39a2b..1b7919104 100644 --- a/tests/suites/test_suite_ssl.data +++ b/tests/suites/test_suite_ssl.data @@ -10545,3 +10545,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 255-bytes plaintext and untruncated SHA-384 +Constant-flow memcpy from offset: large +ssl_cf_memcpy_offset:100:339:48 diff --git a/tests/suites/test_suite_ssl.function b/tests/suites/test_suite_ssl.function index f7c9be051..7c4f865e9 100644 --- a/tests/suites/test_suite_ssl.function +++ b/tests/suites/test_suite_ssl.function @@ -4361,3 +4361,36 @@ exit: mbedtls_free( out ); } /* 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 ) +{ + unsigned char *dst = NULL; + unsigned char *src = NULL; + size_t src_len = offset_max + len; + size_t secret; + + ASSERT_ALLOC( dst, len ); + ASSERT_ALLOC( src, src_len ); + + /* Fill src in a way that we can detect if we copied the right bytes */ + mbedtls_test_rnd_std_rand( NULL, src, src_len ); + + for( secret = offset_min; secret <= (size_t) offset_max; secret++ ) + { + test_set_step( (int) 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 ); + + ASSERT_COMPARE( dst, len, src + secret, len ); + } + +exit: + mbedtls_free( dst ); + mbedtls_free( src ); +} +/* END_CASE */ diff --git a/tests/suites/test_suite_x509write.function b/tests/suites/test_suite_x509write.function index be9e0ae52..31d60009d 100644 --- a/tests/suites/test_suite_x509write.function +++ b/tests/suites/test_suite_x509write.function @@ -5,11 +5,25 @@ #include "mbedtls/pem.h" #include "mbedtls/oid.h" #include "mbedtls/rsa.h" -#if defined(MBEDTLS_USE_PSA_CRYPTO) + +/* These are the same depends as the test function x509_crs_check_opaque(), + * the only function using PSA here. Using a weaker condition would result in + * warnings about the static functions defined in psa_crypto_helpers.h being + * unused. */ +#if defined(MBEDTLS_USE_PSA_CRYPTO) && \ + defined(MBEDTLS_PEM_WRITE_C) && \ + defined(MBEDTLS_X509_CSR_WRITE_C) #include "psa/crypto.h" #include "mbedtls/psa_util.h" -#endif - +#include "test/psa_crypto_helpers.h" +#define PSA_INIT( ) PSA_ASSERT( psa_crypto_init( ) ) +#else +/* Define empty macros so that we can use them in the preamble and teardown + * of every test function that uses PSA conditionally based on + * MBEDTLS_USE_PSA_CRYPTO. */ +#define PSA_INIT( ) ( (void) 0 ) +#define PSA_DONE( ) ( (void) 0 ) +#endif /* MBEDTLS_USE_PSA_CRYPTO && MBEDTLS_PEM_WRITE_C && MBEDTLS_X509_CSR_WRITE_C */ #if defined(MBEDTLS_RSA_C) int mbedtls_rsa_decrypt_func( void *ctx, int mode, size_t *olen, @@ -147,7 +161,7 @@ void x509_csr_check_opaque( char *key_file, int md_type, int key_usage, int cert_type ) { mbedtls_pk_context key; - psa_key_handle_t slot; + psa_key_handle_t slot = 0; psa_algorithm_t md_alg_psa; mbedtls_x509write_csr req; unsigned char buf[4096]; @@ -156,7 +170,7 @@ void x509_csr_check_opaque( char *key_file, int md_type, int key_usage, const char *subject_name = "C=NL,O=PolarSSL,CN=PolarSSL Server 1"; mbedtls_test_rnd_pseudo_info rnd_info; - psa_crypto_init(); + PSA_INIT( ); memset( &rnd_info, 0x2a, sizeof( mbedtls_test_rnd_pseudo_info ) ); md_alg_psa = mbedtls_psa_translate_md( (mbedtls_md_type_t) md_type ); @@ -184,9 +198,12 @@ void x509_csr_check_opaque( char *key_file, int md_type, int key_usage, buf[pem_len] = '\0'; TEST_ASSERT( x509_crt_verifycsr( buf, pem_len + 1 ) == 0 ); + exit: mbedtls_x509write_csr_free( &req ); mbedtls_pk_free( &key ); + psa_destroy_key( slot ); + PSA_DONE( ); } /* END_CASE */