Repository navigation
sakke: keep key state correct across client operations and key changes - #12090
yosuke-wolfssl wants to merge 6 commits into
Conversation
|
@claude review |
|
There was a problem hiding this comment.
🟡 Changes recommended
Replacing the key pair leaves stale receiver-side RSK state and metadata active, allowing subsequent derivations to use the old secret with the new public key instead of rejecting the invalid state.
1 open issue
What changed in this PR
This PR hardens SAKKE key-state handling when a SakkeKey is reused across KMS and client operations, keys are replaced, or cached point-I data changes.
Changes:
- Uses local scalar handling and consistent identity-size validation during point-I and RSK operations.
- Normalizes exported Montgomery values and invalidates derived cached state when keys or point-I values change.
- Adds API regression tests and updates MC/DC white-box and fault-injection coverage.
| File | Description |
|---|---|
| wolfcrypt/src/sakke.c | Updates SAKKE point-I computation, identity validation, Montgomery conversion, and key-state cache handling. |
| tests/unit-mcdc/test_sakke_whitebox.c | Updates white-box coverage for revised point-I computation and validation behavior. |
| tests/unit-mcdc/test_sakke_fault_whitebox.c | Adjusts fault-injection coverage documentation and sweeps for the revised implementation. |
| tests/api/test_sakke.h | Declares and registers the new SAKKE regression test groups. |
| tests/api/test_sakke.c | Adds coverage for oversized identities, KMS/client key reuse, and point-I cache invalidation. |
Review effort: Lite
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings, I checked two more things and found no problem. First, the key->i.* reset in wc_ImportSakkePublicKey() has no guard of its own, but it is fine because the function already sits inside the WOLFCRYPT_SAKKE_CLIENT block that starts at sakke.c:1089. Second, setting i.table to NULL in the new reset paths doesn't leak memory, because the table is always a buffer the caller supplies, never one the library allocates.
Extended reasoning...
The change touches SAKKE key-state handling in wolfcrypt/src/sakke.c, which is security-sensitive crypto code. The two extra checks were made by reading where the client-only struct fields are defined in wolfssl/wolfcrypt/sakke.h and every place sakke.c assigns key->i.table.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
wolfcrypt/src/sakke.c— pre-existing: A sender whose point-I recompute fails can later encapsulate with a corrupted point I for the previously cached identity. sakke_compute_point_r() at sakke.c:6731 recomputes into key->i.i without first clearing key->i.idSz. On error, i.idSz and i.id still name the old identity, so the next call for it is a cache hit on a half-written point. Fix: reset key->i.idSz to 0 before calling sakke_compute_point_i(), matching what this PR now does in wc_MakeSakkePointI(), wc_SetSakkePointI() and wc_ValidateSakkeRsk().Why this was flagged
The key caches point I for identity A (i.idSz/i.id = A). wc_MakeSakkeEncapsulatedSSV() at sakke.c:6848, or the derive path at sakke.c:7045, then runs for identity B, so sakke_compute_point_r() enters the recompute branch at sakke.c:6731-6741. sakke_compute_point_i() then fails after writing part of i. In non-SP builds, sakke_mulmod_base_add() at sakke.c:417-429 writes res via wc_ecc_mulmod() before ecc_projective_add_point()/ecc_map() can return MEMORY_E. Only i.table/i.tableLen are cleared; i.idSz/i.id keep A. A later call for identity A matches the cache check and calls sakke_mulmod_point() on the corrupted point. The result is a wrong R/auth that the receiver cannot derive. The base behaves the same way, so this is pre-existing. The PR fixes this pattern at the three public entry points but leaves this internal recompute site unchanged.
Verification: pre-existing. sakke_compute_point_r() at wolfcrypt/src/sakke.c:6731-6741 is the same in the base. On failure it clears just
key->i.table/key->i.tableLen(6739-6740), soi.idSz/i.idstill name identity A. The next call for identity A passes the check at 6731-6732 as a cache hit and runs on the half-written I (6744). That gives a wrong R, with no error reported.
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #12090
Scan targets checked: wolfcrypt-src, wolfcrypt-bugs, wolfssl-bugs
Not reviewed (no scan target covers them): tests/api/test_sakke.h
Coverage: 4 of 4 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: Max
- sakke_compute_point_i() reads the identity into a function-local mp_int in place of the key's private scalar. - wc_ValidateSakkeRsk() returns BAD_FUNC_ARG when idSz is greater than SAKKE_ID_MAX_SIZE. - test: test_wc_Sakke_KmsKeyAfterClientOp is added. test_sakke_whitebox.c is updated. Issue: F-16912
- wc_ExportSakkeKey() converts the public key out of Montgomery form with sakke_z_from_mont() before encoding it. - test: test_wc_Sakke_KmsKeyAfterClientOp is updated. Issue: F-18182
- wc_MakeSakkeKey() and wc_ImportSakkeKey() clear key->zMont when they store the new public key. - test: test_wc_Sakke_KmsKeyAfterClientOp is updated. Issue: F-18183
- wc_MakeSakkePointI(), wc_SetSakkePointI() and wc_ValidateSakkeRsk() clear the cached identity, point-I table and table length before they overwrite point I. - test: test_wc_Sakke_PointICache. Issue: F-16913
- wc_ImportSakkePublicKey(), wc_ImportSakkeKey() and wc_MakeSakkeKey() clear the cached identity, point-I table and table length when they store a new public key. - test: test_wc_Sakke_PointICache is updated. Issue: F-18184
- wc_ValidateSakkeRsk() records the cached identity after sakke_pairing() succeeds. - sakke_compute_point_r() clears the cached identity, point-I table and table length before it recomputes point I.
5830657 to
1b35e0d
Compare
Problem
A
SakkeKeycaches values derived from its keys. Five paths leave that state overwritten or stale when one object changes role or loads a new KMS key.The API accepts any initialized
SakkeKey, and wolfCrypt's ownsakke_testloads a new Z into a used object.Fix (
wolfcrypt/src/sakke.c)sakke_compute_point_i()reads the identity into a function-localmp_int.wc_ValidateSakkeRsk()returnsBAD_FUNC_ARGforidSz > SAKKE_ID_MAX_SIZE, aswc_SetSakkeIdentity()andwc_MakeSakkePointI()already do. Previously such an identity gaveECC_OUT_OF_RANGE_Eor a falsevalid = 0, and could never be used to derive.wc_ExportSakkeKey()converts Z out of Montgomery form first, aswc_ExportSakkePublicKey()does.wc_ImportSakkePublicKey(),wc_ImportSakkeKey(),wc_MakeSakkeKey()) clearszMontand the cached point I.wc_MakeSakkePointI(),wc_SetSakkePointI(),wc_ValidateSakkeRsk()) clears the cached identity and table first.Closes F-16912, F-16913, F-18182, F-18183, F-18184.
Tests
test_wc_Sakke_KmsKeyAfterClientOpandtest_wc_Sakke_PointICacheare added.test_wc_Sakke_DecisionCoveragechecks oversized identity.tests/unit-mcdcis updated with the library fixes.Verification
--disable-sp,--enable-sp,--enable-sp=small, fastmath, heapmath, smallstack,WOLFSSL_ECC_BLIND_K. The sakke API group (4/4) andtestwolfcryptpass on all; ASan + UBSan clean on three of them.-Werror: default,--enable-sp, fastmath, 0 warnings. ASan + UBSan + LSan clean on heapmath, smallstack and default.done (ok).