From 327c93b1824c0e086ed45b325659ad0fb8f3c428 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 15 Aug 2018 13:56:18 +0100 Subject: [PATCH 01/58] 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 765da7a71b..c817def23c 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 73e4391a0e..d160c42d02 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 7101f461f3..84c83e3303 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 cc470583a1..23b066c5cb 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 ); From 02f5907499a29998ef112324e1c6715446b6b1e7 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 15 Aug 2018 14:00:24 +0100 Subject: [PATCH 02/58] Correct misleading debugging output Usually, debug messages beginning with "=> and "<=" match up and indicate entering of and returning from functions, respectively. This commit fixes one exception to this rule in mbedtls_ssl_read_record(), which sometimes printed two messages of the form "<= XXX". --- library/ssl_tls.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 23b066c5cb..910e584985 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -4322,7 +4322,7 @@ int mbedtls_ssl_read_record( mbedtls_ssl_context *ssl, } else { - MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= reuse previously read message" ) ); + MBEDTLS_SSL_DEBUG_MSG( 2, ( "reuse previously read message" ) ); ssl->keep_current_message = 0; } From a4b143a57ccc16243dce5f206e197ce44559955a Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 15 Aug 2018 14:01:34 +0100 Subject: [PATCH 03/58] Remove nested loop in mbedtls_ssl_read_record() --- library/ssl_tls.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 910e584985..8e209e78ac 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -4294,8 +4294,9 @@ int mbedtls_ssl_read_record( mbedtls_ssl_context *ssl, { do { - do ret = mbedtls_ssl_read_record_layer( ssl ); - while( ret == MBEDTLS_ERR_SSL_CONTINUE_PROCESSING ); + ret = mbedtls_ssl_read_record_layer( ssl ); + if( ret == MBEDTLS_ERR_SSL_CONTINUE_PROCESSING ) + continue; if( ret != 0 ) { From 4162b11eb4cb46822c79269cb241d10d86156f23 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 15 Aug 2018 14:05:04 +0100 Subject: [PATCH 04/58] Make mbedtls_ssl_read_record_layer() static This function was previously global because it was used directly within ssl_parse_certificate_verify() in library/ssl_srv.c. The previous commit removed this dependency, replacing the call by a call to the global parent function mbedtls_ssl_read_record(). This renders mbedtls_ssl_read_record_layer() internal and therefore allows to make it static, and accordingly rename it as ssl_read_record_layer(). --- include/mbedtls/ssl_internal.h | 1 - library/ssl_tls.c | 6 ++++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/include/mbedtls/ssl_internal.h b/include/mbedtls/ssl_internal.h index c817def23c..0522778918 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -479,7 +479,6 @@ int mbedtls_ssl_send_fatal_handshake_failure( mbedtls_ssl_context *ssl ); void mbedtls_ssl_reset_checksum( mbedtls_ssl_context *ssl ); int mbedtls_ssl_derive_keys( mbedtls_ssl_context *ssl ); -int mbedtls_ssl_read_record_layer( mbedtls_ssl_context *ssl ); int mbedtls_ssl_handle_message_type( mbedtls_ssl_context *ssl ); int mbedtls_ssl_prepare_handshake_record( mbedtls_ssl_context *ssl ); void mbedtls_ssl_update_handshake_status( mbedtls_ssl_context *ssl ); diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 8e209e78ac..b8f271527c 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -4283,6 +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. * */ +static int ssl_read_record_layer( mbedtls_ssl_context *ssl ); + int mbedtls_ssl_read_record( mbedtls_ssl_context *ssl, unsigned update_digest ) { @@ -4294,7 +4296,7 @@ int mbedtls_ssl_read_record( mbedtls_ssl_context *ssl, { do { - ret = mbedtls_ssl_read_record_layer( ssl ); + ret = ssl_read_record_layer( ssl ); if( ret == MBEDTLS_ERR_SSL_CONTINUE_PROCESSING ) continue; @@ -4332,7 +4334,7 @@ int mbedtls_ssl_read_record( mbedtls_ssl_context *ssl, return( 0 ); } -int mbedtls_ssl_read_record_layer( mbedtls_ssl_context *ssl ) +static int ssl_read_record_layer( mbedtls_ssl_context *ssl ) { int ret; From 1097b34022a416ee180c13dd7a84d3bcbbd85542 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 15 Aug 2018 14:09:41 +0100 Subject: [PATCH 05/58] Extract message-consuming code-path to separate function The first part of the function ssl_read_record_layer() was to mark the previous message as consumed. This commit moves the corresponding code-path to a separate static function ssl_consume_current_message(). --- library/ssl_tls.c | 28 +++++++++++++++++++++++----- 1 file changed, 23 insertions(+), 5 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index b8f271527c..23a5bddac2 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -4283,6 +4283,9 @@ 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. * */ + +/* Helper functions for mbedtls_ssl_read_record(). */ +static int ssl_consume_current_message( mbedtls_ssl_context *ssl ); static int ssl_read_record_layer( mbedtls_ssl_context *ssl ); int mbedtls_ssl_read_record( mbedtls_ssl_context *ssl, @@ -4334,13 +4337,9 @@ int mbedtls_ssl_read_record( mbedtls_ssl_context *ssl, return( 0 ); } -static int ssl_read_record_layer( mbedtls_ssl_context *ssl ) +static int ssl_consume_current_message( mbedtls_ssl_context *ssl ) { - int ret; - /* - * Step A - * * Consume last content-layer message and potentially * update in_msglen which keeps track of the contents' * consumption state. @@ -4422,6 +4421,25 @@ static int ssl_read_record_layer( mbedtls_ssl_context *ssl ) ssl->in_msglen = 0; } + return( 0 ); +} + +static int ssl_read_record_layer( mbedtls_ssl_context *ssl ) +{ + int ret; + + /* + * Step A + * + * Consume last content-layer message and potentially + * update in_msglen which keeps track of the contents' + * consumption state. + */ + + ret = ssl_consume_current_message( ssl ); + if( ret != 0 ) + return( ret ); + /* * Step B * From 2699459529927fa33061d32c94b78ef5260f501f Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 15 Aug 2018 14:14:59 +0100 Subject: [PATCH 06/58] Move call to ssl_consume_current_message() Subsequent commits will potentially inject buffered messages after the last incoming message has been consumed, but before a new one is fetched. As a preparatory step to this, this commit moves the call to ssl_consume_current_message() from ssl_read_record_layer() to the calling function mbedtls_ssl_read_record(). --- library/ssl_tls.c | 19 ++++--------------- 1 file changed, 4 insertions(+), 15 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 23a5bddac2..54bb443594 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -4299,6 +4299,10 @@ int mbedtls_ssl_read_record( mbedtls_ssl_context *ssl, { do { + ret = ssl_consume_current_message( ssl ); + if( ret != 0 ) + return( ret ); + ret = ssl_read_record_layer( ssl ); if( ret == MBEDTLS_ERR_SSL_CONTINUE_PROCESSING ) continue; @@ -4429,22 +4433,7 @@ static int ssl_read_record_layer( mbedtls_ssl_context *ssl ) int ret; /* - * Step A - * - * Consume last content-layer message and potentially - * update in_msglen which keeps track of the contents' - * consumption state. - */ - - ret = ssl_consume_current_message( ssl ); - if( ret != 0 ) - return( ret ); - - /* - * Step B - * * Fetch and decode new record if current one is fully consumed. - * */ if( ssl->in_msglen > 0 ) From e74d556b43232409d3b98a13e6e224ef15d8a202 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 15 Aug 2018 14:26:08 +0100 Subject: [PATCH 07/58] Introduce function to indicate if record is fully processed This commit introduces a function ssl_record_is_in_progress() to indicate if there is there is more data within the current record to be processed. Further, it moves the corresponding call from ssl_read_record_layer() to the parent function mbedtls_ssl_read_record(). With this change, ssl_read_record_layer() has the sole purpose of fetching and decoding a new record, and hence this commit also renames it to ssl_get_next_record(). --- library/ssl_tls.c | 40 ++++++++++++++++++++++------------------ 1 file changed, 22 insertions(+), 18 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 54bb443594..cfb95eae29 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -4286,7 +4286,8 @@ static void ssl_handshake_wrapup_free_hs_transform( mbedtls_ssl_context *ssl ); /* Helper functions for mbedtls_ssl_read_record(). */ static int ssl_consume_current_message( mbedtls_ssl_context *ssl ); -static int ssl_read_record_layer( mbedtls_ssl_context *ssl ); +static int ssl_get_next_record( mbedtls_ssl_context *ssl ); +static int ssl_record_is_in_progress( mbedtls_ssl_context *ssl ); int mbedtls_ssl_read_record( mbedtls_ssl_context *ssl, unsigned update_digest ) @@ -4303,14 +4304,17 @@ int mbedtls_ssl_read_record( mbedtls_ssl_context *ssl, if( ret != 0 ) return( ret ); - ret = ssl_read_record_layer( ssl ); - if( ret == MBEDTLS_ERR_SSL_CONTINUE_PROCESSING ) - continue; - - if( ret != 0 ) + if( ssl_record_is_in_progress( ssl ) == 0 ) { - MBEDTLS_SSL_DEBUG_RET( 1, ( "mbedtls_ssl_read_record_layer" ), ret ); - return( ret ); + ret = ssl_get_next_record( ssl ); + if( ret == MBEDTLS_ERR_SSL_CONTINUE_PROCESSING ) + continue; + + if( ret != 0 ) + { + MBEDTLS_SSL_DEBUG_RET( 1, ( "mbedtls_ssl_read_record_layer" ), ret ); + return( ret ); + } } ret = mbedtls_ssl_handle_message_type( ssl ); @@ -4428,22 +4432,22 @@ static int ssl_consume_current_message( mbedtls_ssl_context *ssl ) return( 0 ); } -static int ssl_read_record_layer( mbedtls_ssl_context *ssl ) +static int ssl_record_is_in_progress( mbedtls_ssl_context *ssl ) +{ + if( ssl->in_msglen > 0 ) + return( 1 ); + + return( 0 ); +} + +static int ssl_get_next_record( mbedtls_ssl_context *ssl ) { int ret; /* - * Fetch and decode new record if current one is fully consumed. + * Fetch and decode new record */ - if( ssl->in_msglen > 0 ) - { - /* There's something left to be processed in the current record. */ - return( 0 ); - } - - /* Current record either fully processed or to be discarded. */ - if( ( ret = mbedtls_ssl_fetch_input( ssl, mbedtls_ssl_hdr_len( ssl ) ) ) != 0 ) { MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_ssl_fetch_input", ret ); From 40f50848fad3e1371ad5b0a933013f9542d0d749 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 15 Aug 2018 14:48:01 +0100 Subject: [PATCH 08/58] Add frame for loading and storing buffered messages This commit introduces the frame for saving and loading buffered messages within message reading function mbedtls_ssl_read_record(). --- include/mbedtls/ssl.h | 1 + library/ssl_tls.c | 70 +++++++++++++++++++++++++++++++++++++++---- 2 files changed, 65 insertions(+), 6 deletions(-) diff --git a/include/mbedtls/ssl.h b/include/mbedtls/ssl.h index 85ab722062..3a8dd21e99 100644 --- a/include/mbedtls/ssl.h +++ b/include/mbedtls/ssl.h @@ -121,6 +121,7 @@ #define MBEDTLS_ERR_SSL_INVALID_VERIFY_HASH -0x6600 /**< Couldn't set the hash for verifying CertificateVerify */ #define MBEDTLS_ERR_SSL_CONTINUE_PROCESSING -0x6580 /**< Internal-only message signaling that further message-processing should be done */ #define MBEDTLS_ERR_SSL_ASYNC_IN_PROGRESS -0x6500 /**< The asynchronous operation is not completed yet. */ +#define MBEDTLS_ERR_SSL_EARLY_MESSAGE -0x6480 /**< Internal-only message signaling that a message arrived early. */ /* * Various constants diff --git a/library/ssl_tls.c b/library/ssl_tls.c index cfb95eae29..41292a53bc 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -4289,6 +4289,12 @@ static int ssl_consume_current_message( mbedtls_ssl_context *ssl ); static int ssl_get_next_record( mbedtls_ssl_context *ssl ); static int ssl_record_is_in_progress( mbedtls_ssl_context *ssl ); +#if defined(MBEDTLS_SSL_PROTO_DTLS) +static int ssl_load_buffered_message( mbedtls_ssl_context *ssl ); +static int ssl_buffer_message( mbedtls_ssl_context *ssl ); +static int ssl_another_record_in_datagram( mbedtls_ssl_context *ssl ); +#endif /* MBEDTLS_SSL_PROTO_DTLS */ + int mbedtls_ssl_read_record( mbedtls_ssl_context *ssl, unsigned update_digest ) { @@ -4306,19 +4312,47 @@ int mbedtls_ssl_read_record( mbedtls_ssl_context *ssl, if( ssl_record_is_in_progress( ssl ) == 0 ) { - ret = ssl_get_next_record( ssl ); - if( ret == MBEDTLS_ERR_SSL_CONTINUE_PROCESSING ) - continue; +#if defined(MBEDTLS_SSL_PROTO_DTLS) + int have_buffered = 0; - if( ret != 0 ) + /* We only check for buffered messages if the + * current datagram is fully consumed. */ + if( ssl->conf->transport == MBEDTLS_SSL_TRANSPORT_DATAGRAM && + ssl_another_record_in_datagram( ssl ) == 0 ) { - MBEDTLS_SSL_DEBUG_RET( 1, ( "mbedtls_ssl_read_record_layer" ), ret ); - return( ret ); + if( ssl_load_buffered_message( ssl ) == 0 ) + have_buffered = 1; + } + + if( have_buffered == 0 ) +#endif /* MBEDTLS_SSL_PROTO_DTLS */ + { + ret = ssl_get_next_record( ssl ); + if( ret == MBEDTLS_ERR_SSL_CONTINUE_PROCESSING ) + continue; + + if( ret != 0 ) + { + MBEDTLS_SSL_DEBUG_RET( 1, ( "mbedtls_ssl_read_record_layer" ), ret ); + return( ret ); + } } } ret = mbedtls_ssl_handle_message_type( ssl ); +#if defined(MBEDTLS_SSL_PROTO_DTLS) + if( ret == MBEDTLS_ERR_SSL_EARLY_MESSAGE ) + { + /* Buffer future message */ + ret = ssl_buffer_message( ssl ); + if( ret != 0 ) + return( ret ); + + ret = MBEDTLS_ERR_SSL_CONTINUE_PROCESSING; + } +#endif /* MBEDTLS_SSL_PROTO_DTLS */ + } while( MBEDTLS_ERR_SSL_NON_FATAL == ret || MBEDTLS_ERR_SSL_CONTINUE_PROCESSING == ret ); @@ -4345,6 +4379,30 @@ int mbedtls_ssl_read_record( mbedtls_ssl_context *ssl, return( 0 ); } +#if defined(MBEDTLS_SSL_PROTO_DTLS) +static int ssl_another_record_in_datagram( mbedtls_ssl_context *ssl ) +{ + if( ssl->in_left > ssl->next_record_offset ) + return( 1 ); + + return( 0 ); +} + +static int ssl_load_buffered_message( mbedtls_ssl_context *ssl ) +{ + /* No buffering support so far. */ + ((void) ssl ); + return( -1 ); +} + +static int ssl_buffer_message( mbedtls_ssl_context *ssl ) +{ + /* No buffering support so far. */ + ((void) ssl ); + return( 0 ); +} +#endif /* MBEDTLS_SSL_PROTO_DTLS */ + static int ssl_consume_current_message( mbedtls_ssl_context *ssl ) { /* From 2ed6bcc79335314fc2ddf3da0722940bdba962ce Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 15 Aug 2018 15:11:57 +0100 Subject: [PATCH 09/58] Implement support for remembering CCS messages This commit implements support for remembering out-of-order CCS messages. Specifically, a flag is set whenever a CCS message is read which remains until the end of a flight, and when a CCS message is expected and a CCS message has been seen in the current flight, a synthesized CCS record is created. --- include/mbedtls/ssl_internal.h | 3 + library/ssl_tls.c | 101 ++++++++++++++++++++++++++++----- 2 files changed, 89 insertions(+), 15 deletions(-) diff --git a/include/mbedtls/ssl_internal.h b/include/mbedtls/ssl_internal.h index 0522778918..ec840476f7 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -307,6 +307,9 @@ struct mbedtls_ssl_handshake_params resending messages */ unsigned char alt_out_ctr[8]; /*!< Alternative record epoch/counter for resending messages */ + + uint8_t seen_ccs; /*!< Indicates if a CCS message has + * been seen in the current flight. */ #endif /* MBEDTLS_SSL_PROTO_DTLS */ /* diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 41292a53bc..6a44145d7e 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -3069,6 +3069,9 @@ void mbedtls_ssl_recv_flight_completed( mbedtls_ssl_context *ssl ) /* The next incoming flight will start with this msg_seq */ ssl->handshake->in_flight_start_seq = ssl->handshake->in_msg_seq; + /* We don't want to remember CCS's across flight boundaries. */ + ssl->handshake->seen_ccs = 0; + /* Cancel timer */ ssl_set_timer( ssl, 0 ); @@ -4138,15 +4141,6 @@ static int ssl_parse_record_header( mbedtls_ssl_context *ssl ) } #endif - /* Drop unexpected ChangeCipherSpec messages */ - if( ssl->in_msgtype == MBEDTLS_SSL_MSG_CHANGE_CIPHER_SPEC && - ssl->state != MBEDTLS_SSL_CLIENT_CHANGE_CIPHER_SPEC && - ssl->state != MBEDTLS_SSL_SERVER_CHANGE_CIPHER_SPEC ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "dropping unexpected ChangeCipherSpec" ) ); - return( MBEDTLS_ERR_SSL_UNEXPECTED_RECORD ); - } - /* Drop unexpected ApplicationData records, * except at the beginning of renegotiations */ if( ssl->in_msgtype == MBEDTLS_SSL_MSG_APPLICATION_DATA && @@ -4390,16 +4384,75 @@ static int ssl_another_record_in_datagram( mbedtls_ssl_context *ssl ) static int ssl_load_buffered_message( mbedtls_ssl_context *ssl ) { - /* No buffering support so far. */ - ((void) ssl ); - return( -1 ); + mbedtls_ssl_handshake_params * const hs = ssl->handshake; + int ret = 0; + + MBEDTLS_SSL_DEBUG_MSG( 2, ( "=> ssl_load_buffered_messsage" ) ); + + if( hs == NULL ) + return( -1 ); + + if( ssl->state == MBEDTLS_SSL_CLIENT_CHANGE_CIPHER_SPEC || + ssl->state == MBEDTLS_SSL_SERVER_CHANGE_CIPHER_SPEC ) + { + /* Check if we have seen a ChangeCipherSpec before. + * If yes, synthesize a CCS record. */ + if( ! hs->seen_ccs ) + { + MBEDTLS_SSL_DEBUG_MSG( 2, ( "CCS not seen in the current flight" ) ); + ret = -1; + goto exit; + } + + MBEDTLS_SSL_DEBUG_MSG( 2, ( "Inject buffered CCS message" ) ); + ssl->in_msgtype = MBEDTLS_SSL_MSG_CHANGE_CIPHER_SPEC; + ssl->in_msglen = 1; + ssl->in_msg[0] = 1; + + /* As long as they are equal, the exact value doesn't matter. */ + ssl->in_left = 0; + ssl->next_record_offset = 0; + + hs->seen_ccs = 0; + goto exit; + } + ret = -1; + +exit: + + MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= ssl_load_buffered_message" ) ); + return( ret ); } static int ssl_buffer_message( mbedtls_ssl_context *ssl ) { - /* No buffering support so far. */ - ((void) ssl ); - return( 0 ); + int ret = 0; + mbedtls_ssl_handshake_params * const hs = ssl->handshake; + + if( hs == NULL ) + return( 0 ); + + MBEDTLS_SSL_DEBUG_MSG( 2, ( "=> ssl_buffer_message" ) ); + + switch( ssl->in_msgtype ) + { + case MBEDTLS_SSL_MSG_CHANGE_CIPHER_SPEC: + MBEDTLS_SSL_DEBUG_MSG( 2, ( "Remember CCS message" ) ); + hs->seen_ccs = 1; + break; + + case MBEDTLS_SSL_MSG_HANDSHAKE: + /* No support for buffering handshake messages so far. */ + break; + + default: + break; + } + +exit: + + MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= ssl_buffer_message" ) ); + return( ret ); } #endif /* MBEDTLS_SSL_PROTO_DTLS */ @@ -4649,6 +4702,24 @@ int mbedtls_ssl_handle_message_type( mbedtls_ssl_context *ssl ) } } +#if defined(MBEDTLS_SSL_PROTO_DTLS) + /* Drop unexpected ChangeCipherSpec messages */ + if( ssl->conf->transport == MBEDTLS_SSL_TRANSPORT_DATAGRAM && + ssl->in_msgtype == MBEDTLS_SSL_MSG_CHANGE_CIPHER_SPEC && + ssl->state != MBEDTLS_SSL_CLIENT_CHANGE_CIPHER_SPEC && + ssl->state != MBEDTLS_SSL_SERVER_CHANGE_CIPHER_SPEC ) + { + if( ssl->handshake == NULL ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "dropping ChangeCipherSpec outside handshake" ) ); + return( MBEDTLS_ERR_SSL_UNEXPECTED_RECORD ); + } + + MBEDTLS_SSL_DEBUG_MSG( 1, ( "received out-of-order ChangeCipherSpec - remember" ) ); + return( MBEDTLS_ERR_SSL_EARLY_MESSAGE ); + } +#endif + if( ssl->in_msgtype == MBEDTLS_SSL_MSG_ALERT ) { if( ssl->in_msglen != 2 ) From aa5d0c44937727a36a82d4cca0776dad91b6db35 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 16 Aug 2018 13:15:19 +0100 Subject: [PATCH 10/58] Add test for buffering out-of-order CCS --- tests/ssl-opt.sh | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index 4fa8609f94..c056000242 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -5741,6 +5741,16 @@ run_test "DTLS proxy: delay ChangeCipherSpec" \ -s "Extra-header:" \ -c "HTTP/1.0 200 OK" +# Tests for reordering support with DTLS + +run_test "DTLS reordering: Buffer out-of-order CCS message"\ + -p "$P_PXY delay=3 seed=1" \ + "$P_SRV cookies=0 dtls=1 debug_level=2" \ + "$P_CLI dtls=1 debug_level=2" \ + 0 \ + -c "Inject buffered CCS message" \ + -c "Remember CCS message" + # Tests for "randomly unreliable connection": try a variety of flows and peers client_needs_more_time 2 From 9e1ec22c36bb1f96bbcaf834a97840fcced0ca1b Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 15 Aug 2018 15:54:43 +0100 Subject: [PATCH 11/58] Return MBEDTLS_ERR_SSL_EARLY_MESSAGE for future HS messages This leads future HS messages to traverse the buffering function ssl_buffer_message(), which however doesn't do anything at the moment for HS messages. Since the error code MBEDTLS_ERR_SSL_EARLY_MESSAGE is afterwards remapped to MBEDTLS_ERR_SSL_CONTINUE_PROCESSING -- which is what was returned prior to this commit when receiving a future handshake message -- this commit therefore does not yet introduce any change in observable behavior. --- library/ssl_tls.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 6a44145d7e..bca5b403cb 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -3656,6 +3656,14 @@ int mbedtls_ssl_prepare_handshake_record( mbedtls_ssl_context *ssl ) ( ssl->state == MBEDTLS_SSL_HANDSHAKE_OVER && ssl->in_msg[0] != MBEDTLS_SSL_HS_CLIENT_HELLO ) ) ) { + if( recv_msg_seq > ssl->handshake->in_msg_seq ) + { + MBEDTLS_SSL_DEBUG_MSG( 2, ( "received future handshake message of sequence number %u (next %u)", + recv_msg_seq, + ssl->handshake->in_msg_seq ) ); + return( MBEDTLS_ERR_SSL_EARLY_MESSAGE ); + } + /* Retransmit only on last message from previous flight, to avoid * too many retransmissions. * Besides, No sane server ever retransmits HelloVerifyRequest */ From 56e205e2c9db8359bd7755c60d9c88c34d57d572 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 16 Aug 2018 09:06:12 +0100 Subject: [PATCH 12/58] Prepare handshake reassembly in separate function This commit moves the code-path preparing the handshake reassembly buffer, consisting of header, message content, and reassembly bitmap, to a separate function ssl_prepare_reassembly_buffer(). --- library/ssl_tls.c | 56 ++++++++++++++++++++++++++++++----------------- 1 file changed, 36 insertions(+), 20 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index bca5b403cb..e0ce692a8b 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -3470,6 +3470,39 @@ static int ssl_bitmask_check( unsigned char *mask, size_t len ) return( 0 ); } +/* msg_len does not include the handshake header */ +static int ssl_prepare_reassembly_buffer( mbedtls_ssl_context *ssl, /* debug */ + unsigned msg_len, + unsigned char **target ) +{ + size_t alloc_len; + unsigned char *buf; + + MBEDTLS_SSL_DEBUG_MSG( 2, ( "initialize reassembly, total length = %d", + msg_len ) ); + + /* NOTE: That should be checked earlier */ + if( msg_len > MBEDTLS_SSL_IN_CONTENT_LEN ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "handshake message too large" ) ); + return( MBEDTLS_ERR_SSL_FEATURE_UNAVAILABLE ); + } + + alloc_len = 12; /* Handshake header */ + alloc_len += msg_len; /* Content buffer */ + alloc_len += msg_len / 8 + ( msg_len % 8 != 0 ); /* Bitmap */ + + buf = mbedtls_calloc( 1, alloc_len ); + if( buf == NULL ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "alloc failed (%d bytes)", alloc_len ) ); + return( MBEDTLS_ERR_SSL_ALLOC_FAILED ); + } + + *target = buf; + return( 0 ); +} + /* * Reassemble fragmented DTLS handshake messages. * @@ -3495,26 +3528,9 @@ static int ssl_reassemble_dtls_handshake( mbedtls_ssl_context *ssl ) */ if( ssl->handshake->hs_msg == NULL ) { - size_t alloc_len; - - MBEDTLS_SSL_DEBUG_MSG( 2, ( "initialize reassembly, total length = %d", - msg_len ) ); - - if( ssl->in_hslen > MBEDTLS_SSL_IN_CONTENT_LEN ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "handshake message too large" ) ); - return( MBEDTLS_ERR_SSL_FEATURE_UNAVAILABLE ); - } - - /* The bitmask needs one bit per byte of message excluding header */ - alloc_len = 12 + msg_len + msg_len / 8 + ( msg_len % 8 != 0 ); - - ssl->handshake->hs_msg = mbedtls_calloc( 1, alloc_len ); - if( ssl->handshake->hs_msg == NULL ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "alloc failed (%d bytes)", alloc_len ) ); - return( MBEDTLS_ERR_SSL_ALLOC_FAILED ); - } + ret = ssl_prepare_reassembly_buffer( msg_len, &ssl->handshake->hs_msg ); + if( ret != 0 ) + return( ret ); /* Prepare final header: copy msg_type, length and message_seq, * then add standardised fragment_offset and fragment_length */ From d07df86871498129db908377288ae3da8a396aa8 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 16 Aug 2018 09:14:58 +0100 Subject: [PATCH 13/58] Make allocation of reassembly bitmap optional This commit adds a parameter to ssl_prepare_reassembly_buffer() allowing to disable the allocation of space for a reassembly bitmap. This will allow this function to be used for the allocation of buffers for future handshake messages in case these need no fragmentation. --- library/ssl_tls.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index e0ce692a8b..a9f84d497a 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -3473,6 +3473,7 @@ static int ssl_bitmask_check( unsigned char *mask, size_t len ) /* msg_len does not include the handshake header */ static int ssl_prepare_reassembly_buffer( mbedtls_ssl_context *ssl, /* debug */ unsigned msg_len, + unsigned add_bitmap, unsigned char **target ) { size_t alloc_len; @@ -3490,7 +3491,9 @@ static int ssl_prepare_reassembly_buffer( mbedtls_ssl_context *ssl, /* debug */ alloc_len = 12; /* Handshake header */ alloc_len += msg_len; /* Content buffer */ - alloc_len += msg_len / 8 + ( msg_len % 8 != 0 ); /* Bitmap */ + + if( add_bitmap ) + alloc_len += msg_len / 8 + ( msg_len % 8 != 0 ); /* Bitmap */ buf = mbedtls_calloc( 1, alloc_len ); if( buf == NULL ) @@ -3528,7 +3531,8 @@ static int ssl_reassemble_dtls_handshake( mbedtls_ssl_context *ssl ) */ if( ssl->handshake->hs_msg == NULL ) { - ret = ssl_prepare_reassembly_buffer( msg_len, &ssl->handshake->hs_msg ); + ret = ssl_prepare_reassembly_buffer( msg_len, 1, + &ssl->handshake->hs_msg ); if( ret != 0 ) return( ret ); From e25e3b7d960a11c2509698be61a0e4319aabf068 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 16 Aug 2018 09:30:53 +0100 Subject: [PATCH 14/58] Add function to check is HS msg is a proper fragment This commit introduces a static function ssl_hs_is_proper_fragment() to check if the current incoming handshake message is a proper fragment. It is used within mbedtls_ssl_prepare_handshake_record() to decide whether handshake reassembly through ssl_reassemble_dtls_handshake() is needed. The commit changes the behavior of the library in the (unnatural) situation where proper fragments for a handshake message are followed by a non-fragmented version of the same message. In this case, the previous code invoked the handshake reassembly routine ssl_reassemble_dtls_handshake(), while with this commit, the full handshake message is directly forwarded to the user, no altering the handshake reassembly state -- in particular, not freeing it. As a remedy, freeing of a potential handshake reassembly structure is now done as part of the handshake update function mbedtls_ssl_update_handshake_status(). --- library/ssl_tls.c | 29 ++++++++++++++++++++--------- 1 file changed, 20 insertions(+), 9 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index a9f84d497a..c2daeb36e3 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -3409,6 +3409,17 @@ int mbedtls_ssl_write_record( mbedtls_ssl_context *ssl, uint8_t force_flush ) } #if defined(MBEDTLS_SSL_PROTO_DTLS) + +static int ssl_hs_is_proper_fragment( mbedtls_ssl_context *ssl ) +{ + if( ssl->in_msglen < ssl->in_hslen || + memcmp( ssl->in_msg + 6, "\0\0\0", 3 ) != 0 || + memcmp( ssl->in_msg + 9, ssl->in_msg + 1, 3 ) != 0 ) + { + return( 1 ); + } + return( 0 ); +} /* * Mark bits in bitmask (used for DTLS HS reassembly) */ @@ -3636,9 +3647,6 @@ static int ssl_reassemble_dtls_handshake( mbedtls_ssl_context *ssl ) memcpy( ssl->in_msg, ssl->handshake->hs_msg, ssl->in_hslen ); - mbedtls_free( ssl->handshake->hs_msg ); - ssl->handshake->hs_msg = NULL; - MBEDTLS_SSL_DEBUG_BUF( 3, "reassembled handshake message", ssl->in_msg, ssl->in_hslen ); @@ -3646,6 +3654,7 @@ static int ssl_reassemble_dtls_handshake( mbedtls_ssl_context *ssl ) } #endif /* MBEDTLS_SSL_PROTO_DTLS */ + int mbedtls_ssl_prepare_handshake_record( mbedtls_ssl_context *ssl ) { if( ssl->in_msglen < mbedtls_ssl_hs_hdr_len( ssl ) ) @@ -3713,12 +3722,7 @@ int mbedtls_ssl_prepare_handshake_record( mbedtls_ssl_context *ssl ) } /* Wait until message completion to increment in_msg_seq */ - /* Reassemble if current message is fragmented or reassembly is - * already in progress */ - if( ssl->in_msglen < ssl->in_hslen || - memcmp( ssl->in_msg + 6, "\0\0\0", 3 ) != 0 || - memcmp( ssl->in_msg + 9, ssl->in_msg + 1, 3 ) != 0 || - ( ssl->handshake != NULL && ssl->handshake->hs_msg != NULL ) ) + if( ssl_hs_is_proper_fragment( ssl ) == 1 ) { MBEDTLS_SSL_DEBUG_MSG( 2, ( "found fragmented DTLS handshake message" ) ); @@ -3756,6 +3760,13 @@ void mbedtls_ssl_update_handshake_status( mbedtls_ssl_context *ssl ) ssl->handshake != NULL ) { ssl->handshake->in_msg_seq++; + + /* Clear up handshake reassembly structure, if any. */ + if( ssl->handshake->hs_msg != NULL ) + { + mbedtls_free( ssl->handshake->hs_msg ); + ssl->handshake->hs_msg = NULL; + } } #endif } From d7f8ae2508ddb901e5204efc2d8a7f8492db6e22 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 16 Aug 2018 09:45:56 +0100 Subject: [PATCH 15/58] Introduce sub-structure of ssl_handshake_params for buffering This commit introduces a sub-structure `buffering` within mbedtls_ssl_handshake_params that shall contain all data related to the reassembly and/or buffering of handshake messages. Currently, only buffering of CCS messages is implemented, so the only member of this struct is the previously introduced `seen_ccs` field. --- include/mbedtls/ssl_internal.h | 6 +++++- library/ssl_tls.c | 10 +++++----- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/include/mbedtls/ssl_internal.h b/include/mbedtls/ssl_internal.h index ec840476f7..b9084b437f 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -308,8 +308,12 @@ struct mbedtls_ssl_handshake_params unsigned char alt_out_ctr[8]; /*!< Alternative record epoch/counter for resending messages */ - uint8_t seen_ccs; /*!< Indicates if a CCS message has + struct + { + uint8_t seen_ccs; /*!< Indicates if a CCS message has * been seen in the current flight. */ + + } buffering; #endif /* MBEDTLS_SSL_PROTO_DTLS */ /* diff --git a/library/ssl_tls.c b/library/ssl_tls.c index c2daeb36e3..5e573422ec 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -3070,7 +3070,7 @@ void mbedtls_ssl_recv_flight_completed( mbedtls_ssl_context *ssl ) ssl->handshake->in_flight_start_seq = ssl->handshake->in_msg_seq; /* We don't want to remember CCS's across flight boundaries. */ - ssl->handshake->seen_ccs = 0; + ssl->handshake->buffering.seen_ccs = 0; /* Cancel timer */ ssl_set_timer( ssl, 0 ); @@ -4436,11 +4436,11 @@ static int ssl_load_buffered_message( mbedtls_ssl_context *ssl ) { /* Check if we have seen a ChangeCipherSpec before. * If yes, synthesize a CCS record. */ - if( ! hs->seen_ccs ) + if( ! hs->buffering.seen_ccs ) { MBEDTLS_SSL_DEBUG_MSG( 2, ( "CCS not seen in the current flight" ) ); ret = -1; - goto exit; + return( -1 ); } MBEDTLS_SSL_DEBUG_MSG( 2, ( "Inject buffered CCS message" ) ); @@ -4452,7 +4452,7 @@ static int ssl_load_buffered_message( mbedtls_ssl_context *ssl ) ssl->in_left = 0; ssl->next_record_offset = 0; - hs->seen_ccs = 0; + hs->buffering.seen_ccs = 0; goto exit; } ret = -1; @@ -4477,7 +4477,7 @@ static int ssl_buffer_message( mbedtls_ssl_context *ssl ) { case MBEDTLS_SSL_MSG_CHANGE_CIPHER_SPEC: MBEDTLS_SSL_DEBUG_MSG( 2, ( "Remember CCS message" ) ); - hs->seen_ccs = 1; + hs->buffering.seen_ccs = 1; break; case MBEDTLS_SSL_MSG_HANDSHAKE: From 0271f967d60f8c8058aabb610d59e4eb4d69e50c Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 16 Aug 2018 13:23:47 +0100 Subject: [PATCH 16/58] Introduce buffering structure for handshake messages This commit introduces, but does not yet put to use, a sub-structure of mbedtls_ssl_handshake_params::buffering that will be used for the buffering and/or reassembly of handshake messages with handshake sequence numbers that are greater or equal to the next expected sequence number. --- include/mbedtls/ssl_internal.h | 13 ++++++++ library/ssl_tls.c | 58 ++++++++++++++++++++++++++++++++-- 2 files changed, 68 insertions(+), 3 deletions(-) diff --git a/include/mbedtls/ssl_internal.h b/include/mbedtls/ssl_internal.h index b9084b437f..a34d385210 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -155,6 +155,9 @@ #define MBEDTLS_SSL_OUT_PAYLOAD_LEN ( MBEDTLS_SSL_PAYLOAD_OVERHEAD + \ ( MBEDTLS_SSL_OUT_CONTENT_LEN ) ) +/* The maximum number of buffered handshake messages. */ +#define MBEDTLS_SSL_MAX_BUFFERED_HS 2 + /* Maximum length we can advertise as our max content length for RFC 6066 max_fragment_length extension negotiation purposes (the lesser of both sizes, if they are unequal.) @@ -313,6 +316,14 @@ struct mbedtls_ssl_handshake_params uint8_t seen_ccs; /*!< Indicates if a CCS message has * been seen in the current flight. */ + struct mbedtls_ssl_hs_buffer + { + uint8_t is_valid : 1; + uint8_t is_fragmented : 1; + uint8_t is_complete : 1; + unsigned char *data; + } hs[MBEDTLS_SSL_MAX_BUFFERED_HS]; + } buffering; #endif /* MBEDTLS_SSL_PROTO_DTLS */ @@ -372,6 +383,8 @@ struct mbedtls_ssl_handshake_params #endif /* MBEDTLS_SSL_ASYNC_PRIVATE */ }; +typedef struct mbedtls_ssl_hs_buffer mbedtls_ssl_hs_buffer; + /* * This structure contains a full set of runtime transform parameters * either in negotiation or active. diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 5e573422ec..7e01aa35a2 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -167,6 +167,8 @@ static int ssl_get_remaining_payload_in_datagram( mbedtls_ssl_context const *ssl return( (int) remaining ); } +static void ssl_buffering_free( mbedtls_ssl_context *ssl ); + /* * Double the retransmit timeout value, within the allowed range, * returning -1 if the maximum value has already been reached. @@ -3072,6 +3074,9 @@ void mbedtls_ssl_recv_flight_completed( mbedtls_ssl_context *ssl ) /* We don't want to remember CCS's across flight boundaries. */ ssl->handshake->buffering.seen_ccs = 0; + /* Clear future message buffering structure. */ + ssl_buffering_free( ssl ); + /* Cancel timer */ ssl_set_timer( ssl, 0 ); @@ -3747,9 +3752,9 @@ int mbedtls_ssl_prepare_handshake_record( mbedtls_ssl_context *ssl ) 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 && - ssl->handshake != NULL ) + if( ssl->state != MBEDTLS_SSL_HANDSHAKE_OVER && hs != NULL ) { ssl->handshake->update_checksum( ssl, ssl->in_msg, ssl->in_hslen ); } @@ -3759,7 +3764,8 @@ void mbedtls_ssl_update_handshake_status( mbedtls_ssl_context *ssl ) if( ssl->conf->transport == MBEDTLS_SSL_TRANSPORT_DATAGRAM && ssl->handshake != NULL ) { - ssl->handshake->in_msg_seq++; + unsigned offset; + mbedtls_ssl_hs_buffer *hs_buf; /* Clear up handshake reassembly structure, if any. */ if( ssl->handshake->hs_msg != NULL ) @@ -3767,6 +3773,28 @@ void mbedtls_ssl_update_handshake_status( mbedtls_ssl_context *ssl ) mbedtls_free( ssl->handshake->hs_msg ); ssl->handshake->hs_msg = NULL; } + + /* Increment handshake sequence number */ + hs->in_msg_seq++; + + /* + * Clear up handshake buffering and reassembly structure. + */ + + /* Free first entry */ + hs_buf = &hs->buffering.hs[0]; + if( hs_buf->is_valid ) + mbedtls_free( hs_buf->data ); + + /* Shift all other entries */ + for( offset = 0; offset + 1 < MBEDTLS_SSL_MAX_BUFFERED_HS; + offset++, hs_buf++ ) + { + *hs_buf = *(hs_buf + 1); + } + + /* Create a fresh last entry */ + memset( hs_buf, 0, sizeof( mbedtls_ssl_hs_buffer ) ); } #endif } @@ -8286,6 +8314,29 @@ static void ssl_key_cert_free( mbedtls_ssl_key_cert *key_cert ) } #endif /* MBEDTLS_X509_CRT_PARSE_C */ +#if defined(MBEDTLS_SSL_PROTO_DTLS) + +static void ssl_buffering_free( mbedtls_ssl_context *ssl ) +{ + unsigned offset; + mbedtls_ssl_handshake_params * const hs = ssl->handshake; + + if( hs == NULL ) + return; + + for( offset = 0; offset < MBEDTLS_SSL_MAX_BUFFERED_HS; offset++ ) + { + mbedtls_ssl_hs_buffer *hs_buf = &hs->buffering.hs[offset]; + if( hs_buf->is_valid == 1 ) + { + mbedtls_free( hs_buf->data ); + memset( hs_buf, 0, sizeof( mbedtls_ssl_hs_buffer ) ); + } + } +} + +#endif /* MBEDTLS_SSL_PROTO_DTLS */ + void mbedtls_ssl_handshake_free( mbedtls_ssl_context *ssl ) { mbedtls_ssl_handshake_params *handshake = ssl->handshake; @@ -8367,6 +8418,7 @@ void mbedtls_ssl_handshake_free( mbedtls_ssl_context *ssl ) mbedtls_free( handshake->verify_cookie ); mbedtls_free( handshake->hs_msg ); ssl_flight_free( handshake->flight ); + ssl_buffering_free( ssl ); #endif mbedtls_platform_zeroize( handshake, From 12555c61d3a39c215476e841030a65eea0b3b997 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 16 Aug 2018 12:47:53 +0100 Subject: [PATCH 17/58] Introduce function to parse total handshake length This commit introduces a static helper function ssl_get_hs_total_len() parsing the total message length field in the handshake header, and puts it to use in mbedtls_ssl_prepare_handshake_record(). --- library/ssl_tls.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 7e01aa35a2..d7c61655e3 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -55,6 +55,7 @@ #endif static void ssl_reset_in_out_pointers( mbedtls_ssl_context *ssl ); +static uint32_t ssl_get_hs_total_len( mbedtls_ssl_context *ssl ); /* Length of the "epoch" field in the record header */ static inline size_t ssl_ep_len( const mbedtls_ssl_context *ssl ) @@ -3659,6 +3660,12 @@ static int ssl_reassemble_dtls_handshake( mbedtls_ssl_context *ssl ) } #endif /* MBEDTLS_SSL_PROTO_DTLS */ +static uint32_t ssl_get_hs_total_len( mbedtls_ssl_context *ssl ) +{ + return( ( ssl->in_msg[1] << 16 ) | + ( ssl->in_msg[2] << 8 ) | + ssl->in_msg[3] ); +} int mbedtls_ssl_prepare_handshake_record( mbedtls_ssl_context *ssl ) { @@ -3669,10 +3676,7 @@ int mbedtls_ssl_prepare_handshake_record( mbedtls_ssl_context *ssl ) return( MBEDTLS_ERR_SSL_INVALID_RECORD ); } - ssl->in_hslen = mbedtls_ssl_hs_hdr_len( ssl ) + ( - ( ssl->in_msg[1] << 16 ) | - ( ssl->in_msg[2] << 8 ) | - ssl->in_msg[3] ); + ssl->in_hslen = mbedtls_ssl_hs_hdr_len( ssl ) + ssl_get_hs_total_len( ssl ); MBEDTLS_SSL_DEBUG_MSG( 3, ( "handshake message: msglen =" " %d, type = %d, hslen = %d", From 44650b7a7448460d07d02172285151f9a650c746 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 16 Aug 2018 12:51:11 +0100 Subject: [PATCH 18/58] Introduce function checking sanity of the DTLS HS header This commit introduces helper functions - ssl_get_hs_frag_len() - ssl_get_hs_frag_off() to parse the fragment length resp. fragment offset fields in the handshake header. Moreover, building on these helper functions, it adds a function ssl_check_hs_header() checking the validity of a DTLS handshake header with respect to the specification, i.e. the indicated fragment must be a subrange of the total handshake message, and the total handshake fragment length (including header) must not exceed the record content size. These checks were previously performed at a later stage during ssl_reassemble_dtls_handshake(). --- library/ssl_tls.c | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index d7c61655e3..a321eaf420 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -3426,6 +3426,41 @@ static int ssl_hs_is_proper_fragment( mbedtls_ssl_context *ssl ) } return( 0 ); } + +static uint32_t ssl_get_hs_frag_len( mbedtls_ssl_context *ssl ) +{ + return( ( ssl->in_msg[9] << 16 ) | + ( ssl->in_msg[10] << 8 ) | + ssl->in_msg[11] ); +} + +static uint32_t ssl_get_hs_frag_off( mbedtls_ssl_context *ssl ) +{ + return( ( ssl->in_msg[6] << 16 ) | + ( ssl->in_msg[7] << 8 ) | + ssl->in_msg[8] ); +} + +static int ssl_check_hs_header( mbedtls_ssl_context *ssl ) +{ + uint32_t msg_len, frag_off, frag_len; + + msg_len = ssl_get_hs_total_len( ssl ); + frag_off = ssl_get_hs_frag_off( ssl ); + frag_len = ssl_get_hs_frag_len( ssl ); + + if( frag_off > msg_len ) + return( -1 ); + + if( frag_len > msg_len - frag_off ) + return( -1 ); + + if( frag_len + 12 > ssl->in_msglen ) + return( -1 ); + + return( 0 ); +} + /* * Mark bits in bitmask (used for DTLS HS reassembly) */ @@ -3688,6 +3723,12 @@ int mbedtls_ssl_prepare_handshake_record( mbedtls_ssl_context *ssl ) int ret; unsigned int recv_msg_seq = ( ssl->in_msg[4] << 8 ) | ssl->in_msg[5]; + if( ssl_check_hs_header( ssl ) != 0 ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "invalid handshake header" ) ); + return( MBEDTLS_ERR_SSL_INVALID_RECORD ); + } + if( ssl->handshake != NULL && ( ( ssl->state != MBEDTLS_SSL_HANDSHAKE_OVER && recv_msg_seq != ssl->handshake->in_msg_seq ) || From 6d97ef5a0366cb1ee1ae8d586d076fecbb8293e5 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 16 Aug 2018 13:09:04 +0100 Subject: [PATCH 19/58] Use uniform treatment for future messages and proper HS fragments This commit returns the error code MBEDTLS_ERR_SSL_EARLY_MESSAGE for proper handshake fragments, forwarding their treatment to the buffering function ssl_buffer_message(); currently, though, this function does not yet buffer or reassembly HS messages, so: ! This commit temporarily disables support for handshake reassembly ! --- include/mbedtls/ssl_internal.h | 2 - library/ssl_tls.c | 156 ++------------------------------- 2 files changed, 6 insertions(+), 152 deletions(-) diff --git a/include/mbedtls/ssl_internal.h b/include/mbedtls/ssl_internal.h index a34d385210..fbf3e70e84 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -297,8 +297,6 @@ struct mbedtls_ssl_handshake_params unsigned char verify_cookie_len; /*!< Cli: cookie length Srv: flag for sending a cookie */ - unsigned char *hs_msg; /*!< Reassembled handshake message */ - uint32_t retransmit_timeout; /*!< Current value of timeout */ unsigned char retransmit_state; /*!< Retransmission state */ mbedtls_ssl_flight_item *flight; /*!< Current outgoing flight */ diff --git a/library/ssl_tls.c b/library/ssl_tls.c index a321eaf420..ed41686315 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -3558,141 +3558,6 @@ static int ssl_prepare_reassembly_buffer( mbedtls_ssl_context *ssl, /* debug */ return( 0 ); } -/* - * Reassemble fragmented DTLS handshake messages. - * - * Use a temporary buffer for reassembly, divided in two parts: - * - the first holds the reassembled message (including handshake header), - * - the second holds a bitmask indicating which parts of the message - * (excluding headers) have been received so far. - */ -static int ssl_reassemble_dtls_handshake( mbedtls_ssl_context *ssl ) -{ - unsigned char *msg, *bitmask; - size_t frag_len, frag_off; - size_t msg_len = ssl->in_hslen - 12; /* Without headers */ - - if( ssl->handshake == NULL ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "not supported outside handshake (for now)" ) ); - return( MBEDTLS_ERR_SSL_FEATURE_UNAVAILABLE ); - } - - /* - * For first fragment, check size and allocate buffer - */ - if( ssl->handshake->hs_msg == NULL ) - { - ret = ssl_prepare_reassembly_buffer( msg_len, 1, - &ssl->handshake->hs_msg ); - if( ret != 0 ) - return( ret ); - - /* Prepare final header: copy msg_type, length and message_seq, - * then add standardised fragment_offset and fragment_length */ - memcpy( ssl->handshake->hs_msg, ssl->in_msg, 6 ); - memset( ssl->handshake->hs_msg + 6, 0, 3 ); - memcpy( ssl->handshake->hs_msg + 9, - ssl->handshake->hs_msg + 1, 3 ); - } - else - { - /* Make sure msg_type and length are consistent */ - if( memcmp( ssl->handshake->hs_msg, ssl->in_msg, 4 ) != 0 ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "fragment header mismatch" ) ); - return( MBEDTLS_ERR_SSL_INVALID_RECORD ); - } - } - - msg = ssl->handshake->hs_msg + 12; - bitmask = msg + msg_len; - - /* - * Check and copy current fragment - */ - frag_off = ( ssl->in_msg[6] << 16 ) | - ( ssl->in_msg[7] << 8 ) | - ssl->in_msg[8]; - frag_len = ( ssl->in_msg[9] << 16 ) | - ( ssl->in_msg[10] << 8 ) | - ssl->in_msg[11]; - - if( frag_off + frag_len > msg_len ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "invalid fragment offset/len: %d + %d > %d", - frag_off, frag_len, msg_len ) ); - return( MBEDTLS_ERR_SSL_INVALID_RECORD ); - } - - if( frag_len + 12 > ssl->in_msglen ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "invalid fragment length: %d + 12 > %d", - frag_len, ssl->in_msglen ) ); - return( MBEDTLS_ERR_SSL_INVALID_RECORD ); - } - - MBEDTLS_SSL_DEBUG_MSG( 2, ( "adding fragment, offset = %d, length = %d", - frag_off, frag_len ) ); - - memcpy( msg + frag_off, ssl->in_msg + 12, frag_len ); - ssl_bitmask_set( bitmask, frag_off, frag_len ); - - /* - * Do we have the complete message by now? - * If yes, finalize it, else ask to read the next record. - */ - if( ssl_bitmask_check( bitmask, msg_len ) != 0 ) - { - MBEDTLS_SSL_DEBUG_MSG( 2, ( "message is not complete yet" ) ); - return( MBEDTLS_ERR_SSL_CONTINUE_PROCESSING ); - } - - MBEDTLS_SSL_DEBUG_MSG( 2, ( "handshake message completed" ) ); - - if( frag_len + 12 < ssl->in_msglen ) - { - /* - * We'got more handshake messages in the same record. - * This case is not handled now because no know implementation does - * that and it's hard to test, so we prefer to fail cleanly for now. - */ - MBEDTLS_SSL_DEBUG_MSG( 1, ( "last fragment not alone in its record" ) ); - return( MBEDTLS_ERR_SSL_FEATURE_UNAVAILABLE ); - } - - if( ssl->in_left > ssl->next_record_offset ) - { - /* - * We've got more data in the buffer after the current record, - * that we don't want to overwrite. Move it before writing the - * reassembled message, and adjust in_left and next_record_offset. - */ - unsigned char *cur_remain = ssl->in_hdr + ssl->next_record_offset; - unsigned char *new_remain = ssl->in_msg + ssl->in_hslen; - size_t remain_len = ssl->in_left - ssl->next_record_offset; - - /* First compute and check new lengths */ - ssl->next_record_offset = new_remain - ssl->in_hdr; - ssl->in_left = ssl->next_record_offset + remain_len; - - if( ssl->in_left > MBEDTLS_SSL_IN_BUFFER_LEN - - (size_t)( ssl->in_hdr - ssl->in_buf ) ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "reassembled message too large for buffer" ) ); - return( MBEDTLS_ERR_SSL_BUFFER_TOO_SMALL ); - } - - memmove( new_remain, cur_remain, remain_len ); - } - - memcpy( ssl->in_msg, ssl->handshake->hs_msg, ssl->in_hslen ); - - MBEDTLS_SSL_DEBUG_BUF( 3, "reassembled handshake message", - ssl->in_msg, ssl->in_hslen ); - - return( 0 ); -} #endif /* MBEDTLS_SSL_PROTO_DTLS */ static uint32_t ssl_get_hs_total_len( mbedtls_ssl_context *ssl ) @@ -3772,15 +3637,14 @@ int mbedtls_ssl_prepare_handshake_record( mbedtls_ssl_context *ssl ) } /* Wait until message completion to increment in_msg_seq */ + /* Message reassembly is handled alongside buffering of future + * messages; the commonality is that both handshake fragments and + * future messages cannot be forwarded immediately to the handshake + * handshake logic layer. */ if( ssl_hs_is_proper_fragment( ssl ) == 1 ) { MBEDTLS_SSL_DEBUG_MSG( 2, ( "found fragmented DTLS handshake message" ) ); - - if( ( ret = ssl_reassemble_dtls_handshake( ssl ) ) != 0 ) - { - MBEDTLS_SSL_DEBUG_RET( 1, "ssl_reassemble_dtls_handshake", ret ); - return( ret ); - } + return( MBEDTLS_ERR_SSL_EARLY_MESSAGE ); } } else @@ -3812,13 +3676,6 @@ void mbedtls_ssl_update_handshake_status( mbedtls_ssl_context *ssl ) unsigned offset; mbedtls_ssl_hs_buffer *hs_buf; - /* Clear up handshake reassembly structure, if any. */ - if( ssl->handshake->hs_msg != NULL ) - { - mbedtls_free( ssl->handshake->hs_msg ); - ssl->handshake->hs_msg = NULL; - } - /* Increment handshake sequence number */ hs->in_msg_seq++; @@ -4554,7 +4411,7 @@ static int ssl_buffer_message( mbedtls_ssl_context *ssl ) break; case MBEDTLS_SSL_MSG_HANDSHAKE: - /* No support for buffering handshake messages so far. */ + /* TODO: Implement buffering and reassembly here. */ break; default: @@ -8461,7 +8318,6 @@ void mbedtls_ssl_handshake_free( mbedtls_ssl_context *ssl ) #if defined(MBEDTLS_SSL_PROTO_DTLS) mbedtls_free( handshake->verify_cookie ); - mbedtls_free( handshake->hs_msg ); ssl_flight_free( handshake->flight ); ssl_buffering_free( ssl ); #endif From 37f95320814e29fc2d65e4a6b900e28f32a1116f Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 16 Aug 2018 13:55:32 +0100 Subject: [PATCH 20/58] Implement future message buffering and loading This commit implements future handshake message buffering and loading by implementing ssl_load_buffered_message() and ssl_buffer_message(). Whenever a handshake message is received which is - a future handshake message (i.e., the sequence number is larger than the next expected one), or which is - a proper fragment of the next expected handshake message, ssl_buffer_message() is called, which does the following: - Ignore message if its sequence number is too far ahead of the next expected sequence number, as controlled by the macro constant MBEDTLS_SSL_MAX_BUFFERED_HS. - Otherwise, check if buffering for the message with the respective sequence number has already commenced. - If not, allocate space to back up the message within the buffering substructure of mbedtls_ssl_handshake_params. If the message is a proper fragment, allocate additional space for a reassembly bitmap; if it is a full message, omit the bitmap. In any case, fall throuh to the next case. - If the message has already been buffered, check that the header is the same, and add the current fragment if the message is not yet complete (this excludes the case where a future message has been received in a single fragment, hence omitting the bitmap, and is afterwards also received as a series of proper fragments; in this case, the proper fragments will be ignored). For loading buffered messages in ssl_load_buffered_message(), the approach is the following: - Check the first entry in the buffering window (the window is always based at the next expected handshake message). If buffering hasn't started or if reassembly is still in progress, ignore. If the next expected message has been fully received, copy it to the input buffer (which is empty, as ssl_load_buffered_message() is only called in this case). --- library/ssl_tls.c | 171 +++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 170 insertions(+), 1 deletion(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index ed41686315..b6e2c0edb2 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -4354,6 +4354,7 @@ static int ssl_another_record_in_datagram( mbedtls_ssl_context *ssl ) static int ssl_load_buffered_message( mbedtls_ssl_context *ssl ) { mbedtls_ssl_handshake_params * const hs = ssl->handshake; + mbedtls_ssl_hs_buffer * hs_buf; int ret = 0; MBEDTLS_SSL_DEBUG_MSG( 2, ( "=> ssl_load_buffered_messsage" ) ); @@ -4385,6 +4386,58 @@ static int ssl_load_buffered_message( mbedtls_ssl_context *ssl ) hs->buffering.seen_ccs = 0; goto exit; } + + /* Debug only */ + { + unsigned offset; + for( offset = 1; offset < MBEDTLS_SSL_MAX_BUFFERED_HS; offset++ ) + { + hs_buf = &hs->buffering.hs[offset]; + if( hs_buf->is_valid == 1 ) + { + MBEDTLS_SSL_DEBUG_MSG( 2, ( "Future message with sequence number %u %s buffered.", + hs->in_msg_seq + offset, + hs_buf->is_complete ? "fully" : "partitially" ) ); + } + } + } + + /* Check if we have buffered and/or fully reassembled the + * next handshake message. */ + hs_buf = &hs->buffering.hs[0]; + if( ( hs_buf->is_valid == 1 ) && ( hs_buf->is_complete == 1 ) ) + { + /* Synthesize a record containing the buffered HS message. */ + size_t msg_len = ( hs_buf->data[1] << 16 ) | + ( hs_buf->data[2] << 8 ) | + hs_buf->data[3]; + + /* Double-check that we haven't accidentally buffered + * a message that doesn't fit into the input buffer. */ + if( msg_len + 12 > MBEDTLS_SSL_IN_CONTENT_LEN ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "should never happen" ) ); + return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); + } + + MBEDTLS_SSL_DEBUG_MSG( 2, ( "Next handshake message has been buffered - load" ) ); + MBEDTLS_SSL_DEBUG_BUF( 3, "Buffered handshake message (incl. header)", + hs_buf->data, msg_len + 12 ); + + ssl->in_msgtype = MBEDTLS_SSL_MSG_HANDSHAKE; + ssl->in_hslen = msg_len + 12; + ssl->in_msglen = msg_len + 12; + memcpy( ssl->in_msg, hs_buf->data, ssl->in_hslen ); + + ret = 0; + goto exit; + } + else + { + MBEDTLS_SSL_DEBUG_MSG( 2, ( "Next handshake message %u not or only partially bufffered", + hs->in_msg_seq ) ); + } + ret = -1; exit: @@ -4411,8 +4464,124 @@ static int ssl_buffer_message( mbedtls_ssl_context *ssl ) break; case MBEDTLS_SSL_MSG_HANDSHAKE: - /* TODO: Implement buffering and reassembly here. */ + { + unsigned recv_msg_seq_offset; + unsigned recv_msg_seq = ( ssl->in_msg[4] << 8 ) | ssl->in_msg[5]; + mbedtls_ssl_hs_buffer *hs_buf; + size_t msg_len = ssl->in_hslen - 12; + + /* We should never receive an old handshake + * message - double-check nonetheless. */ + if( recv_msg_seq < ssl->handshake->in_msg_seq ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "should never happen" ) ); + return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); + } + + recv_msg_seq_offset = recv_msg_seq - ssl->handshake->in_msg_seq; + if( recv_msg_seq_offset >= MBEDTLS_SSL_MAX_BUFFERED_HS ) + { + /* Silently ignore -- message too far in the future */ + MBEDTLS_SSL_DEBUG_MSG( 2, + ( "Ignore future HS message with sequence number %u, " + "buffering window %u - %u", + recv_msg_seq, ssl->handshake->in_msg_seq, + ssl->handshake->in_msg_seq + MBEDTLS_SSL_MAX_BUFFERED_HS - 1 ) ); + + goto exit; + } + + MBEDTLS_SSL_DEBUG_MSG( 2, ( "Buffering HS message with sequence number %u, offset %u ", + recv_msg_seq, recv_msg_seq_offset ) ); + + hs_buf = &hs->buffering.hs[ recv_msg_seq_offset ]; + + /* Check if the buffering for this seq nr has already commenced. */ + if( ! hs_buf->is_valid ) + { + hs_buf->is_fragmented = + ( ssl_hs_is_proper_fragment( ssl ) == 1 ); + + /* We copy the message back into the input buffer + * after reassembly, so check that it's not too large. + * This is an implementation-specific limitation + * and not one from the standard, hence it is not + * checked in ssl_check_hs_header(). */ + if( msg_len > MBEDTLS_SSL_IN_CONTENT_LEN ) + { + /* Ignore message */ + goto exit; + } + + ret = ssl_prepare_reassembly_buffer( ssl, msg_len, + hs_buf->is_fragmented, + &hs_buf->data ); + if( ret == MBEDTLS_ERR_SSL_ALLOC_FAILED && + recv_msg_seq_offset > 0 ) + { + /* If we run out of RAM trying to buffer a *future* + * message, simply ignore instead of failing. */ + MBEDTLS_SSL_DEBUG_MSG( 2, ( "Not enough RAM available to buffer future message - ignore" ) ); + goto exit; + } + else if( ret != 0 ) + return( ret ); + + /* Prepare final header: copy msg_type, length and message_seq, + * then add standardised fragment_offset and fragment_length */ + memcpy( hs_buf->data, ssl->in_msg, 6 ); + memset( hs_buf->data + 6, 0, 3 ); + memcpy( hs_buf->data + 9, hs_buf->data + 1, 3 ); + + hs_buf->is_valid = 1; + } + else + { + /* Make sure msg_type and length are consistent */ + if( memcmp( hs_buf->data, ssl->in_msg, 4 ) != 0 ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "Fragment header mismatch - ignore" ) ); + /* Ignore */ + goto exit; + } + } + + if( ! hs_buf->is_complete ) + { + size_t frag_len, frag_off; + unsigned char * const msg = hs_buf->data + 12; + + /* + * Check and copy current fragment + */ + + /* Validation of header fields already done in + * mbedtls_ssl_prepare_handshake_record(). */ + frag_off = ssl_get_hs_frag_off( ssl ); + frag_len = ssl_get_hs_frag_len( ssl ); + + MBEDTLS_SSL_DEBUG_MSG( 2, ( "adding fragment, offset = %d, length = %d", + frag_off, frag_len ) ); + memcpy( msg + frag_off, ssl->in_msg + 12, frag_len ); + + if( hs_buf->is_fragmented ) + { + unsigned char * const bitmask = msg + msg_len; + ssl_bitmask_set( bitmask, frag_off, frag_len ); + hs_buf->is_complete = ( ssl_bitmask_check( bitmask, + msg_len ) == 0 ); + } + else + { + hs_buf->is_complete = 1; + } + + MBEDTLS_SSL_DEBUG_MSG( 2, ( "message %scomplete", + hs_buf->is_complete ? "" : "not yet " ) ); + } + break; + } default: break; From e38422107e0f8ea4107fbc85e6253cf8f41cfec8 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 16 Aug 2018 15:28:59 +0100 Subject: [PATCH 21/58] Add test for reordering of handshake messages --- tests/ssl-opt.sh | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index c056000242..15481e183e 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -5743,6 +5743,14 @@ run_test "DTLS proxy: delay ChangeCipherSpec" \ # Tests for reordering support with DTLS +run_test "DTLS reordering: Buffer out-of-order handshake message" \ + -p "$P_PXY delay=2 seed=1" \ + "$P_SRV cookies=0 dtls=1 debug_level=2" \ + "$P_CLI dtls=1 debug_level=2" \ + 0 \ + -c "Buffering HS message" \ + -c "Next handshake message has been buffered - load" + run_test "DTLS reordering: Buffer out-of-order CCS message"\ -p "$P_PXY delay=3 seed=1" \ "$P_SRV cookies=0 dtls=1 debug_level=2" \ From 5f066e7aac1b0a8e8d7178291978bc1b87ee6eac Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 16 Aug 2018 14:56:31 +0100 Subject: [PATCH 22/58] Implement future record buffering This commit implements the buffering of a record from the next epoch. - The buffering substructure of mbedtls_ssl_handshake_params gets another field to hold a raw record (incl. header) from a future epoch. - If ssl_parse_record_header() sees a record from the next epoch, it signals that it might be suitable for buffering by returning MBEDTLS_ERR_SSL_EARLY_MESSAGE. - If ssl_get_next_record() finds this error code, it passes control to ssl_buffer_future_record() which may or may not decide to buffer the record; it does so if - a handshake is in progress, - the record is a handshake record - no record has already been buffered. If these conditions are met, the record is backed up in the aforementioned buffering substructure. - If the current datagram is fully processed, ssl_load_buffered_record() is called to check if a record has been buffered, and if yes, if by now the its epoch is the current one; if yes, it copies the record into the (empty! otherwise, ssl_load_buffered_record() wouldn't have been called) input buffer. --- include/mbedtls/ssl_internal.h | 7 ++ library/ssl_tls.c | 148 ++++++++++++++++++++++++++++++++- 2 files changed, 152 insertions(+), 3 deletions(-) diff --git a/include/mbedtls/ssl_internal.h b/include/mbedtls/ssl_internal.h index fbf3e70e84..6601734013 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -322,6 +322,13 @@ struct mbedtls_ssl_handshake_params unsigned char *data; } hs[MBEDTLS_SSL_MAX_BUFFERED_HS]; + struct + { + unsigned char *data; + size_t len; + unsigned epoch; + } future_record; + } buffering; #endif /* MBEDTLS_SSL_PROTO_DTLS */ diff --git a/library/ssl_tls.c b/library/ssl_tls.c index b6e2c0edb2..85ed1e51c7 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -4097,7 +4097,16 @@ static int ssl_parse_record_header( mbedtls_ssl_context *ssl ) } else #endif /* MBEDTLS_SSL_DTLS_CLIENT_PORT_REUSE && MBEDTLS_SSL_SRV_C */ + { + /* Consider buffering the record. */ + if( rec_epoch == (unsigned int) ssl->in_epoch + 1 ) + { + MBEDTLS_SSL_DEBUG_MSG( 2, ( "Consider record for buffering" ) ); + return( MBEDTLS_ERR_SSL_EARLY_MESSAGE ); + } + return( MBEDTLS_ERR_SSL_UNEXPECTED_RECORD ); + } } #if defined(MBEDTLS_SSL_DTLS_ANTI_REPLAY) @@ -4254,7 +4263,9 @@ static int ssl_record_is_in_progress( mbedtls_ssl_context *ssl ); #if defined(MBEDTLS_SSL_PROTO_DTLS) static int ssl_load_buffered_message( mbedtls_ssl_context *ssl ); +static int ssl_load_buffered_record( mbedtls_ssl_context *ssl ); static int ssl_buffer_message( mbedtls_ssl_context *ssl ); +static int ssl_buffer_future_record( mbedtls_ssl_context *ssl ); static int ssl_another_record_in_datagram( mbedtls_ssl_context *ssl ); #endif /* MBEDTLS_SSL_PROTO_DTLS */ @@ -4689,13 +4700,133 @@ static int ssl_record_is_in_progress( mbedtls_ssl_context *ssl ) return( 0 ); } +#if defined(MBEDTLS_SSL_PROTO_DTLS) + +static void ssl_free_buffered_record( mbedtls_ssl_context *ssl ) +{ + mbedtls_ssl_handshake_params * const hs = ssl->handshake; + if( hs == NULL ) + return; + + mbedtls_free( hs->buffering.future_record.data ); + hs->buffering.future_record.data = NULL; +} + +static int ssl_load_buffered_record( mbedtls_ssl_context *ssl ) +{ + mbedtls_ssl_handshake_params * const hs = ssl->handshake; + unsigned char * rec; + size_t rec_len; + unsigned rec_epoch; + + if( ssl->conf->transport != MBEDTLS_SSL_TRANSPORT_DATAGRAM ) + return( 0 ); + + if( hs == NULL ) + return( 0 ); + + /* Only consider loading future records if the + * input buffer is empty. */ + if( ssl_another_record_in_datagram( ssl ) == 1 ) + return( 0 ); + + rec = hs->buffering.future_record.data; + rec_len = hs->buffering.future_record.len; + rec_epoch = hs->buffering.future_record.epoch; + + if( rec == NULL ) + return( 0 ); + + MBEDTLS_SSL_DEBUG_MSG( 2, ( "=> ssl_load_buffered_record" ) ); + + if( rec_epoch != ssl->in_epoch ) + { + MBEDTLS_SSL_DEBUG_MSG( 2, ( "Buffered record not from current epoch." ) ); + goto exit; + } + + MBEDTLS_SSL_DEBUG_MSG( 2, ( "Found buffered record from current epoch - load" ) ); + + /* Double-check that the record is not too large */ + if( rec_len > MBEDTLS_SSL_IN_BUFFER_LEN - + (size_t)( ssl->in_hdr - ssl->in_buf ) ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "should never happen" ) ); + return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); + } + + memcpy( ssl->in_hdr, rec, rec_len ); + ssl->in_left = rec_len; + ssl->next_record_offset = 0; + + ssl_free_buffered_record( ssl ); + +exit: + MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= ssl_load_buffered_record" ) ); + return( 0 ); +} + +static int ssl_buffer_future_record( mbedtls_ssl_context *ssl ) +{ + mbedtls_ssl_handshake_params * const hs = ssl->handshake; + size_t const rec_hdr_len = 13; + + /* Don't buffer future records outside handshakes. */ + if( hs == NULL ) + return( 0 ); + + /* Only buffer handshake records (we are only interested + * in Finished messages). */ + if( ssl->in_msgtype != MBEDTLS_SSL_MSG_HANDSHAKE ) + return( 0 ); + + /* Don't buffer more than one future epoch record. */ + if( hs->buffering.future_record.data != NULL ) + return( 0 ); + + /* Buffer record */ + MBEDTLS_SSL_DEBUG_MSG( 2, ( "Buffer record from epoch %u", + ssl->in_epoch + 1 ) ); + MBEDTLS_SSL_DEBUG_BUF( 3, "Buffered record", ssl->in_hdr, + rec_hdr_len + ssl->in_msglen ); + + /* ssl_parse_record_header() only considers records + * of the next epoch as candidates for buffering. */ + hs->buffering.future_record.epoch = ssl->in_epoch + 1; + hs->buffering.future_record.len = rec_hdr_len + ssl->in_msglen; + + hs->buffering.future_record.data = + mbedtls_calloc( 1, hs->buffering.future_record.len ); + if( hs->buffering.future_record.data == NULL ) + { + /* If we run out of RAM trying to buffer a + * record from the next epoch, just ignore. */ + return( 0 ); + } + + memcpy( hs->buffering.future_record.data, + ssl->in_hdr, rec_hdr_len + ssl->in_msglen ); + + return( 0 ); +} + +#endif /* MBEDTLS_SSL_PROTO_DTLS */ + static int ssl_get_next_record( mbedtls_ssl_context *ssl ) { int ret; - /* - * Fetch and decode new record - */ +#if defined(MBEDTLS_SSL_PROTO_DTLS) + /* We might have buffered a future record; if so, + * and if the epoch matches now, load it. + * On success, this call will set ssl->in_left to + * the length of the buffered record, so that + * the calls to ssl_fetch_input() below will + * essentially be no-ops. */ + ret = ssl_load_buffered_record( ssl ); + if( ret != 0 ) + return( ret ); +#endif /* MBEDTLS_SSL_PROTO_DTLS */ if( ( ret = mbedtls_ssl_fetch_input( ssl, mbedtls_ssl_hdr_len( ssl ) ) ) != 0 ) { @@ -4709,6 +4840,16 @@ static int ssl_get_next_record( mbedtls_ssl_context *ssl ) if( ssl->conf->transport == MBEDTLS_SSL_TRANSPORT_DATAGRAM && ret != MBEDTLS_ERR_SSL_CLIENT_RECONNECT ) { + if( ret == MBEDTLS_ERR_SSL_EARLY_MESSAGE ) + { + ret = ssl_buffer_future_record( ssl ); + if( ret != 0 ) + return( ret ); + + /* Fall through to handling of unexpected records */ + ret = MBEDTLS_ERR_SSL_UNEXPECTED_RECORD; + } + if( ret == MBEDTLS_ERR_SSL_UNEXPECTED_RECORD ) { /* Skip unexpected record (but not whole datagram) */ @@ -8489,6 +8630,7 @@ void mbedtls_ssl_handshake_free( mbedtls_ssl_context *ssl ) mbedtls_free( handshake->verify_cookie ); ssl_flight_free( handshake->flight ); ssl_buffering_free( ssl ); + ssl_free_buffered_record( ssl ); #endif mbedtls_platform_zeroize( handshake, From b34149c00ff3b629a531feb5f57ea817c10f5c97 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 16 Aug 2018 15:29:06 +0100 Subject: [PATCH 23/58] Add test for buffering of record from next epoch --- tests/ssl-opt.sh | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index 15481e183e..b9601980d6 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -5751,6 +5751,14 @@ run_test "DTLS reordering: Buffer out-of-order handshake message" \ -c "Buffering HS message" \ -c "Next handshake message has been buffered - load" +run_test "DTLS reordering: Buffer record from future epoch" \ + -p "$P_PXY drop=3 seed=2" \ + "$P_SRV cookies=0 dtls=1 debug_level=2" \ + "$P_CLI dtls=1 debug_level=2" \ + 0 \ + -s "Buffer record from epoch 1" \ + -s "Found buffered record from current epoch - load" + run_test "DTLS reordering: Buffer out-of-order CCS message"\ -p "$P_PXY delay=3 seed=1" \ "$P_SRV cookies=0 dtls=1 debug_level=2" \ From b063a5ffade4eade10539b5b198e82af121e54ba Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 16 Aug 2018 16:06:44 +0100 Subject: [PATCH 24/58] Update error codes --- library/error.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/library/error.c b/library/error.c index 774244b454..6c88689190 100644 --- a/library/error.c +++ b/library/error.c @@ -515,6 +515,8 @@ void mbedtls_strerror( int ret, char *buf, size_t buflen ) mbedtls_snprintf( buf, buflen, "SSL - Internal-only message signaling that further message-processing should be done" ); if( use_ret == -(MBEDTLS_ERR_SSL_ASYNC_IN_PROGRESS) ) mbedtls_snprintf( buf, buflen, "SSL - The asynchronous operation is not completed yet" ); + if( use_ret == -(MBEDTLS_ERR_SSL_EARLY_MESSAGE) ) + mbedtls_snprintf( buf, buflen, "SSL - Internal-only message signaling that a message arrived early" ); #endif /* MBEDTLS_SSL_TLS_C */ #if defined(MBEDTLS_X509_USE_C) || defined(MBEDTLS_X509_CREATE_C) From f103542c3db905c19ac99f4de1a18b42f1176e08 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 16 Aug 2018 16:07:27 +0100 Subject: [PATCH 25/58] Adapt ChangeLog --- ChangeLog | 1 + 1 file changed, 1 insertion(+) diff --git a/ChangeLog b/ChangeLog index ef8abc8bf6..9455318617 100644 --- a/ChangeLog +++ b/ChangeLog @@ -35,6 +35,7 @@ Changes Drozd. Fixes #1215 raised by randombit. * Improve compatibility with some alternative CCM implementations by using CCM test vectors from RAM. + * Add support for buffering of out-of-order handshake messages. INTERNAL NOTE: need to bump soversion of libmbedtls: - added new member 'mtu' to public 'mbedtls_ssl_conf' structure From d488b9e490d10906953d1e31a16253d3060e962f Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 16 Aug 2018 16:35:37 +0100 Subject: [PATCH 26/58] Increase maximum number of buffered handshake messages --- include/mbedtls/ssl_internal.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/mbedtls/ssl_internal.h b/include/mbedtls/ssl_internal.h index 6601734013..eb9885a178 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -156,7 +156,7 @@ ( MBEDTLS_SSL_OUT_CONTENT_LEN ) ) /* The maximum number of buffered handshake messages. */ -#define MBEDTLS_SSL_MAX_BUFFERED_HS 2 +#define MBEDTLS_SSL_MAX_BUFFERED_HS 4 /* Maximum length we can advertise as our max content length for RFC 6066 max_fragment_length extension negotiation purposes From 872730481d3d34d287a8a94ff294222778d94b9c Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 16 Aug 2018 16:53:13 +0100 Subject: [PATCH 27/58] Disable datagram packing in reordering tests --- tests/ssl-opt.sh | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index b9601980d6..5434ecfb72 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -5745,24 +5745,24 @@ run_test "DTLS proxy: delay ChangeCipherSpec" \ run_test "DTLS reordering: Buffer out-of-order handshake message" \ -p "$P_PXY delay=2 seed=1" \ - "$P_SRV cookies=0 dtls=1 debug_level=2" \ - "$P_CLI dtls=1 debug_level=2" \ + "$P_SRV dgram_packing=0 cookies=0 dtls=1 debug_level=2" \ + "$P_CLI dgram_packing=0 dtls=1 debug_level=2" \ 0 \ -c "Buffering HS message" \ -c "Next handshake message has been buffered - load" run_test "DTLS reordering: Buffer record from future epoch" \ -p "$P_PXY drop=3 seed=2" \ - "$P_SRV cookies=0 dtls=1 debug_level=2" \ - "$P_CLI dtls=1 debug_level=2" \ + "$P_SRV dgram_packing=0 cookies=0 dtls=1 debug_level=2" \ + "$P_CLI dgram_packing=0 dtls=1 debug_level=2" \ 0 \ -s "Buffer record from epoch 1" \ -s "Found buffered record from current epoch - load" run_test "DTLS reordering: Buffer out-of-order CCS message"\ -p "$P_PXY delay=3 seed=1" \ - "$P_SRV cookies=0 dtls=1 debug_level=2" \ - "$P_CLI dtls=1 debug_level=2" \ + "$P_SRV dgram_packing=0 cookies=0 dtls=1 debug_level=2" \ + "$P_CLI dgram_packing=0 dtls=1 debug_level=2" \ 0 \ -c "Inject buffered CCS message" \ -c "Remember CCS message" From 56d5eaa96c94725df8dc94702e48b4e3eff74911 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 17 Aug 2018 09:06:31 +0100 Subject: [PATCH 28/58] Mark SSL ctx unused in ssl_prepare_reassembly_buffer() if !DEBUG The SSL context is passed to the reassembly preparation function ssl_prepare_reassembly_buffer() solely for the purpose of allowing debugging output. This commit marks the context as unused if debugging is disabled (through !MBEDTLS_DEBUG_C). --- library/ssl_tls.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 85ed1e51c7..c00c974962 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -3531,6 +3531,11 @@ static int ssl_prepare_reassembly_buffer( mbedtls_ssl_context *ssl, /* debug */ size_t alloc_len; unsigned char *buf; +#if !defined(MBEDTLS_DEBUG_C) + /* The SSL context is used for debugging only. */ + ((void) ssl); +#endif + MBEDTLS_SSL_DEBUG_MSG( 2, ( "initialize reassembly, total length = %d", msg_len ) ); From 01ea77836356405885f436f26c93c96fc0edf16a Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 17 Aug 2018 13:33:41 +0100 Subject: [PATCH 29/58] UDP proxy: Add option to delay specific handshake messages --- programs/test/udp_proxy.c | 109 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 109 insertions(+) diff --git a/programs/test/udp_proxy.c b/programs/test/udp_proxy.c index 55e0f249ce..2986ee30a7 100644 --- a/programs/test/udp_proxy.c +++ b/programs/test/udp_proxy.c @@ -40,6 +40,8 @@ #define mbedtls_time time #define mbedtls_time_t time_t #define mbedtls_printf printf +#define mbedtls_calloc calloc +#define mbedtls_free free #define MBEDTLS_EXIT_SUCCESS EXIT_SUCCESS #define MBEDTLS_EXIT_FAILURE EXIT_FAILURE #endif /* MBEDTLS_PLATFORM_C */ @@ -106,6 +108,21 @@ int main( void ) " delay=%%d default: 0 (no delayed packets)\n" \ " delay about 1:N packets randomly\n" \ " delay_ccs=0/1 default: 0 (don't delay ChangeCipherSpec)\n" \ + " delay_cli=%%s Handshake message from client that should be\n"\ + " delayed. Possible values are 'ClientHello',\n" \ + " 'Certificate', 'CertificateVerify', and\n" \ + " 'ClientKeyExchange'.\n" \ + " May be used multiple times, even for the same\n"\ + " message, in which case the respective message\n"\ + " gets delayed multiple times.\n" \ + " delay_srv=%%s Handshake message from server that should be\n"\ + " delayed. Possible values are 'HelloRequest',\n"\ + " 'ServerHello', 'ServerHelloDone', 'Certificate'\n"\ + " 'ServerKeyExchange', 'NewSessionTicket',\n"\ + " 'HelloVerifyRequest' and ''CertificateRequest'.\n"\ + " May be used multiple times, even for the same\n"\ + " message, in which case the respective message\n"\ + " gets delayed multiple times.\n" \ " drop=%%d default: 0 (no dropped packets)\n" \ " drop about 1:N packets randomly\n" \ " mtu=%%d default: 0 (unlimited)\n" \ @@ -121,6 +138,9 @@ int main( void ) /* * global options */ + +#define MAX_DELAYED_HS 10 + static struct options { const char *server_addr; /* address to forward packets to */ @@ -131,6 +151,12 @@ static struct options int duplicate; /* duplicate 1 in N packets (none if 0) */ int delay; /* delay 1 packet in N (none if 0) */ int delay_ccs; /* delay ChangeCipherSpec */ + char* delay_cli[MAX_DELAYED_HS]; /* handshake types of messages from + * client that should be delayed. */ + uint8_t delay_cli_cnt; /* Number of entries in delay_cli. */ + char* delay_srv[MAX_DELAYED_HS]; /* handshake types of messages from + * server that should be delayed. */ + uint8_t delay_srv_cnt; /* Number of entries in delay_srv. */ int drop; /* drop 1 packet in N (none if 0) */ int mtu; /* drop packets larger than this */ int bad_ad; /* inject corrupted ApplicationData record */ @@ -164,6 +190,11 @@ static void get_options( int argc, char *argv[] ) opt.pack = DFL_PACK; /* Other members default to 0 */ + opt.delay_cli_cnt = 0; + opt.delay_srv_cnt = 0; + memset( opt.delay_cli, 0, sizeof( opt.delay_cli ) ); + memset( opt.delay_srv, 0, sizeof( opt.delay_srv ) ); + for( i = 1; i < argc; i++ ) { p = argv[i]; @@ -197,6 +228,43 @@ static void get_options( int argc, char *argv[] ) if( opt.delay_ccs < 0 || opt.delay_ccs > 1 ) exit_usage( p, q ); } + else if( strcmp( p, "delay_cli" ) == 0 || + strcmp( p, "delay_srv" ) == 0 ) + { + uint8_t *delay_cnt; + char **delay_list; + size_t len; + char *buf; + + if( strcmp( p, "delay_cli" ) == 0 ) + { + delay_cnt = &opt.delay_cli_cnt; + delay_list = opt.delay_cli; + } + else + { + delay_cnt = &opt.delay_srv_cnt; + delay_list = opt.delay_srv; + } + + if( *delay_cnt == MAX_DELAYED_HS ) + { + mbedtls_printf( " maximally %d uses of delay_cli argument allows\n", + MAX_DELAYED_HS ); + exit_usage( p, NULL ); + } + + len = strlen( q ); + buf = mbedtls_calloc( 1, len + 1 ); + if( buf == NULL ) + { + mbedtls_printf( " Allocation failure\n" ); + exit( 1 ); + } + memcpy( buf, q, len + 1 ); + + delay_list[ (*delay_cnt)++ ] = buf; + } else if( strcmp( p, "drop" ) == 0 ) { opt.drop = atoi( q ); @@ -540,6 +608,10 @@ int handle_message( const char *way, packet cur; size_t id; + uint8_t delay_idx; + char ** delay_list; + uint8_t delay_list_len; + /* receive packet */ if( ( ret = mbedtls_net_recv( src, cur.buf, sizeof( cur.buf ) ) ) <= 0 ) { @@ -555,6 +627,36 @@ int handle_message( const char *way, id = cur.len % sizeof( dropped ); + if( strcmp( way, "S <- C" ) == 0 ) + { + delay_list = opt.delay_cli; + delay_list_len = opt.delay_cli_cnt; + } + else + { + delay_list = opt.delay_srv; + delay_list_len = opt.delay_srv_cnt; + } + /* Check if message type is in the list of messages + * that should be delayed */ + for( delay_idx = 0; delay_idx < delay_list_len; delay_idx++ ) + { + if( delay_list[ delay_idx ] == NULL ) + continue; + + if( strcmp( delay_list[ delay_idx ], cur.type ) == 0 ) + { + /* Delay message */ + memcpy( &prev, &cur, sizeof( packet ) ); + + /* Remove entry from list */ + mbedtls_free( delay_list[delay_idx] ); + delay_list[delay_idx] = NULL; + + return( 0 ); + } + } + /* do we want to drop, delay, or forward it? */ if( ( opt.mtu != 0 && cur.len > (unsigned) opt.mtu ) || @@ -604,6 +706,7 @@ int main( int argc, char *argv[] ) { int ret = 1; int exit_code = MBEDTLS_EXIT_FAILURE; + uint8_t delay_idx; mbedtls_net_context listen_fd, client_fd, server_fd; @@ -798,6 +901,12 @@ exit: } #endif + for( delay_idx = 0; delay_idx < MAX_DELAYED_HS; delay_idx++ ) + { + mbedtls_free( opt.delay_cli + delay_idx ); + mbedtls_free( opt.delay_srv + delay_idx ); + } + mbedtls_net_free( &client_fd ); mbedtls_net_free( &server_fd ); mbedtls_net_free( &listen_fd ); From 56cdfd1e2995c76a5bb95d74651d1fc9815330b1 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 17 Aug 2018 13:42:15 +0100 Subject: [PATCH 30/58] Refine reordering tests Now that the UDP proxy has the ability to delay specific handshake message on the client and server side, use this to rewrite the reordering tests and thereby make them independent on the choice of PRNG used by the proxy (which is not stable across platforms). --- tests/ssl-opt.sh | 70 ++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 56 insertions(+), 14 deletions(-) diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index 5434ecfb72..4b32314c53 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -5743,29 +5743,71 @@ run_test "DTLS proxy: delay ChangeCipherSpec" \ # Tests for reordering support with DTLS -run_test "DTLS reordering: Buffer out-of-order handshake message" \ - -p "$P_PXY delay=2 seed=1" \ +run_test "DTLS reordering: Buffer out-of-order handshake message on client" \ + -p "$P_PXY delay_srv=ServerHello" \ "$P_SRV dgram_packing=0 cookies=0 dtls=1 debug_level=2" \ "$P_CLI dgram_packing=0 dtls=1 debug_level=2" \ 0 \ -c "Buffering HS message" \ - -c "Next handshake message has been buffered - load" + -c "Next handshake message has been buffered - load"\ + -S "Buffering HS message" \ + -S "Next handshake message has been buffered - load"\ + -C "Inject buffered CCS message" \ + -C "Remember CCS message" \ + -S "Inject buffered CCS message" \ + -S "Remember CCS message" -run_test "DTLS reordering: Buffer record from future epoch" \ - -p "$P_PXY drop=3 seed=2" \ +run_test "DTLS reordering: Buffer out-of-order handshake message on server" \ + -p "$P_PXY delay_cli=Certificate" \ + "$P_SRV dgram_packing=0 auth_mode=required cookies=0 dtls=1 debug_level=2" \ + "$P_CLI dgram_packing=0 dtls=1 debug_level=2" \ + 0 \ + -C "Buffering HS message" \ + -C "Next handshake message has been buffered - load"\ + -s "Buffering HS message" \ + -s "Next handshake message has been buffered - load" \ + -C "Inject buffered CCS message" \ + -C "Remember CCS message" \ + -S "Inject buffered CCS message" \ + -S "Remember CCS message" + +run_test "DTLS reordering: Buffer out-of-order CCS message on client"\ + -p "$P_PXY delay_srv=NewSessionTicket" \ + "$P_SRV dgram_packing=0 cookies=0 dtls=1 debug_level=2" \ + "$P_CLI dgram_packing=0 dtls=1 debug_level=2" \ + 0 \ + -C "Buffering HS message" \ + -C "Next handshake message has been buffered - load"\ + -S "Buffering HS message" \ + -S "Next handshake message has been buffered - load" \ + -c "Inject buffered CCS message" \ + -c "Remember CCS message" \ + -S "Inject buffered CCS message" \ + -S "Remember CCS message" + +run_test "DTLS reordering: Buffer out-of-order CCS message on server"\ + -p "$P_PXY delay_cli=ClientKeyExchange" \ + "$P_SRV dgram_packing=0 cookies=0 dtls=1 debug_level=2" \ + "$P_CLI dgram_packing=0 dtls=1 debug_level=2" \ + 0 \ + -C "Buffering HS message" \ + -C "Next handshake message has been buffered - load"\ + -S "Buffering HS message" \ + -S "Next handshake message has been buffered - load" \ + -C "Inject buffered CCS message" \ + -C "Remember CCS message" \ + -s "Inject buffered CCS message" \ + -s "Remember CCS message" + +run_test "DTLS reordering: Buffer record from future epoch (client and server)" \ + -p "$P_PXY delay_ccs=1" \ "$P_SRV dgram_packing=0 cookies=0 dtls=1 debug_level=2" \ "$P_CLI dgram_packing=0 dtls=1 debug_level=2" \ 0 \ -s "Buffer record from epoch 1" \ - -s "Found buffered record from current epoch - load" - -run_test "DTLS reordering: Buffer out-of-order CCS message"\ - -p "$P_PXY delay=3 seed=1" \ - "$P_SRV dgram_packing=0 cookies=0 dtls=1 debug_level=2" \ - "$P_CLI dgram_packing=0 dtls=1 debug_level=2" \ - 0 \ - -c "Inject buffered CCS message" \ - -c "Remember CCS message" + -s "Found buffered record from current epoch - load" \ + -c "Buffer record from epoch 1" \ + -c "Found buffered record from current epoch - load" # Tests for "randomly unreliable connection": try a variety of flows and peers From 0d4b376ddf559b88b6625ba1821ae4b128f9a08a Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Mon, 20 Aug 2018 09:36:59 +0100 Subject: [PATCH 31/58] Return through cleanup section in ssl_load_buffered_message() --- library/ssl_tls.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index c00c974962..e6b5ad2095 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -4387,7 +4387,7 @@ static int ssl_load_buffered_message( mbedtls_ssl_context *ssl ) { MBEDTLS_SSL_DEBUG_MSG( 2, ( "CCS not seen in the current flight" ) ); ret = -1; - return( -1 ); + goto exit; } MBEDTLS_SSL_DEBUG_MSG( 2, ( "Inject buffered CCS message" ) ); From e00ae375d3cb981e0c804486517b33e99d89b540 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Mon, 20 Aug 2018 09:39:42 +0100 Subject: [PATCH 32/58] Omit debug output in ssl_load_buffered_message outside a handshake --- library/ssl_tls.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index e6b5ad2095..8ead5fa7c7 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -4373,11 +4373,11 @@ static int ssl_load_buffered_message( mbedtls_ssl_context *ssl ) mbedtls_ssl_hs_buffer * hs_buf; int ret = 0; - MBEDTLS_SSL_DEBUG_MSG( 2, ( "=> ssl_load_buffered_messsage" ) ); - if( hs == NULL ) return( -1 ); + MBEDTLS_SSL_DEBUG_MSG( 2, ( "=> ssl_load_buffered_messsage" ) ); + if( ssl->state == MBEDTLS_SSL_CLIENT_CHANGE_CIPHER_SPEC || ssl->state == MBEDTLS_SSL_SERVER_CHANGE_CIPHER_SPEC ) { From 4422bbb096ec1ebfde9112714153f3fdc03b2814 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Mon, 20 Aug 2018 09:40:19 +0100 Subject: [PATCH 33/58] Whitespace fixes --- library/ssl_tls.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 8ead5fa7c7..b8ca1545f9 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -4383,7 +4383,7 @@ static int ssl_load_buffered_message( mbedtls_ssl_context *ssl ) { /* Check if we have seen a ChangeCipherSpec before. * If yes, synthesize a CCS record. */ - if( ! hs->buffering.seen_ccs ) + if( !hs->buffering.seen_ccs ) { MBEDTLS_SSL_DEBUG_MSG( 2, ( "CCS not seen in the current flight" ) ); ret = -1; @@ -4513,7 +4513,7 @@ static int ssl_buffer_message( mbedtls_ssl_context *ssl ) hs_buf = &hs->buffering.hs[ recv_msg_seq_offset ]; /* Check if the buffering for this seq nr has already commenced. */ - if( ! hs_buf->is_valid ) + if( !hs_buf->is_valid ) { hs_buf->is_fragmented = ( ssl_hs_is_proper_fragment( ssl ) == 1 ); @@ -4562,7 +4562,7 @@ static int ssl_buffer_message( mbedtls_ssl_context *ssl ) } } - if( ! hs_buf->is_complete ) + if( !hs_buf->is_complete ) { size_t frag_len, frag_off; unsigned char * const msg = hs_buf->data + 12; From 3a0aad1c9d9380cdcab2b019e7b5a41dafe8d781 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Mon, 20 Aug 2018 09:44:02 +0100 Subject: [PATCH 34/58] Rename `update_digest` to `update_hs_digest` --- include/mbedtls/ssl_internal.h | 8 ++++++-- library/ssl_tls.c | 4 ++-- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/include/mbedtls/ssl_internal.h b/include/mbedtls/ssl_internal.h index eb9885a178..bfc3a5a424 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -515,7 +515,10 @@ void mbedtls_ssl_update_handshake_status( mbedtls_ssl_context *ssl ); * of the logic of (D)TLS from the implementation * of the secure transport. * - * \param ssl SSL context to use + * \param ssl The SSL context to use. + * \param update_hs_digest This indicates if the handshake digest + * should be automatically updated in case + * a handshake message is found. * * \return 0 or non-zero error code. * @@ -581,7 +584,8 @@ void mbedtls_ssl_update_handshake_status( mbedtls_ssl_context *ssl ); * following the above definition. * */ -int mbedtls_ssl_read_record( mbedtls_ssl_context *ssl, unsigned update_digest ); +int mbedtls_ssl_read_record( mbedtls_ssl_context *ssl, + unsigned update_hs_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_tls.c b/library/ssl_tls.c index b8ca1545f9..19523bac92 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -4275,7 +4275,7 @@ static int ssl_another_record_in_datagram( mbedtls_ssl_context *ssl ); #endif /* MBEDTLS_SSL_PROTO_DTLS */ int mbedtls_ssl_read_record( mbedtls_ssl_context *ssl, - unsigned update_digest ) + unsigned update_hs_digest ) { int ret; @@ -4342,7 +4342,7 @@ int mbedtls_ssl_read_record( mbedtls_ssl_context *ssl, } if( ssl->in_msgtype == MBEDTLS_SSL_MSG_HANDSHAKE && - update_digest == 1 ) + update_hs_digest == 1 ) { mbedtls_ssl_update_handshake_status( ssl ); } From caf874189165c202ea1b744c2d4a4f1d572164ac Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Mon, 20 Aug 2018 09:45:51 +0100 Subject: [PATCH 35/58] Fix typo in documentation of UDP proxy argument 'delay_cli' --- programs/test/udp_proxy.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/programs/test/udp_proxy.c b/programs/test/udp_proxy.c index 2986ee30a7..77eaa5d2f4 100644 --- a/programs/test/udp_proxy.c +++ b/programs/test/udp_proxy.c @@ -249,7 +249,7 @@ static void get_options( int argc, char *argv[] ) if( *delay_cnt == MAX_DELAYED_HS ) { - mbedtls_printf( " maximally %d uses of delay_cli argument allows\n", + mbedtls_printf( " maximally %d uses of delay_cli argument allowed\n", MAX_DELAYED_HS ); exit_usage( p, NULL ); } From 4cb782d2f67d186feef72e57c376f5831c20b0c8 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Mon, 20 Aug 2018 11:19:05 +0100 Subject: [PATCH 36/58] Return from ssl_load_buffered_record early if no record is buffered --- library/ssl_tls.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 19523bac92..058173c4aa 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -4730,11 +4730,6 @@ static int ssl_load_buffered_record( mbedtls_ssl_context *ssl ) if( hs == NULL ) return( 0 ); - /* Only consider loading future records if the - * input buffer is empty. */ - if( ssl_another_record_in_datagram( ssl ) == 1 ) - return( 0 ); - rec = hs->buffering.future_record.data; rec_len = hs->buffering.future_record.len; rec_epoch = hs->buffering.future_record.epoch; @@ -4742,6 +4737,11 @@ static int ssl_load_buffered_record( mbedtls_ssl_context *ssl ) if( rec == NULL ) return( 0 ); + /* Only consider loading future records if the + * input buffer is empty. */ + if( ssl_another_record_in_datagram( ssl ) == 1 ) + return( 0 ); + MBEDTLS_SSL_DEBUG_MSG( 2, ( "=> ssl_load_buffered_record" ) ); if( rec_epoch != ssl->in_epoch ) From e678eaa93e37d2833c6a5565a8b320f6a7640249 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 21 Aug 2018 14:57:46 +0100 Subject: [PATCH 37/58] Reject invalid CCS records early This commit moves the length and content check for CCS messages to the function mbedtls_ssl_handle_message_type() which is called after a record has been deprotected. Previously, these checks were performed in the function mbedtls_ssl_parse_change_cipher_spec(); however, now that the arrival of out-of-order CCS messages is remembered as a boolean flag, the check also has to happen when this flag is set. Moving the length and content check to mbedtls_ssl_handle_message_type() allows to treat both checks uniformly. --- library/ssl_tls.c | 49 +++++++++++++++++++++++++++++------------------ 1 file changed, 30 insertions(+), 19 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 058173c4aa..4b64fe6239 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -4476,6 +4476,7 @@ static int ssl_buffer_message( mbedtls_ssl_context *ssl ) { case MBEDTLS_SSL_MSG_CHANGE_CIPHER_SPEC: MBEDTLS_SSL_DEBUG_MSG( 2, ( "Remember CCS message" ) ); + hs->buffering.seen_ccs = 1; break; @@ -4986,23 +4987,38 @@ int mbedtls_ssl_handle_message_type( mbedtls_ssl_context *ssl ) } } -#if defined(MBEDTLS_SSL_PROTO_DTLS) - /* Drop unexpected ChangeCipherSpec messages */ - if( ssl->conf->transport == MBEDTLS_SSL_TRANSPORT_DATAGRAM && - ssl->in_msgtype == MBEDTLS_SSL_MSG_CHANGE_CIPHER_SPEC && - ssl->state != MBEDTLS_SSL_CLIENT_CHANGE_CIPHER_SPEC && - ssl->state != MBEDTLS_SSL_SERVER_CHANGE_CIPHER_SPEC ) + if( ssl->in_msgtype == MBEDTLS_SSL_MSG_CHANGE_CIPHER_SPEC ) { - if( ssl->handshake == NULL ) + if( ssl->in_msglen != 1 ) { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "dropping ChangeCipherSpec outside handshake" ) ); - return( MBEDTLS_ERR_SSL_UNEXPECTED_RECORD ); + MBEDTLS_SSL_DEBUG_MSG( 1, ( "invalid CCS message, len: %d", + ssl->in_msglen ) ); + return( MBEDTLS_ERR_SSL_INVALID_RECORD ); } - MBEDTLS_SSL_DEBUG_MSG( 1, ( "received out-of-order ChangeCipherSpec - remember" ) ); - return( MBEDTLS_ERR_SSL_EARLY_MESSAGE ); - } + if( ssl->in_msg[0] != 1 ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "invalid CCS message, content: %02x", + ssl->in_msg[0] ) ); + return( MBEDTLS_ERR_SSL_INVALID_RECORD ); + } + +#if defined(MBEDTLS_SSL_PROTO_DTLS) + if( ssl->conf->transport == MBEDTLS_SSL_TRANSPORT_DATAGRAM && + ssl->state != MBEDTLS_SSL_CLIENT_CHANGE_CIPHER_SPEC && + ssl->state != MBEDTLS_SSL_SERVER_CHANGE_CIPHER_SPEC ) + { + if( ssl->handshake == NULL ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "dropping ChangeCipherSpec outside handshake" ) ); + return( MBEDTLS_ERR_SSL_UNEXPECTED_RECORD ); + } + + MBEDTLS_SSL_DEBUG_MSG( 1, ( "received out-of-order ChangeCipherSpec - remember" ) ); + return( MBEDTLS_ERR_SSL_EARLY_MESSAGE ); + } #endif + } if( ssl->in_msgtype == MBEDTLS_SSL_MSG_ALERT ) { @@ -5718,13 +5734,8 @@ int mbedtls_ssl_parse_change_cipher_spec( mbedtls_ssl_context *ssl ) return( MBEDTLS_ERR_SSL_UNEXPECTED_MESSAGE ); } - if( ssl->in_msglen != 1 || ssl->in_msg[0] != 1 ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad change cipher spec message" ) ); - mbedtls_ssl_send_alert_message( ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, - MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR ); - return( MBEDTLS_ERR_SSL_BAD_HS_CHANGE_CIPHER_SPEC ); - } + /* CCS records are only accepted if they have length 1 and content '1', + * so we don't need to check this here. */ /* * Switch to our negotiated transform and session parameters for inbound From 2a97b0e7a37b5ccc0e84118552aac7f6e58724c5 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 21 Aug 2018 15:47:49 +0100 Subject: [PATCH 38/58] Introduce function to return size of buffer needed for reassembly A previous commit introduced the function ssl_prepare_reassembly_buffer() which took a message length and a boolean flag indicating if a reassembly bit map was needed, and attempted to heap-allocate a buffer of sufficient size to hold both the message, its header, and potentially the reassembly bitmap. A subsequent commit is going to introduce a limit on the amount of heap allocations allowed for the purpose of buffering, and this change will need to know the reassembly buffer size before attempting the allocation. To this end, this commit changes ssl_prepare_reassembly_buffer() into ssl_get_reassembly_buffer_size() which solely computes the reassembly buffer size, and performing the heap allocation manually in ssl_buffer_message(). --- library/ssl_tls.c | 43 +++++++++---------------------------------- 1 file changed, 9 insertions(+), 34 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 4b64fe6239..7eb1c89a8f 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -3523,28 +3523,10 @@ static int ssl_bitmask_check( unsigned char *mask, size_t len ) } /* msg_len does not include the handshake header */ -static int ssl_prepare_reassembly_buffer( mbedtls_ssl_context *ssl, /* debug */ - unsigned msg_len, - unsigned add_bitmap, - unsigned char **target ) +static size_t ssl_get_reassembly_buffer_size( unsigned msg_len, + unsigned add_bitmap ) { size_t alloc_len; - unsigned char *buf; - -#if !defined(MBEDTLS_DEBUG_C) - /* The SSL context is used for debugging only. */ - ((void) ssl); -#endif - - MBEDTLS_SSL_DEBUG_MSG( 2, ( "initialize reassembly, total length = %d", - msg_len ) ); - - /* NOTE: That should be checked earlier */ - if( msg_len > MBEDTLS_SSL_IN_CONTENT_LEN ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "handshake message too large" ) ); - return( MBEDTLS_ERR_SSL_FEATURE_UNAVAILABLE ); - } alloc_len = 12; /* Handshake header */ alloc_len += msg_len; /* Content buffer */ @@ -3552,15 +3534,7 @@ static int ssl_prepare_reassembly_buffer( mbedtls_ssl_context *ssl, /* debug */ if( add_bitmap ) alloc_len += msg_len / 8 + ( msg_len % 8 != 0 ); /* Bitmap */ - buf = mbedtls_calloc( 1, alloc_len ); - if( buf == NULL ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "alloc failed (%d bytes)", alloc_len ) ); - return( MBEDTLS_ERR_SSL_ALLOC_FAILED ); - } - - *target = buf; - return( 0 ); + return( alloc_len ); } #endif /* MBEDTLS_SSL_PROTO_DTLS */ @@ -4516,6 +4490,8 @@ static int ssl_buffer_message( mbedtls_ssl_context *ssl ) /* Check if the buffering for this seq nr has already commenced. */ if( !hs_buf->is_valid ) { + size_t reassembly_buf_sz; + hs_buf->is_fragmented = ( ssl_hs_is_proper_fragment( ssl ) == 1 ); @@ -4530,11 +4506,10 @@ static int ssl_buffer_message( mbedtls_ssl_context *ssl ) goto exit; } - ret = ssl_prepare_reassembly_buffer( ssl, msg_len, - hs_buf->is_fragmented, - &hs_buf->data ); - if( ret == MBEDTLS_ERR_SSL_ALLOC_FAILED && - recv_msg_seq_offset > 0 ) + reassembly_buf_sz = ssl_get_reassembly_buffer_size( msg_len, + hs_buf->is_fragmented ); + hs_buf->data = mbedtls_calloc( 1, reassembly_buf_sz ); + if( hs_buf->data == NULL ) { /* If we run out of RAM trying to buffer a *future* * message, simply ignore instead of failing. */ From e0b150f96bfa4430d5d3b960f9d40153dfa13dfb Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 21 Aug 2018 15:51:03 +0100 Subject: [PATCH 39/58] Allow limiting the total amount of heap allocations for buffering This commit introduces a compile time constant MBEDTLS_SSL_DTLS_MAX_BUFFERING to mbedtls/config.h which allows the user to control the cumulative size of all heap buffer allocated for the purpose of reassembling and buffering handshake messages. It is put to use by introducing a new field `total_bytes_buffered` to the buffering substructure of `mbedtls_ssl_handshake_params` that keeps track of the total size of heap allocated buffers for the purpose of reassembly and buffering at any time. It is increased whenever a handshake message is buffered or prepared for reassembly, and decreased when a buffered or fully reassembled message is copied into the input buffer and passed to the handshake logic layer. This commit does not yet include future epoch record buffering into account; this will be done in a subsequent commit. Also, it is now conceivable that the reassembly of the next expected handshake message fails because too much buffering space has already been used up for future messages. This case currently leads to an error, but instead, the stack should get rid of buffered messages to be able to buffer the next one. This will need to be implemented in one of the next commits. --- include/mbedtls/config.h | 8 ++++++ include/mbedtls/ssl.h | 4 +++ include/mbedtls/ssl_internal.h | 4 +++ library/ssl_tls.c | 46 ++++++++++++++++++++++++++++++---- 4 files changed, 57 insertions(+), 5 deletions(-) diff --git a/include/mbedtls/config.h b/include/mbedtls/config.h index 70820be56f..70dd4be2b4 100644 --- a/include/mbedtls/config.h +++ b/include/mbedtls/config.h @@ -3010,6 +3010,14 @@ */ //#define MBEDTLS_SSL_OUT_CONTENT_LEN 16384 +/** \def MBEDTLS_SSL_DTLS_MAX_BUFFERING + * + * Maximum number of heap-allocated bytes for the purpose of + * DTLS handshake message reassembly and future message buffering. + * + */ +//#define MBEDTLS_SSL_DTLS_MAX_BUFFERING ( 2 * 16384 ) + //#define MBEDTLS_SSL_DEFAULT_TICKET_LIFETIME 86400 /**< Lifetime of session tickets (if enabled) */ //#define MBEDTLS_PSK_MAX_LEN 32 /**< Max size of TLS pre-shared keys, in bytes (default 256 bits) */ //#define MBEDTLS_SSL_COOKIE_TIMEOUT 60 /**< Default expiration delay of DTLS cookies, in seconds if HAVE_TIME, or in number of cookies issued */ diff --git a/include/mbedtls/ssl.h b/include/mbedtls/ssl.h index 3a8dd21e99..29c139ed16 100644 --- a/include/mbedtls/ssl.h +++ b/include/mbedtls/ssl.h @@ -243,6 +243,10 @@ #define MBEDTLS_SSL_OUT_CONTENT_LEN MBEDTLS_SSL_MAX_CONTENT_LEN #endif +#if !defined(MBEDTLS_SSL_DTLS_MAX_BUFFERING) +#define MBEDTLS_SSL_DTLS_MAX_BUFFERING ( 2 * MBEDTLS_SSL_IN_CONTENT_LEN ) +#endif + /* \} name SECTION: Module settings */ /* diff --git a/include/mbedtls/ssl_internal.h b/include/mbedtls/ssl_internal.h index bfc3a5a424..2c0684f3dc 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -311,6 +311,9 @@ struct mbedtls_ssl_handshake_params struct { + size_t total_bytes_buffered; /*!< Cumulative size of heap allocated + * buffers used for message buffering. */ + uint8_t seen_ccs; /*!< Indicates if a CCS message has * been seen in the current flight. */ @@ -320,6 +323,7 @@ struct mbedtls_ssl_handshake_params uint8_t is_fragmented : 1; uint8_t is_complete : 1; unsigned char *data; + size_t data_len; } hs[MBEDTLS_SSL_MAX_BUFFERED_HS]; struct diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 7eb1c89a8f..f4ed28a669 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -3665,7 +3665,10 @@ void mbedtls_ssl_update_handshake_status( mbedtls_ssl_context *ssl ) /* Free first entry */ hs_buf = &hs->buffering.hs[0]; if( hs_buf->is_valid ) + { + hs->buffering.total_bytes_buffered -= hs_buf->data_len; mbedtls_free( hs_buf->data ); + } /* Shift all other entries */ for( offset = 0; offset + 1 < MBEDTLS_SSL_MAX_BUFFERED_HS; @@ -4506,18 +4509,49 @@ static int ssl_buffer_message( mbedtls_ssl_context *ssl ) goto exit; } + /* Check if we have enough space to buffer the message. */ + if( hs->buffering.total_bytes_buffered > + MBEDTLS_SSL_DTLS_MAX_BUFFERING ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "should never happen" ) ); + return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); + } + reassembly_buf_sz = ssl_get_reassembly_buffer_size( msg_len, hs_buf->is_fragmented ); + + if( reassembly_buf_sz > ( MBEDTLS_SSL_DTLS_MAX_BUFFERING - + hs->buffering.total_bytes_buffered ) ) + { + if( recv_msg_seq_offset > 0 ) + { + /* If we can't buffer a future message because + * of space limitations -- ignore. */ + MBEDTLS_SSL_DEBUG_MSG( 2, ( "Buffering of future message of size %u would exceed the compile-time limit %u (already %u bytes buffered) -- ignore\n", + (unsigned) msg_len, MBEDTLS_SSL_DTLS_MAX_BUFFERING, + (unsigned) hs->buffering.total_bytes_buffered ) ); + goto exit; + } + + /* TODO: Remove future messages in the attempt to make + * space for the current one. */ + MBEDTLS_SSL_DEBUG_MSG( 2, ( "Reassembly of next message of size %u would exceed the compile-time limit %u (already %u bytes buffered) -- fail\n", + (unsigned) msg_len, MBEDTLS_SSL_DTLS_MAX_BUFFERING, + (unsigned) hs->buffering.total_bytes_buffered ) ); + ret = MBEDTLS_ERR_SSL_ALLOC_FAILED; + goto exit; + } + + MBEDTLS_SSL_DEBUG_MSG( 2, ( "initialize reassembly, total length = %d", + msg_len ) ); + hs_buf->data = mbedtls_calloc( 1, reassembly_buf_sz ); if( hs_buf->data == NULL ) { - /* If we run out of RAM trying to buffer a *future* - * message, simply ignore instead of failing. */ - MBEDTLS_SSL_DEBUG_MSG( 2, ( "Not enough RAM available to buffer future message - ignore" ) ); + ret = MBEDTLS_ERR_SSL_ALLOC_FAILED; goto exit; } - else if( ret != 0 ) - return( ret ); + hs_buf->data_len = reassembly_buf_sz; /* Prepare final header: copy msg_type, length and message_seq, * then add standardised fragment_offset and fragment_length */ @@ -4526,6 +4560,8 @@ static int ssl_buffer_message( mbedtls_ssl_context *ssl ) memcpy( hs_buf->data + 9, hs_buf->data + 1, 3 ); hs_buf->is_valid = 1; + + hs->buffering.total_bytes_buffered += reassembly_buf_sz; } else { From 96a6c69d0c41df4b09fc43f05a83c556c5f96fa7 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 21 Aug 2018 15:56:03 +0100 Subject: [PATCH 40/58] Correct bounds check in ssl_buffer_message() The previous bounds check omitted the DTLS handshake header. --- library/ssl_tls.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index f4ed28a669..17010b5943 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -4503,7 +4503,7 @@ static int ssl_buffer_message( mbedtls_ssl_context *ssl ) * This is an implementation-specific limitation * and not one from the standard, hence it is not * checked in ssl_check_hs_header(). */ - if( msg_len > MBEDTLS_SSL_IN_CONTENT_LEN ) + if( msg_len + 12 > MBEDTLS_SSL_IN_CONTENT_LEN ) { /* Ignore message */ goto exit; From e605b196312edf5e20538386d7686d47eec13ec1 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 21 Aug 2018 15:59:07 +0100 Subject: [PATCH 41/58] Add function to free a particular buffering slot This commit adds a static function ssl_buffering_free_slot() which allows to free a particular structure used to buffer and/or reassembly some handshake message. --- library/ssl_tls.c | 31 ++++++++++++++++++------------- 1 file changed, 18 insertions(+), 13 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 17010b5943..5ab172d65b 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -170,6 +170,9 @@ static int ssl_get_remaining_payload_in_datagram( mbedtls_ssl_context const *ssl static void ssl_buffering_free( mbedtls_ssl_context *ssl ); +static void ssl_buffering_free_slot( mbedtls_ssl_context *ssl, + uint8_t slot ); + /* * Double the retransmit timeout value, within the allowed range, * returning -1 if the maximum value has already been reached. @@ -3663,15 +3666,11 @@ void mbedtls_ssl_update_handshake_status( mbedtls_ssl_context *ssl ) */ /* Free first entry */ - hs_buf = &hs->buffering.hs[0]; - if( hs_buf->is_valid ) - { - hs->buffering.total_bytes_buffered -= hs_buf->data_len; - mbedtls_free( hs_buf->data ); - } + ssl_buffering_free_slot( ssl, 0 ); /* Shift all other entries */ - for( offset = 0; offset + 1 < MBEDTLS_SSL_MAX_BUFFERED_HS; + for( offset = 0, hs_buf = &hs->buffering.hs[0]; + offset + 1 < MBEDTLS_SSL_MAX_BUFFERED_HS; offset++, hs_buf++ ) { *hs_buf = *(hs_buf + 1); @@ -8564,13 +8563,19 @@ static void ssl_buffering_free( mbedtls_ssl_context *ssl ) return; for( offset = 0; offset < MBEDTLS_SSL_MAX_BUFFERED_HS; offset++ ) + ssl_buffering_free_slot( ssl, offset ); +} + +static void ssl_buffering_free_slot( mbedtls_ssl_context *ssl, + uint8_t slot ) +{ + mbedtls_ssl_handshake_params * const hs = ssl->handshake; + mbedtls_ssl_hs_buffer * const hs_buf = &hs->buffering.hs[slot]; + if( hs_buf->is_valid == 1 ) { - mbedtls_ssl_hs_buffer *hs_buf = &hs->buffering.hs[offset]; - if( hs_buf->is_valid == 1 ) - { - mbedtls_free( hs_buf->data ); - memset( hs_buf, 0, sizeof( mbedtls_ssl_hs_buffer ) ); - } + hs->buffering.total_bytes_buffered -= hs_buf->data_len; + mbedtls_free( hs_buf->data ); + memset( hs_buf, 0, sizeof( mbedtls_ssl_hs_buffer ) ); } } From 55e9e2aa6b60dabaa5d461742cb73b1fff74324c Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 21 Aug 2018 16:07:55 +0100 Subject: [PATCH 42/58] Free future buffers if next handshake messages can't be reassembled If the next expected handshake message can't be reassembled because buffered future messages have already used up too much of the available space for buffering, free those future message buffers in order to make space for the reassembly, starting with the handshake message that's farthest in the future. --- library/ssl_tls.c | 33 ++++++++++++++++++++++++++++----- 1 file changed, 28 insertions(+), 5 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 5ab172d65b..d0d5d72c57 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -4522,6 +4522,8 @@ static int ssl_buffer_message( mbedtls_ssl_context *ssl ) if( reassembly_buf_sz > ( MBEDTLS_SSL_DTLS_MAX_BUFFERING - hs->buffering.total_bytes_buffered ) ) { + int offset; + if( recv_msg_seq_offset > 0 ) { /* If we can't buffer a future message because @@ -4532,13 +4534,34 @@ static int ssl_buffer_message( mbedtls_ssl_context *ssl ) goto exit; } - /* TODO: Remove future messages in the attempt to make - * space for the current one. */ - MBEDTLS_SSL_DEBUG_MSG( 2, ( "Reassembly of next message of size %u would exceed the compile-time limit %u (already %u bytes buffered) -- fail\n", + /* We don't have enough space to buffer the next expected + * handshake message. Remove buffers used for future msgs + * to gain space, starting with the most distant one. */ + for( offset = MBEDTLS_SSL_MAX_BUFFERED_HS - 1; + offset >= 0; offset-- ) + { + MBEDTLS_SSL_DEBUG_MSG( 2, ( "Free buffering slot %d to make space for reassembly of next handshake message", + offset ) ); + + ssl_buffering_free_slot( ssl, offset ); + + /* Check if we have enough space available now. */ + if( reassembly_buf_sz <= + ( MBEDTLS_SSL_DTLS_MAX_BUFFERING - + hs->buffering.total_bytes_buffered ) ) + { + break; + } + } + + if( offset == -1 ) + { + MBEDTLS_SSL_DEBUG_MSG( 2, ( "Reassembly of next message of size %u would exceed the compile-time limit %u (already %u bytes buffered) -- fail\n", (unsigned) msg_len, MBEDTLS_SSL_DTLS_MAX_BUFFERING, (unsigned) hs->buffering.total_bytes_buffered ) ); - ret = MBEDTLS_ERR_SSL_ALLOC_FAILED; - goto exit; + ret = MBEDTLS_ERR_SSL_BUFFER_TOO_SMALL; + goto exit; + } } MBEDTLS_SSL_DEBUG_MSG( 2, ( "initialize reassembly, total length = %d", From 101bcba26fda95e166f0692ba5bbc120b647f40e Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 21 Aug 2018 16:39:51 +0100 Subject: [PATCH 43/58] UDP proxy: Allow more than one message to be delayed Previously, the UDP proxy could only remember one delayed message for future transmission; if two messages were delayed in succession, without another one being normally forwarded in between, the message that got delayed first would be dropped. This commit enhances the UDP proxy to allow to delay an arbitrary (compile-time fixed) number of messages in succession. --- programs/test/udp_proxy.c | 45 ++++++++++++++++++++++++++++----------- 1 file changed, 33 insertions(+), 12 deletions(-) diff --git a/programs/test/udp_proxy.c b/programs/test/udp_proxy.c index 77eaa5d2f4..0428d28884 100644 --- a/programs/test/udp_proxy.c +++ b/programs/test/udp_proxy.c @@ -556,11 +556,37 @@ int send_packet( const packet *p, const char *why ) return( 0 ); } -static packet prev; +#define MAX_DELAYED_MSG 5 +static size_t prev_len; +static packet prev[MAX_DELAYED_MSG]; void clear_pending( void ) { memset( &prev, 0, sizeof( packet ) ); + prev_len = 0; +} + +void delay_packet( packet *delay ) +{ + if( prev_len == MAX_DELAYED_MSG ) + return; + + memcpy( &prev[prev_len++], delay, sizeof( packet ) ); +} + +int send_delayed() +{ + uint8_t offset; + int ret; + for( offset = 0; offset < prev_len; offset++ ) + { + ret = send_packet( &prev[offset], "delayed" ); + if( ret != 0 ) + return( ret ); + } + + clear_pending(); + return( 0 ); } /* @@ -647,7 +673,7 @@ int handle_message( const char *way, if( strcmp( delay_list[ delay_idx ], cur.type ) == 0 ) { /* Delay message */ - memcpy( &prev, &cur, sizeof( packet ) ); + delay_packet( &cur ); /* Remove entry from list */ mbedtls_free( delay_list[delay_idx] ); @@ -676,12 +702,11 @@ int handle_message( const char *way, strcmp( cur.type, "ApplicationData" ) != 0 && ! ( opt.protect_hvr && strcmp( cur.type, "HelloVerifyRequest" ) == 0 ) && - prev.dst == NULL && cur.len != (size_t) opt.protect_len && dropped[id] < DROP_MAX && rand() % opt.delay == 0 ) ) { - memcpy( &prev, &cur, sizeof( packet ) ); + delay_packet( &cur ); } else { @@ -689,14 +714,10 @@ int handle_message( const char *way, if( ( ret = send_packet( &cur, "forwarded" ) ) != 0 ) return( ret ); - /* send previously delayed message if any */ - if( prev.dst != NULL ) - { - ret = send_packet( &prev, "delayed" ); - memset( &prev, 0, sizeof( packet ) ); - if( ret != 0 ) - return( ret ); - } + /* send previously delayed messages if any */ + ret = send_delayed(); + if( ret != 0 ) + return( ret ); } return( 0 ); From e35670528bc0d93021bba1d22cff63a03ca9ec1a Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 21 Aug 2018 16:50:43 +0100 Subject: [PATCH 44/58] ssl-opt.sh: Add test for reassembly after reordering --- tests/ssl-opt.sh | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index 4b32314c53..8d4ffde77e 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -5757,6 +5757,20 @@ run_test "DTLS reordering: Buffer out-of-order handshake message on client" \ -S "Inject buffered CCS message" \ -S "Remember CCS message" +run_test "DTLS reordering: Buffer out-of-order handshake message on client before reassembling next" \ + -p "$P_PXY delay_srv=Certificate delay_srv=Certificate" \ + "$P_SRV mtu=512 dgram_packing=0 cookies=0 dtls=1 debug_level=2" \ + "$P_CLI dgram_packing=0 dtls=1 debug_level=2" \ + 0 \ + -c "Buffering HS message" \ + -c "Next handshake message has been buffered - load"\ + -S "Buffering HS message" \ + -S "Next handshake message has been buffered - load"\ + -C "Inject buffered CCS message" \ + -C "Remember CCS message" \ + -S "Inject buffered CCS message" \ + -S "Remember CCS message" + run_test "DTLS reordering: Buffer out-of-order handshake message on server" \ -p "$P_PXY delay_cli=Certificate" \ "$P_SRV dgram_packing=0 auth_mode=required cookies=0 dtls=1 debug_level=2" \ From e1801399a9a3513ed9189ba9399daca26338aac3 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 21 Aug 2018 16:51:05 +0100 Subject: [PATCH 45/58] Add another debug message to ssl_buffer_message() Report if there's not enough buffering space available to reassemble the next expected incoming message. --- library/ssl_tls.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index d0d5d72c57..bb4c0000cd 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -4533,6 +4533,12 @@ static int ssl_buffer_message( mbedtls_ssl_context *ssl ) (unsigned) hs->buffering.total_bytes_buffered ) ); goto exit; } + else + { + MBEDTLS_SSL_DEBUG_MSG( 2, ( "Buffering of future message of size %u would exceed the compile-time limit %u (already %u bytes buffered) -- attempt to make space by freeing buffered future messages\n", + (unsigned) msg_len, MBEDTLS_SSL_DTLS_MAX_BUFFERING, + (unsigned) hs->buffering.total_bytes_buffered ) ); + } /* We don't have enough space to buffer the next expected * handshake message. Remove buffers used for future msgs From a02b0b462d2508e70e0a1f870597480e68edb7fd Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 21 Aug 2018 17:20:27 +0100 Subject: [PATCH 46/58] Add function making space for current message reassembly This commit adds a static function ssl_buffer_make_space() which takes a buffer size as an argument and attempts to free as many future message bufffers as necessary to ensure that the desired amount of buffering space is available without violating the total buffering limit set by MBEDTLS_SSL_DTLS_MAX_BUFFERING. --- library/ssl_tls.c | 53 +++++++++++++++++++++++++++-------------------- 1 file changed, 30 insertions(+), 23 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index bb4c0000cd..a1cf5749d0 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -4438,6 +4438,35 @@ exit: return( ret ); } +static int ssl_buffer_make_space( mbedtls_ssl_context *ssl, + size_t desired ) +{ + int offset; + mbedtls_ssl_handshake_params * const hs = ssl->handshake; + + + /* We don't have enough space to buffer the next expected + * handshake message. Remove buffers used for future msgs + * to gain space, starting with the most distant one. */ + for( offset = MBEDTLS_SSL_MAX_BUFFERED_HS - 1; + offset >= 0; offset-- ) + { + MBEDTLS_SSL_DEBUG_MSG( 2, ( "Free buffering slot %d to make space for reassembly of next handshake message", + offset ) ); + + ssl_buffering_free_slot( ssl, offset ); + + /* Check if we have enough space available now. */ + if( desired <= ( MBEDTLS_SSL_DTLS_MAX_BUFFERING - + hs->buffering.total_bytes_buffered ) ) + { + return( 0 ); + } + } + + return( -1 ); +} + static int ssl_buffer_message( mbedtls_ssl_context *ssl ) { int ret = 0; @@ -4522,8 +4551,6 @@ static int ssl_buffer_message( mbedtls_ssl_context *ssl ) if( reassembly_buf_sz > ( MBEDTLS_SSL_DTLS_MAX_BUFFERING - hs->buffering.total_bytes_buffered ) ) { - int offset; - if( recv_msg_seq_offset > 0 ) { /* If we can't buffer a future message because @@ -4540,27 +4567,7 @@ static int ssl_buffer_message( mbedtls_ssl_context *ssl ) (unsigned) hs->buffering.total_bytes_buffered ) ); } - /* We don't have enough space to buffer the next expected - * handshake message. Remove buffers used for future msgs - * to gain space, starting with the most distant one. */ - for( offset = MBEDTLS_SSL_MAX_BUFFERED_HS - 1; - offset >= 0; offset-- ) - { - MBEDTLS_SSL_DEBUG_MSG( 2, ( "Free buffering slot %d to make space for reassembly of next handshake message", - offset ) ); - - ssl_buffering_free_slot( ssl, offset ); - - /* Check if we have enough space available now. */ - if( reassembly_buf_sz <= - ( MBEDTLS_SSL_DTLS_MAX_BUFFERING - - hs->buffering.total_bytes_buffered ) ) - { - break; - } - } - - if( offset == -1 ) + if( ssl_buffer_make_space( ssl, reassembly_buf_sz ) != 0 ) { MBEDTLS_SSL_DEBUG_MSG( 2, ( "Reassembly of next message of size %u would exceed the compile-time limit %u (already %u bytes buffered) -- fail\n", (unsigned) msg_len, MBEDTLS_SSL_DTLS_MAX_BUFFERING, From 01315ea03a142f232d218dfd14a07e963bf95a0c Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 21 Aug 2018 17:22:17 +0100 Subject: [PATCH 47/58] Account for future epoch records in the total buffering size Previous commits introduced the field `total_bytes_buffered` which is supposed to keep track of the cumulative size of all heap allocated buffers used for the purpose of reassembly and/or buffering of future messages. However, the buffering of future epoch records were not reflected in this field so far. This commit changes this, adding the length of a future epoch record to `total_bytes_buffered` when it's buffered, and subtracting it when it's freed. --- library/ssl_tls.c | 37 ++++++++++++++++++++++++++++++++----- 1 file changed, 32 insertions(+), 5 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index a1cf5749d0..72be09716e 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -4438,12 +4438,22 @@ exit: return( ret ); } +static void ssl_free_buffered_record( mbedtls_ssl_context *ssl ); static int ssl_buffer_make_space( mbedtls_ssl_context *ssl, size_t desired ) { int offset; mbedtls_ssl_handshake_params * const hs = ssl->handshake; + /* Get rid of future records epoch first, if such exist. */ + ssl_free_buffered_record( ssl ); + + /* Check if we have enough space available now. */ + if( desired <= ( MBEDTLS_SSL_DTLS_MAX_BUFFERING - + hs->buffering.total_bytes_buffered ) ) + { + return( 0 ); + } /* We don't have enough space to buffer the next expected * handshake message. Remove buffers used for future msgs @@ -4760,8 +4770,14 @@ static void ssl_free_buffered_record( mbedtls_ssl_context *ssl ) if( hs == NULL ) return; - mbedtls_free( hs->buffering.future_record.data ); - hs->buffering.future_record.data = NULL; + if( hs->buffering.future_record.data != NULL ) + { + hs->buffering.total_bytes_buffered -= + hs->buffering.future_record.len; + + mbedtls_free( hs->buffering.future_record.data ); + hs->buffering.future_record.data = NULL; + } } static int ssl_load_buffered_record( mbedtls_ssl_context *ssl ) @@ -4822,6 +4838,7 @@ static int ssl_buffer_future_record( mbedtls_ssl_context *ssl ) { mbedtls_ssl_handshake_params * const hs = ssl->handshake; size_t const rec_hdr_len = 13; + size_t const total_buf_sz = rec_hdr_len + ssl->in_msglen; /* Don't buffer future records outside handshakes. */ if( hs == NULL ) @@ -4836,6 +4853,16 @@ static int ssl_buffer_future_record( mbedtls_ssl_context *ssl ) if( hs->buffering.future_record.data != NULL ) return( 0 ); + /* Don't buffer record if there's not enough buffering space remaining. */ + if( total_buf_sz > ( MBEDTLS_SSL_DTLS_MAX_BUFFERING - + hs->buffering.total_bytes_buffered ) ) + { + MBEDTLS_SSL_DEBUG_MSG( 2, ( "Buffering of future epoch record of size %u would exceed the compile-time limit %u (already %u bytes buffered) -- ignore\n", + (unsigned) total_buf_sz, MBEDTLS_SSL_DTLS_MAX_BUFFERING, + (unsigned) hs->buffering.total_bytes_buffered ) ); + return( 0 ); + } + /* Buffer record */ MBEDTLS_SSL_DEBUG_MSG( 2, ( "Buffer record from epoch %u", ssl->in_epoch + 1 ) ); @@ -4845,7 +4872,7 @@ static int ssl_buffer_future_record( mbedtls_ssl_context *ssl ) /* ssl_parse_record_header() only considers records * of the next epoch as candidates for buffering. */ hs->buffering.future_record.epoch = ssl->in_epoch + 1; - hs->buffering.future_record.len = rec_hdr_len + ssl->in_msglen; + hs->buffering.future_record.len = total_buf_sz; hs->buffering.future_record.data = mbedtls_calloc( 1, hs->buffering.future_record.len ); @@ -4856,9 +4883,9 @@ static int ssl_buffer_future_record( mbedtls_ssl_context *ssl ) return( 0 ); } - memcpy( hs->buffering.future_record.data, - ssl->in_hdr, rec_hdr_len + ssl->in_msglen ); + memcpy( hs->buffering.future_record.data, ssl->in_hdr, total_buf_sz ); + hs->buffering.total_bytes_buffered += total_buf_sz; return( 0 ); } From aa249378536da468d9958852512fe208351dbf91 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 22 Aug 2018 10:27:13 +0100 Subject: [PATCH 48/58] Adapt ChangeLog --- ChangeLog | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/ChangeLog b/ChangeLog index 975b3bac07..f5e5fa5398 100644 --- a/ChangeLog +++ b/ChangeLog @@ -12,6 +12,10 @@ Features last paragraph). * Add support for packing multiple records within a single datagram, enabled by default. + * Add support for buffering out-of-order handshake messages. + The maximum amount of RAM used for this can be controlled by the + compile-time constant MBEDTLS_SSL_DTLS_MAX_BUFFERING defined + in mbedtls/config.h. API Changes * Add function mbedtls_ssl_conf_datagram_packing() to configure From 98081a09e66f358eaa7aeb1cca1fe7b4d836c8bd Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 22 Aug 2018 13:32:50 +0100 Subject: [PATCH 49/58] Don't use uint8_t for bitfields Fixing a build failure using armcc. --- include/mbedtls/ssl_internal.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/include/mbedtls/ssl_internal.h b/include/mbedtls/ssl_internal.h index 3f595a3223..4b4417a5fa 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -319,9 +319,9 @@ struct mbedtls_ssl_handshake_params struct mbedtls_ssl_hs_buffer { - uint8_t is_valid : 1; - uint8_t is_fragmented : 1; - uint8_t is_complete : 1; + unsigned is_valid : 1; + unsigned is_fragmented : 1; + unsigned is_complete : 1; unsigned char *data; size_t data_len; } hs[MBEDTLS_SSL_MAX_BUFFERED_HS]; From 65dc885a3b04572a32c32d708ee10adc9217d77d Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 23 Aug 2018 09:40:49 +0100 Subject: [PATCH 50/58] Use size_t for msg_len argument in ssl_get_reassembly_buffer_size() --- library/ssl_tls.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 2090e33b4a..651d5a55b6 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -3555,7 +3555,7 @@ static int ssl_bitmask_check( unsigned char *mask, size_t len ) } /* msg_len does not include the handshake header */ -static size_t ssl_get_reassembly_buffer_size( unsigned msg_len, +static size_t ssl_get_reassembly_buffer_size( size_t msg_len, unsigned add_bitmap ) { size_t alloc_len; From 12b72c182e6e9885f88e5cc5cb1c5e22e7c25e0d Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 23 Aug 2018 13:15:36 +0100 Subject: [PATCH 51/58] UDP proxy: Fix bug in freeing delayed messages --- programs/test/udp_proxy.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/programs/test/udp_proxy.c b/programs/test/udp_proxy.c index 0428d28884..2585220037 100644 --- a/programs/test/udp_proxy.c +++ b/programs/test/udp_proxy.c @@ -562,7 +562,7 @@ static packet prev[MAX_DELAYED_MSG]; void clear_pending( void ) { - memset( &prev, 0, sizeof( packet ) ); + memset( &prev, 0, sizeof( prev ) ); prev_len = 0; } From b309b92ee83a2f852f886815dae963ce2ab3bb36 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 23 Aug 2018 13:18:05 +0100 Subject: [PATCH 52/58] ssl_buffering_free_slot(): Double-check validity of slot index --- library/ssl_tls.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 651d5a55b6..41803b6094 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -4493,7 +4493,7 @@ static int ssl_buffer_make_space( mbedtls_ssl_context *ssl, MBEDTLS_SSL_DEBUG_MSG( 2, ( "Free buffering slot %d to make space for reassembly of next handshake message", offset ) ); - ssl_buffering_free_slot( ssl, offset ); + ssl_buffering_free_slot( ssl, (uint8_t) offset ); /* Check if we have enough space available now. */ if( desired <= ( MBEDTLS_SSL_DTLS_MAX_BUFFERING - @@ -8681,6 +8681,10 @@ static void ssl_buffering_free_slot( mbedtls_ssl_context *ssl, { mbedtls_ssl_handshake_params * const hs = ssl->handshake; mbedtls_ssl_hs_buffer * const hs_buf = &hs->buffering.hs[slot]; + + if( slot >= MBEDTLS_SSL_MAX_BUFFERED_HS ) + return; + if( hs_buf->is_valid == 1 ) { hs->buffering.total_bytes_buffered -= hs_buf->data_len; From 283f5efe7dac73a6ed0e12f495dfb10b3bdef846 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 24 Aug 2018 09:34:47 +0100 Subject: [PATCH 53/58] Buffering: Free future record epoch after each flight The function ssl_free_buffered_record() frees a future epoch record, if such is present. Previously, it was called in mbedtls_handshake_free(), i.e. an unused buffered record would be cleared at the end of the handshake. This commit moves the call to the function ssl_buffering_free() responsible for freeing all buffering-related data, and which is called not only at the end of the handshake, but at the end of every flight. In particular, future record epochs won't be buffered across flight boundaries anymore, and they shouldn't. --- library/ssl_tls.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 41803b6094..d8d2563780 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -8672,6 +8672,8 @@ static void ssl_buffering_free( mbedtls_ssl_context *ssl ) if( hs == NULL ) return; + ssl_free_buffered_record( ssl ); + for( offset = 0; offset < MBEDTLS_SSL_MAX_BUFFERED_HS; offset++ ) ssl_buffering_free_slot( ssl, offset ); } @@ -8776,7 +8778,6 @@ void mbedtls_ssl_handshake_free( mbedtls_ssl_context *ssl ) mbedtls_free( handshake->verify_cookie ); ssl_flight_free( handshake->flight ); ssl_buffering_free( ssl ); - ssl_free_buffered_record( ssl ); #endif mbedtls_platform_zeroize( handshake, From 6e12c1ea7d2aaa80b1d8265b0a181ffa3a5aa7bd Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 24 Aug 2018 14:39:15 +0100 Subject: [PATCH 54/58] Enhance debugging output --- library/ssl_tls.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index d28be2a39d..ccd73996d8 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -4487,6 +4487,8 @@ static int ssl_buffer_make_space( mbedtls_ssl_context *ssl, { int offset; mbedtls_ssl_handshake_params * const hs = ssl->handshake; + MBEDTLS_SSL_DEBUG_MSG( 2, ( "Attempt to free buffered messages to have %u bytes available", + (unsigned) desired ) ); /* Get rid of future records epoch first, if such exist. */ ssl_free_buffered_record( ssl ); @@ -4495,6 +4497,7 @@ static int ssl_buffer_make_space( mbedtls_ssl_context *ssl, if( desired <= ( MBEDTLS_SSL_DTLS_MAX_BUFFERING - hs->buffering.total_bytes_buffered ) ) { + MBEDTLS_SSL_DEBUG_MSG( 2, ( "Enough space available after freeing future epoch record" ) ); return( 0 ); } @@ -4513,6 +4516,7 @@ static int ssl_buffer_make_space( mbedtls_ssl_context *ssl, if( desired <= ( MBEDTLS_SSL_DTLS_MAX_BUFFERING - hs->buffering.total_bytes_buffered ) ) { + MBEDTLS_SSL_DEBUG_MSG( 2, ( "Enough space available after freeing buffered HS messages" ) ); return( 0 ); } } @@ -4622,8 +4626,10 @@ static int ssl_buffer_message( mbedtls_ssl_context *ssl ) if( ssl_buffer_make_space( ssl, reassembly_buf_sz ) != 0 ) { - MBEDTLS_SSL_DEBUG_MSG( 2, ( "Reassembly of next message of size %u would exceed the compile-time limit %u (already %u bytes buffered) -- fail\n", - (unsigned) msg_len, MBEDTLS_SSL_DTLS_MAX_BUFFERING, + MBEDTLS_SSL_DEBUG_MSG( 2, ( "Reassembly of next message of size %u (%u with bitmap) would exceed the compile-time limit %u (already %u bytes buffered) -- fail\n", + (unsigned) msg_len, + (unsigned) reassembly_buf_sz, + MBEDTLS_SSL_DTLS_MAX_BUFFERING, (unsigned) hs->buffering.total_bytes_buffered ) ); ret = MBEDTLS_ERR_SSL_BUFFER_TOO_SMALL; goto exit; From 5cd017f931d15f3b351a888061841082cb04fdd9 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 24 Aug 2018 14:40:12 +0100 Subject: [PATCH 55/58] ssl-opt.sh: Allow numerical constraints for tests This commit adds functions requires_config_value_at_most() and requires_config_value_at_least() which can be used to only run tests when a numerical value from config.h (e.g. MBEDTLS_SSL_IN_CONTENT_LEN) is within a certain range. --- tests/ssl-opt.sh | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index c12ca6a8e3..bfcc6342df 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -156,6 +156,26 @@ requires_config_disabled() { fi } +requires_config_value_at_least() { + NAME="$1" + DEF_VAL=$( grep ".*#define.*MBEDTLS_SSL_DTLS_MAX_BUFFERING" ../include/mbedtls/config.h | + sed 's/^.*\s\([0-9]*\)$/\1/' ) + VAL=$( ../scripts/config.pl get $NAME || echo "$DEF_VAL" ) + if [ "$VAL" -lt "$2" ]; then + SKIP_NEXT="YES" + fi +} + +requires_config_value_at_most() { + NAME="$1" + DEF_VAL=$( grep ".*#define.*MBEDTLS_SSL_DTLS_MAX_BUFFERING" ../include/mbedtls/config.h | + sed 's/^.*\s\([0-9]*\)$/\1/' ) + VAL=$( ../scripts/config.pl get $NAME || echo "$DEF_VAL" ) + if [ "$VAL" -gt "$2" ]; then + SKIP_NEXT="YES" + fi +} + # skip next test if OpenSSL doesn't support FALLBACK_SCSV requires_openssl_with_fallback_scsv() { if [ -z "${OPENSSL_HAS_FBSCSV:-}" ]; then From a1adcca1dabf048d3e4152df26161c6534081494 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 24 Aug 2018 14:41:07 +0100 Subject: [PATCH 56/58] ssl-opt.sh: Add tests exercising freeing of buffered messages This commit adds tests to ssl-opt.sh which trigger code-paths responsible for freeing future buffered messages when the buffering limitations set by MBEDTLS_SSL_DTLS_MAX_BUFFERING don't allow the next expected message to be reassembled. These tests only work for very specific ranges of MBEDTLS_SSL_DTLS_MAX_BUFFERING and will therefore be skipped on a run of ssl-opt.sh in ordinary configurations. --- tests/ssl-opt.sh | 58 ++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 56 insertions(+), 2 deletions(-) diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index bfcc6342df..ff36e6c574 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -5904,13 +5904,39 @@ run_test "DTLS reordering: Buffer out-of-order handshake message on client" \ -S "Inject buffered CCS message" \ -S "Remember CCS message" -run_test "DTLS reordering: Buffer out-of-order handshake message on client before reassembling next" \ +# The client buffers the ServerKeyExchange before receiving the fragmented +# Certificate message; at the time of writing, together these are aroudn 1200b +# in size, so that the bound below ensures that the certificate can be reassembled +# while keeping the ServerKeyExchange. +requires_config_value_at_least "MBEDTLS_SSL_DTLS_MAX_BUFFERING" 1300 +run_test "DTLS reordering: Buffer out-of-order hs msg before reassembling next" \ -p "$P_PXY delay_srv=Certificate delay_srv=Certificate" \ "$P_SRV mtu=512 dgram_packing=0 cookies=0 dtls=1 debug_level=2" \ "$P_CLI dgram_packing=0 dtls=1 debug_level=2" \ 0 \ -c "Buffering HS message" \ -c "Next handshake message has been buffered - load"\ + -C "attempt to make space by freeing buffered messages" \ + -S "Buffering HS message" \ + -S "Next handshake message has been buffered - load"\ + -C "Inject buffered CCS message" \ + -C "Remember CCS message" \ + -S "Inject buffered CCS message" \ + -S "Remember CCS message" + +# The size constraints ensure that the delayed certificate message can't +# be reassembled while keeping the ServerKeyExchange message, but it can +# when dropping it first. +requires_config_value_at_least "MBEDTLS_SSL_DTLS_MAX_BUFFERING" 900 +requires_config_value_at_most "MBEDTLS_SSL_DTLS_MAX_BUFFERING" 1299 +run_test "DTLS reordering: Buffer out-of-order hs msg before reassembling next, free buffered msg" \ + -p "$P_PXY delay_srv=Certificate delay_srv=Certificate" \ + "$P_SRV mtu=512 dgram_packing=0 cookies=0 dtls=1 debug_level=2" \ + "$P_CLI dgram_packing=0 dtls=1 debug_level=2" \ + 0 \ + -c "Buffering HS message" \ + -c "attempt to make space by freeing buffered future messages" \ + -c "Enough space available after freeing buffered HS messages" \ -S "Buffering HS message" \ -S "Next handshake message has been buffered - load"\ -C "Inject buffered CCS message" \ @@ -5960,7 +5986,7 @@ run_test "DTLS reordering: Buffer out-of-order CCS message on server"\ -s "Inject buffered CCS message" \ -s "Remember CCS message" -run_test "DTLS reordering: Buffer record from future epoch (client and server)" \ +run_test "DTLS reordering: Buffer encrypted Finished message" \ -p "$P_PXY delay_ccs=1" \ "$P_SRV dgram_packing=0 cookies=0 dtls=1 debug_level=2" \ "$P_CLI dgram_packing=0 dtls=1 debug_level=2" \ @@ -5970,6 +5996,34 @@ run_test "DTLS reordering: Buffer record from future epoch (client and server -c "Buffer record from epoch 1" \ -c "Found buffered record from current epoch - load" +# In this test, both the fragmented NewSessionTicket and the ChangeCipherSpec +# from the server are delayed, so that the encrypted Finished message +# is received and buffered. When the fragmented NewSessionTicket comes +# in afterwards, the encrypted Finished message must be freed in order +# to make space for the NewSessionTicket to be reassembled. +# This works only in very particular circumstances: +# - MBEDTLS_SSL_DTLS_MAX_BUFFERING must be large enough to allow buffering +# of the NewSessionTicket, but small enough to also allow buffering of +# the encrypted Finished message. +# - The MTU setting on the server must be so small that the NewSessionTicket +# needs to be fragmented. +# - All messages sent by the server must be small enough to be either sent +# without fragmentation or be reassembled within the bounds of +# MBEDTLS_SSL_DTLS_MAX_BUFFERING. Achieve this by testing with a PSK-based +# handshake, omitting CRTs. +requires_config_value_at_least "MBEDTLS_SSL_DTLS_MAX_BUFFERING" 240 +requires_config_value_at_most "MBEDTLS_SSL_DTLS_MAX_BUFFERING" 280 +run_test "DTLS reordering: Buffer encrypted Finished message, drop for fragmented NewSessionTicket" \ + -p "$P_PXY delay_srv=NewSessionTicket delay_srv=NewSessionTicket delay_ccs=1" \ + "$P_SRV mtu=190 dgram_packing=0 psk=abc123 psk_identity=foo cookies=0 dtls=1 debug_level=2" \ + "$P_CLI dgram_packing=0 dtls=1 debug_level=2 force_ciphersuite=TLS-PSK-WITH-AES-128-CCM-8 psk=abc123 psk_identity=foo" \ + 0 \ + -s "Buffer record from epoch 1" \ + -s "Found buffered record from current epoch - load" \ + -c "Buffer record from epoch 1" \ + -C "Found buffered record from current epoch - load" \ + -c "Enough space available after freeing future epoch record" + # Tests for "randomly unreliable connection": try a variety of flows and peers client_needs_more_time 2 From 2f5aa4c64eb4df3758245a4be7199856795248cb Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 24 Aug 2018 14:43:44 +0100 Subject: [PATCH 57/58] all.sh: Add builds allowing to test dropping buffered messages This commit adds two builds to all.sh which use a value of MBEDTLS_SSL_DTLS_MAX_BUFFERING that allows to run the reordering tests in ssl-opt.sh introduced in the last commit. --- tests/scripts/all.sh | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/tests/scripts/all.sh b/tests/scripts/all.sh index d7d5a8c1a1..0606caae3f 100755 --- a/tests/scripts/all.sh +++ b/tests/scripts/all.sh @@ -558,6 +558,26 @@ make msg "test: small SSL_IN_CONTENT_LEN - ssl-opt.sh MFL tests" if_build_succeeded tests/ssl-opt.sh -f "Max fragment" +msg "build: small MBEDTLS_SSL_DTLS_MAX_BUFFERING #0" +cleanup +cp "$CONFIG_H" "$CONFIG_BAK" +scripts/config.pl set MBEDTLS_SSL_DTLS_MAX_BUFFERING 1000 +CC=gcc cmake -D CMAKE_BUILD_TYPE:String=Asan . +make + +msg "test: small MBEDTLS_SSL_DTLS_MAX_BUFFERING #0 - ssl-opt.sh specific reordering test" +if_build_succeeded tests/ssl-opt.sh -f "DTLS reordering: Buffer out-of-order hs msg before reassembling next, free buffered msg" + +msg "build: small MBEDTLS_SSL_DTLS_MAX_BUFFERING #1" +cleanup +cp "$CONFIG_H" "$CONFIG_BAK" +scripts/config.pl set MBEDTLS_SSL_DTLS_MAX_BUFFERING 240 +CC=gcc cmake -D CMAKE_BUILD_TYPE:String=Asan . +make + +msg "test: small MBEDTLS_SSL_DTLS_MAX_BUFFERING #1 - ssl-opt.sh specific reordering test" +if_build_succeeded tests/ssl-opt.sh -f "DTLS reordering: Buffer encrypted Finished message, drop for fragmented NewSessionTicket" + msg "build: cmake, full config, clang" # ~ 50s cleanup cp "$CONFIG_H" "$CONFIG_BAK" From 159a37f75dc1db92f32fc86259cf8a0f0afc55f8 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 24 Aug 2018 15:07:29 +0100 Subject: [PATCH 58/58] config.h: Don't use arithmetical exp for SSL_DTLS_MAX_BUFFERING The functions requires_config_value_at_least and requires_config_value_at_most only work with numerical constants. --- include/mbedtls/config.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/mbedtls/config.h b/include/mbedtls/config.h index 70dd4be2b4..1cdff71f18 100644 --- a/include/mbedtls/config.h +++ b/include/mbedtls/config.h @@ -3016,7 +3016,7 @@ * DTLS handshake message reassembly and future message buffering. * */ -//#define MBEDTLS_SSL_DTLS_MAX_BUFFERING ( 2 * 16384 ) +//#define MBEDTLS_SSL_DTLS_MAX_BUFFERING 32768 //#define MBEDTLS_SSL_DEFAULT_TICKET_LIFETIME 86400 /**< Lifetime of session tickets (if enabled) */ //#define MBEDTLS_PSK_MAX_LEN 32 /**< Max size of TLS pre-shared keys, in bytes (default 256 bits) */