From 5aa4e2cedd819b4fd307531eaa64f4f5ca8a01d4 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Mon, 6 Aug 2018 09:26:08 +0100 Subject: [PATCH 01/44] Move deduction of internal record buffer pointers to function The SSL/TLS module maintains a number of internally used pointers `out_hdr`, `out_len`, `out_iv`, ..., indicating where to write the various parts of the record header. These pointers have to be kept in sync and sometimes need update: Most notably, the `out_msg` pointer should always point to the beginning of the record payload, and its offset from the pointer `out_iv` pointing to the end of the record header is determined by the length of the explicit IV used in the current record protection mechanism. This commit introduces functions deducing these pointers from the pointers `out_hdr` / `in_hdr` to the beginning of the header of the current outgoing / incoming record. The flexibility gained by these functions will subsequently be used to allow shifting of `out_hdr` for the purpose of packing multiple records into a single datagram. --- library/ssl_tls.c | 138 +++++++++++++++++++++++++++++----------------- 1 file changed, 88 insertions(+), 50 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 530f283b4..4607749ef 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -96,6 +96,10 @@ static int ssl_check_timer( mbedtls_ssl_context *ssl ) return( 0 ); } +static void ssl_update_out_pointers( mbedtls_ssl_context *ssl, + mbedtls_ssl_transform *transform ); +static void ssl_update_in_pointers( mbedtls_ssl_context *ssl, + mbedtls_ssl_transform *transform ); #if defined(MBEDTLS_SSL_PROTO_DTLS) /* * Double the retransmit timeout value, within the allowed range, @@ -2799,14 +2803,7 @@ static void ssl_swap_epochs( mbedtls_ssl_context *ssl ) memcpy( ssl->handshake->alt_out_ctr, tmp_out_ctr, 8 ); /* Adjust to the newly activated transform */ - if( ssl->transform_out != NULL && - ssl->minor_ver >= MBEDTLS_SSL_MINOR_VERSION_2 ) - { - ssl->out_msg = ssl->out_iv + ssl->transform_out->ivlen - - ssl->transform_out->fixed_ivlen; - } - else - ssl->out_msg = ssl->out_iv; + ssl_update_out_pointers( ssl, ssl->transform_out ); #if defined(MBEDTLS_SSL_HW_RECORD_ACCEL) if( mbedtls_ssl_hw_record_activate != NULL ) @@ -5171,16 +5168,7 @@ int mbedtls_ssl_parse_change_cipher_spec( mbedtls_ssl_context *ssl ) #endif /* MBEDTLS_SSL_PROTO_DTLS */ memset( ssl->in_ctr, 0, 8 ); - /* - * Set the in_msg pointer to the correct location based on IV length - */ - if( ssl->minor_ver >= MBEDTLS_SSL_MINOR_VERSION_2 ) - { - ssl->in_msg = ssl->in_iv + ssl->transform_negotiate->ivlen - - ssl->transform_negotiate->fixed_ivlen; - } - else - ssl->in_msg = ssl->in_iv; + ssl_update_in_pointers( ssl, ssl->transform_negotiate ); #if defined(MBEDTLS_SSL_HW_RECORD_ACCEL) if( mbedtls_ssl_hw_record_activate != NULL ) @@ -5631,16 +5619,7 @@ int mbedtls_ssl_write_finished( mbedtls_ssl_context *ssl ) MBEDTLS_SSL_DEBUG_MSG( 2, ( "=> write finished" ) ); - /* - * Set the out_msg pointer to the correct location based on IV length - */ - if( ssl->minor_ver >= MBEDTLS_SSL_MINOR_VERSION_2 ) - { - ssl->out_msg = ssl->out_iv + ssl->transform_negotiate->ivlen - - ssl->transform_negotiate->fixed_ivlen; - } - else - ssl->out_msg = ssl->out_iv; + ssl_update_out_pointers( ssl, ssl->transform_negotiate ); ssl->handshake->calc_finished( ssl, ssl->out_msg + 4, ssl->conf->endpoint ); @@ -5999,6 +5978,78 @@ static int ssl_cookie_check_dummy( void *ctx, } #endif /* MBEDTLS_SSL_DTLS_HELLO_VERIFY && MBEDTLS_SSL_SRV_C */ +/* Once ssl->out_hdr as the address of the beginning of the + * next outgoing record is set, deduce the other pointers. + * + * Note: For TLS, we save the implicit record sequence number + * (entering MAC computation) in the 8 bytes before ssl->out_hdr, + * and the caller has to make sure there's space for this. + */ + +static void ssl_update_out_pointers( mbedtls_ssl_context *ssl, + mbedtls_ssl_transform *transform ) +{ +#if defined(MBEDTLS_SSL_PROTO_DTLS) + if( ssl->conf->transport == MBEDTLS_SSL_TRANSPORT_DATAGRAM ) + { + ssl->out_ctr = ssl->out_hdr + 3; + ssl->out_len = ssl->out_hdr + 11; + ssl->out_iv = ssl->out_hdr + 13; + } + else +#endif + { + ssl->out_ctr = ssl->out_hdr - 8; + ssl->out_len = ssl->out_hdr + 3; + ssl->out_iv = ssl->out_hdr + 5; + } + + /* Adjust out_msg to make space for explicit IV, if used. */ + if( transform != NULL && + ssl->minor_ver >= MBEDTLS_SSL_MINOR_VERSION_2 ) + { + ssl->out_msg = ssl->out_iv + transform->ivlen - transform->fixed_ivlen; + } + else + ssl->out_msg = ssl->out_iv; +} + +/* Once ssl->in_hdr as the address of the beginning of the + * next incoming record is set, deduce the other pointers. + * + * Note: For TLS, we save the implicit record sequence number + * (entering MAC computation) in the 8 bytes before ssl->in_hdr, + * and the caller has to make sure there's space for this. + */ + +static void ssl_update_in_pointers( mbedtls_ssl_context *ssl, + mbedtls_ssl_transform *transform ) +{ +#if defined(MBEDTLS_SSL_PROTO_DTLS) + if( ssl->conf->transport == MBEDTLS_SSL_TRANSPORT_DATAGRAM ) + { + ssl->in_ctr = ssl->in_hdr + 3; + ssl->in_len = ssl->in_hdr + 11; + ssl->in_iv = ssl->in_hdr + 13; + } + else +#endif + { + ssl->in_ctr = ssl->in_hdr - 8; + ssl->in_len = ssl->in_hdr + 3; + ssl->in_iv = ssl->in_hdr + 5; + } + + /* Offset in_msg from in_iv to allow space for explicit IV, if used. */ + if( transform != NULL && + ssl->minor_ver >= MBEDTLS_SSL_MINOR_VERSION_2 ) + { + ssl->in_msg = ssl->in_iv + transform->ivlen - transform->fixed_ivlen; + } + else + ssl->in_msg = ssl->in_iv; +} + /* * Initialize an SSL context */ @@ -6036,37 +6087,24 @@ int mbedtls_ssl_setup( mbedtls_ssl_context *ssl, return( MBEDTLS_ERR_SSL_ALLOC_FAILED ); } + /* Set the incoming and outgoing record pointers. */ #if defined(MBEDTLS_SSL_PROTO_DTLS) if( conf->transport == MBEDTLS_SSL_TRANSPORT_DATAGRAM ) { ssl->out_hdr = ssl->out_buf; - ssl->out_ctr = ssl->out_buf + 3; - ssl->out_len = ssl->out_buf + 11; - ssl->out_iv = ssl->out_buf + 13; - ssl->out_msg = ssl->out_buf + 13; - - ssl->in_hdr = ssl->in_buf; - ssl->in_ctr = ssl->in_buf + 3; - ssl->in_len = ssl->in_buf + 11; - ssl->in_iv = ssl->in_buf + 13; - ssl->in_msg = ssl->in_buf + 13; + ssl->in_hdr = ssl->in_buf; } else -#endif +#endif /* MBEDTLS_SSL_PROTO_DTLS */ { - ssl->out_ctr = ssl->out_buf; - ssl->out_hdr = ssl->out_buf + 8; - ssl->out_len = ssl->out_buf + 11; - ssl->out_iv = ssl->out_buf + 13; - ssl->out_msg = ssl->out_buf + 13; - - ssl->in_ctr = ssl->in_buf; - ssl->in_hdr = ssl->in_buf + 8; - ssl->in_len = ssl->in_buf + 11; - ssl->in_iv = ssl->in_buf + 13; - ssl->in_msg = ssl->in_buf + 13; + ssl->out_hdr = ssl->out_buf + 8; + ssl->in_hdr = ssl->in_buf + 8; } + /* Derive other internal pointers. */ + ssl_update_out_pointers( ssl, NULL /* no transform enabled */ ); + ssl_update_in_pointers ( ssl, NULL /* no transform enabled */ ); + if( ( ret = ssl_handshake_init( ssl ) ) != 0 ) return( ret ); From 198594709baa82d55bba4e5ee442ffb5ffe886b4 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Mon, 6 Aug 2018 09:40:20 +0100 Subject: [PATCH 02/44] Store outgoing record sequence number outside record buffer This commit is another step towards supporting the packing of multiple records within a single datagram. Previously, the incremental outgoing record sequence number was statically stored within the record buffer, at its final place within the record header. This slightly increased efficiency as it was not necessary to copy the sequence number when writing outgoing records. When allowing multiple records within a single datagram, it is necessary to allow the position of the current record within the datagram buffer to be flexible; in particular, there is no static address for the record sequence number field within the record header. This commit introduces an additional field `cur_out_ctr` within the main SSL context structure `mbedtls_ssl_context` to keep track of the outgoing record sequence number independent of the buffer used for the current record / datagram. Whenever a new record is written, this sequence number is copied to the the address `out_ctr` of the sequence number header field within the current outgoing record. --- include/mbedtls/ssl.h | 2 ++ library/ssl_srv.c | 2 +- library/ssl_tls.c | 17 ++++++++++------- 3 files changed, 13 insertions(+), 8 deletions(-) diff --git a/include/mbedtls/ssl.h b/include/mbedtls/ssl.h index a3b514cd4..f27f6c02f 100644 --- a/include/mbedtls/ssl.h +++ b/include/mbedtls/ssl.h @@ -1116,6 +1116,8 @@ struct mbedtls_ssl_context size_t out_msglen; /*!< record header: message length */ size_t out_left; /*!< amount of data not yet written */ + unsigned char cur_out_ctr[8]; /*!< Outgoing record sequence number. */ + #if defined(MBEDTLS_ZLIB_SUPPORT) unsigned char *compress_buf; /*!< zlib data buffer */ #endif diff --git a/library/ssl_srv.c b/library/ssl_srv.c index eda50bb34..7101f461f 100644 --- a/library/ssl_srv.c +++ b/library/ssl_srv.c @@ -1294,7 +1294,7 @@ read_record_header: return( MBEDTLS_ERR_SSL_BAD_HS_CLIENT_HELLO ); } - memcpy( ssl->out_ctr + 2, ssl->in_ctr + 2, 6 ); + memcpy( ssl->cur_out_ctr + 2, ssl->in_ctr + 2, 6 ); #if defined(MBEDTLS_SSL_DTLS_ANTI_REPLAY) if( mbedtls_ssl_dtls_replay_check( ssl ) != 0 ) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 4607749ef..f2373eb51 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -2798,8 +2798,8 @@ static void ssl_swap_epochs( mbedtls_ssl_context *ssl ) ssl->handshake->alt_transform_out = tmp_transform; /* Swap epoch + sequence_number */ - memcpy( tmp_out_ctr, ssl->out_ctr, 8 ); - memcpy( ssl->out_ctr, ssl->handshake->alt_out_ctr, 8 ); + memcpy( tmp_out_ctr, ssl->cur_out_ctr, 8 ); + memcpy( ssl->cur_out_ctr, ssl->handshake->alt_out_ctr, 8 ); memcpy( ssl->handshake->alt_out_ctr, tmp_out_ctr, 8 ); /* Adjust to the newly activated transform */ @@ -3210,6 +3210,7 @@ int mbedtls_ssl_write_record( mbedtls_ssl_context *ssl ) mbedtls_ssl_write_version( ssl->major_ver, ssl->minor_ver, ssl->conf->transport, ssl->out_hdr + 1 ); + memcpy( ssl->out_ctr, ssl->cur_out_ctr, 8 ); ssl->out_len[0] = (unsigned char)( len >> 8 ); ssl->out_len[1] = (unsigned char)( len ); @@ -5671,14 +5672,14 @@ int mbedtls_ssl_write_finished( mbedtls_ssl_context *ssl ) /* Remember current epoch settings for resending */ ssl->handshake->alt_transform_out = ssl->transform_out; - memcpy( ssl->handshake->alt_out_ctr, ssl->out_ctr, 8 ); + memcpy( ssl->handshake->alt_out_ctr, ssl->cur_out_ctr, 8 ); /* Set sequence_number to zero */ - memset( ssl->out_ctr + 2, 0, 6 ); + memset( ssl->cur_out_ctr + 2, 0, 6 ); /* Increment epoch */ for( i = 2; i > 0; i-- ) - if( ++ssl->out_ctr[i - 1] != 0 ) + if( ++ssl->cur_out_ctr[i - 1] != 0 ) break; /* The loop goes to its end iff the counter is wrapping */ @@ -5690,7 +5691,7 @@ int mbedtls_ssl_write_finished( mbedtls_ssl_context *ssl ) } else #endif /* MBEDTLS_SSL_PROTO_DTLS */ - memset( ssl->out_ctr, 0, 8 ); + memset( ssl->cur_out_ctr, 0, 8 ); ssl->transform_out = ssl->transform_negotiate; ssl->session_out = ssl->session_negotiate; @@ -6166,6 +6167,8 @@ static int ssl_session_reset_int( mbedtls_ssl_context *ssl, int partial ) ssl->split_done = 0; #endif + memset( ssl->cur_out_ctr, 0, sizeof( ssl->cur_out_ctr ) ); + ssl->transform_in = NULL; ssl->transform_out = NULL; @@ -7381,7 +7384,7 @@ static int ssl_check_ctr_renegotiate( mbedtls_ssl_context *ssl ) in_ctr_cmp = memcmp( ssl->in_ctr + ep_len, ssl->conf->renego_period + ep_len, 8 - ep_len ); - out_ctr_cmp = memcmp( ssl->out_ctr + ep_len, + out_ctr_cmp = memcmp( ssl->cur_out_ctr + ep_len, ssl->conf->renego_period + ep_len, 8 - ep_len ); if( in_ctr_cmp <= 0 && out_ctr_cmp <= 0 ) From 04484621d0f6f6921f7d01bbef98eff6ceca0fb1 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Mon, 6 Aug 2018 09:49:38 +0100 Subject: [PATCH 03/44] Increment record sequence number in ssl_write_record() Previously, the record sequence number was incremented at the end of each successful call to mbedtls_ssl_flush_output(), which works as long as there is precisely one such call for each outgoing record. When packing multiple records into a single datagram, this property is no longer true, and instead the increment of the record sequence number must happen after the record has been prepared, and not after it has been dispatched. This commit moves the code for incrementing the record sequence number from mbedtls_ssl_flush_output() to ssl_write_record(). --- library/ssl_tls.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index f2373eb51..9342321af 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -2648,7 +2648,7 @@ int mbedtls_ssl_fetch_input( mbedtls_ssl_context *ssl, size_t nb_want ) int mbedtls_ssl_flush_output( mbedtls_ssl_context *ssl ) { int ret; - unsigned char *buf, i; + unsigned char *buf; MBEDTLS_SSL_DEBUG_MSG( 2, ( "=> flush output" ) ); @@ -2691,16 +2691,6 @@ int mbedtls_ssl_flush_output( mbedtls_ssl_context *ssl ) ssl->out_left -= ret; } - for( i = 8; i > ssl_ep_len( ssl ); i-- ) - if( ++ssl->out_ctr[i - 1] != 0 ) - break; - - /* The loop goes to its end iff the counter is wrapping */ - if( i == ssl_ep_len( ssl ) ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "outgoing message counter would wrap" ) ); - return( MBEDTLS_ERR_SSL_COUNTER_WRAPPING ); - } MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= flush output" ) ); @@ -3236,6 +3226,16 @@ int mbedtls_ssl_write_record( mbedtls_ssl_context *ssl ) MBEDTLS_SSL_DEBUG_BUF( 4, "output record sent to network", ssl->out_hdr, mbedtls_ssl_hdr_len( ssl ) + ssl->out_msglen ); + for( i = 8; i > ssl_ep_len( ssl ); i-- ) + if( ++ssl->cur_out_ctr[i - 1] != 0 ) + break; + + /* The loop goes to its end iff the counter is wrapping */ + if( i == ssl_ep_len( ssl ) ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "outgoing message counter would wrap" ) ); + return( MBEDTLS_ERR_SSL_COUNTER_WRAPPING ); + } } if( ( ret = mbedtls_ssl_flush_output( ssl ) ) != 0 ) From 3b235902b86694728b54df430f247e4c145d30dd Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Mon, 6 Aug 2018 09:54:53 +0100 Subject: [PATCH 04/44] Log calls to ssl_flight_append() in debugging output --- library/ssl_tls.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 9342321af..edb233bbd 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -2707,6 +2707,9 @@ int mbedtls_ssl_flush_output( mbedtls_ssl_context *ssl ) static int ssl_flight_append( mbedtls_ssl_context *ssl ) { mbedtls_ssl_flight_item *msg; + MBEDTLS_SSL_DEBUG_MSG( 2, ( "=> ssl_flight_append" ) ); + MBEDTLS_SSL_DEBUG_BUF( 4, "message appended to flight", + ssl->out_msg, ssl->out_msglen ); /* Allocate space for current message */ if( ( msg = mbedtls_calloc( 1, sizeof( mbedtls_ssl_flight_item ) ) ) == NULL ) @@ -2740,6 +2743,7 @@ static int ssl_flight_append( mbedtls_ssl_context *ssl ) cur->next = msg; } + MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= ssl_flight_append" ) ); return( 0 ); } From 2b1e3547548acad8ce742eaef2df24c8d206684e Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Mon, 6 Aug 2018 11:19:13 +0100 Subject: [PATCH 05/44] Increase record buffer pointer after preparing a record The packing of multiple records within a single datagram works by increasing the pointer `out_hdr` (pointing to the beginning of the next outgoing record) within the datagram buffer, as long as space is available and no flush was mandatory. This commit does not yet change the code's behavior of always flushing after preparing a record, but it introduces the logic of increasing `out_hdr` after preparing the record, and resetting it after the flush has been completed. --- library/ssl_tls.c | 60 +++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 53 insertions(+), 7 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index edb233bbd..ad071a976 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -101,6 +101,17 @@ static void ssl_update_out_pointers( mbedtls_ssl_context *ssl, static void ssl_update_in_pointers( mbedtls_ssl_context *ssl, mbedtls_ssl_transform *transform ); #if defined(MBEDTLS_SSL_PROTO_DTLS) + +static uint16_t ssl_get_maximum_datagram_size( mbedtls_ssl_context const *ssl ) +{ + uint16_t mtu = ssl->conf->mtu; + + if( mtu != 0 && mtu < MBEDTLS_SSL_OUT_BUFFER_LEN ) + return( (int) mtu ); + + return( MBEDTLS_SSL_OUT_BUFFER_LEN ); +} + /* * Double the retransmit timeout value, within the allowed range, * returning -1 if the maximum value has already been reached. @@ -2671,8 +2682,7 @@ int mbedtls_ssl_flush_output( mbedtls_ssl_context *ssl ) MBEDTLS_SSL_DEBUG_MSG( 2, ( "message length: %d, out_left: %d", mbedtls_ssl_hdr_len( ssl ) + ssl->out_msglen, ssl->out_left ) ); - buf = ssl->out_hdr + mbedtls_ssl_hdr_len( ssl ) + - ssl->out_msglen - ssl->out_left; + buf = ssl->out_hdr - ssl->out_left; ret = ssl->f_send( ssl->p_bio, buf, ssl->out_left ); MBEDTLS_SSL_DEBUG_RET( 2, "ssl->f_send", ret ); @@ -2691,6 +2701,17 @@ int mbedtls_ssl_flush_output( mbedtls_ssl_context *ssl ) ssl->out_left -= ret; } +#if defined(MBEDTLS_SSL_PROTO_DTLS) + if( ssl->conf->transport == MBEDTLS_SSL_TRANSPORT_DATAGRAM ) + { + ssl->out_hdr = ssl->out_buf; + } + else +#endif + { + ssl->out_hdr = ssl->out_buf + 8; + } + ssl_update_out_pointers( ssl, ssl->transform_out ); MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= flush output" ) ); @@ -3200,6 +3221,9 @@ int mbedtls_ssl_write_record( mbedtls_ssl_context *ssl ) #endif /* MBEDTLS_SSL_HW_RECORD_ACCEL */ if( !done ) { + unsigned i; + size_t protected_record_size; + ssl->out_hdr[0] = (unsigned char) ssl->out_msgtype; mbedtls_ssl_write_version( ssl->major_ver, ssl->minor_ver, ssl->conf->transport, ssl->out_hdr + 1 ); @@ -3221,15 +3245,37 @@ int mbedtls_ssl_write_record( mbedtls_ssl_context *ssl ) ssl->out_len[1] = (unsigned char)( len ); } - ssl->out_left = mbedtls_ssl_hdr_len( ssl ) + ssl->out_msglen; + protected_record_size = len + mbedtls_ssl_hdr_len( ssl ); + +#if defined(MBEDTLS_SSL_PROTO_DTLS) + /* In case of DTLS, double-check that we don't exceed + * the remaining space in the datagram. */ + if( ssl->conf->transport == MBEDTLS_SSL_TRANSPORT_DATAGRAM ) + { + ret = ssl_get_maximum_datagram_size( ssl ); + if( ret < 0 ) + return( ret ); + + if( protected_record_size > (size_t) ret ) + { + /* Should never happen */ + return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); + } + } +#endif /* MBEDTLS_SSL_PROTO_DTLS */ MBEDTLS_SSL_DEBUG_MSG( 3, ( "output record: msgtype = %d, " - "version = [%d:%d], msglen = %d", - ssl->out_hdr[0], ssl->out_hdr[1], ssl->out_hdr[2], - ( ssl->out_len[0] << 8 ) | ssl->out_len[1] ) ); + "version = [%d:%d], msglen = %d", + ssl->out_hdr[0], ssl->out_hdr[1], ssl->out_hdr[2], len ) ); + MBEDTLS_SSL_DEBUG_BUF( 4, "output record sent to network", - ssl->out_hdr, mbedtls_ssl_hdr_len( ssl ) + ssl->out_msglen ); + ssl->out_hdr, protected_record_size ); + + ssl->out_left += protected_record_size; + ssl->out_hdr += protected_record_size; + ssl_update_out_pointers( ssl, ssl->transform_out ); + for( i = 8; i > ssl_ep_len( ssl ); i-- ) if( ++ssl->cur_out_ctr[i - 1] != 0 ) break; From 67bc7c3a384aae3d42de45cc2fb79a83a252c770 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Mon, 6 Aug 2018 11:33:50 +0100 Subject: [PATCH 06/44] Don't immediately flush datagram after preparing a record This commit finally enables datagram packing by modifying the record preparation function ssl_write_record() to not always calling mbedtls_ssl_flush_output(). --- include/mbedtls/ssl_internal.h | 2 +- library/ssl_tls.c | 160 ++++++++++++++++++++++++++------- 2 files changed, 128 insertions(+), 34 deletions(-) diff --git a/include/mbedtls/ssl_internal.h b/include/mbedtls/ssl_internal.h index 18982f89a..765da7a71 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -561,7 +561,7 @@ int mbedtls_ssl_read_record( mbedtls_ssl_context *ssl ); int mbedtls_ssl_fetch_input( mbedtls_ssl_context *ssl, size_t nb_want ); int mbedtls_ssl_write_handshake_msg( mbedtls_ssl_context *ssl ); -int mbedtls_ssl_write_record( mbedtls_ssl_context *ssl ); +int mbedtls_ssl_write_record( mbedtls_ssl_context *ssl, uint8_t force_flush ); int mbedtls_ssl_flush_output( mbedtls_ssl_context *ssl ); int mbedtls_ssl_parse_certificate( mbedtls_ssl_context *ssl ); diff --git a/library/ssl_tls.c b/library/ssl_tls.c index ad071a976..878495b17 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -100,6 +100,10 @@ static void ssl_update_out_pointers( mbedtls_ssl_context *ssl, mbedtls_ssl_transform *transform ); static void ssl_update_in_pointers( mbedtls_ssl_context *ssl, mbedtls_ssl_transform *transform ); + +#define SSL_DONT_FORCE_FLUSH 0 +#define SSL_FORCE_FLUSH 1 + #if defined(MBEDTLS_SSL_PROTO_DTLS) static uint16_t ssl_get_maximum_datagram_size( mbedtls_ssl_context const *ssl ) @@ -112,6 +116,55 @@ static uint16_t ssl_get_maximum_datagram_size( mbedtls_ssl_context const *ssl ) return( MBEDTLS_SSL_OUT_BUFFER_LEN ); } +static int ssl_get_remaining_space_in_datagram( mbedtls_ssl_context const *ssl ) +{ + size_t const bytes_written = ssl->out_left; + uint16_t const mtu = ssl_get_maximum_datagram_size( ssl ); + + /* Double-check that the write-index hasn't gone + * past what we can transmit in a single datagram. */ + if( bytes_written > (size_t) mtu ) + { + /* Should never happen... */ + return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); + } + + return( (int) ( mtu - bytes_written ) ); +} + +static int ssl_get_remaining_payload_in_datagram( mbedtls_ssl_context const *ssl ) +{ + int ret; + size_t remaining, expansion; + size_t max_len = MBEDTLS_SSL_MAX_CONTENT_LEN; + +#if defined(MBEDTLS_SSL_MAX_FRAGMENT_LENGTH) + const size_t mfl = mbedtls_ssl_get_max_frag_len( ssl ); + + if( max_len > mfl ) + max_len = mfl; +#endif + + ret = ssl_get_remaining_space_in_datagram( ssl ); + if( ret < 0 ) + return( ret ); + remaining = (size_t) ret; + + ret = mbedtls_ssl_get_record_expansion( ssl ); + if( ret < 0 ) + return( ret ); + expansion = (size_t) ret; + + if( remaining <= expansion ) + return( 0 ); + + remaining -= expansion; + if( remaining >= max_len ) + remaining = max_len; + + return( (int) remaining ); +} + /* * Double the retransmit timeout value, within the allowed range, * returning -1 if the maximum value has already been reached. @@ -2857,20 +2910,9 @@ int mbedtls_ssl_resend( mbedtls_ssl_context *ssl ) */ int mbedtls_ssl_flight_transmit( mbedtls_ssl_context *ssl ) { - const int ret_payload = mbedtls_ssl_get_max_out_record_payload( ssl ); - const size_t max_record_payload = (size_t) ret_payload; - /* DTLS handshake headers are 12 bytes */ - const size_t max_hs_fragment_len = max_record_payload - 12; - + int ret; MBEDTLS_SSL_DEBUG_MSG( 2, ( "=> mbedtls_ssl_flight_transmit" ) ); - if( ret_payload < 0 ) - { - MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_ssl_get_max_out_record_payload", - ret_payload ); - return( ret_payload ); - } - if( ssl->handshake->retransmit_state != MBEDTLS_SSL_RETRANS_SENDING ) { MBEDTLS_SSL_DEBUG_MSG( 2, ( "initialise flight transmission" ) ); @@ -2884,22 +2926,38 @@ int mbedtls_ssl_flight_transmit( mbedtls_ssl_context *ssl ) while( ssl->handshake->cur_msg != NULL ) { - int ret; + size_t max_frag_len; const mbedtls_ssl_flight_item * const cur = ssl->handshake->cur_msg; + /* Swap epochs before sending Finished: we can't do it after * sending ChangeCipherSpec, in case write returns WANT_READ. * Must be done before copying, may change out_msg pointer */ if( cur->type == MBEDTLS_SSL_MSG_HANDSHAKE && - cur->p[0] == MBEDTLS_SSL_HS_FINISHED ) + cur->p[0] == MBEDTLS_SSL_HS_FINISHED && + ssl->handshake->cur_msg_p == ( cur->p + 12 ) ) { + MBEDTLS_SSL_DEBUG_MSG( 2, ( "swap epochs to send finished message" ) ); ssl_swap_epochs( ssl ); } + ret = ssl_get_remaining_payload_in_datagram( ssl ); + if( ret < 0 ) + return( ret ); + max_frag_len = (size_t) ret; + /* CCS is copied as is, while HS messages may need fragmentation */ if( cur->type == MBEDTLS_SSL_MSG_CHANGE_CIPHER_SPEC ) { + if( max_frag_len == 0 ) + { + if( ( ret = mbedtls_ssl_flush_output( ssl ) ) != 0 ) + return( ret ); + + continue; + } + memcpy( ssl->out_msg, cur->p, cur->len ); - ssl->out_msglen = cur->len; + ssl->out_msglen = cur->len; ssl->out_msgtype = cur->type; /* Update position inside current message */ @@ -2911,14 +2969,31 @@ int mbedtls_ssl_flight_transmit( mbedtls_ssl_context *ssl ) const size_t hs_len = cur->len - 12; const size_t frag_off = p - ( cur->p + 12 ); const size_t rem_len = hs_len - frag_off; - const size_t frag_len = rem_len > max_hs_fragment_len - ? max_hs_fragment_len : rem_len; + size_t cur_hs_frag_len, max_hs_frag_len; - if( frag_off == 0 && frag_len != hs_len ) + if( max_frag_len < 12 ) + { + if( cur->type == MBEDTLS_SSL_MSG_HANDSHAKE && + cur->p[0] == MBEDTLS_SSL_HS_FINISHED ) + { + ssl_swap_epochs( ssl ); + } + + if( ( ret = mbedtls_ssl_flush_output( ssl ) ) != 0 ) + return( ret ); + + continue; + } + max_hs_frag_len = max_frag_len - 12; + + cur_hs_frag_len = rem_len > max_hs_frag_len ? + max_hs_frag_len : rem_len; + + if( frag_off == 0 && cur_hs_frag_len != hs_len ) { MBEDTLS_SSL_DEBUG_MSG( 2, ( "fragmenting handshake message (%u > %u)", - (unsigned) hs_len, - (unsigned) max_hs_fragment_len ) ); + (unsigned) cur_hs_frag_len, + (unsigned) max_hs_frag_len ) ); } /* Messages are stored with handshake headers as if not fragmented, @@ -2930,19 +3005,19 @@ int mbedtls_ssl_flight_transmit( mbedtls_ssl_context *ssl ) ssl->out_msg[7] = ( ( frag_off >> 8 ) & 0xff ); ssl->out_msg[8] = ( ( frag_off ) & 0xff ); - ssl->out_msg[ 9] = ( ( frag_len >> 16 ) & 0xff ); - ssl->out_msg[10] = ( ( frag_len >> 8 ) & 0xff ); - ssl->out_msg[11] = ( ( frag_len ) & 0xff ); + ssl->out_msg[ 9] = ( ( cur_hs_frag_len >> 16 ) & 0xff ); + ssl->out_msg[10] = ( ( cur_hs_frag_len >> 8 ) & 0xff ); + ssl->out_msg[11] = ( ( cur_hs_frag_len ) & 0xff ); MBEDTLS_SSL_DEBUG_BUF( 3, "handshake header", ssl->out_msg, 12 ); - /* Copy the handshake message content and set records fields */ - memcpy( ssl->out_msg + 12, p, frag_len ); - ssl->out_msglen = frag_len + 12; + /* Copy the handshame message content and set records fields */ + memcpy( ssl->out_msg + 12, p, cur_hs_frag_len ); + ssl->out_msglen = cur_hs_frag_len + 12; ssl->out_msgtype = cur->type; /* Update position inside current message */ - ssl->handshake->cur_msg_p += frag_len; + ssl->handshake->cur_msg_p += cur_hs_frag_len; } /* If done with the current message move to the next one if any */ @@ -2961,13 +3036,17 @@ int mbedtls_ssl_flight_transmit( mbedtls_ssl_context *ssl ) } /* Actually send the message out */ - if( ( ret = mbedtls_ssl_write_record( ssl ) ) != 0 ) + if( ( ret = mbedtls_ssl_write_record( ssl, + SSL_DONT_FORCE_FLUSH ) ) != 0 ) { MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_ssl_write_record", ret ); return( ret ); } } + if( ( ret = mbedtls_ssl_flush_output( ssl ) ) != 0 ) + return( ret ); + /* Update state and set timer */ if( ssl->state == MBEDTLS_SSL_HANDSHAKE_OVER ) ssl->handshake->retransmit_state = MBEDTLS_SSL_RETRANS_FINISHED; @@ -3158,7 +3237,7 @@ int mbedtls_ssl_write_handshake_msg( mbedtls_ssl_context *ssl ) else #endif { - if( ( ret = mbedtls_ssl_write_record( ssl ) ) != 0 ) + if( ( ret = mbedtls_ssl_write_record( ssl, SSL_FORCE_FLUSH ) ) != 0 ) { MBEDTLS_SSL_DEBUG_RET( 1, "ssl_write_record", ret ); return( ret ); @@ -3182,10 +3261,11 @@ int mbedtls_ssl_write_handshake_msg( mbedtls_ssl_context *ssl ) * - ssl->out_msglen: length of the record content (excl headers) * - ssl->out_msg: record content */ -int mbedtls_ssl_write_record( mbedtls_ssl_context *ssl ) +int mbedtls_ssl_write_record( mbedtls_ssl_context *ssl, uint8_t force_flush ) { int ret, done = 0; size_t len = ssl->out_msglen; + uint8_t flush = force_flush; MBEDTLS_SSL_DEBUG_MSG( 2, ( "=> write record" ) ); @@ -3288,7 +3368,21 @@ int mbedtls_ssl_write_record( mbedtls_ssl_context *ssl ) } } - if( ( ret = mbedtls_ssl_flush_output( ssl ) ) != 0 ) +#if defined(MBEDTLS_SSL_PROTO_DTLS) + if( ssl->conf->transport == MBEDTLS_SSL_TRANSPORT_DATAGRAM ) + { + size_t remaining = ssl_get_remaining_payload_in_datagram( ssl ); + if( remaining == 0 ) + flush = SSL_FORCE_FLUSH; + else + { + MBEDTLS_SSL_DEBUG_MSG( 2, ( "Stil %u bytes available in current datagram", (unsigned) remaining ) ); + } + } +#endif /* MBEDTLS_SSL_PROTO_DTLS */ + + if( ( flush == SSL_FORCE_FLUSH ) && + ( ret = mbedtls_ssl_flush_output( ssl ) ) != 0 ) { MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_ssl_flush_output", ret ); return( ret ); @@ -4570,7 +4664,7 @@ int mbedtls_ssl_send_alert_message( mbedtls_ssl_context *ssl, ssl->out_msg[0] = level; ssl->out_msg[1] = message; - if( ( ret = mbedtls_ssl_write_record( ssl ) ) != 0 ) + if( ( ret = mbedtls_ssl_write_record( ssl, SSL_FORCE_FLUSH ) ) != 0 ) { MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_ssl_write_record", ret ); return( ret ); @@ -7815,7 +7909,7 @@ static int ssl_write_real( mbedtls_ssl_context *ssl, ssl->out_msgtype = MBEDTLS_SSL_MSG_APPLICATION_DATA; memcpy( ssl->out_msg, buf, len ); - if( ( ret = mbedtls_ssl_write_record( ssl ) ) != 0 ) + if( ( ret = mbedtls_ssl_write_record( ssl, SSL_FORCE_FLUSH ) ) != 0 ) { MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_ssl_write_record", ret ); return( ret ); From b50a253a879f91c6cf6db83e09f5fc3138b6e404 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Mon, 6 Aug 2018 11:52:54 +0100 Subject: [PATCH 07/44] Move size check for records --- library/ssl_tls.c | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 878495b17..d1e699ce4 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1413,14 +1413,6 @@ static int ssl_encrypt_buf( mbedtls_ssl_context *ssl ) MBEDTLS_SSL_DEBUG_BUF( 4, "before encrypt: output payload", ssl->out_msg, ssl->out_msglen ); - if( ssl->out_msglen > MBEDTLS_SSL_OUT_CONTENT_LEN ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "Record content %u too large, maximum %d", - (unsigned) ssl->out_msglen, - MBEDTLS_SSL_OUT_CONTENT_LEN ) ); - return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); - } - /* * Add MAC before if needed */ @@ -3166,6 +3158,23 @@ int mbedtls_ssl_write_handshake_msg( mbedtls_ssl_context *ssl ) } #endif + /* Double-check that we did not exceed the bounds + * of the outgoing record buffer. + * This should never fail as the various message + * writing functions must obey the bounds of the + * outgoing record buffer, but better be safe. + * + * Note: We deliberately do not check for the MTU or MFL here. + */ + if( ssl->out_msglen > MBEDTLS_SSL_OUT_CONTENT_LEN ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "Record too large: " + "size %u, maximum %u", + (unsigned) ssl->out_msglen, + (unsigned) MBEDTLS_SSL_OUT_CONTENT_LEN ) ); + return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); + } + /* * Fill handshake headers */ From 111fa497aa29cd537b823681a9267683d28e30fa Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Mon, 6 Aug 2018 12:26:33 +0100 Subject: [PATCH 08/44] TEST-ONLY: Remove delayed CCS test The test exercising a delayed CCS message is not expected to work when datagram packing is used, as the current UDP proxy is not able to recognize records which are not at the beginning of a datagram. --- tests/ssl-opt.sh | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index c27cc25c8..54794415a 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -5701,16 +5701,6 @@ run_test "DTLS proxy: inject invalid AD record, badmac_limit 2, exchanges 2"\ -s "too many records with bad MAC" \ -s "Verification of the message MAC failed" -run_test "DTLS proxy: delay ChangeCipherSpec" \ - -p "$P_PXY delay_ccs=1" \ - "$P_SRV dtls=1 debug_level=1" \ - "$P_CLI dtls=1 debug_level=1" \ - 0 \ - -c "record from another epoch" \ - -s "record from another epoch" \ - -s "Extra-header:" \ - -c "HTTP/1.0 200 OK" - # Tests for "randomly unreliable connection": try a variety of flows and peers client_needs_more_time 2 From 2a43f6f539309637fd3a41c0835a109b6ec95797 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 10 Aug 2018 11:12:52 +0100 Subject: [PATCH 09/44] Introduce function to reset in/out pointers --- library/ssl_tls.c | 42 +++++++++++++++++++++++++----------------- 1 file changed, 25 insertions(+), 17 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index d1e699ce4..4e3c190d6 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -54,6 +54,8 @@ #include "mbedtls/oid.h" #endif +static void ssl_reset_in_out_pointers( 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 ) { @@ -6215,6 +6217,28 @@ void mbedtls_ssl_init( mbedtls_ssl_context *ssl ) /* * Setup an SSL context */ + +static void ssl_reset_in_out_pointers( mbedtls_ssl_context *ssl ) +{ + /* Set the incoming and outgoing record pointers. */ +#if defined(MBEDTLS_SSL_PROTO_DTLS) + if( ssl->conf->transport == MBEDTLS_SSL_TRANSPORT_DATAGRAM ) + { + ssl->out_hdr = ssl->out_buf; + ssl->in_hdr = ssl->in_buf; + } + else +#endif /* MBEDTLS_SSL_PROTO_DTLS */ + { + ssl->out_hdr = ssl->out_buf + 8; + ssl->in_hdr = ssl->in_buf + 8; + } + + /* Derive other internal pointers. */ + ssl_update_out_pointers( ssl, NULL /* no transform enabled */ ); + ssl_update_in_pointers ( ssl, NULL /* no transform enabled */ ); +} + int mbedtls_ssl_setup( mbedtls_ssl_context *ssl, const mbedtls_ssl_config *conf ) { @@ -6241,23 +6265,7 @@ int mbedtls_ssl_setup( mbedtls_ssl_context *ssl, return( MBEDTLS_ERR_SSL_ALLOC_FAILED ); } - /* Set the incoming and outgoing record pointers. */ -#if defined(MBEDTLS_SSL_PROTO_DTLS) - if( conf->transport == MBEDTLS_SSL_TRANSPORT_DATAGRAM ) - { - ssl->out_hdr = ssl->out_buf; - ssl->in_hdr = ssl->in_buf; - } - else -#endif /* MBEDTLS_SSL_PROTO_DTLS */ - { - ssl->out_hdr = ssl->out_buf + 8; - ssl->in_hdr = ssl->in_buf + 8; - } - - /* Derive other internal pointers. */ - ssl_update_out_pointers( ssl, NULL /* no transform enabled */ ); - ssl_update_in_pointers ( ssl, NULL /* no transform enabled */ ); + ssl_reset_in_out_pointers( ssl ); if( ( ret = ssl_handshake_init( ssl ) ) != 0 ) return( ret ); From 4ccbf064ed77ef0008ed026d31f69b1d253cface Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 10 Aug 2018 11:20:38 +0100 Subject: [PATCH 10/44] Minor improvements in ssl_session_reset_int() --- 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 4e3c190d6..f2bb74838 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -6304,8 +6304,6 @@ static int ssl_session_reset_int( mbedtls_ssl_context *ssl, int partial ) ssl->in_msg = ssl->in_buf + 13; ssl->in_msgtype = 0; ssl->in_msglen = 0; - if( partial == 0 ) - ssl->in_left = 0; #if defined(MBEDTLS_SSL_PROTO_DTLS) ssl->next_record_offset = 0; ssl->in_epoch = 0; @@ -6337,8 +6335,14 @@ static int ssl_session_reset_int( mbedtls_ssl_context *ssl, int partial ) ssl->session_out = NULL; memset( ssl->out_buf, 0, MBEDTLS_SSL_OUT_BUFFER_LEN ); + +#if defined(MBEDTLS_SSL_DTLS_CLIENT_PORT_REUSE) && defined(MBEDTLS_SSL_SRV_C) if( partial == 0 ) +#endif /* MBEDTLS_SSL_DTLS_CLIENT_PORT_REUSE && MBEDTLS_SSL_SRV_C */ + { + ssl->in_left = 0; memset( ssl->in_buf, 0, MBEDTLS_SSL_IN_BUFFER_LEN ); + } #if defined(MBEDTLS_SSL_HW_RECORD_ACCEL) if( mbedtls_ssl_hw_record_reset != NULL ) @@ -6371,7 +6375,9 @@ static int ssl_session_reset_int( mbedtls_ssl_context *ssl, int partial ) #endif #if defined(MBEDTLS_SSL_DTLS_HELLO_VERIFY) && defined(MBEDTLS_SSL_SRV_C) +#if defined(MBEDTLS_SSL_DTLS_CLIENT_PORT_REUSE) if( partial == 0 ) +#endif { mbedtls_free( ssl->cli_id ); ssl->cli_id = NULL; From f29d4702f703e4a3bb0aa2276e7bd6ec7b24defa Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 10 Aug 2018 11:31:15 +0100 Subject: [PATCH 11/44] Reset in/out pointers on SSL session reset If a previous session was interrupted during flushing, the out pointers might point arbitrarily into the output buffer. --- library/ssl_tls.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index f2bb74838..df21cbd2b 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -6300,8 +6300,8 @@ static int ssl_session_reset_int( mbedtls_ssl_context *ssl, int partial ) ssl->secure_renegotiation = MBEDTLS_SSL_LEGACY_RENEGOTIATION; ssl->in_offt = NULL; + ssl_reset_in_out_pointers( ssl ); - ssl->in_msg = ssl->in_buf + 13; ssl->in_msgtype = 0; ssl->in_msglen = 0; #if defined(MBEDTLS_SSL_PROTO_DTLS) @@ -6317,7 +6317,6 @@ static int ssl_session_reset_int( mbedtls_ssl_context *ssl, int partial ) ssl->keep_current_message = 0; - ssl->out_msg = ssl->out_buf + 13; ssl->out_msgtype = 0; ssl->out_msglen = 0; ssl->out_left = 0; From 0defedb48823b931f04e8b626356f80b5b2de7c5 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 10 Aug 2018 12:35:02 +0100 Subject: [PATCH 12/44] Fix unused variable warning in mbedtls_ssl_get_max_record_payload If neither the maximum fragment length extension nor DTLS are used, the SSL context argument is unnecessary as the maximum payload length is hardcoded as MBEDTLS_SSL_MAX_CONTENT_LEN. --- library/ssl_tls.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index df21cbd2b..a57761ecb 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -7328,6 +7328,11 @@ int mbedtls_ssl_get_max_out_record_payload( const mbedtls_ssl_context *ssl ) } #endif +#if !defined(MBEDTLS_SSL_MAX_FRAGMENT_LENGTH) && \ + !defined(MBEDTLS_SSL_PROTO_DTLS) + ((void) ssl); +#endif + return( (int) max_len ); } From 7e7721350bba4d26e374a70b7771cd3c89186701 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 10 Aug 2018 12:38:21 +0100 Subject: [PATCH 13/44] Fix unused variable warning in ssl_session_reset_int() The `partial` argument is only used when DTLS and same port client reconnect are enabled. This commit marks the variable as unused if that's not the case. --- library/ssl_tls.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index a57761ecb..98e508ec6 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -6284,6 +6284,11 @@ static int ssl_session_reset_int( mbedtls_ssl_context *ssl, int partial ) { int ret; +#if !defined(MBEDTLS_SSL_DTLS_CLIENT_PORT_REUSE) || \ + !defined(MBEDTLS_SSL_SRV_C) + ((void) partial); +#endif + ssl->state = MBEDTLS_SSL_HELLO_REQUEST; /* Cancel any possibly running timer */ From 12405e76b5b59ef871a95e02703ee36d9ef71a25 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Mon, 13 Aug 2018 16:45:46 +0100 Subject: [PATCH 14/44] Increase max_frag_len / MTU in fragmentation ref tests The tests "DTLS fragmenting: none (for reference)" and "DTLS fragmenting: none (for reference) (MTU)" used a maximum fragment length resp. MTU value of 2048 which was meant to be large enough so that fragmentation of the certificate message would not be necessary. However, it is not large enough to hold the entire flight to which the certificate belongs, and hence there will be fragmentation as soon as datagram packing is used. This commit increases the maximum fragment length resp. MTU values to 4096 bytes to ensure that even with datagram packing in place, no fragmentation is necessary. A similar change was made in "DTLS fragmenting: client (MTU)". --- tests/ssl-opt.sh | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index 54794415a..1986c25b3 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -4960,11 +4960,11 @@ run_test "DTLS fragmenting: none (for reference)" \ "$P_SRV dtls=1 debug_level=2 auth_mode=required \ crt_file=data_files/server7_int-ca.crt \ key_file=data_files/server7.key \ - max_frag_len=2048" \ + max_frag_len=4096" \ "$P_CLI dtls=1 debug_level=2 \ crt_file=data_files/server8_int-ca2.crt \ key_file=data_files/server8.key \ - max_frag_len=2048" \ + max_frag_len=4096" \ 0 \ -S "found fragmented DTLS handshake message" \ -C "found fragmented DTLS handshake message" \ @@ -5049,11 +5049,11 @@ run_test "DTLS fragmenting: none (for reference) (MTU)" \ "$P_SRV dtls=1 debug_level=2 auth_mode=required \ crt_file=data_files/server7_int-ca.crt \ key_file=data_files/server7.key \ - mtu=2048" \ + mtu=4096" \ "$P_CLI dtls=1 debug_level=2 \ crt_file=data_files/server8_int-ca2.crt \ key_file=data_files/server8.key \ - mtu=2048" \ + mtu=4096" \ 0 \ -S "found fragmented DTLS handshake message" \ -C "found fragmented DTLS handshake message" \ @@ -5066,7 +5066,7 @@ run_test "DTLS fragmenting: client (MTU)" \ "$P_SRV dtls=1 debug_level=2 auth_mode=required \ crt_file=data_files/server7_int-ca.crt \ key_file=data_files/server7.key \ - mtu=2048" \ + mtu=4096" \ "$P_CLI dtls=1 debug_level=2 \ crt_file=data_files/server8_int-ca2.crt \ key_file=data_files/server8.key \ From 04da1892256999a9549775820758a187fcb19070 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 14 Aug 2018 13:22:10 +0100 Subject: [PATCH 15/44] Make datagram packing dynamically configurable This commit adds a public function `mbedtls_ssl_conf_datagram_packing()` that allows to allow / forbid the packing of multiple records within a single datagram. --- include/mbedtls/ssl.h | 37 +++++++++++++++++++++++++++++++++++++ library/ssl_tls.c | 16 +++++++++++++--- 2 files changed, 50 insertions(+), 3 deletions(-) diff --git a/include/mbedtls/ssl.h b/include/mbedtls/ssl.h index f27f6c02f..85ab72206 100644 --- a/include/mbedtls/ssl.h +++ b/include/mbedtls/ssl.h @@ -1102,6 +1102,11 @@ struct mbedtls_ssl_context int keep_current_message; /*!< drop or reuse current message on next call to record layer? */ +#if defined(MBEDTLS_SSL_PROTO_DTLS) + uint8_t disable_datagram_packing; /*!< Disable packing multiple records + * within a single datagram. */ +#endif /* MBEDTLS_SSL_PROTO_DTLS */ + /* * Record layer (outgoing data) */ @@ -1763,6 +1768,38 @@ void mbedtls_ssl_conf_dtls_badmac_limit( mbedtls_ssl_config *conf, unsigned limi #endif /* MBEDTLS_SSL_DTLS_BADMAC_LIMIT */ #if defined(MBEDTLS_SSL_PROTO_DTLS) + +/** + * \brief Allow or disallow packing of multiple handshake records + * within a single datagram. + * + * \param ssl The SSL context to configure. + * \param allow_packing This determines whether datagram packing may + * be used or not. A value of \c 0 means that every + * record will be sent in a separate datagram; a + * value of \c 1 means that, if space permits, + * multiple handshake messages (including CCS) belonging to + * a single flight may be packed within a single datagram. + * + * \note This is enabled by default and should only be disabled + * for test purposes, or if datagram packing causes + * interoperability issues with peers that don't support it. + * + * \note Allowing datagram packing reduces the network load since + * there's less overhead if multiple messages share the same + * datagram. Also, it increases the handshake efficiency + * since messages belonging to a single datagram will not + * be reordered in transit, and so future message buffering + * or flight retransmission (if no buffering is used) as + * means to deal with reordering are needed less frequently. + * + * \note Application datagrams are not affected by this option and + * are currently always sent in separate datagrams. + * + */ +void mbedtls_ssl_conf_datagram_packing( mbedtls_ssl_context *ssl, + unsigned allow_packing ); + /** * \brief Set retransmit timeout values for the DTLS handshake. * (DTLS only, no effect on TLS.) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 98e508ec6..9b8f7fea3 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -2923,6 +2923,9 @@ int mbedtls_ssl_flight_transmit( mbedtls_ssl_context *ssl ) size_t max_frag_len; const mbedtls_ssl_flight_item * const cur = ssl->handshake->cur_msg; + uint8_t const force_flush = ssl->disable_datagram_packing == 1 ? + SSL_FORCE_FLUSH : SSL_DONT_FORCE_FLUSH; + /* Swap epochs before sending Finished: we can't do it after * sending ChangeCipherSpec, in case write returns WANT_READ. * Must be done before copying, may change out_msg pointer */ @@ -3030,8 +3033,7 @@ int mbedtls_ssl_flight_transmit( mbedtls_ssl_context *ssl ) } /* Actually send the message out */ - if( ( ret = mbedtls_ssl_write_record( ssl, - SSL_DONT_FORCE_FLUSH ) ) != 0 ) + if( ( ret = mbedtls_ssl_write_record( ssl, force_flush ) ) != 0 ) { MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_ssl_write_record", ret ); return( ret ); @@ -6432,7 +6434,15 @@ void mbedtls_ssl_conf_dtls_badmac_limit( mbedtls_ssl_config *conf, unsigned limi #endif #if defined(MBEDTLS_SSL_PROTO_DTLS) -void mbedtls_ssl_conf_handshake_timeout( mbedtls_ssl_config *conf, uint32_t min, uint32_t max ) + +void mbedtls_ssl_conf_datagram_packing( mbedtls_ssl_context *ssl, + unsigned allow_packing ) +{ + ssl->disable_datagram_packing = !allow_packing; +} + +void mbedtls_ssl_conf_handshake_timeout( mbedtls_ssl_config *conf, + uint32_t min, uint32_t max ) { conf->hs_timeout_min = min; conf->hs_timeout_max = max; From e7675d0d3df9f89b784ea0b3c9d552e12062776f Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 14 Aug 2018 13:28:56 +0100 Subject: [PATCH 16/44] Add cmd line option to ssl_server2 for datagram packing This commit adds a new command line option `dgram_packing` to the example server application programs/ssl/ssl_server2 allowing to allow/forbid the use of datagram packing. --- programs/ssl/ssl_server2.c | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/programs/ssl/ssl_server2.c b/programs/ssl/ssl_server2.c index 484f84fdd..12f827611 100644 --- a/programs/ssl/ssl_server2.c +++ b/programs/ssl/ssl_server2.c @@ -152,6 +152,7 @@ int main( void ) #define DFL_HS_TO_MAX 0 #define DFL_DTLS_MTU -1 #define DFL_BADMAC_LIMIT -1 +#define DFL_DGRAM_PACKING 1 #define DFL_EXTENDED_MS -1 #define DFL_ETM -1 @@ -299,7 +300,10 @@ int main( void ) " dtls=%%d default: 0 (TLS)\n" \ " hs_timeout=%%d-%%d default: (library default: 1000-60000)\n" \ " range of DTLS handshake timeouts in millisecs\n" \ - " mtu=%%d default: (library default: unlimited)\n" + " mtu=%%d default: (library default: unlimited)\n" \ + " dgram_packing=%%d default: 1 (allowed)\n" \ + " allow or forbid packing of multiple\n" \ + " records within a single datgram.\n" #else #define USAGE_DTLS "" #endif @@ -473,6 +477,7 @@ struct options uint32_t hs_to_min; /* Initial value of DTLS handshake timer */ uint32_t hs_to_max; /* Max value of DTLS handshake timer */ int dtls_mtu; /* UDP Maximum tranport unit for DTLS */ + int dgram_packing; /* allow/forbid datagram packing */ int badmac_limit; /* Limit of records with bad MAC */ } opt; @@ -1342,6 +1347,7 @@ int main( int argc, char *argv[] ) opt.hs_to_min = DFL_HS_TO_MIN; opt.hs_to_max = DFL_HS_TO_MAX; opt.dtls_mtu = DFL_DTLS_MTU; + opt.dgram_packing = DFL_DGRAM_PACKING; opt.badmac_limit = DFL_BADMAC_LIMIT; opt.extended_ms = DFL_EXTENDED_MS; opt.etm = DFL_ETM; @@ -1694,6 +1700,15 @@ int main( int argc, char *argv[] ) if( opt.dtls_mtu < 0 ) goto usage; } + else if( strcmp( p, "dgram_packing" ) == 0 ) + { + opt.dgram_packing = atoi( q ); + if( opt.dgram_packing != 0 && + opt.dgram_packing != 1 ) + { + goto usage; + } + } else if( strcmp( p, "sni" ) == 0 ) { opt.sni = q; @@ -2168,6 +2183,9 @@ int main( int argc, char *argv[] ) if( opt.dtls_mtu != DFL_DTLS_MTU ) mbedtls_ssl_conf_mtu( &conf, opt.dtls_mtu ); + + if( opt.dgram_packing != DFL_DGRAM_PACKING ) + mbedtls_ssl_conf_datagram_packing( &ssl, opt.dgram_packing ); #endif /* MBEDTLS_SSL_PROTO_DTLS */ #if defined(MBEDTLS_SSL_MAX_FRAGMENT_LENGTH) @@ -2178,6 +2196,7 @@ int main( int argc, char *argv[] ) }; #endif + #if defined(MBEDTLS_SSL_TRUNCATED_HMAC) if( opt.trunc_hmac != DFL_TRUNC_HMAC ) mbedtls_ssl_conf_truncated_hmac( &conf, opt.trunc_hmac ); From 4d61591c0c5380921d2815ebe410b4d106acf75b Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 14 Aug 2018 13:33:30 +0100 Subject: [PATCH 17/44] Add cmd line option to ssl_client2 for datagram packing This commit adds a new command line option `dgram_packing` to the example server application programs/ssl/ssl_client2 allowing to allow/forbid the use of datagram packing. --- programs/ssl/ssl_client2.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/programs/ssl/ssl_client2.c b/programs/ssl/ssl_client2.c index 7cdc53a54..e72327315 100644 --- a/programs/ssl/ssl_client2.c +++ b/programs/ssl/ssl_client2.c @@ -107,6 +107,7 @@ int main( void ) #define DFL_HS_TO_MIN 0 #define DFL_HS_TO_MAX 0 #define DFL_DTLS_MTU -1 +#define DFL_DGRAM_PACKING 1 #define DFL_FALLBACK -1 #define DFL_EXTENDED_MS -1 #define DFL_ETM -1 @@ -200,7 +201,10 @@ int main( void ) " dtls=%%d default: 0 (TLS)\n" \ " hs_timeout=%%d-%%d default: (library default: 1000-60000)\n" \ " range of DTLS handshake timeouts in millisecs\n" \ - " mtu=%%d default: (library default: unlimited)\n" + " mtu=%%d default: (library default: unlimited)\n" \ + " dgram_packing=%%d default: 1 (allowed)\n" \ + " allow or forbid packing of multiple\n" \ + " records within a single datgram.\n" #else #define USAGE_DTLS "" #endif @@ -349,6 +353,7 @@ struct options uint32_t hs_to_max; /* Max value of DTLS handshake timer */ int dtls_mtu; /* UDP Maximum tranport unit for DTLS */ int fallback; /* is this a fallback connection? */ + int dgram_packing; /* allow/forbid datagram packing */ int extended_ms; /* negotiate extended master secret? */ int etm; /* negotiate encrypt then mac? */ } opt; @@ -624,6 +629,7 @@ int main( int argc, char *argv[] ) opt.fallback = DFL_FALLBACK; opt.extended_ms = DFL_EXTENDED_MS; opt.etm = DFL_ETM; + opt.dgram_packing = DFL_DGRAM_PACKING; for( i = 1; i < argc; i++ ) { @@ -937,6 +943,15 @@ int main( int argc, char *argv[] ) if( opt.dtls_mtu < 0 ) goto usage; } + else if( strcmp( p, "dgram_packing" ) == 0 ) + { + opt.dgram_packing = atoi( q ); + if( opt.dgram_packing != 0 && + opt.dgram_packing != 1 ) + { + goto usage; + } + } else if( strcmp( p, "recsplit" ) == 0 ) { opt.recsplit = atoi( q ); @@ -1340,6 +1355,9 @@ int main( int argc, char *argv[] ) if( opt.dtls_mtu != DFL_DTLS_MTU ) mbedtls_ssl_conf_mtu( &conf, opt.dtls_mtu ); + + if( opt.dgram_packing != DFL_DGRAM_PACKING ) + mbedtls_ssl_conf_datagram_packing( &ssl, opt.dgram_packing ); #endif /* MBEDTLS_SSL_PROTO_DTLS */ #if defined(MBEDTLS_SSL_MAX_FRAGMENT_LENGTH) From c4305238b5df651d04222c1ffcab2e1784635bdb Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 14 Aug 2018 13:41:21 +0100 Subject: [PATCH 18/44] Re-enable delayed CCS test Now that datagram packing can be dynamically configured, the test exercising the behavior of Mbed TLS when facing an out-of-order CCS message can be re-introduced, disabling datagram packing for the sender of the delayed CCS. --- tests/ssl-opt.sh | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index 1986c25b3..abb8d8f1a 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -5701,6 +5701,16 @@ run_test "DTLS proxy: inject invalid AD record, badmac_limit 2, exchanges 2"\ -s "too many records with bad MAC" \ -s "Verification of the message MAC failed" +run_test "DTLS proxy: delay ChangeCipherSpec" \ + -p "$P_PXY delay_ccs=1" \ + "$P_SRV dtls=1 debug_level=1 dgram_packing=0" \ + "$P_CLI dtls=1 debug_level=1 dgram_packing=0" \ + 0 \ + -c "record from another epoch" \ + -s "record from another epoch" \ + -s "Extra-header:" \ + -c "HTTP/1.0 200 OK" + # Tests for "randomly unreliable connection": try a variety of flows and peers client_needs_more_time 2 From 1c9a24ce8c2c647c6b7e4cef1109efd883c4ec4d Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 14 Aug 2018 13:46:33 +0100 Subject: [PATCH 19/44] Disable datagram packing for various UDP proxy tests The UDP proxy does currently not dissect datagrams into records, an hence the coverage of the reordering, package loss and duplication tests is much smaller if datagram packing is in use. This commit disables datagram packing for most UDP proxy tests, in particular all 3D (drop, duplicate, delay) tests. --- tests/ssl-opt.sh | 108 +++++++++++++++++++++++------------------------ 1 file changed, 54 insertions(+), 54 deletions(-) diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index abb8d8f1a..9b8ef5561 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -5310,11 +5310,11 @@ requires_config_enabled MBEDTLS_ECDSA_C client_needs_more_time 2 run_test "DTLS fragmenting: proxy MTU + 3d" \ -p "$P_PXY mtu=512 drop=8 delay=8 duplicate=8" \ - "$P_SRV dtls=1 debug_level=2 auth_mode=required \ + "$P_SRV dgram_packing=0 dtls=1 debug_level=2 auth_mode=required \ crt_file=data_files/server7_int-ca.crt \ key_file=data_files/server7.key \ mtu=512" \ - "$P_CLI dtls=1 debug_level=2 \ + "$P_CLI dgram_packing=0 dtls=1 debug_level=2 \ crt_file=data_files/server8_int-ca2.crt \ key_file=data_files/server8.key \ mtu=512" \ @@ -5452,7 +5452,7 @@ client_needs_more_time 2 run_test "DTLS fragmenting: 3d, gnutls server, DTLS 1.2" \ -p "$P_PXY drop=8 delay=8 duplicate=8" \ "$G_NEXT_SRV -u" \ - "$P_CLI dtls=1 debug_level=2 \ + "$P_CLI dgram_packing=0 dtls=1 debug_level=2 \ crt_file=data_files/server8_int-ca2.crt \ key_file=data_files/server8.key \ mtu=512 force_version=dtls1_2" \ @@ -5469,7 +5469,7 @@ client_needs_more_time 2 run_test "DTLS fragmenting: 3d, gnutls server, DTLS 1.0" \ -p "$P_PXY drop=8 delay=8 duplicate=8" \ "$G_NEXT_SRV -u" \ - "$P_CLI dtls=1 debug_level=2 \ + "$P_CLI dgram_packing=0 dtls=1 debug_level=2 \ crt_file=data_files/server8_int-ca2.crt \ key_file=data_files/server8.key \ mtu=512 force_version=dtls1_2" \ @@ -5550,7 +5550,7 @@ client_needs_more_time 2 run_test "DTLS fragmenting: 3d, openssl server, DTLS 1.0" \ -p "$P_PXY drop=8 delay=8 duplicate=8" \ "$O_LEGACY_SRV -dtls1 -verify 10" \ - "$P_CLI dtls=1 debug_level=2 \ + "$P_CLI dgram_packing=0 dtls=1 debug_level=2 \ crt_file=data_files/server8_int-ca2.crt \ key_file=data_files/server8.key \ mtu=512 force_version=dtls1" \ @@ -5583,7 +5583,7 @@ requires_config_enabled MBEDTLS_SSL_PROTO_TLS1_1 client_needs_more_time 2 run_test "DTLS fragmenting: 3d, openssl client, DTLS 1.0" \ -p "$P_PXY drop=8 delay=8 duplicate=8" \ - "$P_SRV dtls=1 debug_level=2 \ + "$P_SRV dgram_packing=0 dtls=1 debug_level=2 \ crt_file=data_files/server7_int-ca.crt \ key_file=data_files/server7.key \ mtu=512 force_version=dtls1" \ @@ -5612,8 +5612,8 @@ run_test "DTLS proxy: reference" \ not_with_valgrind # spurious resend due to timeout run_test "DTLS proxy: duplicate every packet" \ -p "$P_PXY duplicate=1" \ - "$P_SRV dtls=1 debug_level=2" \ - "$P_CLI dtls=1 debug_level=2" \ + "$P_SRV dtls=1 dgram_packing=0 debug_level=2" \ + "$P_CLI dtls=1 dgram_packing=0 debug_level=2" \ 0 \ -c "replayed record" \ -s "replayed record" \ @@ -5625,8 +5625,8 @@ run_test "DTLS proxy: duplicate every packet" \ run_test "DTLS proxy: duplicate every packet, server anti-replay off" \ -p "$P_PXY duplicate=1" \ - "$P_SRV dtls=1 debug_level=2 anti_replay=0" \ - "$P_CLI dtls=1 debug_level=2" \ + "$P_SRV dtls=1 dgram_packing=0 debug_level=2 anti_replay=0" \ + "$P_CLI dtls=1 dgram_packing=0 debug_level=2" \ 0 \ -c "replayed record" \ -S "replayed record" \ @@ -5639,24 +5639,24 @@ run_test "DTLS proxy: duplicate every packet, server anti-replay off" \ run_test "DTLS proxy: multiple records in same datagram" \ -p "$P_PXY pack=50" \ - "$P_SRV dtls=1 debug_level=2" \ - "$P_CLI dtls=1 debug_level=2" \ + "$P_SRV dtls=1 dgram_packing=0 debug_level=2" \ + "$P_CLI dtls=1 dgram_packing=0 debug_level=2" \ 0 \ -c "next record in same datagram" \ -s "next record in same datagram" run_test "DTLS proxy: multiple records in same datagram, duplicate every packet" \ -p "$P_PXY pack=50 duplicate=1" \ - "$P_SRV dtls=1 debug_level=2" \ - "$P_CLI dtls=1 debug_level=2" \ + "$P_SRV dtls=1 dgram_packing=0 debug_level=2" \ + "$P_CLI dtls=1 dgram_packing=0 debug_level=2" \ 0 \ -c "next record in same datagram" \ -s "next record in same datagram" run_test "DTLS proxy: inject invalid AD record, default badmac_limit" \ -p "$P_PXY bad_ad=1" \ - "$P_SRV dtls=1 debug_level=1" \ - "$P_CLI dtls=1 debug_level=1 read_timeout=100" \ + "$P_SRV dtls=1 dgram_packing=0 debug_level=1" \ + "$P_CLI dtls=1 dgram_packing=0 debug_level=1 read_timeout=100" \ 0 \ -c "discarding invalid record (mac)" \ -s "discarding invalid record (mac)" \ @@ -5667,8 +5667,8 @@ run_test "DTLS proxy: inject invalid AD record, default badmac_limit" \ run_test "DTLS proxy: inject invalid AD record, badmac_limit 1" \ -p "$P_PXY bad_ad=1" \ - "$P_SRV dtls=1 debug_level=1 badmac_limit=1" \ - "$P_CLI dtls=1 debug_level=1 read_timeout=100" \ + "$P_SRV dtls=1 dgram_packing=0 debug_level=1 badmac_limit=1" \ + "$P_CLI dtls=1 dgram_packing=0 debug_level=1 read_timeout=100" \ 1 \ -C "discarding invalid record (mac)" \ -S "discarding invalid record (mac)" \ @@ -5679,8 +5679,8 @@ run_test "DTLS proxy: inject invalid AD record, badmac_limit 1" \ run_test "DTLS proxy: inject invalid AD record, badmac_limit 2" \ -p "$P_PXY bad_ad=1" \ - "$P_SRV dtls=1 debug_level=1 badmac_limit=2" \ - "$P_CLI dtls=1 debug_level=1 read_timeout=100" \ + "$P_SRV dtls=1 dgram_packing=0 debug_level=1 badmac_limit=2" \ + "$P_CLI dtls=1 dgram_packing=0 debug_level=1 read_timeout=100" \ 0 \ -c "discarding invalid record (mac)" \ -s "discarding invalid record (mac)" \ @@ -5691,8 +5691,8 @@ run_test "DTLS proxy: inject invalid AD record, badmac_limit 2" \ run_test "DTLS proxy: inject invalid AD record, badmac_limit 2, exchanges 2"\ -p "$P_PXY bad_ad=1" \ - "$P_SRV dtls=1 debug_level=1 badmac_limit=2 exchanges=2" \ - "$P_CLI dtls=1 debug_level=1 read_timeout=100 exchanges=2" \ + "$P_SRV dtls=1 dgram_packing=0 debug_level=1 badmac_limit=2 exchanges=2" \ + "$P_CLI dtls=1 dgram_packing=0 debug_level=1 read_timeout=100 exchanges=2" \ 1 \ -c "discarding invalid record (mac)" \ -s "discarding invalid record (mac)" \ @@ -5716,9 +5716,9 @@ run_test "DTLS proxy: delay ChangeCipherSpec" \ client_needs_more_time 2 run_test "DTLS proxy: 3d (drop, delay, duplicate), \"short\" PSK handshake" \ -p "$P_PXY drop=5 delay=5 duplicate=5" \ - "$P_SRV dtls=1 hs_timeout=250-10000 tickets=0 auth_mode=none \ + "$P_SRV dtls=1 dgram_packing=0 hs_timeout=250-10000 tickets=0 auth_mode=none \ psk=abc123" \ - "$P_CLI dtls=1 hs_timeout=250-10000 tickets=0 psk=abc123 \ + "$P_CLI dtls=1 dgram_packing=0 hs_timeout=250-10000 tickets=0 psk=abc123 \ force_ciphersuite=TLS-PSK-WITH-AES-128-CCM-8" \ 0 \ -s "Extra-header:" \ @@ -5727,8 +5727,8 @@ run_test "DTLS proxy: 3d (drop, delay, duplicate), \"short\" PSK handshake" \ client_needs_more_time 2 run_test "DTLS proxy: 3d, \"short\" RSA handshake" \ -p "$P_PXY drop=5 delay=5 duplicate=5" \ - "$P_SRV dtls=1 hs_timeout=250-10000 tickets=0 auth_mode=none" \ - "$P_CLI dtls=1 hs_timeout=250-10000 tickets=0 \ + "$P_SRV dtls=1 dgram_packing=0 hs_timeout=250-10000 tickets=0 auth_mode=none" \ + "$P_CLI dtls=1 dgram_packing=0 hs_timeout=250-10000 tickets=0 \ force_ciphersuite=TLS-RSA-WITH-AES-128-CBC-SHA" \ 0 \ -s "Extra-header:" \ @@ -5737,8 +5737,8 @@ run_test "DTLS proxy: 3d, \"short\" RSA handshake" \ client_needs_more_time 2 run_test "DTLS proxy: 3d, \"short\" (no ticket, no cli_auth) FS handshake" \ -p "$P_PXY drop=5 delay=5 duplicate=5" \ - "$P_SRV dtls=1 hs_timeout=250-10000 tickets=0 auth_mode=none" \ - "$P_CLI dtls=1 hs_timeout=250-10000 tickets=0" \ + "$P_SRV dtls=1 dgram_packing=0 hs_timeout=250-10000 tickets=0 auth_mode=none" \ + "$P_CLI dtls=1 dgram_packing=0 hs_timeout=250-10000 tickets=0" \ 0 \ -s "Extra-header:" \ -c "HTTP/1.0 200 OK" @@ -5746,8 +5746,8 @@ run_test "DTLS proxy: 3d, \"short\" (no ticket, no cli_auth) FS handshake" \ client_needs_more_time 2 run_test "DTLS proxy: 3d, FS, client auth" \ -p "$P_PXY drop=5 delay=5 duplicate=5" \ - "$P_SRV dtls=1 hs_timeout=250-10000 tickets=0 auth_mode=required" \ - "$P_CLI dtls=1 hs_timeout=250-10000 tickets=0" \ + "$P_SRV dtls=1 dgram_packing=0 hs_timeout=250-10000 tickets=0 auth_mode=required" \ + "$P_CLI dtls=1 dgram_packing=0 hs_timeout=250-10000 tickets=0" \ 0 \ -s "Extra-header:" \ -c "HTTP/1.0 200 OK" @@ -5755,8 +5755,8 @@ run_test "DTLS proxy: 3d, FS, client auth" \ client_needs_more_time 2 run_test "DTLS proxy: 3d, FS, ticket" \ -p "$P_PXY drop=5 delay=5 duplicate=5" \ - "$P_SRV dtls=1 hs_timeout=250-10000 tickets=1 auth_mode=none" \ - "$P_CLI dtls=1 hs_timeout=250-10000 tickets=1" \ + "$P_SRV dtls=1 dgram_packing=0 hs_timeout=250-10000 tickets=1 auth_mode=none" \ + "$P_CLI dtls=1 dgram_packing=0 hs_timeout=250-10000 tickets=1" \ 0 \ -s "Extra-header:" \ -c "HTTP/1.0 200 OK" @@ -5764,8 +5764,8 @@ run_test "DTLS proxy: 3d, FS, ticket" \ client_needs_more_time 2 run_test "DTLS proxy: 3d, max handshake (FS, ticket + client auth)" \ -p "$P_PXY drop=5 delay=5 duplicate=5" \ - "$P_SRV dtls=1 hs_timeout=250-10000 tickets=1 auth_mode=required" \ - "$P_CLI dtls=1 hs_timeout=250-10000 tickets=1" \ + "$P_SRV dtls=1 dgram_packing=0 hs_timeout=250-10000 tickets=1 auth_mode=required" \ + "$P_CLI dtls=1 dgram_packing=0 hs_timeout=250-10000 tickets=1" \ 0 \ -s "Extra-header:" \ -c "HTTP/1.0 200 OK" @@ -5773,9 +5773,9 @@ run_test "DTLS proxy: 3d, max handshake (FS, ticket + client auth)" \ client_needs_more_time 2 run_test "DTLS proxy: 3d, max handshake, nbio" \ -p "$P_PXY drop=5 delay=5 duplicate=5" \ - "$P_SRV dtls=1 hs_timeout=250-10000 nbio=2 tickets=1 \ + "$P_SRV dtls=1 dgram_packing=0 hs_timeout=250-10000 nbio=2 tickets=1 \ auth_mode=required" \ - "$P_CLI dtls=1 hs_timeout=250-10000 nbio=2 tickets=1" \ + "$P_CLI dtls=1 dgram_packing=0 hs_timeout=250-10000 nbio=2 tickets=1" \ 0 \ -s "Extra-header:" \ -c "HTTP/1.0 200 OK" @@ -5783,9 +5783,9 @@ run_test "DTLS proxy: 3d, max handshake, nbio" \ client_needs_more_time 4 run_test "DTLS proxy: 3d, min handshake, resumption" \ -p "$P_PXY drop=5 delay=5 duplicate=5" \ - "$P_SRV dtls=1 hs_timeout=250-10000 tickets=0 auth_mode=none \ + "$P_SRV dtls=1 dgram_packing=0 hs_timeout=250-10000 tickets=0 auth_mode=none \ psk=abc123 debug_level=3" \ - "$P_CLI dtls=1 hs_timeout=250-10000 tickets=0 psk=abc123 \ + "$P_CLI dtls=1 dgram_packing=0 hs_timeout=250-10000 tickets=0 psk=abc123 \ debug_level=3 reconnect=1 read_timeout=1000 max_resend=10 \ force_ciphersuite=TLS-PSK-WITH-AES-128-CCM-8" \ 0 \ @@ -5797,9 +5797,9 @@ run_test "DTLS proxy: 3d, min handshake, resumption" \ client_needs_more_time 4 run_test "DTLS proxy: 3d, min handshake, resumption, nbio" \ -p "$P_PXY drop=5 delay=5 duplicate=5" \ - "$P_SRV dtls=1 hs_timeout=250-10000 tickets=0 auth_mode=none \ + "$P_SRV dtls=1 dgram_packing=0 hs_timeout=250-10000 tickets=0 auth_mode=none \ psk=abc123 debug_level=3 nbio=2" \ - "$P_CLI dtls=1 hs_timeout=250-10000 tickets=0 psk=abc123 \ + "$P_CLI dtls=1 dgram_packing=0 hs_timeout=250-10000 tickets=0 psk=abc123 \ debug_level=3 reconnect=1 read_timeout=1000 max_resend=10 \ force_ciphersuite=TLS-PSK-WITH-AES-128-CCM-8 nbio=2" \ 0 \ @@ -5812,9 +5812,9 @@ client_needs_more_time 4 requires_config_enabled MBEDTLS_SSL_RENEGOTIATION run_test "DTLS proxy: 3d, min handshake, client-initiated renego" \ -p "$P_PXY drop=5 delay=5 duplicate=5" \ - "$P_SRV dtls=1 hs_timeout=250-10000 tickets=0 auth_mode=none \ + "$P_SRV dtls=1 dgram_packing=0 hs_timeout=250-10000 tickets=0 auth_mode=none \ psk=abc123 renegotiation=1 debug_level=2" \ - "$P_CLI dtls=1 hs_timeout=250-10000 tickets=0 psk=abc123 \ + "$P_CLI dtls=1 dgram_packing=0 hs_timeout=250-10000 tickets=0 psk=abc123 \ renegotiate=1 debug_level=2 \ force_ciphersuite=TLS-PSK-WITH-AES-128-CCM-8" \ 0 \ @@ -5827,9 +5827,9 @@ client_needs_more_time 4 requires_config_enabled MBEDTLS_SSL_RENEGOTIATION run_test "DTLS proxy: 3d, min handshake, client-initiated renego, nbio" \ -p "$P_PXY drop=5 delay=5 duplicate=5" \ - "$P_SRV dtls=1 hs_timeout=250-10000 tickets=0 auth_mode=none \ + "$P_SRV dtls=1 dgram_packing=0 hs_timeout=250-10000 tickets=0 auth_mode=none \ psk=abc123 renegotiation=1 debug_level=2" \ - "$P_CLI dtls=1 hs_timeout=250-10000 tickets=0 psk=abc123 \ + "$P_CLI dtls=1 dgram_packing=0 hs_timeout=250-10000 tickets=0 psk=abc123 \ renegotiate=1 debug_level=2 \ force_ciphersuite=TLS-PSK-WITH-AES-128-CCM-8" \ 0 \ @@ -5842,10 +5842,10 @@ client_needs_more_time 4 requires_config_enabled MBEDTLS_SSL_RENEGOTIATION run_test "DTLS proxy: 3d, min handshake, server-initiated renego" \ -p "$P_PXY drop=5 delay=5 duplicate=5" \ - "$P_SRV dtls=1 hs_timeout=250-10000 tickets=0 auth_mode=none \ + "$P_SRV dtls=1 dgram_packing=0 hs_timeout=250-10000 tickets=0 auth_mode=none \ psk=abc123 renegotiate=1 renegotiation=1 exchanges=4 \ debug_level=2" \ - "$P_CLI dtls=1 hs_timeout=250-10000 tickets=0 psk=abc123 \ + "$P_CLI dtls=1 dgram_packing=0 hs_timeout=250-10000 tickets=0 psk=abc123 \ renegotiation=1 exchanges=4 debug_level=2 \ force_ciphersuite=TLS-PSK-WITH-AES-128-CCM-8" \ 0 \ @@ -5858,10 +5858,10 @@ client_needs_more_time 4 requires_config_enabled MBEDTLS_SSL_RENEGOTIATION run_test "DTLS proxy: 3d, min handshake, server-initiated renego, nbio" \ -p "$P_PXY drop=5 delay=5 duplicate=5" \ - "$P_SRV dtls=1 hs_timeout=250-10000 tickets=0 auth_mode=none \ + "$P_SRV dtls=1 dgram_packing=0 hs_timeout=250-10000 tickets=0 auth_mode=none \ psk=abc123 renegotiate=1 renegotiation=1 exchanges=4 \ debug_level=2 nbio=2" \ - "$P_CLI dtls=1 hs_timeout=250-10000 tickets=0 psk=abc123 \ + "$P_CLI dtls=1 dgram_packing=0 hs_timeout=250-10000 tickets=0 psk=abc123 \ renegotiation=1 exchanges=4 debug_level=2 nbio=2 \ force_ciphersuite=TLS-PSK-WITH-AES-128-CCM-8" \ 0 \ @@ -5875,7 +5875,7 @@ not_with_valgrind # risk of non-mbedtls peer timing out run_test "DTLS proxy: 3d, openssl server" \ -p "$P_PXY drop=5 delay=5 duplicate=5 protect_hvr=1" \ "$O_SRV -dtls1 -mtu 2048" \ - "$P_CLI dtls=1 hs_timeout=250-60000 tickets=0" \ + "$P_CLI dgram_packing=0 dtls=1 hs_timeout=250-60000 tickets=0" \ 0 \ -c "HTTP/1.0 200 OK" @@ -5884,7 +5884,7 @@ not_with_valgrind # risk of non-mbedtls peer timing out run_test "DTLS proxy: 3d, openssl server, fragmentation" \ -p "$P_PXY drop=5 delay=5 duplicate=5 protect_hvr=1" \ "$O_SRV -dtls1 -mtu 768" \ - "$P_CLI dtls=1 hs_timeout=250-60000 tickets=0" \ + "$P_CLI dgram_packing=0 dtls=1 hs_timeout=250-60000 tickets=0" \ 0 \ -c "HTTP/1.0 200 OK" @@ -5893,7 +5893,7 @@ not_with_valgrind # risk of non-mbedtls peer timing out run_test "DTLS proxy: 3d, openssl server, fragmentation, nbio" \ -p "$P_PXY drop=5 delay=5 duplicate=5 protect_hvr=1" \ "$O_SRV -dtls1 -mtu 768" \ - "$P_CLI dtls=1 hs_timeout=250-60000 nbio=2 tickets=0" \ + "$P_CLI dgram_packing=0 dtls=1 hs_timeout=250-60000 nbio=2 tickets=0" \ 0 \ -c "HTTP/1.0 200 OK" @@ -5903,7 +5903,7 @@ not_with_valgrind # risk of non-mbedtls peer timing out run_test "DTLS proxy: 3d, gnutls server" \ -p "$P_PXY drop=5 delay=5 duplicate=5" \ "$G_SRV -u --mtu 2048 -a" \ - "$P_CLI dtls=1 hs_timeout=250-60000" \ + "$P_CLI dgram_packing=0 dtls=1 hs_timeout=250-60000" \ 0 \ -s "Extra-header:" \ -c "Extra-header:" @@ -5914,7 +5914,7 @@ not_with_valgrind # risk of non-mbedtls peer timing out run_test "DTLS proxy: 3d, gnutls server, fragmentation" \ -p "$P_PXY drop=5 delay=5 duplicate=5" \ "$G_SRV -u --mtu 512" \ - "$P_CLI dtls=1 hs_timeout=250-60000" \ + "$P_CLI dgram_packing=0 dtls=1 hs_timeout=250-60000" \ 0 \ -s "Extra-header:" \ -c "Extra-header:" @@ -5925,7 +5925,7 @@ not_with_valgrind # risk of non-mbedtls peer timing out run_test "DTLS proxy: 3d, gnutls server, fragmentation, nbio" \ -p "$P_PXY drop=5 delay=5 duplicate=5" \ "$G_SRV -u --mtu 512" \ - "$P_CLI dtls=1 hs_timeout=250-60000 nbio=2" \ + "$P_CLI dgram_packing=0 dtls=1 hs_timeout=250-60000 nbio=2" \ 0 \ -s "Extra-header:" \ -c "Extra-header:" From 7ae8a76ced295aa9721ebaaa1f05498756863e02 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 14 Aug 2018 15:43:35 +0100 Subject: [PATCH 20/44] Add tests for datagram packing option to ssl-opt.sh This commit adds four tests to ssl-opt.sh running default DTLS client and server with and without datagram packing enabled, and checking that datagram packing is / is not used by inspecting the debug output. --- tests/ssl-opt.sh | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index 9b8ef5561..995478019 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -912,6 +912,35 @@ run_test "SHA-256 allowed by default in client certificate" \ "$P_CLI key_file=data_files/cli-rsa.key crt_file=data_files/cli-rsa-sha256.crt" \ 0 +# Tests for datagram packing +run_test "DTLS: multiple records in same datagram, client and server" \ + "$P_SRV dtls=1 dgram_packing=1 debug_level=2" \ + "$P_CLI dtls=1 dgram_packing=1 debug_level=2" \ + 0 \ + -c "next record in same datagram" \ + -s "next record in same datagram" + +run_test "DTLS: multiple records in same datagram, client only" \ + "$P_SRV dtls=1 dgram_packing=0 debug_level=2" \ + "$P_CLI dtls=1 dgram_packing=1 debug_level=2" \ + 0 \ + -s "next record in same datagram" \ + -C "next record in same datagram" + +run_test "DTLS: multiple records in same datagram, server only" \ + "$P_SRV dtls=1 dgram_packing=1 debug_level=2" \ + "$P_CLI dtls=1 dgram_packing=0 debug_level=2" \ + 0 \ + -S "next record in same datagram" \ + -c "next record in same datagram" + +run_test "DTLS: multiple records in same datagram, neither client nor server" \ + "$P_SRV dtls=1 dgram_packing=0 debug_level=2" \ + "$P_CLI dtls=1 dgram_packing=0 debug_level=2" \ + 0 \ + -S "next record in same datagram" \ + -C "next record in same datagram" + # Tests for Truncated HMAC extension run_test "Truncated HMAC: client default, server default" \ From d87a59cc3679e04f417be281057e479d7b7ae0ae Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 14 Aug 2018 16:34:55 +0100 Subject: [PATCH 21/44] Adapt ChangeLog --- ChangeLog | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/ChangeLog b/ChangeLog index bab69f676..ef8abc8bf 100644 --- a/ChangeLog +++ b/ChangeLog @@ -4,6 +4,12 @@ mbed TLS ChangeLog (Sorted per branch, date) Features * Add support for fragmentation of outoing DTLS handshake messages. + * Add support for packing multiple records within a single datagram, + enabled by default. + +API Changes + * Add function mbedtls_ssl_conf_datagram_packing() to configure + the use of datagram packing (enabled by default). Bugfix * Fixes an issue with MBEDTLS_CHACHAPOLY_C which would not compile if From bc73e4a822b57d0ab924b817d3ede91ef170cac7 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 17 Aug 2018 15:53:21 +0100 Subject: [PATCH 22/44] Allow GNUTLS_NEXT_CLI / GNUTLS_NEXT_SERV to be unset in ssl-opt.sh --- tests/ssl-opt.sh | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index 995478019..f5de2ee00 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -51,13 +51,13 @@ else O_LEGACY_CLI=false fi -if [ -n "${GNUTLS_NEXT_SERV}" ]; then +if [ -n "${GNUTLS_NEXT_SERV:-}" ]; then G_NEXT_SRV="$GNUTLS_NEXT_SERV --x509certfile data_files/server5.crt --x509keyfile data_files/server5.key" else G_NEXT_SRV=false fi -if [ -n "${GNUTLS_NEXT_CLI}" ]; then +if [ -n "${GNUTLS_NEXT_CLI:-}" ]; then G_NEXT_CLI="echo 'GET / HTTP/1.0' | $GNUTLS_NEXT_CLI --x509cafile data_files/test-ca_cat12.crt" else G_NEXT_CLI=false @@ -772,11 +772,11 @@ if [ -n "${OPENSSL_LEGACY:-}" ]; then O_LEGACY_CLI="$O_LEGACY_CLI -connect localhost:+SRV_PORT" fi -if [ -n "${GNUTLS_NEXT_SERV}" ]; then +if [ -n "${GNUTLS_NEXT_SERV:-}" ]; then G_NEXT_SRV="$G_NEXT_SRV -p $SRV_PORT" fi -if [ -n "${GNUTLS_NEXT_CLI}" ]; then +if [ -n "${GNUTLS_NEXT_CLI:-}" ]; then G_NEXT_CLI="$G_NEXT_CLI -p +SRV_PORT localhost" fi From 4a9d006f5f524890f2d2f77e3df00ccc02fc7364 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 17 Aug 2018 16:10:47 +0100 Subject: [PATCH 23/44] Add missing dependency in ssl-opt.sh --- tests/ssl-opt.sh | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index f5de2ee00..4fa8609f9 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -5605,6 +5605,7 @@ run_test "DTLS fragmenting: 3d, openssl server, DTLS 1.0" \ # -nbio is added to prevent s_client from blocking in case of duplicated # messages at the end of the handshake +requires_openssl_legacy requires_config_enabled MBEDTLS_SSL_PROTO_DTLS requires_config_enabled MBEDTLS_RSA_C requires_config_enabled MBEDTLS_ECDSA_C From e1dcb0355743aab27b3e538ebf9eda53f4f9ef61 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 17 Aug 2018 16:47:58 +0100 Subject: [PATCH 24/44] Don't send empty fragments of nonempty handshake messages This for example lead to the following corner case bug: The code attempted to piggy-back a Finished message at the end of a datagram where precisely 12 bytes of payload were still available. This lead to an empty Finished fragment being sent, and when mbedtls_ssl_flight_transmit() was called again, it believed that it was just starting to send the Finished message, thereby calling ssl_swap_epochs() which had already happened in the call sending the empty fragment. Therefore, the second call would send the 'rest' of the Finished message with wrong epoch. --- library/ssl_tls.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 9b8f7fea3..cc470583a 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -2923,15 +2923,17 @@ int mbedtls_ssl_flight_transmit( mbedtls_ssl_context *ssl ) size_t max_frag_len; const mbedtls_ssl_flight_item * const cur = ssl->handshake->cur_msg; + int const is_finished = + ( cur->type == MBEDTLS_SSL_MSG_HANDSHAKE && + cur->p[0] == MBEDTLS_SSL_HS_FINISHED ); + uint8_t const force_flush = ssl->disable_datagram_packing == 1 ? SSL_FORCE_FLUSH : SSL_DONT_FORCE_FLUSH; /* Swap epochs before sending Finished: we can't do it after * sending ChangeCipherSpec, in case write returns WANT_READ. * Must be done before copying, may change out_msg pointer */ - if( cur->type == MBEDTLS_SSL_MSG_HANDSHAKE && - cur->p[0] == MBEDTLS_SSL_HS_FINISHED && - ssl->handshake->cur_msg_p == ( cur->p + 12 ) ) + if( is_finished && ssl->handshake->cur_msg_p == ( cur->p + 12 ) ) { MBEDTLS_SSL_DEBUG_MSG( 2, ( "swap epochs to send finished message" ) ); ssl_swap_epochs( ssl ); @@ -2968,13 +2970,10 @@ int mbedtls_ssl_flight_transmit( mbedtls_ssl_context *ssl ) const size_t rem_len = hs_len - frag_off; size_t cur_hs_frag_len, max_hs_frag_len; - if( max_frag_len < 12 ) + if( ( max_frag_len < 12 ) || ( max_frag_len == 12 && hs_len != 0 ) ) { - if( cur->type == MBEDTLS_SSL_MSG_HANDSHAKE && - cur->p[0] == MBEDTLS_SSL_HS_FINISHED ) - { + if( is_finished ) ssl_swap_epochs( ssl ); - } if( ( ret = mbedtls_ssl_flush_output( ssl ) ) != 0 ) return( ret ); From 513815a38dd3e864531456d9537298de8b32d7ce Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Mon, 20 Aug 2018 11:56:09 +0100 Subject: [PATCH 25/44] Fix typo in debugging output --- 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 cc470583a..05a2a9f01 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -3388,7 +3388,7 @@ int mbedtls_ssl_write_record( mbedtls_ssl_context *ssl, uint8_t force_flush ) flush = SSL_FORCE_FLUSH; else { - MBEDTLS_SSL_DEBUG_MSG( 2, ( "Stil %u bytes available in current datagram", (unsigned) remaining ) ); + MBEDTLS_SSL_DEBUG_MSG( 2, ( "Still %u bytes available in current datagram", (unsigned) remaining ) ); } } #endif /* MBEDTLS_SSL_PROTO_DTLS */ From 4e1a9c17f29f9b4af76d95202a0030c7aa46873b Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Mon, 20 Aug 2018 12:21:35 +0100 Subject: [PATCH 26/44] ssl-opt.sh: Preserve proxy log, too, if --preserve-logs is specified --- tests/ssl-opt.sh | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index 4fa8609f9..09728314d 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -682,6 +682,9 @@ run_test() { if [ "$PRESERVE_LOGS" -gt 0 ]; then mv $SRV_OUT o-srv-${TESTS}.log mv $CLI_OUT o-cli-${TESTS}.log + if [ -n "$PXY_CMD" ]; then + mv $PXY_OUT o-pxy-${TESTS}.log + fi fi rm -f $SRV_OUT $CLI_OUT $PXY_OUT From f362c297fa199fc4269d940e252b8933426fce2b Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Mon, 20 Aug 2018 12:40:23 +0100 Subject: [PATCH 27/44] ssl-opt.sh Add dependency on gnutls in two fragmentation tests --- tests/ssl-opt.sh | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index 09728314d..b6af4dff0 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -5359,6 +5359,7 @@ run_test "DTLS fragmenting: proxy MTU + 3d" \ # # here and below we just want to test that the we fragment in a way that # pleases other implementations, so we don't need the peer to fragment +requires_gnutls requires_config_enabled MBEDTLS_SSL_PROTO_DTLS requires_config_enabled MBEDTLS_RSA_C requires_config_enabled MBEDTLS_ECDSA_C @@ -5373,6 +5374,7 @@ run_test "DTLS fragmenting: gnutls server, DTLS 1.2" \ -c "fragmenting handshake message" \ -C "error" +requires_gnutls requires_config_enabled MBEDTLS_SSL_PROTO_DTLS requires_config_enabled MBEDTLS_RSA_C requires_config_enabled MBEDTLS_ECDSA_C From f61ff4e1d689388d76abb83f685f48a5c1c1f914 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 21 Aug 2018 13:17:47 +0100 Subject: [PATCH 28/44] ssl_server2: Remove redundant new line --- programs/ssl/ssl_server2.c | 1 - 1 file changed, 1 deletion(-) diff --git a/programs/ssl/ssl_server2.c b/programs/ssl/ssl_server2.c index 4378e4f25..8d414364a 100644 --- a/programs/ssl/ssl_server2.c +++ b/programs/ssl/ssl_server2.c @@ -2193,7 +2193,6 @@ int main( int argc, char *argv[] ) }; #endif - #if defined(MBEDTLS_SSL_TRUNCATED_HMAC) if( opt.trunc_hmac != DFL_TRUNC_HMAC ) mbedtls_ssl_conf_truncated_hmac( &conf, opt.trunc_hmac ); From ecff20554821ca9962c587fd9f55768f4d9fe787 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 21 Aug 2018 13:20:00 +0100 Subject: [PATCH 29/44] Remove stray bracket if MBEDTLS_ZLIB_SUPPORT is defined --- library/ssl_tls.c | 1 - 1 file changed, 1 deletion(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 0b3fea177..08ed75dc2 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -7238,7 +7238,6 @@ int mbedtls_ssl_get_record_expansion( const mbedtls_ssl_context *ssl ) #if defined(MBEDTLS_ZLIB_SUPPORT) if( ssl->session_out->compression != MBEDTLS_SSL_COMPRESS_NULL ) return( MBEDTLS_ERR_SSL_FEATURE_UNAVAILABLE ); - } #endif switch( mbedtls_cipher_get_cipher_mode( &transform->cipher_ctx_enc ) ) From 1f5a15d86dcc7350c5684b350e33b9d769b7cfd4 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 21 Aug 2018 13:31:31 +0100 Subject: [PATCH 30/44] Check retval of remaining_payload_in_datagram in ssl_write_record() --- library/ssl_tls.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 08ed75dc2..e888812f6 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -3383,7 +3383,16 @@ int mbedtls_ssl_write_record( mbedtls_ssl_context *ssl, uint8_t force_flush ) #if defined(MBEDTLS_SSL_PROTO_DTLS) if( ssl->conf->transport == MBEDTLS_SSL_TRANSPORT_DATAGRAM ) { - size_t remaining = ssl_get_remaining_payload_in_datagram( ssl ); + size_t remaining; + ret = ssl_get_remaining_payload_in_datagram( ssl ); + if( ret < 0 ) + { + MBEDTLS_SSL_DEBUG_RET( 1, "ssl_get_remaining_payload_in_datagram", + ret ); + return( ret ); + } + + remaining = (size_t) ret; if( remaining == 0 ) flush = SSL_FORCE_FLUSH; else From 47db877039d61ff28d2c3ce121acaed47e55b437 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 21 Aug 2018 13:32:13 +0100 Subject: [PATCH 31/44] ssl_write_record: Consider setting flush variable only if unset --- 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 e888812f6..e4ea5c2bc 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -3381,7 +3381,8 @@ int mbedtls_ssl_write_record( mbedtls_ssl_context *ssl, uint8_t force_flush ) } #if defined(MBEDTLS_SSL_PROTO_DTLS) - if( ssl->conf->transport == MBEDTLS_SSL_TRANSPORT_DATAGRAM ) + if( ssl->conf->transport == MBEDTLS_SSL_TRANSPORT_DATAGRAM && + flush == SSL_DONT_FORCE_FLUSH ) { size_t remaining; ret = ssl_get_remaining_payload_in_datagram( ssl ); From 5bcf2b081f4ba0ec395478611d19e03f0793c7b6 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 21 Aug 2018 14:25:40 +0100 Subject: [PATCH 32/44] ssl-opt.sh: Allow spurious resend in DTLS session resumption test When a server replies to a cookieless ClientHello with a HelloVerifyRequest, it is supposed to reset the connection and wait for a subsequent ClientHello which includes the cookie from the HelloVerifyRequest. In testing environments, it might happen that the reset of the server takes longer than for the client to replying to the HelloVerifyRequest with the ClientHello+Cookie. In this case, the ClientHello gets lost and the client will need retransmit. This may happen even if the underlying datagram transport is reliable. This commit removes a guard in the ssl-opt.sh test 'DTLS fragmenting: proxy MTU, resumed handshake' which made the test fail in case the log showed a resend from the client. --- tests/ssl-opt.sh | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index 886c44cfa..9b416fbb2 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -5171,6 +5171,9 @@ run_test "DTLS fragmenting: proxy MTU, simple handshake" \ # Since we don't support reading fragmented ClientHello yet, # up the MTU to 1450 (larger than ClientHello with session ticket, # but still smaller than client's Certificate to ensure fragmentation). +# +# A resend on the client-side might happen if the server is +# slow to reset, therefore omitting '-C "resend"' below. not_with_valgrind # spurious resend due to timeout requires_config_enabled MBEDTLS_SSL_PROTO_DTLS requires_config_enabled MBEDTLS_RSA_C @@ -5187,7 +5190,6 @@ run_test "DTLS fragmenting: proxy MTU, resumed handshake" \ mtu=1450 reconnect=1" \ 0 \ -S "resend" \ - -C "resend" \ -s "found fragmented DTLS handshake message" \ -c "found fragmented DTLS handshake message" \ -C "error" From 175cb8fc699a1d755ba81976e53b91a131be445e Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 21 Aug 2018 17:00:10 +0100 Subject: [PATCH 33/44] ssl-opt.sh: Allow resend in DTLS session resumption tests, cont'd This commit continues commit 47db877 by removing resend guards in the ssl-opt.sh tests 'DTLS fragmenting: proxy MTU, XXX' which sometimes made the tests fail in case the log showed a resend from the client. See 47db877 for more information. --- tests/ssl-opt.sh | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index 9b416fbb2..ab53cc46c 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -5194,6 +5194,8 @@ run_test "DTLS fragmenting: proxy MTU, resumed handshake" \ -c "found fragmented DTLS handshake message" \ -C "error" +# A resend on the client-side might happen if the server is +# slow to reset, therefore omitting '-C "resend"' below. not_with_valgrind # spurious resend due to timeout requires_config_enabled MBEDTLS_SSL_PROTO_DTLS requires_config_enabled MBEDTLS_RSA_C @@ -5217,11 +5219,12 @@ run_test "DTLS fragmenting: proxy MTU, ChachaPoly renego" \ mtu=512" \ 0 \ -S "resend" \ - -C "resend" \ -s "found fragmented DTLS handshake message" \ -c "found fragmented DTLS handshake message" \ -C "error" +# A resend on the client-side might happen if the server is +# slow to reset, therefore omitting '-C "resend"' below. not_with_valgrind # spurious resend due to timeout requires_config_enabled MBEDTLS_SSL_PROTO_DTLS requires_config_enabled MBEDTLS_RSA_C @@ -5246,11 +5249,12 @@ run_test "DTLS fragmenting: proxy MTU, AES-GCM renego" \ mtu=512" \ 0 \ -S "resend" \ - -C "resend" \ -s "found fragmented DTLS handshake message" \ -c "found fragmented DTLS handshake message" \ -C "error" +# A resend on the client-side might happen if the server is +# slow to reset, therefore omitting '-C "resend"' below. not_with_valgrind # spurious resend due to timeout requires_config_enabled MBEDTLS_SSL_PROTO_DTLS requires_config_enabled MBEDTLS_RSA_C @@ -5275,11 +5279,12 @@ run_test "DTLS fragmenting: proxy MTU, AES-CCM renego" \ mtu=512" \ 0 \ -S "resend" \ - -C "resend" \ -s "found fragmented DTLS handshake message" \ -c "found fragmented DTLS handshake message" \ -C "error" +# A resend on the client-side might happen if the server is +# slow to reset, therefore omitting '-C "resend"' below. not_with_valgrind # spurious resend due to timeout requires_config_enabled MBEDTLS_SSL_PROTO_DTLS requires_config_enabled MBEDTLS_RSA_C @@ -5305,11 +5310,12 @@ run_test "DTLS fragmenting: proxy MTU, AES-CBC EtM renego" \ mtu=512" \ 0 \ -S "resend" \ - -C "resend" \ -s "found fragmented DTLS handshake message" \ -c "found fragmented DTLS handshake message" \ -C "error" +# A resend on the client-side might happen if the server is +# slow to reset, therefore omitting '-C "resend"' below. not_with_valgrind # spurious resend due to timeout requires_config_enabled MBEDTLS_SSL_PROTO_DTLS requires_config_enabled MBEDTLS_RSA_C @@ -5334,7 +5340,6 @@ run_test "DTLS fragmenting: proxy MTU, AES-CBC non-EtM renego" \ mtu=512" \ 0 \ -S "resend" \ - -C "resend" \ -s "found fragmented DTLS handshake message" \ -c "found fragmented DTLS handshake message" \ -C "error" From b8eec192f6c0150186c07ae1fbc2ea103cd38be1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Mon, 20 Aug 2018 09:34:02 +0200 Subject: [PATCH 34/44] Implement PMTU auto-reduction in handshake --- ChangeLog | 3 +++ include/mbedtls/ssl_internal.h | 1 + library/ssl_tls.c | 29 ++++++++++++++++++++++++++--- tests/ssl-opt.sh | 19 +++++++++++++++++++ 4 files changed, 49 insertions(+), 3 deletions(-) diff --git a/ChangeLog b/ChangeLog index 3f144a7e9..fc4744101 100644 --- a/ChangeLog +++ b/ChangeLog @@ -7,6 +7,9 @@ Features is controlled by the maximum fragment length as set locally or negotiated with the peer, as well as by a new per-connection MTU option, set using mbedtls_ssl_set_mtu(). + * Add support for auto-adjustment of MTU to a safe value during the + handshake when flights do not get through (RFC 6347, section 4.1.1.1, + last paragraph). Bugfix * Fixes an issue with MBEDTLS_CHACHAPOLY_C which would not compile if diff --git a/include/mbedtls/ssl_internal.h b/include/mbedtls/ssl_internal.h index 18982f89a..6be684e05 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -307,6 +307,7 @@ struct mbedtls_ssl_handshake_params resending messages */ unsigned char alt_out_ctr[8]; /*!< Alternative record epoch/counter for resending messages */ + uint16_t mtu; /*!< Handshake mtu, used to fragment outoing messages */ #endif /* MBEDTLS_SSL_PROTO_DTLS */ /* diff --git a/library/ssl_tls.c b/library/ssl_tls.c index faa9467e1..30c1a78f2 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -108,6 +108,15 @@ static int ssl_double_retransmit_timeout( mbedtls_ssl_context *ssl ) if( ssl->handshake->retransmit_timeout >= ssl->conf->hs_timeout_max ) return( -1 ); + /* Implement the final paragraph of RFC 6347 section 4.1.1.1 + * in the following way: after the initial transmission and a first + * retransmission, back off to a temporary estimated MTU of 508 bytes. + * This value is guaranteed to be deliverable (if not guaranteed to be + * delivered) of any compliant IPv4 (and IPv6) network, and should work + * on most non-IP stacks too. */ + if( ssl->handshake->retransmit_timeout != ssl->conf->hs_timeout_min ) + ssl->handshake->mtu = 508; + new_timeout = 2 * ssl->handshake->retransmit_timeout; /* Avoid arithmetic overflow and range overflow */ @@ -7088,6 +7097,20 @@ size_t mbedtls_ssl_get_max_frag_len( const mbedtls_ssl_context *ssl ) } #endif /* MBEDTLS_SSL_MAX_FRAGMENT_LENGTH */ +#if defined(MBEDTLS_SSL_PROTO_DTLS) +static size_t ssl_get_current_mtu( const mbedtls_ssl_context *ssl ) +{ + if( ssl->handshake == NULL || ssl->handshake->mtu == 0 ) + return( ssl->mtu ); + + if( ssl->mtu == 0 ) + return( ssl->handshake->mtu ); + + return( ssl->mtu < ssl->handshake->mtu ? + ssl->mtu : ssl->handshake->mtu ); +} +#endif /* MBEDTLS_SSL_PROTO_DTLS */ + int mbedtls_ssl_get_max_out_record_payload( const mbedtls_ssl_context *ssl ) { size_t max_len = MBEDTLS_SSL_OUT_CONTENT_LEN; @@ -7105,9 +7128,9 @@ int mbedtls_ssl_get_max_out_record_payload( const mbedtls_ssl_context *ssl ) #endif #if defined(MBEDTLS_SSL_PROTO_DTLS) - if( ssl->mtu != 0 ) + if( ssl_get_current_mtu( ssl ) != 0 ) { - const size_t mtu = ssl->mtu; + const size_t mtu = ssl_get_current_mtu( ssl ); const int ret = mbedtls_ssl_get_record_expansion( ssl ); const size_t overhead = (size_t) ret; @@ -7123,7 +7146,7 @@ int mbedtls_ssl_get_max_out_record_payload( const mbedtls_ssl_context *ssl ) if( max_len > mtu - overhead ) max_len = mtu - overhead; } -#endif +#endif /* MBEDTLS_SSL_PROTO_DTLS */ return( (int) max_len ); } diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index ec2717ad5..9fc16bfde 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -5111,6 +5111,25 @@ run_test "DTLS fragmenting: both (MTU)" \ -c "found fragmented DTLS handshake message" \ -C "error" +# Test for automatic MTU reduction on repeated resend +requires_config_enabled MBEDTLS_SSL_PROTO_DTLS +requires_config_enabled MBEDTLS_RSA_C +requires_config_enabled MBEDTLS_ECDSA_C +run_test "DTLS fragmenting: proxy MTU: auto-reduction" \ + -p "$P_PXY mtu=508" \ + "$P_SRV dtls=1 debug_level=2 auth_mode=required \ + crt_file=data_files/server7_int-ca.crt \ + key_file=data_files/server7.key\ + hs_timeout=100-400" \ + "$P_CLI dtls=1 debug_level=2 \ + crt_file=data_files/server8_int-ca2.crt \ + key_file=data_files/server8.key \ + hs_timeout=100-400" \ + 0 \ + -s "found fragmented DTLS handshake message" \ + -c "found fragmented DTLS handshake message" \ + -C "error" + # the proxy shouldn't drop or mess up anything, so we shouldn't need to resend # OTOH the client might resend if the server is to slow to reset after sending # a HelloVerifyRequest, so only check for no retransmission server-side From f47a4afea327073aa69089d6dfca9ad843eaab55 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Wed, 22 Aug 2018 10:38:52 +0200 Subject: [PATCH 35/44] Fix a typo in comments --- 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 6be684e05..7d4418e7b 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -307,7 +307,7 @@ struct mbedtls_ssl_handshake_params resending messages */ unsigned char alt_out_ctr[8]; /*!< Alternative record epoch/counter for resending messages */ - uint16_t mtu; /*!< Handshake mtu, used to fragment outoing messages */ + uint16_t mtu; /*!< Handshake mtu, used to fragment outgoing messages */ #endif /* MBEDTLS_SSL_PROTO_DTLS */ /* From 11682ccc78fd739fca2fe5c6be3319401ea6c0f6 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 22 Aug 2018 14:41:02 +0100 Subject: [PATCH 36/44] Uniformly treat MTU as size_t --- library/ssl_tls.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 8cf7aa1ce..533e8490a 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -109,24 +109,24 @@ static void ssl_update_in_pointers( mbedtls_ssl_context *ssl, #if defined(MBEDTLS_SSL_PROTO_DTLS) static size_t ssl_get_current_mtu( const mbedtls_ssl_context *ssl ); -static uint16_t ssl_get_maximum_datagram_size( mbedtls_ssl_context const *ssl ) +static size_t ssl_get_maximum_datagram_size( mbedtls_ssl_context const *ssl ) { - uint16_t mtu = ssl_get_current_mtu( ssl ); + size_t mtu = ssl_get_current_mtu( ssl ); if( mtu != 0 && mtu < MBEDTLS_SSL_OUT_BUFFER_LEN ) - return( (int) mtu ); + return( mtu ); return( MBEDTLS_SSL_OUT_BUFFER_LEN ); } static int ssl_get_remaining_space_in_datagram( mbedtls_ssl_context const *ssl ) { - size_t const bytes_written = ssl->out_left; - uint16_t const mtu = ssl_get_maximum_datagram_size( ssl ); + size_t const bytes_written = ssl->out_left; + size_t const mtu = ssl_get_maximum_datagram_size( ssl ); /* Double-check that the write-index hasn't gone * past what we can transmit in a single datagram. */ - if( bytes_written > (size_t) mtu ) + if( bytes_written > mtu ) { /* Should never happen... */ return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); From 2c98db24785cb5683b8f63d5fae5a5793cb47d28 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 22 Aug 2018 16:05:47 +0100 Subject: [PATCH 37/44] ssl_write_handshake_msg(): Allow alert on client-side SSLv3 In SSLv3, the client sends a NoCertificate alert in response to a CertificateRequest if it doesn't have a CRT. This previously lead to failure in ssl_write_handshake_msg() which only accepted handshake or CCS records. --- library/ssl_tls.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 533e8490a..e54bb0e50 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -3146,11 +3146,19 @@ int mbedtls_ssl_write_handshake_msg( mbedtls_ssl_context *ssl ) /* * Sanity checks */ - if( ssl->out_msgtype != MBEDTLS_SSL_MSG_HANDSHAKE && + if( ssl->out_msgtype != MBEDTLS_SSL_MSG_HANDSHAKE && ssl->out_msgtype != MBEDTLS_SSL_MSG_CHANGE_CIPHER_SPEC ) { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "should never happen" ) ); - return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); + /* In SSLv3, the client might send a NoCertificate alert. */ +#if defined(MBEDTLS_SSL_PROTO_SSL3) && defined(MBEDTLS_SSL_CLI_C) + if( ! ( ssl->minor_ver == MBEDTLS_SSL_MINOR_VERSION_0 && + ssl->out_msgtype == MBEDTLS_SSL_MSG_ALERT && + ssl->conf->endpoint == MBEDTLS_SSL_IS_CLIENT ) ) +#endif /* MBEDTLS_SSL_PROTO_SSL3 && MBEDTLS_SSL_SRV_C */ + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "should never happen" ) ); + return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); + } } if( ssl->out_msgtype == MBEDTLS_SSL_MSG_HANDSHAKE && From 551835d5e77a1b40566f6f89a12114e88e552e6f Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 22 Aug 2018 16:07:59 +0100 Subject: [PATCH 38/44] ssl_write_handshake_msg(): Always append CCS messages to flights The previous code appended messages to flights only if their handshake type, as derived from the first byte in the message, was different from MBEDTLS_SSL_HS_HELLO_REQUEST. This check should only be performed for handshake records, while CCS records should immediately be appended. --- 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 e54bb0e50..cceb96fd0 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -3256,7 +3256,8 @@ int mbedtls_ssl_write_handshake_msg( mbedtls_ssl_context *ssl ) /* Either send now, or just save to be sent (and resent) later */ #if defined(MBEDTLS_SSL_PROTO_DTLS) if( ssl->conf->transport == MBEDTLS_SSL_TRANSPORT_DATAGRAM && - hs_type != MBEDTLS_SSL_HS_HELLO_REQUEST ) + ( ssl->out_msgtype != MBEDTLS_SSL_MSG_HANDSHAKE || + hs_type != MBEDTLS_SSL_HS_HELLO_REQUEST ) ) { if( ( ret = ssl_flight_append( ssl ) ) != 0 ) { From 554b0af1953a6fd57d20c5914e72daf1bf985c64 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 22 Aug 2018 20:33:41 +0100 Subject: [PATCH 39/44] Fix assertion in mbedtls_ssl_write_record() --- 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 cceb96fd0..0ea7898cf 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -3363,7 +3363,7 @@ int mbedtls_ssl_write_record( mbedtls_ssl_context *ssl, uint8_t force_flush ) * the remaining space in the datagram. */ if( ssl->conf->transport == MBEDTLS_SSL_TRANSPORT_DATAGRAM ) { - ret = ssl_get_maximum_datagram_size( ssl ); + ret = ssl_get_remaining_space_in_datagram( ssl ); if( ret < 0 ) return( ret ); From f4b010efc4b7f5056847810b4be4c960006b78cb Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 24 Aug 2018 10:47:29 +0100 Subject: [PATCH 40/44] Limit MTU by maximum fragment length setting By the standard (RFC 6066, Sect. 4), the Maximum Fragment Length (MFL) extension limits the maximum record payload size, but not the maximum datagram size. However, not inferring any limitations on the MTU when setting the MFL means that a party has no means to dynamically inform the peer about MTU limitations. This commit changes the function ssl_get_remaining_payload_in_datagram() to never return more than MFL - { Total size of all records within the current datagram } thereby limiting the MTU to MFL + { Maximum Record Expansion }. --- library/ssl_tls.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 0ea7898cf..37ba93baf 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -146,6 +146,20 @@ static int ssl_get_remaining_payload_in_datagram( mbedtls_ssl_context const *ssl if( max_len > mfl ) max_len = mfl; + + /* By the standard (RFC 6066 Sect. 4), the MFL extension + * only limits the maximum record payload size, so in theory + * we would be allowed to pack multiple records of payload size + * MFL into a single datagram. However, this would mean that there's + * no way to explicitly communicate MTU restrictions to the peer. + * + * The following reduction of max_len makes sure that we never + * write datagrams larger than MFL + Record Expansion Overhead. + */ + if( max_len <= ssl->out_left ) + return( 0 ); + + max_len -= ssl->out_left; #endif ret = ssl_get_remaining_space_in_datagram( ssl ); From 1841b0a11c34e2c9bda4ccc5c72eb35313a226d5 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 24 Aug 2018 11:13:57 +0100 Subject: [PATCH 41/44] Rename ssl_conf_datagram_packing() to ssl_set_datagram_packing() The naming convention is that functions of the form mbedtls_ssl_conf_xxx() apply to the SSL configuration. --- ChangeLog | 2 +- include/mbedtls/ssl.h | 4 ++-- library/ssl_tls.c | 4 ++-- programs/ssl/ssl_client2.c | 2 +- programs/ssl/ssl_server2.c | 2 +- 5 files changed, 7 insertions(+), 7 deletions(-) diff --git a/ChangeLog b/ChangeLog index 4bf4c8eb9..8f05896b7 100644 --- a/ChangeLog +++ b/ChangeLog @@ -14,7 +14,7 @@ Features enabled by default. API Changes - * Add function mbedtls_ssl_conf_datagram_packing() to configure + * Add function mbedtls_ssl_set_datagram_packing() to configure the use of datagram packing (enabled by default). Bugfix diff --git a/include/mbedtls/ssl.h b/include/mbedtls/ssl.h index c86a0f928..e7f7ea40b 100644 --- a/include/mbedtls/ssl.h +++ b/include/mbedtls/ssl.h @@ -1842,8 +1842,8 @@ void mbedtls_ssl_conf_dtls_badmac_limit( mbedtls_ssl_config *conf, unsigned limi * are currently always sent in separate datagrams. * */ -void mbedtls_ssl_conf_datagram_packing( mbedtls_ssl_context *ssl, - unsigned allow_packing ); +void mbedtls_ssl_set_datagram_packing( mbedtls_ssl_context *ssl, + unsigned allow_packing ); /** * \brief Set retransmit timeout values for the DTLS handshake. diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 37ba93baf..378137c7e 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -6477,8 +6477,8 @@ void mbedtls_ssl_conf_dtls_badmac_limit( mbedtls_ssl_config *conf, unsigned limi #if defined(MBEDTLS_SSL_PROTO_DTLS) -void mbedtls_ssl_conf_datagram_packing( mbedtls_ssl_context *ssl, - unsigned allow_packing ) +void mbedtls_ssl_set_datagram_packing( mbedtls_ssl_context *ssl, + unsigned allow_packing ) { ssl->disable_datagram_packing = !allow_packing; } diff --git a/programs/ssl/ssl_client2.c b/programs/ssl/ssl_client2.c index cfcb27d1c..efd2b3043 100644 --- a/programs/ssl/ssl_client2.c +++ b/programs/ssl/ssl_client2.c @@ -1354,7 +1354,7 @@ int main( int argc, char *argv[] ) opt.hs_to_max ); if( opt.dgram_packing != DFL_DGRAM_PACKING ) - mbedtls_ssl_conf_datagram_packing( &ssl, opt.dgram_packing ); + mbedtls_ssl_set_datagram_packing( &ssl, opt.dgram_packing ); #endif /* MBEDTLS_SSL_PROTO_DTLS */ #if defined(MBEDTLS_SSL_MAX_FRAGMENT_LENGTH) diff --git a/programs/ssl/ssl_server2.c b/programs/ssl/ssl_server2.c index 8d414364a..070c00555 100644 --- a/programs/ssl/ssl_server2.c +++ b/programs/ssl/ssl_server2.c @@ -2182,7 +2182,7 @@ int main( int argc, char *argv[] ) mbedtls_ssl_conf_handshake_timeout( &conf, opt.hs_to_min, opt.hs_to_max ); if( opt.dgram_packing != DFL_DGRAM_PACKING ) - mbedtls_ssl_conf_datagram_packing( &ssl, opt.dgram_packing ); + mbedtls_ssl_set_datagram_packing( &ssl, opt.dgram_packing ); #endif /* MBEDTLS_SSL_PROTO_DTLS */ #if defined(MBEDTLS_SSL_MAX_FRAGMENT_LENGTH) From eb57008d7d02d547b74a20fc5b210d25b9547f52 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 24 Aug 2018 11:28:35 +0100 Subject: [PATCH 42/44] Fix typo in documentation of mbedtls_ssl_set_datagram_packing() --- include/mbedtls/ssl.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/mbedtls/ssl.h b/include/mbedtls/ssl.h index e7f7ea40b..da4b68828 100644 --- a/include/mbedtls/ssl.h +++ b/include/mbedtls/ssl.h @@ -1838,7 +1838,7 @@ void mbedtls_ssl_conf_dtls_badmac_limit( mbedtls_ssl_config *conf, unsigned limi * or flight retransmission (if no buffering is used) as * means to deal with reordering are needed less frequently. * - * \note Application datagrams are not affected by this option and + * \note Application records are not affected by this option and * are currently always sent in separate datagrams. * */ From c92b5c8a0d913cf32586623e065ba113867593a6 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 24 Aug 2018 11:48:01 +0100 Subject: [PATCH 43/44] ssl-opt.sh: Add tests checking that MFL implies bounds on MTU This commit introduces some tests to ssl-opt.sh checking that setting the MFL limits the MTU to MFL + { Maximum Record Expansion }. --- tests/ssl-opt.sh | 78 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 78 insertions(+) diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index e63d45faf..9ac80a5cf 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -5038,6 +5038,32 @@ run_test "DTLS fragmenting: server only (more) (max_frag_len)" \ -c "found fragmented DTLS handshake message" \ -C "error" +# While not required by the standard defining the MFL extension +# (according to which it only applies to records, not to datagrams), +# Mbed TLS will never send datagrams larger than MFL + { Max record expansion }, +# as otherwise there wouldn't be any means to communicate MTU restrictions +# to the peer. +# The next test checks that no datagrams significantly larger than the +# negotiated MFL are sent. +requires_config_enabled MBEDTLS_SSL_PROTO_DTLS +requires_config_enabled MBEDTLS_RSA_C +requires_config_enabled MBEDTLS_ECDSA_C +requires_config_enabled MBEDTLS_SSL_MAX_FRAGMENT_LENGTH +run_test "DTLS fragmenting: server only (more) (max_frag_len), proxy MTU" \ + -p "$P_PXY mtu=560" \ + "$P_SRV dtls=1 debug_level=2 auth_mode=required \ + crt_file=data_files/server7_int-ca.crt \ + key_file=data_files/server7.key \ + max_frag_len=512" \ + "$P_CLI dtls=1 debug_level=2 \ + crt_file=data_files/server8_int-ca2.crt \ + key_file=data_files/server8.key \ + max_frag_len=2048" \ + 0 \ + -S "found fragmented DTLS handshake message" \ + -c "found fragmented DTLS handshake message" \ + -C "error" + requires_config_enabled MBEDTLS_SSL_PROTO_DTLS requires_config_enabled MBEDTLS_RSA_C requires_config_enabled MBEDTLS_ECDSA_C @@ -5056,6 +5082,32 @@ run_test "DTLS fragmenting: client-initiated, server only (max_frag_len)" \ -c "found fragmented DTLS handshake message" \ -C "error" +# While not required by the standard defining the MFL extension +# (according to which it only applies to records, not to datagrams), +# Mbed TLS will never send datagrams larger than MFL + { Max record expansion }, +# as otherwise there wouldn't be any means to communicate MTU restrictions +# to the peer. +# The next test checks that no datagrams significantly larger than the +# negotiated MFL are sent. +requires_config_enabled MBEDTLS_SSL_PROTO_DTLS +requires_config_enabled MBEDTLS_RSA_C +requires_config_enabled MBEDTLS_ECDSA_C +requires_config_enabled MBEDTLS_SSL_MAX_FRAGMENT_LENGTH +run_test "DTLS fragmenting: client-initiated, server only (max_frag_len), proxy MTU" \ + -p "$P_PXY mtu=560" \ + "$P_SRV dtls=1 debug_level=2 auth_mode=none \ + crt_file=data_files/server7_int-ca.crt \ + key_file=data_files/server7.key \ + max_frag_len=2048" \ + "$P_CLI dtls=1 debug_level=2 \ + crt_file=data_files/server8_int-ca2.crt \ + key_file=data_files/server8.key \ + max_frag_len=512" \ + 0 \ + -S "found fragmented DTLS handshake message" \ + -c "found fragmented DTLS handshake message" \ + -C "error" + requires_config_enabled MBEDTLS_SSL_PROTO_DTLS requires_config_enabled MBEDTLS_RSA_C requires_config_enabled MBEDTLS_ECDSA_C @@ -5074,6 +5126,32 @@ run_test "DTLS fragmenting: client-initiated, both (max_frag_len)" \ -c "found fragmented DTLS handshake message" \ -C "error" +# While not required by the standard defining the MFL extension +# (according to which it only applies to records, not to datagrams), +# Mbed TLS will never send datagrams larger than MFL + { Max record expansion }, +# as otherwise there wouldn't be any means to communicate MTU restrictions +# to the peer. +# The next test checks that no datagrams significantly larger than the +# negotiated MFL are sent. +requires_config_enabled MBEDTLS_SSL_PROTO_DTLS +requires_config_enabled MBEDTLS_RSA_C +requires_config_enabled MBEDTLS_ECDSA_C +requires_config_enabled MBEDTLS_SSL_MAX_FRAGMENT_LENGTH +run_test "DTLS fragmenting: client-initiated, both (max_frag_len), proxy MTU" \ + -p "$P_PXY mtu=560" \ + "$P_SRV dtls=1 debug_level=2 auth_mode=required \ + crt_file=data_files/server7_int-ca.crt \ + key_file=data_files/server7.key \ + max_frag_len=2048" \ + "$P_CLI dtls=1 debug_level=2 \ + crt_file=data_files/server8_int-ca2.crt \ + key_file=data_files/server8.key \ + max_frag_len=512" \ + 0 \ + -s "found fragmented DTLS handshake message" \ + -c "found fragmented DTLS handshake message" \ + -C "error" + requires_config_enabled MBEDTLS_SSL_PROTO_DTLS requires_config_enabled MBEDTLS_RSA_C requires_config_enabled MBEDTLS_ECDSA_C From 69ca0ad5c4e5dda5143793bcb114022edc18a473 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 24 Aug 2018 12:11:35 +0100 Subject: [PATCH 44/44] ssl-opt.sh: Remove wrong test exercising MTU implications of MFL The negotiated MFL is always the one suggested by the client, even if the server has a smaller MFL configured locally. Hence, in the test where the client asks for an MFL of 4096 bytes while the server locally has an MFL of 512 bytes configured, the client will still send datagrams of up to ~4K size. --- tests/ssl-opt.sh | 32 +++++--------------------------- 1 file changed, 5 insertions(+), 27 deletions(-) diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index 953afae55..211c8544b 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -5020,6 +5020,10 @@ run_test "DTLS fragmenting: server only (max_frag_len)" \ -c "found fragmented DTLS handshake message" \ -C "error" +# With the MFL extension, the server has no way of forcing +# the client to not exceed a certain MTU; hence, the following +# test can't be replicated with an MTU proxy such as the one +# `client-initiated, server only (max_frag_len)` below. requires_config_enabled MBEDTLS_SSL_PROTO_DTLS requires_config_enabled MBEDTLS_RSA_C requires_config_enabled MBEDTLS_ECDSA_C @@ -5032,33 +5036,7 @@ run_test "DTLS fragmenting: server only (more) (max_frag_len)" \ "$P_CLI dtls=1 debug_level=2 \ crt_file=data_files/server8_int-ca2.crt \ key_file=data_files/server8.key \ - max_frag_len=2048" \ - 0 \ - -S "found fragmented DTLS handshake message" \ - -c "found fragmented DTLS handshake message" \ - -C "error" - -# While not required by the standard defining the MFL extension -# (according to which it only applies to records, not to datagrams), -# Mbed TLS will never send datagrams larger than MFL + { Max record expansion }, -# as otherwise there wouldn't be any means to communicate MTU restrictions -# to the peer. -# The next test checks that no datagrams significantly larger than the -# negotiated MFL are sent. -requires_config_enabled MBEDTLS_SSL_PROTO_DTLS -requires_config_enabled MBEDTLS_RSA_C -requires_config_enabled MBEDTLS_ECDSA_C -requires_config_enabled MBEDTLS_SSL_MAX_FRAGMENT_LENGTH -run_test "DTLS fragmenting: server only (more) (max_frag_len), proxy MTU" \ - -p "$P_PXY mtu=560" \ - "$P_SRV dtls=1 debug_level=2 auth_mode=required \ - crt_file=data_files/server7_int-ca.crt \ - key_file=data_files/server7.key \ - max_frag_len=512" \ - "$P_CLI dtls=1 debug_level=2 \ - crt_file=data_files/server8_int-ca2.crt \ - key_file=data_files/server8.key \ - max_frag_len=2048" \ + max_frag_len=4096" \ 0 \ -S "found fragmented DTLS handshake message" \ -c "found fragmented DTLS handshake message" \