From b2964cbe14c23d5d22d7bee024523a888955c2fe 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 afced7a99..5655d3aab 100644 --- a/library/ssl_cli.c +++ b/library/ssl_cli.c @@ -3463,6 +3463,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 def9bdc152779cc1bff084f40f5bad74523c61bb 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 8710a5076..4bbe0d410 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -5435,6 +5435,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 @@ -5525,35 +5540,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" ) ); @@ -5562,6 +5571,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; @@ -5574,67 +5584,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 bd9d51d969704a13a476f3fba81f56887cf75a88 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 6c3e4763e..62946c892 100644 --- a/ChangeLog +++ b/ChangeLog @@ -31,6 +31,8 @@ Bugfix Fixes #2190. 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 84879e32efe3fd32c122ffc3d8da91a8d0a19bf2 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 4bbe0d410..f62ccf39c 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -5435,6 +5435,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 ) @@ -5449,6 +5450,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 e2734e2be4cc60a2ae6aab825faf249190e08b23 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 f62ccf39c..9c468f7de 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -5559,8 +5559,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 @@ -5568,8 +5569,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 ); } @@ -5581,8 +5583,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 ); } @@ -5598,11 +5601,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 ); } @@ -5611,7 +5617,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; @@ -5628,8 +5635,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 c1e18bdf064a1114d3ce8ba4cdf5eb51d1ddd557 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 9c468f7de..13cb22b25 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -5619,6 +5619,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 46f34d0ac0e7246073d8ab4ff745f1f693020d96 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 13cb22b25..4fa193252 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -5448,7 +5448,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 */ @@ -5592,10 +5592,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 )