wolfCrypt security hardening, portability fixes, and negative test coverage - #10958
Merged
Conversation
|
Frauschi
force-pushed
the
fenrir
branch
2 times, most recently
from
July 22, 2026 08:40
1521775 to
7d339d2
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #10958
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
force-pushed
the
fenrir
branch
2 times, most recently
from
July 22, 2026 11:07
55947e5 to
d7d80ad
Compare
philljj
self-requested a review
July 31, 2026 15:10
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: COMMENT
Findings: 10 total — 10 posted, 0 skipped
10 finding(s) posted as inline comments (see file-level comments below)
Posted findings
- [Medium] ECDH infinity check is skipped for a zero scalar, which is the only way infinity occurs on prime-order curves —
wolfcrypt/src/ecc.c:5046-5055 - [Medium] Infinity rejection is absent from the SP shared-secret paths, so the hardening does not apply to P-256/384/521 in SP builds —
wolfcrypt/src/ecc.c:5046-5055 - [Medium] DH temporary RNG hand-rolls the allocate/init/free sequence instead of using wc_rng_new_ex()/wc_rng_free() —
wolfcrypt/src/dh.c:2673-2706 - [Medium] wc_DhSetKey_ex now instantiates a DRBG per call and silently downgrades to the fixed-base test when it cannot —
wolfcrypt/src/dh.c:2673-2695 - [Medium] New DH pseudoprime test asserts an outcome the implementation only provides on a best-effort basis —
tests/api/test_dh.c:330-350 - [Low] *PSoC6: new attacker-reachable early return leaves verif_res unmodified —
wolfcrypt/src/port/cypress/psoc6_crypto.c:2110-2114 - [Low] New PSoC6 rejection returns a positive error value (-BAD_FUNC_ARG) —
wolfcrypt/src/port/cypress/psoc6_crypto.c:2113 - [Low] Two new lines exceed the 80-column source-text limit —
wolfcrypt/src/dh.c:2684 - [Low] Japanese doxygen for ecc.h not updated alongside the English copy —
doc/dox_comments/header_files/ecc.h:319-320 - [Low] ESP32-C3 register fix and other hardware-port changes have no automated coverage —
wolfcrypt/src/port/Espressif/esp32_mp.c:567-576
Review generated by Skoll
psoc6_ecc_verify_hash_ex serialized the signature r and s components into a fixed 132-byte stack buffer using mp_to_unsigned_bin without checking their sizes. The values come from attacker-supplied ASN.1 in DecodeECC_DSA_Sig with no magnitude cap beyond sp_int capacity, so an oversized r or s wrote past signature_buf, a pre-authentication stack overflow reachable during TLS signature verification. The generic path guards this with wc_ecc_check_r_s_range, but that check is compiled out on WOLFSSL_PSOC6_CRYPTO builds and the port function performed no r/s validation of its own. Reject any r or s whose serialized size exceeds the key size before writing into the buffer. Fixes F-6778.
wc_Chacha_Process validated only its pointer arguments and then produced keystream directly from the context state. A zero-initialized ChaCha context, common for static or global storage, that received a nonce via wc_Chacha_SetIV but never had wc_Chacha_SetKey called would encrypt with an all-zero, attacker-predictable key and still return success. This is the same fail-open class already guarded against in wc_Arc4Process. Add a keySet flag to the ChaCha struct, set it in wc_Chacha_SetKey, and return MISSING_KEY from wc_Chacha_Process when the key was never set. Fixes F-6893.
wc_ecc_shared_secret_gen_sync ran the scalar multiplication and then copied the x-coordinate to the output without checking whether the result was the point at infinity. Both math backends report success for the identity: ecc_map_ex sets x, y to zero and z to one and returns success, and the single precision generators serialize the identity as an all-zero x-coordinate. Either way a shared secret that computed to infinity was handed back as an all-zero secret with a success code, where SP 800-56Ar3 5.7.1.2 requires an error and stop. Check the mapped point on the software path, and detect the all-zero output after the single precision generators, returning ECC_INF_E in both cases. The scan accumulates over the whole buffer so it does not branch on the secret. A key whose private value is resident in an SE050 carries no software scalar, so the software multiply legitimately yields the identity for it. Skip the check for those keys specifically, rather than for a zero scalar: on a prime-order curve a zero scalar is the one way the identity can arise, so exempting it would disable the check for the case it exists to catch. Fixes F-6770.
wc_DhSetKey_ex loads DH parameters as untrusted and validates that the modulus is prime, but it passed no RNG, so the check fell back to a Miller-Rabin test using the fixed small-prime bases 2 through 19. That test is defeatable: a composite crafted as a strong pseudoprime to those known bases passes as prime, letting an attacker supply a composite modulus with a smooth factorization for small-subgroup recovery of the private exponent and shared secret. When no RNG is supplied on the untrusted path, create a temporary RNG so mp_prime_is_prime_ex runs with random witnesses, which such crafted composites cannot reliably pass. Named FFDHE primes still short-circuit the check, and builds without an RNG keep the deterministic test. Fixes F-6776.
wc_ecc_check_key validates a public key's coordinate range, that the point is on the curve, and its order, but the software path had no negative coverage: the existing test only exercised a valid key and NULL, and the off-curve case lived in the crypto-callback test, which validates the device path rather than the software on-curve check. A deletion of either the on-curve check or the coordinate-range checks therefore passed the suite. Add a test that imports secp256r1 public keys that are off the curve and out of coordinate range, asserting IS_POINT_E and ECC_OUT_OF_RANGE_E respectively, exercising the software validation path. Fixes F-6620.
Ed448 verification rejects a non-canonical signature scalar S (S >= L) per RFC 8032, and that range check is the only guard against a malleated signature: because L times the base point is the identity, (R, S + L) recomputes the same R and would otherwise verify. The check had no negative coverage, so a deletion or boundary mutation passed the suite while all canonical KAT signatures kept working. Add a test that signs a message, then verifies crafted signatures whose S half equals the order, exceeds it in a high or low byte, and equals S + L, asserting BAD_FUNC_ARG, plus an in-range wrong S asserting SIG_VERIFY_E. Fixes F-6777.
IntelQaSymCipher ran cpaCySymPerformOp synchronously but never translated its completion status into the return code. Only the AES-GCM decrypt auth check, driven by verifyResult, could set an error. For AES-CBC, AES-GCM encrypt, and 3DES-CBC a non-success status left ret at zero, so the exit path copied the working buffer to the output and returned success. On encrypt that buffer still holds the plaintext copy, so a hardware failure returned success with plaintext written to the ciphertext output. Translate a non-success perform-op status into ASYNC_OP_E, matching the asynchronous port, so hardware failures are surfaced instead of returning zero with unprocessed output. Fixes F-6623.
The ESP32-C3 deactivation path in esp_mp_hw_unlock passed its arguments to DPORT_REG_CLR_BIT and DPORT_REG_SET_BIT in the wrong order and added a stray DR_REG_RSA_BASE offset. The macros take (register address, bit mask), but the code used the bit mask as part of the register address and the register as the bit mask. As a result the RSA clock-enable and memory power-down bits were never updated at deactivation, and reads and writes landed at a bogus peripheral address. The accelerator was left powered up after every bignum and RSA operation. Pass the register address first and the bit mask second and drop the offset, mirroring the activation path and the other targets. Fixes F-6779.
wc_InitRsaKey_Id cast the byte array key->id to word32* and dereferenced it to recover the SE050 key id. key->id is a byte array with no alignment guarantee inside RsaKey, so the dereference is an unaligned 32-bit read that faults or mis-reads on strict-alignment targets, and it violates strict aliasing. Read the value with readUnalignedWord32 instead, which does an alignment-safe byte copy, matching wc_ecc_init_id. Fixes F-6624.
The little-endian large-code path of mlkem_cbd_eta3 cast the caller-supplied byte buffer to word32* and read through it. The buffer has no alignment guarantee, so on strict-alignment targets such as Cortex-M3 and M4 with unaligned-access trapping enabled the read faults or returns wrong values, and the cast violates strict aliasing. Read each word with readUnalignedWord32, which does an alignment-safe byte copy, matching the neighboring mlkem_cbd_eta2. Fixes F-6781.
The little-endian large-code path of mlkem_vec_compress_10_c cast the output byte buffer to word32* and issued five 32-bit stores through it. That buffer is the caller-supplied ML-KEM ciphertext, which has no alignment guarantee, so on strict-alignment targets the store bus-faults and the cast violates strict aliasing. Write each word with writeUnalignedWord32, which does an alignment-safe byte copy, matching mldsa_encode_w1_88_c and the neighboring ML-KEM sampling code. Fixes F-6782.
dgarske
approved these changes
Aug 3, 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.
Summary
This branch is a batch of eleven independent fixes across wolfCrypt and several hardware ports, from a security review. They fall into four groups: fail-open/validation hardening, undefined-behavior/portability fixes, hardware-port correctness, and negative regression test coverage. Each change is a self-contained commit; there is no shared refactoring.
Validation and fail-open hardening
wc_Chacha_Processhad no key-state check, so a zero-initialized context that received only a nonce would encrypt with an all-zero, attacker-predictable key and return success. AkeySetflag is now set inwc_Chacha_SetKeyand checked inwc_Chacha_Process, which returnsMISSING_KEY, mirroringwc_Arc4Process.wc_DhSetKey_exloads parameters as untrusted but ran a fixed-base primality test (bases 2..19) when no RNG was supplied, which a crafted strong pseudoprime can pass. It now creates a temporary RNG somp_prime_is_prime_exruns with random witnesses, and falls back to the deterministic test only when no RNG can be obtained, so there is no new failure mode. Named FFDHE primes still short-circuit.wc_ecc_shared_secret_gen_synccopied the affine x-coordinate to the output without checking for the identity point, so a shared secret that computed to infinity was returned as an all-zero secret with a success code. It now returnsECC_INF_E, as required by SP 800-56Ar3 section 5.7.1.2.Undefined behavior and portability
wc_InitRsaKey_Idcast thebyte id[]array toword32*and dereferenced it, an unaligned 32-bit read that faults or misreads on strict-alignment targets and violates strict aliasing. It now usesreadUnalignedWord32, matching the existingwc_ecc_init_id.mlkem_cbd_eta3(input) andmlkem_vec_compress_10_c(output ciphertext) reinterpreted caller byte buffers throughword32*pointers. Both now usereadUnalignedWord32/writeUnalignedWord32, matching the neighboringmlkem_cbd_eta2andmldsa_encode_w1_88_c.Hardware-port correctness
psoc6_ecc_verify_hash_exwrote attacker-suppliedr/sof unbounded size into a fixed 132-byte stack buffer, a pre-authentication stack overflow reachable during TLS signature verification. It now rejects any component larger than the key size, matching the generic path'swc_ecc_check_r_s_range.IntelQaSymCiphernever translated thecpaCySymPerformOpcompletion status into its return code, so a hardware failure returned success with the plaintext copy written to the ciphertext output. A non-success status now yieldsASYNC_OP_E, matching the asynchronous port.Negative test coverage
wc_ecc_check_key. Added a test that imports secp224r1 public keys that are off the curve and out of coordinate range, assertingIS_POINT_EandECC_OUT_OF_RANGE_E. This closed a mutation-survival gap: the software on-curve and range checks previously had no negative coverage in a software build.Sequals or exceeds the group order, including the malleability caseS + L, asserting rejection. Without the range check,(R, S + L)verifies as a second valid signature for the same message.