mbedtls icon indicating copy to clipboard operation
mbedtls copied to clipboard

PSK check in mbedtls_ssl_tls13_key_schedule_stage_early

Open lhuang04 opened this issue 2 years ago • 3 comments

Suggested enhancement

I found one issue when we tested the following early data fallback 1-RTT test case:

  • Client completes the initial full handshake with the server and receives NewSessionTicket that supports early data.
  • Client initiates early data handshake but server rejects early data. We mocked it by faking the NewSessionTicket.
  • We expect the client can fallback to 1-RTT and continue to have a full-handshake with the server.
  • However we get MBEDTLS_ERR_SSL_INTERNAL_ERROR from mbedtls_ssl_tls13_key_schedule_stage_early when ssl_tls13_postprocess_server_hello processes the second server hello that rejects early data.

Looking into the code, the psk will be clear out after ssl_tls13_write_early_data_postprocess. And psk may be set again in ssl_tls13_parse_server_pre_shared_key_ext only when the server accepts the psk.

So for this test case, it sounds reasonable that psk is not set when we call mbedtls_ssl_tls13_key_schedule_stage_early for the 2nd server hello that rejects the early data. And we should allow such a case, and treat it as if there is no psk, and continue the handshake. It seems be the case in the old logic of tls13-prototype.

Here is our local patch to work around the issue by restoring old logic.

diff --git a/library/ssl_tls13_keys.c b/library/ssl_tls13_keys.c
--- a/library/ssl_tls13_keys.c
+++ b/library/ssl_tls13_keys.c
@@ -1300,21 +1300,12 @@
     hash_alg = handshake->ciphersuite_info->mac;
 #endif /* MBEDTLS_USE_PSA_CRYPTO */

-
 #if defined(MBEDTLS_SSL_TLS1_3_KEY_EXCHANGE_MODE_SOME_PSK_ENABLED)
-    if( mbedtls_ssl_tls13_key_exchange_mode_with_psk( ssl )             ||
-#if defined(MBEDTLS_ZERO_RTT)
-        ssl->handshake->early_data == MBEDTLS_SSL_EARLY_DATA_ON ||
-#endif
-        0 )
+    ret = mbedtls_ssl_tls13_export_handshake_psk( ssl, &psk, &psk_len );
+    if( ret != 0 )
     {
-        ret = mbedtls_ssl_tls13_export_handshake_psk( ssl, &psk, &psk_len );
-        if( ret != 0 )
-        {
-            MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_ssl_tls13_export_handshake_psk",
-                                   ret );
-            return( ret );
-        }
+        psk = NULL;
+        psk_len = 0;
     }
 #endif

cc @ronald-cron-arm , @yuhaoth

Justification

Mbed TLS needs this because

lhuang04 avatar Jan 13 '23 14:01 lhuang04