Repository navigation
Add missing fields to wolfSSL_clear to ensure the WOLFSSL struct is fully reset. Ensure ephemeral keys are always freed between each handshake. - #11964
Conversation
Before, the early return from secure renegotiation in FreeHandshakeResources was causing the ephemeral keys to not be freed until wolfSSL_free was called.
…ully reset. DH params currently omitted due to a known bug. Thanks to Pu Cheng for the report.
… data. clSuitesStack is used by wolfSSL_get_client_ciphers, it is intentionally kept through a wolfSSL_clear for compatibility. Currently it is not being cleared when a new set of client ciphers arrive, so it is giving stale data.
Also test that a renegotiation uses new ephemeral keys.
|
@claude review |
|
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved findings expose use-after-free during pending async key generation, ECC key reuse in DTLS renegotiation, and build failures or crashes in the new tests.
4 open issues
What changed in this PR
This PR strengthens connection reuse by resetting additional WOLFSSL state and separating ephemeral-key cleanup from other handshake resources.
Changes:
- Reset protocol limits, cached peer data, pending output, and cipher state in
wolfSSL_clear(). - Centralize ephemeral-key cleanup and invalidate cached client cipher lists on new ClientHello messages.
- Add regression tests for connection reuse and fresh keys during secure renegotiation.
| File | Description |
|---|---|
wolfssl/internal.h |
Declare shared version-check and ephemeral-key cleanup helpers. |
tests/api/test_tls.h |
Register the new TLS regression tests. |
tests/api/test_tls.c |
Test state reset, interrupted handshakes, and renegotiation key freshness. |
src/tls13.c |
Invalidate cached client cipher lists on TLS 1.3 ClientHello processing. |
src/ssl.c |
Expand state reset and resource cleanup in wolfSSL_clear(). |
src/internal.c |
Centralize ephemeral-key cleanup and refresh TLS client cipher caches. |
Review effort: Balanced
| } | ||
| #endif | ||
|
|
||
| FreeEphemeralKeys(ssl); |
| ssl->peerX448KeyPresent = 0; | ||
| #endif | ||
| ssl->pssAlgo = 0; | ||
| FreeEphemeralKeys(ssl); |
| #ifdef HAVE_ECC | ||
| if (ssl->specs.kea == ecc_diffie_hellman_kea) { | ||
| ExpectIntEQ(wc_ecc_export_x963(ssl->eccTempKey, pub, pubSz), 0); |
| return EXPECT_RESULT(); | ||
| #ifdef HAVE_ECC | ||
| if (ssl->specs.kea == ecc_diffie_hellman_kea) { | ||
| ExpectIntEQ(wc_ecc_export_x963(ssl->eccTempKey, pub, pubSz), 0); |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| #if !defined(WOLFSSL_NO_CA_NAMES) && defined(OPENSSL_EXTRA) | ||
| wolfSSL_sk_X509_NAME_pop_free(ssl->peer_ca_names, NULL); | ||
| ssl->peer_ca_names = NULL; | ||
| #endif | ||
| #ifdef OPENSSL_EXTRA | ||
| wolfSSL_sk_X509_pop_free(ssl->peerCertChain, NULL); | ||
| ssl->peerCertChain = NULL; | ||
| wolfSSL_sk_X509_pop_free(ssl->verifiedChain, NULL); | ||
| ssl->verifiedChain = NULL; | ||
| #endif |
There was a problem hiding this comment.
🔴 Apps that read the peer chain or CA list after a clean shutdown now hit freed memory. Before this change those stacks lasted until wolfSSL_free(). wolfSSL_shutdown() calls wolfSSL_clear() once the shutdown succeeds (src/ssl_api_rw.c:1216, OPENSSL_EXTRA/WPAS_SMALL). The new code there frees peerCertChain, verifiedChain and peer_ca_names, but the getters hand out those exact pointers. Fix: free these stacks only when the next handshake starts, or skip freeing them on the shutdown path. They must stay valid until wolfSSL_free() or the next connection, as the getter docs promise.
Why this was flagged
An OPENSSL_EXTRA application calls wolfSSL_get_peer_cert_chain(), which returns ssl->peerCertChain itself (src/ssl_api_cert.c:3112). The docs say that stack "is owned by the SSL/TLS object and is disposed of with it". wolfSSL_get0_verified_chain() (verifiedChain) and the get0_peer_CA_list/get_client_CA_list getters (src/ssl_api_cert.c:1573, 1657, peer_ca_names) work the same way. The app then calls wolfSSL_shutdown(). When that returns WOLFSSL_SUCCESS, it calls wolfSSL_clear() (src/ssl_api_rw.c:1214-1219). The new lines at src/ssl.c:6036-6045 pop-free all three stacks there. If the app reads the chain after shutdown, for example to log the peer subject before wolfSSL_free(), it now reads freed X509 objects. On the base branch these stacks lived until wolfSSL_ResourceFree (src/internal.c:10612-10624). The same shutdown-time clear now also zeroes peerVerifyRet and alert_history, so wolfSSL_get_verify_result() and wolfSSL_get_alert_history() called after shutdown report a clean result instead of the real one.
Verification: Trigger: an OPENSSL_EXTRA build where the app keeps a stack pointer from wolfSSL_get_peer_cert_chain, calls wolfSSL_shutdown() until it succeeds, and then reads that stack. src/ssl_api_rw.c:1213-1220 calls wolfSSL_clear(ssl), which the diff makes free peerCertChain, verifiedChain and peer_ca_names (src/ssl.c ~6036-6044). In the base src/ssl.c, wolfSSL_clear never touches these stacks, so this use-after-free is a regression.
| if (ssl->options.downgrade) { | ||
| ssl->version.minor = ssl->options.versionSet ? | ||
| ssl->options.maxVersionMinor : ssl->ctx->method->version.minor; | ||
| #ifdef OPENSSL_EXTRA | ||
| /* The version mask only covers TLS. */ | ||
| if (!ssl->options.dtls && | ||
| (SetSSL_CTX_CheckVersion(ssl, ssl->ctx) != 0)) { | ||
| return WOLFSSL_FAILURE; | ||
| } | ||
| #endif | ||
| } |
There was a problem hiding this comment.
🔴 In builds without OPENSSL_EXTRA, a reused WOLFSSL can negotiate TLS 1.3 after wolfSSL_clear() even though wolfSSL_set_options() disabled it. wolfSSL_clear() now raises version.minor back to the method maximum. It re-applies ssl->options.mask only under OPENSSL_EXTRA (ssl.c:5830). Both functions are compiled in every build, and there the lowered ssl->version is the only TLS 1.3 block. Fix: re-apply the mask in every build after the version reset, e.g. take SetSSL_CTX_CheckVersion() out of the OPENSSL_EXTRA guard or repeat the step-down from wolfSSL_set_options().
Why this was flagged
Take a default build (no OPENSSL_EXTRA, HAVE_WEBSERVER or WOLFSSL_WPAS_SMALL) and a server made with wolfSSLv23_server_method(). The app calls wolfSSL_set_options(ssl, WOLFSSL_OP_NO_TLSv1_3), which is unguarded (ssl.c:7665, ssl.h:3282) and lowers ssl->version.minor to TLSv1_2_MINOR (ssl.c:7679-7682). After the first connection the app calls wolfSSL_clear(), which is also public in every build (ssl.h:3589). ssl.c:5828 sets version.minor = ctx->method->version.minor (TLSv1_3_MINOR), and the SetSSL_CTX_CheckVersion() call at ssl.c:5833 is compiled out by #ifdef OPENSSL_EXTRA. The server's supported_versions parser (tls.c:7530-7563) only compares against ssl->version.minor and never reads the mask, so the next accept negotiates TLS 1.3. On the client side, the supported_versions writer checks WOLFSSL_OP_NO_TLSv1_3 only under OPENSSL_EXTRA/HAVE_WEBSERVER/WOLFSSL_WPAS_SMALL (tls.c:7403-7406), so a client offers TLS 1.3 again too. On the base branch wolfSSL_clear() did not touch ssl->version, so the disabled version stayed off for every reuse.
Verification: wolfSSL_clear (ssl.c:5793) is also compiled in every build. The only call that re-applies the mask, SetSSL_CTX_CheckVersion (ssl.c:5830-5836), is inside #ifdef OPENSSL_EXTRA. So in these builds the lowered ssl->version is the only thing that blocks TLS 1.3, and wolfSSL_clear now undoes it. The base wolfSSL_clear did not touch version.minor, so TLS 1.3 stayed blocked.
| ssl->peerX448KeyPresent = 0; | ||
| #endif | ||
| ssl->pssAlgo = 0; | ||
| FreeEphemeralKeys(ssl); |
There was a problem hiding this comment.
🔴 Async-crypt builds can hit a use-after-free when a server abandons a handshake that is waiting on hardware and reuses the object with wolfSSL_clear(). FreeEphemeralKeys() now frees eccTempKey and serverDH_Key there. A pending key generation still has its event in ssl->ctx->event_queue, and ssl->asyncDev still points into the freed key. The block at ssl.c:5985 removes the event only when asyncDev is &ssl->kdfAsyncDev. Fix: before FreeEphemeralKeys(), when ssl->asyncDev is set, remove its event from the CTX queue and set ssl->asyncDev to NULL, generalising the kdfAsyncDev block to any pending device.
Why this was flagged
This needs a WOLFSSL_ASYNC_CRYPT build. A TLS 1.2 ECDHE or DHE server's SendServerKeyExchange makes eccTempKey (internal.c:41929) or serverDH_Key, and gets WC_PENDING_E. wolfSSL_AsyncPush (internal.c:48182-48185) then sets ssl->asyncDev to the key's asyncDev and pushes its event onto ssl->ctx->event_queue. If the application gives up on that handshake and calls wolfSSL_clear(), the new FreeEphemeralKeys() call at ssl.c:5954 frees the key, which holds the WC_ASYNC_DEV and its WOLF_EVENT. The cleanup at ssl.c:5985-5993 only removes the event and clears ssl->asyncDev when asyncDev == &ssl->kdfAsyncDev. So the CTX event queue keeps a pointer into freed memory, and ssl->asyncDev dangles. The next wolfSSL_CTX_AsyncPoll / wolfSSL_AsyncPoll (ssl.c:9429/9442), or the reused object's next connect/accept, then reads or writes freed memory. On the base branch wolfSSL_clear() did not free these keys, so the event stayed valid until wolfSSL_free().
Verification: The diff adds FreeEphemeralKeys(ssl); to wolfSSL_clear (ssl.c:5954). So after clear, the ctx event queue holds a pointer into freed memory. wolfSSL_clear's only queue cleanup is limited to the KDF device: if (ssl->asyncDev == &ssl->kdfAsyncDev) (ssl.c:5985-5993). On the base, wolfSSL_clear did not free eccTempKey or serverDH_Key, so the queued event still pointed at live memory.
| } | ||
| #endif | ||
|
|
||
| FreeEphemeralKeys(ssl); |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: pre-existing: DTLS 1.2 servers, and connections using wolfSSL_KeepHandshakeResources(), still reuse the previous handshake's ECDHE/X25519/X448 key on renegotiation. The new free sits only in FreeHandshakeResources() (internal.c:10705). DTLS calls that only after application data arrives (internal.c:26937), and keepResources skips it (ssl_api_hs.c:301). Fix: free the ephemeral keys when each new handshake starts, e.g. in wolfssl_rehandshake_prepare() and when a server accepts a renegotiation ClientHello, so freshness never depends on FreeHandshakeResources().
A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
For DTLS, wolfssl_handshake_done() sets dtlsHsRetain instead of calling FreeHandshakeResources() (ssl_api_hs.c:300-308). The only later call is in DoProcessAppDataRecord(), and only when application data arrives with no SCR in progress (internal.c:26937-26948). So if the client sends its renegotiation ClientHello before any application data, or the server calls wolfSSL_Rehandshake() first, eccTempKey and eccTempKeyPresent from handshake 1 are still set. wolfssl_rehandshake_prepare() (ssl_api_ext.c:1051-1105) and the client-initiated path free nothing. SendServerKeyExchange then skips EccMakeKey/X25519MakeKey/X448MakeKey because eccTempKeyPresent != 0 (internal.c:41873, 41901, 41928) and signs the old ephemeral public key again. TLS connections with options.keepResources set never call FreeHandshakeResources() (ssl_api_hs.c:301, tls13.c:17100), so they reuse the key the same way. The base branch behaves the same on these paths.
Verification: pre-existing. Trigger: a DTLS 1.2 connection renegotiates before the server has processed any application data, or the app has called wolfSSL_KeepHandshakeResources(). In both cases the server's ECDHE/X25519/X448 temp key from handshake 1 is reused. SendServerKeyExchange regenerates the key only when ssl->eccTempKeyPresent == 0 (internal.c:41873, 41901, 41928).
Description
Thanks to Pu Cheng for the report!
Fixes zd#22693
Testing
Built in + added tests
Checklist