From e1dcb0355743aab27b3e538ebf9eda53f4f9ef61 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 17 Aug 2018 16:47:58 +0100 Subject: [PATCH] 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 );