From b6e7331739d79650dacfbabfa4536257578978ea Mon Sep 17 00:00:00 2001 From: Waleed Elmelegy Date: Tue, 11 Jun 2024 18:44:48 +0100 Subject: [PATCH 01/10] Fix issue in handling legacy_compression_methods in ssl_tls13_parse_client_hello() Signed-off-by: Waleed Elmelegy --- library/ssl_tls13_server.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/library/ssl_tls13_server.c b/library/ssl_tls13_server.c index f5ef92032b..ca3ea53857 100644 --- a/library/ssl_tls13_server.c +++ b/library/ssl_tls13_server.c @@ -1265,6 +1265,8 @@ static int ssl_tls13_parse_client_hello(mbedtls_ssl_context *ssl, mbedtls_ssl_handshake_params *handshake = ssl->handshake; int hrr_required = 0; int no_usable_share_for_key_agreement = 0; + unsigned char legacy_compression_methods_len; + unsigned char legacy_compression_methods; #if defined(MBEDTLS_SSL_TLS1_3_KEY_EXCHANGE_MODE_SOME_PSK_ENABLED) int got_psk = 0; @@ -1362,6 +1364,13 @@ static int ssl_tls13_parse_client_hello(mbedtls_ssl_context *ssl, p += cipher_suites_len; cipher_suites_end = p; + legacy_compression_methods_len = *p; + legacy_compression_methods = *(p+1); + + if (legacy_compression_methods_len != 1 || legacy_compression_methods != 0) { + return SSL_CLIENT_HELLO_TLS1_2; + } + /* * Search for the supported versions extension and parse it to determine * if the client supports TLS 1.3. From a5842ac20eab3260cbdc99df182e87b172b63fa9 Mon Sep 17 00:00:00 2001 From: Waleed Elmelegy Date: Wed, 19 Jun 2024 15:09:48 +0100 Subject: [PATCH 02/10] Improve handling of legacy_compression_methods in ssl_tls13_parse_client_hello() Signed-off-by: Waleed Elmelegy --- library/ssl_tls13_server.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/library/ssl_tls13_server.c b/library/ssl_tls13_server.c index ca3ea53857..ae690e538e 100644 --- a/library/ssl_tls13_server.c +++ b/library/ssl_tls13_server.c @@ -1265,8 +1265,6 @@ static int ssl_tls13_parse_client_hello(mbedtls_ssl_context *ssl, mbedtls_ssl_handshake_params *handshake = ssl->handshake; int hrr_required = 0; int no_usable_share_for_key_agreement = 0; - unsigned char legacy_compression_methods_len; - unsigned char legacy_compression_methods; #if defined(MBEDTLS_SSL_TLS1_3_KEY_EXCHANGE_MODE_SOME_PSK_ENABLED) int got_psk = 0; @@ -1364,19 +1362,17 @@ static int ssl_tls13_parse_client_hello(mbedtls_ssl_context *ssl, p += cipher_suites_len; cipher_suites_end = p; - legacy_compression_methods_len = *p; - legacy_compression_methods = *(p+1); - - if (legacy_compression_methods_len != 1 || legacy_compression_methods != 0) { - return SSL_CLIENT_HELLO_TLS1_2; - } + /* Check if we have enough data to for legacy_compression_methods + * and a length byte. + */ + MBEDTLS_SSL_CHK_BUF_READ_PTR(p, end, 1 + p[0]); /* * Search for the supported versions extension and parse it to determine * if the client supports TLS 1.3. */ ret = mbedtls_ssl_tls13_is_supported_versions_ext_present_in_exts( - ssl, p + 2, end, + ssl, p + 1 + p[0], end, &supported_versions_data, &supported_versions_data_end); if (ret < 0) { MBEDTLS_SSL_DEBUG_RET(1, From 0a9e8a3a1848a9d90661460dac25024ee02de074 Mon Sep 17 00:00:00 2001 From: Waleed Elmelegy Date: Tue, 25 Jun 2024 10:22:49 +0100 Subject: [PATCH 03/10] Correct a small typo in ssl_tls13_parse_client_hello() Signed-off-by: Waleed Elmelegy --- library/ssl_tls13_server.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/ssl_tls13_server.c b/library/ssl_tls13_server.c index ae690e538e..27235a7f18 100644 --- a/library/ssl_tls13_server.c +++ b/library/ssl_tls13_server.c @@ -1362,7 +1362,7 @@ static int ssl_tls13_parse_client_hello(mbedtls_ssl_context *ssl, p += cipher_suites_len; cipher_suites_end = p; - /* Check if we have enough data to for legacy_compression_methods + /* Check if we have enough data for legacy_compression_methods * and a length byte. */ MBEDTLS_SSL_CHK_BUF_READ_PTR(p, end, 1 + p[0]); From e2a6aa5369cec0d40158f02dc870895345d2a46c Mon Sep 17 00:00:00 2001 From: Waleed Elmelegy Date: Tue, 25 Jun 2024 18:16:16 +0100 Subject: [PATCH 04/10] Improve comments explaining legacy_methods_compression handling Signed-off-by: Waleed Elmelegy --- library/ssl_tls13_server.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/library/ssl_tls13_server.c b/library/ssl_tls13_server.c index 27235a7f18..9c949bd0b1 100644 --- a/library/ssl_tls13_server.c +++ b/library/ssl_tls13_server.c @@ -1355,17 +1355,16 @@ static int ssl_tls13_parse_client_hello(mbedtls_ssl_context *ssl, * compression methods and the length of the extensions. * * cipher_suites cipher_suites_len bytes - * legacy_compression_methods 2 bytes - * extensions_len 2 bytes + * legacy_compression_methods length 1 byte */ - MBEDTLS_SSL_CHK_BUF_READ_PTR(p, end, cipher_suites_len + 2 + 2); + MBEDTLS_SSL_CHK_BUF_READ_PTR(p, end, cipher_suites_len + 1); p += cipher_suites_len; cipher_suites_end = p; /* Check if we have enough data for legacy_compression_methods - * and a length byte. + * and the length of the extensions (2 bytes). */ - MBEDTLS_SSL_CHK_BUF_READ_PTR(p, end, 1 + p[0]); + MBEDTLS_SSL_CHK_BUF_READ_PTR(p + 1, end, p[0] + 2); /* * Search for the supported versions extension and parse it to determine From 0b190f1763aefff81cdfe36f252e60ed174d0b0f Mon Sep 17 00:00:00 2001 From: Waleed Elmelegy Date: Thu, 4 Jul 2024 16:38:04 +0000 Subject: [PATCH 05/10] Add regression testing to handling Legacy_compression_methods Signed-off-by: Waleed Elmelegy --- tests/ssl-opt.sh | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index 646daad199..be06e5ad4b 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -14135,6 +14135,17 @@ run_test "TLS 1.3: no HRR in case of PSK key exchange mode" \ -c "Selected key exchange mode: psk$" \ -c "HTTP/1.0 200 OK" +# Legacy_compression_methods testing + +requires_gnutls +requires_config_enabled MBEDTLS_SSL_PROTO_TLS1_2 +requires_config_enabled MBEDTLS_SSL_PROTO_TLS1_3 +run_test "ClientHello parse handle Legacy_compression_methods" \ + "$P_SRV debug_level=3" \ + "$G_CLI --priority=NORMAL:-VERS-ALL:+VERS-TLS1.2:+COMP-DEFLATE localhost" \ + 0 \ + -c "Handshake was completed" + # Test heap memory usage after handshake requires_config_enabled MBEDTLS_SSL_PROTO_TLS1_2 requires_config_enabled MBEDTLS_MEMORY_DEBUG From dc99c890a6c050f6abf8579b983c2d7bb53e9b6b Mon Sep 17 00:00:00 2001 From: Waleed Elmelegy Date: Mon, 15 Jul 2024 17:25:04 +0000 Subject: [PATCH 06/10] Improve legacy compression regression testing Signed-off-by: Waleed Elmelegy --- tests/ssl-opt.sh | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index be06e5ad4b..7a35c43d40 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -14138,13 +14138,25 @@ run_test "TLS 1.3: no HRR in case of PSK key exchange mode" \ # Legacy_compression_methods testing requires_gnutls +requires_config_enabled MBEDTLS_SSL_SRV_C requires_config_enabled MBEDTLS_SSL_PROTO_TLS1_2 requires_config_enabled MBEDTLS_SSL_PROTO_TLS1_3 -run_test "ClientHello parse handle Legacy_compression_methods" \ +run_test "TLS 1.2 ClientHello indicating support for deflate compression method (fallback from TLS 1.3)" \ "$P_SRV debug_level=3" \ "$G_CLI --priority=NORMAL:-VERS-ALL:+VERS-TLS1.2:+COMP-DEFLATE localhost" \ 0 \ - -c "Handshake was completed" + -c "Handshake was completed" \ + -s "dumping .client hello, compression. (2 bytes)" + +requires_gnutls +requires_config_enabled MBEDTLS_SSL_SRV_C +requires_config_enabled MBEDTLS_SSL_PROTO_TLS1_2 +run_test "TLS 1.2 ClientHello indicating support for deflate compression method" \ + "$P_SRV debug_level=3" \ + "$G_CLI --priority=NORMAL:-VERS-ALL:+VERS-TLS1.2:+COMP-DEFLATE localhost" \ + 0 \ + -c "Handshake was completed" \ + -s "dumping .client hello, compression. (2 bytes)" # Test heap memory usage after handshake requires_config_enabled MBEDTLS_SSL_PROTO_TLS1_2 From 835483335e34d33563b9b4644730dca497c338a3 Mon Sep 17 00:00:00 2001 From: Waleed Elmelegy Date: Tue, 16 Jul 2024 10:32:03 +0000 Subject: [PATCH 07/10] Remove redundant legacy compression test Signed-off-by: Waleed Elmelegy --- tests/ssl-opt.sh | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index 7a35c43d40..1c969e4e92 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -14137,17 +14137,6 @@ run_test "TLS 1.3: no HRR in case of PSK key exchange mode" \ # Legacy_compression_methods testing -requires_gnutls -requires_config_enabled MBEDTLS_SSL_SRV_C -requires_config_enabled MBEDTLS_SSL_PROTO_TLS1_2 -requires_config_enabled MBEDTLS_SSL_PROTO_TLS1_3 -run_test "TLS 1.2 ClientHello indicating support for deflate compression method (fallback from TLS 1.3)" \ - "$P_SRV debug_level=3" \ - "$G_CLI --priority=NORMAL:-VERS-ALL:+VERS-TLS1.2:+COMP-DEFLATE localhost" \ - 0 \ - -c "Handshake was completed" \ - -s "dumping .client hello, compression. (2 bytes)" - requires_gnutls requires_config_enabled MBEDTLS_SSL_SRV_C requires_config_enabled MBEDTLS_SSL_PROTO_TLS1_2 From b5df9d8b658c2dbd588b4a604b3574dca9bbaac3 Mon Sep 17 00:00:00 2001 From: Waleed Elmelegy Date: Thu, 22 Aug 2024 16:10:10 +0000 Subject: [PATCH 08/10] Add chanelog entry for fixing legacy comprssion methods issue Signed-off-by: Waleed Elmelegy --- ChangeLog.d/fix-legacy-compression-issue.txt | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 ChangeLog.d/fix-legacy-compression-issue.txt diff --git a/ChangeLog.d/fix-legacy-compression-issue.txt b/ChangeLog.d/fix-legacy-compression-issue.txt new file mode 100644 index 0000000000..e51ee24a9b --- /dev/null +++ b/ChangeLog.d/fix-legacy-compression-issue.txt @@ -0,0 +1,7 @@ +Bugfix + * Fix an issue where ssl_tls13_parse_client_hello() assumed legacy_compression_methods + length would always be zero, which is true for TLS 1.3. However, with TLS 1.3 enabled + by default, all ClientHello requests (including TLS 1.2 requests) are initially + processed by ssl_tls13_parse_client_hello() before being passed to the TLS 1.2 + parsing function. This caused an issue where legacy_compression_methods + might not be zero for TLS 1.2 requests, as it is processed earlier. From 65e73c88bdf76325a3b01a47fc23dd8501f3f573 Mon Sep 17 00:00:00 2001 From: Waleed Elmelegy Date: Thu, 22 Aug 2024 16:27:27 +0000 Subject: [PATCH 09/10] Improve the changelog entry for fixing legacy compression issue Signed-off-by: Waleed Elmelegy --- ChangeLog.d/fix-legacy-compression-issue.txt | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/ChangeLog.d/fix-legacy-compression-issue.txt b/ChangeLog.d/fix-legacy-compression-issue.txt index e51ee24a9b..8b2fe23369 100644 --- a/ChangeLog.d/fix-legacy-compression-issue.txt +++ b/ChangeLog.d/fix-legacy-compression-issue.txt @@ -1,7 +1,7 @@ Bugfix - * Fix an issue where ssl_tls13_parse_client_hello() assumed legacy_compression_methods - length would always be zero, which is true for TLS 1.3. However, with TLS 1.3 enabled - by default, all ClientHello requests (including TLS 1.2 requests) are initially - processed by ssl_tls13_parse_client_hello() before being passed to the TLS 1.2 - parsing function. This caused an issue where legacy_compression_methods - might not be zero for TLS 1.2 requests, as it is processed earlier. + * Fix an issue where TLS 1.2 clients who send a ClientHello message with + legacy_compression_methods get a failure in connection because TLS 1.3 + is enabled by default and the server rejects the ClientHello packet as + malformed for TLS 1.3 in a way that stops the fallback to TLS 1.2. + fixes #8995, #9243. + From 344f79bde6d3d38d20485337816b608b4ade6861 Mon Sep 17 00:00:00 2001 From: Waleed Elmelegy Date: Thu, 22 Aug 2024 16:33:17 +0000 Subject: [PATCH 10/10] Reduce the wording in changelog entry Signed-off-by: Waleed Elmelegy --- ChangeLog.d/fix-legacy-compression-issue.txt | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/ChangeLog.d/fix-legacy-compression-issue.txt b/ChangeLog.d/fix-legacy-compression-issue.txt index 8b2fe23369..2549af8733 100644 --- a/ChangeLog.d/fix-legacy-compression-issue.txt +++ b/ChangeLog.d/fix-legacy-compression-issue.txt @@ -1,7 +1,6 @@ Bugfix - * Fix an issue where TLS 1.2 clients who send a ClientHello message with - legacy_compression_methods get a failure in connection because TLS 1.3 - is enabled by default and the server rejects the ClientHello packet as - malformed for TLS 1.3 in a way that stops the fallback to TLS 1.2. + * Fixes an issue where some TLS 1.2 clients could not connect to an + Mbed TLS 3.6.0 server, due to incorrect handling of + legacy_compression_methods in the ClientHello. fixes #8995, #9243.