From 41ee83972bd0578f8bd916a7451b3f6c2fdebb90 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Thu, 25 Jun 2020 12:34:58 +0200 Subject: [PATCH 01/38] DHM: make drawing of blinding value a function MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In the next commit, we'll need to draw a second random value, in order to blind modular inversion. Having a function for that will avoid repetition. Signed-off-by: Manuel Pégourié-Gonnard --- library/dhm.c | 41 ++++++++++++++++++++++++++++------------- 1 file changed, 28 insertions(+), 13 deletions(-) diff --git a/library/dhm.c b/library/dhm.c index f8d367ee8..7c30c8494 100644 --- a/library/dhm.c +++ b/library/dhm.c @@ -350,6 +350,32 @@ cleanup: return( 0 ); } +/* + * Pick a random R in the range [2, M) for blinding purposes + */ +static int dhm_random_below( mbedtls_mpi *R, const mbedtls_mpi *M, + int (*f_rng)(void *, unsigned char *, size_t), void *p_rng ) +{ + int ret, count; + + count = 0; + do + { + MBEDTLS_MPI_CHK( mbedtls_mpi_fill_random( R, mbedtls_mpi_size( M ), f_rng, p_rng ) ); + + while( mbedtls_mpi_cmp_mpi( R, M ) >= 0 ) + MBEDTLS_MPI_CHK( mbedtls_mpi_shift_r( R, 1 ) ); + + if( count++ > 10 ) + return( MBEDTLS_ERR_MPI_NOT_ACCEPTABLE ); + } + while( mbedtls_mpi_cmp_int( R, 1 ) <= 0 ); + +cleanup: + return( ret ); +} + + /* * Use the blinding method and optimisation suggested in section 10 of: * KOCHER, Paul C. Timing attacks on implementations of Diffie-Hellman, RSA, @@ -359,7 +385,7 @@ cleanup: static int dhm_update_blinding( mbedtls_dhm_context *ctx, int (*f_rng)(void *, unsigned char *, size_t), void *p_rng ) { - int ret, count; + int ret; /* * Don't use any blinding the first time a particular X is used, @@ -394,18 +420,7 @@ static int dhm_update_blinding( mbedtls_dhm_context *ctx, */ /* Vi = random( 2, P-1 ) */ - count = 0; - do - { - MBEDTLS_MPI_CHK( mbedtls_mpi_fill_random( &ctx->Vi, mbedtls_mpi_size( &ctx->P ), f_rng, p_rng ) ); - - while( mbedtls_mpi_cmp_mpi( &ctx->Vi, &ctx->P ) >= 0 ) - MBEDTLS_MPI_CHK( mbedtls_mpi_shift_r( &ctx->Vi, 1 ) ); - - if( count++ > 10 ) - return( MBEDTLS_ERR_MPI_NOT_ACCEPTABLE ); - } - while( mbedtls_mpi_cmp_int( &ctx->Vi, 1 ) <= 0 ); + MBEDTLS_MPI_CHK( dhm_random_below( &ctx->Vi, &ctx->P, f_rng, p_rng ) ); /* Vf = Vi^-X mod P */ MBEDTLS_MPI_CHK( mbedtls_mpi_inv_mod( &ctx->Vf, &ctx->Vi, &ctx->P ) ); From d96edbc6005f23a20e031655afc8077b335b2d36 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Thu, 25 Jun 2020 12:47:22 +0200 Subject: [PATCH 02/38] DHM: blind call to mpi_inv_mod() on secret value MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- library/dhm.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/library/dhm.c b/library/dhm.c index 7c30c8494..5396debc6 100644 --- a/library/dhm.c +++ b/library/dhm.c @@ -386,6 +386,9 @@ static int dhm_update_blinding( mbedtls_dhm_context *ctx, int (*f_rng)(void *, unsigned char *, size_t), void *p_rng ) { int ret; + mbedtls_mpi R; + + mbedtls_mpi_init( &R ); /* * Don't use any blinding the first time a particular X is used, @@ -422,11 +425,21 @@ static int dhm_update_blinding( mbedtls_dhm_context *ctx, /* Vi = random( 2, P-1 ) */ MBEDTLS_MPI_CHK( dhm_random_below( &ctx->Vi, &ctx->P, f_rng, p_rng ) ); - /* Vf = Vi^-X mod P */ - MBEDTLS_MPI_CHK( mbedtls_mpi_inv_mod( &ctx->Vf, &ctx->Vi, &ctx->P ) ); + /* Vf = Vi^-X mod P + * First compute Vi^-1 = R * (R Vi)^-1, (avoiding leaks from inv_mod), + * then elevate to the Xth power. */ + MBEDTLS_MPI_CHK( dhm_random_below( &R, &ctx->P, f_rng, p_rng ) ); + MBEDTLS_MPI_CHK( mbedtls_mpi_mul_mpi( &ctx->Vf, &ctx->Vi, &R ) ); + MBEDTLS_MPI_CHK( mbedtls_mpi_mod_mpi( &ctx->Vf, &ctx->Vf, &ctx->P ) ); + MBEDTLS_MPI_CHK( mbedtls_mpi_inv_mod( &ctx->Vf, &ctx->Vf, &ctx->P ) ); + MBEDTLS_MPI_CHK( mbedtls_mpi_mul_mpi( &ctx->Vf, &ctx->Vf, &R ) ); + MBEDTLS_MPI_CHK( mbedtls_mpi_mod_mpi( &ctx->Vf, &ctx->Vf, &ctx->P ) ); + MBEDTLS_MPI_CHK( mbedtls_mpi_exp_mod( &ctx->Vf, &ctx->Vf, &ctx->X, &ctx->P, &ctx->RP ) ); cleanup: + mbedtls_mpi_free( &R ); + return( ret ); } From 86ad5be18a4694848108ba9f950e93d8de436ee4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Fri, 26 Jun 2020 11:03:19 +0200 Subject: [PATCH 03/38] RSA: remove redundant GCD call in prepare_blinding() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit inv_mod() already returns a specific error code if the value is not invertible, so no need to check in advance that it is. Also, this is a preparation for blinding the call to inv_mod(), which is made easier by avoiding the redundancy (otherwise the call to gcd() would need to be blinded too). Signed-off-by: Manuel Pégourié-Gonnard --- library/rsa.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/library/rsa.c b/library/rsa.c index af1cef651..7c1a73ff0 100644 --- a/library/rsa.c +++ b/library/rsa.c @@ -826,11 +826,14 @@ static int rsa_prepare_blinding( mbedtls_rsa_context *ctx, return( MBEDTLS_ERR_RSA_RNG_FAILED ); MBEDTLS_MPI_CHK( mbedtls_mpi_fill_random( &ctx->Vf, ctx->len - 1, f_rng, p_rng ) ); - MBEDTLS_MPI_CHK( mbedtls_mpi_gcd( &ctx->Vi, &ctx->Vf, &ctx->N ) ); - } while( mbedtls_mpi_cmp_int( &ctx->Vi, 1 ) != 0 ); + + ret = mbedtls_mpi_inv_mod( &ctx->Vi, &ctx->Vf, &ctx->N ); + if( ret != 0 && ret != MBEDTLS_ERR_MPI_NOT_ACCEPTABLE ) + goto cleanup; + + } while( ret == MBEDTLS_ERR_MPI_NOT_ACCEPTABLE ) /* Blinding value: Vi = Vf^(-e) mod N */ - MBEDTLS_MPI_CHK( mbedtls_mpi_inv_mod( &ctx->Vi, &ctx->Vf, &ctx->N ) ); MBEDTLS_MPI_CHK( mbedtls_mpi_exp_mod( &ctx->Vi, &ctx->Vi, &ctx->E, &ctx->N, &ctx->RN ) ); From 49e94e3889e90aa4e5bbce817a5c93efab2c4df9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Fri, 26 Jun 2020 11:19:12 +0200 Subject: [PATCH 04/38] RSA: blind call to mpi_inv_mod() on secret value MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- library/rsa.c | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-) diff --git a/library/rsa.c b/library/rsa.c index 7c1a73ff0..41643d253 100644 --- a/library/rsa.c +++ b/library/rsa.c @@ -808,6 +808,9 @@ static int rsa_prepare_blinding( mbedtls_rsa_context *ctx, int (*f_rng)(void *, unsigned char *, size_t), void *p_rng ) { int ret, count = 0; + mbedtls_mpi R; + + mbedtls_mpi_init( &R ); if( ctx->Vf.p != NULL ) { @@ -827,17 +830,32 @@ static int rsa_prepare_blinding( mbedtls_rsa_context *ctx, MBEDTLS_MPI_CHK( mbedtls_mpi_fill_random( &ctx->Vf, ctx->len - 1, f_rng, p_rng ) ); - ret = mbedtls_mpi_inv_mod( &ctx->Vi, &ctx->Vf, &ctx->N ); - if( ret != 0 && ret != MBEDTLS_ERR_MPI_NOT_ACCEPTABLE ) + /* Compute the Vf^1 as R * (R Vf)^-1 to avoid leaks from inv_mod. + * There's a negligible but non-zero probability that R is not + * invertible mod N, in that case we'd just loop one more time, + * just as if Vf itself wasn't invertible - no need to distinguish. */ + MBEDTLS_MPI_CHK( mbedtls_mpi_fill_random( &R, ctx->len - 1, f_rng, p_rng ) ); + MBEDTLS_MPI_CHK( mbedtls_mpi_mul_mpi( &ctx->Vi, &ctx->Vf, &R ) ); + MBEDTLS_MPI_CHK( mbedtls_mpi_mod_mpi( &ctx->Vi, &ctx->Vi, &ctx->N ) ); + + ret = mbedtls_mpi_inv_mod( &ctx->Vi, &ctx->Vi, &ctx->N ); + if( ret == MBEDTLS_ERR_MPI_NOT_ACCEPTABLE ) + continue; + if( ret != 0 ) goto cleanup; - } while( ret == MBEDTLS_ERR_MPI_NOT_ACCEPTABLE ) + MBEDTLS_MPI_CHK( mbedtls_mpi_mul_mpi( &ctx->Vi, &ctx->Vi, &R ) ); + MBEDTLS_MPI_CHK( mbedtls_mpi_mod_mpi( &ctx->Vi, &ctx->Vi, &ctx->N ) ); + } while( 0 ); - /* Blinding value: Vi = Vf^(-e) mod N */ + /* Blinding value: Vi = Vf^(-e) mod N + * (Vi already contains Vf^-1 at this point) */ MBEDTLS_MPI_CHK( mbedtls_mpi_exp_mod( &ctx->Vi, &ctx->Vi, &ctx->E, &ctx->N, &ctx->RN ) ); cleanup: + mbedtls_mpi_free( &R ); + return( ret ); } From 8be9d3b8333b42f7319305bc93950ec0aabda303 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Fri, 26 Jun 2020 11:33:41 +0200 Subject: [PATCH 05/38] Add ChangeLog entry for base blinding protection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- ChangeLog.d/protect-base-blinding.txt | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 ChangeLog.d/protect-base-blinding.txt diff --git a/ChangeLog.d/protect-base-blinding.txt b/ChangeLog.d/protect-base-blinding.txt new file mode 100644 index 000000000..ca0600cee --- /dev/null +++ b/ChangeLog.d/protect-base-blinding.txt @@ -0,0 +1,6 @@ +Security + * Fix side channel in RSA private key operations and static (finite-field) + Diffie-Hellman. An adversary with precise enough timing and memory access + information (typically an untrusted operating system attacking a secure + enclave) could bypass an existing counter-measure (base blinding) and + potentially fully recover the private key. From cadcf4cec6255a03525474c79f3d96f38c88d860 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Thu, 16 Jul 2020 09:23:30 +0200 Subject: [PATCH 06/38] Fix memory leak on error path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- library/rsa.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/library/rsa.c b/library/rsa.c index 41643d253..d2e36e1a8 100644 --- a/library/rsa.c +++ b/library/rsa.c @@ -826,7 +826,10 @@ static int rsa_prepare_blinding( mbedtls_rsa_context *ctx, /* Unblinding value: Vf = random number, invertible mod N */ do { if( count++ > 10 ) - return( MBEDTLS_ERR_RSA_RNG_FAILED ); + { + ret = MBEDTLS_ERR_RSA_RNG_FAILED; + goto cleanup; + } MBEDTLS_MPI_CHK( mbedtls_mpi_fill_random( &ctx->Vf, ctx->len - 1, f_rng, p_rng ) ); From 87a602dd67a3369bcab644e8c8279c5a6bcc78ec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Thu, 16 Jul 2020 09:48:54 +0200 Subject: [PATCH 07/38] Clarify some comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- library/rsa.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/library/rsa.c b/library/rsa.c index d2e36e1a8..06388addb 100644 --- a/library/rsa.c +++ b/library/rsa.c @@ -833,25 +833,27 @@ static int rsa_prepare_blinding( mbedtls_rsa_context *ctx, MBEDTLS_MPI_CHK( mbedtls_mpi_fill_random( &ctx->Vf, ctx->len - 1, f_rng, p_rng ) ); - /* Compute the Vf^1 as R * (R Vf)^-1 to avoid leaks from inv_mod. - * There's a negligible but non-zero probability that R is not - * invertible mod N, in that case we'd just loop one more time, - * just as if Vf itself wasn't invertible - no need to distinguish. */ + /* Compute Vf^-1 as R * (R Vf)^-1 to avoid leaks from inv_mod. */ MBEDTLS_MPI_CHK( mbedtls_mpi_fill_random( &R, ctx->len - 1, f_rng, p_rng ) ); MBEDTLS_MPI_CHK( mbedtls_mpi_mul_mpi( &ctx->Vi, &ctx->Vf, &R ) ); MBEDTLS_MPI_CHK( mbedtls_mpi_mod_mpi( &ctx->Vi, &ctx->Vi, &ctx->N ) ); + /* At this point, Vi is invertible mod N if and only if both Vf and R + * are invertible mod N. If one of them isn't, we don't need to know + * which one, we just loop and choose new values for both of them. + * (Each iteration succeeds with overwhelming probability.) */ ret = mbedtls_mpi_inv_mod( &ctx->Vi, &ctx->Vi, &ctx->N ); if( ret == MBEDTLS_ERR_MPI_NOT_ACCEPTABLE ) continue; if( ret != 0 ) goto cleanup; + /* Finish the computation of Vf^-1 = R * (R Vf)^-1 */ MBEDTLS_MPI_CHK( mbedtls_mpi_mul_mpi( &ctx->Vi, &ctx->Vi, &R ) ); MBEDTLS_MPI_CHK( mbedtls_mpi_mod_mpi( &ctx->Vi, &ctx->Vi, &ctx->N ) ); } while( 0 ); - /* Blinding value: Vi = Vf^(-e) mod N + /* Blinding value: Vi = Vf^(-e) mod N * (Vi already contains Vf^-1 at this point) */ MBEDTLS_MPI_CHK( mbedtls_mpi_exp_mod( &ctx->Vi, &ctx->Vi, &ctx->E, &ctx->N, &ctx->RN ) ); From a60d0f2acba7dbc677ec9016784d013683e0bc3c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 28 Jul 2020 09:55:33 +0200 Subject: [PATCH 08/38] Factor repeated preprocessor condition to a macro MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The condition is a complex and repeated a few times. There were already some inconsistencies in the repetitions as some of them forgot about DES. Signed-off-by: Manuel Pégourié-Gonnard --- include/mbedtls/ssl_internal.h | 9 +++++++++ library/ssl_tls.c | 15 +++++---------- 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/include/mbedtls/ssl_internal.h b/include/mbedtls/ssl_internal.h index b371094f1..06188bebc 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -152,6 +152,15 @@ #define MBEDTLS_SSL_RETRANS_WAITING 2 #define MBEDTLS_SSL_RETRANS_FINISHED 3 +/* For CBC-specific encrypt/decrypt code */ +#if defined(MBEDTLS_CIPHER_MODE_CBC) && \ + ( defined(MBEDTLS_AES_C) || \ + defined(MBEDTLS_CAMELLIA_C) || \ + defined(MBEDTLS_ARIA_C) || \ + defined(MBEDTLS_DES_C) ) +#define MBEDTLS_SSL_SOME_SUITES_USE_CBC +#endif + /* * Allow extra bytes for record, authentication and encryption overhead: * counter (8) + header (5) + IV(16) + MAC (16-48) + padding (0-256) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index a40b46a1c..34485d2ce 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1433,8 +1433,7 @@ static void ssl_mac( mbedtls_md_context_t *md_ctx, #endif /* MBEDTLS_SSL_PROTO_SSL3 */ #if defined(MBEDTLS_ARC4_C) || defined(MBEDTLS_CIPHER_NULL_CIPHER) || \ - ( defined(MBEDTLS_CIPHER_MODE_CBC) && \ - ( defined(MBEDTLS_AES_C) || defined(MBEDTLS_CAMELLIA_C) || defined(MBEDTLS_ARIA_C)) ) + defined(MBEDTLS_SSL_SOME_SUITES_USE_CBC) #define SSL_SOME_MODES_USE_MAC #endif @@ -1669,8 +1668,7 @@ static int ssl_encrypt_buf( mbedtls_ssl_context *ssl ) } else #endif /* MBEDTLS_GCM_C || MBEDTLS_CCM_C */ -#if defined(MBEDTLS_CIPHER_MODE_CBC) && \ - ( defined(MBEDTLS_AES_C) || defined(MBEDTLS_CAMELLIA_C) || defined(MBEDTLS_ARIA_C) ) +#if defined(MBEDTLS_SSL_SOME_SUITES_USE_CBC) if( mode == MBEDTLS_MODE_CBC ) { int ret; @@ -1789,8 +1787,7 @@ static int ssl_encrypt_buf( mbedtls_ssl_context *ssl ) #endif /* MBEDTLS_SSL_ENCRYPT_THEN_MAC */ } else -#endif /* MBEDTLS_CIPHER_MODE_CBC && - ( MBEDTLS_AES_C || MBEDTLS_CAMELLIA_C || MBEDTLS_ARIA_C ) */ +#endif /* MBEDTLS_SSL_SOME_SUITES_USE_CBC */ { MBEDTLS_SSL_DEBUG_MSG( 1, ( "should never happen" ) ); return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); @@ -1962,8 +1959,7 @@ static int ssl_decrypt_buf( mbedtls_ssl_context *ssl ) } else #endif /* MBEDTLS_GCM_C || MBEDTLS_CCM_C */ -#if defined(MBEDTLS_CIPHER_MODE_CBC) && \ - ( defined(MBEDTLS_AES_C) || defined(MBEDTLS_CAMELLIA_C) || defined(MBEDTLS_ARIA_C) ) +#if defined(MBEDTLS_SSL_SOME_SUITES_USE_CBC) if( mode == MBEDTLS_MODE_CBC ) { /* @@ -2176,8 +2172,7 @@ static int ssl_decrypt_buf( mbedtls_ssl_context *ssl ) ssl->in_msglen -= padlen; } else -#endif /* MBEDTLS_CIPHER_MODE_CBC && - ( MBEDTLS_AES_C || MBEDTLS_CAMELLIA_C || MBEDTLS_ARIA_C ) */ +#endif /* MBEDTLS_SSL_SOME_SUITES_USE_CBC) */ { MBEDTLS_SSL_DEBUG_MSG( 1, ( "should never happen" ) ); return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); From fde750550dc96b76be88d34ef27a4ed3e513279d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 28 Jul 2020 10:19:45 +0200 Subject: [PATCH 09/38] Add dummy constant-flow HMAC function with tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The dummy implementation is not constant-flow at all for now, it's just here as a starting point and a support for developing the tests and putting the infrastructure in place. Depending on the implementation strategy, there might be various corner cases depending on where the lengths fall relative to block boundaries. So it seems safer to just test all possible lengths in a given range than to use only a few randomly-chosen values. Signed-off-by: Manuel Pégourié-Gonnard --- include/mbedtls/ssl_internal.h | 44 +++++++++++++ library/ssl_tls.c | 26 ++++++++ tests/suites/test_suite_ssl.data | 16 +++++ tests/suites/test_suite_ssl.function | 93 ++++++++++++++++++++++++++++ 4 files changed, 179 insertions(+) diff --git a/include/mbedtls/ssl_internal.h b/include/mbedtls/ssl_internal.h index 06188bebc..9ad04181a 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -852,6 +852,50 @@ int mbedtls_ssl_get_key_exchange_md_tls1_2( mbedtls_ssl_context *ssl, #endif /* MBEDTLS_SSL_PROTO_TLS1 || MBEDTLS_SSL_PROTO_TLS1_1 || \ MBEDTLS_SSL_PROTO_TLS1_2 */ +#if defined(MBEDTLS_SSL_SOME_SUITES_USE_CBC) && \ + ( defined(MBEDTLS_SSL_PROTO_TLS1) || \ + defined(MBEDTLS_SSL_PROTO_TLS1_1) | \ + defined(MBEDTLS_SSL_PROTO_TLS1_2) ) +/** \brief Compute the HMAC of variable-length data with constant flow. + * + * This function computes the HMAC of the concatenation of \p add_data and \p + * data, and does with a code flow and memory access pattern that does not + * depend on \p data_len_secret, but only on \p min_data_len and \p + * max_data_len. In particular, this function always reads exactly \p + * max_data_len bytes from \p data. + * + * \param ctx The HMAC context. It must have keys configured + * with mbedtls_md_hmac_starts(). It is reset using + * mbedtls_md_hmac_reset() after the computation is + * complete to prepare for the next computation. + * \param add_data The additional data prepended to \p data. This + * must point to a readable buffer of \p add_data_len + * bytes. + * \param add_data_len The length of \p add_data in bytes. + * \param data The data appended to \p add_data. This must point + * to a readable buffer of \p max_data_len bytes. + * \param data_len_secret The length of the data to process in \p data. + * This must be no less than \p min_data_len and no + * greated than \p max_data_len. + * \param min_data_len The minimal length of \p data in bytes. + * \param max_data_len The maximal length of \p data in bytes. + * \param output The HMAC will be written here. This must point to + * a writeable buffer of sufficient size to hold the + * HMAC value. + * + * \retval 0 + * Success. + * \retval MBEDTLS_ERR_PLATFORM_HW_ACCEL_FAILED + * The hardware accelerator failed. + */ +int mbedtls_ssl_cf_hmac( + mbedtls_md_context_t *ctx, + const unsigned char *add_data, size_t add_data_len, + const unsigned char *data, size_t data_len_secret, + size_t min_data_len, size_t max_data_len, + unsigned char *output ); +#endif /* MBEDTLS_SSL_SOME_SUITES_USE_CBC && TLS 1.0-1.2 */ + #ifdef __cplusplus } #endif diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 34485d2ce..8bc79be05 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1805,6 +1805,32 @@ static int ssl_encrypt_buf( mbedtls_ssl_context *ssl ) return( 0 ); } +#if defined(MBEDTLS_SSL_SOME_SUITES_USE_CBC) && \ + ( defined(MBEDTLS_SSL_PROTO_TLS1) || \ + defined(MBEDTLS_SSL_PROTO_TLS1_1) || \ + defined(MBEDTLS_SSL_PROTO_TLS1_2) ) +/* + * Compute HMAC of variable-length data with constant flow. + */ +int mbedtls_ssl_cf_hmac( + mbedtls_md_context_t *ctx, + const unsigned char *add_data, size_t add_data_len, + const unsigned char *data, size_t data_len_secret, + size_t min_data_len, size_t max_data_len, + unsigned char *output ) +{ + /* WORK IN PROGRESS - THIS IS NOT CONSTANT FLOW AT ALL */ + (void) min_data_len; + (void) max_data_len; + mbedtls_md_hmac_update( ctx, add_data, add_data_len ); + mbedtls_md_hmac_update( ctx, data, data_len_secret ); + mbedtls_md_hmac_finish( ctx, output ); + mbedtls_md_hmac_reset( ctx ); + + return( 0 ); +} +#endif /* MBEDTLS_SSL_SOME_SUITES_USE_CBC && TLS 1.0-1.2 */ + static int ssl_decrypt_buf( mbedtls_ssl_context *ssl ) { mbedtls_cipher_mode_t mode; diff --git a/tests/suites/test_suite_ssl.data b/tests/suites/test_suite_ssl.data index 147350744..db92a72bc 100644 --- a/tests/suites/test_suite_ssl.data +++ b/tests/suites/test_suite_ssl.data @@ -57,3 +57,19 @@ ssl_dtls_replay:"abcd12340000abcd12340100":"abcd123400ff":0 SSL SET_HOSTNAME memory leak: call ssl_set_hostname twice ssl_set_hostname_twice:"server0":"server1" + +Constant-flow HMAC: MD5 +depends_on:MBEDTLS_MD5_C +ssl_cf_hmac:MBEDTLS_MD_MD5 + +Constant-flow HMAC: SHA1 +depends_on:MBEDTLS_SHA1_C +ssl_cf_hmac:MBEDTLS_MD_SHA1 + +Constant-flow HMAC: SHA256 +depends_on:MBEDTLS_SHA256_C +ssl_cf_hmac:MBEDTLS_MD_SHA256 + +Constant-flow HMAC: SHA384 +depends_on:MBEDTLS_SHA512_C:!MBEDTLS_SHA512_NO_SHA384 +ssl_cf_hmac:MBEDTLS_MD_SHA384 diff --git a/tests/suites/test_suite_ssl.function b/tests/suites/test_suite_ssl.function index 326f22d3b..fc953a125 100644 --- a/tests/suites/test_suite_ssl.function +++ b/tests/suites/test_suite_ssl.function @@ -52,3 +52,96 @@ void ssl_set_hostname_twice( char *hostname0, char *hostname1 ) mbedtls_ssl_free( &ssl ); } /* END_CASE */ + +/* BEGIN_CASE depends_on:MBEDTLS_CIPHER_MODE_CBC:MBEDTLS_AES_C:MBEDTLS_SSL_PROTO_TLS1_2 */ +void ssl_cf_hmac( int hash ) +{ + /* + * Test the function mbedtls_ssl_cf_hmac() against a reference + * implementation. + * + * Note: the dependency is actually on TLS 1.0-1.2 and (AES or ARIA or + * Camellia or DES), but since the test framework doesn't support + * alternation in dependencies, just depend on the most common. + */ + mbedtls_md_context_t ctx, ref_ctx; + const mbedtls_md_info_t *md_info; + size_t out_len, block_size; + size_t min_in_len, in_len, max_in_len, i; + /* TLS additional data is 13 bytes (hence the "lucky 13" name) */ + unsigned char add_data[13]; + unsigned char ref_out[MBEDTLS_MD_MAX_SIZE]; + unsigned char *data = NULL; + unsigned char *out = NULL; + unsigned char rec_num = 0; + + mbedtls_md_init( &ctx ); + mbedtls_md_init( &ref_ctx ); + + md_info = mbedtls_md_info_from_type( hash ); + TEST_ASSERT( md_info != NULL ); + out_len = mbedtls_md_get_size( md_info ); + TEST_ASSERT( out_len != 0 ); + block_size = hash == MBEDTLS_MD_SHA384 ? 128 : 64; + + /* Use allocated out buffer to catch overwrites */ + out = mbedtls_calloc( 1, out_len ); + TEST_ASSERT( out != NULL ); + + /* Set up contexts with the given hash and a dummy key */ + TEST_ASSERT( 0 == mbedtls_md_setup( &ctx, md_info, 1 ) ); + TEST_ASSERT( 0 == mbedtls_md_setup( &ref_ctx, md_info, 1 ) ); + memset( ref_out, 42, sizeof( ref_out ) ); + TEST_ASSERT( 0 == mbedtls_md_hmac_starts( &ctx, ref_out, out_len ) ); + TEST_ASSERT( 0 == mbedtls_md_hmac_starts( &ref_ctx, ref_out, out_len ) ); + memset( ref_out, 0, sizeof( ref_out ) ); + + /* + * Test all possible lengths up to a point. The difference between + * max_in_len and min_in_len is at most 255, and make sure they both vary + * by at least one block size. + */ + for( max_in_len = 0; max_in_len <= 255 + block_size; max_in_len++ ) + { + /* Use allocated in buffer to catch overreads */ + data = mbedtls_calloc( 1, max_in_len ); + TEST_ASSERT( data != NULL || max_in_len == 0 ); + + min_in_len = max_in_len > 255 ? max_in_len - 255 : 0; + for( in_len = min_in_len; in_len <= max_in_len; in_len++ ) + { + /* Set up dummy data and add_data */ + rec_num++; + memset( add_data, rec_num, sizeof( add_data ) ); + for( i = 0; i < in_len; i++ ) + data[i] = ( i & 0xff ) ^ rec_num; + + /* Get the function's result */ + TEST_ASSERT( 0 == mbedtls_ssl_cf_hmac( &ctx, add_data, sizeof( add_data ), + data, in_len, + min_in_len, max_in_len, + out ) ); + + /* Compute the reference result */ + TEST_ASSERT( 0 == mbedtls_md_hmac_update( &ref_ctx, add_data, + sizeof( add_data ) ) ); + TEST_ASSERT( 0 == mbedtls_md_hmac_update( &ref_ctx, data, in_len ) ); + TEST_ASSERT( 0 == mbedtls_md_hmac_finish( &ref_ctx, ref_out ) ); + TEST_ASSERT( 0 == mbedtls_md_hmac_reset( &ref_ctx ) ); + + /* Compare */ + TEST_ASSERT( 0 == memcmp( out, ref_out, out_len ) ); + } + + mbedtls_free( data ); + data = NULL; + } + +exit: + mbedtls_md_free( &ref_ctx ); + mbedtls_md_free( &ctx ); + + mbedtls_free( data ); + mbedtls_free( out ); +} +/* END_CASE */ From 368fc65f80db347fa10849dbf3081969056a79c2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 28 Jul 2020 10:43:03 +0200 Subject: [PATCH 10/38] 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 8bc79be05..fbf43ab8f 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1445,7 +1445,7 @@ static void ssl_mac( mbedtls_md_context_t *md_ctx, defined(MBEDTLS_SSL_PROTO_TLS1_2) ) /* This function makes sure every byte in the memory region is accessed * (in ascending addresses order) */ -static void ssl_read_memory( unsigned char *p, size_t len ) +static void ssl_read_memory( const unsigned char *p, size_t len ) { unsigned char acc = 0; volatile unsigned char force; @@ -1819,12 +1819,83 @@ int mbedtls_ssl_cf_hmac( size_t min_data_len, size_t max_data_len, unsigned char *output ) { - /* WORK IN PROGRESS - THIS IS NOT CONSTANT FLOW AT ALL */ - (void) min_data_len; - (void) max_data_len; + /* WORK IN PROGRESS - THIS IS ONLY PSEUDO-CONTANT-TIME */ + + /* + * Process MAC and always update for padlen afterwards to make + * total time independent of padlen. + * + * Known timing attacks: + * - Lucky Thirteen (http://www.isg.rhul.ac.uk/tls/TLStiming.pdf) + * + * To compensate for different timings for the MAC calculation + * depending on how much padding was removed (which is determined + * by padlen), process extra_run more blocks through the hash + * function. + * + * The formula in the paper is + * extra_run = ceil( (L1-55) / 64 ) - ceil( (L2-55) / 64 ) + * where L1 is the size of the header plus the decrypted message + * plus CBC padding and L2 is the size of the header plus the + * decrypted message. This is for an underlying hash function + * with 64-byte blocks. + * We use ( (Lx+8) / 64 ) to handle 'negative Lx' values + * correctly. We round down instead of up, so -56 is the correct + * value for our calculations instead of -55. + * + * Repeat the formula rather than defining a block_size variable. + * This avoids requiring division by a variable at runtime + * (which would be marginally less efficient and would require + * linking an extra division function in some builds). + */ + size_t j, extra_run = 0; + /* This size is enough to server either as input to + * md_process() or as output to md_finish() */ + unsigned char tmp[128]; + + memset( tmp, 0, sizeof( tmp ) ); + + switch( mbedtls_md_get_type( ctx->md_info ) ) + { +#if defined(MBEDTLS_MD5_C) || defined(MBEDTLS_SHA1_C) || \ +defined(MBEDTLS_SHA256_C) + case MBEDTLS_MD_MD5: + case MBEDTLS_MD_SHA1: + case MBEDTLS_MD_SHA256: + /* 8 bytes of message size, 64-byte compression blocks */ + extra_run = ( add_data_len + max_data_len + 8 ) / 64 - + ( add_data_len + data_len_secret + 8 ) / 64; + break; +#endif +#if defined(MBEDTLS_SHA512_C) + case MBEDTLS_MD_SHA384: + /* 16 bytes of message size, 128-byte compression blocks */ + extra_run = ( add_data_len + max_data_len + 16 ) / 128 - + ( add_data_len + data_len_secret + 16 ) / 128; + break; +#endif + default: + return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); + } + mbedtls_md_hmac_update( ctx, add_data, add_data_len ); mbedtls_md_hmac_update( ctx, data, data_len_secret ); + /* Make sure we access everything even when padlen > 0. This + * makes the synchronisation requirements for just-in-time + * Prime+Probe attacks much tighter and hopefully impractical. */ + ssl_read_memory( data + min_data_len, max_data_len - min_data_len ); mbedtls_md_hmac_finish( ctx, output ); + + /* Dummy calls to compression function. + * Call mbedtls_md_process at least once due to cache attacks + * that observe whether md_process() was called of not. + * Respect the usual start-(process|update)-finish sequence for + * the sake of hardware accelerators that might require it. */ + mbedtls_md_starts( ctx ); + for( j = 0; j < extra_run + 1; j++ ) + mbedtls_md_process( ctx, tmp ); + mbedtls_md_finish( ctx, tmp ); + mbedtls_md_hmac_reset( ctx ); return( 0 ); @@ -2238,34 +2309,8 @@ static int ssl_decrypt_buf( mbedtls_ssl_context *ssl ) defined(MBEDTLS_SSL_PROTO_TLS1_2) if( ssl->minor_ver > MBEDTLS_SSL_MINOR_VERSION_0 ) { - /* - * Process MAC and always update for padlen afterwards to make - * total time independent of padlen. - * - * Known timing attacks: - * - Lucky Thirteen (http://www.isg.rhul.ac.uk/tls/TLStiming.pdf) - * - * To compensate for different timings for the MAC calculation - * depending on how much padding was removed (which is determined - * by padlen), process extra_run more blocks through the hash - * function. - * - * The formula in the paper is - * extra_run = ceil( (L1-55) / 64 ) - ceil( (L2-55) / 64 ) - * where L1 is the size of the header plus the decrypted message - * plus CBC padding and L2 is the size of the header plus the - * decrypted message. This is for an underlying hash function - * with 64-byte blocks. - * We use ( (Lx+8) / 64 ) to handle 'negative Lx' values - * correctly. We round down instead of up, so -56 is the correct - * value for our calculations instead of -55. - * - * Repeat the formula rather than defining a block_size variable. - * This avoids requiring division by a variable at runtime - * (which would be marginally less efficient and would require - * linking an extra division function in some builds). - */ - size_t j, extra_run = 0; + int ret; + unsigned char add_data[13]; /* * The next two sizes are the minimum and maximum values of @@ -2280,60 +2325,21 @@ static int ssl_decrypt_buf( mbedtls_ssl_context *ssl ) const size_t max_len = ssl->in_msglen + padlen; const size_t min_len = ( max_len > 256 ) ? max_len - 256 : 0; - switch( ssl->transform_in->ciphersuite_info->mac ) + memcpy( add_data + 0, ssl->in_ctr, 8 ); + memcpy( add_data + 8, ssl->in_hdr, 3 ); + memcpy( add_data + 11, ssl->in_len, 2 ); + + ret = mbedtls_ssl_cf_hmac( &ssl->transform_in->md_ctx_dec, + add_data, sizeof( add_data ), + ssl->in_msg, ssl->in_msglen, + min_len, max_len, + mac_expect ); + if( ret != 0 ) { -#if defined(MBEDTLS_MD5_C) || defined(MBEDTLS_SHA1_C) || \ - defined(MBEDTLS_SHA256_C) - case MBEDTLS_MD_MD5: - case MBEDTLS_MD_SHA1: - case MBEDTLS_MD_SHA256: - /* 8 bytes of message size, 64-byte compression blocks */ - extra_run = ( 13 + ssl->in_msglen + padlen + 8 ) / 64 - - ( 13 + ssl->in_msglen + 8 ) / 64; - break; -#endif -#if defined(MBEDTLS_SHA512_C) - case MBEDTLS_MD_SHA384: - /* 16 bytes of message size, 128-byte compression blocks */ - extra_run = ( 13 + ssl->in_msglen + padlen + 16 ) / 128 - - ( 13 + ssl->in_msglen + 16 ) / 128; - break; -#endif - default: - MBEDTLS_SSL_DEBUG_MSG( 1, ( "should never happen" ) ); - return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); + MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_ssl_cf_hmac", ret ); + return( ret ); } - extra_run &= correct * 0xFF; - - mbedtls_md_hmac_update( &ssl->transform_in->md_ctx_dec, ssl->in_ctr, 8 ); - mbedtls_md_hmac_update( &ssl->transform_in->md_ctx_dec, ssl->in_hdr, 3 ); - mbedtls_md_hmac_update( &ssl->transform_in->md_ctx_dec, ssl->in_len, 2 ); - mbedtls_md_hmac_update( &ssl->transform_in->md_ctx_dec, ssl->in_msg, - ssl->in_msglen ); - /* Make sure we access everything even when padlen > 0. This - * makes the synchronisation requirements for just-in-time - * Prime+Probe attacks much tighter and hopefully impractical. */ - ssl_read_memory( ssl->in_msg + ssl->in_msglen, padlen ); - mbedtls_md_hmac_finish( &ssl->transform_in->md_ctx_dec, mac_expect ); - - /* Dummy calls to compression function. - * Call mbedtls_md_process at least once due to cache attacks - * that observe whether md_process() was called of not. - * Respect the usual start-(process|update)-finish sequence for - * the sake of hardware accelerators that might require it. */ - mbedtls_md_starts( &ssl->transform_in->md_ctx_dec ); - for( j = 0; j < extra_run + 1; j++ ) - mbedtls_md_process( &ssl->transform_in->md_ctx_dec, ssl->in_msg ); - { - /* The switch statement above already checks that we're using - * one of MD-5, SHA-1, SHA-256 or SHA-384. */ - unsigned char tmp[384 / 8]; - mbedtls_md_finish( &ssl->transform_in->md_ctx_dec, tmp ); - } - - mbedtls_md_hmac_reset( &ssl->transform_in->md_ctx_dec ); - /* Make sure we access all the memory that could contain the MAC, * before we check it in the next code block. This makes the * synchronisation requirements for just-in-time Prime+Probe From a237722118fea83d9fb261327d8b7357de43148b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 28 Jul 2020 10:53:06 +0200 Subject: [PATCH 11/38] Add MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This option allows to test the constant-flow nature of selected code, using MemSan and the fundamental observation behind ctgrind that the set of operations allowed on undefined memory by dynamic analysers is the same as the set of operations allowed on secret data to avoid leaking it to a local attacker via side channels, namely, any operation except branching and dereferencing. (This isn't the full story, as on some CPUs some instructions have variable execution depending on the inputs, most notably division and on some cores multiplication. However, testing that no branch or memory access depends on secret data is already a good start.) Signed-off-by: Manuel Pégourié-Gonnard --- include/mbedtls/check_config.h | 10 ++++++++++ include/mbedtls/config.h | 13 +++++++++++++ library/version_features.c | 3 +++ programs/ssl/query_config.c | 8 ++++++++ scripts/config.pl | 1 + tests/scripts/all.sh | 12 ++++++++++++ tests/suites/helpers.function | 16 ++++++++++++++++ 7 files changed, 63 insertions(+) diff --git a/include/mbedtls/check_config.h b/include/mbedtls/check_config.h index 8ce73ceff..2bcf960e9 100644 --- a/include/mbedtls/check_config.h +++ b/include/mbedtls/check_config.h @@ -199,6 +199,16 @@ #error "MBEDTLS_ENTROPY_FORCE_SHA256 defined, but not all prerequisites" #endif +#if defined(__has_feature) +#if __has_feature(memory_sanitizer) +#define MBEDTLS_HAS_MEMSAN +#endif +#endif +#if defined(MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN) && !defined(MBEDTLS_HAS_MEMSAN) +#error "MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN requires building with MemorySanitizer" +#endif +#undef MBEDTLS_HAS_MEMSAN + #if defined(MBEDTLS_TEST_NULL_ENTROPY) && \ ( !defined(MBEDTLS_ENTROPY_C) || !defined(MBEDTLS_NO_DEFAULT_ENTROPY_SOURCES) ) #error "MBEDTLS_TEST_NULL_ENTROPY defined, but not all prerequisites" diff --git a/include/mbedtls/config.h b/include/mbedtls/config.h index f7e55aef5..967668080 100644 --- a/include/mbedtls/config.h +++ b/include/mbedtls/config.h @@ -551,6 +551,19 @@ //#define MBEDTLS_ECP_RANDOMIZE_MXZ_ALT //#define MBEDTLS_ECP_NORMALIZE_MXZ_ALT +/** + * \def MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN + * + * Enable testing of the constant-flow nature of some sensitive functions with + * clang's MemorySanitizer. This causes some existing tests to also test + * non-functional properties of the code under test. + * + * This setting requires compiling with clang -fsanitize=memory. + * + * Uncomment to enable testing of the constant-flow nature of seletected code. + */ +//#define MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN + /** * \def MBEDTLS_TEST_NULL_ENTROPY * diff --git a/library/version_features.c b/library/version_features.c index 51662bfd2..6b2a0c1b7 100644 --- a/library/version_features.c +++ b/library/version_features.c @@ -279,6 +279,9 @@ static const char *features[] = { #if defined(MBEDTLS_ECP_NORMALIZE_MXZ_ALT) "MBEDTLS_ECP_NORMALIZE_MXZ_ALT", #endif /* MBEDTLS_ECP_NORMALIZE_MXZ_ALT */ +#if defined(MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN) + "MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN", +#endif /* MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN */ #if defined(MBEDTLS_TEST_NULL_ENTROPY) "MBEDTLS_TEST_NULL_ENTROPY", #endif /* MBEDTLS_TEST_NULL_ENTROPY */ diff --git a/programs/ssl/query_config.c b/programs/ssl/query_config.c index 968549ac8..1e5c311d2 100644 --- a/programs/ssl/query_config.c +++ b/programs/ssl/query_config.c @@ -741,6 +741,14 @@ int query_config( const char *config ) } #endif /* MBEDTLS_ECP_NORMALIZE_MXZ_ALT */ +#if defined(MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN) + if( strcmp( "MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN", config ) == 0 ) + { + MACRO_EXPANSION_TO_STR( MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN ); + return( 0 ); + } +#endif /* MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN */ + #if defined(MBEDTLS_TEST_NULL_ENTROPY) if( strcmp( "MBEDTLS_TEST_NULL_ENTROPY", config ) == 0 ) { diff --git a/scripts/config.pl b/scripts/config.pl index 006a58de7..38fa97cf8 100755 --- a/scripts/config.pl +++ b/scripts/config.pl @@ -128,6 +128,7 @@ MBEDTLS_REMOVE_3DES_CIPHERSUITES MBEDTLS_REMOVE_ARC4_CIPHERSUITES MBEDTLS_RSA_NO_CRT MBEDTLS_SSL_HW_RECORD_ACCEL +MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN MBEDTLS_TEST_NULL_ENTROPY MBEDTLS_X509_ALLOW_UNSUPPORTED_CRITICAL_EXTENSION MBEDTLS_ZLIB_SUPPORT diff --git a/tests/scripts/all.sh b/tests/scripts/all.sh index d831d344c..02f61d6e3 100755 --- a/tests/scripts/all.sh +++ b/tests/scripts/all.sh @@ -1031,6 +1031,18 @@ component_test_full_cmake_clang () { if_build_succeeded env OPENSSL_CMD="$OPENSSL_NEXT" tests/compat.sh -e '^$' -f 'ARIA\|CHACHA' } +component_test_memsan_constant_flow () { + msg "build: cmake memsan, full config with constant flow testing" + scripts/config.pl full + scripts/config.pl set MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN + scripts/config.pl unset MBEDTLS_AESNI_C # memsan doesn't grok asm + CC=clang cmake -D CMAKE_BUILD_TYPE:String=MemSan . + make + + msg "test: main suites (memsan constant flow)" + make test +} + component_test_default_no_deprecated () { # Test that removing the deprecated features from the default # configuration leaves something consistent. diff --git a/tests/suites/helpers.function b/tests/suites/helpers.function index cd9346e5c..95b658da4 100644 --- a/tests/suites/helpers.function +++ b/tests/suites/helpers.function @@ -46,6 +46,22 @@ typedef UINT32 uint32_t; #include #endif +#if defined(MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN) +#include + +/* Use macros to avoid messing up with origin tracking */ +#define TEST_CF_SECRET __msan_allocated_memory +// void __msan_allocated_memory(const volatile void* data, size_t size); +#define TEST_CF_PUBLIC __msan_unpoison +// void __msan_unpoison(const volatile void *a, size_t size); + +#else /* MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN */ + +#define TEST_CF_SECRET(ptr, size) +#define TEST_CF_PUBLIC(ptr, size) + +#endif /* MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN */ + /* Type for Hex parameters */ typedef struct data_tag { From 0dab12ec2cdeb4f3de9fea56f837b28329b89e53 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 28 Jul 2020 11:02:57 +0200 Subject: [PATCH 12/38] 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 fc953a125..989d89520 100644 --- a/tests/suites/test_suite_ssl.function +++ b/tests/suites/test_suite_ssl.function @@ -117,10 +117,13 @@ void ssl_cf_hmac( int hash ) data[i] = ( i & 0xff ) ^ rec_num; /* Get the function's result */ + TEST_CF_SECRET( &in_len, sizeof( in_len ) ); TEST_ASSERT( 0 == mbedtls_ssl_cf_hmac( &ctx, add_data, sizeof( add_data ), data, in_len, min_in_len, max_in_len, out ) ); + TEST_CF_PUBLIC( &in_len, sizeof( in_len ) ); + TEST_CF_PUBLIC( out, out_len ); /* Compute the reference result */ TEST_ASSERT( 0 == mbedtls_md_hmac_update( &ref_ctx, add_data, From de02b580c8bd2d122746616205344dae4a2e9243 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 28 Jul 2020 11:25:34 +0200 Subject: [PATCH 13/38] 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 fbf43ab8f..b2d129454 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1809,6 +1809,48 @@ static int ssl_encrypt_buf( mbedtls_ssl_context *ssl ) ( defined(MBEDTLS_SSL_PROTO_TLS1) || \ defined(MBEDTLS_SSL_PROTO_TLS1_1) || \ defined(MBEDTLS_SSL_PROTO_TLS1_2) ) +/* + * Constant-flow conditional memcpy: + * - if c1 == c2, equivalent to memcpy(dst, src, len), + * - otherwise, a no-op, + * but with execution flow independent of the values of c1 and c2. + * + * Use only bit operations to avoid branches that could be used by some + * compilers on some platforms to translate comparison operators. + */ +static void mbedtls_ssl_cf_memcpy_if_eq(unsigned char *dst, + const unsigned char *src, + size_t len, + size_t c1, size_t c2 ) +{ + /* diff = 0 if c1 == c2, non-zero otherwise */ + const size_t diff = c1 ^ c2; + + /* MSVC has a warning about unary minus on unsigned integer types, + * but this is well-defined and precisely what we want to do here. */ +#if defined(_MSC_VER) +#pragma warning( push ) +#pragma warning( disable : 4146 ) +#endif + + /* diff_msb's most significant bit is bit equal to c1 != c2 */ + const size_t diff_msb = ( diff | -diff ); + + /* diff1 = c1 != c2 */ + const size_t diff1 = diff_msb >> ( sizeof( diff_msb ) * 8 - 1 ); + + /* mask = c1 != c2 ? 0xff : 0x00 */ + unsigned char mask = (unsigned char) -diff1; + +#if defined(_MSC_VER) +#pragma warning( pop ) +#endif + + /* dst[i] = c1 != c2 ? dst[i] : src[i] */ + for( size_t i = 0; i < len; i++ ) + dst[i] = ( dst[i] & mask ) | ( src[i] & ~mask ); +} + /* * Compute HMAC of variable-length data with constant flow. */ @@ -1819,85 +1861,61 @@ int mbedtls_ssl_cf_hmac( size_t min_data_len, size_t max_data_len, unsigned char *output ) { - /* WORK IN PROGRESS - THIS IS ONLY PSEUDO-CONTANT-TIME */ - /* - * Process MAC and always update for padlen afterwards to make - * total time independent of padlen. + * This function breaks the HMAC abstraction and uses the md_clone() + * extension to the MD API in order to get constant-flow behaviour. * - * Known timing attacks: - * - Lucky Thirteen (http://www.isg.rhul.ac.uk/tls/TLStiming.pdf) + * HMAC(msg) is defined as HASH(okey + HASH(ikey + msg)) where + means + * concatenation, and okey/ikey is the XOR of the key with some fix bit + * patterns (see RFC 2104, sec. 2), which are stored in ctx->hmac_ctx. * - * To compensate for different timings for the MAC calculation - * depending on how much padding was removed (which is determined - * by padlen), process extra_run more blocks through the hash - * function. + * We'll first compute inner_hash = HASH(ikey + msg) by hashing up to + * minlen, then cloning the context, and for each byte up to maxlen + * finishing up the hash computation, keeping only the correct result. * - * The formula in the paper is - * extra_run = ceil( (L1-55) / 64 ) - ceil( (L2-55) / 64 ) - * where L1 is the size of the header plus the decrypted message - * plus CBC padding and L2 is the size of the header plus the - * decrypted message. This is for an underlying hash function - * with 64-byte blocks. - * We use ( (Lx+8) / 64 ) to handle 'negative Lx' values - * correctly. We round down instead of up, so -56 is the correct - * value for our calculations instead of -55. - * - * Repeat the formula rather than defining a block_size variable. - * This avoids requiring division by a variable at runtime - * (which would be marginally less efficient and would require - * linking an extra division function in some builds). + * Then we only need to compute HASH(okey + inner_hash) and we're done. */ - size_t j, extra_run = 0; - /* This size is enough to server either as input to - * md_process() or as output to md_finish() */ - unsigned char tmp[128]; + const mbedtls_md_type_t md_alg = mbedtls_md_get_type( ctx->md_info ); + const size_t block_size = md_alg == MBEDTLS_MD_SHA384 ? 128 : 64; + const unsigned char * const ikey = (unsigned char *) ctx->hmac_ctx; + const unsigned char * const okey = ikey + block_size; + const size_t hash_size = mbedtls_md_get_size( ctx->md_info ); - memset( tmp, 0, sizeof( tmp ) ); + unsigned char aux_out[MBEDTLS_MD_MAX_SIZE]; + mbedtls_md_context_t aux; + size_t offset; - switch( mbedtls_md_get_type( ctx->md_info ) ) + mbedtls_md_init( &aux ); + mbedtls_md_setup( &aux, ctx->md_info, 0 ); + + /* After hmac_start() of hmac_reset(), ikey has already been hashed, + * so we can start directly with the message */ + mbedtls_md_update( ctx, add_data, add_data_len ); + mbedtls_md_update( ctx, data, min_data_len ); + + /* For each possible length, compute the hash up to that point */ + for( offset = min_data_len; offset <= max_data_len; offset++ ) { -#if defined(MBEDTLS_MD5_C) || defined(MBEDTLS_SHA1_C) || \ -defined(MBEDTLS_SHA256_C) - case MBEDTLS_MD_MD5: - case MBEDTLS_MD_SHA1: - case MBEDTLS_MD_SHA256: - /* 8 bytes of message size, 64-byte compression blocks */ - extra_run = ( add_data_len + max_data_len + 8 ) / 64 - - ( add_data_len + data_len_secret + 8 ) / 64; - break; -#endif -#if defined(MBEDTLS_SHA512_C) - case MBEDTLS_MD_SHA384: - /* 16 bytes of message size, 128-byte compression blocks */ - extra_run = ( add_data_len + max_data_len + 16 ) / 128 - - ( add_data_len + data_len_secret + 16 ) / 128; - break; -#endif - default: - return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); + mbedtls_md_clone( &aux, ctx ); + mbedtls_md_finish( &aux, aux_out ); + /* Keep only the correct inner_hash in the output buffer */ + mbedtls_ssl_cf_memcpy_if_eq( output, aux_out, hash_size, + offset, data_len_secret ); + + if( offset < max_data_len ) + mbedtls_md_update( ctx, data + offset, 1 ); } - mbedtls_md_hmac_update( ctx, add_data, add_data_len ); - mbedtls_md_hmac_update( ctx, data, data_len_secret ); - /* Make sure we access everything even when padlen > 0. This - * makes the synchronisation requirements for just-in-time - * Prime+Probe attacks much tighter and hopefully impractical. */ - ssl_read_memory( data + min_data_len, max_data_len - min_data_len ); - mbedtls_md_hmac_finish( ctx, output ); - - /* Dummy calls to compression function. - * Call mbedtls_md_process at least once due to cache attacks - * that observe whether md_process() was called of not. - * Respect the usual start-(process|update)-finish sequence for - * the sake of hardware accelerators that might require it. */ + /* Now compute HASH(okey + inner_hash) */ mbedtls_md_starts( ctx ); - for( j = 0; j < extra_run + 1; j++ ) - mbedtls_md_process( ctx, tmp ); - mbedtls_md_finish( ctx, tmp ); + mbedtls_md_update( ctx, okey, block_size ); + mbedtls_md_update( ctx, output, hash_size ); + mbedtls_md_finish( ctx, output ); + /* Done, get ready for next time */ mbedtls_md_hmac_reset( ctx ); + mbedtls_md_free( &aux ); return( 0 ); } #endif /* MBEDTLS_SSL_SOME_SUITES_USE_CBC && TLS 1.0-1.2 */ From 1e94128f3065866f7949921b5ac86dbdb54e5faa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 28 Jul 2020 11:35:39 +0200 Subject: [PATCH 14/38] 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 9ad04181a..e67c20b41 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -161,6 +161,13 @@ #define MBEDTLS_SSL_SOME_SUITES_USE_CBC #endif +#if defined(MBEDTLS_SSL_SOME_SUITES_USE_CBC) && \ + ( defined(MBEDTLS_SSL_PROTO_TLS1) || \ + defined(MBEDTLS_SSL_PROTO_TLS1_1) || \ + defined(MBEDTLS_SSL_PROTO_TLS1_2) ) +#define MBEDTLS_SSL_SOME_SUITES_USE_TLS_CBC +#endif + /* * Allow extra bytes for record, authentication and encryption overhead: * counter (8) + header (5) + IV(16) + MAC (16-48) + padding (0-256) @@ -852,10 +859,7 @@ int mbedtls_ssl_get_key_exchange_md_tls1_2( mbedtls_ssl_context *ssl, #endif /* MBEDTLS_SSL_PROTO_TLS1 || MBEDTLS_SSL_PROTO_TLS1_1 || \ MBEDTLS_SSL_PROTO_TLS1_2 */ -#if defined(MBEDTLS_SSL_SOME_SUITES_USE_CBC) && \ - ( defined(MBEDTLS_SSL_PROTO_TLS1) || \ - defined(MBEDTLS_SSL_PROTO_TLS1_1) | \ - defined(MBEDTLS_SSL_PROTO_TLS1_2) ) +#if defined(MBEDTLS_SSL_SOME_SUITES_USE_TLS_CBC) /** \brief Compute the HMAC of variable-length data with constant flow. * * This function computes the HMAC of the concatenation of \p add_data and \p @@ -894,7 +898,7 @@ int mbedtls_ssl_cf_hmac( const unsigned char *data, size_t data_len_secret, size_t min_data_len, size_t max_data_len, unsigned char *output ); -#endif /* MBEDTLS_SSL_SOME_SUITES_USE_CBC && TLS 1.0-1.2 */ +#endif /* MBEDTLS_SSL_SOME_SUITES_USE_TLS_CBC */ #ifdef __cplusplus } diff --git a/library/ssl_tls.c b/library/ssl_tls.c index b2d129454..a0b586a49 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1805,10 +1805,7 @@ static int ssl_encrypt_buf( mbedtls_ssl_context *ssl ) return( 0 ); } -#if defined(MBEDTLS_SSL_SOME_SUITES_USE_CBC) && \ - ( defined(MBEDTLS_SSL_PROTO_TLS1) || \ - defined(MBEDTLS_SSL_PROTO_TLS1_1) || \ - defined(MBEDTLS_SSL_PROTO_TLS1_2) ) +#if defined(MBEDTLS_SSL_SOME_SUITES_USE_TLS_CBC) /* * Constant-flow conditional memcpy: * - if c1 == c2, equivalent to memcpy(dst, src, len), @@ -1918,7 +1915,7 @@ int mbedtls_ssl_cf_hmac( mbedtls_md_free( &aux ); return( 0 ); } -#endif /* MBEDTLS_SSL_SOME_SUITES_USE_CBC && TLS 1.0-1.2 */ +#endif /* MBEDTLS_SSL_SOME_SUITES_USE_TLS_CBC */ static int ssl_decrypt_buf( mbedtls_ssl_context *ssl ) { diff --git a/tests/suites/test_suite_ssl.function b/tests/suites/test_suite_ssl.function index 989d89520..360be203f 100644 --- a/tests/suites/test_suite_ssl.function +++ b/tests/suites/test_suite_ssl.function @@ -53,7 +53,7 @@ void ssl_set_hostname_twice( char *hostname0, char *hostname1 ) } /* END_CASE */ -/* BEGIN_CASE depends_on:MBEDTLS_CIPHER_MODE_CBC:MBEDTLS_AES_C:MBEDTLS_SSL_PROTO_TLS1_2 */ +/* BEGIN_CASE depends_on:MBEDTLS_SSL_SOME_SUITES_USE_TLS_CBC */ void ssl_cf_hmac( int hash ) { /* From 74503bb5fc797546e532601de452a3d03c509079 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 28 Jul 2020 11:42:31 +0200 Subject: [PATCH 15/38] 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 e67c20b41..60ba22419 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -869,9 +869,11 @@ int mbedtls_ssl_get_key_exchange_md_tls1_2( mbedtls_ssl_context *ssl, * max_data_len bytes from \p data. * * \param ctx The HMAC context. It must have keys configured - * with mbedtls_md_hmac_starts(). It is reset using - * mbedtls_md_hmac_reset() after the computation is - * complete to prepare for the next computation. + * with mbedtls_md_hmac_starts() and use one of the + * following hashes: SHA-384, SHA-256, SHA-1 or MD-5. + * It is reset using mbedtls_md_hmac_reset() after + * the computation is complete to prepare for the + * next computation. * \param add_data The additional data prepended to \p data. This * must point to a readable buffer of \p add_data_len * bytes. diff --git a/library/ssl_tls.c b/library/ssl_tls.c index a0b586a49..5f940f0c6 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1863,7 +1863,7 @@ int mbedtls_ssl_cf_hmac( * extension to the MD API in order to get constant-flow behaviour. * * HMAC(msg) is defined as HASH(okey + HASH(ikey + msg)) where + means - * concatenation, and okey/ikey is the XOR of the key with some fix bit + * concatenation, and okey/ikey are the XOR of the key with some fixed bit * patterns (see RFC 2104, sec. 2), which are stored in ctx->hmac_ctx. * * We'll first compute inner_hash = HASH(ikey + msg) by hashing up to @@ -1873,6 +1873,8 @@ int mbedtls_ssl_cf_hmac( * Then we only need to compute HASH(okey + inner_hash) and we're done. */ const mbedtls_md_type_t md_alg = mbedtls_md_get_type( ctx->md_info ); + /* TLS 1.0-1.2 only support SHA-384, SHA-256, SHA-1, MD-5, + * all of which have the same block size except SHA-384. */ const size_t block_size = md_alg == MBEDTLS_MD_SHA384 ? 128 : 64; const unsigned char * const ikey = (unsigned char *) ctx->hmac_ctx; const unsigned char * const okey = ikey + block_size; From a6c1317685118a6448fe4398e52916402b7019fe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 28 Jul 2020 11:45:02 +0200 Subject: [PATCH 16/38] 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 5f940f0c6..74fc552df 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1876,7 +1876,7 @@ int mbedtls_ssl_cf_hmac( /* TLS 1.0-1.2 only support SHA-384, SHA-256, SHA-1, MD-5, * all of which have the same block size except SHA-384. */ const size_t block_size = md_alg == MBEDTLS_MD_SHA384 ? 128 : 64; - const unsigned char * const ikey = (unsigned char *) ctx->hmac_ctx; + const unsigned char * const ikey = ctx->hmac_ctx; const unsigned char * const okey = ikey + block_size; const size_t hash_size = mbedtls_md_get_size( ctx->md_info ); From 5bb6f3c3dba5dba2b6dd5b1f805ad5563f82e7b1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 28 Jul 2020 11:49:42 +0200 Subject: [PATCH 17/38] 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 74fc552df..3e5337660 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1883,39 +1883,51 @@ int mbedtls_ssl_cf_hmac( unsigned char aux_out[MBEDTLS_MD_MAX_SIZE]; mbedtls_md_context_t aux; size_t offset; + int ret; mbedtls_md_init( &aux ); - mbedtls_md_setup( &aux, ctx->md_info, 0 ); + +#define MD_CHK( func_call ) \ + do { \ + ret = (func_call); \ + if( ret != 0 ) \ + goto cleanup; \ + } while( 0 ) + + MD_CHK( mbedtls_md_setup( &aux, ctx->md_info, 0 ) ); /* After hmac_start() of hmac_reset(), ikey has already been hashed, * so we can start directly with the message */ - mbedtls_md_update( ctx, add_data, add_data_len ); - mbedtls_md_update( ctx, data, min_data_len ); + MD_CHK( mbedtls_md_update( ctx, add_data, add_data_len ) ); + MD_CHK( mbedtls_md_update( ctx, data, min_data_len ) ); /* For each possible length, compute the hash up to that point */ for( offset = min_data_len; offset <= max_data_len; offset++ ) { - mbedtls_md_clone( &aux, ctx ); - mbedtls_md_finish( &aux, aux_out ); + MD_CHK( mbedtls_md_clone( &aux, ctx ) ); + MD_CHK( mbedtls_md_finish( &aux, aux_out ) ); /* Keep only the correct inner_hash in the output buffer */ mbedtls_ssl_cf_memcpy_if_eq( output, aux_out, hash_size, offset, data_len_secret ); if( offset < max_data_len ) - mbedtls_md_update( ctx, data + offset, 1 ); + MD_CHK( mbedtls_md_update( ctx, data + offset, 1 ) ); } /* Now compute HASH(okey + inner_hash) */ - mbedtls_md_starts( ctx ); - mbedtls_md_update( ctx, okey, block_size ); - mbedtls_md_update( ctx, output, hash_size ); - mbedtls_md_finish( ctx, output ); + MD_CHK( mbedtls_md_starts( ctx ) ); + MD_CHK( mbedtls_md_update( ctx, okey, block_size ) ); + MD_CHK( mbedtls_md_update( ctx, output, hash_size ) ); + MD_CHK( mbedtls_md_finish( ctx, output ) ); /* Done, get ready for next time */ - mbedtls_md_hmac_reset( ctx ); + MD_CHK( mbedtls_md_hmac_reset( ctx ) ); +#undef MD_CHK + +cleanup: mbedtls_md_free( &aux ); - return( 0 ); + return( ret ); } #endif /* MBEDTLS_SSL_SOME_SUITES_USE_TLS_CBC */ From 2b80249c043762a991240bc7e04807f4443cc31e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Wed, 22 Jul 2020 11:09:28 +0200 Subject: [PATCH 18/38] 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 02f61d6e3..f5a256f68 100755 --- a/tests/scripts/all.sh +++ b/tests/scripts/all.sh @@ -1032,14 +1032,20 @@ component_test_full_cmake_clang () { } component_test_memsan_constant_flow () { - msg "build: cmake memsan, full config with constant flow testing" + # This tests both (1) accesses to undefined memory, and (2) branches or + # memory access depending on secret values. To distinguish between those: + # - unset MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN - does the failure persist? + # - or alternatively, change the build type to MemSanDbg, which enables + # origin tracking and nicer stack traces (which are useful for debugging + # anyway), and check if the origin was TEST_CF_SECRET() or something else. + msg "build: cmake MSan (clang), full config with constant flow testing" scripts/config.pl full scripts/config.pl set MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN scripts/config.pl unset MBEDTLS_AESNI_C # memsan doesn't grok asm CC=clang cmake -D CMAKE_BUILD_TYPE:String=MemSan . make - msg "test: main suites (memsan constant flow)" + msg "test: main suites (Msan + constant flow)" make test } From 2b2f956f22abcdd8b288e17c12058b20b23fb5e0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 28 Jul 2020 11:54:35 +0200 Subject: [PATCH 19/38] 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 967668080..1c3f14e82 100644 --- a/include/mbedtls/config.h +++ b/include/mbedtls/config.h @@ -560,7 +560,7 @@ * * This setting requires compiling with clang -fsanitize=memory. * - * Uncomment to enable testing of the constant-flow nature of seletected code. + * Uncomment to enable testing of the constant-flow nature of selected code. */ //#define MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN diff --git a/include/mbedtls/ssl_internal.h b/include/mbedtls/ssl_internal.h index 60ba22419..c435f7a9c 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -882,11 +882,11 @@ int mbedtls_ssl_get_key_exchange_md_tls1_2( mbedtls_ssl_context *ssl, * to a readable buffer of \p max_data_len bytes. * \param data_len_secret The length of the data to process in \p data. * This must be no less than \p min_data_len and no - * greated than \p max_data_len. + * greater than \p max_data_len. * \param min_data_len The minimal length of \p data in bytes. * \param max_data_len The maximal length of \p data in bytes. * \param output The HMAC will be written here. This must point to - * a writeable buffer of sufficient size to hold the + * a writable buffer of sufficient size to hold the * HMAC value. * * \retval 0 From 94fd8dc0663a8aa5ca30ef4a44d0a41a1dd7219e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 28 Jul 2020 11:56:05 +0200 Subject: [PATCH 20/38] 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 3e5337660..c0a59e514 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1815,10 +1815,10 @@ static int ssl_encrypt_buf( mbedtls_ssl_context *ssl ) * Use only bit operations to avoid branches that could be used by some * compilers on some platforms to translate comparison operators. */ -static void mbedtls_ssl_cf_memcpy_if_eq(unsigned char *dst, - const unsigned char *src, - size_t len, - size_t c1, size_t c2 ) +static void mbedtls_ssl_cf_memcpy_if_eq( unsigned char *dst, + const unsigned char *src, + size_t len, + size_t c1, size_t c2 ) { /* diff = 0 if c1 == c2, non-zero otherwise */ const size_t diff = c1 ^ c2; From 09ac297f7fb8c32fe3c1d624773114545168f7d6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 28 Jul 2020 11:57:25 +0200 Subject: [PATCH 21/38] 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 c0a59e514..533f301ae 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1830,14 +1830,14 @@ static void mbedtls_ssl_cf_memcpy_if_eq( unsigned char *dst, #pragma warning( disable : 4146 ) #endif - /* diff_msb's most significant bit is bit equal to c1 != c2 */ + /* diff_msb's most significant bit is equal to c1 != c2 */ const size_t diff_msb = ( diff | -diff ); /* diff1 = c1 != c2 */ const size_t diff1 = diff_msb >> ( sizeof( diff_msb ) * 8 - 1 ); /* mask = c1 != c2 ? 0xff : 0x00 */ - unsigned char mask = (unsigned char) -diff1; + const unsigned char mask = (unsigned char) -diff1; #if defined(_MSC_VER) #pragma warning( pop ) From 253b0de23d4738d615dc9f6df470a9955a06bfc8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Wed, 29 Jul 2020 10:04:36 +0200 Subject: [PATCH 22/38] 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 533f301ae..96c7e6c6d 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1844,7 +1844,8 @@ static void mbedtls_ssl_cf_memcpy_if_eq( unsigned char *dst, #endif /* dst[i] = c1 != c2 ? dst[i] : src[i] */ - for( size_t i = 0; i < len; i++ ) + size_t i; + for( i = 0; i < len; i++ ) dst[i] = ( dst[i] & mask ) | ( src[i] & ~mask ); } From e1b1fffaadeb4c6d8728734ed015036b92cd3ff3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Wed, 29 Jul 2020 12:54:04 +0200 Subject: [PATCH 23/38] 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 96c7e6c6d..81139b409 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1851,6 +1851,9 @@ static void mbedtls_ssl_cf_memcpy_if_eq( unsigned char *dst, /* * Compute HMAC of variable-length data with constant flow. + * + * Only works with MD-5, SHA-1, SHA-256 and SHA-384. + * (Otherwise, computation of block_size needs to be adapted.) */ int mbedtls_ssl_cf_hmac( mbedtls_md_context_t *ctx, From 681615a27640868b7d10894072f9e6a1fc5f7487 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Fri, 31 Jul 2020 10:00:17 +0200 Subject: [PATCH 24/38] 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 360be203f..cf807b545 100644 --- a/tests/suites/test_suite_ssl.function +++ b/tests/suites/test_suite_ssl.function @@ -59,10 +59,6 @@ void ssl_cf_hmac( int hash ) /* * Test the function mbedtls_ssl_cf_hmac() against a reference * implementation. - * - * Note: the dependency is actually on TLS 1.0-1.2 and (AES or ARIA or - * Camellia or DES), but since the test framework doesn't support - * alternation in dependencies, just depend on the most common. */ mbedtls_md_context_t ctx, ref_ctx; const mbedtls_md_info_t *md_info; From f82cb79b875267a1809d61b38c9e30cb81a70ccd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Fri, 31 Jul 2020 12:53:39 +0200 Subject: [PATCH 25/38] 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 c435f7a9c..4a3eee566 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -152,7 +152,7 @@ #define MBEDTLS_SSL_RETRANS_WAITING 2 #define MBEDTLS_SSL_RETRANS_FINISHED 3 -/* For CBC-specific encrypt/decrypt code */ +/* This macro determines whether CBC is supported. */ #if defined(MBEDTLS_CIPHER_MODE_CBC) && \ ( defined(MBEDTLS_AES_C) || \ defined(MBEDTLS_CAMELLIA_C) || \ @@ -161,6 +161,8 @@ #define MBEDTLS_SSL_SOME_SUITES_USE_CBC #endif +/* This macro determines whether the CBC construct used in TLS 1.0-1.2 (as + * opposed to the very different CBC construct used in SSLv3) is supported. */ #if defined(MBEDTLS_SSL_SOME_SUITES_USE_CBC) && \ ( defined(MBEDTLS_SSL_PROTO_TLS1) || \ defined(MBEDTLS_SSL_PROTO_TLS1_1) || \ From 7433fa4f4a0addbcf7ff11e4d08ed5298d46f642 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Fri, 31 Jul 2020 12:59:34 +0200 Subject: [PATCH 26/38] 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 1c3f14e82..73efeb443 100644 --- a/include/mbedtls/config.h +++ b/include/mbedtls/config.h @@ -560,6 +560,9 @@ * * This setting requires compiling with clang -fsanitize=memory. * + * \warning This macro is only used for extended testing; it is not considered + * part of the library's API, so it may change or disappear at any time. + * * Uncomment to enable testing of the constant-flow nature of selected code. */ //#define MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN From 3eaa10389ddd99128215ddaad46f1892eeb9e9dd Mon Sep 17 00:00:00 2001 From: gabor-mezei-arm Date: Wed, 15 Jul 2020 10:55:00 +0200 Subject: [PATCH 27/38] Zeroising of plaintext buffers to erase unused application data from memory Signed-off-by: gabor-mezei-arm --- ChangeLog.d/zeroising_of_plaintext_buffer.txt | 4 ++++ library/ssl_tls.c | 4 ++++ 2 files changed, 8 insertions(+) create mode 100644 ChangeLog.d/zeroising_of_plaintext_buffer.txt diff --git a/ChangeLog.d/zeroising_of_plaintext_buffer.txt b/ChangeLog.d/zeroising_of_plaintext_buffer.txt new file mode 100644 index 000000000..d7dee29a4 --- /dev/null +++ b/ChangeLog.d/zeroising_of_plaintext_buffer.txt @@ -0,0 +1,4 @@ +Security + * Zeroising of plaintext buffers in mbedtls_ssl_read() to erase unused + application data from memory. Reported in #689 by + Johan Uppman Bruce of Sectra. \ No newline at end of file diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 02b8f2654..c7787cb23 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -8569,6 +8569,10 @@ int mbedtls_ssl_read( mbedtls_ssl_context *ssl, unsigned char *buf, size_t len ) memcpy( buf, ssl->in_offt, n ); ssl->in_msglen -= n; + /* Zeroising the plaintext buffer to erase unused application data + from the memory. */ + mbedtls_platform_zeroize( ssl->in_offt, n ); + if( ssl->in_msglen == 0 ) { /* all bytes consumed */ From b394b43cf8ac0883e365a5d6d5062c0b19c8043a Mon Sep 17 00:00:00 2001 From: gabor-mezei-arm Date: Thu, 16 Jul 2020 10:19:18 +0200 Subject: [PATCH 28/38] Add missing newline Signed-off-by: gabor-mezei-arm --- ChangeLog.d/zeroising_of_plaintext_buffer.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ChangeLog.d/zeroising_of_plaintext_buffer.txt b/ChangeLog.d/zeroising_of_plaintext_buffer.txt index d7dee29a4..f618beb91 100644 --- a/ChangeLog.d/zeroising_of_plaintext_buffer.txt +++ b/ChangeLog.d/zeroising_of_plaintext_buffer.txt @@ -1,4 +1,4 @@ Security * Zeroising of plaintext buffers in mbedtls_ssl_read() to erase unused application data from memory. Reported in #689 by - Johan Uppman Bruce of Sectra. \ No newline at end of file + Johan Uppman Bruce of Sectra. From d8dc8e29c1da9beb961075ebff08e2c5b9bd26a1 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 16 Jul 2020 18:18:22 +0200 Subject: [PATCH 29/38] x509parse_crl: more negative test cases Add a few more negative test cases for mbedtls_x509_crl_parse. The test data is manually adapted from the existing positive test case "X509 CRL ASN1 (TBSCertList, sig present)" which decomposes as 305c 3047 tbsCertList TBSCertList 020100 version INTEGER OPTIONAL 300d signatureAlgorithm AlgorithmIdentifier 06092a864886f70d01010e 0500 300f issuer Name 310d300b0603550403130441424344 170c303930313031303030303030 thisUpdate Time 3014 revokedCertificates 3012 entry 1 8202abcd userCertificate CertificateSerialNumber 170c303831323331323335393539 revocationDate Time 300d signatureAlgorithm AlgorithmIdentifier 06092a864886f70d01010e 0500 03020001 signatureValue BIT STRING Signed-off-by: Gilles Peskine --- tests/suites/test_suite_x509parse.data | 32 ++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/tests/suites/test_suite_x509parse.data b/tests/suites/test_suite_x509parse.data index 2c52d081d..cfc939b10 100644 --- a/tests/suites/test_suite_x509parse.data +++ b/tests/suites/test_suite_x509parse.data @@ -1336,6 +1336,38 @@ X509 CRL ASN1 (TBSCertList, sig present, len mismatch) depends_on:MBEDTLS_RSA_C:MBEDTLS_SHA256_C x509parse_crl:"305d3047020100300d06092a864886f70d01010e0500300f310d300b0603550403130441424344170c303930313031303030303030301430128202abcd170c303831323331323335393539300d06092a864886f70d01010e05000302000100":"":MBEDTLS_ERR_X509_INVALID_FORMAT + MBEDTLS_ERR_ASN1_LENGTH_MISMATCH +X509 CRL ASN1 (TBSCertList, signatureValue missing) +depends_on:MBEDTLS_RSA_C:MBEDTLS_SHA256_C +x509parse_crl:"30583047020100300d06092a864886f70d01010e0500300f310d300b0603550403130441424344170c303930313031303030303030301430128202abcd170c303831323331323335393539300d06092a864886f70d01010e0500":"":MBEDTLS_ERR_X509_INVALID_SIGNATURE + MBEDTLS_ERR_ASN1_OUT_OF_DATA + +X509 CRL ASN1 (TBSCertList, signatureAlgorithm missing) +depends_on:MBEDTLS_RSA_C:MBEDTLS_SHA256_C +x509parse_crl:"30493047020100300d06092a864886f70d01010e0500300f310d300b0603550403130441424344170c303930313031303030303030301430128202abcd170c303831323331323335393539":"":MBEDTLS_ERR_X509_INVALID_ALG + MBEDTLS_ERR_ASN1_OUT_OF_DATA + +X509 CRL ASN1 (TBSCertList, single empty entry at end) +depends_on:MBEDTLS_RSA_C:MBEDTLS_SHA256_C +x509parse_crl:"30373035020100300d06092a864886f70d01010e0500300f310d300b0603550403130441424344170c30393031303130303030303030023000":"":MBEDTLS_ERR_X509_INVALID_SERIAL + MBEDTLS_ERR_ASN1_OUT_OF_DATA + +X509 CRL ASN1 (TBSCertList, good entry then empty entry at end) +depends_on:MBEDTLS_RSA_C:MBEDTLS_SHA256_C +x509parse_crl:"304b3049020100300d06092a864886f70d01010e0500300f310d300b0603550403130441424344170c303930313031303030303030301630128202abcd170c3038313233313233353935393000":"":MBEDTLS_ERR_X509_INVALID_SERIAL + MBEDTLS_ERR_ASN1_OUT_OF_DATA + +X509 CRL ASN1 (TBSCertList, missing time in entry) +depends_on:MBEDTLS_RSA_C:MBEDTLS_SHA256_C +x509parse_crl:"304e3039020100300d06092a864886f70d01010e0500300f310d300b0603550403130441424344170c303930313031303030303030300630048202abcd300d06092a864886f70d01010e050003020001":"":MBEDTLS_ERR_X509_INVALID_DATE + MBEDTLS_ERR_ASN1_OUT_OF_DATA + +X509 CRL ASN1 (TBSCertList, missing time in entry at end) +depends_on:MBEDTLS_RSA_C:MBEDTLS_SHA256_C +x509parse_crl:"303b3039020100300d06092a864886f70d01010e0500300f310d300b0603550403130441424344170c303930313031303030303030300630048202abcd":"":MBEDTLS_ERR_X509_INVALID_DATE + MBEDTLS_ERR_ASN1_OUT_OF_DATA + +X509 CRL ASN1 (TBSCertList, invalid tag for time in entry) +depends_on:MBEDTLS_RSA_C:MBEDTLS_SHA256_C +x509parse_crl:"305c3047020100300d06092a864886f70d01010e0500300f310d300b0603550403130441424344170c303930313031303030303030301430128202abcd190c303831323331323335393539300d06092a864886f70d01010e050003020001":"":MBEDTLS_ERR_X509_INVALID_DATE + MBEDTLS_ERR_ASN1_UNEXPECTED_TAG + +X509 CRL ASN1 (TBSCertList, invalid tag for serial) +depends_on:MBEDTLS_RSA_C:MBEDTLS_SHA256_C +x509parse_crl:"305c3047020100300d06092a864886f70d01010e0500300f310d300b0603550403130441424344170c303930313031303030303030301430128402abcd170c303831323331323335393539300d06092a864886f70d01010e050003020001":"":MBEDTLS_ERR_X509_INVALID_SERIAL + MBEDTLS_ERR_ASN1_UNEXPECTED_TAG + X509 CRL ASN1 (TBSCertList, sig present) depends_on:MBEDTLS_RSA_C:MBEDTLS_SHA256_C x509parse_crl:"305c3047020100300d06092a864886f70d01010e0500300f310d300b0603550403130441424344170c303930313031303030303030301430128202abcd170c303831323331323335393539300d06092a864886f70d01010e050003020001":"CRL version \: 1\nissuer name \: CN=ABCD\nthis update \: 2009-01-01 00\:00\:00\nnext update \: 0000-00-00 00\:00\:00\nRevoked certificates\:\nserial number\: AB\:CD revocation date\: 2008-12-31 23\:59\:59\nsigned using \: RSA with SHA-224\n":0 From 6579235d9cbbcecbbd6d3940edad9aa6b8493c6e Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 16 Jul 2020 18:26:29 +0200 Subject: [PATCH 30/38] x509_crl_parse: fix 1-byte buffer overflow and entry->raw.tag In the entries (mbedtls_x509_crl_entry values) on the list constructed by mbedtls_x509_crl_parse_der(), set entry->raw.tag to (SEQUENCE | CONSTRUCTED) rather than to the tag of the first ASN.1 element of the entry (which happens to be the tag of the serial number, so INTEGER or INTEGER | CONTEXT_SPECIFIC). This is doesn't really matter in practice (and in particular the value is never used in Mbed TLS itself), and isn't documented, but at least it's consistent with how mbedtls_x509_buf is normally used. The primary importance of this change is that the old code tried to access the tag of the first element of the entry even when the entry happened to be empty. If the entry was empty and not followed by anything else in the CRL, this could cause a read 1 byte after the end of the buffer containing the CRL. The test case "X509 CRL ASN1 (TBSCertList, single empty entry at end)" hit the problematic buffer overflow, which is detected with ASan. Credit to OSS-Fuzz for detecting the problem. Signed-off-by: Gilles Peskine --- ChangeLog.d/x509parse_crl-empty_entry.txt | 4 ++++ library/x509_crl.c | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) create mode 100644 ChangeLog.d/x509parse_crl-empty_entry.txt diff --git a/ChangeLog.d/x509parse_crl-empty_entry.txt b/ChangeLog.d/x509parse_crl-empty_entry.txt new file mode 100644 index 000000000..483abb10a --- /dev/null +++ b/ChangeLog.d/x509parse_crl-empty_entry.txt @@ -0,0 +1,4 @@ +Security + * Fix a 1-byte buffer overread in mbedtls_x509_crl_parse_der(). + Credit to OSS-Fuzz for detecting the problem and to Philippe Antoine + for pinpointing the problematic code. diff --git a/library/x509_crl.c b/library/x509_crl.c index 94c0c01af..e20a258be 100644 --- a/library/x509_crl.c +++ b/library/x509_crl.c @@ -285,13 +285,13 @@ static int x509_get_entries( unsigned char **p, size_t len2; const unsigned char *end2; + cur_entry->raw.tag = **p; if( ( ret = mbedtls_asn1_get_tag( p, end, &len2, MBEDTLS_ASN1_SEQUENCE | MBEDTLS_ASN1_CONSTRUCTED ) ) != 0 ) { return( ret ); } - cur_entry->raw.tag = **p; cur_entry->raw.p = *p; cur_entry->raw.len = len2; end2 = *p + len2; From 4ddfdbf76ab4ee378ec86ca36343c4a5c787a7b9 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 6 Aug 2020 16:05:35 +0200 Subject: [PATCH 31/38] Add the decomposition of the base case as a comment Put the base good case first, then the bad cases derived from it. Signed-off-by: Gilles Peskine --- tests/suites/test_suite_x509parse.data | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-) diff --git a/tests/suites/test_suite_x509parse.data b/tests/suites/test_suite_x509parse.data index cfc939b10..f5462a857 100644 --- a/tests/suites/test_suite_x509parse.data +++ b/tests/suites/test_suite_x509parse.data @@ -1336,6 +1336,28 @@ X509 CRL ASN1 (TBSCertList, sig present, len mismatch) depends_on:MBEDTLS_RSA_C:MBEDTLS_SHA256_C x509parse_crl:"305d3047020100300d06092a864886f70d01010e0500300f310d300b0603550403130441424344170c303930313031303030303030301430128202abcd170c303831323331323335393539300d06092a864886f70d01010e05000302000100":"":MBEDTLS_ERR_X509_INVALID_FORMAT + MBEDTLS_ERR_ASN1_LENGTH_MISMATCH +# 305c +# 3047 tbsCertList TBSCertList +# 020100 version INTEGER OPTIONAL +# 300d signatureAlgorithm AlgorithmIdentifi +# 06092a864886f70d01010e +# 0500 +# 300f issuer Name +# 310d300b0603550403130441424344 +# 170c303930313031303030303030 thisUpdate Time +# 3014 revokedCertificates +# 3012 entry 1 +# 8202abcd userCertificate CertificateSerialNum +# 170c303831323331323335393539 revocationDate Time +# 300d signatureAlgorithm AlgorithmIdentifi +# 06092a864886f70d01010e +# 0500 +# 03020001 signatureValue BIT STRING +# The subsequent TBSCertList negative tests remove or modify some elements. +X509 CRL ASN1 (TBSCertList, sig present) +depends_on:MBEDTLS_RSA_C:MBEDTLS_SHA256_C +x509parse_crl:"305c3047020100300d06092a864886f70d01010e0500300f310d300b0603550403130441424344170c303930313031303030303030301430128202abcd170c303831323331323335393539300d06092a864886f70d01010e050003020001":"CRL version \: 1\nissuer name \: CN=ABCD\nthis update \: 2009-01-01 00\:00\:00\nnext update \: 0000-00-00 00\:00\:00\nRevoked certificates\:\nserial number\: AB\:CD revocation date\: 2008-12-31 23\:59\:59\nsigned using \: RSA with SHA-224\n":0 + X509 CRL ASN1 (TBSCertList, signatureValue missing) depends_on:MBEDTLS_RSA_C:MBEDTLS_SHA256_C x509parse_crl:"30583047020100300d06092a864886f70d01010e0500300f310d300b0603550403130441424344170c303930313031303030303030301430128202abcd170c303831323331323335393539300d06092a864886f70d01010e0500":"":MBEDTLS_ERR_X509_INVALID_SIGNATURE + MBEDTLS_ERR_ASN1_OUT_OF_DATA @@ -1368,10 +1390,6 @@ X509 CRL ASN1 (TBSCertList, invalid tag for serial) depends_on:MBEDTLS_RSA_C:MBEDTLS_SHA256_C x509parse_crl:"305c3047020100300d06092a864886f70d01010e0500300f310d300b0603550403130441424344170c303930313031303030303030301430128402abcd170c303831323331323335393539300d06092a864886f70d01010e050003020001":"":MBEDTLS_ERR_X509_INVALID_SERIAL + MBEDTLS_ERR_ASN1_UNEXPECTED_TAG -X509 CRL ASN1 (TBSCertList, sig present) -depends_on:MBEDTLS_RSA_C:MBEDTLS_SHA256_C -x509parse_crl:"305c3047020100300d06092a864886f70d01010e0500300f310d300b0603550403130441424344170c303930313031303030303030301430128202abcd170c303831323331323335393539300d06092a864886f70d01010e050003020001":"CRL version \: 1\nissuer name \: CN=ABCD\nthis update \: 2009-01-01 00\:00\:00\nnext update \: 0000-00-00 00\:00\:00\nRevoked certificates\:\nserial number\: AB\:CD revocation date\: 2008-12-31 23\:59\:59\nsigned using \: RSA with SHA-224\n":0 - X509 CRL ASN1 (TBSCertList, no entries) depends_on:MBEDTLS_RSA_C:MBEDTLS_SHA256_C x509parse_crl:"30463031020100300d06092a864886f70d01010e0500300f310d300b0603550403130441424344170c303930313031303030303030300d06092a864886f70d01010e050003020001":"CRL version \: 1\nissuer name \: CN=ABCD\nthis update \: 2009-01-01 00\:00\:00\nnext update \: 0000-00-00 00\:00\:00\nRevoked certificates\:\nsigned using \: RSA with SHA-224\n":0 From ce45d1a75949644b68bcae18b3f3115b99c68098 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 25 Aug 2020 11:07:25 +0200 Subject: [PATCH 32/38] Use temporary buffer to hold the peer's HMAC MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This paves the way for a constant-flow implementation of HMAC checking, by making sure that the comparison happens at a constant address. The missing step is obviously to copy the HMAC from the secret offset to this temporary buffer with constant flow, which will be done in the next few commits. Signed-off-by: Manuel Pégourié-Gonnard --- library/ssl_tls.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 1b4c38050..05176ba2e 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -2319,6 +2319,7 @@ static int ssl_decrypt_buf( mbedtls_ssl_context *ssl ) if( auth_done == 0 ) { unsigned char mac_expect[MBEDTLS_SSL_MAC_ADD]; + unsigned char mac_peer[MBEDTLS_SSL_MAC_ADD]; ssl->in_msglen -= ssl->transform_in->maclen; @@ -2333,6 +2334,8 @@ static int ssl_decrypt_buf( mbedtls_ssl_context *ssl ) ssl->in_msg, ssl->in_msglen, ssl->in_ctr, ssl->in_msgtype, mac_expect ); + memcpy( mac_peer, ssl->in_msg + ssl->in_msglen, + ssl->transform_in->maclen ); } else #endif /* MBEDTLS_SSL_PROTO_SSL3 */ @@ -2377,6 +2380,8 @@ static int ssl_decrypt_buf( mbedtls_ssl_context *ssl ) * attacks much tighter and hopefully impractical. */ ssl_read_memory( ssl->in_msg + min_len, max_len - min_len + ssl->transform_in->maclen ); + memcpy( mac_peer, ssl->in_msg + ssl->in_msglen, + ssl->transform_in->maclen ); } else #endif /* MBEDTLS_SSL_PROTO_TLS1 || MBEDTLS_SSL_PROTO_TLS1_1 || \ @@ -2388,11 +2393,10 @@ static int ssl_decrypt_buf( mbedtls_ssl_context *ssl ) #if defined(MBEDTLS_SSL_DEBUG_ALL) MBEDTLS_SSL_DEBUG_BUF( 4, "expected mac", mac_expect, ssl->transform_in->maclen ); - MBEDTLS_SSL_DEBUG_BUF( 4, "message mac", ssl->in_msg + ssl->in_msglen, - ssl->transform_in->maclen ); + MBEDTLS_SSL_DEBUG_BUF( 4, "message mac", mac_peer, ssl->transform_in->maclen ); #endif - if( mbedtls_ssl_safer_memcmp( ssl->in_msg + ssl->in_msglen, mac_expect, + if( mbedtls_ssl_safer_memcmp( mac_peer, mac_expect, ssl->transform_in->maclen ) != 0 ) { #if defined(MBEDTLS_SSL_DEBUG_ALL) From 590b2d96140d866da37bcd2177a45ed790ff4984 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 25 Aug 2020 11:18:11 +0200 Subject: [PATCH 33/38] Add mbedtls_ssl_cf_memcpy_offset() with tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The tests are supposed to be failing now (in all.sh component test_memsan_constant_flow), but they don't as apparently MemSan doesn't complain when the src argument of memcpy() is uninitialized, see https://github.com/google/sanitizers/issues/1296 The next commit will add an option to test constant flow with valgrind, which will hopefully correctly flag the current non-constant-flow implementation. Signed-off-by: Manuel Pégourié-Gonnard --- include/mbedtls/ssl_internal.h | 24 ++++++++++++++++++++ library/ssl_tls.c | 29 +++++++++++++++++------- tests/suites/test_suite_ssl.data | 12 ++++++++++ tests/suites/test_suite_ssl.function | 33 ++++++++++++++++++++++++++++ 4 files changed, 90 insertions(+), 8 deletions(-) diff --git a/include/mbedtls/ssl_internal.h b/include/mbedtls/ssl_internal.h index 3c4df4764..6ba6c2af0 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -900,6 +900,30 @@ int mbedtls_ssl_cf_hmac( const unsigned char *data, size_t data_len_secret, size_t min_data_len, size_t max_data_len, unsigned char *output ); + +/** \brief Copy data from a secret position with constant flow. + * + * This function copies \p len bytes from \p src_base + \p offset_secret to \p + * dst, with a code flow and memory access pattern that does not depend on \p + * offset_secret, but only on \p offset_min, \p offset_max and \p len. + * + * \param dst The destination buffer. This must point to a writable + * buffer of at least \p len bytes. + * \param src_base The base of the source buffer. This must point to a + * readable buffer of at least \p offset_max + \p len + * bytes. + * \param offset_secret The offset in the source buffer from which to copy. + * This must be no less than \p offset_min and no greater + * than \p offset_max. + * \param offset_min The minimal value of \p offset_secret. + * \param offset_max The maximal value of \p offset_secret. + * \param len The number of bytes to copy. + */ +void mbedtls_ssl_cf_memcpy_offset( unsigned char *dst, + const unsigned char *src_base, + size_t offset_secret, + size_t offset_min, size_t offset_max, + size_t len ); #endif /* MBEDTLS_SSL_SOME_SUITES_USE_TLS_CBC */ #ifdef __cplusplus diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 05176ba2e..0cc6c30b1 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1931,6 +1931,23 @@ cleanup: mbedtls_md_free( &aux ); return( ret ); } + +/* + * Constant-flow memcpy from variable position in buffer. + * - functionally equivalent to memcpy(dst, src + offset_secret, len) + * - but with execution flow independant from the value of offset_secret. + */ +void mbedtls_ssl_cf_memcpy_offset( unsigned char *dst, + const unsigned char *src_base, + size_t offset_secret, + size_t offset_min, size_t offset_max, + size_t len ) +{ + /* WIP - THIS IS NOT ACTUALLY CONSTANT-FLOW! + * This is just to be able to write tests and check they work. */ + ssl_read_memory( src_base + offset_min, offset_max - offset_min + len ); + memcpy( dst, src_base + offset_secret, len ); +} #endif /* MBEDTLS_SSL_SOME_SUITES_USE_TLS_CBC */ static int ssl_decrypt_buf( mbedtls_ssl_context *ssl ) @@ -2374,14 +2391,10 @@ static int ssl_decrypt_buf( mbedtls_ssl_context *ssl ) return( ret ); } - /* Make sure we access all the memory that could contain the MAC, - * before we check it in the next code block. This makes the - * synchronisation requirements for just-in-time Prime+Probe - * attacks much tighter and hopefully impractical. */ - ssl_read_memory( ssl->in_msg + min_len, - max_len - min_len + ssl->transform_in->maclen ); - memcpy( mac_peer, ssl->in_msg + ssl->in_msglen, - ssl->transform_in->maclen ); + mbedtls_ssl_cf_memcpy_offset( mac_peer, ssl->in_msg, + ssl->in_msglen, + min_len, max_len, + ssl->transform_in->maclen ); } else #endif /* MBEDTLS_SSL_PROTO_TLS1 || MBEDTLS_SSL_PROTO_TLS1_1 || \ diff --git a/tests/suites/test_suite_ssl.data b/tests/suites/test_suite_ssl.data index db92a72bc..66f6b8492 100644 --- a/tests/suites/test_suite_ssl.data +++ b/tests/suites/test_suite_ssl.data @@ -73,3 +73,15 @@ ssl_cf_hmac:MBEDTLS_MD_SHA256 Constant-flow HMAC: SHA384 depends_on:MBEDTLS_SHA512_C:!MBEDTLS_SHA512_NO_SHA384 ssl_cf_hmac:MBEDTLS_MD_SHA384 + +# these are the numbers we'd get with an empty plaintext and truncated HMAC +Constant-flow memcpy from offset: small +ssl_cf_memcpy_offset:0:5:10 + +# we could get this with 255-bytes plaintext and untruncated SHA-256 +Constant-flow memcpy from offset: medium +ssl_cf_memcpy_offset:0:255:32 + +# we could get this with 355-bytes plaintext and untruncated SHA-384 +Constant-flow memcpy from offset: large +ssl_cf_memcpy_offset:100:339:48 diff --git a/tests/suites/test_suite_ssl.function b/tests/suites/test_suite_ssl.function index cf807b545..c1f7d0de3 100644 --- a/tests/suites/test_suite_ssl.function +++ b/tests/suites/test_suite_ssl.function @@ -144,3 +144,36 @@ exit: mbedtls_free( out ); } /* END_CASE */ + +/* BEGIN_CASE depends_on:MBEDTLS_SSL_SOME_SUITES_USE_TLS_CBC */ +void ssl_cf_memcpy_offset( int offset_min, int offset_max, int len ) +{ + unsigned char *dst = NULL; + unsigned char *src = NULL; + size_t src_len = offset_max + len; + size_t secret; + + dst = mbedtls_calloc( 1, len ); + TEST_ASSERT( dst != NULL ); + src = mbedtls_calloc( 1, src_len ); + TEST_ASSERT( src != NULL ); + + /* Fill src in a way that we can detect if we copied the right bytes */ + rnd_std_rand( NULL, src, src_len ); + + for( secret = offset_min; secret <= (size_t) offset_max; secret++ ) + { + TEST_CF_SECRET( &secret, sizeof( secret ) ); + mbedtls_ssl_cf_memcpy_offset( dst, src, secret, + offset_min, offset_max, len ); + TEST_CF_PUBLIC( &secret, sizeof( secret ) ); + TEST_CF_PUBLIC( dst, len ); + + TEST_ASSERT( memcmp( dst, src + secret, len ) == 0 ); + } + +exit: + mbedtls_free( dst ); + mbedtls_free( src ); +} +/* END_CASE */ From f08284769d770ea9ed2840c4d66d455b6877435a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 25 Aug 2020 11:26:37 +0200 Subject: [PATCH 34/38] Add an option to test constant-flow with valgrind MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Currently the new component in all.sh fails because mbedtls_ssl_cf_memcpy_offset() is not actually constant flow - this is on purpose to be able to verify that the new test works. Signed-off-by: Manuel Pégourié-Gonnard --- include/mbedtls/config.h | 17 +++++++++++++++++ library/version_features.c | 3 +++ programs/ssl/query_config.c | 8 ++++++++ scripts/config.pl | 1 + tests/scripts/all.sh | 22 ++++++++++++++++++++++ tests/suites/helpers.function | 32 +++++++++++++++++++++++++++++++- 6 files changed, 82 insertions(+), 1 deletion(-) diff --git a/include/mbedtls/config.h b/include/mbedtls/config.h index 751fa0a72..a00eec5e4 100644 --- a/include/mbedtls/config.h +++ b/include/mbedtls/config.h @@ -565,6 +565,23 @@ */ //#define MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN +/** + * \def MBEDTLS_TEST_CONSTANT_FLOW_VALGRIND + * + * Enable testing of the constant-flow nature of some sensitive functions with + * valgrind's memcheck tool. This causes some existing tests to also test + * non-functional properties of the code under test. + * + * This setting requires valgrind headers for building, and is only useful for + * testing if the tests suites are run with valgrind's memcheck. + * + * \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_VALGRIND + /** * \def MBEDTLS_TEST_NULL_ENTROPY * diff --git a/library/version_features.c b/library/version_features.c index de14a2ff6..cbf38dc2c 100644 --- a/library/version_features.c +++ b/library/version_features.c @@ -280,6 +280,9 @@ static const char *features[] = { #if defined(MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN) "MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN", #endif /* MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN */ +#if defined(MBEDTLS_TEST_CONSTANT_FLOW_VALGRIND) + "MBEDTLS_TEST_CONSTANT_FLOW_VALGRIND", +#endif /* MBEDTLS_TEST_CONSTANT_FLOW_VALGRIND */ #if defined(MBEDTLS_TEST_NULL_ENTROPY) "MBEDTLS_TEST_NULL_ENTROPY", #endif /* MBEDTLS_TEST_NULL_ENTROPY */ diff --git a/programs/ssl/query_config.c b/programs/ssl/query_config.c index 7c8fbd8cd..798c9178f 100644 --- a/programs/ssl/query_config.c +++ b/programs/ssl/query_config.c @@ -747,6 +747,14 @@ int query_config( const char *config ) } #endif /* MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN */ +#if defined(MBEDTLS_TEST_CONSTANT_FLOW_VALGRIND) + if( strcmp( "MBEDTLS_TEST_CONSTANT_FLOW_VALGRIND", config ) == 0 ) + { + MACRO_EXPANSION_TO_STR( MBEDTLS_TEST_CONSTANT_FLOW_VALGRIND ); + return( 0 ); + } +#endif /* MBEDTLS_TEST_CONSTANT_FLOW_VALGRIND */ + #if defined(MBEDTLS_TEST_NULL_ENTROPY) if( strcmp( "MBEDTLS_TEST_NULL_ENTROPY", config ) == 0 ) { diff --git a/scripts/config.pl b/scripts/config.pl index 1cf143506..e5cc69756 100755 --- a/scripts/config.pl +++ b/scripts/config.pl @@ -127,6 +127,7 @@ MBEDTLS_REMOVE_ARC4_CIPHERSUITES MBEDTLS_RSA_NO_CRT MBEDTLS_SSL_HW_RECORD_ACCEL MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN +MBEDTLS_TEST_CONSTANT_FLOW_VALGRIND 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 8317cabc4..83299c97f 100755 --- a/tests/scripts/all.sh +++ b/tests/scripts/all.sh @@ -1047,6 +1047,28 @@ component_test_memsan_constant_flow () { make test } +component_test_valgrind_constant_flow () { + # This tests both (1) everything that valgrind's memcheck usually checks + # (heap buffer overflows, use of uninitialized memory, use-after-free, + # etc.) and (2) branches or memory access depending on secret values, + # which will be reported as uninitialized memory. To distinguish between + # secret and actually uninitialized: + # - unset MBEDTLS_TEST_CONSTANT_FLOW_VALGRIND - does the failure persist? + # - or alternatively, build with debug info and manually run the offending + # test suite with valgrind --track-origins=yes, then check if the origin + # was TEST_CF_SECRET() or something else. + msg "build: cmake release GCC, full config with constant flow testing" + scripts/config.pl full + scripts/config.pl set MBEDTLS_TEST_CONSTANT_FLOW_VALGRIND + cmake -D CMAKE_BUILD_TYPE:String=Release . + make + + # this only shows a summary of the results (how many of each type) + # details are left in Testing//DynamicAnalysis.xml + msg "test: main suites (valgrind + constant flow)" + make memcheck +} + 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 0b3d44b66..4f46ea967 100644 --- a/tests/suites/helpers.function +++ b/tests/suites/helpers.function @@ -46,6 +46,27 @@ typedef UINT32 uint32_t; #include #endif +/* + * Define the two macros + * + * #define TEST_CF_SECRET(ptr, size) + * #define TEST_CF_PUBLIC(ptr, size) + * + * that can be used in tests to mark a memory area as secret (no branch or + * memory access should depend on it) or public (default, only needs to be + * marked explicitly when it was derived from secret data). + * + * Arguments: + * - ptr: a pointer to the memory area to be marked + * - size: the size in bytes of the memory area + * + * Implementation: + * The basic idea is that of ctgrind : we can + * re-use tools that were designed for checking use of uninitialized memory. + * This file contains two implementations: one based on MemorySanitizer, the + * other on valgrind's memcheck. If none of them is enabled, dummy macros that + * do nothing are defined for convenience. + */ #if defined(MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN) #include @@ -55,7 +76,16 @@ typedef UINT32 uint32_t; #define TEST_CF_PUBLIC __msan_unpoison // void __msan_unpoison(const volatile void *a, size_t size); -#else /* MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN */ +#elif defined(MBEDTLS_TEST_CONSTANT_FLOW_VALGRIND) +#include + +#define TEST_CF_SECRET VALGRIND_MAKE_MEM_UNDEFINED +// VALGRIND_MAKE_MEM_UNDEFINED(_qzz_addr, _qzz_len) +#define TEST_CF_PUBLIC VALGRIND_MAKE_MEM_DEFINED +// VALGRIND_MAKE_MEM_DEFINED(_qzz_addr, _qzz_len) + +#else /* MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN || + MBEDTLS_TEST_CONSTANT_FLOW_VALGRIND */ #define TEST_CF_SECRET(ptr, size) #define TEST_CF_PUBLIC(ptr, size) From 46f49a8bbc5c5e1a97cfb245b667acbf2327444e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 25 Aug 2020 11:42:21 +0200 Subject: [PATCH 35/38] Improve comments on constant-flow testing in config.h 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 | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/include/mbedtls/config.h b/include/mbedtls/config.h index a00eec5e4..90a9aaafc 100644 --- a/include/mbedtls/config.h +++ b/include/mbedtls/config.h @@ -554,9 +554,10 @@ * * 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 non-functional property of the code under test. * - * This setting requires compiling with clang -fsanitize=memory. + * This setting requires compiling with clang -fsanitize=memory. The test + * suites can then be run normally. * * \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. @@ -570,10 +571,12 @@ * * Enable testing of the constant-flow nature of some sensitive functions with * valgrind's memcheck tool. This causes some existing tests to also test - * non-functional properties of the code under test. + * this non-functional property of the code under test. * * This setting requires valgrind headers for building, and is only useful for - * testing if the tests suites are run with valgrind's memcheck. + * testing if the tests suites are run with valgrind's memcheck. This can be + * done for an individual test suite with 'valgrind ./test_suite_xxx', or when + * using CMake, this can be done for all test suites with 'make memcheck'. * * \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. From ab9ec3287900c82f9a02e034301cf5a1b74ccb4a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 25 Aug 2020 11:43:10 +0200 Subject: [PATCH 36/38] Fix a typo in a comment 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 | 34 ++++++++-------------------------- 1 file changed, 8 insertions(+), 26 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 0cc6c30b1..2471600c9 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1435,27 +1435,6 @@ static void ssl_mac( mbedtls_md_context_t *md_ctx, #define SSL_SOME_MODES_USE_MAC #endif -/* The function below is only used in the Lucky 13 counter-measure in - * ssl_decrypt_buf(). These are the defines that guard the call site. */ -#if defined(SSL_SOME_MODES_USE_MAC) && \ - ( defined(MBEDTLS_SSL_PROTO_TLS1) || \ - defined(MBEDTLS_SSL_PROTO_TLS1_1) || \ - 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( const unsigned char *p, size_t len ) -{ - unsigned char acc = 0; - volatile unsigned char force; - - for( ; len != 0; p++, len-- ) - acc ^= *p; - - force = acc; - (void) force; -} -#endif /* SSL_SOME_MODES_USE_MAC && ( TLS1 || TLS1_1 || TLS1_2 ) */ - /* * Encryption/decryption functions */ @@ -1935,7 +1914,7 @@ cleanup: /* * Constant-flow memcpy from variable position in buffer. * - functionally equivalent to memcpy(dst, src + offset_secret, len) - * - but with execution flow independant from the value of offset_secret. + * - but with execution flow independent from the value of offset_secret. */ void mbedtls_ssl_cf_memcpy_offset( unsigned char *dst, const unsigned char *src_base, @@ -1943,10 +1922,13 @@ void mbedtls_ssl_cf_memcpy_offset( unsigned char *dst, size_t offset_min, size_t offset_max, size_t len ) { - /* WIP - THIS IS NOT ACTUALLY CONSTANT-FLOW! - * This is just to be able to write tests and check they work. */ - ssl_read_memory( src_base + offset_min, offset_max - offset_min + len ); - memcpy( dst, src_base + offset_secret, len ); + size_t offset; + + for( offset = offset_min; offset <= offset_max; offset++ ) + { + mbedtls_ssl_cf_memcpy_if_eq( dst, src_base + offset, len, + offset, offset_secret ); + } } #endif /* MBEDTLS_SSL_SOME_SUITES_USE_TLS_CBC */ From c9ebbd58435069cc74fbe4c22f6e7c45c3aced22 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Thu, 20 Aug 2020 12:17:05 +0200 Subject: [PATCH 37/38] Add a ChangeLog entry for local Lucky13 variant MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- ChangeLog.d/local-lucky13.txt | 9 +++++++++ 1 file changed, 9 insertions(+) create mode 100644 ChangeLog.d/local-lucky13.txt diff --git a/ChangeLog.d/local-lucky13.txt b/ChangeLog.d/local-lucky13.txt new file mode 100644 index 000000000..5a3eed0ba --- /dev/null +++ b/ChangeLog.d/local-lucky13.txt @@ -0,0 +1,9 @@ +Security + * Fix a local timing side channel vulnerability in (D)TLS record decryption + when using a CBC ciphersuites without the Encrypt-then-Mac extension. In + those circumstances, a local attacker able to observe the state of the + cache could use well-chosen functions to measure the exact computation + time of the HMAC, and follow up with the usual range of Lucky 13 attacks, + including plaintext recovery and key recovery. Found and reported by Tuba + Yavuz, Farhaan Fowze, Ken (Yihan) Bai, Grant Hernandez, and Kevin Butler + (University of Florida) and Dave Tian (Purdue University). From f0a3cddefecaa9509a06e65881f9f9f3a6431809 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Wed, 26 Aug 2020 10:10:11 +0200 Subject: [PATCH 38/38] Clarify that the Lucky 13 fix is quite general MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- ChangeLog.d/local-lucky13.txt | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/ChangeLog.d/local-lucky13.txt b/ChangeLog.d/local-lucky13.txt index 5a3eed0ba..adf493abe 100644 --- a/ChangeLog.d/local-lucky13.txt +++ b/ChangeLog.d/local-lucky13.txt @@ -1,9 +1,11 @@ Security - * Fix a local timing side channel vulnerability in (D)TLS record decryption - when using a CBC ciphersuites without the Encrypt-then-Mac extension. In - those circumstances, a local attacker able to observe the state of the - cache could use well-chosen functions to measure the exact computation - time of the HMAC, and follow up with the usual range of Lucky 13 attacks, - including plaintext recovery and key recovery. Found and reported by Tuba - Yavuz, Farhaan Fowze, Ken (Yihan) Bai, Grant Hernandez, and Kevin Butler + * In (D)TLS record decryption, when using a CBC ciphersuites without the + Encrypt-then-Mac extension, use constant code flow memory access patterns + to extract and check the MAC. This is an improvement to the existing + countermeasure against Lucky 13 attacks. The previous countermeasure was + effective against network-based attackers, but less so against local + attackers. The new countermeasure defends against local attackers, even + if they have access to fine-grained measurements. In particular, this + fixes a local Lucky 13 cache attack found and reported by Tuba Yavuz, + Farhaan Fowze, Ken (Yihan) Bai, Grant Hernandez, and Kevin Butler (University of Florida) and Dave Tian (Purdue University).