From 1332f35a4eadb89a066e8a3fe8ea9f18c59ad372 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 5 Feb 2019 15:06:15 +0000 Subject: [PATCH 01/67] Don't reuse CRT from initial handshake during renegotiation After mitigating the 'triple handshake attack' by checking that the peer's end-CRT didn't change during renegotation, the current code avoids re-parsing the CRT by moving the CRT-pointer from the old session to the new one. While efficient, this will no longer work once only the hash of the peer's CRT is stored beyond the handshake. This commit removes the code-path moving the old CRT, and instead frees the entire peer CRT chain from the initial handshake as soon as the 'triple handshake attack' protection has completed. --- library/ssl_tls.c | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index a5c8c33f9..b6f1d9326 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -6315,18 +6315,12 @@ static int ssl_parse_certificate_chain( mbedtls_ssl_context *ssl ) 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; + /* Now we can safely free the original chain. */ + mbedtls_x509_crt_free( ssl->session_negotiate->peer_cert ); + mbedtls_free( ssl->session_negotiate->peer_cert ); + ssl->session_negotiate->peer_cert = NULL; - /* 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; - continue; + /* Intentional fallthrough. */ } #endif /* MBEDTLS_SSL_RENEGOTIATION && MBEDTLS_SSL_CLI_C */ From 933b9fc815b49b7bd21b2a3dcb0d5f5a799fdab1 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 5 Feb 2019 11:42:30 +0000 Subject: [PATCH 02/67] Break overly long line in definition of mbedtls_ssl_get_session() --- library/ssl_tls.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index b6f1d9326..18fa2ea43 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -8833,7 +8833,8 @@ const mbedtls_x509_crt *mbedtls_ssl_get_peer_cert( const mbedtls_ssl_context *ss #endif /* MBEDTLS_X509_CRT_PARSE_C */ #if defined(MBEDTLS_SSL_CLI_C) -int mbedtls_ssl_get_session( const mbedtls_ssl_context *ssl, mbedtls_ssl_session *dst ) +int mbedtls_ssl_get_session( const mbedtls_ssl_context *ssl, + mbedtls_ssl_session *dst ) { if( ssl == NULL || dst == NULL || From 22141593e135068110bd9f8b5836313b14f69778 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 5 Feb 2019 12:38:15 +0000 Subject: [PATCH 03/67] Introduce helper function to clear peer CRT from session structure This commit introduces a helper function `ssl_clear_peer_cert()` which frees all data related to the peer's certificate from an `mbedtls_ssl_session` structure. Currently, this is the peer's certificate itself, while eventually, it'll be its digest only. --- library/ssl_tls.c | 29 ++++++++++++++--------------- 1 file changed, 14 insertions(+), 15 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 18fa2ea43..e06ce999c 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -6155,6 +6155,16 @@ static int ssl_check_peer_crt_unchanged( mbedtls_ssl_context *ssl, } #endif /* MBEDTLS_SSL_RENEGOTIATION && MBEDTLS_SSL_CLI_C */ +static void ssl_clear_peer_cert( mbedtls_ssl_session *session ) +{ + if( session->peer_cert != NULL ) + { + mbedtls_x509_crt_free( session->peer_cert ); + mbedtls_free( session->peer_cert ); + session->peer_cert = NULL; + } +} + /* * Once the certificate message is read, parse it into a cert chain and * perform basic checks, but leave actual verification to the caller @@ -6248,13 +6258,8 @@ static int ssl_parse_certificate_chain( mbedtls_ssl_context *ssl ) /* 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; - } + /* In case we tried to reuse a session but it failed. */ + ssl_clear_peer_cert( ssl->session_negotiate ); /* Iterate through and parse the CRTs in the provided chain. */ while( i < ssl->in_hslen ) @@ -6316,9 +6321,7 @@ static int ssl_parse_certificate_chain( mbedtls_ssl_context *ssl ) } /* Now we can safely free the original chain. */ - mbedtls_x509_crt_free( ssl->session_negotiate->peer_cert ); - mbedtls_free( ssl->session_negotiate->peer_cert ); - ssl->session_negotiate->peer_cert = NULL; + ssl_clear_peer_cert( ssl->session ); /* Intentional fallthrough. */ } @@ -10211,11 +10214,7 @@ void mbedtls_ssl_session_free( mbedtls_ssl_session *session ) return; #if defined(MBEDTLS_X509_CRT_PARSE_C) - if( session->peer_cert != NULL ) - { - mbedtls_x509_crt_free( session->peer_cert ); - mbedtls_free( session->peer_cert ); - } + ssl_clear_peer_cert( session ); #endif #if defined(MBEDTLS_SSL_SESSION_TICKETS) && defined(MBEDTLS_SSL_CLI_C) From 8794fd927c248faa4142c11286f88c4e97fbade2 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 5 Feb 2019 12:38:45 +0000 Subject: [PATCH 04/67] Introduce CRT counter to CRT chain parsing function So far, we've used the `peer_cert` pointer to detect whether we're parsing the first CRT, but that will soon be removed if `MBEDTLS_SSL_KEEP_PEER_CERTIFICATE` is unset. --- 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 e06ce999c..94184659d 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -6171,7 +6171,7 @@ static void ssl_clear_peer_cert( mbedtls_ssl_session *session ) */ static int ssl_parse_certificate_chain( mbedtls_ssl_context *ssl ) { - int ret; + int ret, crt_cnt=0; size_t i, n; uint8_t alert; @@ -6298,7 +6298,7 @@ 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 ) + if( crt_cnt++ == 0 ) { /* During client-side renegotiation, check that the server's * end-CRTs hasn't changed compared to the initial handshake, From b8a085744f38cb1a86c1dadfbf34c631564ed359 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 5 Feb 2019 12:49:06 +0000 Subject: [PATCH 05/67] Introduce helper to check for no-CRT notification from client This commit introduces a server-side static helper function `ssl_srv_check_client_no_crt_notification()`, which checks if the message we received during the incoming certificate state notifies the server of the lack of certificate on the client. For SSLv3, such a notification comes as a specific alert, while for all other TLS versions, it comes as a `Certificate` handshake message with an empty CRT list. --- library/ssl_tls.c | 109 +++++++++++++++++++++++----------------------- 1 file changed, 55 insertions(+), 54 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 94184659d..389b2f2d3 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -6175,52 +6175,6 @@ static int ssl_parse_certificate_chain( mbedtls_ssl_context *ssl ) size_t i, n; uint8_t alert; -#if defined(MBEDTLS_SSL_SRV_C) -#if defined(MBEDTLS_SSL_PROTO_SSL3) - /* - * Check if the client sent an empty certificate - */ - if( ssl->conf->endpoint == MBEDTLS_SSL_IS_SERVER && - ssl->minor_ver == MBEDTLS_SSL_MINOR_VERSION_0 ) - { - if( ssl->in_msglen == 2 && - ssl->in_msgtype == MBEDTLS_SSL_MSG_ALERT && - ssl->in_msg[0] == MBEDTLS_SSL_ALERT_LEVEL_WARNING && - ssl->in_msg[1] == MBEDTLS_SSL_ALERT_MSG_NO_CERT ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "SSLv3 client has no certificate" ) ); - - /* The client was asked for a certificate but didn't send - one. The client should know what's going on, so we - don't send an alert. */ - ssl->session_negotiate->verify_result = MBEDTLS_X509_BADCERT_MISSING; - return( MBEDTLS_ERR_SSL_NO_CLIENT_CERTIFICATE ); - } - } -#endif /* MBEDTLS_SSL_PROTO_SSL3 */ - -#if defined(MBEDTLS_SSL_PROTO_TLS1) || defined(MBEDTLS_SSL_PROTO_TLS1_1) || \ - defined(MBEDTLS_SSL_PROTO_TLS1_2) - if( ssl->conf->endpoint == MBEDTLS_SSL_IS_SERVER && - ssl->minor_ver != MBEDTLS_SSL_MINOR_VERSION_0 ) - { - if( ssl->in_hslen == 3 + mbedtls_ssl_hs_hdr_len( ssl ) && - ssl->in_msgtype == MBEDTLS_SSL_MSG_HANDSHAKE && - ssl->in_msg[0] == MBEDTLS_SSL_HS_CERTIFICATE && - memcmp( ssl->in_msg + mbedtls_ssl_hs_hdr_len( ssl ), "\0\0\0", 3 ) == 0 ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "TLSv1 client has no certificate" ) ); - - /* The client was asked for a certificate but didn't send - one. The client should know what's going on, so we - don't send an alert. */ - ssl->session_negotiate->verify_result = MBEDTLS_X509_BADCERT_MISSING; - return( MBEDTLS_ERR_SSL_NO_CLIENT_CERTIFICATE ); - } - } -#endif /* MBEDTLS_SSL_PROTO_TLS1 || MBEDTLS_SSL_PROTO_TLS1_1 || \ - MBEDTLS_SSL_PROTO_TLS1_2 */ -#endif /* MBEDTLS_SSL_SRV_C */ if( ssl->in_msgtype != MBEDTLS_SSL_MSG_HANDSHAKE ) { @@ -6381,6 +6335,48 @@ static int ssl_parse_certificate_chain( mbedtls_ssl_context *ssl ) return( 0 ); } +#if defined(MBEDTLS_SSL_SRV_C) +static int ssl_srv_check_client_no_crt_notification( mbedtls_ssl_context *ssl ) +{ + if( ssl->conf->endpoint == MBEDTLS_SSL_IS_CLIENT ) + return( -1 ); + +#if defined(MBEDTLS_SSL_PROTO_SSL3) + /* + * Check if the client sent an empty certificate + */ + if( ssl->minor_ver == MBEDTLS_SSL_MINOR_VERSION_0 ) + { + if( ssl->in_msglen == 2 && + ssl->in_msgtype == MBEDTLS_SSL_MSG_ALERT && + ssl->in_msg[0] == MBEDTLS_SSL_ALERT_LEVEL_WARNING && + ssl->in_msg[1] == MBEDTLS_SSL_ALERT_MSG_NO_CERT ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "SSLv3 client has no certificate" ) ); + return( 0 ); + } + + return( -1 ); + } +#endif /* MBEDTLS_SSL_PROTO_SSL3 */ + +#if defined(MBEDTLS_SSL_PROTO_TLS1) || defined(MBEDTLS_SSL_PROTO_TLS1_1) || \ + defined(MBEDTLS_SSL_PROTO_TLS1_2) + if( ssl->in_hslen == 3 + mbedtls_ssl_hs_hdr_len( ssl ) && + ssl->in_msgtype == MBEDTLS_SSL_MSG_HANDSHAKE && + ssl->in_msg[0] == MBEDTLS_SSL_HS_CERTIFICATE && + memcmp( ssl->in_msg + mbedtls_ssl_hs_hdr_len( ssl ), "\0\0\0", 3 ) == 0 ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "TLSv1 client has no certificate" ) ); + return( 0 ); + } + + return( -1 ); +#endif /* MBEDTLS_SSL_PROTO_TLS1 || MBEDTLS_SSL_PROTO_TLS1_1 || \ + MBEDTLS_SSL_PROTO_TLS1_2 */ +} +#endif /* MBEDTLS_SSL_SRV_C */ + int mbedtls_ssl_parse_certificate( mbedtls_ssl_context *ssl ) { int ret; @@ -6443,16 +6439,21 @@ int mbedtls_ssl_parse_certificate( mbedtls_ssl_context *ssl ) return( ret ); } +#if defined(MBEDTLS_SSL_SRV_C) + if( ssl_srv_check_client_no_crt_notification( ssl ) == 0 ) + { + ssl->session_negotiate->verify_result = MBEDTLS_X509_BADCERT_MISSING; + ssl->state++; + + if( authmode == MBEDTLS_SSL_VERIFY_OPTIONAL ) + return( 0 ); + + return( MBEDTLS_ERR_SSL_NO_CLIENT_CERTIFICATE ); + } +#endif /* MBEDTLS_SSL_SRV_C */ + if( ( ret = ssl_parse_certificate_chain( ssl ) ) != 0 ) { -#if defined(MBEDTLS_SSL_SRV_C) - if( ret == MBEDTLS_ERR_SSL_NO_CLIENT_CERTIFICATE && - authmode == MBEDTLS_SSL_VERIFY_OPTIONAL ) - { - ret = 0; - } -#endif - ssl->state++; return( ret ); } From a46c28779655a75650c3d74963760870892001e9 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 5 Feb 2019 13:08:01 +0000 Subject: [PATCH 06/67] Clear peer's CRT chain outside before parsing new one If an attempt for session resumption fails, the `session_negotiate` structure might be partially filled, and in particular already contain a peer certificate structure. This certificate structure needs to be freed before parsing the certificate sent in the `Certificate` message. This commit moves the code-path taking care of this from the helper function `ssl_parse_certificate_chain()`, whose purpose should be parsing only, to the top-level handler `mbedtls_ssl_parse_certificate()`. The fact that we don't know the state of `ssl->session_negotiate` after a failed attempt for session resumption is undesirable, and a separate issue #2414 has been opened to improve on this. --- library/ssl_tls.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 389b2f2d3..a5d9ca5de 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -6212,9 +6212,6 @@ static int ssl_parse_certificate_chain( mbedtls_ssl_context *ssl ) /* 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. */ - ssl_clear_peer_cert( ssl->session_negotiate ); - /* Iterate through and parse the CRTs in the provided chain. */ while( i < ssl->in_hslen ) { @@ -6452,6 +6449,9 @@ int mbedtls_ssl_parse_certificate( mbedtls_ssl_context *ssl ) } #endif /* MBEDTLS_SSL_SRV_C */ + /* In case we tried to reuse a session but it failed. */ + ssl_clear_peer_cert( ssl->session_negotiate ); + if( ( ret = ssl_parse_certificate_chain( ssl ) ) != 0 ) { ssl->state++; From 613d490bf14220b19ac06c068bf18c0faab1918f Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 5 Feb 2019 13:11:17 +0000 Subject: [PATCH 07/67] Unify state machine update in mbedtls_ssl_parse_certificate() The handler `mbedtls_ssl_parse_certificate()` for incoming `Certificate` messages contains many branches updating the handshake state. For easier reasoning about state evolution, this commit introduces a single code-path updating the state machine at the end of `mbedtls_ssl_parse_certificate()`. --- library/ssl_tls.c | 29 ++++++++++++----------------- 1 file changed, 12 insertions(+), 17 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index a5d9ca5de..767010902 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -6376,7 +6376,7 @@ static int ssl_srv_check_client_no_crt_notification( mbedtls_ssl_context *ssl ) int mbedtls_ssl_parse_certificate( mbedtls_ssl_context *ssl ) { - int ret; + int ret = 0; const mbedtls_ssl_ciphersuite_t * const ciphersuite_info = ssl->handshake->ciphersuite_info; #if defined(MBEDTLS_SSL_SRV_C) && defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) @@ -6396,8 +6396,7 @@ int mbedtls_ssl_parse_certificate( mbedtls_ssl_context *ssl ) ciphersuite_info->key_exchange == MBEDTLS_KEY_EXCHANGE_ECJPAKE ) { MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= skip parse certificate" ) ); - ssl->state++; - return( 0 ); + goto exit; } #if defined(MBEDTLS_SSL_SRV_C) @@ -6405,8 +6404,7 @@ int mbedtls_ssl_parse_certificate( mbedtls_ssl_context *ssl ) ciphersuite_info->key_exchange == MBEDTLS_KEY_EXCHANGE_RSA_PSK ) { MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= skip parse certificate" ) ); - ssl->state++; - return( 0 ); + goto exit; } if( ssl->conf->endpoint == MBEDTLS_SSL_IS_SERVER && @@ -6414,9 +6412,7 @@ int mbedtls_ssl_parse_certificate( mbedtls_ssl_context *ssl ) { ssl->session_negotiate->verify_result = MBEDTLS_X509_BADCERT_SKIP_VERIFY; MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= skip parse certificate" ) ); - - ssl->state++; - return( 0 ); + goto exit; } #endif @@ -6440,12 +6436,13 @@ int mbedtls_ssl_parse_certificate( mbedtls_ssl_context *ssl ) if( ssl_srv_check_client_no_crt_notification( ssl ) == 0 ) { ssl->session_negotiate->verify_result = MBEDTLS_X509_BADCERT_MISSING; - ssl->state++; if( authmode == MBEDTLS_SSL_VERIFY_OPTIONAL ) - return( 0 ); + ret = 0; + else + ret = MBEDTLS_ERR_SSL_NO_CLIENT_CERTIFICATE; - return( MBEDTLS_ERR_SSL_NO_CLIENT_CERTIFICATE ); + goto exit; } #endif /* MBEDTLS_SSL_SRV_C */ @@ -6453,10 +6450,7 @@ int mbedtls_ssl_parse_certificate( mbedtls_ssl_context *ssl ) ssl_clear_peer_cert( ssl->session_negotiate ); if( ( ret = ssl_parse_certificate_chain( ssl ) ) != 0 ) - { - ssl->state++; - return( ret ); - } + goto exit; #if defined(MBEDTLS_SSL__ECP_RESTARTABLE) if( ssl->handshake->ecrs_enabled) @@ -6602,10 +6596,11 @@ crt_verify: #endif /* MBEDTLS_DEBUG_C */ } - ssl->state++; - MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= parse certificate" ) ); +exit: + + ssl->state++; return( ret ); } #endif /* !MBEDTLS_KEY_EXCHANGE_RSA_ENABLED From b71e90acc55bb87765723b5eeacac9646ae39a91 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 5 Feb 2019 13:20:55 +0000 Subject: [PATCH 08/67] Use helper macro to detect whether some ciphersuite uses CRTs --- library/ssl_tls.c | 18 +++--------------- 1 file changed, 3 insertions(+), 15 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 767010902..104ed2250 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -5971,13 +5971,7 @@ int mbedtls_ssl_send_alert_message( mbedtls_ssl_context *ssl, /* * Handshake functions */ -#if !defined(MBEDTLS_KEY_EXCHANGE_RSA_ENABLED) && \ - !defined(MBEDTLS_KEY_EXCHANGE_RSA_PSK_ENABLED) && \ - !defined(MBEDTLS_KEY_EXCHANGE_DHE_RSA_ENABLED) && \ - !defined(MBEDTLS_KEY_EXCHANGE_ECDHE_RSA_ENABLED) && \ - !defined(MBEDTLS_KEY_EXCHANGE_ECDHE_ECDSA_ENABLED) && \ - !defined(MBEDTLS_KEY_EXCHANGE_ECDH_RSA_ENABLED) && \ - !defined(MBEDTLS_KEY_EXCHANGE_ECDH_ECDSA_ENABLED) +#if !defined(MBEDTLS_KEY_EXCHANGE__WITH_CERT__ENABLED) /* No certificate support -> dummy functions */ int mbedtls_ssl_write_certificate( mbedtls_ssl_context *ssl ) { @@ -6019,7 +6013,7 @@ int mbedtls_ssl_parse_certificate( mbedtls_ssl_context *ssl ) return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); } -#else +#else /* MBEDTLS_KEY_EXCHANGE__WITH_CERT__ENABLED */ /* Some certificate support -> implement write and parse */ int mbedtls_ssl_write_certificate( mbedtls_ssl_context *ssl ) @@ -6603,13 +6597,7 @@ exit: ssl->state++; return( ret ); } -#endif /* !MBEDTLS_KEY_EXCHANGE_RSA_ENABLED - !MBEDTLS_KEY_EXCHANGE_RSA_PSK_ENABLED - !MBEDTLS_KEY_EXCHANGE_DHE_RSA_ENABLED - !MBEDTLS_KEY_EXCHANGE_ECDHE_RSA_ENABLED - !MBEDTLS_KEY_EXCHANGE_ECDHE_ECDSA_ENABLED - !MBEDTLS_KEY_EXCHANGE_ECDH_RSA_ENABLED - !MBEDTLS_KEY_EXCHANGE_ECDH_ECDSA_ENABLED */ +#endif /* MBEDTLS_KEY_EXCHANGE__WITH_CERT__ENABLED */ int mbedtls_ssl_write_change_cipher_spec( mbedtls_ssl_context *ssl ) { From 5097cba93c531cdaee8e6ac459aa3ec3111ead6b Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 5 Feb 2019 13:36:46 +0000 Subject: [PATCH 09/67] Introduce helper function to determine whether suite uses server CRT This commit introduces a static helper function `mbedtls_ssl_ciphersuite_uses_srv_cert()` which determines whether a ciphersuite may make use of server-side CRTs. This function is in turn uses in `mbedtls_ssl_parse_certificate()` to skip certificate parsing for ciphersuites which don't involve CRTs. Note: Ciphersuites not using server-side CRTs don't allow client-side CRTs either, so it is safe to guard `mbedtls_ssl_{parse/write}_certificate()` this way. Note: Previously, the code uses a positive check over the suites - MBEDTLS_KEY_EXCHANGE_PSK - MBEDTLS_KEY_EXCHANGE_DHE_PSK - MBEDTLS_KEY_EXCHANGE_ECDHE_PSK - MBEDTLS_KEY_EXCHANGE_ECJPAKE, while now, it uses a negative check over `mbedtls_ssl_ciphersuite_uses_srv_cert()`, which checks for the suites - MBEDTLS_KEY_EXCHANGE_RSA - MBEDTLS_KEY_EXCHANGE_RSA_PSK - MBEDTLS_KEY_EXCHANGE_DHE_RSA - MBEDTLS_KEY_EXCHANGE_ECDH_RSA - MBEDTLS_KEY_EXCHANGE_ECDHE_RSA - MBEDTLS_KEY_EXCHANGE_ECDH_ECDSA - MBEDTLS_KEY_EXCHANGE_ECDHE_ECDSA This is equivalent since, together, those are all ciphersuites. Quoting ssl_ciphersuites.h: ``` typedef enum { MBEDTLS_KEY_EXCHANGE_NONE = 0, MBEDTLS_KEY_EXCHANGE_RSA, MBEDTLS_KEY_EXCHANGE_DHE_RSA, MBEDTLS_KEY_EXCHANGE_ECDHE_RSA, MBEDTLS_KEY_EXCHANGE_ECDHE_ECDSA, MBEDTLS_KEY_EXCHANGE_PSK, MBEDTLS_KEY_EXCHANGE_DHE_PSK, MBEDTLS_KEY_EXCHANGE_RSA_PSK, MBEDTLS_KEY_EXCHANGE_ECDHE_PSK, MBEDTLS_KEY_EXCHANGE_ECDH_RSA, MBEDTLS_KEY_EXCHANGE_ECDH_ECDSA, MBEDTLS_KEY_EXCHANGE_ECJPAKE, } mbedtls_key_exchange_type_t; ``` --- include/mbedtls/ssl_ciphersuites.h | 18 ++++++++++++++++++ library/ssl_tls.c | 21 ++++----------------- 2 files changed, 22 insertions(+), 17 deletions(-) diff --git a/include/mbedtls/ssl_ciphersuites.h b/include/mbedtls/ssl_ciphersuites.h index 71053e5ba..712678330 100644 --- a/include/mbedtls/ssl_ciphersuites.h +++ b/include/mbedtls/ssl_ciphersuites.h @@ -486,6 +486,24 @@ static inline int mbedtls_ssl_ciphersuite_cert_req_allowed( const mbedtls_ssl_ci } } +static inline int mbedtls_ssl_ciphersuite_uses_srv_cert( const mbedtls_ssl_ciphersuite_t *info ) +{ + switch( info->key_exchange ) + { + case MBEDTLS_KEY_EXCHANGE_RSA: + case MBEDTLS_KEY_EXCHANGE_RSA_PSK: + case MBEDTLS_KEY_EXCHANGE_DHE_RSA: + case MBEDTLS_KEY_EXCHANGE_ECDH_RSA: + case MBEDTLS_KEY_EXCHANGE_ECDHE_RSA: + case MBEDTLS_KEY_EXCHANGE_ECDH_ECDSA: + case MBEDTLS_KEY_EXCHANGE_ECDHE_ECDSA: + return( 1 ); + + default: + return( 0 ); + } +} + #if defined(MBEDTLS_KEY_EXCHANGE__SOME__DHE_ENABLED) static inline int mbedtls_ssl_ciphersuite_uses_dhe( const mbedtls_ssl_ciphersuite_t *info ) { diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 104ed2250..cd8fffc71 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -5979,10 +5979,7 @@ int mbedtls_ssl_write_certificate( mbedtls_ssl_context *ssl ) MBEDTLS_SSL_DEBUG_MSG( 2, ( "=> write certificate" ) ); - if( ciphersuite_info->key_exchange == MBEDTLS_KEY_EXCHANGE_PSK || - ciphersuite_info->key_exchange == MBEDTLS_KEY_EXCHANGE_DHE_PSK || - ciphersuite_info->key_exchange == MBEDTLS_KEY_EXCHANGE_ECDHE_PSK || - ciphersuite_info->key_exchange == MBEDTLS_KEY_EXCHANGE_ECJPAKE ) + if( !mbedtls_ssl_ciphersuite_uses_srv_cert( ciphersuite_info ) ) { MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= skip write certificate" ) ); ssl->state++; @@ -5999,10 +5996,7 @@ int mbedtls_ssl_parse_certificate( mbedtls_ssl_context *ssl ) MBEDTLS_SSL_DEBUG_MSG( 2, ( "=> parse certificate" ) ); - if( ciphersuite_info->key_exchange == MBEDTLS_KEY_EXCHANGE_PSK || - ciphersuite_info->key_exchange == MBEDTLS_KEY_EXCHANGE_DHE_PSK || - ciphersuite_info->key_exchange == MBEDTLS_KEY_EXCHANGE_ECDHE_PSK || - ciphersuite_info->key_exchange == MBEDTLS_KEY_EXCHANGE_ECJPAKE ) + if( !mbedtls_ssl_ciphersuite_uses_srv_cert( ciphersuite_info ) ) { MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= skip parse certificate" ) ); ssl->state++; @@ -6025,10 +6019,7 @@ int mbedtls_ssl_write_certificate( mbedtls_ssl_context *ssl ) MBEDTLS_SSL_DEBUG_MSG( 2, ( "=> write certificate" ) ); - if( ciphersuite_info->key_exchange == MBEDTLS_KEY_EXCHANGE_PSK || - ciphersuite_info->key_exchange == MBEDTLS_KEY_EXCHANGE_DHE_PSK || - ciphersuite_info->key_exchange == MBEDTLS_KEY_EXCHANGE_ECDHE_PSK || - ciphersuite_info->key_exchange == MBEDTLS_KEY_EXCHANGE_ECJPAKE ) + if( !mbedtls_ssl_ciphersuite_uses_srv_cert( ciphersuite_info ) ) { MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= skip write certificate" ) ); ssl->state++; @@ -6169,7 +6160,6 @@ static int ssl_parse_certificate_chain( mbedtls_ssl_context *ssl ) size_t i, n; uint8_t alert; - if( ssl->in_msgtype != MBEDTLS_SSL_MSG_HANDSHAKE ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad certificate message" ) ); @@ -6384,10 +6374,7 @@ int mbedtls_ssl_parse_certificate( mbedtls_ssl_context *ssl ) MBEDTLS_SSL_DEBUG_MSG( 2, ( "=> parse certificate" ) ); - if( ciphersuite_info->key_exchange == MBEDTLS_KEY_EXCHANGE_PSK || - ciphersuite_info->key_exchange == MBEDTLS_KEY_EXCHANGE_DHE_PSK || - ciphersuite_info->key_exchange == MBEDTLS_KEY_EXCHANGE_ECDHE_PSK || - ciphersuite_info->key_exchange == MBEDTLS_KEY_EXCHANGE_ECJPAKE ) + if( !mbedtls_ssl_ciphersuite_uses_srv_cert( ciphersuite_info ) ) { MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= skip parse certificate" ) ); goto exit; From 6b9a6f3f3700e123ee985f0bf518e27ef180235a Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 7 Feb 2019 10:11:07 +0000 Subject: [PATCH 10/67] Add helper function to check whether a CRT msg is expected This commit adds a helper function `ssl_parse_certificate_coordinate()` which checks whether a `Certificate` message is expected from the peer. The logic is the following: - For ciphersuites which don't use server-side CRTs, no Certificate message is expected (neither for the server, nor the client). - On the server, no client certificate is expected in the following cases: * The server server didn't request a Certificate, which is controlled by the `authmode` setting. * A RSA-PSK suite is used; this is the only suite using server CRTs but not allowing client-side authentication. --- library/ssl_tls.c | 62 ++++++++++++++++++++++++++++++++--------------- 1 file changed, 42 insertions(+), 20 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index cd8fffc71..e3663fc92 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -6358,11 +6358,49 @@ static int ssl_srv_check_client_no_crt_notification( mbedtls_ssl_context *ssl ) } #endif /* MBEDTLS_SSL_SRV_C */ +/* Check if a certificate message is expected. + * Return either + * - SSL_CERTIFICATE_EXPECTED, or + * - SSL_CERTIFICATE_SKIP + * indicating whether a Certificate message is expected or not. + */ +#define SSL_CERTIFICATE_EXPECTED 0 +#define SSL_CERTIFICATE_SKIP 1 +static int ssl_parse_certificate_coordinate( mbedtls_ssl_context *ssl, + int authmode ) +{ + const mbedtls_ssl_ciphersuite_t *ciphersuite_info = + ssl->handshake->ciphersuite_info; + + if( !mbedtls_ssl_ciphersuite_uses_srv_cert( ciphersuite_info ) ) + return( SSL_CERTIFICATE_SKIP ); + +#if defined(MBEDTLS_SSL_SRV_C) + if( ssl->conf->endpoint == MBEDTLS_SSL_IS_SERVER ) + { + if( ciphersuite_info->key_exchange == MBEDTLS_KEY_EXCHANGE_RSA_PSK ) + return( SSL_CERTIFICATE_SKIP ); + + if( authmode == MBEDTLS_SSL_VERIFY_NONE ) + { + /* NOTE: Is it intentional that we set verify_result + * to SKIP_VERIFY on server-side only? */ + ssl->session_negotiate->verify_result = + MBEDTLS_X509_BADCERT_SKIP_VERIFY; + return( SSL_CERTIFICATE_SKIP ); + } + } +#endif /* MBEDTLS_SSL_SRV_C */ + + return( SSL_CERTIFICATE_EXPECTED ); +} + int mbedtls_ssl_parse_certificate( mbedtls_ssl_context *ssl ) { int ret = 0; - const mbedtls_ssl_ciphersuite_t * const ciphersuite_info = - ssl->handshake->ciphersuite_info; + const mbedtls_ssl_ciphersuite_t *ciphersuite_info = + ssl->handshake->ciphersuite_info; + int crt_expected; #if defined(MBEDTLS_SSL_SRV_C) && defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) const int authmode = ssl->handshake->sni_authmode != MBEDTLS_SSL_VERIFY_UNSET ? ssl->handshake->sni_authmode @@ -6374,29 +6412,13 @@ int mbedtls_ssl_parse_certificate( mbedtls_ssl_context *ssl ) MBEDTLS_SSL_DEBUG_MSG( 2, ( "=> parse certificate" ) ); - if( !mbedtls_ssl_ciphersuite_uses_srv_cert( ciphersuite_info ) ) + crt_expected = ssl_parse_certificate_coordinate( ssl, authmode ); + if( crt_expected == SSL_CERTIFICATE_SKIP ) { MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= skip parse certificate" ) ); goto exit; } -#if defined(MBEDTLS_SSL_SRV_C) - if( ssl->conf->endpoint == MBEDTLS_SSL_IS_SERVER && - ciphersuite_info->key_exchange == MBEDTLS_KEY_EXCHANGE_RSA_PSK ) - { - MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= skip parse certificate" ) ); - goto exit; - } - - if( ssl->conf->endpoint == MBEDTLS_SSL_IS_SERVER && - authmode == MBEDTLS_SSL_VERIFY_NONE ) - { - ssl->session_negotiate->verify_result = MBEDTLS_X509_BADCERT_SKIP_VERIFY; - MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= skip parse certificate" ) ); - goto exit; - } -#endif - #if defined(MBEDTLS_SSL__ECP_RESTARTABLE) if( ssl->handshake->ecrs_enabled && ssl->handshake->ecrs_state == ssl_ecrs_crt_verify ) From ae39b9eb4851c5be5683d5b7922305734ea1c1f1 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 7 Feb 2019 12:32:43 +0000 Subject: [PATCH 11/67] Make use of macro and helper detecting whether CertRequest allowed This commit simplifies the client-side code for outgoing CertificateVerify messages, and server-side code for outgoing CertificateRequest messages and incoming CertificateVerify messages, through the use of the macro `MBEDTLS_KEY_EXCHANGE__CERT_REQ_ALLOWED__ENABLED` indicating whether a ciphersuite allowing CertificateRequest messages is enabled in the configuration, as well as the helper function `mbedtls_ssl_ciphersuite_cert_req_allowed()` indicating whether a particular ciphersuite allows CertificateRequest messages. These were already used in the client-side code to simplify the parsing functions for CertificateRequest messages. --- library/ssl_cli.c | 28 +++++------------------- library/ssl_srv.c | 56 +++++++++-------------------------------------- 2 files changed, 15 insertions(+), 69 deletions(-) diff --git a/library/ssl_cli.c b/library/ssl_cli.c index cd9793e46..b47f3dfa0 100644 --- a/library/ssl_cli.c +++ b/library/ssl_cli.c @@ -3306,12 +3306,7 @@ ecdh_calc_secret: return( 0 ); } -#if !defined(MBEDTLS_KEY_EXCHANGE_RSA_ENABLED) && \ - !defined(MBEDTLS_KEY_EXCHANGE_DHE_RSA_ENABLED) && \ - !defined(MBEDTLS_KEY_EXCHANGE_ECDH_RSA_ENABLED) && \ - !defined(MBEDTLS_KEY_EXCHANGE_ECDHE_RSA_ENABLED) && \ - !defined(MBEDTLS_KEY_EXCHANGE_ECDH_ECDSA_ENABLED)&& \ - !defined(MBEDTLS_KEY_EXCHANGE_ECDHE_ECDSA_ENABLED) +#if !defined(MBEDTLS_KEY_EXCHANGE__CERT_REQ_ALLOWED__ENABLED) static int ssl_write_certificate_verify( mbedtls_ssl_context *ssl ) { const mbedtls_ssl_ciphersuite_t *ciphersuite_info = @@ -3326,11 +3321,7 @@ static int ssl_write_certificate_verify( mbedtls_ssl_context *ssl ) return( ret ); } - if( ciphersuite_info->key_exchange == MBEDTLS_KEY_EXCHANGE_PSK || - ciphersuite_info->key_exchange == MBEDTLS_KEY_EXCHANGE_RSA_PSK || - ciphersuite_info->key_exchange == MBEDTLS_KEY_EXCHANGE_ECDHE_PSK || - ciphersuite_info->key_exchange == MBEDTLS_KEY_EXCHANGE_DHE_PSK || - ciphersuite_info->key_exchange == MBEDTLS_KEY_EXCHANGE_ECJPAKE ) + if( !mbedtls_ssl_ciphersuite_cert_req_allowed( ciphersuite_info ) ) { MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= skip write certificate verify" ) ); ssl->state++; @@ -3340,7 +3331,7 @@ static int ssl_write_certificate_verify( mbedtls_ssl_context *ssl ) MBEDTLS_SSL_DEBUG_MSG( 1, ( "should never happen" ) ); return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); } -#else +#else /* !MBEDTLS_KEY_EXCHANGE__CERT_REQ_ALLOWED__ENABLED */ static int ssl_write_certificate_verify( mbedtls_ssl_context *ssl ) { int ret = MBEDTLS_ERR_SSL_FEATURE_UNAVAILABLE; @@ -3369,11 +3360,7 @@ static int ssl_write_certificate_verify( mbedtls_ssl_context *ssl ) return( ret ); } - if( ciphersuite_info->key_exchange == MBEDTLS_KEY_EXCHANGE_PSK || - ciphersuite_info->key_exchange == MBEDTLS_KEY_EXCHANGE_RSA_PSK || - ciphersuite_info->key_exchange == MBEDTLS_KEY_EXCHANGE_ECDHE_PSK || - ciphersuite_info->key_exchange == MBEDTLS_KEY_EXCHANGE_DHE_PSK || - ciphersuite_info->key_exchange == MBEDTLS_KEY_EXCHANGE_ECJPAKE ) + if( !mbedtls_ssl_ciphersuite_cert_req_allowed( ciphersuite_info ) ) { MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= skip write certificate verify" ) ); ssl->state++; @@ -3514,12 +3501,7 @@ sign: return( ret ); } -#endif /* !MBEDTLS_KEY_EXCHANGE_RSA_ENABLED && - !MBEDTLS_KEY_EXCHANGE_DHE_RSA_ENABLED && - !MBEDTLS_KEY_EXCHANGE_ECDH_RSA_ENABLED && - !MBEDTLS_KEY_EXCHANGE_ECDHE_RSA_ENABLED && - !MBEDTLS_KEY_EXCHANGE_ECDH_ECDSA_ENABLED && - !MBEDTLS_KEY_EXCHANGE_ECDHE_ECDSA_ENABLED */ +#endif /* MBEDTLS_KEY_EXCHANGE__CERT_REQ_ALLOWED__ENABLED */ #if defined(MBEDTLS_SSL_SESSION_TICKETS) static int ssl_parse_new_session_ticket( mbedtls_ssl_context *ssl ) diff --git a/library/ssl_srv.c b/library/ssl_srv.c index d40a21f6f..884bdd17b 100644 --- a/library/ssl_srv.c +++ b/library/ssl_srv.c @@ -2801,12 +2801,7 @@ static int ssl_write_server_hello( mbedtls_ssl_context *ssl ) return( ret ); } -#if !defined(MBEDTLS_KEY_EXCHANGE_RSA_ENABLED) && \ - !defined(MBEDTLS_KEY_EXCHANGE_DHE_RSA_ENABLED) && \ - !defined(MBEDTLS_KEY_EXCHANGE_ECDH_RSA_ENABLED) && \ - !defined(MBEDTLS_KEY_EXCHANGE_ECDHE_RSA_ENABLED) && \ - !defined(MBEDTLS_KEY_EXCHANGE_ECDH_ECDSA_ENABLED)&& \ - !defined(MBEDTLS_KEY_EXCHANGE_ECDHE_ECDSA_ENABLED) +#if !defined(MBEDTLS_KEY_EXCHANGE__CERT_REQ_ALLOWED__ENABLED) static int ssl_write_certificate_request( mbedtls_ssl_context *ssl ) { const mbedtls_ssl_ciphersuite_t *ciphersuite_info = @@ -2814,11 +2809,7 @@ static int ssl_write_certificate_request( mbedtls_ssl_context *ssl ) MBEDTLS_SSL_DEBUG_MSG( 2, ( "=> write certificate request" ) ); - if( ciphersuite_info->key_exchange == MBEDTLS_KEY_EXCHANGE_PSK || - ciphersuite_info->key_exchange == MBEDTLS_KEY_EXCHANGE_RSA_PSK || - ciphersuite_info->key_exchange == MBEDTLS_KEY_EXCHANGE_DHE_PSK || - ciphersuite_info->key_exchange == MBEDTLS_KEY_EXCHANGE_ECDHE_PSK || - ciphersuite_info->key_exchange == MBEDTLS_KEY_EXCHANGE_ECJPAKE ) + if( !mbedtls_ssl_ciphersuite_cert_req_allowed( ciphersuite_info ) ) { MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= skip write certificate request" ) ); ssl->state++; @@ -2828,7 +2819,7 @@ static int ssl_write_certificate_request( mbedtls_ssl_context *ssl ) MBEDTLS_SSL_DEBUG_MSG( 1, ( "should never happen" ) ); return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); } -#else +#else /* !MBEDTLS_KEY_EXCHANGE__CERT_REQ_ALLOWED__ENABLED */ static int ssl_write_certificate_request( mbedtls_ssl_context *ssl ) { int ret = MBEDTLS_ERR_SSL_FEATURE_UNAVAILABLE; @@ -2852,11 +2843,7 @@ static int ssl_write_certificate_request( mbedtls_ssl_context *ssl ) #endif authmode = ssl->conf->authmode; - if( ciphersuite_info->key_exchange == MBEDTLS_KEY_EXCHANGE_PSK || - ciphersuite_info->key_exchange == MBEDTLS_KEY_EXCHANGE_RSA_PSK || - ciphersuite_info->key_exchange == MBEDTLS_KEY_EXCHANGE_DHE_PSK || - ciphersuite_info->key_exchange == MBEDTLS_KEY_EXCHANGE_ECDHE_PSK || - ciphersuite_info->key_exchange == MBEDTLS_KEY_EXCHANGE_ECJPAKE || + if( !mbedtls_ssl_ciphersuite_cert_req_allowed( ciphersuite_info ) || authmode == MBEDTLS_SSL_VERIFY_NONE ) { MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= skip write certificate request" ) ); @@ -2995,12 +2982,7 @@ static int ssl_write_certificate_request( mbedtls_ssl_context *ssl ) return( ret ); } -#endif /* !MBEDTLS_KEY_EXCHANGE_RSA_ENABLED && - !MBEDTLS_KEY_EXCHANGE_DHE_RSA_ENABLED && - !MBEDTLS_KEY_EXCHANGE_ECDH_RSA_ENABLED && - !MBEDTLS_KEY_EXCHANGE_ECDHE_RSA_ENABLED && - !MBEDTLS_KEY_EXCHANGE_ECDH_ECDSA_ENABLED && - !MBEDTLS_KEY_EXCHANGE_ECDHE_ECDSA_ENABLED */ +#endif /* MBEDTLS_KEY_EXCHANGE__CERT_REQ_ALLOWED__ENABLED */ #if defined(MBEDTLS_KEY_EXCHANGE_ECDH_RSA_ENABLED) || \ defined(MBEDTLS_KEY_EXCHANGE_ECDH_ECDSA_ENABLED) @@ -4147,12 +4129,7 @@ static int ssl_parse_client_key_exchange( mbedtls_ssl_context *ssl ) return( 0 ); } -#if !defined(MBEDTLS_KEY_EXCHANGE_RSA_ENABLED) && \ - !defined(MBEDTLS_KEY_EXCHANGE_DHE_RSA_ENABLED) && \ - !defined(MBEDTLS_KEY_EXCHANGE_ECDH_RSA_ENABLED) && \ - !defined(MBEDTLS_KEY_EXCHANGE_ECDHE_RSA_ENABLED) && \ - !defined(MBEDTLS_KEY_EXCHANGE_ECDH_ECDSA_ENABLED)&& \ - !defined(MBEDTLS_KEY_EXCHANGE_ECDHE_ECDSA_ENABLED) +#if !defined(MBEDTLS_KEY_EXCHANGE__CERT_REQ_ALLOWED__ENABLED) static int ssl_parse_certificate_verify( mbedtls_ssl_context *ssl ) { const mbedtls_ssl_ciphersuite_t *ciphersuite_info = @@ -4160,11 +4137,7 @@ static int ssl_parse_certificate_verify( mbedtls_ssl_context *ssl ) MBEDTLS_SSL_DEBUG_MSG( 2, ( "=> parse certificate verify" ) ); - if( ciphersuite_info->key_exchange == MBEDTLS_KEY_EXCHANGE_PSK || - ciphersuite_info->key_exchange == MBEDTLS_KEY_EXCHANGE_RSA_PSK || - ciphersuite_info->key_exchange == MBEDTLS_KEY_EXCHANGE_ECDHE_PSK || - ciphersuite_info->key_exchange == MBEDTLS_KEY_EXCHANGE_DHE_PSK || - ciphersuite_info->key_exchange == MBEDTLS_KEY_EXCHANGE_ECJPAKE ) + if( !mbedtls_ssl_ciphersuite_cert_req_allowed( ciphersuite_info ) ) { MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= skip parse certificate verify" ) ); ssl->state++; @@ -4174,7 +4147,7 @@ static int ssl_parse_certificate_verify( mbedtls_ssl_context *ssl ) MBEDTLS_SSL_DEBUG_MSG( 1, ( "should never happen" ) ); return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); } -#else +#else /* !MBEDTLS_KEY_EXCHANGE__CERT_REQ_ALLOWED__ENABLED */ static int ssl_parse_certificate_verify( mbedtls_ssl_context *ssl ) { int ret = MBEDTLS_ERR_SSL_FEATURE_UNAVAILABLE; @@ -4191,11 +4164,7 @@ static int ssl_parse_certificate_verify( mbedtls_ssl_context *ssl ) MBEDTLS_SSL_DEBUG_MSG( 2, ( "=> parse certificate verify" ) ); - if( ciphersuite_info->key_exchange == MBEDTLS_KEY_EXCHANGE_PSK || - ciphersuite_info->key_exchange == MBEDTLS_KEY_EXCHANGE_RSA_PSK || - ciphersuite_info->key_exchange == MBEDTLS_KEY_EXCHANGE_ECDHE_PSK || - ciphersuite_info->key_exchange == MBEDTLS_KEY_EXCHANGE_DHE_PSK || - ciphersuite_info->key_exchange == MBEDTLS_KEY_EXCHANGE_ECJPAKE || + if( !mbedtls_ssl_ciphersuite_cert_req_allowed( ciphersuite_info ) || ssl->session_negotiate->peer_cert == NULL ) { MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= skip parse certificate verify" ) ); @@ -4343,12 +4312,7 @@ static int ssl_parse_certificate_verify( mbedtls_ssl_context *ssl ) return( ret ); } -#endif /* !MBEDTLS_KEY_EXCHANGE_RSA_ENABLED && - !MBEDTLS_KEY_EXCHANGE_DHE_RSA_ENABLED && - !MBEDTLS_KEY_EXCHANGE_ECDH_RSA_ENABLED && - !MBEDTLS_KEY_EXCHANGE_ECDHE_RSA_ENABLED && - !MBEDTLS_KEY_EXCHANGE_ECDH_ECDSA_ENABLED && - !MBEDTLS_KEY_EXCHANGE_ECDHE_ECDSA_ENABLED */ +#endif /* MBEDTLS_KEY_EXCHANGE__CERT_REQ_ALLOWED__ENABLED */ #if defined(MBEDTLS_SSL_SESSION_TICKETS) static int ssl_write_new_session_ticket( mbedtls_ssl_context *ssl ) From a7c1df63216d0a6d38f416e066ed64a5e375d2c3 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 5 Feb 2019 14:35:46 +0000 Subject: [PATCH 12/67] Don't progress TLS state machine on peer CRT chain parsing error --- library/ssl_tls.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index e3663fc92..ff38eb046 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -6453,7 +6453,7 @@ int mbedtls_ssl_parse_certificate( mbedtls_ssl_context *ssl ) ssl_clear_peer_cert( ssl->session_negotiate ); if( ( ret = ssl_parse_certificate_chain( ssl ) ) != 0 ) - goto exit; + return( ret ); #if defined(MBEDTLS_SSL__ECP_RESTARTABLE) if( ssl->handshake->ecrs_enabled) From 3cf5061091c5ffb5d545260fa57c926b6eaf76c8 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 5 Feb 2019 14:36:34 +0000 Subject: [PATCH 13/67] Introduce helper function for peer CRT chain verification --- library/ssl_tls.c | 285 ++++++++++++++++++++++++---------------------- 1 file changed, 150 insertions(+), 135 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index ff38eb046..52c207553 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -6395,11 +6395,155 @@ static int ssl_parse_certificate_coordinate( mbedtls_ssl_context *ssl, return( SSL_CERTIFICATE_EXPECTED ); } -int mbedtls_ssl_parse_certificate( mbedtls_ssl_context *ssl ) +static int ssl_parse_certificate_verify( mbedtls_ssl_context *ssl, + int authmode, + mbedtls_x509_crt *chain, + void *rs_ctx ) { int ret = 0; const mbedtls_ssl_ciphersuite_t *ciphersuite_info = ssl->handshake->ciphersuite_info; + mbedtls_x509_crt *ca_chain; + mbedtls_x509_crl *ca_crl; + + if( authmode == MBEDTLS_SSL_VERIFY_NONE ) + return( 0 ); + +#if defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) + if( ssl->handshake->sni_ca_chain != NULL ) + { + ca_chain = ssl->handshake->sni_ca_chain; + ca_crl = ssl->handshake->sni_ca_crl; + } + else +#endif + { + ca_chain = ssl->conf->ca_chain; + ca_crl = ssl->conf->ca_crl; + } + + /* + * Main check: verify certificate + */ + ret = mbedtls_x509_crt_verify_restartable( + chain, + ca_chain, ca_crl, + ssl->conf->cert_profile, + ssl->hostname, + &ssl->session_negotiate->verify_result, + ssl->conf->f_vrfy, ssl->conf->p_vrfy, rs_ctx ); + + if( ret != 0 ) + { + MBEDTLS_SSL_DEBUG_RET( 1, "x509_verify_cert", ret ); + } + +#if defined(MBEDTLS_SSL__ECP_RESTARTABLE) + if( ret == MBEDTLS_ERR_ECP_IN_PROGRESS ) + return( MBEDTLS_ERR_SSL_CRYPTO_IN_PROGRESS ); +#endif + + /* + * Secondary checks: always done, but change 'ret' only if it was 0 + */ + +#if defined(MBEDTLS_ECP_C) + { + const mbedtls_pk_context *pk = &chain->pk; + + /* If certificate uses an EC key, make sure the curve is OK */ + if( mbedtls_pk_can_do( pk, MBEDTLS_PK_ECKEY ) && + mbedtls_ssl_check_curve( ssl, mbedtls_pk_ec( *pk )->grp.id ) != 0 ) + { + ssl->session_negotiate->verify_result |= MBEDTLS_X509_BADCERT_BAD_KEY; + + MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad certificate (EC key curve)" ) ); + if( ret == 0 ) + ret = MBEDTLS_ERR_SSL_BAD_HS_CERTIFICATE; + } + } +#endif /* MBEDTLS_ECP_C */ + + if( mbedtls_ssl_check_cert_usage( chain, + ciphersuite_info, + ! ssl->conf->endpoint, + &ssl->session_negotiate->verify_result ) != 0 ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad certificate (usage extensions)" ) ); + if( ret == 0 ) + ret = MBEDTLS_ERR_SSL_BAD_HS_CERTIFICATE; + } + + /* mbedtls_x509_crt_verify_with_profile is supposed to report a + * verification failure through MBEDTLS_ERR_X509_CERT_VERIFY_FAILED, + * with details encoded in the verification flags. All other kinds + * of error codes, including those from the user provided f_vrfy + * functions, are treated as fatal and lead to a failure of + * ssl_parse_certificate even if verification was optional. */ + if( authmode == MBEDTLS_SSL_VERIFY_OPTIONAL && + ( ret == MBEDTLS_ERR_X509_CERT_VERIFY_FAILED || + ret == MBEDTLS_ERR_SSL_BAD_HS_CERTIFICATE ) ) + { + ret = 0; + } + + if( ca_chain == NULL && authmode == MBEDTLS_SSL_VERIFY_REQUIRED ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "got no CA chain" ) ); + ret = MBEDTLS_ERR_SSL_CA_CHAIN_REQUIRED; + } + + if( ret != 0 ) + { + uint8_t alert; + + /* The certificate may have been rejected for several reasons. + Pick one and send the corresponding alert. Which alert to send + may be a subject of debate in some cases. */ + if( ssl->session_negotiate->verify_result & MBEDTLS_X509_BADCERT_OTHER ) + alert = MBEDTLS_SSL_ALERT_MSG_ACCESS_DENIED; + else if( ssl->session_negotiate->verify_result & MBEDTLS_X509_BADCERT_CN_MISMATCH ) + alert = MBEDTLS_SSL_ALERT_MSG_BAD_CERT; + else if( ssl->session_negotiate->verify_result & MBEDTLS_X509_BADCERT_KEY_USAGE ) + alert = MBEDTLS_SSL_ALERT_MSG_UNSUPPORTED_CERT; + else if( ssl->session_negotiate->verify_result & MBEDTLS_X509_BADCERT_EXT_KEY_USAGE ) + alert = MBEDTLS_SSL_ALERT_MSG_UNSUPPORTED_CERT; + else if( ssl->session_negotiate->verify_result & MBEDTLS_X509_BADCERT_NS_CERT_TYPE ) + alert = MBEDTLS_SSL_ALERT_MSG_UNSUPPORTED_CERT; + else if( ssl->session_negotiate->verify_result & MBEDTLS_X509_BADCERT_BAD_PK ) + alert = MBEDTLS_SSL_ALERT_MSG_UNSUPPORTED_CERT; + else if( ssl->session_negotiate->verify_result & MBEDTLS_X509_BADCERT_BAD_KEY ) + alert = MBEDTLS_SSL_ALERT_MSG_UNSUPPORTED_CERT; + else if( ssl->session_negotiate->verify_result & MBEDTLS_X509_BADCERT_EXPIRED ) + alert = MBEDTLS_SSL_ALERT_MSG_CERT_EXPIRED; + else if( ssl->session_negotiate->verify_result & MBEDTLS_X509_BADCERT_REVOKED ) + alert = MBEDTLS_SSL_ALERT_MSG_CERT_REVOKED; + else if( ssl->session_negotiate->verify_result & MBEDTLS_X509_BADCERT_NOT_TRUSTED ) + alert = MBEDTLS_SSL_ALERT_MSG_UNKNOWN_CA; + else + alert = MBEDTLS_SSL_ALERT_MSG_CERT_UNKNOWN; + mbedtls_ssl_send_alert_message( ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, + alert ); + } + +#if defined(MBEDTLS_DEBUG_C) + if( ssl->session_negotiate->verify_result != 0 ) + { + MBEDTLS_SSL_DEBUG_MSG( 3, ( "! Certificate verification flags %x", + ssl->session_negotiate->verify_result ) ); + } + else + { + MBEDTLS_SSL_DEBUG_MSG( 3, ( "Certificate verification flags clear" ) ); + } +#endif /* MBEDTLS_DEBUG_C */ + + return( ret ); +} + +int mbedtls_ssl_parse_certificate( mbedtls_ssl_context *ssl ) +{ + int ret = 0; int crt_expected; #if defined(MBEDTLS_SSL_SRV_C) && defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) const int authmode = ssl->handshake->sni_authmode != MBEDTLS_SSL_VERIFY_UNSET @@ -6464,140 +6608,11 @@ crt_verify: rs_ctx = &ssl->handshake->ecrs_ctx; #endif - if( authmode != MBEDTLS_SSL_VERIFY_NONE ) - { - mbedtls_x509_crt *ca_chain; - mbedtls_x509_crl *ca_crl; - -#if defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) - if( ssl->handshake->sni_ca_chain != NULL ) - { - ca_chain = ssl->handshake->sni_ca_chain; - ca_crl = ssl->handshake->sni_ca_crl; - } - else -#endif - { - ca_chain = ssl->conf->ca_chain; - ca_crl = ssl->conf->ca_crl; - } - - /* - * Main check: verify certificate - */ - ret = mbedtls_x509_crt_verify_restartable( - ssl->session_negotiate->peer_cert, - ca_chain, ca_crl, - ssl->conf->cert_profile, - ssl->hostname, - &ssl->session_negotiate->verify_result, - ssl->conf->f_vrfy, ssl->conf->p_vrfy, rs_ctx ); - - if( ret != 0 ) - { - MBEDTLS_SSL_DEBUG_RET( 1, "x509_verify_cert", ret ); - } - -#if defined(MBEDTLS_SSL__ECP_RESTARTABLE) - if( ret == MBEDTLS_ERR_ECP_IN_PROGRESS ) - return( MBEDTLS_ERR_SSL_CRYPTO_IN_PROGRESS ); -#endif - - /* - * Secondary checks: always done, but change 'ret' only if it was 0 - */ - -#if defined(MBEDTLS_ECP_C) - { - const mbedtls_pk_context *pk = &ssl->session_negotiate->peer_cert->pk; - - /* If certificate uses an EC key, make sure the curve is OK */ - if( mbedtls_pk_can_do( pk, MBEDTLS_PK_ECKEY ) && - mbedtls_ssl_check_curve( ssl, mbedtls_pk_ec( *pk )->grp.id ) != 0 ) - { - ssl->session_negotiate->verify_result |= MBEDTLS_X509_BADCERT_BAD_KEY; - - MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad certificate (EC key curve)" ) ); - if( ret == 0 ) - ret = MBEDTLS_ERR_SSL_BAD_HS_CERTIFICATE; - } - } -#endif /* MBEDTLS_ECP_C */ - - if( mbedtls_ssl_check_cert_usage( ssl->session_negotiate->peer_cert, - ciphersuite_info, - ! ssl->conf->endpoint, - &ssl->session_negotiate->verify_result ) != 0 ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad certificate (usage extensions)" ) ); - if( ret == 0 ) - ret = MBEDTLS_ERR_SSL_BAD_HS_CERTIFICATE; - } - - /* mbedtls_x509_crt_verify_with_profile is supposed to report a - * verification failure through MBEDTLS_ERR_X509_CERT_VERIFY_FAILED, - * with details encoded in the verification flags. All other kinds - * of error codes, including those from the user provided f_vrfy - * functions, are treated as fatal and lead to a failure of - * ssl_parse_certificate even if verification was optional. */ - if( authmode == MBEDTLS_SSL_VERIFY_OPTIONAL && - ( ret == MBEDTLS_ERR_X509_CERT_VERIFY_FAILED || - ret == MBEDTLS_ERR_SSL_BAD_HS_CERTIFICATE ) ) - { - ret = 0; - } - - if( ca_chain == NULL && authmode == MBEDTLS_SSL_VERIFY_REQUIRED ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "got no CA chain" ) ); - ret = MBEDTLS_ERR_SSL_CA_CHAIN_REQUIRED; - } - - if( ret != 0 ) - { - uint8_t alert; - - /* The certificate may have been rejected for several reasons. - Pick one and send the corresponding alert. Which alert to send - may be a subject of debate in some cases. */ - if( ssl->session_negotiate->verify_result & MBEDTLS_X509_BADCERT_OTHER ) - alert = MBEDTLS_SSL_ALERT_MSG_ACCESS_DENIED; - else if( ssl->session_negotiate->verify_result & MBEDTLS_X509_BADCERT_CN_MISMATCH ) - alert = MBEDTLS_SSL_ALERT_MSG_BAD_CERT; - else if( ssl->session_negotiate->verify_result & MBEDTLS_X509_BADCERT_KEY_USAGE ) - alert = MBEDTLS_SSL_ALERT_MSG_UNSUPPORTED_CERT; - else if( ssl->session_negotiate->verify_result & MBEDTLS_X509_BADCERT_EXT_KEY_USAGE ) - alert = MBEDTLS_SSL_ALERT_MSG_UNSUPPORTED_CERT; - else if( ssl->session_negotiate->verify_result & MBEDTLS_X509_BADCERT_NS_CERT_TYPE ) - alert = MBEDTLS_SSL_ALERT_MSG_UNSUPPORTED_CERT; - else if( ssl->session_negotiate->verify_result & MBEDTLS_X509_BADCERT_BAD_PK ) - alert = MBEDTLS_SSL_ALERT_MSG_UNSUPPORTED_CERT; - else if( ssl->session_negotiate->verify_result & MBEDTLS_X509_BADCERT_BAD_KEY ) - alert = MBEDTLS_SSL_ALERT_MSG_UNSUPPORTED_CERT; - else if( ssl->session_negotiate->verify_result & MBEDTLS_X509_BADCERT_EXPIRED ) - alert = MBEDTLS_SSL_ALERT_MSG_CERT_EXPIRED; - else if( ssl->session_negotiate->verify_result & MBEDTLS_X509_BADCERT_REVOKED ) - alert = MBEDTLS_SSL_ALERT_MSG_CERT_REVOKED; - else if( ssl->session_negotiate->verify_result & MBEDTLS_X509_BADCERT_NOT_TRUSTED ) - alert = MBEDTLS_SSL_ALERT_MSG_UNKNOWN_CA; - else - alert = MBEDTLS_SSL_ALERT_MSG_CERT_UNKNOWN; - mbedtls_ssl_send_alert_message( ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, - alert ); - } - -#if defined(MBEDTLS_DEBUG_C) - if( ssl->session_negotiate->verify_result != 0 ) - { - MBEDTLS_SSL_DEBUG_MSG( 3, ( "! Certificate verification flags %x", - ssl->session_negotiate->verify_result ) ); - } - else - { - MBEDTLS_SSL_DEBUG_MSG( 3, ( "Certificate verification flags clear" ) ); - } -#endif /* MBEDTLS_DEBUG_C */ - } + ret = ssl_parse_certificate_verify( ssl, authmode, + ssl->session_negotiate->peer_cert, + rs_ctx ); + if( ret != 0 ) + return( ret ); MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= parse certificate" ) ); From 35e41771fec50350f5a4d10007d5aa7427628018 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 5 Feb 2019 15:37:23 +0000 Subject: [PATCH 14/67] Allow passing any X.509 CRT chain to ssl_parse_certificate_chain() This commit modifies the helper `ssl_parse_certificate_chain()` to accep any target X.509 CRT chain instead of hardcoding it to `session_negotiate->peer_cert`. This increases modularity and paves the way towards removing `mbedtls_ssl_session::peer_cert`. --- library/ssl_tls.c | 88 ++++++++++++++++++++++------------------------- 1 file changed, 41 insertions(+), 47 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 52c207553..bce2f260f 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -6154,9 +6154,13 @@ static void ssl_clear_peer_cert( mbedtls_ssl_session *session ) * Once the certificate message is read, parse it into a cert chain and * perform basic checks, but leave actual verification to the caller */ -static int ssl_parse_certificate_chain( mbedtls_ssl_context *ssl ) +static int ssl_parse_certificate_chain( mbedtls_ssl_context *ssl, + mbedtls_x509_crt *chain ) { - int ret, crt_cnt=0; + int ret; +#if defined(MBEDTLS_SSL_RENEGOTIATION) && defined(MBEDTLS_SSL_CLI_C) + int crt_cnt=0; +#endif size_t i, n; uint8_t alert; @@ -6233,58 +6237,34 @@ static int ssl_parse_certificate_chain( mbedtls_ssl_context *ssl ) } /* Check if we're handling the first CRT in the chain. */ - if( crt_cnt++ == 0 ) +#if defined(MBEDTLS_SSL_RENEGOTIATION) && defined(MBEDTLS_SSL_CLI_C) + if( crt_cnt++ == 0 && + ssl->conf->endpoint == MBEDTLS_SSL_IS_CLIENT && + ssl->renego_status == MBEDTLS_SSL_RENEGOTIATION_IN_PROGRESS ) { /* 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 ) + 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( 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 ); - } - - /* Now we can safely free the original chain. */ - ssl_clear_peer_cert( ssl->session ); - - /* Intentional fallthrough. */ + 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 ); } + + /* Now we can safely free the original chain. */ + ssl_clear_peer_cert( ssl->session ); + } #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 ); + ret = mbedtls_x509_crt_parse_der( chain, ssl->in_msg + i, n ); switch( ret ) { case 0: /*ok*/ @@ -6312,7 +6292,7 @@ static int ssl_parse_certificate_chain( mbedtls_ssl_context *ssl ) i += n; } - MBEDTLS_SSL_DEBUG_CRT( 3, "peer certificate", ssl->session_negotiate->peer_cert ); + MBEDTLS_SSL_DEBUG_CRT( 3, "peer certificate", chain ); return( 0 ); } @@ -6593,10 +6573,24 @@ int mbedtls_ssl_parse_certificate( mbedtls_ssl_context *ssl ) } #endif /* MBEDTLS_SSL_SRV_C */ - /* In case we tried to reuse a session but it failed. */ + /* Clear existing peer CRT structure in case we tried to + * reuse a session but it failed, and allocate a new one. */ ssl_clear_peer_cert( ssl->session_negotiate ); + 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 ); - if( ( ret = ssl_parse_certificate_chain( ssl ) ) != 0 ) + ret = ssl_parse_certificate_chain( ssl, ssl->session_negotiate->peer_cert ); + if( ret != 0 ) return( ret ); #if defined(MBEDTLS_SSL__ECP_RESTARTABLE) From 58fccf2f624e4c1b836ac7c44e99299cfb457c49 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 6 Feb 2019 14:30:46 +0000 Subject: [PATCH 15/67] Give ssl_session_copy() external linkage A subsequent commit will need this function in the session ticket and session cache implementations. As the latter are server-side, this commit also removes the MBEDTLS_SSL_CLI_C guard. For now, the function is declared in ssl_internal.h and hence not part of the public API. --- include/mbedtls/ssl_internal.h | 3 +++ library/ssl_tls.c | 10 +++++----- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/include/mbedtls/ssl_internal.h b/include/mbedtls/ssl_internal.h index 8089441c4..ee79504db 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -989,6 +989,9 @@ int mbedtls_ssl_dtls_replay_check( mbedtls_ssl_context *ssl ); void mbedtls_ssl_dtls_replay_update( mbedtls_ssl_context *ssl ); #endif +int mbedtls_ssl_session_copy( mbedtls_ssl_session *dst, + const mbedtls_ssl_session *src ); + /* constant-time buffer comparison */ static inline int mbedtls_ssl_safer_memcmp( const void *a, const void *b, size_t n ) { diff --git a/library/ssl_tls.c b/library/ssl_tls.c index bce2f260f..4d7c624e3 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -368,8 +368,8 @@ static unsigned int ssl_mfl_code_to_length( int mfl ) } #endif /* MBEDTLS_SSL_MAX_FRAGMENT_LENGTH */ -#if defined(MBEDTLS_SSL_CLI_C) -static int ssl_session_copy( mbedtls_ssl_session *dst, const mbedtls_ssl_session *src ) +int mbedtls_ssl_session_copy( mbedtls_ssl_session *dst, + const mbedtls_ssl_session *src ) { mbedtls_ssl_session_free( dst ); memcpy( dst, src, sizeof( mbedtls_ssl_session ) ); @@ -408,7 +408,6 @@ static int ssl_session_copy( mbedtls_ssl_session *dst, const mbedtls_ssl_session return( 0 ); } -#endif /* MBEDTLS_SSL_CLI_C */ #if defined(MBEDTLS_SSL_HW_RECORD_ACCEL) int (*mbedtls_ssl_hw_record_init)( mbedtls_ssl_context *ssl, @@ -7979,7 +7978,8 @@ int mbedtls_ssl_set_session( mbedtls_ssl_context *ssl, const mbedtls_ssl_session return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); } - if( ( ret = ssl_session_copy( ssl->session_negotiate, session ) ) != 0 ) + if( ( ret = mbedtls_ssl_session_copy( ssl->session_negotiate, + session ) ) != 0 ) return( ret ); ssl->handshake->resume = 1; @@ -8849,7 +8849,7 @@ int mbedtls_ssl_get_session( const mbedtls_ssl_context *ssl, return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); } - return( ssl_session_copy( dst, ssl->session ) ); + return( mbedtls_ssl_session_copy( dst, ssl->session ) ); } #endif /* MBEDTLS_SSL_CLI_C */ From a177b38618ef5e0abb9fc87973155b091250ed3e Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 6 Feb 2019 14:53:19 +0000 Subject: [PATCH 16/67] Simplify session cache implementation via mbedtls_ssl_session_copy() --- library/ssl_cache.c | 46 +++++++++++++++++++++++++++++++++------------ 1 file changed, 34 insertions(+), 12 deletions(-) diff --git a/library/ssl_cache.c b/library/ssl_cache.c index 47867f132..f5425944e 100644 --- a/library/ssl_cache.c +++ b/library/ssl_cache.c @@ -40,6 +40,7 @@ #endif #include "mbedtls/ssl_cache.h" +#include "mbedtls/ssl_internal.h" #include @@ -92,9 +93,12 @@ int mbedtls_ssl_cache_get( void *data, mbedtls_ssl_session *session ) entry->session.id_len ) != 0 ) continue; - memcpy( session->master, entry->session.master, 48 ); - - session->verify_result = entry->session.verify_result; + ret = mbedtls_ssl_session_copy( session, &entry->session ); + if( ret != 0 ) + { + ret = 1; + goto exit; + } #if defined(MBEDTLS_X509_CRT_PARSE_C) /* @@ -102,6 +106,10 @@ int mbedtls_ssl_cache_get( void *data, mbedtls_ssl_session *session ) */ if( entry->peer_cert.p != NULL ) { + /* `session->peer_cert` is NULL after the call to + * mbedtls_ssl_session_copy(), because cache entries + * have the `peer_cert` field set to NULL. */ + if( ( session->peer_cert = mbedtls_calloc( 1, sizeof(mbedtls_x509_crt) ) ) == NULL ) { @@ -239,8 +247,6 @@ int mbedtls_ssl_cache_set( void *data, const mbedtls_ssl_session *session ) #endif } - memcpy( &cur->session, session, sizeof( mbedtls_ssl_session ) ); - #if defined(MBEDTLS_X509_CRT_PARSE_C) /* * If we're reusing an entry, free its certificate first @@ -250,23 +256,39 @@ int mbedtls_ssl_cache_set( void *data, const mbedtls_ssl_session *session ) mbedtls_free( cur->peer_cert.p ); memset( &cur->peer_cert, 0, sizeof(mbedtls_x509_buf) ); } +#endif /* MBEDTLS_X509_CRT_PARSE_C */ - /* - * Store peer certificate - */ - if( session->peer_cert != NULL ) + /* Copy the entire session; this temporarily makes a copy of the + * X.509 CRT structure even though we only want to store the raw CRT. + * This inefficiency will go away as soon as we implement on-demand + * parsing of CRTs, in which case there's no need for the `peer_cert` + * field anymore in the first place, and we're done after this call. */ + ret = mbedtls_ssl_session_copy( &cur->session, session ); + if( ret != 0 ) { - cur->peer_cert.p = mbedtls_calloc( 1, session->peer_cert->raw.len ); + ret = 1; + goto exit; + } + +#if defined(MBEDTLS_X509_CRT_PARSE_C) + /* If present, free the X.509 structure and only store the raw CRT data. */ + if( cur->session.peer_cert != NULL ) + { + cur->peer_cert.p = + mbedtls_calloc( 1, cur->session.peer_cert->raw.len ); if( cur->peer_cert.p == NULL ) { ret = 1; goto exit; } - memcpy( cur->peer_cert.p, session->peer_cert->raw.p, - session->peer_cert->raw.len ); + memcpy( cur->peer_cert.p, + cur->session.peer_cert->raw.p, + cur->session.peer_cert->raw.len ); cur->peer_cert.len = session->peer_cert->raw.len; + mbedtls_x509_crt_free( cur->session.peer_cert ); + mbedtls_free( cur->session.peer_cert ); cur->session.peer_cert = NULL; } #endif /* MBEDTLS_X509_CRT_PARSE_C */ From f02d5501d82e4888e67778100d72c657382aa813 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 6 Feb 2019 17:37:32 +0000 Subject: [PATCH 17/67] Re-classify errors on missing peer CRT mbedtls_ssl_parse_certificate() will fail if a ciphersuite requires a certificate, but none is provided. While it is sensible to double- check this, failure should be reported as an internal error and not as an unexpected message. --- library/ssl_cli.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/library/ssl_cli.c b/library/ssl_cli.c index b47f3dfa0..b317b2d4d 100644 --- a/library/ssl_cli.c +++ b/library/ssl_cli.c @@ -2315,8 +2315,8 @@ static int ssl_write_encrypted_pms( mbedtls_ssl_context *ssl, if( ssl->session_negotiate->peer_cert == NULL ) { - MBEDTLS_SSL_DEBUG_MSG( 2, ( "certificate required" ) ); - return( MBEDTLS_ERR_SSL_UNEXPECTED_MESSAGE ); + /* Should never happen */ + return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); } /* @@ -2427,8 +2427,8 @@ static int ssl_get_ecdh_params_from_cert( mbedtls_ssl_context *ssl ) if( ssl->session_negotiate->peer_cert == NULL ) { - MBEDTLS_SSL_DEBUG_MSG( 2, ( "certificate required" ) ); - return( MBEDTLS_ERR_SSL_UNEXPECTED_MESSAGE ); + /* Should never happen */ + return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); } if( ! mbedtls_pk_can_do( &ssl->session_negotiate->peer_cert->pk, @@ -2749,10 +2749,8 @@ start_processing: if( ssl->session_negotiate->peer_cert == NULL ) { - MBEDTLS_SSL_DEBUG_MSG( 2, ( "certificate required" ) ); - mbedtls_ssl_send_alert_message( ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, - MBEDTLS_SSL_ALERT_MSG_HANDSHAKE_FAILURE ); - return( MBEDTLS_ERR_SSL_UNEXPECTED_MESSAGE ); + /* Should never happen */ + return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); } /* From 869144b3e9627ec06095ebc6d7c7bd7ca4660f58 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 5 Feb 2019 11:33:12 +0000 Subject: [PATCH 18/67] Improve documentation of mbedtls_ssl_get_peer_cert() --- include/mbedtls/ssl.h | 34 +++++++++++++++++++++++++--------- 1 file changed, 25 insertions(+), 9 deletions(-) diff --git a/include/mbedtls/ssl.h b/include/mbedtls/ssl.h index 6e1bb3ae6..5915eaecb 100644 --- a/include/mbedtls/ssl.h +++ b/include/mbedtls/ssl.h @@ -3230,18 +3230,34 @@ int mbedtls_ssl_get_max_out_record_payload( const mbedtls_ssl_context *ssl ); #if defined(MBEDTLS_X509_CRT_PARSE_C) /** - * \brief Return the peer certificate from the current connection + * \brief Return the peer certificate from the current connection. * - * Note: Can be NULL in case no certificate was sent during - * the handshake. Different calls for the same connection can - * return the same or different pointers for the same - * certificate and even a different certificate altogether. - * The peer cert CAN change in a single connection if - * renegotiation is performed. + * For ciphersuites not using certificate-based peer + * authentication (such as PSK-based ciphersuites), no + * peer certificate is available, and this function returns + * \c NULL. * - * \param ssl SSL context + * \param ssl The SSL context to use. This must be initialized and setup. * - * \return the current peer certificate + * \return The current peer certificate, or \c NULL if + * none is available. It is owned by the SSL context + * and valid only until the next call to the SSL API. + * + * \note For one-time inspection of the peer's certificate during + * the handshake, consider registering an X.509 CRT verification + * callback through mbedtls_ssl_conf_verify() instead of calling + * this function. Using mbedtls_ssl_conf_verify() also comes at + * the benefit of allowing you to influence the verification + * process, for example by masking expected and tolerated + * verification failures. + * + * \warning You must not use the pointer returned by this function + * after any further call to the SSL API, including + * mbedtls_ssl_read() and mbedtls_ssl_write(); this is + * because the pointer might change during renegotiation, + * which happens transparently to the user. + * If you want to use the certificate across API calls, + * you must make a copy. */ const mbedtls_x509_crt *mbedtls_ssl_get_peer_cert( const mbedtls_ssl_context *ssl ); #endif /* MBEDTLS_X509_CRT_PARSE_C */ From b90f655a784ee70f4759a8e0ea3e20a5f0cfd2d6 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 5 Feb 2019 17:04:00 +0000 Subject: [PATCH 19/67] Add configuration option to remove peer CRT after handshake --- include/mbedtls/config.h | 22 ++++++++++++++++++++++ include/mbedtls/ssl.h | 8 ++++++-- 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/include/mbedtls/config.h b/include/mbedtls/config.h index 9fc512eec..0ea17fb74 100644 --- a/include/mbedtls/config.h +++ b/include/mbedtls/config.h @@ -1404,6 +1404,28 @@ */ #define MBEDTLS_SSL_FALLBACK_SCSV +/** + * \def MBEDTLS_SSL_KEEP_PEER_CERTIFICATE + * + * This option controls the presence of the API mbedtls_ssl_get_peer_cert() + * giving access to the peer's certificate after completion of the handshake. + * + * Unless you need mbedtls_ssl_peer_cert() in your application, it is + * recommended to disable this option for reduced RAM usage. + * + * \note If this option is disabled, mbedtls_ssl_get_peer_cert() is still + * defined, but always returns \c NULL. + * + * \note This option has no influence on the protection against the + * triple handshake attack. Even if it is disabled, Mbed TLS will + * still ensure that certificates do not change during renegotiation, + * for exaple by keeping a hash of the peer's certificate. + * + * Comment this macro to disable storing the peer's certificate + * after the handshake. + */ +#define MBEDTLS_SSL_KEEP_PEER_CERTIFICATE + /** * \def MBEDTLS_SSL_HW_RECORD_ACCEL * diff --git a/include/mbedtls/ssl.h b/include/mbedtls/ssl.h index 5915eaecb..9ae3ee1fe 100644 --- a/include/mbedtls/ssl.h +++ b/include/mbedtls/ssl.h @@ -3240,8 +3240,12 @@ int mbedtls_ssl_get_max_out_record_payload( const mbedtls_ssl_context *ssl ); * \param ssl The SSL context to use. This must be initialized and setup. * * \return The current peer certificate, or \c NULL if - * none is available. It is owned by the SSL context - * and valid only until the next call to the SSL API. + * none is available, which might be because the chosen + * ciphersuite does not use peer certificates, or because + * #MBEDTLS_SSL_KEEP_PEER_CERTIFICATE has been disabled. + * If this functions does not return \c NULL, the returned + * certificate is owned by the SSL context and valid only + * until the next call to the SSL API. * * \note For one-time inspection of the peer's certificate during * the handshake, consider registering an X.509 CRT verification From c88289a64d211c27a0b307566ebfffa629077ecb Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 7 Feb 2019 15:13:38 +0000 Subject: [PATCH 20/67] Update version_features.c --- library/version_features.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/library/version_features.c b/library/version_features.c index 245f957e4..5e9d9239b 100644 --- a/library/version_features.c +++ b/library/version_features.c @@ -456,6 +456,9 @@ static const char *features[] = { #if defined(MBEDTLS_SSL_FALLBACK_SCSV) "MBEDTLS_SSL_FALLBACK_SCSV", #endif /* MBEDTLS_SSL_FALLBACK_SCSV */ +#if defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) + "MBEDTLS_SSL_KEEP_PEER_CERTIFICATE", +#endif /* MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ #if defined(MBEDTLS_SSL_HW_RECORD_ACCEL) "MBEDTLS_SSL_HW_RECORD_ACCEL", #endif /* MBEDTLS_SSL_HW_RECORD_ACCEL */ From 9fb6e2e203786096c4bdb9c2c16845fbeb85d5d4 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 5 Feb 2019 17:00:50 +0000 Subject: [PATCH 21/67] Extend mbedtls_ssl_session by buffer holding peer CRT digest --- include/mbedtls/ssl.h | 25 ++++++++++++++++++++++++- library/ssl_tls.c | 27 +++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 1 deletion(-) diff --git a/include/mbedtls/ssl.h b/include/mbedtls/ssl.h index 9ae3ee1fe..5df5eb1b5 100644 --- a/include/mbedtls/ssl.h +++ b/include/mbedtls/ssl.h @@ -812,6 +812,22 @@ typedef int mbedtls_ssl_async_resume_t( mbedtls_ssl_context *ssl, typedef void mbedtls_ssl_async_cancel_t( mbedtls_ssl_context *ssl ); #endif /* MBEDTLS_SSL_ASYNC_PRIVATE */ +#if defined(MBEDTLS_X509_CRT_PARSE_C) +#define MBEDTLS_SSL_PEER_CERT_DIGEST_MAX_LEN 48 +#if defined(MBEDTLS_SHA256_C) +#define MBEDTLS_SSL_PEER_CERT_DIGEST_DFL_TYPE MBEDTLS_MD_SHA256 +#define MBEDTLS_SSL_PEER_CERT_DIGEST_DFL_LEN 32 +#elif defined(MBEDTLS_SHA512_C) +#define MBEDTLS_SSL_PEER_CERT_DIGEST_DFL_TYPE MBEDTLS_MD_SHA384 +#define MBEDTLS_SSL_PEER_CERT_DIGEST_DFL_LEN 48 +#elif defined(MBEDTLS_SHA1_C) +#define MBEDTLS_SSL_PEER_CERT_DIGEST_DFL_TYPE MBEDTLS_MD_SHA1 +#define MBEDTLS_SSL_PEER_CERT_DIGEST_DFL_LEN 20 +#else +#error "Bad configuration - need SHA-1, SHA-256 or SHA-512 enabled to compute digest of peer CRT." +#endif +#endif /* MBEDTLS_X509_CRT_PARSE_C */ + /* * This structure is used for storing current session data. * @@ -835,7 +851,14 @@ struct mbedtls_ssl_session unsigned char master[48]; /*!< the master secret */ #if defined(MBEDTLS_X509_CRT_PARSE_C) - mbedtls_x509_crt *peer_cert; /*!< peer X.509 cert chain */ + mbedtls_x509_crt *peer_cert; /*!< peer X.509 cert chain */ +#if !defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) + /*! The digest of the peer's end-CRT. This must be kept to detect CRT + * changes during renegotiation, mitigating the triple handshake attack. */ + unsigned char *peer_cert_digest; + size_t peer_cert_digest_len; + mbedtls_md_type_t peer_cert_digest_type; +#endif /* MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ #endif /* MBEDTLS_X509_CRT_PARSE_C */ uint32_t verify_result; /*!< verification result */ diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 4d7c624e3..2fa48b6c0 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -393,6 +393,22 @@ int mbedtls_ssl_session_copy( mbedtls_ssl_session *dst, return( ret ); } } + +#if !defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) + if( src->peer_cert_digest != NULL ) + { + dst->peer_cert_digest_len = src->peer_cert_digest_len; + dst->peer_cert_digest = + mbedtls_calloc( 1, dst->peer_cert_digest_len ); + if( dst->peer_cert_digest == NULL ) + return( MBEDTLS_ERR_SSL_ALLOC_FAILED ); + + memcpy( dst->peer_cert_digest, src->peer_cert_digest, + src->peer_cert_digest_len ); + dst->peer_cert_digest_type = src->peer_cert_digest_type; + } +#endif /* MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ + #endif /* MBEDTLS_X509_CRT_PARSE_C */ #if defined(MBEDTLS_SSL_SESSION_TICKETS) && defined(MBEDTLS_SSL_CLI_C) @@ -6147,6 +6163,17 @@ static void ssl_clear_peer_cert( mbedtls_ssl_session *session ) mbedtls_free( session->peer_cert ); session->peer_cert = NULL; } + +#if !defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) + if( session->peer_cert_digest != NULL ) + { + /* Zeroization is not necessary. */ + mbedtls_free( session->peer_cert_digest ); + session->peer_cert_digest = NULL; + session->peer_cert_digest_type = MBEDTLS_MD_NONE; + session->peer_cert_digest_len = 0; + } +#endif /* MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ } /* From 3008d2869f74a306368a1484b58687dd886d6502 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 5 Feb 2019 17:02:28 +0000 Subject: [PATCH 22/67] Compute digest of peer's end-CRT in mbedtls_ssl_parse_certificate() --- library/ssl_tls.c | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 2fa48b6c0..471d3587a 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -6634,6 +6634,33 @@ crt_verify: if( ret != 0 ) return( ret ); +#if !defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) + /* Remember digest of the peer's end-CRT. */ + ssl->session_negotiate->peer_cert_digest = + mbedtls_calloc( 1, MBEDTLS_SSL_PEER_CERT_DIGEST_DFL_LEN ); + if( ssl->session_negotiate->peer_cert_digest == NULL ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "alloc(%d bytes) failed", + sizeof( MBEDTLS_SSL_PEER_CERT_DIGEST_DFL_LEN ) ) ); + mbedtls_ssl_send_alert_message( ssl, + MBEDTLS_SSL_ALERT_LEVEL_FATAL, + MBEDTLS_SSL_ALERT_MSG_INTERNAL_ERROR ); + return( MBEDTLS_ERR_SSL_ALLOC_FAILED ); + } + ret = mbedtls_md( mbedtls_md_info_from_type( + MBEDTLS_SSL_PEER_CERT_DIGEST_DFL_TYPE ), + ssl->session_negotiate->peer_cert->raw.p, + ssl->session_negotiate->peer_cert->raw.len, + ssl->session_negotiate->peer_cert_digest ); + if( ret != 0 ) + return( ret ); + + ssl->session_negotiate->peer_cert_digest_type = + MBEDTLS_SSL_PEER_CERT_DIGEST_DFL_TYPE; + ssl->session_negotiate->peer_cert_digest_len = + MBEDTLS_SSL_PEER_CERT_DIGEST_DFL_LEN; +#endif /* !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ + MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= parse certificate" ) ); exit: From df75938b845ab52b5f96bf82cbe2e18cf4cd6ae2 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 5 Feb 2019 17:02:46 +0000 Subject: [PATCH 23/67] Mitigate triple handshake attack by comparing digests only This paves the way for the removal of the peer CRT chain from `mbedtls_ssl_session`. --- library/ssl_tls.c | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 471d3587a..9e5b40b48 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -6139,6 +6139,8 @@ write_msg: } #if defined(MBEDTLS_SSL_RENEGOTIATION) && defined(MBEDTLS_SSL_CLI_C) + +#if defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) static int ssl_check_peer_crt_unchanged( mbedtls_ssl_context *ssl, unsigned char *crt_buf, size_t crt_buf_len ) @@ -6153,6 +6155,35 @@ static int ssl_check_peer_crt_unchanged( mbedtls_ssl_context *ssl, return( memcmp( peer_crt->raw.p, crt_buf, crt_buf_len ) ); } +#else /* MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ +static int ssl_check_peer_crt_unchanged( mbedtls_ssl_context *ssl, + unsigned char *crt_buf, + size_t crt_buf_len ) +{ + int ret; + unsigned char const * const peer_cert_digest = + ssl->session->peer_cert_digest; + mbedtls_md_type_t const peer_cert_digest_type = + ssl->session->peer_cert_digest_type; + mbedtls_md_info_t const * const digest_info = + mbedtls_md_info_from_type( peer_cert_digest_type ); + unsigned char tmp_digest[MBEDTLS_SSL_PEER_CERT_DIGEST_MAX_LEN]; + size_t digest_len; + + if( peer_cert_digest == NULL || digest_info == NULL ) + return( -1 ); + + digest_len = mbedtls_md_get_size( digest_info ); + if( digest_len > MBEDTLS_SSL_PEER_CERT_DIGEST_MAX_LEN ) + return( -1 ); + + ret = mbedtls_md( digest_info, crt_buf, crt_buf_len, tmp_digest ); + if( ret != 0 ) + return( -1 ); + + return( memcmp( tmp_digest, peer_cert_digest, digest_len ) ); +} +#endif /* MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ #endif /* MBEDTLS_SSL_RENEGOTIATION && MBEDTLS_SSL_CLI_C */ static void ssl_clear_peer_cert( mbedtls_ssl_session *session ) From e4aeb76a2c670128f57f7d1ad1e4e3ec2423ed47 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 5 Feb 2019 17:19:52 +0000 Subject: [PATCH 24/67] Parse and verify peer CRT chain in local variable `mbedtls_ssl_parse_certificate()` parses the peer's certificate chain directly into the `peer_cert` field of the `mbedtls_ssl_session` structure being established. To allow to optionally remove this field from the session structure, this commit changes this to parse the peer's chain into a local variable instead first, which can then either be freed after CRT verification - in case the chain should not be stored - or mapped to the `peer_cert` if it should be kept. For now, only the latter is implemented. --- include/mbedtls/ssl_internal.h | 3 ++ library/ssl_tls.c | 66 ++++++++++++++++++++++++---------- 2 files changed, 51 insertions(+), 18 deletions(-) diff --git a/include/mbedtls/ssl_internal.h b/include/mbedtls/ssl_internal.h index ee79504db..d46895cc6 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -396,6 +396,9 @@ struct mbedtls_ssl_handshake_params ssl_ecrs_cke_ecdh_calc_secret, /*!< ClientKeyExchange: ECDH step 2 */ ssl_ecrs_crt_vrfy_sign, /*!< CertificateVerify: pk_sign() */ } ecrs_state; /*!< current (or last) operation */ +#if defined(MBEDTLS_X509_CRT_PARSE_C) + mbedtls_x509_crt *ecrs_peer_cert; /*!< The peer's CRT chain. */ +#endif /* MBEDTLS_X509_CRT_PARSE_C */ size_t ecrs_n; /*!< place for saving a length */ #endif #if defined(MBEDTLS_SSL_PROTO_DTLS) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 9e5b40b48..c682166f8 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -6590,6 +6590,7 @@ int mbedtls_ssl_parse_certificate( mbedtls_ssl_context *ssl ) const int authmode = ssl->conf->authmode; #endif void *rs_ctx = NULL; + mbedtls_x509_crt *chain = NULL; MBEDTLS_SSL_DEBUG_MSG( 2, ( "=> parse certificate" ) ); @@ -6604,6 +6605,8 @@ int mbedtls_ssl_parse_certificate( mbedtls_ssl_context *ssl ) if( ssl->handshake->ecrs_enabled && ssl->handshake->ecrs_state == ssl_ecrs_crt_verify ) { + chain = ssl->handshake->ecrs_peer_cert; + ssl->handshake->ecrs_peer_cert = NULL; goto crt_verify; } #endif @@ -6613,7 +6616,7 @@ int mbedtls_ssl_parse_certificate( mbedtls_ssl_context *ssl ) /* mbedtls_ssl_read_record may have sent an alert already. We let it decide whether to alert. */ MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_ssl_read_record", ret ); - return( ret ); + goto exit; } #if defined(MBEDTLS_SSL_SRV_C) @@ -6633,22 +6636,24 @@ int mbedtls_ssl_parse_certificate( mbedtls_ssl_context *ssl ) /* Clear existing peer CRT structure in case we tried to * reuse a session but it failed, and allocate a new one. */ ssl_clear_peer_cert( ssl->session_negotiate ); - ssl->session_negotiate->peer_cert = - mbedtls_calloc( 1, sizeof( mbedtls_x509_crt ) ); - if( ssl->session_negotiate->peer_cert == NULL ) + + chain = mbedtls_calloc( 1, sizeof( mbedtls_x509_crt ) ); + if( chain == 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 ); - ret = ssl_parse_certificate_chain( ssl, ssl->session_negotiate->peer_cert ); + ret = MBEDTLS_ERR_SSL_ALLOC_FAILED; + goto exit; + } + mbedtls_x509_crt_init( chain ); + + ret = ssl_parse_certificate_chain( ssl, chain ); if( ret != 0 ) - return( ret ); + goto exit; #if defined(MBEDTLS_SSL__ECP_RESTARTABLE) if( ssl->handshake->ecrs_enabled) @@ -6660,12 +6665,12 @@ crt_verify: #endif ret = ssl_parse_certificate_verify( ssl, authmode, - ssl->session_negotiate->peer_cert, - rs_ctx ); + chain, rs_ctx ); if( ret != 0 ) - return( ret ); + goto exit; #if !defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) + /* Remember digest of the peer's end-CRT. */ ssl->session_negotiate->peer_cert_digest = mbedtls_calloc( 1, MBEDTLS_SSL_PEER_CERT_DIGEST_DFL_LEN ); @@ -6676,15 +6681,16 @@ crt_verify: mbedtls_ssl_send_alert_message( ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, MBEDTLS_SSL_ALERT_MSG_INTERNAL_ERROR ); - return( MBEDTLS_ERR_SSL_ALLOC_FAILED ); + + ret = MBEDTLS_ERR_SSL_ALLOC_FAILED; + goto exit; } ret = mbedtls_md( mbedtls_md_info_from_type( - MBEDTLS_SSL_PEER_CERT_DIGEST_DFL_TYPE ), - ssl->session_negotiate->peer_cert->raw.p, - ssl->session_negotiate->peer_cert->raw.len, + MBEDTLS_SSL_PEER_CERT_DIGEST_DFL_TYPE ), + chain->raw.p, chain->raw.len, ssl->session_negotiate->peer_cert_digest ); if( ret != 0 ) - return( ret ); + goto exit; ssl->session_negotiate->peer_cert_digest_type = MBEDTLS_SSL_PEER_CERT_DIGEST_DFL_TYPE; @@ -6692,11 +6698,30 @@ crt_verify: MBEDTLS_SSL_PEER_CERT_DIGEST_DFL_LEN; #endif /* !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ + ssl->session_negotiate->peer_cert = chain; + chain = NULL; + MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= parse certificate" ) ); exit: - ssl->state++; + if( ret == 0 ) + ssl->state++; + +#if defined(MBEDTLS_SSL__ECP_RESTARTABLE) + if( ret == MBEDTLS_ERR_SSL_CRYPTO_IN_PROGRESS ) + { + ssl->handshake->ecrs_peer_cert = chain; + chain = NULL; + } +#endif + + if( chain != NULL ) + { + mbedtls_x509_crt_free( chain ); + mbedtls_free( chain ); + } + return( ret ); } #endif /* MBEDTLS_KEY_EXCHANGE__WITH_CERT__ENABLED */ @@ -10283,6 +10308,11 @@ void mbedtls_ssl_handshake_free( mbedtls_ssl_context *ssl ) #if defined(MBEDTLS_SSL__ECP_RESTARTABLE) mbedtls_x509_crt_restart_free( &handshake->ecrs_ctx ); + if( handshake->ecrs_peer_cert != NULL ) + { + mbedtls_x509_crt_free( handshake->ecrs_peer_cert ); + mbedtls_free( handshake->ecrs_peer_cert ); + } #endif #if defined(MBEDTLS_SSL_PROTO_DTLS) From 4a2f8e584f38c268db5d469655062eef7733c69f Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 6 Feb 2019 15:23:38 +0000 Subject: [PATCH 25/67] Add peer CRT digest to session tickets This commit changes the format of session tickets to include the digest of the peer's CRT if MBEDTLS_SSL_KEEP_PEER_CERTIFICATE is disabled. This commit does not yet remove the peer CRT itself. --- library/ssl_tls.c | 64 +++++++++++++++++++++++++++- tests/suites/test_suite_ssl.function | 48 +++++++++++++++++++-- 2 files changed, 107 insertions(+), 5 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index c682166f8..5e39579fb 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -9023,6 +9023,12 @@ const mbedtls_ssl_session *mbedtls_ssl_get_session_pointer( const mbedtls_ssl_co #define SSL_SERIALIZED_SESSION_CONFIG_TICKET 0 #endif /* MBEDTLS_SSL_SESSION_TICKETS */ +#if defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) +#define SSL_SERIALIZED_SESSION_CONFIG_KEEP_CRT 1 +#else +#define SSL_SERIALIZED_SESSION_CONFIG_KEEP_CRT 0 +#endif /* MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ + #define SSL_SERIALIZED_SESSION_CONFIG_TIME_BIT 0 #define SSL_SERIALIZED_SESSION_CONFIG_CRT_BIT 1 #define SSL_SERIALIZED_SESSION_CONFIG_CLIENT_TICKET_BIT 2 @@ -9030,6 +9036,7 @@ const mbedtls_ssl_session *mbedtls_ssl_get_session_pointer( const mbedtls_ssl_co #define SSL_SERIALIZED_SESSION_CONFIG_TRUNC_HMAC_BIT 4 #define SSL_SERIALIZED_SESSION_CONFIG_ETM_BIT 5 #define SSL_SERIALIZED_SESSION_CONFIG_TICKET_BIT 6 +#define SSL_SERIALIZED_SESSION_CONFIG_KEEP_CRT_BIT 7 #define SSL_SERIALIZED_SESSION_CONFIG_BITFLAG \ ( (uint16_t) ( \ @@ -9039,7 +9046,8 @@ const mbedtls_ssl_session *mbedtls_ssl_get_session_pointer( const mbedtls_ssl_co ( SSL_SERIALIZED_SESSION_CONFIG_MFL << SSL_SERIALIZED_SESSION_CONFIG_MFL_BIT ) | \ ( SSL_SERIALIZED_SESSION_CONFIG_TRUNC_HMAC << SSL_SERIALIZED_SESSION_CONFIG_TRUNC_HMAC_BIT ) | \ ( SSL_SERIALIZED_SESSION_CONFIG_ETM << SSL_SERIALIZED_SESSION_CONFIG_ETM_BIT ) | \ - ( SSL_SERIALIZED_SESSION_CONFIG_TICKET << SSL_SERIALIZED_SESSION_CONFIG_TICKET_BIT ) ) ) + ( SSL_SERIALIZED_SESSION_CONFIG_TICKET << SSL_SERIALIZED_SESSION_CONFIG_TICKET_BIT ) | \ + ( SSL_SERIALIZED_SESSION_CONFIG_KEEP_CRT << SSL_SERIALIZED_SESSION_CONFIG_KEEP_CRT_BIT ) ) ) static unsigned char ssl_serialized_session_header[] = { MBEDTLS_VERSION_MAJOR, @@ -9073,6 +9081,8 @@ static unsigned char ssl_serialized_session_header[] = { * opaque master[48]; // fixed length in the standard * uint32 verify_result; * opaque peer_cert<0..2^24-1>; // length 0 means no peer cert + * uint8_t peer_cert_digest_type; + * opaque peer_cert_digest<0..2^8-1>; * opaque ticket<0..2^24-1>; // length 0 means no ticket * uint32 ticket_lifetime; * uint8 mfl_code; // up to 255 according to standard @@ -9184,6 +9194,31 @@ int mbedtls_ssl_session_save( const mbedtls_ssl_session *session, p += cert_len; } } + + /* Digest of peer certificate */ +#if !defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) + if( session->peer_cert_digest != NULL ) + { + used += 1 /* type */ + 1 /* length */ + session->peer_cert_digest_len; + if( used <= buf_len ) + { + *p++ = (unsigned char) session->peer_cert_digest_type; + *p++ = (unsigned char) session->peer_cert_digest_len; + memcpy( p, session->peer_cert_digest, + session->peer_cert_digest_len ); + p += session->peer_cert_digest_len; + } + } + else + { + used += 2; + if( used <= buf_len ) + { + *p++ = (unsigned char) MBEDTLS_MD_NONE; + *p++ = 0; + } + } +#endif /* !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ #endif /* MBEDTLS_X509_CRT_PARSE_C */ /* @@ -9325,6 +9360,9 @@ static int ssl_session_load( mbedtls_ssl_session *session, * we exit early before we replaced them with valid ones. */ #if defined(MBEDTLS_X509_CRT_PARSE_C) session->peer_cert = NULL; +#if !defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) + session->peer_cert_digest = NULL; +#endif /* MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ #endif #if defined(MBEDTLS_SSL_SESSION_TICKETS) && defined(MBEDTLS_SSL_CLI_C) session->ticket = NULL; @@ -9369,6 +9407,30 @@ static int ssl_session_load( mbedtls_ssl_session *session, p += cert_len; } + +#if !defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) + /* Deserialize CRT digest from the end of the ticket. */ + if( 2 > (size_t)( end - p ) ) + return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); + + session->peer_cert_digest_type = (mbedtls_md_type_t) *p++; + session->peer_cert_digest_len = (size_t) *p++; + + if( session->peer_cert_digest_len != 0 ) + { + if( session->peer_cert_digest_len > (size_t)( end - p ) ) + return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); + + session->peer_cert_digest = + mbedtls_calloc( 1, session->peer_cert_digest_len ); + if( session->peer_cert_digest == NULL ) + return( MBEDTLS_ERR_SSL_ALLOC_FAILED ); + + memcpy( session->peer_cert_digest, p, + session->peer_cert_digest_len ); + p += session->peer_cert_digest_len; + } +#endif /* MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ #endif /* MBEDTLS_X509_CRT_PARSE_C */ /* diff --git a/tests/suites/test_suite_ssl.function b/tests/suites/test_suite_ssl.function index 65f585274..77a6afb5e 100644 --- a/tests/suites/test_suite_ssl.function +++ b/tests/suites/test_suite_ssl.function @@ -1,6 +1,7 @@ /* BEGIN_HEADER */ #include #include +#include /* * Helper function setting up inverse record transformations @@ -286,15 +287,40 @@ static int ssl_populate_session( mbedtls_ssl_session *session, #if defined(MBEDTLS_X509_CRT_PARSE_C) && defined(MBEDTLS_FS_IO) if( strlen( crt_file ) != 0 ) { + mbedtls_x509_crt tmp_crt; int ret; + mbedtls_x509_crt_init( &tmp_crt ); + ret = mbedtls_x509_crt_parse_file( &tmp_crt, crt_file ); + if( ret != 0 ) + return( ret ); + +#if !defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) + /* Calculate digest of temporary CRT. */ + session->peer_cert_digest = + mbedtls_calloc( 1, MBEDTLS_SSL_PEER_CERT_DIGEST_DFL_LEN ); + if( session->peer_cert_digest == NULL ) + return( -1 ); + ret = mbedtls_md( mbedtls_md_info_from_type( + MBEDTLS_SSL_PEER_CERT_DIGEST_DFL_TYPE ), + tmp_crt.raw.p, tmp_crt.raw.len, + session->peer_cert_digest ); + if( ret != 0 ) + return( ret ); + session->peer_cert_digest_type = + MBEDTLS_SSL_PEER_CERT_DIGEST_DFL_TYPE; + session->peer_cert_digest_len = + MBEDTLS_SSL_PEER_CERT_DIGEST_DFL_LEN; +#endif /* !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ + + /* Move temporary CRT. */ session->peer_cert = mbedtls_calloc( 1, sizeof( *session->peer_cert ) ); if( session->peer_cert == NULL ) return( -1 ); + *session->peer_cert = tmp_crt; + memset( &tmp_crt, 0, sizeof( tmp_crt ) ); - ret = mbedtls_x509_crt_parse_file( session->peer_cert, crt_file ); - if( ret != 0 ) - return( ret ); + mbedtls_x509_crt_free( &tmp_crt ); } #else (void) crt_file; @@ -690,7 +716,21 @@ void ssl_serialize_session_save_load( int ticket_len, char *crt_file ) restored.peer_cert->raw.p, original.peer_cert->raw.len ) == 0 ); } -#endif +#if !defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) + TEST_ASSERT( original.peer_cert_digest_type == + restored.peer_cert_digest_type ); + TEST_ASSERT( original.peer_cert_digest_len == + restored.peer_cert_digest_len ); + TEST_ASSERT( ( original.peer_cert_digest == NULL ) == + ( restored.peer_cert_digest == NULL ) ); + if( original.peer_cert_digest != NULL ) + { + TEST_ASSERT( memcmp( original.peer_cert_digest, + restored.peer_cert_digest, + original.peer_cert_digest_len ) == 0 ); + } +#endif /* !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ +#endif /* MBEDTLS_X509_CRT_PARSE_C */ TEST_ASSERT( original.verify_result == restored.verify_result ); #if defined(MBEDTLS_SSL_SESSION_TICKETS) && defined(MBEDTLS_SSL_CLI_C) From 2e6d34761fde536def4420908dc1b9e9e0eceb5a Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 6 Feb 2019 15:40:27 +0000 Subject: [PATCH 26/67] Remove peer CRT from mbedtls_ssl_session if !KEEP_PEER_CERT --- include/mbedtls/ssl.h | 5 +++-- include/mbedtls/ssl_cache.h | 3 ++- library/ssl_cache.c | 20 ++++++++++++-------- library/ssl_tls.c | 18 ++++++++++++------ tests/suites/test_suite_ssl.function | 7 ++++--- 5 files changed, 33 insertions(+), 20 deletions(-) diff --git a/include/mbedtls/ssl.h b/include/mbedtls/ssl.h index 5df5eb1b5..0e5c1fe98 100644 --- a/include/mbedtls/ssl.h +++ b/include/mbedtls/ssl.h @@ -851,14 +851,15 @@ struct mbedtls_ssl_session unsigned char master[48]; /*!< the master secret */ #if defined(MBEDTLS_X509_CRT_PARSE_C) +#if defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) mbedtls_x509_crt *peer_cert; /*!< peer X.509 cert chain */ -#if !defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) +#else /* MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ /*! The digest of the peer's end-CRT. This must be kept to detect CRT * changes during renegotiation, mitigating the triple handshake attack. */ unsigned char *peer_cert_digest; size_t peer_cert_digest_len; mbedtls_md_type_t peer_cert_digest_type; -#endif /* MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ +#endif /* !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ #endif /* MBEDTLS_X509_CRT_PARSE_C */ uint32_t verify_result; /*!< verification result */ diff --git a/include/mbedtls/ssl_cache.h b/include/mbedtls/ssl_cache.h index 52ba0948c..84254d3d1 100644 --- a/include/mbedtls/ssl_cache.h +++ b/include/mbedtls/ssl_cache.h @@ -70,7 +70,8 @@ struct mbedtls_ssl_cache_entry mbedtls_time_t timestamp; /*!< entry timestamp */ #endif mbedtls_ssl_session session; /*!< entry session */ -#if defined(MBEDTLS_X509_CRT_PARSE_C) +#if defined(MBEDTLS_X509_CRT_PARSE_C) && \ + defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) mbedtls_x509_buf peer_cert; /*!< entry peer_cert */ #endif mbedtls_ssl_cache_entry *next; /*!< chain pointer */ diff --git a/library/ssl_cache.c b/library/ssl_cache.c index f5425944e..62a0a2987 100644 --- a/library/ssl_cache.c +++ b/library/ssl_cache.c @@ -100,7 +100,8 @@ int mbedtls_ssl_cache_get( void *data, mbedtls_ssl_session *session ) goto exit; } -#if defined(MBEDTLS_X509_CRT_PARSE_C) +#if defined(MBEDTLS_X509_CRT_PARSE_C) && \ + defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) /* * Restore peer certificate (without rest of the original chain) */ @@ -127,7 +128,7 @@ int mbedtls_ssl_cache_get( void *data, mbedtls_ssl_session *session ) goto exit; } } -#endif /* MBEDTLS_X509_CRT_PARSE_C */ +#endif /* MBEDTLS_X509_CRT_PARSE_C && MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ ret = 0; goto exit; @@ -247,7 +248,8 @@ int mbedtls_ssl_cache_set( void *data, const mbedtls_ssl_session *session ) #endif } -#if defined(MBEDTLS_X509_CRT_PARSE_C) +#if defined(MBEDTLS_X509_CRT_PARSE_C) && \ + defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) /* * If we're reusing an entry, free its certificate first */ @@ -256,7 +258,7 @@ int mbedtls_ssl_cache_set( void *data, const mbedtls_ssl_session *session ) mbedtls_free( cur->peer_cert.p ); memset( &cur->peer_cert, 0, sizeof(mbedtls_x509_buf) ); } -#endif /* MBEDTLS_X509_CRT_PARSE_C */ +#endif /* MBEDTLS_X509_CRT_PARSE_C && MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ /* Copy the entire session; this temporarily makes a copy of the * X.509 CRT structure even though we only want to store the raw CRT. @@ -270,7 +272,8 @@ int mbedtls_ssl_cache_set( void *data, const mbedtls_ssl_session *session ) goto exit; } -#if defined(MBEDTLS_X509_CRT_PARSE_C) +#if defined(MBEDTLS_X509_CRT_PARSE_C) && \ + defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) /* If present, free the X.509 structure and only store the raw CRT data. */ if( cur->session.peer_cert != NULL ) { @@ -291,7 +294,7 @@ int mbedtls_ssl_cache_set( void *data, const mbedtls_ssl_session *session ) mbedtls_free( cur->session.peer_cert ); cur->session.peer_cert = NULL; } -#endif /* MBEDTLS_X509_CRT_PARSE_C */ +#endif /* MBEDTLS_X509_CRT_PARSE_C && MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ ret = 0; @@ -333,9 +336,10 @@ void mbedtls_ssl_cache_free( mbedtls_ssl_cache_context *cache ) mbedtls_ssl_session_free( &prv->session ); -#if defined(MBEDTLS_X509_CRT_PARSE_C) +#if defined(MBEDTLS_X509_CRT_PARSE_C) && \ + defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) mbedtls_free( prv->peer_cert.p ); -#endif /* MBEDTLS_X509_CRT_PARSE_C */ +#endif /* MBEDTLS_X509_CRT_PARSE_C && MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ mbedtls_free( prv ); } diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 5e39579fb..63b79a633 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -9104,7 +9104,9 @@ int mbedtls_ssl_session_save( const mbedtls_ssl_session *session, uint64_t start; #endif #if defined(MBEDTLS_X509_CRT_PARSE_C) +#if defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) size_t cert_len; +#endif #endif /* @@ -9175,6 +9177,7 @@ int mbedtls_ssl_session_save( const mbedtls_ssl_session *session, * Peer's end-entity certificate */ #if defined(MBEDTLS_X509_CRT_PARSE_C) +#if defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) if( session->peer_cert == NULL ) cert_len = 0; else @@ -9195,8 +9198,8 @@ int mbedtls_ssl_session_save( const mbedtls_ssl_session *session, } } +#else /* MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ /* Digest of peer certificate */ -#if !defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) if( session->peer_cert_digest != NULL ) { used += 1 /* type */ + 1 /* length */ + session->peer_cert_digest_len; @@ -9295,8 +9298,10 @@ static int ssl_session_load( mbedtls_ssl_session *session, uint64_t start; #endif #if defined(MBEDTLS_X509_CRT_PARSE_C) +#if defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) size_t cert_len; -#endif /* MBEDTLS_X509_CRT_PARSE_C */ +#endif +#endif /* * Check version identifier @@ -9359,10 +9364,11 @@ static int ssl_session_load( mbedtls_ssl_session *session, /* Immediately clear invalid pointer values that have been read, in case * we exit early before we replaced them with valid ones. */ #if defined(MBEDTLS_X509_CRT_PARSE_C) +#if defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) session->peer_cert = NULL; -#if !defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) +#else session->peer_cert_digest = NULL; -#endif /* MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ +#endif /* !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ #endif #if defined(MBEDTLS_SSL_SESSION_TICKETS) && defined(MBEDTLS_SSL_CLI_C) session->ticket = NULL; @@ -9372,6 +9378,7 @@ static int ssl_session_load( mbedtls_ssl_session *session, * Peer certificate */ #if defined(MBEDTLS_X509_CRT_PARSE_C) +#if defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) if( 3 > (size_t)( end - p ) ) return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); @@ -9407,8 +9414,7 @@ static int ssl_session_load( mbedtls_ssl_session *session, p += cert_len; } - -#if !defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) +#else /* defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ /* Deserialize CRT digest from the end of the ticket. */ if( 2 > (size_t)( end - p ) ) return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); diff --git a/tests/suites/test_suite_ssl.function b/tests/suites/test_suite_ssl.function index 77a6afb5e..b59c204e2 100644 --- a/tests/suites/test_suite_ssl.function +++ b/tests/suites/test_suite_ssl.function @@ -311,14 +311,14 @@ static int ssl_populate_session( mbedtls_ssl_session *session, MBEDTLS_SSL_PEER_CERT_DIGEST_DFL_TYPE; session->peer_cert_digest_len = MBEDTLS_SSL_PEER_CERT_DIGEST_DFL_LEN; -#endif /* !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ - +#else /* !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ /* Move temporary CRT. */ session->peer_cert = mbedtls_calloc( 1, sizeof( *session->peer_cert ) ); if( session->peer_cert == NULL ) return( -1 ); *session->peer_cert = tmp_crt; memset( &tmp_crt, 0, sizeof( tmp_crt ) ); +#endif /* MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ mbedtls_x509_crt_free( &tmp_crt ); } @@ -706,6 +706,7 @@ void ssl_serialize_session_save_load( int ticket_len, char *crt_file ) restored.master, sizeof( original.master ) ) == 0 ); #if defined(MBEDTLS_X509_CRT_PARSE_C) +#if defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) TEST_ASSERT( ( original.peer_cert == NULL ) == ( restored.peer_cert == NULL ) ); if( original.peer_cert != NULL ) @@ -716,7 +717,7 @@ void ssl_serialize_session_save_load( int ticket_len, char *crt_file ) restored.peer_cert->raw.p, original.peer_cert->raw.len ) == 0 ); } -#if !defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) +#else /* MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ TEST_ASSERT( original.peer_cert_digest_type == restored.peer_cert_digest_type ); TEST_ASSERT( original.peer_cert_digest_len == From 32c530ece2e1433cc0a8ab26c3a4907ccfcfd853 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 6 Feb 2019 16:13:41 +0000 Subject: [PATCH 27/67] Add raw public key buffer bounds to mbedtls_x509_crt struct This commit adds an ASN.1 buffer field `pk_raw` to `mbedtls_x509_crt` which stores the bounds of the raw public key data within an X.509 CRT. This will be useful in subsequent commits to extract the peer's public key from its certificate chain. --- include/mbedtls/x509_crt.h | 1 + library/x509_crt.c | 2 ++ 2 files changed, 3 insertions(+) diff --git a/include/mbedtls/x509_crt.h b/include/mbedtls/x509_crt.h index dc3260b87..09ba69f39 100644 --- a/include/mbedtls/x509_crt.h +++ b/include/mbedtls/x509_crt.h @@ -70,6 +70,7 @@ typedef struct mbedtls_x509_crt mbedtls_x509_time valid_from; /**< Start time of certificate validity. */ mbedtls_x509_time valid_to; /**< End time of certificate validity. */ + mbedtls_x509_buf pk_raw; mbedtls_pk_context pk; /**< Container for the public key context. */ mbedtls_x509_buf issuer_id; /**< Optional X.509 v2/v3 issuer unique identifier. */ diff --git a/library/x509_crt.c b/library/x509_crt.c index 97d4b9504..e4a35f64d 100644 --- a/library/x509_crt.c +++ b/library/x509_crt.c @@ -990,11 +990,13 @@ static int x509_crt_parse_der_core( mbedtls_x509_crt *crt, /* * SubjectPublicKeyInfo */ + crt->pk_raw.p = p; if( ( ret = mbedtls_pk_parse_subpubkey( &p, end, &crt->pk ) ) != 0 ) { mbedtls_x509_crt_free( crt ); return( ret ); } + crt->pk_raw.len = p - crt->pk_raw.p; /* * issuerUniqueID [1] IMPLICIT UniqueIdentifier OPTIONAL, From 3bf8cdf2f837e95ec6369e670023a8d19a259bc0 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 6 Feb 2019 16:18:31 +0000 Subject: [PATCH 28/67] Add field for peer's raw public key to TLS handshake param structure When removing the (session-local) copy of the peer's CRT chain, we must keep a handshake-local copy of the peer's public key, as (naturally) every key exchange will make use of that public key at some point to verify that the peer actually owns the corresponding private key (e.g., verify signatures from ServerKeyExchange or CertificateVerify, or encrypt a PMS in a RSA-based exchange, or extract static (EC)DH parameters). This commit adds a PK context field `peer_pubkey` to the handshake parameter structure `mbedtls_handshake_params_init()` and adapts the init and free functions accordingly. It does not yet make actual use of the new field. --- include/mbedtls/ssl_internal.h | 4 ++++ library/ssl_tls.c | 10 ++++++++++ 2 files changed, 14 insertions(+) diff --git a/include/mbedtls/ssl_internal.h b/include/mbedtls/ssl_internal.h index d46895cc6..7b9265c51 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -401,6 +401,10 @@ struct mbedtls_ssl_handshake_params #endif /* MBEDTLS_X509_CRT_PARSE_C */ size_t ecrs_n; /*!< place for saving a length */ #endif +#if defined(MBEDTLS_X509_CRT_PARSE_C) && \ + !defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) + mbedtls_pk_context peer_pubkey; /*!< The public key from the peer. */ +#endif /* MBEDTLS_X509_CRT_PARSE_C && !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ #if defined(MBEDTLS_SSL_PROTO_DTLS) unsigned int out_msg_seq; /*!< Outgoing handshake sequence number */ unsigned int in_msg_seq; /*!< Incoming handshake sequence number */ diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 63b79a633..d752aeb1a 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -7506,6 +7506,11 @@ static void ssl_handshake_params_init( mbedtls_ssl_handshake_params *handshake ) #if defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) handshake->sni_authmode = MBEDTLS_SSL_VERIFY_UNSET; #endif + +#if defined(MBEDTLS_X509_CRT_PARSE_C) && \ + !defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) + mbedtls_pk_init( &handshake->peer_pubkey ); +#endif } void mbedtls_ssl_transform_init( mbedtls_ssl_transform *transform ) @@ -10383,6 +10388,11 @@ void mbedtls_ssl_handshake_free( mbedtls_ssl_context *ssl ) } #endif +#if defined(MBEDTLS_X509_CRT_PARSE_C) && \ + !defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) + mbedtls_pk_free( &handshake->peer_pubkey ); +#endif /* MBEDTLS_X509_CRT_PARSE_C && !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ + #if defined(MBEDTLS_SSL_PROTO_DTLS) mbedtls_free( handshake->verify_cookie ); ssl_flight_free( handshake->flight ); From cf291d63ddb251dd1cfa32451439c2dd99685bea Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 6 Feb 2019 16:19:04 +0000 Subject: [PATCH 29/67] Make a copy of peer's raw public key after verifying its CRT chain This commit modifies `mbedtls_ssl_parse_certificate()` to store a copy of the peer's public key after parsing and verifying the peer's CRT chain. So far, this leads to heavy memory duplication: We have the CRT chain in the I/O buffer, then parse (and, thereby, copy) it to a `mbedtls_x509_crt` structure, and then make another copy of the peer's public key, plus the overhead from the MPI and ECP structures. This inefficiency will soon go away to a significant extend, because: - Another PR adds functionality to parse CRTs without taking ownership of the input buffers. Applying this here will allow parsing and verifying the peer's chain without making an additional raw copy. The overhead reduces to the size of `mbedtls_x509_crt`, the public key, and the DN structures referenced in the CRT. - Once copyless parsing is in place and the removal of the peer CRT is fully implemented, we can extract the public key bounds from the parsed certificate and then free the entire chain before parsing the public key again. This means that we never store the parsed public key twice at the same time. --- library/ssl_tls.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index d752aeb1a..c739fe776 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -6698,6 +6698,24 @@ crt_verify: MBEDTLS_SSL_PEER_CERT_DIGEST_DFL_LEN; #endif /* !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ +#if !defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) + /* Make a copy of the peer's raw public key. */ + mbedtls_pk_init( &ssl->handshake->peer_pubkey ); + { + unsigned char *p, *end; + p = chain->pk_raw.p; + end = p + chain->pk_raw.len; + ret = mbedtls_pk_parse_subpubkey( &p, end, + &ssl->handshake->peer_pubkey ); + if( ret != 0 ) + { + /* We should have parsed the public key before. */ + ret = MBEDTLS_ERR_SSL_INTERNAL_ERROR; + goto exit; + } + } +#endif /* !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ + ssl->session_negotiate->peer_cert = chain; chain = NULL; From 374800a231cff33225419725231cb5a6da47dce8 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 6 Feb 2019 16:49:54 +0000 Subject: [PATCH 30/67] Adapt ssl_write_encrypted_pms() to use raw public key We must dispatch between the peer's public key stored as part of the peer's CRT in the current session structure (situation until now, and future behaviour if MBEDTLS_SSL_KEEP_PEER_CERTIFICATE is enabled), and the sole public key stored in the handshake structure (new, if MBEDTLS_SSL_KEEP_PEER_CERTIFICATE is disabled). --- library/ssl_cli.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/library/ssl_cli.c b/library/ssl_cli.c index b317b2d4d..e819379ff 100644 --- a/library/ssl_cli.c +++ b/library/ssl_cli.c @@ -2288,6 +2288,7 @@ static int ssl_write_encrypted_pms( mbedtls_ssl_context *ssl, int ret; size_t len_bytes = ssl->minor_ver == MBEDTLS_SSL_MINOR_VERSION_0 ? 0 : 2; unsigned char *p = ssl->handshake->premaster + pms_offset; + mbedtls_pk_context * peer_pk; if( offset + len_bytes > MBEDTLS_SSL_OUT_CONTENT_LEN ) { @@ -2313,23 +2314,27 @@ static int ssl_write_encrypted_pms( mbedtls_ssl_context *ssl, ssl->handshake->pmslen = 48; +#if !defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) + peer_pk = &ssl->handshake->peer_pubkey; +#else /* !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ if( ssl->session_negotiate->peer_cert == NULL ) { /* Should never happen */ return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); } + peer_pk = &ssl->session_negotiate->peer_cert->pk; +#endif /* MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ /* * Now write it out, encrypted */ - if( ! mbedtls_pk_can_do( &ssl->session_negotiate->peer_cert->pk, - MBEDTLS_PK_RSA ) ) + if( ! mbedtls_pk_can_do( peer_pk, MBEDTLS_PK_RSA ) ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "certificate key type mismatch" ) ); return( MBEDTLS_ERR_SSL_PK_TYPE_MISMATCH ); } - if( ( ret = mbedtls_pk_encrypt( &ssl->session_negotiate->peer_cert->pk, + if( ( ret = mbedtls_pk_encrypt( peer_pk, p, ssl->handshake->pmslen, ssl->out_msg + offset + len_bytes, olen, MBEDTLS_SSL_OUT_CONTENT_LEN - offset - len_bytes, From 53b6b7e09b3f2d7954a4ee2758b8424a93909622 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 6 Feb 2019 17:44:07 +0000 Subject: [PATCH 31/67] Adapt ssl_get_ecdh_params_from_cert() to use raw public key We must dispatch between the peer's public key stored as part of the peer's CRT in the current session structure (situation until now, and future behaviour if MBEDTLS_SSL_KEEP_PEER_CERTIFICATE is enabled), and the sole public key stored in the handshake structure (new, if MBEDTLS_SSL_KEEP_PEER_CERTIFICATE is disabled). --- library/ssl_cli.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/library/ssl_cli.c b/library/ssl_cli.c index e819379ff..48eb75a64 100644 --- a/library/ssl_cli.c +++ b/library/ssl_cli.c @@ -2429,21 +2429,26 @@ static int ssl_get_ecdh_params_from_cert( mbedtls_ssl_context *ssl ) { int ret; const mbedtls_ecp_keypair *peer_key; + mbedtls_pk_context * peer_pk; +#if !defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) + peer_pk = &ssl->handshake->peer_pubkey; +#else /* !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ if( ssl->session_negotiate->peer_cert == NULL ) { /* Should never happen */ return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); } + peer_pk = &ssl->session_negotiate->peer_cert->pk; +#endif /* MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ - if( ! mbedtls_pk_can_do( &ssl->session_negotiate->peer_cert->pk, - MBEDTLS_PK_ECKEY ) ) + if( ! mbedtls_pk_can_do( peer_pk, MBEDTLS_PK_ECKEY ) ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "server key not ECDH capable" ) ); return( MBEDTLS_ERR_SSL_PK_TYPE_MISMATCH ); } - peer_key = mbedtls_pk_ec( ssl->session_negotiate->peer_cert->pk ); + peer_key = mbedtls_pk_ec( *peer_pk ); if( ( ret = mbedtls_ecdh_get_params( &ssl->handshake->ecdh_ctx, peer_key, MBEDTLS_ECDH_THEIRS ) ) != 0 ) From 69fad138537db250719197bd3d8e9864c73efae6 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 6 Feb 2019 18:26:03 +0000 Subject: [PATCH 32/67] Adapt client-side signature verification to use raw public key We must dispatch between the peer's public key stored as part of the peer's CRT in the current session structure (situation until now, and future behaviour if MBEDTLS_SSL_KEEP_PEER_CERTIFICATE is enabled), and the sole public key stored in the handshake structure (new, if MBEDTLS_SSL_KEEP_PEER_CERTIFICATE is disabled). --- library/ssl_cli.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/library/ssl_cli.c b/library/ssl_cli.c index 48eb75a64..e39ce7a95 100644 --- a/library/ssl_cli.c +++ b/library/ssl_cli.c @@ -2655,6 +2655,8 @@ start_processing: size_t params_len = p - params; void *rs_ctx = NULL; + mbedtls_pk_context * peer_pk; + /* * Handle the digitally-signed structure */ @@ -2757,16 +2759,21 @@ start_processing: MBEDTLS_SSL_DEBUG_BUF( 3, "parameters hash", hash, hashlen ); +#if !defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) + peer_pk = &ssl->handshake->peer_pubkey; +#else /* !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ if( ssl->session_negotiate->peer_cert == NULL ) { /* Should never happen */ return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); } + peer_pk = &ssl->session_negotiate->peer_cert->pk; +#endif /* MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ /* * Verify signature */ - if( ! mbedtls_pk_can_do( &ssl->session_negotiate->peer_cert->pk, pk_alg ) ) + if( !mbedtls_pk_can_do( peer_pk, pk_alg ) ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad server key exchange message" ) ); mbedtls_ssl_send_alert_message( ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, @@ -2779,8 +2786,7 @@ start_processing: rs_ctx = &ssl->handshake->ecrs_ctx.pk; #endif - if( ( ret = mbedtls_pk_verify_restartable( - &ssl->session_negotiate->peer_cert->pk, + if( ( ret = mbedtls_pk_verify_restartable( peer_pk, md_alg, hash, hashlen, p, sig_len, rs_ctx ) ) != 0 ) { #if defined(MBEDTLS_SSL__ECP_RESTARTABLE) From 0833c1082b85d66a8915bb6ae1928ede474004e9 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 6 Feb 2019 18:31:04 +0000 Subject: [PATCH 33/67] Adapt server-side signature verification to use raw public key We must dispatch between the peer's public key stored as part of the peer's CRT in the current session structure (situation until now, and future behaviour if MBEDTLS_SSL_KEEP_PEER_CERTIFICATE is enabled), and the sole public key stored in the handshake structure (new, if MBEDTLS_SSL_KEEP_PEER_CERTIFICATE is disabled). --- library/ssl_srv.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/library/ssl_srv.c b/library/ssl_srv.c index 884bdd17b..53ee2c0a2 100644 --- a/library/ssl_srv.c +++ b/library/ssl_srv.c @@ -4161,6 +4161,7 @@ static int ssl_parse_certificate_verify( mbedtls_ssl_context *ssl ) mbedtls_md_type_t md_alg; const mbedtls_ssl_ciphersuite_t *ciphersuite_info = ssl->handshake->ciphersuite_info; + mbedtls_pk_context * peer_pk; MBEDTLS_SSL_DEBUG_MSG( 2, ( "=> parse certificate verify" ) ); @@ -4192,6 +4193,17 @@ static int ssl_parse_certificate_verify( mbedtls_ssl_context *ssl ) i = mbedtls_ssl_hs_hdr_len( ssl ); +#if !defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) + peer_pk = &ssl->handshake->peer_pubkey; +#else /* !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ + if( ssl->session_negotiate->peer_cert == NULL ) + { + /* Should never happen */ + return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); + } + peer_pk = &ssl->session_negotiate->peer_cert->pk; +#endif /* MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ + /* * struct { * SignatureAndHashAlgorithm algorithm; -- TLS 1.2 only @@ -4206,8 +4218,7 @@ static int ssl_parse_certificate_verify( mbedtls_ssl_context *ssl ) hashlen = 36; /* For ECDSA, use SHA-1, not MD-5 + SHA-1 */ - if( mbedtls_pk_can_do( &ssl->session_negotiate->peer_cert->pk, - MBEDTLS_PK_ECDSA ) ) + if( mbedtls_pk_can_do( peer_pk, MBEDTLS_PK_ECDSA ) ) { hash_start += 16; hashlen -= 16; @@ -4262,7 +4273,7 @@ static int ssl_parse_certificate_verify( mbedtls_ssl_context *ssl ) /* * Check the certificate's key type matches the signature alg */ - if( ! mbedtls_pk_can_do( &ssl->session_negotiate->peer_cert->pk, pk_alg ) ) + if( !mbedtls_pk_can_do( peer_pk, pk_alg ) ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "sig_alg doesn't match cert key" ) ); return( MBEDTLS_ERR_SSL_BAD_HS_CERTIFICATE_VERIFY ); @@ -4298,7 +4309,7 @@ static int ssl_parse_certificate_verify( mbedtls_ssl_context *ssl ) ssl->handshake->calc_verify( ssl, hash, &dummy_hlen ); } - if( ( ret = mbedtls_pk_verify( &ssl->session_negotiate->peer_cert->pk, + if( ( ret = mbedtls_pk_verify( peer_pk, md_alg, hash_start, hashlen, ssl->in_msg + i, sig_len ) ) != 0 ) { From b265f5f19118c9d90818a430f2aee9b4ce39f1f7 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 7 Feb 2019 13:28:57 +0000 Subject: [PATCH 34/67] Use mbedtls_ssl_get_peer_cert() to query peer cert in cert_app --- programs/x509/cert_app.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/programs/x509/cert_app.c b/programs/x509/cert_app.c index 8eded3413..fd527199c 100644 --- a/programs/x509/cert_app.c +++ b/programs/x509/cert_app.c @@ -467,9 +467,12 @@ int main( int argc, char *argv[] ) /* * 5. Print the certificate */ +#if !defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) + mbedtls_printf( " . Peer certificate information ... skipped\n" ); +#else mbedtls_printf( " . Peer certificate information ...\n" ); ret = mbedtls_x509_crt_info( (char *) buf, sizeof( buf ) - 1, " ", - ssl.session->peer_cert ); + mbedtls_ssl_get_peer_cert( &ssl ) ); if( ret == -1 ) { mbedtls_printf( " failed\n ! mbedtls_x509_crt_info returned %d\n\n", ret ); @@ -477,6 +480,7 @@ int main( int argc, char *argv[] ) } mbedtls_printf( "%s\n", buf ); +#endif /* MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ mbedtls_ssl_close_notify( &ssl ); From cd90126ab3e701a79f84967bbf62f0078937f3da Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 7 Feb 2019 13:17:25 +0000 Subject: [PATCH 35/67] Adapt client auth detection in ssl_parse_certificate_verify() The server expects a CertificateVerify message only if it has previously received a Certificate from the client. So far, this was detected by looking at the `peer_cert` field in the current session. Preparing to remove the latter, this commit changes this to instead determine the presence of a peer certificate by checking the new `peer_cert_digest` pointer. --- library/ssl_srv.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/library/ssl_srv.c b/library/ssl_srv.c index 53ee2c0a2..06fe3ee2c 100644 --- a/library/ssl_srv.c +++ b/library/ssl_srv.c @@ -4165,14 +4165,29 @@ static int ssl_parse_certificate_verify( mbedtls_ssl_context *ssl ) MBEDTLS_SSL_DEBUG_MSG( 2, ( "=> parse certificate verify" ) ); - if( !mbedtls_ssl_ciphersuite_cert_req_allowed( ciphersuite_info ) || - ssl->session_negotiate->peer_cert == NULL ) + if( !mbedtls_ssl_ciphersuite_cert_req_allowed( ciphersuite_info ) ) { MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= skip parse certificate verify" ) ); ssl->state++; return( 0 ); } +#if defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) + if( ssl->session_negotiate->peer_cert == NULL ) + { + MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= skip parse certificate verify" ) ); + ssl->state++; + return( 0 ); + } +#else /* MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ + if( ssl->session_negotiate->peer_cert_digest == NULL ) + { + MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= skip parse certificate verify" ) ); + ssl->state++; + return( 0 ); + } +#endif /* !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ + /* Read the message without adding it to the checksum */ ret = mbedtls_ssl_read_record( ssl, 0 /* no checksum update */ ); if( 0 != ret ) From d5258faa29c9e36608a0aedd5ef99365fc0dbb4f Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 7 Feb 2019 12:27:42 +0000 Subject: [PATCH 36/67] Adapt mbedtls_ssl_session_copy() to removal of `peer_cert` field --- library/ssl_tls.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index c739fe776..40d597569 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -375,6 +375,8 @@ int mbedtls_ssl_session_copy( mbedtls_ssl_session *dst, memcpy( dst, src, sizeof( mbedtls_ssl_session ) ); #if defined(MBEDTLS_X509_CRT_PARSE_C) + +#if defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) if( src->peer_cert != NULL ) { int ret; @@ -393,8 +395,7 @@ int mbedtls_ssl_session_copy( mbedtls_ssl_session *dst, return( ret ); } } - -#if !defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) +#else /* MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ if( src->peer_cert_digest != NULL ) { dst->peer_cert_digest_len = src->peer_cert_digest_len; From 506289750783774dc6c93b1369141288a455f3df Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 7 Feb 2019 13:17:53 +0000 Subject: [PATCH 37/67] Adapt ssl_clear_peer_cert() to removal of `peer_cert` field --- library/ssl_tls.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 40d597569..fff5e9d8f 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -6189,14 +6189,14 @@ static int ssl_check_peer_crt_unchanged( mbedtls_ssl_context *ssl, static void ssl_clear_peer_cert( mbedtls_ssl_session *session ) { +#if defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) if( session->peer_cert != NULL ) { mbedtls_x509_crt_free( session->peer_cert ); mbedtls_free( session->peer_cert ); session->peer_cert = NULL; } - -#if !defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) +#else if( session->peer_cert_digest != NULL ) { /* Zeroization is not necessary. */ @@ -6205,7 +6205,7 @@ static void ssl_clear_peer_cert( mbedtls_ssl_session *session ) session->peer_cert_digest_type = MBEDTLS_MD_NONE; session->peer_cert_digest_len = 0; } -#endif /* MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ +#endif /* !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ } /* From 81d11aa6400250486039e04f3fe16f2b6347b4e1 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 7 Feb 2019 13:18:21 +0000 Subject: [PATCH 38/67] Adapt mbedtls_ssl_parse_certificate() to removal of peer_cert field --- 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 fff5e9d8f..6561d6396 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -6715,10 +6715,10 @@ crt_verify: goto exit; } } -#endif /* !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ - +#else ssl->session_negotiate->peer_cert = chain; chain = NULL; +#endif /* !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= parse certificate" ) ); From 8b6d2cd5afcb81e1b40c78376627b9053c4cba2f Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 7 Feb 2019 13:44:35 +0000 Subject: [PATCH 39/67] Add dependency to ssl-opt.sh tests which need peer CRT debug info --- tests/ssl-opt.sh | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index 3dd69a5f2..7025d88da 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -3597,6 +3597,7 @@ run_test "Authentication: send CA list in CertificateRequest, client self sig # Tests for certificate selection based on SHA verson requires_config_disabled MBEDTLS_X509_REMOVE_INFO +requires_config_enabled MBEDTLS_SSL_KEEP_PEER_CERTIFICATE run_test "Certificate hash: client TLS 1.2 -> SHA-2" \ "$P_SRV crt_file=data_files/server5.crt \ key_file=data_files/server5.key \ @@ -3608,6 +3609,7 @@ run_test "Certificate hash: client TLS 1.2 -> SHA-2" \ -C "signed using.*ECDSA with SHA1" requires_config_disabled MBEDTLS_X509_REMOVE_INFO +requires_config_enabled MBEDTLS_SSL_KEEP_PEER_CERTIFICATE run_test "Certificate hash: client TLS 1.1 -> SHA-1" \ "$P_SRV crt_file=data_files/server5.crt \ key_file=data_files/server5.key \ @@ -3619,6 +3621,7 @@ run_test "Certificate hash: client TLS 1.1 -> SHA-1" \ -c "signed using.*ECDSA with SHA1" requires_config_disabled MBEDTLS_X509_REMOVE_INFO +requires_config_enabled MBEDTLS_SSL_KEEP_PEER_CERTIFICATE run_test "Certificate hash: client TLS 1.0 -> SHA-1" \ "$P_SRV crt_file=data_files/server5.crt \ key_file=data_files/server5.key \ @@ -3630,6 +3633,7 @@ run_test "Certificate hash: client TLS 1.0 -> SHA-1" \ -c "signed using.*ECDSA with SHA1" requires_config_disabled MBEDTLS_X509_REMOVE_INFO +requires_config_enabled MBEDTLS_SSL_KEEP_PEER_CERTIFICATE run_test "Certificate hash: client TLS 1.1, no SHA-1 -> SHA-2 (order 1)" \ "$P_SRV crt_file=data_files/server5.crt \ key_file=data_files/server5.key \ @@ -3642,6 +3646,7 @@ run_test "Certificate hash: client TLS 1.1, no SHA-1 -> SHA-2 (order 1)" \ -C "signed using.*ECDSA with SHA1" requires_config_disabled MBEDTLS_X509_REMOVE_INFO +requires_config_enabled MBEDTLS_SSL_KEEP_PEER_CERTIFICATE run_test "Certificate hash: client TLS 1.1, no SHA-1 -> SHA-2 (order 2)" \ "$P_SRV crt_file=data_files/server6.crt \ key_file=data_files/server6.key \ @@ -3656,6 +3661,7 @@ run_test "Certificate hash: client TLS 1.1, no SHA-1 -> SHA-2 (order 2)" \ # tests for SNI requires_config_disabled MBEDTLS_X509_REMOVE_INFO +requires_config_enabled MBEDTLS_SSL_KEEP_PEER_CERTIFICATE run_test "SNI: no SNI callback" \ "$P_SRV debug_level=3 \ crt_file=data_files/server5.crt key_file=data_files/server5.key" \ @@ -3666,6 +3672,7 @@ run_test "SNI: no SNI callback" \ -c "subject name *: C=NL, O=PolarSSL, CN=localhost" requires_config_disabled MBEDTLS_X509_REMOVE_INFO +requires_config_enabled MBEDTLS_SSL_KEEP_PEER_CERTIFICATE run_test "SNI: matching cert 1" \ "$P_SRV debug_level=3 \ crt_file=data_files/server5.crt key_file=data_files/server5.key \ @@ -3677,6 +3684,7 @@ run_test "SNI: matching cert 1" \ -c "subject name *: C=NL, O=PolarSSL, CN=localhost" requires_config_disabled MBEDTLS_X509_REMOVE_INFO +requires_config_enabled MBEDTLS_SSL_KEEP_PEER_CERTIFICATE run_test "SNI: matching cert 2" \ "$P_SRV debug_level=3 \ crt_file=data_files/server5.crt key_file=data_files/server5.key \ @@ -3798,6 +3806,7 @@ run_test "SNI: CA override with CRL" \ # Tests for SNI and DTLS requires_config_disabled MBEDTLS_X509_REMOVE_INFO +requires_config_enabled MBEDTLS_SSL_KEEP_PEER_CERTIFICATE run_test "SNI: DTLS, no SNI callback" \ "$P_SRV debug_level=3 dtls=1 \ crt_file=data_files/server5.crt key_file=data_files/server5.key" \ @@ -3808,6 +3817,7 @@ run_test "SNI: DTLS, no SNI callback" \ -c "subject name *: C=NL, O=PolarSSL, CN=localhost" requires_config_disabled MBEDTLS_X509_REMOVE_INFO +requires_config_enabled MBEDTLS_SSL_KEEP_PEER_CERTIFICATE run_test "SNI: DTLS, matching cert 1" \ "$P_SRV debug_level=3 dtls=1 \ crt_file=data_files/server5.crt key_file=data_files/server5.key \ @@ -3819,6 +3829,7 @@ run_test "SNI: DTLS, matching cert 1" \ -c "subject name *: C=NL, O=PolarSSL, CN=localhost" requires_config_disabled MBEDTLS_X509_REMOVE_INFO +requires_config_enabled MBEDTLS_SSL_KEEP_PEER_CERTIFICATE run_test "SNI: DTLS, matching cert 2" \ "$P_SRV debug_level=3 dtls=1 \ crt_file=data_files/server5.crt key_file=data_files/server5.key \ From bfab9dfea183ad9bd3e9aa20ca8534c5afb4ffca Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 7 Feb 2019 13:18:46 +0000 Subject: [PATCH 40/67] Guard mbedtls_ssl_get_peer_cert() by new compile-time option --- library/ssl_tls.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 6561d6396..ab457b7a6 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -8967,7 +8967,11 @@ const mbedtls_x509_crt *mbedtls_ssl_get_peer_cert( const mbedtls_ssl_context *ss if( ssl == NULL || ssl->session == NULL ) return( NULL ); +#if defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) return( ssl->session->peer_cert ); +#else + return( NULL ); +#endif /* MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ } #endif /* MBEDTLS_X509_CRT_PARSE_C */ From 597ffe43a10cd4947e3b97e4b074ddfc4428ea2d Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 7 Feb 2019 13:42:45 +0000 Subject: [PATCH 41/67] Adapt ChangeLog --- ChangeLog | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/ChangeLog b/ChangeLog index b67c2ed50..49c3acf5f 100644 --- a/ChangeLog +++ b/ChangeLog @@ -42,6 +42,13 @@ Features API Changes * Add a new X.509 API call `mbedtls_x509_parse_der_nocopy()`. See the Features section for more information. + * Allow to opt in to the removal the API mbedtls_ssl_get_peer_cert() + for the benefit of saving RAM, by disabling the new compile-time + option MBEDTLS_SSL_KEEP_PEER_CERTIFICATE (enabled by default for + API stability). Disabling this option makes mbedtls_ssl_get_peer_cert() + always return NULL, and removes the peer_cert field from the + mbedtls_ssl_session structure which otherwise stores the peer's + certificate. Bugfix * Server's RSA certificate in certs.c was SHA-1 signed. In the default From 17572473c6ec3741560f2cd2d7a0b07ea9567ceb Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 8 Feb 2019 07:19:04 +0000 Subject: [PATCH 42/67] Correct compile-time guards for ssl_clear_peer_cert() It is used in `mbedtls_ssl_session_free()` under `MBEDTLS_X509_CRT_PARSE_C`, but defined only if `MBEDTLS_KEY_EXCHANGE__WITH_CERT__ENABLED`. Issue #2422 tracks the use of `MBEDTLS_KEY_EXCHANGE__WITH_CERT_ENABLED` instead of `MBEDTLS_X509_CRT_PARSE_C` for code and fields related to CRT-based ciphersuites. --- library/ssl_tls.c | 44 +++++++++++++++++++++++--------------------- 1 file changed, 23 insertions(+), 21 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index ab457b7a6..46f86ddf5 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -5984,6 +5984,29 @@ int mbedtls_ssl_send_alert_message( mbedtls_ssl_context *ssl, return( 0 ); } +#if defined(MBEDTLS_X509_CRT_PARSE_C) +static void ssl_clear_peer_cert( mbedtls_ssl_session *session ) +{ +#if defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) + if( session->peer_cert != NULL ) + { + mbedtls_x509_crt_free( session->peer_cert ); + mbedtls_free( session->peer_cert ); + session->peer_cert = NULL; + } +#else /* MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ + if( session->peer_cert_digest != NULL ) + { + /* Zeroization is not necessary. */ + mbedtls_free( session->peer_cert_digest ); + session->peer_cert_digest = NULL; + session->peer_cert_digest_type = MBEDTLS_MD_NONE; + session->peer_cert_digest_len = 0; + } +#endif /* !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ +} +#endif /* MBEDTLS_X509_CRT_PARSE_C */ + /* * Handshake functions */ @@ -6187,27 +6210,6 @@ static int ssl_check_peer_crt_unchanged( mbedtls_ssl_context *ssl, #endif /* MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ #endif /* MBEDTLS_SSL_RENEGOTIATION && MBEDTLS_SSL_CLI_C */ -static void ssl_clear_peer_cert( mbedtls_ssl_session *session ) -{ -#if defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) - if( session->peer_cert != NULL ) - { - mbedtls_x509_crt_free( session->peer_cert ); - mbedtls_free( session->peer_cert ); - session->peer_cert = NULL; - } -#else - if( session->peer_cert_digest != NULL ) - { - /* Zeroization is not necessary. */ - mbedtls_free( session->peer_cert_digest ); - session->peer_cert_digest = NULL; - session->peer_cert_digest_type = MBEDTLS_MD_NONE; - session->peer_cert_digest_len = 0; - } -#endif /* !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ -} - /* * Once the certificate message is read, parse it into a cert chain and * perform basic checks, but leave actual verification to the caller From 6c83db7f7bbf3c04731a399d49025ee6477bab6f Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 8 Feb 2019 14:06:00 +0000 Subject: [PATCH 43/67] Free peer's public key as soon as it's no longer needed On constrained devices, this saves a significant amount of RAM that might be needed for subsequent expensive operations like ECDHE. --- library/ssl_cli.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/library/ssl_cli.c b/library/ssl_cli.c index e39ce7a95..21797e7cf 100644 --- a/library/ssl_cli.c +++ b/library/ssl_cli.c @@ -2354,6 +2354,10 @@ static int ssl_write_encrypted_pms( mbedtls_ssl_context *ssl, } #endif +#if !defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) + /* We don't need the peer's public key anymore. Free it. */ + mbedtls_pk_free( peer_pk ); +#endif /* !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ return( 0 ); } #endif /* MBEDTLS_KEY_EXCHANGE_RSA_ENABLED || @@ -2463,6 +2467,13 @@ static int ssl_get_ecdh_params_from_cert( mbedtls_ssl_context *ssl ) return( MBEDTLS_ERR_SSL_BAD_HS_CERTIFICATE ); } +#if !defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) + /* We don't need the peer's public key anymore. Free it, + * so that more RAM is available for upcoming expensive + * operations like ECDHE. */ + mbedtls_pk_free( peer_pk ); +#endif /* !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ + return( ret ); } #endif /* MBEDTLS_KEY_EXCHANGE_ECDH_RSA_ENABLED) || @@ -2801,6 +2812,13 @@ start_processing: #endif return( ret ); } + +#if !defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) + /* We don't need the peer's public key anymore. Free it, + * so that more RAM is available for upcoming expensive + * operations like ECDHE. */ + mbedtls_pk_free( peer_pk ); +#endif /* !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ } #endif /* MBEDTLS_KEY_EXCHANGE__WITH_SERVER_SIGNATURE__ENABLED */ From 0cc7af5be568d295eb54b45860ef3cf50c05af7c Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 8 Feb 2019 14:39:16 +0000 Subject: [PATCH 44/67] Parse peer's CRT chain in-place from the input buffer --- library/ssl_tls.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 46f86ddf5..283586757 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -6324,7 +6324,13 @@ static int ssl_parse_certificate_chain( mbedtls_ssl_context *ssl, #endif /* MBEDTLS_SSL_RENEGOTIATION && MBEDTLS_SSL_CLI_C */ /* Parse the next certificate in the chain. */ +#if defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) ret = mbedtls_x509_crt_parse_der( chain, ssl->in_msg + i, n ); +#else + /* If we don't need to store the CRT chani permanently, parse + * it in-place from the input buffer instead of making a copy. */ + ret = mbedtls_x509_crt_parse_der_nocopy( chain, ssl->in_msg + i, n ); +#endif /* MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ switch( ret ) { case 0: /*ok*/ From 34106f6ae25142f1e11629c522b4bc8608d22499 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 8 Feb 2019 14:59:05 +0000 Subject: [PATCH 45/67] Free peer CRT chain immediately after verifying it If we don't need to store the peer's CRT chain permanently, we may free it immediately after verifying it. Moreover, since we parse the CRT chain in-place from the input buffer in this case, pointers from the CRT structure remain valid after freeing the structure, and we use that to extract the digest and pubkey from the CRT after freeing the structure. --- library/ssl_tls.c | 116 ++++++++++++++++++++++++++++++---------------- 1 file changed, 77 insertions(+), 39 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 283586757..f31f1cb04 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -6587,6 +6587,58 @@ static int ssl_parse_certificate_verify( mbedtls_ssl_context *ssl, return( ret ); } +#if !defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) +static int ssl_remember_peer_crt_digest( mbedtls_ssl_context *ssl, + unsigned char *start, size_t len ) +{ + int ret; + /* Remember digest of the peer's end-CRT. */ + ssl->session_negotiate->peer_cert_digest = + mbedtls_calloc( 1, MBEDTLS_SSL_PEER_CERT_DIGEST_DFL_LEN ); + if( ssl->session_negotiate->peer_cert_digest == NULL ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "alloc(%d bytes) failed", + sizeof( MBEDTLS_SSL_PEER_CERT_DIGEST_DFL_LEN ) ) ); + mbedtls_ssl_send_alert_message( ssl, + MBEDTLS_SSL_ALERT_LEVEL_FATAL, + MBEDTLS_SSL_ALERT_MSG_INTERNAL_ERROR ); + + return( MBEDTLS_ERR_SSL_ALLOC_FAILED ); + } + + ret = mbedtls_md( mbedtls_md_info_from_type( + MBEDTLS_SSL_PEER_CERT_DIGEST_DFL_TYPE ), + start, len, + ssl->session_negotiate->peer_cert_digest ); + + ssl->session_negotiate->peer_cert_digest_type = + MBEDTLS_SSL_PEER_CERT_DIGEST_DFL_TYPE; + ssl->session_negotiate->peer_cert_digest_len = + MBEDTLS_SSL_PEER_CERT_DIGEST_DFL_LEN; + + return( ret ); +} + +static int ssl_remember_peer_pubkey( mbedtls_ssl_context *ssl, + unsigned char *start, size_t len ) +{ + unsigned char *end = start + len; + int ret; + + /* Make a copy of the peer's raw public key. */ + mbedtls_pk_init( &ssl->handshake->peer_pubkey ); + ret = mbedtls_pk_parse_subpubkey( &start, end, + &ssl->handshake->peer_pubkey ); + if( ret != 0 ) + { + /* We should have parsed the public key before. */ + return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); + } + + return( 0 ); +} +#endif /* !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ + int mbedtls_ssl_parse_certificate( mbedtls_ssl_context *ssl ) { int ret = 0; @@ -6679,54 +6731,40 @@ crt_verify: goto exit; #if !defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) - - /* Remember digest of the peer's end-CRT. */ - ssl->session_negotiate->peer_cert_digest = - mbedtls_calloc( 1, MBEDTLS_SSL_PEER_CERT_DIGEST_DFL_LEN ); - if( ssl->session_negotiate->peer_cert_digest == NULL ) { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "alloc(%d bytes) failed", - sizeof( MBEDTLS_SSL_PEER_CERT_DIGEST_DFL_LEN ) ) ); - mbedtls_ssl_send_alert_message( ssl, - MBEDTLS_SSL_ALERT_LEVEL_FATAL, - MBEDTLS_SSL_ALERT_MSG_INTERNAL_ERROR ); + unsigned char *crt_start, *pk_start; + size_t crt_len, pk_len; - ret = MBEDTLS_ERR_SSL_ALLOC_FAILED; - goto exit; - } - ret = mbedtls_md( mbedtls_md_info_from_type( - MBEDTLS_SSL_PEER_CERT_DIGEST_DFL_TYPE ), - chain->raw.p, chain->raw.len, - ssl->session_negotiate->peer_cert_digest ); - if( ret != 0 ) - goto exit; + /* We parse the CRT chain without copying, so + * these pointers point into the input buffer, + * and are hence still valid after freeing the + * CRT chain. */ - ssl->session_negotiate->peer_cert_digest_type = - MBEDTLS_SSL_PEER_CERT_DIGEST_DFL_TYPE; - ssl->session_negotiate->peer_cert_digest_len = - MBEDTLS_SSL_PEER_CERT_DIGEST_DFL_LEN; -#endif /* !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ + crt_start = chain->raw.p; + crt_len = chain->raw.len; -#if !defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) - /* Make a copy of the peer's raw public key. */ - mbedtls_pk_init( &ssl->handshake->peer_pubkey ); - { - unsigned char *p, *end; - p = chain->pk_raw.p; - end = p + chain->pk_raw.len; - ret = mbedtls_pk_parse_subpubkey( &p, end, - &ssl->handshake->peer_pubkey ); + pk_start = chain->pk_raw.p; + pk_len = chain->pk_raw.len; + + /* Free the CRT structures before computing + * digest and copying the peer's public key. */ + mbedtls_x509_crt_free( chain ); + mbedtls_free( chain ); + chain = NULL; + + ret = ssl_remember_peer_crt_digest( ssl, crt_start, crt_len ); + if( ret != 0 ) + goto exit; + + ret = ssl_remember_peer_pubkey( ssl, pk_start, pk_len ); if( ret != 0 ) - { - /* We should have parsed the public key before. */ - ret = MBEDTLS_ERR_SSL_INTERNAL_ERROR; goto exit; - } } -#else +#else /* !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ + /* Pass ownership to session structure. */ ssl->session_negotiate->peer_cert = chain; chain = NULL; -#endif /* !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ +#endif /* MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= parse certificate" ) ); From 92820a1dff39f705ec00c5c9ce28cca404dba2a6 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 19 Feb 2019 11:10:48 +0000 Subject: [PATCH 46/67] Add test for !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE to all.sh --- tests/scripts/all.sh | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/tests/scripts/all.sh b/tests/scripts/all.sh index cd6d2dabc..628a05644 100755 --- a/tests/scripts/all.sh +++ b/tests/scripts/all.sh @@ -885,6 +885,22 @@ component_test_no_max_fragment_length () { if_build_succeeded tests/ssl-opt.sh -f "Max fragment length" } +component_test_asan_remove_peer_certificate () { + msg "build: default config with MBEDTLS_SSL_KEEP_PEER_CERTIFICATE disabled (ASan build)" + scripts/config.pl unset MBEDTLS_SSL_KEEP_PEER_CERTIFICATE + CC=gcc cmake -D CMAKE_BUILD_TYPE:String=Asan . + make + + msg "test: !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE" + make test + + msg "test: ssl-opt.sh, !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE" + if_build_succeeded tests/ssl-opt.sh + + msg "test: compat.sh, !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE" + if_build_succeeded tests/compat.sh +} + component_test_no_max_fragment_length_small_ssl_out_content_len () { msg "build: no MFL extension, small SSL_OUT_CONTENT_LEN (ASan build)" scripts/config.pl unset MBEDTLS_SSL_MAX_FRAGMENT_LENGTH From e669770b521d0c9525e4177a202671e4d6b544f3 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 22 Feb 2019 16:27:15 +0000 Subject: [PATCH 47/67] Remove misleading and redundant guard around restartable ECC field `MBEDTLS_SSL__ECP_RESTARTABLE` is only defined if `MBEDTLS_KEY_EXCHANGE_ECDHE_ECDSA_ENABLED` is set, which requires `MBEDTLS_X509_PARSE_C` to be set (this is checked in `check_config.`). The additional `MBEDTLS_X509_PARSE_C` guard around the `ecrs_peer_cert` field is therefore not necessary; moreover, it's misleading, because it hasn't been used consistently throughout the code. --- include/mbedtls/ssl_internal.h | 2 -- 1 file changed, 2 deletions(-) diff --git a/include/mbedtls/ssl_internal.h b/include/mbedtls/ssl_internal.h index 7b9265c51..d18656fb1 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -396,9 +396,7 @@ struct mbedtls_ssl_handshake_params ssl_ecrs_cke_ecdh_calc_secret, /*!< ClientKeyExchange: ECDH step 2 */ ssl_ecrs_crt_vrfy_sign, /*!< CertificateVerify: pk_sign() */ } ecrs_state; /*!< current (or last) operation */ -#if defined(MBEDTLS_X509_CRT_PARSE_C) mbedtls_x509_crt *ecrs_peer_cert; /*!< The peer's CRT chain. */ -#endif /* MBEDTLS_X509_CRT_PARSE_C */ size_t ecrs_n; /*!< place for saving a length */ #endif #if defined(MBEDTLS_X509_CRT_PARSE_C) && \ From 257ef65d9445cacaaa2cbac29aacb241245728f0 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Mon, 25 Feb 2019 10:03:26 +0000 Subject: [PATCH 48/67] Remove question in comment about verify flags on cli vs. server --- library/ssl_tls.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index f31f1cb04..018029f7c 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -6429,8 +6429,6 @@ static int ssl_parse_certificate_coordinate( mbedtls_ssl_context *ssl, if( authmode == MBEDTLS_SSL_VERIFY_NONE ) { - /* NOTE: Is it intentional that we set verify_result - * to SKIP_VERIFY on server-side only? */ ssl->session_negotiate->verify_result = MBEDTLS_X509_BADCERT_SKIP_VERIFY; return( SSL_CERTIFICATE_SKIP ); From 9d64b789cf4e0b1374bd5f323fb6a8dff55703a3 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Mon, 25 Feb 2019 10:06:59 +0000 Subject: [PATCH 49/67] Set peer CRT length only after successful allocation --- 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 018029f7c..07e30a369 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -398,15 +398,15 @@ int mbedtls_ssl_session_copy( mbedtls_ssl_session *dst, #else /* MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ if( src->peer_cert_digest != NULL ) { - dst->peer_cert_digest_len = src->peer_cert_digest_len; dst->peer_cert_digest = - mbedtls_calloc( 1, dst->peer_cert_digest_len ); + mbedtls_calloc( 1, src->peer_cert_digest_len ); if( dst->peer_cert_digest == NULL ) return( MBEDTLS_ERR_SSL_ALLOC_FAILED ); memcpy( dst->peer_cert_digest, src->peer_cert_digest, src->peer_cert_digest_len ); dst->peer_cert_digest_type = src->peer_cert_digest_type; + dst->peer_cert_digest_len = src->peer_cert_digest_len; } #endif /* MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ From dd689316d148f992307601a2975d9da139ed1cc5 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Mon, 25 Feb 2019 10:08:06 +0000 Subject: [PATCH 50/67] Fix indentation of Doxygen comment in ssl_internal.h --- include/mbedtls/ssl_internal.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/mbedtls/ssl_internal.h b/include/mbedtls/ssl_internal.h index d18656fb1..3eb37b8c3 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -396,7 +396,7 @@ struct mbedtls_ssl_handshake_params ssl_ecrs_cke_ecdh_calc_secret, /*!< ClientKeyExchange: ECDH step 2 */ ssl_ecrs_crt_vrfy_sign, /*!< CertificateVerify: pk_sign() */ } ecrs_state; /*!< current (or last) operation */ - mbedtls_x509_crt *ecrs_peer_cert; /*!< The peer's CRT chain. */ + mbedtls_x509_crt *ecrs_peer_cert; /*!< The peer's CRT chain. */ size_t ecrs_n; /*!< place for saving a length */ #endif #if defined(MBEDTLS_X509_CRT_PARSE_C) && \ From 3ed64578d26fe72c1f4df35ebc64a4d365e7a84f Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Mon, 25 Feb 2019 10:13:33 +0000 Subject: [PATCH 51/67] Improve documentation of MBEDTLS_SSL_KEEP_PEER_CERTIFICATE --- include/mbedtls/config.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/mbedtls/config.h b/include/mbedtls/config.h index 0ea17fb74..e5d593312 100644 --- a/include/mbedtls/config.h +++ b/include/mbedtls/config.h @@ -1407,7 +1407,7 @@ /** * \def MBEDTLS_SSL_KEEP_PEER_CERTIFICATE * - * This option controls the presence of the API mbedtls_ssl_get_peer_cert() + * This option controls the availability of the API mbedtls_ssl_get_peer_cert() * giving access to the peer's certificate after completion of the handshake. * * Unless you need mbedtls_ssl_peer_cert() in your application, it is From 24bc5708144105d04acbedd56014a73afbc61500 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Mon, 25 Feb 2019 10:13:43 +0000 Subject: [PATCH 52/67] Improve documentation of mbedtls_ssl_get_peer_cert() --- include/mbedtls/ssl.h | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/include/mbedtls/ssl.h b/include/mbedtls/ssl.h index 0e5c1fe98..65c8d94d9 100644 --- a/include/mbedtls/ssl.h +++ b/include/mbedtls/ssl.h @@ -3256,20 +3256,16 @@ int mbedtls_ssl_get_max_out_record_payload( const mbedtls_ssl_context *ssl ); /** * \brief Return the peer certificate from the current connection. * - * For ciphersuites not using certificate-based peer - * authentication (such as PSK-based ciphersuites), no - * peer certificate is available, and this function returns - * \c NULL. - * * \param ssl The SSL context to use. This must be initialized and setup. * - * \return The current peer certificate, or \c NULL if - * none is available, which might be because the chosen - * ciphersuite does not use peer certificates, or because - * #MBEDTLS_SSL_KEEP_PEER_CERTIFICATE has been disabled. - * If this functions does not return \c NULL, the returned - * certificate is owned by the SSL context and valid only - * until the next call to the SSL API. + * \return The current peer certificate, if available. + * The returned certificate is owned by the SSL context and + * is valid only until the next call to the SSL API. + * \return \c NULL if no peer certificate is available. This might + * be because the chosen ciphersuite doesn't use CRTs + * (PSK-based ciphersuites, for example), or because + * #MBEDTLS_SSL_KEEP_PEER_CERTIFICATE has been disabled, + * allowing the stack to free the peer's CRT to save memory. * * \note For one-time inspection of the peer's certificate during * the handshake, consider registering an X.509 CRT verification From 975c463b3f7dd2f6233a370b6a1c2e4f0cb7adb1 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Mon, 25 Feb 2019 17:43:18 +0000 Subject: [PATCH 53/67] ssl_client2: Extract peer CRT info from verification callback So far, `ssl_client2` printed the CRT info for the peer's CRT by requesting the latter through `mbedtls_ssl_get_peer_cert()` at the end of the handshake, and printing it via `mbedtls_x509_crt_info()`. When `MBEDTLS_SSL_KEEP_PEER_CERTIFICATE` is disabled, this does no longer work because the peer's CRT isn't stored beyond the handshake. This makes some tests in `ssl-opt.sh` fail which rely on the CRT info output for the peer certificate. This commit modifies `ssl_client2` to extract the peer CRT info from the verification callback, which is always called at a time when the peer's CRT is available. This way, the peer's CRT info is still printed if `MBEDTLS_SSL_KEEP_PEER_CERTIFICATE` is disabled. --- programs/ssl/ssl_client2.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/programs/ssl/ssl_client2.c b/programs/ssl/ssl_client2.c index 93633161c..72d857b61 100644 --- a/programs/ssl/ssl_client2.c +++ b/programs/ssl/ssl_client2.c @@ -494,6 +494,8 @@ static int my_send( void *ctx, const unsigned char *buf, size_t len ) } #if defined(MBEDTLS_X509_CRT_PARSE_C) +static unsigned char peer_crt_info[1024] = { 0 }; + /* * Enabled if debug_level > 1 in code below */ @@ -506,8 +508,14 @@ static int my_verify( void *data, mbedtls_x509_crt *crt, ((void) data); #if !defined(MBEDTLS_X509_REMOVE_INFO) - mbedtls_printf( "\nVerify requested for (Depth %d):\n", depth ); mbedtls_x509_crt_info( buf, sizeof( buf ) - 1, "", crt ); + if( depth == 0 ) + memcpy( peer_crt_info, buf, sizeof( buf ) ); + + if( opt.debug_level == 0 ) + return( 0 ); + + mbedtls_printf( "\nVerify requested for (Depth %d):\n", depth ); mbedtls_printf( "%s", buf ); #else ((void) crt); @@ -1641,8 +1649,7 @@ int main( int argc, char *argv[] ) mbedtls_ssl_conf_sig_hashes( &conf, ssl_sig_hashes_for_test ); } - if( opt.debug_level > 0 ) - mbedtls_ssl_conf_verify( &conf, my_verify, NULL ); + mbedtls_ssl_conf_verify( &conf, my_verify, NULL ); #endif /* MBEDTLS_X509_CRT_PARSE_C */ #if defined(MBEDTLS_SSL_DTLS_CONNECTION_ID) @@ -2021,13 +2028,8 @@ int main( int argc, char *argv[] ) mbedtls_printf( " ok\n" ); #if !defined(MBEDTLS_X509_REMOVE_INFO) - if( mbedtls_ssl_get_peer_cert( &ssl ) != NULL ) - { - mbedtls_printf( " . Peer certificate information ...\n" ); - mbedtls_x509_crt_info( (char *) buf, sizeof( buf ) - 1, " ", - mbedtls_ssl_get_peer_cert( &ssl ) ); - mbedtls_printf( "%s\n", buf ); - } + mbedtls_printf( " . Peer certificate information ...\n" ); + mbedtls_printf( "%s\n", peer_crt_info ); #endif /* !MBEDTLS_X509_REMOVE_INFO */ #endif /* MBEDTLS_X509_CRT_PARSE_C */ From 890d7ee4cbc90bf2ae5e9680466c2e9168356a36 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Mon, 25 Feb 2019 18:01:57 +0000 Subject: [PATCH 54/67] Reintroduce numerous ssl-opt.sh tests if !MBEDTLS_SSL_KEEP_PEER_CERT --- tests/ssl-opt.sh | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index 7025d88da..3dd69a5f2 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -3597,7 +3597,6 @@ run_test "Authentication: send CA list in CertificateRequest, client self sig # Tests for certificate selection based on SHA verson requires_config_disabled MBEDTLS_X509_REMOVE_INFO -requires_config_enabled MBEDTLS_SSL_KEEP_PEER_CERTIFICATE run_test "Certificate hash: client TLS 1.2 -> SHA-2" \ "$P_SRV crt_file=data_files/server5.crt \ key_file=data_files/server5.key \ @@ -3609,7 +3608,6 @@ run_test "Certificate hash: client TLS 1.2 -> SHA-2" \ -C "signed using.*ECDSA with SHA1" requires_config_disabled MBEDTLS_X509_REMOVE_INFO -requires_config_enabled MBEDTLS_SSL_KEEP_PEER_CERTIFICATE run_test "Certificate hash: client TLS 1.1 -> SHA-1" \ "$P_SRV crt_file=data_files/server5.crt \ key_file=data_files/server5.key \ @@ -3621,7 +3619,6 @@ run_test "Certificate hash: client TLS 1.1 -> SHA-1" \ -c "signed using.*ECDSA with SHA1" requires_config_disabled MBEDTLS_X509_REMOVE_INFO -requires_config_enabled MBEDTLS_SSL_KEEP_PEER_CERTIFICATE run_test "Certificate hash: client TLS 1.0 -> SHA-1" \ "$P_SRV crt_file=data_files/server5.crt \ key_file=data_files/server5.key \ @@ -3633,7 +3630,6 @@ run_test "Certificate hash: client TLS 1.0 -> SHA-1" \ -c "signed using.*ECDSA with SHA1" requires_config_disabled MBEDTLS_X509_REMOVE_INFO -requires_config_enabled MBEDTLS_SSL_KEEP_PEER_CERTIFICATE run_test "Certificate hash: client TLS 1.1, no SHA-1 -> SHA-2 (order 1)" \ "$P_SRV crt_file=data_files/server5.crt \ key_file=data_files/server5.key \ @@ -3646,7 +3642,6 @@ run_test "Certificate hash: client TLS 1.1, no SHA-1 -> SHA-2 (order 1)" \ -C "signed using.*ECDSA with SHA1" requires_config_disabled MBEDTLS_X509_REMOVE_INFO -requires_config_enabled MBEDTLS_SSL_KEEP_PEER_CERTIFICATE run_test "Certificate hash: client TLS 1.1, no SHA-1 -> SHA-2 (order 2)" \ "$P_SRV crt_file=data_files/server6.crt \ key_file=data_files/server6.key \ @@ -3661,7 +3656,6 @@ run_test "Certificate hash: client TLS 1.1, no SHA-1 -> SHA-2 (order 2)" \ # tests for SNI requires_config_disabled MBEDTLS_X509_REMOVE_INFO -requires_config_enabled MBEDTLS_SSL_KEEP_PEER_CERTIFICATE run_test "SNI: no SNI callback" \ "$P_SRV debug_level=3 \ crt_file=data_files/server5.crt key_file=data_files/server5.key" \ @@ -3672,7 +3666,6 @@ run_test "SNI: no SNI callback" \ -c "subject name *: C=NL, O=PolarSSL, CN=localhost" requires_config_disabled MBEDTLS_X509_REMOVE_INFO -requires_config_enabled MBEDTLS_SSL_KEEP_PEER_CERTIFICATE run_test "SNI: matching cert 1" \ "$P_SRV debug_level=3 \ crt_file=data_files/server5.crt key_file=data_files/server5.key \ @@ -3684,7 +3677,6 @@ run_test "SNI: matching cert 1" \ -c "subject name *: C=NL, O=PolarSSL, CN=localhost" requires_config_disabled MBEDTLS_X509_REMOVE_INFO -requires_config_enabled MBEDTLS_SSL_KEEP_PEER_CERTIFICATE run_test "SNI: matching cert 2" \ "$P_SRV debug_level=3 \ crt_file=data_files/server5.crt key_file=data_files/server5.key \ @@ -3806,7 +3798,6 @@ run_test "SNI: CA override with CRL" \ # Tests for SNI and DTLS requires_config_disabled MBEDTLS_X509_REMOVE_INFO -requires_config_enabled MBEDTLS_SSL_KEEP_PEER_CERTIFICATE run_test "SNI: DTLS, no SNI callback" \ "$P_SRV debug_level=3 dtls=1 \ crt_file=data_files/server5.crt key_file=data_files/server5.key" \ @@ -3817,7 +3808,6 @@ run_test "SNI: DTLS, no SNI callback" \ -c "subject name *: C=NL, O=PolarSSL, CN=localhost" requires_config_disabled MBEDTLS_X509_REMOVE_INFO -requires_config_enabled MBEDTLS_SSL_KEEP_PEER_CERTIFICATE run_test "SNI: DTLS, matching cert 1" \ "$P_SRV debug_level=3 dtls=1 \ crt_file=data_files/server5.crt key_file=data_files/server5.key \ @@ -3829,7 +3819,6 @@ run_test "SNI: DTLS, matching cert 1" \ -c "subject name *: C=NL, O=PolarSSL, CN=localhost" requires_config_disabled MBEDTLS_X509_REMOVE_INFO -requires_config_enabled MBEDTLS_SSL_KEEP_PEER_CERTIFICATE run_test "SNI: DTLS, matching cert 2" \ "$P_SRV debug_level=3 dtls=1 \ crt_file=data_files/server5.crt key_file=data_files/server5.key \ From f9ca30d042f3a676eed55efdb3c3f758623f37be Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 26 Feb 2019 11:38:29 +0000 Subject: [PATCH 55/67] ssl_client2: Zeroize peer CRT info buffer when reconnecting --- programs/ssl/ssl_client2.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/programs/ssl/ssl_client2.c b/programs/ssl/ssl_client2.c index 72d857b61..1af760fc1 100644 --- a/programs/ssl/ssl_client2.c +++ b/programs/ssl/ssl_client2.c @@ -494,7 +494,7 @@ static int my_send( void *ctx, const unsigned char *buf, size_t len ) } #if defined(MBEDTLS_X509_CRT_PARSE_C) -static unsigned char peer_crt_info[1024] = { 0 }; +static unsigned char peer_crt_info[1024]; /* * Enabled if debug_level > 1 in code below @@ -1650,6 +1650,7 @@ int main( int argc, char *argv[] ) } mbedtls_ssl_conf_verify( &conf, my_verify, NULL ); + memset( peer_crt_info, 0, sizeof( peer_crt_info ) ); #endif /* MBEDTLS_X509_CRT_PARSE_C */ #if defined(MBEDTLS_SSL_DTLS_CONNECTION_ID) @@ -2513,6 +2514,8 @@ reconnect: mbedtls_printf( " . Reconnecting with saved session..." ); + memset( peer_crt_info, 0, sizeof( peer_crt_info ) ); + if( ( ret = mbedtls_ssl_session_reset( &ssl ) ) != 0 ) { mbedtls_printf( " failed\n ! mbedtls_ssl_session_reset returned -0x%x\n\n", From 2984bd254317b2a2b660227d7e09c68ccf6d8d73 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 26 Feb 2019 11:43:09 +0000 Subject: [PATCH 56/67] Add config sanity check for !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE --- include/mbedtls/check_config.h | 8 ++++++++ include/mbedtls/ssl.h | 7 +++++-- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/include/mbedtls/check_config.h b/include/mbedtls/check_config.h index 00fb9b367..76ab07a9f 100644 --- a/include/mbedtls/check_config.h +++ b/include/mbedtls/check_config.h @@ -278,6 +278,14 @@ #error "MBEDTLS_KEY_EXCHANGE_ECJPAKE_ENABLED defined, but not all prerequisites" #endif +#if defined(MBEDTLS_KEY_EXCHANGE__WITH_CERT__ENABLED) && \ + !defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) && \ + ( !defined(MBEDTLS_SHA256_C) && \ + !defined(MBEDTLS_SHA512_C) && \ + !defined(MBEDTLS_SHA1_C) ) +#error "!MBEDTLS_SSL_KEEP_PEER_CERTIFICATE requires MBEDTLS_SHA512_C, MBEDTLS_SHA256_C or MBEDTLS_SHA1_C" +#endif + #if defined(MBEDTLS_MEMORY_BUFFER_ALLOC_C) && \ ( !defined(MBEDTLS_PLATFORM_C) || !defined(MBEDTLS_PLATFORM_MEMORY) ) #error "MBEDTLS_MEMORY_BUFFER_ALLOC_C defined, but not all prerequisites" diff --git a/include/mbedtls/ssl.h b/include/mbedtls/ssl.h index 65c8d94d9..9a36009a7 100644 --- a/include/mbedtls/ssl.h +++ b/include/mbedtls/ssl.h @@ -812,7 +812,8 @@ typedef int mbedtls_ssl_async_resume_t( mbedtls_ssl_context *ssl, typedef void mbedtls_ssl_async_cancel_t( mbedtls_ssl_context *ssl ); #endif /* MBEDTLS_SSL_ASYNC_PRIVATE */ -#if defined(MBEDTLS_X509_CRT_PARSE_C) +#if defined(MBEDTLS_KEY_EXCHANGE__WITH_CERT__ENABLED) && \ + !defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) #define MBEDTLS_SSL_PEER_CERT_DIGEST_MAX_LEN 48 #if defined(MBEDTLS_SHA256_C) #define MBEDTLS_SSL_PEER_CERT_DIGEST_DFL_TYPE MBEDTLS_MD_SHA256 @@ -824,9 +825,11 @@ typedef void mbedtls_ssl_async_cancel_t( mbedtls_ssl_context *ssl ); #define MBEDTLS_SSL_PEER_CERT_DIGEST_DFL_TYPE MBEDTLS_MD_SHA1 #define MBEDTLS_SSL_PEER_CERT_DIGEST_DFL_LEN 20 #else +/* This is already checked in check_config.h, but be sure. */ #error "Bad configuration - need SHA-1, SHA-256 or SHA-512 enabled to compute digest of peer CRT." #endif -#endif /* MBEDTLS_X509_CRT_PARSE_C */ +#endif /* MBEDTLS_KEY_EXCHANGE__WITH_CERT__ENABLED && + !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ /* * This structure is used for storing current session data. From e9839c001b8102545e6777f425ddb5c82e20f576 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 26 Feb 2019 11:51:06 +0000 Subject: [PATCH 57/67] Add debug output in case of assertion failure --- library/ssl_cli.c | 1 + 1 file changed, 1 insertion(+) diff --git a/library/ssl_cli.c b/library/ssl_cli.c index 21797e7cf..c1cb6c32d 100644 --- a/library/ssl_cli.c +++ b/library/ssl_cli.c @@ -2320,6 +2320,7 @@ static int ssl_write_encrypted_pms( mbedtls_ssl_context *ssl, if( ssl->session_negotiate->peer_cert == NULL ) { /* Should never happen */ + MBEDTLS_SSL_DEBUG_MSG( 1, ( "should never happen" ) ); return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); } peer_pk = &ssl->session_negotiate->peer_cert->pk; From 42de8f8a426c925ba5c0d33ed51cf54f08301952 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 26 Feb 2019 11:51:34 +0000 Subject: [PATCH 58/67] Fix typo in documentation of ssl_parse_certificate_chain() --- library/ssl_tls.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 07e30a369..fce29f5fa 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -6327,7 +6327,7 @@ static int ssl_parse_certificate_chain( mbedtls_ssl_context *ssl, #if defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) ret = mbedtls_x509_crt_parse_der( chain, ssl->in_msg + i, n ); #else - /* If we don't need to store the CRT chani permanently, parse + /* If we don't need to store the CRT chain permanently, parse * it in-place from the input buffer instead of making a copy. */ ret = mbedtls_x509_crt_parse_der_nocopy( chain, ssl->in_msg + i, n ); #endif /* MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ From c39e23ebb68cf0bb49f68a54f77bf88494d04dd1 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 26 Feb 2019 12:36:01 +0000 Subject: [PATCH 59/67] Add further debug statements on assertion failures --- library/ssl_cli.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/library/ssl_cli.c b/library/ssl_cli.c index c1cb6c32d..e5185d4c5 100644 --- a/library/ssl_cli.c +++ b/library/ssl_cli.c @@ -2442,6 +2442,7 @@ static int ssl_get_ecdh_params_from_cert( mbedtls_ssl_context *ssl ) if( ssl->session_negotiate->peer_cert == NULL ) { /* Should never happen */ + MBEDTLS_SSL_DEBUG_MSG( 1, ( "should never happen" ) ); return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); } peer_pk = &ssl->session_negotiate->peer_cert->pk; @@ -2777,6 +2778,7 @@ start_processing: if( ssl->session_negotiate->peer_cert == NULL ) { /* Should never happen */ + MBEDTLS_SSL_DEBUG_MSG( 1, ( "should never happen" ) ); return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); } peer_pk = &ssl->session_negotiate->peer_cert->pk; From b7fab76890df42725b14f37bf9aa42ecffd5cae7 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 26 Feb 2019 12:36:53 +0000 Subject: [PATCH 60/67] ssl_client2: Reset peer CRT info string on reconnect --- programs/ssl/ssl_client2.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/programs/ssl/ssl_client2.c b/programs/ssl/ssl_client2.c index 1af760fc1..ac4d31ce1 100644 --- a/programs/ssl/ssl_client2.c +++ b/programs/ssl/ssl_client2.c @@ -2360,6 +2360,8 @@ send_request: mbedtls_printf( " . Restarting connection from same port..." ); fflush( stdout ); + memset( peer_crt_info, 0, sizeof( peer_crt_info ) ); + if( ( ret = mbedtls_ssl_session_reset( &ssl ) ) != 0 ) { mbedtls_printf( " failed\n ! mbedtls_ssl_session_reset returned -0x%x\n\n", From b6f72417419a6850b30569327611a2847be196d9 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 26 Feb 2019 14:38:40 +0000 Subject: [PATCH 61/67] Update programs/ssl/query_config.c --- programs/ssl/query_config.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/programs/ssl/query_config.c b/programs/ssl/query_config.c index dbb6cc1a6..ab3c772ab 100644 --- a/programs/ssl/query_config.c +++ b/programs/ssl/query_config.c @@ -1258,6 +1258,14 @@ int query_config( const char *config ) } #endif /* MBEDTLS_SSL_FALLBACK_SCSV */ +#if defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) + if( strcmp( "MBEDTLS_SSL_KEEP_PEER_CERTIFICATE", config ) == 0 ) + { + MACRO_EXPANSION_TO_STR( MBEDTLS_SSL_KEEP_PEER_CERTIFICATE ); + return( 0 ); + } +#endif /* MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ + #if defined(MBEDTLS_SSL_HW_RECORD_ACCEL) if( strcmp( "MBEDTLS_SSL_HW_RECORD_ACCEL", config ) == 0 ) { From 488c8dee47a8df3f270179072e8d876f25eec748 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 27 Feb 2019 08:34:31 +0000 Subject: [PATCH 62/67] Add missing compile time guard in ssl_client2 --- programs/ssl/ssl_client2.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/programs/ssl/ssl_client2.c b/programs/ssl/ssl_client2.c index ac4d31ce1..d859101c1 100644 --- a/programs/ssl/ssl_client2.c +++ b/programs/ssl/ssl_client2.c @@ -2360,7 +2360,9 @@ send_request: mbedtls_printf( " . Restarting connection from same port..." ); fflush( stdout ); +#if defined(MBEDTLS_X509_CRT_PARSE_C) memset( peer_crt_info, 0, sizeof( peer_crt_info ) ); +#endif /* MBEDTLS_X509_CRT_PARSE_C */ if( ( ret = mbedtls_ssl_session_reset( &ssl ) ) != 0 ) { @@ -2516,7 +2518,9 @@ reconnect: mbedtls_printf( " . Reconnecting with saved session..." ); +#if defined(MBEDTLS_X509_CRT_PARSE_C) memset( peer_crt_info, 0, sizeof( peer_crt_info ) ); +#endif /* MBEDTLS_X509_CRT_PARSE_C */ if( ( ret = mbedtls_ssl_session_reset( &ssl ) ) != 0 ) { From fd5dc8ae07dbd0541e3d3494ba7c4ee1d74c3903 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 1 Mar 2019 08:10:46 +0000 Subject: [PATCH 63/67] Fix unused variable warning in ssl_parse_certificate_coordinate() This was triggered in client-only builds. --- library/ssl_tls.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index fce29f5fa..932580408 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -6434,6 +6434,8 @@ static int ssl_parse_certificate_coordinate( mbedtls_ssl_context *ssl, return( SSL_CERTIFICATE_SKIP ); } } +#else + ((void) authmode); #endif /* MBEDTLS_SSL_SRV_C */ return( SSL_CERTIFICATE_EXPECTED ); From 2326d2036176d24d6098652c62fee9250fbc35da Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 6 Jun 2019 14:54:55 +0100 Subject: [PATCH 64/67] Validate consistency of certificate hash type and length in session --- library/ssl_tls.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 932580408..f2034a4a8 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -9498,6 +9498,13 @@ static int ssl_session_load( mbedtls_ssl_session *session, if( session->peer_cert_digest_len != 0 ) { + const mbedtls_md_info_t *md_info = + mbedtls_md_info_from_type( session->peer_cert_digest_type ); + if( md_info == NULL ) + return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); + if( session->peer_cert_digest_len != mbedtls_md_get_size( md_info ) ) + return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); + if( session->peer_cert_digest_len > (size_t)( end - p ) ) return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); From 17daaa5cc6eea906bdd400f2bfa8a0d103445a85 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 18 Jun 2019 12:31:45 +0100 Subject: [PATCH 65/67] Move return statement in ssl_srv_check_client_no_crt_notification The previous placing of the return statement made it look like there are configurations for which no return statement is emitted; while that's not true (if this function is used, at least some version of TLS must be enabled), it's still clearer to move the failing return statement to outside of all preprocessor guards. --- library/ssl_tls.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index f2034a4a8..35b267aa3 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -6398,9 +6398,10 @@ static int ssl_srv_check_client_no_crt_notification( mbedtls_ssl_context *ssl ) return( 0 ); } - return( -1 ); #endif /* MBEDTLS_SSL_PROTO_TLS1 || MBEDTLS_SSL_PROTO_TLS1_1 || \ MBEDTLS_SSL_PROTO_TLS1_2 */ + + return( -1 ); } #endif /* MBEDTLS_SSL_SRV_C */ From d972f005bfad5dd99396d8ea1d2e35ea0c5d6b9c Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 18 Jun 2019 12:41:15 +0100 Subject: [PATCH 66/67] Use consistent error messages in check_config.h --- include/mbedtls/check_config.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/mbedtls/check_config.h b/include/mbedtls/check_config.h index 76ab07a9f..67cb77856 100644 --- a/include/mbedtls/check_config.h +++ b/include/mbedtls/check_config.h @@ -283,7 +283,7 @@ ( !defined(MBEDTLS_SHA256_C) && \ !defined(MBEDTLS_SHA512_C) && \ !defined(MBEDTLS_SHA1_C) ) -#error "!MBEDTLS_SSL_KEEP_PEER_CERTIFICATE requires MBEDTLS_SHA512_C, MBEDTLS_SHA256_C or MBEDTLS_SHA1_C" +#error "MBEDTLS_SSL_KEEP_PEER_CERTIFICATE defined, but not all prerequesites" #endif #if defined(MBEDTLS_MEMORY_BUFFER_ALLOC_C) && \ From 0528f82fa92a0af9d47c651eb5e916b5e3c91823 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 18 Jun 2019 12:45:31 +0100 Subject: [PATCH 67/67] Clarify documentation of serialized session format --- library/ssl_tls.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 35b267aa3..364216287 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -9155,9 +9155,11 @@ static unsigned char ssl_serialized_session_header[] = { * opaque session_id[32]; * opaque master[48]; // fixed length in the standard * uint32 verify_result; - * opaque peer_cert<0..2^24-1>; // length 0 means no peer cert - * uint8_t peer_cert_digest_type; - * opaque peer_cert_digest<0..2^8-1>; + * select (MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) { + * case enabled: opaque peer_cert<0..2^24-1>; // length 0 means no cert + * case disabled: uint8_t peer_cert_digest_type; + * opaque peer_cert_digest<0..2^8-1>; + * } * opaque ticket<0..2^24-1>; // length 0 means no ticket * uint32 ticket_lifetime; * uint8 mfl_code; // up to 255 according to standard