diff --git a/ChangeLog b/ChangeLog index 0c247f9821..5084c48ec4 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 @@ -26,6 +32,9 @@ Bugfix the mbedtls_cipher_update() documentation. Contributed by Andy Leiserson. * Fix overriding and ignoring return values when parsing and writing to a file in pk_sign program. Found by kevlut in #1142. + * 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 diff --git a/library/ssl_cli.c b/library/ssl_cli.c index 415c506ead..1899886b0c 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; @@ -2456,10 +2456,27 @@ 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; - if( ssl->in_hslen < mbedtls_ssl_hs_hdr_len( ssl ) + 2 + n ) + /* + * 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. + * 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 ); @@ -2501,25 +2518,51 @@ 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 - m += 2; - n += sig_alg_len; - - if( ssl->in_hslen < mbedtls_ssl_hs_hdr_len( ssl ) + 2 + n ) + /* + * 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 furthest 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; } #endif /* MBEDTLS_SSL_PROTO_TLS1_2 */ /* 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 );