Open
Conversation
Frauschi
force-pushed
the
zd21460
branch
7 times, most recently
from
9695426 to
a46a2f6
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #10102
Scan targets checked: src, src-bugs, src-compliance, wolfcrypt-api_misuse, wolfcrypt-bugs, wolfcrypt-compliance, wolfcrypt-concurrency, wolfcrypt-portability, wolfcrypt-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.
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #10102
Scan targets checked: src, src-bugs, src-compliance, wolfcrypt-api_misuse, wolfcrypt-bugs, wolfcrypt-compliance, wolfcrypt-concurrency, wolfcrypt-portability, wolfcrypt-src
Findings: 2
2 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
zd21460
branch
2 times, most recently
from
9f0c33c to
2ab737b
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #10102
Scan targets checked: src, src-bugs, src-compliance, wolfcrypt-api_misuse, wolfcrypt-bugs, wolfcrypt-compliance, wolfcrypt-concurrency, wolfcrypt-portability, wolfcrypt-src
Findings: 2
2 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
The guard `if (cmac->totalSz != 0)` was used to skip XOR-chaining on the first block (where digest is all-zeros and the XOR is a no-op). However, totalSz is word32 and wraps to zero after 2^28 block flushes (4 GiB), causing the guard to erroneously fire again and discard the live CBC-MAC chain state. Any two messages sharing a common suffix beyond the 4 GiB mark then produce identical CMAC tags, enabling a zero-work prefix-substitution forgery. The fix removes the guard, making the XOR unconditional; the no-op property on the first block is preserved because digest is zero-initialized by wc_InitCmac_ex. Identified by: Nicholas Carlini (Anthropic) & Thai Duong (Calif.io)
wc_VerifyEccsiHash did not validate that r and s lie in [1, q-1] after decoding them from the signature buffer. With s=0 the scalar multiplication [s](...) returns the point at infinity (J_x=0); with r=0 the final mp_cmp(0,0)==MP_EQ check then accepts the forged signature unconditionally against any message and any identity. Add [1, q-1] range checks for r (in wc_VerifyEccsiHash, after params are loaded) and for s (in eccsi_calc_j, after eccsi_decode_sig_s), mirroring the checks already present in wc_ecc_check_r_s_range. Add a defense-in-depth point-at-infinity guard on J before the final comparison. Reported-by: Nicholas Carlini (Anthropic) & Bronson Yen (Calif.io)
EVP_DecryptFinal_ex() called wc_ChaCha20Poly1305_Final() which only computes the Poly1305 tag, writing it into ctx->authTag and overwriting the expected tag stored there by EVP_CTRL_AEAD_SET_TAG. No comparison was ever performed, so any forged tag was accepted. Fix: save the expected tag before calling Final(), then verify with wc_ChaCha20Poly1305_CheckTag() on the decrypt path, mirroring the existing AES-GCM branch. Add a regression test that asserts EVP_DecryptFinal_ex() rejects an all-zero forged tag. Reported-by: Nicholas Carlini (Anthropic) & Bronson Yen (Calif.io)
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #10102
Scan targets checked: src, src-bugs, src-compliance, wolfcrypt-api_misuse, wolfcrypt-bugs, wolfcrypt-compliance, wolfcrypt-concurrency, wolfcrypt-portability, wolfcrypt-src
No new issues found in the changed files. ✅
Contributor
|
only failing subtest is multi-test PRB |
douzzer
requested changes
Apr 3, 2026
Contributor
douzzer
left a comment
douzzer
left a comment
There was a problem hiding this comment.
"ecc: fix invalid-curve attack via missing on-curve validation" is causing an intolerable regression in ECC throughput:
benchmark-wolfcrypt-intelasm-sp-asm-all:
3 asym alg(s) not fast enough in any trial:
ECDHE[SECP256R1],256,agree -- best trial 49.27% of baseline, Z+1241.91
ECC[SECP256R1],256,encrypt -- best trial 50.89% of baseline, Z+573.18
ECC[SECP256R1],256,decrypt -- best trial 34.30% of baseline, Z+1831.97
We'll need to find a middle ground for validation that doesn't clobber performance.
wc_PKCS7_DecodeAuthEnvelopedData() accepted an attacker-controlled GCM tag length from the mac OCTET STRING and did not validate it against the parsed aes-ICVlen parameter. In parallel, wc_AesGcmDecrypt() accepted very short tags on decrypt while encrypt enforced WOLFSSL_MIN_AUTH_TAG_SZ. This made short-tag verification reachable through CMS AuthEnvelopedData and weakened integrity checks by allowing tag truncation. Fixes: - validate parsed macSz range in AuthEnvelopedData decode - require authTagSz to match parsed macSz - reject undersized GCM tags in PKCS7 decode - enforce WOLFSSL_MIN_AUTH_TAG_SZ in wc_AesGcmDecrypt() and wc_AesGcmDecryptFinal() Also add a regression test in pkcs7authenveloped vectors that truncates the final MAC OCTET STRING length from 16 to 1 and verifies decode fails. Reported by: Nicholas Carlini (Anthropic) & Thai Duong (Calif.io)
When an untrusted issuer has CA:FALSE and no verify_cb is registered, the !isCa branch now fails closed (ret=WOLFSSL_FAILURE, goto exit) instead of falling through and skipping X509StoreVerifyCert for the leaf. SetupStoreCtxError_ex is also hardened to never overwrite a previously recorded error with success, preventing a later valid chain link from clobbering ctx->error back to X509_V_OK. Tests added for both the no-callback rejection and the error-preservation cases. Reported by: Nicholas Carlini (Anthropic) & Thai Duong (Calif.io)
In TLSX_EchChangeSNI, the ctx->extensions branch set extensions unconditionally even when TLSX_Find returned NULL. This caused TLSX_UseSNI to attach the attacker-controlled publicName to the shared WOLFSSL_CTX when no inner SNI was configured. TLSX_EchRestoreSNI then failed to clean it up because its removal was gated on serverNameX != NULL. The inner ClientHello was sized before the pollution but written after it, causing TLSX_SNI_Write to memcpy 255 bytes past the allocation boundary. Fix by mirroring the guarded pattern of the ssl->extensions branch: only set extensions when TLSX_Find returns non-NULL, and only perform the SNI swap when extensions is non-NULL. Also move TLSX_Remove in TLSX_EchRestoreSNI outside the serverNameX guard so any injected publicName SNI is always cleaned up. Also return BAD_FUNC_ARG when ECH is used without an inner SNI, preventing ECH ClientHello construction in an invalid configuration. Reported by: Nicholas Carlini (Anthropic) & Thai Duong (Calif.io)
When an EC_KEY is created via EC_KEY_new + EC_KEY_set_group + EC_KEY_set_private_key (no public point set), SetECKeyInternal incorrectly marks the internal ecc_key as ECC_PRIVATEKEY (instead of ECC_PRIVATEKEY_ONLY) because pub_key is always non-NULL — EC_KEY_new always allocates it as an empty, zero-initialised EC_POINT. ECC_populate_EVP_PKEY only calls wc_ecc_make_pub for ECC_PRIVATEKEY_ONLY keys, so the zero public-key point was serialised into the DER stored in pkey->pkey.ptr. After commit 929dd99 made wc_ecc_import_x963_ex always pass untrusted=1, the re-decode inside wolfSSL_EVP_PKEY2PKCS8 → wolfSSL_d2i_PrivateKey_EVP correctly rejected that zero point with an on-curve failure, causing EVP_PKEY2PKCS8 to return NULL. Fix: in ECC_populate_EVP_PKEY, also call wc_ecc_make_pub when the key type is ECC_PRIVATEKEY but pubkey.x is zero (meaning the public key was never actually populated). This reconstructs the public key from the private scalar so that the encoded DER contains a valid on-curve point.
Contributor
|
I removed one ECC commit due to a regression in performance that needs investigated farther. The other commits stayed exactly the same. |
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
5 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.
zd21460