Merge pull request #7626 from lealem47/parseServerHello

Improved fix for TLS1.3 to TLS1.2 client downgrade

Author: JacobBarthelmeh

src/tls13.c

@@ -5298,13 +5298,6 @@ int DoTls13ServerHello(WOLFSSL* ssl, const byte* input, word32* inOutIdx,
                     return ret;
             }
 #endif /* WOLFSSL_DTLS13 */
-
-#ifndef WOLFSSL_NO_TLS12
-            return DoServerHello(ssl, input, inOutIdx, helloSz);
-#else
-            SendAlert(ssl, alert_fatal, wolfssl_alert_protocol_version);
-            return VERSION_ERROR;
-#endif
         }
     }
 
@@ -5330,8 +5323,9 @@ int DoTls13ServerHello(WOLFSSL* ssl, const byte* input, word32* inOutIdx,
     /* restore message type */
     *extMsgType = args->extMsgType;
 
-    if (args->totalExtSz > 0) {
-        /* Parse and handle extensions. */
+    /* Parse and handle extensions, unless lower than TLS1.3. In that case,
+     * extensions will be parsed in DoServerHello. */
+    if (args->totalExtSz > 0 && IsAtLeastTLSv1_3(ssl->version)) {
         ret = TLSX_Parse(ssl, input + args->idx, args->totalExtSz,
             *extMsgType, NULL);
         if (ret != 0) {
@@ -5350,7 +5344,9 @@ int DoTls13ServerHello(WOLFSSL* ssl, const byte* input, word32* inOutIdx,
             ssl->msgsReceived.got_hello_retry_request = 1;
             ssl->msgsReceived.got_server_hello = 0;
         }
+    }
 
+    if (args->totalExtSz > 0) {
         args->idx += args->totalExtSz;
     }
 
@@ -5359,7 +5355,9 @@ int DoTls13ServerHello(WOLFSSL* ssl, const byte* input, word32* inOutIdx,
         DtlsCIDOnExtensionsParsed(ssl);
 #endif /* WOLFSSL_DTLS_CID */
 
-    *inOutIdx = args->idx;
+    if (IsAtLeastTLSv1_3(ssl->version)) {
+        *inOutIdx = args->idx;
+    }
 
     ssl->options.serverState = SERVER_HELLO_COMPLETE;
 
@@ -5403,7 +5401,7 @@ int DoTls13ServerHello(WOLFSSL* ssl, const byte* input, word32* inOutIdx,
         else
             ssl->chVersion.minor = TLSv1_2_MINOR;
         /* Complete TLS v1.2 processing of ServerHello. */
-        ret = CompleteServerHello(ssl);
+        ret = DoServerHello(ssl, input, inOutIdx, helloSz);
 #else
         WOLFSSL_MSG("Client using higher version, fatal error");
         WOLFSSL_ERROR_VERBOSE(VERSION_ERROR);

tests/api.c

@@ -68416,6 +68416,67 @@ static int test_extra_alerts_wrong_cs(void)
 }
 #endif
 
+#if defined(WOLFSSL_TLS13) && !defined(WOLFSSL_NO_TLS12) &&   \
+    defined(HAVE_MANUAL_MEMIO_TESTS_DEPENDENCIES)
+
+#define TEST_CS_DOWNGRADE_CLIENT "ECDHE-RSA-AES256-GCM-SHA384"
+
+byte test_wrong_cs_downgrade_sh[] = {
+  0x16, 0x03, 0x03, 0x00, 0x56, 0x02, 0x00, 0x00, 0x52, 0x03, 0x03, 0x10,
+  0x2c, 0x88, 0xd9, 0x7a, 0x23, 0xc9, 0xbd, 0x11, 0x3b, 0x64, 0x24, 0xab,
+  0x5b, 0x45, 0x33, 0xf6, 0x2c, 0x34, 0xe4, 0xcf, 0xf4, 0x78, 0xc8, 0x62,
+  0x06, 0xc7, 0xe5, 0x30, 0x39, 0xbf, 0xa1, 0x20, 0xa3, 0x06, 0x74, 0xc3,
+  0xa9, 0x74, 0x52, 0x8a, 0xfb, 0xae, 0xf0, 0xd8, 0x6f, 0xb2, 0x9d, 0xfe,
+  0x78, 0xf0, 0x3f, 0x51, 0x8f, 0x9c, 0xcf, 0xbe, 0x61, 0x43, 0x9d, 0xf8,
+  0x85, 0xe5, 0x2f, 0x54,
+  0xc0, 0x2f, /* ECDHE-RSA-AES128-GCM-SHA256 */
+  0x00, 0x00, 0x0a, 0x00, 0x0b, 0x00,
+  0x02, 0x01, 0x00, 0x00, 0x17, 0x00, 0x00
+};
+
+static int test_wrong_cs_downgrade(void)
+{
+    EXPECT_DECLS;
+#ifdef BUILD_TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256
+    struct test_memio_ctx test_ctx;
+    WOLFSSL_CTX *ctx_c = NULL;
+    WOLFSSL *ssl_c = NULL;
+
+    XMEMSET(&test_ctx, 0, sizeof(test_ctx));
+    ExpectIntEQ(test_memio_setup(&test_ctx, &ctx_c, NULL, &ssl_c, NULL,
+        wolfSSLv23_client_method, NULL), 0);
+
+    ExpectIntEQ(wolfSSL_set_cipher_list(ssl_c, TEST_CS_DOWNGRADE_CLIENT),
+        WOLFSSL_SUCCESS);
+
+    /* CH */
+    ExpectIntNE(wolfSSL_connect(ssl_c), WOLFSSL_SUCCESS);
+    ExpectIntEQ(wolfSSL_get_error(ssl_c, WOLFSSL_FATAL_ERROR),
+        WOLFSSL_ERROR_WANT_READ);
+
+    /* consume CH */
+    test_ctx.s_len = 0;
+    /* inject SH */
+    XMEMCPY(test_ctx.c_buff, test_wrong_cs_downgrade_sh,
+        sizeof(test_wrong_cs_downgrade_sh));
+    test_ctx.c_len = sizeof(test_wrong_cs_downgrade_sh);
+
+    ExpectIntNE(wolfSSL_connect(ssl_c), WOLFSSL_SUCCESS);
+    ExpectIntNE(wolfSSL_get_error(ssl_c, WOLFSSL_FATAL_ERROR),
+        WOLFSSL_ERROR_WANT_READ);
+
+    wolfSSL_free(ssl_c);
+    wolfSSL_CTX_free(ctx_c);
+#endif
+    return EXPECT_RESULT();
+}
+#else
+static int test_wrong_cs_downgrade(void)
+{
+    return TEST_SKIPPED;
+}
+#endif
+
 #if !defined(WOLFSSL_NO_TLS12) && defined(WOLFSSL_EXTRA_ALERTS) &&             \
     defined(HAVE_MANUAL_MEMIO_TESTS_DEPENDENCIES) && !defined(WOLFSSL_SP_MATH)
 
@@ -74020,6 +74081,7 @@ TEST_CASE testCases[] = {
     TEST_DECL(test_ticket_nonce_malloc),
 #endif
     TEST_DECL(test_ticket_ret_create),
+    TEST_DECL(test_wrong_cs_downgrade),
     TEST_DECL(test_extra_alerts_wrong_cs),
     TEST_DECL(test_extra_alerts_skip_hs),
     TEST_DECL(test_extra_alerts_bad_psk),