From f6ef14614997ff6822119694c949e51a41a6f398 Mon Sep 17 00:00:00 2001 From: Juliusz Sosinowicz Date: Tue, 16 Jan 2024 12:54:07 +0100 Subject: [PATCH] EarlySanityCheckMsgReceived: version_negotiated should always be checked Multiple handshake messages in one record will fail the MsgCheckBoundary() check on the client side when the client is set to TLS 1.3 but allows downgrading. --> ClientHello <-- ServerHello + rest of TLS 1.2 flight Client returns OUT_OF_ORDER_E because in TLS 1.3 the ServerHello has to be the last message in a record. In TLS 1.2 the ServerHello can be in the same record as the rest of the server's first flight. --- src/internal.c | 13 +++---------- tests/api.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 52 insertions(+), 10 deletions(-) diff --git a/src/internal.c b/src/internal.c index 31334eb43..186988957 100644 --- a/src/internal.c +++ b/src/internal.c @@ -10971,18 +10971,11 @@ int EarlySanityCheckMsgReceived(WOLFSSL* ssl, byte type, word32 msgSz) { int ret = 0; #ifndef WOLFSSL_DISABLE_EARLY_SANITY_CHECKS - byte version_negotiated = 0; - - WOLFSSL_ENTER("EarlySanityCheckMsgReceived"); - -#ifdef WOLFSSL_DTLS /* Version has only been negotiated after we either send or process a * ServerHello message */ - if (ssl->options.dtls) - version_negotiated = ssl->options.serverState >= SERVER_HELLO_COMPLETE; - else -#endif - version_negotiated = 1; + byte version_negotiated = ssl->options.serverState >= SERVER_HELLO_COMPLETE; + + WOLFSSL_ENTER("EarlySanityCheckMsgReceived"); if (version_negotiated) ret = MsgCheckEncryption(ssl, type, ssl->keys.decryptedCur == 1); diff --git a/tests/api.c b/tests/api.c index df8584bc8..d941a02b1 100644 --- a/tests/api.c +++ b/tests/api.c @@ -68852,6 +68852,54 @@ static int test_self_signed_stapling(void) return EXPECT_RESULT(); } +static int test_tls_multi_handshakes_one_record(void) +{ + EXPECT_DECLS; +#if defined(HAVE_MANUAL_MEMIO_TESTS_DEPENDENCIES) && !defined(WOLFSSL_NO_TLS12) + struct test_memio_ctx test_ctx; + WOLFSSL_CTX *ctx_c = NULL, *ctx_s = NULL; + WOLFSSL *ssl_c = NULL, *ssl_s = NULL; + int newRecIdx = RECORD_HEADER_SZ; + int idx = 0; + + XMEMSET(&test_ctx, 0, sizeof(test_ctx)); + + ExpectIntEQ(test_memio_setup(&test_ctx, &ctx_c, &ctx_s, &ssl_c, &ssl_s, + wolfTLS_client_method, wolfTLSv1_2_server_method), 0); + + ExpectIntEQ(wolfSSL_connect(ssl_c), -1); + ExpectIntEQ(wolfSSL_get_error(ssl_c, -1), WOLFSSL_ERROR_WANT_READ); + ExpectIntEQ(wolfSSL_accept(ssl_s), -1); + ExpectIntEQ(wolfSSL_get_error(ssl_s, -1), WOLFSSL_ERROR_WANT_READ); + + /* Combine server handshake msgs into one record */ + while (idx < test_ctx.c_len) { + word16 recLen; + + ato16(((RecordLayerHeader*)(test_ctx.c_buff + idx))->length, &recLen); + idx += RECORD_HEADER_SZ; + + XMEMMOVE(test_ctx.c_buff + newRecIdx, test_ctx.c_buff + idx, + (size_t)recLen); + + newRecIdx += recLen; + idx += recLen; + } + c16toa(newRecIdx - RECORD_HEADER_SZ, + ((RecordLayerHeader*)test_ctx.c_buff)->length); + test_ctx.c_len = newRecIdx; + + ExpectIntEQ(wolfSSL_connect(ssl_c), -1); + ExpectIntEQ(wolfSSL_get_error(ssl_c, -1), WOLFSSL_ERROR_WANT_READ); + + wolfSSL_free(ssl_c); + wolfSSL_free(ssl_s); + wolfSSL_CTX_free(ctx_c); + wolfSSL_CTX_free(ctx_s); +#endif + return EXPECT_RESULT(); +} + /*----------------------------------------------------------------------------* | Main *----------------------------------------------------------------------------*/ @@ -70152,6 +70200,7 @@ TEST_CASE testCases[] = { TEST_DECL(test_dtls_empty_keyshare_with_cookie), TEST_DECL(test_tls13_pq_groups), TEST_DECL(test_tls13_early_data), + TEST_DECL(test_tls_multi_handshakes_one_record), /* This test needs to stay at the end to clean up any caches allocated. */ TEST_DECL(test_wolfSSL_Cleanup) };