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. diff --git a/library/cipher.c b/library/cipher.c index d51ccd77ff..4ec40d2cac 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,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 ) - return( MBEDTLS_ERR_CIPHER_AUTH_FAILED ); - - return( 0 ); + { + ret = MBEDTLS_ERR_CIPHER_AUTH_FAILED; + goto exit; + } } #endif /* MBEDTLS_GCM_C */ @@ -1162,13 +1169,16 @@ 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; + goto exit; + } } #endif /* MBEDTLS_CHACHAPOLY_C */ - return( 0 ); +exit: + mbedtls_platform_zeroize( check_tag, tag_len ); + return( ret ); } #endif /* MBEDTLS_GCM_C || MBEDTLS_CHACHAPOLY_C */ diff --git a/library/psa_crypto.c b/library/psa_crypto.c index e3db912f72..a89430d4ea 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, diff --git a/library/rsa.c b/library/rsa.c index a395542c3a..8a5d40ff1e 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 */ diff --git a/library/ssl_cookie.c b/library/ssl_cookie.c index faf92e7c0f..abf29ae717 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 */ diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 537adc2a0c..f07b189c59 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -3631,22 +3631,6 @@ int mbedtls_ssl_parse_finished( mbedtls_ssl_context *ssl ) MBEDTLS_SSL_DEBUG_MSG( 2, ( "=> parse finished" ) ); - ssl->handshake->calc_finished( ssl, buf, ssl->conf->endpoint ^ 1 ); - - if( ( ret = mbedtls_ssl_read_record( ssl, 1 ) ) != 0 ) - { - MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_ssl_read_record", ret ); - return( ret ); - } - - if( ssl->in_msgtype != MBEDTLS_SSL_MSG_HANDSHAKE ) - { - 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 ); - } - /* 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 ) @@ -3655,13 +3639,31 @@ int mbedtls_ssl_parse_finished( mbedtls_ssl_context *ssl ) #endif hash_len = 12; + ssl->handshake->calc_finished( ssl, buf, ssl->conf->endpoint ^ 1 ); + + if( ( ret = mbedtls_ssl_read_record( ssl, 1 ) ) != 0 ) + { + MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_ssl_read_record", ret ); + goto exit; + } + + if( ssl->in_msgtype != MBEDTLS_SSL_MSG_HANDSHAKE ) + { + 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 ); + ret = MBEDTLS_ERR_SSL_UNEXPECTED_MESSAGE; + goto exit; + } + if( ssl->in_msg[0] != MBEDTLS_SSL_HS_FINISHED || ssl->in_hslen != mbedtls_ssl_hs_hdr_len( ssl ) + hash_len ) { 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 )