From 7b29362d907d58e00a7a8033e1ef5faf4049740b Mon Sep 17 00:00:00 2001 From: Stefan Eissing Date: Tue, 15 Aug 2023 12:35:13 +0200 Subject: [PATCH 1/3] Updating a shared session objects needs to do copy on write --- src/internal.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/internal.c b/src/internal.c index 0ff5e9d0d4..443743a97f 100644 --- a/src/internal.c +++ b/src/internal.c @@ -31810,6 +31810,15 @@ int SendCertificateVerify(WOLFSSL* ssl) #ifdef HAVE_SESSION_TICKET int SetTicket(WOLFSSL* ssl, const byte* ticket, word32 length) { + /* If the session is shared, we need to copy-on-write */ + if (ssl->session->ref.count > 1) { + WOLFSSL_SESSION* nsession = wolfSSL_SESSION_dup(ssl->session); + if (nsession == NULL) + return MEMORY_E; + wolfSSL_FreeSession(ssl->ctx, ssl->session); + ssl->session = nsession; + } + /* Free old dynamic ticket if we already had one */ if (ssl->session->ticketLenAlloc > 0) { XFREE(ssl->session->ticket, ssl->heap, DYNAMIC_TYPE_SESSION_TICK); From 06d81f7f8fee1384a8ff76ae0a13acc5c8b42d38 Mon Sep 17 00:00:00 2001 From: Juliusz Sosinowicz Date: Wed, 16 Aug 2023 17:11:38 +0200 Subject: [PATCH 2/3] Add a test case that negotiates tickets during another handshake --- tests/api.c | 94 +++++++++++++++++++++++++++++++++++++++++++++++++++ tests/utils.h | 4 +++ 2 files changed, 98 insertions(+) diff --git a/tests/api.c b/tests/api.c index 3cd00f20f7..a489404d0c 100644 --- a/tests/api.c +++ b/tests/api.c @@ -5855,7 +5855,9 @@ static int test_ssl_memio_do_handshake(test_ssl_memio_ctx* ctx, int max_rounds, } while ((!handshake_complete) && (max_rounds > 0)) { if (!hs_c) { + wolfSSL_SetLoggingPrefix("client"); ret = wolfSSL_connect(ctx->c_ssl); + wolfSSL_SetLoggingPrefix(NULL); if (ret == WOLFSSL_SUCCESS) { hs_c = 1; } @@ -5872,7 +5874,9 @@ static int test_ssl_memio_do_handshake(test_ssl_memio_ctx* ctx, int max_rounds, } } if (!hs_s) { + wolfSSL_SetLoggingPrefix("server"); ret = wolfSSL_accept(ctx->s_ssl); + wolfSSL_SetLoggingPrefix(NULL); if (ret == WOLFSSL_SUCCESS) { hs_s = 1; } @@ -5921,7 +5925,9 @@ static int test_ssl_memio_read_write(test_ssl_memio_ctx* ctx) msglen_s = ctx->s_msglen; } + wolfSSL_SetLoggingPrefix("client"); ExpectIntEQ(wolfSSL_write(ctx->c_ssl, msg_c, msglen_c), msglen_c); + wolfSSL_SetLoggingPrefix("server"); ExpectIntGT(idx = wolfSSL_read(ctx->s_ssl, input, sizeof(input) - 1), 0); if (idx >= 0) { input[idx] = '\0'; @@ -5929,7 +5935,9 @@ static int test_ssl_memio_read_write(test_ssl_memio_ctx* ctx) ExpectIntGT(fprintf(stderr, "Client message: %s\n", input), 0); ExpectIntEQ(wolfSSL_write(ctx->s_ssl, msg_s, msglen_s), msglen_s); ctx->s_cb.return_code = EXPECT_RESULT(); + wolfSSL_SetLoggingPrefix("client"); ExpectIntGT(idx = wolfSSL_read(ctx->c_ssl, input, sizeof(input) - 1), 0); + wolfSSL_SetLoggingPrefix(NULL); if (idx >= 0) { input[idx] = '\0'; } @@ -64352,6 +64360,91 @@ static int test_session_ticket_no_id(void) } #endif +static int test_session_ticket_hs_update(void) +{ + EXPECT_DECLS; +#if defined(HAVE_MANUAL_MEMIO_TESTS_DEPENDENCIES) && defined(WOLFSSL_TLS13) && \ + defined(HAVE_SESSION_TICKET) && !defined(WOLFSSL_NO_DEF_TICKET_ENC_CB) + struct test_memio_ctx test_ctx; + struct test_memio_ctx test_ctx2; + struct test_memio_ctx test_ctx3; + WOLFSSL_CTX *ctx_c = NULL; + WOLFSSL_CTX *ctx_s = NULL; + WOLFSSL *ssl_c = NULL; + WOLFSSL *ssl_c2 = NULL; + WOLFSSL *ssl_c3 = NULL; + WOLFSSL *ssl_s = NULL; + WOLFSSL *ssl_s2 = NULL; + WOLFSSL *ssl_s3 = NULL; + WOLFSSL_SESSION *sess = NULL; + byte read_data[1]; + + XMEMSET(&test_ctx, 0, sizeof(test_ctx)); + XMEMSET(&test_ctx2, 0, sizeof(test_ctx2)); + XMEMSET(&test_ctx3, 0, sizeof(test_ctx3)); + ExpectIntEQ(test_memio_setup(&test_ctx, &ctx_c, &ctx_s, &ssl_c, &ssl_s, + wolfTLSv1_3_client_method, wolfTLSv1_3_server_method), 0); + + /* Generate tickets */ + ExpectIntEQ(test_memio_do_handshake(ssl_c, ssl_s, 10, NULL), 0); + wolfSSL_SetLoggingPrefix("client"); + /* Read the ticket msg */ + ExpectIntEQ(wolfSSL_read(ssl_c, read_data, sizeof(read_data)), + WOLFSSL_FATAL_ERROR); + ExpectIntEQ(wolfSSL_get_error(ssl_c, WOLFSSL_FATAL_ERROR), + WOLFSSL_ERROR_WANT_READ); + wolfSSL_SetLoggingPrefix(NULL); + + ExpectIntEQ(test_memio_setup(&test_ctx2, &ctx_c, &ctx_s, &ssl_c2, &ssl_s2, + wolfTLSv1_3_client_method, wolfTLSv1_3_server_method), 0); + ExpectIntEQ(test_memio_setup(&test_ctx3, &ctx_c, &ctx_s, &ssl_c3, &ssl_s3, + wolfTLSv1_3_client_method, wolfTLSv1_3_server_method), 0); + + ExpectNotNull(sess = wolfSSL_get1_session(ssl_c)); + ExpectIntEQ(wolfSSL_set_session(ssl_c2, sess), WOLFSSL_SUCCESS); + ExpectIntEQ(wolfSSL_set_session(ssl_c3, sess), WOLFSSL_SUCCESS); + + wolfSSL_SetLoggingPrefix("client"); + /* Exchange intial flights for the second connection */ + ExpectIntEQ(wolfSSL_connect(ssl_c2), WOLFSSL_FATAL_ERROR); + ExpectIntEQ(wolfSSL_get_error(ssl_c2, WOLFSSL_FATAL_ERROR), + WOLFSSL_ERROR_WANT_READ); + wolfSSL_SetLoggingPrefix(NULL); + wolfSSL_SetLoggingPrefix("server"); + ExpectIntEQ(wolfSSL_accept(ssl_s2), WOLFSSL_FATAL_ERROR); + ExpectIntEQ(wolfSSL_get_error(ssl_s2, WOLFSSL_FATAL_ERROR), + WOLFSSL_ERROR_WANT_READ); + wolfSSL_SetLoggingPrefix(NULL); + + /* Complete third connection so that new tickets are exchanged */ + ExpectIntEQ(test_memio_do_handshake(ssl_c3, ssl_s3, 10, NULL), 0); + /* Read the ticket msg */ + wolfSSL_SetLoggingPrefix("client"); + ExpectIntEQ(wolfSSL_read(ssl_c3, read_data, sizeof(read_data)), + WOLFSSL_FATAL_ERROR); + ExpectIntEQ(wolfSSL_get_error(ssl_c3, WOLFSSL_FATAL_ERROR), + WOLFSSL_ERROR_WANT_READ); + wolfSSL_SetLoggingPrefix(NULL); + + /* Complete second connection */ + ExpectIntEQ(test_memio_do_handshake(ssl_c2, ssl_s2, 10, NULL), 0); + + ExpectIntEQ(wolfSSL_session_reused(ssl_c2), 1); + ExpectIntEQ(wolfSSL_session_reused(ssl_c3), 1); + + wolfSSL_free(ssl_c); + wolfSSL_free(ssl_c2); + wolfSSL_free(ssl_c3); + wolfSSL_free(ssl_s); + wolfSSL_free(ssl_s2); + wolfSSL_free(ssl_s3); + wolfSSL_CTX_free(ctx_c); + wolfSSL_CTX_free(ctx_s); + wolfSSL_SESSION_free(sess); +#endif + return EXPECT_RESULT(); +} + #if defined(WOLFSSL_DTLS) && !defined(WOLFSSL_NO_TLS12) && \ defined(HAVE_IO_TESTS_DEPENDENCIES) && defined(HAVE_SECURE_RENEGOTIATION) static void test_dtls_downgrade_scr_server_ctx_ready_server(WOLFSSL_CTX* ctx) @@ -65733,6 +65826,7 @@ TEST_CASE testCases[] = { TEST_DECL(test_TLSX_CA_NAMES_bad_extension), TEST_DECL(test_dtls_1_0_hvr_downgrade), TEST_DECL(test_session_ticket_no_id), + TEST_DECL(test_session_ticket_hs_update), TEST_DECL(test_dtls_downgrade_scr_server), TEST_DECL(test_dtls_downgrade_scr), /* This test needs to stay at the end to clean up any caches allocated. */ diff --git a/tests/utils.h b/tests/utils.h index e50615d72b..c54633d1c5 100644 --- a/tests/utils.h +++ b/tests/utils.h @@ -207,7 +207,9 @@ int test_memio_do_handshake(WOLFSSL *ssl_c, WOLFSSL *ssl_s, *rounds = 0; while (!handshake_complete && max_rounds > 0) { if (!hs_c) { + wolfSSL_SetLoggingPrefix("client"); ret = wolfSSL_connect(ssl_c); + wolfSSL_SetLoggingPrefix(NULL); if (ret == WOLFSSL_SUCCESS) { hs_c = 1; } @@ -219,7 +221,9 @@ int test_memio_do_handshake(WOLFSSL *ssl_c, WOLFSSL *ssl_s, } } if (!hs_s) { + wolfSSL_SetLoggingPrefix("server"); ret = wolfSSL_accept(ssl_s); + wolfSSL_SetLoggingPrefix(NULL); if (ret == WOLFSSL_SUCCESS) { hs_s = 1; } From 8ce71cc19c0ef7817262de85a1aec544270d1d64 Mon Sep 17 00:00:00 2001 From: Juliusz Sosinowicz Date: Thu, 17 Aug 2023 17:38:02 +0200 Subject: [PATCH 3/3] Call HaveUniqueSessionObj when we need to have a unique session object --- src/internal.c | 29 +++++++++++++++++++++-------- src/ssl.c | 16 ++-------------- src/tls13.c | 6 ++++++ wolfssl/internal.h | 1 + 4 files changed, 30 insertions(+), 22 deletions(-) diff --git a/src/internal.c b/src/internal.c index 443743a97f..ac6e58d62d 100644 --- a/src/internal.c +++ b/src/internal.c @@ -27456,6 +27456,20 @@ static int HashSkeData(WOLFSSL* ssl, enum wc_HashType hashType, /* client only parts */ #ifndef NO_WOLFSSL_CLIENT + int HaveUniqueSessionObj(WOLFSSL* ssl) + { + if (ssl->session->ref.count > 1) { + WOLFSSL_SESSION* newSession = wolfSSL_SESSION_dup(ssl->session); + if (newSession == NULL) { + WOLFSSL_MSG("Session duplicate failed"); + return 0; + } + wolfSSL_FreeSession(ssl->ctx, ssl->session); + ssl->session = newSession; + } + return 1; + } + #ifndef WOLFSSL_NO_TLS12 /* handle generation of client_hello (1) */ @@ -28295,6 +28309,11 @@ static int HashSkeData(WOLFSSL* ssl, enum wc_HashType hashType, else { if (DSH_CheckSessionId(ssl)) { if (SetCipherSpecs(ssl) == 0) { + if (!HaveUniqueSessionObj(ssl)) { + WOLFSSL_MSG("Unable to have unique session object"); + WOLFSSL_ERROR_VERBOSE(MEMORY_ERROR); + return MEMORY_ERROR; + } XMEMCPY(ssl->arrays->masterSecret, ssl->session->masterSecret, SECRET_LEN); @@ -31810,14 +31829,8 @@ int SendCertificateVerify(WOLFSSL* ssl) #ifdef HAVE_SESSION_TICKET int SetTicket(WOLFSSL* ssl, const byte* ticket, word32 length) { - /* If the session is shared, we need to copy-on-write */ - if (ssl->session->ref.count > 1) { - WOLFSSL_SESSION* nsession = wolfSSL_SESSION_dup(ssl->session); - if (nsession == NULL) - return MEMORY_E; - wolfSSL_FreeSession(ssl->ctx, ssl->session); - ssl->session = nsession; - } + if (!HaveUniqueSessionObj(ssl)) + return MEMORY_ERROR; /* Free old dynamic ticket if we already had one */ if (ssl->session->ticketLenAlloc > 0) { diff --git a/src/ssl.c b/src/ssl.c index 78f0bad9b0..94f70e35af 100644 --- a/src/ssl.c +++ b/src/ssl.c @@ -14173,11 +14173,7 @@ int wolfSSL_SetSession(WOLFSSL* ssl, WOLFSSL_SESSION* session) if (ssl->session == session) { WOLFSSL_MSG("ssl->session and session same"); } - else -#ifdef HAVE_STUNNEL - /* stunnel depends on the ex_data not being duplicated. Copy OpenSSL - * behaviour for now. */ - if (session->type != WOLFSSL_SESSION_TYPE_CACHE) { + else if (session->type != WOLFSSL_SESSION_TYPE_CACHE) { if (wolfSSL_SESSION_up_ref(session) == WOLFSSL_SUCCESS) { wolfSSL_FreeSession(ssl->ctx, ssl->session); ssl->session = session; @@ -14185,9 +14181,7 @@ int wolfSSL_SetSession(WOLFSSL* ssl, WOLFSSL_SESSION* session) else ret = WOLFSSL_FAILURE; } - else -#endif - { + else { ret = wolfSSL_DupSession(session, ssl->session, 0); if (ret != WOLFSSL_SUCCESS) WOLFSSL_MSG("Session duplicate failed"); @@ -20607,7 +20601,6 @@ int wolfSSL_DupSession(const WOLFSSL_SESSION* input, WOLFSSL_SESSION* output, WOLFSSL_SESSION* wolfSSL_SESSION_dup(WOLFSSL_SESSION* session) { -#ifdef HAVE_EXT_CACHE WOLFSSL_SESSION* copy; WOLFSSL_ENTER("wolfSSL_SESSION_dup"); @@ -20630,11 +20623,6 @@ WOLFSSL_SESSION* wolfSSL_SESSION_dup(WOLFSSL_SESSION* session) copy = NULL; } return copy; -#else - WOLFSSL_MSG("wolfSSL_SESSION_dup feature not compiled in"); - (void)session; - return NULL; -#endif /* HAVE_EXT_CACHE */ } void wolfSSL_FreeSession(WOLFSSL_CTX* ctx, WOLFSSL_SESSION* session) diff --git a/src/tls13.c b/src/tls13.c index 19898edff1..19c43fffed 100644 --- a/src/tls13.c +++ b/src/tls13.c @@ -3704,6 +3704,12 @@ static int SetupPskKey(WOLFSSL* ssl, PreSharedKey* psk, int clientHello) if (psk == NULL) return BAD_FUNC_ARG; + if (!HaveUniqueSessionObj(ssl)) { + WOLFSSL_MSG("Unable to have unique session object"); + WOLFSSL_ERROR_VERBOSE(MEMORY_ERROR); + return MEMORY_ERROR; + } + suite[0] = ssl->options.cipherSuite0; suite[1] = ssl->options.cipherSuite; diff --git a/wolfssl/internal.h b/wolfssl/internal.h index deb876ed8f..4cd5931853 100644 --- a/wolfssl/internal.h +++ b/wolfssl/internal.h @@ -6209,6 +6209,7 @@ WOLFSSL_LOCAL void DoCertFatalAlert(WOLFSSL* ssl, int ret); WOLFSSL_LOCAL int cipherExtraData(WOLFSSL* ssl); #ifndef NO_WOLFSSL_CLIENT + WOLFSSL_LOCAL int HaveUniqueSessionObj(WOLFSSL* ssl); WOLFSSL_LOCAL int SendClientHello(WOLFSSL* ssl); WOLFSSL_LOCAL int DoHelloVerifyRequest(WOLFSSL* ssl, const byte* input, word32* inOutIdx, word32 size);