From f704ece006435d5f363657f5cf52207c20e006fc Mon Sep 17 00:00:00 2001 From: lhuang04 Date: Tue, 27 Apr 2021 08:27:46 -0700 Subject: [PATCH 1/6] Review fix for client side EncryptedExtensions parsing Summary: * Minor style * Boundary check for `buflen`, and remove todo * Replace `MBEDTLS_ERR_SSL_BAD_HS_SERVER_HELLO` by `MBEDTLS_ERR_SSL_BAD_HS_ENCRYPTED_EXTENSIONS` * Add break to default case Test Plan: `tests/ssl-opt.sh` Reviewers: Subscribers: Tasks: Tags: --- library/ssl_tls13_client.c | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/library/ssl_tls13_client.c b/library/ssl_tls13_client.c index 033803a8ed85..f531bd8a864a 100644 --- a/library/ssl_tls13_client.c +++ b/library/ssl_tls13_client.c @@ -2617,12 +2617,15 @@ static int ssl_encrypted_extensions_parse( mbedtls_ssl_context* ssl, unsigned char* buf, size_t buflen ) { - int ret=0; + int ret = 0; size_t ext_len; unsigned char *ext; - /* TODO: Add bounds checks! Only then remove the next line. */ - ((void) buflen ); + if( buflen < 2 ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "Extension buffer length too short - bad encrypted extensions message" ) ); + return( MBEDTLS_ERR_SSL_BAD_HS_ENCRYPTED_EXTENSIONS ); + } ext_len = ( ( buf[0] << 8 ) | ( buf[1] ) ); @@ -2630,17 +2633,17 @@ static int ssl_encrypted_extensions_parse( mbedtls_ssl_context* ssl, ext = buf; /* Checking for an extension length that is too short */ - if( ext_len > 0UL && ext_len < 4UL ) + if( ext_len > 0 && ext_len < 4 ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "Extension length too short - bad encrypted extensions message" ) ); - return( MBEDTLS_ERR_SSL_BAD_HS_SERVER_HELLO ); + return( MBEDTLS_ERR_SSL_BAD_HS_ENCRYPTED_EXTENSIONS ); } /* Checking for an extension length that is not aligned with the rest of the message */ if( buflen != 2 + ext_len ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "Extension length misaligned - bad encrypted extensions message" ) ); - return( MBEDTLS_ERR_SSL_BAD_HS_SERVER_HELLO ); + return( MBEDTLS_ERR_SSL_BAD_HS_ENCRYPTED_EXTENSIONS ); } MBEDTLS_SSL_DEBUG_MSG( 2, ( "encrypted extensions, total extension length: %d", ext_len ) ); @@ -2649,10 +2652,8 @@ static int ssl_encrypted_extensions_parse( mbedtls_ssl_context* ssl, while( ext_len ) { - unsigned int ext_id = ( ( ext[0] << 8 ) - | ( ext[1] ) ); - unsigned int ext_size = ( ( ext[2] << 8 ) - | ( ext[3] ) ); + unsigned int ext_id = ( ( ext[0] << 8 ) | ( ext[1] ) ); + unsigned int ext_size = ( ( ext[2] << 8 ) | ( ext[3] ) ); if( ext_size + 4 > ext_len ) { @@ -2686,7 +2687,7 @@ static int ssl_encrypted_extensions_parse( mbedtls_ssl_context* ssl, case MBEDTLS_TLS_EXT_ALPN: MBEDTLS_SSL_DEBUG_MSG( 3, ( "found alpn extension" ) ); - if( ( ret = ssl_parse_alpn_ext( ssl, ext + 4, (size_t)ext_size ) ) != 0 ) + if( ( ret = ssl_parse_alpn_ext( ssl, ext + 4, (size_t) ext_size ) ) != 0 ) { return( ret ); } @@ -2719,6 +2720,7 @@ static int ssl_encrypted_extensions_parse( mbedtls_ssl_context* ssl, default: MBEDTLS_SSL_DEBUG_MSG( 3, ( "unknown extension found: %d ( ignoring )", ext_id ) ); + break; } ext_len -= 4 + ext_size; From ddea08f54e0cc0df382c80c3b6b534d56a47dc8e Mon Sep 17 00:00:00 2001 From: lhuang04 Date: Wed, 28 Apr 2021 06:19:16 -0700 Subject: [PATCH 2/6] Change `ext_size` to `size_t` Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags: --- library/ssl_tls13_client.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/library/ssl_tls13_client.c b/library/ssl_tls13_client.c index f531bd8a864a..8ea38e8b4349 100644 --- a/library/ssl_tls13_client.c +++ b/library/ssl_tls13_client.c @@ -2653,7 +2653,7 @@ static int ssl_encrypted_extensions_parse( mbedtls_ssl_context* ssl, while( ext_len ) { unsigned int ext_id = ( ( ext[0] << 8 ) | ( ext[1] ) ); - unsigned int ext_size = ( ( ext[2] << 8 ) | ( ext[3] ) ); + size_t ext_size = ( ( ext[2] << 8 ) | ( ext[3] ) ); if( ext_size + 4 > ext_len ) { @@ -2687,7 +2687,7 @@ static int ssl_encrypted_extensions_parse( mbedtls_ssl_context* ssl, case MBEDTLS_TLS_EXT_ALPN: MBEDTLS_SSL_DEBUG_MSG( 3, ( "found alpn extension" ) ); - if( ( ret = ssl_parse_alpn_ext( ssl, ext + 4, (size_t) ext_size ) ) != 0 ) + if( ( ret = ssl_parse_alpn_ext( ssl, ext + 4, ext_size ) ) != 0 ) { return( ret ); } @@ -2709,7 +2709,7 @@ static int ssl_encrypted_extensions_parse( mbedtls_ssl_context* ssl, MBEDTLS_SSL_DEBUG_MSG(3, ("found early data extension")); ret = ssl_parse_encrypted_extensions_early_data_ext( - ssl, ext + 4, (size_t) ext_size ); + ssl, ext + 4, ext_size ); if( ret != 0 ) { MBEDTLS_SSL_DEBUG_RET( 1, "ssl_parse_early_data_ext", ret ); From 9ec22a288acca7fe91f8ad4c45e86fd78ad017f4 Mon Sep 17 00:00:00 2001 From: lhuang04 Date: Wed, 28 Apr 2021 06:24:44 -0700 Subject: [PATCH 3/6] change unsigned char* buf to const Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags: --- library/ssl_tls13_client.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/library/ssl_tls13_client.c b/library/ssl_tls13_client.c index 8ea38e8b4349..26d8c11ceaa2 100644 --- a/library/ssl_tls13_client.c +++ b/library/ssl_tls13_client.c @@ -2531,7 +2531,7 @@ static int ssl_encrypted_extensions_fetch( mbedtls_ssl_context* ssl, #endif /* MBEDTLS_SSL_USE_MPS */ static int ssl_encrypted_extensions_parse( mbedtls_ssl_context* ssl, - unsigned char* buf, + const unsigned char* buf, size_t buflen ); static int ssl_encrypted_extensions_postprocess( mbedtls_ssl_context* ssl ); @@ -2614,12 +2614,12 @@ static int ssl_encrypted_extensions_fetch( mbedtls_ssl_context* ssl, #endif /* MBEDTLS_SSL_USE_MPS */ static int ssl_encrypted_extensions_parse( mbedtls_ssl_context* ssl, - unsigned char* buf, + const unsigned char* buf, size_t buflen ) { int ret = 0; size_t ext_len; - unsigned char *ext; + const unsigned char *ext; if( buflen < 2 ) { From e14038925ddca9fc7fb7539b300fa2897c29ba80 Mon Sep 17 00:00:00 2001 From: lhuang04 Date: Wed, 28 Apr 2021 06:30:04 -0700 Subject: [PATCH 4/6] debug msgs and comments Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags: --- library/ssl_tls13_client.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/library/ssl_tls13_client.c b/library/ssl_tls13_client.c index 26d8c11ceaa2..8e57494db77b 100644 --- a/library/ssl_tls13_client.c +++ b/library/ssl_tls13_client.c @@ -2623,7 +2623,7 @@ static int ssl_encrypted_extensions_parse( mbedtls_ssl_context* ssl, if( buflen < 2 ) { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "Extension buffer length too short - bad encrypted extensions message" ) ); + MBEDTLS_SSL_DEBUG_MSG( 1, ( "EncryptedExtension message too short" ) ); return( MBEDTLS_ERR_SSL_BAD_HS_ENCRYPTED_EXTENSIONS ); } @@ -2635,19 +2635,17 @@ static int ssl_encrypted_extensions_parse( mbedtls_ssl_context* ssl, /* Checking for an extension length that is too short */ if( ext_len > 0 && ext_len < 4 ) { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "Extension length too short - bad encrypted extensions message" ) ); + MBEDTLS_SSL_DEBUG_MSG( 1, ( "EncryptedExtension message too short" ) ); return( MBEDTLS_ERR_SSL_BAD_HS_ENCRYPTED_EXTENSIONS ); } /* Checking for an extension length that is not aligned with the rest of the message */ if( buflen != 2 + ext_len ) { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "Extension length misaligned - bad encrypted extensions message" ) ); + MBEDTLS_SSL_DEBUG_MSG( 1, ( "EncryptedExtension lengths misaligned" ) ); return( MBEDTLS_ERR_SSL_BAD_HS_ENCRYPTED_EXTENSIONS ); } - MBEDTLS_SSL_DEBUG_MSG( 2, ( "encrypted extensions, total extension length: %d", ext_len ) ); - MBEDTLS_SSL_DEBUG_BUF( 3, "encrypted extensions extensions", ext, ext_len ); while( ext_len ) @@ -2699,14 +2697,14 @@ static int ssl_encrypted_extensions_parse( mbedtls_ssl_context* ssl, case MBEDTLS_TLS_EXT_SERVERNAME: MBEDTLS_SSL_DEBUG_MSG( 3, ( "found server_name extension" ) ); - /* The server_name extension is an empty extension */ + /* The server_name extension should be an empty extension */ break; #endif /* MBEDTLS_SSL_SERVER_NAME_INDICATION */ #if defined(MBEDTLS_ZERO_RTT) case MBEDTLS_TLS_EXT_EARLY_DATA: - MBEDTLS_SSL_DEBUG_MSG(3, ("found early data extension")); + MBEDTLS_SSL_DEBUG_MSG(3, ( "found early data extension" )); ret = ssl_parse_encrypted_extensions_early_data_ext( ssl, ext + 4, ext_size ); From 4504ac9525450bd187019b975d513cab985c788a Mon Sep 17 00:00:00 2001 From: lhuang04 Date: Wed, 28 Apr 2021 06:55:29 -0700 Subject: [PATCH 5/6] break ret from parser Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags: --- library/ssl_tls13_client.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/library/ssl_tls13_client.c b/library/ssl_tls13_client.c index 8e57494db77b..39b7ebf938ab 100644 --- a/library/ssl_tls13_client.c +++ b/library/ssl_tls13_client.c @@ -2671,9 +2671,11 @@ static int ssl_encrypted_extensions_parse( mbedtls_ssl_context* ssl, case MBEDTLS_TLS_EXT_MAX_FRAGMENT_LENGTH: MBEDTLS_SSL_DEBUG_MSG( 3, ( "found max_fragment_length extension" ) ); - if( ( ret = ssl_parse_max_fragment_length_ext( ssl, - ext + 4, ext_size ) ) != 0 ) + ret = ssl_parse_max_fragment_length_ext( ssl, ext + 4, + ext_size ); + if( ret != 0 ) { + MBEDTLS_SSL_DEBUG_RET( 1, "ssl_parse_max_fragment_length_ext", ret ); return( ret ); } @@ -2685,8 +2687,10 @@ static int ssl_encrypted_extensions_parse( mbedtls_ssl_context* ssl, case MBEDTLS_TLS_EXT_ALPN: MBEDTLS_SSL_DEBUG_MSG( 3, ( "found alpn extension" ) ); - if( ( ret = ssl_parse_alpn_ext( ssl, ext + 4, ext_size ) ) != 0 ) + ret = ssl_parse_alpn_ext( ssl, ext + 4, ext_size ); + if( ret != 0 ) { + MBEDTLS_SSL_DEBUG_RET( 1, "ssl_parse_alpn_ext", ret ); return( ret ); } From 82ca5bce5a1e3ed66d1559f53513f4aad43fde46 Mon Sep 17 00:00:00 2001 From: lhuang04 Date: Sun, 2 May 2021 06:08:34 -0700 Subject: [PATCH 6/6] cast before calculate number from buffer Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags: --- library/ssl_tls13_client.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/library/ssl_tls13_client.c b/library/ssl_tls13_client.c index 39b7ebf938ab..ba80bbea41af 100644 --- a/library/ssl_tls13_client.c +++ b/library/ssl_tls13_client.c @@ -2627,7 +2627,7 @@ static int ssl_encrypted_extensions_parse( mbedtls_ssl_context* ssl, return( MBEDTLS_ERR_SSL_BAD_HS_ENCRYPTED_EXTENSIONS ); } - ext_len = ( ( buf[0] << 8 ) | ( buf[1] ) ); + ext_len = ( ( (size_t) buf[0] << 8 ) | ( (size_t) buf[1] ) ); buf += 2; /* skip extension length */ ext = buf; @@ -2650,8 +2650,8 @@ static int ssl_encrypted_extensions_parse( mbedtls_ssl_context* ssl, while( ext_len ) { - unsigned int ext_id = ( ( ext[0] << 8 ) | ( ext[1] ) ); - size_t ext_size = ( ( ext[2] << 8 ) | ( ext[3] ) ); + unsigned int ext_id = ( ( (unsigned int) ext[0] << 8 ) | ( (unsigned int) ext[1] ) ); + size_t ext_size = ( ( (size_t) ext[2] << 8 ) | ( (size_t) ext[3] ) ); if( ext_size + 4 > ext_len ) {