From 281fd1bdd8aacc3c68ea42e305a5ed2469635571 Mon Sep 17 00:00:00 2001 From: Xiaokang Qian Date: Tue, 20 Sep 2022 11:35:41 +0000 Subject: [PATCH 01/19] Add server name check when proposeing pre-share key Signed-off-by: Xiaokang Qian --- include/mbedtls/ssl.h | 26 ++++++++++++ library/ssl_client.c | 18 +++++++++ library/ssl_tls.c | 83 ++++++++++++++++++++++++++++++++++++++ library/ssl_tls13_server.c | 14 +++++++ programs/ssl/ssl_client2.c | 15 +++++++ tests/ssl-opt.sh | 41 +++++++++++++++++++ 6 files changed, 197 insertions(+) diff --git a/include/mbedtls/ssl.h b/include/mbedtls/ssl.h index 5a02182c0e..5e493f5f27 100644 --- a/include/mbedtls/ssl.h +++ b/include/mbedtls/ssl.h @@ -1201,6 +1201,11 @@ struct mbedtls_ssl_session #if defined(MBEDTLS_SSL_PROTO_TLS1_3) mbedtls_ssl_tls13_application_secrets MBEDTLS_PRIVATE(app_secrets); #endif +#if defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) + uint8_t MBEDTLS_PRIVATE(hostname_len); /*!< host_name length */ + char *MBEDTLS_PRIVATE(hostname); /*!< host name binded with tickets */ + uint8_t hostname_mismatch; /*!< whether new host_name match with saved one */ +#endif /* MBEDTLS_SSL_SERVER_NAME_INDICATION */ }; /* @@ -3662,6 +3667,27 @@ void mbedtls_ssl_conf_sig_algs( mbedtls_ssl_config *conf, * On too long input failure, old hostname is unchanged. */ int mbedtls_ssl_set_hostname( mbedtls_ssl_context *ssl, const char *hostname ); + +/** + * \brief Reset the hostname to the new server name when reconnection. + * + * \param ssl SSL context + * \param hostname the server hostname, may be NULL + * \param rec_hostname the server rec_hostname, may be NULL + + * \note Maximum hostname length MBEDTLS_SSL_MAX_HOST_NAME_LEN. + * + * \return 0 if successful, MBEDTLS_ERR_SSL_ALLOC_FAILED on + * allocation failure, MBEDTLS_ERR_SSL_BAD_INPUT_DATA on + * too long input rec_hostname. + * + * Rec_hostname set to the one provided on success. + * On allocation failure hostname is unchanged. + * On too long input failure, old hostname is unchanged. + */ +int mbedtls_ssl_reset_hostname( mbedtls_ssl_context *ssl, + const char *hostname, + const char *rec_hostname ); #endif /* MBEDTLS_X509_CRT_PARSE_C */ #if defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) diff --git a/library/ssl_client.c b/library/ssl_client.c index 1b591253f8..bd9edf15f0 100644 --- a/library/ssl_client.c +++ b/library/ssl_client.c @@ -54,6 +54,7 @@ static int ssl_write_hostname_ext( mbedtls_ssl_context *ssl, { unsigned char *p = buf; size_t hostname_len; + size_t cmp_hostname_len; *olen = 0; @@ -64,8 +65,25 @@ static int ssl_write_hostname_ext( mbedtls_ssl_context *ssl, ( "client hello, adding server name extension: %s", ssl->hostname ) ); + ssl->session_negotiate->hostname_mismatch = 0; hostname_len = strlen( ssl->hostname ); + cmp_hostname_len = hostname_len < ssl->session_negotiate->hostname_len ? + hostname_len : ssl->session_negotiate->hostname_len; + + if( hostname_len != ssl->session_negotiate->hostname_len || + memcmp( ssl->hostname, ssl->session_negotiate->hostname, cmp_hostname_len ) ) + ssl->session_negotiate->hostname_mismatch = 1; + + if( ssl->session_negotiate->hostname == NULL ) + { + ssl->session_negotiate->hostname = mbedtls_calloc( 1, hostname_len ); + if( ssl->session_negotiate->hostname == NULL ) + return MBEDTLS_ERR_SSL_ALLOC_FAILED; + memcpy(ssl->session_negotiate->hostname, ssl->hostname, hostname_len); + } + ssl->session_negotiate->hostname_len = hostname_len; + MBEDTLS_SSL_CHK_BUF_PTR( p, end, hostname_len + 9 ); /* diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 9741a6ef5f..9238761639 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -297,6 +297,18 @@ int mbedtls_ssl_session_copy( mbedtls_ssl_session *dst, } #endif /* MBEDTLS_SSL_SESSION_TICKETS && MBEDTLS_SSL_CLI_C */ +#if defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) + if( src->hostname != NULL ) + { + dst->hostname = mbedtls_calloc( 1, src->hostname_len + 1 ); + if( dst->hostname == NULL ) + return( MBEDTLS_ERR_SSL_ALLOC_FAILED ); + + memcpy( dst->hostname, src->hostname, src->hostname_len ); + dst->hostname[src->hostname_len] = '\0'; + } +#endif + return( 0 ); } @@ -1978,6 +1990,11 @@ static int ssl_tls13_session_save( const mbedtls_ssl_session *session, #if defined(MBEDTLS_SSL_CLI_C) if( session->endpoint == MBEDTLS_SSL_IS_CLIENT ) { +#if defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) + needed += 1 /* hostname_len */ + + session->hostname_len; /* hostname */ +#endif + needed += 4 /* ticket_lifetime */ + 2; /* ticket_len */ @@ -2004,6 +2021,19 @@ static int ssl_tls13_session_save( const mbedtls_ssl_session *session, memcpy( p, session->resumption_key, session->resumption_key_len ); p += session->resumption_key_len; +#if defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) && defined(MBEDTLS_SSL_CLI_C) + if( session->endpoint == MBEDTLS_SSL_IS_CLIENT && + session->hostname_len != 0 && + session->hostname != NULL ) + { + /* save host name */ + p[0] = session->hostname_len; + p++; + memcpy( p, session->hostname, session->hostname_len ); + p += session->hostname_len; + } +#endif /* MBEDTLS_SSL_SERVER_NAME_INDICATION && MBEDTLS_SSL_CLI_C */ + #if defined(MBEDTLS_HAVE_TIME) && defined(MBEDTLS_SSL_SRV_C) if( session->endpoint == MBEDTLS_SSL_IS_SERVER ) { @@ -2062,6 +2092,22 @@ static int ssl_tls13_session_load( mbedtls_ssl_session *session, memcpy( session->resumption_key, p, session->resumption_key_len ); p += session->resumption_key_len; +#if defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) && defined(MBEDTLS_SSL_CLI_C) + if( session->endpoint == MBEDTLS_SSL_IS_CLIENT ) + { + /* load host name */ + session->hostname_len = p[0]; + p += 1; + if( end - p < session->hostname_len ) + return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); + session->hostname = mbedtls_calloc( 1, session->hostname_len ); + if( session->hostname == NULL ) + return( MBEDTLS_ERR_SSL_ALLOC_FAILED ); + memcpy( session->hostname, p, session->hostname_len ); + p += session->hostname_len; + } +#endif /* MBEDTLS_SSL_SERVER_NAME_INDICATION && MBEDTLS_SSL_CLI_C */ + #if defined(MBEDTLS_HAVE_TIME) && defined(MBEDTLS_SSL_SRV_C) if( session->endpoint == MBEDTLS_SSL_IS_SERVER ) { @@ -2430,6 +2476,39 @@ int mbedtls_ssl_set_hostname( mbedtls_ssl_context *ssl, const char *hostname ) return( 0 ); } + +int mbedtls_ssl_reset_hostname( mbedtls_ssl_context *ssl, + const char *hostname, + const char *rec_hostname ) +{ + /* Initialize to suppress unnecessary compiler warning */ + size_t rec_hostname_len = 0; + + if( hostname == NULL || rec_hostname == NULL ) + return( 0 ); + + rec_hostname_len = strlen( rec_hostname ); + if( rec_hostname_len > MBEDTLS_SSL_MAX_HOST_NAME_LEN ) + return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); + + if( rec_hostname_len == strlen( hostname ) && + memcmp( hostname, rec_hostname, rec_hostname_len ) == 0 ) + return( 0 ); + + if( ssl->hostname != NULL ) + { + mbedtls_platform_zeroize( ssl->hostname, strlen( ssl->hostname ) ); + mbedtls_free( ssl->hostname ); + ssl->hostname = NULL; + ssl->hostname = mbedtls_calloc( 1, rec_hostname_len + 1 ); + if( ssl->hostname == NULL ) + return( MBEDTLS_ERR_SSL_ALLOC_FAILED ); + memcpy( ssl->hostname, rec_hostname, rec_hostname_len ); + ssl->hostname[rec_hostname_len] = '\0'; + } + + return( 0 ); +} #endif /* MBEDTLS_X509_CRT_PARSE_C */ #if defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) @@ -3679,6 +3758,10 @@ void mbedtls_ssl_session_free( mbedtls_ssl_session *session ) mbedtls_free( session->ticket ); #endif +#if defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) + mbedtls_free( session->hostname ); +#endif + mbedtls_platform_zeroize( session, sizeof( mbedtls_ssl_session ) ); } diff --git a/library/ssl_tls13_server.c b/library/ssl_tls13_server.c index 61a1bad578..60b1e05f43 100644 --- a/library/ssl_tls13_server.c +++ b/library/ssl_tls13_server.c @@ -235,6 +235,20 @@ static int ssl_tls13_offered_psks_check_identity_match_ticket( (int)age_diff_in_ms ) ); goto exit; } +#if 0 +#if defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) + if( ssl->session_negotiate->hostname != NULL && + ssl->session_negotiate->hostname_len != 0 && + memcmp( ssl->session_negotiate->hostname, + ssl->session->hostname, ssl->session->hostname_len ) != 0 ) + { + MBEDTLS_SSL_DEBUG_MSG( + 3, ( "Session hostname not matched with stored session ticket" ) ); + goto exit; + } + +#endif /* MBEDTLS_SSL_SERVER_NAME_INDICATION */ +#endif ret = 0; diff --git a/programs/ssl/ssl_client2.c b/programs/ssl/ssl_client2.c index 6377162b2d..bc93352707 100644 --- a/programs/ssl/ssl_client2.c +++ b/programs/ssl/ssl_client2.c @@ -366,6 +366,7 @@ int main( void ) "\n usage: ssl_client2 param=<>...\n" \ "\n acceptable parameters:\n" \ " server_name=%%s default: localhost\n" \ + " rec_server_name=%%s default: localhost\n" \ " server_addr=%%s default: given by name\n" \ " server_port=%%d default: 4433\n" \ " request_page=%%s default: \".\"\n" \ @@ -455,6 +456,7 @@ int main( void ) struct options { const char *server_name; /* hostname of the server (client only) */ + const char *rec_s_name; /* hostname of the server (re-connect) */ const char *server_addr; /* address of the server (client only) */ const char *server_port; /* port on which the ssl service runs */ int debug_level; /* level of debugging */ @@ -876,6 +878,7 @@ int main( int argc, char *argv[] ) } opt.server_name = DFL_SERVER_NAME; + opt.rec_s_name = DFL_SERVER_NAME; opt.server_addr = DFL_SERVER_ADDR; opt.server_port = DFL_SERVER_PORT; opt.debug_level = DFL_DEBUG_LEVEL; @@ -961,6 +964,8 @@ int main( int argc, char *argv[] ) if( strcmp( p, "server_name" ) == 0 ) opt.server_name = q; + else if( strcmp( p, "rec_server_name" ) == 0 ) + opt.rec_s_name = q; else if( strcmp( p, "server_addr" ) == 0 ) opt.server_addr = q; else if( strcmp( p, "server_port" ) == 0 ) @@ -3113,6 +3118,16 @@ reconnect: goto exit; } +#if defined(MBEDTLS_X509_CRT_PARSE_C) + if( ( ret = mbedtls_ssl_reset_hostname( &ssl, opt.server_name, + opt.rec_s_name ) ) != 0 ) + { + mbedtls_printf( " failed\n ! mbedtls_ssl_reset_hostname returned %d\n\n", + ret ); + goto exit; + } +#endif + if( ( ret = mbedtls_net_connect( &server_fd, opt.server_addr, opt.server_port, opt.transport == MBEDTLS_SSL_TRANSPORT_STREAM ? diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index 48dd89e357..414b8607bb 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -12873,6 +12873,47 @@ run_test "TLS 1.2: Check rsa_pss_rsae compatibility issue, m->G" \ -c "Protocol is TLSv1.2" \ -c "HTTP/1.0 200 [Oo][Kk]" +requires_config_enabled MBEDTLS_SSL_PROTO_TLS1_3 +requires_config_enabled MBEDTLS_SSL_SESSION_TICKETS +requires_config_enabled MBEDTLS_SSL_SRV_C +requires_config_enabled MBEDTLS_SSL_CLI_C +requires_config_enabled MBEDTLS_DEBUG_C +run_test "TLS 1.3: NewSessionTicket: servername check, m->m" \ + "$P_SRV debug_level=4 crt_file=data_files/server5.crt key_file=data_files/server5.key force_version=tls13 tickets=1 \ + sni=localhost,data_files/server2.crt,data_files/server2.key,-,-,-,polarssl.example,data_files/server1-nospace.crt,data_files/server1.key,-,-,-" \ + "$P_CLI debug_level=4 server_name=localhost reco_mode=1 reconnect=1" \ + 0 \ + -c "Protocol is TLSv1.3" \ + -c "got new session ticket." \ + -c "Saving session for reuse... ok" \ + -c "Reconnecting with saved session" \ + -c "HTTP/1.0 200 OK" \ + -s "=> write NewSessionTicket msg" \ + -s "server state: MBEDTLS_SSL_NEW_SESSION_TICKET" \ + -s "server state: MBEDTLS_SSL_NEW_SESSION_TICKET_FLUSH" \ + -s "key exchange mode: ephemeral" \ + -s "key exchange mode: psk_ephemeral" \ + -s "found pre_shared_key extension" + +requires_config_enabled MBEDTLS_SSL_PROTO_TLS1_3 +requires_config_enabled MBEDTLS_SSL_SESSION_TICKETS +requires_config_enabled MBEDTLS_SSL_SRV_C +requires_config_enabled MBEDTLS_SSL_CLI_C +requires_config_enabled MBEDTLS_DEBUG_C +run_test "TLS 1.3: NewSessionTicket: servername negative check, m->m" \ + "$P_SRV debug_level=4 crt_file=data_files/server5.crt key_file=data_files/server5.key force_version=tls13 tickets=1 \ + sni=localhost,data_files/server2.crt,data_files/server2.key,-,-,-,polarssl.example,data_files/server1-nospace.crt,data_files/server1.key,-,-,-" \ + "$P_CLI debug_level=4 server_name=localhost rec_server_name=remote reco_mode=1 reconnect=1" \ + 1 \ + -c "Protocol is TLSv1.3" \ + -c "got new session ticket." \ + -c "Saving session for reuse... ok" \ + -c "Reconnecting with saved session" \ + -c "hostname mismatch the session ticker, should not resume" \ + -s "=> write NewSessionTicket msg" \ + -s "server state: MBEDTLS_SSL_NEW_SESSION_TICKET" \ + -s "server state: MBEDTLS_SSL_NEW_SESSION_TICKET_FLUSH" + # Test heap memory usage after handshake requires_config_enabled MBEDTLS_SSL_PROTO_TLS1_2 requires_config_enabled MBEDTLS_MEMORY_DEBUG From ecd7528c7f0973752556b6c688a4cbe3042c4754 Mon Sep 17 00:00:00 2001 From: Xiaokang Qian Date: Wed, 28 Sep 2022 07:11:02 +0000 Subject: [PATCH 02/19] Address some comments Hostname_len has at least one byte Change structure serialized_session_tls13 Fix various issues Signed-off-by: Xiaokang Qian --- library/ssl_client.c | 2 +- library/ssl_tls.c | 23 +++++++++++++---------- 2 files changed, 14 insertions(+), 11 deletions(-) diff --git a/library/ssl_client.c b/library/ssl_client.c index bd9edf15f0..2c5f664945 100644 --- a/library/ssl_client.c +++ b/library/ssl_client.c @@ -79,7 +79,7 @@ static int ssl_write_hostname_ext( mbedtls_ssl_context *ssl, { ssl->session_negotiate->hostname = mbedtls_calloc( 1, hostname_len ); if( ssl->session_negotiate->hostname == NULL ) - return MBEDTLS_ERR_SSL_ALLOC_FAILED; + return( MBEDTLS_ERR_SSL_ALLOC_FAILED ); memcpy(ssl->session_negotiate->hostname, ssl->hostname, hostname_len); } ssl->session_negotiate->hostname_len = hostname_len; diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 9238761639..fa4e6930e5 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -300,12 +300,11 @@ int mbedtls_ssl_session_copy( mbedtls_ssl_session *dst, #if defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) if( src->hostname != NULL ) { - dst->hostname = mbedtls_calloc( 1, src->hostname_len + 1 ); + dst->hostname = mbedtls_calloc( 1, src->hostname_len ); if( dst->hostname == NULL ) return( MBEDTLS_ERR_SSL_ALLOC_FAILED ); memcpy( dst->hostname, src->hostname, src->hostname_len ); - dst->hostname[src->hostname_len] = '\0'; } #endif @@ -1957,6 +1956,7 @@ mbedtls_ssl_mode_t mbedtls_ssl_get_mode_from_ciphersuite( * uint32 ticket_age_add; * uint8 ticket_flags; * opaque resumption_key<0..255>; + * opaque hostname<1..2^8-1>; * select ( endpoint ) { * case client: ClientOnlyData; * case server: uint64 start_time; @@ -2021,14 +2021,14 @@ static int ssl_tls13_session_save( const mbedtls_ssl_session *session, memcpy( p, session->resumption_key, session->resumption_key_len ); p += session->resumption_key_len; + p[0] = session->hostname_len; + p++; #if defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) && defined(MBEDTLS_SSL_CLI_C) if( session->endpoint == MBEDTLS_SSL_IS_CLIENT && - session->hostname_len != 0 && + session->hostname_len > 0 && session->hostname != NULL ) { /* save host name */ - p[0] = session->hostname_len; - p++; memcpy( p, session->hostname, session->hostname_len ); p += session->hostname_len; } @@ -2100,11 +2100,14 @@ static int ssl_tls13_session_load( mbedtls_ssl_session *session, p += 1; if( end - p < session->hostname_len ) return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); - session->hostname = mbedtls_calloc( 1, session->hostname_len ); - if( session->hostname == NULL ) - return( MBEDTLS_ERR_SSL_ALLOC_FAILED ); - memcpy( session->hostname, p, session->hostname_len ); - p += session->hostname_len; + if( session->hostname_len > 0 ) + { + session->hostname = mbedtls_calloc( 1, session->hostname_len ); + if( session->hostname == NULL ) + return( MBEDTLS_ERR_SSL_ALLOC_FAILED ); + memcpy( session->hostname, p, session->hostname_len ); + p += session->hostname_len; + } } #endif /* MBEDTLS_SSL_SERVER_NAME_INDICATION && MBEDTLS_SSL_CLI_C */ From 6af2a6da74c62b8e788f40d9e377042404a6f328 Mon Sep 17 00:00:00 2001 From: Xiaokang Qian Date: Sat, 8 Oct 2022 10:50:19 +0000 Subject: [PATCH 03/19] Fix session save-load overflow issue Signed-off-by: Xiaokang Qian --- library/ssl_tls.c | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index fa4e6930e5..6a0a67800c 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -297,14 +297,15 @@ int mbedtls_ssl_session_copy( mbedtls_ssl_session *dst, } #endif /* MBEDTLS_SSL_SESSION_TICKETS && MBEDTLS_SSL_CLI_C */ -#if defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) - if( src->hostname != NULL ) +#if defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) && defined(MBEDTLS_SSL_CLI_C) + if( src->endpoint == MBEDTLS_SSL_IS_CLIENT && src->hostname != NULL ) { dst->hostname = mbedtls_calloc( 1, src->hostname_len ); if( dst->hostname == NULL ) return( MBEDTLS_ERR_SSL_ALLOC_FAILED ); memcpy( dst->hostname, src->hostname, src->hostname_len ); + dst->hostname_len = src->hostname_len; } #endif @@ -1945,6 +1946,7 @@ mbedtls_ssl_mode_t mbedtls_ssl_get_mode_from_ciphersuite( /* Serialization of TLS 1.3 sessions: * * struct { + * opaque hostname<1..2^16-1>; * uint64 ticket_received; * uint32 ticket_lifetime; * opaque ticket<1..2^16-1>; @@ -1956,7 +1958,7 @@ mbedtls_ssl_mode_t mbedtls_ssl_get_mode_from_ciphersuite( * uint32 ticket_age_add; * uint8 ticket_flags; * opaque resumption_key<0..255>; - * opaque hostname<1..2^8-1>; + * * select ( endpoint ) { * case client: ClientOnlyData; * case server: uint64 start_time; @@ -2021,13 +2023,13 @@ static int ssl_tls13_session_save( const mbedtls_ssl_session *session, memcpy( p, session->resumption_key, session->resumption_key_len ); p += session->resumption_key_len; - p[0] = session->hostname_len; - p++; #if defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) && defined(MBEDTLS_SSL_CLI_C) - if( session->endpoint == MBEDTLS_SSL_IS_CLIENT && - session->hostname_len > 0 && - session->hostname != NULL ) + if( session->endpoint == MBEDTLS_SSL_IS_CLIENT ) { + p[0] = session->hostname_len; + p++; + if ( session->hostname_len > 0 && + session->hostname != NULL ) /* save host name */ memcpy( p, session->hostname, session->hostname_len ); p += session->hostname_len; @@ -2096,8 +2098,11 @@ static int ssl_tls13_session_load( mbedtls_ssl_session *session, if( session->endpoint == MBEDTLS_SSL_IS_CLIENT ) { /* load host name */ + if( end - p < 1 ) + return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); session->hostname_len = p[0]; p += 1; + if( end - p < session->hostname_len ) return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); if( session->hostname_len > 0 ) From be98f96de2dc7f41497c87e2b48bcaaf6f4d2a6e Mon Sep 17 00:00:00 2001 From: Xiaokang Qian Date: Sat, 8 Oct 2022 11:09:20 +0000 Subject: [PATCH 04/19] Remove useless hostname check in server side Signed-off-by: Xiaokang Qian --- library/ssl_tls13_server.c | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/library/ssl_tls13_server.c b/library/ssl_tls13_server.c index 60b1e05f43..61a1bad578 100644 --- a/library/ssl_tls13_server.c +++ b/library/ssl_tls13_server.c @@ -235,20 +235,6 @@ static int ssl_tls13_offered_psks_check_identity_match_ticket( (int)age_diff_in_ms ) ); goto exit; } -#if 0 -#if defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) - if( ssl->session_negotiate->hostname != NULL && - ssl->session_negotiate->hostname_len != 0 && - memcmp( ssl->session_negotiate->hostname, - ssl->session->hostname, ssl->session->hostname_len ) != 0 ) - { - MBEDTLS_SSL_DEBUG_MSG( - 3, ( "Session hostname not matched with stored session ticket" ) ); - goto exit; - } - -#endif /* MBEDTLS_SSL_SERVER_NAME_INDICATION */ -#endif ret = 0; From fb8ac46add99338d5c706555e2621d880f0c1090 Mon Sep 17 00:00:00 2001 From: Xiaokang Qian Date: Sat, 8 Oct 2022 11:09:54 +0000 Subject: [PATCH 05/19] Change the name of servername when re-connect Signed-off-by: Xiaokang Qian --- programs/ssl/ssl_client2.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/programs/ssl/ssl_client2.c b/programs/ssl/ssl_client2.c index bc93352707..a7705ac9fa 100644 --- a/programs/ssl/ssl_client2.c +++ b/programs/ssl/ssl_client2.c @@ -366,7 +366,6 @@ int main( void ) "\n usage: ssl_client2 param=<>...\n" \ "\n acceptable parameters:\n" \ " server_name=%%s default: localhost\n" \ - " rec_server_name=%%s default: localhost\n" \ " server_addr=%%s default: given by name\n" \ " server_port=%%d default: 4433\n" \ " request_page=%%s default: \".\"\n" \ @@ -406,6 +405,7 @@ int main( void ) " exchanges=%%d default: 1\n" \ " reconnect=%%d number of reconnections using session resumption\n" \ " default: 0 (disabled)\n" \ + " reco_server_name=%%s default: localhost\n" \ " reco_delay=%%d default: 0 seconds\n" \ " reco_mode=%%d 0: copy session, 1: serialize session\n" \ " default: 1\n" \ @@ -456,7 +456,6 @@ int main( void ) struct options { const char *server_name; /* hostname of the server (client only) */ - const char *rec_s_name; /* hostname of the server (re-connect) */ const char *server_addr; /* address of the server (client only) */ const char *server_port; /* port on which the ssl service runs */ int debug_level; /* level of debugging */ @@ -500,6 +499,7 @@ struct options int recsplit; /* enable record splitting? */ int dhmlen; /* minimum DHM params len in bits */ int reconnect; /* attempt to resume session */ + const char *reco_server_name; /* hostname of the server (re-connect) */ int reco_delay; /* delay in seconds before resuming session */ int reco_mode; /* how to keep the session around */ int reconnect_hard; /* unexpectedly reconnect from the same port */ @@ -878,7 +878,6 @@ int main( int argc, char *argv[] ) } opt.server_name = DFL_SERVER_NAME; - opt.rec_s_name = DFL_SERVER_NAME; opt.server_addr = DFL_SERVER_ADDR; opt.server_port = DFL_SERVER_PORT; opt.debug_level = DFL_DEBUG_LEVEL; @@ -926,6 +925,7 @@ int main( int argc, char *argv[] ) opt.recsplit = DFL_RECSPLIT; opt.dhmlen = DFL_DHMLEN; opt.reconnect = DFL_RECONNECT; + opt.reco_server_name = DFL_SERVER_NAME; opt.reco_delay = DFL_RECO_DELAY; opt.reco_mode = DFL_RECO_MODE; opt.reconnect_hard = DFL_RECONNECT_HARD; @@ -964,8 +964,6 @@ int main( int argc, char *argv[] ) if( strcmp( p, "server_name" ) == 0 ) opt.server_name = q; - else if( strcmp( p, "rec_server_name" ) == 0 ) - opt.rec_s_name = q; else if( strcmp( p, "server_addr" ) == 0 ) opt.server_addr = q; else if( strcmp( p, "server_port" ) == 0 ) @@ -1124,6 +1122,8 @@ int main( int argc, char *argv[] ) if( opt.reconnect < 0 || opt.reconnect > 2 ) goto usage; } + else if( strcmp( p, "rec_server_name" ) == 0 ) + opt.reco_server_name = q; else if( strcmp( p, "reco_delay" ) == 0 ) { opt.reco_delay = atoi( q ); @@ -3120,7 +3120,7 @@ reconnect: #if defined(MBEDTLS_X509_CRT_PARSE_C) if( ( ret = mbedtls_ssl_reset_hostname( &ssl, opt.server_name, - opt.rec_s_name ) ) != 0 ) + opt.reco_server_name ) ) != 0 ) { mbedtls_printf( " failed\n ! mbedtls_ssl_reset_hostname returned %d\n\n", ret ); From adf84a4a8c036268beed5b92a7430e30b905b91a Mon Sep 17 00:00:00 2001 From: Xiaokang Qian Date: Sun, 9 Oct 2022 09:21:22 +0000 Subject: [PATCH 06/19] Remove public api mbedtls_ssl_reset_hostname() Signed-off-by: Xiaokang Qian --- include/mbedtls/ssl.h | 26 +++----------------------- library/ssl_tls.c | 33 --------------------------------- programs/ssl/ssl_client2.c | 4 ++-- tests/ssl-opt.sh | 2 +- 4 files changed, 6 insertions(+), 59 deletions(-) diff --git a/include/mbedtls/ssl.h b/include/mbedtls/ssl.h index 5e493f5f27..e6c545e058 100644 --- a/include/mbedtls/ssl.h +++ b/include/mbedtls/ssl.h @@ -1182,7 +1182,6 @@ struct mbedtls_ssl_session #endif /* MBEDTLS_SSL_SESSION_TICKETS && MBEDTLS_SSL_CLI_C */ #if defined(MBEDTLS_SSL_PROTO_TLS1_3) && defined(MBEDTLS_SSL_SESSION_TICKETS) - uint8_t MBEDTLS_PRIVATE(endpoint); /*!< 0: client, 1: server */ uint8_t MBEDTLS_PRIVATE(ticket_flags); /*!< Ticket flags */ uint32_t MBEDTLS_PRIVATE(ticket_age_add); /*!< Randomly generated value used to obscure the age of the ticket */ uint8_t MBEDTLS_PRIVATE(resumption_key_len); /*!< resumption_key length */ @@ -1200,12 +1199,14 @@ struct mbedtls_ssl_session #if defined(MBEDTLS_SSL_PROTO_TLS1_3) mbedtls_ssl_tls13_application_secrets MBEDTLS_PRIVATE(app_secrets); -#endif + + uint8_t MBEDTLS_PRIVATE(endpoint); /*!< 0: client, 1: server */ #if defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) uint8_t MBEDTLS_PRIVATE(hostname_len); /*!< host_name length */ char *MBEDTLS_PRIVATE(hostname); /*!< host name binded with tickets */ uint8_t hostname_mismatch; /*!< whether new host_name match with saved one */ #endif /* MBEDTLS_SSL_SERVER_NAME_INDICATION */ +#endif /* MBEDTLS_SSL_PROTO_TLS1_3 */ }; /* @@ -3667,27 +3668,6 @@ void mbedtls_ssl_conf_sig_algs( mbedtls_ssl_config *conf, * On too long input failure, old hostname is unchanged. */ int mbedtls_ssl_set_hostname( mbedtls_ssl_context *ssl, const char *hostname ); - -/** - * \brief Reset the hostname to the new server name when reconnection. - * - * \param ssl SSL context - * \param hostname the server hostname, may be NULL - * \param rec_hostname the server rec_hostname, may be NULL - - * \note Maximum hostname length MBEDTLS_SSL_MAX_HOST_NAME_LEN. - * - * \return 0 if successful, MBEDTLS_ERR_SSL_ALLOC_FAILED on - * allocation failure, MBEDTLS_ERR_SSL_BAD_INPUT_DATA on - * too long input rec_hostname. - * - * Rec_hostname set to the one provided on success. - * On allocation failure hostname is unchanged. - * On too long input failure, old hostname is unchanged. - */ -int mbedtls_ssl_reset_hostname( mbedtls_ssl_context *ssl, - const char *hostname, - const char *rec_hostname ); #endif /* MBEDTLS_X509_CRT_PARSE_C */ #if defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 6a0a67800c..abadc80d01 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -2484,39 +2484,6 @@ int mbedtls_ssl_set_hostname( mbedtls_ssl_context *ssl, const char *hostname ) return( 0 ); } - -int mbedtls_ssl_reset_hostname( mbedtls_ssl_context *ssl, - const char *hostname, - const char *rec_hostname ) -{ - /* Initialize to suppress unnecessary compiler warning */ - size_t rec_hostname_len = 0; - - if( hostname == NULL || rec_hostname == NULL ) - return( 0 ); - - rec_hostname_len = strlen( rec_hostname ); - if( rec_hostname_len > MBEDTLS_SSL_MAX_HOST_NAME_LEN ) - return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); - - if( rec_hostname_len == strlen( hostname ) && - memcmp( hostname, rec_hostname, rec_hostname_len ) == 0 ) - return( 0 ); - - if( ssl->hostname != NULL ) - { - mbedtls_platform_zeroize( ssl->hostname, strlen( ssl->hostname ) ); - mbedtls_free( ssl->hostname ); - ssl->hostname = NULL; - ssl->hostname = mbedtls_calloc( 1, rec_hostname_len + 1 ); - if( ssl->hostname == NULL ) - return( MBEDTLS_ERR_SSL_ALLOC_FAILED ); - memcpy( ssl->hostname, rec_hostname, rec_hostname_len ); - ssl->hostname[rec_hostname_len] = '\0'; - } - - return( 0 ); -} #endif /* MBEDTLS_X509_CRT_PARSE_C */ #if defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) diff --git a/programs/ssl/ssl_client2.c b/programs/ssl/ssl_client2.c index a7705ac9fa..9102ab40a1 100644 --- a/programs/ssl/ssl_client2.c +++ b/programs/ssl/ssl_client2.c @@ -3119,10 +3119,10 @@ reconnect: } #if defined(MBEDTLS_X509_CRT_PARSE_C) - if( ( ret = mbedtls_ssl_reset_hostname( &ssl, opt.server_name, + if( ( ret = mbedtls_ssl_set_hostname( &ssl, opt.reco_server_name ) ) != 0 ) { - mbedtls_printf( " failed\n ! mbedtls_ssl_reset_hostname returned %d\n\n", + mbedtls_printf( " failed\n ! mbedtls_ssl_set_hostname returned %d\n\n", ret ); goto exit; } diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index 414b8607bb..2a63610df7 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -12909,7 +12909,7 @@ run_test "TLS 1.3: NewSessionTicket: servername negative check, m->m" \ -c "got new session ticket." \ -c "Saving session for reuse... ok" \ -c "Reconnecting with saved session" \ - -c "hostname mismatch the session ticker, should not resume" \ + -c "hostname mismatch the session ticket, should not resume" \ -s "=> write NewSessionTicket msg" \ -s "server state: MBEDTLS_SSL_NEW_SESSION_TICKET" \ -s "server state: MBEDTLS_SSL_NEW_SESSION_TICKET_FLUSH" From bc663a04612be598fad51ecf5450345196ee50ae Mon Sep 17 00:00:00 2001 From: Xiaokang Qian Date: Sun, 9 Oct 2022 11:14:39 +0000 Subject: [PATCH 07/19] Refine code based on commnets Change code layout Change hostname_len type to size_t Fix various issues Signed-off-by: Xiaokang Qian --- include/mbedtls/ssl.h | 3 +-- library/ssl_client.c | 46 ++++++++++++++++++++++-------------- library/ssl_misc.h | 4 ++++ library/ssl_tls.c | 37 ++++++++++++++++------------- library/ssl_tls13_generic.c | 47 +++++++++++++++++++++++++++++++++++++ programs/ssl/ssl_client2.c | 2 +- 6 files changed, 102 insertions(+), 37 deletions(-) diff --git a/include/mbedtls/ssl.h b/include/mbedtls/ssl.h index e6c545e058..93ad7f3512 100644 --- a/include/mbedtls/ssl.h +++ b/include/mbedtls/ssl.h @@ -1202,9 +1202,8 @@ struct mbedtls_ssl_session uint8_t MBEDTLS_PRIVATE(endpoint); /*!< 0: client, 1: server */ #if defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) - uint8_t MBEDTLS_PRIVATE(hostname_len); /*!< host_name length */ + size_t MBEDTLS_PRIVATE(hostname_len); /*!< host_name length */ char *MBEDTLS_PRIVATE(hostname); /*!< host name binded with tickets */ - uint8_t hostname_mismatch; /*!< whether new host_name match with saved one */ #endif /* MBEDTLS_SSL_SERVER_NAME_INDICATION */ #endif /* MBEDTLS_SSL_PROTO_TLS1_3 */ }; diff --git a/library/ssl_client.c b/library/ssl_client.c index 2c5f664945..8080e3ee79 100644 --- a/library/ssl_client.c +++ b/library/ssl_client.c @@ -54,7 +54,6 @@ static int ssl_write_hostname_ext( mbedtls_ssl_context *ssl, { unsigned char *p = buf; size_t hostname_len; - size_t cmp_hostname_len; *olen = 0; @@ -65,25 +64,8 @@ static int ssl_write_hostname_ext( mbedtls_ssl_context *ssl, ( "client hello, adding server name extension: %s", ssl->hostname ) ); - ssl->session_negotiate->hostname_mismatch = 0; hostname_len = strlen( ssl->hostname ); - cmp_hostname_len = hostname_len < ssl->session_negotiate->hostname_len ? - hostname_len : ssl->session_negotiate->hostname_len; - - if( hostname_len != ssl->session_negotiate->hostname_len || - memcmp( ssl->hostname, ssl->session_negotiate->hostname, cmp_hostname_len ) ) - ssl->session_negotiate->hostname_mismatch = 1; - - if( ssl->session_negotiate->hostname == NULL ) - { - ssl->session_negotiate->hostname = mbedtls_calloc( 1, hostname_len ); - if( ssl->session_negotiate->hostname == NULL ) - return( MBEDTLS_ERR_SSL_ALLOC_FAILED ); - memcpy(ssl->session_negotiate->hostname, ssl->hostname, hostname_len); - } - ssl->session_negotiate->hostname_len = hostname_len; - MBEDTLS_SSL_CHK_BUF_PTR( p, end, hostname_len + 9 ); /* @@ -888,6 +870,34 @@ static int ssl_prepare_client_hello( mbedtls_ssl_context *ssl ) } } +#if defined(MBEDTLS_SSL_PROTO_TLS1_3) && \ + defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) + if( ssl->handshake->resume ) + { + if( ssl->hostname != NULL && ssl->session_negotiate->hostname != NULL ) + { + if( strcmp( ssl->hostname, ssl->session_negotiate->hostname ) ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, + ( "hostname mismatch the session ticket, should not resume " ) ); + return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); + } + } + else if( ssl->session_negotiate->hostname != NULL ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, + ( "hostname missed, should not resume " ) ); + return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); + } + } + else + { + mbedtls_ssl_session_set_hostname( ssl->session_negotiate, + ssl->hostname ); + } +#endif /* MBEDTLS_SSL_PROTO_TLS1_3 && + MBEDTLS_SSL_SERVER_NAME_INDICATION */ + return( 0 ); } /* diff --git a/library/ssl_misc.h b/library/ssl_misc.h index afacb76f01..f92a4dbecc 100644 --- a/library/ssl_misc.h +++ b/library/ssl_misc.h @@ -2201,6 +2201,10 @@ static inline int mbedtls_ssl_tls13_sig_alg_is_supported( return( 1 ); } +#if defined(MBEDTLS_X509_CRT_PARSE_C) +int mbedtls_ssl_session_set_hostname( mbedtls_ssl_session *ssl, + const char *hostname ); +#endif #endif /* MBEDTLS_SSL_PROTO_TLS1_3 */ #if defined(MBEDTLS_SSL_PROTO_TLS1_2) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index abadc80d01..959d01540d 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -297,14 +297,16 @@ int mbedtls_ssl_session_copy( mbedtls_ssl_session *dst, } #endif /* MBEDTLS_SSL_SESSION_TICKETS && MBEDTLS_SSL_CLI_C */ -#if defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) && defined(MBEDTLS_SSL_CLI_C) +#if defined(MBEDTLS_SSL_PROTO_TLS1_3) && \ + defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) && \ + defined(MBEDTLS_SSL_CLI_C) if( src->endpoint == MBEDTLS_SSL_IS_CLIENT && src->hostname != NULL ) { - dst->hostname = mbedtls_calloc( 1, src->hostname_len ); + dst->hostname = mbedtls_calloc( 1, src->hostname_len + 1 ); if( dst->hostname == NULL ) return( MBEDTLS_ERR_SSL_ALLOC_FAILED ); - memcpy( dst->hostname, src->hostname, src->hostname_len ); + strcpy( dst->hostname, src->hostname ); dst->hostname_len = src->hostname_len; } #endif @@ -1958,7 +1960,6 @@ mbedtls_ssl_mode_t mbedtls_ssl_get_mode_from_ciphersuite( * uint32 ticket_age_add; * uint8 ticket_flags; * opaque resumption_key<0..255>; - * * select ( endpoint ) { * case client: ClientOnlyData; * case server: uint64 start_time; @@ -1993,7 +1994,7 @@ static int ssl_tls13_session_save( const mbedtls_ssl_session *session, if( session->endpoint == MBEDTLS_SSL_IS_CLIENT ) { #if defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) - needed += 1 /* hostname_len */ + needed += 2 /* hostname_len */ + session->hostname_len; /* hostname */ #endif @@ -2026,13 +2027,15 @@ static int ssl_tls13_session_save( const mbedtls_ssl_session *session, #if defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) && defined(MBEDTLS_SSL_CLI_C) if( session->endpoint == MBEDTLS_SSL_IS_CLIENT ) { - p[0] = session->hostname_len; - p++; + MBEDTLS_PUT_UINT16_BE( session->hostname_len, p, 0 ); + p += 2; if ( session->hostname_len > 0 && session->hostname != NULL ) - /* save host name */ - memcpy( p, session->hostname, session->hostname_len ); - p += session->hostname_len; + { + /* save host name */ + memcpy( p, session->hostname, session->hostname_len ); + p += session->hostname_len; + } } #endif /* MBEDTLS_SSL_SERVER_NAME_INDICATION && MBEDTLS_SSL_CLI_C */ @@ -2098,19 +2101,20 @@ static int ssl_tls13_session_load( mbedtls_ssl_session *session, if( session->endpoint == MBEDTLS_SSL_IS_CLIENT ) { /* load host name */ - if( end - p < 1 ) + if( end - p < 2 ) return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); - session->hostname_len = p[0]; - p += 1; + session->hostname_len = MBEDTLS_GET_UINT16_BE( p, 0); + p += 2; - if( end - p < session->hostname_len ) + if( end - p < ( long int )session->hostname_len ) return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); if( session->hostname_len > 0 ) { - session->hostname = mbedtls_calloc( 1, session->hostname_len ); + session->hostname = mbedtls_calloc( 1, session->hostname_len + 1 ); if( session->hostname == NULL ) return( MBEDTLS_ERR_SSL_ALLOC_FAILED ); memcpy( session->hostname, p, session->hostname_len ); + session->hostname[session->hostname_len] = '\0'; p += session->hostname_len; } } @@ -3733,7 +3737,8 @@ void mbedtls_ssl_session_free( mbedtls_ssl_session *session ) mbedtls_free( session->ticket ); #endif -#if defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) +#if defined(MBEDTLS_SSL_PROTO_TLS1_3) && \ + defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) mbedtls_free( session->hostname ); #endif diff --git a/library/ssl_tls13_generic.c b/library/ssl_tls13_generic.c index abb7a14816..1b827ac60e 100644 --- a/library/ssl_tls13_generic.c +++ b/library/ssl_tls13_generic.c @@ -1485,4 +1485,51 @@ int mbedtls_ssl_tls13_generate_and_write_ecdh_key_exchange( } #endif /* MBEDTLS_ECDH_C */ +#if defined(MBEDTLS_X509_CRT_PARSE_C) +int mbedtls_ssl_session_set_hostname( mbedtls_ssl_session *ssl, + const char *hostname ) +{ + /* Initialize to suppress unnecessary compiler warning */ + size_t hostname_len = 0; + + /* Check if new hostname is valid before + * making any change to current one */ + if( hostname != NULL ) + { + hostname_len = strlen( hostname ); + + if( hostname_len > MBEDTLS_SSL_MAX_HOST_NAME_LEN ) + return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); + } + + /* Now it's clear that we will overwrite the old hostname, + * so we can free it safely */ + + if( ssl->hostname != NULL ) + { + mbedtls_platform_zeroize( ssl->hostname, strlen( ssl->hostname ) ); + mbedtls_free( ssl->hostname ); + } + + /* Passing NULL as hostname shall clear the old one */ + + if( hostname == NULL ) + { + ssl->hostname = NULL; + } + else + { + ssl->hostname = mbedtls_calloc( 1, hostname_len + 1 ); + if( ssl->hostname == NULL ) + return( MBEDTLS_ERR_SSL_ALLOC_FAILED ); + + memcpy( ssl->hostname, hostname, hostname_len ); + + ssl->hostname[hostname_len] = '\0'; + ssl->hostname_len = hostname_len; + } + + return( 0 ); +} +#endif /* MBEDTLS_X509_CRT_PARSE_C */ #endif /* MBEDTLS_SSL_TLS_C && MBEDTLS_SSL_PROTO_TLS1_3 */ diff --git a/programs/ssl/ssl_client2.c b/programs/ssl/ssl_client2.c index 9102ab40a1..be474d4737 100644 --- a/programs/ssl/ssl_client2.c +++ b/programs/ssl/ssl_client2.c @@ -3120,7 +3120,7 @@ reconnect: #if defined(MBEDTLS_X509_CRT_PARSE_C) if( ( ret = mbedtls_ssl_set_hostname( &ssl, - opt.reco_server_name ) ) != 0 ) + opt.reco_server_name ) ) != 0 ) { mbedtls_printf( " failed\n ! mbedtls_ssl_set_hostname returned %d\n\n", ret ); From 2f9efd3038468da8760149d17716b4ec3bb0c5b3 Mon Sep 17 00:00:00 2001 From: Xiaokang Qian Date: Mon, 10 Oct 2022 11:24:08 +0000 Subject: [PATCH 08/19] Address comments base on review Change function name to ssl_session_set_hostname() Remove hostname_len Change hostname to c_string Update test cases to multi session tickets Signed-off-by: Xiaokang Qian --- include/mbedtls/ssl.h | 1 - library/ssl_client.c | 56 +++++++++++++++++++++++++++++++++++-- library/ssl_misc.h | 5 ---- library/ssl_tls.c | 32 +++++++++++---------- library/ssl_tls13_generic.c | 47 ------------------------------- tests/ssl-opt.sh | 4 +-- 6 files changed, 72 insertions(+), 73 deletions(-) diff --git a/include/mbedtls/ssl.h b/include/mbedtls/ssl.h index 93ad7f3512..0efc8e7c81 100644 --- a/include/mbedtls/ssl.h +++ b/include/mbedtls/ssl.h @@ -1202,7 +1202,6 @@ struct mbedtls_ssl_session uint8_t MBEDTLS_PRIVATE(endpoint); /*!< 0: client, 1: server */ #if defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) - size_t MBEDTLS_PRIVATE(hostname_len); /*!< host_name length */ char *MBEDTLS_PRIVATE(hostname); /*!< host name binded with tickets */ #endif /* MBEDTLS_SSL_SERVER_NAME_INDICATION */ #endif /* MBEDTLS_SSL_PROTO_TLS1_3 */ diff --git a/library/ssl_client.c b/library/ssl_client.c index 8080e3ee79..0fda2712e4 100644 --- a/library/ssl_client.c +++ b/library/ssl_client.c @@ -713,6 +713,51 @@ static int ssl_generate_random( mbedtls_ssl_context *ssl ) MBEDTLS_CLIENT_HELLO_RANDOM_LEN - gmt_unix_time_len ); return( ret ); } + +static int ssl_session_set_hostname( mbedtls_ssl_session *session, + const char *hostname ) +{ + /* Initialize to suppress unnecessary compiler warning */ + size_t hostname_len = 0; + + /* Check if new hostname is valid before + * making any change to current one */ + if( hostname != NULL ) + { + hostname_len = strlen( hostname ); + + if( hostname_len > MBEDTLS_SSL_MAX_HOST_NAME_LEN ) + return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); + } + + /* Now it's clear that we will overwrite the old hostname, + * so we can free it safely */ + + if( session->hostname != NULL ) + { + mbedtls_platform_zeroize( session->hostname, + strlen( session->hostname ) ); + mbedtls_free( session->hostname ); + } + + /* Passing NULL as hostname shall clear the old one */ + + if( hostname == NULL ) + { + session->hostname = NULL; + } + else + { + session->hostname = mbedtls_calloc( 1, hostname_len + 1 ); + if( session->hostname == NULL ) + return( MBEDTLS_ERR_SSL_ALLOC_FAILED ); + + memcpy( session->hostname, hostname, hostname_len ); + } + + return( 0 ); +} + MBEDTLS_CHECK_RETURN_CRITICAL static int ssl_prepare_client_hello( mbedtls_ssl_context *ssl ) { @@ -876,7 +921,12 @@ static int ssl_prepare_client_hello( mbedtls_ssl_context *ssl ) { if( ssl->hostname != NULL && ssl->session_negotiate->hostname != NULL ) { - if( strcmp( ssl->hostname, ssl->session_negotiate->hostname ) ) + size_t hostname_len = strlen( ssl->hostname ); + size_t negotiate_hostname_len = + strlen( ssl->session_negotiate->hostname ); + if( hostname_len != negotiate_hostname_len || \ + strncmp( ssl->hostname, ssl->session_negotiate->hostname, + hostname_len ) ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "hostname mismatch the session ticket, should not resume " ) ); @@ -892,8 +942,8 @@ static int ssl_prepare_client_hello( mbedtls_ssl_context *ssl ) } else { - mbedtls_ssl_session_set_hostname( ssl->session_negotiate, - ssl->hostname ); + ssl_session_set_hostname( ssl->session_negotiate, + ssl->hostname ); } #endif /* MBEDTLS_SSL_PROTO_TLS1_3 && MBEDTLS_SSL_SERVER_NAME_INDICATION */ diff --git a/library/ssl_misc.h b/library/ssl_misc.h index f92a4dbecc..3ac81a365d 100644 --- a/library/ssl_misc.h +++ b/library/ssl_misc.h @@ -2200,11 +2200,6 @@ static inline int mbedtls_ssl_tls13_sig_alg_is_supported( } return( 1 ); } - -#if defined(MBEDTLS_X509_CRT_PARSE_C) -int mbedtls_ssl_session_set_hostname( mbedtls_ssl_session *ssl, - const char *hostname ); -#endif #endif /* MBEDTLS_SSL_PROTO_TLS1_3 */ #if defined(MBEDTLS_SSL_PROTO_TLS1_2) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 959d01540d..35e9fecf5d 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -302,12 +302,12 @@ int mbedtls_ssl_session_copy( mbedtls_ssl_session *dst, defined(MBEDTLS_SSL_CLI_C) if( src->endpoint == MBEDTLS_SSL_IS_CLIENT && src->hostname != NULL ) { - dst->hostname = mbedtls_calloc( 1, src->hostname_len + 1 ); + size_t hostname_len = strlen( src->hostname ); + dst->hostname = mbedtls_calloc( 1, hostname_len + 1 ); if( dst->hostname == NULL ) return( MBEDTLS_ERR_SSL_ALLOC_FAILED ); - strcpy( dst->hostname, src->hostname ); - dst->hostname_len = src->hostname_len; + strncpy( dst->hostname, src->hostname, hostname_len ); } #endif @@ -1993,9 +1993,10 @@ static int ssl_tls13_session_save( const mbedtls_ssl_session *session, #if defined(MBEDTLS_SSL_CLI_C) if( session->endpoint == MBEDTLS_SSL_IS_CLIENT ) { + size_t hostname_len = session->hostname == NULL?0:strlen( session->hostname ); #if defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) needed += 2 /* hostname_len */ - + session->hostname_len; /* hostname */ + + hostname_len; /* hostname */ #endif needed += 4 /* ticket_lifetime */ @@ -2027,14 +2028,15 @@ static int ssl_tls13_session_save( const mbedtls_ssl_session *session, #if defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) && defined(MBEDTLS_SSL_CLI_C) if( session->endpoint == MBEDTLS_SSL_IS_CLIENT ) { - MBEDTLS_PUT_UINT16_BE( session->hostname_len, p, 0 ); + size_t hostname_len = session->hostname == NULL?0:strlen(session->hostname); + MBEDTLS_PUT_UINT16_BE( hostname_len, p, 0 ); p += 2; - if ( session->hostname_len > 0 && + if ( hostname_len > 0 && session->hostname != NULL ) { /* save host name */ - memcpy( p, session->hostname, session->hostname_len ); - p += session->hostname_len; + memcpy( p, session->hostname, hostname_len ); + p += hostname_len; } } #endif /* MBEDTLS_SSL_SERVER_NAME_INDICATION && MBEDTLS_SSL_CLI_C */ @@ -2100,22 +2102,22 @@ static int ssl_tls13_session_load( mbedtls_ssl_session *session, #if defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) && defined(MBEDTLS_SSL_CLI_C) if( session->endpoint == MBEDTLS_SSL_IS_CLIENT ) { + size_t hostname_len; /* load host name */ if( end - p < 2 ) return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); - session->hostname_len = MBEDTLS_GET_UINT16_BE( p, 0); + hostname_len = MBEDTLS_GET_UINT16_BE( p, 0); p += 2; - if( end - p < ( long int )session->hostname_len ) + if( end - p < ( long int )hostname_len ) return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); - if( session->hostname_len > 0 ) + if( hostname_len > 0 ) { - session->hostname = mbedtls_calloc( 1, session->hostname_len + 1 ); + session->hostname = mbedtls_calloc( 1, hostname_len + 1 ); if( session->hostname == NULL ) return( MBEDTLS_ERR_SSL_ALLOC_FAILED ); - memcpy( session->hostname, p, session->hostname_len ); - session->hostname[session->hostname_len] = '\0'; - p += session->hostname_len; + memcpy( session->hostname, p, hostname_len ); + p += hostname_len; } } #endif /* MBEDTLS_SSL_SERVER_NAME_INDICATION && MBEDTLS_SSL_CLI_C */ diff --git a/library/ssl_tls13_generic.c b/library/ssl_tls13_generic.c index 1b827ac60e..abb7a14816 100644 --- a/library/ssl_tls13_generic.c +++ b/library/ssl_tls13_generic.c @@ -1485,51 +1485,4 @@ int mbedtls_ssl_tls13_generate_and_write_ecdh_key_exchange( } #endif /* MBEDTLS_ECDH_C */ -#if defined(MBEDTLS_X509_CRT_PARSE_C) -int mbedtls_ssl_session_set_hostname( mbedtls_ssl_session *ssl, - const char *hostname ) -{ - /* Initialize to suppress unnecessary compiler warning */ - size_t hostname_len = 0; - - /* Check if new hostname is valid before - * making any change to current one */ - if( hostname != NULL ) - { - hostname_len = strlen( hostname ); - - if( hostname_len > MBEDTLS_SSL_MAX_HOST_NAME_LEN ) - return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); - } - - /* Now it's clear that we will overwrite the old hostname, - * so we can free it safely */ - - if( ssl->hostname != NULL ) - { - mbedtls_platform_zeroize( ssl->hostname, strlen( ssl->hostname ) ); - mbedtls_free( ssl->hostname ); - } - - /* Passing NULL as hostname shall clear the old one */ - - if( hostname == NULL ) - { - ssl->hostname = NULL; - } - else - { - ssl->hostname = mbedtls_calloc( 1, hostname_len + 1 ); - if( ssl->hostname == NULL ) - return( MBEDTLS_ERR_SSL_ALLOC_FAILED ); - - memcpy( ssl->hostname, hostname, hostname_len ); - - ssl->hostname[hostname_len] = '\0'; - ssl->hostname_len = hostname_len; - } - - return( 0 ); -} -#endif /* MBEDTLS_X509_CRT_PARSE_C */ #endif /* MBEDTLS_SSL_TLS_C && MBEDTLS_SSL_PROTO_TLS1_3 */ diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index 2a63610df7..2be5506a41 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -12879,7 +12879,7 @@ requires_config_enabled MBEDTLS_SSL_SRV_C requires_config_enabled MBEDTLS_SSL_CLI_C requires_config_enabled MBEDTLS_DEBUG_C run_test "TLS 1.3: NewSessionTicket: servername check, m->m" \ - "$P_SRV debug_level=4 crt_file=data_files/server5.crt key_file=data_files/server5.key force_version=tls13 tickets=1 \ + "$P_SRV debug_level=4 crt_file=data_files/server5.crt key_file=data_files/server5.key force_version=tls13 tickets=4 \ sni=localhost,data_files/server2.crt,data_files/server2.key,-,-,-,polarssl.example,data_files/server1-nospace.crt,data_files/server1.key,-,-,-" \ "$P_CLI debug_level=4 server_name=localhost reco_mode=1 reconnect=1" \ 0 \ @@ -12901,7 +12901,7 @@ requires_config_enabled MBEDTLS_SSL_SRV_C requires_config_enabled MBEDTLS_SSL_CLI_C requires_config_enabled MBEDTLS_DEBUG_C run_test "TLS 1.3: NewSessionTicket: servername negative check, m->m" \ - "$P_SRV debug_level=4 crt_file=data_files/server5.crt key_file=data_files/server5.key force_version=tls13 tickets=1 \ + "$P_SRV debug_level=4 crt_file=data_files/server5.crt key_file=data_files/server5.key force_version=tls13 tickets=4 \ sni=localhost,data_files/server2.crt,data_files/server2.key,-,-,-,polarssl.example,data_files/server1-nospace.crt,data_files/server1.key,-,-,-" \ "$P_CLI debug_level=4 server_name=localhost rec_server_name=remote reco_mode=1 reconnect=1" \ 1 \ From a3b451f9502b495171a2df825dc78be6452b286f Mon Sep 17 00:00:00 2001 From: Xiaokang Qian Date: Tue, 11 Oct 2022 06:20:56 +0000 Subject: [PATCH 09/19] Adress kinds of comments base on review Rename function name to mbedtls_ssl_session_set_hostname Add two extra check cases for server name Fix some coding styles Signed-off-by: Xiaokang Qian --- library/ssl_client.c | 81 ++++++++++---------------------------------- library/ssl_misc.h | 7 ++++ library/ssl_tls.c | 67 +++++++++++++++++++++++++++++------- 3 files changed, 80 insertions(+), 75 deletions(-) diff --git a/library/ssl_client.c b/library/ssl_client.c index 0fda2712e4..993539573b 100644 --- a/library/ssl_client.c +++ b/library/ssl_client.c @@ -714,50 +714,6 @@ static int ssl_generate_random( mbedtls_ssl_context *ssl ) return( ret ); } -static int ssl_session_set_hostname( mbedtls_ssl_session *session, - const char *hostname ) -{ - /* Initialize to suppress unnecessary compiler warning */ - size_t hostname_len = 0; - - /* Check if new hostname is valid before - * making any change to current one */ - if( hostname != NULL ) - { - hostname_len = strlen( hostname ); - - if( hostname_len > MBEDTLS_SSL_MAX_HOST_NAME_LEN ) - return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); - } - - /* Now it's clear that we will overwrite the old hostname, - * so we can free it safely */ - - if( session->hostname != NULL ) - { - mbedtls_platform_zeroize( session->hostname, - strlen( session->hostname ) ); - mbedtls_free( session->hostname ); - } - - /* Passing NULL as hostname shall clear the old one */ - - if( hostname == NULL ) - { - session->hostname = NULL; - } - else - { - session->hostname = mbedtls_calloc( 1, hostname_len + 1 ); - if( session->hostname == NULL ) - return( MBEDTLS_ERR_SSL_ALLOC_FAILED ); - - memcpy( session->hostname, hostname, hostname_len ); - } - - return( 0 ); -} - MBEDTLS_CHECK_RETURN_CRITICAL static int ssl_prepare_client_hello( mbedtls_ssl_context *ssl ) { @@ -917,34 +873,33 @@ static int ssl_prepare_client_hello( mbedtls_ssl_context *ssl ) #if defined(MBEDTLS_SSL_PROTO_TLS1_3) && \ defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) - if( ssl->handshake->resume ) + if( ssl->tls_version == MBEDTLS_SSL_VERSION_TLS1_3 && + ssl->handshake->resume ) { - if( ssl->hostname != NULL && ssl->session_negotiate->hostname != NULL ) + int hostname_mismatch = 0; + if( ssl->session_negotiate->hostname != NULL ) { - size_t hostname_len = strlen( ssl->hostname ); - size_t negotiate_hostname_len = - strlen( ssl->session_negotiate->hostname ); - if( hostname_len != negotiate_hostname_len || \ - strncmp( ssl->hostname, ssl->session_negotiate->hostname, - hostname_len ) ) + if( ssl->hostname != NULL ) { - MBEDTLS_SSL_DEBUG_MSG( 1, - ( "hostname mismatch the session ticket, should not resume " ) ); - return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); + if( strcmp( ssl->hostname, ssl->session_negotiate->hostname) ) + hostname_mismatch = 1; } + else + hostname_mismatch = 1; } - else if( ssl->session_negotiate->hostname != NULL ) + else + hostname_mismatch = ssl->hostname != NULL; + + if( hostname_mismatch ) { - MBEDTLS_SSL_DEBUG_MSG( 1, - ( "hostname missed, should not resume " ) ); - return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); + MBEDTLS_SSL_DEBUG_MSG( 1, + ( "hostname mismatch the session ticket, should not resume " ) ); + return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); } } else - { - ssl_session_set_hostname( ssl->session_negotiate, - ssl->hostname ); - } + mbedtls_ssl_session_set_hostname( ssl->session_negotiate, + ssl->hostname ); #endif /* MBEDTLS_SSL_PROTO_TLS1_3 && MBEDTLS_SSL_SERVER_NAME_INDICATION */ diff --git a/library/ssl_misc.h b/library/ssl_misc.h index 3ac81a365d..f20897d1c3 100644 --- a/library/ssl_misc.h +++ b/library/ssl_misc.h @@ -2200,6 +2200,7 @@ static inline int mbedtls_ssl_tls13_sig_alg_is_supported( } return( 1 ); } + #endif /* MBEDTLS_SSL_PROTO_TLS1_3 */ #if defined(MBEDTLS_SSL_PROTO_TLS1_2) @@ -2493,4 +2494,10 @@ int mbedtls_ssl_tls13_write_binders_of_pre_shared_key_ext( unsigned char *buf, unsigned char *end ); #endif /* MBEDTLS_KEY_EXCHANGE_SOME_PSK_ENABLED */ +#if defined(MBEDTLS_SSL_PROTO_TLS1_3) && \ + defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) +int mbedtls_ssl_session_set_hostname( mbedtls_ssl_session *session, + const char *hostname ); +#endif + #endif /* ssl_misc.h */ diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 35e9fecf5d..062259b5cd 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -300,14 +300,11 @@ int mbedtls_ssl_session_copy( mbedtls_ssl_session *dst, #if defined(MBEDTLS_SSL_PROTO_TLS1_3) && \ defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) && \ defined(MBEDTLS_SSL_CLI_C) - if( src->endpoint == MBEDTLS_SSL_IS_CLIENT && src->hostname != NULL ) + if( src->endpoint == MBEDTLS_SSL_IS_CLIENT ) { - size_t hostname_len = strlen( src->hostname ); - dst->hostname = mbedtls_calloc( 1, hostname_len + 1 ); - if( dst->hostname == NULL ) - return( MBEDTLS_ERR_SSL_ALLOC_FAILED ); - - strncpy( dst->hostname, src->hostname, hostname_len ); + dst->hostname = NULL; + mbedtls_ssl_session_set_hostname( dst, + src->hostname ); } #endif @@ -1975,6 +1972,11 @@ static int ssl_tls13_session_save( const mbedtls_ssl_session *session, size_t *olen ) { unsigned char *p = buf; +#if defined(MBEDTLS_SSL_CLI_C) && \ + defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) + size_t hostname_len = ( session->hostname == NULL ) ? + 0 : strlen( session->hostname ); +#endif size_t needed = 1 /* endpoint */ + 2 /* ciphersuite */ + 4 /* ticket_age_add */ @@ -1993,7 +1995,6 @@ static int ssl_tls13_session_save( const mbedtls_ssl_session *session, #if defined(MBEDTLS_SSL_CLI_C) if( session->endpoint == MBEDTLS_SSL_IS_CLIENT ) { - size_t hostname_len = session->hostname == NULL?0:strlen( session->hostname ); #if defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) needed += 2 /* hostname_len */ + hostname_len; /* hostname */ @@ -2028,7 +2029,6 @@ static int ssl_tls13_session_save( const mbedtls_ssl_session *session, #if defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) && defined(MBEDTLS_SSL_CLI_C) if( session->endpoint == MBEDTLS_SSL_IS_CLIENT ) { - size_t hostname_len = session->hostname == NULL?0:strlen(session->hostname); MBEDTLS_PUT_UINT16_BE( hostname_len, p, 0 ); p += 2; if ( hostname_len > 0 && @@ -2106,7 +2106,7 @@ static int ssl_tls13_session_load( mbedtls_ssl_session *session, /* load host name */ if( end - p < 2 ) return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); - hostname_len = MBEDTLS_GET_UINT16_BE( p, 0); + hostname_len = MBEDTLS_GET_UINT16_BE( p, 0 ); p += 2; if( end - p < ( long int )hostname_len ) @@ -2464,7 +2464,6 @@ int mbedtls_ssl_set_hostname( mbedtls_ssl_context *ssl, const char *hostname ) /* Now it's clear that we will overwrite the old hostname, * so we can free it safely */ - if( ssl->hostname != NULL ) { mbedtls_platform_zeroize( ssl->hostname, strlen( ssl->hostname ) ); @@ -2472,7 +2471,6 @@ int mbedtls_ssl_set_hostname( mbedtls_ssl_context *ssl, const char *hostname ) } /* Passing NULL as hostname shall clear the old one */ - if( hostname == NULL ) { ssl->hostname = NULL; @@ -8863,4 +8861,49 @@ int mbedtls_ssl_write_alpn_ext( mbedtls_ssl_context *ssl, } #endif /* MBEDTLS_SSL_ALPN */ +#if defined(MBEDTLS_SSL_PROTO_TLS1_3) && \ + defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) +int mbedtls_ssl_session_set_hostname( mbedtls_ssl_session *session, + const char *hostname ) +{ + /* Initialize to suppress unnecessary compiler warning */ + size_t hostname_len = 0; + + /* Check if new hostname is valid before + * making any change to current one */ + if( hostname != NULL ) + { + hostname_len = strlen( hostname ); + + if( hostname_len > MBEDTLS_SSL_MAX_HOST_NAME_LEN ) + return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); + } + + /* Now it's clear that we will overwrite the old hostname, + * so we can free it safely */ + if( session->hostname != NULL ) + { + mbedtls_platform_zeroize( session->hostname, + strlen( session->hostname ) ); + mbedtls_free( session->hostname ); + } + + /* Passing NULL as hostname shall clear the old one */ + if( hostname == NULL ) + { + session->hostname = NULL; + } + else + { + session->hostname = mbedtls_calloc( 1, hostname_len + 1 ); + if( session->hostname == NULL ) + return( MBEDTLS_ERR_SSL_ALLOC_FAILED ); + + memcpy( session->hostname, hostname, hostname_len ); + } + + return( 0 ); +} +#endif /* MBEDTLS_SSL_PROTO_TLS1_3 && MBEDTLS_SSL_SERVER_NAME_INDICATION */ + #endif /* MBEDTLS_SSL_TLS_C */ From d7adc374d325dea0647e0c0c99b1270046116f90 Mon Sep 17 00:00:00 2001 From: Xiaokang Qian Date: Tue, 11 Oct 2022 09:05:11 +0000 Subject: [PATCH 10/19] Refine the server name compare logic Signed-off-by: Xiaokang Qian --- library/ssl_client.c | 18 +++++------------- 1 file changed, 5 insertions(+), 13 deletions(-) diff --git a/library/ssl_client.c b/library/ssl_client.c index 993539573b..48a19c9e4d 100644 --- a/library/ssl_client.c +++ b/library/ssl_client.c @@ -876,19 +876,11 @@ static int ssl_prepare_client_hello( mbedtls_ssl_context *ssl ) if( ssl->tls_version == MBEDTLS_SSL_VERSION_TLS1_3 && ssl->handshake->resume ) { - int hostname_mismatch = 0; - if( ssl->session_negotiate->hostname != NULL ) - { - if( ssl->hostname != NULL ) - { - if( strcmp( ssl->hostname, ssl->session_negotiate->hostname) ) - hostname_mismatch = 1; - } - else - hostname_mismatch = 1; - } - else - hostname_mismatch = ssl->hostname != NULL; + int hostname_mismatch = ssl->hostname != NULL || + ssl->session_negotiate->hostname != NULL; + if( ssl->hostname != NULL && ssl->session_negotiate->hostname != NULL ) + hostname_mismatch = strcmp( + ssl->hostname, ssl->session_negotiate->hostname ) != 0; if( hostname_mismatch ) { From 03409290d2bd418d0e105f1283b131f5dbe15d6f Mon Sep 17 00:00:00 2001 From: Xiaokang Qian Date: Wed, 12 Oct 2022 02:49:52 +0000 Subject: [PATCH 11/19] Add MBEDTLS_SSL_SESSION_TICKETS guard to server name check Signed-off-by: Xiaokang Qian --- include/mbedtls/ssl.h | 7 ++++--- library/ssl_client.c | 11 ++++++++--- library/ssl_misc.h | 2 ++ library/ssl_tls.c | 30 ++++++++++++++++++++++-------- 4 files changed, 36 insertions(+), 14 deletions(-) diff --git a/include/mbedtls/ssl.h b/include/mbedtls/ssl.h index 0efc8e7c81..4cf67044ae 100644 --- a/include/mbedtls/ssl.h +++ b/include/mbedtls/ssl.h @@ -1182,6 +1182,7 @@ struct mbedtls_ssl_session #endif /* MBEDTLS_SSL_SESSION_TICKETS && MBEDTLS_SSL_CLI_C */ #if defined(MBEDTLS_SSL_PROTO_TLS1_3) && defined(MBEDTLS_SSL_SESSION_TICKETS) + uint8_t MBEDTLS_PRIVATE(endpoint); /*!< 0: client, 1: server */ uint8_t MBEDTLS_PRIVATE(ticket_flags); /*!< Ticket flags */ uint32_t MBEDTLS_PRIVATE(ticket_age_add); /*!< Randomly generated value used to obscure the age of the ticket */ uint8_t MBEDTLS_PRIVATE(resumption_key_len); /*!< resumption_key length */ @@ -1200,10 +1201,10 @@ struct mbedtls_ssl_session #if defined(MBEDTLS_SSL_PROTO_TLS1_3) mbedtls_ssl_tls13_application_secrets MBEDTLS_PRIVATE(app_secrets); - uint8_t MBEDTLS_PRIVATE(endpoint); /*!< 0: client, 1: server */ -#if defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) +#if defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) && \ + defined(MBEDTLS_SSL_SESSION_TICKETS) char *MBEDTLS_PRIVATE(hostname); /*!< host name binded with tickets */ -#endif /* MBEDTLS_SSL_SERVER_NAME_INDICATION */ +#endif /* MBEDTLS_SSL_SERVER_NAME_INDICATION && MBEDTLS_SSL_SESSION_TICKETS */ #endif /* MBEDTLS_SSL_PROTO_TLS1_3 */ }; diff --git a/library/ssl_client.c b/library/ssl_client.c index 48a19c9e4d..baf82a69de 100644 --- a/library/ssl_client.c +++ b/library/ssl_client.c @@ -872,6 +872,7 @@ static int ssl_prepare_client_hello( mbedtls_ssl_context *ssl ) } #if defined(MBEDTLS_SSL_PROTO_TLS1_3) && \ + defined(MBEDTLS_SSL_SESSION_TICKETS) && \ defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) if( ssl->tls_version == MBEDTLS_SSL_VERSION_TLS1_3 && ssl->handshake->resume ) @@ -885,14 +886,18 @@ static int ssl_prepare_client_hello( mbedtls_ssl_context *ssl ) if( hostname_mismatch ) { MBEDTLS_SSL_DEBUG_MSG( 1, - ( "hostname mismatch the session ticket, should not resume " ) ); + ( "hostname mismatch the session ticket," + " should not resume " ) ); return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); } } else - mbedtls_ssl_session_set_hostname( ssl->session_negotiate, - ssl->hostname ); + { + return mbedtls_ssl_session_set_hostname( ssl->session_negotiate, + ssl->hostname ); + } #endif /* MBEDTLS_SSL_PROTO_TLS1_3 && + MBEDTLS_SSL_SESSION_TICKETS && MBEDTLS_SSL_SERVER_NAME_INDICATION */ return( 0 ); diff --git a/library/ssl_misc.h b/library/ssl_misc.h index f20897d1c3..6ba9ad519e 100644 --- a/library/ssl_misc.h +++ b/library/ssl_misc.h @@ -2495,7 +2495,9 @@ int mbedtls_ssl_tls13_write_binders_of_pre_shared_key_ext( #endif /* MBEDTLS_KEY_EXCHANGE_SOME_PSK_ENABLED */ #if defined(MBEDTLS_SSL_PROTO_TLS1_3) && \ + defined(MBEDTLS_SSL_SESSION_TICKETS) && \ defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) +MBEDTLS_CHECK_RETURN_CRITICAL int mbedtls_ssl_session_set_hostname( mbedtls_ssl_session *session, const char *hostname ); #endif diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 062259b5cd..6563b907c5 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -298,13 +298,14 @@ int mbedtls_ssl_session_copy( mbedtls_ssl_session *dst, #endif /* MBEDTLS_SSL_SESSION_TICKETS && MBEDTLS_SSL_CLI_C */ #if defined(MBEDTLS_SSL_PROTO_TLS1_3) && \ + defined(MBEDTLS_SSL_SESSION_TICKETS) && \ defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) && \ defined(MBEDTLS_SSL_CLI_C) if( src->endpoint == MBEDTLS_SSL_IS_CLIENT ) { dst->hostname = NULL; - mbedtls_ssl_session_set_hostname( dst, - src->hostname ); + return mbedtls_ssl_session_set_hostname( dst, + src->hostname ); } #endif @@ -1973,6 +1974,7 @@ static int ssl_tls13_session_save( const mbedtls_ssl_session *session, { unsigned char *p = buf; #if defined(MBEDTLS_SSL_CLI_C) && \ + defined(MBEDTLS_SSL_SESSION_TICKETS) && \ defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) size_t hostname_len = ( session->hostname == NULL ) ? 0 : strlen( session->hostname ); @@ -1995,7 +1997,8 @@ static int ssl_tls13_session_save( const mbedtls_ssl_session *session, #if defined(MBEDTLS_SSL_CLI_C) if( session->endpoint == MBEDTLS_SSL_IS_CLIENT ) { -#if defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) +#if defined(MBEDTLS_SSL_SESSION_TICKETS) && \ + defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) needed += 2 /* hostname_len */ + hostname_len; /* hostname */ #endif @@ -2026,7 +2029,9 @@ static int ssl_tls13_session_save( const mbedtls_ssl_session *session, memcpy( p, session->resumption_key, session->resumption_key_len ); p += session->resumption_key_len; -#if defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) && defined(MBEDTLS_SSL_CLI_C) +#if defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) && \ + defined(MBEDTLS_SSL_SESSION_TICKETS) && \ + defined(MBEDTLS_SSL_CLI_C) if( session->endpoint == MBEDTLS_SSL_IS_CLIENT ) { MBEDTLS_PUT_UINT16_BE( hostname_len, p, 0 ); @@ -2039,7 +2044,9 @@ static int ssl_tls13_session_save( const mbedtls_ssl_session *session, p += hostname_len; } } -#endif /* MBEDTLS_SSL_SERVER_NAME_INDICATION && MBEDTLS_SSL_CLI_C */ +#endif /* MBEDTLS_SSL_SERVER_NAME_INDICATION && + MBEDTLS_SSL_SESSION_TICKETS && + MBEDTLS_SSL_CLI_C */ #if defined(MBEDTLS_HAVE_TIME) && defined(MBEDTLS_SSL_SRV_C) if( session->endpoint == MBEDTLS_SSL_IS_SERVER ) @@ -2099,7 +2106,9 @@ static int ssl_tls13_session_load( mbedtls_ssl_session *session, memcpy( session->resumption_key, p, session->resumption_key_len ); p += session->resumption_key_len; -#if defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) && defined(MBEDTLS_SSL_CLI_C) +#if defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) && \ + defined(MBEDTLS_SSL_SESSION_TICKETS) && \ + defined(MBEDTLS_SSL_CLI_C) if( session->endpoint == MBEDTLS_SSL_IS_CLIENT ) { size_t hostname_len; @@ -2120,7 +2129,9 @@ static int ssl_tls13_session_load( mbedtls_ssl_session *session, p += hostname_len; } } -#endif /* MBEDTLS_SSL_SERVER_NAME_INDICATION && MBEDTLS_SSL_CLI_C */ +#endif /* MBEDTLS_SSL_SERVER_NAME_INDICATION && + MBEDTLS_SSL_SESSION_TICKETS && + MBEDTLS_SSL_CLI_C */ #if defined(MBEDTLS_HAVE_TIME) && defined(MBEDTLS_SSL_SRV_C) if( session->endpoint == MBEDTLS_SSL_IS_SERVER ) @@ -8862,6 +8873,7 @@ int mbedtls_ssl_write_alpn_ext( mbedtls_ssl_context *ssl, #endif /* MBEDTLS_SSL_ALPN */ #if defined(MBEDTLS_SSL_PROTO_TLS1_3) && \ + defined(MBEDTLS_SSL_SESSION_TICKETS) && \ defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) int mbedtls_ssl_session_set_hostname( mbedtls_ssl_session *session, const char *hostname ) @@ -8904,6 +8916,8 @@ int mbedtls_ssl_session_set_hostname( mbedtls_ssl_session *session, return( 0 ); } -#endif /* MBEDTLS_SSL_PROTO_TLS1_3 && MBEDTLS_SSL_SERVER_NAME_INDICATION */ +#endif /* MBEDTLS_SSL_PROTO_TLS1_3 && + MBEDTLS_SSL_SESSION_TICKETS && + MBEDTLS_SSL_SERVER_NAME_INDICATION */ #endif /* MBEDTLS_SSL_TLS_C */ From ed0620cb1372f8e17da6d771bc26a8f41089bfe2 Mon Sep 17 00:00:00 2001 From: Xiaokang Qian Date: Wed, 12 Oct 2022 06:58:13 +0000 Subject: [PATCH 12/19] Refine code base on comments Move code to proper macro guards protection Fix typo issues Signed-off-by: Xiaokang Qian --- library/ssl_client.c | 6 +-- library/ssl_misc.h | 3 +- library/ssl_tls.c | 101 +++++++++++++++++++------------------------ tests/ssl-opt.sh | 2 +- 4 files changed, 51 insertions(+), 61 deletions(-) diff --git a/library/ssl_client.c b/library/ssl_client.c index baf82a69de..341e882cce 100644 --- a/library/ssl_client.c +++ b/library/ssl_client.c @@ -885,9 +885,9 @@ static int ssl_prepare_client_hello( mbedtls_ssl_context *ssl ) if( hostname_mismatch ) { - MBEDTLS_SSL_DEBUG_MSG( 1, - ( "hostname mismatch the session ticket," - " should not resume " ) ); + MBEDTLS_SSL_DEBUG_MSG( + 1, ( "Hostname mismatch the session ticket, " + "disable session resumption." ) ); return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); } } diff --git a/library/ssl_misc.h b/library/ssl_misc.h index 6ba9ad519e..82a0d5c8a2 100644 --- a/library/ssl_misc.h +++ b/library/ssl_misc.h @@ -2496,7 +2496,8 @@ int mbedtls_ssl_tls13_write_binders_of_pre_shared_key_ext( #if defined(MBEDTLS_SSL_PROTO_TLS1_3) && \ defined(MBEDTLS_SSL_SESSION_TICKETS) && \ - defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) + defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) && \ + defined(MBEDTLS_SSL_TLS_C) MBEDTLS_CHECK_RETURN_CRITICAL int mbedtls_ssl_session_set_hostname( mbedtls_ssl_session *session, const char *hostname ); diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 6563b907c5..521b922b7d 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -295,12 +295,9 @@ int mbedtls_ssl_session_copy( mbedtls_ssl_session *dst, memcpy( dst->ticket, src->ticket, src->ticket_len ); } -#endif /* MBEDTLS_SSL_SESSION_TICKETS && MBEDTLS_SSL_CLI_C */ #if defined(MBEDTLS_SSL_PROTO_TLS1_3) && \ - defined(MBEDTLS_SSL_SESSION_TICKETS) && \ - defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) && \ - defined(MBEDTLS_SSL_CLI_C) + defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) if( src->endpoint == MBEDTLS_SSL_IS_CLIENT ) { dst->hostname = NULL; @@ -308,6 +305,7 @@ int mbedtls_ssl_session_copy( mbedtls_ssl_session *dst, src->hostname ); } #endif +#endif /* MBEDTLS_SSL_SESSION_TICKETS && MBEDTLS_SSL_CLI_C */ return( 0 ); } @@ -1974,10 +1972,9 @@ static int ssl_tls13_session_save( const mbedtls_ssl_session *session, { unsigned char *p = buf; #if defined(MBEDTLS_SSL_CLI_C) && \ - defined(MBEDTLS_SSL_SESSION_TICKETS) && \ defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) size_t hostname_len = ( session->hostname == NULL ) ? - 0 : strlen( session->hostname ); + 0 : strlen( session->hostname ) + 1; #endif size_t needed = 1 /* endpoint */ + 2 /* ciphersuite */ @@ -2029,24 +2026,7 @@ static int ssl_tls13_session_save( const mbedtls_ssl_session *session, memcpy( p, session->resumption_key, session->resumption_key_len ); p += session->resumption_key_len; -#if defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) && \ - defined(MBEDTLS_SSL_SESSION_TICKETS) && \ - defined(MBEDTLS_SSL_CLI_C) - if( session->endpoint == MBEDTLS_SSL_IS_CLIENT ) - { - MBEDTLS_PUT_UINT16_BE( hostname_len, p, 0 ); - p += 2; - if ( hostname_len > 0 && - session->hostname != NULL ) - { - /* save host name */ - memcpy( p, session->hostname, hostname_len ); - p += hostname_len; - } - } -#endif /* MBEDTLS_SSL_SERVER_NAME_INDICATION && - MBEDTLS_SSL_SESSION_TICKETS && - MBEDTLS_SSL_CLI_C */ + #if defined(MBEDTLS_HAVE_TIME) && defined(MBEDTLS_SSL_SRV_C) if( session->endpoint == MBEDTLS_SSL_IS_SERVER ) @@ -2059,6 +2039,17 @@ static int ssl_tls13_session_save( const mbedtls_ssl_session *session, #if defined(MBEDTLS_SSL_CLI_C) if( session->endpoint == MBEDTLS_SSL_IS_CLIENT ) { +#if defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) + MBEDTLS_PUT_UINT16_BE( hostname_len, p, 0 ); + p += 2; + if ( hostname_len > 0 ) + { + /* save host name */ + memcpy( p, session->hostname, hostname_len ); + p += hostname_len; + } +#endif /* MBEDTLS_SSL_SERVER_NAME_INDICATION */ + #if defined(MBEDTLS_HAVE_TIME) MBEDTLS_PUT_UINT64_BE( (uint64_t) session->ticket_received, p, 0 ); p += 8; @@ -2106,33 +2097,6 @@ static int ssl_tls13_session_load( mbedtls_ssl_session *session, memcpy( session->resumption_key, p, session->resumption_key_len ); p += session->resumption_key_len; -#if defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) && \ - defined(MBEDTLS_SSL_SESSION_TICKETS) && \ - defined(MBEDTLS_SSL_CLI_C) - if( session->endpoint == MBEDTLS_SSL_IS_CLIENT ) - { - size_t hostname_len; - /* load host name */ - if( end - p < 2 ) - return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); - hostname_len = MBEDTLS_GET_UINT16_BE( p, 0 ); - p += 2; - - if( end - p < ( long int )hostname_len ) - return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); - if( hostname_len > 0 ) - { - session->hostname = mbedtls_calloc( 1, hostname_len + 1 ); - if( session->hostname == NULL ) - return( MBEDTLS_ERR_SSL_ALLOC_FAILED ); - memcpy( session->hostname, p, hostname_len ); - p += hostname_len; - } - } -#endif /* MBEDTLS_SSL_SERVER_NAME_INDICATION && - MBEDTLS_SSL_SESSION_TICKETS && - MBEDTLS_SSL_CLI_C */ - #if defined(MBEDTLS_HAVE_TIME) && defined(MBEDTLS_SSL_SRV_C) if( session->endpoint == MBEDTLS_SSL_IS_SERVER ) { @@ -2146,6 +2110,28 @@ static int ssl_tls13_session_load( mbedtls_ssl_session *session, #if defined(MBEDTLS_SSL_CLI_C) if( session->endpoint == MBEDTLS_SSL_IS_CLIENT ) { +#if defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) && \ + defined(MBEDTLS_SSL_SESSION_TICKETS) + size_t hostname_len; + /* load host name */ + if( end - p < 2 ) + return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); + hostname_len = MBEDTLS_GET_UINT16_BE( p, 0 ); + p += 2; + + if( end - p < ( long int )hostname_len ) + return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); + if( hostname_len > 0 ) + { + session->hostname = mbedtls_calloc( 1, hostname_len ); + if( session->hostname == NULL ) + return( MBEDTLS_ERR_SSL_ALLOC_FAILED ); + memcpy( session->hostname, p, hostname_len ); + p += hostname_len; + } +#endif /* MBEDTLS_SSL_SERVER_NAME_INDICATION && + MBEDTLS_SSL_SESSION_TICKETS */ + #if defined(MBEDTLS_HAVE_TIME) if( end - p < 8 ) return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); @@ -2475,6 +2461,7 @@ int mbedtls_ssl_set_hostname( mbedtls_ssl_context *ssl, const char *hostname ) /* Now it's clear that we will overwrite the old hostname, * so we can free it safely */ + if( ssl->hostname != NULL ) { mbedtls_platform_zeroize( ssl->hostname, strlen( ssl->hostname ) ); @@ -2482,6 +2469,7 @@ int mbedtls_ssl_set_hostname( mbedtls_ssl_context *ssl, const char *hostname ) } /* Passing NULL as hostname shall clear the old one */ + if( hostname == NULL ) { ssl->hostname = NULL; @@ -3745,13 +3733,12 @@ void mbedtls_ssl_session_free( mbedtls_ssl_session *session ) #endif #if defined(MBEDTLS_SSL_SESSION_TICKETS) && defined(MBEDTLS_SSL_CLI_C) - mbedtls_free( session->ticket ); -#endif - #if defined(MBEDTLS_SSL_PROTO_TLS1_3) && \ defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) mbedtls_free( session->hostname ); #endif + mbedtls_free( session->ticket ); +#endif mbedtls_platform_zeroize( session, sizeof( mbedtls_ssl_session ) ); } @@ -8874,7 +8861,8 @@ int mbedtls_ssl_write_alpn_ext( mbedtls_ssl_context *ssl, #if defined(MBEDTLS_SSL_PROTO_TLS1_3) && \ defined(MBEDTLS_SSL_SESSION_TICKETS) && \ - defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) + defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) && \ + defined(MBEDTLS_SSL_TLS_C) int mbedtls_ssl_session_set_hostname( mbedtls_ssl_session *session, const char *hostname ) { @@ -8918,6 +8906,7 @@ int mbedtls_ssl_session_set_hostname( mbedtls_ssl_session *session, } #endif /* MBEDTLS_SSL_PROTO_TLS1_3 && MBEDTLS_SSL_SESSION_TICKETS && - MBEDTLS_SSL_SERVER_NAME_INDICATION */ + MBEDTLS_SSL_SERVER_NAME_INDICATION && + MBEDTLS_SSL_TLS_C */ #endif /* MBEDTLS_SSL_TLS_C */ diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index 2be5506a41..f6437f5153 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -12909,7 +12909,7 @@ run_test "TLS 1.3: NewSessionTicket: servername negative check, m->m" \ -c "got new session ticket." \ -c "Saving session for reuse... ok" \ -c "Reconnecting with saved session" \ - -c "hostname mismatch the session ticket, should not resume" \ + -c "Hostname mismatch the session ticket, disable session resumption." \ -s "=> write NewSessionTicket msg" \ -s "server state: MBEDTLS_SSL_NEW_SESSION_TICKET" \ -s "server state: MBEDTLS_SSL_NEW_SESSION_TICKET_FLUSH" From ed3afcd6c31a12e33fe02a70d646dcf4f90e3259 Mon Sep 17 00:00:00 2001 From: Xiaokang Qian Date: Wed, 12 Oct 2022 08:31:11 +0000 Subject: [PATCH 13/19] Fix various typo and macro guards issues Signed-off-by: Xiaokang Qian --- include/mbedtls/ssl.h | 8 ++++---- library/ssl_client.c | 2 ++ library/ssl_misc.h | 2 +- library/ssl_tls.c | 4 ++-- 4 files changed, 9 insertions(+), 7 deletions(-) diff --git a/include/mbedtls/ssl.h b/include/mbedtls/ssl.h index 4cf67044ae..88ccb27c4f 100644 --- a/include/mbedtls/ssl.h +++ b/include/mbedtls/ssl.h @@ -1188,6 +1188,10 @@ struct mbedtls_ssl_session uint8_t MBEDTLS_PRIVATE(resumption_key_len); /*!< resumption_key length */ unsigned char MBEDTLS_PRIVATE(resumption_key)[MBEDTLS_SSL_TLS1_3_TICKET_RESUMPTION_KEY_LEN]; +#if defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) && defined(MBEDTLS_SSL_CLI_C) + char *MBEDTLS_PRIVATE(hostname); /*!< host name binded with tickets */ +#endif /* MBEDTLS_SSL_SERVER_NAME_INDICATION && MBEDTLS_SSL_CLI_C */ + #if defined(MBEDTLS_HAVE_TIME) && defined(MBEDTLS_SSL_CLI_C) mbedtls_time_t MBEDTLS_PRIVATE(ticket_received); /*!< time ticket was received */ #endif /* MBEDTLS_HAVE_TIME && MBEDTLS_SSL_CLI_C */ @@ -1201,10 +1205,6 @@ struct mbedtls_ssl_session #if defined(MBEDTLS_SSL_PROTO_TLS1_3) mbedtls_ssl_tls13_application_secrets MBEDTLS_PRIVATE(app_secrets); -#if defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) && \ - defined(MBEDTLS_SSL_SESSION_TICKETS) - char *MBEDTLS_PRIVATE(hostname); /*!< host name binded with tickets */ -#endif /* MBEDTLS_SSL_SERVER_NAME_INDICATION && MBEDTLS_SSL_SESSION_TICKETS */ #endif /* MBEDTLS_SSL_PROTO_TLS1_3 */ }; diff --git a/library/ssl_client.c b/library/ssl_client.c index 341e882cce..10566deaa9 100644 --- a/library/ssl_client.c +++ b/library/ssl_client.c @@ -880,8 +880,10 @@ static int ssl_prepare_client_hello( mbedtls_ssl_context *ssl ) int hostname_mismatch = ssl->hostname != NULL || ssl->session_negotiate->hostname != NULL; if( ssl->hostname != NULL && ssl->session_negotiate->hostname != NULL ) + { hostname_mismatch = strcmp( ssl->hostname, ssl->session_negotiate->hostname ) != 0; + } if( hostname_mismatch ) { diff --git a/library/ssl_misc.h b/library/ssl_misc.h index 82a0d5c8a2..828937c3f6 100644 --- a/library/ssl_misc.h +++ b/library/ssl_misc.h @@ -2497,7 +2497,7 @@ int mbedtls_ssl_tls13_write_binders_of_pre_shared_key_ext( #if defined(MBEDTLS_SSL_PROTO_TLS1_3) && \ defined(MBEDTLS_SSL_SESSION_TICKETS) && \ defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) && \ - defined(MBEDTLS_SSL_TLS_C) + defined(MBEDTLS_SSL_CLI_C) MBEDTLS_CHECK_RETURN_CRITICAL int mbedtls_ssl_session_set_hostname( mbedtls_ssl_session *session, const char *hostname ); diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 521b922b7d..c677455132 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -8862,7 +8862,7 @@ int mbedtls_ssl_write_alpn_ext( mbedtls_ssl_context *ssl, #if defined(MBEDTLS_SSL_PROTO_TLS1_3) && \ defined(MBEDTLS_SSL_SESSION_TICKETS) && \ defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) && \ - defined(MBEDTLS_SSL_TLS_C) + defined(MBEDTLS_SSL_CLI_C) int mbedtls_ssl_session_set_hostname( mbedtls_ssl_session *session, const char *hostname ) { @@ -8907,6 +8907,6 @@ int mbedtls_ssl_session_set_hostname( mbedtls_ssl_session *session, #endif /* MBEDTLS_SSL_PROTO_TLS1_3 && MBEDTLS_SSL_SESSION_TICKETS && MBEDTLS_SSL_SERVER_NAME_INDICATION && - MBEDTLS_SSL_TLS_C */ + MBEDTLS_SSL_CLI_C */ #endif /* MBEDTLS_SSL_TLS_C */ From 8730644da1601547e3aaf4c551fcc993ada95b5a Mon Sep 17 00:00:00 2001 From: Xiaokang Qian Date: Wed, 12 Oct 2022 09:47:38 +0000 Subject: [PATCH 14/19] Move ticket and hostname set code just after shallow-copy Signed-off-by: Xiaokang Qian --- library/ssl_tls.c | 40 ++++++++++++++++++---------------------- 1 file changed, 18 insertions(+), 22 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index c677455132..d37102657a 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -245,8 +245,25 @@ int mbedtls_ssl_session_copy( mbedtls_ssl_session *dst, memcpy( dst, src, sizeof( mbedtls_ssl_session ) ); #if defined(MBEDTLS_SSL_SESSION_TICKETS) && defined(MBEDTLS_SSL_CLI_C) - dst->ticket = NULL; + if( src->ticket != NULL ) + { + dst->ticket = mbedtls_calloc( 1, src->ticket_len ); + if( dst->ticket == NULL ) + return( MBEDTLS_ERR_SSL_ALLOC_FAILED ); + + memcpy( dst->ticket, src->ticket, src->ticket_len ); + } + +#if defined(MBEDTLS_SSL_PROTO_TLS1_3) && \ + defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) + if( src->endpoint == MBEDTLS_SSL_IS_CLIENT ) + { + dst->hostname = NULL; + return mbedtls_ssl_session_set_hostname( dst, + src->hostname ); + } #endif +#endif /* MBEDTLS_SSL_SESSION_TICKETS && MBEDTLS_SSL_CLI_C */ #if defined(MBEDTLS_X509_CRT_PARSE_C) @@ -286,27 +303,6 @@ int mbedtls_ssl_session_copy( mbedtls_ssl_session *dst, #endif /* MBEDTLS_X509_CRT_PARSE_C */ -#if defined(MBEDTLS_SSL_SESSION_TICKETS) && defined(MBEDTLS_SSL_CLI_C) - if( src->ticket != NULL ) - { - dst->ticket = mbedtls_calloc( 1, src->ticket_len ); - if( dst->ticket == NULL ) - return( MBEDTLS_ERR_SSL_ALLOC_FAILED ); - - memcpy( dst->ticket, src->ticket, src->ticket_len ); - } - -#if defined(MBEDTLS_SSL_PROTO_TLS1_3) && \ - defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) - if( src->endpoint == MBEDTLS_SSL_IS_CLIENT ) - { - dst->hostname = NULL; - return mbedtls_ssl_session_set_hostname( dst, - src->hostname ); - } -#endif -#endif /* MBEDTLS_SSL_SESSION_TICKETS && MBEDTLS_SSL_CLI_C */ - return( 0 ); } From baa4764d770ba5829453755d77610f215ba7e84e Mon Sep 17 00:00:00 2001 From: Xiaokang Qian Date: Wed, 12 Oct 2022 10:21:27 +0000 Subject: [PATCH 15/19] Fix typo issues Signed-off-by: Xiaokang Qian --- include/mbedtls/ssl.h | 3 +-- library/ssl_tls.c | 4 +--- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/include/mbedtls/ssl.h b/include/mbedtls/ssl.h index 88ccb27c4f..09b1dfcf85 100644 --- a/include/mbedtls/ssl.h +++ b/include/mbedtls/ssl.h @@ -1204,8 +1204,7 @@ struct mbedtls_ssl_session #if defined(MBEDTLS_SSL_PROTO_TLS1_3) mbedtls_ssl_tls13_application_secrets MBEDTLS_PRIVATE(app_secrets); - -#endif /* MBEDTLS_SSL_PROTO_TLS1_3 */ +#endif }; /* diff --git a/library/ssl_tls.c b/library/ssl_tls.c index d37102657a..2de52314d0 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -262,7 +262,7 @@ int mbedtls_ssl_session_copy( mbedtls_ssl_session *dst, return mbedtls_ssl_session_set_hostname( dst, src->hostname ); } -#endif +#endif /* MBEDTLS_SSL_PROTO_TLS1_3 && MBEDTLS_SSL_SERVER_NAME_INDICATION */ #endif /* MBEDTLS_SSL_SESSION_TICKETS && MBEDTLS_SSL_CLI_C */ #if defined(MBEDTLS_X509_CRT_PARSE_C) @@ -2022,8 +2022,6 @@ static int ssl_tls13_session_save( const mbedtls_ssl_session *session, memcpy( p, session->resumption_key, session->resumption_key_len ); p += session->resumption_key_len; - - #if defined(MBEDTLS_HAVE_TIME) && defined(MBEDTLS_SSL_SRV_C) if( session->endpoint == MBEDTLS_SSL_IS_SERVER ) { From 307a7303fda210f86be5ac04d4a677337ae0b719 Mon Sep 17 00:00:00 2001 From: Xiaokang Qian Date: Wed, 12 Oct 2022 11:14:32 +0000 Subject: [PATCH 16/19] Rebase and replace session_negotiate Signed-off-by: Xiaokang Qian --- library/ssl_client.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/library/ssl_client.c b/library/ssl_client.c index 10566deaa9..9e4f0218f9 100644 --- a/library/ssl_client.c +++ b/library/ssl_client.c @@ -878,11 +878,11 @@ static int ssl_prepare_client_hello( mbedtls_ssl_context *ssl ) ssl->handshake->resume ) { int hostname_mismatch = ssl->hostname != NULL || - ssl->session_negotiate->hostname != NULL; - if( ssl->hostname != NULL && ssl->session_negotiate->hostname != NULL ) + session_negotiate->hostname != NULL; + if( ssl->hostname != NULL && session_negotiate->hostname != NULL ) { hostname_mismatch = strcmp( - ssl->hostname, ssl->session_negotiate->hostname ) != 0; + ssl->hostname, session_negotiate->hostname ) != 0; } if( hostname_mismatch ) @@ -895,7 +895,7 @@ static int ssl_prepare_client_hello( mbedtls_ssl_context *ssl ) } else { - return mbedtls_ssl_session_set_hostname( ssl->session_negotiate, + return mbedtls_ssl_session_set_hostname( session_negotiate, ssl->hostname ); } #endif /* MBEDTLS_SSL_PROTO_TLS1_3 && From 997669aeeba045e8573a1c49f5922f8aa4d4f85f Mon Sep 17 00:00:00 2001 From: Xiaokang Qian Date: Wed, 12 Oct 2022 14:30:27 +0000 Subject: [PATCH 17/19] Fix heap use-after-free corruption issue Signed-off-by: Xiaokang Qian --- library/ssl_tls.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 2de52314d0..a36f5b1f12 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -258,9 +258,12 @@ int mbedtls_ssl_session_copy( mbedtls_ssl_session *dst, defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) if( src->endpoint == MBEDTLS_SSL_IS_CLIENT ) { + int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; dst->hostname = NULL; - return mbedtls_ssl_session_set_hostname( dst, - src->hostname ); + ret = mbedtls_ssl_session_set_hostname( dst, + src->hostname ); + if( ret != 0) + return ret; } #endif /* MBEDTLS_SSL_PROTO_TLS1_3 && MBEDTLS_SSL_SERVER_NAME_INDICATION */ #endif /* MBEDTLS_SSL_SESSION_TICKETS && MBEDTLS_SSL_CLI_C */ From 126bf8e4d78b72420cb728f749e2973bf7e66fdc Mon Sep 17 00:00:00 2001 From: Xiaokang Qian Date: Thu, 13 Oct 2022 02:22:40 +0000 Subject: [PATCH 18/19] Address some comments Delete reference immediately after shallow copy Fix format issues Signed-off-by: Xiaokang Qian --- library/ssl_tls.c | 53 ++++++++++++++++++++++++++--------------------- 1 file changed, 29 insertions(+), 24 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index a36f5b1f12..610df54b4e 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -243,29 +243,12 @@ int mbedtls_ssl_session_copy( mbedtls_ssl_session *dst, { mbedtls_ssl_session_free( dst ); memcpy( dst, src, sizeof( mbedtls_ssl_session ) ); - #if defined(MBEDTLS_SSL_SESSION_TICKETS) && defined(MBEDTLS_SSL_CLI_C) - if( src->ticket != NULL ) - { - dst->ticket = mbedtls_calloc( 1, src->ticket_len ); - if( dst->ticket == NULL ) - return( MBEDTLS_ERR_SSL_ALLOC_FAILED ); - - memcpy( dst->ticket, src->ticket, src->ticket_len ); - } - + dst->ticket = NULL; #if defined(MBEDTLS_SSL_PROTO_TLS1_3) && \ defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) - if( src->endpoint == MBEDTLS_SSL_IS_CLIENT ) - { - int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; - dst->hostname = NULL; - ret = mbedtls_ssl_session_set_hostname( dst, - src->hostname ); - if( ret != 0) - return ret; - } -#endif /* MBEDTLS_SSL_PROTO_TLS1_3 && MBEDTLS_SSL_SERVER_NAME_INDICATION */ + dst->hostname = NULL; +#endif #endif /* MBEDTLS_SSL_SESSION_TICKETS && MBEDTLS_SSL_CLI_C */ #if defined(MBEDTLS_X509_CRT_PARSE_C) @@ -306,6 +289,29 @@ int mbedtls_ssl_session_copy( mbedtls_ssl_session *dst, #endif /* MBEDTLS_X509_CRT_PARSE_C */ +#if defined(MBEDTLS_SSL_SESSION_TICKETS) && defined(MBEDTLS_SSL_CLI_C) + if( src->ticket != NULL ) + { + dst->ticket = mbedtls_calloc( 1, src->ticket_len ); + if( dst->ticket == NULL ) + return( MBEDTLS_ERR_SSL_ALLOC_FAILED ); + + memcpy( dst->ticket, src->ticket, src->ticket_len ); + } + +#if defined(MBEDTLS_SSL_PROTO_TLS1_3) && \ + defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) + if( src->endpoint == MBEDTLS_SSL_IS_CLIENT ) + { + int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; + ret = mbedtls_ssl_session_set_hostname( dst, src->hostname ); + if( ret != 0 ) + return ( ret ); + } +#endif /* MBEDTLS_SSL_PROTO_TLS1_3 && + MBEDTLS_SSL_SERVER_NAME_INDICATION */ +#endif /* MBEDTLS_SSL_SESSION_TICKETS && MBEDTLS_SSL_CLI_C */ + return( 0 ); } @@ -1943,7 +1949,7 @@ mbedtls_ssl_mode_t mbedtls_ssl_get_mode_from_ciphersuite( /* Serialization of TLS 1.3 sessions: * * struct { - * opaque hostname<1..2^16-1>; + * opaque hostname<0..2^16-1>; * uint64 ticket_received; * uint32 ticket_lifetime; * opaque ticket<1..2^16-1>; @@ -1993,8 +1999,7 @@ static int ssl_tls13_session_save( const mbedtls_ssl_session *session, #if defined(MBEDTLS_SSL_CLI_C) if( session->endpoint == MBEDTLS_SSL_IS_CLIENT ) { -#if defined(MBEDTLS_SSL_SESSION_TICKETS) && \ - defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) +#if defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) needed += 2 /* hostname_len */ + hostname_len; /* hostname */ #endif @@ -2039,7 +2044,7 @@ static int ssl_tls13_session_save( const mbedtls_ssl_session *session, #if defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) MBEDTLS_PUT_UINT16_BE( hostname_len, p, 0 ); p += 2; - if ( hostname_len > 0 ) + if( hostname_len > 0 ) { /* save host name */ memcpy( p, session->hostname, hostname_len ); From 28af501cae7f74a352c9b4f0ab5b41768f653a8c Mon Sep 17 00:00:00 2001 From: Xiaokang Qian Date: Thu, 13 Oct 2022 08:18:19 +0000 Subject: [PATCH 19/19] Fix the ticket_lifetime equal to 0 issue Signed-off-by: Xiaokang Qian --- library/ssl_tls13_server.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/library/ssl_tls13_server.c b/library/ssl_tls13_server.c index 61a1bad578..c8a7827501 100644 --- a/library/ssl_tls13_server.c +++ b/library/ssl_tls13_server.c @@ -2848,7 +2848,8 @@ static int ssl_tls13_write_new_session_ticket_body( mbedtls_ssl_context *ssl, * MAY treat a ticket as valid for a shorter period of time than what * is stated in the ticket_lifetime. */ - ticket_lifetime %= 604800; + if( ticket_lifetime > 604800 ) + ticket_lifetime = 604800; MBEDTLS_PUT_UINT32_BE( ticket_lifetime, p, 0 ); MBEDTLS_SSL_DEBUG_MSG( 3, ( "ticket_lifetime: %u", ( unsigned int )ticket_lifetime ) );