Ecies dev id change - #11399
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new API test has a logic error in its “control” leg (missing decrypt while comparing plaintext), and the ECIES devId change appears to silently disable device routing in PLUTON_CRYPTO_ECC builds without WOLF_CRYPTO_CB.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR changes ECIES device routing so Crypto Callback dispatch (and the ECIES DEM primitives) use an explicit device ID carried by the ECIES context, rather than inheriting devId from the ECC keys. This makes ECIES offload opt-in per-context and prevents key-bound devices from implicitly affecting ECIES routing.
Changes:
- Add
wc_ecc_ctx_set_dev_id()/wc_ecc_ctx_get_dev_id()(WOLF_CRYPTO_CB-only) and persist the setting acrosswc_ecc_ctx_reset(). - Update ECIES CryptoCb dispatch (
wc_CryptoCb_EciesEncrypt/Decrypt) to accept an explicitdevIdparameter and route based on the context’s device selection. - Update tests, benchmark, and documentation to set context device IDs explicitly and to validate the new routing behavior.
File summaries
| File | Description |
|---|---|
| wolfssl/wolfcrypt/ecc.h | Adds public API for ECIES context device-id set/get (WOLF_CRYPTO_CB-only). |
| wolfssl/wolfcrypt/cryptocb.h | Updates ECIES CryptoCb prototypes to take an explicit devId and documents semantics. |
| wolfcrypt/src/ecc.c | Stores context devId, preserves it across reset, and routes ECIES dispatch + DEM primitives based on context device-id. |
| wolfcrypt/src/cryptocb.c | Routes ECIES CryptoCb device lookup using the passed-in devId instead of privKey->devId. |
| wolfcrypt/test/test.c | Updates ECIES tests to explicitly apply context device IDs and adjusts CryptoCb re-entrancy handling accordingly. |
| wolfcrypt/benchmark/benchmark.c | Ensures ECIES “-dev” benchmark rows set the context devId so they actually measure device routing. |
| tests/unit-mcdc/test_cryptocb_whitebox.c | Updates whitebox tests for new ECIES CryptoCb signatures and dispatch semantics. |
| tests/api/test_ecc.h | Registers a new API test for ECIES devId non-inheritance behavior. |
| tests/api/test_ecc.c | Adds/updates API tests for the new context devId accessors and for “devId not inherited” ECIES behavior. |
| wolfcrypt/src/port/xilinx/versal_gen2_asu/README.md | Documents the new requirement to set ECIES context device ID for ASU offload. |
| IDE/XilinxSDK/vitis_sdt/lib/sw_services/wolfssl/src/user_settings_template.h | Enables WC_BENCH_ECIES_KDF to ensure benchmark coverage of the KDF-based ECIES path relevant to the port. |
| doc/dox_comments/header_files/ecc.h | Adds Doxygen docs for the new context devId APIs and notes ECIES routing behavior. |
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| /* devId for the ECIES crypto callback dispatch and for the DEM AES/HMAC | ||
| * primitives. It comes from the context only - it is never inherited from | ||
| * privKey->devId - so a caller that did not call wc_ecc_ctx_set_dev_id(), | ||
| * or that passed no context at all, gets software. */ | ||
| int eciesDevId = INVALID_DEVID; |
| /* devId for the ECIES crypto callback dispatch and for the DEM AES/HMAC | ||
| * primitives. It comes from the context only - it is never inherited from | ||
| * privKey->devId - so a caller that did not call wc_ecc_ctx_set_dev_id(), | ||
| * or that passed no context at all, gets software. */ | ||
| int eciesDevId = INVALID_DEVID; |
7ad79c3 to
263f0d9
Compare
|
Add wc_ecc_ctx_set_dev_id() and _get_dev_id(); the key devId is unused.
Also add a test that checks the key's devId does not pick the device.
Update the README and turn on WC_BENCH_ECIES_KDF in the Vitis template.
Use wc_HKDF_ex() with the same heap and devId as the AES and HMAC steps.
The last step compared old plaintext without decrypting first.
263f0d9 to
f4395f0
Compare
There was a problem hiding this comment.
🟢 Approval recommended
The changes consistently implement the new explicit ECIES context device-id model across core code, dispatch helpers, tests/benchmarks, and documentation with added targeted test coverage to prevent silent regressions.
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 0 new
- Review effort level: Lite
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11399
Scan targets checked: wolfcrypt-bugs, wolfcrypt-port-bugs, wolfcrypt-rs-bugs, wolfcrypt-src, wolfssl-bugs, wolfssl-src
Findings: 4
4 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
| if (ctx != NULL) | ||
| eciesDevId = ctx->devId; | ||
|
|
||
| #ifndef WOLF_CRYPTO_CB_FIND |
There was a problem hiding this comment.
CryptoCb finder bypasses explicit ECIES device selection · Preprocessor-conditional security bypass
WOLF_CRYPTO_CB_FIND removes the INVALID_DEVID guard, so an unset or NULL ECIES context still reaches a finder-selected callback. This can expose ECIES plaintext and key material to an unintended device.
Related known finding #3564 (similar but distinct): Both affect ECIES encrypt/decrypt when the caller supplies a NULL or unset context, but #3564 faults in default KDF/IV initialization and deterministic ciphertext, whereas this candidate faults in CryptoCb finder dispatch despite no explicit device ID. The operations, root causes, and required patches differ.
Suggested fix: Suppress finder dispatch for ECIES and its HKDF, AES, and HMAC steps unless the context carries an explicit device ID.
| NULL), 0); | ||
| #endif | ||
| ExpectIntEQ(cbInvoked, 0); | ||
| ExpectIntEQ(XMEMCMP(plain, msg, sizeof(msg)), 0); |
There was a problem hiding this comment.
NULL-context decrypt reuses earlier plaintext · Weak or missing assertions
plain still contains stage (1)'s decrypted message, and stage (2) neither clears it nor verifies plainSz; a NULL-context decrypt that returns success without producing output still passes the plaintext check.
Suggested fix: Clear plain before stage (2) and assert plainSz == sizeof(msg) after its decrypt.
| ret = wc_HKDF(WC_SHA256, sharedSecret, sharedSz, ctx->kdfSalt, | ||
| ctx->kdfSaltSz, ctx->kdfInfo, ctx->kdfInfoSz, | ||
| keys, (word32)keysLen); | ||
| ret = wc_HKDF_ex(WC_SHA256, sharedSecret, sharedSz, |
There was a problem hiding this comment.
Pending HKDF callbacks retain released ECIES buffers · Use-after-free / double-free
wc_HKDF_ex() can return WC_PENDING_E, but ECIES immediately zeroes/releases sharedSecret and keys, leaving async callbacks with expired buffers. Unlike known #7105, this affects KDF temporaries in both directions.
Related known finding #7105 (similar but distinct): Both affect ECIES decryption buffer/key lifetime, but #7105 frees and repurposes the caller-owned pubKey during ciphertext parsing, whereas this candidate releases internal HKDF temporary buffers while an asynchronous callback is pending. The faulting operations, ownership violations, and fixes are distinct.
Suggested fix: Retain sharedSecret and keys in resumable ECIES state until a WC_PENDING_E KDF callback completes.
Basis: wolfSSL Asynchronous Cryptography Design: hardware-offloaded calls return WC_PENDING_E and must be invoked until completion.
| ExpectIntEQ(wc_ecc_ctx_set_peer_salt(cliCtx, srvSalt), 0); | ||
| ExpectIntEQ(wc_ecc_ctx_set_peer_salt(srvCtx, cliSalt), 0); | ||
|
|
||
| XMEMSET(&cnt, 0, sizeof(cnt)); |
There was a problem hiding this comment.
Step-count test combines encryption and decryption assertions · Weak or missing assertions
cnt is cleared only before encryption and checked after decryption, so either changed direction can omit device routing while the other supplies every required count.
Suggested fix: Reset and assert the KDF, cipher, and HMAC counters independently around encryption and decryption.
Update to make it so ECIES callbacks require an explicit devID to be set. All underlying operations if the device does not support full ECIES offload will utilize this explicit devID other than ECDH which will still utilize the devID set during the ecc key init.