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.
diff --git a/library/ssl_tls.c b/library/ssl_tls.c
index 4d6fc95..135caa0 100644
--- a/library/ssl_tls.c
+++ b/library/ssl_tls.c
@@ -4987,6 +4987,13 @@
{
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. */
@@ -5970,21 +5977,9 @@
}
}
- /*
- * 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( ssl->conf->transport == MBEDTLS_SSL_TRANSPORT_DATAGRAM )
{
- 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 )
@@ -5995,6 +5990,9 @@
else
#endif
{
+ /*
+ * Fetch record contents from underlying transport.
+ */
ret = mbedtls_ssl_fetch_input( ssl,
mbedtls_ssl_in_hdr_len( ssl ) + ssl->in_msglen );
if( ret != 0 )