From 3d699e43eae24a156271391a5a3c376e418b95d1 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 30 Jan 2019 14:46:35 +0000 Subject: [PATCH 1/7] SSL/TLS client: Remove old session ticket on renegotiation Context: During a handshake, the SSL/TLS handshake logic constructs an instance of ::mbedtls_ssl_session representing the SSL session being established. This structure contains information such as the session's master secret, the peer certificate, or the session ticket issues by the server (if applicable). During a renegotiation, the new session is constructed aside the existing one and destroys and replaces the latter only when the renegotiation is complete. While conceptually clear, this means that during the renegotiation, large pieces of information such as the peer's CRT or the session ticket exist twice in memory, even though the original versions are removed eventually. This commit starts removing this memory inefficiency by freeing the old session's SessionTicket before the one for the new session is allocated. --- library/ssl_cli.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/library/ssl_cli.c b/library/ssl_cli.c index be80de71d..c6e64a4b6 100644 --- a/library/ssl_cli.c +++ b/library/ssl_cli.c @@ -3583,6 +3583,15 @@ static int ssl_parse_new_session_ticket( mbedtls_ssl_context *ssl ) if( ticket_len == 0 ) return( 0 ); + if( ssl->session != NULL && ssl->session->ticket != NULL ) + { + mbedtls_platform_zeroize( ssl->session->ticket, + ssl->session->ticket_len ); + mbedtls_free( ssl->session->ticket ); + ssl->session->ticket = NULL; + ssl->session->ticket_len = 0; + } + mbedtls_platform_zeroize( ssl->session_negotiate->ticket, ssl->session_negotiate->ticket_len ); mbedtls_free( ssl->session_negotiate->ticket ); From 33c3dc859119aa72cb8981693e1683ef9da8599e Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 30 Jan 2019 14:46:46 +0000 Subject: [PATCH 2/7] Don't store the peer CRT chain twice during renegotiation Context: During a handshake, the SSL/TLS handshake logic constructs an instance of ::mbedtls_ssl_session representing the SSL session being established. This structure contains information such as the session's master secret, the peer certificate, or the session ticket issues by the server (if applicable). During a renegotiation, the new session is constructed aside the existing one and destroys and replaces the latter only when the renegotiation is complete. While conceptually clear, this means that during the renegotiation, large pieces of information such as the peer's CRT or the session ticket exist twice in memory, even though the original versions are removed eventually. This commit removes the simultaneous presence of two peer CRT chains in memory during renegotiation, in the following way: - Unlike in the case of SessionTickets handled in the previous commit, we cannot simply free the peer's CRT chain from the previous handshake before parsing the new one, as we need to verify that the peer's end-CRT hasn't changed to mitigate the 'Triple Handshake Attack'. - Instead, we perform a binary comparison of the original peer end-CRT with the one presented during renegotiation, and if it succeeds, we avoid re-parsing CRT by moving the corresponding CRT pointer from the old to the new session structure. - The remaining CRTs in the peer's chain are not affected by the triple handshake attack protection, and for them we may employ the canonical approach of freeing them before parsing the remainder of the new chain. Note that this commit intends to not change any observable behavior of the stack. In particular: - The peer's CRT chain is still verified during renegotiation. - The tail of the peer's CRT chain may change during renegotiation. --- library/ssl_tls.c | 160 +++++++++++++++++++++++++++------------------- 1 file changed, 96 insertions(+), 64 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index b61453fe5..005362337 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -6120,6 +6120,21 @@ write_msg: return( ret ); } +static int ssl_check_peer_crt_unchanged( mbedtls_ssl_context *ssl, + unsigned char *crt_buf, + size_t crt_buf_len ) +{ + mbedtls_x509_crt const * const peer_crt = ssl->session->peer_cert; + + if( peer_crt == NULL ) + return( -1 ); + + if( peer_crt->raw.len != crt_buf_len ) + return( -1 ); + + return( memcmp( peer_crt->raw.p, crt_buf, crt_buf_len) ); +} + /* * Once the certificate message is read, parse it into a cert chain and * perform basic checks, but leave actual verification to the caller @@ -6210,35 +6225,29 @@ static int ssl_parse_certificate_chain( mbedtls_ssl_context *ssl ) return( MBEDTLS_ERR_SSL_BAD_HS_CERTIFICATE ); } + /* Make &ssl->in_msg[i] point to the beginning of the CRT chain. */ + i += 3; + /* In case we tried to reuse a session but it failed */ if( ssl->session_negotiate->peer_cert != NULL ) { mbedtls_x509_crt_free( ssl->session_negotiate->peer_cert ); mbedtls_free( ssl->session_negotiate->peer_cert ); + ssl->session_negotiate->peer_cert = NULL; } - if( ( ssl->session_negotiate->peer_cert = mbedtls_calloc( 1, - sizeof( mbedtls_x509_crt ) ) ) == NULL ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "alloc(%d bytes) failed", - sizeof( mbedtls_x509_crt ) ) ); - mbedtls_ssl_send_alert_message( ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, - MBEDTLS_SSL_ALERT_MSG_INTERNAL_ERROR ); - return( MBEDTLS_ERR_SSL_ALLOC_FAILED ); - } - - mbedtls_x509_crt_init( ssl->session_negotiate->peer_cert ); - - i += 3; - + /* Iterate through and parse the CRTs in the provided chain. */ while( i < ssl->in_hslen ) { + /* Check that there's room for the next CRT's length fields. */ if ( i + 3 > ssl->in_hslen ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad certificate message" ) ); mbedtls_ssl_send_alert_message( ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, - MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR ); + MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR ); return( MBEDTLS_ERR_SSL_BAD_HS_CERTIFICATE ); } + /* In theory, the CRT can be up to 2**24 Bytes, but we don't support + * anything beyond 2**16 ~ 64K. */ if( ssl->in_msg[i] != 0 ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad certificate message" ) ); @@ -6247,6 +6256,7 @@ static int ssl_parse_certificate_chain( mbedtls_ssl_context *ssl ) return( MBEDTLS_ERR_SSL_BAD_HS_CERTIFICATE ); } + /* Read length of the next CRT in the chain. */ n = ( (unsigned int) ssl->in_msg[i + 1] << 8 ) | (unsigned int) ssl->in_msg[i + 2]; i += 3; @@ -6259,67 +6269,89 @@ static int ssl_parse_certificate_chain( mbedtls_ssl_context *ssl ) return( MBEDTLS_ERR_SSL_BAD_HS_CERTIFICATE ); } + /* Check if we're handling the first CRT in the chain. */ + if( ssl->session_negotiate->peer_cert == NULL ) + { + /* During client-side renegotiation, check the server's end-CRTs + * hasn't changed compared to the initial handshake, mitigating + * the triple handshake attack. On success, reuse the original + * end-CRT instead of parsing it again. */ +#if defined(MBEDTLS_SSL_RENEGOTIATION) && defined(MBEDTLS_SSL_CLI_C) + if( ssl->conf->endpoint == MBEDTLS_SSL_IS_CLIENT && + ssl->renego_status == MBEDTLS_SSL_RENEGOTIATION_IN_PROGRESS ) + { + MBEDTLS_SSL_DEBUG_MSG( 3, ( "Check that peer CRT hasn't changed during renegotiation" ) ); + if( ssl_check_peer_crt_unchanged( ssl, &ssl->in_msg[i], n ) != 0 ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "new server cert during renegotiation" ) ); + mbedtls_ssl_send_alert_message( ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, + MBEDTLS_SSL_ALERT_MSG_ACCESS_DENIED ); + return( MBEDTLS_ERR_SSL_BAD_HS_CERTIFICATE ); + } + + /* Move CRT chain structure to new session instance. */ + ssl->session_negotiate->peer_cert = ssl->session->peer_cert; + ssl->session->peer_cert = NULL; + + /* Delete all remaining CRTs from the original CRT chain. */ + mbedtls_x509_crt_free( ssl->session_negotiate->peer_cert->next ); + ssl->session_negotiate->peer_cert->next = NULL; + + i += n; + continue; + } +#endif /* MBEDTLS_SSL_RENEGOTIATION && MBEDTLS_SSL_CLI_C */ + + /* Outside of client-side renegotiation, create a fresh X.509 CRT + * instance to parse the end-CRT into. */ + + ssl->session_negotiate->peer_cert = + mbedtls_calloc( 1, sizeof( mbedtls_x509_crt ) ); + if( ssl->session_negotiate->peer_cert == NULL ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "alloc(%d bytes) failed", + sizeof( mbedtls_x509_crt ) ) ); + mbedtls_ssl_send_alert_message( ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, + MBEDTLS_SSL_ALERT_MSG_INTERNAL_ERROR ); + return( MBEDTLS_ERR_SSL_ALLOC_FAILED ); + } + + mbedtls_x509_crt_init( ssl->session_negotiate->peer_cert ); + + /* Intentional fall through */ + } + + /* Parse the next certificate in the chain. */ ret = mbedtls_x509_crt_parse_der( ssl->session_negotiate->peer_cert, - ssl->in_msg + i, n ); + ssl->in_msg + i, n ); switch( ret ) { - case 0: /*ok*/ - case MBEDTLS_ERR_X509_UNKNOWN_SIG_ALG + MBEDTLS_ERR_OID_NOT_FOUND: - /* Ignore certificate with an unknown algorithm: maybe a - prior certificate was already trusted. */ - break; + case 0: /*ok*/ + case MBEDTLS_ERR_X509_UNKNOWN_SIG_ALG + MBEDTLS_ERR_OID_NOT_FOUND: + /* Ignore certificate with an unknown algorithm: maybe a + prior certificate was already trusted. */ + break; - case MBEDTLS_ERR_X509_ALLOC_FAILED: - alert = MBEDTLS_SSL_ALERT_MSG_INTERNAL_ERROR; - goto crt_parse_der_failed; + case MBEDTLS_ERR_X509_ALLOC_FAILED: + alert = MBEDTLS_SSL_ALERT_MSG_INTERNAL_ERROR; + goto crt_parse_der_failed; - case MBEDTLS_ERR_X509_UNKNOWN_VERSION: - alert = MBEDTLS_SSL_ALERT_MSG_UNSUPPORTED_CERT; - goto crt_parse_der_failed; + case MBEDTLS_ERR_X509_UNKNOWN_VERSION: + alert = MBEDTLS_SSL_ALERT_MSG_UNSUPPORTED_CERT; + goto crt_parse_der_failed; - default: - alert = MBEDTLS_SSL_ALERT_MSG_BAD_CERT; - crt_parse_der_failed: - mbedtls_ssl_send_alert_message( ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, alert ); - MBEDTLS_SSL_DEBUG_RET( 1, " mbedtls_x509_crt_parse_der", ret ); - return( ret ); + default: + alert = MBEDTLS_SSL_ALERT_MSG_BAD_CERT; + crt_parse_der_failed: + mbedtls_ssl_send_alert_message( ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, alert ); + MBEDTLS_SSL_DEBUG_RET( 1, " mbedtls_x509_crt_parse_der", ret ); + return( ret ); } i += n; } MBEDTLS_SSL_DEBUG_CRT( 3, "peer certificate", ssl->session_negotiate->peer_cert ); - - /* - * On client, make sure the server cert doesn't change during renego to - * avoid "triple handshake" attack: https://secure-resumption.com/ - */ -#if defined(MBEDTLS_SSL_RENEGOTIATION) && defined(MBEDTLS_SSL_CLI_C) - if( ssl->conf->endpoint == MBEDTLS_SSL_IS_CLIENT && - ssl->renego_status == MBEDTLS_SSL_RENEGOTIATION_IN_PROGRESS ) - { - if( ssl->session->peer_cert == NULL ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "new server cert during renegotiation" ) ); - mbedtls_ssl_send_alert_message( ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, - MBEDTLS_SSL_ALERT_MSG_ACCESS_DENIED ); - return( MBEDTLS_ERR_SSL_BAD_HS_CERTIFICATE ); - } - - if( ssl->session->peer_cert->raw.len != - ssl->session_negotiate->peer_cert->raw.len || - memcmp( ssl->session->peer_cert->raw.p, - ssl->session_negotiate->peer_cert->raw.p, - ssl->session->peer_cert->raw.len ) != 0 ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "server cert changed during renegotiation" ) ); - mbedtls_ssl_send_alert_message( ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, - MBEDTLS_SSL_ALERT_MSG_ACCESS_DENIED ); - return( MBEDTLS_ERR_SSL_BAD_HS_CERTIFICATE ); - } - } -#endif /* MBEDTLS_SSL_RENEGOTIATION && MBEDTLS_SSL_CLI_C */ - return( 0 ); } From 1f2e466843170e5aadf5378256ce427f54d2fee9 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 30 Jan 2019 15:14:21 +0000 Subject: [PATCH 3/7] Adapt ChangeLog --- ChangeLog | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ChangeLog b/ChangeLog index 1b60a00eb..b78b663fe 100644 --- a/ChangeLog +++ b/ChangeLog @@ -111,6 +111,8 @@ Bugfix leading content octet. Fixes #1610. Changes + * Reduce RAM consumption during session renegotiation by not storing + the peer CRT chain and session ticket twice. * Include configuration file in all header files that use configuration, instead of relying on other header files that they include. Inserted as an enhancement for #1371 From 285ff0c3627b2655990a6ab1f063d058d523c84f Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 31 Jan 2019 07:44:03 +0000 Subject: [PATCH 4/7] Add compile-time guards around helper routine --- library/ssl_tls.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 005362337..dd4c0d609 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -6120,6 +6120,7 @@ write_msg: return( ret ); } +#if defined(MBEDTLS_SSL_RENEGOTIATION) && defined(MBEDTLS_SSL_CLI_C) static int ssl_check_peer_crt_unchanged( mbedtls_ssl_context *ssl, unsigned char *crt_buf, size_t crt_buf_len ) @@ -6134,6 +6135,7 @@ static int ssl_check_peer_crt_unchanged( mbedtls_ssl_context *ssl, return( memcmp( peer_crt->raw.p, crt_buf, crt_buf_len) ); } +#endif /* MBEDTLS_SSL_RENEGOTIATION && MBEDTLS_SSL_CLI_C */ /* * Once the certificate message is read, parse it into a cert chain and From fd39919aa3638a415b12d401db42a10153266296 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 31 Jan 2019 07:44:17 +0000 Subject: [PATCH 5/7] Improve formatting of ssl_parse_certificate_chain() --- library/ssl_tls.c | 32 ++++++++++++++++++++------------ 1 file changed, 20 insertions(+), 12 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index dd4c0d609..6680c11a7 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -6244,8 +6244,9 @@ static int ssl_parse_certificate_chain( mbedtls_ssl_context *ssl ) /* Check that there's room for the next CRT's length fields. */ if ( i + 3 > ssl->in_hslen ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad certificate message" ) ); - mbedtls_ssl_send_alert_message( ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, - MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR ); + mbedtls_ssl_send_alert_message( ssl, + MBEDTLS_SSL_ALERT_LEVEL_FATAL, + MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR ); return( MBEDTLS_ERR_SSL_BAD_HS_CERTIFICATE ); } /* In theory, the CRT can be up to 2**24 Bytes, but we don't support @@ -6253,8 +6254,9 @@ static int ssl_parse_certificate_chain( mbedtls_ssl_context *ssl ) if( ssl->in_msg[i] != 0 ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad certificate message" ) ); - mbedtls_ssl_send_alert_message( ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, - MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR ); + mbedtls_ssl_send_alert_message( ssl, + MBEDTLS_SSL_ALERT_LEVEL_FATAL, + MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR ); return( MBEDTLS_ERR_SSL_BAD_HS_CERTIFICATE ); } @@ -6266,8 +6268,9 @@ static int ssl_parse_certificate_chain( mbedtls_ssl_context *ssl ) if( n < 128 || i + n > ssl->in_hslen ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad certificate message" ) ); - mbedtls_ssl_send_alert_message( ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, - MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR ); + mbedtls_ssl_send_alert_message( ssl, + MBEDTLS_SSL_ALERT_LEVEL_FATAL, + MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR ); return( MBEDTLS_ERR_SSL_BAD_HS_CERTIFICATE ); } @@ -6283,11 +6286,14 @@ static int ssl_parse_certificate_chain( mbedtls_ssl_context *ssl ) ssl->renego_status == MBEDTLS_SSL_RENEGOTIATION_IN_PROGRESS ) { MBEDTLS_SSL_DEBUG_MSG( 3, ( "Check that peer CRT hasn't changed during renegotiation" ) ); - if( ssl_check_peer_crt_unchanged( ssl, &ssl->in_msg[i], n ) != 0 ) + if( ssl_check_peer_crt_unchanged( ssl, + &ssl->in_msg[i], + n ) != 0 ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "new server cert during renegotiation" ) ); - mbedtls_ssl_send_alert_message( ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, - MBEDTLS_SSL_ALERT_MSG_ACCESS_DENIED ); + mbedtls_ssl_send_alert_message( ssl, + MBEDTLS_SSL_ALERT_LEVEL_FATAL, + MBEDTLS_SSL_ALERT_MSG_ACCESS_DENIED ); return( MBEDTLS_ERR_SSL_BAD_HS_CERTIFICATE ); } @@ -6296,7 +6302,8 @@ static int ssl_parse_certificate_chain( mbedtls_ssl_context *ssl ) ssl->session->peer_cert = NULL; /* Delete all remaining CRTs from the original CRT chain. */ - mbedtls_x509_crt_free( ssl->session_negotiate->peer_cert->next ); + mbedtls_x509_crt_free( + ssl->session_negotiate->peer_cert->next ); ssl->session_negotiate->peer_cert->next = NULL; i += n; @@ -6313,8 +6320,9 @@ static int ssl_parse_certificate_chain( mbedtls_ssl_context *ssl ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "alloc(%d bytes) failed", sizeof( mbedtls_x509_crt ) ) ); - mbedtls_ssl_send_alert_message( ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, - MBEDTLS_SSL_ALERT_MSG_INTERNAL_ERROR ); + mbedtls_ssl_send_alert_message( ssl, + MBEDTLS_SSL_ALERT_LEVEL_FATAL, + MBEDTLS_SSL_ALERT_MSG_INTERNAL_ERROR ); return( MBEDTLS_ERR_SSL_ALLOC_FAILED ); } From fe870275121698426566e425b14da90f07cac848 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 31 Jan 2019 16:37:56 +0000 Subject: [PATCH 6/7] Fix memory leak --- library/ssl_tls.c | 1 + 1 file changed, 1 insertion(+) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 6680c11a7..6ffbf7b83 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -6304,6 +6304,7 @@ static int ssl_parse_certificate_chain( mbedtls_ssl_context *ssl ) /* Delete all remaining CRTs from the original CRT chain. */ mbedtls_x509_crt_free( ssl->session_negotiate->peer_cert->next ); + mbedtls_free( ssl->session_negotiate->peer_cert->next ); ssl->session_negotiate->peer_cert->next = NULL; i += n; From 68b856d0ac7d7c6d58e0d0caaa989f985fd832c1 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 8 Feb 2019 14:00:04 +0000 Subject: [PATCH 7/7] Fix style issue and wording --- library/ssl_tls.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 6ffbf7b83..ede295462 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -6133,7 +6133,7 @@ static int ssl_check_peer_crt_unchanged( mbedtls_ssl_context *ssl, if( peer_crt->raw.len != crt_buf_len ) return( -1 ); - return( memcmp( peer_crt->raw.p, crt_buf, crt_buf_len) ); + return( memcmp( peer_crt->raw.p, crt_buf, crt_buf_len ) ); } #endif /* MBEDTLS_SSL_RENEGOTIATION && MBEDTLS_SSL_CLI_C */ @@ -6277,10 +6277,10 @@ static int ssl_parse_certificate_chain( mbedtls_ssl_context *ssl ) /* Check if we're handling the first CRT in the chain. */ if( ssl->session_negotiate->peer_cert == NULL ) { - /* During client-side renegotiation, check the server's end-CRTs - * hasn't changed compared to the initial handshake, mitigating - * the triple handshake attack. On success, reuse the original - * end-CRT instead of parsing it again. */ + /* During client-side renegotiation, check that the server's + * end-CRTs hasn't changed compared to the initial handshake, + * mitigating the triple handshake attack. On success, reuse + * the original end-CRT instead of parsing it again. */ #if defined(MBEDTLS_SSL_RENEGOTIATION) && defined(MBEDTLS_SSL_CLI_C) if( ssl->conf->endpoint == MBEDTLS_SSL_IS_CLIENT && ssl->renego_status == MBEDTLS_SSL_RENEGOTIATION_IN_PROGRESS )