From 6499bedfa838118149368ee30c9660b499c686bd 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 0c93a748e..4d03c9213 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" @@ -134,13 +135,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 aede1836301114c35ee2886119e31b72d99833ce 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 ba586a05e..6840ca0c6 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1259,6 +1259,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 0983dc49d6897a4244564fa2fc92d1d068ef4484 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 6840ca0c6..906d6ac4d 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -2731,6 +2731,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 fbaeea4693486b35587ec9eafc9ffa09cdcfe309 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 8f7843dc6..ac8ba3f97 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.1.9 branch released 2017-08-10 From 6e052b0fbe4d822c4281ceeaca91452731ca25f8 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 906d6ac4d..5c012a198 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1261,7 +1261,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 0ca15967d16305456499c5f203b7c200ae3351ef 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 4d03c9213..2d99d4018 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -154,11 +154,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 e40802aebc22bb107f532cdf7eaf1f9fc20aaf1b 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 2d99d4018..c1534420a 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -154,14 +154,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 ) )