Repository navigation
Ensure the ECDSA signature scheme is bound to the certificate curve. Always validate MFL for TLS 1.3, even when WOLFSSL_OLD_UNSUPPORTED_EXTENSION is defined. - #11494
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The ECDSA validation compares curve sizes rather than exact curve identities, allowing same-sized mismatched curves.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Strengthens TLS 1.3 validation for ECDSA signature schemes and Maximum Fragment Length responses.
Changes:
- Validates peer ECDSA certificate curves during
CertificateVerify. - Enforces TLS 1.3 MFL validation despite legacy extension handling.
- Adds regression tests for both behaviors.
File summaries
| File | Description |
|---|---|
wolfssl/internal.h |
Exposes the ECC strength comparison helper. |
src/internal.c |
Makes the ECC helper externally accessible. |
src/tls13.c |
Adds ECDSA curve validation. |
src/tls.c |
Enforces TLS 1.3 MFL response checks. |
tests/api/test_tls13.c |
Tests ECDSA scheme/curve mismatches. |
tests/api/test_tls13.h |
Registers the new TLS 1.3 test. |
tests/api/test_tls_parse.c |
Tests TLS 1.3 MFL validation. |
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
Re-ran the F03 checks at f4010ce, built from base 3babd37 plus the three PR commits,
|
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11494
Scan targets checked: wolfssl-src, wolfssl-bugs
Coverage: 3 of 6 in-scope changed file(s) opened by the reviewer; not opened: tests/api/test_tls13.c, tests/api/test_tls_parse.c, wolfssl/internal.h
Fenrir result: Approved ✅
No new issues found in the changed files.
Advisory only — this automated result does not count as a GitHub approval.
Review tier: Lite
f4010ce to
554ef2a
Compare
6099ef8 to
8102a6a
Compare
Frauschi
left a comment
There was a problem hiding this comment.
The hybrid BOTH-mode regression needs fixing before this goes in. secp256k1 still gets through for the same reason the brainpool check was added, so it makes sense to close it here as well, on both the receive and the send side.
7cea4c1 to
9fe918b
Compare
9fe918b to
d6a3006
Compare
0c59a3b
d6a3006 to
0c59a3b
Compare
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11494
Scan targets checked: wolfssl-src, wolfssl-bugs
Not reviewed (no scan target covers them): tests/api/test_tls13.h, tests/test-ecc-cust-curves.conf
Coverage: 9 of 9 in-scope changed file(s) opened by the reviewer
Findings: 2
2 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
Review tier: Max
| } | ||
| #endif | ||
| #if defined(HAVE_ECC) && defined(WOLFSSL_TLS13) | ||
| if ((ssl->options.sigAlgo == ecc_dsa_sa_algo) && |
There was a problem hiding this comment.
ALTERNATIVE CKS signing path not bound to the alt ECC key's curve; new receiver check rejects it · Cryptographic correctness
The sender-side curve binding only checks the native key. In ALTERNATIVE mode, SendTls13CertificateVerify signs with the alt ECC key but uses the hash options.hashAlgo picked for the native key. Example: an RSA native key with a P-256 alt key under WOLFSSL_STRONGEST_HASH_SIG sends ecdsa_secp521r1_sha512. The new DoTls13CertificateVerify check rejects that, so these handshakes now fail.
Suggested fix: In the ALTERNATIVE branch, take the ECDSA hash from the alt key's curve (P-256/384/521 → SHA-256/384/512) and fail if the peer did not offer that scheme.
Basis: RFC 8446 §4.2.3: each ecdsa_secpXXXr1_shaYYY scheme fixes both the curve and the hash; §4.4.3: the CertificateVerify algorithm MUST be one offered in signature_algorithms.
| if ((args->altSigAlgo == ecc_dsa_sa_algo) && | ||
| (ssl->peerEccDsaKeyPresent)) { | ||
| WOLFSSL_MSG("Doing ECC peer cert alt verify"); | ||
| if (!EccPeerCurveMatchesSigAlgo(ssl, |
There was a problem hiding this comment.
New alt-key ECDSA curve rejection in BOTH mode has no test · Missing edge-case coverage on a function the PR also changed
No test reaches this rejection branch. test_dual_alg_both_ecdsa_mldsa only covers hybrids where the native key is ECC. It never uses an ML-DSA- or Falcon-native cert whose alt ECC key is on the wrong curve (e.g. MLDSA_44_P256 with a P-384 alt key). On that failure path peerAuthGood also stays 1 from the native verify.
Suggested fix: Add a BOTH-mode test with an ML-DSA-native cert and a P-384 alt key that asserts SIG_VERIFY_E on the client and checks the resulting peerAuthGood state.
Related known findings (similar but distinct; listed for context, not part of this finding)
- F-7651 (open): File/function: both are in DoTls13CertificateVerify and concern the WOLFSSL_DUAL_ALG_CERTS CKS-BOTH path. Operation: F-7651 is the BOTH branch skipping the advertised-scheme check and accepting an unadvertised composite. The candidate is that the new EccPeerCurveMatchesSigAlgo rejection for the alt ECC key has no test, and it also notes that peerAuthGood stays set. Root cause: F-7651 is a missing validation, while this is missing test coverage of a newly added check. Patch: F-7651 needs a validatio
…PPORTED_EXTENSION defined. Fixes F-11837.
…atches expected sigalg for the OID sum.
Remove secp256k1 support in TLS 1.3, as it is not supported in the spec. Compare the OID sum to confirm the exact same curve is being used.
Unblind key before calling DecodeAltPrivateKey in SendTls13CertificateVerify. Set pkCurveOID in wolfSSL_CTX_use_certificate.
Add tests for BOTH-mode ML-DSA + ECDSA. Fix: In DoTls13CertificateVerify, reset peerAuthGood on error.
0c59a3b to
114bc8f
Compare

Description
Fixes zd#22471, F-11837
Thanks to Eva Crystal (0xiviel), XSource Security for the ECDSA report!
Testing
Built in/added tests, provided reproducer
Checklist