Make --enable-all --enable-asynccrypt-sw work end to end - #11060
Merged
Conversation
ecc_encrypt_cryptocb_test() calls wc_ecc_make_key() twice and checks the return code directly. Under WOLFSSL_ASYNC_CRYPT the software simulator returns WC_PENDING_E from that call, so the test aborts with -108 before it reaches a single ECIES operation. The sibling ecc_encrypt_test() right below it already wraps the same two calls in wc_AsyncWait(); this one was added later and never got the same treatment. Mirror the sibling exactly: wc_AsyncWait() on each key's asyncDev with WC_ASYNC_FLAG_NONE, guarded by WOLFSSL_ASYNC_CRYPT so non-async builds are untouched by the preprocessor. The failure needs --enable-all together with the software async simulator. --enable-all turns on cryptocb, which suppresses the auto-enabled simulator in configure.ac, so the simulator only appears if --enable-asynccrypt-sw is passed explicitly. No configuration under .github/ combines the two, which is why this was never seen in CI. Verified with --enable-all --enable-asynccrypt-sw under the CFLAGS the os-check workflow applies: "ECC Enc test failed! error code=-108" before, "ECC Enc test passed!" after. Rebuilt the same tree as plain --enable-all to confirm the non-async path is unaffected, where testwolfcrypt passes in full. Fixing this uncovers the next test to run, ECCSI, failing the same way. That one is a library defect rather than a test defect and is fixed in the following commit.
wc_MakeEccsiKey() returns whatever wc_ecc_make_key_ex() returns. In an async build the ECC key generation goes pending and the caller receives WC_PENDING_E, but ECCSI exposes no asynchronous API, so there is nothing the caller can do with it. There is no ECCSI event to poll, no devId to wait on that the caller is given, and no documented resume path. The error is simply unusable. Wait for the operation to finish before returning, so wc_MakeEccsiKey() keeps the synchronous contract the rest of the ECCSI API assumes. Only this one call site needs it. eccsi.c reaches exactly three of the ECC entry points that can go pending, all of them wc_ecc_make_key_ex(), and the other two in eccsi_make_pair() and eccsi_gen_sig() are preceded by wc_ecc_free(&key->pubkey). That clears asyncDev.marker, and the pending path in _ecc_make_key_ex() is gated on the marker still being WOLFSSL_ASYNC_MARKER_ECC, so those two cannot return WC_PENDING_E. Adding a wait there would be dead code. sakke.c reaches none of the four entry points at all. Found while validating an unrelated change under --enable-all with --enable-asynccrypt-sw. No configuration under .github/ combines those two, because --enable-all turns on cryptocb and configure.ac only auto-enables the software simulator when no backend is set, so this went unnoticed. Verified with --enable-all --enable-asynccrypt-sw under the CFLAGS the os-check workflow applies: testwolfcrypt reported "ECCSI test failed! error code=-108" before and reports "ECCSI test passed!" after, with SAKKE passing as well. Reconfigured the same tree as plain --enable-all to confirm the non-async path is untouched, where make check passes in full. That configuration still cannot complete testwolfcrypt. The next failure is cryptocb_test(), which re-runs whole sub-suites through a crypto callback and touches no ECCSI or SAKKE code. It is a separate problem and is not addressed here.
cryptocb_test() generates a key with wc_ecc_make_key() and assigns the result straight to ret. In an async build that call returns WC_PENDING_E, which is not an encoded test result, so the raw -108 propagated out of the test and printed as "error L=108" with no error code at all. The key is reached through myCryptoDevCb, which services EC key generation by calling wc_ecc_make_key_ex() on the same key after setting key->devId = INVALID_DEVID. That comment says the intent is to force software, and it does stop the crypto callback from dispatching again, but the pending path in _ecc_make_key_ex() is gated on asyncDev.marker rather than devId. The marker is untouched, so the inner call still goes pending and the callback hands WC_PENDING_E back to its caller. Wait at the call site rather than in the callback. Every other key generation in this file already does exactly that, a callback returning WC_PENDING_E is legitimate for a real asynchronous device, and the same devId idiom appears 48 times in myCryptoDevCb against 48 different keys, so there is no single place in the callback to fix. With this, testwolfcrypt passes in full under --enable-all with --enable-asynccrypt-sw, where it previously stopped here. Verified against plain --enable-all as well, which is unaffected: the addition compiles out entirely without WOLFSSL_ASYNC_CRYPT. That configuration still cannot complete make check. unit.test fails in the cipher suite runner on TLS 1.3 post-handshake authentication, which is a record layer problem in the library rather than a test defect and is not addressed here. All 2111 API tests pass.
A TLS 1.3 client doing post-handshake authentication fails the next read with -326 VERSION_ERROR in an asynchronous build, and the connection dies. It is reproducible with the shipped examples: ./examples/server/server -v 4 -l TLS13-AES128-GCM-SHA256 -Q -2 -p 11119 & ./examples/client/client -v 4 -l TLS13-AES128-GCM-SHA256 -Q -2 -p 11119 DoTls13CertificateRequest() handles a post-handshake CertificateRequest by resetting the handshake states, setting processReply back to doProcessInit and calling wolfSSL_connect_TLSv13() to send the certificate, certificate verify and finished flight. It runs that from inside DoTls13HandShakeMsg(), which is itself inside DoProcessReplyEx(). When the flight goes pending, WC_PENDING_E travels back out of DoTls13HandShakeMsg() and DoProcessReplyEx() returns immediately, so the end of record accounting that follows never runs and the record is left short by ssl->keys.padSz bytes. The client resumes through wolfSSL_negotiate() from ReceiveData() rather than by reprocessing the record, so nothing ever advances past those bytes. The next ProcessReply() starts a fresh record at that offset and parses the record's own MAC as a record header, which fails the version check in GetRecordHeader(). Measured on the failing read: idx 50, length 67, padSz 17, with the header bytes decoding as type 0x16 and version 0x70 0xEA. Advance past the padding when the handler returns pending having already marked the record complete. processReply is what distinguishes the two cases: post-handshake auth leaves it at doProcessInit, while an ordinary pending message leaves it at runProcessingOneMessage and must not be advanced, since that one really is reprocessed on resume. Confirmed by tracing every pending return in the failing run: three ordinary ones at runProcessingOneMessage, and only the post-handshake one at doProcessInit, where idx plus padSz lands exactly on the buffer length. This mirrors what the early data path a few lines below already does when it returns early after marking the record done. Synchronous builds are unaffected. wolfSSL_connect_TLSv13() completes in place there, DoTls13HandShakeMsg() returns 0, and the normal accounting runs.
Writing application data over DTLS fails with -132 BUFFER_E in an asynchronous build once a record goes pending. It is reproducible with the shipped examples: ./examples/server/server -e -u -f -v 3 -l ECDHE-RSA-AES128-SHA -2 -p 11151 & ./examples/client/client -B 4000,1310 -u -f -v 3 -l ECDHE-RSA-AES128-SHA -2 -p 11151 The client reports "Oops, want to write past output buffer size" and the write fails. This is the DTLS MTU configuration from tests/test-dtls-mtu.conf. SendData() sizes the output buffer by calling wolfssl_local_GetRecordSize(), which runs BuildMessage() with sizeOnly set and asyncOkay clear. That probe keeps its own arguments on the stack, but the state machine it drives lives in ssl->options.buildMsgState, which is shared with the asynchronous BuildMessage() that may still be in flight for the same record. Because SendData() re-probes the size on every retry, the sequence is: the real BuildMessage() suspends at BUILD_MSG_ENCRYPT, the retry's probe resets the shared state to BUILD_MSG_BEGIN and leaves it there, and the resumed call then re-runs the begin and size stages against arguments that already carry the header and cipher overhead. For a 1310 byte payload with ECDHE-RSA-AES128-SHA that takes the header from 13 to 21 bytes and the record from 1361 to 1417, which no longer fits the 1361 byte buffer the first probe correctly sized. Save the state around the probe and put it back afterwards. The probe still needs to start from BUILD_MSG_BEGIN to compute a size, so restoring is the narrowest fix; the arguments themselves are already private to the probe. Verified with --enable-all --enable-asynccrypt-sw: the reproducer above goes from a failed write to completing its benchmark. Full make check in that configuration passes, 17 passed and 6 skipped with no failures, though that run also needs the wolfcrypt test fixes for ECIES, ECCSI and the crypto callback test, which that configuration trips over first and which are not part of this change. Non-async builds are unaffected, since the saved value is only read and written under WOLFSSL_ASYNC_CRYPT.
Review follow-up on the two preceding commits. Only skip the padding when the record content is actually consumed. The post-handshake auth advance ran on any pending return that left processReply at doProcessInit, which assumes the certificate_request was the last message in its record. Two states break that: a fragmented certificate_request makes DoTls13HandShakeMsg() rewind inOutIdx so the fragment can be reprocessed, and RFC 8446 section 5.1 lets a peer coalesce several handshake messages into one record, leaving the index inside the record. Adding padSz in either case points the index at record content. Both states are already mishandled without this series, since post-handshake auth forces processReply to doProcessInit regardless, so this is a narrower guard rather than a regression, and there is no memory safety consequence either way: the largest possible index is the end of the record. The new test mirrors the end of record check that follows the message handlers. Also restore buildArgsSet across the record size probe. The probe borrows buildMsgState and buildArgsSet from the suspended build; the previous commit put back only the first. FreeBuildMsgArgs() clears buildArgsSet unconditionally on the way out, and the resumed call skips the block that would set it again, so the flag stays clear for the rest of the record. That is currently harmless, because the only thing it guards is freeing a dynamically allocated IV and no cipher in the tree needs one, but it leaves the fix one field short of its own premise. Add test_record_size_preserves_build_msg_state, which parks a connection at BUILD_MSG_ENCRYPT with the arguments flag set, runs the probe, and requires both fields to survive. It fails without the restore and passes with it. Guard the post-handshake auth block on WOLFSSL_POST_HANDSHAKE_AUTH as well. The only code that leaves processReply at doProcessInit from inside DoTls13HandShakeMsg() is the post-handshake auth branch, which is itself behind that macro, so the check cannot fire without it. Say that the oversized record is rejected rather than that it overflows the buffer. BuildMessage() catches it and returns BUFFER_E; the old wording read like memory corruption. Verified with --enable-all --enable-asynccrypt-sw and with plain --enable-all, both warning free under the os-check CFLAGS. The two reproducers from the preceding commits still complete.
Nothing in CI exercised an asynchronous build that actually returns WC_PENDING_E from a full feature set, which is why the five preceding fixes all describe failures no workflow could see. The async workflow has two groups and neither reaches these paths. The asynccrypt-all entries pass --enable-asynccrypt --enable-all, but --enable-all turns on cryptocb, and configure.ac only auto-enables the software simulator when cryptocb, pkcallbacks, Cavium and Intel QA are all off. Those builds therefore define WOLFSSL_ASYNC_CRYPT without ever suspending an operation. The asynccrypt-sw entries do suspend, but they build only OCSP stapling, so they compile neither TLS 1.3 post-handshake authentication nor DTLS. Add one entry that pairs --enable-asynccrypt-sw with --enable-all and --enable-dtls13, which covers both reproducers: the post-handshake auth case in tests/test-tls13.conf and the DTLS MTU case in tests/test-dtls-mtu.conf. Measured at 1.6 minutes locally. Declared as 3 to match the neighbouring asynccrypt-all entries, which measure 1.4 locally against their declared 3, so this machine runs roughly twice as fast as whatever those values were taken from and 1.6 here projects to about 3.4 there. The value is only a scheduling weight, and the existing entries are left alone rather than rewritten from local timings.
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11060
Scan targets checked: wolfcrypt-bugs, wolfcrypt-port-bugs, wolfcrypt-rs-bugs, wolfcrypt-src, wolfssl-bugs, wolfssl-src
Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
Frauschi
commented
Aug 5, 2026
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11060
Scan targets checked: wolfcrypt-bugs, wolfcrypt-port-bugs, wolfcrypt-rs-bugs, wolfcrypt-src, wolfssl-bugs, wolfssl-src
Findings: 1
High (1)
EccVerify still dereferences NULL key on async pending path
File: src/internal.c:6208
Function: EccVerify
Category: NULL pointer dereference
The PR guards wolfSSL_AsyncInit with if (key) but the WC_PENDING_E handler a few lines later still does wolfSSL_AsyncPush(ssl, &key->asyncDev) unconditionally; EccVerify is called with key = ssl->hsKey which can be NULL (e.g. the WOLFSSL_CHECK_SIG_FAULTS self-verify with a PK-callback-only private key), leading to a near-NULL dereference inside wolfSSL_AsyncPush.
Recommendation: Guard this call with if (key && ret == WC_PENDING_E), mirroring the pattern already used in EccSign.
Referenced code: src/internal.c:6208-6210 (3 lines)
This review was generated automatically by Fenrir. Findings are non-blocking.
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11060
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. ✅
Member
|
Jenkins retest this please "PRB-fips-ready-config" |
dgarske
requested changes
Aug 5, 2026
Second review follow-up on the async record layer series. Stop the probe reselecting the cipher side. BuildMessage()'s BUILD_MSG_BEGIN case can call SetKeysSide() for DTLS with secure renegotiation, which swaps the active encryption state and clears recordSzOverhead. That is not part of a size calculation, and after the previous commit the suspended build survives to resume against whatever side the probe last chose, so a DTLS 1.2 record suspended for PREV_ORDER could resume against the renegotiation keys. Skip it when sizeOnly is set; the sizes are the same either way. The probe itself has to keep running. Not re-entering BuildMessage at all while a build is suspended looks tidier, but wolfssl_local_GetMaxPlaintextSize() derives the DTLS fragment size from this result, so falling back to the upper bound there shrinks fragments inconsistently between calls and the MTU reproducer fails its buffer comparison. Saving and restoring the two fields is what keeps the answer exact. Resume inside the record when handshake content is left. The previous commit declined to skip the padding for a fragmented or coalesced certificate_request, which was right, but left processReply at doProcessInit with the index inside the record, so the resume still started a fresh record parse in the middle of one. Mirror both halves of the end of record block instead: set runProcessingOneMessage when content remains, advance past the padding only at the boundary. Note the shared state at the source. BuildMessage() and BuildTls13Message() write ssl->options.buildMsgState even for a sizeOnly probe with asyncOkay clear, where everything else goes to the caller's own arguments. Nothing said so at those sites, so the next sizeOnly caller would reintroduce this. Record why only one of the three wc_ecc_make_key_ex() calls in eccsi.c needs a wait: the other two are preceded by wc_ecc_free(), which clears the marker their pending path is gated on. Moving either free would make them pend. Test changes. Force the overhead cache cold before probing, otherwise an AEAD suite answers from the cache without ever calling BuildMessage and the assertions hold no matter what the probe did. Compare against BuildMessage's own figure rather than only checking the size is positive, and run the whole thing for TLS 1.3 as well as TLS 1.2, since BuildTls13Message() clobbers the state by a different route: its sizeOnly return bypasses exit_buildmsg entirely. Checked by stubbing the restore out again, which fails the test. Also spell the new guard in cryptocb_test() as #if defined(WOLFSSL_ASYNC_CRYPT) to match the rest of that file, which uses that form 170 times against 4.
EccVerify() dereferenced key unconditionally in both of its async spots:
ret = wolfSSL_AsyncInit(ssl, &key->asyncDev, WC_ASYNC_FLAG_CALL_AGAIN);
...
ret = wolfSSL_AsyncPush(ssl, &key->asyncDev);
Its siblings EccSign() and VerifyRsaSign() wrap both calls in if (key),
because with HAVE_PK_CALLBACKS the private key can live only in the
callback and is never decoded into ssl->hsKey. The self verify step that
WOLFSSL_CHECK_SIG_FAULTS performs after signing passes that same
ssl->hsKey to EccVerify(), so an --enable-asynccrypt build dereferences
NULL there: at the init on entry, and again at the push if EccVerifyCb
returns WC_PENDING_E. Note wc_ecc_verify_hash() cannot produce that
second case, since it rejects a NULL key before starting any async work.
The check on the push goes inside the pending branch rather than into its
condition, because unlike EccSign() this block has an else that maps a
non zero ret onto a verification failure. Extending the condition would
send a pending result down that else and log a bogus
WOLFSSL_ERROR_VERBOSE() for it.
Reproduced with --enable-asynccrypt --enable-pkcallbacks
--enable-faultharden --enable-ecc --enable-supportedcurves and
-DTEST_PK_PRIVKEY, which is what makes examples/server.c leave the
private key to the callback. Running examples/server against
examples/client with ECC certificates, the server takes SIGSEGV without
this change and exits cleanly with it, reaching the verify callback with
keySz 0 instead. The push needed one more step to reach: the in tree
myEccVerify() is synchronous, so it was patched to return WC_PENDING_E
once, after which the server dies in wolfAsync_EventQueuePush() at the
offset of ecc_key.asyncDev, reached from EccVerify() by way of
SendTls13CertificateVerify().
Note the crash is only reachable once the two callers stop dereferencing
ssl->buffers.key->length on the line just above the EccVerify() call,
which is what PR 11000 fixes. Both sites were patched locally to
reproduce. This guard is needed in addition to that fix, not instead of
it, and only matters for asynchronous builds.
dgarske
approved these changes
Aug 5, 2026
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
4 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.
Description
Nothing in CI builds an asynchronous configuration that actually returns
WC_PENDING_Efrom a full feature set.--enable-allturns on cryptocb, andconfigure.aconly auto-enables the software simulator when no backend is set, so theasynccrypt-allentries defineWOLFSSL_ASYNC_CRYPTwithout ever suspending an operation. Theasynccrypt-swentries do suspend, but they build only OCSP stapling, so they compile neither TLS 1.3 post-handshake authentication nor DTLS.Building that combination surfaced five defects: three in the wolfCrypt tests and ECCSI, two in the TLS record layer. This series fixes all five and adds the CI entry that covers them.
wolfCrypt
ecc_encrypt_cryptocb_test()checkedwc_ecc_make_key()directly while the siblingecc_encrypt_test()already waited. Mirrors the sibling.wc_MakeEccsiKey()(library) - returnedWC_PENDING_Eto a caller that cannot use it: ECCSI exposes no async API, no event to poll and no resume path. Waits before returning. Only this one of the threewc_ecc_make_key_ex()calls ineccsi.cneeds it; the other two are preceded bywc_ecc_free(), which clears the marker their pending path is gated on.cryptocb_test()- the mock device clearsdevIdbut notasyncDev.marker, so its inner keygen still goes pending and handsWC_PENDING_Eback to its own caller. Waits at the call site, as every other keygen in that file does.TLS record layer (
src/internal.c)DoTls13CertificateRequest()sends the client's flight from insideDoTls13HandShakeMsg(). When that flight goes pending,DoProcessReplyEx()returns before the end-of-record accounting, leaving the record short byssl->keys.padSz. The client resumes throughwolfSSL_negotiate()rather than by reprocessing the record, so the nextProcessReply()parses the record's own MAC as a record header and fails with-326 VERSION_ERROR. Fix mirrors both halves of the end-of-record block: setrunProcessingOneMessagewhen content remains, advance past the padding only at the boundary.SendData()sizes its buffer withBuildMessage(..., sizeOnly=1), which keeps its arguments on the stack but drives the sharedssl->options.buildMsgState. A re-probe on retry rewinds the state, so the resumed build re-runs its begin and size stages against arguments that already carry the overhead and overruns the buffer it correctly sized, failing with-132 BUFFER_E. Fix saves and restoresbuildMsgStateandbuildArgsSetaround the probe, and skipsSetKeysSide()whensizeOnlyis set.CI
Adds
asynccrypt-sw-all-dtls13(--enable-asynccrypt-sw --enable-all --enable-dtls13) to.github/workflows/async.yml, which covers both reproducers.Reproducers
Both need
./configure --enable-all --enable-asynccrypt-sw.Testing
--enable-all --enable-asynccrypt-sw:testwolfcryptpasses in full,make check17 passed / 6 skipped / 0 failed. Both reproducers complete.--enable-all:make check17 passed / 6 skipped / 0 failed. Non-async paths are unaffected; the changes are guarded byWOLFSSL_ASYNC_CRYPT.test_record_size_preserves_build_msg_state(TLS 1.2 and TLS 1.3) parks a connection atBUILD_MSG_ENCRYPT, runs the probe and requires the state to survive. Proven non-vacuous by stubbing the restore back out.pass [1.6 min]locally withparallel-make-check.py.