From ce516ff449d9e2eeaaf917a85de78ee1ca0efa13 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 9 Nov 2017 18:57:39 +0000 Subject: [PATCH 1/2] Fix heap corruption in ssl_decrypt_buf Previously, MAC validation for an incoming record proceeded as follows: 1) Make a copy of the MAC contained in the record; 2) Compute the expected MAC in place, overwriting the presented one; 3) Compare both. This resulted in a record buffer overflow if truncated MAC was used, as in this case the record buffer only reserved 10 bytes for the MAC, but the MAC computation routine in 2) always wrote a full digest. For specially crafted records, this could be used to perform a controlled write of up to 6 bytes past the boundary of the heap buffer holding the record, thereby corrupting the heap structures and potentially leading to a crash or remote code execution. This commit fixes this by making the following change: 1) Compute the expected MAC in a temporary buffer that has the size of the underlying message digest. 2) Compare to this to the MAC contained in the record, potentially restricting to the first 10 bytes if truncated HMAC is used. A similar fix is applied to the encryption routine `ssl_encrypt_buf`. --- library/ssl_tls.c | 36 +++++++++++++++++------------------- tests/ssl-opt.sh | 20 ++++++++++---------- 2 files changed, 27 insertions(+), 29 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 8088e881ce..1b8d7df585 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1284,14 +1284,17 @@ static int ssl_encrypt_buf( mbedtls_ssl_context *ssl ) defined(MBEDTLS_SSL_PROTO_TLS1_2) if( ssl->minor_ver >= MBEDTLS_SSL_MINOR_VERSION_1 ) { + unsigned char mac[MBEDTLS_SSL_MAC_ADD]; + mbedtls_md_hmac_update( &ssl->transform_out->md_ctx_enc, ssl->out_ctr, 8 ); mbedtls_md_hmac_update( &ssl->transform_out->md_ctx_enc, ssl->out_hdr, 3 ); mbedtls_md_hmac_update( &ssl->transform_out->md_ctx_enc, ssl->out_len, 2 ); mbedtls_md_hmac_update( &ssl->transform_out->md_ctx_enc, ssl->out_msg, ssl->out_msglen ); - mbedtls_md_hmac_finish( &ssl->transform_out->md_ctx_enc, - ssl->out_msg + ssl->out_msglen ); + mbedtls_md_hmac_finish( &ssl->transform_out->md_ctx_enc, mac ); mbedtls_md_hmac_reset( &ssl->transform_out->md_ctx_enc ); + + memcpy( ssl->out_msg + ssl->out_msglen, mac, ssl->transform_out->maclen ); } else #endif @@ -1553,8 +1556,6 @@ static int ssl_encrypt_buf( mbedtls_ssl_context *ssl ) return( 0 ); } -#define SSL_MAX_MAC_SIZE 48 - static int ssl_decrypt_buf( mbedtls_ssl_context *ssl ) { size_t i; @@ -1722,7 +1723,7 @@ static int ssl_decrypt_buf( mbedtls_ssl_context *ssl ) #if defined(MBEDTLS_SSL_ENCRYPT_THEN_MAC) if( ssl->session_in->encrypt_then_mac == MBEDTLS_SSL_ETM_ENABLED ) { - unsigned char computed_mac[SSL_MAX_MAC_SIZE]; + unsigned char mac_expect[MBEDTLS_SSL_MAC_ADD]; unsigned char pseudo_hdr[13]; MBEDTLS_SSL_DEBUG_MSG( 3, ( "using encrypt then mac" ) ); @@ -1740,16 +1741,16 @@ static int ssl_decrypt_buf( mbedtls_ssl_context *ssl ) mbedtls_md_hmac_update( &ssl->transform_in->md_ctx_dec, pseudo_hdr, 13 ); mbedtls_md_hmac_update( &ssl->transform_in->md_ctx_dec, ssl->in_iv, ssl->in_msglen ); - mbedtls_md_hmac_finish( &ssl->transform_in->md_ctx_dec, computed_mac ); + mbedtls_md_hmac_finish( &ssl->transform_in->md_ctx_dec, mac_expect ); mbedtls_md_hmac_reset( &ssl->transform_in->md_ctx_dec ); MBEDTLS_SSL_DEBUG_BUF( 4, "message mac", ssl->in_iv + ssl->in_msglen, ssl->transform_in->maclen ); - MBEDTLS_SSL_DEBUG_BUF( 4, "computed mac", computed_mac, + MBEDTLS_SSL_DEBUG_BUF( 4, "expected mac", mac_expect, ssl->transform_in->maclen ); - if( mbedtls_ssl_safer_memcmp( ssl->in_iv + ssl->in_msglen, computed_mac, - ssl->transform_in->maclen ) != 0 ) + if( mbedtls_ssl_safer_memcmp( ssl->in_iv + ssl->in_msglen, mac_expect, + ssl->transform_in->maclen ) != 0 ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "message mac does not match" ) ); @@ -1909,15 +1910,13 @@ static int ssl_decrypt_buf( mbedtls_ssl_context *ssl ) #if defined(SSL_SOME_MODES_USE_MAC) if( auth_done == 0 ) { - unsigned char tmp[SSL_MAX_MAC_SIZE]; + unsigned char mac_expect[MBEDTLS_SSL_MAC_ADD]; ssl->in_msglen -= ssl->transform_in->maclen; ssl->in_len[0] = (unsigned char)( ssl->in_msglen >> 8 ); ssl->in_len[1] = (unsigned char)( ssl->in_msglen ); - memcpy( tmp, ssl->in_msg + ssl->in_msglen, ssl->transform_in->maclen ); - #if defined(MBEDTLS_SSL_PROTO_SSL3) if( ssl->minor_ver == MBEDTLS_SSL_MINOR_VERSION_0 ) { @@ -1956,8 +1955,7 @@ static int ssl_decrypt_buf( mbedtls_ssl_context *ssl ) 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 ); - mbedtls_md_hmac_finish( &ssl->transform_in->md_ctx_dec, - ssl->in_msg + ssl->in_msglen ); + mbedtls_md_hmac_finish( &ssl->transform_in->md_ctx_dec, mac_expect ); /* Call mbedtls_md_process at least once due to cache attacks */ for( j = 0; j < extra_run + 1; j++ ) mbedtls_md_process( &ssl->transform_in->md_ctx_dec, ssl->in_msg ); @@ -1972,12 +1970,12 @@ static int ssl_decrypt_buf( mbedtls_ssl_context *ssl ) return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); } - MBEDTLS_SSL_DEBUG_BUF( 4, "message mac", tmp, ssl->transform_in->maclen ); - MBEDTLS_SSL_DEBUG_BUF( 4, "computed mac", ssl->in_msg + ssl->in_msglen, - ssl->transform_in->maclen ); + 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 ); - if( mbedtls_ssl_safer_memcmp( tmp, ssl->in_msg + ssl->in_msglen, - ssl->transform_in->maclen ) != 0 ) + if( mbedtls_ssl_safer_memcmp( ssl->in_msg + ssl->in_msglen, mac_expect, + ssl->transform_in->maclen ) != 0 ) { #if defined(MBEDTLS_SSL_DEBUG_ALL) MBEDTLS_SSL_DEBUG_MSG( 1, ( "message mac does not match" ) ); diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index d8f0fd720b..5a1ef5bfb7 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -699,40 +699,40 @@ run_test "Truncated HMAC: client default, server default" \ "$P_SRV debug_level=4" \ "$P_CLI force_ciphersuite=TLS-RSA-WITH-AES-128-CBC-SHA" \ 0 \ - -s "dumping 'computed mac' (20 bytes)" \ - -S "dumping 'computed mac' (10 bytes)" + -s "dumping 'expected mac' (20 bytes)" \ + -S "dumping 'expected mac' (10 bytes)" run_test "Truncated HMAC: client disabled, server default" \ "$P_SRV debug_level=4" \ "$P_CLI force_ciphersuite=TLS-RSA-WITH-AES-128-CBC-SHA \ trunc_hmac=0" \ 0 \ - -s "dumping 'computed mac' (20 bytes)" \ - -S "dumping 'computed mac' (10 bytes)" + -s "dumping 'expected mac' (20 bytes)" \ + -S "dumping 'expected mac' (10 bytes)" run_test "Truncated HMAC: client enabled, server default" \ "$P_SRV debug_level=4" \ "$P_CLI force_ciphersuite=TLS-RSA-WITH-AES-128-CBC-SHA \ trunc_hmac=1" \ 0 \ - -s "dumping 'computed mac' (20 bytes)" \ - -S "dumping 'computed mac' (10 bytes)" + -s "dumping 'expected mac' (20 bytes)" \ + -S "dumping 'expected mac' (10 bytes)" run_test "Truncated HMAC: client enabled, server disabled" \ "$P_SRV debug_level=4 trunc_hmac=0" \ "$P_CLI force_ciphersuite=TLS-RSA-WITH-AES-128-CBC-SHA \ trunc_hmac=1" \ 0 \ - -s "dumping 'computed mac' (20 bytes)" \ - -S "dumping 'computed mac' (10 bytes)" + -s "dumping 'expected mac' (20 bytes)" \ + -S "dumping 'expected mac' (10 bytes)" run_test "Truncated HMAC: client enabled, server enabled" \ "$P_SRV debug_level=4 trunc_hmac=1" \ "$P_CLI force_ciphersuite=TLS-RSA-WITH-AES-128-CBC-SHA \ trunc_hmac=1" \ 0 \ - -S "dumping 'computed mac' (20 bytes)" \ - -s "dumping 'computed mac' (10 bytes)" + -S "dumping 'expected mac' (20 bytes)" \ + -s "dumping 'expected mac' (10 bytes)" # Tests for Encrypt-then-MAC extension From b09c5721f57d6988ec4a1dadeac7e6cc6ca65b9c Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Mon, 20 Nov 2017 10:43:35 +0000 Subject: [PATCH 2/2] Adapt ChangeLog --- ChangeLog | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/ChangeLog b/ChangeLog index 1b01eb6821..e5ba2139b9 100644 --- a/ChangeLog +++ b/ChangeLog @@ -3,6 +3,14 @@ mbed TLS ChangeLog (Sorted per branch, date) = mbed TLS 2.1.10 branch released 2017-xx-xx +Security + * Fix heap corruption in implementation of truncated HMAC extension. + When the truncated HMAC extension is enabled and CBC is used, + sending a malicious application packet can be used to selectively + corrupt 6 bytes on the peer's heap, potentially leading to crash or + remote code execution. This can be triggered remotely from either + side in both TLS and DTLS. + Bugfix * Fix ssl_parse_record_header() to silently discard invalid DTLS records as recommended in RFC 6347 Section 4.1.2.7.