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] 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 */