From 6b916b1616f7994c8f7dfea2bb3754917b3964ef Mon Sep 17 00:00:00 2001 From: XiaokangQian Date: Mon, 25 Apr 2022 07:29:34 +0000 Subject: [PATCH 1/8] Add client certificate parse and certificate verify Change-Id: I638db78922a03db6f8bd70c6c5f56fb60365547d Signed-off-by: XiaokangQian --- include/mbedtls/ssl.h | 1 + library/ssl_tls13_generic.c | 241 ++++++++++++++++++++++++++++++++---- library/ssl_tls13_server.c | 18 +++ 3 files changed, 238 insertions(+), 22 deletions(-) diff --git a/include/mbedtls/ssl.h b/include/mbedtls/ssl.h index 06860e22c6..6295b1816c 100644 --- a/include/mbedtls/ssl.h +++ b/include/mbedtls/ssl.h @@ -492,6 +492,7 @@ #define MBEDTLS_SSL_ALERT_MSG_UNSUPPORTED_EXT 110 /* 0x6E */ #define MBEDTLS_SSL_ALERT_MSG_UNRECOGNIZED_NAME 112 /* 0x70 */ #define MBEDTLS_SSL_ALERT_MSG_UNKNOWN_PSK_IDENTITY 115 /* 0x73 */ +#define MBEDTLS_SSL_ALERT_MSG_CERT_REQUIRED 116 /* 0x74 */ #define MBEDTLS_SSL_ALERT_MSG_NO_APPLICATION_PROTOCOL 120 /* 0x78 */ #define MBEDTLS_SSL_HS_HELLO_REQUEST 0 diff --git a/library/ssl_tls13_generic.c b/library/ssl_tls13_generic.c index e69fd7b622..a260717e13 100644 --- a/library/ssl_tls13_generic.c +++ b/library/ssl_tls13_generic.c @@ -153,6 +153,30 @@ static void ssl_tls13_create_verify_structure( const unsigned char *transcript_h *verify_buffer_len = idx; } +/* Coordinate: Check whether a certificate verify message is expected. + * Returns a negative value on failure, and otherwise + * - SSL_CERTIFICATE_VERIFY_SKIP + * - SSL_CERTIFICATE_VERIFY_READ + * to indicate if the CertificateVerify message should be present or not. + */ +#define SSL_CERTIFICATE_VERIFY_SKIP 0 +#define SSL_CERTIFICATE_VERIFY_READ 1 +static int ssl_tls13_read_certificate_verify_coordinate( mbedtls_ssl_context *ssl ) +{ + if( mbedtls_ssl_tls13_some_psk_enabled( ssl ) ) + return( SSL_CERTIFICATE_VERIFY_SKIP ); + +#if !defined(MBEDTLS_KEY_EXCHANGE_WITH_CERT_ENABLED) + MBEDTLS_SSL_DEBUG_MSG( 1, ( "should never happen" ) ); + return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); +#else + if( ssl->session_negotiate->peer_cert == NULL ) + return( SSL_CERTIFICATE_VERIFY_SKIP ); + + return( SSL_CERTIFICATE_VERIFY_READ ); +#endif /* MBEDTLS_KEY_EXCHANGE_WITH_CERT_ENABLED */ +} + static int ssl_tls13_parse_certificate_verify( mbedtls_ssl_context *ssl, const unsigned char *buf, const unsigned char *end, @@ -315,6 +339,20 @@ int mbedtls_ssl_tls13_process_certificate_verify( mbedtls_ssl_context *ssl ) MBEDTLS_SSL_DEBUG_MSG( 2, ( "=> parse certificate verify" ) ); + MBEDTLS_SSL_PROC_CHK_NEG( ssl_tls13_read_certificate_verify_coordinate( ssl ) ); + if( ret == SSL_CERTIFICATE_VERIFY_SKIP ) + { + MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= skip parse certificate verify" ) ); + ret = 0; + goto cleanup; + } + else if( ret != SSL_CERTIFICATE_VERIFY_READ ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "should never happen" ) ); + ret = MBEDTLS_ERR_SSL_INTERNAL_ERROR; + goto cleanup; + } + MBEDTLS_SSL_PROC_CHK( mbedtls_ssl_tls13_fetch_handshake_msg( ssl, MBEDTLS_SSL_HS_CERTIFICATE_VERIFY, &buf, &buf_len ) ); @@ -367,13 +405,66 @@ cleanup: /* * - * STATE HANDLING: Incoming Certificate, client-side only currently. + * STATE HANDLING: Incoming Certificate. * */ -/* - * Implementation +/* Coordination: Check if a certificate is expected. + * Returns a negative error code on failure, and otherwise + * 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_tls13_read_certificate_coordinate( mbedtls_ssl_context *ssl ) +{ +#if defined(MBEDTLS_SSL_SRV_C) + int authmode = ssl->conf->authmode; +#endif /* MBEDTLS_SSL_SRV_C */ + +#if defined(MBEDTLS_SSL_SRV_C) + if( ssl->conf->endpoint == MBEDTLS_SSL_IS_SERVER ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "Switch to handshake keys for inbound traffic" ) ); + + mbedtls_ssl_set_inbound_transform( ssl, ssl->handshake->transform_handshake ); + } +#endif /* MBEDTLS_SSL_SRV_C */ + + if( mbedtls_ssl_tls13_some_psk_enabled( ssl ) ) + return( SSL_CERTIFICATE_SKIP ); + +#if !defined(MBEDTLS_KEY_EXCHANGE_ECDHE_ECDSA_ENABLED) + ( ( void )authmode ); + MBEDTLS_SSL_DEBUG_MSG( 1, ( "should never happen" ) ); + return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); +#else +#if defined(MBEDTLS_SSL_SRV_C) + if( ssl->conf->endpoint == MBEDTLS_SSL_IS_SERVER ) + { + /* If SNI was used, overwrite authentication mode + * from the configuration. */ +#if defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) + if( ssl->handshake->sni_authmode != MBEDTLS_SSL_VERIFY_UNSET ) + authmode = ssl->handshake->sni_authmode; +#endif /* MBEDTLS_SSL_SERVER_NAME_INDICATION */ + + 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 ); +#endif /* !MBEDTLS_KEY_EXCHANGE_ECDHE_ECDSA_ENABLED */ +} #if defined(MBEDTLS_KEY_EXCHANGE_ECDHE_ECDSA_ENABLED) #if defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) @@ -415,10 +506,39 @@ static int ssl_tls13_parse_certificate( mbedtls_ssl_context *ssl, const unsigned char *p = buf; const unsigned char *certificate_list_end; - MBEDTLS_SSL_CHK_BUF_READ_PTR( p, end, 4 ); + MBEDTLS_SSL_CHK_BUF_READ_PTR( p, end, 1 ); certificate_request_context_len = p[0]; - certificate_list_len = MBEDTLS_GET_UINT24_BE( p, 1 ); - p += 4; + p++; + +#if defined(MBEDTLS_SSL_SRV_C) + if( ssl->conf->endpoint == MBEDTLS_SSL_IS_SERVER ) + { + MBEDTLS_SSL_CHK_BUF_READ_PTR( p, end, + certificate_request_context_len + 3 ); + + /* check whether we got an empty certificate message */ + if( memcmp( p + certificate_request_context_len , "\0\0\0", 3 ) == 0 ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, + ( "client has no certificate - empty certificate message received" ) ); + + ssl->session_negotiate->verify_result = MBEDTLS_X509_BADCERT_MISSING; + if( ssl->conf->authmode == MBEDTLS_SSL_VERIFY_OPTIONAL ) + return( 0 ); + else + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "client certificate required" ) ); + MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_CERT_REQUIRED, + MBEDTLS_ERR_SSL_NO_CLIENT_CERTIFICATE ); + return( MBEDTLS_ERR_SSL_NO_CLIENT_CERTIFICATE ); + } + } + } +#endif /* MBEDTLS_SSL_SRV_C */ + + MBEDTLS_SSL_CHK_BUF_READ_PTR( p, end, 3 ); + certificate_list_len = MBEDTLS_GET_UINT24_BE( p, 0 ); + p += 3; /* In theory, the certificate list can be up to 2^24 Bytes, but we don't * support anything beyond 2^16 = 64K. @@ -547,10 +667,56 @@ static int ssl_tls13_parse_certificate( mbedtls_ssl_context *ssl, static int ssl_tls13_validate_certificate( mbedtls_ssl_context *ssl ) { int ret = 0; + int authmode = ssl->conf->authmode; mbedtls_x509_crt *ca_chain; mbedtls_x509_crl *ca_crl; uint32_t verify_result = 0; + /* If SNI was used, overwrite authentication mode + * from the configuration. */ +#if defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) + if( ssl->handshake->sni_authmode != MBEDTLS_SSL_VERIFY_UNSET ) + authmode = ssl->handshake->sni_authmode; +#endif + + /* + * If the client hasn't sent a certificate ( i.e. it sent + * an empty certificate chain ), this is reflected in the peer CRT + * structure being unset. + * Check for that and handle it depending on the + * server's authentication mode. + */ +#if defined(MBEDTLS_SSL_SRV_C) + if( ssl->conf->endpoint == MBEDTLS_SSL_IS_SERVER && + ssl->session_negotiate->peer_cert == NULL ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "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. */ + + /* Note that for authmode == VERIFY_NONE we don't end up in this + * routine in the first place, because ssl_tls13_read_certificate_coordinate + * will return CERTIFICATE_SKIP. */ + ssl->session_negotiate->verify_result = MBEDTLS_X509_BADCERT_MISSING; + if( authmode == MBEDTLS_SSL_VERIFY_OPTIONAL ) + return( 0 ); + else + return( MBEDTLS_ERR_SSL_NO_CLIENT_CERTIFICATE ); + } +#endif /* MBEDTLS_SSL_SRV_C */ + + if( authmode == MBEDTLS_SSL_VERIFY_NONE ) + { + /* NOTE: This happens on client-side only, with the + * server-side case of VERIFY_NONE being handled earlier + * and leading to `ssl->verify_result` being set to + * MBEDTLS_X509_BADCERT_SKIP_VERIFY -- + * is this difference intentional? */ + return( 0 ); + } + #if defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) if( ssl->handshake->sni_ca_chain != NULL ) { @@ -593,8 +759,21 @@ static int ssl_tls13_validate_certificate( mbedtls_ssl_context *ssl ) ret = MBEDTLS_ERR_SSL_BAD_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_tls13_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_CERTIFICATE ) ) + { + ret = 0; + } - if( ca_chain == NULL ) + + 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; @@ -654,29 +833,47 @@ int mbedtls_ssl_tls13_process_certificate( mbedtls_ssl_context *ssl ) int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; MBEDTLS_SSL_DEBUG_MSG( 2, ( "=> parse certificate" ) ); + /* Coordination: + * Check if we expect a certificate, and if yes, + * check if a non-empty certificate has been sent. + */ + MBEDTLS_SSL_PROC_CHK_NEG( ssl_tls13_read_certificate_coordinate( ssl ) ); #if defined(MBEDTLS_KEY_EXCHANGE_ECDHE_ECDSA_ENABLED) - unsigned char *buf; - size_t buf_len; + if( ret == SSL_CERTIFICATE_EXPECTED ) + { + unsigned char *buf; + size_t buf_len; - MBEDTLS_SSL_PROC_CHK( mbedtls_ssl_tls13_fetch_handshake_msg( - ssl, MBEDTLS_SSL_HS_CERTIFICATE, - &buf, &buf_len ) ); + MBEDTLS_SSL_PROC_CHK( mbedtls_ssl_tls13_fetch_handshake_msg( + ssl, MBEDTLS_SSL_HS_CERTIFICATE, + &buf, &buf_len ) ); - /* Parse the certificate chain sent by the peer. */ - MBEDTLS_SSL_PROC_CHK( ssl_tls13_parse_certificate( ssl, buf, buf + buf_len ) ); - /* Validate the certificate chain and set the verification results. */ - MBEDTLS_SSL_PROC_CHK( ssl_tls13_validate_certificate( ssl ) ); + /* Parse the certificate chain sent by the peer. */ + MBEDTLS_SSL_PROC_CHK( ssl_tls13_parse_certificate( ssl, buf, + buf + buf_len ) ); + /* Validate the certificate chain and set the verification results. */ + MBEDTLS_SSL_PROC_CHK( ssl_tls13_validate_certificate( ssl ) ); + + mbedtls_ssl_add_hs_msg_to_checksum( ssl, MBEDTLS_SSL_HS_CERTIFICATE, + buf, buf_len ); + } + else +#endif /* MBEDTLS_KEY_EXCHANGE_ECDHE_ECDSA_ENABLED */ + if( ret == SSL_CERTIFICATE_SKIP ) + { + MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= skip parse certificate" ) ); + ret = 0; + } + else + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "should never happen" ) ); + ret = MBEDTLS_ERR_SSL_INTERNAL_ERROR; + } - mbedtls_ssl_add_hs_msg_to_checksum( ssl, MBEDTLS_SSL_HS_CERTIFICATE, - buf, buf_len ); cleanup: MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= parse certificate" ) ); -#else - MBEDTLS_SSL_DEBUG_MSG( 1, ( "should never happen" ) ); - ret = MBEDTLS_ERR_SSL_INTERNAL_ERROR; -#endif /* MBEDTLS_KEY_EXCHANGE_ECDHE_ECDSA_ENABLED */ return( ret ); } #if defined(MBEDTLS_KEY_EXCHANGE_WITH_CERT_ENABLED) diff --git a/library/ssl_tls13_server.c b/library/ssl_tls13_server.c index 8c8eb2ab6a..1288e49444 100644 --- a/library/ssl_tls13_server.c +++ b/library/ssl_tls13_server.c @@ -1613,6 +1613,24 @@ int mbedtls_ssl_tls13_handshake_server_step( mbedtls_ssl_context *ssl ) ret = ssl_tls13_handshake_wrapup( ssl ); break; + case MBEDTLS_SSL_CLIENT_CERTIFICATE: + ret = mbedtls_ssl_tls13_process_certificate( ssl ); + if( ret == 0 ) + { + mbedtls_ssl_handshake_set_state( + ssl, MBEDTLS_SSL_CLIENT_CERTIFICATE_VERIFY ); + } + break; + + case MBEDTLS_SSL_CLIENT_CERTIFICATE_VERIFY: + ret = mbedtls_ssl_tls13_process_certificate_verify( ssl ); + if( ret == 0 ) + { + mbedtls_ssl_handshake_set_state( + ssl, MBEDTLS_SSL_CLIENT_FINISHED ); + } + break; + default: MBEDTLS_SSL_DEBUG_MSG( 1, ( "invalid state %d", ssl->state ) ); return( MBEDTLS_ERR_SSL_FEATURE_UNAVAILABLE ); From 189ded2b076c10745104fccc63474502113ea977 Mon Sep 17 00:00:00 2001 From: XiaokangQian Date: Tue, 10 May 2022 08:12:17 +0000 Subject: [PATCH 2/8] Remove coordinate functions and change state machine in server side Change-Id: Id4abf78f493e77afc289409db691c9c61acde1d2 Signed-off-by: XiaokangQian --- library/ssl_misc.h | 5 ++ library/ssl_tls13_generic.c | 100 ++---------------------------------- library/ssl_tls13_server.c | 12 ++++- tests/ssl-opt.sh | 2 +- 4 files changed, 20 insertions(+), 99 deletions(-) diff --git a/library/ssl_misc.h b/library/ssl_misc.h index 9fcb2b2963..01420dde23 100644 --- a/library/ssl_misc.h +++ b/library/ssl_misc.h @@ -509,6 +509,11 @@ struct mbedtls_ssl_handshake_params uint8_t sni_authmode; /*!< authmode from SNI callback */ #endif +#if defined(MBEDTLS_SSL_SRV_C) + /** cert_request_send to indicate whether client certitifacte request */ + uint16_t cert_request_send; +#endif /* MBEDTLS_SSL_SRV_C */ + #if defined(MBEDTLS_SSL_SESSION_TICKETS) uint8_t new_session_ticket; /*!< use NewSessionTicket? */ #endif /* MBEDTLS_SSL_SESSION_TICKETS */ diff --git a/library/ssl_tls13_generic.c b/library/ssl_tls13_generic.c index a260717e13..ab4d077a55 100644 --- a/library/ssl_tls13_generic.c +++ b/library/ssl_tls13_generic.c @@ -153,30 +153,6 @@ static void ssl_tls13_create_verify_structure( const unsigned char *transcript_h *verify_buffer_len = idx; } -/* Coordinate: Check whether a certificate verify message is expected. - * Returns a negative value on failure, and otherwise - * - SSL_CERTIFICATE_VERIFY_SKIP - * - SSL_CERTIFICATE_VERIFY_READ - * to indicate if the CertificateVerify message should be present or not. - */ -#define SSL_CERTIFICATE_VERIFY_SKIP 0 -#define SSL_CERTIFICATE_VERIFY_READ 1 -static int ssl_tls13_read_certificate_verify_coordinate( mbedtls_ssl_context *ssl ) -{ - if( mbedtls_ssl_tls13_some_psk_enabled( ssl ) ) - return( SSL_CERTIFICATE_VERIFY_SKIP ); - -#if !defined(MBEDTLS_KEY_EXCHANGE_WITH_CERT_ENABLED) - MBEDTLS_SSL_DEBUG_MSG( 1, ( "should never happen" ) ); - return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); -#else - if( ssl->session_negotiate->peer_cert == NULL ) - return( SSL_CERTIFICATE_VERIFY_SKIP ); - - return( SSL_CERTIFICATE_VERIFY_READ ); -#endif /* MBEDTLS_KEY_EXCHANGE_WITH_CERT_ENABLED */ -} - static int ssl_tls13_parse_certificate_verify( mbedtls_ssl_context *ssl, const unsigned char *buf, const unsigned char *end, @@ -339,19 +315,13 @@ int mbedtls_ssl_tls13_process_certificate_verify( mbedtls_ssl_context *ssl ) MBEDTLS_SSL_DEBUG_MSG( 2, ( "=> parse certificate verify" ) ); - MBEDTLS_SSL_PROC_CHK_NEG( ssl_tls13_read_certificate_verify_coordinate( ssl ) ); - if( ret == SSL_CERTIFICATE_VERIFY_SKIP ) + if( ssl->handshake->cert_request_send && + ssl->session_negotiate->peer_cert == NULL ) { MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= skip parse certificate verify" ) ); ret = 0; goto cleanup; } - else if( ret != SSL_CERTIFICATE_VERIFY_READ ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "should never happen" ) ); - ret = MBEDTLS_ERR_SSL_INTERNAL_ERROR; - goto cleanup; - } MBEDTLS_SSL_PROC_CHK( mbedtls_ssl_tls13_fetch_handshake_msg( ssl, @@ -409,63 +379,6 @@ cleanup: * */ -/* Coordination: Check if a certificate is expected. - * Returns a negative error code on failure, and otherwise - * 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_tls13_read_certificate_coordinate( mbedtls_ssl_context *ssl ) -{ -#if defined(MBEDTLS_SSL_SRV_C) - int authmode = ssl->conf->authmode; -#endif /* MBEDTLS_SSL_SRV_C */ - -#if defined(MBEDTLS_SSL_SRV_C) - if( ssl->conf->endpoint == MBEDTLS_SSL_IS_SERVER ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "Switch to handshake keys for inbound traffic" ) ); - - mbedtls_ssl_set_inbound_transform( ssl, ssl->handshake->transform_handshake ); - } -#endif /* MBEDTLS_SSL_SRV_C */ - - if( mbedtls_ssl_tls13_some_psk_enabled( ssl ) ) - return( SSL_CERTIFICATE_SKIP ); - -#if !defined(MBEDTLS_KEY_EXCHANGE_ECDHE_ECDSA_ENABLED) - ( ( void )authmode ); - MBEDTLS_SSL_DEBUG_MSG( 1, ( "should never happen" ) ); - return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); -#else -#if defined(MBEDTLS_SSL_SRV_C) - if( ssl->conf->endpoint == MBEDTLS_SSL_IS_SERVER ) - { - /* If SNI was used, overwrite authentication mode - * from the configuration. */ -#if defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) - if( ssl->handshake->sni_authmode != MBEDTLS_SSL_VERIFY_UNSET ) - authmode = ssl->handshake->sni_authmode; -#endif /* MBEDTLS_SSL_SERVER_NAME_INDICATION */ - - 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 ); -#endif /* !MBEDTLS_KEY_EXCHANGE_ECDHE_ECDSA_ENABLED */ -} - #if defined(MBEDTLS_KEY_EXCHANGE_ECDHE_ECDSA_ENABLED) #if defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) /* @@ -837,9 +750,8 @@ int mbedtls_ssl_tls13_process_certificate( mbedtls_ssl_context *ssl ) * Check if we expect a certificate, and if yes, * check if a non-empty certificate has been sent. */ - MBEDTLS_SSL_PROC_CHK_NEG( ssl_tls13_read_certificate_coordinate( ssl ) ); #if defined(MBEDTLS_KEY_EXCHANGE_ECDHE_ECDSA_ENABLED) - if( ret == SSL_CERTIFICATE_EXPECTED ) + if( ssl->handshake->cert_request_send ) { unsigned char *buf; size_t buf_len; @@ -859,16 +771,10 @@ int mbedtls_ssl_tls13_process_certificate( mbedtls_ssl_context *ssl ) } else #endif /* MBEDTLS_KEY_EXCHANGE_ECDHE_ECDSA_ENABLED */ - if( ret == SSL_CERTIFICATE_SKIP ) { MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= skip parse certificate" ) ); ret = 0; } - else - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "should never happen" ) ); - ret = MBEDTLS_ERR_SSL_INTERNAL_ERROR; - } cleanup: diff --git a/library/ssl_tls13_server.c b/library/ssl_tls13_server.c index 1288e49444..fa297cb78f 100644 --- a/library/ssl_tls13_server.c +++ b/library/ssl_tls13_server.c @@ -1342,6 +1342,8 @@ static int ssl_tls13_certificate_request_coordinate( mbedtls_ssl_context *ssl ) if( authmode == MBEDTLS_SSL_VERIFY_NONE ) return( SSL_CERTIFICATE_REQUEST_SKIP ); + ssl->handshake->cert_request_send = 1; + return( SSL_CERTIFICATE_REQUEST_SEND_REQUEST ); } @@ -1495,7 +1497,15 @@ static int ssl_tls13_write_server_finished( mbedtls_ssl_context *ssl ) MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE ); return( ret ); } - mbedtls_ssl_handshake_set_state( ssl, MBEDTLS_SSL_CLIENT_FINISHED ); + if( ssl->handshake->cert_request_send ) + { + mbedtls_ssl_handshake_set_state( ssl, MBEDTLS_SSL_CLIENT_CERTIFICATE ); + + MBEDTLS_SSL_DEBUG_MSG( 1, ( "Switch to handshake keys for inbound traffic" ) ); + mbedtls_ssl_set_inbound_transform( ssl, ssl->handshake->transform_handshake ); + } + else + mbedtls_ssl_handshake_set_state( ssl, MBEDTLS_SSL_CLIENT_FINISHED ); return( 0 ); } diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index 6eaa53477d..2aeee4ffbc 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -11310,7 +11310,7 @@ requires_config_enabled MBEDTLS_SSL_CLI_C run_test "TLS 1.3: Server side check - mbedtls" \ "$P_SRV debug_level=4 crt_file=data_files/server5.crt key_file=data_files/server5.key force_version=tls13 tickets=0" \ "$P_CLI debug_level=4 force_version=tls13" \ - 0 \ + 1 \ -s "tls13 server state: MBEDTLS_SSL_CLIENT_HELLO" \ -s "tls13 server state: MBEDTLS_SSL_SERVER_HELLO" \ -s "tls13 server state: MBEDTLS_SSL_ENCRYPTED_EXTENSIONS" \ From c3017f620fdef568eaa11245cc2f3f258109907d Mon Sep 17 00:00:00 2001 From: XiaokangQian Date: Fri, 13 May 2022 05:55:41 +0000 Subject: [PATCH 3/8] Remove useless guards and refine checking Change-Id: I9cd3073826fc65c203e479d83bed72331ff8963d Signed-off-by: XiaokangQian --- library/ssl_misc.h | 5 +-- library/ssl_tls13_generic.c | 67 +++++++++++++++---------------------- library/ssl_tls13_server.c | 16 +++++---- tests/ssl-opt.sh | 6 ++-- 4 files changed, 43 insertions(+), 51 deletions(-) diff --git a/library/ssl_misc.h b/library/ssl_misc.h index 01420dde23..2d513339f6 100644 --- a/library/ssl_misc.h +++ b/library/ssl_misc.h @@ -510,8 +510,9 @@ struct mbedtls_ssl_handshake_params #endif #if defined(MBEDTLS_SSL_SRV_C) - /** cert_request_send to indicate whether client certitifacte request */ - uint16_t cert_request_send; + /* Flag indicating if a CertificateRequest message has been sent + * to the client or not. */ + uint16_t certificate_request_sent; #endif /* MBEDTLS_SSL_SRV_C */ #if defined(MBEDTLS_SSL_SESSION_TICKETS) diff --git a/library/ssl_tls13_generic.c b/library/ssl_tls13_generic.c index ab4d077a55..786a72c025 100644 --- a/library/ssl_tls13_generic.c +++ b/library/ssl_tls13_generic.c @@ -315,14 +315,6 @@ int mbedtls_ssl_tls13_process_certificate_verify( mbedtls_ssl_context *ssl ) MBEDTLS_SSL_DEBUG_MSG( 2, ( "=> parse certificate verify" ) ); - if( ssl->handshake->cert_request_send && - ssl->session_negotiate->peer_cert == NULL ) - { - MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= skip parse certificate verify" ) ); - ret = 0; - goto cleanup; - } - MBEDTLS_SSL_PROC_CHK( mbedtls_ssl_tls13_fetch_handshake_msg( ssl, MBEDTLS_SSL_HS_CERTIFICATE_VERIFY, &buf, &buf_len ) ); @@ -472,6 +464,13 @@ static int ssl_tls13_parse_certificate( mbedtls_ssl_context *ssl, mbedtls_free( ssl->session_negotiate->peer_cert ); } + if( certificate_list_len == 0 ) + { + ssl->session_negotiate->peer_cert = NULL; + ret = 0; + goto exit; + } + if( ( ssl->session_negotiate->peer_cert = mbedtls_calloc( 1, sizeof( mbedtls_x509_crt ) ) ) == NULL ) { @@ -557,6 +556,7 @@ static int ssl_tls13_parse_certificate( mbedtls_ssl_context *ssl, return( MBEDTLS_ERR_SSL_DECODE_ERROR ); } +exit: MBEDTLS_SSL_DEBUG_CRT( 3, "peer certificate", ssl->session_negotiate->peer_cert ); return( ret ); @@ -620,15 +620,15 @@ static int ssl_tls13_validate_certificate( mbedtls_ssl_context *ssl ) } #endif /* MBEDTLS_SSL_SRV_C */ - if( authmode == MBEDTLS_SSL_VERIFY_NONE ) +#if defined(MBEDTLS_SSL_CLI_C) + if( ssl->conf->endpoint == MBEDTLS_SSL_IS_CLIENT && + ssl->session_negotiate->peer_cert == NULL ) { - /* NOTE: This happens on client-side only, with the - * server-side case of VERIFY_NONE being handled earlier - * and leading to `ssl->verify_result` being set to - * MBEDTLS_X509_BADCERT_SKIP_VERIFY -- - * is this difference intentional? */ - return( 0 ); + MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_NO_CERT, + MBEDTLS_ERR_SSL_FATAL_ALERT_MESSAGE ); + return( MBEDTLS_ERR_SSL_FATAL_ALERT_MESSAGE ); } +#endif /* MBEDTLS_SSL_CLI_C */ #if defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) if( ssl->handshake->sni_ca_chain != NULL ) @@ -746,36 +746,23 @@ int mbedtls_ssl_tls13_process_certificate( mbedtls_ssl_context *ssl ) int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; MBEDTLS_SSL_DEBUG_MSG( 2, ( "=> parse certificate" ) ); - /* Coordination: - * Check if we expect a certificate, and if yes, - * check if a non-empty certificate has been sent. - */ #if defined(MBEDTLS_KEY_EXCHANGE_ECDHE_ECDSA_ENABLED) - if( ssl->handshake->cert_request_send ) - { - unsigned char *buf; - size_t buf_len; + unsigned char *buf; + size_t buf_len; - MBEDTLS_SSL_PROC_CHK( mbedtls_ssl_tls13_fetch_handshake_msg( - ssl, MBEDTLS_SSL_HS_CERTIFICATE, - &buf, &buf_len ) ); + MBEDTLS_SSL_PROC_CHK( mbedtls_ssl_tls13_fetch_handshake_msg( + ssl, MBEDTLS_SSL_HS_CERTIFICATE, + &buf, &buf_len ) ); - /* Parse the certificate chain sent by the peer. */ - MBEDTLS_SSL_PROC_CHK( ssl_tls13_parse_certificate( ssl, buf, - buf + buf_len ) ); - /* Validate the certificate chain and set the verification results. */ - MBEDTLS_SSL_PROC_CHK( ssl_tls13_validate_certificate( ssl ) ); + /* Parse the certificate chain sent by the peer. */ + MBEDTLS_SSL_PROC_CHK( ssl_tls13_parse_certificate( ssl, buf, + buf + buf_len ) ); + /* Validate the certificate chain and set the verification results. */ + MBEDTLS_SSL_PROC_CHK( ssl_tls13_validate_certificate( ssl ) ); - mbedtls_ssl_add_hs_msg_to_checksum( ssl, MBEDTLS_SSL_HS_CERTIFICATE, - buf, buf_len ); - } - else + mbedtls_ssl_add_hs_msg_to_checksum( ssl, MBEDTLS_SSL_HS_CERTIFICATE, + buf, buf_len ); #endif /* MBEDTLS_KEY_EXCHANGE_ECDHE_ECDSA_ENABLED */ - { - MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= skip parse certificate" ) ); - ret = 0; - } - cleanup: diff --git a/library/ssl_tls13_server.c b/library/ssl_tls13_server.c index fa297cb78f..43018d0dbf 100644 --- a/library/ssl_tls13_server.c +++ b/library/ssl_tls13_server.c @@ -1342,7 +1342,7 @@ static int ssl_tls13_certificate_request_coordinate( mbedtls_ssl_context *ssl ) if( authmode == MBEDTLS_SSL_VERIFY_NONE ) return( SSL_CERTIFICATE_REQUEST_SKIP ); - ssl->handshake->cert_request_send = 1; + ssl->handshake->certificate_request_sent = 1; return( SSL_CERTIFICATE_REQUEST_SEND_REQUEST ); } @@ -1497,7 +1497,8 @@ static int ssl_tls13_write_server_finished( mbedtls_ssl_context *ssl ) MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE ); return( ret ); } - if( ssl->handshake->cert_request_send ) + + if( ssl->handshake->certificate_request_sent ) { mbedtls_ssl_handshake_set_state( ssl, MBEDTLS_SSL_CLIENT_CERTIFICATE ); @@ -1517,9 +1518,9 @@ static int ssl_tls13_process_client_finished( mbedtls_ssl_context *ssl ) int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; MBEDTLS_SSL_DEBUG_MSG( 1, - ( "Switch to handshake traffic keys for inbound traffic" ) ); - mbedtls_ssl_set_inbound_transform( ssl, ssl->handshake->transform_handshake ); - + ( "Switch to handshake traffic keys for outbound traffic" ) ); + if( ! ssl->handshake->certificate_request_sent ) + mbedtls_ssl_set_inbound_transform( ssl, ssl->handshake->transform_handshake ); ret = mbedtls_ssl_tls13_process_finished_message( ssl ); if( ret != 0 ) return( ret ); @@ -1625,11 +1626,14 @@ int mbedtls_ssl_tls13_handshake_server_step( mbedtls_ssl_context *ssl ) case MBEDTLS_SSL_CLIENT_CERTIFICATE: ret = mbedtls_ssl_tls13_process_certificate( ssl ); - if( ret == 0 ) + if( ret == 0 && ssl->session_negotiate->peer_cert != NULL) { mbedtls_ssl_handshake_set_state( ssl, MBEDTLS_SSL_CLIENT_CERTIFICATE_VERIFY ); } + else + mbedtls_ssl_handshake_set_state( + ssl, MBEDTLS_SSL_CLIENT_FINISHED ); break; case MBEDTLS_SSL_CLIENT_CERTIFICATE_VERIFY: diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index 2aeee4ffbc..eb9c1f48f9 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -11291,7 +11291,7 @@ requires_config_enabled MBEDTLS_SSL_SRV_C run_test "TLS 1.3: Server side check - gnutls with client authentication" \ "$P_SRV debug_level=4 auth_mode=required crt_file=data_files/server5.crt key_file=data_files/server5.key force_version=tls13 tickets=0" \ "$G_NEXT_CLI localhost -d 4 --x509certfile data_files/server5.crt --x509keyfile data_files/server5.key --priority=NORMAL:-VERS-ALL:+VERS-TLS1.3:%NO_TICKETS:%DISABLE_TLS13_COMPAT_MODE -V" \ - 1 \ + 0 \ -s "tls13 server state: MBEDTLS_SSL_CLIENT_HELLO" \ -s "tls13 server state: MBEDTLS_SSL_SERVER_HELLO" \ -s "tls13 server state: MBEDTLS_SSL_ENCRYPTED_EXTENSIONS" \ @@ -11310,7 +11310,7 @@ requires_config_enabled MBEDTLS_SSL_CLI_C run_test "TLS 1.3: Server side check - mbedtls" \ "$P_SRV debug_level=4 crt_file=data_files/server5.crt key_file=data_files/server5.key force_version=tls13 tickets=0" \ "$P_CLI debug_level=4 force_version=tls13" \ - 1 \ + 0 \ -s "tls13 server state: MBEDTLS_SSL_CLIENT_HELLO" \ -s "tls13 server state: MBEDTLS_SSL_SERVER_HELLO" \ -s "tls13 server state: MBEDTLS_SSL_ENCRYPTED_EXTENSIONS" \ @@ -11329,7 +11329,7 @@ requires_config_enabled MBEDTLS_SSL_CLI_C run_test "TLS 1.3: Server side check - mbedtls with client authentication" \ "$P_SRV debug_level=4 auth_mode=required crt_file=data_files/server5.crt key_file=data_files/server5.key force_version=tls13 tickets=0" \ "$P_CLI debug_level=4 crt_file=data_files/server5.crt key_file=data_files/server5.key force_version=tls13" \ - 1 \ + 0 \ -s "tls13 server state: MBEDTLS_SSL_CLIENT_HELLO" \ -s "tls13 server state: MBEDTLS_SSL_SERVER_HELLO" \ -s "tls13 server state: MBEDTLS_SSL_ENCRYPTED_EXTENSIONS" \ From 63e713e8abe5201e5bea917eb3341de25b8ebae5 Mon Sep 17 00:00:00 2001 From: XiaokangQian Date: Sun, 15 May 2022 04:26:57 +0000 Subject: [PATCH 4/8] Fix comments Change-Id: Ib741f876f4d296df79565a2b8a2971918db1a77f Signed-off-by: XiaokangQian --- library/ssl_misc.h | 2 +- library/ssl_tls13_generic.c | 83 ++++++++++++------------------------- 2 files changed, 27 insertions(+), 58 deletions(-) diff --git a/library/ssl_misc.h b/library/ssl_misc.h index 2d513339f6..9912d6c29a 100644 --- a/library/ssl_misc.h +++ b/library/ssl_misc.h @@ -512,7 +512,7 @@ struct mbedtls_ssl_handshake_params #if defined(MBEDTLS_SSL_SRV_C) /* Flag indicating if a CertificateRequest message has been sent * to the client or not. */ - uint16_t certificate_request_sent; + uint8_t certificate_request_sent; #endif /* MBEDTLS_SSL_SRV_C */ #if defined(MBEDTLS_SSL_SESSION_TICKETS) diff --git a/library/ssl_tls13_generic.c b/library/ssl_tls13_generic.c index 786a72c025..ef27644ec6 100644 --- a/library/ssl_tls13_generic.c +++ b/library/ssl_tls13_generic.c @@ -411,39 +411,10 @@ static int ssl_tls13_parse_certificate( mbedtls_ssl_context *ssl, const unsigned char *p = buf; const unsigned char *certificate_list_end; - MBEDTLS_SSL_CHK_BUF_READ_PTR( p, end, 1 ); + MBEDTLS_SSL_CHK_BUF_READ_PTR( p, end, 4 ); certificate_request_context_len = p[0]; - p++; - -#if defined(MBEDTLS_SSL_SRV_C) - if( ssl->conf->endpoint == MBEDTLS_SSL_IS_SERVER ) - { - MBEDTLS_SSL_CHK_BUF_READ_PTR( p, end, - certificate_request_context_len + 3 ); - - /* check whether we got an empty certificate message */ - if( memcmp( p + certificate_request_context_len , "\0\0\0", 3 ) == 0 ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, - ( "client has no certificate - empty certificate message received" ) ); - - ssl->session_negotiate->verify_result = MBEDTLS_X509_BADCERT_MISSING; - if( ssl->conf->authmode == MBEDTLS_SSL_VERIFY_OPTIONAL ) - return( 0 ); - else - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "client certificate required" ) ); - MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_CERT_REQUIRED, - MBEDTLS_ERR_SSL_NO_CLIENT_CERTIFICATE ); - return( MBEDTLS_ERR_SSL_NO_CLIENT_CERTIFICATE ); - } - } - } -#endif /* MBEDTLS_SSL_SRV_C */ - - MBEDTLS_SSL_CHK_BUF_READ_PTR( p, end, 3 ); - certificate_list_len = MBEDTLS_GET_UINT24_BE( p, 0 ); - p += 3; + certificate_list_len = MBEDTLS_GET_UINT24_BE( p, 1 ); + p += 4; /* In theory, the certificate list can be up to 2^24 Bytes, but we don't * support anything beyond 2^16 = 64K. @@ -547,6 +518,7 @@ static int ssl_tls13_parse_certificate( mbedtls_ssl_context *ssl, p += extensions_len; } +exit: /* Check that all the message is consumed. */ if( p != end ) { @@ -556,7 +528,6 @@ static int ssl_tls13_parse_certificate( mbedtls_ssl_context *ssl, return( MBEDTLS_ERR_SSL_DECODE_ERROR ); } -exit: MBEDTLS_SSL_DEBUG_CRT( 3, "peer certificate", ssl->session_negotiate->peer_cert ); return( ret ); @@ -599,36 +570,34 @@ static int ssl_tls13_validate_certificate( mbedtls_ssl_context *ssl ) * Check for that and handle it depending on the * server's authentication mode. */ -#if defined(MBEDTLS_SSL_SRV_C) - if( ssl->conf->endpoint == MBEDTLS_SSL_IS_SERVER && - ssl->session_negotiate->peer_cert == NULL ) + if( ssl->session_negotiate->peer_cert == NULL ) { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "client has no certificate" ) ); +#if defined(MBEDTLS_SSL_SRV_C) + if( ssl->conf->endpoint == MBEDTLS_SSL_IS_SERVER ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "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. */ - - /* Note that for authmode == VERIFY_NONE we don't end up in this - * routine in the first place, because ssl_tls13_read_certificate_coordinate - * will return CERTIFICATE_SKIP. */ - ssl->session_negotiate->verify_result = MBEDTLS_X509_BADCERT_MISSING; - if( authmode == MBEDTLS_SSL_VERIFY_OPTIONAL ) - return( 0 ); - else - return( MBEDTLS_ERR_SSL_NO_CLIENT_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; + if( authmode == MBEDTLS_SSL_VERIFY_OPTIONAL ) + return( 0 ); + else + return( MBEDTLS_ERR_SSL_NO_CLIENT_CERTIFICATE ); + } #endif /* MBEDTLS_SSL_SRV_C */ #if defined(MBEDTLS_SSL_CLI_C) - if( ssl->conf->endpoint == MBEDTLS_SSL_IS_CLIENT && - ssl->session_negotiate->peer_cert == NULL ) - { - MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_NO_CERT, - MBEDTLS_ERR_SSL_FATAL_ALERT_MESSAGE ); - return( MBEDTLS_ERR_SSL_FATAL_ALERT_MESSAGE ); - } + if( ssl->conf->endpoint == MBEDTLS_SSL_IS_CLIENT ) + { + MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_NO_CERT, + MBEDTLS_ERR_SSL_FATAL_ALERT_MESSAGE ); + return( MBEDTLS_ERR_SSL_FATAL_ALERT_MESSAGE ); + } #endif /* MBEDTLS_SSL_CLI_C */ + } #if defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) if( ssl->handshake->sni_ca_chain != NULL ) From 989f06d52d32e7f640c264e86242d4a6c0bc3ada Mon Sep 17 00:00:00 2001 From: XiaokangQian Date: Tue, 17 May 2022 01:50:15 +0000 Subject: [PATCH 5/8] Change some comments base on review Change-Id: I3db2b8ca8162eb368d2f17dfeffee8b25f9edf6f Signed-off-by: XiaokangQian --- library/ssl_tls13_generic.c | 28 +++++++++++++++++++--------- library/ssl_tls13_server.c | 2 +- 2 files changed, 20 insertions(+), 10 deletions(-) diff --git a/library/ssl_tls13_generic.c b/library/ssl_tls13_generic.c index ef27644ec6..49ed112672 100644 --- a/library/ssl_tls13_generic.c +++ b/library/ssl_tls13_generic.c @@ -551,32 +551,39 @@ static int ssl_tls13_parse_certificate( mbedtls_ssl_context *ssl, static int ssl_tls13_validate_certificate( mbedtls_ssl_context *ssl ) { int ret = 0; - int authmode = ssl->conf->authmode; + int authmode = MBEDTLS_SSL_VERIFY_REQUIRED; mbedtls_x509_crt *ca_chain; mbedtls_x509_crl *ca_crl; uint32_t verify_result = 0; /* If SNI was used, overwrite authentication mode * from the configuration. */ +#if defined(MBEDTLS_SSL_SRV_C) + if( ssl->conf->endpoint == MBEDTLS_SSL_IS_SERVER ) + { #if defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) - if( ssl->handshake->sni_authmode != MBEDTLS_SSL_VERIFY_UNSET ) - authmode = ssl->handshake->sni_authmode; + if( ssl->handshake->sni_authmode != MBEDTLS_SSL_VERIFY_UNSET ) + authmode = ssl->handshake->sni_authmode; + else +#endif + authmode = ssl->conf->authmode; + } #endif /* - * If the client hasn't sent a certificate ( i.e. it sent + * If the peer hasn't sent a certificate ( i.e. it sent * an empty certificate chain ), this is reflected in the peer CRT * structure being unset. * Check for that and handle it depending on the - * server's authentication mode. + * authentication mode. */ if( ssl->session_negotiate->peer_cert == NULL ) { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "peer has not sent a certificate" ) ); + #if defined(MBEDTLS_SSL_SRV_C) if( ssl->conf->endpoint == MBEDTLS_SSL_IS_SERVER ) { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "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. @@ -585,7 +592,11 @@ static int ssl_tls13_validate_certificate( mbedtls_ssl_context *ssl ) if( authmode == MBEDTLS_SSL_VERIFY_OPTIONAL ) return( 0 ); else - return( MBEDTLS_ERR_SSL_NO_CLIENT_CERTIFICATE ); + { + MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_NO_CERT, + MBEDTLS_ERR_SSL_FATAL_ALERT_MESSAGE ); + return( MBEDTLS_ERR_SSL_FATAL_ALERT_MESSAGE ); + } } #endif /* MBEDTLS_SSL_SRV_C */ @@ -654,7 +665,6 @@ static int ssl_tls13_validate_certificate( mbedtls_ssl_context *ssl ) ret = 0; } - if( ca_chain == NULL && authmode == MBEDTLS_SSL_VERIFY_REQUIRED ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "got no CA chain" ) ); diff --git a/library/ssl_tls13_server.c b/library/ssl_tls13_server.c index 43018d0dbf..e45db76c2e 100644 --- a/library/ssl_tls13_server.c +++ b/library/ssl_tls13_server.c @@ -1626,7 +1626,7 @@ int mbedtls_ssl_tls13_handshake_server_step( mbedtls_ssl_context *ssl ) case MBEDTLS_SSL_CLIENT_CERTIFICATE: ret = mbedtls_ssl_tls13_process_certificate( ssl ); - if( ret == 0 && ssl->session_negotiate->peer_cert != NULL) + if( ret == 0 && ssl->session_negotiate->peer_cert != NULL ) { mbedtls_ssl_handshake_set_state( ssl, MBEDTLS_SSL_CLIENT_CERTIFICATE_VERIFY ); From aca9048b5fc80677a45fbabf884eec18bad3a582 Mon Sep 17 00:00:00 2001 From: XiaokangQian Date: Thu, 19 May 2022 07:19:31 +0000 Subject: [PATCH 6/8] Change base on review Fix comments Add test cases for client authentication with empty certificate Change-Id: Id8a741ddd997ca92e36832f26088eb0e67830ad8 Signed-off-by: XiaokangQian --- library/ssl_tls13_generic.c | 6 +++--- library/ssl_tls13_server.c | 6 ++++-- tests/ssl-opt.sh | 34 ++++++++++++++++++++++++++++++++++ 3 files changed, 41 insertions(+), 5 deletions(-) diff --git a/library/ssl_tls13_generic.c b/library/ssl_tls13_generic.c index 49ed112672..17efa8c22b 100644 --- a/library/ssl_tls13_generic.c +++ b/library/ssl_tls13_generic.c @@ -594,8 +594,8 @@ static int ssl_tls13_validate_certificate( mbedtls_ssl_context *ssl ) else { MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_NO_CERT, - MBEDTLS_ERR_SSL_FATAL_ALERT_MESSAGE ); - return( MBEDTLS_ERR_SSL_FATAL_ALERT_MESSAGE ); + MBEDTLS_ERR_SSL_NO_CLIENT_CERTIFICATE ); + return( MBEDTLS_ERR_SSL_NO_CLIENT_CERTIFICATE ); } } #endif /* MBEDTLS_SSL_SRV_C */ @@ -741,9 +741,9 @@ int mbedtls_ssl_tls13_process_certificate( mbedtls_ssl_context *ssl ) mbedtls_ssl_add_hs_msg_to_checksum( ssl, MBEDTLS_SSL_HS_CERTIFICATE, buf, buf_len ); -#endif /* MBEDTLS_KEY_EXCHANGE_ECDHE_ECDSA_ENABLED */ cleanup: +#endif /* MBEDTLS_KEY_EXCHANGE_ECDHE_ECDSA_ENABLED */ MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= parse certificate" ) ); return( ret ); diff --git a/library/ssl_tls13_server.c b/library/ssl_tls13_server.c index e45db76c2e..f3843b1e85 100644 --- a/library/ssl_tls13_server.c +++ b/library/ssl_tls13_server.c @@ -1517,10 +1517,12 @@ static int ssl_tls13_process_client_finished( mbedtls_ssl_context *ssl ) { int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; - MBEDTLS_SSL_DEBUG_MSG( 1, - ( "Switch to handshake traffic keys for outbound traffic" ) ); if( ! ssl->handshake->certificate_request_sent ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, + ( "Switch to handshake traffic keys for inbound traffic" ) ); mbedtls_ssl_set_inbound_transform( ssl, ssl->handshake->transform_handshake ); + } ret = mbedtls_ssl_tls13_process_finished_message( ssl ); if( ret != 0 ) return( ret ); diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index eb9c1f48f9..33a9552259 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -11339,6 +11339,40 @@ run_test "TLS 1.3: Server side check - mbedtls with client authentication" \ -s "=> parse client hello" \ -s "<= parse client hello" +requires_config_enabled MBEDTLS_SSL_PROTO_TLS1_3 +requires_config_enabled MBEDTLS_DEBUG_C +requires_config_enabled MBEDTLS_SSL_SRV_C +requires_config_enabled MBEDTLS_SSL_CLI_C +run_test "TLS 1.3: Server side check - mbedtls with client empty certificate" \ + "$P_SRV debug_level=4 auth_mode=required crt_file=data_files/server5.crt key_file=data_files/server5.key force_version=tls13 tickets=0" \ + "$P_CLI debug_level=4 crt_file=none key_file=none force_version=tls13" \ + 1 \ + -s "tls13 server state: MBEDTLS_SSL_CLIENT_HELLO" \ + -s "tls13 server state: MBEDTLS_SSL_SERVER_HELLO" \ + -s "tls13 server state: MBEDTLS_SSL_ENCRYPTED_EXTENSIONS" \ + -s "tls13 server state: MBEDTLS_SSL_SERVER_CERTIFICATE" \ + -s "=> write certificate request" \ + -s "SSL - No client certification received from the client, but required by the authentication mode" \ + -c "client state: MBEDTLS_SSL_CERTIFICATE_REQUEST" \ + -s "=> parse client hello" \ + -s "<= parse client hello" + +requires_config_enabled MBEDTLS_SSL_PROTO_TLS1_3 +requires_config_enabled MBEDTLS_DEBUG_C +requires_config_enabled MBEDTLS_SSL_SRV_C +requires_config_enabled MBEDTLS_SSL_CLI_C +run_test "TLS 1.3: Server side check - mbedtls with optional client authentication" \ + "$P_SRV debug_level=4 auth_mode=optional crt_file=data_files/server5.crt key_file=data_files/server5.key force_version=tls13 tickets=0" \ + "$P_CLI debug_level=4 force_version=tls13 crt_file=none key_file=none" \ + 0 \ + -s "tls13 server state: MBEDTLS_SSL_CLIENT_HELLO" \ + -s "tls13 server state: MBEDTLS_SSL_SERVER_HELLO" \ + -s "tls13 server state: MBEDTLS_SSL_ENCRYPTED_EXTENSIONS" \ + -s "tls13 server state: MBEDTLS_SSL_SERVER_CERTIFICATE" \ + -s "=> write certificate request" \ + -c "client state: MBEDTLS_SSL_CERTIFICATE_REQUEST" \ + -s "=> parse client hello" \ + -s "<= parse client hello" requires_config_enabled MBEDTLS_DEBUG_C requires_config_enabled MBEDTLS_SSL_CLI_C From 9a4e1dd8a6945373ea11e51ebb9b94e57fbba637 Mon Sep 17 00:00:00 2001 From: XiaokangQian Date: Thu, 26 May 2022 00:58:11 +0000 Subject: [PATCH 7/8] Add back openssl client auth test Change-Id: Iea3b70381c3851102c542d1c55c0303bc3a14a92 Signed-off-by: XiaokangQian --- tests/ssl-opt.sh | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index 33a9552259..21d17f9728 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -11241,10 +11241,6 @@ run_test "TLS 1.3: Server side check - openssl" \ -s "tls13 server state: MBEDTLS_SSL_CLIENT_FINISHED" \ -s "tls13 server state: MBEDTLS_SSL_HANDSHAKE_WRAPUP" -# Skip this test before openssl exit code issue fixed -# On fail, openssl return different exit code on OpenCI and internal CI for -# this test. -skip_next_test requires_config_enabled MBEDTLS_SSL_PROTO_TLS1_3 requires_config_enabled MBEDTLS_DEBUG_C requires_config_enabled MBEDTLS_SSL_SRV_C @@ -11252,7 +11248,7 @@ requires_openssl_tls1_3 run_test "TLS 1.3: Server side check - openssl with client authentication" \ "$P_SRV debug_level=4 auth_mode=required crt_file=data_files/server5.crt key_file=data_files/server5.key force_version=tls13 tickets=0" \ "$O_NEXT_CLI -msg -debug -cert data_files/server5.crt -key data_files/server5.key -tls1_3 -no_middlebox" \ - 1 \ + 0 \ -s "tls13 server state: MBEDTLS_SSL_CLIENT_HELLO" \ -s "tls13 server state: MBEDTLS_SSL_SERVER_HELLO" \ -s "tls13 server state: MBEDTLS_SSL_ENCRYPTED_EXTENSIONS" \ From e7a5da597f3c6424b01493e9be2263075c752478 Mon Sep 17 00:00:00 2001 From: XiaokangQian Date: Mon, 30 May 2022 00:59:29 +0000 Subject: [PATCH 8/8] Remove SNI related code Change-Id: Ic44bdb27b1bdc5c9057078dfed936fc36bddebbe Signed-off-by: XiaokangQian --- library/ssl_tls13_generic.c | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/library/ssl_tls13_generic.c b/library/ssl_tls13_generic.c index 17efa8c22b..239be53a18 100644 --- a/library/ssl_tls13_generic.c +++ b/library/ssl_tls13_generic.c @@ -560,14 +560,7 @@ static int ssl_tls13_validate_certificate( mbedtls_ssl_context *ssl ) * from the configuration. */ #if defined(MBEDTLS_SSL_SRV_C) if( ssl->conf->endpoint == MBEDTLS_SSL_IS_SERVER ) - { -#if defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) - if( ssl->handshake->sni_authmode != MBEDTLS_SSL_VERIFY_UNSET ) - authmode = ssl->handshake->sni_authmode; - else -#endif - authmode = ssl->conf->authmode; - } + authmode = ssl->conf->authmode; #endif /*