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