From a60d0f2acba7dbc677ec9016784d013683e0bc3c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 28 Jul 2020 09:55:33 +0200 Subject: [PATCH 01/19] Factor repeated preprocessor condition to a macro MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The condition is a complex and repeated a few times. There were already some inconsistencies in the repetitions as some of them forgot about DES. Signed-off-by: Manuel Pégourié-Gonnard --- include/mbedtls/ssl_internal.h | 9 +++++++++ library/ssl_tls.c | 15 +++++---------- 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/include/mbedtls/ssl_internal.h b/include/mbedtls/ssl_internal.h index b371094f..06188beb 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -152,6 +152,15 @@ #define MBEDTLS_SSL_RETRANS_WAITING 2 #define MBEDTLS_SSL_RETRANS_FINISHED 3 +/* For CBC-specific encrypt/decrypt code */ +#if defined(MBEDTLS_CIPHER_MODE_CBC) && \ + ( defined(MBEDTLS_AES_C) || \ + defined(MBEDTLS_CAMELLIA_C) || \ + defined(MBEDTLS_ARIA_C) || \ + defined(MBEDTLS_DES_C) ) +#define MBEDTLS_SSL_SOME_SUITES_USE_CBC +#endif + /* * Allow extra bytes for record, authentication and encryption overhead: * counter (8) + header (5) + IV(16) + MAC (16-48) + padding (0-256) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index a40b46a1..34485d2c 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1433,8 +1433,7 @@ static void ssl_mac( mbedtls_md_context_t *md_ctx, #endif /* MBEDTLS_SSL_PROTO_SSL3 */ #if defined(MBEDTLS_ARC4_C) || defined(MBEDTLS_CIPHER_NULL_CIPHER) || \ - ( defined(MBEDTLS_CIPHER_MODE_CBC) && \ - ( defined(MBEDTLS_AES_C) || defined(MBEDTLS_CAMELLIA_C) || defined(MBEDTLS_ARIA_C)) ) + defined(MBEDTLS_SSL_SOME_SUITES_USE_CBC) #define SSL_SOME_MODES_USE_MAC #endif @@ -1669,8 +1668,7 @@ static int ssl_encrypt_buf( mbedtls_ssl_context *ssl ) } else #endif /* MBEDTLS_GCM_C || MBEDTLS_CCM_C */ -#if defined(MBEDTLS_CIPHER_MODE_CBC) && \ - ( defined(MBEDTLS_AES_C) || defined(MBEDTLS_CAMELLIA_C) || defined(MBEDTLS_ARIA_C) ) +#if defined(MBEDTLS_SSL_SOME_SUITES_USE_CBC) if( mode == MBEDTLS_MODE_CBC ) { int ret; @@ -1789,8 +1787,7 @@ static int ssl_encrypt_buf( mbedtls_ssl_context *ssl ) #endif /* MBEDTLS_SSL_ENCRYPT_THEN_MAC */ } else -#endif /* MBEDTLS_CIPHER_MODE_CBC && - ( MBEDTLS_AES_C || MBEDTLS_CAMELLIA_C || MBEDTLS_ARIA_C ) */ +#endif /* MBEDTLS_SSL_SOME_SUITES_USE_CBC */ { MBEDTLS_SSL_DEBUG_MSG( 1, ( "should never happen" ) ); return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); @@ -1962,8 +1959,7 @@ static int ssl_decrypt_buf( mbedtls_ssl_context *ssl ) } else #endif /* MBEDTLS_GCM_C || MBEDTLS_CCM_C */ -#if defined(MBEDTLS_CIPHER_MODE_CBC) && \ - ( defined(MBEDTLS_AES_C) || defined(MBEDTLS_CAMELLIA_C) || defined(MBEDTLS_ARIA_C) ) +#if defined(MBEDTLS_SSL_SOME_SUITES_USE_CBC) if( mode == MBEDTLS_MODE_CBC ) { /* @@ -2176,8 +2172,7 @@ static int ssl_decrypt_buf( mbedtls_ssl_context *ssl ) ssl->in_msglen -= padlen; } else -#endif /* MBEDTLS_CIPHER_MODE_CBC && - ( MBEDTLS_AES_C || MBEDTLS_CAMELLIA_C || MBEDTLS_ARIA_C ) */ +#endif /* MBEDTLS_SSL_SOME_SUITES_USE_CBC) */ { MBEDTLS_SSL_DEBUG_MSG( 1, ( "should never happen" ) ); return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); From fde750550dc96b76be88d34ef27a4ed3e513279d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 28 Jul 2020 10:19:45 +0200 Subject: [PATCH 02/19] Add dummy constant-flow HMAC function with tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The dummy implementation is not constant-flow at all for now, it's just here as a starting point and a support for developing the tests and putting the infrastructure in place. Depending on the implementation strategy, there might be various corner cases depending on where the lengths fall relative to block boundaries. So it seems safer to just test all possible lengths in a given range than to use only a few randomly-chosen values. Signed-off-by: Manuel Pégourié-Gonnard --- include/mbedtls/ssl_internal.h | 44 +++++++++++++ library/ssl_tls.c | 26 ++++++++ tests/suites/test_suite_ssl.data | 16 +++++ tests/suites/test_suite_ssl.function | 93 ++++++++++++++++++++++++++++ 4 files changed, 179 insertions(+) diff --git a/include/mbedtls/ssl_internal.h b/include/mbedtls/ssl_internal.h index 06188beb..9ad04181 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -852,6 +852,50 @@ int mbedtls_ssl_get_key_exchange_md_tls1_2( mbedtls_ssl_context *ssl, #endif /* MBEDTLS_SSL_PROTO_TLS1 || MBEDTLS_SSL_PROTO_TLS1_1 || \ MBEDTLS_SSL_PROTO_TLS1_2 */ +#if defined(MBEDTLS_SSL_SOME_SUITES_USE_CBC) && \ + ( defined(MBEDTLS_SSL_PROTO_TLS1) || \ + defined(MBEDTLS_SSL_PROTO_TLS1_1) | \ + defined(MBEDTLS_SSL_PROTO_TLS1_2) ) +/** \brief Compute the HMAC of variable-length data with constant flow. + * + * This function computes the HMAC of the concatenation of \p add_data and \p + * data, and does with a code flow and memory access pattern that does not + * depend on \p data_len_secret, but only on \p min_data_len and \p + * max_data_len. In particular, this function always reads exactly \p + * max_data_len bytes from \p data. + * + * \param ctx The HMAC context. It must have keys configured + * with mbedtls_md_hmac_starts(). It is reset using + * mbedtls_md_hmac_reset() after the computation is + * complete to prepare for the next computation. + * \param add_data The additional data prepended to \p data. This + * must point to a readable buffer of \p add_data_len + * bytes. + * \param add_data_len The length of \p add_data in bytes. + * \param data The data appended to \p add_data. This must point + * to a readable buffer of \p max_data_len bytes. + * \param data_len_secret The length of the data to process in \p data. + * This must be no less than \p min_data_len and no + * greated than \p max_data_len. + * \param min_data_len The minimal length of \p data in bytes. + * \param max_data_len The maximal length of \p data in bytes. + * \param output The HMAC will be written here. This must point to + * a writeable buffer of sufficient size to hold the + * HMAC value. + * + * \retval 0 + * Success. + * \retval MBEDTLS_ERR_PLATFORM_HW_ACCEL_FAILED + * The hardware accelerator failed. + */ +int mbedtls_ssl_cf_hmac( + mbedtls_md_context_t *ctx, + const unsigned char *add_data, size_t add_data_len, + const unsigned char *data, size_t data_len_secret, + size_t min_data_len, size_t max_data_len, + unsigned char *output ); +#endif /* MBEDTLS_SSL_SOME_SUITES_USE_CBC && TLS 1.0-1.2 */ + #ifdef __cplusplus } #endif diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 34485d2c..8bc79be0 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1805,6 +1805,32 @@ static int ssl_encrypt_buf( mbedtls_ssl_context *ssl ) return( 0 ); } +#if defined(MBEDTLS_SSL_SOME_SUITES_USE_CBC) && \ + ( defined(MBEDTLS_SSL_PROTO_TLS1) || \ + defined(MBEDTLS_SSL_PROTO_TLS1_1) || \ + defined(MBEDTLS_SSL_PROTO_TLS1_2) ) +/* + * Compute HMAC of variable-length data with constant flow. + */ +int mbedtls_ssl_cf_hmac( + mbedtls_md_context_t *ctx, + const unsigned char *add_data, size_t add_data_len, + const unsigned char *data, size_t data_len_secret, + size_t min_data_len, size_t max_data_len, + unsigned char *output ) +{ + /* WORK IN PROGRESS - THIS IS NOT CONSTANT FLOW AT ALL */ + (void) min_data_len; + (void) max_data_len; + mbedtls_md_hmac_update( ctx, add_data, add_data_len ); + mbedtls_md_hmac_update( ctx, data, data_len_secret ); + mbedtls_md_hmac_finish( ctx, output ); + mbedtls_md_hmac_reset( ctx ); + + return( 0 ); +} +#endif /* MBEDTLS_SSL_SOME_SUITES_USE_CBC && TLS 1.0-1.2 */ + static int ssl_decrypt_buf( mbedtls_ssl_context *ssl ) { mbedtls_cipher_mode_t mode; diff --git a/tests/suites/test_suite_ssl.data b/tests/suites/test_suite_ssl.data index 14735074..db92a72b 100644 --- a/tests/suites/test_suite_ssl.data +++ b/tests/suites/test_suite_ssl.data @@ -57,3 +57,19 @@ ssl_dtls_replay:"abcd12340000abcd12340100":"abcd123400ff":0 SSL SET_HOSTNAME memory leak: call ssl_set_hostname twice ssl_set_hostname_twice:"server0":"server1" + +Constant-flow HMAC: MD5 +depends_on:MBEDTLS_MD5_C +ssl_cf_hmac:MBEDTLS_MD_MD5 + +Constant-flow HMAC: SHA1 +depends_on:MBEDTLS_SHA1_C +ssl_cf_hmac:MBEDTLS_MD_SHA1 + +Constant-flow HMAC: SHA256 +depends_on:MBEDTLS_SHA256_C +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 diff --git a/tests/suites/test_suite_ssl.function b/tests/suites/test_suite_ssl.function index 326f22d3..fc953a12 100644 --- a/tests/suites/test_suite_ssl.function +++ b/tests/suites/test_suite_ssl.function @@ -52,3 +52,96 @@ void ssl_set_hostname_twice( char *hostname0, char *hostname1 ) mbedtls_ssl_free( &ssl ); } /* END_CASE */ + +/* BEGIN_CASE depends_on:MBEDTLS_CIPHER_MODE_CBC:MBEDTLS_AES_C:MBEDTLS_SSL_PROTO_TLS1_2 */ +void ssl_cf_hmac( int hash ) +{ + /* + * Test the function mbedtls_ssl_cf_hmac() against a reference + * implementation. + * + * Note: the dependency is actually on TLS 1.0-1.2 and (AES or ARIA or + * Camellia or DES), but since the test framework doesn't support + * alternation in dependencies, just depend on the most common. + */ + mbedtls_md_context_t ctx, ref_ctx; + const mbedtls_md_info_t *md_info; + size_t out_len, block_size; + size_t min_in_len, in_len, max_in_len, i; + /* TLS additional data is 13 bytes (hence the "lucky 13" name) */ + unsigned char add_data[13]; + unsigned char ref_out[MBEDTLS_MD_MAX_SIZE]; + unsigned char *data = NULL; + unsigned char *out = NULL; + unsigned char rec_num = 0; + + mbedtls_md_init( &ctx ); + mbedtls_md_init( &ref_ctx ); + + md_info = mbedtls_md_info_from_type( hash ); + TEST_ASSERT( md_info != NULL ); + out_len = mbedtls_md_get_size( md_info ); + TEST_ASSERT( out_len != 0 ); + block_size = hash == MBEDTLS_MD_SHA384 ? 128 : 64; + + /* Use allocated out buffer to catch overwrites */ + out = mbedtls_calloc( 1, out_len ); + TEST_ASSERT( out != NULL ); + + /* Set up contexts with the given hash and a dummy key */ + TEST_ASSERT( 0 == mbedtls_md_setup( &ctx, md_info, 1 ) ); + TEST_ASSERT( 0 == mbedtls_md_setup( &ref_ctx, md_info, 1 ) ); + memset( ref_out, 42, sizeof( ref_out ) ); + TEST_ASSERT( 0 == mbedtls_md_hmac_starts( &ctx, ref_out, out_len ) ); + TEST_ASSERT( 0 == mbedtls_md_hmac_starts( &ref_ctx, ref_out, out_len ) ); + memset( ref_out, 0, sizeof( ref_out ) ); + + /* + * Test all possible lengths up to a point. The difference between + * max_in_len and min_in_len is at most 255, and make sure they both vary + * by at least one block size. + */ + for( max_in_len = 0; max_in_len <= 255 + block_size; max_in_len++ ) + { + /* Use allocated in buffer to catch overreads */ + data = mbedtls_calloc( 1, max_in_len ); + TEST_ASSERT( data != NULL || max_in_len == 0 ); + + min_in_len = max_in_len > 255 ? max_in_len - 255 : 0; + for( in_len = min_in_len; in_len <= max_in_len; in_len++ ) + { + /* Set up dummy data and add_data */ + rec_num++; + memset( add_data, rec_num, sizeof( add_data ) ); + for( i = 0; i < in_len; i++ ) + data[i] = ( i & 0xff ) ^ rec_num; + + /* Get the function's result */ + TEST_ASSERT( 0 == mbedtls_ssl_cf_hmac( &ctx, add_data, sizeof( add_data ), + data, in_len, + min_in_len, max_in_len, + out ) ); + + /* Compute the reference result */ + TEST_ASSERT( 0 == mbedtls_md_hmac_update( &ref_ctx, add_data, + sizeof( add_data ) ) ); + TEST_ASSERT( 0 == mbedtls_md_hmac_update( &ref_ctx, data, in_len ) ); + TEST_ASSERT( 0 == mbedtls_md_hmac_finish( &ref_ctx, ref_out ) ); + TEST_ASSERT( 0 == mbedtls_md_hmac_reset( &ref_ctx ) ); + + /* Compare */ + TEST_ASSERT( 0 == memcmp( out, ref_out, out_len ) ); + } + + mbedtls_free( data ); + data = NULL; + } + +exit: + mbedtls_md_free( &ref_ctx ); + mbedtls_md_free( &ctx ); + + mbedtls_free( data ); + mbedtls_free( out ); +} +/* END_CASE */ From 368fc65f80db347fa10849dbf3081969056a79c2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 28 Jul 2020 10:43:03 +0200 Subject: [PATCH 03/19] Use existing implementation of cf_hmac() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Just move code from ssl_decrypt_buf() to the new cf_hmac() function and then call cf_hmac() from there. This makes the new cf_hmac() function used and validates that its interface works for using it in ssl_decrypt_buf(). Signed-off-by: Manuel Pégourié-Gonnard --- library/ssl_tls.c | 172 ++++++++++++++++++++++++---------------------- 1 file changed, 89 insertions(+), 83 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 8bc79be0..fbf43ab8 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1445,7 +1445,7 @@ static void ssl_mac( mbedtls_md_context_t *md_ctx, 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( unsigned char *p, size_t len ) +static void ssl_read_memory( const unsigned char *p, size_t len ) { unsigned char acc = 0; volatile unsigned char force; @@ -1819,12 +1819,83 @@ int mbedtls_ssl_cf_hmac( size_t min_data_len, size_t max_data_len, unsigned char *output ) { - /* WORK IN PROGRESS - THIS IS NOT CONSTANT FLOW AT ALL */ - (void) min_data_len; - (void) max_data_len; + /* WORK IN PROGRESS - THIS IS ONLY PSEUDO-CONTANT-TIME */ + + /* + * Process MAC and always update for padlen afterwards to make + * total time independent of padlen. + * + * Known timing attacks: + * - Lucky Thirteen (http://www.isg.rhul.ac.uk/tls/TLStiming.pdf) + * + * To compensate for different timings for the MAC calculation + * depending on how much padding was removed (which is determined + * by padlen), process extra_run more blocks through the hash + * function. + * + * The formula in the paper is + * extra_run = ceil( (L1-55) / 64 ) - ceil( (L2-55) / 64 ) + * where L1 is the size of the header plus the decrypted message + * plus CBC padding and L2 is the size of the header plus the + * decrypted message. This is for an underlying hash function + * with 64-byte blocks. + * We use ( (Lx+8) / 64 ) to handle 'negative Lx' values + * correctly. We round down instead of up, so -56 is the correct + * value for our calculations instead of -55. + * + * Repeat the formula rather than defining a block_size variable. + * This avoids requiring division by a variable at runtime + * (which would be marginally less efficient and would require + * linking an extra division function in some builds). + */ + size_t j, extra_run = 0; + /* This size is enough to server either as input to + * md_process() or as output to md_finish() */ + unsigned char tmp[128]; + + memset( tmp, 0, sizeof( tmp ) ); + + switch( mbedtls_md_get_type( ctx->md_info ) ) + { +#if defined(MBEDTLS_MD5_C) || defined(MBEDTLS_SHA1_C) || \ +defined(MBEDTLS_SHA256_C) + case MBEDTLS_MD_MD5: + case MBEDTLS_MD_SHA1: + case MBEDTLS_MD_SHA256: + /* 8 bytes of message size, 64-byte compression blocks */ + extra_run = ( add_data_len + max_data_len + 8 ) / 64 - + ( add_data_len + data_len_secret + 8 ) / 64; + break; +#endif +#if defined(MBEDTLS_SHA512_C) + case MBEDTLS_MD_SHA384: + /* 16 bytes of message size, 128-byte compression blocks */ + extra_run = ( add_data_len + max_data_len + 16 ) / 128 - + ( add_data_len + data_len_secret + 16 ) / 128; + break; +#endif + default: + return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); + } + mbedtls_md_hmac_update( ctx, add_data, add_data_len ); mbedtls_md_hmac_update( ctx, data, data_len_secret ); + /* Make sure we access everything even when padlen > 0. This + * makes the synchronisation requirements for just-in-time + * Prime+Probe attacks much tighter and hopefully impractical. */ + ssl_read_memory( data + min_data_len, max_data_len - min_data_len ); mbedtls_md_hmac_finish( ctx, output ); + + /* Dummy calls to compression function. + * Call mbedtls_md_process at least once due to cache attacks + * that observe whether md_process() was called of not. + * Respect the usual start-(process|update)-finish sequence for + * the sake of hardware accelerators that might require it. */ + mbedtls_md_starts( ctx ); + for( j = 0; j < extra_run + 1; j++ ) + mbedtls_md_process( ctx, tmp ); + mbedtls_md_finish( ctx, tmp ); + mbedtls_md_hmac_reset( ctx ); return( 0 ); @@ -2238,34 +2309,8 @@ static int ssl_decrypt_buf( mbedtls_ssl_context *ssl ) defined(MBEDTLS_SSL_PROTO_TLS1_2) if( ssl->minor_ver > MBEDTLS_SSL_MINOR_VERSION_0 ) { - /* - * Process MAC and always update for padlen afterwards to make - * total time independent of padlen. - * - * Known timing attacks: - * - Lucky Thirteen (http://www.isg.rhul.ac.uk/tls/TLStiming.pdf) - * - * To compensate for different timings for the MAC calculation - * depending on how much padding was removed (which is determined - * by padlen), process extra_run more blocks through the hash - * function. - * - * The formula in the paper is - * extra_run = ceil( (L1-55) / 64 ) - ceil( (L2-55) / 64 ) - * where L1 is the size of the header plus the decrypted message - * plus CBC padding and L2 is the size of the header plus the - * decrypted message. This is for an underlying hash function - * with 64-byte blocks. - * We use ( (Lx+8) / 64 ) to handle 'negative Lx' values - * correctly. We round down instead of up, so -56 is the correct - * value for our calculations instead of -55. - * - * Repeat the formula rather than defining a block_size variable. - * This avoids requiring division by a variable at runtime - * (which would be marginally less efficient and would require - * linking an extra division function in some builds). - */ - size_t j, extra_run = 0; + int ret; + unsigned char add_data[13]; /* * The next two sizes are the minimum and maximum values of @@ -2280,60 +2325,21 @@ static int ssl_decrypt_buf( mbedtls_ssl_context *ssl ) const size_t max_len = ssl->in_msglen + padlen; const size_t min_len = ( max_len > 256 ) ? max_len - 256 : 0; - switch( ssl->transform_in->ciphersuite_info->mac ) + memcpy( add_data + 0, ssl->in_ctr, 8 ); + memcpy( add_data + 8, ssl->in_hdr, 3 ); + memcpy( add_data + 11, ssl->in_len, 2 ); + + ret = mbedtls_ssl_cf_hmac( &ssl->transform_in->md_ctx_dec, + add_data, sizeof( add_data ), + ssl->in_msg, ssl->in_msglen, + min_len, max_len, + mac_expect ); + if( ret != 0 ) { -#if defined(MBEDTLS_MD5_C) || defined(MBEDTLS_SHA1_C) || \ - defined(MBEDTLS_SHA256_C) - case MBEDTLS_MD_MD5: - case MBEDTLS_MD_SHA1: - case MBEDTLS_MD_SHA256: - /* 8 bytes of message size, 64-byte compression blocks */ - extra_run = ( 13 + ssl->in_msglen + padlen + 8 ) / 64 - - ( 13 + ssl->in_msglen + 8 ) / 64; - break; -#endif -#if defined(MBEDTLS_SHA512_C) - case MBEDTLS_MD_SHA384: - /* 16 bytes of message size, 128-byte compression blocks */ - extra_run = ( 13 + ssl->in_msglen + padlen + 16 ) / 128 - - ( 13 + ssl->in_msglen + 16 ) / 128; - break; -#endif - default: - MBEDTLS_SSL_DEBUG_MSG( 1, ( "should never happen" ) ); - return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); + MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_ssl_cf_hmac", ret ); + return( ret ); } - extra_run &= correct * 0xFF; - - mbedtls_md_hmac_update( &ssl->transform_in->md_ctx_dec, ssl->in_ctr, 8 ); - mbedtls_md_hmac_update( &ssl->transform_in->md_ctx_dec, ssl->in_hdr, 3 ); - mbedtls_md_hmac_update( &ssl->transform_in->md_ctx_dec, ssl->in_len, 2 ); - mbedtls_md_hmac_update( &ssl->transform_in->md_ctx_dec, ssl->in_msg, - ssl->in_msglen ); - /* Make sure we access everything even when padlen > 0. This - * makes the synchronisation requirements for just-in-time - * Prime+Probe attacks much tighter and hopefully impractical. */ - ssl_read_memory( ssl->in_msg + ssl->in_msglen, padlen ); - mbedtls_md_hmac_finish( &ssl->transform_in->md_ctx_dec, mac_expect ); - - /* Dummy calls to compression function. - * Call mbedtls_md_process at least once due to cache attacks - * that observe whether md_process() was called of not. - * Respect the usual start-(process|update)-finish sequence for - * the sake of hardware accelerators that might require it. */ - mbedtls_md_starts( &ssl->transform_in->md_ctx_dec ); - for( j = 0; j < extra_run + 1; j++ ) - mbedtls_md_process( &ssl->transform_in->md_ctx_dec, ssl->in_msg ); - { - /* The switch statement above already checks that we're using - * one of MD-5, SHA-1, SHA-256 or SHA-384. */ - unsigned char tmp[384 / 8]; - mbedtls_md_finish( &ssl->transform_in->md_ctx_dec, tmp ); - } - - mbedtls_md_hmac_reset( &ssl->transform_in->md_ctx_dec ); - /* 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 From a237722118fea83d9fb261327d8b7357de43148b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 28 Jul 2020 10:53:06 +0200 Subject: [PATCH 04/19] Add MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This option allows to test the constant-flow nature of selected code, using MemSan and the fundamental observation behind ctgrind that the set of operations allowed on undefined memory by dynamic analysers is the same as the set of operations allowed on secret data to avoid leaking it to a local attacker via side channels, namely, any operation except branching and dereferencing. (This isn't the full story, as on some CPUs some instructions have variable execution depending on the inputs, most notably division and on some cores multiplication. However, testing that no branch or memory access depends on secret data is already a good start.) Signed-off-by: Manuel Pégourié-Gonnard --- include/mbedtls/check_config.h | 10 ++++++++++ include/mbedtls/config.h | 13 +++++++++++++ library/version_features.c | 3 +++ programs/ssl/query_config.c | 8 ++++++++ scripts/config.pl | 1 + tests/scripts/all.sh | 12 ++++++++++++ tests/suites/helpers.function | 16 ++++++++++++++++ 7 files changed, 63 insertions(+) diff --git a/include/mbedtls/check_config.h b/include/mbedtls/check_config.h index 8ce73cef..2bcf960e 100644 --- a/include/mbedtls/check_config.h +++ b/include/mbedtls/check_config.h @@ -199,6 +199,16 @@ #error "MBEDTLS_ENTROPY_FORCE_SHA256 defined, but not all prerequisites" #endif +#if defined(__has_feature) +#if __has_feature(memory_sanitizer) +#define MBEDTLS_HAS_MEMSAN +#endif +#endif +#if defined(MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN) && !defined(MBEDTLS_HAS_MEMSAN) +#error "MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN requires building with MemorySanitizer" +#endif +#undef MBEDTLS_HAS_MEMSAN + #if defined(MBEDTLS_TEST_NULL_ENTROPY) && \ ( !defined(MBEDTLS_ENTROPY_C) || !defined(MBEDTLS_NO_DEFAULT_ENTROPY_SOURCES) ) #error "MBEDTLS_TEST_NULL_ENTROPY defined, but not all prerequisites" diff --git a/include/mbedtls/config.h b/include/mbedtls/config.h index f7e55aef..96766808 100644 --- a/include/mbedtls/config.h +++ b/include/mbedtls/config.h @@ -551,6 +551,19 @@ //#define MBEDTLS_ECP_RANDOMIZE_MXZ_ALT //#define MBEDTLS_ECP_NORMALIZE_MXZ_ALT +/** + * \def MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN + * + * 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 setting requires compiling with clang -fsanitize=memory. + * + * Uncomment to enable testing of the constant-flow nature of seletected code. + */ +//#define MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN + /** * \def MBEDTLS_TEST_NULL_ENTROPY * diff --git a/library/version_features.c b/library/version_features.c index 51662bfd..6b2a0c1b 100644 --- a/library/version_features.c +++ b/library/version_features.c @@ -279,6 +279,9 @@ static const char *features[] = { #if defined(MBEDTLS_ECP_NORMALIZE_MXZ_ALT) "MBEDTLS_ECP_NORMALIZE_MXZ_ALT", #endif /* MBEDTLS_ECP_NORMALIZE_MXZ_ALT */ +#if defined(MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN) + "MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN", +#endif /* MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN */ #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 968549ac..1e5c311d 100644 --- a/programs/ssl/query_config.c +++ b/programs/ssl/query_config.c @@ -741,6 +741,14 @@ int query_config( const char *config ) } #endif /* MBEDTLS_ECP_NORMALIZE_MXZ_ALT */ +#if defined(MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN) + if( strcmp( "MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN", config ) == 0 ) + { + MACRO_EXPANSION_TO_STR( MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN ); + return( 0 ); + } +#endif /* MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN */ + #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 006a58de..38fa97cf 100755 --- a/scripts/config.pl +++ b/scripts/config.pl @@ -128,6 +128,7 @@ MBEDTLS_REMOVE_3DES_CIPHERSUITES MBEDTLS_REMOVE_ARC4_CIPHERSUITES MBEDTLS_RSA_NO_CRT MBEDTLS_SSL_HW_RECORD_ACCEL +MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN 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 d831d344..02f61d6e 100755 --- a/tests/scripts/all.sh +++ b/tests/scripts/all.sh @@ -1031,6 +1031,18 @@ component_test_full_cmake_clang () { if_build_succeeded env OPENSSL_CMD="$OPENSSL_NEXT" tests/compat.sh -e '^$' -f 'ARIA\|CHACHA' } +component_test_memsan_constant_flow () { + msg "build: cmake memsan, full config with constant flow testing" + scripts/config.pl full + scripts/config.pl set MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN + scripts/config.pl unset MBEDTLS_AESNI_C # memsan doesn't grok asm + CC=clang cmake -D CMAKE_BUILD_TYPE:String=MemSan . + make + + msg "test: main suites (memsan constant flow)" + make test +} + 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 cd9346e5..95b658da 100644 --- a/tests/suites/helpers.function +++ b/tests/suites/helpers.function @@ -46,6 +46,22 @@ typedef UINT32 uint32_t; #include #endif +#if defined(MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN) +#include + +/* Use macros to avoid messing up with origin tracking */ +#define TEST_CF_SECRET __msan_allocated_memory +// void __msan_allocated_memory(const volatile void* data, size_t size); +#define TEST_CF_PUBLIC __msan_unpoison +// void __msan_unpoison(const volatile void *a, size_t size); + +#else /* MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN */ + +#define TEST_CF_SECRET(ptr, size) +#define TEST_CF_PUBLIC(ptr, size) + +#endif /* MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN */ + /* Type for Hex parameters */ typedef struct data_tag { From 0dab12ec2cdeb4f3de9fea56f837b28329b89e53 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 28 Jul 2020 11:02:57 +0200 Subject: [PATCH 05/19] Start testing cf_hmac() for constant flow MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Currently this breaks all.sh component test_memsan_constant_flow, just as expected, as the current implementation is not constant flow. This will be fixed in the next commit. Signed-off-by: Manuel Pégourié-Gonnard --- tests/suites/test_suite_ssl.function | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/suites/test_suite_ssl.function b/tests/suites/test_suite_ssl.function index fc953a12..989d8952 100644 --- a/tests/suites/test_suite_ssl.function +++ b/tests/suites/test_suite_ssl.function @@ -117,10 +117,13 @@ void ssl_cf_hmac( int hash ) data[i] = ( i & 0xff ) ^ rec_num; /* Get the function's result */ + TEST_CF_SECRET( &in_len, sizeof( in_len ) ); TEST_ASSERT( 0 == mbedtls_ssl_cf_hmac( &ctx, add_data, sizeof( add_data ), data, in_len, min_in_len, max_in_len, out ) ); + TEST_CF_PUBLIC( &in_len, sizeof( in_len ) ); + TEST_CF_PUBLIC( out, out_len ); /* Compute the reference result */ TEST_ASSERT( 0 == mbedtls_md_hmac_update( &ref_ctx, add_data, From de02b580c8bd2d122746616205344dae4a2e9243 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 28 Jul 2020 11:25:34 +0200 Subject: [PATCH 06/19] Implement cf_hmac() actually with constant flow 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 | 148 ++++++++++++++++++++++++++-------------------- 1 file changed, 83 insertions(+), 65 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index fbf43ab8..b2d12945 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1809,6 +1809,48 @@ static int ssl_encrypt_buf( mbedtls_ssl_context *ssl ) ( defined(MBEDTLS_SSL_PROTO_TLS1) || \ defined(MBEDTLS_SSL_PROTO_TLS1_1) || \ defined(MBEDTLS_SSL_PROTO_TLS1_2) ) +/* + * Constant-flow conditional memcpy: + * - if c1 == c2, equivalent to memcpy(dst, src, len), + * - otherwise, a no-op, + * but with execution flow independent of the values of c1 and c2. + * + * Use only bit operations to avoid branches that could be used by some + * compilers on some platforms to translate comparison operators. + */ +static void mbedtls_ssl_cf_memcpy_if_eq(unsigned char *dst, + const unsigned char *src, + size_t len, + size_t c1, size_t c2 ) +{ + /* diff = 0 if c1 == c2, non-zero otherwise */ + const size_t diff = c1 ^ c2; + + /* MSVC has a warning about unary minus on unsigned integer types, + * but this is well-defined and precisely what we want to do here. */ +#if defined(_MSC_VER) +#pragma warning( push ) +#pragma warning( disable : 4146 ) +#endif + + /* diff_msb's most significant bit is bit equal to c1 != c2 */ + const size_t diff_msb = ( diff | -diff ); + + /* diff1 = c1 != c2 */ + const size_t diff1 = diff_msb >> ( sizeof( diff_msb ) * 8 - 1 ); + + /* mask = c1 != c2 ? 0xff : 0x00 */ + unsigned char mask = (unsigned char) -diff1; + +#if defined(_MSC_VER) +#pragma warning( pop ) +#endif + + /* dst[i] = c1 != c2 ? dst[i] : src[i] */ + for( size_t i = 0; i < len; i++ ) + dst[i] = ( dst[i] & mask ) | ( src[i] & ~mask ); +} + /* * Compute HMAC of variable-length data with constant flow. */ @@ -1819,85 +1861,61 @@ int mbedtls_ssl_cf_hmac( size_t min_data_len, size_t max_data_len, unsigned char *output ) { - /* WORK IN PROGRESS - THIS IS ONLY PSEUDO-CONTANT-TIME */ - /* - * Process MAC and always update for padlen afterwards to make - * total time independent of padlen. + * This function breaks the HMAC abstraction and uses the md_clone() + * extension to the MD API in order to get constant-flow behaviour. * - * Known timing attacks: - * - Lucky Thirteen (http://www.isg.rhul.ac.uk/tls/TLStiming.pdf) + * HMAC(msg) is defined as HASH(okey + HASH(ikey + msg)) where + means + * concatenation, and okey/ikey is the XOR of the key with some fix bit + * patterns (see RFC 2104, sec. 2), which are stored in ctx->hmac_ctx. * - * To compensate for different timings for the MAC calculation - * depending on how much padding was removed (which is determined - * by padlen), process extra_run more blocks through the hash - * function. + * We'll first compute inner_hash = HASH(ikey + msg) by hashing up to + * minlen, then cloning the context, and for each byte up to maxlen + * finishing up the hash computation, keeping only the correct result. * - * The formula in the paper is - * extra_run = ceil( (L1-55) / 64 ) - ceil( (L2-55) / 64 ) - * where L1 is the size of the header plus the decrypted message - * plus CBC padding and L2 is the size of the header plus the - * decrypted message. This is for an underlying hash function - * with 64-byte blocks. - * We use ( (Lx+8) / 64 ) to handle 'negative Lx' values - * correctly. We round down instead of up, so -56 is the correct - * value for our calculations instead of -55. - * - * Repeat the formula rather than defining a block_size variable. - * This avoids requiring division by a variable at runtime - * (which would be marginally less efficient and would require - * linking an extra division function in some builds). + * Then we only need to compute HASH(okey + inner_hash) and we're done. */ - size_t j, extra_run = 0; - /* This size is enough to server either as input to - * md_process() or as output to md_finish() */ - unsigned char tmp[128]; + const mbedtls_md_type_t md_alg = mbedtls_md_get_type( ctx->md_info ); + const size_t block_size = md_alg == MBEDTLS_MD_SHA384 ? 128 : 64; + const unsigned char * const ikey = (unsigned char *) ctx->hmac_ctx; + const unsigned char * const okey = ikey + block_size; + const size_t hash_size = mbedtls_md_get_size( ctx->md_info ); - memset( tmp, 0, sizeof( tmp ) ); + unsigned char aux_out[MBEDTLS_MD_MAX_SIZE]; + mbedtls_md_context_t aux; + size_t offset; - switch( mbedtls_md_get_type( ctx->md_info ) ) + mbedtls_md_init( &aux ); + mbedtls_md_setup( &aux, ctx->md_info, 0 ); + + /* After hmac_start() of hmac_reset(), ikey has already been hashed, + * so we can start directly with the message */ + mbedtls_md_update( ctx, add_data, add_data_len ); + mbedtls_md_update( ctx, data, min_data_len ); + + /* For each possible length, compute the hash up to that point */ + for( offset = min_data_len; offset <= max_data_len; offset++ ) { -#if defined(MBEDTLS_MD5_C) || defined(MBEDTLS_SHA1_C) || \ -defined(MBEDTLS_SHA256_C) - case MBEDTLS_MD_MD5: - case MBEDTLS_MD_SHA1: - case MBEDTLS_MD_SHA256: - /* 8 bytes of message size, 64-byte compression blocks */ - extra_run = ( add_data_len + max_data_len + 8 ) / 64 - - ( add_data_len + data_len_secret + 8 ) / 64; - break; -#endif -#if defined(MBEDTLS_SHA512_C) - case MBEDTLS_MD_SHA384: - /* 16 bytes of message size, 128-byte compression blocks */ - extra_run = ( add_data_len + max_data_len + 16 ) / 128 - - ( add_data_len + data_len_secret + 16 ) / 128; - break; -#endif - default: - return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); + mbedtls_md_clone( &aux, ctx ); + mbedtls_md_finish( &aux, aux_out ); + /* Keep only the correct inner_hash in the output buffer */ + mbedtls_ssl_cf_memcpy_if_eq( output, aux_out, hash_size, + offset, data_len_secret ); + + if( offset < max_data_len ) + mbedtls_md_update( ctx, data + offset, 1 ); } - mbedtls_md_hmac_update( ctx, add_data, add_data_len ); - mbedtls_md_hmac_update( ctx, data, data_len_secret ); - /* Make sure we access everything even when padlen > 0. This - * makes the synchronisation requirements for just-in-time - * Prime+Probe attacks much tighter and hopefully impractical. */ - ssl_read_memory( data + min_data_len, max_data_len - min_data_len ); - mbedtls_md_hmac_finish( ctx, output ); - - /* Dummy calls to compression function. - * Call mbedtls_md_process at least once due to cache attacks - * that observe whether md_process() was called of not. - * Respect the usual start-(process|update)-finish sequence for - * the sake of hardware accelerators that might require it. */ + /* Now compute HASH(okey + inner_hash) */ mbedtls_md_starts( ctx ); - for( j = 0; j < extra_run + 1; j++ ) - mbedtls_md_process( ctx, tmp ); - mbedtls_md_finish( ctx, tmp ); + mbedtls_md_update( ctx, okey, block_size ); + mbedtls_md_update( ctx, output, hash_size ); + mbedtls_md_finish( ctx, output ); + /* Done, get ready for next time */ mbedtls_md_hmac_reset( ctx ); + mbedtls_md_free( &aux ); return( 0 ); } #endif /* MBEDTLS_SSL_SOME_SUITES_USE_CBC && TLS 1.0-1.2 */ From 1e94128f3065866f7949921b5ac86dbdb54e5faa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 28 Jul 2020 11:35:39 +0200 Subject: [PATCH 07/19] Factor repeated condition to its own macro MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- include/mbedtls/ssl_internal.h | 14 +++++++++----- library/ssl_tls.c | 7 ++----- tests/suites/test_suite_ssl.function | 2 +- 3 files changed, 12 insertions(+), 11 deletions(-) diff --git a/include/mbedtls/ssl_internal.h b/include/mbedtls/ssl_internal.h index 9ad04181..e67c20b4 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -161,6 +161,13 @@ #define MBEDTLS_SSL_SOME_SUITES_USE_CBC #endif +#if defined(MBEDTLS_SSL_SOME_SUITES_USE_CBC) && \ + ( defined(MBEDTLS_SSL_PROTO_TLS1) || \ + defined(MBEDTLS_SSL_PROTO_TLS1_1) || \ + defined(MBEDTLS_SSL_PROTO_TLS1_2) ) +#define MBEDTLS_SSL_SOME_SUITES_USE_TLS_CBC +#endif + /* * Allow extra bytes for record, authentication and encryption overhead: * counter (8) + header (5) + IV(16) + MAC (16-48) + padding (0-256) @@ -852,10 +859,7 @@ int mbedtls_ssl_get_key_exchange_md_tls1_2( mbedtls_ssl_context *ssl, #endif /* MBEDTLS_SSL_PROTO_TLS1 || MBEDTLS_SSL_PROTO_TLS1_1 || \ MBEDTLS_SSL_PROTO_TLS1_2 */ -#if defined(MBEDTLS_SSL_SOME_SUITES_USE_CBC) && \ - ( defined(MBEDTLS_SSL_PROTO_TLS1) || \ - defined(MBEDTLS_SSL_PROTO_TLS1_1) | \ - defined(MBEDTLS_SSL_PROTO_TLS1_2) ) +#if defined(MBEDTLS_SSL_SOME_SUITES_USE_TLS_CBC) /** \brief Compute the HMAC of variable-length data with constant flow. * * This function computes the HMAC of the concatenation of \p add_data and \p @@ -894,7 +898,7 @@ 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 ); -#endif /* MBEDTLS_SSL_SOME_SUITES_USE_CBC && TLS 1.0-1.2 */ +#endif /* MBEDTLS_SSL_SOME_SUITES_USE_TLS_CBC */ #ifdef __cplusplus } diff --git a/library/ssl_tls.c b/library/ssl_tls.c index b2d12945..a0b586a4 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1805,10 +1805,7 @@ static int ssl_encrypt_buf( mbedtls_ssl_context *ssl ) return( 0 ); } -#if defined(MBEDTLS_SSL_SOME_SUITES_USE_CBC) && \ - ( defined(MBEDTLS_SSL_PROTO_TLS1) || \ - defined(MBEDTLS_SSL_PROTO_TLS1_1) || \ - defined(MBEDTLS_SSL_PROTO_TLS1_2) ) +#if defined(MBEDTLS_SSL_SOME_SUITES_USE_TLS_CBC) /* * Constant-flow conditional memcpy: * - if c1 == c2, equivalent to memcpy(dst, src, len), @@ -1918,7 +1915,7 @@ int mbedtls_ssl_cf_hmac( mbedtls_md_free( &aux ); return( 0 ); } -#endif /* MBEDTLS_SSL_SOME_SUITES_USE_CBC && TLS 1.0-1.2 */ +#endif /* MBEDTLS_SSL_SOME_SUITES_USE_TLS_CBC */ static int ssl_decrypt_buf( mbedtls_ssl_context *ssl ) { diff --git a/tests/suites/test_suite_ssl.function b/tests/suites/test_suite_ssl.function index 989d8952..360be203 100644 --- a/tests/suites/test_suite_ssl.function +++ b/tests/suites/test_suite_ssl.function @@ -53,7 +53,7 @@ void ssl_set_hostname_twice( char *hostname0, char *hostname1 ) } /* END_CASE */ -/* BEGIN_CASE depends_on:MBEDTLS_CIPHER_MODE_CBC:MBEDTLS_AES_C:MBEDTLS_SSL_PROTO_TLS1_2 */ +/* BEGIN_CASE depends_on:MBEDTLS_SSL_SOME_SUITES_USE_TLS_CBC */ void ssl_cf_hmac( int hash ) { /* From 74503bb5fc797546e532601de452a3d03c509079 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 28 Jul 2020 11:42:31 +0200 Subject: [PATCH 08/19] Improve some comments and internal documentation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- include/mbedtls/ssl_internal.h | 8 +++++--- library/ssl_tls.c | 4 +++- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/include/mbedtls/ssl_internal.h b/include/mbedtls/ssl_internal.h index e67c20b4..60ba2241 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -869,9 +869,11 @@ int mbedtls_ssl_get_key_exchange_md_tls1_2( mbedtls_ssl_context *ssl, * max_data_len bytes from \p data. * * \param ctx The HMAC context. It must have keys configured - * with mbedtls_md_hmac_starts(). It is reset using - * mbedtls_md_hmac_reset() after the computation is - * complete to prepare for the next computation. + * with mbedtls_md_hmac_starts() and use one of the + * following hashes: SHA-384, SHA-256, SHA-1 or MD-5. + * It is reset using mbedtls_md_hmac_reset() after + * the computation is complete to prepare for the + * next computation. * \param add_data The additional data prepended to \p data. This * must point to a readable buffer of \p add_data_len * bytes. diff --git a/library/ssl_tls.c b/library/ssl_tls.c index a0b586a4..5f940f0c 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1863,7 +1863,7 @@ int mbedtls_ssl_cf_hmac( * extension to the MD API in order to get constant-flow behaviour. * * HMAC(msg) is defined as HASH(okey + HASH(ikey + msg)) where + means - * concatenation, and okey/ikey is the XOR of the key with some fix bit + * concatenation, and okey/ikey are the XOR of the key with some fixed bit * patterns (see RFC 2104, sec. 2), which are stored in ctx->hmac_ctx. * * We'll first compute inner_hash = HASH(ikey + msg) by hashing up to @@ -1873,6 +1873,8 @@ int mbedtls_ssl_cf_hmac( * Then we only need to compute HASH(okey + inner_hash) and we're done. */ const mbedtls_md_type_t md_alg = mbedtls_md_get_type( ctx->md_info ); + /* TLS 1.0-1.2 only support SHA-384, SHA-256, SHA-1, MD-5, + * all of which have the same block size except SHA-384. */ const size_t block_size = md_alg == MBEDTLS_MD_SHA384 ? 128 : 64; const unsigned char * const ikey = (unsigned char *) ctx->hmac_ctx; const unsigned char * const okey = ikey + block_size; From a6c1317685118a6448fe4398e52916402b7019fe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 28 Jul 2020 11:45:02 +0200 Subject: [PATCH 09/19] Remove unnecessary cast MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is C, not C++, casts between void * and other pointer types are free. Signed-off-by: Manuel Pégourié-Gonnard --- library/ssl_tls.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 5f940f0c..74fc552d 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1876,7 +1876,7 @@ int mbedtls_ssl_cf_hmac( /* TLS 1.0-1.2 only support SHA-384, SHA-256, SHA-1, MD-5, * all of which have the same block size except SHA-384. */ const size_t block_size = md_alg == MBEDTLS_MD_SHA384 ? 128 : 64; - const unsigned char * const ikey = (unsigned char *) ctx->hmac_ctx; + const unsigned char * const ikey = ctx->hmac_ctx; const unsigned char * const okey = ikey + block_size; const size_t hash_size = mbedtls_md_get_size( ctx->md_info ); From 5bb6f3c3dba5dba2b6dd5b1f805ad5563f82e7b1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 28 Jul 2020 11:49:42 +0200 Subject: [PATCH 10/19] Check errors from the MD layer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Could be out-of-memory for some functions, accelerator issues for others. Signed-off-by: Manuel Pégourié-Gonnard --- library/ssl_tls.c | 36 ++++++++++++++++++++++++------------ 1 file changed, 24 insertions(+), 12 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 74fc552d..3e533766 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1883,39 +1883,51 @@ int mbedtls_ssl_cf_hmac( unsigned char aux_out[MBEDTLS_MD_MAX_SIZE]; mbedtls_md_context_t aux; size_t offset; + int ret; mbedtls_md_init( &aux ); - mbedtls_md_setup( &aux, ctx->md_info, 0 ); + +#define MD_CHK( func_call ) \ + do { \ + ret = (func_call); \ + if( ret != 0 ) \ + goto cleanup; \ + } while( 0 ) + + MD_CHK( mbedtls_md_setup( &aux, ctx->md_info, 0 ) ); /* After hmac_start() of hmac_reset(), ikey has already been hashed, * so we can start directly with the message */ - mbedtls_md_update( ctx, add_data, add_data_len ); - mbedtls_md_update( ctx, data, min_data_len ); + MD_CHK( mbedtls_md_update( ctx, add_data, add_data_len ) ); + MD_CHK( mbedtls_md_update( ctx, data, min_data_len ) ); /* For each possible length, compute the hash up to that point */ for( offset = min_data_len; offset <= max_data_len; offset++ ) { - mbedtls_md_clone( &aux, ctx ); - mbedtls_md_finish( &aux, aux_out ); + MD_CHK( mbedtls_md_clone( &aux, ctx ) ); + MD_CHK( mbedtls_md_finish( &aux, aux_out ) ); /* Keep only the correct inner_hash in the output buffer */ mbedtls_ssl_cf_memcpy_if_eq( output, aux_out, hash_size, offset, data_len_secret ); if( offset < max_data_len ) - mbedtls_md_update( ctx, data + offset, 1 ); + MD_CHK( mbedtls_md_update( ctx, data + offset, 1 ) ); } /* Now compute HASH(okey + inner_hash) */ - mbedtls_md_starts( ctx ); - mbedtls_md_update( ctx, okey, block_size ); - mbedtls_md_update( ctx, output, hash_size ); - mbedtls_md_finish( ctx, output ); + MD_CHK( mbedtls_md_starts( ctx ) ); + MD_CHK( mbedtls_md_update( ctx, okey, block_size ) ); + MD_CHK( mbedtls_md_update( ctx, output, hash_size ) ); + MD_CHK( mbedtls_md_finish( ctx, output ) ); /* Done, get ready for next time */ - mbedtls_md_hmac_reset( ctx ); + MD_CHK( mbedtls_md_hmac_reset( ctx ) ); +#undef MD_CHK + +cleanup: mbedtls_md_free( &aux ); - return( 0 ); + return( ret ); } #endif /* MBEDTLS_SSL_SOME_SUITES_USE_TLS_CBC */ From 2b80249c043762a991240bc7e04807f4443cc31e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Wed, 22 Jul 2020 11:09:28 +0200 Subject: [PATCH 11/19] Add comment on memsan + constant-flow testing --- tests/scripts/all.sh | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/tests/scripts/all.sh b/tests/scripts/all.sh index 02f61d6e..f5a256f6 100755 --- a/tests/scripts/all.sh +++ b/tests/scripts/all.sh @@ -1032,14 +1032,20 @@ component_test_full_cmake_clang () { } component_test_memsan_constant_flow () { - msg "build: cmake memsan, full config with constant flow testing" + # This tests both (1) accesses to undefined memory, and (2) branches or + # memory access depending on secret values. To distinguish between those: + # - unset MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN - does the failure persist? + # - or alternatively, change the build type to MemSanDbg, which enables + # origin tracking and nicer stack traces (which are useful for debugging + # anyway), and check if the origin was TEST_CF_SECRET() or something else. + msg "build: cmake MSan (clang), full config with constant flow testing" scripts/config.pl full scripts/config.pl set MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN scripts/config.pl unset MBEDTLS_AESNI_C # memsan doesn't grok asm CC=clang cmake -D CMAKE_BUILD_TYPE:String=MemSan . make - msg "test: main suites (memsan constant flow)" + msg "test: main suites (Msan + constant flow)" make test } From 2b2f956f22abcdd8b288e17c12058b20b23fb5e0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 28 Jul 2020 11:54:35 +0200 Subject: [PATCH 12/19] Fix typos in comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Janos Follath Signed-off-by: Manuel Pégourié-Gonnard --- include/mbedtls/config.h | 2 +- include/mbedtls/ssl_internal.h | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/include/mbedtls/config.h b/include/mbedtls/config.h index 96766808..1c3f14e8 100644 --- a/include/mbedtls/config.h +++ b/include/mbedtls/config.h @@ -560,7 +560,7 @@ * * This setting requires compiling with clang -fsanitize=memory. * - * Uncomment to enable testing of the constant-flow nature of seletected code. + * Uncomment to enable testing of the constant-flow nature of selected code. */ //#define MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN diff --git a/include/mbedtls/ssl_internal.h b/include/mbedtls/ssl_internal.h index 60ba2241..c435f7a9 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -882,11 +882,11 @@ int mbedtls_ssl_get_key_exchange_md_tls1_2( mbedtls_ssl_context *ssl, * to a readable buffer of \p max_data_len bytes. * \param data_len_secret The length of the data to process in \p data. * This must be no less than \p min_data_len and no - * greated than \p max_data_len. + * greater than \p max_data_len. * \param min_data_len The minimal length of \p data in bytes. * \param max_data_len The maximal length of \p data in bytes. * \param output The HMAC will be written here. This must point to - * a writeable buffer of sufficient size to hold the + * a writable buffer of sufficient size to hold the * HMAC value. * * \retval 0 From 94fd8dc0663a8aa5ca30ef4a44d0a41a1dd7219e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 28 Jul 2020 11:56:05 +0200 Subject: [PATCH 13/19] Fix typos in comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Janos Follath Signed-off-by: Manuel Pégourié-Gonnard --- library/ssl_tls.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 3e533766..c0a59e51 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1815,10 +1815,10 @@ static int ssl_encrypt_buf( mbedtls_ssl_context *ssl ) * Use only bit operations to avoid branches that could be used by some * compilers on some platforms to translate comparison operators. */ -static void mbedtls_ssl_cf_memcpy_if_eq(unsigned char *dst, - const unsigned char *src, - size_t len, - size_t c1, size_t c2 ) +static void mbedtls_ssl_cf_memcpy_if_eq( unsigned char *dst, + const unsigned char *src, + size_t len, + size_t c1, size_t c2 ) { /* diff = 0 if c1 == c2, non-zero otherwise */ const size_t diff = c1 ^ c2; From 09ac297f7fb8c32fe3c1d624773114545168f7d6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 28 Jul 2020 11:57:25 +0200 Subject: [PATCH 14/19] Add missing const for consistency 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 | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index c0a59e51..533f301a 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1830,14 +1830,14 @@ static void mbedtls_ssl_cf_memcpy_if_eq( unsigned char *dst, #pragma warning( disable : 4146 ) #endif - /* diff_msb's most significant bit is bit equal to c1 != c2 */ + /* diff_msb's most significant bit is equal to c1 != c2 */ const size_t diff_msb = ( diff | -diff ); /* diff1 = c1 != c2 */ const size_t diff1 = diff_msb >> ( sizeof( diff_msb ) * 8 - 1 ); /* mask = c1 != c2 ? 0xff : 0x00 */ - unsigned char mask = (unsigned char) -diff1; + const unsigned char mask = (unsigned char) -diff1; #if defined(_MSC_VER) #pragma warning( pop ) From 253b0de23d4738d615dc9f6df470a9955a06bfc8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Wed, 29 Jul 2020 10:04:36 +0200 Subject: [PATCH 15/19] Remove use of C99 construct MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is an LTS branch, C99 isn't allowed yet, it breaks versions of MSVC that we still support for this branch. Signed-off-by: Manuel Pégourié-Gonnard --- library/ssl_tls.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 533f301a..96c7e6c6 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1844,7 +1844,8 @@ static void mbedtls_ssl_cf_memcpy_if_eq( unsigned char *dst, #endif /* dst[i] = c1 != c2 ? dst[i] : src[i] */ - for( size_t i = 0; i < len; i++ ) + size_t i; + for( i = 0; i < len; i++ ) dst[i] = ( dst[i] & mask ) | ( src[i] & ~mask ); } From e1b1fffaadeb4c6d8728734ed015036b92cd3ff3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Wed, 29 Jul 2020 12:54:04 +0200 Subject: [PATCH 16/19] Add comment that was lost while backporting 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 | 3 +++ 1 file changed, 3 insertions(+) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 96c7e6c6..81139b40 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1851,6 +1851,9 @@ static void mbedtls_ssl_cf_memcpy_if_eq( unsigned char *dst, /* * Compute HMAC of variable-length data with constant flow. + * + * Only works with MD-5, SHA-1, SHA-256 and SHA-384. + * (Otherwise, computation of block_size needs to be adapted.) */ int mbedtls_ssl_cf_hmac( mbedtls_md_context_t *ctx, From 681615a27640868b7d10894072f9e6a1fc5f7487 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Fri, 31 Jul 2020 10:00:17 +0200 Subject: [PATCH 17/19] Remove obsolete comment about test dependency 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_ssl.function | 4 ---- 1 file changed, 4 deletions(-) diff --git a/tests/suites/test_suite_ssl.function b/tests/suites/test_suite_ssl.function index 360be203..cf807b54 100644 --- a/tests/suites/test_suite_ssl.function +++ b/tests/suites/test_suite_ssl.function @@ -59,10 +59,6 @@ void ssl_cf_hmac( int hash ) /* * Test the function mbedtls_ssl_cf_hmac() against a reference * implementation. - * - * Note: the dependency is actually on TLS 1.0-1.2 and (AES or ARIA or - * Camellia or DES), but since the test framework doesn't support - * alternation in dependencies, just depend on the most common. */ mbedtls_md_context_t ctx, ref_ctx; const mbedtls_md_info_t *md_info; From f82cb79b875267a1809d61b38c9e30cb81a70ccd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Fri, 31 Jul 2020 12:53:39 +0200 Subject: [PATCH 18/19] Add comments clarifying differences between macros MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- include/mbedtls/ssl_internal.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/include/mbedtls/ssl_internal.h b/include/mbedtls/ssl_internal.h index c435f7a9..4a3eee56 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -152,7 +152,7 @@ #define MBEDTLS_SSL_RETRANS_WAITING 2 #define MBEDTLS_SSL_RETRANS_FINISHED 3 -/* For CBC-specific encrypt/decrypt code */ +/* This macro determines whether CBC is supported. */ #if defined(MBEDTLS_CIPHER_MODE_CBC) && \ ( defined(MBEDTLS_AES_C) || \ defined(MBEDTLS_CAMELLIA_C) || \ @@ -161,6 +161,8 @@ #define MBEDTLS_SSL_SOME_SUITES_USE_CBC #endif +/* This macro determines whether the CBC construct used in TLS 1.0-1.2 (as + * opposed to the very different CBC construct used in SSLv3) is supported. */ #if defined(MBEDTLS_SSL_SOME_SUITES_USE_CBC) && \ ( defined(MBEDTLS_SSL_PROTO_TLS1) || \ defined(MBEDTLS_SSL_PROTO_TLS1_1) || \ From 7433fa4f4a0addbcf7ff11e4d08ed5298d46f642 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Fri, 31 Jul 2020 12:59:34 +0200 Subject: [PATCH 19/19] Add warning about test-only config.h option 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 | 3 +++ 1 file changed, 3 insertions(+) diff --git a/include/mbedtls/config.h b/include/mbedtls/config.h index 1c3f14e8..73efeb44 100644 --- a/include/mbedtls/config.h +++ b/include/mbedtls/config.h @@ -560,6 +560,9 @@ * * This setting requires compiling with clang -fsanitize=memory. * + * \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_MEMSAN