From 2f9efd3038468da8760149d17716b4ec3bb0c5b3 Mon Sep 17 00:00:00 2001 From: Xiaokang Qian Date: Mon, 10 Oct 2022 11:24:08 +0000 Subject: [PATCH] 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 \