From 3c31afaca685fc7182b8d61433e69e423a0ef68a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Thu, 13 Aug 2020 12:08:54 +0200 Subject: [PATCH 1/9] Use temporary buffer to hold the peer's HMAC MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This paves the way for a constant-flow implementation of HMAC checking, by making sure that the comparison happens at a constant address. The missing step is obviously to copy the HMAC from the secret offset to this temporary buffer with constant flow, which will be done in the next few commits. Signed-off-by: Manuel Pégourié-Gonnard --- library/ssl_msg.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/library/ssl_msg.c b/library/ssl_msg.c index c4cdf21ba..e90e59993 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -1637,6 +1637,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 @@ -1663,6 +1664,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 */ @@ -1699,6 +1701,7 @@ int mbedtls_ssl_decrypt_buf( mbedtls_ssl_context const *ssl, * attacks much tighter and hopefully impractical. */ ssl_read_memory( data + min_len, max_len - min_len + transform->maclen ); + memcpy( mac_peer, data + rec->data_len, transform->maclen ); } else #endif /* MBEDTLS_SSL_PROTO_TLS1 || MBEDTLS_SSL_PROTO_TLS1_1 || \ @@ -1710,10 +1713,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) From 7fe2c5f086770a0f90ec5c15cc85880f3751568d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 18 Aug 2020 12:02:54 +0200 Subject: [PATCH 2/9] Add mbedtls_ssl_cf_memcpy_offset() with tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The tests are supposed to be failing now (in all.sh component test_memsan_constant_flow), but they don't as apparently MemSan doesn't complain when the src argument of memcpy() is uninitialized, see https://github.com/google/sanitizers/issues/1296 The next commit will add an option to test constant flow with valgrind, which will hopefully correctly flag the current non-constant-flow implementation. Signed-off-by: Manuel Pégourié-Gonnard --- library/ssl_invasive.h | 24 ++++++++++++++++++++ library/ssl_msg.c | 31 +++++++++++++++++++------- tests/suites/test_suite_ssl.data | 12 ++++++++++ tests/suites/test_suite_ssl.function | 33 ++++++++++++++++++++++++++++ 4 files changed, 92 insertions(+), 8 deletions(-) diff --git a/library/ssl_invasive.h b/library/ssl_invasive.h index f04b81668..60b62094e 100644 --- a/library/ssl_invasive.h +++ b/library/ssl_invasive.h @@ -73,6 +73,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 e90e59993..5dc8012ba 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -1193,6 +1193,24 @@ 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 independant 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 ) +{ + /* WIP - THIS IS NOT ACTUALLY CONSTANT-FLOW! + * This is just to be able to write tests and check they work. */ + ssl_read_memory( src_base + offset_min, offset_max - offset_min + len ); + memcpy( dst, src_base + offset_secret, len ); +} #endif /* MBEDTLS_SSL_SOME_SUITES_USE_TLS_CBC */ int mbedtls_ssl_decrypt_buf( mbedtls_ssl_context const *ssl, @@ -1674,7 +1692,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. @@ -1695,13 +1713,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 ); - memcpy( mac_peer, data + rec->data_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 || \ 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 */ From feb0396d20593ea557835657d04f449bc52d4557 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Thu, 20 Aug 2020 09:59:33 +0200 Subject: [PATCH 3/9] Fix memory leak in test_suite_x509write with PSA crypto MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The documentation of mbedtls_pk_wrap_as_opaque is quite clear: * \param handle Output: a PSA key handle. * It's the caller's responsibility to call * psa_destroy_key() on that handle after calling * mbedtls_pk_free() on the PK context. But the test failed to call psa_destroy_key(). While at it, also use PSA_DONE(): it ensures that if we fail to destroy the key, we'll get an explicit error message about it without the need for valgrind. This is a preliminary to adding a valgrind-based test for constant-flow code: we need to make sure the rest of the tests are fully valgrind-clean, which they weren't. Signed-off-by: Manuel Pégourié-Gonnard --- tests/suites/test_suite_x509write.function | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/tests/suites/test_suite_x509write.function b/tests/suites/test_suite_x509write.function index be9e0ae52..b205b74d7 100644 --- a/tests/suites/test_suite_x509write.function +++ b/tests/suites/test_suite_x509write.function @@ -5,12 +5,20 @@ #include "mbedtls/pem.h" #include "mbedtls/oid.h" #include "mbedtls/rsa.h" + #if defined(MBEDTLS_USE_PSA_CRYPTO) #include "psa/crypto.h" #include "mbedtls/psa_util.h" +#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 - #if defined(MBEDTLS_RSA_C) int mbedtls_rsa_decrypt_func( void *ctx, int mode, size_t *olen, const unsigned char *input, unsigned char *output, @@ -156,7 +164,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 +192,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 */ From 73afa37507dbb6ec76fbed2e21a9305fbb1d6c09 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Wed, 19 Aug 2020 10:27:38 +0200 Subject: [PATCH 4/9] Add option to test constant-flow with valgrind MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Currently the new component in all.sh fails because mbedtls_ssl_cf_memcpy_offset() is not actually constant flow - this is on purpose to be able to verify that the new test works. Signed-off-by: Manuel Pégourié-Gonnard --- include/mbedtls/config.h | 17 ++++++++++++++ library/version_features.c | 3 +++ programs/test/query_config.c | 8 +++++++ scripts/config.py | 1 + tests/include/test/constant_flow.h | 36 ++++++++++++++++++++++++++++-- tests/scripts/all.sh | 22 ++++++++++++++++++ 6 files changed, 85 insertions(+), 2 deletions(-) diff --git a/include/mbedtls/config.h b/include/mbedtls/config.h index 5eceea1bc..6337258dc 100644 --- a/include/mbedtls/config.h +++ b/include/mbedtls/config.h @@ -1924,6 +1924,23 @@ */ //#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 + * non-functional properties 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. + * + * \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/version_features.c b/library/version_features.c index 2470d8d1d..16719a6eb 100644 --- a/library/version_features.c +++ b/library/version_features.c @@ -560,6 +560,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 23dc51512..c242ab511 100644 --- a/programs/test/query_config.c +++ b/programs/test/query_config.c @@ -1546,6 +1546,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 793e9dfa7..37a2475e6 100755 --- a/scripts/config.py +++ b/scripts/config.py @@ -195,6 +195,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 98bee7e48..9307a2513 100644 --- a/tests/include/test/constant_flow.h +++ b/tests/include/test/constant_flow.h @@ -32,6 +32,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 @@ -41,11 +63,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 6cc0cf806..caeb2fe79 100755 --- a/tests/scripts/all.sh +++ b/tests/scripts/all.sh @@ -1111,6 +1111,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. From de1cf2c5e1df1206f02633581eea3e86c3d7d985 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Wed, 19 Aug 2020 12:35:30 +0200 Subject: [PATCH 5/9] Make mbedtls_ssl_cf_memcpy_offset() constant-flow MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit all.sh component test_valgrind_constant_flow is now passing. Signed-off-by: Manuel Pégourié-Gonnard --- library/ssl_msg.c | 32 +++++++------------------------- 1 file changed, 7 insertions(+), 25 deletions(-) diff --git a/library/ssl_msg.c b/library/ssl_msg.c index 5dc8012ba..0de53204b 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -314,27 +314,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 */ @@ -1206,10 +1185,13 @@ MBEDTLS_STATIC_TESTABLE void mbedtls_ssl_cf_memcpy_offset( size_t offset_min, size_t offset_max, size_t len ) { - /* WIP - THIS IS NOT ACTUALLY CONSTANT-FLOW! - * This is just to be able to write tests and check they work. */ - ssl_read_memory( src_base + offset_min, offset_max - offset_min + len ); - memcpy( dst, src_base + offset_secret, 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 */ From 8a79b9b68c3e30bd9654fe74d0ee48ef2062f7a0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Mon, 24 Aug 2020 10:29:30 +0200 Subject: [PATCH 6/9] Fix "unused function" warning in some configs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- tests/suites/test_suite_x509write.function | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/tests/suites/test_suite_x509write.function b/tests/suites/test_suite_x509write.function index b205b74d7..de6a1064b 100644 --- a/tests/suites/test_suite_x509write.function +++ b/tests/suites/test_suite_x509write.function @@ -6,7 +6,13 @@ #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" #include "test/psa_crypto_helpers.h" @@ -17,7 +23,7 @@ * MBEDTLS_USE_PSA_CRYPTO. */ #define PSA_INIT( ) ( (void) 0 ) #define PSA_DONE( ) ( (void) 0 ) -#endif +#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, From dd00bfce34434bf43ff4d7cb144cda7c146c3427 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Mon, 24 Aug 2020 12:58:36 +0200 Subject: [PATCH 7/9] Improve comments on constant-flow testing in config.h MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- include/mbedtls/config.h | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/include/mbedtls/config.h b/include/mbedtls/config.h index 6337258dc..ca9527dbf 100644 --- a/include/mbedtls/config.h +++ b/include/mbedtls/config.h @@ -1913,9 +1913,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. @@ -1929,10 +1930,12 @@ * * Enable testing of the constant-flow nature of some sensitive functions with * valgrind's memcheck tool. 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 valgrind headers for building, and is only useful for - * testing if the tests suites are run with valgrind's memcheck. + * 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. From ba6fc9796a9f4da88af8952a2d18053c7d8c6ba2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Mon, 24 Aug 2020 12:59:55 +0200 Subject: [PATCH 8/9] Fix a typo in a comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- library/ssl_msg.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/ssl_msg.c b/library/ssl_msg.c index 0de53204b..01822668c 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -1176,7 +1176,7 @@ cleanup: /* * Constant-flow memcpy from variable position in buffer. * - functionally equivalent to memcpy(dst, src + offset_secret, len) - * - but with execution flow independant from the value of offset_secret. + * - but with execution flow independent from the value of offset_secret. */ MBEDTLS_STATIC_TESTABLE void mbedtls_ssl_cf_memcpy_offset( unsigned char *dst, From 04b7488411ee602e986cdb07a217e59e9ae046bc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 25 Aug 2020 10:45:51 +0200 Subject: [PATCH 9/9] Fix potential use of uninitialised variable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If any of the TEST_ASSERT()s that are before the call to mbedtls_pk_warp_as_opaque() failed, when reaching the exit label psa_destroy_key() would be called with an uninitialized argument. Found by Clang. Signed-off-by: Manuel Pégourié-Gonnard --- tests/suites/test_suite_x509write.function | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/suites/test_suite_x509write.function b/tests/suites/test_suite_x509write.function index de6a1064b..31d60009d 100644 --- a/tests/suites/test_suite_x509write.function +++ b/tests/suites/test_suite_x509write.function @@ -161,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];