Security and correctness fixes, plus a DTLS 1.3 scheduled work API - #11018
Open
Frauschi wants to merge 12 commits into
Open
Security and correctness fixes, plus a DTLS 1.3 scheduled work API#11018Frauschi wants to merge 12 commits into
Frauschi wants to merge 12 commits into
Conversation
|
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11018
Scan targets checked: wolfcrypt-bugs, wolfcrypt-port-bugs, wolfcrypt-rs-bugs, wolfcrypt-src, wolfssl-bugs, wolfssl-src
Findings: 3
3 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
Frauschi
commented
Jul 30, 2026
Frauschi
commented
Jul 30, 2026
Frauschi
commented
Jul 30, 2026
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11018
Scan targets checked: wolfcrypt-bugs, wolfcrypt-port-bugs, wolfcrypt-rs-bugs, wolfcrypt-src, wolfssl-bugs, wolfssl-src
No new issues found in the changed files. ✅
dgarske
reviewed
Jul 31, 2026
dgarske
left a comment
dgarske
left a comment
Member
There was a problem hiding this comment.
Skoll Code Review
Scan type: reviewOverall recommendation: REQUEST_CHANGES
Findings: 21 total — 19 posted, 2 skipped
19 finding(s) posted as inline comments (see file-level comments below)
Posted findings
- [High] wolfSSL_clear() wipes the DTLS 1.3 epoch table but leaves ssl-dtls13Epoch stale, breaking the next handshake with BAD_STATE_E —
src/ssl.c:5706-5716 - [High] wolfSSL_dtls13_do_scheduled_work() leaves options.sendKeyUpdate set after WANT_WRITE, sending a duplicate KeyUpdate —
src/ssl_api_dtls.c:1530-1539 - [Medium] The scheduled-work pump has no way to flush a partially sent record, and the predicate cannot report one —
src/ssl_api_dtls.c:1518-1524 - [Medium] HMAC_Init failure path also clears ctx-type, breaking a retry with the same digest —
src/ssl_crypto.c:1848-1857 - [Medium] dtlsProcessPendingPeer duplicates SockAddrSet instead of sharing it —
src/internal.c:23744-23775 - [Medium] Three new peerLock call sites use three different lock-failure policies —
src/internal.c:23724-23727 - [Low] HPKE RNG save/restore mutates a key the ECH server shares across connections —
wolfcrypt/src/hpke.c:1078-1090 - [Low] New API docs omit that WOLFSSL_FATAL_ERROR can be a retryable WANT_WRITE —
doc/dox_comments/header_files/ssl.h:3973-3976 - [Low] No test covers DTLS 1.3 object reuse after wolfSSL_clear —
tests/api.c:9900-10035 - [Low] test_wc_SrpComputeKey_oom hard-codes the allocation ordinal, making it build-fragile —
tests/api.c:36030-36042 - [Low] hpke_test_multi exercises the fixed encap/decap paths but got no RNG-restore assertion —
wolfcrypt/test/test.c:38036-38041 - [Low] DtlsResetState comment misstates what a recursive write-lock does —
src/dtls.c:106-113 - [Low] test_AEAD_drain_scheduled_work omits the !WOLFSSL_LEANPSK guard used everywhere else —
tests/api/test_dtls.c:4024-4031 - [Low] prevRng in wc_HpkeDecap is declared under a broader guard than any of its uses —
wolfcrypt/src/hpke.c:1025-1028 - [Info] New set_rng restore calls discard their return code without a (void) cast —
wolfcrypt/src/hpke.c:843 - [Info] ProcessReplyEx body not re-indented under the new conditional —
src/internal.c:25137-25145 - [Info] Unnecessary address-of on array arguments to ForceZero —
src/ssl.c:5695-5696 - [Info] New test guards use RC_NO_RNG, which looks like a typo for WC_NO_RNG —
tests/api.c:35932,36006 - [Info] TEST_DTLS13_PUMP is not statement-safe —
tests/utils.h:57-62
Skipped findings
- [Low]
AEAD hard-limit and send-limit coverage is silently dropped under WOLFSSL_RW_THREADED - [Low]
dtls13Rtx.mutex is taken for fields nothing else guards with it
Review generated by Skoll
Frauschi
force-pushed
the
fenrir_3
branch
2 times, most recently
from
July 31, 2026 22:17
4f29465 to
ea93da0
Compare
dgarske
previously approved these changes
Jul 31, 2026
douzzer
requested changes
Aug 1, 2026
douzzer
left a comment
douzzer
left a comment
Contributor
There was a problem hiding this comment.
unescaped error code operands (missing WC_NO_ERR_TRACE()):
wolfcrypt/src/hpke.c:1087: int ret = NOT_COMPILED_IN;
TimingPadVerify passes (pLen - macSz - padLen - 1) to ssl->hmac and relies on the callee recovering the record length by modular addition. TLS_hmac now does that addition with overflow checking and returns BUFFER_E before hashing anything, so a record whose padding length byte exceeds pLen - macSz - 1 is rejected without a MAC being computed at all, while a smaller padding byte gets the full constant time HMAC. The padding length byte is taken straight from the decrypted record, so this hands an attacker a Lucky13 style timing oracle worth an entire HMAC. Clamp the padding length in constant time before it is used, so the length handed to ssl->hmac never wraps and every value of the padding length byte results in the same amount of hashing. The rejection decision is unchanged, since MaskPadding already flags an out of range padding length. The overflow check in TLS_hmac stays as a backstop for genuinely bogus sizes. Add a regression test that drives TimingPadVerify over every padding length byte with a recording MAC callback and asserts the callback is always invoked with a length that does not wrap. Fixes F-7240.
wolfSSL_dtls_set_pending_peer acquired the DTLS peer lock for reading and then mutated the shared state under it. It frees and clears pendingPeer, calls SockAddrSet which frees and reallocates the address buffer, and writes processingPendingRecord. A read lock allows several holders at once, so two threads in this function can both free pendingPeer.sa, and a reader in wolfSSL_dtls_get_peer or the wolfio send path can observe a dangling pointer. Acquire the lock for writing instead, matching wolfSSL_dtls_set_peer, which already does so for the same class of mutation. Only affects builds that define WOLFSSL_RW_THREADED, since the locking is compiled out otherwise. Fixes F-7222.
X509PrintDirType derived the length of the alt name payload with XSTRLEN. A directoryName entry holds raw DER, which routinely contains zero bytes, and under WC_ASN_NO_HEAP the buffer is not NUL terminated at all, so the computed length was wrong in general and reading it was already out of bounds in the no-heap case. The length then fed an unsigned loop bound of src_len - 5, so any entry that XSTRLEN measured as shorter than five bytes wrapped the bound to nearly UINT32_MAX and the tag scan read far past the end of the entry. A certificate with a short or empty directoryName alt name reaches this from the ordinary certificate printing path. Take the length from entry->len, which the parser already fills in, and return early when the entry is too short to hold an OID, a tag and a length. The scan is unchanged for entries of five bytes or more, and the ASN parsing helpers now receive the true buffer length as their bound. Add a regression test that prints a certificate carrying a directoryName with an embedded zero byte before the common name, and one carrying a directoryName too short for the tag scan. Fixes F-7223.
wolfSSL_X509_check_host takes an explicit length and its own validation accepts a buffer with no NUL terminator, since only an embedded NUL is rejected and a trailing one is merely stripped when present. The iPAddress check then called CheckIPAddr, which drops the length and measures the buffer with XSTRLEN, reading past the end of a caller supplied buffer that is length delimited rather than terminated. This ran on every call, not only when checking an IP address, and is compiled in whenever WOLFSSL_IP_ALT_NAME is defined, which OPENSSL_ALL and WOLFSSL_QT enable. Call CheckHostName directly with the caller's length and the IP flag set. That is what CheckIPAddr does internally, minus the length being recomputed. Behaviour is unchanged for NUL terminated input, because the normalization above already leaves chklen equal to the string length in that case. It also fixes a matching bug, since a length delimited IP address sitting in a longer buffer no longer fails to match an iPAddress entry. Add a regression test covering an interior slice of a longer buffer and a buffer sized exactly to the name with no terminator. Fixes F-7248.
WOLFSSL_HMAC_CTX keeps a copy of the inner and outer pads outside the embedded wolfCrypt HMAC object so that a later init with a NULL key can restore the key. Those pads are the key combined with the fixed padding, so for any key no longer than the hash block size the key falls out of a single exclusive or. Cleanup only called wc_HmacFree on the embedded object, which zeroes what it is given but cannot reach the enclosing context, so the saved pads survived. HMAC_CTX_free then returned that heap block to the allocator with the key material still in it, where it stayed until some later allocation happened to overwrite it. Wipe both saved pads in wolfSSL_HMAC_cleanup, which HMAC_CTX_cleanup, HMAC_CTX_reset and HMAC_CTX_free all reach. Do the same on the set key failure path in the init function, since the context is reported as unkeyed there while the previous key's pads would otherwise remain. The session ticket key callback had the same leak for the same reason. It holds a WOLFSSL_HMAC_CTX on the stack, hands it to the application to be keyed with the long lived ticket HMAC key, and then only freed the embedded object, leaving the pads on the stack after every ticket encrypt and every ticket decrypt including the error paths. Have it clean up through wolfSSL_HMAC_CTX_cleanup so it picks up the wipe. Add a regression test that keys a context, checks the pads were populated, runs cleanup and requires both arrays to be zero. Fixes F-7256 and F-7257.
SSL_clear recycles a WOLFSSL object for a new connection, which is the usual pattern in connection pooling servers, and wolfSSL_shutdown calls it on success as well. It reset the option and state fields but left every piece of key material from the previous connection in place. The teardown path in SSL_ResourceFree is careful here and force zeroes the keys struct and the TLS 1.3 traffic secrets, so a reused object ended up holding material that a freed one would not. The keys struct keeps the write keys, MAC secrets and IVs, clientSecret and serverSecret keep the TLS 1.3 traffic secrets, the DTLS 1.3 epoch table keeps traffic keys, IVs and sequence number keys for every epoch, and the handshake arrays keep the master secret, the pre master secret, the PSK key and the TLS 1.3 key schedule secret. The tls-unique fields keep the Finished values of the connection that just ended, so the next caller could bind to the wrong session. The buffers are sized for the largest supported algorithm, so a later handshake that negotiates something smaller only overwrites a prefix and the tail survives. Force zero all of it. A freshly created object has these zeroed already, with two exceptions that are put back after the wipe: the multicast peer identifier sentinel, and the unprotected DTLS 1.3 epoch 0 together with the epoch pointers aimed at it, which only InitSSL sets up and without which the next handshake has no valid epoch. Wipe the handshake arrays in place rather than releasing them. They have to stay allocated because wolfSSL_set_secret, the exporter and the accessors that run after a connection all read from them on an object that is being recycled rather than freed, and because the key agreement routines take preMasterSz as the size of the buffer they may write, so that is restored to what a freshly allocated Arrays carries. An application that asked to keep the arrays still gets back everything the API can hand it, so the master secret and the exporter secret only go when it did not ask, while the pre master secret, the PSK key and the key schedule secret always do because nothing reads those back. wolfSSL_set_secret and wolfSSL_make_eap_keys both reached into the arrays without checking that they are there, which the ordinary handshake teardown can already leave them not to be, so both now report a bad argument instead. Add a regression test that runs a handshake, clears the object with the arrays kept, and requires the write keys, both traffic secrets and the pre master secret to be gone while the master secret, the exporter secret and the client random survive. It then takes that request back, clears again, and requires the master and exporter secrets to be gone with the arrays themselves still present. Fixes F-7258.
wc_ecc_ctx_new_ex records the caller's heap hint in the context and then calls wc_ecc_ctx_reset, which goes through ecc_ctx_init. That function opens by clearing the whole context and only restores the algorithm choices, the protocol role and the RNG, so the heap hint was lost on every context the _ex variant produced, and on every later call to the public reset. The context was then freed with a null hint, so it went to the default allocator rather than the heap it came from, and the temporary buffers that wc_ecc_encrypt_ex and wc_ecc_decrypt take from the same hint went to the default allocator too. A default build hides this because XMALLOC discards the hint, but with static memory the block belongs to the caller's pool and handing it to the system allocator is a free of memory that was never allocated there. Save the hint before ecc_ctx_init and restore it afterwards. Doing it in the reset covers both the constructor and the public reset. The other callers of ecc_ctx_init pass an uninitialized context on the stack, so the hint must not be read there. Add a regression test that creates a context from a static heap and requires the heap to be whole again once the context is freed. Fixes F-7082.
The encap and decap paths create a temporary RNG, install it into a key that belongs to the caller so the shared secret computation can blind, and then free it without taking it back out. Both wc_ecc_set_rng and wc_curve25519_set_rng only record the pointer, and nothing else ever writes that field, so the caller's key was left pointing at freed memory. Encap does this to the ephemeral key and decap to the receiver key, including the curve25519 branch. Repeated HPKE calls hide it because each one installs a fresh RNG first, but any other use of the key that consults it, such as an ECDH or a signature under timing resistance, reads the freed object. ECH holds exactly such a long lived key. Save whatever RNG the key already had and put it back before freeing the temporary one, so the key is handed back to the caller unchanged. Clearing the field instead would silently drop an RNG the caller had installed for blinding of their own. Extend the HPKE round trip test to give both keys an RNG of its own and to require them to still have it once the seal and open have finished. Fixes F-7083.
wc_rng_bank_init derives each instance's personalization string from &ctx->rngs[i], which is already the address of the instance, so the DRBG read the leading bytes of the instance struct rather than the address itself. Those bytes had just been cleared by the memset over the whole array and nothing writes to instance i before it is initialized, so every DRBG in the bank was instantiated with the same all zero personalization string. wc_rng_bank_inst_reinit does this correctly by taking the address of a local pointer variable, which yields the pointer value and so a distinct nonce per instance. Take the instance pointer into a local here as well and pass its address, so both paths agree and each instance gets its own value. Fixes F-7085.
In a small stack build wc_SrpComputeKey allocates six objects up front and checks them together afterwards, so a failure of any one of them jumps to the cleanup with the others allocated but not yet passed through mp_init_multi. The cleanup decided whether to zeroize the four temporaries by testing the return code against MP_INIT_E, which does not hold on the allocation failure path, so it called mp_forcezero on uninitialized memory. That takes its length from the size field of the object being zeroized, so an unset field turns into a write of arbitrary length past the end of the allocation. Track whether mp_init_multi succeeded and gate the zeroize on that instead. The flag is only set once the objects really are initialized, so it covers the init failure case the return code test was aiming at as well. Add a regression test that fails the last of the six allocations through a custom allocator and requires the call to report a memory error without touching the objects it never initialized. Fixes F-7084.
With WOLFSSL_RW_THREADED the read path performs no scheduled work, because transmitting from the reader would race the write thread over the output buffer and the sending key schedule, neither of which is covered by a lock. Post-handshake the only remaining consumers are on the write path, and wolfSSL_dtls_retransmit() only helps while the handshake is unfinished. An application that reads without writing therefore never acknowledges a NewSessionTicket, KeyUpdate or connection ID message, and the peer keeps retransmitting what it is waiting to have acknowledged. RFC 9147 relies on those ACKs, so this is a protocol level break rather than a missed optimisation. Add wolfSSL_dtls13_do_scheduled_work() so such an application can send that work from its write thread, and wolfSSL_dtls13_pending_work() so it can tell when there is any. Both entry points ask the same helpers rather than each testing conditions of their own, so they cannot drift into the predicate promising work the pump then declines or silently discards, which would leave a drain loop spinning or mislead the caller about what happened. That covers key updates in particular: none is sent while one of ours is unacknowledged, since DTLS must not have two in flight and Tls13UpdateKeys() drops a locally scheduled one in that state, and a peer request is kept rather than dropped until it can be answered. The predicate also errs towards reporting work when it cannot tell, so a loop surfaces the error rather than stopping silently. Refusing an object is treated as a usage error and leaves ssl->error alone. That field is sticky, since SendData() only clears it for WANT_WRITE, pending async work and the DTLS MAC and decrypt cases, and wolfSSL_write() skips its write-dup drain while it is set, so recording one would disable the very drain a write-dup application depends on. Write-dup pairs are out of scope on both sides. They park the read side's work in the shared WriteDup struct, which only wolfSSL_write() reconciles, so they already have a drain. Completing a key update we started ourselves is out of scope too: that needs the peer's acknowledgement processed, which rotates the sending keys and creates an epoch, and the epoch table has no locking while the read thread mutates it as well. The declaration is gated to match where the definitions live, so a lean build is not promised a symbol it does not get.
Several DTLS tests drive a connection with reads alone and then assert that something was sent, an ACK in most cases. With WOLFSSL_RW_THREADED that only happens once the application asks for it, so stand in for such an application and pump where the send is expected. The helper is a no-op elsewhere, so builds whose read path sends for itself are unchanged. test_dtls13_ack_overflow needs the same treatment in its setup, where the ACK the first reads scheduled would otherwise be left in the seen-record list and counted by the assertions that follow. It sits in the dtls13 group rather than dtls, so a run of the dtls group alone does not cover it. Add a test for the new API that runs in every build rather than only the threaded one. It schedules a key update the way the AEAD failure limit does and requires the predicate to report it, the pump to perform it and put a record on the wire, and the wait for the peer's acknowledgement not to be reported as work. It then drives the state that would wedge a drain loop, a peer requesting a KeyUpdate while ours is unacknowledged, and requires pump and predicate to agree that nothing can be sent and the request to be kept until it can. It also covers the bad argument cases, a DTLS 1.2 object being refused rather than quietly succeeding, and that refusing an object records no error against the connection and leaves it usable. The AEAD limit test excludes its second key update and its hard limit check from threaded builds. Both need the acknowledgement processing that stays off the write path: without it the decrypting epoch stops matching the one the drop counter is placed on, so the read never reaches the limit and the test hangs rather than failing.
Contributor
Author
Fixed |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
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.
Summary
Ten of the twelve commits fix distinct Fenrir-found memory-safety, key-hygiene, and timing issues in wolfSSL and wolfCrypt, each one independent and reviewable on its own. The last two add a small DTLS 1.3 API and test coverage for a protocol-level gap that only appears under
WOLFSSL_RW_THREADED.Fixes
src/internal.cTimingPadVerifypassedssl->hmaca length that underflows when the padding byte exceeds the record, so an out-of-range byte skipped the MAC entirely while a valid one paid for a full HMAC. That is a Lucky13-style timing oracle. Now clamped in constant time; the rejection decision is unchanged.src/x509.cX509PrintDirTypemeasured a raw-DERdirectoryNamewithXSTRLEN. A measured length below five wrapped an unsigned loop bound to nearUINT32_MAXand the tag scan ran far past the entry. Reached from ordinary certificate printing. Now usesentry->len.src/x509.cwolfSSL_X509_check_hostaccepts a length-delimited buffer, then handed it toCheckIPAddr, which discards the length and re-measures withXSTRLEN, reading past the end on every call. Now callsCheckHostNamewith the caller's length, which also fixes a missediPAddressmatch.wolfcrypt/src/srp.cmp_forcezeroon uninitialized memory and took the length from an unset size field. That is a write of arbitrary length past the allocation. Now gated on an init flag.wolfcrypt/src/hpke.csrc/ssl.cwolfSSL_clearleft the previous connection's key material in a recycled object: theKeysstruct, the TLS 1.3 traffic secrets, the DTLS 1.3 epoch keys, the handshake arrays and the tls-unique binding. All now force-zeroed, with the arrays wiped in place so the post-connection accessors keep working.src/ssl_crypto.c,src/ssl_api_ext.cWOLFSSL_HMAC_CTXkeeps the HMAC pads outside the wolfCrypt object, and for any key up to the block size the key falls out of a single exclusive or. Cleanup never wiped them, soHMAC_CTX_freehanded the key back to the allocator. The session ticket key callback leaked the ticket HMAC key onto the stack the same way, on every encrypt and decrypt.wolfcrypt/src/ecc.cwc_ecc_ctx_resetcleared the heap hintwc_ecc_ctx_new_exhad just recorded, so the context was freed to the default allocator rather than the heap it came from. Under static memory that is a free of memory never allocated there.wolfcrypt/src/rng_bank.cwc_rng_bank_initpersonalized each DRBG from&ctx->rngs[i], hashing the just-cleared struct instead of the instance address, so every instance in the bank got the same all-zero string. Now matcheswc_rng_bank_inst_reinit.src/ssl_api_dtls.c,src/dtls.c,src/internal.cwolfSSL_dtls_set_pending_peermutated shared state under a read lock, which admits several holders, so two threads could both freependingPeer.sawhile the send path read it. Now takes the write lock.DtlsResetStatetook no lock at all and now holds it too.DTLS 1.3 scheduled work
Under
WOLFSSL_RW_THREADEDthe read path performs no scheduled work, because transmitting from the reader would race the write thread over the output buffer and the sending key schedule, neither of which is covered by a lock. Post-handshake the only remaining consumers sit on the write path, andwolfSSL_dtls_retransmit()only helps while the handshake is unfinished. An application that reads without writing therefore never acknowledges a NewSessionTicket, KeyUpdate or connection ID message, and the peer keeps retransmitting. RFC 9147 depends on those ACKs, so this is a protocol break rather than a missed optimisation.This adds two entry points so such an application can drive that work from its write thread:
wolfSSL_dtls13_pending_work()reports whether anything is waiting.wolfSSL_dtls13_do_scheduled_work()sends it.Both ask the same helpers rather than each testing conditions of their own, so the predicate cannot drift into promising work the pump then declines, which would leave a drain loop spinning. The predicate errs towards reporting work when it cannot tell, so a loop surfaces an error rather than stopping silently. Refusing an object is treated as a usage error and deliberately leaves
ssl->erroralone: that field is sticky, andwolfSSL_write()skips its write-dup drain while it is set, so recording an error there would disable the very drain a write-dup application depends on.Two things are deliberately out of scope. Write-dup pairs already park the read side's work in the shared
WriteDupstruct, whichwolfSSL_write()reconciles, so they have a drain. Completing a key update we started ourselves needs the peer's acknowledgement processed, which rotates the sending keys and creates an epoch, and the epoch table has no locking while the read thread mutates it as well. That is a larger change and is not attempted here.These last two commits have no finding ID behind them. They came out of investigating why
test_wolfSSL_dtls_AEAD_limitfails underWOLFSSL_RW_THREADED, which it does on master as well.