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.
This commit is contained in:
parent
d5c0f826e6
commit
955a5c98df
1 changed files with 10 additions and 12 deletions
|
@ -4987,6 +4987,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. */
|
||||
|
@ -5970,21 +5977,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( 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 @@ static int ssl_get_next_record( mbedtls_ssl_context *ssl )
|
|||
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 )
|
||||
|
|
Loading…
Reference in a new issue