Add curve448 crypto callback support - #11209
Conversation
|
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11209
Scan targets checked: wolfcrypt-bugs, wolfcrypt-port-bugs, wolfcrypt-rs-bugs, wolfcrypt-src, wolfssl-bugs, wolfssl-src
Findings: 13
13 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
1019cfb to
3ae848c
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11209
Scan targets checked: wolfcrypt-bugs, wolfcrypt-port-bugs, wolfcrypt-rs-bugs, wolfcrypt-src, wolfssl-bugs, wolfssl-src
Findings: 9
9 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
3ae848c to
30f74d8
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11209
Scan targets checked: wolfcrypt-bugs, wolfcrypt-port-bugs, wolfcrypt-rs-bugs, wolfcrypt-src, wolfssl-bugs, wolfssl-src
Findings: 7
7 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
Mirror curve25519: keygen, shared secret, make pub and generic callbacks, wc_curve448_init_ex/new/delete, new wc_curve448_generic API with scalar clamp checks, WOLF_CRYPTO_CB_ONLY_CURVE448 mode, TLS devId plumbing, tests (test.c, api unit tests, swdev), benchmark devId, CI entries.
d564771 to
220833e
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11209
Scan targets checked: wolfcrypt-port-bugs, wolfcrypt-rs-bugs, wolfcrypt-src, wolfssl-bugs, wolfssl-src
Failed targets: wolfcrypt-bugs
Findings: 6
6 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
Assert cryptocb output in curve448_onlycb_test, reject an all-zero wc_curve448_generic result, guard the new API test for CB-only builds.
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11209
Scan targets checked: wolfcrypt-bugs, wolfcrypt-port-bugs, wolfcrypt-rs-bugs, wolfcrypt-src, wolfssl-bugs, wolfssl-src
Findings: 8
8 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
| /* the dispatcher must hand over a usable payload */ | ||
| if ((info->pk.curve448kg.rng == NULL) || | ||
| (info->pk.curve448kg.size != CURVE448_KEY_SIZE)) { | ||
| return BAD_FUNC_ARG; |
There was a problem hiding this comment.
🔵 [Low] Curve448 keygen stub leaves key-devId at INVALID_DEVID on the BAD_FUNC_ARG path · Incorrect error handling
The payload-validation early return in the WC_PK_TYPE_CURVE448_KEYGEN branch returns before restoring info->pk.curve448kg.key->devId, which was set to INVALID_DEVID at line 80921. Every other early return in this callback restores devIdArg first, so the caller's key is left unbound from the device.
Fix: Restore info->pk.curve448kg.key->devId = devIdArg; before returning BAD_FUNC_ARG.
| wc_MemZero_Add("wc_curve448_shared_secret_ex o", o, CURVE448_PUB_KEY_SIZE); | ||
| #endif | ||
|
|
||
| if (ret == 0) { |
There was a problem hiding this comment.
⚪ [Info] Always-true ret == 0 guard left behind by the shared-secret restructure · Dead error handling
The argument checks now return early (lines 299 and 303) and the crypto-callback block either returns or resets ret = 0 (line 330), so ret is unconditionally 0 when this guard is evaluated. The check can never be false and no longer protects anything.
Related known finding #7089 (similar but distinct): Both findings concern an always-decided error-handling guard caused by prior control flow, but this is the Curve448 shared-secret operation in wc_curve448_shared_secret_ex, whereas issue 7089 covers separate CMAC operations. The functions and required patches differ.
Fix: Drop the guard and call curve448() directly, keeping the subsequent if (ret == 0) checks that test its result.
| /* the dispatcher must hand over a usable payload */ | ||
| if ((info->pk.curve448kg.rng == NULL) || | ||
| (info->pk.curve448kg.size != CURVE448_KEY_SIZE)) { | ||
| return BAD_FUNC_ARG; |
There was a problem hiding this comment.
🔵 [Low] Curve448 keygen callback leaves key-devId cleared when returning BAD_FUNC_ARG · Incorrect error handling
This early return fires after info->pk.curve448kg.key->devId was overwritten with INVALID_DEVID at line 80921 and skips the /* reset devId */ restore that every sibling branch in this callback performs, leaving the caller's key unbound from the device.
Fix: Restore info->pk.curve448kg.key->devId = devIdArg; before returning BAD_FUNC_ARG.
| ret = wc_curve448_shared_secret(&key, &pubKey, out, &outLen); | ||
| if (ret != 0) | ||
| ret = WC_TEST_RET_ENC_EC(ret); | ||
| else if ((outLen != CURVE448_KEY_SIZE) || (out[0] != 0xA5)) |
There was a problem hiding this comment.
🔵 [Low] curve448_onlycb_test verifies only the first byte of callback-supplied output · Weak or missing assertions
The shared-secret assertion inspects out[0] only, and the keygen assertion at line 80188 inspects key.p[0] only, while curve448_buf_is() — added in the same hunk — full-buffer-checks the make_pub/generic markers. A device that fills only part of the 56-byte output still passes.
Fix: Assert with curve448_buf_is(out, 0xA5, CURVE448_PUB_KEY_SIZE) and curve448_buf_is(key.p, 0xC3, CURVE448_PUB_KEY_SIZE).
| /* try the find callback first, else grab the first registered device */ | ||
| dev = wc_CryptoCb_FindDevice(INVALID_DEVID, WC_ALGO_TYPE_PK); | ||
| if (dev == NULL || dev->cb == NULL) | ||
| dev = wc_CryptoCb_FindDeviceByIndex(0); |
There was a problem hiding this comment.
🟠 [Medium] X448 private scalar dispatched to an arbitrary registered crypto-callback device, ignoring the key's devId · Cryptographic correctness
wc_CryptoCb_FindDeviceByIndex(0) returns the first registered device regardless of algorithm, and wc_curve448_make_pub (curve448.c:90) and wc_curve448_generic call it with no devId gating. Every software X448 keygen and lazy wc_curve448_export_public derivation hands the raw 56-byte private scalar to that device's callback, even for keys created with INVALID_DEVID or bound to a different device. wc_curve448_make_key and wc_curve448_shared_secret_ex do respect key->devId. wc_CryptoCb_Curve448Generic (cryptocb.c:1539) shares the defect.
Fix: Plumb the owning key's devId into the make-pub/generic dispatch and drop the FindDeviceByIndex(0) fallback so unbound keys stay in software.
| ExpectIntEQ(wc_InitRng(&rng), 0); | ||
| #ifndef WC_NO_CONSTRUCTORS | ||
| /* exercise the new constructor path */ | ||
| ExpectNotNull(keyA = wc_curve448_new(HEAP_HINT, devId, &ret)); |
There was a problem hiding this comment.
🔵 [Low] New constructor/destructor NULL-tolerant parameters left untested · Missing edge-case coverage on a function the PR also changed
The PR documents result_code in wc_curve448_new and key_p in wc_curve448_delete as optional, but the only new tests always pass non-NULL for both; wc_curve448_delete(NULL, NULL) returns before reaching the key_p store, so the NULL-tolerant branches are never executed.
Fix: Add a wc_curve448_new(HEAP_HINT, devId, NULL) call and a successful wc_curve448_delete(key, NULL) call to the test.
| curve448_key* key); | ||
| WOLFSSL_LOCAL int wc_CryptoCb_Curve448(curve448_key* private_key, | ||
| curve448_key* public_key, byte* out, word32* outlen, int endian); | ||
| WOLFSSL_LOCAL int wc_CryptoCb_Curve448MakePub(int public_size, byte* pub, |
There was a problem hiding this comment.
🔵 [Low] X448 make-pub/generic dispatch hands the raw private scalar to an arbitrary registered crypto device · Cryptographic correctness
wc_CryptoCb_Curve448MakePub/wc_CryptoCb_Curve448Generic take no devId and fall back to wc_CryptoCb_FindDeviceByIndex(0), so wc_curve448_make_pub() and wc_curve448_generic() now forward the raw X448 private scalar to whatever callback is registered first — including keys explicitly created with INVALID_DEVID. Mirrors the existing curve25519 helpers, but is a new information flow for X448.
Fix: Add devId-carrying variants (or a key-aware wrapper) so the scalar is only offloaded to the device the caller selected.
| ExpectIntEQ(wc_InitRng(&rng), 0); | ||
| #ifndef WC_NO_CONSTRUCTORS | ||
| /* exercise the new constructor path */ | ||
| ExpectNotNull(keyA = wc_curve448_new(HEAP_HINT, devId, &ret)); |
There was a problem hiding this comment.
🔵 [Low] New wc_curve448_new/wc_curve448_delete NULL guards are uncovered by the added tests · Missing edge-case coverage on a function the PR also changed
The only constructor/destructor coverage is the success path plus wc_curve448_delete(NULL, NULL). The result_code != NULL guard in wc_curve448_new and the key_p != NULL guard in wc_curve448_delete are never exercised, so a regression that dereferences either NULL out-param passes CI.
Fix: Add cases calling wc_curve448_new(HEAP_HINT, devId, NULL) and wc_curve448_delete(key, NULL).
2 participants
Mirror curve25519 CB's -
keygen, shared secret, make pub and generic callbacks.
wc_curve448_init_ex/new/delete, new wc_curve448_generic API, WOLF_CRYPTO_CB_ONLY_CURVE448 mode.
TLS devId plumbing, tests (test.c, api unit tests, swdev), benchmark devId, CI entries.