Repository navigation
Conversation
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues are identified for the ECCSI key-reuse and verification changes.
No open issues 🎉
What changed in this PR
This PR fixes ECCSI key reuse and verification in SP-ECC builds by keeping KPAK in normal form and correcting state handling.
Changes:
- Clears the identity hash on key regeneration and validates PVTs as public keys.
- Uses SP acceleration for P-256 and a generic fallback for other curves, removing the
kpakMontfield fromEccsiKey. - Adds key-reuse and curve regression tests, plus two SP-ECC CI configurations.
| File | Description |
|---|---|
wolfssl/wolfcrypt/eccsi.h |
Removes kpakMont, potentially changing the public struct size. |
wolfcrypt/test/test.c |
Updates comments to describe checks without Montgomery-state assumptions. |
wolfcrypt/src/eccsi.c |
Fixes key state, PVT validation, and generic verification fallback. |
tests/api/test_eccsi.h |
Registers the new ECCSI tests. |
tests/api/test_eccsi.c |
Adds key-reuse and multi-curve regression coverage. |
.github/configs/os-check-linux.json |
Adds SP-ECC configurations covering both fallback paths. |
Review effort: Balanced
|
8ccb7c1 to
7b0b048
Compare
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. It changes ECCSI verification arithmetic in wolfCrypt and removes a field from the public EccsiKey struct, so a maintainer familiar with the ECC/SP code should still look at it.
What was reviewed:
eccsi_mulmod_point_add: the SP P-256 path and the fallthrough to the generic path. The temporary Montgomery copy of the KPAK (normalization inmontA->zis used for x and y first, then for z) is freed on every path, including errors.- Removal of
kpakMont: the KPAK now stays in normal form for export, HS computation andeccsi_calc_y. TheWOLFSSL_SP_MATHbranch returnsNOT_COMPILED_IN. wc_MakeEccsiKeyclearsidHashSz.wc_ValidateEccsiPvtnow sets theECC_PUBLICKEYtype beforewc_ecc_check_key, and its argument check uses||instead of|.
Extended reasoning...
The change rewrites how ECCSI verification handles the KPAK's Montgomery form in wolfcrypt/src/eccsi.c and removes the kpakMont bitfield from the public EccsiKey struct, which changes the ABI. It also adds new API tests and two CI build configurations. This is crypto code, and its correctness depends on build options (WOLFSSL_HAVE_SP_ECC, WOLFSSL_SP_NO_256, WOLFSSL_SP_MATH, custom curves) that a single review run cannot fully exercise. A Copilot review is also on the timeline, and its content isn't visible here. Because this is a security-sensitive crypto path with configuration-dependent behavior, human review is warranted rather than automatic approval.
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #12018
Scan targets checked: wolfcrypt-src, wolfcrypt-bugs, wolfssl-src, wolfssl-bugs
Not reviewed (no scan target covers them): .github/configs/os-check-linux.json, tests/api/test_eccsi.h
Coverage: 4 of 4 in-scope changed file(s) opened by the reviewer
Findings: 1
1 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
7b0b048 to
3632138
Compare
Verifying a signature without SP-ECC converts the KPAK held in the key to Montgomery form and sets kpakMont. wc_MakeEccsiKey() replaced the KPAK with a fresh normal-form point but left the flag set, so the next export, KPAK hash or pair issuance reduced the new point a second time. Pairs made after that were bound to the wrong KPAK and their signatures failed to verify. Convert the KPAK back to normal form before generating the key, so the flag also stays correct when key generation fails before it writes the new point, and add a test that makes, signs and verifies twice with one key object. Fixes F-16944.
wc_ValidateEccsiPvt() copies the PVT into the scratch key->pubkey and runs wc_ecc_check_key() on it, but left the scratch key's type as it was. After wc_MakeEccsiPair() or wc_SignEccsiHash() that type is ECC_PRIVATEKEY with a zeroed scalar, so the private key range check failed and a valid PVT was reported invalid. After wc_VerifyEccsiHash() the scalar holds r, which SP-ECC builds then rejected in the [k]G check. Mark the scratch key as public before the check, since RFC 6507 PVT validation involves no private key, and extend the key reuse test to validate the PVT after each operation. Also use a logical OR in the function's argument check. Fixes F-16945.
wc_MakeEccsiKey() replaces the KPAK but kept the identity hash set with wc_SetEccsiHash(). HS is computed over the KPAK, so signing after a new key succeeded and produced signatures that no holder of the new KPAK could verify. Clear the hash so that signing and verifying return BAD_STATE_E until the caller sets one computed with the new key.
Verification needs the KPAK in Montgomery form for the generic point add, so it converted key->ecc.pubkey in place and tracked that with kpakMont. Every function that read or replaced the KPAK had to undo or reset the conversion, and missing one corrupted the key. The SP and generic selection was also wrong: SP-ECC builds without SP P-256 took the generic add without converting the KPAK, so every valid signature failed to verify, and builds with SP P-256 had no fallback for other curves, so verification returned NOT_COMPILED_IN for keys on curves such as brainpoolP256r1. Select the SP code in eccsi_mulmod_point_add() only for P-256, as the other point helpers do, and in the generic code convert a temporary copy of the KPAK. The stored KPAK now stays in normal form, so the conversion helpers, the kpakMont field and the resets in the import and key generation functions are removed. Add a test that signs and verifies on each 256-bit curve compiled in. Fixes F-14285.
ECCSI picks between the SP P-256 code and the generic point operations per curve, and the generic path inside an SP-ECC build was not built by any CI configuration. Add one configuration with SP-ECC but without SP P-256, so P-256 and brainpool keys take the generic point add, and one with SP P-256 and the custom curves, so brainpoolP256r1 and secp256k1 keys use the generic fallback next to the SP code. Both are full builds, since --enable-cryptonly does not build tests/unit.test and so would skip the ECCSI API tests.
3632138 to
a9a22f8
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #12018
Scan targets checked: wolfssl-src, wolfssl-bugs
Unchanged since last review (not re-run): wolfcrypt-src, wolfcrypt-bugs
Not reviewed (no scan target covers them): .github/configs/os-check-linux.json, tests/api/test_eccsi.h
Coverage: 1 of 1 in-scope changed file(s) opened by the reviewer
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
Fenrir's latest completed scan found no issues; clearing the prior automated change request.
Description
Fixes stale state on a reused
EccsiKeyand ECCSI verification in SP-ECC builds, which failed whenever the generic point add got a KPAK in the wrong form.Fixes F-16944, F-16945 and F-14285.
wc_MakeEccsiKey()no longer leaves a stalekpakMontflag after a verify, which made the next export, HS and pair use a wrong KPAKwc_ValidateEccsiPvt()checks the PVT as a public key, so a valid PVT no longer fails after making a pair or signingwc_MakeEccsiKey()clears the identity hash, so signing with a hash for the old KPAK returnsBAD_STATE_Eeccsi_mulmod_point_add()uses SP only for P-256 and the generic code otherwise, converting a copy of the KPAK;kpakMontand the in-place conversion are removedBreaking changes
EccsiKeyloses itskpakMontbitfield, sosizeof(EccsiKey)can changeLimitations
wc_SetEccsiHash()again, as beforeTesting
test_wc_Eccsi_KeyReuse: two key generations with sign, verify and PVT validation on one key objecttest_wc_Eccsi_Curves: pair and PVT validation, sign and verify on P-256, brainpoolP256r1 and secp256k1eccsi-sp-no-p256andeccsi-sp-custcurves