From 3d8663b4f97dfefc0f695bf30943b19c22be8bff 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 57d353ebd..a85474588 100644 --- a/library/ssl_cli.c +++ b/library/ssl_cli.c @@ -2675,7 +2675,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 071f9a3e47585527ab14009450e4b9ad3e8d45f8 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 a85474588..7da59f050 100644 --- a/library/ssl_cli.c +++ b/library/ssl_cli.c @@ -2700,9 +2700,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 7da5088289348abd7a3d21a12c31eaf2537d9831 Mon Sep 17 00:00:00 2001 From: Krzysztof Stachowiak Date: Wed, 4 Apr 2018 13:47:40 +0200 Subject: [PATCH 3/6] Update change log --- ChangeLog | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/ChangeLog b/ChangeLog index ee9b669e0..f2488a250 100644 --- a/ChangeLog +++ b/ChangeLog @@ -9,6 +9,12 @@ Security a non DER-compliant certificate correctly signed by a trusted CA, or a trusted CA with a non DER-compliant certificate. Found by luocm on GitHub. Fixes #825. + * 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. Bugfix * Fix spurious uninitialized variable warning in cmac.c. Fix independently @@ -28,6 +34,9 @@ Bugfix ECPrivateKey structure. Found by jethrogb, fixed in #1379. * Return plaintext data sooner on unpadded CBC decryption, as stated in the mbedtls_cipher_update() documentation. Contributed by Andy Leiserson. + * Fix buffer length assertions in the ssl_parse_certificate_request() + function which leads to a potential one byte overread of the message + buffer. Changes * Support cmake build where Mbed TLS is a subproject. Fix From 314f16136fb7314a16ed0124c925e076c4cb4e73 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 7da59f050..393fb9c3a 100644 --- a/library/ssl_cli.c +++ b/library/ssl_cli.c @@ -2672,6 +2672,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 5ca4c5a15db3a1154259794c3b991a948734771a 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 393fb9c3a..25f7b7268 100644 --- a/library/ssl_cli.c +++ b/library/ssl_cli.c @@ -2741,14 +2741,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 affb4f8e90dbc064ef8b794f6383e680893ada89 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 25f7b7268..0d8836406 100644 --- a/library/ssl_cli.c +++ b/library/ssl_cli.c @@ -2683,7 +2683,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. @@ -2712,12 +2712,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],