ECIES: support X25519 and X448 keys - #11241
Conversation
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11241
Scan targets checked: wolfcrypt-bugs, wolfcrypt-port-bugs, wolfcrypt-rs-bugs, wolfcrypt-src, wolfssl-bugs, wolfssl-src
Findings: 10
10 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
|
Select the key type with wc_ecc_ctx_set_curve_id(), then use the new wc_ecc_encrypt_ex2()/wc_ecc_decrypt_ex2(); the ecc_key entry points keep their ABI. The ephemeral key is a raw little-endian u-coordinate (RFC 7748).
0236095 to
ccd8f55
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11241
Scan targets checked: wolfcrypt-bugs, wolfcrypt-port-bugs, wolfcrypt-rs-bugs, wolfcrypt-src, wolfssl-bugs, wolfssl-src
Findings: 6
6 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
| ret = wc_ecc_ctx_set_dev_id(ctx, devId); | ||
| #endif | ||
| if (ret == 0) { | ||
| ret = wc_ecc_encrypt_ex2(ephKey, srvKey, msg, (word32)sizeof(msg), |
There was a problem hiding this comment.
ecies_x25519_test() wire-encoding check relies on the default AES-128-CBC DEM with no WOLFSSL_AES_128 guard · Missing/incorrect platform conditionals
The context is built with wc_ecc_ctx_new(0, rng) and never pinned with wc_ecc_ctx_set_algo(), so the DEM is the default ecAES_128_CBC. In a NO_AES_128 + WOLFSSL_AES_256 build (the file's top guard admits it) wc_AesSetKey rejects the 16-byte key and wc_ecc_encrypt_ex2 returns BAD_FUNC_ARG, failing the test.
Fix: Wrap the wire-encoding block in defined(WOLFSSL_AES_128) or pin the DEM with wc_ecc_ctx_set_algo() to an algorithm the build supports.
| ret = wc_ecc_ctx_set_dev_id(ctx, devId); | ||
| #endif | ||
| if (ret == 0) { | ||
| ret = wc_ecc_encrypt_ex2(ephKey, srvKey, msg, (word32)sizeof(msg), |
There was a problem hiding this comment.
ecies_x448_test() wire-encoding check relies on the default AES-128-CBC DEM with no WOLFSSL_AES_128 guard · Missing/incorrect platform conditionals
Same defect as in ecies_x25519_test: the context from wc_ecc_ctx_new(0, rng) keeps the default ecAES_128_CBC DEM without a WOLFSSL_AES_128 guard, so wc_ecc_encrypt_ex2 returns BAD_FUNC_ARG and the test fails in a NO_AES_128 + WOLFSSL_AES_256 build.
Fix: Wrap the wire-encoding block in defined(WOLFSSL_AES_128) or pin the DEM with wc_ecc_ctx_set_algo() to an algorithm the build supports.
| return BAD_FUNC_ARG; | ||
|
|
||
| if (ctxDevId == INVALID_DEVID && keyDevId != INVALID_DEVID) { | ||
| (void)wc_ecc_ctx_set_dev_id(ctx, keyDevId); |
There was a problem hiding this comment.
ECIES crypto-callback dispatch writes the key devId into the caller's ecEncCtx, causing unbounded recursion · Resource leaks
wc_CryptoCb_EciesDevId persists privKey->devId into the caller's ecEncCtx. A callback that forwards to software by clearing only privKey->devId (the contract before this PR) re-enters dispatch because ctx->devId still names the device, recursing until the stack is exhausted. The stored devId also misroutes later operations that reuse the same ctx with an INVALID_DEVID key.
Fix: Use keyDevId as a local fallback for the dispatch decision only; do not mutate ctx->devId.
| { | ||
| /* wc_ecc_size() rather than dp->size directly: with a crypto callback | ||
| * that implements key export, the size comes from the device. */ | ||
| int sz = wc_ecc_size((ecc_key*)privKey); |
There was a problem hiding this comment.
ecies_pub_key_size no longer guarantees privKey-dp is non-NULL before ecies_peer_import dereferences it · NULL pointer dereference
The removed ecc_public_key_size() rejected key->dp == NULL; the replacement calls wc_ecc_size(), which under WOLF_CRYPTO_CB with WOLF_CRYPTO_CB_SETKEY/WOLF_CRYPTO_CB_EXPORT_KEY returns a device-supplied size for a key whose dp is NULL. wc_ecc_decrypt_ex2 then reaches ecies_peer_import, which dereferences ((ecc_key*)privKey)->dp->id at ecc.c:15920.
Related known finding #7129 (similar but distinct): Both are ECIES-related NULL-dereference precondition failures in ecc.c, but #7129 writes through ctx->kdfSalt in wc_ecc_ctx_set_kdf_salt; this dereferences privKey->dp after wc_ecc_size in ecies_pub_key_size. The operations, root objects, and required guards differ.
Fix: Reject ((ecc_key*)privKey)->dp == NULL in ecies_pub_key_size before computing the uncompressed/compressed size.
| /* Per-algorithm devIds exactly as declared with | ||
| * wc_ecc_ctx_set_algo_dev_ids(); INVALID_DEVID (the default, and the only | ||
| * value without WOLF_CRYPTO_CB) keeps that primitive in software. */ | ||
| static int ecies_enc_devid(ecEncCtx* ctx) |
There was a problem hiding this comment.
ECIES DEM/KDF/MAC primitives lose the private key's devId with no override for NULL-ctx callers · Logic errors
wc_AesInit()/wc_HmacInit()/wc_HKDF_ex() now take ctx->encDevId/kdfDevId/macDevId instead of the private key's devId, and these default to INVALID_DEVID. Callers that pass ctx == NULL get a zeroed localCtx, so their AES/HMAC offload silently drops to software — and fails outright in WOLF_CRYPTO_CB_ONLY_AES builds — with no API to restore it.
Related known finding #9931 (similar but distinct): Both affect ECIES context handling in ecc.c, but this finding passes INVALID_DEVID into AES/HMAC/HKDF when ctx is NULL, while #9931 accepts unsupported algorithm values in wc_ecc_ctx_set_algo. Their root causes and required patches are independent.
Fix: Fall back to the ecc_key's own devId when the context's per-algorithm devId is INVALID_DEVID.
| { | ||
| /* wc_ecc_size() rather than dp->size directly: with a crypto callback | ||
| * that implements key export, the size comes from the device. */ | ||
| int sz = wc_ecc_size((ecc_key*)privKey); |
There was a problem hiding this comment.
ECIES pub-key sizing drops the key-dp == NULL guard, enabling a NULL deref in peer import · NULL pointer dereference
The removed ecc_public_key_size() rejected key->dp == NULL with BAD_FUNC_ARG; its replacement uses wc_ecc_size(), which under WOLF_CRYPTO_CB with WOLF_CRYPTO_CB_SETKEY/WOLF_CRYPTO_CB_EXPORT_KEY returns a positive device-reported size for a key whose dp is NULL (ecc.c:13021-13034). wc_ecc_decrypt_ex2 then proceeds and ecies_peer_import dereferences ((ecc_key*)privKey)->dp->id at ecc.c:15920, crashing instead of returning an error.
Related known finding #7129 (similar but distinct): Both are ECIES NULL-pointer failures caused by a missing context/key precondition check, but they affect different functions and faulting operations: #7129 copies through an unset ctx->kdfSalt, while this candidate accepts a privKey with dp == NULL and later dereferences it during peer import. The root objects and required guards differ, so one patch cannot fix both.
Fix: Reject privKey->dp == NULL in the non-Montgomery branch of ecies_pub_key_size before computing the size.
2 participants
Select the key type with wc_ecc_ctx_set_curve_id(), then use the new wc_ecc_encrypt_ex2()/wc_ecc_decrypt_ex2(); the ecc_key entry points keep their ABI. The ephemeral key is a raw little-endian u-coordinate (RFC 7748).