From 57e53d1a438ecbe721e5bb4e6bb05dfcfddef199 Mon Sep 17 00:00:00 2001 From: Juliusz Sosinowicz Date: Thu, 29 Jun 2023 15:49:32 +0200 Subject: [PATCH 1/2] Don't allow a resumption handshake inside of a SCR --- src/internal.c | 44 +++++++++++++++++++++---- src/ssl.c | 9 ++++- src/tls.c | 7 ++++ tests/api.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++ wolfssl/internal.h | 3 ++ 5 files changed, 137 insertions(+), 8 deletions(-) diff --git a/src/internal.c b/src/internal.c index 13360f93a..e9d8d1287 100644 --- a/src/internal.c +++ b/src/internal.c @@ -7387,7 +7387,7 @@ int InitSSL(WOLFSSL* ssl, WOLFSSL_CTX* ctx, int writeDup) ret = wolfSSL_UseSecureRenegotiation(ssl); if (ret != WOLFSSL_SUCCESS) return ret; - } + } } #endif /* HAVE_SECURE_RENEGOTIATION */ @@ -15400,6 +15400,9 @@ int DoFinished(WOLFSSL* ssl, const byte* input, word32* inOutIdx, word32 size, #endif ssl->options.handShakeState = HANDSHAKE_DONE; ssl->options.handShakeDone = 1; +#ifdef HAVE_SECURE_RENEGOTIATION + ssl->options.resumed = ssl->options.resuming; +#endif } } else { @@ -15416,6 +15419,9 @@ int DoFinished(WOLFSSL* ssl, const byte* input, word32* inOutIdx, word32 size, #endif ssl->options.handShakeState = HANDSHAKE_DONE; ssl->options.handShakeDone = 1; +#ifdef HAVE_SECURE_RENEGOTIATION + ssl->options.resumed = ssl->options.resuming; +#endif } } #ifdef WOLFSSL_DTLS @@ -15965,8 +15971,10 @@ static int DoHandShakeMsgType(WOLFSSL* ssl, byte* input, word32* inOutIdx, } if (ssl->options.side == WOLFSSL_CLIENT_END && ssl->options.dtls == 0 && - ssl->options.serverState == NULL_STATE && type != server_hello) { - WOLFSSL_MSG("First server message not server hello"); + ssl->options.serverState == NULL_STATE && type != server_hello && + type != hello_request) { + WOLFSSL_MSG("First server message not server hello or " + "hello request"); SendAlert(ssl, alert_fatal, unexpected_message); WOLFSSL_ERROR_VERBOSE(OUT_OF_ORDER_E); return OUT_OF_ORDER_E; @@ -21917,6 +21925,9 @@ int SendFinished(WOLFSSL* ssl) #endif ssl->options.handShakeState = HANDSHAKE_DONE; ssl->options.handShakeDone = 1; +#ifdef HAVE_SECURE_RENEGOTIATION + ssl->options.resumed = ssl->options.resuming; +#endif } } else { @@ -21929,6 +21940,9 @@ int SendFinished(WOLFSSL* ssl) #endif ssl->options.handShakeState = HANDSHAKE_DONE; ssl->options.handShakeDone = 1; +#ifdef HAVE_SECURE_RENEGOTIATION + ssl->options.resumed = ssl->options.resuming; +#endif } } @@ -27130,13 +27144,20 @@ static int HashSkeData(WOLFSSL* ssl, enum wc_HashType hashType, return BAD_FUNC_ARG; } - idSz = ssl->options.resuming ? ssl->session->sessionIDSz : 0; - #ifdef WOLFSSL_TLS13 if (IsAtLeastTLSv1_3(ssl->version)) return SendTls13ClientHello(ssl); #endif +#ifdef HAVE_SECURE_RENEGOTIATION + /* We don't want to resume in SCR */ + if (IsSCR(ssl)) + ssl->options.resuming = 0; +#endif + + idSz = ssl->options.resuming ? ssl->session->sessionIDSz : 0; + + WOLFSSL_START(WC_FUNC_CLIENT_HELLO_SEND); WOLFSSL_ENTER("SendClientHello"); @@ -34297,6 +34318,9 @@ static int DoSessionTicket(WOLFSSL* ssl, const byte* input, word32* inOutIdx, ssl->options.dtlsStateful = 1; #endif /* WOLFSSL_DTLS */ + /* Reset to sane value for SCR */ + ssl->options.resuming = 0; + /* protocol version, random and session id length check */ if (OPAQUE16_LEN + RAN_LEN + OPAQUE8_LEN > helloSz) return BUFFER_ERROR; @@ -34490,7 +34514,7 @@ static int DoSessionTicket(WOLFSSL* ssl, const byte* input, word32* inOutIdx, ret = BUFFER_ERROR; /* session ID greater than 32 bytes long */ goto out; } - else if (b > 0) { + else if (b > 0 && !IsSCR(ssl)) { if ((i - begin) + b > helloSz) { ret = BUFFER_ERROR; goto out; @@ -34503,8 +34527,14 @@ static int DoSessionTicket(WOLFSSL* ssl, const byte* input, word32* inOutIdx, if (b == ID_LEN) ssl->options.resuming = 1; /* client wants to resume */ WOLFSSL_MSG("Client wants to resume session"); - i += b; } +#ifdef HAVE_SECURE_RENEGOTIATION + else { + /* We don't want to resume in SCR */ + ssl->arrays->sessionIDSz = 0; + } +#endif + i += b; #ifdef WOLFSSL_DTLS /* cookie */ diff --git a/src/ssl.c b/src/ssl.c index 917f47f65..7c5012929 100644 --- a/src/ssl.c +++ b/src/ssl.c @@ -4103,6 +4103,8 @@ int wolfSSL_Rehandshake(WOLFSSL* ssl) if (ssl->options.side == WOLFSSL_SERVER_END) { /* Reset option to send certificate verify. */ ssl->options.sendVerify = 0; + /* Reset resuming flag to do full secure handshake. */ + ssl->options.resuming = 0; } else { /* Reset resuming flag to do full secure handshake. */ @@ -21328,8 +21330,13 @@ int wolfSSL_session_reused(WOLFSSL* ssl) { int resuming = 0; WOLFSSL_ENTER("wolfSSL_session_reused"); - if (ssl) + if (ssl) { +#ifndef HAVE_SECURE_RENEGOTIATION resuming = ssl->options.resuming; +#else + resuming = ssl->options.resuming || ssl->options.resumed; +#endif + } WOLFSSL_LEAVE("wolfSSL_session_reused", resuming); return resuming; } diff --git a/src/tls.c b/src/tls.c index 442a66535..50c640ac7 100644 --- a/src/tls.c +++ b/src/tls.c @@ -5379,6 +5379,13 @@ static int TLSX_SessionTicket_Parse(WOLFSSL* ssl, const byte* input, return 0; } +#ifdef HAVE_SECURE_RENEGOTIATION + if (IsSCR(ssl)) { + WOLFSSL_MSG("Client sent session ticket during SCR. Ignoring."); + return 0; + } +#endif + if (length > SESSION_TICKET_LEN) { ret = BAD_TICKET_MSG_SZ; WOLFSSL_ERROR_VERBOSE(ret); diff --git a/tests/api.c b/tests/api.c index 2e0c906eb..c772fbf81 100644 --- a/tests/api.c +++ b/tests/api.c @@ -62040,6 +62040,87 @@ static int test_dtls_ipv6_check(void) } #endif +#if !defined(NO_WOLFSSL_CLIENT) && !defined(NO_WOLFSSL_SERVER) && \ + defined(HAVE_IO_TESTS_DEPENDENCIES) && defined(HAVE_SECURE_RENEGOTIATION) + +static WOLFSSL_SESSION* test_wolfSSL_SCR_after_resumption_session = NULL; + +static void test_wolfSSL_SCR_after_resumption_ctx_ready(WOLFSSL_CTX* ctx) +{ + AssertIntEQ(wolfSSL_CTX_UseSecureRenegotiation(ctx), WOLFSSL_SUCCESS); +} + +static void test_wolfSSL_SCR_after_resumption_on_result(WOLFSSL* ssl) +{ + if (test_wolfSSL_SCR_after_resumption_session == NULL) { + test_wolfSSL_SCR_after_resumption_session = wolfSSL_get1_session(ssl); + AssertNotNull(test_wolfSSL_SCR_after_resumption_session); + } + else { + char testMsg[] = "Message after SCR"; + char msgBuf[sizeof(testMsg)]; + int ret; + if (!wolfSSL_is_server(ssl)) { + AssertIntEQ(WOLFSSL_SUCCESS, + wolfSSL_set_session(ssl, + test_wolfSSL_SCR_after_resumption_session)); + } + AssertIntEQ(wolfSSL_Rehandshake(ssl), WOLFSSL_SUCCESS); + AssertIntEQ(wolfSSL_write(ssl, testMsg, sizeof(testMsg)), + sizeof(testMsg)); + ret = wolfSSL_read(ssl, msgBuf, sizeof(msgBuf)); + if (ret != sizeof(msgBuf)) /* Possibly APP_DATA_READY error. Retry. */ + ret = wolfSSL_read(ssl, msgBuf, sizeof(msgBuf)); + AssertIntEQ(ret, sizeof(msgBuf)); + } +} + +static void test_wolfSSL_SCR_after_resumption_ssl_ready(WOLFSSL* ssl) +{ + AssertIntEQ(WOLFSSL_SUCCESS, + wolfSSL_set_session(ssl, test_wolfSSL_SCR_after_resumption_session)); +} + +static int test_wolfSSL_SCR_after_resumption(void) +{ + EXPECT_DECLS; + callback_functions func_cb_client; + callback_functions func_cb_server; + + XMEMSET(&func_cb_client, 0, sizeof(func_cb_client)); + XMEMSET(&func_cb_server, 0, sizeof(func_cb_server)); + + func_cb_client.method = wolfTLSv1_2_client_method; + func_cb_client.ctx_ready = test_wolfSSL_SCR_after_resumption_ctx_ready; + func_cb_client.on_result = test_wolfSSL_SCR_after_resumption_on_result; + func_cb_server.method = wolfTLSv1_2_server_method; + func_cb_server.ctx_ready = test_wolfSSL_SCR_after_resumption_ctx_ready; + + test_wolfSSL_client_server_nofail(&func_cb_client, &func_cb_server); + + ExpectIntEQ(func_cb_client.return_code, TEST_SUCCESS); + ExpectIntEQ(func_cb_server.return_code, TEST_SUCCESS); + + func_cb_client.ssl_ready = test_wolfSSL_SCR_after_resumption_ssl_ready; + func_cb_server.on_result = test_wolfSSL_SCR_after_resumption_on_result; + + test_wolfSSL_client_server_nofail(&func_cb_client, &func_cb_server); + + ExpectIntEQ(func_cb_client.return_code, TEST_SUCCESS); + ExpectIntEQ(func_cb_server.return_code, TEST_SUCCESS); + + wolfSSL_SESSION_free(test_wolfSSL_SCR_after_resumption_session); + + return EXPECT_RESULT(); +} + +#else +static int test_wolfSSL_SCR_after_resumption(void) +{ + return TEST_SKIPPED; +} +#endif + static int test_wolfSSL_configure_args(void) { EXPECT_DECLS; @@ -63290,6 +63371,7 @@ TEST_CASE testCases[] = { /* Can't memory test as client/server hangs. */ TEST_DECL(test_dtls_msg_from_other_peer), TEST_DECL(test_dtls_ipv6_check), + TEST_DECL(test_wolfSSL_SCR_after_resumption), /* This test needs to stay at the end to clean up any caches allocated. */ TEST_DECL(test_wolfSSL_Cleanup) }; diff --git a/wolfssl/internal.h b/wolfssl/internal.h index 186c9e036..625950588 100644 --- a/wolfssl/internal.h +++ b/wolfssl/internal.h @@ -4517,6 +4517,9 @@ struct Options { word16 failNoCertxPSK:1; /* fail for no cert except with PSK */ word16 downgrade:1; /* allow downgrade of versions */ word16 resuming:1; +#ifdef HAVE_SECURE_RENEGOTIATION + word16 resumed:1; /* resuming may be reset on SCR */ +#endif word16 isPSK:1; word16 haveSessionId:1; /* server may not send */ word16 tls:1; /* using TLS ? */ From 2248140bf3dccccf7dc4330635dc2b0759bf4547 Mon Sep 17 00:00:00 2001 From: Juliusz Sosinowicz Date: Thu, 6 Jul 2023 17:25:54 +0200 Subject: [PATCH 2/2] Clear ssl->arrays->sessionIDSz at start of function --- src/internal.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/internal.c b/src/internal.c index e9d8d1287..d1973e107 100644 --- a/src/internal.c +++ b/src/internal.c @@ -34320,6 +34320,7 @@ static int DoSessionTicket(WOLFSSL* ssl, const byte* input, word32* inOutIdx, /* Reset to sane value for SCR */ ssl->options.resuming = 0; + ssl->arrays->sessionIDSz = 0; /* protocol version, random and session id length check */ if (OPAQUE16_LEN + RAN_LEN + OPAQUE8_LEN > helloSz) @@ -34528,12 +34529,6 @@ static int DoSessionTicket(WOLFSSL* ssl, const byte* input, word32* inOutIdx, ssl->options.resuming = 1; /* client wants to resume */ WOLFSSL_MSG("Client wants to resume session"); } -#ifdef HAVE_SECURE_RENEGOTIATION - else { - /* We don't want to resume in SCR */ - ssl->arrays->sessionIDSz = 0; - } -#endif i += b; #ifdef WOLFSSL_DTLS