From f0f43c51c48c308ba54dad436cd08853cb817c88 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 d14b7296d..6938e3d7d 100644 --- a/library/dhm.c +++ b/library/dhm.c @@ -335,6 +335,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, @@ -344,7 +370,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, @@ -379,18 +405,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 a35e98a0601f29ee4eb8f9316d2e854c52737328 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 6938e3d7d..929136adc 100644 --- a/library/dhm.c +++ b/library/dhm.c @@ -371,6 +371,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, @@ -407,11 +410,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 6ab924de1de252e306c929339a0e74466854572d 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 96afccb0e..2878505c0 100644 --- a/library/rsa.c +++ b/library/rsa.c @@ -764,11 +764,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 406c7aedc474fd77ebc1a7baf1b86c6a8b0e7b62 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 2878505c0..d69456b04 100644 --- a/library/rsa.c +++ b/library/rsa.c @@ -746,6 +746,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 ) { @@ -765,17 +768,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 ff913e0ba6350d9016fe8d3946bc205ef5e1c4ba 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 ab601d6a1c4215f7a50ba059ea4a4f3f87e7e69b 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 d69456b04..677280f65 100644 --- a/library/rsa.c +++ b/library/rsa.c @@ -764,7 +764,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 b2b1d8e76213c27809e6dcaffd1aa438e37c035c 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 677280f65..ae0d1f058 100644 --- a/library/rsa.c +++ b/library/rsa.c @@ -771,25 +771,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 8ebb88d1e09c04f2a018544916cb08cf601b6da5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 28 Jul 2020 09:55:33 +0200 Subject: [PATCH 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 | 8 ++++++++ library/ssl_tls.c | 15 +++++---------- 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/include/mbedtls/ssl_internal.h b/include/mbedtls/ssl_internal.h index 4aa746373..35b8eedc0 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -144,6 +144,14 @@ #define MBEDTLS_SSL_RETRANS_WAITING 2 #define MBEDTLS_SSL_RETRANS_FINISHED 3 +/* For CBC-specific encrypt/decrypt code */ +#if defined(MBEDTLS_CIPHER_MODE_CBC) && \ + ( defined(MBEDTLS_AES_C) || \ + defined(MBEDTLS_CAMELLIA_C) || \ + defined(MBEDTLS_DES_C) ) +#define MBEDTLS_SSL_SOME_SUITES_USE_CBC +#endif + /* * Allow extra bytes for record, authentication and encryption overhead: * counter (8) + header (5) + IV(16) + MAC (16-48) + padding (0-256) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 66596bffc..7d9eae4ae 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1301,8 +1301,7 @@ static void ssl_mac( mbedtls_md_context_t *md_ctx, #endif /* MBEDTLS_SSL_PROTO_SSL3 */ #if defined(MBEDTLS_ARC4_C) || defined(MBEDTLS_CIPHER_NULL_CIPHER) || \ - ( defined(MBEDTLS_CIPHER_MODE_CBC) && \ - ( defined(MBEDTLS_AES_C) || defined(MBEDTLS_CAMELLIA_C) ) ) + defined(MBEDTLS_SSL_SOME_SUITES_USE_CBC) #define SSL_SOME_MODES_USE_MAC #endif @@ -1523,8 +1522,7 @@ static int ssl_encrypt_buf( mbedtls_ssl_context *ssl ) } else #endif /* MBEDTLS_GCM_C || MBEDTLS_CCM_C */ -#if defined(MBEDTLS_CIPHER_MODE_CBC) && \ - ( defined(MBEDTLS_AES_C) || defined(MBEDTLS_CAMELLIA_C) ) +#if defined(MBEDTLS_SSL_SOME_SUITES_USE_CBC) if( mode == MBEDTLS_MODE_CBC ) { int ret; @@ -1643,8 +1641,7 @@ static int ssl_encrypt_buf( mbedtls_ssl_context *ssl ) #endif /* MBEDTLS_SSL_ENCRYPT_THEN_MAC */ } else -#endif /* MBEDTLS_CIPHER_MODE_CBC && - ( MBEDTLS_AES_C || MBEDTLS_CAMELLIA_C ) */ +#endif /* MBEDTLS_SSL_SOME_SUITES_USE_CBC */ { MBEDTLS_SSL_DEBUG_MSG( 1, ( "should never happen" ) ); return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); @@ -1787,8 +1784,7 @@ static int ssl_decrypt_buf( mbedtls_ssl_context *ssl ) } else #endif /* MBEDTLS_GCM_C || MBEDTLS_CCM_C */ -#if defined(MBEDTLS_CIPHER_MODE_CBC) && \ - ( defined(MBEDTLS_AES_C) || defined(MBEDTLS_CAMELLIA_C) ) +#if defined(MBEDTLS_SSL_SOME_SUITES_USE_CBC) if( mode == MBEDTLS_MODE_CBC ) { /* @@ -1999,8 +1995,7 @@ static int ssl_decrypt_buf( mbedtls_ssl_context *ssl ) ssl->in_msglen -= padlen; } else -#endif /* MBEDTLS_CIPHER_MODE_CBC && - ( MBEDTLS_AES_C || MBEDTLS_CAMELLIA_C ) */ +#endif /* MBEDTLS_SSL_SOME_SUITES_USE_CBC) */ { MBEDTLS_SSL_DEBUG_MSG( 1, ( "should never happen" ) ); return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); From 3ba2bcaf0d2d5980b20d2d6f118a2d11a5f3f673 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 28 Jul 2020 10:19:45 +0200 Subject: [PATCH 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 | 92 ++++++++++++++++++++++++++++ 4 files changed, 178 insertions(+) diff --git a/include/mbedtls/ssl_internal.h b/include/mbedtls/ssl_internal.h index 35b8eedc0..9a0c19254 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -740,6 +740,50 @@ int mbedtls_ssl_get_key_exchange_md_tls1_2( mbedtls_ssl_context *ssl, #endif /* MBEDTLS_SSL_PROTO_TLS1 || MBEDTLS_SSL_PROTO_TLS1_1 || \ MBEDTLS_SSL_PROTO_TLS1_2 */ +#if defined(MBEDTLS_SSL_SOME_SUITES_USE_CBC) && \ + ( defined(MBEDTLS_SSL_PROTO_TLS1) || \ + defined(MBEDTLS_SSL_PROTO_TLS1_1) | \ + defined(MBEDTLS_SSL_PROTO_TLS1_2) ) +/** \brief Compute the HMAC of variable-length data with constant flow. + * + * This function computes the HMAC of the concatenation of \p add_data and \p + * data, and does with a code flow and memory access pattern that does not + * depend on \p data_len_secret, but only on \p min_data_len and \p + * max_data_len. In particular, this function always reads exactly \p + * max_data_len bytes from \p data. + * + * \param ctx The HMAC context. It must have keys configured + * with mbedtls_md_hmac_starts(). It is reset using + * mbedtls_md_hmac_reset() after the computation is + * complete to prepare for the next computation. + * \param add_data The additional data prepended to \p data. This + * must point to a readable buffer of \p add_data_len + * bytes. + * \param add_data_len The length of \p add_data in bytes. + * \param data The data appended to \p add_data. This must point + * to a readable buffer of \p max_data_len bytes. + * \param data_len_secret The length of the data to process in \p data. + * This must be no less than \p min_data_len and no + * greated than \p max_data_len. + * \param min_data_len The minimal length of \p data in bytes. + * \param max_data_len The maximal length of \p data in bytes. + * \param output The HMAC will be written here. This must point to + * a writeable buffer of sufficient size to hold the + * HMAC value. + * + * \retval 0 + * Success. + * \retval non-zero + * Failure. + */ +int mbedtls_ssl_cf_hmac( + mbedtls_md_context_t *ctx, + const unsigned char *add_data, size_t add_data_len, + const unsigned char *data, size_t data_len_secret, + size_t min_data_len, size_t max_data_len, + unsigned char *output ); +#endif /* MBEDTLS_SSL_SOME_SUITES_USE_CBC && TLS 1.0-1.2 */ + #ifdef __cplusplus } #endif diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 7d9eae4ae..c2e5bde99 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1659,6 +1659,32 @@ static int ssl_encrypt_buf( mbedtls_ssl_context *ssl ) return( 0 ); } +#if defined(MBEDTLS_SSL_SOME_SUITES_USE_CBC) && \ + ( defined(MBEDTLS_SSL_PROTO_TLS1) || \ + defined(MBEDTLS_SSL_PROTO_TLS1_1) || \ + defined(MBEDTLS_SSL_PROTO_TLS1_2) ) +/* + * Compute HMAC of variable-length data with constant flow. + */ +int mbedtls_ssl_cf_hmac( + mbedtls_md_context_t *ctx, + const unsigned char *add_data, size_t add_data_len, + const unsigned char *data, size_t data_len_secret, + size_t min_data_len, size_t max_data_len, + unsigned char *output ) +{ + /* WORK IN PROGRESS - THIS IS NOT CONSTANT FLOW AT ALL */ + (void) min_data_len; + (void) max_data_len; + mbedtls_md_hmac_update( ctx, add_data, add_data_len ); + mbedtls_md_hmac_update( ctx, data, data_len_secret ); + mbedtls_md_hmac_finish( ctx, output ); + mbedtls_md_hmac_reset( ctx ); + + return( 0 ); +} +#endif /* MBEDTLS_SSL_SOME_SUITES_USE_CBC && TLS 1.0-1.2 */ + static int ssl_decrypt_buf( mbedtls_ssl_context *ssl ) { size_t i; diff --git a/tests/suites/test_suite_ssl.data b/tests/suites/test_suite_ssl.data index b92c1fe8a..f85d26b11 100644 --- a/tests/suites/test_suite_ssl.data +++ b/tests/suites/test_suite_ssl.data @@ -57,3 +57,19 @@ ssl_dtls_replay:"abcd12340000,abcd12340100":"abcd123400ff":0 SSL SET_HOSTNAME memory leak: call ssl_set_hostname twice ssl_set_hostname_twice:"server0":"server1" + +Constant-flow HMAC: MD5 +depends_on:MBEDTLS_MD5_C +ssl_cf_hmac:MBEDTLS_MD_MD5 + +Constant-flow HMAC: SHA1 +depends_on:MBEDTLS_SHA1_C +ssl_cf_hmac:MBEDTLS_MD_SHA1 + +Constant-flow HMAC: SHA256 +depends_on:MBEDTLS_SHA256_C +ssl_cf_hmac:MBEDTLS_MD_SHA256 + +Constant-flow HMAC: SHA384 +depends_on:MBEDTLS_SHA512_C:!MBEDTLS_SHA512_NO_SHA384 +ssl_cf_hmac:MBEDTLS_MD_SHA384 diff --git a/tests/suites/test_suite_ssl.function b/tests/suites/test_suite_ssl.function index 1cd2ed5bb..2eafb2f94 100644 --- a/tests/suites/test_suite_ssl.function +++ b/tests/suites/test_suite_ssl.function @@ -54,3 +54,95 @@ void ssl_set_hostname_twice( char *hostname0, char *hostname1 ) } /* END_CASE */ +/* BEGIN_CASE depends_on:MBEDTLS_CIPHER_MODE_CBC:MBEDTLS_AES_C:MBEDTLS_SSL_PROTO_TLS1_2 */ +void ssl_cf_hmac( int hash ) +{ + /* + * Test the function mbedtls_ssl_cf_hmac() against a reference + * implementation. + * + * Note: the dependency is actually on TLS 1.0-1.2 and (AES or ARIA or + * Camellia or DES), but since the test framework doesn't support + * alternation in dependencies, just depend on the most common. + */ + mbedtls_md_context_t ctx, ref_ctx; + const mbedtls_md_info_t *md_info; + size_t out_len, block_size; + size_t min_in_len, in_len, max_in_len, i; + /* TLS additional data is 13 bytes (hence the "lucky 13" name) */ + unsigned char add_data[13]; + unsigned char ref_out[MBEDTLS_MD_MAX_SIZE]; + unsigned char *data = NULL; + unsigned char *out = NULL; + unsigned char rec_num = 0; + + mbedtls_md_init( &ctx ); + mbedtls_md_init( &ref_ctx ); + + md_info = mbedtls_md_info_from_type( hash ); + TEST_ASSERT( md_info != NULL ); + out_len = mbedtls_md_get_size( md_info ); + TEST_ASSERT( out_len != 0 ); + block_size = hash == MBEDTLS_MD_SHA384 ? 128 : 64; + + /* Use allocated out buffer to catch overwrites */ + out = mbedtls_calloc( 1, out_len ); + TEST_ASSERT( out != NULL ); + + /* Set up contexts with the given hash and a dummy key */ + TEST_ASSERT( 0 == mbedtls_md_setup( &ctx, md_info, 1 ) ); + TEST_ASSERT( 0 == mbedtls_md_setup( &ref_ctx, md_info, 1 ) ); + memset( ref_out, 42, sizeof( ref_out ) ); + TEST_ASSERT( 0 == mbedtls_md_hmac_starts( &ctx, ref_out, out_len ) ); + TEST_ASSERT( 0 == mbedtls_md_hmac_starts( &ref_ctx, ref_out, out_len ) ); + memset( ref_out, 0, sizeof( ref_out ) ); + + /* + * Test all possible lengths up to a point. The difference between + * max_in_len and min_in_len is at most 255, and make sure they both vary + * by at least one block size. + */ + for( max_in_len = 0; max_in_len <= 255 + block_size; max_in_len++ ) + { + /* Use allocated in buffer to catch overreads */ + data = mbedtls_calloc( 1, max_in_len ); + TEST_ASSERT( data != NULL || max_in_len == 0 ); + + min_in_len = max_in_len > 255 ? max_in_len - 255 : 0; + for( in_len = min_in_len; in_len <= max_in_len; in_len++ ) + { + /* Set up dummy data and add_data */ + rec_num++; + memset( add_data, rec_num, sizeof( add_data ) ); + for( i = 0; i < in_len; i++ ) + data[i] = ( i & 0xff ) ^ rec_num; + + /* Get the function's result */ + TEST_ASSERT( 0 == mbedtls_ssl_cf_hmac( &ctx, add_data, sizeof( add_data ), + data, in_len, + min_in_len, max_in_len, + out ) ); + + /* Compute the reference result */ + TEST_ASSERT( 0 == mbedtls_md_hmac_update( &ref_ctx, add_data, + sizeof( add_data ) ) ); + TEST_ASSERT( 0 == mbedtls_md_hmac_update( &ref_ctx, data, in_len ) ); + TEST_ASSERT( 0 == mbedtls_md_hmac_finish( &ref_ctx, ref_out ) ); + TEST_ASSERT( 0 == mbedtls_md_hmac_reset( &ref_ctx ) ); + + /* Compare */ + TEST_ASSERT( 0 == memcmp( out, ref_out, out_len ) ); + } + + mbedtls_free( data ); + data = NULL; + } + +exit: + mbedtls_md_free( &ref_ctx ); + mbedtls_md_free( &ctx ); + + mbedtls_free( data ); + mbedtls_free( out ); +} +/* END_CASE */ From d11971875a51673ecaef6e90d3a648d035609ae3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 28 Jul 2020 10:43:03 +0200 Subject: [PATCH 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 c2e5bde99..bc056eea5 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1313,7 +1313,7 @@ static void ssl_mac( mbedtls_md_context_t *md_ctx, defined(MBEDTLS_SSL_PROTO_TLS1_2) ) /* This function makes sure every byte in the memory region is accessed * (in ascending addresses order) */ -static void ssl_read_memory( unsigned char *p, size_t len ) +static void ssl_read_memory( const unsigned char *p, size_t len ) { unsigned char acc = 0; volatile unsigned char force; @@ -1673,12 +1673,83 @@ int mbedtls_ssl_cf_hmac( size_t min_data_len, size_t max_data_len, unsigned char *output ) { - /* WORK IN PROGRESS - THIS IS NOT CONSTANT FLOW AT ALL */ - (void) min_data_len; - (void) max_data_len; + /* WORK IN PROGRESS - THIS IS ONLY PSEUDO-CONTANT-TIME */ + + /* + * Process MAC and always update for padlen afterwards to make + * total time independent of padlen. + * + * Known timing attacks: + * - Lucky Thirteen (http://www.isg.rhul.ac.uk/tls/TLStiming.pdf) + * + * To compensate for different timings for the MAC calculation + * depending on how much padding was removed (which is determined + * by padlen), process extra_run more blocks through the hash + * function. + * + * The formula in the paper is + * extra_run = ceil( (L1-55) / 64 ) - ceil( (L2-55) / 64 ) + * where L1 is the size of the header plus the decrypted message + * plus CBC padding and L2 is the size of the header plus the + * decrypted message. This is for an underlying hash function + * with 64-byte blocks. + * We use ( (Lx+8) / 64 ) to handle 'negative Lx' values + * correctly. We round down instead of up, so -56 is the correct + * value for our calculations instead of -55. + * + * Repeat the formula rather than defining a block_size variable. + * This avoids requiring division by a variable at runtime + * (which would be marginally less efficient and would require + * linking an extra division function in some builds). + */ + size_t j, extra_run = 0; + /* This size is enough to server either as input to + * md_process() or as output to md_finish() */ + unsigned char tmp[128]; + + memset( tmp, 0, sizeof( tmp ) ); + + switch( mbedtls_md_get_type( ctx->md_info ) ) + { +#if defined(MBEDTLS_MD5_C) || defined(MBEDTLS_SHA1_C) || \ +defined(MBEDTLS_SHA256_C) + case MBEDTLS_MD_MD5: + case MBEDTLS_MD_SHA1: + case MBEDTLS_MD_SHA256: + /* 8 bytes of message size, 64-byte compression blocks */ + extra_run = ( add_data_len + max_data_len + 8 ) / 64 - + ( add_data_len + data_len_secret + 8 ) / 64; + break; +#endif +#if defined(MBEDTLS_SHA512_C) + case MBEDTLS_MD_SHA384: + /* 16 bytes of message size, 128-byte compression blocks */ + extra_run = ( add_data_len + max_data_len + 16 ) / 128 - + ( add_data_len + data_len_secret + 16 ) / 128; + break; +#endif + default: + return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); + } + mbedtls_md_hmac_update( ctx, add_data, add_data_len ); mbedtls_md_hmac_update( ctx, data, data_len_secret ); + /* Make sure we access everything even when padlen > 0. This + * makes the synchronisation requirements for just-in-time + * Prime+Probe attacks much tighter and hopefully impractical. */ + ssl_read_memory( data + min_data_len, max_data_len - min_data_len ); mbedtls_md_hmac_finish( ctx, output ); + + /* Dummy calls to compression function. + * Call mbedtls_md_process at least once due to cache attacks + * that observe whether md_process() was called of not. + * Respect the usual start-(process|update)-finish sequence for + * the sake of hardware accelerators that might require it. */ + mbedtls_md_starts( ctx ); + for( j = 0; j < extra_run + 1; j++ ) + mbedtls_md_process( ctx, tmp ); + mbedtls_md_finish( ctx, tmp ); + mbedtls_md_hmac_reset( ctx ); return( 0 ); @@ -2061,34 +2132,8 @@ static int ssl_decrypt_buf( mbedtls_ssl_context *ssl ) defined(MBEDTLS_SSL_PROTO_TLS1_2) if( ssl->minor_ver > MBEDTLS_SSL_MINOR_VERSION_0 ) { - /* - * Process MAC and always update for padlen afterwards to make - * total time independent of padlen. - * - * Known timing attacks: - * - Lucky Thirteen (http://www.isg.rhul.ac.uk/tls/TLStiming.pdf) - * - * To compensate for different timings for the MAC calculation - * depending on how much padding was removed (which is determined - * by padlen), process extra_run more blocks through the hash - * function. - * - * The formula in the paper is - * extra_run = ceil( (L1-55) / 64 ) - ceil( (L2-55) / 64 ) - * where L1 is the size of the header plus the decrypted message - * plus CBC padding and L2 is the size of the header plus the - * decrypted message. This is for an underlying hash function - * with 64-byte blocks. - * We use ( (Lx+8) / 64 ) to handle 'negative Lx' values - * correctly. We round down instead of up, so -56 is the correct - * value for our calculations instead of -55. - * - * Repeat the formula rather than defining a block_size variable. - * This avoids requiring division by a variable at runtime - * (which would be marginally less efficient and would require - * linking an extra division function in some builds). - */ - size_t j, extra_run = 0; + int ret; + unsigned char add_data[13]; /* * The next two sizes are the minimum and maximum values of @@ -2103,60 +2148,21 @@ static int ssl_decrypt_buf( mbedtls_ssl_context *ssl ) const size_t max_len = ssl->in_msglen + padlen; const size_t min_len = ( max_len > 256 ) ? max_len - 256 : 0; - switch( ssl->transform_in->ciphersuite_info->mac ) + memcpy( add_data + 0, ssl->in_ctr, 8 ); + memcpy( add_data + 8, ssl->in_hdr, 3 ); + memcpy( add_data + 11, ssl->in_len, 2 ); + + ret = mbedtls_ssl_cf_hmac( &ssl->transform_in->md_ctx_dec, + add_data, sizeof( add_data ), + ssl->in_msg, ssl->in_msglen, + min_len, max_len, + mac_expect ); + if( ret != 0 ) { -#if defined(MBEDTLS_MD5_C) || defined(MBEDTLS_SHA1_C) || \ - defined(MBEDTLS_SHA256_C) - case MBEDTLS_MD_MD5: - case MBEDTLS_MD_SHA1: - case MBEDTLS_MD_SHA256: - /* 8 bytes of message size, 64-byte compression blocks */ - extra_run = ( 13 + ssl->in_msglen + padlen + 8 ) / 64 - - ( 13 + ssl->in_msglen + 8 ) / 64; - break; -#endif -#if defined(MBEDTLS_SHA512_C) - case MBEDTLS_MD_SHA384: - /* 16 bytes of message size, 128-byte compression blocks */ - extra_run = ( 13 + ssl->in_msglen + padlen + 16 ) / 128 - - ( 13 + ssl->in_msglen + 16 ) / 128; - break; -#endif - default: - MBEDTLS_SSL_DEBUG_MSG( 1, ( "should never happen" ) ); - return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); + MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_ssl_cf_hmac", ret ); + return( ret ); } - extra_run &= correct * 0xFF; - - mbedtls_md_hmac_update( &ssl->transform_in->md_ctx_dec, ssl->in_ctr, 8 ); - mbedtls_md_hmac_update( &ssl->transform_in->md_ctx_dec, ssl->in_hdr, 3 ); - mbedtls_md_hmac_update( &ssl->transform_in->md_ctx_dec, ssl->in_len, 2 ); - mbedtls_md_hmac_update( &ssl->transform_in->md_ctx_dec, ssl->in_msg, - ssl->in_msglen ); - /* Make sure we access everything even when padlen > 0. This - * makes the synchronisation requirements for just-in-time - * Prime+Probe attacks much tighter and hopefully impractical. */ - ssl_read_memory( ssl->in_msg + ssl->in_msglen, padlen ); - mbedtls_md_hmac_finish( &ssl->transform_in->md_ctx_dec, mac_expect ); - - /* Dummy calls to compression function. - * Call mbedtls_md_process at least once due to cache attacks - * that observe whether md_process() was called of not. - * Respect the usual start-(process|update)-finish sequence for - * the sake of hardware accelerators that might require it. */ - mbedtls_md_starts( &ssl->transform_in->md_ctx_dec ); - for( j = 0; j < extra_run + 1; j++ ) - mbedtls_md_process( &ssl->transform_in->md_ctx_dec, ssl->in_msg ); - { - /* The switch statement above already checks that we're using - * one of MD-5, SHA-1, SHA-256 or SHA-384. */ - unsigned char tmp[384 / 8]; - mbedtls_md_finish( &ssl->transform_in->md_ctx_dec, tmp ); - } - - mbedtls_md_hmac_reset( &ssl->transform_in->md_ctx_dec ); - /* Make sure we access all the memory that could contain the MAC, * before we check it in the next code block. This makes the * synchronisation requirements for just-in-time Prime+Probe From 40597cef016d432f6e5145768c4a71dc19747d26 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 28 Jul 2020 10:53:06 +0200 Subject: [PATCH 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 +++ scripts/config.pl | 1 + tests/scripts/all.sh | 12 ++++++++++++ tests/suites/helpers.function | 16 ++++++++++++++++ 6 files changed, 55 insertions(+) diff --git a/include/mbedtls/check_config.h b/include/mbedtls/check_config.h index 1215e6eb9..040c1d259 100644 --- a/include/mbedtls/check_config.h +++ b/include/mbedtls/check_config.h @@ -182,6 +182,16 @@ #error "MBEDTLS_ENTROPY_FORCE_SHA256 defined, but not all prerequisites" #endif +#if defined(__has_feature) +#if __has_feature(memory_sanitizer) +#define MBEDTLS_HAS_MEMSAN +#endif +#endif +#if defined(MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN) && !defined(MBEDTLS_HAS_MEMSAN) +#error "MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN requires building with MemorySanitizer" +#endif +#undef MBEDTLS_HAS_MEMSAN + #if defined(MBEDTLS_TEST_NULL_ENTROPY) && \ ( !defined(MBEDTLS_ENTROPY_C) || !defined(MBEDTLS_NO_DEFAULT_ENTROPY_SOURCES) ) #error "MBEDTLS_TEST_NULL_ENTROPY defined, but not all prerequisites" diff --git a/include/mbedtls/config.h b/include/mbedtls/config.h index f08cc4a19..0e7dc77d8 100644 --- a/include/mbedtls/config.h +++ b/include/mbedtls/config.h @@ -444,6 +444,19 @@ //#define MBEDTLS_ECP_RANDOMIZE_MXZ_ALT //#define MBEDTLS_ECP_NORMALIZE_MXZ_ALT +/** + * \def MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN + * + * Enable testing of the constant-flow nature of some sensitive functions with + * clang's MemorySanitizer. This causes some existing tests to also test + * non-functional properties of the code under test. + * + * This setting requires compiling with clang -fsanitize=memory. + * + * Uncomment to enable testing of the constant-flow nature of seletected code. + */ +//#define MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN + /** * \def MBEDTLS_TEST_NULL_ENTROPY * diff --git a/library/version_features.c b/library/version_features.c index 454c2be7f..b5ac2da62 100644 --- a/library/version_features.c +++ b/library/version_features.c @@ -255,6 +255,9 @@ static const char *features[] = { #if defined(MBEDTLS_ECP_NORMALIZE_MXZ_ALT) "MBEDTLS_ECP_NORMALIZE_MXZ_ALT", #endif /* MBEDTLS_ECP_NORMALIZE_MXZ_ALT */ +#if defined(MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN) + "MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN", +#endif /* MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN */ #if defined(MBEDTLS_TEST_NULL_ENTROPY) "MBEDTLS_TEST_NULL_ENTROPY", #endif /* MBEDTLS_TEST_NULL_ENTROPY */ diff --git a/scripts/config.pl b/scripts/config.pl index 87f59bc60..6153dd7a3 100755 --- a/scripts/config.pl +++ b/scripts/config.pl @@ -126,6 +126,7 @@ MBEDTLS_REMOVE_3DES_CIPHERSUITES MBEDTLS_REMOVE_ARC4_CIPHERSUITES MBEDTLS_RSA_NO_CRT MBEDTLS_SSL_HW_RECORD_ACCEL +MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN MBEDTLS_TEST_NULL_ENTROPY MBEDTLS_X509_ALLOW_UNSUPPORTED_CRITICAL_EXTENSION MBEDTLS_ZLIB_SUPPORT diff --git a/tests/scripts/all.sh b/tests/scripts/all.sh index 5de2f064d..f010af8c3 100755 --- a/tests/scripts/all.sh +++ b/tests/scripts/all.sh @@ -963,6 +963,18 @@ component_test_full_cmake_clang () { if_build_succeeded env OPENSSL_CMD="$OPENSSL_LEGACY" GNUTLS_CLI="$GNUTLS_LEGACY_CLI" GNUTLS_SERV="$GNUTLS_LEGACY_SERV" tests/compat.sh -e '^$' -f 'NULL\|DES\|RC4\|ARCFOUR' } +component_test_memsan_constant_flow () { + msg "build: cmake memsan, full config with constant flow testing" + scripts/config.pl full + scripts/config.pl set MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN + scripts/config.pl unset MBEDTLS_AESNI_C # memsan doesn't grok asm + CC=clang cmake -D CMAKE_BUILD_TYPE:String=MemSan . + make + + msg "test: main suites (memsan constant flow)" + make test +} + component_test_default_no_deprecated () { # Test that removing the deprecated features from the default # configuration leaves something consistent. diff --git a/tests/suites/helpers.function b/tests/suites/helpers.function index 96e18da16..2a6d34c01 100644 --- a/tests/suites/helpers.function +++ b/tests/suites/helpers.function @@ -38,6 +38,22 @@ typedef UINT32 uint32_t; #include #endif +#if defined(MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN) +#include + +/* Use macros to avoid messing up with origin tracking */ +#define TEST_CF_SECRET __msan_allocated_memory +// void __msan_allocated_memory(const volatile void* data, size_t size); +#define TEST_CF_PUBLIC __msan_unpoison +// void __msan_unpoison(const volatile void *a, size_t size); + +#else /* MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN */ + +#define TEST_CF_SECRET(ptr, size) +#define TEST_CF_PUBLIC(ptr, size) + +#endif /* MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN */ + /*----------------------------------------------------------------------------*/ /* Constants */ From 961b4dd407f9b421fff81c4b0e329ef3dbcd607c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 28 Jul 2020 11:02:57 +0200 Subject: [PATCH 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 2eafb2f94..f7e3be32c 100644 --- a/tests/suites/test_suite_ssl.function +++ b/tests/suites/test_suite_ssl.function @@ -118,10 +118,13 @@ void ssl_cf_hmac( int hash ) data[i] = ( i & 0xff ) ^ rec_num; /* Get the function's result */ + TEST_CF_SECRET( &in_len, sizeof( in_len ) ); TEST_ASSERT( 0 == mbedtls_ssl_cf_hmac( &ctx, add_data, sizeof( add_data ), data, in_len, min_in_len, max_in_len, out ) ); + TEST_CF_PUBLIC( &in_len, sizeof( in_len ) ); + TEST_CF_PUBLIC( out, out_len ); /* Compute the reference result */ TEST_ASSERT( 0 == mbedtls_md_hmac_update( &ref_ctx, add_data, From 4508c67c42c65fff660f31a3073f12dd5ed62771 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 28 Jul 2020 11:25:34 +0200 Subject: [PATCH 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 bc056eea5..55c39783f 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1663,6 +1663,48 @@ static int ssl_encrypt_buf( mbedtls_ssl_context *ssl ) ( defined(MBEDTLS_SSL_PROTO_TLS1) || \ defined(MBEDTLS_SSL_PROTO_TLS1_1) || \ defined(MBEDTLS_SSL_PROTO_TLS1_2) ) +/* + * Constant-flow conditional memcpy: + * - if c1 == c2, equivalent to memcpy(dst, src, len), + * - otherwise, a no-op, + * but with execution flow independent of the values of c1 and c2. + * + * Use only bit operations to avoid branches that could be used by some + * compilers on some platforms to translate comparison operators. + */ +static void mbedtls_ssl_cf_memcpy_if_eq(unsigned char *dst, + const unsigned char *src, + size_t len, + size_t c1, size_t c2 ) +{ + /* diff = 0 if c1 == c2, non-zero otherwise */ + const size_t diff = c1 ^ c2; + + /* MSVC has a warning about unary minus on unsigned integer types, + * but this is well-defined and precisely what we want to do here. */ +#if defined(_MSC_VER) +#pragma warning( push ) +#pragma warning( disable : 4146 ) +#endif + + /* diff_msb's most significant bit is bit equal to c1 != c2 */ + const size_t diff_msb = ( diff | -diff ); + + /* diff1 = c1 != c2 */ + const size_t diff1 = diff_msb >> ( sizeof( diff_msb ) * 8 - 1 ); + + /* mask = c1 != c2 ? 0xff : 0x00 */ + unsigned char mask = (unsigned char) -diff1; + +#if defined(_MSC_VER) +#pragma warning( pop ) +#endif + + /* dst[i] = c1 != c2 ? dst[i] : src[i] */ + for( size_t i = 0; i < len; i++ ) + dst[i] = ( dst[i] & mask ) | ( src[i] & ~mask ); +} + /* * Compute HMAC of variable-length data with constant flow. */ @@ -1673,85 +1715,61 @@ int mbedtls_ssl_cf_hmac( size_t min_data_len, size_t max_data_len, unsigned char *output ) { - /* WORK IN PROGRESS - THIS IS ONLY PSEUDO-CONTANT-TIME */ - /* - * Process MAC and always update for padlen afterwards to make - * total time independent of padlen. + * This function breaks the HMAC abstraction and uses the md_clone() + * extension to the MD API in order to get constant-flow behaviour. * - * Known timing attacks: - * - Lucky Thirteen (http://www.isg.rhul.ac.uk/tls/TLStiming.pdf) + * HMAC(msg) is defined as HASH(okey + HASH(ikey + msg)) where + means + * concatenation, and okey/ikey is the XOR of the key with some fix bit + * patterns (see RFC 2104, sec. 2), which are stored in ctx->hmac_ctx. * - * To compensate for different timings for the MAC calculation - * depending on how much padding was removed (which is determined - * by padlen), process extra_run more blocks through the hash - * function. + * We'll first compute inner_hash = HASH(ikey + msg) by hashing up to + * minlen, then cloning the context, and for each byte up to maxlen + * finishing up the hash computation, keeping only the correct result. * - * The formula in the paper is - * extra_run = ceil( (L1-55) / 64 ) - ceil( (L2-55) / 64 ) - * where L1 is the size of the header plus the decrypted message - * plus CBC padding and L2 is the size of the header plus the - * decrypted message. This is for an underlying hash function - * with 64-byte blocks. - * We use ( (Lx+8) / 64 ) to handle 'negative Lx' values - * correctly. We round down instead of up, so -56 is the correct - * value for our calculations instead of -55. - * - * Repeat the formula rather than defining a block_size variable. - * This avoids requiring division by a variable at runtime - * (which would be marginally less efficient and would require - * linking an extra division function in some builds). + * Then we only need to compute HASH(okey + inner_hash) and we're done. */ - size_t j, extra_run = 0; - /* This size is enough to server either as input to - * md_process() or as output to md_finish() */ - unsigned char tmp[128]; + const mbedtls_md_type_t md_alg = mbedtls_md_get_type( ctx->md_info ); + const size_t block_size = md_alg == MBEDTLS_MD_SHA384 ? 128 : 64; + const unsigned char * const ikey = (unsigned char *) ctx->hmac_ctx; + const unsigned char * const okey = ikey + block_size; + const size_t hash_size = mbedtls_md_get_size( ctx->md_info ); - memset( tmp, 0, sizeof( tmp ) ); + unsigned char aux_out[MBEDTLS_MD_MAX_SIZE]; + mbedtls_md_context_t aux; + size_t offset; - switch( mbedtls_md_get_type( ctx->md_info ) ) + mbedtls_md_init( &aux ); + mbedtls_md_setup( &aux, ctx->md_info, 0 ); + + /* After hmac_start() of hmac_reset(), ikey has already been hashed, + * so we can start directly with the message */ + mbedtls_md_update( ctx, add_data, add_data_len ); + mbedtls_md_update( ctx, data, min_data_len ); + + /* For each possible length, compute the hash up to that point */ + for( offset = min_data_len; offset <= max_data_len; offset++ ) { -#if defined(MBEDTLS_MD5_C) || defined(MBEDTLS_SHA1_C) || \ -defined(MBEDTLS_SHA256_C) - case MBEDTLS_MD_MD5: - case MBEDTLS_MD_SHA1: - case MBEDTLS_MD_SHA256: - /* 8 bytes of message size, 64-byte compression blocks */ - extra_run = ( add_data_len + max_data_len + 8 ) / 64 - - ( add_data_len + data_len_secret + 8 ) / 64; - break; -#endif -#if defined(MBEDTLS_SHA512_C) - case MBEDTLS_MD_SHA384: - /* 16 bytes of message size, 128-byte compression blocks */ - extra_run = ( add_data_len + max_data_len + 16 ) / 128 - - ( add_data_len + data_len_secret + 16 ) / 128; - break; -#endif - default: - return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); + mbedtls_md_clone( &aux, ctx ); + mbedtls_md_finish( &aux, aux_out ); + /* Keep only the correct inner_hash in the output buffer */ + mbedtls_ssl_cf_memcpy_if_eq( output, aux_out, hash_size, + offset, data_len_secret ); + + if( offset < max_data_len ) + mbedtls_md_update( ctx, data + offset, 1 ); } - mbedtls_md_hmac_update( ctx, add_data, add_data_len ); - mbedtls_md_hmac_update( ctx, data, data_len_secret ); - /* Make sure we access everything even when padlen > 0. This - * makes the synchronisation requirements for just-in-time - * Prime+Probe attacks much tighter and hopefully impractical. */ - ssl_read_memory( data + min_data_len, max_data_len - min_data_len ); - mbedtls_md_hmac_finish( ctx, output ); - - /* Dummy calls to compression function. - * Call mbedtls_md_process at least once due to cache attacks - * that observe whether md_process() was called of not. - * Respect the usual start-(process|update)-finish sequence for - * the sake of hardware accelerators that might require it. */ + /* Now compute HASH(okey + inner_hash) */ mbedtls_md_starts( ctx ); - for( j = 0; j < extra_run + 1; j++ ) - mbedtls_md_process( ctx, tmp ); - mbedtls_md_finish( ctx, tmp ); + mbedtls_md_update( ctx, okey, block_size ); + mbedtls_md_update( ctx, output, hash_size ); + mbedtls_md_finish( ctx, output ); + /* Done, get ready for next time */ mbedtls_md_hmac_reset( ctx ); + mbedtls_md_free( &aux ); return( 0 ); } #endif /* MBEDTLS_SSL_SOME_SUITES_USE_CBC && TLS 1.0-1.2 */ From 41df0f2bca3d5b38311ab2424fa68481451b623e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 28 Jul 2020 11:35:39 +0200 Subject: [PATCH 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 9a0c19254..42172ad2e 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -152,6 +152,13 @@ #define MBEDTLS_SSL_SOME_SUITES_USE_CBC #endif +#if defined(MBEDTLS_SSL_SOME_SUITES_USE_CBC) && \ + ( defined(MBEDTLS_SSL_PROTO_TLS1) || \ + defined(MBEDTLS_SSL_PROTO_TLS1_1) || \ + defined(MBEDTLS_SSL_PROTO_TLS1_2) ) +#define MBEDTLS_SSL_SOME_SUITES_USE_TLS_CBC +#endif + /* * Allow extra bytes for record, authentication and encryption overhead: * counter (8) + header (5) + IV(16) + MAC (16-48) + padding (0-256) @@ -740,10 +747,7 @@ int mbedtls_ssl_get_key_exchange_md_tls1_2( mbedtls_ssl_context *ssl, #endif /* MBEDTLS_SSL_PROTO_TLS1 || MBEDTLS_SSL_PROTO_TLS1_1 || \ MBEDTLS_SSL_PROTO_TLS1_2 */ -#if defined(MBEDTLS_SSL_SOME_SUITES_USE_CBC) && \ - ( defined(MBEDTLS_SSL_PROTO_TLS1) || \ - defined(MBEDTLS_SSL_PROTO_TLS1_1) | \ - defined(MBEDTLS_SSL_PROTO_TLS1_2) ) +#if defined(MBEDTLS_SSL_SOME_SUITES_USE_TLS_CBC) /** \brief Compute the HMAC of variable-length data with constant flow. * * This function computes the HMAC of the concatenation of \p add_data and \p @@ -782,7 +786,7 @@ int mbedtls_ssl_cf_hmac( const unsigned char *data, size_t data_len_secret, size_t min_data_len, size_t max_data_len, unsigned char *output ); -#endif /* MBEDTLS_SSL_SOME_SUITES_USE_CBC && TLS 1.0-1.2 */ +#endif /* MBEDTLS_SSL_SOME_SUITES_USE_TLS_CBC */ #ifdef __cplusplus } diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 55c39783f..5889659e2 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1659,10 +1659,7 @@ static int ssl_encrypt_buf( mbedtls_ssl_context *ssl ) return( 0 ); } -#if defined(MBEDTLS_SSL_SOME_SUITES_USE_CBC) && \ - ( defined(MBEDTLS_SSL_PROTO_TLS1) || \ - defined(MBEDTLS_SSL_PROTO_TLS1_1) || \ - defined(MBEDTLS_SSL_PROTO_TLS1_2) ) +#if defined(MBEDTLS_SSL_SOME_SUITES_USE_TLS_CBC) /* * Constant-flow conditional memcpy: * - if c1 == c2, equivalent to memcpy(dst, src, len), @@ -1772,7 +1769,7 @@ int mbedtls_ssl_cf_hmac( mbedtls_md_free( &aux ); return( 0 ); } -#endif /* MBEDTLS_SSL_SOME_SUITES_USE_CBC && TLS 1.0-1.2 */ +#endif /* MBEDTLS_SSL_SOME_SUITES_USE_TLS_CBC */ static int ssl_decrypt_buf( mbedtls_ssl_context *ssl ) { diff --git a/tests/suites/test_suite_ssl.function b/tests/suites/test_suite_ssl.function index f7e3be32c..d3204778c 100644 --- a/tests/suites/test_suite_ssl.function +++ b/tests/suites/test_suite_ssl.function @@ -54,7 +54,7 @@ void ssl_set_hostname_twice( char *hostname0, char *hostname1 ) } /* END_CASE */ -/* BEGIN_CASE depends_on:MBEDTLS_CIPHER_MODE_CBC:MBEDTLS_AES_C:MBEDTLS_SSL_PROTO_TLS1_2 */ +/* BEGIN_CASE depends_on:MBEDTLS_SSL_SOME_SUITES_USE_TLS_CBC */ void ssl_cf_hmac( int hash ) { /* From ec956b186124159d1f881f30a99d878c9a2e1eac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 28 Jul 2020 11:42:31 +0200 Subject: [PATCH 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 42172ad2e..4b9056895 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -757,9 +757,11 @@ int mbedtls_ssl_get_key_exchange_md_tls1_2( mbedtls_ssl_context *ssl, * max_data_len bytes from \p data. * * \param ctx The HMAC context. It must have keys configured - * with mbedtls_md_hmac_starts(). It is reset using - * mbedtls_md_hmac_reset() after the computation is - * complete to prepare for the next computation. + * with mbedtls_md_hmac_starts() and use one of the + * following hashes: SHA-384, SHA-256, SHA-1 or MD-5. + * It is reset using mbedtls_md_hmac_reset() after + * the computation is complete to prepare for the + * next computation. * \param add_data The additional data prepended to \p data. This * must point to a readable buffer of \p add_data_len * bytes. diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 5889659e2..07fad1c09 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1717,7 +1717,7 @@ int mbedtls_ssl_cf_hmac( * extension to the MD API in order to get constant-flow behaviour. * * HMAC(msg) is defined as HASH(okey + HASH(ikey + msg)) where + means - * concatenation, and okey/ikey is the XOR of the key with some fix bit + * concatenation, and okey/ikey are the XOR of the key with some fixed bit * patterns (see RFC 2104, sec. 2), which are stored in ctx->hmac_ctx. * * We'll first compute inner_hash = HASH(ikey + msg) by hashing up to @@ -1727,6 +1727,8 @@ int mbedtls_ssl_cf_hmac( * Then we only need to compute HASH(okey + inner_hash) and we're done. */ const mbedtls_md_type_t md_alg = mbedtls_md_get_type( ctx->md_info ); + /* TLS 1.0-1.2 only support SHA-384, SHA-256, SHA-1, MD-5, + * all of which have the same block size except SHA-384. */ const size_t block_size = md_alg == MBEDTLS_MD_SHA384 ? 128 : 64; const unsigned char * const ikey = (unsigned char *) ctx->hmac_ctx; const unsigned char * const okey = ikey + block_size; From c9ef5a2b761fe25a6b3d3386e95102515d426a6c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 28 Jul 2020 11:45:02 +0200 Subject: [PATCH 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 07fad1c09..a16568cc4 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1730,7 +1730,7 @@ int mbedtls_ssl_cf_hmac( /* TLS 1.0-1.2 only support SHA-384, SHA-256, SHA-1, MD-5, * all of which have the same block size except SHA-384. */ const size_t block_size = md_alg == MBEDTLS_MD_SHA384 ? 128 : 64; - const unsigned char * const ikey = (unsigned char *) ctx->hmac_ctx; + const unsigned char * const ikey = ctx->hmac_ctx; const unsigned char * const okey = ikey + block_size; const size_t hash_size = mbedtls_md_get_size( ctx->md_info ); From 0cd0c731fdea220a485a97654d29f1b734e17c27 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 28 Jul 2020 11:49:42 +0200 Subject: [PATCH 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 a16568cc4..6e6cc0897 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1737,39 +1737,51 @@ int mbedtls_ssl_cf_hmac( unsigned char aux_out[MBEDTLS_MD_MAX_SIZE]; mbedtls_md_context_t aux; size_t offset; + int ret; mbedtls_md_init( &aux ); - mbedtls_md_setup( &aux, ctx->md_info, 0 ); + +#define MD_CHK( func_call ) \ + do { \ + ret = (func_call); \ + if( ret != 0 ) \ + goto cleanup; \ + } while( 0 ) + + MD_CHK( mbedtls_md_setup( &aux, ctx->md_info, 0 ) ); /* After hmac_start() of hmac_reset(), ikey has already been hashed, * so we can start directly with the message */ - mbedtls_md_update( ctx, add_data, add_data_len ); - mbedtls_md_update( ctx, data, min_data_len ); + MD_CHK( mbedtls_md_update( ctx, add_data, add_data_len ) ); + MD_CHK( mbedtls_md_update( ctx, data, min_data_len ) ); /* For each possible length, compute the hash up to that point */ for( offset = min_data_len; offset <= max_data_len; offset++ ) { - mbedtls_md_clone( &aux, ctx ); - mbedtls_md_finish( &aux, aux_out ); + MD_CHK( mbedtls_md_clone( &aux, ctx ) ); + MD_CHK( mbedtls_md_finish( &aux, aux_out ) ); /* Keep only the correct inner_hash in the output buffer */ mbedtls_ssl_cf_memcpy_if_eq( output, aux_out, hash_size, offset, data_len_secret ); if( offset < max_data_len ) - mbedtls_md_update( ctx, data + offset, 1 ); + MD_CHK( mbedtls_md_update( ctx, data + offset, 1 ) ); } /* Now compute HASH(okey + inner_hash) */ - mbedtls_md_starts( ctx ); - mbedtls_md_update( ctx, okey, block_size ); - mbedtls_md_update( ctx, output, hash_size ); - mbedtls_md_finish( ctx, output ); + MD_CHK( mbedtls_md_starts( ctx ) ); + MD_CHK( mbedtls_md_update( ctx, okey, block_size ) ); + MD_CHK( mbedtls_md_update( ctx, output, hash_size ) ); + MD_CHK( mbedtls_md_finish( ctx, output ) ); /* Done, get ready for next time */ - mbedtls_md_hmac_reset( ctx ); + MD_CHK( mbedtls_md_hmac_reset( ctx ) ); +#undef MD_CHK + +cleanup: mbedtls_md_free( &aux ); - return( 0 ); + return( ret ); } #endif /* MBEDTLS_SSL_SOME_SUITES_USE_TLS_CBC */ From 2b223fd539aec6e9a86fe616a731222d0bbc7309 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Wed, 22 Jul 2020 11:09:28 +0200 Subject: [PATCH 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 f010af8c3..06ad5ee0f 100755 --- a/tests/scripts/all.sh +++ b/tests/scripts/all.sh @@ -964,14 +964,20 @@ component_test_full_cmake_clang () { } component_test_memsan_constant_flow () { - msg "build: cmake memsan, full config with constant flow testing" + # This tests both (1) accesses to undefined memory, and (2) branches or + # memory access depending on secret values. To distinguish between those: + # - unset MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN - does the failure persist? + # - or alternatively, change the build type to MemSanDbg, which enables + # origin tracking and nicer stack traces (which are useful for debugging + # anyway), and check if the origin was TEST_CF_SECRET() or something else. + msg "build: cmake MSan (clang), full config with constant flow testing" scripts/config.pl full scripts/config.pl set MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN scripts/config.pl unset MBEDTLS_AESNI_C # memsan doesn't grok asm CC=clang cmake -D CMAKE_BUILD_TYPE:String=MemSan . make - msg "test: main suites (memsan constant flow)" + msg "test: main suites (Msan + constant flow)" make test } From 2810110bbaf6c6a9c9eb123e79748f5e0e3a1a45 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 28 Jul 2020 11:54:35 +0200 Subject: [PATCH 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 0e7dc77d8..bd029cb89 100644 --- a/include/mbedtls/config.h +++ b/include/mbedtls/config.h @@ -453,7 +453,7 @@ * * This setting requires compiling with clang -fsanitize=memory. * - * Uncomment to enable testing of the constant-flow nature of seletected code. + * Uncomment to enable testing of the constant-flow nature of selected code. */ //#define MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN diff --git a/include/mbedtls/ssl_internal.h b/include/mbedtls/ssl_internal.h index 4b9056895..24b13a3c6 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -770,11 +770,11 @@ int mbedtls_ssl_get_key_exchange_md_tls1_2( mbedtls_ssl_context *ssl, * to a readable buffer of \p max_data_len bytes. * \param data_len_secret The length of the data to process in \p data. * This must be no less than \p min_data_len and no - * greated than \p max_data_len. + * greater than \p max_data_len. * \param min_data_len The minimal length of \p data in bytes. * \param max_data_len The maximal length of \p data in bytes. * \param output The HMAC will be written here. This must point to - * a writeable buffer of sufficient size to hold the + * a writable buffer of sufficient size to hold the * HMAC value. * * \retval 0 From 2da9a545597a9e6e480e2554044a418e59433b6b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 28 Jul 2020 11:56:05 +0200 Subject: [PATCH 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 6e6cc0897..725596ddd 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1669,10 +1669,10 @@ static int ssl_encrypt_buf( mbedtls_ssl_context *ssl ) * Use only bit operations to avoid branches that could be used by some * compilers on some platforms to translate comparison operators. */ -static void mbedtls_ssl_cf_memcpy_if_eq(unsigned char *dst, - const unsigned char *src, - size_t len, - size_t c1, size_t c2 ) +static void mbedtls_ssl_cf_memcpy_if_eq( unsigned char *dst, + const unsigned char *src, + size_t len, + size_t c1, size_t c2 ) { /* diff = 0 if c1 == c2, non-zero otherwise */ const size_t diff = c1 ^ c2; From 2f484bd9799e676ff3d48e664339eaf08242808d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 28 Jul 2020 11:57:25 +0200 Subject: [PATCH 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 725596ddd..1248c6e2d 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1684,14 +1684,14 @@ static void mbedtls_ssl_cf_memcpy_if_eq( unsigned char *dst, #pragma warning( disable : 4146 ) #endif - /* diff_msb's most significant bit is bit equal to c1 != c2 */ + /* diff_msb's most significant bit is equal to c1 != c2 */ const size_t diff_msb = ( diff | -diff ); /* diff1 = c1 != c2 */ const size_t diff1 = diff_msb >> ( sizeof( diff_msb ) * 8 - 1 ); /* mask = c1 != c2 ? 0xff : 0x00 */ - unsigned char mask = (unsigned char) -diff1; + const unsigned char mask = (unsigned char) -diff1; #if defined(_MSC_VER) #pragma warning( pop ) From e05e57619b7ba05e7474e1f9742dc0271b607ee9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Wed, 29 Jul 2020 10:04:36 +0200 Subject: [PATCH 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 1248c6e2d..a65143c7e 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1698,7 +1698,8 @@ static void mbedtls_ssl_cf_memcpy_if_eq( unsigned char *dst, #endif /* dst[i] = c1 != c2 ? dst[i] : src[i] */ - for( size_t i = 0; i < len; i++ ) + size_t i; + for( i = 0; i < len; i++ ) dst[i] = ( dst[i] & mask ) | ( src[i] & ~mask ); } From 7cf5ebc90fae38400eadcdd8f878e46ee7b51990 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Wed, 29 Jul 2020 12:54:04 +0200 Subject: [PATCH 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 a65143c7e..3b4bfadf6 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1705,6 +1705,9 @@ static void mbedtls_ssl_cf_memcpy_if_eq( unsigned char *dst, /* * Compute HMAC of variable-length data with constant flow. + * + * Only works with MD-5, SHA-1, SHA-256 and SHA-384. + * (Otherwise, computation of block_size needs to be adapted.) */ int mbedtls_ssl_cf_hmac( mbedtls_md_context_t *ctx, From 21b198355d0c9aa1bb4448586161864db2f54d48 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Fri, 31 Jul 2020 10:00:17 +0200 Subject: [PATCH 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 d3204778c..0987374fb 100644 --- a/tests/suites/test_suite_ssl.function +++ b/tests/suites/test_suite_ssl.function @@ -60,10 +60,6 @@ void ssl_cf_hmac( int hash ) /* * Test the function mbedtls_ssl_cf_hmac() against a reference * implementation. - * - * Note: the dependency is actually on TLS 1.0-1.2 and (AES or ARIA or - * Camellia or DES), but since the test framework doesn't support - * alternation in dependencies, just depend on the most common. */ mbedtls_md_context_t ctx, ref_ctx; const mbedtls_md_info_t *md_info; From 757c2d5c2c33fa364d31d96a502a356020aa3b06 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Fri, 31 Jul 2020 12:53:39 +0200 Subject: [PATCH 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 24b13a3c6..d737c87c5 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -144,7 +144,7 @@ #define MBEDTLS_SSL_RETRANS_WAITING 2 #define MBEDTLS_SSL_RETRANS_FINISHED 3 -/* For CBC-specific encrypt/decrypt code */ +/* This macro determines whether CBC is supported. */ #if defined(MBEDTLS_CIPHER_MODE_CBC) && \ ( defined(MBEDTLS_AES_C) || \ defined(MBEDTLS_CAMELLIA_C) || \ @@ -152,6 +152,8 @@ #define MBEDTLS_SSL_SOME_SUITES_USE_CBC #endif +/* This macro determines whether the CBC construct used in TLS 1.0-1.2 (as + * opposed to the very different CBC construct used in SSLv3) is supported. */ #if defined(MBEDTLS_SSL_SOME_SUITES_USE_CBC) && \ ( defined(MBEDTLS_SSL_PROTO_TLS1) || \ defined(MBEDTLS_SSL_PROTO_TLS1_1) || \ From 4c575fba853683058d26f99dc8856cb1bdd628a5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Fri, 31 Jul 2020 12:59:34 +0200 Subject: [PATCH 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 bd029cb89..2ef54c5b4 100644 --- a/include/mbedtls/config.h +++ b/include/mbedtls/config.h @@ -453,6 +453,9 @@ * * This setting requires compiling with clang -fsanitize=memory. * + * \warning This macro is only used for extended testing; it is not considered + * part of the library's API, so it may change or disappear at any time. + * * Uncomment to enable testing of the constant-flow nature of selected code. */ //#define MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN From ef73875913c66767e7a954aa0b68f42f0756d9b2 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 25a43bb38..30f0eba9e 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -7320,6 +7320,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_zeroize( ssl->in_offt, n ); + if( ssl->in_msglen == 0 ) { /* all bytes consumed */ From 0e6f3b766188d71df4c79ae09183d6e0dc7c691c 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 4ac28b8d1eef9dfc9a97eb3ea6761cf993ca7158 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 41f5cc79b..d231fde31 100644 --- a/tests/suites/test_suite_x509parse.data +++ b/tests/suites/test_suite_x509parse.data @@ -1324,6 +1324,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 78e54b9b1de13f8934b85a275a39dc4efa2eb8a2 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 846cd45d0..1ecf7eb99 100644 --- a/library/x509_crl.c +++ b/library/x509_crl.c @@ -289,13 +289,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 e447f47cc897043fb4f17a99f48afb0fa36b1b6d 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 d231fde31..66cbde6ec 100644 --- a/tests/suites/test_suite_x509parse.data +++ b/tests/suites/test_suite_x509parse.data @@ -1324,6 +1324,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 @@ -1356,10 +1378,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 bf7a49eacce9776ec8e8c41e0d599e81efdb765b 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 fdd1a7111..a3212cced 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -2142,6 +2142,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; @@ -2156,6 +2157,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 */ @@ -2200,6 +2203,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 || \ @@ -2211,11 +2216,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 3b490a0a0146f703c3cd6cf3bdd82f78db102cf0 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 c48ba9859..6a04c8d05 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -788,6 +788,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 a3212cced..8c9925b58 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1785,6 +1785,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 ) @@ -2197,14 +2214,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 f85d26b11..84b56c308 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 0987374fb..0efcce7fe 100644 --- a/tests/suites/test_suite_ssl.function +++ b/tests/suites/test_suite_ssl.function @@ -145,3 +145,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 426c2d4a388d087cea397016f4fcd64b7355106f 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 +++ scripts/config.pl | 1 + tests/scripts/all.sh | 22 ++++++++++++++++++++++ tests/suites/helpers.function | 32 +++++++++++++++++++++++++++++++- 5 files changed, 74 insertions(+), 1 deletion(-) diff --git a/include/mbedtls/config.h b/include/mbedtls/config.h index 82d8d8b7f..8f3dba97d 100644 --- a/include/mbedtls/config.h +++ b/include/mbedtls/config.h @@ -458,6 +458,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 dbe6ba6ad..a3e3df4f9 100644 --- a/library/version_features.c +++ b/library/version_features.c @@ -256,6 +256,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/scripts/config.pl b/scripts/config.pl index 2dadc87c0..80b6cd88c 100755 --- a/scripts/config.pl +++ b/scripts/config.pl @@ -125,6 +125,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 81396945b..50f98eb29 100755 --- a/tests/scripts/all.sh +++ b/tests/scripts/all.sh @@ -979,6 +979,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 3309173d3..c1c76ec09 100644 --- a/tests/suites/helpers.function +++ b/tests/suites/helpers.function @@ -38,6 +38,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 @@ -47,7 +68,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 f4435c4fedb36d8b38b38ab5c942f45d152ae25b 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 8f3dba97d..d8332dae5 100644 --- a/include/mbedtls/config.h +++ b/include/mbedtls/config.h @@ -447,9 +447,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. @@ -463,10 +464,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 520e78b830efe750960fa93c54949659498b6b5c 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 8c9925b58..3b06feee8 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1303,27 +1303,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 */ @@ -1789,7 +1768,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, @@ -1797,10 +1776,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 c3f68378bc7cd703554a40866ac2bdfdcfe16167 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 f530c8018b5e2b5f6aca04ded9e31874d368f751 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).