From 41e0cdf8c16f255a8ecf7a49a6342598e580d5cf Mon Sep 17 00:00:00 2001 From: Waleed Elmelegy Date: Tue, 11 Jun 2024 18:44:48 +0100 Subject: [PATCH 01/11] 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 566ed54d6eed952e5be0b2368ee368963c524f22 Mon Sep 17 00:00:00 2001 From: Waleed Elmelegy Date: Wed, 19 Jun 2024 15:09:48 +0100 Subject: [PATCH 02/11] 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 3918598e52484f4dcaf84b787f8448d1eb22e5b0 Mon Sep 17 00:00:00 2001 From: Waleed Elmelegy Date: Tue, 25 Jun 2024 10:22:49 +0100 Subject: [PATCH 03/11] 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 a1c4f4cab65c6a24de03570d794909e6a8753e5f Mon Sep 17 00:00:00 2001 From: Waleed Elmelegy Date: Tue, 25 Jun 2024 18:16:16 +0100 Subject: [PATCH 04/11] 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 790f3b16d42711d541589d729de2e26bbdcf7ec8 Mon Sep 17 00:00:00 2001 From: Waleed Elmelegy Date: Thu, 4 Jul 2024 16:38:04 +0000 Subject: [PATCH 05/11] 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 0b8f129048..fbf7f32b99 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -14142,6 +14142,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 38c8757b2c0de6bab622aaaf8b3dbc9676caf0d3 Mon Sep 17 00:00:00 2001 From: Waleed Elmelegy Date: Mon, 15 Jul 2024 17:25:04 +0000 Subject: [PATCH 06/11] 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 fbf7f32b99..c3891edea2 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -14145,13 +14145,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 1297309fdb3bcac9ea1608b88fe5f12e82dfcd8a Mon Sep 17 00:00:00 2001 From: Waleed Elmelegy Date: Tue, 16 Jul 2024 10:32:03 +0000 Subject: [PATCH 07/11] 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 c3891edea2..216bbd061b 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -14144,17 +14144,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 f669fef85600989c0426dd69dea5a45a986d3084 Mon Sep 17 00:00:00 2001 From: Waleed Elmelegy Date: Thu, 22 Aug 2024 16:10:10 +0000 Subject: [PATCH 08/11] 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 5183e1ab17eceb2c7a07c3ded11deab26f3a1423 Mon Sep 17 00:00:00 2001 From: Waleed Elmelegy Date: Thu, 22 Aug 2024 16:27:27 +0000 Subject: [PATCH 09/11] 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 d930a3e9506d7bb0a25437210647ada3bdd882d6 Mon Sep 17 00:00:00 2001 From: Waleed Elmelegy Date: Thu, 22 Aug 2024 16:33:17 +0000 Subject: [PATCH 10/11] 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. From 8ac9caf89be14396a7bbe7133fb1d97096f71b1c Mon Sep 17 00:00:00 2001 From: Waleed Elmelegy Date: Thu, 22 Aug 2024 16:42:18 +0000 Subject: [PATCH 11/11] Fix the capitalisation in the changelog entry Signed-off-by: Waleed Elmelegy --- ChangeLog.d/fix-legacy-compression-issue.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ChangeLog.d/fix-legacy-compression-issue.txt b/ChangeLog.d/fix-legacy-compression-issue.txt index 2549af8733..b77e7a44f0 100644 --- a/ChangeLog.d/fix-legacy-compression-issue.txt +++ b/ChangeLog.d/fix-legacy-compression-issue.txt @@ -2,5 +2,5 @@ Bugfix * 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. + Fixes #8995, #9243.