From f8ba5cf8e952106d6f039218d36349fc6928b7ee Mon Sep 17 00:00:00 2001 From: Krzysztof Stachowiak Date: Tue, 20 Mar 2018 11:19:50 +0100 Subject: [PATCH 1/7] 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 415c506ead..5a49d53670 100644 --- a/library/ssl_cli.c +++ b/library/ssl_cli.c @@ -2459,7 +2459,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" ) ); return( MBEDTLS_ERR_SSL_BAD_HS_CERTIFICATE_REQUEST ); From 444678ea8b7c9a2a38af2723ff103c87f6d9252a Mon Sep 17 00:00:00 2001 From: Krzysztof Stachowiak Date: Tue, 20 Mar 2018 14:09:53 +0100 Subject: [PATCH 2/7] Add a missing buffer size check --- library/ssl_cli.c | 35 ++++++++++++++++++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-) diff --git a/library/ssl_cli.c b/library/ssl_cli.c index 5a49d53670..a66cbb0dbf 100644 --- a/library/ssl_cli.c +++ b/library/ssl_cli.c @@ -2511,9 +2511,42 @@ static int ssl_parse_certificate_request( mbedtls_ssl_context *ssl ) // TODO: should check the signature part against our pk_key though 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; + 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" + ",%d", sig_alg[i], sig_alg[i + 1] ) ); + } +#endif + + n += 2 + sig_alg_len; m += 2; - n += sig_alg_len; if( ssl->in_hslen < mbedtls_ssl_hs_hdr_len( ssl ) + 2 + n ) { From 0ac812f5ce11bd3321c8f5ec6f3de35c20361a72 Mon Sep 17 00:00:00 2001 From: Krzysztof Stachowiak Date: Thu, 5 Apr 2018 08:50:20 +0200 Subject: [PATCH 3/7] Adjust 2.1 specific code to match the buffer verification tests --- library/ssl_cli.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/library/ssl_cli.c b/library/ssl_cli.c index a66cbb0dbf..6e8e7e81b8 100644 --- a/library/ssl_cli.c +++ b/library/ssl_cli.c @@ -2402,7 +2402,7 @@ static int ssl_parse_certificate_request( mbedtls_ssl_context *ssl ) { int ret; unsigned char *buf, *p; - size_t n = 0, m = 0; + size_t n = 0; size_t cert_type_len = 0, dn_len = 0; const mbedtls_ssl_ciphersuite_t *ciphersuite_info = ssl->transform_negotiate->ciphersuite_info; @@ -2546,7 +2546,6 @@ static int ssl_parse_certificate_request( mbedtls_ssl_context *ssl ) #endif n += 2 + sig_alg_len; - m += 2; if( ssl->in_hslen < mbedtls_ssl_hs_hdr_len( ssl ) + 2 + n ) { @@ -2558,11 +2557,11 @@ static int ssl_parse_certificate_request( mbedtls_ssl_context *ssl ) /* Ignore certificate_authorities, we only have one cert anyway */ // TODO: should not send cert if no CA matches - dn_len = ( ( buf[mbedtls_ssl_hs_hdr_len( ssl ) + 1 + m + n] << 8 ) - | ( buf[mbedtls_ssl_hs_hdr_len( ssl ) + 2 + m + n] ) ); + dn_len = ( ( buf[mbedtls_ssl_hs_hdr_len( ssl ) + 1 + n] << 8 ) + | ( buf[mbedtls_ssl_hs_hdr_len( ssl ) + 2 + n] ) ); n += dn_len; - if( ssl->in_hslen != mbedtls_ssl_hs_hdr_len( ssl ) + 3 + m + n ) + if( ssl->in_hslen != mbedtls_ssl_hs_hdr_len( ssl ) + 3 + n ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad certificate request message" ) ); return( MBEDTLS_ERR_SSL_BAD_HS_CERTIFICATE_REQUEST ); From 8fc134fcb114fd1c01873b6c409af1c02471f069 Mon Sep 17 00:00:00 2001 From: Krzysztof Stachowiak Date: Thu, 5 Apr 2018 08:51:35 +0200 Subject: [PATCH 4/7] Update change log --- ChangeLog | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/ChangeLog b/ChangeLog index dd04b1d639..9f995dc523 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 * Add missing dependencies in test suites that led to build failures @@ -24,6 +30,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 * Improve testing in configurations that omit certain hashes or From 57e1a9fdfc300927102006759550b8bc2a17ebde Mon Sep 17 00:00:00 2001 From: Krzysztof Stachowiak Date: Thu, 5 Apr 2018 10:20:09 +0200 Subject: [PATCH 5/7] 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 6e8e7e81b8..1fbc94dcd9 100644 --- a/library/ssl_cli.c +++ b/library/ssl_cli.c @@ -2456,6 +2456,13 @@ static int ssl_parse_certificate_request( mbedtls_ssl_context *ssl ) // Retrieve cert 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 99fb6e94614e1b7102336c4e3e8d911bd655e3eb Mon Sep 17 00:00:00 2001 From: Krzysztof Stachowiak Date: Thu, 5 Apr 2018 14:48:18 +0200 Subject: [PATCH 6/7] Remove a redundant test --- library/ssl_cli.c | 6 ------ 1 file changed, 6 deletions(-) diff --git a/library/ssl_cli.c b/library/ssl_cli.c index 1fbc94dcd9..c4cdd92001 100644 --- a/library/ssl_cli.c +++ b/library/ssl_cli.c @@ -2553,12 +2553,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" ) ); - return( MBEDTLS_ERR_SSL_BAD_HS_CERTIFICATE_REQUEST ); - } } #endif /* MBEDTLS_SSL_PROTO_TLS1_2 */ From 28485d0a01360b777b97f2846363d699516cc07c Mon Sep 17 00:00:00 2001 From: Krzysztof Stachowiak Date: Thu, 5 Apr 2018 14:48:55 +0200 Subject: [PATCH 7/7] 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 c4cdd92001..1899886b0c 100644 --- a/library/ssl_cli.c +++ b/library/ssl_cli.c @@ -2467,7 +2467,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. @@ -2524,12 +2524,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],