From e7835d92c1dc2391df199e77d7d095d235e6b95e 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 0d9d71068d..70f2d006d6 100644 --- a/library/cipher.c +++ b/library/cipher.c @@ -1175,6 +1175,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 ) { @@ -1195,9 +1201,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 */ @@ -1217,13 +1221,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 60aebec47e02d03d720d568a557b02e57bed6219 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 9f5b9461af..f257651d7b 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -2210,6 +2210,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); @@ -2244,12 +2245,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 c2f7b75a71e6d8da7c2204e2a053acf1d095521a 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 7516786def..358169e876 100644 --- a/library/ssl_cookie.c +++ b/library/ssl_cookie.c @@ -217,15 +217,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 ); @@ -239,8 +244,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 f0fd4c3aee3622cff5911f7fbf546e8d2a89b810 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 | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index a974dbb2e8..4a1191abee 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -2884,7 +2884,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 ) @@ -2892,7 +2892,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; } hash_len = 12; @@ -2901,7 +2902,8 @@ int mbedtls_ssl_parse_finished( mbedtls_ssl_context *ssl ) { 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; } if( ssl->in_hslen != mbedtls_ssl_hs_hdr_len( ssl ) + hash_len ) @@ -2909,7 +2911,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_DECODE_ERROR ); + ret = MBEDTLS_ERR_SSL_DECODE_ERROR; + goto exit; } if( mbedtls_ct_memcmp( ssl->in_msg + mbedtls_ssl_hs_hdr_len( ssl ), @@ -2918,7 +2921,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_HANDSHAKE_FAILURE ); + ret = MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE; + goto exit; } #if defined(MBEDTLS_SSL_RENEGOTIATION) @@ -2947,7 +2951,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 14d5fef6b7cc8da650c5d211be9fe7fb42010022 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 e3ec0568e7..36f487f3a7 100644 --- a/library/rsa.c +++ b/library/rsa.c @@ -1896,9 +1896,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 36d33f37b65f4843e089850a15038652ca694fa9 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 0000000000..a43e34f845 --- /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 b49c7acd77..0000000000 --- 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 a4174312da06da490df076845577a076783d28f5 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 | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 4a1191abee..d868e49650 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -2874,7 +2874,7 @@ int mbedtls_ssl_write_finished( mbedtls_ssl_context *ssl ) int mbedtls_ssl_parse_finished( mbedtls_ssl_context *ssl ) { int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; - unsigned int hash_len; + unsigned int hash_len = 12; unsigned char buf[SSL_MAX_HASH_LEN]; MBEDTLS_SSL_DEBUG_MSG( 2, ( "=> parse finished" ) ); @@ -2896,8 +2896,6 @@ int mbedtls_ssl_parse_finished( mbedtls_ssl_context *ssl ) goto exit; } - hash_len = 12; - if( ssl->in_msg[0] != MBEDTLS_SSL_HS_FINISHED ) { mbedtls_ssl_send_alert_message( ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, From cd74298c83446e026b55acce557c7d4eca8a4fa1 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 70f2d006d6..03e84c6c85 100644 --- a/library/cipher.c +++ b/library/cipher.c @@ -1201,7 +1201,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 */ @@ -1221,10 +1224,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 ); }