From 93ba3e39185f82a3469387c3b0f95ccbed8d283a Mon Sep 17 00:00:00 2001 From: Paul Elliott Date: Fri, 18 Mar 2022 21:51:13 +0000 Subject: [PATCH 1/2] Add mbedtls_ssl_is_handshake_over() function Add function to query if SSL handshake is over or not, in order to determine when to stop calling mbedtls_ssl_handshake_step among other things. Document function, and add warnings that the previous method of ascertaining if handshake was over is now deprecated, and may break in future releases. Signed-off-by: Paul Elliott --- ChangeLog.d/add_handshake_completion_accessor | 4 +++ include/mbedtls/ssl.h | 31 ++++++++++++++++++- 2 files changed, 34 insertions(+), 1 deletion(-) create mode 100644 ChangeLog.d/add_handshake_completion_accessor diff --git a/ChangeLog.d/add_handshake_completion_accessor b/ChangeLog.d/add_handshake_completion_accessor new file mode 100644 index 000000000..e2b28cf63 --- /dev/null +++ b/ChangeLog.d/add_handshake_completion_accessor @@ -0,0 +1,4 @@ +Features + * Add function mbedtls_ssl_is_handshake_over() to enable querying if the SSL + Handshake has completed or not, and thus whether to continue calling + mbedtls_ssl_handshake_step(), requested in #4383 diff --git a/include/mbedtls/ssl.h b/include/mbedtls/ssl.h index b819bbad8..8751fa8bf 100644 --- a/include/mbedtls/ssl.h +++ b/include/mbedtls/ssl.h @@ -4357,12 +4357,41 @@ int mbedtls_ssl_get_session( const mbedtls_ssl_context *ssl, */ int mbedtls_ssl_handshake( mbedtls_ssl_context *ssl ); +/** + * \brief After calling mbedtls_ssl_handshake() to start the SSL + * handshake you can call this function to check whether the + * handshake is over for a given SSL context. This function + * should be also used to determine when to stop calling + * mbedtls_handshake_step() for that context. + * + * \param ssl SSL context + * + * \return \c 1 if handshake is over, \c 0 if it is still ongoing. + */ +static inline int mbedtls_ssl_is_handshake_over( mbedtls_ssl_context *ssl ) +{ + return( ssl->MBEDTLS_PRIVATE( state ) == MBEDTLS_SSL_HANDSHAKE_OVER ); +} + /** * \brief Perform a single step of the SSL handshake * * \note The state of the context (ssl->state) will be at * the next state after this function returns \c 0. Do not - * call this function if state is MBEDTLS_SSL_HANDSHAKE_OVER. + * call this function if mbedtls_ssl_is_handshake_over() + * returns \c 1. + * + * \warning Whilst in the past you may have used direct access to the + * context state (ssl->state) in order to ascertain when to + * stop calling this function and although you can still do + * so with something like ssl->MBEDTLS_PRIVATE(state) or by + * defining MBEDTLS_ALLOW_PRIVATE_ACCESS, this is now + * considered deprecated and could be broken in any future + * release. If you still find you have good reason for such + * direct access, then please do contact the team to explain + * this (raise an issue or post to the mailing list), so that + * we can add a solution to your problem that will be + * guaranteed to work in the future. * * \param ssl SSL context * From 27b0d94e25f075dc1cf780e05d27b4da2a3174f7 Mon Sep 17 00:00:00 2001 From: Paul Elliott Date: Fri, 18 Mar 2022 21:55:32 +0000 Subject: [PATCH 2/2] Use mbedtls_ssl_is_handshake_over() Switch over to using the new function both internally and in tests. Signed-off-by: Paul Elliott --- library/ssl_msg.c | 28 ++++++++++++++-------------- library/ssl_tls.c | 12 ++++++------ tests/suites/test_suite_ssl.function | 6 +++--- 3 files changed, 23 insertions(+), 23 deletions(-) diff --git a/library/ssl_msg.c b/library/ssl_msg.c index 5f80ed511..f3bebf4b7 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -1820,7 +1820,7 @@ int mbedtls_ssl_fetch_input( mbedtls_ssl_context *ssl, size_t nb_want ) { len = in_buf_len - ( ssl->in_hdr - ssl->in_buf ); - if( ssl->state != MBEDTLS_SSL_HANDSHAKE_OVER ) + if( mbedtls_ssl_is_handshake_over( ssl ) == 0 ) timeout = ssl->handshake->retransmit_timeout; else timeout = ssl->conf->read_timeout; @@ -1844,7 +1844,7 @@ int mbedtls_ssl_fetch_input( mbedtls_ssl_context *ssl, size_t nb_want ) MBEDTLS_SSL_DEBUG_MSG( 2, ( "timeout" ) ); mbedtls_ssl_set_timer( ssl, 0 ); - if( ssl->state != MBEDTLS_SSL_HANDSHAKE_OVER ) + if( mbedtls_ssl_is_handshake_over( ssl ) == 0 ) { if( ssl_double_retransmit_timeout( ssl ) != 0 ) { @@ -2279,7 +2279,7 @@ int mbedtls_ssl_flight_transmit( mbedtls_ssl_context *ssl ) return( ret ); /* Update state and set timer */ - if( ssl->state == MBEDTLS_SSL_HANDSHAKE_OVER ) + if( mbedtls_ssl_is_handshake_over( ssl ) == 1 ) ssl->handshake->retransmit_state = MBEDTLS_SSL_RETRANS_FINISHED; else { @@ -2835,9 +2835,9 @@ int mbedtls_ssl_prepare_handshake_record( mbedtls_ssl_context *ssl ) } if( ssl->handshake != NULL && - ( ( ssl->state != MBEDTLS_SSL_HANDSHAKE_OVER && + ( ( mbedtls_ssl_is_handshake_over( ssl ) == 0 && recv_msg_seq != ssl->handshake->in_msg_seq ) || - ( ssl->state == MBEDTLS_SSL_HANDSHAKE_OVER && + ( mbedtls_ssl_is_handshake_over( ssl ) == 1 && ssl->in_msg[0] != MBEDTLS_SSL_HS_CLIENT_HELLO ) ) ) { if( recv_msg_seq > ssl->handshake->in_msg_seq ) @@ -2903,7 +2903,7 @@ void mbedtls_ssl_update_handshake_status( mbedtls_ssl_context *ssl ) { mbedtls_ssl_handshake_params * const hs = ssl->handshake; - if( ssl->state != MBEDTLS_SSL_HANDSHAKE_OVER && hs != NULL ) + if( mbedtls_ssl_is_handshake_over( ssl ) == 0 && hs != NULL ) { ssl->handshake->update_checksum( ssl, ssl->in_msg, ssl->in_hslen ); } @@ -3524,7 +3524,7 @@ static int ssl_check_client_reconnect( mbedtls_ssl_context *ssl ) */ if( rec_epoch == 0 && ssl->conf->endpoint == MBEDTLS_SSL_IS_SERVER && - ssl->state == MBEDTLS_SSL_HANDSHAKE_OVER && + mbedtls_ssl_is_handshake_over( ssl ) == 1 && ssl->in_msgtype == MBEDTLS_SSL_MSG_HANDSHAKE && ssl->in_left > 13 && ssl->in_buf[13] == MBEDTLS_SSL_HS_CLIENT_HELLO ) @@ -4681,7 +4681,7 @@ int mbedtls_ssl_handle_message_type( mbedtls_ssl_context *ssl ) /* Drop unexpected ApplicationData records, * except at the beginning of renegotiations */ if( ssl->in_msgtype == MBEDTLS_SSL_MSG_APPLICATION_DATA && - ssl->state != MBEDTLS_SSL_HANDSHAKE_OVER + mbedtls_ssl_is_handshake_over( ssl ) == 0 #if defined(MBEDTLS_SSL_RENEGOTIATION) && ! ( ssl->renego_status == MBEDTLS_SSL_RENEGOTIATION_IN_PROGRESS && ssl->state == MBEDTLS_SSL_SERVER_HELLO ) @@ -4693,7 +4693,7 @@ int mbedtls_ssl_handle_message_type( mbedtls_ssl_context *ssl ) } if( ssl->handshake != NULL && - ssl->state == MBEDTLS_SSL_HANDSHAKE_OVER ) + mbedtls_ssl_is_handshake_over( ssl ) == 1 ) { mbedtls_ssl_handshake_wrapup_free_hs_transform( ssl ); } @@ -5117,7 +5117,7 @@ static int ssl_check_ctr_renegotiate( mbedtls_ssl_context *ssl ) int in_ctr_cmp; int out_ctr_cmp; - if( ssl->state != MBEDTLS_SSL_HANDSHAKE_OVER || + if( mbedtls_ssl_is_handshake_over( ssl ) == 0 || ssl->renego_status == MBEDTLS_SSL_RENEGOTIATION_PENDING || ssl->conf->disable_renegotiation == MBEDTLS_SSL_RENEGOTIATION_DISABLED ) { @@ -5295,7 +5295,7 @@ int mbedtls_ssl_read( mbedtls_ssl_context *ssl, unsigned char *buf, size_t len ) } #endif - if( ssl->state != MBEDTLS_SSL_HANDSHAKE_OVER ) + if( mbedtls_ssl_is_handshake_over( ssl ) == 0 ) { ret = mbedtls_ssl_handshake( ssl ); if( ret != MBEDTLS_ERR_SSL_WAITING_SERVER_HELLO_RENEGO && @@ -5406,7 +5406,7 @@ int mbedtls_ssl_read( mbedtls_ssl_context *ssl, unsigned char *buf, size_t len ) /* We're going to return something now, cancel timer, * except if handshake (renegotiation) is in progress */ - if( ssl->state == MBEDTLS_SSL_HANDSHAKE_OVER ) + if( mbedtls_ssl_is_handshake_over( ssl ) == 1 ) mbedtls_ssl_set_timer( ssl, 0 ); #if defined(MBEDTLS_SSL_PROTO_DTLS) @@ -5550,7 +5550,7 @@ int mbedtls_ssl_write( mbedtls_ssl_context *ssl, const unsigned char *buf, size_ } #endif - if( ssl->state != MBEDTLS_SSL_HANDSHAKE_OVER ) + if( mbedtls_ssl_is_handshake_over( ssl ) == 0 ) { if( ( ret = mbedtls_ssl_handshake( ssl ) ) != 0 ) { @@ -5581,7 +5581,7 @@ int mbedtls_ssl_close_notify( mbedtls_ssl_context *ssl ) if( ssl->out_left != 0 ) return( mbedtls_ssl_flush_output( ssl ) ); - if( ssl->state == MBEDTLS_SSL_HANDSHAKE_OVER ) + if( mbedtls_ssl_is_handshake_over( ssl ) == 1 ) { if( ( ret = mbedtls_ssl_send_alert_message( ssl, MBEDTLS_SSL_ALERT_LEVEL_WARNING, diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 2220721f1..1d7438d68 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -120,7 +120,7 @@ int mbedtls_ssl_get_peer_cid( mbedtls_ssl_context *ssl, *enabled = MBEDTLS_SSL_CID_DISABLED; if( ssl->conf->transport != MBEDTLS_SSL_TRANSPORT_DATAGRAM || - ssl->state != MBEDTLS_SSL_HANDSHAKE_OVER ) + mbedtls_ssl_is_handshake_over( ssl ) == 0 ) { return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); } @@ -2750,7 +2750,7 @@ int mbedtls_ssl_handshake_step( mbedtls_ssl_context *ssl ) if( ssl == NULL || ssl->conf == NULL || ssl->handshake == NULL || - ssl->state == MBEDTLS_SSL_HANDSHAKE_OVER ) + mbedtls_ssl_is_handshake_over( ssl ) == 1 ) { return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); } @@ -2833,7 +2833,7 @@ int mbedtls_ssl_handshake( mbedtls_ssl_context *ssl ) MBEDTLS_SSL_DEBUG_MSG( 2, ( "=> handshake" ) ); /* Main handshake loop */ - while( ssl->state != MBEDTLS_SSL_HANDSHAKE_OVER ) + while( mbedtls_ssl_is_handshake_over( ssl ) == 0 ) { ret = mbedtls_ssl_handshake_step( ssl ); @@ -2933,7 +2933,7 @@ int mbedtls_ssl_renegotiate( mbedtls_ssl_context *ssl ) /* On server, just send the request */ if( ssl->conf->endpoint == MBEDTLS_SSL_IS_SERVER ) { - if( ssl->state != MBEDTLS_SSL_HANDSHAKE_OVER ) + if( mbedtls_ssl_is_handshake_over( ssl ) == 0 ) return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); ssl->renego_status = MBEDTLS_SSL_RENEGOTIATION_PENDING; @@ -2953,7 +2953,7 @@ int mbedtls_ssl_renegotiate( mbedtls_ssl_context *ssl ) */ if( ssl->renego_status != MBEDTLS_SSL_RENEGOTIATION_IN_PROGRESS ) { - if( ssl->state != MBEDTLS_SSL_HANDSHAKE_OVER ) + if( mbedtls_ssl_is_handshake_over( ssl ) == 0 ) return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); if( ( ret = mbedtls_ssl_start_renegotiation( ssl ) ) != 0 ) @@ -3235,7 +3235,7 @@ int mbedtls_ssl_context_save( mbedtls_ssl_context *ssl, * (only DTLS) but are currently used to simplify the implementation. */ /* The initial handshake must be over */ - if( ssl->state != MBEDTLS_SSL_HANDSHAKE_OVER ) + if( mbedtls_ssl_is_handshake_over( ssl ) == 0 ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "Initial handshake isn't over" ) ); return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); diff --git a/tests/suites/test_suite_ssl.function b/tests/suites/test_suite_ssl.function index 855cfc77a..65313b940 100644 --- a/tests/suites/test_suite_ssl.function +++ b/tests/suites/test_suite_ssl.function @@ -1044,7 +1044,7 @@ int mbedtls_move_handshake_to_state( mbedtls_ssl_context *ssl, { /* If /p second_ssl ends the handshake procedure before /p ssl then * there is no need to call the next step */ - if( second_ssl->state != MBEDTLS_SSL_HANDSHAKE_OVER ) + if( !mbedtls_ssl_is_handshake_over( second_ssl ) ) { ret = mbedtls_ssl_handshake_step( second_ssl ); if( ret != 0 && ret != MBEDTLS_ERR_SSL_WANT_READ && @@ -2048,8 +2048,8 @@ void perform_handshake( handshake_test_options* options ) goto exit; } - TEST_ASSERT( client.ssl.state == MBEDTLS_SSL_HANDSHAKE_OVER ); - TEST_ASSERT( server.ssl.state == MBEDTLS_SSL_HANDSHAKE_OVER ); + TEST_ASSERT( mbedtls_ssl_is_handshake_over( &client.ssl ) == 1 ); + TEST_ASSERT( mbedtls_ssl_is_handshake_over( &server.ssl ) == 1 ); /* Check that both sides have negotiated the expected version. */ mbedtls_test_set_step( 0 );