From 8ebb88d1e09c04f2a018544916cb08cf601b6da5 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 | 8 ++++++++ library/ssl_tls.c | 15 +++++---------- 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/include/mbedtls/ssl_internal.h b/include/mbedtls/ssl_internal.h index 4aa746373..35b8eedc0 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -144,6 +144,14 @@ #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_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 66596bffc..7d9eae4ae 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1301,8 +1301,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_SSL_SOME_SUITES_USE_CBC) #define SSL_SOME_MODES_USE_MAC #endif @@ -1523,8 +1522,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) ) +#if defined(MBEDTLS_SSL_SOME_SUITES_USE_CBC) if( mode == MBEDTLS_MODE_CBC ) { int ret; @@ -1643,8 +1641,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 ) */ +#endif /* MBEDTLS_SSL_SOME_SUITES_USE_CBC */ { MBEDTLS_SSL_DEBUG_MSG( 1, ( "should never happen" ) ); return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); @@ -1787,8 +1784,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) ) +#if defined(MBEDTLS_SSL_SOME_SUITES_USE_CBC) if( mode == MBEDTLS_MODE_CBC ) { /* @@ -1999,8 +1995,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 ) */ +#endif /* MBEDTLS_SSL_SOME_SUITES_USE_CBC) */ { MBEDTLS_SSL_DEBUG_MSG( 1, ( "should never happen" ) ); return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); From 3ba2bcaf0d2d5980b20d2d6f118a2d11a5f3f673 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 | 92 ++++++++++++++++++++++++++++ 4 files changed, 178 insertions(+) diff --git a/include/mbedtls/ssl_internal.h b/include/mbedtls/ssl_internal.h index 35b8eedc0..9a0c19254 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -740,6 +740,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 non-zero + * Failure. + */ +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 7d9eae4ae..c2e5bde99 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1659,6 +1659,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 ) { size_t i; diff --git a/tests/suites/test_suite_ssl.data b/tests/suites/test_suite_ssl.data index b92c1fe8a..f85d26b11 100644 --- a/tests/suites/test_suite_ssl.data +++ b/tests/suites/test_suite_ssl.data @@ -57,3 +57,19 @@ ssl_dtls_replay:"abcd12340000,abcd12340100":"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 1cd2ed5bb..2eafb2f94 100644 --- a/tests/suites/test_suite_ssl.function +++ b/tests/suites/test_suite_ssl.function @@ -54,3 +54,95 @@ 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 */ +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 d11971875a51673ecaef6e90d3a648d035609ae3 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 c2e5bde99..bc056eea5 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1313,7 +1313,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; @@ -1673,12 +1673,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 ); @@ -2061,34 +2132,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 @@ -2103,60 +2148,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 40597cef016d432f6e5145768c4a71dc19747d26 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 +++ scripts/config.pl | 1 + tests/scripts/all.sh | 12 ++++++++++++ tests/suites/helpers.function | 16 ++++++++++++++++ 6 files changed, 55 insertions(+) diff --git a/include/mbedtls/check_config.h b/include/mbedtls/check_config.h index 1215e6eb9..040c1d259 100644 --- a/include/mbedtls/check_config.h +++ b/include/mbedtls/check_config.h @@ -182,6 +182,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 f08cc4a19..0e7dc77d8 100644 --- a/include/mbedtls/config.h +++ b/include/mbedtls/config.h @@ -444,6 +444,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 454c2be7f..b5ac2da62 100644 --- a/library/version_features.c +++ b/library/version_features.c @@ -255,6 +255,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/scripts/config.pl b/scripts/config.pl index 87f59bc60..6153dd7a3 100755 --- a/scripts/config.pl +++ b/scripts/config.pl @@ -126,6 +126,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 5de2f064d..f010af8c3 100755 --- a/tests/scripts/all.sh +++ b/tests/scripts/all.sh @@ -963,6 +963,18 @@ component_test_full_cmake_clang () { if_build_succeeded env OPENSSL_CMD="$OPENSSL_LEGACY" GNUTLS_CLI="$GNUTLS_LEGACY_CLI" GNUTLS_SERV="$GNUTLS_LEGACY_SERV" tests/compat.sh -e '^$' -f 'NULL\|DES\|RC4\|ARCFOUR' } +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 96e18da16..2a6d34c01 100644 --- a/tests/suites/helpers.function +++ b/tests/suites/helpers.function @@ -38,6 +38,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 */ + /*----------------------------------------------------------------------------*/ /* Constants */ From 961b4dd407f9b421fff81c4b0e329ef3dbcd607c 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 2eafb2f94..f7e3be32c 100644 --- a/tests/suites/test_suite_ssl.function +++ b/tests/suites/test_suite_ssl.function @@ -118,10 +118,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 4508c67c42c65fff660f31a3073f12dd5ed62771 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 bc056eea5..55c39783f 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1663,6 +1663,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. */ @@ -1673,85 +1715,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 41df0f2bca3d5b38311ab2424fa68481451b623e 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 9a0c19254..42172ad2e 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -152,6 +152,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) @@ -740,10 +747,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 @@ -782,7 +786,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 55c39783f..5889659e2 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1659,10 +1659,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), @@ -1772,7 +1769,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 f7e3be32c..d3204778c 100644 --- a/tests/suites/test_suite_ssl.function +++ b/tests/suites/test_suite_ssl.function @@ -54,7 +54,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 ec956b186124159d1f881f30a99d878c9a2e1eac 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 42172ad2e..4b9056895 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -757,9 +757,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 5889659e2..07fad1c09 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1717,7 +1717,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 @@ -1727,6 +1727,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 c9ef5a2b761fe25a6b3d3386e95102515d426a6c 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 07fad1c09..a16568cc4 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1730,7 +1730,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 0cd0c731fdea220a485a97654d29f1b734e17c27 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 a16568cc4..6e6cc0897 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1737,39 +1737,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 2b223fd539aec6e9a86fe616a731222d0bbc7309 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 f010af8c3..06ad5ee0f 100755 --- a/tests/scripts/all.sh +++ b/tests/scripts/all.sh @@ -964,14 +964,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 2810110bbaf6c6a9c9eb123e79748f5e0e3a1a45 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 0e7dc77d8..bd029cb89 100644 --- a/include/mbedtls/config.h +++ b/include/mbedtls/config.h @@ -453,7 +453,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 4b9056895..24b13a3c6 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -770,11 +770,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 2da9a545597a9e6e480e2554044a418e59433b6b 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 6e6cc0897..725596ddd 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1669,10 +1669,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 2f484bd9799e676ff3d48e664339eaf08242808d 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 725596ddd..1248c6e2d 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1684,14 +1684,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 e05e57619b7ba05e7474e1f9742dc0271b607ee9 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 1248c6e2d..a65143c7e 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1698,7 +1698,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 7cf5ebc90fae38400eadcdd8f878e46ee7b51990 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 a65143c7e..3b4bfadf6 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1705,6 +1705,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 21b198355d0c9aa1bb4448586161864db2f54d48 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 d3204778c..0987374fb 100644 --- a/tests/suites/test_suite_ssl.function +++ b/tests/suites/test_suite_ssl.function @@ -60,10 +60,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 757c2d5c2c33fa364d31d96a502a356020aa3b06 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 24b13a3c6..d737c87c5 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -144,7 +144,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) || \ @@ -152,6 +152,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 4c575fba853683058d26f99dc8856cb1bdd628a5 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 bd029cb89..2ef54c5b4 100644 --- a/include/mbedtls/config.h +++ b/include/mbedtls/config.h @@ -453,6 +453,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