From e04527755b613730bfd9abf8618bfa88a5e53798 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 10 Jul 2019 17:12:07 +0100 Subject: [PATCH] Check for sufficient datagram size in ssl_parse_record_header() Previously, ssl_parse_record_header() did not check whether the current datagram is large enough to hold a record of the advertised size. This could lead to records being silently skipped over or backed up on the basis of an invalid record length. Concretely, the following would happen: 1) In the case of a record from an old epoch, the record would be 'skipped over' by setting next_record_offset according to the advertised but non-validated length, and only in the subsequent mbedtls_ssl_fetch_input() it would be noticed in an assertion failure if the record length is too large for the current incoming datagram. While not critical, this is fragile, and also contrary to the intend that MBEDTLS_ERR_SSL_INTERNAL_ERROR should never be trigger-able by external input. 2) In the case of a future record being buffered, it might be that we backup a record before we have validated its length, hence copying parts of the input buffer that don't belong to the current record. This is a bug, and it's by luck that it doesn't seem to have critical consequences. This commit fixes this by modifying ssl_parse_record_header() to check that the current incoming datagram is large enough to hold a record of the advertised length, returning MBEDTLS_ERR_SSL_INVALID_RECORD otherwise. --- library/ssl_tls.c | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 343590206..647e49d62 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -4818,6 +4818,13 @@ static int ssl_parse_record_header( mbedtls_ssl_context *ssl ) { unsigned int rec_epoch = ( ssl->in_ctr[0] << 8 ) | ssl->in_ctr[1]; + /* Check that the datagram is large enough to contain a record + * of the advertised length. */ + if( ssl->in_left < mbedtls_ssl_in_hdr_len( ssl ) + ssl->in_msglen ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "Datagram too small to contain record." ) ); + return( MBEDTLS_ERR_SSL_INVALID_RECORD ); + } if( rec_epoch == (unsigned) ssl->in_epoch + 1 ) { /* Consider buffering the record. */ @@ -5801,21 +5808,9 @@ static int ssl_get_next_record( mbedtls_ssl_context *ssl ) } } - /* - * Make sure the entire record contents are available. - * - * In TLS, this means fetching them from the underlying transport. - * In DTLS, it means checking that the incoming datagram is large enough. - */ #if defined(MBEDTLS_SSL_PROTO_DTLS) if( MBEDTLS_SSL_TRANSPORT_IS_DTLS( ssl->conf->transport ) ) { - if( ssl->in_left < mbedtls_ssl_in_hdr_len( ssl ) + ssl->in_msglen ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "Datagram too small to contain record." ) ); - return( MBEDTLS_ERR_SSL_INVALID_RECORD ); - } - /* Remember offset of next record within datagram. */ ssl->next_record_offset = ssl->in_msglen + mbedtls_ssl_in_hdr_len( ssl ); if( ssl->next_record_offset < ssl->in_left ) @@ -5827,6 +5822,9 @@ static int ssl_get_next_record( mbedtls_ssl_context *ssl ) #endif /* MBEDTLS_SSL_PROTO_DTLS */ #if defined(MBEDTLS_SSL_PROTO_TLS) { + /* + * Fetch record contents from underlying transport. + */ ret = mbedtls_ssl_fetch_input( ssl, mbedtls_ssl_in_hdr_len( ssl ) + ssl->in_msglen ); if( ret != 0 )