From a8434e8f95290b0429c3ce77a7764ed7dc985143 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Mon, 18 Sep 2017 10:54:39 +0100 Subject: [PATCH 1/7] Add compile-time checks for size of record content and payload --- include/mbedtls/ssl_internal.h | 29 ++++++++++++++++++++++++----- 1 file changed, 24 insertions(+), 5 deletions(-) diff --git a/include/mbedtls/ssl_internal.h b/include/mbedtls/ssl_internal.h index 756360b18..916817a22 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -24,6 +24,7 @@ #define MBEDTLS_SSL_INTERNAL_H #include "ssl.h" +#include "cipher.h" #if defined(MBEDTLS_MD5_C) #include "md5.h" @@ -138,13 +139,31 @@ #define MBEDTLS_SSL_PADDING_ADD 0 #endif -#define MBEDTLS_SSL_BUFFER_LEN ( MBEDTLS_SSL_MAX_CONTENT_LEN \ - + MBEDTLS_SSL_COMPRESSION_ADD \ - + 29 /* counter + header + IV */ \ - + MBEDTLS_SSL_MAC_ADD \ - + MBEDTLS_SSL_PADDING_ADD \ +#define MBEDTLS_SSL_PAYLOAD_LEN ( MBEDTLS_SSL_MAX_CONTENT_LEN \ + + MBEDTLS_SSL_COMPRESSION_ADD \ + + MBEDTLS_MAX_IV_LENGTH \ + + MBEDTLS_SSL_MAC_ADD \ + + MBEDTLS_SSL_PADDING_ADD \ ) +/* + * Check that we obey the standard's message size bounds + */ + +#if MBEDTLS_SSL_MAX_CONTENT_LEN > 16384 +#error Bad configuration - record content too large. +#endif + +#if MBEDTLS_SSL_PAYLOAD_LEN > 16384 + 2048 +#error Bad configuration - protected record payload too large. +#endif + +#define MBEDTLS_SSL_BUFFER_LEN ( MBEDTLS_SSL_PAYLOAD_LEN \ + + 5 /* TLS record header */ \ + + 8 /* Additional DTLS fields */ \ + ) + + /* * TLS extension flags (for extensions with outgoing ServerHello content * that need it (e.g. for RENEGOTIATION_INFO the server already knows because From d33f1ca34c39cd42e11f5d0997603a291a4d08df Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Mon, 18 Sep 2017 10:55:31 +0100 Subject: [PATCH 2/7] Add run-time check for record content size in ssl_encrypt_buf --- library/ssl_tls.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index b388156df..970a043e4 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1268,6 +1268,13 @@ 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_MAX_CONTENT_LEN ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "Record content too large, maximum %d", + MBEDTLS_SSL_MAX_CONTENT_LEN ) ); + return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); + } + /* * Add MAC before if needed */ From 9648f8b59cbea751921f5da49c5bcb8cad823b64 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Mon, 18 Sep 2017 10:55:54 +0100 Subject: [PATCH 3/7] Add run-time check for handshake message size in ssl_write_record --- library/ssl_tls.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 970a043e4..d2ca10157 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -2742,6 +2742,15 @@ int mbedtls_ssl_write_record( mbedtls_ssl_context *ssl ) if( ssl->conf->transport == MBEDTLS_SSL_TRANSPORT_DATAGRAM ) { /* Make room for the additional DTLS fields */ + if( MBEDTLS_SSL_MAX_CONTENT_LEN - ssl->out_msglen < 8 ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "DTLS handshake message too large: " + "size %u, maximum %u", + (unsigned) ( ssl->in_hslen - 4 ), + (unsigned) ( MBEDTLS_SSL_MAX_CONTENT_LEN - 12 ) ) ); + return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); + } + memmove( ssl->out_msg + 12, ssl->out_msg + 4, len - 4 ); ssl->out_msglen += 8; len += 8; From 81e96dd54afa16e2be9a91c5465be6cc4a420071 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Mon, 18 Sep 2017 11:07:25 +0100 Subject: [PATCH 4/7] Adapt ChangeLog --- ChangeLog | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ChangeLog b/ChangeLog index e199682ea..1e3614b9b 100644 --- a/ChangeLog +++ b/ChangeLog @@ -5,6 +5,8 @@ mbed TLS ChangeLog (Sorted per branch, date) Bugfix * Fix ssl_parse_record_header() to silently discard invalid DTLS records as recommended in RFC 6347 Section 4.1.2.7. + * Add size-checks for record and handshake message content, securing + fragile yet non-exploitable code-paths. = mbed TLS 2.6.0 branch released 2017-08-10 From 184f6752566bcae1e6dcdfa8408b9f859d7a5111 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 4 Oct 2017 13:47:33 +0100 Subject: [PATCH 5/7] Improve debugging output --- 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 d2ca10157..162367429 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1270,7 +1270,8 @@ static int ssl_encrypt_buf( mbedtls_ssl_context *ssl ) if( ssl->out_msglen > MBEDTLS_SSL_MAX_CONTENT_LEN ) { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "Record content too large, maximum %d", + MBEDTLS_SSL_DEBUG_MSG( 1, ( "Record content %u too large, maximum %d", + (unsigned) ssl->out_msglen, MBEDTLS_SSL_MAX_CONTENT_LEN ) ); return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); } From d25d44413453bd72e00171812b79905e123c6c1d Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 4 Oct 2017 13:56:42 +0100 Subject: [PATCH 6/7] Don't allocate space for DTLS header if DTLS is disabled --- include/mbedtls/ssl_internal.h | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/include/mbedtls/ssl_internal.h b/include/mbedtls/ssl_internal.h index 916817a22..3ce494565 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -158,11 +158,17 @@ #error Bad configuration - protected record payload too large. #endif -#define MBEDTLS_SSL_BUFFER_LEN ( MBEDTLS_SSL_PAYLOAD_LEN \ - + 5 /* TLS record header */ \ - + 8 /* Additional DTLS fields */ \ - ) +#if !defined(MBEDTLS_SSL_PROTO_DTLS) +/* https://tools.ietf.org/html/rfc5246#section-6.2 */ +#define MBEDTLS_SSL_HEADER_LEN 5 +#else +/* https://tools.ietf.org/html/rfc6347#section-4.1 */ +/* 8 additional bytes for epoch and sequence number */ +#define MBEDTLS_SSL_HEADER_LEN 13 +#endif +#define MBEDTLS_SSL_BUFFER_LEN \ + ( ( MBEDTLS_SSL_HEADER_LEN ) + ( MBEDTLS_SSL_PAYLOAD_LEN ) ) /* * TLS extension flags (for extensions with outgoing ServerHello content From 25d6d1a1df3c9ddc077ae62468e4fc5ae06a607d Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 7 Dec 2017 08:22:51 +0000 Subject: [PATCH 7/7] Correct record header size in case of TLS The previous commit reduced the internal header size to 5 bytes in case of TLS. This is not a valid since in that situation Mbed TLS internally uses the first 8 bytes of the message buffer for the implicit record sequence number. --- include/mbedtls/ssl_internal.h | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/include/mbedtls/ssl_internal.h b/include/mbedtls/ssl_internal.h index 3ce494565..476409547 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -158,14 +158,10 @@ #error Bad configuration - protected record payload too large. #endif -#if !defined(MBEDTLS_SSL_PROTO_DTLS) -/* https://tools.ietf.org/html/rfc5246#section-6.2 */ -#define MBEDTLS_SSL_HEADER_LEN 5 -#else -/* https://tools.ietf.org/html/rfc6347#section-4.1 */ -/* 8 additional bytes for epoch and sequence number */ +/* Note: Even though the TLS record header is only 5 bytes + long, we're internally using 8 bytes to store the + implicit sequence number. */ #define MBEDTLS_SSL_HEADER_LEN 13 -#endif #define MBEDTLS_SSL_BUFFER_LEN \ ( ( MBEDTLS_SSL_HEADER_LEN ) + ( MBEDTLS_SSL_PAYLOAD_LEN ) )