From 327c93b1824c0e086ed45b325659ad0fb8f3c428 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 15 Aug 2018 13:56:18 +0100 Subject: [PATCH] Add parameter to ssl_read_record() controlling checksum update Previously, mbedtls_ssl_read_record() always updated the handshake checksum in case a handshake record was received. While desirable most of the time, for the CertificateVerify message the checksum update must only happen after the message has been fully processed, because the validation requires the handshake digest up to but excluding the CertificateVerify itself. As a remedy, the bulk of mbedtls_ssl_read_record() was previously duplicated within ssl_parse_certificate_verify(), hardening maintenance in case mbedtls_ssl_read_record() is subject to changes. This commit adds a boolean parameter to mbedtls_ssl_read_record() indicating whether the checksum should be updated in case of a handshake message or not. This allows using it also for ssl_parse_certificate_verify(), manually updating the checksum after the message has been processed. --- include/mbedtls/ssl_internal.h | 2 +- library/ssl_cli.c | 10 +++++----- library/ssl_srv.c | 21 +++------------------ library/ssl_tls.c | 16 +++++++++------- 4 files changed, 18 insertions(+), 31 deletions(-) diff --git a/include/mbedtls/ssl_internal.h b/include/mbedtls/ssl_internal.h index 765da7a71..c817def23 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -557,7 +557,7 @@ void mbedtls_ssl_update_handshake_status( mbedtls_ssl_context *ssl ); * following the above definition. * */ -int mbedtls_ssl_read_record( mbedtls_ssl_context *ssl ); +int mbedtls_ssl_read_record( mbedtls_ssl_context *ssl, unsigned update_digest ); int mbedtls_ssl_fetch_input( mbedtls_ssl_context *ssl, size_t nb_want ); int mbedtls_ssl_write_handshake_msg( mbedtls_ssl_context *ssl ); diff --git a/library/ssl_cli.c b/library/ssl_cli.c index 73e4391a0..d160c42d0 100644 --- a/library/ssl_cli.c +++ b/library/ssl_cli.c @@ -1500,7 +1500,7 @@ static int ssl_parse_server_hello( mbedtls_ssl_context *ssl ) buf = ssl->in_msg; - if( ( ret = mbedtls_ssl_read_record( ssl ) ) != 0 ) + if( ( ret = mbedtls_ssl_read_record( ssl, 1 ) ) != 0 ) { /* No alert on a read error. */ MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_ssl_read_record", ret ); @@ -2349,7 +2349,7 @@ static int ssl_parse_server_key_exchange( mbedtls_ssl_context *ssl ) #endif /* MBEDTLS_KEY_EXCHANGE_ECDH_RSA_ENABLED || MBEDTLS_KEY_EXCHANGE_ECDH_ECDSA_ENABLED */ - if( ( ret = mbedtls_ssl_read_record( ssl ) ) != 0 ) + if( ( ret = mbedtls_ssl_read_record( ssl, 1 ) ) != 0 ) { MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_ssl_read_record", ret ); return( ret ); @@ -2656,7 +2656,7 @@ static int ssl_parse_certificate_request( mbedtls_ssl_context *ssl ) return( 0 ); } - if( ( ret = mbedtls_ssl_read_record( ssl ) ) != 0 ) + if( ( ret = mbedtls_ssl_read_record( ssl, 1 ) ) != 0 ) { MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_ssl_read_record", ret ); return( ret ); @@ -2808,7 +2808,7 @@ static int ssl_parse_server_hello_done( mbedtls_ssl_context *ssl ) MBEDTLS_SSL_DEBUG_MSG( 2, ( "=> parse server hello done" ) ); - if( ( ret = mbedtls_ssl_read_record( ssl ) ) != 0 ) + if( ( ret = mbedtls_ssl_read_record( ssl, 1 ) ) != 0 ) { MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_ssl_read_record", ret ); return( ret ); @@ -3297,7 +3297,7 @@ static int ssl_parse_new_session_ticket( mbedtls_ssl_context *ssl ) MBEDTLS_SSL_DEBUG_MSG( 2, ( "=> parse new session ticket" ) ); - if( ( ret = mbedtls_ssl_read_record( ssl ) ) != 0 ) + if( ( ret = mbedtls_ssl_read_record( ssl, 1 ) ) != 0 ) { MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_ssl_read_record", ret ); return( ret ); diff --git a/library/ssl_srv.c b/library/ssl_srv.c index 7101f461f..84c83e330 100644 --- a/library/ssl_srv.c +++ b/library/ssl_srv.c @@ -3728,7 +3728,7 @@ static int ssl_parse_client_key_exchange( mbedtls_ssl_context *ssl ) } else #endif - if( ( ret = mbedtls_ssl_read_record( ssl ) ) != 0 ) + if( ( ret = mbedtls_ssl_read_record( ssl, 1 ) ) != 0 ) { MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_ssl_read_record", ret ); return( ret ); @@ -4038,25 +4038,10 @@ static int ssl_parse_certificate_verify( mbedtls_ssl_context *ssl ) } /* Read the message without adding it to the checksum */ - do { - - do ret = mbedtls_ssl_read_record_layer( ssl ); - while( ret == MBEDTLS_ERR_SSL_CONTINUE_PROCESSING ); - - if( ret != 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 || - MBEDTLS_ERR_SSL_CONTINUE_PROCESSING == ret ); - + ret = mbedtls_ssl_read_record( ssl, 0 /* no checksum update */ ); if( 0 != ret ) { - MBEDTLS_SSL_DEBUG_RET( 1, ( "mbedtls_ssl_handle_message_type" ), ret ); + MBEDTLS_SSL_DEBUG_RET( 1, ( "mbedtls_ssl_read_record" ), ret ); return( ret ); } diff --git a/library/ssl_tls.c b/library/ssl_tls.c index cc470583a..23b066c5c 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -4283,7 +4283,8 @@ static void ssl_handshake_wrapup_free_hs_transform( mbedtls_ssl_context *ssl ); * RFC 6347 4.1.2.7) and continue reading until a valid record is found. * */ -int mbedtls_ssl_read_record( mbedtls_ssl_context *ssl ) +int mbedtls_ssl_read_record( mbedtls_ssl_context *ssl, + unsigned update_digest ) { int ret; @@ -4313,7 +4314,8 @@ int mbedtls_ssl_read_record( mbedtls_ssl_context *ssl ) return( ret ); } - if( ssl->in_msgtype == MBEDTLS_SSL_MSG_HANDSHAKE ) + if( ssl->in_msgtype == MBEDTLS_SSL_MSG_HANDSHAKE && + update_digest == 1 ) { mbedtls_ssl_update_handshake_status( ssl ); } @@ -4900,7 +4902,7 @@ int mbedtls_ssl_parse_certificate( mbedtls_ssl_context *ssl ) } #endif - if( ( ret = mbedtls_ssl_read_record( ssl ) ) != 0 ) + if( ( ret = mbedtls_ssl_read_record( ssl, 1 ) ) != 0 ) { /* mbedtls_ssl_read_record may have sent an alert already. We let it decide whether to alert. */ @@ -5275,7 +5277,7 @@ int mbedtls_ssl_parse_change_cipher_spec( mbedtls_ssl_context *ssl ) MBEDTLS_SSL_DEBUG_MSG( 2, ( "=> parse change cipher spec" ) ); - if( ( ret = mbedtls_ssl_read_record( ssl ) ) != 0 ) + if( ( ret = mbedtls_ssl_read_record( ssl, 1 ) ) != 0 ) { MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_ssl_read_record", ret ); return( ret ); @@ -5904,7 +5906,7 @@ int mbedtls_ssl_parse_finished( mbedtls_ssl_context *ssl ) ssl->handshake->calc_finished( ssl, buf, ssl->conf->endpoint ^ 1 ); - if( ( ret = mbedtls_ssl_read_record( ssl ) ) != 0 ) + if( ( ret = mbedtls_ssl_read_record( ssl, 1 ) ) != 0 ) { MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_ssl_read_record", ret ); return( ret ); @@ -7653,7 +7655,7 @@ int mbedtls_ssl_read( mbedtls_ssl_context *ssl, unsigned char *buf, size_t len ) ssl_set_timer( ssl, ssl->conf->read_timeout ); } - if( ( ret = mbedtls_ssl_read_record( ssl ) ) != 0 ) + if( ( ret = mbedtls_ssl_read_record( ssl, 1 ) ) != 0 ) { if( ret == MBEDTLS_ERR_SSL_CONN_EOF ) return( 0 ); @@ -7668,7 +7670,7 @@ int mbedtls_ssl_read( mbedtls_ssl_context *ssl, unsigned char *buf, size_t len ) /* * OpenSSL sends empty messages to randomize the IV */ - if( ( ret = mbedtls_ssl_read_record( ssl ) ) != 0 ) + if( ( ret = mbedtls_ssl_read_record( ssl, 1 ) ) != 0 ) { if( ret == MBEDTLS_ERR_SSL_CONN_EOF ) return( 0 );