Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
67 changes: 57 additions & 10 deletions src/internal.c
Original file line number Diff line number Diff line change
Expand Up @@ -7624,6 +7624,58 @@ static int SetSSL_CTX_CertsAndKeys(WOLFSSL* ssl, WOLFSSL_CTX* ctx)
}
#endif /* NO_CERTS */

#ifndef NO_DH
/* Give the SSL object its own copy of the context's DH parameters.
*
* The parameters are plain buffers with no reference count on them, so a
* session must not point at the context's: the context is free to replace
* them at any time. They are small and only present when the application
* asked for them, so a copy per session is the cheap way to keep them safe.
*
* @param [in, out] ssl SSL object. Any parameters it owns are let go of.
* @param [in] ctx SSL context object.
* @return 0 on success.
* @return MEMORY_E when dynamic memory allocation fails.
*/
int CopySSL_CTX_DhParams(WOLFSSL* ssl, WOLFSSL_CTX* ctx)
{
byte* p;
byte* g;

if ((ctx->serverDH_P.buffer == NULL) || (ctx->serverDH_G.buffer == NULL)) {
return 0;
}

p = (byte*)XMALLOC(ctx->serverDH_P.length, ssl->heap,
DYNAMIC_TYPE_PUBLIC_KEY);
g = (byte*)XMALLOC(ctx->serverDH_G.length, ssl->heap,
DYNAMIC_TYPE_PUBLIC_KEY);
if ((p == NULL) || (g == NULL)) {
XFREE(p, ssl->heap, DYNAMIC_TYPE_PUBLIC_KEY);
XFREE(g, ssl->heap, DYNAMIC_TYPE_PUBLIC_KEY);
return MEMORY_E;
}

/* Let go of any parameters this object already had. */
if (ssl->buffers.weOwnDH) {
XFREE(ssl->buffers.serverDH_P.buffer, ssl->heap,
DYNAMIC_TYPE_PUBLIC_KEY);
XFREE(ssl->buffers.serverDH_G.buffer, ssl->heap,
DYNAMIC_TYPE_PUBLIC_KEY);
}

XMEMCPY(p, ctx->serverDH_P.buffer, ctx->serverDH_P.length);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 [Low] DH parameter copy reads context buffers without synchronization · Use-after-free / double-free

CopySSL_CTX_DhParams() reads ctx->serverDH_P/G under no lock, while wolfssl_ctx_set_tmp_dh() (src/ssl_load.c:6028-6034) frees the old buffers and then stores the new pointer and length separately. A wolfSSL_CTX_SetTmpDH*() concurrent with wolfSSL_new() makes this XMEMCPY read freed memory, or a new pointer with the previous larger length (heap over-read into the session's prime). Adjacent to known finding #8917, which covers the certificate/private-key buffers in SetSSL_CTX_CertsAndKeys — different fields and a different fix site.

Related known finding #6557 (similar but distinct): Both are unsynchronized reads of shared mutable state in src/internal.c, but this copies ctx DH buffers during CopySSL_CTX_DhParams while #6557 decrypts with ticket key state in DefTicketEncCb. The raced operations, objects, root ownership, and required locks/fixes differ.

Fix: Guard context DH parameter replacement and this copy with a CTX-level lock, or reference-count the parameter buffers.

XMEMCPY(g, ctx->serverDH_G.buffer, ctx->serverDH_G.length);
ssl->buffers.serverDH_P.buffer = p;
ssl->buffers.serverDH_P.length = ctx->serverDH_P.length;
ssl->buffers.serverDH_G.buffer = g;
ssl->buffers.serverDH_G.length = ctx->serverDH_G.length;
ssl->buffers.weOwnDH = 1;

return 0;
}
#endif /* !NO_DH */

int SetSSL_CTX(WOLFSSL* ssl, WOLFSSL_CTX* ctx, int writeDup)
{
int ret = WOLFSSL_SUCCESS; /* set default ret */
Expand Down Expand Up @@ -7812,8 +7864,9 @@ int SetSSL_CTX(WOLFSSL* ssl, WOLFSSL_CTX* ctx, int writeDup)
!defined(HAVE_SELFTEST)
ssl->options.dhKeyTested = ctx->dhKeyTested;
#endif
ssl->buffers.serverDH_P = ctx->serverDH_P;
ssl->buffers.serverDH_G = ctx->serverDH_G;
if (CopySSL_CTX_DhParams(ssl, ctx) != 0) {
return MEMORY_E;
}
#endif

#if defined(HAVE_RPK)
Expand Down Expand Up @@ -9688,7 +9741,6 @@ void wolfSSL_ResourceFree(WOLFSSL* ssl)
}
XFREE(ssl->buffers.serverDH_Priv.buffer, ssl->heap, DYNAMIC_TYPE_PRIVATE_KEY);
XFREE(ssl->buffers.serverDH_Pub.buffer, ssl->heap, DYNAMIC_TYPE_PUBLIC_KEY);
/* parameters (p,g) may be owned by ctx */
if (ssl->buffers.weOwnDH) {
XFREE(ssl->buffers.serverDH_G.buffer, ssl->heap, DYNAMIC_TYPE_PUBLIC_KEY);
XFREE(ssl->buffers.serverDH_P.buffer, ssl->heap, DYNAMIC_TYPE_PUBLIC_KEY);
Expand Down Expand Up @@ -10077,13 +10129,8 @@ void FreeHandshakeResources(WOLFSSL* ssl)
ssl->buffers.serverDH_Priv.buffer = NULL;
XFREE(ssl->buffers.serverDH_Pub.buffer, ssl->heap, DYNAMIC_TYPE_PUBLIC_KEY);
ssl->buffers.serverDH_Pub.buffer = NULL;
/* parameters (p,g) may be owned by ctx */
if (ssl->buffers.weOwnDH) {
XFREE(ssl->buffers.serverDH_G.buffer, ssl->heap, DYNAMIC_TYPE_PUBLIC_KEY);
ssl->buffers.serverDH_G.buffer = NULL;
XFREE(ssl->buffers.serverDH_P.buffer, ssl->heap, DYNAMIC_TYPE_PUBLIC_KEY);
ssl->buffers.serverDH_P.buffer = NULL;
}
/* The parameters (p,g) are kept: a renegotiation or a reused object needs

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the point of the PR, to keep the buffers

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the point of the PR

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 [Low] DH p/g retention in FreeHandshakeResources is unconditional, including for objects that re-parse them · Resource leaks on error paths

The p/g free was dropped for every side. A client object retains the peer's prime and generator (up to maxDhKeySz) for its whole lifetime although no code reads them after the handshake and GetDhPublicKey frees and re-allocates them at internal.c:35081 on any later ServerKeyExchange. The same applies to sniffer sessions freed via FreeHandshakeResources in sniffer.c:4658. Only the server case that inherits parameters from the CTX needs the retention.

Fix: Retain p/g only where they can be reused, e.g. keep the free when ssl->options.side == WOLFSSL_CLIENT_END.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 [Low] Reused server object keeps the previous peer's negotiated FFDHE group · TLS protocol issues

FreeHandshakeResources() no longer clears serverDH_P/G, and wolfSSL_clear() does not reset them, so a reused server object retains the group installed by tlsx_ffdhe_find_group() (src/tls.c:5546) for the previous peer. TLSX_SupportedFFDHE_Set() returns early when the next client offers no FFDHE group, so that handshake runs on the earlier peer's group instead of the context-configured parameters — a peer offering only the server's weakest acceptable group fixes it for later connections on that object.

Related known finding #9094 (similar but distinct): Both concern FFDHE group handling during server negotiation, but this retains a prior connection's installed DH parameters through FreeHandshakeResources/wolfSSL_clear, whereas #9094 skips offered-group enforcement for custom-DH builds in DoClientHello. They have different faulting paths and require separate lifecycle versus negotiation fixes.

Fix: Re-copy the context's parameters in wolfSSL_clear(), or drop FFDHE-installed parameters when the handshake ends.

* them again, and they are freed with the object. */
#endif /* !NO_DH */

#if !defined(NO_CERTS) && !defined(OPENSSL_EXTRA) && \
Expand Down
11 changes: 8 additions & 3 deletions src/ssl_api_hs.c
Original file line number Diff line number Diff line change
Expand Up @@ -1592,9 +1592,14 @@ void wolfSSL_set_accept_state(WOLFSSL* ssl)

#ifndef NO_DH
if ((!ssl->options.haveDH) && (ssl->ctx->haveDH)) {
ssl->buffers.serverDH_P = ssl->ctx->serverDH_P;
ssl->buffers.serverDH_G = ssl->ctx->serverDH_G;
ssl->options.haveDH = 1;
/* Nothing to return the failure through, so leave the object
* without DH rather than claim parameters it does not have. */
if (CopySSL_CTX_DhParams(ssl, ssl->ctx) == 0) {
ssl->options.haveDH = 1;
}
else {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚪ [Info] Allocation-failure branch of the new DH copy is untested · Missing edge-case coverage on a function the PR also changed

The added tests only exercise the successful copy. Neither the MEMORY_E branch of CopySSL_CTX_DhParams() (which must leave the object's existing parameters and haveDH untouched) nor the resulting SetSSL_CTX() failure return is covered, so a regression in that ordering would pass CI.

Fix: Add a case that forces the allocation failure and assert haveDH stays 0 with the prior parameters intact.

WOLFSSL_MSG("Unable to copy DH parameters from context");

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚪ [Info] New DH-copy failure paths are not exercised by the added tests · Missing edge-case coverage on a function the PR also changed

CopySSL_CTX_DhParams documents a MEMORY_E return, and both new call sites add handling for it: the else branch here (which deliberately leaves haveDH clear) and the return MEMORY_E in SetSSL_CTX at internal.c:7868. All four new/updated tests only drive the successful copy, so neither error branch is covered.

Fix: Add a case under the existing allocation-failure facility (WOLFSSL_MEM_FAIL_COUNT) asserting haveDH stays 0 here and that wolfSSL_new() returns NULL.

}
}
#endif
}
Expand Down
152 changes: 141 additions & 11 deletions tests/api/test_ssl_hs.c
Original file line number Diff line number Diff line change
Expand Up @@ -810,10 +810,15 @@ int test_wolfSSL_set_accept_state_reinit(void)

wolfSSL_set_accept_state(ssl);

/* Picked up as the object's own copy, not the context's buffers. */
if ((ssl != NULL) && (ctx != NULL)) {
ExpectIntEQ(ssl->options.haveDH, 1);
ExpectPtrEq(ssl->buffers.serverDH_P.buffer, ctx->serverDH_P.buffer);
ExpectPtrEq(ssl->buffers.serverDH_G.buffer, ctx->serverDH_G.buffer);
ExpectNotNull(ssl->buffers.serverDH_P.buffer);
ExpectNotNull(ssl->buffers.serverDH_G.buffer);
ExpectPtrNE(ssl->buffers.serverDH_P.buffer, ctx->serverDH_P.buffer);
ExpectPtrNE(ssl->buffers.serverDH_G.buffer, ctx->serverDH_G.buffer);
ExpectIntEQ(ssl->buffers.serverDH_P.length, ctx->serverDH_P.length);
ExpectIntEQ(ssl->buffers.serverDH_G.length, ctx->serverDH_G.length);
}

wolfSSL_free(ssl);
Expand Down Expand Up @@ -968,8 +973,8 @@ int test_wolfSSL_SSL_do_handshake_quic(void)
/* Test that wolfSSL_set_connect_state() discards server DH parameters.
*
* A client generates its own DH parameters, so any server ones are dropped.
* Parameters the object owns are freed; parameters merely borrowed from the
* context are only unlinked, since the context still owns them.
* The object holds its own copy either way, whether set on it directly or
* taken from the context, so the context is left untouched.
*
* @return TEST_SUCCESS on success.
*/
Expand Down Expand Up @@ -1016,7 +1021,7 @@ int test_wolfSSL_set_connect_state_dh(void)
wolfSSL_CTX_free(ctx);
ctx = NULL;

/* Parameters borrowed from the context are left for the context to free. */
/* Parameters taken from the context are a copy the object owns. */
ExpectNotNull(ctx = wolfSSL_CTX_new(wolfTLSv1_2_server_method()));
ExpectIntEQ(wolfSSL_CTX_use_certificate_file(ctx, svrCertFile,
WOLFSSL_FILETYPE_PEM), WOLFSSL_SUCCESS);
Expand All @@ -1026,10 +1031,11 @@ int test_wolfSSL_set_connect_state_dh(void)
WOLFSSL_FILETYPE_PEM), WOLFSSL_SUCCESS);
ExpectNotNull(ssl = wolfSSL_new(ctx));

/* Inherited from the context, so not owned here. */
if (ssl != NULL) {
ExpectPtrEq(ssl->buffers.serverDH_P.buffer, ctx->serverDH_P.buffer);
ExpectIntEQ(ssl->buffers.weOwnDH, 0);
/* Copied from the context, so owned here. */
if ((ssl != NULL) && (ctx != NULL)) {
ExpectNotNull(ssl->buffers.serverDH_P.buffer);
ExpectPtrNE(ssl->buffers.serverDH_P.buffer, ctx->serverDH_P.buffer);
ExpectIntEQ(ssl->buffers.weOwnDH, 1);
}

wolfSSL_set_connect_state(ssl);
Expand All @@ -1044,8 +1050,9 @@ int test_wolfSSL_set_connect_state_dh(void)
ExpectNotNull(ctx->serverDH_G.buffer);
}
ExpectNotNull(ssl2 = wolfSSL_new(ctx));
if (ssl2 != NULL) {
ExpectPtrEq(ssl2->buffers.serverDH_P.buffer, ctx->serverDH_P.buffer);
if ((ssl2 != NULL) && (ctx != NULL)) {
ExpectNotNull(ssl2->buffers.serverDH_P.buffer);
ExpectPtrNE(ssl2->buffers.serverDH_P.buffer, ctx->serverDH_P.buffer);
}

wolfSSL_free(ssl2);
Expand All @@ -1055,6 +1062,129 @@ int test_wolfSSL_set_connect_state_dh(void)
return EXPECT_RESULT();
}

/* DH parameters taken from the context are the object's own copy, and they
* last as long as the object does.
*
* The copy is what lets the context replace its parameters while sessions are
* running. Since the session then holds the only copy it will ever have, the
* end of a handshake must not take it away: a reused object needs it again.
*
* @return TEST_SUCCESS on success.
*/
int test_wolfSSL_dh_ctx_params_reuse(void)
{
EXPECT_DECLS;
/* Handing the object a second connection needs its certificate and key to
* outlast the first handshake, which only these builds arrange. */
#if (defined(OPENSSL_EXTRA) || defined(WOLFSSL_WPAS_SMALL)) && \
defined(HAVE_MANUAL_MEMIO_TESTS_DEPENDENCIES) && \
defined(BUILD_TLS_DHE_RSA_WITH_AES_128_GCM_SHA256) && \
!defined(WOLFSSL_NO_TLS12) && !defined(NO_DH) && !defined(NO_RSA) && \
!defined(NO_FILESYSTEM) && !defined(NO_WOLFSSL_SERVER) && \
!defined(NO_WOLFSSL_CLIENT)
struct test_memio_ctx test_ctx;
WOLFSSL_CTX* ctx_c = NULL;
WOLFSSL_CTX* ctx_s = NULL;
WOLFSSL* ssl_c = NULL;
WOLFSSL* ssl_s = NULL;
const byte* startP = NULL;
const byte* startG = NULL;

XMEMSET(&test_ctx, 0, sizeof(test_ctx));
test_ctx.c_ciphers = test_ctx.s_ciphers = "DHE-RSA-AES128-GCM-SHA256";

/* The parameters go on the context before any session takes them. A
* context made here is used as-is, so it gets the credentials and the
* memio wiring that test_memio_setup would otherwise have given it. */
ExpectNotNull(ctx_s = wolfSSL_CTX_new(wolfTLSv1_2_server_method()));
ExpectIntEQ(wolfSSL_CTX_use_PrivateKey_file(ctx_s, svrKeyFile,
CERT_FILETYPE), WOLFSSL_SUCCESS);
ExpectIntEQ(wolfSSL_CTX_use_certificate_file(ctx_s, svrCertFile,
CERT_FILETYPE), WOLFSSL_SUCCESS);
ExpectIntEQ(wolfSSL_CTX_set_cipher_list(ctx_s, test_ctx.s_ciphers),
WOLFSSL_SUCCESS);
wolfSSL_SetIORecv(ctx_s, test_memio_read_cb);
wolfSSL_SetIOSend(ctx_s, test_memio_write_cb);
ExpectIntEQ(wolfSSL_CTX_SetTmpDH_file(ctx_s, dhParamFile, CERT_FILETYPE),
WOLFSSL_SUCCESS);

ExpectIntEQ(test_memio_setup(&test_ctx, &ctx_c, &ctx_s, &ssl_c, &ssl_s,
wolfTLSv1_2_client_method, wolfTLSv1_2_server_method), 0);

/* test_memio_setup puts its own parameters on the server session, which
* would stand in for the ones under test. Take a fresh session from the
* same context so the parameters really are the context's. */
wolfSSL_free(ssl_s);
ssl_s = NULL;
ExpectNotNull(ssl_s = wolfSSL_new(ctx_s));
wolfSSL_SetIOWriteCtx(ssl_s, &test_ctx);
wolfSSL_SetIOReadCtx(ssl_s, &test_ctx);

/* Its own copy, matching the context's but not the same buffers. */
if ((ssl_s != NULL) && (ctx_s != NULL)) {
ExpectNotNull(ssl_s->buffers.serverDH_P.buffer);
ExpectNotNull(ssl_s->buffers.serverDH_G.buffer);
ExpectPtrNE(ssl_s->buffers.serverDH_P.buffer, ctx_s->serverDH_P.buffer);
ExpectPtrNE(ssl_s->buffers.serverDH_G.buffer, ctx_s->serverDH_G.buffer);
ExpectIntEQ(ssl_s->buffers.serverDH_P.length, ctx_s->serverDH_P.length);
ExpectIntEQ(ssl_s->buffers.serverDH_G.length, ctx_s->serverDH_G.length);
ExpectIntEQ(ssl_s->buffers.weOwnDH, 1);
startP = ssl_s->buffers.serverDH_P.buffer;
startG = ssl_s->buffers.serverDH_G.buffer;
}

/* Offering no FFDHE group keeps the server on the parameters it was
* given, rather than switching to a named group and dropping them. A build
* without supported curves sends no groups at all, so there is nothing to
* narrow down. */
#ifdef HAVE_SUPPORTED_CURVES
ExpectIntEQ(wolfSSL_UseSupportedCurve(ssl_c, WOLFSSL_ECC_SECP256R1),

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

adding missing HAVE_SUPPORTED_CURVES

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

adding missing HAVE_SUPPORTED_CURVES

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

WOLFSSL_SUCCESS);
#endif

ExpectIntEQ(test_memio_do_handshake(ssl_c, ssl_s, 10, NULL), 0);

/* The handshake used them and the cleanup after it left them alone. */
if (ssl_s != NULL) {
ExpectPtrEq(ssl_s->buffers.serverDH_P.buffer, startP);
ExpectPtrEq(ssl_s->buffers.serverDH_G.buffer, startG);
}

/* So the object can be handed a second connection. */
ExpectIntEQ(wolfSSL_clear(ssl_s), WOLFSSL_SUCCESS);
wolfSSL_free(ssl_c);
ssl_c = NULL;
ExpectNotNull(ssl_c = wolfSSL_new(ctx_c));
wolfSSL_SetIOWriteCtx(ssl_c, &test_ctx);
wolfSSL_SetIOReadCtx(ssl_c, &test_ctx);
#ifdef HAVE_SUPPORTED_CURVES
ExpectIntEQ(wolfSSL_UseSupportedCurve(ssl_c, WOLFSSL_ECC_SECP256R1),
WOLFSSL_SUCCESS);
#endif
test_memio_clear_buffer(&test_ctx, 0);
test_memio_clear_buffer(&test_ctx, 1);

/* The second handshake ran on the parameters the first one left behind. */
ExpectIntEQ(test_memio_do_handshake(ssl_c, ssl_s, 10, NULL), 0);
if (ssl_s != NULL) {
ExpectPtrEq(ssl_s->buffers.serverDH_P.buffer, startP);
ExpectPtrEq(ssl_s->buffers.serverDH_G.buffer, startG);
}

/* And the context still has its own to hand out. */
if (ctx_s != NULL) {
ExpectNotNull(ctx_s->serverDH_P.buffer);
ExpectNotNull(ctx_s->serverDH_G.buffer);
}

wolfSSL_free(ssl_c);
wolfSSL_free(ssl_s);
wolfSSL_CTX_free(ctx_c);
wolfSSL_CTX_free(ctx_s);
#endif
return EXPECT_RESULT();
}

/* The helpers below only install I/O callbacks, so they need nothing beyond
* TLS 1.2 itself. The guard is nevertheless the union of their six callers'
* guards, so the block is neither compiled without a caller nor missing when
Expand Down
2 changes: 2 additions & 0 deletions tests/api/test_ssl_hs.h
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ int test_wolfSSL_set_accept_state_static_ecc(void);
int test_wolfSSL_negotiate_bad_args(void);
int test_wolfSSL_SSL_do_handshake_quic(void);
int test_wolfSSL_set_connect_state_dh(void);
int test_wolfSSL_dh_ctx_params_reuse(void);
int test_wolfSSL_connect_bad_args(void);
int test_wolfSSL_accept_bad_args(void);
int test_wolfSSL_connect_step_failures(void);
Expand All @@ -62,6 +63,7 @@ int test_wolfSSL_hs_info_cb(void);
TEST_DECL_GROUP("ssl_hs", test_wolfSSL_negotiate_bad_args), \
TEST_DECL_GROUP("ssl_hs", test_wolfSSL_SSL_do_handshake_quic), \
TEST_DECL_GROUP("ssl_hs", test_wolfSSL_set_connect_state_dh), \
TEST_DECL_GROUP("ssl_hs", test_wolfSSL_dh_ctx_params_reuse), \
TEST_DECL_GROUP("ssl_hs", test_wolfSSL_connect_bad_args), \
TEST_DECL_GROUP("ssl_hs", test_wolfSSL_accept_bad_args), \
TEST_DECL_GROUP("ssl_hs", test_wolfSSL_connect_step_failures), \
Expand Down
Loading
Loading