From dc269bbd0853b6d85ecea5ded6d4a1208f2886c8 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 13 Dec 2021 12:32:43 +0100 Subject: [PATCH 1/8] mbedtls_cipher_check_tag: zeroize expected tag on tag mismatch Signed-off-by: Gilles Peskine --- library/cipher.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/library/cipher.c b/library/cipher.c index d51ccd77f..0c5bcda66 100644 --- a/library/cipher.c +++ b/library/cipher.c @@ -1125,6 +1125,12 @@ int mbedtls_cipher_check_tag( mbedtls_cipher_context_t *ctx, } #endif /* MBEDTLS_USE_PSA_CRYPTO */ + /* Status to return on a non-authenticated algorithm. It would make sense + * to return MBEDTLS_ERR_CIPHER_INVALID_CONTEXT or perhaps + * MBEDTLS_ERR_CIPHER_BAD_INPUT_DATA, but at the time I write this our + * unit tests assume 0. */ + ret = 0; + #if defined(MBEDTLS_GCM_C) if( MBEDTLS_MODE_GCM == ctx->cipher_info->mode ) { @@ -1140,9 +1146,7 @@ int mbedtls_cipher_check_tag( mbedtls_cipher_context_t *ctx, /* Check the tag in "constant-time" */ if( mbedtls_ct_memcmp( tag, check_tag, tag_len ) != 0 ) - return( MBEDTLS_ERR_CIPHER_AUTH_FAILED ); - - return( 0 ); + ret = MBEDTLS_ERR_CIPHER_AUTH_FAILED; } #endif /* MBEDTLS_GCM_C */ @@ -1162,13 +1166,12 @@ int mbedtls_cipher_check_tag( mbedtls_cipher_context_t *ctx, /* Check the tag in "constant-time" */ if( mbedtls_ct_memcmp( tag, check_tag, tag_len ) != 0 ) - return( MBEDTLS_ERR_CIPHER_AUTH_FAILED ); - - return( 0 ); + ret = MBEDTLS_ERR_CIPHER_AUTH_FAILED; } #endif /* MBEDTLS_CHACHAPOLY_C */ - return( 0 ); + mbedtls_platform_zeroize( check_tag, tag_len ); + return( ret ); } #endif /* MBEDTLS_GCM_C || MBEDTLS_CHACHAPOLY_C */ From b3f4e5b1e19927d85d38ccf5478e316270a52f20 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 13 Dec 2021 12:33:18 +0100 Subject: [PATCH 2/8] PSA hash verification: zeroize expected hash on hash mismatch Signed-off-by: Gilles Peskine --- library/psa_crypto.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index e3db912f7..a89430d4e 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -2249,6 +2249,7 @@ psa_status_t psa_hash_verify( psa_hash_operation_t *operation, status = PSA_ERROR_INVALID_SIGNATURE; exit: + mbedtls_platform_zeroize( actual_hash, sizeof( actual_hash ) ); if( status != PSA_SUCCESS ) psa_hash_abort(operation); @@ -2283,12 +2284,18 @@ psa_status_t psa_hash_compare( psa_algorithm_t alg, actual_hash, sizeof(actual_hash), &actual_hash_length ); if( status != PSA_SUCCESS ) - return( status ); + goto exit; if( actual_hash_length != hash_length ) - return( PSA_ERROR_INVALID_SIGNATURE ); + { + status = PSA_ERROR_INVALID_SIGNATURE; + goto exit; + } if( mbedtls_psa_safer_memcmp( hash, actual_hash, actual_hash_length ) != 0 ) - return( PSA_ERROR_INVALID_SIGNATURE ); - return( PSA_SUCCESS ); + status = PSA_ERROR_INVALID_SIGNATURE; + +exit: + mbedtls_platform_zeroize( actual_hash, sizeof( actual_hash ) ); + return( status ); } psa_status_t psa_hash_clone( const psa_hash_operation_t *source_operation, From 69d3b86baaac0ac9c70189e0c593110c28a01115 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 13 Dec 2021 12:35:08 +0100 Subject: [PATCH 3/8] mbedtls_ssl_cookie_check: zeroize expected cookie on cookie mismatch Signed-off-by: Gilles Peskine --- library/ssl_cookie.c | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/library/ssl_cookie.c b/library/ssl_cookie.c index faf92e7c0..abf29ae71 100644 --- a/library/ssl_cookie.c +++ b/library/ssl_cookie.c @@ -218,15 +218,20 @@ int mbedtls_ssl_cookie_check( void *p_ctx, #if defined(MBEDTLS_THREADING_C) if( mbedtls_mutex_unlock( &ctx->mutex ) != 0 ) - return( MBEDTLS_ERROR_ADD( MBEDTLS_ERR_SSL_INTERNAL_ERROR, - MBEDTLS_ERR_THREADING_MUTEX_ERROR ) ); + { + ret = MBEDTLS_ERROR_ADD( MBEDTLS_ERR_SSL_INTERNAL_ERROR, + MBEDTLS_ERR_THREADING_MUTEX_ERROR ); + } #endif if( ret != 0 ) - return( ret ); + goto exit; if( mbedtls_ct_memcmp( cookie + 4, ref_hmac, sizeof( ref_hmac ) ) != 0 ) - return( -1 ); + { + ret = -1; + goto exit; + } #if defined(MBEDTLS_HAVE_TIME) cur_time = (unsigned long) mbedtls_time( NULL ); @@ -240,8 +245,13 @@ int mbedtls_ssl_cookie_check( void *p_ctx, ( (unsigned long) cookie[3] ); if( ctx->timeout != 0 && cur_time - cookie_time > ctx->timeout ) - return( -1 ); + { + ret = -1; + goto exit; + } - return( 0 ); +exit: + mbedtls_platform_zeroize( ref_hmac, sizeof( ref_hmac ) ); + return( ret ); } #endif /* MBEDTLS_SSL_COOKIE_C */ From f91b2e5a97ca0f60f8a172ad058b48e4703da804 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 13 Dec 2021 12:36:15 +0100 Subject: [PATCH 4/8] mbedtls_ssl_parse_finished: zeroize expected finished value on error Signed-off-by: Gilles Peskine --- library/ssl_tls.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 537adc2a0..ae1a1066e 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -3636,7 +3636,7 @@ int mbedtls_ssl_parse_finished( mbedtls_ssl_context *ssl ) if( ( ret = mbedtls_ssl_read_record( ssl, 1 ) ) != 0 ) { MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_ssl_read_record", ret ); - return( ret ); + goto exit; } if( ssl->in_msgtype != MBEDTLS_SSL_MSG_HANDSHAKE ) @@ -3644,7 +3644,8 @@ int mbedtls_ssl_parse_finished( mbedtls_ssl_context *ssl ) MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad finished message" ) ); mbedtls_ssl_send_alert_message( ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, MBEDTLS_SSL_ALERT_MSG_UNEXPECTED_MESSAGE ); - return( MBEDTLS_ERR_SSL_UNEXPECTED_MESSAGE ); + ret = MBEDTLS_ERR_SSL_UNEXPECTED_MESSAGE; + goto exit; } /* There is currently no ciphersuite using another length with TLS 1.2 */ @@ -3661,7 +3662,8 @@ int mbedtls_ssl_parse_finished( mbedtls_ssl_context *ssl ) MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad finished message" ) ); mbedtls_ssl_send_alert_message( ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR ); - return( MBEDTLS_ERR_SSL_BAD_HS_FINISHED ); + ret = MBEDTLS_ERR_SSL_BAD_HS_FINISHED; + goto exit; } if( mbedtls_ct_memcmp( ssl->in_msg + mbedtls_ssl_hs_hdr_len( ssl ), @@ -3670,7 +3672,8 @@ int mbedtls_ssl_parse_finished( mbedtls_ssl_context *ssl ) MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad finished message" ) ); mbedtls_ssl_send_alert_message( ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, MBEDTLS_SSL_ALERT_MSG_DECRYPT_ERROR ); - return( MBEDTLS_ERR_SSL_BAD_HS_FINISHED ); + ret = MBEDTLS_ERR_SSL_BAD_HS_FINISHED; + goto exit; } #if defined(MBEDTLS_SSL_RENEGOTIATION) @@ -3699,7 +3702,9 @@ int mbedtls_ssl_parse_finished( mbedtls_ssl_context *ssl ) MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= parse finished" ) ); - return( 0 ); +exit: + mbedtls_platform_zeroize( buf, hash_len ); + return( ret ); } static void ssl_handshake_params_init( mbedtls_ssl_handshake_params *handshake ) From 8c99a760d5ee083b1401ee1fcee90c1b4655bf75 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 13 Dec 2021 12:37:55 +0100 Subject: [PATCH 5/8] PKCS#1v1.5 signature: better cleanup of temporary values Zeroize temporary buffers used to sanity-check the signature. If there is an error, overwrite the tentative signature in the output buffer. Signed-off-by: Gilles Peskine --- library/rsa.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/library/rsa.c b/library/rsa.c index a395542c3..8a5d40ff1 100644 --- a/library/rsa.c +++ b/library/rsa.c @@ -1942,9 +1942,13 @@ int mbedtls_rsa_rsassa_pkcs1_v15_sign( mbedtls_rsa_context *ctx, memcpy( sig, sig_try, ctx->len ); cleanup: + mbedtls_platform_zeroize( sig_try, ctx->len ); + mbedtls_platform_zeroize( verif, ctx->len ); mbedtls_free( sig_try ); mbedtls_free( verif ); + if( ret != 0 ) + memset( sig, '!', ctx->len ); return( ret ); } #endif /* MBEDTLS_PKCS1_V15 */ From d61551c0170ea886428f243f299d6a31b6519234 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 13 Dec 2021 12:43:11 +0100 Subject: [PATCH 6/8] Generalize MAC zeroization changelog entry Signed-off-by: Gilles Peskine --- ChangeLog.d/mac-zeroize.txt | 6 ++++++ ChangeLog.d/ssl-mac-zeroize.txt | 5 ----- 2 files changed, 6 insertions(+), 5 deletions(-) create mode 100644 ChangeLog.d/mac-zeroize.txt delete mode 100644 ChangeLog.d/ssl-mac-zeroize.txt diff --git a/ChangeLog.d/mac-zeroize.txt b/ChangeLog.d/mac-zeroize.txt new file mode 100644 index 000000000..a43e34f84 --- /dev/null +++ b/ChangeLog.d/mac-zeroize.txt @@ -0,0 +1,6 @@ +Security + * Zeroize several intermediate variables used to calculate the expected + value when verifying a MAC or AEAD tag. This hardens the library in + case the value leaks through a memory disclosure vulnerability. For + example, a memory disclosure vulnerability could have allowed a + man-in-the-middle to inject fake ciphertext into a DTLS connection. diff --git a/ChangeLog.d/ssl-mac-zeroize.txt b/ChangeLog.d/ssl-mac-zeroize.txt deleted file mode 100644 index b49c7acd7..000000000 --- a/ChangeLog.d/ssl-mac-zeroize.txt +++ /dev/null @@ -1,5 +0,0 @@ -Security - * Zeroize intermediate variables used to calculate the MAC in CBC cipher - suites. This hardens the library in case stack memory leaks through a - memory disclosure vulnerabilty, which could formerly have allowed a - man-in-the-middle to inject fake ciphertext into a DTLS connection. From 622d80453b5f8c057ec5f5bfb8ee6c88762af86c Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 13 Dec 2021 14:38:40 +0100 Subject: [PATCH 7/8] Initialize hash_len before using it Signed-off-by: Gilles Peskine --- library/ssl_tls.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index ae1a1066e..f07b189c5 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -3631,6 +3631,14 @@ int mbedtls_ssl_parse_finished( mbedtls_ssl_context *ssl ) MBEDTLS_SSL_DEBUG_MSG( 2, ( "=> parse finished" ) ); + /* There is currently no ciphersuite using another length with TLS 1.2 */ +#if defined(MBEDTLS_SSL_PROTO_SSL3) + if( ssl->minor_ver == MBEDTLS_SSL_MINOR_VERSION_0 ) + hash_len = 36; + else +#endif + hash_len = 12; + ssl->handshake->calc_finished( ssl, buf, ssl->conf->endpoint ^ 1 ); if( ( ret = mbedtls_ssl_read_record( ssl, 1 ) ) != 0 ) @@ -3648,14 +3656,6 @@ int mbedtls_ssl_parse_finished( mbedtls_ssl_context *ssl ) goto exit; } - /* There is currently no ciphersuite using another length with TLS 1.2 */ -#if defined(MBEDTLS_SSL_PROTO_SSL3) - if( ssl->minor_ver == MBEDTLS_SSL_MINOR_VERSION_0 ) - hash_len = 36; - else -#endif - hash_len = 12; - if( ssl->in_msg[0] != MBEDTLS_SSL_HS_FINISHED || ssl->in_hslen != mbedtls_ssl_hs_hdr_len( ssl ) + hash_len ) { From f9a0501683166e97a61070b76f819ba39be28a67 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 13 Dec 2021 16:57:47 +0100 Subject: [PATCH 8/8] mbedtls_cipher_check_tag: jump on error for more robustness to refactoring Signed-off-by: Gilles Peskine --- library/cipher.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/library/cipher.c b/library/cipher.c index 0c5bcda66..4ec40d2ca 100644 --- a/library/cipher.c +++ b/library/cipher.c @@ -1146,7 +1146,10 @@ int mbedtls_cipher_check_tag( mbedtls_cipher_context_t *ctx, /* Check the tag in "constant-time" */ if( mbedtls_ct_memcmp( tag, check_tag, tag_len ) != 0 ) + { ret = MBEDTLS_ERR_CIPHER_AUTH_FAILED; + goto exit; + } } #endif /* MBEDTLS_GCM_C */ @@ -1166,10 +1169,14 @@ int mbedtls_cipher_check_tag( mbedtls_cipher_context_t *ctx, /* Check the tag in "constant-time" */ if( mbedtls_ct_memcmp( tag, check_tag, tag_len ) != 0 ) + { ret = MBEDTLS_ERR_CIPHER_AUTH_FAILED; + goto exit; + } } #endif /* MBEDTLS_CHACHAPOLY_C */ +exit: mbedtls_platform_zeroize( check_tag, tag_len ); return( ret ); }