From bc145f797843a75756bfe0a0328d249e0a769cb5 Mon Sep 17 00:00:00 2001 From: Krzysztof Stachowiak Date: Tue, 20 Mar 2018 11:19:50 +0100 Subject: [PATCH 1/6] Correct buffer size check Further in the code the next field from the binary buffer is read. The check contained an off by one error. --- library/ssl_cli.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/library/ssl_cli.c b/library/ssl_cli.c index 5367fdd0a..c96c812e4 100644 --- a/library/ssl_cli.c +++ b/library/ssl_cli.c @@ -2662,7 +2662,17 @@ static int ssl_parse_certificate_request( mbedtls_ssl_context *ssl ) cert_type_len = buf[mbedtls_ssl_hs_hdr_len( ssl )]; n = cert_type_len; - if( ssl->in_hslen < mbedtls_ssl_hs_hdr_len( ssl ) + 2 + n ) + /* + * In the subsequent code there are two paths that make read from buf: + * * the length of the signature algorithms field (if minor version of + * SSL is 3), + * * distinguished name length otherwise. + * Both reach at most the index: + * ...hdr_len + 2 + n, + * therefore the buffer length at this point must be greater than that + * regardless of the actual code path. + */ + if( ssl->in_hslen <= mbedtls_ssl_hs_hdr_len( ssl ) + 2 + n ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad certificate request message" ) ); mbedtls_ssl_send_alert_message( ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, From bc231cc9b0cecf9723e9e12a6de2cc78f847e1b6 Mon Sep 17 00:00:00 2001 From: Krzysztof Stachowiak Date: Tue, 20 Mar 2018 14:09:53 +0100 Subject: [PATCH 2/6] Add a missing buffer size check --- library/ssl_cli.c | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/library/ssl_cli.c b/library/ssl_cli.c index c96c812e4..e4b2c993f 100644 --- a/library/ssl_cli.c +++ b/library/ssl_cli.c @@ -2687,9 +2687,32 @@ static int ssl_parse_certificate_request( mbedtls_ssl_context *ssl ) size_t sig_alg_len = ( ( buf[mbedtls_ssl_hs_hdr_len( ssl ) + 1 + n] << 8 ) | ( buf[mbedtls_ssl_hs_hdr_len( ssl ) + 2 + n] ) ); #if defined(MBEDTLS_DEBUG_C) - unsigned char* sig_alg = buf + mbedtls_ssl_hs_hdr_len( ssl ) + 3 + n; + unsigned char* sig_alg; size_t i; +#endif + /* + * The farthes access in buf is in the loop few lines below: + * sig_alg[i + 1], + * where: + * sig_alg = buf + ...hdr_len + 3 + n, + * max(i) = sig_alg_len - 1. + * Therefore the farthest access is: + * buf[...hdr_len + 3 + n + sig_alg_len - 1 + 1], + * which reduces to: + * buf[...hdr_len + 3 + n + sig_alg_len], + * which is one less than we need the buf to be. + */ + if( ssl->in_hslen <= mbedtls_ssl_hs_hdr_len( ssl ) + 3 + n + sig_alg_len ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad certificate request message" ) ); + mbedtls_ssl_send_alert_message( ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, + MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR ); + return( MBEDTLS_ERR_SSL_BAD_HS_CERTIFICATE_REQUEST ); + } + +#if defined(MBEDTLS_DEBUG_C) + sig_alg = buf + mbedtls_ssl_hs_hdr_len( ssl ) + 3 + n; for( i = 0; i < sig_alg_len; i += 2 ) { MBEDTLS_SSL_DEBUG_MSG( 3, ( "Supported Signature Algorithm found: %d" From c0b13f7f0c845b1ef348dd82e0f5790e6c07ebc9 Mon Sep 17 00:00:00 2001 From: Krzysztof Stachowiak Date: Tue, 20 Mar 2018 14:10:15 +0100 Subject: [PATCH 3/6] Update change log --- ChangeLog | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/ChangeLog b/ChangeLog index cfe27f3eb..d2e9842d6 100644 --- a/ChangeLog +++ b/ChangeLog @@ -17,6 +17,12 @@ Security implementation allowed an offline 2^80 brute force attack on the HMAC key of a single, uninterrupted connection (with no resumption of the session). + * Fix buffer length assertion in the ssl_parse_certificate_request() + function which leads to an arbitrary overread of the message buffer. The + overreads could occur upon receiving a message malformed at the point + where an optional signature algorithms list is expected in the cases of + the signature algorithms section being too short. In the debug builds + the overread data is printed to the standard output. Features * Extend PKCS#8 interface by introducing support for the entire SHA @@ -47,6 +53,9 @@ Bugfix * Fix memory allocation corner cases in memory_buffer_alloc.c module. Found by Guido Vranken. #639 * Log correct number of ciphersuites used in Client Hello message. #918 + * Fix buffer length assertions in the ssl_parse_certificate_request() + function which leads to a potential one byte overread of the message + buffer. Changes * Fix tag lengths and value ranges in the documentation of CCM encryption. From 73b183c3bb6e729bcd3b47804de52fdc9194940c Mon Sep 17 00:00:00 2001 From: Krzysztof Stachowiak Date: Thu, 5 Apr 2018 10:20:09 +0200 Subject: [PATCH 4/6] Add buffer size check before cert_type_len read --- library/ssl_cli.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/library/ssl_cli.c b/library/ssl_cli.c index e4b2c993f..84f2f3d00 100644 --- a/library/ssl_cli.c +++ b/library/ssl_cli.c @@ -2659,6 +2659,13 @@ static int ssl_parse_certificate_request( mbedtls_ssl_context *ssl ) buf = ssl->in_msg; /* certificate_types */ + if( ssl->in_hslen <= mbedtls_ssl_hs_hdr_len( ssl ) ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad certificate request message" ) ); + mbedtls_ssl_send_alert_message( ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, + MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR ); + return( MBEDTLS_ERR_SSL_BAD_HS_CERTIFICATE_REQUEST ); + } cert_type_len = buf[mbedtls_ssl_hs_hdr_len( ssl )]; n = cert_type_len; From cd09fc812dcbfbee63e69f243381d7d0343fc54c Mon Sep 17 00:00:00 2001 From: Krzysztof Stachowiak Date: Thu, 5 Apr 2018 14:48:18 +0200 Subject: [PATCH 5/6] Remove a redundant test --- library/ssl_cli.c | 8 -------- 1 file changed, 8 deletions(-) diff --git a/library/ssl_cli.c b/library/ssl_cli.c index 84f2f3d00..e74f5dd69 100644 --- a/library/ssl_cli.c +++ b/library/ssl_cli.c @@ -2728,14 +2728,6 @@ static int ssl_parse_certificate_request( mbedtls_ssl_context *ssl ) #endif n += 2 + sig_alg_len; - - if( ssl->in_hslen < mbedtls_ssl_hs_hdr_len( ssl ) + 2 + n ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad certificate request message" ) ); - mbedtls_ssl_send_alert_message( ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, - MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR ); - return( MBEDTLS_ERR_SSL_BAD_HS_CERTIFICATE_REQUEST ); - } } #endif /* MBEDTLS_SSL_PROTO_TLS1_2 */ From 94d49978eb8ac94f97c8c5f15aad9d45fe30efc1 Mon Sep 17 00:00:00 2001 From: Krzysztof Stachowiak Date: Thu, 5 Apr 2018 14:48:55 +0200 Subject: [PATCH 6/6] Improve comments style --- library/ssl_cli.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/library/ssl_cli.c b/library/ssl_cli.c index e74f5dd69..65b2c7656 100644 --- a/library/ssl_cli.c +++ b/library/ssl_cli.c @@ -2670,7 +2670,7 @@ static int ssl_parse_certificate_request( mbedtls_ssl_context *ssl ) n = cert_type_len; /* - * In the subsequent code there are two paths that make read from buf: + * In the subsequent code there are two paths that read from buf: * * the length of the signature algorithms field (if minor version of * SSL is 3), * * distinguished name length otherwise. @@ -2699,12 +2699,12 @@ static int ssl_parse_certificate_request( mbedtls_ssl_context *ssl ) #endif /* - * The farthes access in buf is in the loop few lines below: + * The furthest access in buf is in the loop few lines below: * sig_alg[i + 1], * where: * sig_alg = buf + ...hdr_len + 3 + n, * max(i) = sig_alg_len - 1. - * Therefore the farthest access is: + * Therefore the furthest access is: * buf[...hdr_len + 3 + n + sig_alg_len - 1 + 1], * which reduces to: * buf[...hdr_len + 3 + n + sig_alg_len],