Mbed-TLS / mbedtls

An open source, portable, easy to use, readable and flexible TLS library, and reference implementation of the PSA Cryptography API. Releases are on a varying cadence, typically around 3 - 6 months between releases.
https://www.trustedfirmware.org/projects/mbed-tls/
Other
5.54k stars 2.6k forks source link

Fail to complete TLS handshake to httpstat.us by TLS1.3 #8669

Closed BensonLiou closed 8 months ago

BensonLiou commented 10 months ago

Summary

Received "protocol version (70)" error from server while connecting by ssl_client2

System information

Mbed TLS version (number or commit id): 593e9cb60062df6af46a25dcb105bc96be92f158 Operating system and version: WLS2 ubuntu (5.15.90.1-microsoft-standard-WSL2) Configuration (if not default, please attach mbedtls_config.h):

diff --git a/include/mbedtls/mbedtls_config.h b/include/mbedtls/mbedtls_config.h
index a3e3f8347..e19030589 100644
--- a/include/mbedtls/mbedtls_config.h
+++ b/include/mbedtls/mbedtls_config.h
@@ -1774,7 +1774,7 @@
  *
  * Uncomment this macro to enable the support for TLS 1.3.
  */
-//#define MBEDTLS_SSL_PROTO_TLS1_3
+#define MBEDTLS_SSL_PROTO_TLS1_3

 /**
  * \def MBEDTLS_SSL_TLS1_3_COMPATIBILITY_MODE
@@ -1796,7 +1796,7 @@
  * effect on the build.
  *
  */
-//#define MBEDTLS_SSL_TLS1_3_COMPATIBILITY_MODE
+#define MBEDTLS_SSL_TLS1_3_COMPATIBILITY_MODE

Compiler and options (if you used a pre-built binary, please indicate how you obtained it): gcc version 11.4.0 (Ubuntu 11.4.0-1ubuntu1~22.04) Additional environment information: NA

Expected behavior

Complete the SSL handshake

Actual behavior

Received "protocol version (70)" error from server

Steps to reproduce

execute programs/ssl/ssl_client2 server_name=httpstat.us server_port=443 auth_mode=none force_version=tls13

Additional information

There is no issue on openssl. I could connect to same server by openssl s_client -connect httpstat.us:443 -tls1_3

I found the random number is different in two ClientHello. According to RFC8446, two ClientHello have to be same. I could fix this issue with following patch

diff --git a/library/ssl_client.c b/library/ssl_client.c
index 7a7840662..d3303057a 100644
--- a/library/ssl_client.c
+++ b/library/ssl_client.c
@@ -802,10 +802,13 @@ static int ssl_prepare_client_hello(mbedtls_ssl_context *ssl)
         (ssl->handshake->cookie == NULL))
 #endif
     {
-        ret = ssl_generate_random(ssl);
-        if (ret != 0) {
-            MBEDTLS_SSL_DEBUG_RET(1, "Random bytes generation failed", ret);
-            return ret;
+        if (ssl->handshake->hello_retry_request_count == 0)
+        {
+            ret = ssl_generate_random(ssl);
+            if (ret != 0) {
+                MBEDTLS_SSL_DEBUG_RET(1, "Random bytes generation failed", ret);
+                return ret;
+            }
         }
     }

Please check if this patch is good enough

ronald-cron-arm commented 10 months ago

Thanks for the report. The random bytes in the second ClientHello should indeed be the same as the ones in the first ClientHello. We have to fix that. The proposed patch seems to go on the right direction but probably misses some "#if defined(MBEDTLS_SSL_PROTO_TLS1_3)" guard as the hello_retry_request_count field does not exist if TLS 1.3 is not enabled in the build.

BensonLiou commented 10 months ago

Thanks for the suggestion. Will you create a PR or I should do it?

tom-cosgrove-arm commented 10 months ago

If you would like to create a PR it would be welcome