From af0665d8b0f39d14ba88209dfdb169a0b99d3734 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 24 May 2017 09:16:26 +0100 Subject: [PATCH] Simplify retaining of messages for future processing There are situations in which it is not clear what message to expect next. For example, the message following the ServerHello might be either a Certificate, a ServerKeyExchange or a CertificateRequest. We deal with this situation in the following way: Initially, the message processing function for one of the allowed message types is called, which fetches and decodes a new message. If that message is not the expected one, the function returns successfully (instead of throwing an error as usual for unexpected messages), and the handshake continues to the processing function for the next possible message. To not have this function fetch a new message, a flag in the SSL context structure is used to indicate that the last message was retained for further processing, and if that's set, the following processing function will not fetch a new record. This commit simplifies the usage of this message-retaining parameter by doing the check within the record-fetching routine instead of the specific message-processing routines. The code gets cleaner this way and allows retaining messages to be used in other situations as well without much effort. This will be used in the next commits. --- include/mbedtls/ssl.h | 4 ++- library/ssl_cli.c | 70 ++++++++++++++++++++----------------------- library/ssl_tls.c | 37 ++++++++++++++--------- 3 files changed, 59 insertions(+), 52 deletions(-) diff --git a/include/mbedtls/ssl.h b/include/mbedtls/ssl.h index 49b1aca8c..51c1c60d7 100644 --- a/include/mbedtls/ssl.h +++ b/include/mbedtls/ssl.h @@ -845,7 +845,9 @@ struct mbedtls_ssl_context size_t in_hslen; /*!< current handshake message length, including the handshake header */ int nb_zero; /*!< # of 0-length encrypted messages */ - int record_read; /*!< record is already present */ + + int keep_current_message; /*!< drop or reuse current message + on next call to record layer? */ /* * Record layer (outgoing data) diff --git a/library/ssl_cli.c b/library/ssl_cli.c index 616cadc08..a2b9f8cfe 100644 --- a/library/ssl_cli.c +++ b/library/ssl_cli.c @@ -1471,6 +1471,8 @@ static int ssl_parse_server_hello( mbedtls_ssl_context *ssl ) } MBEDTLS_SSL_DEBUG_MSG( 1, ( "non-handshake message during renego" ) ); + + ssl->keep_current_message = 1; return( MBEDTLS_ERR_SSL_WAITING_SERVER_HELLO_RENEGO ); } #endif /* MBEDTLS_SSL_RENEGOTIATION */ @@ -2316,13 +2318,17 @@ static int ssl_parse_server_key_exchange( mbedtls_ssl_context *ssl ) if( ciphersuite_info->key_exchange == MBEDTLS_KEY_EXCHANGE_PSK || ciphersuite_info->key_exchange == MBEDTLS_KEY_EXCHANGE_RSA_PSK ) { - ssl->record_read = 1; + /* Current message is probably either + * CertificateRequest or ServerHelloDone */ + ssl->keep_current_message = 1; goto exit; } - MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad server key exchange message" ) ); + MBEDTLS_SSL_DEBUG_MSG( 1, ( "server key exchange message must " + "not be skipped" ) ); mbedtls_ssl_send_alert_message( ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, MBEDTLS_SSL_ALERT_MSG_UNEXPECTED_MESSAGE ); + return( MBEDTLS_ERR_SSL_UNEXPECTED_MESSAGE ); } @@ -2640,38 +2646,32 @@ static int ssl_parse_certificate_request( mbedtls_ssl_context *ssl ) return( 0 ); } - if( ssl->record_read == 0 ) + if( ( ret = mbedtls_ssl_read_record( ssl ) ) != 0 ) { - if( ( ret = mbedtls_ssl_read_record( ssl ) ) != 0 ) - { - MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_ssl_read_record", ret ); - return( ret ); - } - - if( ssl->in_msgtype != MBEDTLS_SSL_MSG_HANDSHAKE ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad certificate request message" ) ); - mbedtls_ssl_send_alert_message( ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, - MBEDTLS_SSL_ALERT_MSG_UNEXPECTED_MESSAGE ); - return( MBEDTLS_ERR_SSL_UNEXPECTED_MESSAGE ); - } - - ssl->record_read = 1; + MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_ssl_read_record", ret ); + return( ret ); } - ssl->client_auth = 0; - ssl->state++; + if( ssl->in_msgtype != MBEDTLS_SSL_MSG_HANDSHAKE ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad certificate request message" ) ); + mbedtls_ssl_send_alert_message( ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, + MBEDTLS_SSL_ALERT_MSG_UNEXPECTED_MESSAGE ); + return( MBEDTLS_ERR_SSL_UNEXPECTED_MESSAGE ); + } - if( ssl->in_msg[0] == MBEDTLS_SSL_HS_CERTIFICATE_REQUEST ) - ssl->client_auth++; + ssl->state++; + ssl->client_auth = ( ssl->in_msg[0] == MBEDTLS_SSL_HS_CERTIFICATE_REQUEST ); MBEDTLS_SSL_DEBUG_MSG( 3, ( "got %s certificate request", ssl->client_auth ? "a" : "no" ) ); if( ssl->client_auth == 0 ) + { + /* Current message is probably the ServerHelloDone */ + ssl->keep_current_message = 1; goto exit; - - ssl->record_read = 0; + } /* * struct { @@ -2766,21 +2766,17 @@ static int ssl_parse_server_hello_done( mbedtls_ssl_context *ssl ) MBEDTLS_SSL_DEBUG_MSG( 2, ( "=> parse server hello done" ) ); - if( ssl->record_read == 0 ) + if( ( ret = mbedtls_ssl_read_record( ssl ) ) != 0 ) { - if( ( ret = mbedtls_ssl_read_record( ssl ) ) != 0 ) - { - MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_ssl_read_record", ret ); - return( ret ); - } - - if( ssl->in_msgtype != MBEDTLS_SSL_MSG_HANDSHAKE ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad server hello done message" ) ); - return( MBEDTLS_ERR_SSL_UNEXPECTED_MESSAGE ); - } + MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_ssl_read_record", ret ); + return( ret ); + } + + if( ssl->in_msgtype != MBEDTLS_SSL_MSG_HANDSHAKE ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad server hello done message" ) ); + return( MBEDTLS_ERR_SSL_UNEXPECTED_MESSAGE ); } - ssl->record_read = 0; if( ssl->in_hslen != mbedtls_ssl_hs_hdr_len( ssl ) || ssl->in_msg[0] != MBEDTLS_SSL_HS_SERVER_HELLO_DONE ) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 208500a66..0a29970e8 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -3720,27 +3720,35 @@ int mbedtls_ssl_read_record( mbedtls_ssl_context *ssl ) MBEDTLS_SSL_DEBUG_MSG( 2, ( "=> read record" ) ); - do { + if( ssl->keep_current_message == 0 ) + { + do { - if( ( ret = mbedtls_ssl_read_record_layer( ssl ) ) != 0 ) + if( ( ret = mbedtls_ssl_read_record_layer( ssl ) ) != 0 ) + { + MBEDTLS_SSL_DEBUG_RET( 1, ( "mbedtls_ssl_read_record_layer" ), ret ); + return( ret ); + } + + ret = mbedtls_ssl_handle_message_type( ssl ); + + } while( MBEDTLS_ERR_SSL_NON_FATAL == ret ); + + if( 0 != ret ) { MBEDTLS_SSL_DEBUG_RET( 1, ( "mbedtls_ssl_read_record_layer" ), ret ); return( ret ); } - ret = mbedtls_ssl_handle_message_type( ssl ); - - } while( MBEDTLS_ERR_SSL_NON_FATAL == ret ); - - if( 0 != ret ) - { - MBEDTLS_SSL_DEBUG_RET( 1, ( "mbedtls_ssl_handle_message_type" ), ret ); - return( ret ); + if( ssl->in_msgtype == MBEDTLS_SSL_MSG_HANDSHAKE ) + { + mbedtls_ssl_update_handshake_status( ssl ); + } } - - if( ssl->in_msgtype == MBEDTLS_SSL_MSG_HANDSHAKE ) + else { - mbedtls_ssl_update_handshake_status( ssl ); + MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= reuse previously read message" ) ); + ssl->keep_current_message = 0; } MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= read record" ) ); @@ -5603,7 +5611,8 @@ static int ssl_session_reset_int( mbedtls_ssl_context *ssl, int partial ) ssl->in_hslen = 0; ssl->nb_zero = 0; - ssl->record_read = 0; + + ssl->keep_current_message = 0; ssl->out_msg = ssl->out_buf + 13; ssl->out_msgtype = 0;