From ce45d1a75949644b68bcae18b3f3115b99c68098 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 25 Aug 2020 11:07:25 +0200 Subject: [PATCH 1/5] 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_tls.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 1b4c38050..05176ba2e 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -2319,6 +2319,7 @@ static int ssl_decrypt_buf( mbedtls_ssl_context *ssl ) if( auth_done == 0 ) { unsigned char mac_expect[MBEDTLS_SSL_MAC_ADD]; + unsigned char mac_peer[MBEDTLS_SSL_MAC_ADD]; ssl->in_msglen -= ssl->transform_in->maclen; @@ -2333,6 +2334,8 @@ static int ssl_decrypt_buf( mbedtls_ssl_context *ssl ) ssl->in_msg, ssl->in_msglen, ssl->in_ctr, ssl->in_msgtype, mac_expect ); + memcpy( mac_peer, ssl->in_msg + ssl->in_msglen, + ssl->transform_in->maclen ); } else #endif /* MBEDTLS_SSL_PROTO_SSL3 */ @@ -2377,6 +2380,8 @@ static int ssl_decrypt_buf( mbedtls_ssl_context *ssl ) * attacks much tighter and hopefully impractical. */ ssl_read_memory( ssl->in_msg + min_len, max_len - min_len + ssl->transform_in->maclen ); + memcpy( mac_peer, ssl->in_msg + ssl->in_msglen, + ssl->transform_in->maclen ); } else #endif /* MBEDTLS_SSL_PROTO_TLS1 || MBEDTLS_SSL_PROTO_TLS1_1 || \ @@ -2388,11 +2393,10 @@ static int ssl_decrypt_buf( mbedtls_ssl_context *ssl ) #if defined(MBEDTLS_SSL_DEBUG_ALL) MBEDTLS_SSL_DEBUG_BUF( 4, "expected mac", mac_expect, ssl->transform_in->maclen ); - MBEDTLS_SSL_DEBUG_BUF( 4, "message mac", ssl->in_msg + ssl->in_msglen, - ssl->transform_in->maclen ); + MBEDTLS_SSL_DEBUG_BUF( 4, "message mac", mac_peer, ssl->transform_in->maclen ); #endif - if( mbedtls_ssl_safer_memcmp( ssl->in_msg + ssl->in_msglen, mac_expect, + if( mbedtls_ssl_safer_memcmp( mac_peer, mac_expect, ssl->transform_in->maclen ) != 0 ) { #if defined(MBEDTLS_SSL_DEBUG_ALL) From 590b2d96140d866da37bcd2177a45ed790ff4984 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 25 Aug 2020 11:18:11 +0200 Subject: [PATCH 2/5] 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 --- include/mbedtls/ssl_internal.h | 24 ++++++++++++++++++++ library/ssl_tls.c | 29 +++++++++++++++++------- tests/suites/test_suite_ssl.data | 12 ++++++++++ tests/suites/test_suite_ssl.function | 33 ++++++++++++++++++++++++++++ 4 files changed, 90 insertions(+), 8 deletions(-) diff --git a/include/mbedtls/ssl_internal.h b/include/mbedtls/ssl_internal.h index 3c4df4764..6ba6c2af0 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -900,6 +900,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_SSL_SOME_SUITES_USE_TLS_CBC */ #ifdef __cplusplus diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 05176ba2e..0cc6c30b1 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1931,6 +1931,23 @@ 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. + */ +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 */ static int ssl_decrypt_buf( mbedtls_ssl_context *ssl ) @@ -2374,14 +2391,10 @@ static int ssl_decrypt_buf( mbedtls_ssl_context *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( ssl->in_msg + min_len, - max_len - min_len + ssl->transform_in->maclen ); - memcpy( mac_peer, ssl->in_msg + ssl->in_msglen, - ssl->transform_in->maclen ); + mbedtls_ssl_cf_memcpy_offset( mac_peer, ssl->in_msg, + ssl->in_msglen, + min_len, max_len, + ssl->transform_in->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 db92a72bc..66f6b8492 100644 --- a/tests/suites/test_suite_ssl.data +++ b/tests/suites/test_suite_ssl.data @@ -73,3 +73,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 355-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 cf807b545..c1f7d0de3 100644 --- a/tests/suites/test_suite_ssl.function +++ b/tests/suites/test_suite_ssl.function @@ -144,3 +144,36 @@ exit: mbedtls_free( out ); } /* END_CASE */ + +/* BEGIN_CASE depends_on:MBEDTLS_SSL_SOME_SUITES_USE_TLS_CBC */ +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; + + dst = mbedtls_calloc( 1, len ); + TEST_ASSERT( dst != NULL ); + src = mbedtls_calloc( 1, src_len ); + TEST_ASSERT( src != NULL ); + + /* Fill src in a way that we can detect if we copied the right bytes */ + rnd_std_rand( NULL, src, src_len ); + + for( secret = offset_min; secret <= (size_t) offset_max; 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 ); + + TEST_ASSERT( memcmp( dst, src + secret, len ) == 0 ); + } + +exit: + mbedtls_free( dst ); + mbedtls_free( src ); +} +/* END_CASE */ From f08284769d770ea9ed2840c4d66d455b6877435a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 25 Aug 2020 11:26:37 +0200 Subject: [PATCH 3/5] Add an 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/ssl/query_config.c | 8 ++++++++ scripts/config.pl | 1 + tests/scripts/all.sh | 22 ++++++++++++++++++++++ tests/suites/helpers.function | 32 +++++++++++++++++++++++++++++++- 6 files changed, 82 insertions(+), 1 deletion(-) diff --git a/include/mbedtls/config.h b/include/mbedtls/config.h index 751fa0a72..a00eec5e4 100644 --- a/include/mbedtls/config.h +++ b/include/mbedtls/config.h @@ -565,6 +565,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_NULL_ENTROPY * diff --git a/library/version_features.c b/library/version_features.c index de14a2ff6..cbf38dc2c 100644 --- a/library/version_features.c +++ b/library/version_features.c @@ -280,6 +280,9 @@ static const char *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_NULL_ENTROPY) "MBEDTLS_TEST_NULL_ENTROPY", #endif /* MBEDTLS_TEST_NULL_ENTROPY */ diff --git a/programs/ssl/query_config.c b/programs/ssl/query_config.c index 7c8fbd8cd..798c9178f 100644 --- a/programs/ssl/query_config.c +++ b/programs/ssl/query_config.c @@ -747,6 +747,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_NULL_ENTROPY) if( strcmp( "MBEDTLS_TEST_NULL_ENTROPY", config ) == 0 ) { diff --git a/scripts/config.pl b/scripts/config.pl index 1cf143506..e5cc69756 100755 --- a/scripts/config.pl +++ b/scripts/config.pl @@ -127,6 +127,7 @@ MBEDTLS_REMOVE_ARC4_CIPHERSUITES MBEDTLS_RSA_NO_CRT MBEDTLS_SSL_HW_RECORD_ACCEL MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN +MBEDTLS_TEST_CONSTANT_FLOW_VALGRIND MBEDTLS_TEST_NULL_ENTROPY MBEDTLS_X509_ALLOW_UNSUPPORTED_CRITICAL_EXTENSION MBEDTLS_ZLIB_SUPPORT diff --git a/tests/scripts/all.sh b/tests/scripts/all.sh index 8317cabc4..83299c97f 100755 --- a/tests/scripts/all.sh +++ b/tests/scripts/all.sh @@ -1047,6 +1047,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.pl full + scripts/config.pl 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/helpers.function b/tests/suites/helpers.function index 0b3d44b66..4f46ea967 100644 --- a/tests/suites/helpers.function +++ b/tests/suites/helpers.function @@ -46,6 +46,27 @@ typedef UINT32 uint32_t; #include #endif +/* + * Define 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 @@ -55,7 +76,16 @@ typedef UINT32 uint32_t; #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) From 46f49a8bbc5c5e1a97cfb245b667acbf2327444e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 25 Aug 2020 11:42:21 +0200 Subject: [PATCH 4/5] 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 a00eec5e4..90a9aaafc 100644 --- a/include/mbedtls/config.h +++ b/include/mbedtls/config.h @@ -554,9 +554,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. @@ -570,10 +571,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 ab9ec3287900c82f9a02e034301cf5a1b74ccb4a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 25 Aug 2020 11:43:10 +0200 Subject: [PATCH 5/5] 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_tls.c | 34 ++++++++-------------------------- 1 file changed, 8 insertions(+), 26 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 0cc6c30b1..2471600c9 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1435,27 +1435,6 @@ static void ssl_mac( mbedtls_md_context_t *md_ctx, #define SSL_SOME_MODES_USE_MAC #endif -/* The function below is only used in the Lucky 13 counter-measure in - * ssl_decrypt_buf(). These are the defines that guard the call site. */ -#if defined(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 */ @@ -1935,7 +1914,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. */ void mbedtls_ssl_cf_memcpy_offset( unsigned char *dst, const unsigned char *src_base, @@ -1943,10 +1922,13 @@ void mbedtls_ssl_cf_memcpy_offset( unsigned char *dst, 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 */