From 60848e65741051452f56945b7ca380cb9c7ad60c Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 5 Feb 2019 15:06:15 +0000 Subject: [PATCH 01/68] 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 4c23f0e07..e8d4a7634 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -5901,18 +5901,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 f852b1c03534a0fd9d45310c183191f8671c3d9b Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 5 Feb 2019 11:42:30 +0000 Subject: [PATCH 02/68] 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 e8d4a7634..e3470f136 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -8532,7 +8532,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 1294a0b260997b6d0ae3927d00dab59156324511 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 5 Feb 2019 12:38:15 +0000 Subject: [PATCH 03/68] 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 e3470f136..1ccb27891 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -5741,6 +5741,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 @@ -5834,13 +5844,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 ) @@ -5902,9 +5907,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. */ } @@ -9420,11 +9423,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 a028c5bbd82d1d46a4fae5b374dfe7077cc5527a Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 5 Feb 2019 12:38:45 +0000 Subject: [PATCH 04/68] 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 1ccb27891..d2cb8937d 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -5757,7 +5757,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; @@ -5884,7 +5884,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 4a55f638e259d101229f903737618b697139432a Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 5 Feb 2019 12:49:06 +0000 Subject: [PATCH 05/68] 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 d2cb8937d..f89029b35 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -5761,52 +5761,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 ) { @@ -5967,6 +5921,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; @@ -6029,16 +6025,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 7a955a043edbc48ec6b8dbf2dd660b1fc11deff5 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 5 Feb 2019 13:08:01 +0000 Subject: [PATCH 06/68] 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 f89029b35..6239d67c0 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -5798,9 +5798,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 ) { @@ -6038,6 +6035,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 6bdfab2cccaf91d8546b471a8e4e4d2979635caa Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 5 Feb 2019 13:11:17 +0000 Subject: [PATCH 07/68] 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 6239d67c0..8653afc83 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -5962,7 +5962,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->transform_negotiate->ciphersuite_info; #if defined(MBEDTLS_SSL_SRV_C) && defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) @@ -5982,8 +5982,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) @@ -5991,8 +5990,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 && @@ -6000,9 +5998,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 @@ -6026,12 +6022,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 */ @@ -6039,10 +6036,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) @@ -6188,10 +6182,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 214899390089dceb8c481787eacc43cdda750d62 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 5 Feb 2019 13:20:55 +0000 Subject: [PATCH 08/68] 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 8653afc83..fdd3e7c37 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -5557,13 +5557,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 ) { @@ -5605,7 +5599,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 ) @@ -6189,13 +6183,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 7177a88a36727edab6c6a031b9842f898e735fe2 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 5 Feb 2019 13:36:46 +0000 Subject: [PATCH 09/68] 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 fdd3e7c37..16f836df9 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -5565,10 +5565,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++; @@ -5585,10 +5582,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++; @@ -5611,10 +5605,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++; @@ -5755,7 +5746,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" ) ); @@ -5970,10 +5960,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 28f2fcd08dd1b6172af0b6b6fcc14382e067508a Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 7 Feb 2019 10:11:07 +0000 Subject: [PATCH 10/68] 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 16f836df9..ef3ec2333 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -5944,11 +5944,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->transform_negotiate->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->transform_negotiate->ciphersuite_info; + const mbedtls_ssl_ciphersuite_t *ciphersuite_info = + ssl->transform_negotiate->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 @@ -5960,29 +5998,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 77adddc9e91b0405464724de45dec277515bac81 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 7 Feb 2019 12:32:43 +0000 Subject: [PATCH 11/68] 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 87fa1e0d9..b564bf631 100644 --- a/library/ssl_cli.c +++ b/library/ssl_cli.c @@ -3456,12 +3456,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 = @@ -3476,11 +3471,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++; @@ -3490,7 +3481,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; @@ -3519,11 +3510,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++; @@ -3666,12 +3653,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 46e24e443..5313b11a5 100644 --- a/library/ssl_srv.c +++ b/library/ssl_srv.c @@ -2680,12 +2680,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 = @@ -2693,11 +2688,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++; @@ -2707,7 +2698,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; @@ -2731,11 +2722,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" ) ); @@ -2874,12 +2861,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) @@ -4048,12 +4030,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 = @@ -4061,11 +4038,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++; @@ -4075,7 +4048,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; @@ -4092,11 +4065,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" ) ); @@ -4241,12 +4210,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 fcd9e71cdf8284b8f514623cc930febb3e39ef16 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 5 Feb 2019 14:35:46 +0000 Subject: [PATCH 12/68] 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 ef3ec2333..7d88582b6 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -6039,7 +6039,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 6863619a2f94c23948a4abbf7269bc31437f99ab Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 5 Feb 2019 14:36:34 +0000 Subject: [PATCH 13/68] 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 7d88582b6..01428f03a 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -5981,11 +5981,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->transform_negotiate->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 @@ -6050,140 +6194,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 c7bd780e02eafea41ad2577b6cccc9bee06b761a Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 5 Feb 2019 15:37:23 +0000 Subject: [PATCH 14/68] 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 01428f03a..fd697630f 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -5740,9 +5740,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; @@ -5819,58 +5823,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*/ @@ -5898,7 +5878,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 ); } @@ -6179,10 +6159,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 52055ae91f0bfb93502f495cf80474ebca8cceae Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 6 Feb 2019 14:30:46 +0000 Subject: [PATCH 15/68] 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 be7f41b1d..e76f4f864 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -766,6 +766,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 fd697630f..ac652d2d7 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -279,8 +279,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 ) ); @@ -319,7 +319,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, @@ -7613,7 +7612,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; @@ -8548,7 +8548,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 aee8717877f38731f15fb056c8046ea884ee24f1 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 6 Feb 2019 14:53:19 +0000 Subject: [PATCH 16/68] 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 0329f75a9315644212b4aac7ba6351f80a188acb Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 6 Feb 2019 15:04:32 +0000 Subject: [PATCH 17/68] Increase robustness and documentation of ticket implementation --- library/ssl_ticket.c | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/library/ssl_ticket.c b/library/ssl_ticket.c index 7de4e66b1..2ad543698 100644 --- a/library/ssl_ticket.c +++ b/library/ssl_ticket.c @@ -204,6 +204,9 @@ static int ssl_save_session( const mbedtls_ssl_session *session, if( left < sizeof( mbedtls_ssl_session ) ) return( MBEDTLS_ERR_SSL_BUFFER_TOO_SMALL ); + /* This also copies the values of pointer fields in the + * session to be serialized, but they'll be ignored when + * loading the session through ssl_load_session(). */ memcpy( p, session, sizeof( mbedtls_ssl_session ) ); p += sizeof( mbedtls_ssl_session ); left -= sizeof( mbedtls_ssl_session ); @@ -250,18 +253,24 @@ static int ssl_load_session( mbedtls_ssl_session *session, memcpy( session, p, sizeof( mbedtls_ssl_session ) ); p += sizeof( mbedtls_ssl_session ); + /* Non-NULL pointer fields of `session` are meaningless + * and potentially harmful. Zeroize them for safety. */ #if defined(MBEDTLS_X509_CRT_PARSE_C) + session->peer_cert = NULL; +#endif /* MBEDTLS_X509_CRT_PARSE_C */ +#if defined(MBEDTLS_SSL_SESSION_TICKETS) && defined(MBEDTLS_SSL_CLI_C) + session->ticket = NULL; +#endif /* MBEDTLS_SSL_SESSION_TICKETS && MBEDTLS_SSL_CLI_C */ + +#if defined(MBEDTLS_X509_CRT_PARSE_C) + /* Deserialize CRT from the end of the ticket. */ if( 3 > (size_t)( end - p ) ) return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); cert_len = ( p[0] << 16 ) | ( p[1] << 8 ) | p[2]; p += 3; - if( cert_len == 0 ) - { - session->peer_cert = NULL; - } - else + if( cert_len != 0 ) { int ret; From 8273df8383dca8ff23d11ba57d78c13ccd24c3af Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 6 Feb 2019 17:37:32 +0000 Subject: [PATCH 18/68] 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 b564bf631..b0c8b302e 100644 --- a/library/ssl_cli.c +++ b/library/ssl_cli.c @@ -2292,8 +2292,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 ); } /* @@ -2404,8 +2404,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, @@ -2744,10 +2744,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 4a82c1ccb416288579b32269ed60c13e53ba94dc Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 5 Feb 2019 11:33:12 +0000 Subject: [PATCH 19/68] 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 46007a72b..c4d6605b5 100644 --- a/include/mbedtls/ssl.h +++ b/include/mbedtls/ssl.h @@ -2972,18 +2972,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 bb278f52ca2b32fd80966f4c81257c63010e8eee Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 5 Feb 2019 17:04:00 +0000 Subject: [PATCH 20/68] 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 e6abf24d5..091ce018f 100644 --- a/include/mbedtls/config.h +++ b/include/mbedtls/config.h @@ -1354,6 +1354,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 c4d6605b5..d736c2101 100644 --- a/include/mbedtls/ssl.h +++ b/include/mbedtls/ssl.h @@ -2982,8 +2982,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 8d84fd83ff5dd409a77e216c09b576d3b36481aa Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 7 Feb 2019 15:13:38 +0000 Subject: [PATCH 21/68] 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 61094d4ed..4674dea90 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 9198ad110153626981e4ef70fe0504b27c30472f Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 5 Feb 2019 17:00:50 +0000 Subject: [PATCH 22/68] 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 d736c2101..6dcc43b93 100644 --- a/include/mbedtls/ssl.h +++ b/include/mbedtls/ssl.h @@ -787,6 +787,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. */ @@ -802,7 +818,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 ac652d2d7..26832bc96 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -304,6 +304,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) @@ -5733,6 +5749,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 6bbd94c4eb4646ae8d0bba2f27532b7496f2ff72 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 5 Feb 2019 17:02:28 +0000 Subject: [PATCH 23/68] 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 26832bc96..6a3548613 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -6220,6 +6220,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 177475a3aa5636bd846a9587761b3536c1f4c848 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 5 Feb 2019 17:02:46 +0000 Subject: [PATCH 24/68] 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 6a3548613..d4df533cf 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -5725,6 +5725,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 ) @@ -5739,6 +5741,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 3dad311ef02bd98e1a9b388082ec8c1f11bc53fe Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 5 Feb 2019 17:19:52 +0000 Subject: [PATCH 25/68] 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 e76f4f864..7cd0d1c4a 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -331,6 +331,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 d4df533cf..4ca8f326f 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -6176,6 +6176,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" ) ); @@ -6190,6 +6191,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 @@ -6199,7 +6202,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) @@ -6219,22 +6222,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) @@ -6246,12 +6251,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 ); @@ -6262,15 +6267,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; @@ -6278,11 +6284,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 */ @@ -9487,6 +9512,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 c5fcbb33c0a6ca1de75a5942a4b3d1d9296f67e7 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 6 Feb 2019 15:23:38 +0000 Subject: [PATCH 26/68] 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_ticket.c | 62 +++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 59 insertions(+), 3 deletions(-) diff --git a/library/ssl_ticket.c b/library/ssl_ticket.c index 2ad543698..ef9f7e13d 100644 --- a/library/ssl_ticket.c +++ b/library/ssl_ticket.c @@ -187,9 +187,11 @@ int mbedtls_ssl_ticket_setup( mbedtls_ssl_ticket_context *ctx, /* * Serialize a session in the following format: - * 0 . n-1 session structure, n = sizeof(mbedtls_ssl_session) - * n . n+2 peer_cert length = m (0 if no certificate) - * n+3 . n+2+m peer cert ASN.1 + * 0 . n-1 session structure, n = sizeof(mbedtls_ssl_session) + * n . n+2 peer_cert length = m (0 if no certificate) + * n+3 . n+2+m peer cert ASN.1 + * n+3+m . n+3+m length of peer certificate digest = k (0 if n digest) + * n+4+m . n+4+k peer certificate digest (digest type encoded in session) */ static int ssl_save_session( const mbedtls_ssl_session *session, unsigned char *buf, size_t buf_len, @@ -199,6 +201,9 @@ static int ssl_save_session( const mbedtls_ssl_session *session, size_t left = buf_len; #if defined(MBEDTLS_X509_CRT_PARSE_C) size_t cert_len; +#if !defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) + size_t cert_digest_len; +#endif /* MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ #endif /* MBEDTLS_X509_CRT_PARSE_C */ if( left < sizeof( mbedtls_ssl_session ) ) @@ -223,11 +228,32 @@ static int ssl_save_session( const mbedtls_ssl_session *session, *p++ = (unsigned char)( ( cert_len >> 16 ) & 0xFF ); *p++ = (unsigned char)( ( cert_len >> 8 ) & 0xFF ); *p++ = (unsigned char)( ( cert_len ) & 0xFF ); + left -= 3; if( session->peer_cert != NULL ) memcpy( p, session->peer_cert->raw.p, cert_len ); p += cert_len; + left -= cert_len; + +#if !defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) + if( session->peer_cert_digest != NULL ) + cert_digest_len = 0; + else + cert_digest_len = session->peer_cert_digest_len; + + if( left < 1 + cert_digest_len ) + return( MBEDTLS_ERR_SSL_BUFFER_TOO_SMALL ); + + *p++ = (unsigned char) cert_digest_len; + left--; + + if( session->peer_cert_digest != NULL ) + memcpy( p, session->peer_cert_digest, cert_digest_len ); + + p += cert_digest_len; + left -= cert_digest_len; +#endif /* !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ #endif /* MBEDTLS_X509_CRT_PARSE_C */ *olen = p - buf; @@ -245,6 +271,9 @@ static int ssl_load_session( mbedtls_ssl_session *session, const unsigned char * const end = buf + len; #if defined(MBEDTLS_X509_CRT_PARSE_C) size_t cert_len; +#if !defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) + size_t cert_digest_len; +#endif /* MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ #endif /* MBEDTLS_X509_CRT_PARSE_C */ if( sizeof( mbedtls_ssl_session ) > (size_t)( end - p ) ) @@ -257,6 +286,9 @@ static int ssl_load_session( mbedtls_ssl_session *session, * and potentially harmful. Zeroize them for safety. */ #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 /* MBEDTLS_X509_CRT_PARSE_C */ #if defined(MBEDTLS_SSL_SESSION_TICKETS) && defined(MBEDTLS_SSL_CLI_C) session->ticket = NULL; @@ -295,6 +327,30 @@ static int ssl_load_session( mbedtls_ssl_session *session, p += cert_len; } +#if !defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) + /* Deserialize CRT digest from the end of the ticket. */ + if( 1 > (size_t)( end - p ) ) + return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); + + cert_digest_len = (size_t) p[0]; + p++; + + if( cert_digest_len != 0 ) + { + if( cert_digest_len > (size_t)( end - p ) || + cert_digest_len != session->peer_cert_digest_len ) + { + return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); + } + + session->peer_cert_digest = mbedtls_calloc( 1, cert_digest_len ); + if( session->peer_cert_digest == NULL ) + return( MBEDTLS_ERR_SSL_ALLOC_FAILED ); + + memcpy( session->peer_cert_digest, p, cert_digest_len ); + p += cert_digest_len; + } +#endif /* MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ #endif /* MBEDTLS_X509_CRT_PARSE_C */ if( p != end ) From c966bd16beb036a2c73b685aed16e82c197ebab6 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 6 Feb 2019 15:40:27 +0000 Subject: [PATCH 27/68] Remove peer CRT from tickets if !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE --- library/ssl_ticket.c | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/library/ssl_ticket.c b/library/ssl_ticket.c index ef9f7e13d..e4054b4e5 100644 --- a/library/ssl_ticket.c +++ b/library/ssl_ticket.c @@ -187,11 +187,16 @@ int mbedtls_ssl_ticket_setup( mbedtls_ssl_ticket_context *ctx, /* * Serialize a session in the following format: - * 0 . n-1 session structure, n = sizeof(mbedtls_ssl_session) - * n . n+2 peer_cert length = m (0 if no certificate) - * n+3 . n+2+m peer cert ASN.1 - * n+3+m . n+3+m length of peer certificate digest = k (0 if n digest) - * n+4+m . n+4+k peer certificate digest (digest type encoded in session) + * + * - If MBEDTLS_SSL_KEEP_PEER_CERTIFICATE is enabled: + * 0 . n-1 session structure, n = sizeof(mbedtls_ssl_session) + * n . n+2 peer_cert length = m (0 if no certificate) + * n+3 . n+2+m peer cert ASN.1 + * + * - If MBEDTLS_SSL_KEEP_PEER_CERTIFICATE is disabled: + * 0 . n-1 session structure, n = sizeof(mbedtls_ssl_session) + * n . n length of peer certificate digest = k (0 if n digest) + * n+1 . n+k peer certificate digest (digest type encoded in session) */ static int ssl_save_session( const mbedtls_ssl_session *session, unsigned char *buf, size_t buf_len, @@ -200,8 +205,9 @@ static int ssl_save_session( const mbedtls_ssl_session *session, unsigned char *p = buf; size_t left = buf_len; #if defined(MBEDTLS_X509_CRT_PARSE_C) +#if defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) size_t cert_len; -#if !defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) +#else size_t cert_digest_len; #endif /* MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ #endif /* MBEDTLS_X509_CRT_PARSE_C */ @@ -217,6 +223,7 @@ static int ssl_save_session( const mbedtls_ssl_session *session, left -= sizeof( mbedtls_ssl_session ); #if defined(MBEDTLS_X509_CRT_PARSE_C) +#if defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) if( session->peer_cert == NULL ) cert_len = 0; else @@ -235,8 +242,7 @@ static int ssl_save_session( const mbedtls_ssl_session *session, p += cert_len; left -= cert_len; - -#if !defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) +#else /* MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ if( session->peer_cert_digest != NULL ) cert_digest_len = 0; else @@ -270,8 +276,9 @@ static int ssl_load_session( mbedtls_ssl_session *session, const unsigned char *p = buf; const unsigned char * const end = buf + len; #if defined(MBEDTLS_X509_CRT_PARSE_C) +#if defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) size_t cert_len; -#if !defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) +#else size_t cert_digest_len; #endif /* MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ #endif /* MBEDTLS_X509_CRT_PARSE_C */ @@ -295,6 +302,7 @@ static int ssl_load_session( mbedtls_ssl_session *session, #endif /* MBEDTLS_SSL_SESSION_TICKETS && MBEDTLS_SSL_CLI_C */ #if defined(MBEDTLS_X509_CRT_PARSE_C) +#if defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) /* Deserialize CRT from the end of the ticket. */ if( 3 > (size_t)( end - p ) ) return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); @@ -327,7 +335,7 @@ static int ssl_load_session( mbedtls_ssl_session *session, p += cert_len; } -#if !defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) +#else /* MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ /* Deserialize CRT digest from the end of the ticket. */ if( 1 > (size_t)( end - p ) ) return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); From a887d1a5b6e71e1a080b1944474692cb622dd20e Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 6 Feb 2019 15:57:49 +0000 Subject: [PATCH 28/68] Remove peer CRT from cache if !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE --- include/mbedtls/ssl_cache.h | 3 ++- library/ssl_cache.c | 20 ++++++++++++-------- 2 files changed, 14 insertions(+), 9 deletions(-) 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 ); } From 494dd7a6b4eed3c4fd35ad07895346e45ee90da6 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 6 Feb 2019 16:13:41 +0000 Subject: [PATCH 29/68] 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 72c39019b..b3f27be93 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 e3f169f2c..5d82816f2 100644 --- a/library/x509_crt.c +++ b/library/x509_crt.c @@ -996,11 +996,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 75173121fe12a96a2b8d879aebda7377677892d0 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 6 Feb 2019 16:18:31 +0000 Subject: [PATCH 30/68] 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 7cd0d1c4a..549911572 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -336,6 +336,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 4ca8f326f..290dbe08d 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -7173,6 +7173,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 } static void ssl_transform_init( mbedtls_ssl_transform *transform ) @@ -9519,6 +9524,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 a27475335aba72c2743448f02a93b68c0c78d807 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 6 Feb 2019 16:19:04 +0000 Subject: [PATCH 31/68] 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 290dbe08d..0afdd61c4 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -6284,6 +6284,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 c7d7e29b462866638132e68e160202d2ba4379c3 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 6 Feb 2019 16:49:54 +0000 Subject: [PATCH 32/68] 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 b0c8b302e..0056896c5 100644 --- a/library/ssl_cli.c +++ b/library/ssl_cli.c @@ -2265,6 +2265,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 ) { @@ -2290,23 +2291,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 be7f50866d3aa8a0cc174a477af1f953de1e0a81 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 6 Feb 2019 17:44:07 +0000 Subject: [PATCH 33/68] 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 0056896c5..a75852439 100644 --- a/library/ssl_cli.c +++ b/library/ssl_cli.c @@ -2406,21 +2406,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 a6899bb89d8e0f5d264c37d972e0fb0c1069a920 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 6 Feb 2019 18:26:03 +0000 Subject: [PATCH 34/68] 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 a75852439..d4022496d 100644 --- a/library/ssl_cli.c +++ b/library/ssl_cli.c @@ -2650,6 +2650,8 @@ start_processing: size_t params_len = p - params; void *rs_ctx = NULL; + mbedtls_pk_context * peer_pk; + /* * Handle the digitally-signed structure */ @@ -2752,16 +2754,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, @@ -2774,8 +2781,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 a1ab9be36721ba1922d6bc9c83ae3d590e5bdbbf Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 6 Feb 2019 18:31:04 +0000 Subject: [PATCH 35/68] 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 5313b11a5..3fe0d6c07 100644 --- a/library/ssl_srv.c +++ b/library/ssl_srv.c @@ -4062,6 +4062,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->transform_negotiate->ciphersuite_info; + mbedtls_pk_context * peer_pk; MBEDTLS_SSL_DEBUG_MSG( 2, ( "=> parse certificate verify" ) ); @@ -4093,6 +4094,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 @@ -4107,8 +4119,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; @@ -4163,7 +4174,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 ); @@ -4196,7 +4207,7 @@ static int ssl_parse_certificate_verify( mbedtls_ssl_context *ssl ) /* Calculate hash and verify signature */ ssl->handshake->calc_verify( ssl, hash ); - 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 57b33c9e4eecdd2d9bcc696349391c37bca2f946 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 7 Feb 2019 13:28:57 +0000 Subject: [PATCH 36/68] 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 626c4d101..38fbd51bf 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 2a831a4ba7c1344ecd219e735adca1987e090154 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 7 Feb 2019 13:17:25 +0000 Subject: [PATCH 37/68] 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 3fe0d6c07..c96908956 100644 --- a/library/ssl_srv.c +++ b/library/ssl_srv.c @@ -4066,14 +4066,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 abe6f66c00452dd314cfc3ca80c1136a420510cd Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 7 Feb 2019 13:29:55 +0000 Subject: [PATCH 38/68] Remove peer CRT from mbedtls_ssl_session if new option is disabled --- include/mbedtls/ssl.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/include/mbedtls/ssl.h b/include/mbedtls/ssl.h index 6dcc43b93..dad8ebd06 100644 --- a/include/mbedtls/ssl.h +++ b/include/mbedtls/ssl.h @@ -818,14 +818,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 */ From 94cc26dfa6ba5d400c7b5731ca7dbcacb33b65e4 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 7 Feb 2019 12:26:46 +0000 Subject: [PATCH 39/68] Adapt session ticket implementation to removal of `peer_cert` field --- library/ssl_ticket.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/library/ssl_ticket.c b/library/ssl_ticket.c index e4054b4e5..a93a6ba14 100644 --- a/library/ssl_ticket.c +++ b/library/ssl_ticket.c @@ -292,8 +292,9 @@ static int ssl_load_session( mbedtls_ssl_session *session, /* Non-NULL pointer fields of `session` are meaningless * and potentially harmful. Zeroize them for safety. */ #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_X509_CRT_PARSE_C */ From 6d1986e6f5184bfb5669528cb7a0abb3493fa80a Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 7 Feb 2019 12:27:42 +0000 Subject: [PATCH 40/68] 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 0afdd61c4..b3e50a64e 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -286,6 +286,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; @@ -304,8 +306,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 13c327d5003dc6f4e15c175379d4f1d718f7dbf2 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 7 Feb 2019 13:17:53 +0000 Subject: [PATCH 41/68] 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 b3e50a64e..f012fb6a8 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -5775,14 +5775,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. */ @@ -5791,7 +5791,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 b6c5eca2d51467beec0ae18e66a7b32d593aef00 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 7 Feb 2019 13:18:21 +0000 Subject: [PATCH 42/68] 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 f012fb6a8..141c2550d 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -6301,10 +6301,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 d0aac143035a153d3c308675bf0dccf8a0fbce55 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 7 Feb 2019 13:44:35 +0000 Subject: [PATCH 43/68] 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 ff05f6493..f81bddbd6 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -2956,6 +2956,7 @@ run_test "Authentication: send CA list in CertificateRequest, client self sig # Tests for certificate selection based on SHA verson +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 \ @@ -2966,6 +2967,7 @@ run_test "Certificate hash: client TLS 1.2 -> SHA-2" \ -c "signed using.*ECDSA with SHA256" \ -C "signed using.*ECDSA with SHA1" +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 \ @@ -2976,6 +2978,7 @@ run_test "Certificate hash: client TLS 1.1 -> SHA-1" \ -C "signed using.*ECDSA with SHA256" \ -c "signed using.*ECDSA with SHA1" +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 \ @@ -2986,6 +2989,7 @@ run_test "Certificate hash: client TLS 1.0 -> SHA-1" \ -C "signed using.*ECDSA with SHA256" \ -c "signed using.*ECDSA with SHA1" +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 \ @@ -2997,6 +3001,7 @@ run_test "Certificate hash: client TLS 1.1, no SHA-1 -> SHA-2 (order 1)" \ -c "signed using.*ECDSA with SHA256" \ -C "signed using.*ECDSA with SHA1" +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 \ @@ -3010,6 +3015,7 @@ run_test "Certificate hash: client TLS 1.1, no SHA-1 -> SHA-2 (order 2)" \ # tests for SNI +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" \ @@ -3019,6 +3025,7 @@ run_test "SNI: no SNI callback" \ -c "issuer name *: C=NL, O=PolarSSL, CN=Polarssl Test EC CA" \ -c "subject name *: C=NL, O=PolarSSL, CN=localhost" +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 \ @@ -3029,6 +3036,7 @@ run_test "SNI: matching cert 1" \ -c "issuer name *: C=NL, O=PolarSSL, CN=PolarSSL Test CA" \ -c "subject name *: C=NL, O=PolarSSL, CN=localhost" +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 \ @@ -3146,6 +3154,7 @@ run_test "SNI: CA override with CRL" \ # Tests for SNI and DTLS +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" \ @@ -3155,6 +3164,7 @@ run_test "SNI: DTLS, no SNI callback" \ -c "issuer name *: C=NL, O=PolarSSL, CN=Polarssl Test EC CA" \ -c "subject name *: C=NL, O=PolarSSL, CN=localhost" +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 \ @@ -3165,6 +3175,7 @@ run_test "SNI: DTLS, matching cert 1" \ -c "issuer name *: C=NL, O=PolarSSL, CN=PolarSSL Test CA" \ -c "subject name *: C=NL, O=PolarSSL, CN=localhost" +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 e68245750ab48a6a834885d2b6e78ea9db5c2a31 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 7 Feb 2019 13:18:46 +0000 Subject: [PATCH 44/68] 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 141c2550d..df5e03649 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -8666,7 +8666,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 e31505d64ed8b9384a85b1d094e22b0d96098a90 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 7 Feb 2019 13:42:45 +0000 Subject: [PATCH 45/68] Adapt ChangeLog --- ChangeLog | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/ChangeLog b/ChangeLog index 2500919d8..bf58d41bd 100644 --- a/ChangeLog +++ b/ChangeLog @@ -11,6 +11,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 * Fix a compilation issue with mbedtls_ecp_restart_ctx not being defined From b9d447908089bce8434766c4ff41144aee7e0865 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 8 Feb 2019 07:19:04 +0000 Subject: [PATCH 46/68] 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 df5e03649..b75101b25 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -5570,6 +5570,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 */ @@ -5773,27 +5796,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 ae553dde3a6aa563722d7667032a43449997d648 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 8 Feb 2019 14:06:00 +0000 Subject: [PATCH 47/68] 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 d4022496d..1312e011b 100644 --- a/library/ssl_cli.c +++ b/library/ssl_cli.c @@ -2331,6 +2331,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 || @@ -2440,6 +2444,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) || @@ -2796,6 +2807,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 0056eab3cd6d3a84015a81d655418e59f036de60 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 8 Feb 2019 14:39:16 +0000 Subject: [PATCH 48/68] 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 b75101b25..03944b43d 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -5910,7 +5910,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 6b8fbab290bd8df8b51c747c86f2f8e68a00517c Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 8 Feb 2019 14:59:05 +0000 Subject: [PATCH 49/68] 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 03944b43d..219fe475e 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -6173,6 +6173,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; @@ -6265,54 +6317,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 545ced45f7cb8bbdc991911e16f4adf025f405a4 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 19 Feb 2019 11:10:48 +0000 Subject: [PATCH 50/68] 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 26881595a..3953f638a 100755 --- a/tests/scripts/all.sh +++ b/tests/scripts/all.sh @@ -968,6 +968,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 1aed7779ec344b7d37ee2fec7a1b33e38790d270 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 22 Feb 2019 16:27:15 +0000 Subject: [PATCH 51/68] 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 549911572..0d543c19b 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -331,9 +331,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 3acc9b90421b796b2aafac79bdd0113646686bde Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Mon, 25 Feb 2019 10:03:26 +0000 Subject: [PATCH 52/68] 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 219fe475e..9eaee9d07 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -6015,8 +6015,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 accc5998ae38eab3458af9d446359e4c95f0bf8a Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Mon, 25 Feb 2019 10:06:59 +0000 Subject: [PATCH 53/68] 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 9eaee9d07..82ba623e0 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -309,15 +309,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 3fd3f5ebe4678f22a9da966198f231492a1db7d5 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Mon, 25 Feb 2019 10:08:06 +0000 Subject: [PATCH 54/68] 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 0d543c19b..5dde239df 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -331,7 +331,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 fd7f298c6a460fa391e769b7d91ee312af592a6b Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Mon, 25 Feb 2019 10:13:33 +0000 Subject: [PATCH 55/68] 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 091ce018f..484ff725d 100644 --- a/include/mbedtls/config.h +++ b/include/mbedtls/config.h @@ -1357,7 +1357,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 958efeb48161e42942a1cdd6b144488663f894e4 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Mon, 25 Feb 2019 10:13:43 +0000 Subject: [PATCH 56/68] 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 dad8ebd06..6e2337930 100644 --- a/include/mbedtls/ssl.h +++ b/include/mbedtls/ssl.h @@ -2998,20 +2998,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 a9766c2c23074fffa70e0783fd9ecfbd4d0c8ca7 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Mon, 25 Feb 2019 17:43:18 +0000 Subject: [PATCH 57/68] 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 c2a8d42d2..3089d8657 100644 --- a/programs/ssl/ssl_client2.c +++ b/programs/ssl/ssl_client2.c @@ -478,6 +478,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 */ @@ -487,8 +489,14 @@ static int my_verify( void *data, mbedtls_x509_crt *crt, char buf[1024]; ((void) data); - 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 ); if ( ( *flags ) == 0 ) @@ -1503,8 +1511,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( opt.auth_mode != DFL_AUTH_MODE ) @@ -1833,13 +1840,8 @@ int main( int argc, char *argv[] ) else mbedtls_printf( " ok\n" ); - 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_CRT_PARSE_C */ #if defined(MBEDTLS_SSL_RENEGOTIATION) From fe9aec4cb1b594c0a61b40ffe2649f89627a7df1 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Mon, 25 Feb 2019 18:01:57 +0000 Subject: [PATCH 58/68] 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 f81bddbd6..ff05f6493 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -2956,7 +2956,6 @@ run_test "Authentication: send CA list in CertificateRequest, client self sig # Tests for certificate selection based on SHA verson -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 \ @@ -2967,7 +2966,6 @@ run_test "Certificate hash: client TLS 1.2 -> SHA-2" \ -c "signed using.*ECDSA with SHA256" \ -C "signed using.*ECDSA with SHA1" -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 \ @@ -2978,7 +2976,6 @@ run_test "Certificate hash: client TLS 1.1 -> SHA-1" \ -C "signed using.*ECDSA with SHA256" \ -c "signed using.*ECDSA with SHA1" -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 \ @@ -2989,7 +2986,6 @@ run_test "Certificate hash: client TLS 1.0 -> SHA-1" \ -C "signed using.*ECDSA with SHA256" \ -c "signed using.*ECDSA with SHA1" -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 \ @@ -3001,7 +2997,6 @@ run_test "Certificate hash: client TLS 1.1, no SHA-1 -> SHA-2 (order 1)" \ -c "signed using.*ECDSA with SHA256" \ -C "signed using.*ECDSA with SHA1" -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 \ @@ -3015,7 +3010,6 @@ run_test "Certificate hash: client TLS 1.1, no SHA-1 -> SHA-2 (order 2)" \ # tests for SNI -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" \ @@ -3025,7 +3019,6 @@ run_test "SNI: no SNI callback" \ -c "issuer name *: C=NL, O=PolarSSL, CN=Polarssl Test EC CA" \ -c "subject name *: C=NL, O=PolarSSL, CN=localhost" -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 \ @@ -3036,7 +3029,6 @@ run_test "SNI: matching cert 1" \ -c "issuer name *: C=NL, O=PolarSSL, CN=PolarSSL Test CA" \ -c "subject name *: C=NL, O=PolarSSL, CN=localhost" -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 \ @@ -3154,7 +3146,6 @@ run_test "SNI: CA override with CRL" \ # Tests for SNI and DTLS -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" \ @@ -3164,7 +3155,6 @@ run_test "SNI: DTLS, no SNI callback" \ -c "issuer name *: C=NL, O=PolarSSL, CN=Polarssl Test EC CA" \ -c "subject name *: C=NL, O=PolarSSL, CN=localhost" -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 \ @@ -3175,7 +3165,6 @@ run_test "SNI: DTLS, matching cert 1" \ -c "issuer name *: C=NL, O=PolarSSL, CN=PolarSSL Test CA" \ -c "subject name *: C=NL, O=PolarSSL, CN=localhost" -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 a1051b4e9ae55827be7792272dfe59f28f07307a Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 26 Feb 2019 11:38:29 +0000 Subject: [PATCH 59/68] 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 3089d8657..73b707c6a 100644 --- a/programs/ssl/ssl_client2.c +++ b/programs/ssl/ssl_client2.c @@ -478,7 +478,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 @@ -1512,6 +1512,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( opt.auth_mode != DFL_AUTH_MODE ) @@ -2217,6 +2218,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 fe4ef0c1ae20dde1eb5a63b3083576b2e16579cc Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 26 Feb 2019 11:43:09 +0000 Subject: [PATCH 60/68] 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 ea05938ed..962d3db87 100644 --- a/include/mbedtls/check_config.h +++ b/include/mbedtls/check_config.h @@ -280,6 +280,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 6e2337930..b793ac04b 100644 --- a/include/mbedtls/ssl.h +++ b/include/mbedtls/ssl.h @@ -787,7 +787,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 @@ -799,9 +800,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 68838740136c5b5a3ee2fd3fbcd81bcccefad7fd Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 26 Feb 2019 11:44:20 +0000 Subject: [PATCH 61/68] Fix typo in SSL ticket documentation --- library/ssl_ticket.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/ssl_ticket.c b/library/ssl_ticket.c index a93a6ba14..ed65bcd63 100644 --- a/library/ssl_ticket.c +++ b/library/ssl_ticket.c @@ -195,7 +195,7 @@ int mbedtls_ssl_ticket_setup( mbedtls_ssl_ticket_context *ctx, * * - If MBEDTLS_SSL_KEEP_PEER_CERTIFICATE is disabled: * 0 . n-1 session structure, n = sizeof(mbedtls_ssl_session) - * n . n length of peer certificate digest = k (0 if n digest) + * n . n length of peer certificate digest = k (0 if no digest) * n+1 . n+k peer certificate digest (digest type encoded in session) */ static int ssl_save_session( const mbedtls_ssl_session *session, From 62d58ed97501cd5ce2db5c84c25f3b87a8516274 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 26 Feb 2019 11:51:06 +0000 Subject: [PATCH 62/68] 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 1312e011b..d309f6d96 100644 --- a/library/ssl_cli.c +++ b/library/ssl_cli.c @@ -2297,6 +2297,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 353a6f0d50565073c71b55a1ae813a6bcfdae4dd Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 26 Feb 2019 11:51:34 +0000 Subject: [PATCH 63/68] 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 82ba623e0..a956bab8f 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -5913,7 +5913,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 bd5580abb1792883883895c617d6a935f924c4ed Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 26 Feb 2019 12:36:01 +0000 Subject: [PATCH 64/68] 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 d309f6d96..4e5b3a602 100644 --- a/library/ssl_cli.c +++ b/library/ssl_cli.c @@ -2419,6 +2419,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; @@ -2772,6 +2773,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 23699efe78fbdb45e468b2d1dfdd18299dfd3776 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 26 Feb 2019 12:36:53 +0000 Subject: [PATCH 65/68] 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 73b707c6a..f370bf0fc 100644 --- a/programs/ssl/ssl_client2.c +++ b/programs/ssl/ssl_client2.c @@ -2147,6 +2147,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 775655eead4606969179f56a65f03242fe60fe45 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 26 Feb 2019 14:38:40 +0000 Subject: [PATCH 66/68] 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 329a5dfee..0f875115f 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 bdf75eb2434fa6713fac4a00ed61242ab7d351fd Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 27 Feb 2019 08:34:31 +0000 Subject: [PATCH 67/68] 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 f370bf0fc..f7e24598d 100644 --- a/programs/ssl/ssl_client2.c +++ b/programs/ssl/ssl_client2.c @@ -2147,7 +2147,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 ) { @@ -2220,7 +2222,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 84d9d2734f2e40205943e21b44e3b20023ccebeb Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 1 Mar 2019 08:10:46 +0000 Subject: [PATCH 68/68] 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 a956bab8f..660d548e4 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -6020,6 +6020,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 );