From 2df1f1f16f667f987d6f5fa6e1e66f1c59f3cfa1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Thu, 9 Jul 2020 12:11:39 +0200 Subject: [PATCH 01/20] 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 | 14 +++++++++----- library/ssl_msg.c | 17 +++++------------ 2 files changed, 14 insertions(+), 17 deletions(-) diff --git a/include/mbedtls/ssl_internal.h b/include/mbedtls/ssl_internal.h index c68038c7b..6bea84c34 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -146,12 +146,16 @@ #define MBEDTLS_SSL_COMPRESSION_ADD 0 #endif +#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 + #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_DES_C) ) ) + defined(MBEDTLS_SSL_SOME_SUITES_USE_CBC) #define MBEDTLS_SSL_SOME_MODES_USE_MAC #endif diff --git a/library/ssl_msg.c b/library/ssl_msg.c index d32afac56..083814cfe 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -609,10 +609,7 @@ int mbedtls_ssl_encrypt_buf( mbedtls_ssl_context *ssl, /* The PRNG is used for dynamic IV generation that's used * for CBC transformations in TLS 1.1 and TLS 1.2. */ -#if !( defined(MBEDTLS_CIPHER_MODE_CBC) && \ - ( defined(MBEDTLS_AES_C) || \ - defined(MBEDTLS_ARIA_C) || \ - defined(MBEDTLS_CAMELLIA_C) ) && \ +#if !( defined(MBEDTLS_SSL_SOME_SUITES_USE_CBC) && \ ( defined(MBEDTLS_SSL_PROTO_TLS1_1) || defined(MBEDTLS_SSL_PROTO_TLS1_2) ) ) ((void) f_rng); ((void) p_rng); @@ -910,8 +907,7 @@ int mbedtls_ssl_encrypt_buf( mbedtls_ssl_context *ssl, } else #endif /* MBEDTLS_GCM_C || MBEDTLS_CCM_C || MBEDTLS_CHACHAPOLY_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 = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; @@ -1050,8 +1046,7 @@ int mbedtls_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 ); @@ -1239,8 +1234,7 @@ int mbedtls_ssl_decrypt_buf( mbedtls_ssl_context const *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 ) { size_t minlen = 0; @@ -1493,8 +1487,7 @@ int mbedtls_ssl_decrypt_buf( mbedtls_ssl_context const *ssl, rec->data_len -= 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 045f094c815d3fcca45b87f65e7735c6de301f50 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Thu, 2 Jul 2020 11:34:02 +0200 Subject: [PATCH 02/20] 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 --- library/ssl_invasive.h | 78 +++++++++++++++++++++++ library/ssl_msg.c | 28 +++++++++ tests/suites/test_suite_ssl.data | 16 +++++ tests/suites/test_suite_ssl.function | 93 ++++++++++++++++++++++++++++ visualc/VS2010/mbedTLS.vcxproj | 1 + 5 files changed, 216 insertions(+) create mode 100644 library/ssl_invasive.h diff --git a/library/ssl_invasive.h b/library/ssl_invasive.h new file mode 100644 index 000000000..9697a91b2 --- /dev/null +++ b/library/ssl_invasive.h @@ -0,0 +1,78 @@ +/** + * \file ssl_invasive.h + * + * \brief SSL module: interfaces for invasive testing only. + * + * The interfaces in this file are intended for testing purposes only. + * They SHOULD NOT be made available in library integrations except when + * building the library for testing. + */ +/* + * Copyright (C) 2020, ARM Limited, All Rights Reserved + * SPDX-License-Identifier: Apache-2.0 + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + * This file is part of mbed TLS (https://tls.mbed.org) + */ +#ifndef MBEDTLS_SSL_INVASIVE_H +#define MBEDTLS_SSL_INVASIVE_H + +#include "common.h" +#include "mbedtls/md.h" + +#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 */ + +#endif /* MBEDTLS_SSL_INVASIVE_H */ diff --git a/library/ssl_msg.c b/library/ssl_msg.c index 083814cfe..7f233cd9f 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -47,6 +47,8 @@ #include "mbedtls/platform_util.h" #include "mbedtls/version.h" +#include "ssl_invasive.h" + #include #if defined(MBEDTLS_USE_PSA_CRYPTO) @@ -1064,6 +1066,32 @@ int mbedtls_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 */ + int mbedtls_ssl_decrypt_buf( mbedtls_ssl_context const *ssl, mbedtls_ssl_transform *transform, mbedtls_record *rec ) diff --git a/tests/suites/test_suite_ssl.data b/tests/suites/test_suite_ssl.data index d3158fd4c..35364861a 100644 --- a/tests/suites/test_suite_ssl.data +++ b/tests/suites/test_suite_ssl.data @@ -9504,3 +9504,19 @@ ssl_serialize_session_load_buf_size:42:"data_files/server5.crt" Session serialization, load buffer size: large ticket, cert depends_on:MBEDTLS_SSL_SESSION_TICKETS:MBEDTLS_SSL_CLI_C:MBEDTLS_X509_USE_C:MBEDTLS_PEM_PARSE_C:MBEDTLS_ECDSA_C:MBEDTLS_ECP_DP_SECP256R1_ENABLED:MBEDTLS_SHA256_C:MBEDTLS_FS_IO ssl_serialize_session_load_buf_size:1023:"data_files/server5.crt" + +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 5cf6e8bd7..711d2d1f0 100644 --- a/tests/suites/test_suite_ssl.function +++ b/tests/suites/test_suite_ssl.function @@ -7,6 +7,8 @@ #include #include +#include + typedef struct log_pattern { const char *pattern; @@ -4050,3 +4052,94 @@ void resize_buffers_renegotiate_mfl( int mfl, int legacy_renegotiation, goto exit; } /* 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 */ + ASSERT_ALLOC( out, out_len ); + + /* Set up contexts with the given hash and a dummy key */ + TEST_EQUAL( 0, mbedtls_md_setup( &ctx, md_info, 1 ) ); + TEST_EQUAL( 0, mbedtls_md_setup( &ref_ctx, md_info, 1 ) ); + memset( ref_out, 42, sizeof( ref_out ) ); + TEST_EQUAL( 0, mbedtls_md_hmac_starts( &ctx, ref_out, out_len ) ); + TEST_EQUAL( 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 */ + ASSERT_ALLOC( data, max_in_len != 0 ? max_in_len : 1 ); + + 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_EQUAL( 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_EQUAL( 0, mbedtls_md_hmac_update( &ref_ctx, add_data, + sizeof( add_data ) ) ); + TEST_EQUAL( 0, mbedtls_md_hmac_update( &ref_ctx, data, in_len ) ); + TEST_EQUAL( 0, mbedtls_md_hmac_finish( &ref_ctx, ref_out ) ); + TEST_EQUAL( 0, mbedtls_md_hmac_reset( &ref_ctx ) ); + + /* Compare */ + ASSERT_COMPARE( out, out_len, 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 */ diff --git a/visualc/VS2010/mbedTLS.vcxproj b/visualc/VS2010/mbedTLS.vcxproj index 4422b7a2d..15eb6b583 100644 --- a/visualc/VS2010/mbedTLS.vcxproj +++ b/visualc/VS2010/mbedTLS.vcxproj @@ -245,6 +245,7 @@ + From 8aa29e382fc799bd26d4c384d866adcb01c5d714 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 7 Jul 2020 12:30:39 +0200 Subject: [PATCH 03/20] 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, opening the door for making it static in the next commit. It also validates that its interface works for using it in ssl_decrypt_buf(). Signed-off-by: Manuel Pégourié-Gonnard --- library/ssl_msg.c | 167 +++++++++++++++++++++++----------------------- 1 file changed, 82 insertions(+), 85 deletions(-) diff --git a/library/ssl_msg.c b/library/ssl_msg.c index 7f233cd9f..3c1a01e42 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -322,7 +322,7 @@ int (*mbedtls_ssl_hw_record_finish)( mbedtls_ssl_context *ssl ) = NULL; 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; @@ -1080,12 +1080,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[MBEDTLS_MD_MAX_BLOCK_SIZE]; + + 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 ); @@ -1567,38 +1638,6 @@ int mbedtls_ssl_decrypt_buf( mbedtls_ssl_context const *ssl, defined(MBEDTLS_SSL_PROTO_TLS1_2) if( transform->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; - /* This size is enough to server either as input to - * md_process() or as output to md_finish() */ - unsigned char tmp[MBEDTLS_MD_MAX_BLOCK_SIZE]; - /* * The next two sizes are the minimum and maximum values of * in_msglen over all padlen values. @@ -1612,58 +1651,16 @@ int mbedtls_ssl_decrypt_buf( mbedtls_ssl_context const *ssl, const size_t max_len = rec->data_len + padlen; const size_t min_len = ( max_len > 256 ) ? max_len - 256 : 0; - memset( tmp, 0, sizeof( tmp ) ); - - switch( mbedtls_md_get_type( transform->md_ctx_dec.md_info ) ) + ret = mbedtls_ssl_cf_hmac( &transform->md_ctx_dec, + add_data, add_data_len, + data, rec->data_len, 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 = - ( add_data_len + rec->data_len + padlen + 8 ) / 64 - - ( add_data_len + rec->data_len + 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 + rec->data_len + padlen + 16 ) / 128 - - ( add_data_len + rec->data_len + 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( &transform->md_ctx_dec, add_data, - add_data_len ); - mbedtls_md_hmac_update( &transform->md_ctx_dec, data, - rec->data_len ); - /* 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 + rec->data_len, padlen ); - mbedtls_md_hmac_finish( &transform->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( &transform->md_ctx_dec ); - for( j = 0; j < extra_run + 1; j++ ) - mbedtls_md_process( &transform->md_ctx_dec, tmp ); - mbedtls_md_finish( &transform->md_ctx_dec, tmp ); - - mbedtls_md_hmac_reset( &transform->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 65a6fa3e2669cb02af5399d0f60b5bed3e62a9be Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Thu, 9 Jul 2020 09:52:17 +0200 Subject: [PATCH 04/20] Make cf_hmac() STATIC_TESTABLE MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The test function now depends on MBEDTLS_TEST_HOOKS, which is enabled by config.py full, and since there are already components in all.sh exercising the full config, this test function is sill exercised even with this new dependency. Since this is the first time a test function depends on MBEDTLS_TEST_HOOKS, fix a bug in check-names.sh that wasn't apparent so far: headers from library/*.h were not considered when looking for macro definitions. This became apparent because MBEDTLS_STATIC_TESTABLE is defined in library/common.h and started being used in library/ssl_msg.c, so was flagged as a likely typo. Signed-off-by: Manuel Pégourié-Gonnard --- library/ssl_invasive.h | 5 +++-- library/ssl_msg.c | 2 +- tests/scripts/check-names.sh | 1 + tests/scripts/list-macros.sh | 1 + tests/suites/test_suite_ssl.function | 2 +- 5 files changed, 7 insertions(+), 4 deletions(-) diff --git a/library/ssl_invasive.h b/library/ssl_invasive.h index 9697a91b2..f40f0d4f5 100644 --- a/library/ssl_invasive.h +++ b/library/ssl_invasive.h @@ -31,7 +31,8 @@ #include "common.h" #include "mbedtls/md.h" -#if defined(MBEDTLS_SSL_SOME_SUITES_USE_CBC) && \ +#if defined(MBEDTLS_TEST_HOOKS) && \ + defined(MBEDTLS_SSL_SOME_SUITES_USE_CBC) && \ ( defined(MBEDTLS_SSL_PROTO_TLS1) || \ defined(MBEDTLS_SSL_PROTO_TLS1_1) | \ defined(MBEDTLS_SSL_PROTO_TLS1_2) ) @@ -73,6 +74,6 @@ 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_TEST_HOOKS && MBEDTLS_SSL_SOME_SUITES_USE_CBC && TLS 1.0-1.2 */ #endif /* MBEDTLS_SSL_INVASIVE_H */ diff --git a/library/ssl_msg.c b/library/ssl_msg.c index 3c1a01e42..d77b65843 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -1073,7 +1073,7 @@ int mbedtls_ssl_encrypt_buf( mbedtls_ssl_context *ssl, /* * Compute HMAC of variable-length data with constant flow. */ -int mbedtls_ssl_cf_hmac( +MBEDTLS_STATIC_TESTABLE 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, diff --git a/tests/scripts/check-names.sh b/tests/scripts/check-names.sh index e2019ccad..1a8253c80 100755 --- a/tests/scripts/check-names.sh +++ b/tests/scripts/check-names.sh @@ -98,6 +98,7 @@ done printf "Likely typos: " sort -u actual-macros enum-consts > _caps HEADERS=$( ls include/mbedtls/*.h include/psa/*.h | egrep -v 'compat-1\.3\.h' ) +HEADERS="$HEADERS library/*.h" HEADERS="$HEADERS 3rdparty/everest/include/everest/everest.h 3rdparty/everest/include/everest/x25519.h" LIBRARY="$( ls library/*.c )" LIBRARY="$LIBRARY 3rdparty/everest/library/everest.c 3rdparty/everest/library/x25519.c" diff --git a/tests/scripts/list-macros.sh b/tests/scripts/list-macros.sh index 786aef925..cf6afc5a0 100755 --- a/tests/scripts/list-macros.sh +++ b/tests/scripts/list-macros.sh @@ -25,6 +25,7 @@ if [ -d include/mbedtls ]; then :; else fi HEADERS=$( ls include/mbedtls/*.h include/psa/*.h | egrep -v 'compat-1\.3\.h' ) +HEADERS="$HEADERS library/*.h" HEADERS="$HEADERS 3rdparty/everest/include/everest/everest.h 3rdparty/everest/include/everest/x25519.h" sed -n -e 's/.*#define \([a-zA-Z0-9_]*\).*/\1/p' $HEADERS \ diff --git a/tests/suites/test_suite_ssl.function b/tests/suites/test_suite_ssl.function index 711d2d1f0..a0e72dd38 100644 --- a/tests/suites/test_suite_ssl.function +++ b/tests/suites/test_suite_ssl.function @@ -4053,7 +4053,7 @@ void resize_buffers_renegotiate_mfl( int mfl, int legacy_renegotiation, } /* END_CASE */ -/* BEGIN_CASE depends_on:MBEDTLS_CIPHER_MODE_CBC:MBEDTLS_AES_C:MBEDTLS_SSL_PROTO_TLS1_2 */ +/* BEGIN_CASE depends_on:MBEDTLS_CIPHER_MODE_CBC:MBEDTLS_AES_C:MBEDTLS_SSL_PROTO_TLS1_2:MBEDTLS_TEST_HOOKS */ void ssl_cf_hmac( int hash ) { /* From 6240defd17f23c636db24caf36a4ebf975585ecb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Fri, 10 Jul 2020 09:35:54 +0200 Subject: [PATCH 05/20] 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/test/query_config.c | 8 +++++ scripts/config.py | 1 + tests/include/test/constant_flow.h | 51 ++++++++++++++++++++++++++++++ tests/scripts/all.sh | 12 +++++++ visualc/VS2010/mbedTLS.vcxproj | 1 + 8 files changed, 99 insertions(+) create mode 100644 tests/include/test/constant_flow.h diff --git a/include/mbedtls/check_config.h b/include/mbedtls/check_config.h index f2148a8b5..15cc21b64 100644 --- a/include/mbedtls/check_config.h +++ b/include/mbedtls/check_config.h @@ -186,6 +186,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 e00c546e5..124c597b6 100644 --- a/include/mbedtls/config.h +++ b/include/mbedtls/config.h @@ -1906,6 +1906,19 @@ */ //#define MBEDTLS_SSL_VARIABLE_BUFFER_LENGTH +/** + * \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_HOOKS * diff --git a/library/version_features.c b/library/version_features.c index 64e9e86db..2470d8d1d 100644 --- a/library/version_features.c +++ b/library/version_features.c @@ -557,6 +557,9 @@ static const char * const features[] = { #if defined(MBEDTLS_SSL_VARIABLE_BUFFER_LENGTH) "MBEDTLS_SSL_VARIABLE_BUFFER_LENGTH", #endif /* MBEDTLS_SSL_VARIABLE_BUFFER_LENGTH */ +#if defined(MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN) + "MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN", +#endif /* MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN */ #if defined(MBEDTLS_TEST_HOOKS) "MBEDTLS_TEST_HOOKS", #endif /* MBEDTLS_TEST_HOOKS */ diff --git a/programs/test/query_config.c b/programs/test/query_config.c index 98b065bfe..23dc51512 100644 --- a/programs/test/query_config.c +++ b/programs/test/query_config.c @@ -1538,6 +1538,14 @@ int query_config( const char *config ) } #endif /* MBEDTLS_SSL_VARIABLE_BUFFER_LENGTH */ +#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_HOOKS) if( strcmp( "MBEDTLS_TEST_HOOKS", config ) == 0 ) { diff --git a/scripts/config.py b/scripts/config.py index 3d297dc3d..793e9dfa7 100755 --- a/scripts/config.py +++ b/scripts/config.py @@ -194,6 +194,7 @@ EXCLUDE_FROM_FULL = frozenset([ 'MBEDTLS_RSA_NO_CRT', # influences the use of RSA in X.509 and TLS 'MBEDTLS_SHA512_NO_SHA384', # removes a feature 'MBEDTLS_SSL_HW_RECORD_ACCEL', # build dependency (hook functions) + 'MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN', # build dependency (clang+memsan) 'MBEDTLS_TEST_NULL_ENTROPY', # removes a feature 'MBEDTLS_X509_ALLOW_UNSUPPORTED_CRITICAL_EXTENSION', # influences the use of X.509 in TLS 'MBEDTLS_ZLIB_SUPPORT', # build dependency (libz) diff --git a/tests/include/test/constant_flow.h b/tests/include/test/constant_flow.h new file mode 100644 index 000000000..98bee7e48 --- /dev/null +++ b/tests/include/test/constant_flow.h @@ -0,0 +1,51 @@ +/** + * \file constant_flow.h + * + * \brief This file contains tools to ensure tested code has constant flow. + */ + +/* + * Copyright (C) 2020, ARM Limited, All Rights Reserved + * SPDX-License-Identifier: Apache-2.0 + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + * This file is part of mbed TLS (https://tls.mbed.org) + */ + +#ifndef TEST_CONSTANT_FLOW_H +#define TEST_CONSTANT_FLOW_H + +#if !defined(MBEDTLS_CONFIG_FILE) +#include "mbedtls/config.h" +#else +#include MBEDTLS_CONFIG_FILE +#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 */ + +#endif /* TEST_CONSTANT_FLOW_H */ diff --git a/tests/scripts/all.sh b/tests/scripts/all.sh index ec61d1962..9c9247f8f 100755 --- a/tests/scripts/all.sh +++ b/tests/scripts/all.sh @@ -1074,6 +1074,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.py full + scripts/config.py set MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN + scripts/config.py 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/visualc/VS2010/mbedTLS.vcxproj b/visualc/VS2010/mbedTLS.vcxproj index 15eb6b583..578289f17 100644 --- a/visualc/VS2010/mbedTLS.vcxproj +++ b/visualc/VS2010/mbedTLS.vcxproj @@ -232,6 +232,7 @@ + From 9670a59230def4d2a433fe81e9f5029fa71062a3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Fri, 10 Jul 2020 10:21:46 +0200 Subject: [PATCH 06/20] 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 | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/suites/test_suite_ssl.function b/tests/suites/test_suite_ssl.function index a0e72dd38..a7f544574 100644 --- a/tests/suites/test_suite_ssl.function +++ b/tests/suites/test_suite_ssl.function @@ -9,6 +9,8 @@ #include +#include + typedef struct log_pattern { const char *pattern; @@ -4115,10 +4117,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_EQUAL( 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_EQUAL( 0, mbedtls_md_hmac_update( &ref_ctx, add_data, From 7a8b1e6b7136702941b6da5fe75ca4673f402130 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Wed, 15 Jul 2020 11:52:14 +0200 Subject: [PATCH 07/20] 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_msg.c | 151 ++++++++++++++++++++++++++-------------------- 1 file changed, 86 insertions(+), 65 deletions(-) diff --git a/library/ssl_msg.c b/library/ssl_msg.c index d77b65843..9b4d1dbc7 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -1070,8 +1070,53 @@ int mbedtls_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. + * + * Only works with MD-5, SHA-1, SHA-256 and SHA-384. + * (Otherwise, computation of block_size needs to be adapted.) */ MBEDTLS_STATIC_TESTABLE int mbedtls_ssl_cf_hmac( mbedtls_md_context_t *ctx, @@ -1080,85 +1125,61 @@ MBEDTLS_STATIC_TESTABLE 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[MBEDTLS_MD_MAX_BLOCK_SIZE]; + 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 ed0e86428d166d1cf6bc06b5b19a530f7e8dbc97 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 21 Jul 2020 11:20:30 +0200 Subject: [PATCH 08/20] 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 | 8 ++++++++ library/ssl_invasive.h | 7 ++----- library/ssl_msg.c | 7 ++----- tests/suites/test_suite_ssl.function | 6 +----- 4 files changed, 13 insertions(+), 15 deletions(-) diff --git a/include/mbedtls/ssl_internal.h b/include/mbedtls/ssl_internal.h index 6bea84c34..61ac46417 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -154,6 +154,14 @@ #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 + + #if defined(MBEDTLS_ARC4_C) || defined(MBEDTLS_CIPHER_NULL_CIPHER) || \ defined(MBEDTLS_SSL_SOME_SUITES_USE_CBC) #define MBEDTLS_SSL_SOME_MODES_USE_MAC diff --git a/library/ssl_invasive.h b/library/ssl_invasive.h index f40f0d4f5..49ee83f26 100644 --- a/library/ssl_invasive.h +++ b/library/ssl_invasive.h @@ -32,10 +32,7 @@ #include "mbedtls/md.h" #if defined(MBEDTLS_TEST_HOOKS) && \ - defined(MBEDTLS_SSL_SOME_SUITES_USE_CBC) && \ - ( defined(MBEDTLS_SSL_PROTO_TLS1) || \ - defined(MBEDTLS_SSL_PROTO_TLS1_1) | \ - defined(MBEDTLS_SSL_PROTO_TLS1_2) ) + 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 @@ -74,6 +71,6 @@ 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_TEST_HOOKS && MBEDTLS_SSL_SOME_SUITES_USE_CBC && TLS 1.0-1.2 */ +#endif /* MBEDTLS_TEST_HOOKS && MBEDTLS_SSL_SOME_SUITES_USE_TLS_CBC */ #endif /* MBEDTLS_SSL_INVASIVE_H */ diff --git a/library/ssl_msg.c b/library/ssl_msg.c index 9b4d1dbc7..086db8842 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -1066,10 +1066,7 @@ int mbedtls_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), @@ -1182,7 +1179,7 @@ MBEDTLS_STATIC_TESTABLE 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 */ int mbedtls_ssl_decrypt_buf( mbedtls_ssl_context const *ssl, mbedtls_ssl_transform *transform, diff --git a/tests/suites/test_suite_ssl.function b/tests/suites/test_suite_ssl.function index a7f544574..ebb9a1a57 100644 --- a/tests/suites/test_suite_ssl.function +++ b/tests/suites/test_suite_ssl.function @@ -4055,16 +4055,12 @@ void resize_buffers_renegotiate_mfl( int mfl, int legacy_renegotiation, } /* END_CASE */ -/* BEGIN_CASE depends_on:MBEDTLS_CIPHER_MODE_CBC:MBEDTLS_AES_C:MBEDTLS_SSL_PROTO_TLS1_2:MBEDTLS_TEST_HOOKS */ +/* BEGIN_CASE depends_on:MBEDTLS_SSL_SOME_SUITES_USE_TLS_CBC:MBEDTLS_TEST_HOOKS */ 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 ca8287cbaf78a6277c934d7493ee8acfd5a797ad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Wed, 22 Jul 2020 10:29:39 +0200 Subject: [PATCH 09/20] Use test_set_step() in loop in cf_hmac test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We only have a single integer available for two nested loops, but the loop sizes are small enough compared to the integer's range that we can encode both indexes. Since the integer is displayed in decimal in case of errors, use a power of 10 to pack the two indexes together. Signed-off-by: Manuel Pégourié-Gonnard --- tests/suites/test_suite_ssl.function | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/suites/test_suite_ssl.function b/tests/suites/test_suite_ssl.function index ebb9a1a57..602dc36d5 100644 --- a/tests/suites/test_suite_ssl.function +++ b/tests/suites/test_suite_ssl.function @@ -4100,12 +4100,16 @@ void ssl_cf_hmac( int hash ) */ for( max_in_len = 0; max_in_len <= 255 + block_size; max_in_len++ ) { + test_set_step( max_in_len * 10000 ); + /* Use allocated in buffer to catch overreads */ ASSERT_ALLOC( data, max_in_len != 0 ? max_in_len : 1 ); 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++ ) { + test_set_step( max_in_len * 10000 + in_len ); + /* Set up dummy data and add_data */ rec_num++; memset( add_data, rec_num, sizeof( add_data ) ); From c3219006ff5b6c2f2c05c01319147f91e67590cd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Wed, 22 Jul 2020 10:32:52 +0200 Subject: [PATCH 10/20] Fix suboptimal use of ASSER_ALLOC() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Passing a length of 0 to it is perfectly acceptable, the macro was designed to handle it correctly. Signed-off-by: Manuel Pégourié-Gonnard --- tests/suites/test_suite_ssl.function | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/suites/test_suite_ssl.function b/tests/suites/test_suite_ssl.function index 602dc36d5..5f301b5b6 100644 --- a/tests/suites/test_suite_ssl.function +++ b/tests/suites/test_suite_ssl.function @@ -4103,7 +4103,7 @@ void ssl_cf_hmac( int hash ) test_set_step( max_in_len * 10000 ); /* Use allocated in buffer to catch overreads */ - ASSERT_ALLOC( data, max_in_len != 0 ? max_in_len : 1 ); + ASSERT_ALLOC( data, max_in_len ); 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++ ) From baccf803ad3b2e7a335092b1d74510d4db310ecf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Wed, 22 Jul 2020 10:37:27 +0200 Subject: [PATCH 11/20] 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 --- library/ssl_invasive.h | 8 +++++--- library/ssl_msg.c | 4 +++- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/library/ssl_invasive.h b/library/ssl_invasive.h index 49ee83f26..2a8d6b52d 100644 --- a/library/ssl_invasive.h +++ b/library/ssl_invasive.h @@ -42,9 +42,11 @@ * 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_msg.c b/library/ssl_msg.c index 086db8842..3d6203ce1 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -1127,7 +1127,7 @@ MBEDTLS_STATIC_TESTABLE 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 @@ -1137,6 +1137,8 @@ MBEDTLS_STATIC_TESTABLE 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 9713e13e689a747d0a531990b13aef92573b2a5c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Wed, 22 Jul 2020 10:40:31 +0200 Subject: [PATCH 12/20] 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_msg.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/ssl_msg.c b/library/ssl_msg.c index 3d6203ce1..e8f561637 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -1140,7 +1140,7 @@ MBEDTLS_STATIC_TESTABLE 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 44c9fdde6e796e8f3a05133e180ebde4ee22af48 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Wed, 22 Jul 2020 10:48:47 +0200 Subject: [PATCH 13/20] 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_msg.c | 36 ++++++++++++++++++++++++------------ 1 file changed, 24 insertions(+), 12 deletions(-) diff --git a/library/ssl_msg.c b/library/ssl_msg.c index e8f561637..fefbd863b 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -1147,39 +1147,51 @@ MBEDTLS_STATIC_TESTABLE 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 0b2112d30459df36b297d36452a8928fc450b6e5 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 14/20] 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 9c9247f8f..0a7c441f3 100755 --- a/tests/scripts/all.sh +++ b/tests/scripts/all.sh @@ -1075,14 +1075,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.py full scripts/config.py set MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN scripts/config.py 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 e0765f35d53814dc3cf36303e0759022d0830ff1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Wed, 22 Jul 2020 12:22:51 +0200 Subject: [PATCH 15/20] Use int ret = MBEDTLS_ERROR_CORRUPTION_DETECTED; idiom Co-authored-by: Gilles Peskine --- library/ssl_msg.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/ssl_msg.c b/library/ssl_msg.c index fefbd863b..ae4882db6 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -1147,7 +1147,7 @@ MBEDTLS_STATIC_TESTABLE int mbedtls_ssl_cf_hmac( unsigned char aux_out[MBEDTLS_MD_MAX_SIZE]; mbedtls_md_context_t aux; size_t offset; - int ret; + int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; mbedtls_md_init( &aux ); From 390fb4ff34e4584a2faccedec64196babe7bb724 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Fri, 24 Jul 2020 11:08:40 +0200 Subject: [PATCH 16/20] Fix typos in comments Co-authored-by: Janos Follath --- include/mbedtls/config.h | 2 +- library/ssl_invasive.h | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/include/mbedtls/config.h b/include/mbedtls/config.h index 124c597b6..91d54cadf 100644 --- a/include/mbedtls/config.h +++ b/include/mbedtls/config.h @@ -1915,7 +1915,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/library/ssl_invasive.h b/library/ssl_invasive.h index 2a8d6b52d..f04b81668 100644 --- a/library/ssl_invasive.h +++ b/library/ssl_invasive.h @@ -55,11 +55,11 @@ * 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 e747843903b145d2247305c4cba00b1042d2ab9c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Fri, 24 Jul 2020 11:09:22 +0200 Subject: [PATCH 17/20] Fix a whitespace issue Co-authored-by: Janos Follath --- library/ssl_msg.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/library/ssl_msg.c b/library/ssl_msg.c index ae4882db6..4be9c6253 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -1076,10 +1076,10 @@ int mbedtls_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 f009542747da1feecf81737e07bed3c73930ae7b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Fri, 24 Jul 2020 11:13:01 +0200 Subject: [PATCH 18/20] 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_msg.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/library/ssl_msg.c b/library/ssl_msg.c index 4be9c6253..32c1b873d 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -1091,14 +1091,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 05579c4094200e68582831a168d7abf3d939acc9 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 19/20] 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 61ac46417..6f7b45823 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -146,6 +146,7 @@ #define MBEDTLS_SSL_COMPRESSION_ADD 0 #endif +/* This macro determines whether CBC is supported. */ #if defined(MBEDTLS_CIPHER_MODE_CBC) && \ ( defined(MBEDTLS_AES_C) || \ defined(MBEDTLS_CAMELLIA_C) || \ @@ -154,6 +155,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) || \ @@ -161,7 +164,6 @@ #define MBEDTLS_SSL_SOME_SUITES_USE_TLS_CBC #endif - #if defined(MBEDTLS_ARC4_C) || defined(MBEDTLS_CIPHER_NULL_CIPHER) || \ defined(MBEDTLS_SSL_SOME_SUITES_USE_CBC) #define MBEDTLS_SSL_SOME_MODES_USE_MAC From 8ff863b9928e30fb92ab3ec69ebe0891e801110b 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 20/20] 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 91d54cadf..3bb631f19 100644 --- a/include/mbedtls/config.h +++ b/include/mbedtls/config.h @@ -1915,6 +1915,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