Complete the crypto callback offload path - #26
Conversation
d3bc5fa to
776ec82
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #26
Scan targets checked: wolfpsa-bugs, wolfpsa-src
Findings: 2
Required changes (2)
Ed25519 key generation stores an all-zero private key when the offload device retains the scalar
File: src/psa_ed25519_ed448.c:308
Function: psa_asymmetric_generate_key_ed25519
Category: Key material exposure
The PR wires the devId into wc_ed25519_init_ex, so wc_ed25519_make_key now dispatches via wc_CryptoCb_Ed25519Gen. wc_ed25519_export_private_only performs no privKeySet check and blindly copies the zeroed key->k, so a device that returns success while keeping the scalar yields PSA_SUCCESS with an all-zero private key. The sibling ECC path added an ecc.type != ECC_PRIVATEKEY guard for exactly this case; this path has none.
Recommendation: Reject the result with PSA_ERROR_HARDWARE_FAILURE when ed_key.privKeySet is not set after wc_ed25519_make_key.
Referenced code: src/psa_ed25519_ed448.c:308-310 (3 lines)
X25519 key generation stores an all-zero private key when the offload device retains the scalar
File: src/psa_montgomery.c:91
Function: psa_asymmetric_generate_key_x25519
Category: Key material exposure
The PR wires the devId into wc_curve25519_init_ex, so wc_curve25519_make_key now dispatches via wc_CryptoCb_Curve25519Gen. wc_curve25519_export_private_raw_ex performs no privSet check and copies the zeroed key->k, so a device that returns success while keeping the scalar yields PSA_SUCCESS with an all-zero private key. Adjacent to the ecc.type != ECC_PRIVATEKEY guard added for ECC in this PR, which has no counterpart here.
Related known finding #11587 (similar but distinct): Both involve X25519 in psa_montgomery.c, but the candidate faults key generation/export when offload retains the private scalar, whereas #11587 faults RNG-backed blinding during key agreement. Their root causes and required patches differ.
Recommendation: Reject the result with PSA_ERROR_HARDWARE_FAILURE when key.privSet is not set after wc_curve25519_make_key.
Referenced code: src/psa_montgomery.c:91-95 (5 lines)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
776ec82 to
2cf6e7f
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #26
Scan targets checked: wolfpsa-bugs, wolfpsa-src
Findings: 3
1 finding(s) posted as inline comments (see file-level comments below)
Required changes (2)
Ed448 key generation lacks the device-held-scalar guard its siblings received
File: src/psa_ed25519_ed448.c:644
Function: psa_asymmetric_generate_key_ed448
Category: Copy-paste errors
This PR routes Ed448 keygen to the offload device via wc_ed448_init_ex() but omits the privKeySet check added to the parallel psa_asymmetric_generate_key_ed25519() (line 313) and to the ECC, X25519 and X448 keygen paths. A backend that keeps the scalar leaves ed_key.k zero and the export at line 653 is reached with no device-fault reporting.
Recommendation: After a successful wc_ed448_make_key(), return PSA_ERROR_HARDWARE_FAILURE when !ed_key.privKeySet, matching the Ed25519 path.
Referenced code: src/psa_ed25519_ed448.c:644-648 (5 lines)
Ed448 key generation exports the private scalar without the device-populated guard applied to every sibling keygen path
File: src/psa_ed25519_ed448.c:653
Function: psa_asymmetric_generate_key_ed448
Category: Key material exposure
The PR threads wolfPSA_GetDefaultDevID() into wc_ed448_init_ex() here but omits the privKeySet check it added to the Ed25519, X25519 and X448 keygen paths. wc_ed448_export_private_only() copies ed_key.k unconditionally, so a callback device that accepts the Ed448 keygen and keeps the scalar yields an all-zero 57-byte private key stored with PSA_SUCCESS.
Recommendation: After wc_ed448_make_key() succeeds, return PSA_ERROR_HARDWARE_FAILURE when !ed_key.privKeySet, mirroring the Ed25519 path.
Referenced code: src/psa_ed25519_ed448.c:653-656 (4 lines)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
2cf6e7f to
9dd51c8
Compare
|
@wolfSSL-Fenrir-bot One of the three is real and is fixed in the force-push; the two Ed448 findings are false positives. Test coverage: fixed. The mute switch only fired for Ed448: no action. Both findings assume |
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #26
Scan targets checked: wolfpsa-bugs, wolfpsa-src
Findings: 1
Required changes (1)
Ed448 key generation offloads to a crypto callback without the device-held-key guard applied to every sibling path
File: src/psa_ed25519_ed448.c:653
Function: psa_asymmetric_generate_key_ed448
Category: Key material exposure
The PR threads the default devId into wc_ed448_init_ex (line 631) and wc_InitRng_ex (line 637), making wc_ed448_make_key dispatchable to a crypto callback, but omits the privKeySet check added for Ed25519 (line 313), ECC (psa_ecc.c:388), X25519 and X448. A device that returns success while retaining the scalar yields an all-zero Ed448 private and public key returned as PSA_SUCCESS, since wc_ed448_export_private_only / wc_ed448_export_public copy k and p unconditionally.
Recommendation: After wc_ed448_make_key succeeds, reject with PSA_ERROR_HARDWARE_FAILURE when !ed_key.privKeySet, mirroring the Ed25519 guard.
Referenced code: src/psa_ed25519_ed448.c:653-657 (5 lines)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
9dd51c8 to
5e13668
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #26
Scan targets checked: wolfpsa-bugs, wolfpsa-src
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.
5e13668 to
b714b33
Compare
wolfPSA_SetDefaultDevID() only reached the algorithms whose call sites already used a devId-aware initializer. Everything else went through the plain wc_*Init() form and ignored the configured value, so RSA, ECC, Ed25519, Ed448, X25519, the RNG, CMAC, HKDF, PBKDF2 and the SHA-1/SHA-2 families always ran locally. Move those call sites to the _ex forms. The plain forms are not uniform, which the getter now accounts for. Most pin INVALID_DEVID, but the SHA-2 family calls wc_CryptoCb_DefaultDevID(), and wc_ecc_init and wc_InitCmac select the CAAM device on QNX and iMXRT builds. Passing wolfPSA's own default unconditionally would have taken offload away from those builds, so an unconfigured wolfPSA defers to the same wolfCrypt selection. A configured devId wins over it, INVALID_DEVID included: passing that explicitly is how an application keeps every operation on the host. Several algorithms stay local. PSA_ALG_DETERMINISTIC_ECDSA promises RFC 6979 and the callback contract carries no deterministic flag, so an offloaded sign would quietly return a randomized signature. The RSA key whose only job is to decode a DER blob keeps INVALID_DEVID, because wc_RsaEncryptSize() answers a hard-coded 2048 bits for a device that declines the call, which would let a zero-modulus key past the size check. AES key wrap keeps the keyless wc_AesKeyWrap() wrappers: wc_AesKeyWrap_ex() falls back to software when the device has no keywrap handler, and a device that installed the KEK through WOLF_CRYPTO_CB_AES_SETKEY leaves no software key schedule for that fallback to run on. X448, RIPEMD-160, MD5, Ascon and ChaCha20-Poly1305 stay local because wolfCrypt either exposes no devId for them or ignores the one it accepts. Three paths that only a device can reach are now handled. The HMAC path of psa_mac_* never called wc_HmacInit() at all, so it ran with devId 0 from its zeroed allocation, and a callback registered on device 0 captured wolfPSA's HMACs while every other algorithm stayed local. ECC key generation read the private scalar straight out of the key struct, so a backend that keeps the scalar produced an all-zero private key with PSA_SUCCESS; it checks the key type first, the way wolfCrypt's own exporters do, and reports HARDWARE_FAILURE. The Ed25519 and X25519 generate paths call those exporters instead of bypassing them, so no zero key was ever emitted there, but the refusal arrived as BAD_FUNC_ARG and ECC_BAD_ARG_E, which both map to PSA_ERROR_INVALID_ARGUMENT and blame the caller for a fault that was the device's; they check the private-scalar flag directly so all three report the same status. The CMAC init-failure paths free the context, because the callback branch can return an error after attaching per-context state. On the build side, cryptocb.c was missing from the wolfCrypt source list, so no WOLF_CRYPTO_CB build could link, and the Makefile had no header dependency tracking, which let a header edit leave every object stale. psa_devid_cryptocb_test covers four states: nothing registered leaves the default at INVALID_DEVID, a registered device with no explicit setting must catch every algorithm family through wolfCrypt's own selection, an explicit devId must override that selection while the other device stays registered, and an explicit INVALID_DEVID must stop dispatching entirely. Its probe device sits on a non-zero devId so an algorithm skipping its wc_*Init() goes missing from the counts rather than blending in. It links its own WOLF_CRYPTO_CB build of the library from a directory scoped to the flag set, so an ASAN lane never reuses uninstrumented objects, and it stays out of the default target because that second build is not free. A muted-device case per curve covers the private-scalar guards: the callback reports a successful keygen without filling in the key, and removing any one guard turns the suite red rather than leaving it green.
The devId is read by roughly sixty wolfCrypt initializer call sites, so an application that changes it while other threads issue PSA calls had a formal data race on a plain int. Hold it in a wolfSSL_Atomic_Int instead. The existing WOLFPSA_LOCK mutex would have been the wrong instrument: wolfpsa_kdf_pbkdf2 re-reads the value once per block per iteration, so a PBKDF2 at cost 10000 would have paid ten thousand mutex round trips for a value that changes at most once in a program's life. Whether a devId has been configured folds into the same variable, because two separate ints cannot be updated atomically together and a reader could otherwise see the flag set against a stale value. WOLFPSA_DEVID_DEFAULT marks "wolfPSA expresses no preference", which keeps INVALID_DEVID available as the explicit request for local execution. It is public rather than a private sentinel because every setting is now reversible: an explicit devId and the INVALID_DEVID opt-out both give way to it. The header records what the atomic does not buy. It is not a barrier, so an operation already under way keeps the devId its wolfCrypt context was initialised with. It covers this setting only, not wolfCrypt's unsynchronized device table, so registration still belongs in single-threaded init or teardown. And on a SINGLE_THREADED or WOLFSSL_NO_ATOMICS build the accessors degrade to plain loads and stores, which the header says rather than promising defined behaviour everywhere. The setter also stops accepting what it cannot deliver. A real devId asks for an offload that does not exist in a library built without WOLF_CRYPTO_CB, so that now returns NOT_COMPILED_IN and leaves the default alone. It still cannot tell whether a devId names a registered device: wolfCrypt_Init() clears the callback table, so registration has to follow psa_crypto_init(), and refusing an as-yet-unregistered devId would forbid configuring wolfPSA before that point. That registration is reachable from the shared library through wolfPSA_RegisterCryptoCb() and wolfPSA_UnRegisterCryptoCb(). The wolfCrypt objects a devId selects live inside libwolfpsa, and exporting the raw wc_CryptoCb_* names instead would have bound them by link order, so an application that also links libwolfssl could have registered into the wrong device table. X448 joins the list of algorithms that carry the devId. It was excluded only because wolfCrypt had no curve448 callbacks; it has them now, so psa_montgomery.c threads the devId through wc_curve448_init_ex(). Deriving an X448 public key moves off wc_curve448_make_pub(), which takes raw buffers rather than a key and is therefore offered to whichever device happens to be registered, ignoring the configured devId even when the caller had forced local execution. X25519 has the same shape and no such escape, so the header documents it rather than pretending otherwise: wc_CryptoCb_Curve25519MakePub() takes no devId at all and falls back to the device at index 0, and wc_curve25519_export_public_ex() derives a missing public point through that same call. Key generation and key agreement are unaffected. Separately, X448 key generation gains the private-scalar check the other generate paths carry, because wc_curve448_make_key() does dispatch through wc_CryptoCb_Curve448Gen(). A device that reports success while keeping the scalar is refused as HARDWARE_FAILURE rather than as an argument error. Ed448 needs no such check: wc_ed448_make_key() has no callback dispatch at all, only sign and verify, so the condition is unreachable there. The muted-device case in psa_devid_cryptocb_test gains an X448 entry beside the three it already carries. Passing a devId to the HMAC path exposed a second defect: wc_HmacSetKey() stores the caller's buffer in Hmac.keyRaw rather than copying it, and psa_mac_setup freed that buffer while the operation lived on, so a device read freed memory on the next update. The operation owns its key material now, under WOLF_CRYPTO_CB only, since that is the build where keyRaw exists; psa_mac_abort() shares the same teardown instead of duplicating it. CI runs psa_devid_cryptocb_test a second time under AddressSanitizer, since reading freed memory is silent otherwise. The new case in psa_14_misc_test covers the setter, asserting only what holds in every build: the setter's verdict and the getter's report agree. It cannot key off its own WOLF_CRYPTO_CB, because that TU is compiled without user_settings.h and so says nothing about how the linked library was built.
psa_cipher_decrypt() copies the IV prefix out of the input with XMEMCPY() before handing the remainder to psa_cipher_update(), so it never reaches that function's NULL-input guard and dereferenced a NULL input directly. psa_cipher_encrypt() only ever touches the input through psa_cipher_update() and was already safe. Reject a NULL input with a non-zero length in psa_cipher_decrypt() and cover both entry points in psa_cipher_oneshot_len_test.
b714b33 to
3fe3876
Compare
4 participants
wolfPSA_SetDefaultDevID()existed but only reached the algorithms whose call sites already used a devId-aware wolfCrypt initializer. Everything else went through the plainwc_*Init()form and silently ignored it, so an application that registered an offload backend got partial coverage with no way to tell which algorithms were actually dispatched.The first commit threads the devId through the remaining call sites; the second makes the setting atomic and reversible, covers X448, and adds the registration entry points a shared-library user needs. The third is unrelated to the devId work and rides along as a follow-up to #25, since this branch is rebased onto that merge.
What changed
RSA, ECC, Ed25519, Ed448, X25519, X448, the RNG, CMAC, HKDF, PBKDF2 and the SHA-1/SHA-2 families now carry the configured devId.
An unset devId defers to wolfCrypt's own selection rather than pinning
INVALID_DEVID. The plain init forms are not uniform (the SHA-2 family callswc_CryptoCb_DefaultDevID(), andwc_ecc_init/wc_InitCmacselect the CAAM device on QNX and iMXRT), so pinning would have taken offload away from those builds. An explicit devId wins over that selection,INVALID_DEVIDincluded, which is how an application forces every operation onto the host.The setting is now a
wolfSSL_Atomic_Int: it is read by roughly sixty call sites, so changing it under concurrent PSA traffic was a data race on a plainint.New API.
WOLFPSA_DEVID_DEFAULTrestores wolfCrypt's choice, so no setting is a one-way door.wolfPSA_RegisterCryptoCb()/wolfPSA_UnRegisterCryptoCb()register against the device table wolfPSA dispatches through; the rawwc_CryptoCb_*names are deliberately not exported, becauselibwolfpsalinks its own copy of wolfCrypt, so an application that also linkslibwolfsslhas two tables and a bare call binds by link order, registering into the wrong one with no error.Bugs fixed
wc_HmacInit()wc_HmacSetKey()borrows the key buffer rather than copying itpsa_mac_setupfreed it while the operation lived on, so a device read freed memoryPSA_SUCCESScryptocb.cmissing from the wolfCrypt source listWOLF_CRYPTO_CBbuild could linkFollow-up to #25
psa_cipher_decrypt()has noinput == NULLguard. #25 added theoutput == NULL && output_size > 0check but not the input equivalent, and the omission is live on master.psa_cipher_encrypt()is unaffected because it only ever reaches the input throughpsa_cipher_update(), which guards it; decrypt copies the IV prefix out of the input withXMEMCPY()before it delegates, sopsa_cipher_decrypt(key, PSA_ALG_CBC_NO_PADDING, NULL, 32, ...)dereferenced NULL. The guard belongs inpsa_cipher_decrypt()itself, andpsa_cipher_oneshot_len_testnow covers both entry points: it exits 139 against the unpatched source and passes against the fix.Not offloaded
Deterministic ECDSA stays local because the callback contract carries no RFC 6979 flag, so an offloaded sign would quietly return a randomized signature. The RSA key that only decodes a DER blob keeps
INVALID_DEVID, becausewc_RsaEncryptSize()answers a hard-coded 2048 bits for a declining device, which would let a zero-modulus key past the size check. X448 public-key derivation, RIPEMD-160, MD5, Ascon and ChaCha20-Poly1305 have no usable devId.AES key wrap is deferred. Threading a devId there is unsafe against current wolfCrypt: a device that accepts
wc_AesSetKey()underWOLF_CRYPTO_CB_AES_SETKEYbut implements no keywrap handler leaveskeyInstalled = 1with no software key schedule,wc_AesKeyWrap_ex()falls back to software, and RFC 3394 then runs over all-zero round keys and returnsPSA_SUCCESS. Confirmed with an A/B control: accepting the dispatch yields KEK-independent ciphertext, byte-identical across two different KEKs. The stock build never definesWOLF_CRYPTO_CB, so shipped configurations were unaffected. The fix belongs in wolfCrypt and is in progress separately; AES-KW joins the coverage list once it lands.Testing
New
psa_devid_cryptocb_testcovers all five states of the setting: nothing registered, a registered device with no explicit setting, an explicit devId overriding that selection, an explicitINVALID_DEVIDopt-out, andWOLFPSA_DEVID_DEFAULTrestoring wolfCrypt's choice. Its probe device sits on a non-zero devId, so an algorithm that skips itswc_*Init()carries devId 0 and goes missing from the counts instead of blending in, which is what catches the HMAC bug; its callback reads the HMAC key the way a real backend would, which surfaces the borrowed buffer, so CI runs the test again under AddressSanitizer. It also asserts that deterministic ECDSA stays local and that a device keeping the EC private scalar is refused withPSA_ERROR_HARDWARE_FAILURE; both fail if the corresponding guard is removed.The test links its own
WOLF_CRYPTO_CBbuild, since the dispatch is otherwise compiled out and it inspects wolfCrypt structures directly. That build directory is scoped to the flag set so an ASAN lane never reuses uninstrumented objects, and it stays out of the default target:make -C test psa_devid_cryptocb_test.Review notes
wolfpsa.mapgains two entries; a new public symbol needs implementation, header and version script or it will not be reachable fromlibwolfpsa.so. A backend keeping per-operation state indevCtxmust be built withWOLF_CRYPTO_CB_COPYandWOLF_CRYPTO_CB_FREE. That and the remaining caveats (X25519 public-key derivation,WOLF_CRYPTO_CB_FIND, the unsynchronizedgCryptoDev[]table) are documented inwolfpsa/psa_engine.h.