Repository navigation
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add missing fields to wolfSSL_clear to ensure the WOLFSSL struct is fully reset. Ensure ephemeral keys are always freed between each handshake. #11964
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.
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
base: master
Are you sure you want to change the base?
Add missing fields to wolfSSL_clear to ensure the WOLFSSL struct is fully reset. Ensure ephemeral keys are always freed between each handshake. #11964
Changes from all commits
fae4e55267cd8bd4d99ae4beaae9File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟣 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).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 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.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.