From 647719a172f145aab621e88c7f0d38a0381f0525 Mon Sep 17 00:00:00 2001 From: XiaokangQian Date: Tue, 7 Dec 2021 09:16:29 +0000 Subject: [PATCH 01/24] Add hello retry request in client side Signed-off-by: XiaokangQian --- include/mbedtls/ssl.h | 2 + library/ssl_misc.h | 9 + library/ssl_tls.c | 99 ++++++++- library/ssl_tls13_client.c | 421 ++++++++++++++++++++++++++++++++++++- 4 files changed, 522 insertions(+), 9 deletions(-) diff --git a/include/mbedtls/ssl.h b/include/mbedtls/ssl.h index f9bbf0c8b..350ee2cb2 100644 --- a/include/mbedtls/ssl.h +++ b/include/mbedtls/ssl.h @@ -505,6 +505,7 @@ #define MBEDTLS_SSL_HS_CERTIFICATE_VERIFY 15 #define MBEDTLS_SSL_HS_CLIENT_KEY_EXCHANGE 16 #define MBEDTLS_SSL_HS_FINISHED 20 +#define MBEDTLS_SSL_HS_MESSAGE_HASH 254 /* * TLS extensions @@ -643,6 +644,7 @@ typedef enum MBEDTLS_SSL_CLIENT_CERTIFICATE_VERIFY, #if defined(MBEDTLS_SSL_TLS1_3_COMPATIBILITY_MODE) MBEDTLS_SSL_CLIENT_CCS_AFTER_SERVER_FINISHED, + MBEDTLS_SSL_CLIENT_CCS_BEFORE_2ND_CLIENT_HELLO, #endif /* MBEDTLS_SSL_TLS1_3_COMPATIBILITY_MODE */ #endif /* MBEDTLS_SSL_PROTO_TLS1_3 */ } diff --git a/library/ssl_misc.h b/library/ssl_misc.h index 788fafdd9..b58b0af90 100644 --- a/library/ssl_misc.h +++ b/library/ssl_misc.h @@ -591,6 +591,11 @@ struct mbedtls_ssl_handshake_params int tls13_kex_modes; /*!< key exchange modes for TLS 1.3 */ #endif /* MBEDTLS_SSL_PROTO_TLS1_3 */ +#if defined(MBEDTLS_SSL_CLI_C) + /*!< Number of Hello Retry Request messages received from the server. */ + int hello_retry_requests_received; +#endif /* MBEDTLS_SSL_CLI_C */ + #if defined(MBEDTLS_SSL_PROTO_TLS1_2) && \ defined(MBEDTLS_KEY_EXCHANGE_WITH_CERT_ENABLED) mbedtls_ssl_sig_hash_set_t hash_algs; /*!< Set of suitable sig-hash pairs */ @@ -1442,6 +1447,8 @@ void mbedtls_ssl_update_out_pointers( mbedtls_ssl_context *ssl, void mbedtls_ssl_update_in_pointers( mbedtls_ssl_context *ssl ); int mbedtls_ssl_session_reset_int( mbedtls_ssl_context *ssl, int partial ); +void mbedtls_ssl_tls13_session_reset_msg_layer( mbedtls_ssl_context *ssl, + int partial ); /* * Send pending alert @@ -1721,6 +1728,8 @@ void mbedtls_ssl_tls13_add_hs_msg_to_checksum( mbedtls_ssl_context *ssl, unsigned char const *msg, size_t msg_len ); +int mbedtls_ssl_reset_transcript_for_hrr( mbedtls_ssl_context *ssl ); + #endif /* MBEDTLS_SSL_PROTO_TLS1_3 */ #if defined(MBEDTLS_KEY_EXCHANGE_WITH_CERT_ENABLED) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 5e8b60b9b..759adecea 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -3400,8 +3400,8 @@ error: * If partial is non-zero, keep data in the input buffer and client ID. * (Use when a DTLS client reconnects from the same port.) */ -static void ssl_session_reset_msg_layer( mbedtls_ssl_context *ssl, - int partial ) +void mbedtls_ssl_tls13_session_reset_msg_layer( mbedtls_ssl_context *ssl, + int partial ) { #if defined(MBEDTLS_SSL_VARIABLE_BUFFER_LENGTH) size_t in_buf_len = ssl->in_buf_len; @@ -3467,7 +3467,7 @@ int mbedtls_ssl_session_reset_int( mbedtls_ssl_context *ssl, int partial ) ssl->state = MBEDTLS_SSL_HELLO_REQUEST; - ssl_session_reset_msg_layer( ssl, partial ); + mbedtls_ssl_tls13_session_reset_msg_layer( ssl, partial ); /* Reset renegotiation state */ #if defined(MBEDTLS_SSL_RENEGOTIATION) @@ -7384,6 +7384,7 @@ int mbedtls_ssl_get_handshake_transcript( mbedtls_ssl_context *ssl, } return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); } + #endif /* !MBEDTLS_USE_PSA_CRYPTO */ #if defined(MBEDTLS_KEY_EXCHANGE_WITH_CERT_ENABLED) || \ @@ -7614,5 +7615,97 @@ int mbedtls_ssl_write_sig_alg_ext( mbedtls_ssl_context *ssl, unsigned char *buf, return( 0 ); } #endif /* MBEDTLS_KEY_EXCHANGE_WITH_CERT_ENABLED */ +#if defined(MBEDTLS_SSL_PROTO_TLS1_3_EXPERIMENTAL) + +static int ssl_hash_transcript_core( mbedtls_ssl_context *ssl, + mbedtls_md_type_t md, + unsigned char *transcript, + size_t len, + size_t *olen ) +{ + int ret; + size_t hash_size; + + if( len < 4 ) + return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); + + ret = mbedtls_ssl_get_handshake_transcript( ssl, md, + transcript + 4, + len - 4, + &hash_size ); + if( ret != 0 ) + { + MBEDTLS_SSL_DEBUG_RET( 4, "mbedtls_ssl_get_handshake_transcript", ret ); + return( ret ); + } + + transcript[0] = MBEDTLS_SSL_HS_MESSAGE_HASH; + transcript[1] = 0; + transcript[2] = 0; + transcript[3] = (unsigned char) hash_size; + + *olen = 4 + hash_size; + return( 0 ); +} + +/* Reset SSL context and update hash for handling HRR. + * + * Replace Transcript-Hash(X) by + * Transcript-Hash( message_hash || + * 00 00 Hash.length || + * X ) + * A few states of the handshake are preserved, including: + * - session ID + * - session ticket + * - negotiated ciphersuite + */ +int mbedtls_ssl_reset_transcript_for_hrr( mbedtls_ssl_context *ssl ) +{ + int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; + unsigned char hash_transcript[ MBEDTLS_MD_MAX_SIZE + 4 ]; + size_t hash_olen; + + MBEDTLS_SSL_DEBUG_MSG( 3, ( "Reset SSL session for HRR" ) ); + +#if defined(MBEDTLS_SHA256_C) + ret = ssl_hash_transcript_core( ssl, MBEDTLS_MD_SHA256, + hash_transcript, + sizeof( hash_transcript ), + &hash_olen ); + if( ret != 0 ) + { + MBEDTLS_SSL_DEBUG_RET( 4, "ssl_hash_transcript_core", ret ); + return( ret ); + } + MBEDTLS_SSL_DEBUG_BUF( 4, "Truncated SHA-256 handshake transcript", + hash_transcript, hash_olen ); + +#if defined(MBEDTLS_USE_PSA_CRYPTO) + psa_hash_abort( &ssl->handshake->fin_sha256_psa ); + psa_hash_setup( &ssl->handshake->fin_sha256_psa, PSA_ALG_SHA_256 ); +#else + mbedtls_sha256_starts( &ssl->handshake->fin_sha256, 0 ); +#endif + ssl_update_checksum_sha256( ssl, hash_transcript, hash_olen ); +#endif /* MBEDTLS_SHA256_C */ + +#if defined(MBEDTLS_SHA512_C) + ret = ssl_hash_transcript_core( ssl, MBEDTLS_MD_SHA384, + hash_transcript, + sizeof( hash_transcript ), + &hash_olen ); + if( ret != 0 ) + { + MBEDTLS_SSL_DEBUG_RET( 4, "ssl_hash_transcript_core", ret ); + return( ret ); + } + MBEDTLS_SSL_DEBUG_BUF( 4, "Truncated SHA-384 handshake transcript", + hash_transcript, hash_olen ); +#endif /* MBEDTLS_SHA512_C */ + + return( ret ); +} + +#endif /* MBEDTLS_SSL_PROTO_TLS1_3_EXPERIMENTAL */ #endif /* MBEDTLS_SSL_TLS_C */ diff --git a/library/ssl_tls13_client.c b/library/ssl_tls13_client.c index d046495cb..9f80d8c4d 100644 --- a/library/ssl_tls13_client.c +++ b/library/ssl_tls13_client.c @@ -115,6 +115,48 @@ static int ssl_tls13_parse_supported_versions_ext( mbedtls_ssl_context *ssl, #if defined(MBEDTLS_KEY_EXCHANGE_WITH_CERT_ENABLED) +/* + * Key Shares Extension + * + * enum { + * ... (0xFFFF) + * } NamedGroup; + * + * struct { + * NamedGroup group; + * opaque key_exchange<1..2^16-1>; + * } KeyShareEntry; + * + * struct { + * select(role) { + * case client: + * KeyShareEntry client_shares<0..2^16-1>; + * } + * } KeyShare; + */ + +static int ssl_reset_ecdhe_share( mbedtls_ssl_context *ssl ) +{ + mbedtls_ecdh_free( &ssl->handshake->ecdh_ctx ); + return( 0 ); +} + +static int ssl_reset_key_share( mbedtls_ssl_context *ssl ) +{ + uint16_t group_id = ssl->handshake->offered_group_id; + if( group_id == 0 ) + return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); + + if( mbedtls_ssl_tls13_named_group_is_ecdhe( group_id ) ) + return( ssl_reset_ecdhe_share( ssl ) ); + else if( 0 /* other KEMs? */ ) + { + /* Do something */ + } + + return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); +} + /* * Functions for writing key_share extension. */ @@ -1208,6 +1250,370 @@ cleanup: return( ret ); } +static int ssl_hrr_parse( mbedtls_ssl_context *ssl, + const unsigned char *buf, size_t buflen ) +{ + int ret; /* return value */ + int i; /* scratch value */ + /* pointer to the end of the buffer for length checks */ + const unsigned char* msg_end = buf + buflen; + + size_t ext_len; /* stores length of all extensions */ + unsigned int ext_id; /* id of an extension */ + const unsigned char* ext; /* pointer to an individual extension */ + unsigned int ext_size; /* size of an individual extension */ + + const mbedtls_ssl_ciphersuite_t* suite_info; /* pointer to ciphersuite */ + +#if defined(MBEDTLS_SSL_COOKIE_C) + size_t cookie_len; + unsigned char *cookie; +#endif /* MBEDTLS_SSL_COOKIE_C */ + + /* Check for minimal length */ + /* struct { + * ProtocolVersion legacy_version = 0x0303; + * Random random; + * opaque legacy_session_id_echo<0..32>; + * CipherSuite cipher_suite; + * uint8 legacy_compression_method = 0; + * Extension extensions<6..2 ^ 16 - 1>; + * } ServerHello; + * + * + * 38 = 32 ( random bytes ) + 2 ( ciphersuite ) + 2 ( version ) + + * 1 ( legacy_compression_method ) + 1 ( minimum for legacy_session_id_echo ) + */ + if( buflen < 38 ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, + ( "bad hello retry request message - min size not reached" ) ); + MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR, + MBEDTLS_ERR_SSL_DECODE_ERROR ); + return( MBEDTLS_ERR_SSL_DECODE_ERROR ); + } + + MBEDTLS_SSL_DEBUG_BUF( 4, "hello retry request", buf, buflen ); + + MBEDTLS_SSL_DEBUG_BUF( 3, "hello retry request, version", buf + 0, 2 ); + mbedtls_ssl_read_version( &ssl->major_ver, &ssl->minor_ver, + ssl->conf->transport, buf + 0 ); + + /* The version field must contain 0x303 */ + if( buf[0] != 0x03 || buf[1] != 0x03 ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "Unsupported version of TLS." ) ); + MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_PROTOCOL_VERSION, + MBEDTLS_ERR_SSL_BAD_PROTOCOL_VERSION ); + return( MBEDTLS_ERR_SSL_BAD_PROTOCOL_VERSION ); + } + + /* skip version */ + buf += 2; + + /* Internally we use the correct 1.3 version */ + ssl->major_ver = 0x03; + ssl->minor_ver = 0x04; + + /* store server-provided random values */ + memcpy( ssl->handshake->randbytes + 32, buf, 32 ); + MBEDTLS_SSL_DEBUG_BUF( 3, "hello retry request, random bytes", buf + 2, 32 ); + + /* skip random bytes */ + buf += 32; + + if( ssl_tls13_check_server_hello_session_id_echo( ssl, &buf, msg_end ) != 0 ) + { + MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_HANDSHAKE_FAILURE, + MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE ); + return( MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE ); + } + + /* read server-selected ciphersuite, which follows random bytes */ + i = ( buf[0] << 8 ) | buf[1]; + + /* skip ciphersuite */ + buf += 2; + + /* TBD: Check whether we have offered this ciphersuite */ + /* Via the force_ciphersuite version we may have instructed the client */ + /* to use a difference ciphersuite. */ + + /* Configure ciphersuites */ + ssl->handshake->ciphersuite_info = mbedtls_ssl_ciphersuite_from_id( i ); + + if( ssl->handshake->ciphersuite_info == NULL ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, + ( "ciphersuite info for %04x not found", (unsigned int)i ) ); + MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_INTERNAL_ERROR, + MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); + return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); + } + + mbedtls_ssl_optimize_checksum( ssl, ssl->handshake->ciphersuite_info ); + + ssl->session_negotiate->ciphersuite = i; + + suite_info = mbedtls_ssl_ciphersuite_from_id( + ssl->session_negotiate->ciphersuite ); + if( suite_info == NULL ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad hello retry request message" ) ); + MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_HANDSHAKE_FAILURE, + MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE ); + return( MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE ); + } + + MBEDTLS_SSL_DEBUG_MSG( 3, + ( "hello retry request, chosen ciphersuite: ( %04x ) - %s", + (unsigned int)i, suite_info->name ) ); + +#if defined(MBEDTLS_HAVE_TIME) + ssl->session_negotiate->start = time( NULL ); +#endif /* MBEDTLS_HAVE_TIME */ + + i = 0; + while ( 1 ) + { + if( ssl->conf->ciphersuite_list[i] == 0 ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad hello retry request message" ) ); + MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_HANDSHAKE_FAILURE, + MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE ); + return( MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE ); + } + + if( ssl->conf->ciphersuite_list[i++] == + ssl->session_negotiate->ciphersuite ) + { + break; + } + } + + /* Ensure that compression method is set to zero */ + if( buf[0] != 0 ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad hello retry request message" ) ); + MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_ILLEGAL_PARAMETER, + MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER ); + return( MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER ); + } + + /* skip compression */ + buf++; + + /* Are we reading beyond the message buffer? */ + if( ( buf + 2 ) > msg_end ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad hello retry request message" ) ); + MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR, + MBEDTLS_ERR_SSL_DECODE_ERROR ); + return( MBEDTLS_ERR_SSL_DECODE_ERROR ); + } + + ext_len = ( ( buf[0] << 8 ) | ( buf[1] ) ); + buf += 2; /* skip extension length */ + + /* Are we reading beyond the message buffer? */ + if( ( buf + ext_len ) > msg_end ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad hello retry request message" ) ); + MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR, + MBEDTLS_ERR_SSL_DECODE_ERROR ); + return( MBEDTLS_ERR_SSL_DECODE_ERROR ); + } + + ext = buf; + + MBEDTLS_SSL_DEBUG_MSG( 3, + ( "hello retry request, total extension length: %" + MBEDTLS_PRINTF_SIZET , ext_len ) ); + + MBEDTLS_SSL_DEBUG_BUF( 3, "extensions", ext, ext_len ); + + while ( ext_len ) + { + ext_id = ( ( ext[0] << 8 ) | ( ext[1] ) ); + ext_size = ( ( ext[2] << 8 ) | ( ext[3] ) ); + + if( ext_size + 4 > ext_len ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad hello retry request message" ) ); + MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR, + MBEDTLS_ERR_SSL_DECODE_ERROR ); + return( MBEDTLS_ERR_SSL_DECODE_ERROR ); + } + + switch( ext_id ) + { +#if defined(MBEDTLS_SSL_COOKIE_C) + case MBEDTLS_TLS_EXT_COOKIE: + + /* Retrieve length field of cookie */ + if( ext_size >= 2 ) + { + cookie = (unsigned char *) ( ext + 4 ); + cookie_len = ( cookie[0] << 8 ) | cookie[1]; + cookie += 2; + } + else + { + MBEDTLS_SSL_DEBUG_MSG( 1, + ( "bad HRR message - cookie length mismatch" ) ); + return( MBEDTLS_ERR_SSL_DECODE_ERROR ); + } + + if( ( cookie_len + 2 ) != ext_size ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, + ( "bad HRR message - cookie length mismatch" ) ); + return( MBEDTLS_ERR_SSL_DECODE_ERROR ); + } + + MBEDTLS_SSL_DEBUG_BUF( 3, "cookie extension", cookie, cookie_len ); + + mbedtls_free( ssl->handshake->verify_cookie ); + + ssl->handshake->verify_cookie = mbedtls_calloc( 1, cookie_len ); + if( ssl->handshake->verify_cookie == NULL ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, + ( "alloc failed ( %" MBEDTLS_PRINTF_SIZET " bytes )", + cookie_len ) ); + return( MBEDTLS_ERR_SSL_ALLOC_FAILED ); + } + + memcpy( ssl->handshake->verify_cookie, cookie, cookie_len ); + ssl->handshake->verify_cookie_len = (unsigned char) cookie_len; + break; +#endif /* MBEDTLS_SSL_COOKIE_C */ + + case MBEDTLS_TLS_EXT_SUPPORTED_VERSIONS: + MBEDTLS_SSL_DEBUG_MSG( 3, ( "found supported_versions extension" ) ); + + ret = ssl_tls13_parse_supported_versions_ext( ssl, + ext + 4, + ext + 4 + ext_size ); + if( ret != 0 ) + return( ret ); + break; + +#if defined(MBEDTLS_ECDH_C) || defined(MBEDTLS_ECDSA_C) + case MBEDTLS_TLS_EXT_KEY_SHARE: + { + /* Variables for parsing the key_share */ + const mbedtls_ecp_group_id* grp_id; + const mbedtls_ecp_curve_info *curve_info = NULL; + int tls_id; + int found = 0; + + MBEDTLS_SSL_DEBUG_BUF( 3, "key_share extension", ext + 4, ext_size ); + + /* Read selected_group */ + tls_id = ( ( ext[4] << 8 ) | ( ext[5] ) ); + MBEDTLS_SSL_DEBUG_MSG( 3, ( "selected_group ( %d )", tls_id ) ); + + /* Upon receipt of this extension in a HelloRetryRequest, the client + * MUST first verify that the selected_group field corresponds to a + * group which was provided in the "supported_groups" extension in the + * original ClientHello. + * The supported_group was based on the info in ssl->conf->curve_list. + * + * If the server provided a key share that was not sent in the ClientHello + * then the client MUST abort the handshake with an "illegal_parameter" alert. */ + for( grp_id = ssl->conf->curve_list; *grp_id != MBEDTLS_ECP_DP_NONE; grp_id++ ) + { + curve_info = mbedtls_ecp_curve_info_from_grp_id( *grp_id ); + if( curve_info == NULL || curve_info->tls_id != tls_id ) + continue; + + /* We found a match */ + found = 1; + break; + } + + /* Client MUST verify that the selected_group field does not + * correspond to a group which was provided in the "key_share" + * extension in the original ClientHello. If the server sent an + * HRR message with a key share already provided in the + * ClientHello then the client MUST abort the handshake with + * an "illegal_parameter" alert. */ + if( found == 0 || tls_id == ssl->handshake->offered_group_id ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "Invalid key share in HRR" ) ); + MBEDTLS_SSL_PEND_FATAL_ALERT( + MBEDTLS_SSL_ALERT_MSG_ILLEGAL_PARAMETER, + MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER ); + return( MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER ); + } + + /* Remember server's preference for next ClientHello */ + ssl->handshake->offered_group_id= tls_id; + break; + } + +#endif /* MBEDTLS_ECDH_C || MBEDTLS_ECDSA_C */ + default: + MBEDTLS_SSL_DEBUG_MSG( 3, + ( "unknown extension found: %u ( ignoring )", + ext_id ) ); + } + + /* Jump to next extension */ + ext_len -= 4 + ext_size; + ext += 4 + ext_size; + + if( ext_len > 0 && ext_len < 4 ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad hello retry request message" ) ); + return( MBEDTLS_ERR_SSL_DECODE_ERROR ); + } + + } + + return( 0 ); +} + +static int ssl_hrr_postprocess( mbedtls_ssl_context *ssl ) +{ + int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; + + if( ssl->handshake->hello_retry_requests_received > 0 ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "Multiple HRRs received" ) ); + MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_LEVEL_FATAL, + MBEDTLS_SSL_ALERT_MSG_HANDSHAKE_FAILURE ); + return( MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE ); + } + + ssl->handshake->hello_retry_requests_received++; + +#if defined(MBEDTLS_SSL_TLS1_3_COMPATIBILITY_MODE) + /* If not offering early data, the client sends a dummy CCS record + * immediately before its second flight. This may either be before + * its second ClientHello or before its encrypted handshake flight. */ + mbedtls_ssl_handshake_set_state( ssl, + MBEDTLS_SSL_CLIENT_CCS_BEFORE_2ND_CLIENT_HELLO ); +#else + mbedtls_ssl_handshake_set_state( ssl, MBEDTLS_SSL_CLIENT_HELLO ); +#endif /* MBEDTLS_SSL_TLS1_3_COMPATIBILITY_MODE */ + + mbedtls_ssl_tls13_session_reset_msg_layer( ssl, 0 ); + + /* Reset everything that's going to be re-generated in the new ClientHello. + * + * Currently, we're always resetting the key share, even if the server + * was fine with it. Once we have separated key share generation from + * key share writing, we can confine this to the case where the server + * requested a different share. */ + ret = ssl_reset_key_share( ssl ); + if( ret != 0 ) + return( ret ); + + return( 0 ); +} + /* * Wait and parse ServerHello handshake message. * Handler for MBEDTLS_SSL_SERVER_HELLO @@ -1215,8 +1621,8 @@ cleanup: static int ssl_tls13_process_server_hello( mbedtls_ssl_context *ssl ) { int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; - unsigned char *buf; - size_t buf_len; + unsigned char *buf = NULL; + size_t buf_len = 0; MBEDTLS_SSL_DEBUG_MSG( 2, ( "=> %s", __func__ ) ); @@ -1246,10 +1652,13 @@ static int ssl_tls13_process_server_hello( mbedtls_ssl_context *ssl ) } else if( ret == SSL_SERVER_HELLO_COORDINATE_HRR ) { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "HRR not supported" ) ); - MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_HANDSHAKE_FAILURE , - MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE ); - ret = MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE; + MBEDTLS_SSL_PROC_CHK( ssl_hrr_parse( ssl, buf, buf_len ) ); + MBEDTLS_SSL_PROC_CHK( mbedtls_ssl_reset_transcript_for_hrr( ssl ) ); + + mbedtls_ssl_tls13_add_hs_msg_to_checksum( ssl, MBEDTLS_SSL_HS_SERVER_HELLO, + buf, buf_len ); + + MBEDTLS_SSL_PROC_CHK( ssl_hrr_postprocess( ssl ) ); } cleanup: From 51eff22c9b33bbd540e34b397ae1bea87eb454f0 Mon Sep 17 00:00:00 2001 From: XiaokangQian Date: Fri, 10 Dec 2021 10:33:56 +0000 Subject: [PATCH 02/24] Align oode style with server hello parse Signed-off-by: XiaokangQian --- library/ssl_tls.c | 4 +- library/ssl_tls13_client.c | 257 ++++++++++++++----------------------- 2 files changed, 100 insertions(+), 161 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 759adecea..290c75e5f 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -7615,7 +7615,7 @@ int mbedtls_ssl_write_sig_alg_ext( mbedtls_ssl_context *ssl, unsigned char *buf, return( 0 ); } #endif /* MBEDTLS_KEY_EXCHANGE_WITH_CERT_ENABLED */ -#if defined(MBEDTLS_SSL_PROTO_TLS1_3_EXPERIMENTAL) +#if defined(MBEDTLS_SSL_PROTO_TLS1_3) static int ssl_hash_transcript_core( mbedtls_ssl_context *ssl, mbedtls_md_type_t md, @@ -7706,6 +7706,6 @@ int mbedtls_ssl_reset_transcript_for_hrr( mbedtls_ssl_context *ssl ) return( ret ); } -#endif /* MBEDTLS_SSL_PROTO_TLS1_3_EXPERIMENTAL */ +#endif /* MBEDTLS_SSL_PROTO_TLS1_3 */ #endif /* MBEDTLS_SSL_TLS_C */ diff --git a/library/ssl_tls13_client.c b/library/ssl_tls13_client.c index 9f80d8c4d..18b9074e6 100644 --- a/library/ssl_tls13_client.c +++ b/library/ssl_tls13_client.c @@ -789,7 +789,8 @@ cleanup: /* Returns a negative value on failure, and otherwise * - SSL_SERVER_HELLO_COORDINATE_HELLO or * - SSL_SERVER_HELLO_COORDINATE_HRR - * to indicate which message is expected and to be parsed next. */ + * to indicate which message is expected and to be parsed next. + */ #define SSL_SERVER_HELLO_COORDINATE_HELLO 0 #define SSL_SERVER_HELLO_COORDINATE_HRR 1 static int ssl_server_hello_is_hrr( mbedtls_ssl_context *ssl, @@ -1251,28 +1252,27 @@ cleanup: } static int ssl_hrr_parse( mbedtls_ssl_context *ssl, - const unsigned char *buf, size_t buflen ) + const unsigned char *buf, + const unsigned char *end ) { - int ret; /* return value */ - int i; /* scratch value */ + int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; + int cipher_suite; + /* pointer to the end of the buffer for length checks */ - const unsigned char* msg_end = buf + buflen; + const unsigned char *p = buf; + const unsigned char *extensions_end; + size_t extensions_len; /* stores length of all extensions */ - size_t ext_len; /* stores length of all extensions */ - unsigned int ext_id; /* id of an extension */ - const unsigned char* ext; /* pointer to an individual extension */ - unsigned int ext_size; /* size of an individual extension */ - - const mbedtls_ssl_ciphersuite_t* suite_info; /* pointer to ciphersuite */ + const mbedtls_ssl_ciphersuite_t* ciphersuite_info; /* pointer to ciphersuite */ #if defined(MBEDTLS_SSL_COOKIE_C) size_t cookie_len; unsigned char *cookie; #endif /* MBEDTLS_SSL_COOKIE_C */ - /* Check for minimal length */ - /* struct { - * ProtocolVersion legacy_version = 0x0303; + /* Check for minimal length + * struct { + * ProtocolVersion legacy_version = 0x0303; * Random random; * opaque legacy_session_id_echo<0..32>; * CipherSuite cipher_suite; @@ -1280,119 +1280,90 @@ static int ssl_hrr_parse( mbedtls_ssl_context *ssl, * Extension extensions<6..2 ^ 16 - 1>; * } ServerHello; * - * * 38 = 32 ( random bytes ) + 2 ( ciphersuite ) + 2 ( version ) + - * 1 ( legacy_compression_method ) + 1 ( minimum for legacy_session_id_echo ) + * 1 ( legacy_compression_method ) + + * 1 ( minimum for legacy_session_id_echo ) */ - if( buflen < 38 ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, - ( "bad hello retry request message - min size not reached" ) ); - MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR, - MBEDTLS_ERR_SSL_DECODE_ERROR ); - return( MBEDTLS_ERR_SSL_DECODE_ERROR ); - } + MBEDTLS_SSL_CHK_BUF_READ_PTR( p, end, 38 ); - MBEDTLS_SSL_DEBUG_BUF( 4, "hello retry request", buf, buflen ); + MBEDTLS_SSL_DEBUG_BUF( 4, "hello retry request", p, end - p ); - MBEDTLS_SSL_DEBUG_BUF( 3, "hello retry request, version", buf + 0, 2 ); - mbedtls_ssl_read_version( &ssl->major_ver, &ssl->minor_ver, - ssl->conf->transport, buf + 0 ); + MBEDTLS_SSL_DEBUG_BUF( 3, "hello retry request, version", p, 2 ); /* The version field must contain 0x303 */ - if( buf[0] != 0x03 || buf[1] != 0x03 ) + if( MBEDTLS_GET_UINT16_BE( p, 0 ) != 0x303 ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "Unsupported version of TLS." ) ); MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_PROTOCOL_VERSION, - MBEDTLS_ERR_SSL_BAD_PROTOCOL_VERSION ); + MBEDTLS_ERR_SSL_BAD_PROTOCOL_VERSION ); return( MBEDTLS_ERR_SSL_BAD_PROTOCOL_VERSION ); } /* skip version */ - buf += 2; + p += 2; /* Internally we use the correct 1.3 version */ - ssl->major_ver = 0x03; - ssl->minor_ver = 0x04; + ssl->major_ver = MBEDTLS_SSL_MAJOR_VERSION_3; + ssl->minor_ver = MBEDTLS_SSL_MINOR_VERSION_4; /* store server-provided random values */ - memcpy( ssl->handshake->randbytes + 32, buf, 32 ); - MBEDTLS_SSL_DEBUG_BUF( 3, "hello retry request, random bytes", buf + 2, 32 ); + memcpy( ssl->handshake->randbytes + MBEDTLS_SERVER_HELLO_RANDOM_LEN, + p, MBEDTLS_SERVER_HELLO_RANDOM_LEN ); + MBEDTLS_SSL_DEBUG_BUF( 3, "hello retry request, random bytes", + p + 2, MBEDTLS_SERVER_HELLO_RANDOM_LEN ); /* skip random bytes */ - buf += 32; + p += MBEDTLS_SERVER_HELLO_RANDOM_LEN; - if( ssl_tls13_check_server_hello_session_id_echo( ssl, &buf, msg_end ) != 0 ) + /* ... + * opaque legacy_session_id_echo<0..32>; + * ... + */ + if( ssl_tls13_check_server_hello_session_id_echo( ssl, &p, end ) != 0 ) { MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_HANDSHAKE_FAILURE, - MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE ); + MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE ); return( MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE ); } /* read server-selected ciphersuite, which follows random bytes */ - i = ( buf[0] << 8 ) | buf[1]; + cipher_suite = MBEDTLS_GET_UINT16_BE( p, 0 ); /* skip ciphersuite */ - buf += 2; + p += 2; - /* TBD: Check whether we have offered this ciphersuite */ - /* Via the force_ciphersuite version we may have instructed the client */ - /* to use a difference ciphersuite. */ + /* + * Check whether we have offered this ciphersuite + * Via the force_ciphersuite version we may have instructed the client + * to use a difference ciphersuite. + */ + ciphersuite_info = mbedtls_ssl_ciphersuite_from_id( cipher_suite ); + if( ciphersuite_info == NULL || + ssl_tls13_cipher_suite_is_offered( ssl, cipher_suite ) == 0 ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "ciphersuite(%04x) not found or not offered", + (unsigned int)cipher_suite ) ); + + MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_ILLEGAL_PARAMETER, + MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER ); + return( MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER ); + } /* Configure ciphersuites */ - ssl->handshake->ciphersuite_info = mbedtls_ssl_ciphersuite_from_id( i ); - - if( ssl->handshake->ciphersuite_info == NULL ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, - ( "ciphersuite info for %04x not found", (unsigned int)i ) ); - MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_INTERNAL_ERROR, - MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); - return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); - } - - mbedtls_ssl_optimize_checksum( ssl, ssl->handshake->ciphersuite_info ); - - ssl->session_negotiate->ciphersuite = i; - - suite_info = mbedtls_ssl_ciphersuite_from_id( - ssl->session_negotiate->ciphersuite ); - if( suite_info == NULL ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad hello retry request message" ) ); - MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_HANDSHAKE_FAILURE, - MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE ); - return( MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE ); - } + mbedtls_ssl_optimize_checksum( ssl, ciphersuite_info ); + ssl->handshake->ciphersuite_info = ciphersuite_info; + ssl->session_negotiate->ciphersuite = cipher_suite; MBEDTLS_SSL_DEBUG_MSG( 3, ( "hello retry request, chosen ciphersuite: ( %04x ) - %s", - (unsigned int)i, suite_info->name ) ); + (unsigned int)cipher_suite, ciphersuite_info->name ) ); #if defined(MBEDTLS_HAVE_TIME) ssl->session_negotiate->start = time( NULL ); #endif /* MBEDTLS_HAVE_TIME */ - i = 0; - while ( 1 ) - { - if( ssl->conf->ciphersuite_list[i] == 0 ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad hello retry request message" ) ); - MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_HANDSHAKE_FAILURE, - MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE ); - return( MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE ); - } - - if( ssl->conf->ciphersuite_list[i++] == - ssl->session_negotiate->ciphersuite ) - { - break; - } - } - /* Ensure that compression method is set to zero */ - if( buf[0] != 0 ) + if( p[0] != 0 ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad hello retry request message" ) ); MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_ILLEGAL_PARAMETER, @@ -1401,80 +1372,51 @@ static int ssl_hrr_parse( mbedtls_ssl_context *ssl, } /* skip compression */ - buf++; + p++; /* Are we reading beyond the message buffer? */ - if( ( buf + 2 ) > msg_end ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad hello retry request message" ) ); - MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR, - MBEDTLS_ERR_SSL_DECODE_ERROR ); - return( MBEDTLS_ERR_SSL_DECODE_ERROR ); - } + MBEDTLS_SSL_CHK_BUF_READ_PTR( p, end, 2 ); - ext_len = ( ( buf[0] << 8 ) | ( buf[1] ) ); - buf += 2; /* skip extension length */ + extensions_len = MBEDTLS_GET_UINT16_BE( p, 0 ); + p += 2; /* skip extension length */ /* Are we reading beyond the message buffer? */ - if( ( buf + ext_len ) > msg_end ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad hello retry request message" ) ); - MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR, - MBEDTLS_ERR_SSL_DECODE_ERROR ); - return( MBEDTLS_ERR_SSL_DECODE_ERROR ); - } - - ext = buf; + MBEDTLS_SSL_CHK_BUF_READ_PTR( p, end, extensions_len ); + extensions_end = p + extensions_len; MBEDTLS_SSL_DEBUG_MSG( 3, ( "hello retry request, total extension length: %" - MBEDTLS_PRINTF_SIZET , ext_len ) ); + MBEDTLS_PRINTF_SIZET , extensions_len ) ); + MBEDTLS_SSL_DEBUG_BUF( 3, "extensions", p, extensions_len ); - MBEDTLS_SSL_DEBUG_BUF( 3, "extensions", ext, ext_len ); - - while ( ext_len ) + while ( p < extensions_end ) { - ext_id = ( ( ext[0] << 8 ) | ( ext[1] ) ); - ext_size = ( ( ext[2] << 8 ) | ( ext[3] ) ); + unsigned int extension_type; + const unsigned char *extensions_data_end; + unsigned int extension_data_len; /* size of an individual extension */ - if( ext_size + 4 > ext_len ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad hello retry request message" ) ); - MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR, - MBEDTLS_ERR_SSL_DECODE_ERROR ); - return( MBEDTLS_ERR_SSL_DECODE_ERROR ); - } + MBEDTLS_SSL_CHK_BUF_READ_PTR( p, extensions_end, 4 ); + extension_type = MBEDTLS_GET_UINT16_BE( p, 0 ); + extension_data_len = MBEDTLS_GET_UINT16_BE( p + 2, 0 ); - switch( ext_id ) + p += 4; + MBEDTLS_SSL_CHK_BUF_READ_PTR( p, extensions_end, extension_data_len ); + extensions_data_end = p + extension_data_len; + + switch( extension_type ) { #if defined(MBEDTLS_SSL_COOKIE_C) case MBEDTLS_TLS_EXT_COOKIE: /* Retrieve length field of cookie */ - if( ext_size >= 2 ) - { - cookie = (unsigned char *) ( ext + 4 ); - cookie_len = ( cookie[0] << 8 ) | cookie[1]; - cookie += 2; - } - else - { - MBEDTLS_SSL_DEBUG_MSG( 1, - ( "bad HRR message - cookie length mismatch" ) ); - return( MBEDTLS_ERR_SSL_DECODE_ERROR ); - } - - if( ( cookie_len + 2 ) != ext_size ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, - ( "bad HRR message - cookie length mismatch" ) ); - return( MBEDTLS_ERR_SSL_DECODE_ERROR ); - } + MBEDTLS_SSL_CHK_BUF_READ_PTR( p, extensions_data_end, 2 ); + cookie_len = MBEDTLS_GET_UINT16_BE( p, 0 ); + cookie = (unsigned char *) ( p + 2 ); + MBEDTLS_SSL_CHK_BUF_READ_PTR( p, extensions_data_end, cookie_len + 2 ); MBEDTLS_SSL_DEBUG_BUF( 3, "cookie extension", cookie, cookie_len ); mbedtls_free( ssl->handshake->verify_cookie ); - ssl->handshake->verify_cookie = mbedtls_calloc( 1, cookie_len ); if( ssl->handshake->verify_cookie == NULL ) { @@ -1493,8 +1435,8 @@ static int ssl_hrr_parse( mbedtls_ssl_context *ssl, MBEDTLS_SSL_DEBUG_MSG( 3, ( "found supported_versions extension" ) ); ret = ssl_tls13_parse_supported_versions_ext( ssl, - ext + 4, - ext + 4 + ext_size ); + p, + p + extension_data_len ); if( ret != 0 ) return( ret ); break; @@ -1508,10 +1450,10 @@ static int ssl_hrr_parse( mbedtls_ssl_context *ssl, int tls_id; int found = 0; - MBEDTLS_SSL_DEBUG_BUF( 3, "key_share extension", ext + 4, ext_size ); + MBEDTLS_SSL_DEBUG_BUF( 3, "key_share extension", p, extension_data_len ); /* Read selected_group */ - tls_id = ( ( ext[4] << 8 ) | ( ext[5] ) ); + tls_id = MBEDTLS_GET_UINT16_BE( p, 0 ); MBEDTLS_SSL_DEBUG_MSG( 3, ( "selected_group ( %d )", tls_id ) ); /* Upon receipt of this extension in a HelloRetryRequest, the client @@ -1557,19 +1499,13 @@ static int ssl_hrr_parse( mbedtls_ssl_context *ssl, default: MBEDTLS_SSL_DEBUG_MSG( 3, ( "unknown extension found: %u ( ignoring )", - ext_id ) ); + extension_type ) ); } /* Jump to next extension */ - ext_len -= 4 + ext_size; - ext += 4 + ext_size; - - if( ext_len > 0 && ext_len < 4 ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad hello retry request message" ) ); - return( MBEDTLS_ERR_SSL_DECODE_ERROR ); - } - + //extensions_len -= 4 + extension_data_len; + //ext += 4 + extension_data_len; + p += extension_data_len; } return( 0 ); @@ -1592,7 +1528,8 @@ static int ssl_hrr_postprocess( mbedtls_ssl_context *ssl ) #if defined(MBEDTLS_SSL_TLS1_3_COMPATIBILITY_MODE) /* If not offering early data, the client sends a dummy CCS record * immediately before its second flight. This may either be before - * its second ClientHello or before its encrypted handshake flight. */ + * its second ClientHello or before its encrypted handshake flight. + */ mbedtls_ssl_handshake_set_state( ssl, MBEDTLS_SSL_CLIENT_CCS_BEFORE_2ND_CLIENT_HELLO ); #else @@ -1606,7 +1543,8 @@ static int ssl_hrr_postprocess( mbedtls_ssl_context *ssl ) * Currently, we're always resetting the key share, even if the server * was fine with it. Once we have separated key share generation from * key share writing, we can confine this to the case where the server - * requested a different share. */ + * requested a different share. + */ ret = ssl_reset_key_share( ssl ); if( ret != 0 ) return( ret ); @@ -1652,11 +1590,12 @@ static int ssl_tls13_process_server_hello( mbedtls_ssl_context *ssl ) } else if( ret == SSL_SERVER_HELLO_COORDINATE_HRR ) { - MBEDTLS_SSL_PROC_CHK( ssl_hrr_parse( ssl, buf, buf_len ) ); + MBEDTLS_SSL_PROC_CHK( ssl_hrr_parse( ssl, buf, buf + buf_len ) ); MBEDTLS_SSL_PROC_CHK( mbedtls_ssl_reset_transcript_for_hrr( ssl ) ); - mbedtls_ssl_tls13_add_hs_msg_to_checksum( ssl, MBEDTLS_SSL_HS_SERVER_HELLO, - buf, buf_len ); + mbedtls_ssl_tls13_add_hs_msg_to_checksum( ssl, + MBEDTLS_SSL_HS_SERVER_HELLO, + buf, buf_len ); MBEDTLS_SSL_PROC_CHK( ssl_hrr_postprocess( ssl ) ); } From 0b56a8f85c8558f8ea5a0af5b74c216d9bbc0be7 Mon Sep 17 00:00:00 2001 From: XiaokangQian Date: Wed, 22 Dec 2021 02:39:32 +0000 Subject: [PATCH 03/24] Replace curve_list with group_list and add update test scripts Signed-off-by: XiaokangQian --- library/ssl_tls13_client.c | 29 +++++++++++++++++++++-------- library/ssl_tls13_generic.c | 29 +++++++++++++++++++++++++++++ tests/ssl-opt.sh | 8 ++++---- 3 files changed, 54 insertions(+), 12 deletions(-) diff --git a/library/ssl_tls13_client.c b/library/ssl_tls13_client.c index 18b9074e6..fbdb671ca 100644 --- a/library/ssl_tls13_client.c +++ b/library/ssl_tls13_client.c @@ -135,6 +135,7 @@ static int ssl_tls13_parse_supported_versions_ext( mbedtls_ssl_context *ssl, * } KeyShare; */ +#if defined(MBEDTLS_ECDH_C) static int ssl_reset_ecdhe_share( mbedtls_ssl_context *ssl ) { mbedtls_ecdh_free( &ssl->handshake->ecdh_ctx ); @@ -156,6 +157,13 @@ static int ssl_reset_key_share( mbedtls_ssl_context *ssl ) return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); } +#else +static int ssl_reset_key_share( mbedtls_ssl_context *ssl ) +{ + ((void) ssl); + return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); +} +#endif /* MBEDTLS_ECDH_C */ /* * Functions for writing key_share extension. @@ -1445,7 +1453,7 @@ static int ssl_hrr_parse( mbedtls_ssl_context *ssl, case MBEDTLS_TLS_EXT_KEY_SHARE: { /* Variables for parsing the key_share */ - const mbedtls_ecp_group_id* grp_id; + const uint16_t* grp_id; const mbedtls_ecp_curve_info *curve_info = NULL; int tls_id; int found = 0; @@ -1460,13 +1468,14 @@ static int ssl_hrr_parse( mbedtls_ssl_context *ssl, * MUST first verify that the selected_group field corresponds to a * group which was provided in the "supported_groups" extension in the * original ClientHello. - * The supported_group was based on the info in ssl->conf->curve_list. + * The supported_group was based on the info in ssl->conf->group_list. * * If the server provided a key share that was not sent in the ClientHello - * then the client MUST abort the handshake with an "illegal_parameter" alert. */ - for( grp_id = ssl->conf->curve_list; *grp_id != MBEDTLS_ECP_DP_NONE; grp_id++ ) + * then the client MUST abort the handshake with an "illegal_parameter" alert. + */ + for( grp_id = ssl->conf->group_list; *grp_id != MBEDTLS_ECP_DP_NONE; grp_id++ ) { - curve_info = mbedtls_ecp_curve_info_from_grp_id( *grp_id ); + curve_info = mbedtls_ecp_curve_info_from_tls_id( *grp_id ); if( curve_info == NULL || curve_info->tls_id != tls_id ) continue; @@ -1480,7 +1489,8 @@ static int ssl_hrr_parse( mbedtls_ssl_context *ssl, * extension in the original ClientHello. If the server sent an * HRR message with a key share already provided in the * ClientHello then the client MUST abort the handshake with - * an "illegal_parameter" alert. */ + * an "illegal_parameter" alert. + */ if( found == 0 || tls_id == ssl->handshake->offered_group_id ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "Invalid key share in HRR" ) ); @@ -1513,7 +1523,9 @@ static int ssl_hrr_parse( mbedtls_ssl_context *ssl, static int ssl_hrr_postprocess( mbedtls_ssl_context *ssl ) { +#if defined(MBEDTLS_KEY_EXCHANGE_WITH_CERT_ENABLED) int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; +#endif /* MBEDTLS_KEY_EXCHANGE_WITH_CERT_ENABLED */ if( ssl->handshake->hello_retry_requests_received > 0 ) { @@ -1545,9 +1557,11 @@ static int ssl_hrr_postprocess( mbedtls_ssl_context *ssl ) * key share writing, we can confine this to the case where the server * requested a different share. */ +#if defined(MBEDTLS_KEY_EXCHANGE_WITH_CERT_ENABLED) ret = ssl_reset_key_share( ssl ); if( ret != 0 ) return( ret ); +#endif /* MBEDTLS_KEY_EXCHANGE_WITH_CERT_ENABLED */ return( 0 ); } @@ -1840,8 +1854,6 @@ static int ssl_tls13_write_change_cipher_spec( mbedtls_ssl_context *ssl ) if( ret != 0 ) return( ret ); - mbedtls_ssl_handshake_set_state( ssl, MBEDTLS_SSL_CLIENT_FINISHED ); - return( 0 ); } #endif /* MBEDTLS_SSL_TLS1_3_COMPATIBILITY_MODE */ @@ -1952,6 +1964,7 @@ int mbedtls_ssl_tls13_handshake_client_step( mbedtls_ssl_context *ssl ) */ #if defined(MBEDTLS_SSL_TLS1_3_COMPATIBILITY_MODE) case MBEDTLS_SSL_CLIENT_CCS_AFTER_SERVER_FINISHED: + case MBEDTLS_SSL_CLIENT_CCS_BEFORE_2ND_CLIENT_HELLO: ret = ssl_tls13_write_change_cipher_spec( ssl ); break; #endif /* MBEDTLS_SSL_TLS1_3_COMPATIBILITY_MODE */ diff --git a/library/ssl_tls13_generic.c b/library/ssl_tls13_generic.c index a87af94dc..9e6c52a4e 100644 --- a/library/ssl_tls13_generic.c +++ b/library/ssl_tls13_generic.c @@ -1058,6 +1058,32 @@ void mbedtls_ssl_tls13_handshake_wrapup( mbedtls_ssl_context *ssl ) */ #if defined(MBEDTLS_SSL_TLS1_3_COMPATIBILITY_MODE) +static int ssl_write_change_cipher_spec_postprocess( mbedtls_ssl_context* ssl ) +{ + +#if defined(MBEDTLS_SSL_CLI_C) + if( ssl->conf->endpoint == MBEDTLS_SSL_IS_CLIENT ) + { + switch( ssl->state ) + { + case MBEDTLS_SSL_CLIENT_CCS_BEFORE_2ND_CLIENT_HELLO: + mbedtls_ssl_handshake_set_state( ssl, MBEDTLS_SSL_CLIENT_HELLO ); + break; + case MBEDTLS_SSL_CLIENT_CCS_AFTER_SERVER_FINISHED: + mbedtls_ssl_handshake_set_state( ssl, MBEDTLS_SSL_CLIENT_FINISHED ); + break; + default: + MBEDTLS_SSL_DEBUG_MSG( 1, ( "should never happen" ) ); + return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); + } + } +#else + ((void) ssl); +#endif /* MBEDTLS_SSL_CLI_C */ + + return( 0 ); +} + static int ssl_tls13_write_change_cipher_spec_body( mbedtls_ssl_context *ssl, unsigned char *buf, unsigned char *end, @@ -1088,6 +1114,9 @@ int mbedtls_ssl_tls13_write_change_cipher_spec( mbedtls_ssl_context *ssl ) ssl->out_msgtype = MBEDTLS_SSL_MSG_CHANGE_CIPHER_SPEC; + /* Update state */ + MBEDTLS_SSL_PROC_CHK( ssl_write_change_cipher_spec_postprocess( ssl ) ); + /* Dispatch message */ MBEDTLS_SSL_PROC_CHK( mbedtls_ssl_write_record( ssl, 1 ) ); diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index 7a1436fab..7435511f2 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -9219,8 +9219,8 @@ run_test "TLS 1.3: HelloRetryRequest check - openssl" \ "$P_CLI debug_level=4 force_version=tls13" \ 1 \ -c "received HelloRetryRequest message" \ - -c "HRR not supported" \ - -c "Last error was: -0x6E00 - SSL - The handshake negotiation failed" + -c "tls13 client state: MBEDTLS_SSL_CLIENT_HELLO(1)" \ + -c "Last error was: -0x7180 - SSL - Verification of the message MAC failed" requires_gnutls_tls1_3 requires_gnutls_next_no_ticket @@ -9234,8 +9234,8 @@ run_test "TLS 1.3: HelloRetryRequest check - gnutls" \ "$P_CLI debug_level=4 force_version=tls13" \ 1 \ -c "received HelloRetryRequest message" \ - -c "HRR not supported" \ - -c "Last error was: -0x6E00 - SSL - The handshake negotiation failed" \ + -c "tls13 client state: MBEDTLS_SSL_CLIENT_HELLO(1)" \ + -c "Last error was: -0x7180 - SSL - Verification of the message MAC failed" \ -s "HELLO RETRY REQUEST was queued" for i in $(ls opt-testcases/*.sh) From b851da8a44d526331de6e0eaac47483f84d11705 Mon Sep 17 00:00:00 2001 From: XiaokangQian Date: Fri, 14 Jan 2022 04:03:11 +0000 Subject: [PATCH 04/24] Re-construct the code to merge hello and hrr based on comments Signed-off-by: XiaokangQian --- library/ssl_tls13_client.c | 401 +++++++++++-------------------------- 1 file changed, 118 insertions(+), 283 deletions(-) diff --git a/library/ssl_tls13_client.c b/library/ssl_tls13_client.c index fbdb671ca..f3126d27a 100644 --- a/library/ssl_tls13_client.c +++ b/library/ssl_tls13_client.c @@ -421,6 +421,69 @@ static int ssl_tls13_read_public_ecdhe_share( mbedtls_ssl_context *ssl, } #endif /* MBEDTLS_ECDH_C */ +static int ssl_tls13_hrr_check_key_share_ext( mbedtls_ssl_context *ssl, + const unsigned char *buf, + const unsigned char *end ) +{ + /* Variables for parsing the key_share */ + const uint16_t* grp_id; + const mbedtls_ecp_curve_info *curve_info = NULL; + const unsigned char *p = buf; + int tls_id; + int found = 0; + + const uint16_t *group_list = mbedtls_ssl_get_groups( ssl ); + if( group_list == NULL ) + return( MBEDTLS_ERR_SSL_BAD_CONFIG ); + + MBEDTLS_SSL_DEBUG_BUF( 3, "key_share extension", p, end - buf ); + + /* Read selected_group */ + tls_id = MBEDTLS_GET_UINT16_BE( p, 0 ); + MBEDTLS_SSL_DEBUG_MSG( 3, ( "selected_group ( %d )", tls_id ) ); + + /* Upon receipt of this extension in a HelloRetryRequest, the client + * MUST first verify that the selected_group field corresponds to a + * group which was provided in the "supported_groups" extension in the + * original ClientHello. + * The supported_group was based on the info in ssl->conf->group_list. + * + * If the server provided a key share that was not sent in the ClientHello + * then the client MUST abort the handshake with an "illegal_parameter" alert. + */ + for ( ; *group_list != 0; group_list++ ) + { + curve_info = mbedtls_ecp_curve_info_from_tls_id( *group_list ); + if( curve_info == NULL || curve_info->tls_id != tls_id ) + continue; + + /* We found a match */ + found = 1; + break; + } + + /* Client MUST verify that the selected_group field does not + * correspond to a group which was provided in the "key_share" + * extension in the original ClientHello. If the server sent an + * HRR message with a key share already provided in the + * ClientHello then the client MUST abort the handshake with + * an "illegal_parameter" alert. + */ + if( found == 0 || tls_id == ssl->handshake->offered_group_id ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "Invalid key share in HRR" ) ); + MBEDTLS_SSL_PEND_FATAL_ALERT( + MBEDTLS_SSL_ALERT_MSG_ILLEGAL_PARAMETER, + MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER ); + return( MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER ); + } + + /* Remember server's preference for next ClientHello */ + ssl->handshake->offered_group_id= tls_id; + + return( 0 ); +} + /* * ssl_tls13_parse_key_share_ext() * Parse key_share extension in Server Hello @@ -943,7 +1006,8 @@ static int ssl_tls13_cipher_suite_is_offered( mbedtls_ssl_context *ssl, */ static int ssl_tls13_parse_server_hello( mbedtls_ssl_context *ssl, const unsigned char *buf, - const unsigned char *end ) + const unsigned char *end, + int hrr ) { int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; const unsigned char *p = buf; @@ -951,6 +1015,10 @@ static int ssl_tls13_parse_server_hello( mbedtls_ssl_context *ssl, const unsigned char *extensions_end; uint16_t cipher_suite; const mbedtls_ssl_ciphersuite_t *ciphersuite_info; +#if defined(MBEDTLS_SSL_COOKIE_C) + size_t cookie_len; + unsigned char *cookie; +#endif /* MBEDTLS_SSL_COOKIE_C */ /* * Check there is space for minimal fields @@ -1093,6 +1161,32 @@ static int ssl_tls13_parse_server_hello( mbedtls_ssl_context *ssl, switch( extension_type ) { +#if defined(MBEDTLS_SSL_COOKIE_C) + case MBEDTLS_TLS_EXT_COOKIE: + + /* Retrieve length field of cookie */ + MBEDTLS_SSL_CHK_BUF_READ_PTR( p, extensions_end, 2 ); + cookie_len = MBEDTLS_GET_UINT16_BE( p, 0 ); + cookie = (unsigned char *) ( p + 2 ); + + MBEDTLS_SSL_CHK_BUF_READ_PTR( p, extensions_end, cookie_len + 2 ); + MBEDTLS_SSL_DEBUG_BUF( 3, "cookie extension", cookie, cookie_len ); + + mbedtls_free( ssl->handshake->verify_cookie ); + ssl->handshake->verify_cookie = mbedtls_calloc( 1, cookie_len ); + if( ssl->handshake->verify_cookie == NULL ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, + ( "alloc failed ( %" MBEDTLS_PRINTF_SIZET " bytes )", + cookie_len ) ); + return( MBEDTLS_ERR_SSL_ALLOC_FAILED ); + } + + memcpy( ssl->handshake->verify_cookie, cookie, cookie_len ); + ssl->handshake->verify_cookie_len = (unsigned char) cookie_len; + break; +#endif /* MBEDTLS_SSL_COOKIE_C */ + case MBEDTLS_TLS_EXT_SUPPORTED_VERSIONS: MBEDTLS_SSL_DEBUG_MSG( 3, ( "found supported_versions extension" ) ); @@ -1116,8 +1210,13 @@ static int ssl_tls13_parse_server_hello( mbedtls_ssl_context *ssl, #if defined(MBEDTLS_KEY_EXCHANGE_WITH_CERT_ENABLED) case MBEDTLS_TLS_EXT_KEY_SHARE: MBEDTLS_SSL_DEBUG_MSG( 3, ( "found key_shares extension" ) ); - if( ( ret = ssl_tls13_parse_key_share_ext( ssl, - p, p + extension_data_len ) ) != 0 ) + if( hrr ) + ret = ssl_tls13_hrr_check_key_share_ext( ssl, + p, p + extension_data_len ); + else + ret = ssl_tls13_parse_key_share_ext( ssl, + p, p + extension_data_len ); + if( ret != 0 ) { MBEDTLS_SSL_DEBUG_RET( 1, "ssl_tls13_parse_key_share_ext", @@ -1259,268 +1358,6 @@ cleanup: return( ret ); } -static int ssl_hrr_parse( mbedtls_ssl_context *ssl, - const unsigned char *buf, - const unsigned char *end ) -{ - int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; - int cipher_suite; - - /* pointer to the end of the buffer for length checks */ - const unsigned char *p = buf; - const unsigned char *extensions_end; - size_t extensions_len; /* stores length of all extensions */ - - const mbedtls_ssl_ciphersuite_t* ciphersuite_info; /* pointer to ciphersuite */ - -#if defined(MBEDTLS_SSL_COOKIE_C) - size_t cookie_len; - unsigned char *cookie; -#endif /* MBEDTLS_SSL_COOKIE_C */ - - /* Check for minimal length - * struct { - * ProtocolVersion legacy_version = 0x0303; - * Random random; - * opaque legacy_session_id_echo<0..32>; - * CipherSuite cipher_suite; - * uint8 legacy_compression_method = 0; - * Extension extensions<6..2 ^ 16 - 1>; - * } ServerHello; - * - * 38 = 32 ( random bytes ) + 2 ( ciphersuite ) + 2 ( version ) + - * 1 ( legacy_compression_method ) + - * 1 ( minimum for legacy_session_id_echo ) - */ - MBEDTLS_SSL_CHK_BUF_READ_PTR( p, end, 38 ); - - MBEDTLS_SSL_DEBUG_BUF( 4, "hello retry request", p, end - p ); - - MBEDTLS_SSL_DEBUG_BUF( 3, "hello retry request, version", p, 2 ); - - /* The version field must contain 0x303 */ - if( MBEDTLS_GET_UINT16_BE( p, 0 ) != 0x303 ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "Unsupported version of TLS." ) ); - MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_PROTOCOL_VERSION, - MBEDTLS_ERR_SSL_BAD_PROTOCOL_VERSION ); - return( MBEDTLS_ERR_SSL_BAD_PROTOCOL_VERSION ); - } - - /* skip version */ - p += 2; - - /* Internally we use the correct 1.3 version */ - ssl->major_ver = MBEDTLS_SSL_MAJOR_VERSION_3; - ssl->minor_ver = MBEDTLS_SSL_MINOR_VERSION_4; - - /* store server-provided random values */ - memcpy( ssl->handshake->randbytes + MBEDTLS_SERVER_HELLO_RANDOM_LEN, - p, MBEDTLS_SERVER_HELLO_RANDOM_LEN ); - MBEDTLS_SSL_DEBUG_BUF( 3, "hello retry request, random bytes", - p + 2, MBEDTLS_SERVER_HELLO_RANDOM_LEN ); - - /* skip random bytes */ - p += MBEDTLS_SERVER_HELLO_RANDOM_LEN; - - /* ... - * opaque legacy_session_id_echo<0..32>; - * ... - */ - if( ssl_tls13_check_server_hello_session_id_echo( ssl, &p, end ) != 0 ) - { - MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_HANDSHAKE_FAILURE, - MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE ); - return( MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE ); - } - - /* read server-selected ciphersuite, which follows random bytes */ - cipher_suite = MBEDTLS_GET_UINT16_BE( p, 0 ); - - /* skip ciphersuite */ - p += 2; - - /* - * Check whether we have offered this ciphersuite - * Via the force_ciphersuite version we may have instructed the client - * to use a difference ciphersuite. - */ - ciphersuite_info = mbedtls_ssl_ciphersuite_from_id( cipher_suite ); - if( ciphersuite_info == NULL || - ssl_tls13_cipher_suite_is_offered( ssl, cipher_suite ) == 0 ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "ciphersuite(%04x) not found or not offered", - (unsigned int)cipher_suite ) ); - - MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_ILLEGAL_PARAMETER, - MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER ); - return( MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER ); - } - - /* Configure ciphersuites */ - mbedtls_ssl_optimize_checksum( ssl, ciphersuite_info ); - ssl->handshake->ciphersuite_info = ciphersuite_info; - ssl->session_negotiate->ciphersuite = cipher_suite; - - MBEDTLS_SSL_DEBUG_MSG( 3, - ( "hello retry request, chosen ciphersuite: ( %04x ) - %s", - (unsigned int)cipher_suite, ciphersuite_info->name ) ); - -#if defined(MBEDTLS_HAVE_TIME) - ssl->session_negotiate->start = time( NULL ); -#endif /* MBEDTLS_HAVE_TIME */ - - /* Ensure that compression method is set to zero */ - if( p[0] != 0 ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad hello retry request message" ) ); - MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_ILLEGAL_PARAMETER, - MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER ); - return( MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER ); - } - - /* skip compression */ - p++; - - /* Are we reading beyond the message buffer? */ - MBEDTLS_SSL_CHK_BUF_READ_PTR( p, end, 2 ); - - extensions_len = MBEDTLS_GET_UINT16_BE( p, 0 ); - p += 2; /* skip extension length */ - - /* Are we reading beyond the message buffer? */ - MBEDTLS_SSL_CHK_BUF_READ_PTR( p, end, extensions_len ); - extensions_end = p + extensions_len; - - MBEDTLS_SSL_DEBUG_MSG( 3, - ( "hello retry request, total extension length: %" - MBEDTLS_PRINTF_SIZET , extensions_len ) ); - MBEDTLS_SSL_DEBUG_BUF( 3, "extensions", p, extensions_len ); - - while ( p < extensions_end ) - { - unsigned int extension_type; - const unsigned char *extensions_data_end; - unsigned int extension_data_len; /* size of an individual extension */ - - MBEDTLS_SSL_CHK_BUF_READ_PTR( p, extensions_end, 4 ); - extension_type = MBEDTLS_GET_UINT16_BE( p, 0 ); - extension_data_len = MBEDTLS_GET_UINT16_BE( p + 2, 0 ); - - p += 4; - MBEDTLS_SSL_CHK_BUF_READ_PTR( p, extensions_end, extension_data_len ); - extensions_data_end = p + extension_data_len; - - switch( extension_type ) - { -#if defined(MBEDTLS_SSL_COOKIE_C) - case MBEDTLS_TLS_EXT_COOKIE: - - /* Retrieve length field of cookie */ - MBEDTLS_SSL_CHK_BUF_READ_PTR( p, extensions_data_end, 2 ); - cookie_len = MBEDTLS_GET_UINT16_BE( p, 0 ); - cookie = (unsigned char *) ( p + 2 ); - - MBEDTLS_SSL_CHK_BUF_READ_PTR( p, extensions_data_end, cookie_len + 2 ); - MBEDTLS_SSL_DEBUG_BUF( 3, "cookie extension", cookie, cookie_len ); - - mbedtls_free( ssl->handshake->verify_cookie ); - ssl->handshake->verify_cookie = mbedtls_calloc( 1, cookie_len ); - if( ssl->handshake->verify_cookie == NULL ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, - ( "alloc failed ( %" MBEDTLS_PRINTF_SIZET " bytes )", - cookie_len ) ); - return( MBEDTLS_ERR_SSL_ALLOC_FAILED ); - } - - memcpy( ssl->handshake->verify_cookie, cookie, cookie_len ); - ssl->handshake->verify_cookie_len = (unsigned char) cookie_len; - break; -#endif /* MBEDTLS_SSL_COOKIE_C */ - - case MBEDTLS_TLS_EXT_SUPPORTED_VERSIONS: - MBEDTLS_SSL_DEBUG_MSG( 3, ( "found supported_versions extension" ) ); - - ret = ssl_tls13_parse_supported_versions_ext( ssl, - p, - p + extension_data_len ); - if( ret != 0 ) - return( ret ); - break; - -#if defined(MBEDTLS_ECDH_C) || defined(MBEDTLS_ECDSA_C) - case MBEDTLS_TLS_EXT_KEY_SHARE: - { - /* Variables for parsing the key_share */ - const uint16_t* grp_id; - const mbedtls_ecp_curve_info *curve_info = NULL; - int tls_id; - int found = 0; - - MBEDTLS_SSL_DEBUG_BUF( 3, "key_share extension", p, extension_data_len ); - - /* Read selected_group */ - tls_id = MBEDTLS_GET_UINT16_BE( p, 0 ); - MBEDTLS_SSL_DEBUG_MSG( 3, ( "selected_group ( %d )", tls_id ) ); - - /* Upon receipt of this extension in a HelloRetryRequest, the client - * MUST first verify that the selected_group field corresponds to a - * group which was provided in the "supported_groups" extension in the - * original ClientHello. - * The supported_group was based on the info in ssl->conf->group_list. - * - * If the server provided a key share that was not sent in the ClientHello - * then the client MUST abort the handshake with an "illegal_parameter" alert. - */ - for( grp_id = ssl->conf->group_list; *grp_id != MBEDTLS_ECP_DP_NONE; grp_id++ ) - { - curve_info = mbedtls_ecp_curve_info_from_tls_id( *grp_id ); - if( curve_info == NULL || curve_info->tls_id != tls_id ) - continue; - - /* We found a match */ - found = 1; - break; - } - - /* Client MUST verify that the selected_group field does not - * correspond to a group which was provided in the "key_share" - * extension in the original ClientHello. If the server sent an - * HRR message with a key share already provided in the - * ClientHello then the client MUST abort the handshake with - * an "illegal_parameter" alert. - */ - if( found == 0 || tls_id == ssl->handshake->offered_group_id ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "Invalid key share in HRR" ) ); - MBEDTLS_SSL_PEND_FATAL_ALERT( - MBEDTLS_SSL_ALERT_MSG_ILLEGAL_PARAMETER, - MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER ); - return( MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER ); - } - - /* Remember server's preference for next ClientHello */ - ssl->handshake->offered_group_id= tls_id; - break; - } - -#endif /* MBEDTLS_ECDH_C || MBEDTLS_ECDSA_C */ - default: - MBEDTLS_SSL_DEBUG_MSG( 3, - ( "unknown extension found: %u ( ignoring )", - extension_type ) ); - } - - /* Jump to next extension */ - //extensions_len -= 4 + extension_data_len; - //ext += 4 + extension_data_len; - p += extension_data_len; - } - - return( 0 ); -} - static int ssl_hrr_postprocess( mbedtls_ssl_context *ssl ) { #if defined(MBEDTLS_KEY_EXCHANGE_WITH_CERT_ENABLED) @@ -1575,6 +1412,7 @@ static int ssl_tls13_process_server_hello( mbedtls_ssl_context *ssl ) int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; unsigned char *buf = NULL; size_t buf_len = 0; + int hrr = -1; MBEDTLS_SSL_DEBUG_MSG( 2, ( "=> %s", __func__ ) ); @@ -1586,31 +1424,28 @@ static int ssl_tls13_process_server_hello( mbedtls_ssl_context *ssl ) ssl->major_ver = MBEDTLS_SSL_MAJOR_VERSION_3; ssl->handshake->extensions_present = MBEDTLS_SSL_EXT_NONE; - ret = ssl_tls13_server_hello_coordinate( ssl, &buf, &buf_len ); + hrr = ssl_tls13_server_hello_coordinate( ssl, &buf, &buf_len ); /* Parsing step * We know what message to expect by now and call * the respective parsing function. */ - if( ret == SSL_SERVER_HELLO_COORDINATE_HELLO ) - { - MBEDTLS_SSL_PROC_CHK( ssl_tls13_parse_server_hello( ssl, buf, - buf + buf_len ) ); - - mbedtls_ssl_tls13_add_hs_msg_to_checksum( ssl, - MBEDTLS_SSL_HS_SERVER_HELLO, - buf, buf_len ); - - MBEDTLS_SSL_PROC_CHK( ssl_tls13_finalize_server_hello( ssl ) ); - } - else if( ret == SSL_SERVER_HELLO_COORDINATE_HRR ) - { - MBEDTLS_SSL_PROC_CHK( ssl_hrr_parse( ssl, buf, buf + buf_len ) ); + MBEDTLS_SSL_DEBUG_MSG( 2, ( " hrr = %d ", hrr ) ); + MBEDTLS_SSL_PROC_CHK( ssl_tls13_parse_server_hello( ssl, buf, + buf + buf_len, + hrr ) ); + if( hrr == SSL_SERVER_HELLO_COORDINATE_HRR ) MBEDTLS_SSL_PROC_CHK( mbedtls_ssl_reset_transcript_for_hrr( ssl ) ); - mbedtls_ssl_tls13_add_hs_msg_to_checksum( ssl, - MBEDTLS_SSL_HS_SERVER_HELLO, - buf, buf_len ); + mbedtls_ssl_tls13_add_hs_msg_to_checksum( ssl, + MBEDTLS_SSL_HS_SERVER_HELLO, + buf, buf_len ); + if( hrr == SSL_SERVER_HELLO_COORDINATE_HELLO ) + { + MBEDTLS_SSL_PROC_CHK( ssl_tls13_finalize_server_hello( ssl ) ); + } + else if( hrr == SSL_SERVER_HELLO_COORDINATE_HRR ) + { MBEDTLS_SSL_PROC_CHK( ssl_hrr_postprocess( ssl ) ); } From 16acd4b3e45b039917a82b237700849c81781425 Mon Sep 17 00:00:00 2001 From: XiaokangQian Date: Fri, 14 Jan 2022 07:35:47 +0000 Subject: [PATCH 05/24] Reject the second HRR earlier and align naming styles Signed-off-by: XiaokangQian --- library/ssl_tls13_client.c | 44 ++++++++++++++++++++++---------------- 1 file changed, 26 insertions(+), 18 deletions(-) diff --git a/library/ssl_tls13_client.c b/library/ssl_tls13_client.c index f3126d27a..c54cb755d 100644 --- a/library/ssl_tls13_client.c +++ b/library/ssl_tls13_client.c @@ -142,7 +142,7 @@ static int ssl_reset_ecdhe_share( mbedtls_ssl_context *ssl ) return( 0 ); } -static int ssl_reset_key_share( mbedtls_ssl_context *ssl ) +static int ssl_tls13_reset_key_share( mbedtls_ssl_context *ssl ) { uint16_t group_id = ssl->handshake->offered_group_id; if( group_id == 0 ) @@ -158,7 +158,7 @@ static int ssl_reset_key_share( mbedtls_ssl_context *ssl ) return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); } #else -static int ssl_reset_key_share( mbedtls_ssl_context *ssl ) +static int ssl_tls13_reset_key_share( mbedtls_ssl_context *ssl ) { ((void) ssl); return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); @@ -426,7 +426,6 @@ static int ssl_tls13_hrr_check_key_share_ext( mbedtls_ssl_context *ssl, const unsigned char *end ) { /* Variables for parsing the key_share */ - const uint16_t* grp_id; const mbedtls_ecp_curve_info *curve_info = NULL; const unsigned char *p = buf; int tls_id; @@ -933,6 +932,21 @@ static int ssl_tls13_server_hello_coordinate( mbedtls_ssl_context *ssl, break; case SSL_SERVER_HELLO_COORDINATE_HRR: MBEDTLS_SSL_DEBUG_MSG( 2, ( "received HelloRetryRequest message" ) ); + /* If a client receives a second + * HelloRetryRequest in the same connection (i.e., where the ClientHello + * was itself in response to a HelloRetryRequest), it MUST abort the + * handshake with an "unexpected_message" alert. + */ + if( ssl->handshake->hello_retry_requests_received > 0 ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "Multiple HRRs received" ) ); + MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_UNEXPECTED_MESSAGE, + MBEDTLS_ERR_SSL_UNEXPECTED_MESSAGE ); + return( MBEDTLS_ERR_SSL_UNEXPECTED_MESSAGE ); + } + + ssl->handshake->hello_retry_requests_received++; + break; } @@ -1358,22 +1372,12 @@ cleanup: return( ret ); } -static int ssl_hrr_postprocess( mbedtls_ssl_context *ssl ) +static int ssl_tls13_finalize_hrr( mbedtls_ssl_context *ssl ) { #if defined(MBEDTLS_KEY_EXCHANGE_WITH_CERT_ENABLED) int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; #endif /* MBEDTLS_KEY_EXCHANGE_WITH_CERT_ENABLED */ - if( ssl->handshake->hello_retry_requests_received > 0 ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "Multiple HRRs received" ) ); - MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_LEVEL_FATAL, - MBEDTLS_SSL_ALERT_MSG_HANDSHAKE_FAILURE ); - return( MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE ); - } - - ssl->handshake->hello_retry_requests_received++; - #if defined(MBEDTLS_SSL_TLS1_3_COMPATIBILITY_MODE) /* If not offering early data, the client sends a dummy CCS record * immediately before its second flight. This may either be before @@ -1395,7 +1399,7 @@ static int ssl_hrr_postprocess( mbedtls_ssl_context *ssl ) * requested a different share. */ #if defined(MBEDTLS_KEY_EXCHANGE_WITH_CERT_ENABLED) - ret = ssl_reset_key_share( ssl ); + ret = ssl_tls13_reset_key_share( ssl ); if( ret != 0 ) return( ret ); #endif /* MBEDTLS_KEY_EXCHANGE_WITH_CERT_ENABLED */ @@ -1424,12 +1428,16 @@ static int ssl_tls13_process_server_hello( mbedtls_ssl_context *ssl ) ssl->major_ver = MBEDTLS_SSL_MAJOR_VERSION_3; ssl->handshake->extensions_present = MBEDTLS_SSL_EXT_NONE; - hrr = ssl_tls13_server_hello_coordinate( ssl, &buf, &buf_len ); + ret = ssl_tls13_server_hello_coordinate( ssl, &buf, &buf_len ); + if( ret != SSL_SERVER_HELLO_COORDINATE_HELLO && + ret != SSL_SERVER_HELLO_COORDINATE_HRR ) + goto cleanup; + else + hrr = ret; /* Parsing step * We know what message to expect by now and call * the respective parsing function. */ - MBEDTLS_SSL_DEBUG_MSG( 2, ( " hrr = %d ", hrr ) ); MBEDTLS_SSL_PROC_CHK( ssl_tls13_parse_server_hello( ssl, buf, buf + buf_len, hrr ) ); @@ -1446,7 +1454,7 @@ static int ssl_tls13_process_server_hello( mbedtls_ssl_context *ssl ) } else if( hrr == SSL_SERVER_HELLO_COORDINATE_HRR ) { - MBEDTLS_SSL_PROC_CHK( ssl_hrr_postprocess( ssl ) ); + MBEDTLS_SSL_PROC_CHK( ssl_tls13_finalize_hrr( ssl ) ); } cleanup: From b48894eca4a07a20a2806791765cf19471d80f8a Mon Sep 17 00:00:00 2001 From: XiaokangQian Date: Mon, 17 Jan 2022 02:05:52 +0000 Subject: [PATCH 06/24] Add buffer check for named group Signed-off-by: XiaokangQian --- library/ssl_tls13_client.c | 1 + 1 file changed, 1 insertion(+) diff --git a/library/ssl_tls13_client.c b/library/ssl_tls13_client.c index c54cb755d..3b2313aa3 100644 --- a/library/ssl_tls13_client.c +++ b/library/ssl_tls13_client.c @@ -438,6 +438,7 @@ static int ssl_tls13_hrr_check_key_share_ext( mbedtls_ssl_context *ssl, MBEDTLS_SSL_DEBUG_BUF( 3, "key_share extension", p, end - buf ); /* Read selected_group */ + MBEDTLS_SSL_CHK_BUF_READ_PTR( p, end, 2 ); tls_id = MBEDTLS_GET_UINT16_BE( p, 0 ); MBEDTLS_SSL_DEBUG_MSG( 3, ( "selected_group ( %d )", tls_id ) ); From 8945db36ab6ddc132e5ca44ca9cda8da8c9dbe11 Mon Sep 17 00:00:00 2001 From: XiaokangQian Date: Mon, 17 Jan 2022 05:38:29 +0000 Subject: [PATCH 07/24] Reduce paramter hrr from ssl_tls13_parse_server_hello Signed-off-by: XiaokangQian --- library/ssl_tls13_client.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/library/ssl_tls13_client.c b/library/ssl_tls13_client.c index 3b2313aa3..40748e97d 100644 --- a/library/ssl_tls13_client.c +++ b/library/ssl_tls13_client.c @@ -1021,8 +1021,7 @@ static int ssl_tls13_cipher_suite_is_offered( mbedtls_ssl_context *ssl, */ static int ssl_tls13_parse_server_hello( mbedtls_ssl_context *ssl, const unsigned char *buf, - const unsigned char *end, - int hrr ) + const unsigned char *end) { int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; const unsigned char *p = buf; @@ -1166,6 +1165,9 @@ static int ssl_tls13_parse_server_hello( mbedtls_ssl_context *ssl, { unsigned int extension_type; size_t extension_data_len; +#if defined(MBEDTLS_KEY_EXCHANGE_WITH_CERT_ENABLED) + int hrr; +#endif /* MBEDTLS_KEY_EXCHANGE_WITH_CERT_ENABLED */ MBEDTLS_SSL_CHK_BUF_READ_PTR( p, extensions_end, 4 ); extension_type = MBEDTLS_GET_UINT16_BE( p, 0 ); @@ -1224,13 +1226,16 @@ static int ssl_tls13_parse_server_hello( mbedtls_ssl_context *ssl, #if defined(MBEDTLS_KEY_EXCHANGE_WITH_CERT_ENABLED) case MBEDTLS_TLS_EXT_KEY_SHARE: + hrr = ssl_server_hello_is_hrr( ssl, buf, end ); MBEDTLS_SSL_DEBUG_MSG( 3, ( "found key_shares extension" ) ); - if( hrr ) + if( hrr == SSL_SERVER_HELLO_COORDINATE_HRR ) ret = ssl_tls13_hrr_check_key_share_ext( ssl, p, p + extension_data_len ); - else + else if( hrr == SSL_SERVER_HELLO_COORDINATE_HELLO ) ret = ssl_tls13_parse_key_share_ext( ssl, p, p + extension_data_len ); + else + ret = hrr; if( ret != 0 ) { MBEDTLS_SSL_DEBUG_RET( 1, @@ -1440,8 +1445,7 @@ static int ssl_tls13_process_server_hello( mbedtls_ssl_context *ssl ) * the respective parsing function. */ MBEDTLS_SSL_PROC_CHK( ssl_tls13_parse_server_hello( ssl, buf, - buf + buf_len, - hrr ) ); + buf + buf_len ) ); if( hrr == SSL_SERVER_HELLO_COORDINATE_HRR ) MBEDTLS_SSL_PROC_CHK( mbedtls_ssl_reset_transcript_for_hrr( ssl ) ); From d9e068e10b4b6aa7134af9667c035462e2cd2de5 Mon Sep 17 00:00:00 2001 From: XiaokangQian Date: Tue, 18 Jan 2022 06:23:32 +0000 Subject: [PATCH 08/24] Change code based on comments Align coding styles Add hrr parameter for ssl_tls13_parse_server_hello Add reset steps for SHA384 in HRR Signed-off-by: XiaokangQian --- library/ssl_misc.h | 2 +- library/ssl_tls.c | 18 ++++++++---- library/ssl_tls13_client.c | 57 +++++++++++++++++++------------------- 3 files changed, 43 insertions(+), 34 deletions(-) diff --git a/library/ssl_misc.h b/library/ssl_misc.h index b58b0af90..8bea627a2 100644 --- a/library/ssl_misc.h +++ b/library/ssl_misc.h @@ -593,7 +593,7 @@ struct mbedtls_ssl_handshake_params #if defined(MBEDTLS_SSL_CLI_C) /*!< Number of Hello Retry Request messages received from the server. */ - int hello_retry_requests_received; + int hello_retry_request_count; #endif /* MBEDTLS_SSL_CLI_C */ #if defined(MBEDTLS_SSL_PROTO_TLS1_2) && \ diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 290c75e5f..940b306fc 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -7630,9 +7630,9 @@ static int ssl_hash_transcript_core( mbedtls_ssl_context *ssl, return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); ret = mbedtls_ssl_get_handshake_transcript( ssl, md, - transcript + 4, - len - 4, - &hash_size ); + transcript + 4, + len - 4, + &hash_size ); if( ret != 0 ) { MBEDTLS_SSL_DEBUG_RET( 4, "mbedtls_ssl_get_handshake_transcript", ret ); @@ -7689,7 +7689,7 @@ int mbedtls_ssl_reset_transcript_for_hrr( mbedtls_ssl_context *ssl ) ssl_update_checksum_sha256( ssl, hash_transcript, hash_olen ); #endif /* MBEDTLS_SHA256_C */ -#if defined(MBEDTLS_SHA512_C) +#if defined(MBEDTLS_SHA384_C) ret = ssl_hash_transcript_core( ssl, MBEDTLS_MD_SHA384, hash_transcript, sizeof( hash_transcript ), @@ -7701,7 +7701,15 @@ int mbedtls_ssl_reset_transcript_for_hrr( mbedtls_ssl_context *ssl ) } MBEDTLS_SSL_DEBUG_BUF( 4, "Truncated SHA-384 handshake transcript", hash_transcript, hash_olen ); -#endif /* MBEDTLS_SHA512_C */ + +#if defined(MBEDTLS_USE_PSA_CRYPTO) + psa_hash_abort( &ssl->handshake->fin_sha384_psa ); + psa_hash_setup( &ssl->handshake->fin_sha384_psa, PSA_ALG_SHA_384 ); +#else + mbedtls_sha512_starts( &ssl->handshake->fin_sha512, 1 ); +#endif + ssl_update_checksum_sha384( ssl, hash_transcript, hash_olen ); +#endif /* MBEDTLS_SHA384_C */ return( ret ); } diff --git a/library/ssl_tls13_client.c b/library/ssl_tls13_client.c index 40748e97d..e259b4dc7 100644 --- a/library/ssl_tls13_client.c +++ b/library/ssl_tls13_client.c @@ -136,7 +136,7 @@ static int ssl_tls13_parse_supported_versions_ext( mbedtls_ssl_context *ssl, */ #if defined(MBEDTLS_ECDH_C) -static int ssl_reset_ecdhe_share( mbedtls_ssl_context *ssl ) +static int ssl_tls13_reset_ecdhe_share( mbedtls_ssl_context *ssl ) { mbedtls_ecdh_free( &ssl->handshake->ecdh_ctx ); return( 0 ); @@ -149,7 +149,7 @@ static int ssl_tls13_reset_key_share( mbedtls_ssl_context *ssl ) return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); if( mbedtls_ssl_tls13_named_group_is_ecdhe( group_id ) ) - return( ssl_reset_ecdhe_share( ssl ) ); + return( ssl_tls13_reset_ecdhe_share( ssl ) ); else if( 0 /* other KEMs? */ ) { /* Do something */ @@ -421,7 +421,7 @@ static int ssl_tls13_read_public_ecdhe_share( mbedtls_ssl_context *ssl, } #endif /* MBEDTLS_ECDH_C */ -static int ssl_tls13_hrr_check_key_share_ext( mbedtls_ssl_context *ssl, +static int ssl_tls13_parse_hrr_key_share_ext( mbedtls_ssl_context *ssl, const unsigned char *buf, const unsigned char *end ) { @@ -451,7 +451,7 @@ static int ssl_tls13_hrr_check_key_share_ext( mbedtls_ssl_context *ssl, * If the server provided a key share that was not sent in the ClientHello * then the client MUST abort the handshake with an "illegal_parameter" alert. */ - for ( ; *group_list != 0; group_list++ ) + for( ; *group_list != 0; group_list++ ) { curve_info = mbedtls_ecp_curve_info_from_tls_id( *group_list ); if( curve_info == NULL || curve_info->tls_id != tls_id ) @@ -938,7 +938,7 @@ static int ssl_tls13_server_hello_coordinate( mbedtls_ssl_context *ssl, * was itself in response to a HelloRetryRequest), it MUST abort the * handshake with an "unexpected_message" alert. */ - if( ssl->handshake->hello_retry_requests_received > 0 ) + if( ssl->handshake->hello_retry_request_count > 0 ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "Multiple HRRs received" ) ); MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_UNEXPECTED_MESSAGE, @@ -946,7 +946,7 @@ static int ssl_tls13_server_hello_coordinate( mbedtls_ssl_context *ssl, return( MBEDTLS_ERR_SSL_UNEXPECTED_MESSAGE ); } - ssl->handshake->hello_retry_requests_received++; + ssl->handshake->hello_retry_request_count++; break; } @@ -1021,7 +1021,8 @@ static int ssl_tls13_cipher_suite_is_offered( mbedtls_ssl_context *ssl, */ static int ssl_tls13_parse_server_hello( mbedtls_ssl_context *ssl, const unsigned char *buf, - const unsigned char *end) + const unsigned char *end, + int is_hrr ) { int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; const unsigned char *p = buf; @@ -1165,9 +1166,6 @@ static int ssl_tls13_parse_server_hello( mbedtls_ssl_context *ssl, { unsigned int extension_type; size_t extension_data_len; -#if defined(MBEDTLS_KEY_EXCHANGE_WITH_CERT_ENABLED) - int hrr; -#endif /* MBEDTLS_KEY_EXCHANGE_WITH_CERT_ENABLED */ MBEDTLS_SSL_CHK_BUF_READ_PTR( p, extensions_end, 4 ); extension_type = MBEDTLS_GET_UINT16_BE( p, 0 ); @@ -1181,6 +1179,14 @@ static int ssl_tls13_parse_server_hello( mbedtls_ssl_context *ssl, #if defined(MBEDTLS_SSL_COOKIE_C) case MBEDTLS_TLS_EXT_COOKIE: + if( !is_hrr ) + { + MBEDTLS_SSL_PEND_FATAL_ALERT( + MBEDTLS_SSL_ALERT_MSG_UNSUPPORTED_EXT, + MBEDTLS_ERR_SSL_UNSUPPORTED_EXTENSION ); + return( MBEDTLS_ERR_SSL_UNSUPPORTED_EXTENSION ); + } + /* Retrieve length field of cookie */ MBEDTLS_SSL_CHK_BUF_READ_PTR( p, extensions_end, 2 ); cookie_len = MBEDTLS_GET_UINT16_BE( p, 0 ); @@ -1226,16 +1232,15 @@ static int ssl_tls13_parse_server_hello( mbedtls_ssl_context *ssl, #if defined(MBEDTLS_KEY_EXCHANGE_WITH_CERT_ENABLED) case MBEDTLS_TLS_EXT_KEY_SHARE: - hrr = ssl_server_hello_is_hrr( ssl, buf, end ); MBEDTLS_SSL_DEBUG_MSG( 3, ( "found key_shares extension" ) ); - if( hrr == SSL_SERVER_HELLO_COORDINATE_HRR ) - ret = ssl_tls13_hrr_check_key_share_ext( ssl, + if( is_hrr == SSL_SERVER_HELLO_COORDINATE_HRR ) + ret = ssl_tls13_parse_hrr_key_share_ext( ssl, p, p + extension_data_len ); - else if( hrr == SSL_SERVER_HELLO_COORDINATE_HELLO ) + else if( is_hrr == SSL_SERVER_HELLO_COORDINATE_HELLO ) ret = ssl_tls13_parse_key_share_ext( ssl, p, p + extension_data_len ); else - ret = hrr; + ret = is_hrr; if( ret != 0 ) { MBEDTLS_SSL_DEBUG_RET( 1, @@ -1422,7 +1427,7 @@ static int ssl_tls13_process_server_hello( mbedtls_ssl_context *ssl ) int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; unsigned char *buf = NULL; size_t buf_len = 0; - int hrr = -1; + int is_hrr = -1; MBEDTLS_SSL_DEBUG_MSG( 2, ( "=> %s", __func__ ) ); @@ -1435,32 +1440,28 @@ static int ssl_tls13_process_server_hello( mbedtls_ssl_context *ssl ) ssl->handshake->extensions_present = MBEDTLS_SSL_EXT_NONE; ret = ssl_tls13_server_hello_coordinate( ssl, &buf, &buf_len ); - if( ret != SSL_SERVER_HELLO_COORDINATE_HELLO && - ret != SSL_SERVER_HELLO_COORDINATE_HRR ) + if( ret < 0 ) goto cleanup; else - hrr = ret; + is_hrr = ( ret == SSL_SERVER_HELLO_COORDINATE_HRR ); /* Parsing step * We know what message to expect by now and call * the respective parsing function. */ MBEDTLS_SSL_PROC_CHK( ssl_tls13_parse_server_hello( ssl, buf, - buf + buf_len ) ); - if( hrr == SSL_SERVER_HELLO_COORDINATE_HRR ) + buf + buf_len, + is_hrr ) ); + if( is_hrr ) MBEDTLS_SSL_PROC_CHK( mbedtls_ssl_reset_transcript_for_hrr( ssl ) ); mbedtls_ssl_tls13_add_hs_msg_to_checksum( ssl, MBEDTLS_SSL_HS_SERVER_HELLO, buf, buf_len ); - if( hrr == SSL_SERVER_HELLO_COORDINATE_HELLO ) - { - MBEDTLS_SSL_PROC_CHK( ssl_tls13_finalize_server_hello( ssl ) ); - } - else if( hrr == SSL_SERVER_HELLO_COORDINATE_HRR ) - { + if( is_hrr ) MBEDTLS_SSL_PROC_CHK( ssl_tls13_finalize_hrr( ssl ) ); - } + else + MBEDTLS_SSL_PROC_CHK( ssl_tls13_finalize_server_hello( ssl ) ); cleanup: MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= %s", __func__ ) ); From 6db08dd2cb893478e63bbbe389cec941f5ad3276 Mon Sep 17 00:00:00 2001 From: XiaokangQian Date: Tue, 18 Jan 2022 06:36:23 +0000 Subject: [PATCH 09/24] Change ssl-opt.sh to make hrr tests pass Signed-off-by: XiaokangQian --- tests/ssl-opt.sh | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index 7435511f2..37fa287cc 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -9217,10 +9217,10 @@ requires_openssl_tls1_3 run_test "TLS 1.3: HelloRetryRequest check - openssl" \ "$O_NEXT_SRV -ciphersuites TLS_AES_256_GCM_SHA384 -sigalgs ecdsa_secp256r1_sha256 -groups P-256 -msg -tls1_3 -num_tickets 0 -no_resume_ephemeral -no_cache" \ "$P_CLI debug_level=4 force_version=tls13" \ - 1 \ + 0 \ -c "received HelloRetryRequest message" \ -c "tls13 client state: MBEDTLS_SSL_CLIENT_HELLO(1)" \ - -c "Last error was: -0x7180 - SSL - Verification of the message MAC failed" + -c "HTTP/1.0 200 ok" requires_gnutls_tls1_3 requires_gnutls_next_no_ticket @@ -9235,7 +9235,7 @@ run_test "TLS 1.3: HelloRetryRequest check - gnutls" \ 1 \ -c "received HelloRetryRequest message" \ -c "tls13 client state: MBEDTLS_SSL_CLIENT_HELLO(1)" \ - -c "Last error was: -0x7180 - SSL - Verification of the message MAC failed" \ + -c "Last error was: -0x6E00 - SSL - The handshake negotiation failed" \ -s "HELLO RETRY REQUEST was queued" for i in $(ls opt-testcases/*.sh) From 53f20b71c58b4af878a3b02cf6801558d47149d7 Mon Sep 17 00:00:00 2001 From: XiaokangQian Date: Tue, 18 Jan 2022 10:47:33 +0000 Subject: [PATCH 10/24] Improve ssl_tls13_parse_server_hello Avoid coping random bytes in hrr Send illegal parameter alert when cipher suite mismatch Send illegal parameter alert when supported_version not exist Signed-off-by: XiaokangQian --- library/ssl_tls13_client.c | 40 ++++++++++++++++++++++++++++++-------- 1 file changed, 32 insertions(+), 8 deletions(-) diff --git a/library/ssl_tls13_client.c b/library/ssl_tls13_client.c index e259b4dc7..dceef3082 100644 --- a/library/ssl_tls13_client.c +++ b/library/ssl_tls13_client.c @@ -1030,6 +1030,7 @@ static int ssl_tls13_parse_server_hello( mbedtls_ssl_context *ssl, const unsigned char *extensions_end; uint16_t cipher_suite; const mbedtls_ssl_ciphersuite_t *ciphersuite_info; + int supported_versions_exist = 0; #if defined(MBEDTLS_SSL_COOKIE_C) size_t cookie_len; unsigned char *cookie; @@ -1071,10 +1072,13 @@ static int ssl_tls13_parse_server_hello( mbedtls_ssl_context *ssl, * with Random defined as: * opaque Random[MBEDTLS_SERVER_HELLO_RANDOM_LEN]; */ - memcpy( &ssl->handshake->randbytes[MBEDTLS_CLIENT_HELLO_RANDOM_LEN], p, - MBEDTLS_SERVER_HELLO_RANDOM_LEN ); - MBEDTLS_SSL_DEBUG_BUF( 3, "server hello, random bytes", - p, MBEDTLS_SERVER_HELLO_RANDOM_LEN ); + if( !is_hrr ) + { + memcpy( &ssl->handshake->randbytes[MBEDTLS_CLIENT_HELLO_RANDOM_LEN], p, + MBEDTLS_SERVER_HELLO_RANDOM_LEN ); + MBEDTLS_SSL_DEBUG_BUF( 3, "server hello, random bytes", + p, MBEDTLS_SERVER_HELLO_RANDOM_LEN ); + } p += MBEDTLS_SERVER_HELLO_RANDOM_LEN; /* ... @@ -1116,6 +1120,19 @@ static int ssl_tls13_parse_server_hello( mbedtls_ssl_context *ssl, return( MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER ); } + /* + * Check whether this ciphersuite is the same with what we received in HRR. + */ + if( ( !is_hrr ) && ( ssl->handshake->hello_retry_request_count > 0 ) && + ( cipher_suite != ssl->session_negotiate->ciphersuite ) ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "ciphersuite(%04x) not the one from HRR", + cipher_suite ) ); + + MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_ILLEGAL_PARAMETER, + MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER ); + return( MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER ); + } /* Configure ciphersuites */ mbedtls_ssl_optimize_checksum( ssl, ciphersuite_info ); @@ -1211,6 +1228,7 @@ static int ssl_tls13_parse_server_hello( mbedtls_ssl_context *ssl, #endif /* MBEDTLS_SSL_COOKIE_C */ case MBEDTLS_TLS_EXT_SUPPORTED_VERSIONS: + supported_versions_exist = 1; MBEDTLS_SSL_DEBUG_MSG( 3, ( "found supported_versions extension" ) ); @@ -1233,14 +1251,12 @@ static int ssl_tls13_parse_server_hello( mbedtls_ssl_context *ssl, #if defined(MBEDTLS_KEY_EXCHANGE_WITH_CERT_ENABLED) case MBEDTLS_TLS_EXT_KEY_SHARE: MBEDTLS_SSL_DEBUG_MSG( 3, ( "found key_shares extension" ) ); - if( is_hrr == SSL_SERVER_HELLO_COORDINATE_HRR ) + if( is_hrr ) ret = ssl_tls13_parse_hrr_key_share_ext( ssl, p, p + extension_data_len ); - else if( is_hrr == SSL_SERVER_HELLO_COORDINATE_HELLO ) + else ret = ssl_tls13_parse_key_share_ext( ssl, p, p + extension_data_len ); - else - ret = is_hrr; if( ret != 0 ) { MBEDTLS_SSL_DEBUG_RET( 1, @@ -1266,6 +1282,14 @@ static int ssl_tls13_parse_server_hello( mbedtls_ssl_context *ssl, p += extension_data_len; } + if( !supported_versions_exist ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "supported_versions not exist" ) ); + MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_ILLEGAL_PARAMETER, + MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER ); + return( MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER ); + } + return( 0 ); } From 78b1fa7e816333eb7441bdaa46b4bafd7e9d5d82 Mon Sep 17 00:00:00 2001 From: XiaokangQian Date: Wed, 19 Jan 2022 06:56:30 +0000 Subject: [PATCH 11/24] Update code base on comments Move reset transcript for hrr to generic Reset SHA256 or SHA384 other than both Rename message layer reset Add check log for hrr parse successfully Signed-off-by: XiaokangQian --- library/ssl_misc.h | 4 +- library/ssl_tls.c | 106 +----------------------------------- library/ssl_tls13_client.c | 36 +++++------- library/ssl_tls13_generic.c | 105 +++++++++++++++++++++++++++++++++++ tests/ssl-opt.sh | 2 + 5 files changed, 127 insertions(+), 126 deletions(-) diff --git a/library/ssl_misc.h b/library/ssl_misc.h index 8bea627a2..b07ff3644 100644 --- a/library/ssl_misc.h +++ b/library/ssl_misc.h @@ -1447,8 +1447,8 @@ void mbedtls_ssl_update_out_pointers( mbedtls_ssl_context *ssl, void mbedtls_ssl_update_in_pointers( mbedtls_ssl_context *ssl ); int mbedtls_ssl_session_reset_int( mbedtls_ssl_context *ssl, int partial ); -void mbedtls_ssl_tls13_session_reset_msg_layer( mbedtls_ssl_context *ssl, - int partial ); +void mbedtls_ssl_session_reset_msg_layer( mbedtls_ssl_context *ssl, + int partial ); /* * Send pending alert diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 940b306fc..223199c61 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -3400,8 +3400,8 @@ error: * If partial is non-zero, keep data in the input buffer and client ID. * (Use when a DTLS client reconnects from the same port.) */ -void mbedtls_ssl_tls13_session_reset_msg_layer( mbedtls_ssl_context *ssl, - int partial ) +void mbedtls_ssl_session_reset_msg_layer( mbedtls_ssl_context *ssl, + int partial ) { #if defined(MBEDTLS_SSL_VARIABLE_BUFFER_LENGTH) size_t in_buf_len = ssl->in_buf_len; @@ -3467,7 +3467,7 @@ int mbedtls_ssl_session_reset_int( mbedtls_ssl_context *ssl, int partial ) ssl->state = MBEDTLS_SSL_HELLO_REQUEST; - mbedtls_ssl_tls13_session_reset_msg_layer( ssl, partial ); + mbedtls_ssl_session_reset_msg_layer( ssl, partial ); /* Reset renegotiation state */ #if defined(MBEDTLS_SSL_RENEGOTIATION) @@ -7615,105 +7615,5 @@ int mbedtls_ssl_write_sig_alg_ext( mbedtls_ssl_context *ssl, unsigned char *buf, return( 0 ); } #endif /* MBEDTLS_KEY_EXCHANGE_WITH_CERT_ENABLED */ -#if defined(MBEDTLS_SSL_PROTO_TLS1_3) - -static int ssl_hash_transcript_core( mbedtls_ssl_context *ssl, - mbedtls_md_type_t md, - unsigned char *transcript, - size_t len, - size_t *olen ) -{ - int ret; - size_t hash_size; - - if( len < 4 ) - return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); - - ret = mbedtls_ssl_get_handshake_transcript( ssl, md, - transcript + 4, - len - 4, - &hash_size ); - if( ret != 0 ) - { - MBEDTLS_SSL_DEBUG_RET( 4, "mbedtls_ssl_get_handshake_transcript", ret ); - return( ret ); - } - - transcript[0] = MBEDTLS_SSL_HS_MESSAGE_HASH; - transcript[1] = 0; - transcript[2] = 0; - transcript[3] = (unsigned char) hash_size; - - *olen = 4 + hash_size; - return( 0 ); -} - -/* Reset SSL context and update hash for handling HRR. - * - * Replace Transcript-Hash(X) by - * Transcript-Hash( message_hash || - * 00 00 Hash.length || - * X ) - * A few states of the handshake are preserved, including: - * - session ID - * - session ticket - * - negotiated ciphersuite - */ -int mbedtls_ssl_reset_transcript_for_hrr( mbedtls_ssl_context *ssl ) -{ - int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; - unsigned char hash_transcript[ MBEDTLS_MD_MAX_SIZE + 4 ]; - size_t hash_olen; - - MBEDTLS_SSL_DEBUG_MSG( 3, ( "Reset SSL session for HRR" ) ); - -#if defined(MBEDTLS_SHA256_C) - ret = ssl_hash_transcript_core( ssl, MBEDTLS_MD_SHA256, - hash_transcript, - sizeof( hash_transcript ), - &hash_olen ); - if( ret != 0 ) - { - MBEDTLS_SSL_DEBUG_RET( 4, "ssl_hash_transcript_core", ret ); - return( ret ); - } - MBEDTLS_SSL_DEBUG_BUF( 4, "Truncated SHA-256 handshake transcript", - hash_transcript, hash_olen ); - -#if defined(MBEDTLS_USE_PSA_CRYPTO) - psa_hash_abort( &ssl->handshake->fin_sha256_psa ); - psa_hash_setup( &ssl->handshake->fin_sha256_psa, PSA_ALG_SHA_256 ); -#else - mbedtls_sha256_starts( &ssl->handshake->fin_sha256, 0 ); -#endif - ssl_update_checksum_sha256( ssl, hash_transcript, hash_olen ); -#endif /* MBEDTLS_SHA256_C */ - -#if defined(MBEDTLS_SHA384_C) - ret = ssl_hash_transcript_core( ssl, MBEDTLS_MD_SHA384, - hash_transcript, - sizeof( hash_transcript ), - &hash_olen ); - if( ret != 0 ) - { - MBEDTLS_SSL_DEBUG_RET( 4, "ssl_hash_transcript_core", ret ); - return( ret ); - } - MBEDTLS_SSL_DEBUG_BUF( 4, "Truncated SHA-384 handshake transcript", - hash_transcript, hash_olen ); - -#if defined(MBEDTLS_USE_PSA_CRYPTO) - psa_hash_abort( &ssl->handshake->fin_sha384_psa ); - psa_hash_setup( &ssl->handshake->fin_sha384_psa, PSA_ALG_SHA_384 ); -#else - mbedtls_sha512_starts( &ssl->handshake->fin_sha512, 1 ); -#endif - ssl_update_checksum_sha384( ssl, hash_transcript, hash_olen ); -#endif /* MBEDTLS_SHA384_C */ - - return( ret ); -} - -#endif /* MBEDTLS_SSL_PROTO_TLS1_3 */ #endif /* MBEDTLS_SSL_TLS_C */ diff --git a/library/ssl_tls13_client.c b/library/ssl_tls13_client.c index dceef3082..b04bf0986 100644 --- a/library/ssl_tls13_client.c +++ b/library/ssl_tls13_client.c @@ -136,12 +136,6 @@ static int ssl_tls13_parse_supported_versions_ext( mbedtls_ssl_context *ssl, */ #if defined(MBEDTLS_ECDH_C) -static int ssl_tls13_reset_ecdhe_share( mbedtls_ssl_context *ssl ) -{ - mbedtls_ecdh_free( &ssl->handshake->ecdh_ctx ); - return( 0 ); -} - static int ssl_tls13_reset_key_share( mbedtls_ssl_context *ssl ) { uint16_t group_id = ssl->handshake->offered_group_id; @@ -149,7 +143,10 @@ static int ssl_tls13_reset_key_share( mbedtls_ssl_context *ssl ) return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); if( mbedtls_ssl_tls13_named_group_is_ecdhe( group_id ) ) - return( ssl_tls13_reset_ecdhe_share( ssl ) ); + { + mbedtls_ecdh_free( &ssl->handshake->ecdh_ctx ); + return( 0 ); + } else if( 0 /* other KEMs? */ ) { /* Do something */ @@ -425,7 +422,6 @@ static int ssl_tls13_parse_hrr_key_share_ext( mbedtls_ssl_context *ssl, const unsigned char *buf, const unsigned char *end ) { - /* Variables for parsing the key_share */ const mbedtls_ecp_curve_info *curve_info = NULL; const unsigned char *p = buf; int tls_id; @@ -1030,7 +1026,7 @@ static int ssl_tls13_parse_server_hello( mbedtls_ssl_context *ssl, const unsigned char *extensions_end; uint16_t cipher_suite; const mbedtls_ssl_ciphersuite_t *ciphersuite_info; - int supported_versions_exist = 0; + int supported_versions_ext_found = 0; #if defined(MBEDTLS_SSL_COOKIE_C) size_t cookie_len; unsigned char *cookie; @@ -1228,7 +1224,7 @@ static int ssl_tls13_parse_server_hello( mbedtls_ssl_context *ssl, #endif /* MBEDTLS_SSL_COOKIE_C */ case MBEDTLS_TLS_EXT_SUPPORTED_VERSIONS: - supported_versions_exist = 1; + supported_versions_ext_found = 1; MBEDTLS_SSL_DEBUG_MSG( 3, ( "found supported_versions extension" ) ); @@ -1282,9 +1278,9 @@ static int ssl_tls13_parse_server_hello( mbedtls_ssl_context *ssl, p += extension_data_len; } - if( !supported_versions_exist ) + if( !supported_versions_ext_found ) { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "supported_versions not exist" ) ); + MBEDTLS_SSL_DEBUG_MSG( 1, ( "supported_versions not found" ) ); MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_ILLEGAL_PARAMETER, MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER ); return( MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER ); @@ -1424,14 +1420,12 @@ static int ssl_tls13_finalize_hrr( mbedtls_ssl_context *ssl ) mbedtls_ssl_handshake_set_state( ssl, MBEDTLS_SSL_CLIENT_HELLO ); #endif /* MBEDTLS_SSL_TLS1_3_COMPATIBILITY_MODE */ - mbedtls_ssl_tls13_session_reset_msg_layer( ssl, 0 ); + mbedtls_ssl_session_reset_msg_layer( ssl, 0 ); - /* Reset everything that's going to be re-generated in the new ClientHello. - * - * Currently, we're always resetting the key share, even if the server - * was fine with it. Once we have separated key share generation from - * key share writing, we can confine this to the case where the server - * requested a different share. + /* + * We are going to re-generate a shared secret corresponding to the group selected by the server, + * which is different from the group for which we generated a shared secret in the first client + * hello. Thus, reset the shared secret. */ #if defined(MBEDTLS_KEY_EXCHANGE_WITH_CERT_ENABLED) ret = ssl_tls13_reset_key_share( ssl ); @@ -1451,7 +1445,7 @@ static int ssl_tls13_process_server_hello( mbedtls_ssl_context *ssl ) int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; unsigned char *buf = NULL; size_t buf_len = 0; - int is_hrr = -1; + int is_hrr = 0; MBEDTLS_SSL_DEBUG_MSG( 2, ( "=> %s", __func__ ) ); @@ -1488,7 +1482,7 @@ static int ssl_tls13_process_server_hello( mbedtls_ssl_context *ssl ) MBEDTLS_SSL_PROC_CHK( ssl_tls13_finalize_server_hello( ssl ) ); cleanup: - MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= %s", __func__ ) ); + MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= %s:is_hrr = %d", __func__, is_hrr ) ); return( ret ); } diff --git a/library/ssl_tls13_generic.c b/library/ssl_tls13_generic.c index 9e6c52a4e..88e7de925 100644 --- a/library/ssl_tls13_generic.c +++ b/library/ssl_tls13_generic.c @@ -1128,6 +1128,111 @@ cleanup: #endif /* MBEDTLS_SSL_TLS1_3_COMPATIBILITY_MODE */ +static int ssl_hash_transcript_core( mbedtls_ssl_context *ssl, + mbedtls_md_type_t md, + unsigned char *transcript, + size_t len, + size_t *olen ) +{ + int ret; + size_t hash_size; + + if( len < 4 ) + return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); + + ret = mbedtls_ssl_get_handshake_transcript( ssl, md, + transcript + 4, + len - 4, + &hash_size ); + if( ret != 0 ) + { + MBEDTLS_SSL_DEBUG_RET( 4, "mbedtls_ssl_get_handshake_transcript", ret ); + return( ret ); + } + + transcript[0] = MBEDTLS_SSL_HS_MESSAGE_HASH; + transcript[1] = 0; + transcript[2] = 0; + transcript[3] = (unsigned char) hash_size; + + *olen = 4 + hash_size; + return( 0 ); +} + +/* Reset SSL context and update hash for handling HRR. + * + * Replace Transcript-Hash(X) by + * Transcript-Hash( message_hash || + * 00 00 Hash.length || + * X ) + * A few states of the handshake are preserved, including: + * - session ID + * - session ticket + * - negotiated ciphersuite + */ +int mbedtls_ssl_reset_transcript_for_hrr( mbedtls_ssl_context *ssl ) +{ + int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; + unsigned char hash_transcript[ MBEDTLS_MD_MAX_SIZE + 4 ]; + size_t hash_olen; + const mbedtls_ssl_ciphersuite_t *ciphersuite_info; + uint16_t cipher_suite = ssl->session_negotiate->ciphersuite; + ciphersuite_info = mbedtls_ssl_ciphersuite_from_id( cipher_suite ); + + MBEDTLS_SSL_DEBUG_MSG( 3, ( "Reset SSL session for HRR" ) ); + + if( ciphersuite_info->mac == MBEDTLS_MD_SHA256 ) + { +#if defined(MBEDTLS_SHA256_C) + ret = ssl_hash_transcript_core( ssl, MBEDTLS_MD_SHA256, + hash_transcript, + sizeof( hash_transcript ), + &hash_olen ); + if( ret != 0 ) + { + MBEDTLS_SSL_DEBUG_RET( 4, "ssl_hash_transcript_core", ret ); + return( ret ); + } + MBEDTLS_SSL_DEBUG_BUF( 4, "Truncated SHA-256 handshake transcript", + hash_transcript, hash_olen ); + +#if defined(MBEDTLS_USE_PSA_CRYPTO) + psa_hash_abort( &ssl->handshake->fin_sha256_psa ); + psa_hash_setup( &ssl->handshake->fin_sha256_psa, PSA_ALG_SHA_256 ); +#else + mbedtls_sha256_starts( &ssl->handshake->fin_sha256, 0 ); +#endif + ssl->handshake->update_checksum( ssl, hash_transcript, hash_olen ); +#endif /* MBEDTLS_SHA256_C */ + } + else if( ciphersuite_info->mac == MBEDTLS_MD_SHA384 ) + { +#if defined(MBEDTLS_SHA384_C) + ret = ssl_hash_transcript_core( ssl, MBEDTLS_MD_SHA384, + hash_transcript, + sizeof( hash_transcript ), + &hash_olen ); + if( ret != 0 ) + { + MBEDTLS_SSL_DEBUG_RET( 4, "ssl_hash_transcript_core", ret ); + return( ret ); + } + MBEDTLS_SSL_DEBUG_BUF( 4, "Truncated SHA-384 handshake transcript", + hash_transcript, hash_olen ); + +#if defined(MBEDTLS_USE_PSA_CRYPTO) + psa_hash_abort( &ssl->handshake->fin_sha384_psa ); + psa_hash_setup( &ssl->handshake->fin_sha384_psa, PSA_ALG_SHA_384 ); +#else + mbedtls_sha512_starts( &ssl->handshake->fin_sha512, 1 ); +#endif + ssl->handshake->update_checksum( ssl, hash_transcript, hash_olen ); +#endif /* MBEDTLS_SHA384_C */ + } + + return( ret ); +} + #endif /* MBEDTLS_SSL_PROTO_TLS1_3 */ #endif /* MBEDTLS_SSL_TLS_C */ diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index 37fa287cc..3ea5940ec 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -9219,6 +9219,7 @@ run_test "TLS 1.3: HelloRetryRequest check - openssl" \ "$P_CLI debug_level=4 force_version=tls13" \ 0 \ -c "received HelloRetryRequest message" \ + -c "<= ssl_tls13_process_server_hello:is_hrr = 1" \ -c "tls13 client state: MBEDTLS_SSL_CLIENT_HELLO(1)" \ -c "HTTP/1.0 200 ok" @@ -9234,6 +9235,7 @@ run_test "TLS 1.3: HelloRetryRequest check - gnutls" \ "$P_CLI debug_level=4 force_version=tls13" \ 1 \ -c "received HelloRetryRequest message" \ + -c "<= ssl_tls13_process_server_hello:is_hrr = 1" \ -c "tls13 client state: MBEDTLS_SSL_CLIENT_HELLO(1)" \ -c "Last error was: -0x6E00 - SSL - The handshake negotiation failed" \ -s "HELLO RETRY REQUEST was queued" From 355e09ae9d416bf242e7a3b4727535d7928beb81 Mon Sep 17 00:00:00 2001 From: XiaokangQian Date: Thu, 20 Jan 2022 11:14:50 +0000 Subject: [PATCH 12/24] Change code base on comments Change functions name Change some comments Improve hrr test case for gnutls Signed-off-by: XiaokangQian --- library/ssl_tls13_client.c | 110 +++++++++++++----------------------- library/ssl_tls13_generic.c | 4 +- tests/ssl-opt.sh | 7 +-- 3 files changed, 43 insertions(+), 78 deletions(-) diff --git a/library/ssl_tls13_client.c b/library/ssl_tls13_client.c index b04bf0986..6546c77b7 100644 --- a/library/ssl_tls13_client.c +++ b/library/ssl_tls13_client.c @@ -115,52 +115,27 @@ static int ssl_tls13_parse_supported_versions_ext( mbedtls_ssl_context *ssl, #if defined(MBEDTLS_KEY_EXCHANGE_WITH_CERT_ENABLED) -/* - * Key Shares Extension - * - * enum { - * ... (0xFFFF) - * } NamedGroup; - * - * struct { - * NamedGroup group; - * opaque key_exchange<1..2^16-1>; - * } KeyShareEntry; - * - * struct { - * select(role) { - * case client: - * KeyShareEntry client_shares<0..2^16-1>; - * } - * } KeyShare; - */ - -#if defined(MBEDTLS_ECDH_C) static int ssl_tls13_reset_key_share( mbedtls_ssl_context *ssl ) { uint16_t group_id = ssl->handshake->offered_group_id; if( group_id == 0 ) return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); +#if defined(MBEDTLS_ECDH_C) if( mbedtls_ssl_tls13_named_group_is_ecdhe( group_id ) ) { mbedtls_ecdh_free( &ssl->handshake->ecdh_ctx ); return( 0 ); } - else if( 0 /* other KEMs? */ ) + else +#endif /* MBEDTLS_ECDH_C */ + if( 0 /* other KEMs? */ ) { /* Do something */ } return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); } -#else -static int ssl_tls13_reset_key_share( mbedtls_ssl_context *ssl ) -{ - ((void) ssl); - return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); -} -#endif /* MBEDTLS_ECDH_C */ /* * Functions for writing key_share extension. @@ -475,7 +450,7 @@ static int ssl_tls13_parse_hrr_key_share_ext( mbedtls_ssl_context *ssl, } /* Remember server's preference for next ClientHello */ - ssl->handshake->offered_group_id= tls_id; + ssl->handshake->offered_group_id = tls_id; return( 0 ); } @@ -906,20 +881,9 @@ static int ssl_tls13_server_hello_coordinate( mbedtls_ssl_context *ssl, { int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; - MBEDTLS_SSL_PROC_CHK( mbedtls_ssl_read_record( ssl, 0 ) ); - - if( ( ssl->in_msgtype != MBEDTLS_SSL_MSG_HANDSHAKE ) || - ( ssl->in_msg[0] != MBEDTLS_SSL_HS_SERVER_HELLO ) ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "unexpected message" ) ); - - MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_UNEXPECTED_MESSAGE, - MBEDTLS_ERR_SSL_UNEXPECTED_MESSAGE ); - return( MBEDTLS_ERR_SSL_UNEXPECTED_MESSAGE ); - } - - *buf = ssl->in_msg + 4; - *buf_len = ssl->in_hslen - 4; + MBEDTLS_SSL_PROC_CHK( mbedtls_ssl_tls13_fetch_handshake_msg( ssl, + MBEDTLS_SSL_HS_SERVER_HELLO, + buf, buf_len ) ); ret = ssl_server_hello_is_hrr( ssl, *buf, *buf + *buf_len ); switch( ret ) @@ -1022,6 +986,7 @@ static int ssl_tls13_parse_server_hello( mbedtls_ssl_context *ssl, { int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; const unsigned char *p = buf; + mbedtls_ssl_handshake_params *handshake = ssl->handshake; size_t extensions_len; const unsigned char *extensions_end; uint16_t cipher_suite; @@ -1070,7 +1035,7 @@ static int ssl_tls13_parse_server_hello( mbedtls_ssl_context *ssl, */ if( !is_hrr ) { - memcpy( &ssl->handshake->randbytes[MBEDTLS_CLIENT_HELLO_RANDOM_LEN], p, + memcpy( &handshake->randbytes[MBEDTLS_CLIENT_HELLO_RANDOM_LEN], p, MBEDTLS_SERVER_HELLO_RANDOM_LEN ); MBEDTLS_SSL_DEBUG_BUF( 3, "server hello, random bytes", p, MBEDTLS_SERVER_HELLO_RANDOM_LEN ); @@ -1099,32 +1064,34 @@ static int ssl_tls13_parse_server_hello( mbedtls_ssl_context *ssl, p += 2; + ciphersuite_info = mbedtls_ssl_ciphersuite_from_id( cipher_suite ); /* * Check whether this ciphersuite is supported and offered. * Via the force_ciphersuite version we may have instructed the client * to use a different ciphersuite. */ - ciphersuite_info = mbedtls_ssl_ciphersuite_from_id( cipher_suite ); if( ciphersuite_info == NULL || ssl_tls13_cipher_suite_is_offered( ssl, cipher_suite ) == 0 ) { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "ciphersuite(%04x) not found or not offered", - cipher_suite ) ); - - MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_ILLEGAL_PARAMETER, - MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER ); - return( MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER ); + ret = MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER; } - /* * Check whether this ciphersuite is the same with what we received in HRR. + * If we received an HRR before and that the proposed selected + * ciphersuite in this server hello is not the same as the one + * proposed in the HRR, we abort the handshake and send an + * "illegal_parameter" alert. */ - if( ( !is_hrr ) && ( ssl->handshake->hello_retry_request_count > 0 ) && - ( cipher_suite != ssl->session_negotiate->ciphersuite ) ) + else if( ( !is_hrr ) && ( handshake->hello_retry_request_count > 0 ) && + ( cipher_suite != ssl->session_negotiate->ciphersuite ) ) { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "ciphersuite(%04x) not the one from HRR", - cipher_suite ) ); + ret = MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER; + } + if( ret == MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "invalid ciphersuite(%04x) parameter", + cipher_suite ) ); MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_ILLEGAL_PARAMETER, MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER ); return( MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER ); @@ -1133,7 +1100,7 @@ static int ssl_tls13_parse_server_hello( mbedtls_ssl_context *ssl, /* Configure ciphersuites */ mbedtls_ssl_optimize_checksum( ssl, ciphersuite_info ); - ssl->handshake->ciphersuite_info = ciphersuite_info; + handshake->ciphersuite_info = ciphersuite_info; ssl->session_negotiate->ciphersuite = cipher_suite; MBEDTLS_SSL_DEBUG_MSG( 3, ( "server hello, chosen ciphersuite: ( %04x ) - %s", @@ -1208,9 +1175,9 @@ static int ssl_tls13_parse_server_hello( mbedtls_ssl_context *ssl, MBEDTLS_SSL_CHK_BUF_READ_PTR( p, extensions_end, cookie_len + 2 ); MBEDTLS_SSL_DEBUG_BUF( 3, "cookie extension", cookie, cookie_len ); - mbedtls_free( ssl->handshake->verify_cookie ); - ssl->handshake->verify_cookie = mbedtls_calloc( 1, cookie_len ); - if( ssl->handshake->verify_cookie == NULL ) + mbedtls_free( handshake->verify_cookie ); + handshake->verify_cookie = mbedtls_calloc( 1, cookie_len ); + if( handshake->verify_cookie == NULL ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "alloc failed ( %" MBEDTLS_PRINTF_SIZET " bytes )", @@ -1218,8 +1185,8 @@ static int ssl_tls13_parse_server_hello( mbedtls_ssl_context *ssl, return( MBEDTLS_ERR_SSL_ALLOC_FAILED ); } - memcpy( ssl->handshake->verify_cookie, cookie, cookie_len ); - ssl->handshake->verify_cookie_len = (unsigned char) cookie_len; + memcpy( handshake->verify_cookie, cookie, cookie_len ); + handshake->verify_cookie_len = (unsigned char) cookie_len; break; #endif /* MBEDTLS_SSL_COOKIE_C */ @@ -1289,7 +1256,7 @@ static int ssl_tls13_parse_server_hello( mbedtls_ssl_context *ssl, return( 0 ); } -static int ssl_tls13_finalize_server_hello( mbedtls_ssl_context *ssl ) +static int ssl_tls13_postprocess_server_hello( mbedtls_ssl_context *ssl ) { int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; mbedtls_ssl_key_set traffic_keys; @@ -1403,11 +1370,10 @@ cleanup: return( ret ); } -static int ssl_tls13_finalize_hrr( mbedtls_ssl_context *ssl ) +static int ssl_tls13_postprocess_hrr( mbedtls_ssl_context *ssl ) { #if defined(MBEDTLS_KEY_EXCHANGE_WITH_CERT_ENABLED) int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; -#endif /* MBEDTLS_KEY_EXCHANGE_WITH_CERT_ENABLED */ #if defined(MBEDTLS_SSL_TLS1_3_COMPATIBILITY_MODE) /* If not offering early data, the client sends a dummy CCS record @@ -1423,11 +1389,11 @@ static int ssl_tls13_finalize_hrr( mbedtls_ssl_context *ssl ) mbedtls_ssl_session_reset_msg_layer( ssl, 0 ); /* - * We are going to re-generate a shared secret corresponding to the group selected by the server, - * which is different from the group for which we generated a shared secret in the first client - * hello. Thus, reset the shared secret. + * We are going to re-generate a shared secret corresponding to the group + * selected by the server, which is different from the group for which we + * generated a shared secret in the first client hello. + * Thus, reset the shared secret. */ -#if defined(MBEDTLS_KEY_EXCHANGE_WITH_CERT_ENABLED) ret = ssl_tls13_reset_key_share( ssl ); if( ret != 0 ) return( ret ); @@ -1477,9 +1443,9 @@ static int ssl_tls13_process_server_hello( mbedtls_ssl_context *ssl ) buf, buf_len ); if( is_hrr ) - MBEDTLS_SSL_PROC_CHK( ssl_tls13_finalize_hrr( ssl ) ); + MBEDTLS_SSL_PROC_CHK( ssl_tls13_postprocess_hrr( ssl ) ); else - MBEDTLS_SSL_PROC_CHK( ssl_tls13_finalize_server_hello( ssl ) ); + MBEDTLS_SSL_PROC_CHK( ssl_tls13_postprocess_server_hello( ssl ) ); cleanup: MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= %s:is_hrr = %d", __func__, is_hrr ) ); diff --git a/library/ssl_tls13_generic.c b/library/ssl_tls13_generic.c index 88e7de925..b4af2e053 100644 --- a/library/ssl_tls13_generic.c +++ b/library/ssl_tls13_generic.c @@ -1058,7 +1058,7 @@ void mbedtls_ssl_tls13_handshake_wrapup( mbedtls_ssl_context *ssl ) */ #if defined(MBEDTLS_SSL_TLS1_3_COMPATIBILITY_MODE) -static int ssl_write_change_cipher_spec_postprocess( mbedtls_ssl_context* ssl ) +static int ssl_tls13_finalize_change_cipher_spec( mbedtls_ssl_context* ssl ) { #if defined(MBEDTLS_SSL_CLI_C) @@ -1115,7 +1115,7 @@ int mbedtls_ssl_tls13_write_change_cipher_spec( mbedtls_ssl_context *ssl ) ssl->out_msgtype = MBEDTLS_SSL_MSG_CHANGE_CIPHER_SPEC; /* Update state */ - MBEDTLS_SSL_PROC_CHK( ssl_write_change_cipher_spec_postprocess( ssl ) ); + MBEDTLS_SSL_PROC_CHK( ssl_tls13_finalize_change_cipher_spec( ssl ) ); /* Dispatch message */ MBEDTLS_SSL_PROC_CHK( mbedtls_ssl_write_record( ssl, 1 ) ); diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index 3ea5940ec..e6e94b386 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -9231,14 +9231,13 @@ requires_config_enabled MBEDTLS_DEBUG_C requires_config_enabled MBEDTLS_SSL_CLI_C requires_config_disabled MBEDTLS_USE_PSA_CRYPTO run_test "TLS 1.3: HelloRetryRequest check - gnutls" \ - "$G_NEXT_SRV -d 4 --priority=NONE:+GROUP-SECP256R1:+AES-256-GCM:+SHA384:+AEAD:+SIGN-ECDSA-SECP256R1-SHA256:+VERS-TLS1.3:%NO_TICKETS" \ + "$G_NEXT_SRV -d 4 --priority=NONE:+GROUP-SECP256R1:+AES-256-GCM:+SHA384:+AEAD:+SIGN-ECDSA-SECP256R1-SHA256:+VERS-TLS1.3:%NO_TICKETS --disable-client-cert" \ "$P_CLI debug_level=4 force_version=tls13" \ - 1 \ + 0 \ -c "received HelloRetryRequest message" \ -c "<= ssl_tls13_process_server_hello:is_hrr = 1" \ -c "tls13 client state: MBEDTLS_SSL_CLIENT_HELLO(1)" \ - -c "Last error was: -0x6E00 - SSL - The handshake negotiation failed" \ - -s "HELLO RETRY REQUEST was queued" + -c "HTTP/1.0 200 OK" for i in $(ls opt-testcases/*.sh) do From 2b01dc30cb00c7a680edec039237a0d0d4b1a761 Mon Sep 17 00:00:00 2001 From: XiaokangQian Date: Fri, 21 Jan 2022 02:53:13 +0000 Subject: [PATCH 13/24] Add hrr no change check and allign mbedtls_ssl_session_reset_msg_layer Signed-off-by: XiaokangQian --- library/ssl_tls.c | 29 +++++++++++++++++++++++++++++ library/ssl_tls13_client.c | 15 +++++++++++++++ 2 files changed, 44 insertions(+) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 223199c61..3399a8823 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -3403,6 +3403,7 @@ error: void mbedtls_ssl_session_reset_msg_layer( mbedtls_ssl_context *ssl, int partial ) { +#if defined(MBEDTLS_SSL_LEGACY_MSG_LAYER_REQUIRED) #if defined(MBEDTLS_SSL_VARIABLE_BUFFER_LENGTH) size_t in_buf_len = ssl->in_buf_len; size_t out_buf_len = ssl->out_buf_len; @@ -3453,12 +3454,40 @@ void mbedtls_ssl_session_reset_msg_layer( mbedtls_ssl_context *ssl, mbedtls_ssl_dtls_replay_reset( ssl ); #endif +#if defined(MBEDTLS_SSL_PROTO_TLS1_2) if( ssl->transform ) { mbedtls_ssl_transform_free( ssl->transform ); mbedtls_free( ssl->transform ); ssl->transform = NULL; } +#endif /* MBEDTLS_SSL_PROTO_TLS1_2 */ + +#else + ((void) partial); +#endif /* MBEDTLS_SSL_LEGACY_MSG_LAYER_REQUIRED */ + +#if defined(MBEDTLS_SSL_PROTO_TLS1_3) + mbedtls_ssl_transform_free( ssl->transform_application ); + mbedtls_free( ssl->transform_application ); + ssl->transform_application = NULL; + + if( ssl->handshake != NULL ) + { + mbedtls_ssl_transform_free( ssl->handshake->transform_earlydata ); + mbedtls_free( ssl->handshake->transform_earlydata ); + ssl->handshake->transform_earlydata = NULL; + + mbedtls_ssl_transform_free( ssl->handshake->transform_handshake ); + mbedtls_free( ssl->handshake->transform_handshake ); + ssl->handshake->transform_handshake = NULL; + } + +#if defined(MBEDTLS_ZERO_RTT) && defined(MBEDTLS_SSL_CLI_C) + ssl->early_data_buf = NULL; + ssl->early_data_len = 0; +#endif /* MBEDTLS_ZERO_RTT && MBEDTLS_SSL_CLI_C */ +#endif /* MBEDTLS_SSL_PROTO_TLS1_3 */ } int mbedtls_ssl_session_reset_int( mbedtls_ssl_context *ssl, int partial ) diff --git a/library/ssl_tls13_client.c b/library/ssl_tls13_client.c index 6546c77b7..609db0306 100644 --- a/library/ssl_tls13_client.c +++ b/library/ssl_tls13_client.c @@ -905,6 +905,21 @@ static int ssl_tls13_server_hello_coordinate( mbedtls_ssl_context *ssl, MBEDTLS_ERR_SSL_UNEXPECTED_MESSAGE ); return( MBEDTLS_ERR_SSL_UNEXPECTED_MESSAGE ); } + /* + * Clients must abort the handshake with an "illegal_parameter" + * alert if the HelloRetryRequest would not result in any change + * in the ClientHello. + * In a PSK only key exchange that what we expect. + */ + if( ! mbedtls_ssl_conf_tls13_some_ephemeral_enabled( ssl ) ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, + ( "Unexpected HRR in pure PSK key exchange." ) ); + MBEDTLS_SSL_PEND_FATAL_ALERT( + MBEDTLS_SSL_ALERT_MSG_ILLEGAL_PARAMETER, + MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER); + return( MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER ); + } ssl->handshake->hello_retry_request_count++; From 43550bd761dc0a8c97258a20b02d0afa4138ecb9 Mon Sep 17 00:00:00 2001 From: XiaokangQian Date: Fri, 21 Jan 2022 04:32:58 +0000 Subject: [PATCH 14/24] Prepare function to parse hrr cookie extension Signed-off-by: XiaokangQian --- library/ssl_tls13_client.c | 73 +++++++++++++++++++++++++------------- 1 file changed, 48 insertions(+), 25 deletions(-) diff --git a/library/ssl_tls13_client.c b/library/ssl_tls13_client.c index 609db0306..6168ddd35 100644 --- a/library/ssl_tls13_client.c +++ b/library/ssl_tls13_client.c @@ -518,6 +518,40 @@ static int ssl_tls13_parse_key_share_ext( mbedtls_ssl_context *ssl, #endif /* MBEDTLS_KEY_EXCHANGE_WITH_CERT_ENABLED */ +#if defined(MBEDTLS_SSL_COOKIE_C) +static int ssl_tls13_parse_cookie_ext( mbedtls_ssl_context *ssl, + const unsigned char *buf, + const unsigned char *end ) +{ + size_t cookie_len; + const unsigned char *p = buf; + mbedtls_ssl_handshake_params *handshake = ssl->handshake; + + /* Retrieve length field of cookie */ + MBEDTLS_SSL_CHK_BUF_READ_PTR( p, end, 2 ); + cookie_len = MBEDTLS_GET_UINT16_BE( p, 0 ); + p += 2; + + MBEDTLS_SSL_CHK_BUF_READ_PTR( p, end, cookie_len ); + MBEDTLS_SSL_DEBUG_BUF( 3, "cookie extension", p, cookie_len ); + + mbedtls_free( handshake->verify_cookie ); + handshake->verify_cookie = mbedtls_calloc( 1, cookie_len ); + if( handshake->verify_cookie == NULL ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, + ( "alloc failed ( %" MBEDTLS_PRINTF_SIZET " bytes )", + cookie_len ) ); + return( MBEDTLS_ERR_SSL_ALLOC_FAILED ); + } + + memcpy( handshake->verify_cookie, p, cookie_len ); + handshake->verify_cookie_len = (unsigned char) cookie_len; + + return( 0 ); +} +#endif /* MBEDTLS_SSL_COOKIE_C */ + /* Write cipher_suites * CipherSuite cipher_suites<2..2^16-2>; */ @@ -1007,10 +1041,6 @@ static int ssl_tls13_parse_server_hello( mbedtls_ssl_context *ssl, uint16_t cipher_suite; const mbedtls_ssl_ciphersuite_t *ciphersuite_info; int supported_versions_ext_found = 0; -#if defined(MBEDTLS_SSL_COOKIE_C) - size_t cookie_len; - unsigned char *cookie; -#endif /* MBEDTLS_SSL_COOKIE_C */ /* * Check there is space for minimal fields @@ -1161,6 +1191,7 @@ static int ssl_tls13_parse_server_hello( mbedtls_ssl_context *ssl, { unsigned int extension_type; size_t extension_data_len; + const unsigned char *extension_data_end; MBEDTLS_SSL_CHK_BUF_READ_PTR( p, extensions_end, 4 ); extension_type = MBEDTLS_GET_UINT16_BE( p, 0 ); @@ -1168,6 +1199,7 @@ static int ssl_tls13_parse_server_hello( mbedtls_ssl_context *ssl, p += 4; MBEDTLS_SSL_CHK_BUF_READ_PTR( p, extensions_end, extension_data_len ); + extension_data_end = p + extension_data_len; switch( extension_type ) { @@ -1182,26 +1214,15 @@ static int ssl_tls13_parse_server_hello( mbedtls_ssl_context *ssl, return( MBEDTLS_ERR_SSL_UNSUPPORTED_EXTENSION ); } - /* Retrieve length field of cookie */ - MBEDTLS_SSL_CHK_BUF_READ_PTR( p, extensions_end, 2 ); - cookie_len = MBEDTLS_GET_UINT16_BE( p, 0 ); - cookie = (unsigned char *) ( p + 2 ); - - MBEDTLS_SSL_CHK_BUF_READ_PTR( p, extensions_end, cookie_len + 2 ); - MBEDTLS_SSL_DEBUG_BUF( 3, "cookie extension", cookie, cookie_len ); - - mbedtls_free( handshake->verify_cookie ); - handshake->verify_cookie = mbedtls_calloc( 1, cookie_len ); - if( handshake->verify_cookie == NULL ) + ret = ssl_tls13_parse_cookie_ext( ssl, + p, extension_data_end ); + if( ret != 0 ) { - MBEDTLS_SSL_DEBUG_MSG( 1, - ( "alloc failed ( %" MBEDTLS_PRINTF_SIZET " bytes )", - cookie_len ) ); - return( MBEDTLS_ERR_SSL_ALLOC_FAILED ); + MBEDTLS_SSL_DEBUG_RET( 1, + "ssl_tls13_parse_cookie_ext", + ret ); + return( ret ); } - - memcpy( handshake->verify_cookie, cookie, cookie_len ); - handshake->verify_cookie_len = (unsigned char) cookie_len; break; #endif /* MBEDTLS_SSL_COOKIE_C */ @@ -1212,7 +1233,7 @@ static int ssl_tls13_parse_server_hello( mbedtls_ssl_context *ssl, ret = ssl_tls13_parse_supported_versions_ext( ssl, p, - p + extension_data_len ); + extension_data_end ); if( ret != 0 ) return( ret ); break; @@ -1231,10 +1252,10 @@ static int ssl_tls13_parse_server_hello( mbedtls_ssl_context *ssl, MBEDTLS_SSL_DEBUG_MSG( 3, ( "found key_shares extension" ) ); if( is_hrr ) ret = ssl_tls13_parse_hrr_key_share_ext( ssl, - p, p + extension_data_len ); + p, extension_data_end ); else ret = ssl_tls13_parse_key_share_ext( ssl, - p, p + extension_data_len ); + p, extension_data_end ); if( ret != 0 ) { MBEDTLS_SSL_DEBUG_RET( 1, @@ -1412,6 +1433,8 @@ static int ssl_tls13_postprocess_hrr( mbedtls_ssl_context *ssl ) ret = ssl_tls13_reset_key_share( ssl ); if( ret != 0 ) return( ret ); +#else + ((void) ssl); #endif /* MBEDTLS_KEY_EXCHANGE_WITH_CERT_ENABLED */ return( 0 ); From f1e7d12cb657680e04430a3ffec4cc98f94c7605 Mon Sep 17 00:00:00 2001 From: XiaokangQian Date: Fri, 21 Jan 2022 05:49:57 +0000 Subject: [PATCH 15/24] Fix compile issues in mbedtls_ssl_session_reset_msg_layer Signed-off-by: XiaokangQian --- library/ssl_tls.c | 9 --------- 1 file changed, 9 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 3399a8823..6f37201bf 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -3403,7 +3403,6 @@ error: void mbedtls_ssl_session_reset_msg_layer( mbedtls_ssl_context *ssl, int partial ) { -#if defined(MBEDTLS_SSL_LEGACY_MSG_LAYER_REQUIRED) #if defined(MBEDTLS_SSL_VARIABLE_BUFFER_LENGTH) size_t in_buf_len = ssl->in_buf_len; size_t out_buf_len = ssl->out_buf_len; @@ -3463,10 +3462,6 @@ void mbedtls_ssl_session_reset_msg_layer( mbedtls_ssl_context *ssl, } #endif /* MBEDTLS_SSL_PROTO_TLS1_2 */ -#else - ((void) partial); -#endif /* MBEDTLS_SSL_LEGACY_MSG_LAYER_REQUIRED */ - #if defined(MBEDTLS_SSL_PROTO_TLS1_3) mbedtls_ssl_transform_free( ssl->transform_application ); mbedtls_free( ssl->transform_application ); @@ -3483,10 +3478,6 @@ void mbedtls_ssl_session_reset_msg_layer( mbedtls_ssl_context *ssl, ssl->handshake->transform_handshake = NULL; } -#if defined(MBEDTLS_ZERO_RTT) && defined(MBEDTLS_SSL_CLI_C) - ssl->early_data_buf = NULL; - ssl->early_data_len = 0; -#endif /* MBEDTLS_ZERO_RTT && MBEDTLS_SSL_CLI_C */ #endif /* MBEDTLS_SSL_PROTO_TLS1_3 */ } From 0ece99828710e1dc4efdc851d04a214949595ee6 Mon Sep 17 00:00:00 2001 From: XiaokangQian Date: Mon, 24 Jan 2022 08:56:23 +0000 Subject: [PATCH 16/24] Refine code in mbedtls_ssl_reset_transcript_for_hrr Signed-off-by: XiaokangQian --- library/ssl_tls13_generic.c | 77 +++++++++++-------------------------- 1 file changed, 23 insertions(+), 54 deletions(-) diff --git a/library/ssl_tls13_generic.c b/library/ssl_tls13_generic.c index b4af2e053..9aa214873 100644 --- a/library/ssl_tls13_generic.c +++ b/library/ssl_tls13_generic.c @@ -1128,37 +1128,6 @@ cleanup: #endif /* MBEDTLS_SSL_TLS1_3_COMPATIBILITY_MODE */ -static int ssl_hash_transcript_core( mbedtls_ssl_context *ssl, - mbedtls_md_type_t md, - unsigned char *transcript, - size_t len, - size_t *olen ) -{ - int ret; - size_t hash_size; - - if( len < 4 ) - return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); - - ret = mbedtls_ssl_get_handshake_transcript( ssl, md, - transcript + 4, - len - 4, - &hash_size ); - if( ret != 0 ) - { - MBEDTLS_SSL_DEBUG_RET( 4, "mbedtls_ssl_get_handshake_transcript", ret ); - return( ret ); - } - - transcript[0] = MBEDTLS_SSL_HS_MESSAGE_HASH; - transcript[1] = 0; - transcript[2] = 0; - transcript[3] = (unsigned char) hash_size; - - *olen = 4 + hash_size; - return( 0 ); -} - /* Reset SSL context and update hash for handling HRR. * * Replace Transcript-Hash(X) by @@ -1174,27 +1143,35 @@ int mbedtls_ssl_reset_transcript_for_hrr( mbedtls_ssl_context *ssl ) { int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; unsigned char hash_transcript[ MBEDTLS_MD_MAX_SIZE + 4 ]; - size_t hash_olen; + size_t hash_len; const mbedtls_ssl_ciphersuite_t *ciphersuite_info; uint16_t cipher_suite = ssl->session_negotiate->ciphersuite; ciphersuite_info = mbedtls_ssl_ciphersuite_from_id( cipher_suite ); MBEDTLS_SSL_DEBUG_MSG( 3, ( "Reset SSL session for HRR" ) ); + ret = mbedtls_ssl_get_handshake_transcript( ssl, ciphersuite_info->mac, + hash_transcript + 4, + MBEDTLS_MD_MAX_SIZE, + &hash_len ); + if( ret != 0 ) + { + MBEDTLS_SSL_DEBUG_RET( 4, "mbedtls_ssl_get_handshake_transcript", ret ); + return( ret ); + } + + hash_transcript[0] = MBEDTLS_SSL_HS_MESSAGE_HASH; + hash_transcript[1] = 0; + hash_transcript[2] = 0; + hash_transcript[3] = (unsigned char) hash_len; + + hash_len += 4; + if( ciphersuite_info->mac == MBEDTLS_MD_SHA256 ) { #if defined(MBEDTLS_SHA256_C) - ret = ssl_hash_transcript_core( ssl, MBEDTLS_MD_SHA256, - hash_transcript, - sizeof( hash_transcript ), - &hash_olen ); - if( ret != 0 ) - { - MBEDTLS_SSL_DEBUG_RET( 4, "ssl_hash_transcript_core", ret ); - return( ret ); - } MBEDTLS_SSL_DEBUG_BUF( 4, "Truncated SHA-256 handshake transcript", - hash_transcript, hash_olen ); + hash_transcript, hash_len ); #if defined(MBEDTLS_USE_PSA_CRYPTO) psa_hash_abort( &ssl->handshake->fin_sha256_psa ); @@ -1202,23 +1179,13 @@ int mbedtls_ssl_reset_transcript_for_hrr( mbedtls_ssl_context *ssl ) #else mbedtls_sha256_starts( &ssl->handshake->fin_sha256, 0 ); #endif - ssl->handshake->update_checksum( ssl, hash_transcript, hash_olen ); #endif /* MBEDTLS_SHA256_C */ } else if( ciphersuite_info->mac == MBEDTLS_MD_SHA384 ) { #if defined(MBEDTLS_SHA384_C) - ret = ssl_hash_transcript_core( ssl, MBEDTLS_MD_SHA384, - hash_transcript, - sizeof( hash_transcript ), - &hash_olen ); - if( ret != 0 ) - { - MBEDTLS_SSL_DEBUG_RET( 4, "ssl_hash_transcript_core", ret ); - return( ret ); - } MBEDTLS_SSL_DEBUG_BUF( 4, "Truncated SHA-384 handshake transcript", - hash_transcript, hash_olen ); + hash_transcript, hash_len ); #if defined(MBEDTLS_USE_PSA_CRYPTO) psa_hash_abort( &ssl->handshake->fin_sha384_psa ); @@ -1226,10 +1193,12 @@ int mbedtls_ssl_reset_transcript_for_hrr( mbedtls_ssl_context *ssl ) #else mbedtls_sha512_starts( &ssl->handshake->fin_sha512, 1 ); #endif - ssl->handshake->update_checksum( ssl, hash_transcript, hash_olen ); #endif /* MBEDTLS_SHA384_C */ } +#if defined(MBEDTLS_SHA256_C) || defined(MBEDTLS_SHA384_C) + ssl->handshake->update_checksum( ssl, hash_transcript, hash_len ); +#endif /* MBEDTLS_SHA256_C || MBEDTLS_SHA384_C */ return( ret ); } From d59be77ce7879a0b236c1ba49b5643e89f44d535 Mon Sep 17 00:00:00 2001 From: XiaokangQian Date: Mon, 24 Jan 2022 10:12:51 +0000 Subject: [PATCH 17/24] Refine code based on comments Add comments for parse hrr key share and cookie Change variable names based on RFC8466 Refine fatal allerts in parse server hello and hrr Signed-off-by: XiaokangQian --- library/ssl_tls13_client.c | 114 ++++++++++++++++++++++++------------- 1 file changed, 74 insertions(+), 40 deletions(-) diff --git a/library/ssl_tls13_client.c b/library/ssl_tls13_client.c index 6168ddd35..785a58c08 100644 --- a/library/ssl_tls13_client.c +++ b/library/ssl_tls13_client.c @@ -393,13 +393,21 @@ static int ssl_tls13_read_public_ecdhe_share( mbedtls_ssl_context *ssl, } #endif /* MBEDTLS_ECDH_C */ +/* + * ssl_tls13_parse_hrr_key_share_ext() + * Parse key_share extension in Hello Retry Request + * + * struct { + * NamedGroup selected_group; + * } KeyShareHelloRetryRequest; + */ static int ssl_tls13_parse_hrr_key_share_ext( mbedtls_ssl_context *ssl, const unsigned char *buf, const unsigned char *end ) { const mbedtls_ecp_curve_info *curve_info = NULL; const unsigned char *p = buf; - int tls_id; + int selected_group; int found = 0; const uint16_t *group_list = mbedtls_ssl_get_groups( ssl ); @@ -410,8 +418,8 @@ static int ssl_tls13_parse_hrr_key_share_ext( mbedtls_ssl_context *ssl, /* Read selected_group */ MBEDTLS_SSL_CHK_BUF_READ_PTR( p, end, 2 ); - tls_id = MBEDTLS_GET_UINT16_BE( p, 0 ); - MBEDTLS_SSL_DEBUG_MSG( 3, ( "selected_group ( %d )", tls_id ) ); + selected_group = MBEDTLS_GET_UINT16_BE( p, 0 ); + MBEDTLS_SSL_DEBUG_MSG( 3, ( "selected_group ( %d )", selected_group ) ); /* Upon receipt of this extension in a HelloRetryRequest, the client * MUST first verify that the selected_group field corresponds to a @@ -425,7 +433,7 @@ static int ssl_tls13_parse_hrr_key_share_ext( mbedtls_ssl_context *ssl, for( ; *group_list != 0; group_list++ ) { curve_info = mbedtls_ecp_curve_info_from_tls_id( *group_list ); - if( curve_info == NULL || curve_info->tls_id != tls_id ) + if( curve_info == NULL || curve_info->tls_id != selected_group ) continue; /* We found a match */ @@ -440,7 +448,7 @@ static int ssl_tls13_parse_hrr_key_share_ext( mbedtls_ssl_context *ssl, * ClientHello then the client MUST abort the handshake with * an "illegal_parameter" alert. */ - if( found == 0 || tls_id == ssl->handshake->offered_group_id ) + if( found == 0 || selected_group == ssl->handshake->offered_group_id ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "Invalid key share in HRR" ) ); MBEDTLS_SSL_PEND_FATAL_ALERT( @@ -450,7 +458,7 @@ static int ssl_tls13_parse_hrr_key_share_ext( mbedtls_ssl_context *ssl, } /* Remember server's preference for next ClientHello */ - ssl->handshake->offered_group_id = tls_id; + ssl->handshake->offered_group_id = selected_group; return( 0 ); } @@ -519,6 +527,22 @@ static int ssl_tls13_parse_key_share_ext( mbedtls_ssl_context *ssl, #endif /* MBEDTLS_KEY_EXCHANGE_WITH_CERT_ENABLED */ #if defined(MBEDTLS_SSL_COOKIE_C) +/* + * ssl_tls13_parse_cookie_ext() + * Parse cookie extension in Hello Retry Request + * + * struct { + * opaque cookie<1..2^16-1>; + * } Cookie; + * + * When sending a HelloRetryRequest, the server MAY provide a "cookie" + * extension to the client (this is an exception to the usual rule that + * the only extensions that may be sent are those that appear in the + * ClientHello). When sending the new ClientHello, the client MUST copy + * the contents of the extension received in the HelloRetryRequest into + * a "cookie" extension in the new ClientHello. Clients MUST NOT use + * cookies in their initial ClientHello in subsequent connections. + */ static int ssl_tls13_parse_cookie_ext( mbedtls_ssl_context *ssl, const unsigned char *buf, const unsigned char *end ) @@ -1028,6 +1052,8 @@ static int ssl_tls13_cipher_suite_is_offered( mbedtls_ssl_context *ssl, * Extension extensions<6..2^16-1>; * } ServerHello; */ +#define ALERT_UNSUPPORTED_EXTENSION_FOUND ( 1 << 0 ) +#define ALERT_ILLEGAL_PARAMETER_FOUND ( 1 << 1 ) static int ssl_tls13_parse_server_hello( mbedtls_ssl_context *ssl, const unsigned char *buf, const unsigned char *end, @@ -1041,6 +1067,7 @@ static int ssl_tls13_parse_server_hello( mbedtls_ssl_context *ssl, uint16_t cipher_suite; const mbedtls_ssl_ciphersuite_t *ciphersuite_info; int supported_versions_ext_found = 0; + int fatal_alert_found = 0; /* * Check there is space for minimal fields @@ -1068,7 +1095,8 @@ static int ssl_tls13_parse_server_hello( mbedtls_ssl_context *ssl, MBEDTLS_SSL_DEBUG_MSG( 1, ( "Unsupported version of TLS." ) ); MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_PROTOCOL_VERSION, MBEDTLS_ERR_SSL_BAD_PROTOCOL_VERSION ); - return( MBEDTLS_ERR_SSL_BAD_PROTOCOL_VERSION ); + ret = MBEDTLS_ERR_SSL_BAD_PROTOCOL_VERSION; + goto cleanup; } p += 2; @@ -1093,9 +1121,8 @@ static int ssl_tls13_parse_server_hello( mbedtls_ssl_context *ssl, */ if( ssl_tls13_check_server_hello_session_id_echo( ssl, &p, end ) != 0 ) { - MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_ILLEGAL_PARAMETER, - MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER ); - return( MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER ); + fatal_alert_found |= ALERT_ILLEGAL_PARAMETER_FOUND; + goto cleanup; } /* ... @@ -1121,7 +1148,6 @@ static int ssl_tls13_parse_server_hello( mbedtls_ssl_context *ssl, ret = MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER; } /* - * Check whether this ciphersuite is the same with what we received in HRR. * If we received an HRR before and that the proposed selected * ciphersuite in this server hello is not the same as the one * proposed in the HRR, we abort the handshake and send an @@ -1137,9 +1163,8 @@ static int ssl_tls13_parse_server_hello( mbedtls_ssl_context *ssl, { MBEDTLS_SSL_DEBUG_MSG( 1, ( "invalid ciphersuite(%04x) parameter", cipher_suite ) ); - MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_ILLEGAL_PARAMETER, - MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER ); - return( MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER ); + fatal_alert_found |= ALERT_ILLEGAL_PARAMETER_FOUND; + goto cleanup; } /* Configure ciphersuites */ @@ -1163,9 +1188,8 @@ static int ssl_tls13_parse_server_hello( mbedtls_ssl_context *ssl, if( p[0] != 0 ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad legacy compression method" ) ); - MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_ILLEGAL_PARAMETER, - MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER ); - return( MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER ); + fatal_alert_found |= ALERT_ILLEGAL_PARAMETER_FOUND; + goto cleanup; } p++; @@ -1208,10 +1232,8 @@ static int ssl_tls13_parse_server_hello( mbedtls_ssl_context *ssl, if( !is_hrr ) { - MBEDTLS_SSL_PEND_FATAL_ALERT( - MBEDTLS_SSL_ALERT_MSG_UNSUPPORTED_EXT, - MBEDTLS_ERR_SSL_UNSUPPORTED_EXTENSION ); - return( MBEDTLS_ERR_SSL_UNSUPPORTED_EXTENSION ); + fatal_alert_found |= ALERT_UNSUPPORTED_EXTENSION_FOUND; + goto cleanup; } ret = ssl_tls13_parse_cookie_ext( ssl, @@ -1221,7 +1243,7 @@ static int ssl_tls13_parse_server_hello( mbedtls_ssl_context *ssl, MBEDTLS_SSL_DEBUG_RET( 1, "ssl_tls13_parse_cookie_ext", ret ); - return( ret ); + goto cleanup; } break; #endif /* MBEDTLS_SSL_COOKIE_C */ @@ -1235,21 +1257,25 @@ static int ssl_tls13_parse_server_hello( mbedtls_ssl_context *ssl, p, extension_data_end ); if( ret != 0 ) - return( ret ); + goto cleanup; break; case MBEDTLS_TLS_EXT_PRE_SHARED_KEY: MBEDTLS_SSL_DEBUG_MSG( 3, ( "found pre_shared_key extension." ) ); MBEDTLS_SSL_DEBUG_MSG( 3, ( "pre_shared_key:Not supported yet" ) ); - MBEDTLS_SSL_PEND_FATAL_ALERT( - MBEDTLS_SSL_ALERT_MSG_UNSUPPORTED_EXT, - MBEDTLS_ERR_SSL_UNSUPPORTED_EXTENSION ); - return( MBEDTLS_ERR_SSL_UNSUPPORTED_EXTENSION ); + fatal_alert_found |= ALERT_UNSUPPORTED_EXTENSION_FOUND; + goto cleanup; #if defined(MBEDTLS_KEY_EXCHANGE_WITH_CERT_ENABLED) case MBEDTLS_TLS_EXT_KEY_SHARE: MBEDTLS_SSL_DEBUG_MSG( 3, ( "found key_shares extension" ) ); + if( ! mbedtls_ssl_conf_tls13_some_ephemeral_enabled( ssl ) ) + { + fatal_alert_found |= ALERT_UNSUPPORTED_EXTENSION_FOUND; + goto cleanup; + } + if( is_hrr ) ret = ssl_tls13_parse_hrr_key_share_ext( ssl, p, extension_data_end ); @@ -1261,7 +1287,7 @@ static int ssl_tls13_parse_server_hello( mbedtls_ssl_context *ssl, MBEDTLS_SSL_DEBUG_RET( 1, "ssl_tls13_parse_key_share_ext", ret ); - return( ret ); + goto cleanup; } break; #endif /* MBEDTLS_KEY_EXCHANGE_WITH_CERT_ENABLED */ @@ -1272,10 +1298,8 @@ static int ssl_tls13_parse_server_hello( mbedtls_ssl_context *ssl, ( "unknown extension found: %u ( ignoring )", extension_type ) ); - MBEDTLS_SSL_PEND_FATAL_ALERT( - MBEDTLS_SSL_ALERT_MSG_UNSUPPORTED_EXT, - MBEDTLS_ERR_SSL_UNSUPPORTED_EXTENSION ); - return( MBEDTLS_ERR_SSL_UNSUPPORTED_EXTENSION ); + fatal_alert_found |= ALERT_UNSUPPORTED_EXTENSION_FOUND; + goto cleanup; } p += extension_data_len; @@ -1284,12 +1308,25 @@ static int ssl_tls13_parse_server_hello( mbedtls_ssl_context *ssl, if( !supported_versions_ext_found ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "supported_versions not found" ) ); - MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_ILLEGAL_PARAMETER, - MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER ); - return( MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER ); + fatal_alert_found |= ALERT_ILLEGAL_PARAMETER_FOUND; + goto cleanup; } - return( 0 ); +cleanup: + + if( fatal_alert_found & ALERT_UNSUPPORTED_EXTENSION_FOUND ) + { + MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_UNSUPPORTED_EXT, + MBEDTLS_ERR_SSL_UNSUPPORTED_EXTENSION ); + ret = MBEDTLS_ERR_SSL_UNSUPPORTED_EXTENSION; + } + else if ( fatal_alert_found & ALERT_ILLEGAL_PARAMETER_FOUND ) + { + MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_ILLEGAL_PARAMETER, + MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER ); + ret = MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER; + } + return( ret ); } static int ssl_tls13_postprocess_server_hello( mbedtls_ssl_context *ssl ) @@ -1466,10 +1503,7 @@ static int ssl_tls13_process_server_hello( mbedtls_ssl_context *ssl ) goto cleanup; else is_hrr = ( ret == SSL_SERVER_HELLO_COORDINATE_HRR ); - /* Parsing step - * We know what message to expect by now and call - * the respective parsing function. - */ + MBEDTLS_SSL_PROC_CHK( ssl_tls13_parse_server_hello( ssl, buf, buf + buf_len, is_hrr ) ); From b119a35d07a90697427a9b5e38cfdfc721f98426 Mon Sep 17 00:00:00 2001 From: XiaokangQian Date: Wed, 26 Jan 2022 03:29:10 +0000 Subject: [PATCH 18/24] Refine fatal alert in parse_server_hello Signed-off-by: XiaokangQian --- library/ssl_tls13_client.c | 29 +++++++++++++---------------- 1 file changed, 13 insertions(+), 16 deletions(-) diff --git a/library/ssl_tls13_client.c b/library/ssl_tls13_client.c index 785a58c08..5e6a747a8 100644 --- a/library/ssl_tls13_client.c +++ b/library/ssl_tls13_client.c @@ -1052,8 +1052,6 @@ static int ssl_tls13_cipher_suite_is_offered( mbedtls_ssl_context *ssl, * Extension extensions<6..2^16-1>; * } ServerHello; */ -#define ALERT_UNSUPPORTED_EXTENSION_FOUND ( 1 << 0 ) -#define ALERT_ILLEGAL_PARAMETER_FOUND ( 1 << 1 ) static int ssl_tls13_parse_server_hello( mbedtls_ssl_context *ssl, const unsigned char *buf, const unsigned char *end, @@ -1067,7 +1065,7 @@ static int ssl_tls13_parse_server_hello( mbedtls_ssl_context *ssl, uint16_t cipher_suite; const mbedtls_ssl_ciphersuite_t *ciphersuite_info; int supported_versions_ext_found = 0; - int fatal_alert_found = 0; + int fatal_alert = 0; /* * Check there is space for minimal fields @@ -1121,7 +1119,7 @@ static int ssl_tls13_parse_server_hello( mbedtls_ssl_context *ssl, */ if( ssl_tls13_check_server_hello_session_id_echo( ssl, &p, end ) != 0 ) { - fatal_alert_found |= ALERT_ILLEGAL_PARAMETER_FOUND; + fatal_alert = MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER; goto cleanup; } @@ -1145,7 +1143,7 @@ static int ssl_tls13_parse_server_hello( mbedtls_ssl_context *ssl, if( ciphersuite_info == NULL || ssl_tls13_cipher_suite_is_offered( ssl, cipher_suite ) == 0 ) { - ret = MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER; + fatal_alert = MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER; } /* * If we received an HRR before and that the proposed selected @@ -1156,14 +1154,13 @@ static int ssl_tls13_parse_server_hello( mbedtls_ssl_context *ssl, else if( ( !is_hrr ) && ( handshake->hello_retry_request_count > 0 ) && ( cipher_suite != ssl->session_negotiate->ciphersuite ) ) { - ret = MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER; + fatal_alert = MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER; } - if( ret == MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER ) + if( fatal_alert == MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "invalid ciphersuite(%04x) parameter", cipher_suite ) ); - fatal_alert_found |= ALERT_ILLEGAL_PARAMETER_FOUND; goto cleanup; } @@ -1188,7 +1185,7 @@ static int ssl_tls13_parse_server_hello( mbedtls_ssl_context *ssl, if( p[0] != 0 ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad legacy compression method" ) ); - fatal_alert_found |= ALERT_ILLEGAL_PARAMETER_FOUND; + fatal_alert = MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER; goto cleanup; } p++; @@ -1232,7 +1229,7 @@ static int ssl_tls13_parse_server_hello( mbedtls_ssl_context *ssl, if( !is_hrr ) { - fatal_alert_found |= ALERT_UNSUPPORTED_EXTENSION_FOUND; + fatal_alert = MBEDTLS_ERR_SSL_UNSUPPORTED_EXTENSION; goto cleanup; } @@ -1264,7 +1261,7 @@ static int ssl_tls13_parse_server_hello( mbedtls_ssl_context *ssl, MBEDTLS_SSL_DEBUG_MSG( 3, ( "found pre_shared_key extension." ) ); MBEDTLS_SSL_DEBUG_MSG( 3, ( "pre_shared_key:Not supported yet" ) ); - fatal_alert_found |= ALERT_UNSUPPORTED_EXTENSION_FOUND; + fatal_alert = MBEDTLS_ERR_SSL_UNSUPPORTED_EXTENSION; goto cleanup; #if defined(MBEDTLS_KEY_EXCHANGE_WITH_CERT_ENABLED) @@ -1272,7 +1269,7 @@ static int ssl_tls13_parse_server_hello( mbedtls_ssl_context *ssl, MBEDTLS_SSL_DEBUG_MSG( 3, ( "found key_shares extension" ) ); if( ! mbedtls_ssl_conf_tls13_some_ephemeral_enabled( ssl ) ) { - fatal_alert_found |= ALERT_UNSUPPORTED_EXTENSION_FOUND; + fatal_alert = MBEDTLS_ERR_SSL_UNSUPPORTED_EXTENSION; goto cleanup; } @@ -1298,7 +1295,7 @@ static int ssl_tls13_parse_server_hello( mbedtls_ssl_context *ssl, ( "unknown extension found: %u ( ignoring )", extension_type ) ); - fatal_alert_found |= ALERT_UNSUPPORTED_EXTENSION_FOUND; + fatal_alert = MBEDTLS_ERR_SSL_UNSUPPORTED_EXTENSION; goto cleanup; } @@ -1308,19 +1305,19 @@ static int ssl_tls13_parse_server_hello( mbedtls_ssl_context *ssl, if( !supported_versions_ext_found ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "supported_versions not found" ) ); - fatal_alert_found |= ALERT_ILLEGAL_PARAMETER_FOUND; + fatal_alert = MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER; goto cleanup; } cleanup: - if( fatal_alert_found & ALERT_UNSUPPORTED_EXTENSION_FOUND ) + if( fatal_alert & MBEDTLS_ERR_SSL_UNSUPPORTED_EXTENSION ) { MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_UNSUPPORTED_EXT, MBEDTLS_ERR_SSL_UNSUPPORTED_EXTENSION ); ret = MBEDTLS_ERR_SSL_UNSUPPORTED_EXTENSION; } - else if ( fatal_alert_found & ALERT_ILLEGAL_PARAMETER_FOUND ) + else if ( fatal_alert & MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER ) { MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_ILLEGAL_PARAMETER, MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER ); From 7bae3b616c1ba9a3d4d4fbc17f0ebc5aafa46f15 Mon Sep 17 00:00:00 2001 From: XiaokangQian Date: Wed, 26 Jan 2022 06:31:39 +0000 Subject: [PATCH 19/24] Add more ciphersuites into test cases for hrr Signed-off-by: XiaokangQian --- tests/ssl-opt.sh | 35 +++++++++++++++++++++++++++++++++-- 1 file changed, 33 insertions(+), 2 deletions(-) diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index e6e94b386..bf0eb7805 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -9214,7 +9214,22 @@ requires_config_enabled MBEDTLS_DEBUG_C requires_config_enabled MBEDTLS_SSL_CLI_C requires_config_disabled MBEDTLS_USE_PSA_CRYPTO requires_openssl_tls1_3 -run_test "TLS 1.3: HelloRetryRequest check - openssl" \ +run_test "TLS 1.3: HelloRetryRequest check, ciphersuite TLS_AES_128_GCM_SHA256 - openssl" \ + "$O_NEXT_SRV -ciphersuites TLS_AES_128_GCM_SHA256 -sigalgs ecdsa_secp256r1_sha256 -groups P-256 -msg -tls1_3 -num_tickets 0 -no_resume_ephemeral -no_cache" \ + "$P_CLI debug_level=4 force_version=tls13" \ + 0 \ + -c "received HelloRetryRequest message" \ + -c "<= ssl_tls13_process_server_hello:is_hrr = 1" \ + -c "tls13 client state: MBEDTLS_SSL_CLIENT_HELLO(1)" \ + -c "HTTP/1.0 200 ok" + +requires_config_enabled MBEDTLS_SSL_PROTO_TLS1_3 +requires_config_enabled MBEDTLS_SSL_TLS1_3_COMPATIBILITY_MODE +requires_config_enabled MBEDTLS_DEBUG_C +requires_config_enabled MBEDTLS_SSL_CLI_C +requires_config_disabled MBEDTLS_USE_PSA_CRYPTO +requires_openssl_tls1_3 +run_test "TLS 1.3: HelloRetryRequest check, ciphersuite TLS_AES_256_GCM_SHA384 - openssl" \ "$O_NEXT_SRV -ciphersuites TLS_AES_256_GCM_SHA384 -sigalgs ecdsa_secp256r1_sha256 -groups P-256 -msg -tls1_3 -num_tickets 0 -no_resume_ephemeral -no_cache" \ "$P_CLI debug_level=4 force_version=tls13" \ 0 \ @@ -9230,7 +9245,23 @@ requires_config_enabled MBEDTLS_SSL_TLS1_3_COMPATIBILITY_MODE requires_config_enabled MBEDTLS_DEBUG_C requires_config_enabled MBEDTLS_SSL_CLI_C requires_config_disabled MBEDTLS_USE_PSA_CRYPTO -run_test "TLS 1.3: HelloRetryRequest check - gnutls" \ +run_test "TLS 1.3: HelloRetryRequest check, ciphersuite TLS_AES_128_GCM_SHA256 - gnutls" \ + "$G_NEXT_SRV -d 4 --priority=NONE:+GROUP-SECP256R1:+AES-128-GCM:+SHA256:+AEAD:+SIGN-ECDSA-SECP256R1-SHA256:+VERS-TLS1.3:%NO_TICKETS --disable-client-cert" \ + "$P_CLI debug_level=4 force_version=tls13" \ + 0 \ + -c "received HelloRetryRequest message" \ + -c "<= ssl_tls13_process_server_hello:is_hrr = 1" \ + -c "tls13 client state: MBEDTLS_SSL_CLIENT_HELLO(1)" \ + -c "HTTP/1.0 200 OK" + +requires_gnutls_tls1_3 +requires_gnutls_next_no_ticket +requires_config_enabled MBEDTLS_SSL_PROTO_TLS1_3 +requires_config_enabled MBEDTLS_SSL_TLS1_3_COMPATIBILITY_MODE +requires_config_enabled MBEDTLS_DEBUG_C +requires_config_enabled MBEDTLS_SSL_CLI_C +requires_config_disabled MBEDTLS_USE_PSA_CRYPTO +run_test "TLS 1.3: HelloRetryRequest check, ciphersuite TLS_AES_256_GCM_SHA384 - gnutls" \ "$G_NEXT_SRV -d 4 --priority=NONE:+GROUP-SECP256R1:+AES-256-GCM:+SHA384:+AEAD:+SIGN-ECDSA-SECP256R1-SHA256:+VERS-TLS1.3:%NO_TICKETS --disable-client-cert" \ "$P_CLI debug_level=4 force_version=tls13" \ 0 \ From aec1f3e913b7ad68299bc9a2ca584885f7a49dd3 Mon Sep 17 00:00:00 2001 From: XiaokangQian Date: Wed, 26 Jan 2022 06:57:00 +0000 Subject: [PATCH 20/24] Cookie fields are used only by DTLS 1.3 Signed-off-by: XiaokangQian --- library/ssl_tls13_client.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/library/ssl_tls13_client.c b/library/ssl_tls13_client.c index 5e6a747a8..6bcbad036 100644 --- a/library/ssl_tls13_client.c +++ b/library/ssl_tls13_client.c @@ -559,6 +559,7 @@ static int ssl_tls13_parse_cookie_ext( mbedtls_ssl_context *ssl, MBEDTLS_SSL_CHK_BUF_READ_PTR( p, end, cookie_len ); MBEDTLS_SSL_DEBUG_BUF( 3, "cookie extension", p, cookie_len ); +#if defined(MBEDTLS_SSL_PROTO_DTLS) mbedtls_free( handshake->verify_cookie ); handshake->verify_cookie = mbedtls_calloc( 1, cookie_len ); if( handshake->verify_cookie == NULL ) @@ -571,6 +572,7 @@ static int ssl_tls13_parse_cookie_ext( mbedtls_ssl_context *ssl, memcpy( handshake->verify_cookie, p, cookie_len ); handshake->verify_cookie_len = (unsigned char) cookie_len; +#endif /* MBEDTLS_SSL_PROTO_DTLS */ return( 0 ); } @@ -1226,6 +1228,13 @@ static int ssl_tls13_parse_server_hello( mbedtls_ssl_context *ssl, { #if defined(MBEDTLS_SSL_COOKIE_C) case MBEDTLS_TLS_EXT_COOKIE: + /* + * Currently, we only support the cookies in DTLS 1.3. + */ +#if !defined(MBEDTLS_SSL_PROTO_DTLS) + fatal_alert = MBEDTLS_ERR_SSL_UNSUPPORTED_EXTENSION; + goto cleanup; +#else if( !is_hrr ) { @@ -1242,6 +1251,7 @@ static int ssl_tls13_parse_server_hello( mbedtls_ssl_context *ssl, ret ); goto cleanup; } +#endif /* MBEDTLS_SSL_PROTO_DTLS */ break; #endif /* MBEDTLS_SSL_COOKIE_C */ From 52da558103b8dae5a24261ae0741865ba5e9ded1 Mon Sep 17 00:00:00 2001 From: XiaokangQian Date: Wed, 26 Jan 2022 09:49:29 +0000 Subject: [PATCH 21/24] Change code base on comments Align the alert type in parse_server_hello Remove MBEDTLS_SSL_COOKIE_C guard Enable cookie for both DTLS and TLS1.3 Signed-off-by: XiaokangQian --- library/ssl_misc.h | 10 ++++++---- library/ssl_tls13_client.c | 38 ++++++++++++-------------------------- 2 files changed, 18 insertions(+), 30 deletions(-) diff --git a/library/ssl_misc.h b/library/ssl_misc.h index b07ff3644..7bd31186b 100644 --- a/library/ssl_misc.h +++ b/library/ssl_misc.h @@ -692,14 +692,16 @@ struct mbedtls_ssl_handshake_params } buffering; -#if defined(MBEDTLS_SSL_PROTO_DTLS) - unsigned int out_msg_seq; /*!< Outgoing handshake sequence number */ - unsigned int in_msg_seq; /*!< Incoming handshake sequence number */ - +#if defined(MBEDTLS_SSL_PROTO_DTLS) || defined(MBEDTLS_SSL_PROTO_TLS1_3) unsigned char *verify_cookie; /*!< Cli: HelloVerifyRequest cookie Srv: unused */ unsigned char verify_cookie_len; /*!< Cli: cookie length Srv: flag for sending a cookie */ +#endif /* MBEDTLS_SSL_PROTO_DTLS || MBEDTLS_SSL_PROTO_TLS1_3 */ + +#if defined(MBEDTLS_SSL_PROTO_DTLS) + unsigned int out_msg_seq; /*!< Outgoing handshake sequence number */ + unsigned int in_msg_seq; /*!< Incoming handshake sequence number */ uint32_t retransmit_timeout; /*!< Current value of timeout */ mbedtls_ssl_flight_item *flight; /*!< Current outgoing flight */ diff --git a/library/ssl_tls13_client.c b/library/ssl_tls13_client.c index 6bcbad036..b8f4bce06 100644 --- a/library/ssl_tls13_client.c +++ b/library/ssl_tls13_client.c @@ -526,7 +526,6 @@ static int ssl_tls13_parse_key_share_ext( mbedtls_ssl_context *ssl, #endif /* MBEDTLS_KEY_EXCHANGE_WITH_CERT_ENABLED */ -#if defined(MBEDTLS_SSL_COOKIE_C) /* * ssl_tls13_parse_cookie_ext() * Parse cookie extension in Hello Retry Request @@ -559,7 +558,6 @@ static int ssl_tls13_parse_cookie_ext( mbedtls_ssl_context *ssl, MBEDTLS_SSL_CHK_BUF_READ_PTR( p, end, cookie_len ); MBEDTLS_SSL_DEBUG_BUF( 3, "cookie extension", p, cookie_len ); -#if defined(MBEDTLS_SSL_PROTO_DTLS) mbedtls_free( handshake->verify_cookie ); handshake->verify_cookie = mbedtls_calloc( 1, cookie_len ); if( handshake->verify_cookie == NULL ) @@ -572,11 +570,9 @@ static int ssl_tls13_parse_cookie_ext( mbedtls_ssl_context *ssl, memcpy( handshake->verify_cookie, p, cookie_len ); handshake->verify_cookie_len = (unsigned char) cookie_len; -#endif /* MBEDTLS_SSL_PROTO_DTLS */ return( 0 ); } -#endif /* MBEDTLS_SSL_COOKIE_C */ /* Write cipher_suites * CipherSuite cipher_suites<2..2^16-2>; @@ -1121,7 +1117,7 @@ static int ssl_tls13_parse_server_hello( mbedtls_ssl_context *ssl, */ if( ssl_tls13_check_server_hello_session_id_echo( ssl, &p, end ) != 0 ) { - fatal_alert = MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER; + fatal_alert = MBEDTLS_SSL_ALERT_MSG_ILLEGAL_PARAMETER; goto cleanup; } @@ -1145,7 +1141,7 @@ static int ssl_tls13_parse_server_hello( mbedtls_ssl_context *ssl, if( ciphersuite_info == NULL || ssl_tls13_cipher_suite_is_offered( ssl, cipher_suite ) == 0 ) { - fatal_alert = MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER; + fatal_alert = MBEDTLS_SSL_ALERT_MSG_ILLEGAL_PARAMETER; } /* * If we received an HRR before and that the proposed selected @@ -1156,10 +1152,10 @@ static int ssl_tls13_parse_server_hello( mbedtls_ssl_context *ssl, else if( ( !is_hrr ) && ( handshake->hello_retry_request_count > 0 ) && ( cipher_suite != ssl->session_negotiate->ciphersuite ) ) { - fatal_alert = MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER; + fatal_alert = MBEDTLS_SSL_ALERT_MSG_ILLEGAL_PARAMETER; } - if( fatal_alert == MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER ) + if( fatal_alert == MBEDTLS_SSL_ALERT_MSG_ILLEGAL_PARAMETER ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "invalid ciphersuite(%04x) parameter", cipher_suite ) ); @@ -1187,7 +1183,7 @@ static int ssl_tls13_parse_server_hello( mbedtls_ssl_context *ssl, if( p[0] != 0 ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad legacy compression method" ) ); - fatal_alert = MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER; + fatal_alert = MBEDTLS_SSL_ALERT_MSG_ILLEGAL_PARAMETER; goto cleanup; } p++; @@ -1226,19 +1222,11 @@ static int ssl_tls13_parse_server_hello( mbedtls_ssl_context *ssl, switch( extension_type ) { -#if defined(MBEDTLS_SSL_COOKIE_C) case MBEDTLS_TLS_EXT_COOKIE: - /* - * Currently, we only support the cookies in DTLS 1.3. - */ -#if !defined(MBEDTLS_SSL_PROTO_DTLS) - fatal_alert = MBEDTLS_ERR_SSL_UNSUPPORTED_EXTENSION; - goto cleanup; -#else if( !is_hrr ) { - fatal_alert = MBEDTLS_ERR_SSL_UNSUPPORTED_EXTENSION; + fatal_alert = MBEDTLS_SSL_ALERT_MSG_UNSUPPORTED_EXT; goto cleanup; } @@ -1251,9 +1239,7 @@ static int ssl_tls13_parse_server_hello( mbedtls_ssl_context *ssl, ret ); goto cleanup; } -#endif /* MBEDTLS_SSL_PROTO_DTLS */ break; -#endif /* MBEDTLS_SSL_COOKIE_C */ case MBEDTLS_TLS_EXT_SUPPORTED_VERSIONS: supported_versions_ext_found = 1; @@ -1271,7 +1257,7 @@ static int ssl_tls13_parse_server_hello( mbedtls_ssl_context *ssl, MBEDTLS_SSL_DEBUG_MSG( 3, ( "found pre_shared_key extension." ) ); MBEDTLS_SSL_DEBUG_MSG( 3, ( "pre_shared_key:Not supported yet" ) ); - fatal_alert = MBEDTLS_ERR_SSL_UNSUPPORTED_EXTENSION; + fatal_alert = MBEDTLS_SSL_ALERT_MSG_UNSUPPORTED_EXT; goto cleanup; #if defined(MBEDTLS_KEY_EXCHANGE_WITH_CERT_ENABLED) @@ -1279,7 +1265,7 @@ static int ssl_tls13_parse_server_hello( mbedtls_ssl_context *ssl, MBEDTLS_SSL_DEBUG_MSG( 3, ( "found key_shares extension" ) ); if( ! mbedtls_ssl_conf_tls13_some_ephemeral_enabled( ssl ) ) { - fatal_alert = MBEDTLS_ERR_SSL_UNSUPPORTED_EXTENSION; + fatal_alert = MBEDTLS_SSL_ALERT_MSG_UNSUPPORTED_EXT; goto cleanup; } @@ -1305,7 +1291,7 @@ static int ssl_tls13_parse_server_hello( mbedtls_ssl_context *ssl, ( "unknown extension found: %u ( ignoring )", extension_type ) ); - fatal_alert = MBEDTLS_ERR_SSL_UNSUPPORTED_EXTENSION; + fatal_alert = MBEDTLS_SSL_ALERT_MSG_UNSUPPORTED_EXT; goto cleanup; } @@ -1315,19 +1301,19 @@ static int ssl_tls13_parse_server_hello( mbedtls_ssl_context *ssl, if( !supported_versions_ext_found ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "supported_versions not found" ) ); - fatal_alert = MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER; + fatal_alert = MBEDTLS_SSL_ALERT_MSG_ILLEGAL_PARAMETER; goto cleanup; } cleanup: - if( fatal_alert & MBEDTLS_ERR_SSL_UNSUPPORTED_EXTENSION ) + if( fatal_alert == MBEDTLS_SSL_ALERT_MSG_UNSUPPORTED_EXT ) { MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_UNSUPPORTED_EXT, MBEDTLS_ERR_SSL_UNSUPPORTED_EXTENSION ); ret = MBEDTLS_ERR_SSL_UNSUPPORTED_EXTENSION; } - else if ( fatal_alert & MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER ) + else if ( fatal_alert == MBEDTLS_SSL_ALERT_MSG_ILLEGAL_PARAMETER ) { MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_ILLEGAL_PARAMETER, MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER ); From 34909746df56e55e28975ac1d27fdb434de0ab0b Mon Sep 17 00:00:00 2001 From: XiaokangQian Date: Thu, 27 Jan 2022 02:25:04 +0000 Subject: [PATCH 22/24] Change cookie free code and some comments Signed-off-by: XiaokangQian --- library/ssl_misc.h | 8 +++++--- library/ssl_tls.c | 4 ++-- library/ssl_tls13_client.c | 1 + 3 files changed, 8 insertions(+), 5 deletions(-) diff --git a/library/ssl_misc.h b/library/ssl_misc.h index 7bd31186b..0c43c795a 100644 --- a/library/ssl_misc.h +++ b/library/ssl_misc.h @@ -694,9 +694,11 @@ struct mbedtls_ssl_handshake_params #if defined(MBEDTLS_SSL_PROTO_DTLS) || defined(MBEDTLS_SSL_PROTO_TLS1_3) unsigned char *verify_cookie; /*!< Cli: HelloVerifyRequest cookie - Srv: unused */ - unsigned char verify_cookie_len; /*!< Cli: cookie length - Srv: flag for sending a cookie */ + * for dtls / tls 1.3 + * Srv: unused */ + unsigned char verify_cookie_len; /*!< Cli: cookie length for + * dtls / tls 1.3 + * Srv: flag for sending a cookie */ #endif /* MBEDTLS_SSL_PROTO_DTLS || MBEDTLS_SSL_PROTO_TLS1_3 */ #if defined(MBEDTLS_SSL_PROTO_DTLS) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 6f37201bf..82da11f06 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -5689,11 +5689,11 @@ void mbedtls_ssl_handshake_free( mbedtls_ssl_context *ssl ) mbedtls_pk_free( &handshake->peer_pubkey ); #endif /* MBEDTLS_X509_CRT_PARSE_C && !MBEDTLS_SSL_KEEP_PEER_CERTIFICATE */ -#if defined(MBEDTLS_SSL_PROTO_DTLS) +#if defined(MBEDTLS_SSL_PROTO_DTLS) || defined(MBEDTLS_SSL_PROTO_TLS1_3) mbedtls_free( handshake->verify_cookie ); mbedtls_ssl_flight_free( handshake->flight ); mbedtls_ssl_buffering_free( ssl ); -#endif +#endif /* MBEDTLS_SSL_PROTO_DTLS || MBEDTLS_SSL_PROTO_TLS1_3 */ #if defined(MBEDTLS_ECDH_C) && \ defined(MBEDTLS_USE_PSA_CRYPTO) diff --git a/library/ssl_tls13_client.c b/library/ssl_tls13_client.c index b8f4bce06..75978a8ed 100644 --- a/library/ssl_tls13_client.c +++ b/library/ssl_tls13_client.c @@ -559,6 +559,7 @@ static int ssl_tls13_parse_cookie_ext( mbedtls_ssl_context *ssl, MBEDTLS_SSL_DEBUG_BUF( 3, "cookie extension", p, cookie_len ); mbedtls_free( handshake->verify_cookie ); + handshake->verify_cookie_len = 0; handshake->verify_cookie = mbedtls_calloc( 1, cookie_len ); if( handshake->verify_cookie == NULL ) { From a909061c2a518e1636a60706a8f6e25e3497bfde Mon Sep 17 00:00:00 2001 From: XiaokangQian Date: Thu, 27 Jan 2022 03:48:27 +0000 Subject: [PATCH 23/24] Refine HRR parse successfully message in test cases Signed-off-by: XiaokangQian --- library/ssl_tls13_client.c | 3 ++- tests/ssl-opt.sh | 16 ++++++++-------- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/library/ssl_tls13_client.c b/library/ssl_tls13_client.c index 75978a8ed..ca91d67da 100644 --- a/library/ssl_tls13_client.c +++ b/library/ssl_tls13_client.c @@ -1514,7 +1514,8 @@ static int ssl_tls13_process_server_hello( mbedtls_ssl_context *ssl ) MBEDTLS_SSL_PROC_CHK( ssl_tls13_postprocess_server_hello( ssl ) ); cleanup: - MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= %s:is_hrr = %d", __func__, is_hrr ) ); + MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= %s ( %s )", __func__, + is_hrr?"HelloRetryRequest":"ServerHello" ) ); return( ret ); } diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index bf0eb7805..295b82ea8 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -9219,8 +9219,8 @@ run_test "TLS 1.3: HelloRetryRequest check, ciphersuite TLS_AES_128_GCM_SHA25 "$P_CLI debug_level=4 force_version=tls13" \ 0 \ -c "received HelloRetryRequest message" \ - -c "<= ssl_tls13_process_server_hello:is_hrr = 1" \ - -c "tls13 client state: MBEDTLS_SSL_CLIENT_HELLO(1)" \ + -c "<= ssl_tls13_process_server_hello ( HelloRetryRequest )" \ + -c "tls13 client state: MBEDTLS_SSL_CLIENT_HELLO" \ -c "HTTP/1.0 200 ok" requires_config_enabled MBEDTLS_SSL_PROTO_TLS1_3 @@ -9234,8 +9234,8 @@ run_test "TLS 1.3: HelloRetryRequest check, ciphersuite TLS_AES_256_GCM_SHA38 "$P_CLI debug_level=4 force_version=tls13" \ 0 \ -c "received HelloRetryRequest message" \ - -c "<= ssl_tls13_process_server_hello:is_hrr = 1" \ - -c "tls13 client state: MBEDTLS_SSL_CLIENT_HELLO(1)" \ + -c "<= ssl_tls13_process_server_hello ( HelloRetryRequest )" \ + -c "tls13 client state: MBEDTLS_SSL_CLIENT_HELLO" \ -c "HTTP/1.0 200 ok" requires_gnutls_tls1_3 @@ -9250,8 +9250,8 @@ run_test "TLS 1.3: HelloRetryRequest check, ciphersuite TLS_AES_128_GCM_SHA25 "$P_CLI debug_level=4 force_version=tls13" \ 0 \ -c "received HelloRetryRequest message" \ - -c "<= ssl_tls13_process_server_hello:is_hrr = 1" \ - -c "tls13 client state: MBEDTLS_SSL_CLIENT_HELLO(1)" \ + -c "<= ssl_tls13_process_server_hello ( HelloRetryRequest )" \ + -c "tls13 client state: MBEDTLS_SSL_CLIENT_HELLO" \ -c "HTTP/1.0 200 OK" requires_gnutls_tls1_3 @@ -9266,8 +9266,8 @@ run_test "TLS 1.3: HelloRetryRequest check, ciphersuite TLS_AES_256_GCM_SHA38 "$P_CLI debug_level=4 force_version=tls13" \ 0 \ -c "received HelloRetryRequest message" \ - -c "<= ssl_tls13_process_server_hello:is_hrr = 1" \ - -c "tls13 client state: MBEDTLS_SSL_CLIENT_HELLO(1)" \ + -c "<= ssl_tls13_process_server_hello ( HelloRetryRequest )" \ + -c "tls13 client state: MBEDTLS_SSL_CLIENT_HELLO" \ -c "HTTP/1.0 200 OK" for i in $(ls opt-testcases/*.sh) From 8499b6ce2574eae0bc0a8cab63ccff7b300545ba Mon Sep 17 00:00:00 2001 From: XiaokangQian Date: Thu, 27 Jan 2022 09:00:11 +0000 Subject: [PATCH 24/24] Only free verify_cookie in tls 1.3 case. Signed-off-by: XiaokangQian --- library/ssl_tls.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 82da11f06..f261a6a89 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -5691,9 +5691,12 @@ void mbedtls_ssl_handshake_free( mbedtls_ssl_context *ssl ) #if defined(MBEDTLS_SSL_PROTO_DTLS) || defined(MBEDTLS_SSL_PROTO_TLS1_3) mbedtls_free( handshake->verify_cookie ); +#endif /* MBEDTLS_SSL_PROTO_DTLS || MBEDTLS_SSL_PROTO_TLS1_3 */ + +#if defined(MBEDTLS_SSL_PROTO_DTLS) mbedtls_ssl_flight_free( handshake->flight ); mbedtls_ssl_buffering_free( ssl ); -#endif /* MBEDTLS_SSL_PROTO_DTLS || MBEDTLS_SSL_PROTO_TLS1_3 */ +#endif /* MBEDTLS_SSL_PROTO_DTLS */ #if defined(MBEDTLS_ECDH_C) && \ defined(MBEDTLS_USE_PSA_CRYPTO)