From 251bab5ceb0b54303e899e1622dd02e1fb2a418c Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Mon, 20 Nov 2017 10:30:08 +0000 Subject: [PATCH 1/4] 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 | 32 ++++++++++++++++---------------- tests/ssl-opt.sh | 20 ++++++++++---------- 2 files changed, 26 insertions(+), 26 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 54867da97f..63047aee20 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1141,12 +1141,16 @@ static int ssl_encrypt_buf( ssl_context *ssl ) defined(POLARSSL_SSL_PROTO_TLS1_2) if( ssl->minor_ver >= SSL_MINOR_VERSION_1 ) { + unsigned char mac[SSL_MAC_ADD]; + md_hmac_update( &ssl->transform_out->md_ctx_enc, ssl->out_ctr, 13 ); md_hmac_update( &ssl->transform_out->md_ctx_enc, ssl->out_msg, ssl->out_msglen ); - md_hmac_finish( &ssl->transform_out->md_ctx_enc, - ssl->out_msg + ssl->out_msglen ); + md_hmac_finish( &ssl->transform_out->md_ctx_enc, mac ); md_hmac_reset( &ssl->transform_out->md_ctx_enc ); + + memcpy( ssl->out_msg + ssl->out_msglen, mac, + ssl->transform_out->maclen ); } else #endif @@ -1419,8 +1423,6 @@ static int ssl_encrypt_buf( ssl_context *ssl ) return( 0 ); } -#define POLARSSL_SSL_MAX_MAC_SIZE 48 - static int ssl_decrypt_buf( ssl_context *ssl ) { size_t i; @@ -1588,7 +1590,7 @@ static int ssl_decrypt_buf( ssl_context *ssl ) #if defined(POLARSSL_SSL_ENCRYPT_THEN_MAC) if( ssl->session_in->encrypt_then_mac == SSL_ETM_ENABLED ) { - unsigned char computed_mac[POLARSSL_SSL_MAX_MAC_SIZE]; + unsigned char mac_expect[SSL_MAC_ADD]; unsigned char pseudo_hdr[13]; SSL_DEBUG_MSG( 3, ( "using encrypt then mac" ) ); @@ -1606,15 +1608,15 @@ static int ssl_decrypt_buf( ssl_context *ssl ) md_hmac_update( &ssl->transform_in->md_ctx_dec, pseudo_hdr, 13 ); md_hmac_update( &ssl->transform_in->md_ctx_dec, ssl->in_iv, ssl->in_msglen ); - md_hmac_finish( &ssl->transform_in->md_ctx_dec, computed_mac ); + md_hmac_finish( &ssl->transform_in->md_ctx_dec, mac_expect ); md_hmac_reset( &ssl->transform_in->md_ctx_dec ); SSL_DEBUG_BUF( 4, "message mac", ssl->in_iv + ssl->in_msglen, ssl->transform_in->maclen ); - SSL_DEBUG_BUF( 4, "computed mac", computed_mac, + SSL_DEBUG_BUF( 4, "computed mac", mac_expect, ssl->transform_in->maclen ); - if( safer_memcmp( ssl->in_iv + ssl->in_msglen, computed_mac, + if( safer_memcmp( ssl->in_iv + ssl->in_msglen, mac_expect, ssl->transform_in->maclen ) != 0 ) { SSL_DEBUG_MSG( 1, ( "message mac does not match" ) ); @@ -1775,15 +1777,13 @@ static int ssl_decrypt_buf( ssl_context *ssl ) #if defined(POLARSSL_SOME_MODES_USE_MAC) if( auth_done == 0 ) { - unsigned char tmp[POLARSSL_SSL_MAX_MAC_SIZE]; + unsigned char mac_expect[SSL_MAC_ADD]; ssl->in_msglen -= ssl->transform_in->maclen; ssl->in_hdr[3] = (unsigned char)( ssl->in_msglen >> 8 ); ssl->in_hdr[4] = (unsigned char)( ssl->in_msglen ); - memcpy( tmp, ssl->in_msg + ssl->in_msglen, ssl->transform_in->maclen ); - #if defined(POLARSSL_SSL_PROTO_SSL3) if( ssl->minor_ver == SSL_MINOR_VERSION_0 ) { @@ -1820,8 +1820,8 @@ static int ssl_decrypt_buf( ssl_context *ssl ) md_hmac_update( &ssl->transform_in->md_ctx_dec, ssl->in_ctr, 13 ); md_hmac_update( &ssl->transform_in->md_ctx_dec, ssl->in_msg, ssl->in_msglen ); - md_hmac_finish( &ssl->transform_in->md_ctx_dec, - ssl->in_msg + ssl->in_msglen ); + md_hmac_finish( &ssl->transform_in->md_ctx_dec, mac_expect ); + /* Call md_process at least once due to cache attacks */ for( j = 0; j < extra_run + 1; j++ ) md_process( &ssl->transform_in->md_ctx_dec, ssl->in_msg ); @@ -1836,11 +1836,11 @@ static int ssl_decrypt_buf( ssl_context *ssl ) return( POLARSSL_ERR_SSL_INTERNAL_ERROR ); } - SSL_DEBUG_BUF( 4, "message mac", tmp, ssl->transform_in->maclen ); - SSL_DEBUG_BUF( 4, "computed mac", ssl->in_msg + ssl->in_msglen, + SSL_DEBUG_BUF( 4, "expected mac", mac_expect, ssl->transform_in->maclen ); + SSL_DEBUG_BUF( 4, "message mac", ssl->in_msg + ssl->in_msglen, ssl->transform_in->maclen ); - if( safer_memcmp( tmp, ssl->in_msg + ssl->in_msglen, + if( safer_memcmp( ssl->in_msg + ssl->in_msglen, mac_expect, ssl->transform_in->maclen ) != 0 ) { #if defined(POLARSSL_SSL_DEBUG_ALL) diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index 0f976682b2..53bf2acdd6 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -504,40 +504,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 4d48bb6ca3513914733ac09e53e09b86c7eff590 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Mon, 20 Nov 2017 10:31:05 +0000 Subject: [PATCH 2/4] Adapt ChangeLog --- ChangeLog | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/ChangeLog b/ChangeLog index 6a1be9892a..f77278b0d2 100644 --- a/ChangeLog +++ b/ChangeLog @@ -2,6 +2,14 @@ mbed TLS ChangeLog (Sorted per branch, date) = mbed TLS 1.3.22 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. + Bugfix * Fix memory leak in ssl_set_hostname() when called multiple times. Found by projectgus and jethrogb, #836. From 0a139f9a03c80b9533f7a2e5566afdbf191a611e Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 21 Nov 2017 17:41:59 +0000 Subject: [PATCH 3/4] Modify debug output Tests from ssl-opt.sh now expect 'expected mac XXX' and no longer 'computed mac XXX'. --- 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 63047aee20..5877e58df6 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1159,7 +1159,7 @@ static int ssl_encrypt_buf( ssl_context *ssl ) return( POLARSSL_ERR_SSL_INTERNAL_ERROR ); } - SSL_DEBUG_BUF( 4, "computed mac", + SSL_DEBUG_BUF( 4, "expected mac", ssl->out_msg + ssl->out_msglen, ssl->transform_out->maclen ); @@ -1613,7 +1613,7 @@ static int ssl_decrypt_buf( ssl_context *ssl ) SSL_DEBUG_BUF( 4, "message mac", ssl->in_iv + ssl->in_msglen, ssl->transform_in->maclen ); - SSL_DEBUG_BUF( 4, "computed mac", mac_expect, + SSL_DEBUG_BUF( 4, "expected mac", mac_expect, ssl->transform_in->maclen ); if( safer_memcmp( ssl->in_iv + ssl->in_msglen, mac_expect, From ad951d131d2ca54b9e7f1dc6f3bd957444eaef28 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 29 Nov 2017 17:51:03 +0000 Subject: [PATCH 4/4] Correct dangerous typo in include/polarssl/ssl.h The definition of SSL_MAC_ADD depends on the presence of the configuration option POLARSSL_ARC4_C, which was misspelled as POLARSSL_RC4_C in ssl.h, leading to a too small buffer and subsequently to a buffer overflow during record processing. This commit fixes the typo. --- ChangeLog | 2 ++ include/polarssl/ssl.h | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/ChangeLog b/ChangeLog index f77278b0d2..67777d4e58 100644 --- a/ChangeLog +++ b/ChangeLog @@ -11,6 +11,8 @@ Security side. Bugfix + * Fix typo in ssl.h leading to a too small value of SSL_MAC_ADD + in case CBC is disabled but ARC4 is enabled. * Fix memory leak in ssl_set_hostname() when called multiple times. Found by projectgus and jethrogb, #836. * Fix usage help in ssl_server2 example. Found and fixed by Bei Lin. diff --git a/include/polarssl/ssl.h b/include/polarssl/ssl.h index 9a3fb8a4bf..32c07c2db7 100644 --- a/include/polarssl/ssl.h +++ b/include/polarssl/ssl.h @@ -303,7 +303,7 @@ #define SSL_COMPRESSION_ADD 0 #endif -#if defined(POLARSSL_RC4_C) || defined(POLARSSL_CIPHER_MODE_CBC) +#if defined(POLARSSL_ARC4_C) || defined(POLARSSL_CIPHER_MODE_CBC) /* Ciphersuites using HMAC */ #if defined(POLARSSL_SHA512_C) #define SSL_MAC_ADD 48 /* SHA-384 used for HMAC */