Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 4 commits into from
Jun 6, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f306135
Improved fix for TLS1.3 to TLS1.2 client downgrade
lealem47 Jun 6, 2024
7cc0ac1
Adding test case
lealem47 Jun 6, 2024
3de358e
Ensure extensions are only parsed once
lealem47 Jun 6, 2024
5a1ac27
Reviewer feedback
lealem47 Jun 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 9 additions & 11 deletions src/tls13.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}

Expand All @@ -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) {
Expand All @@ -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) {
JacobBarthelmeh marked this conversation as resolved.
Show resolved Hide resolved
args->idx += args->totalExtSz;
}

Expand All @@ -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;

Expand Down Expand Up @@ -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);
Expand Down
62 changes: 62 additions & 0 deletions tests/api.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down Expand Up @@ -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),
Expand Down