From f3061359d848b1283728a0b23c8f2245841749df Mon Sep 17 00:00:00 2001 From: Lealem Amedie Date: Thu, 6 Jun 2024 12:22:50 -0600 Subject: [PATCH 1/4] Improved fix for TLS1.3 to TLS1.2 client downgrade --- src/tls13.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/src/tls13.c b/src/tls13.c index 56de461f7c..ec41cf9b38 100644 --- a/src/tls13.c +++ b/src/tls13.c @@ -5023,6 +5023,7 @@ typedef struct Dsh13Args { int DoTls13ServerHello(WOLFSSL* ssl, const byte* input, word32* inOutIdx, word32 helloSz, byte* extMsgType) { + word32 inOutIdxCopy; int ret; byte suite[2]; byte tls12minor; @@ -5298,13 +5299,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 } } @@ -5359,6 +5353,7 @@ int DoTls13ServerHello(WOLFSSL* ssl, const byte* input, word32* inOutIdx, DtlsCIDOnExtensionsParsed(ssl); #endif /* WOLFSSL_DTLS_CID */ + inOutIdxCopy = *inOutIdx; *inOutIdx = args->idx; ssl->options.serverState = SERVER_HELLO_COMPLETE; @@ -5403,8 +5398,9 @@ 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, &inOutIdxCopy, helloSz); #else + (void)inOutIdxCopy; WOLFSSL_MSG("Client using higher version, fatal error"); WOLFSSL_ERROR_VERBOSE(VERSION_ERROR); ret = VERSION_ERROR; From 7cc0ac14c43c6930b7c17ac8f5c3023460c62e11 Mon Sep 17 00:00:00 2001 From: Lealem Amedie Date: Thu, 6 Jun 2024 13:24:07 -0600 Subject: [PATCH 2/4] Adding test case --- tests/api.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 62 insertions(+) diff --git a/tests/api.c b/tests/api.c index 1025116302..a4de0811a0 100644 --- a/tests/api.c +++ b/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), From 3de358ef0642711f71a4653db98e8bd6fe39d5c3 Mon Sep 17 00:00:00 2001 From: Lealem Amedie Date: Thu, 6 Jun 2024 14:10:56 -0600 Subject: [PATCH 3/4] Ensure extensions are only parsed once --- src/tls13.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/tls13.c b/src/tls13.c index ec41cf9b38..85b76b04e5 100644 --- a/src/tls13.c +++ b/src/tls13.c @@ -5324,8 +5324,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) { @@ -5344,7 +5345,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; } From 5a1ac2742c6784006d7ef76e47c4638e4387a67e Mon Sep 17 00:00:00 2001 From: Lealem Amedie Date: Thu, 6 Jun 2024 16:08:39 -0600 Subject: [PATCH 4/4] Reviewer feedback --- src/tls13.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/tls13.c b/src/tls13.c index 85b76b04e5..c7c370abd0 100644 --- a/src/tls13.c +++ b/src/tls13.c @@ -5023,7 +5023,6 @@ typedef struct Dsh13Args { int DoTls13ServerHello(WOLFSSL* ssl, const byte* input, word32* inOutIdx, word32 helloSz, byte* extMsgType) { - word32 inOutIdxCopy; int ret; byte suite[2]; byte tls12minor; @@ -5356,8 +5355,9 @@ int DoTls13ServerHello(WOLFSSL* ssl, const byte* input, word32* inOutIdx, DtlsCIDOnExtensionsParsed(ssl); #endif /* WOLFSSL_DTLS_CID */ - inOutIdxCopy = *inOutIdx; - *inOutIdx = args->idx; + if (IsAtLeastTLSv1_3(ssl->version)) { + *inOutIdx = args->idx; + } ssl->options.serverState = SERVER_HELLO_COMPLETE; @@ -5401,9 +5401,8 @@ int DoTls13ServerHello(WOLFSSL* ssl, const byte* input, word32* inOutIdx, else ssl->chVersion.minor = TLSv1_2_MINOR; /* Complete TLS v1.2 processing of ServerHello. */ - ret = DoServerHello(ssl, input, &inOutIdxCopy, helloSz); + ret = DoServerHello(ssl, input, inOutIdx, helloSz); #else - (void)inOutIdxCopy; WOLFSSL_MSG("Client using higher version, fatal error"); WOLFSSL_ERROR_VERBOSE(VERSION_ERROR); ret = VERSION_ERROR;