Repository navigation
Conversation
|
There was a problem hiding this comment.
🟡 Changes recommended
Async raw ECC verification fails with SP math because signature operands are uninitialized, and the new tests incorrectly fail configurations without public-key derivation or an available key-generation device.
3 open issues
What changed in this PR
This PR corrects wolfCrypt’s handling of ECC curves whose order is wider than the prime, preventing private-key truncation and supporting correctly sized exports and raw signatures.
Changes:
- Extends private-key blinding, private-only exports, DER encoding and NO_ASN signatures to cover the order width.
- Adds export length queries and compatible import handling while preserving prime-width component exports.
- Adds regression tests, small-curve CI configurations and API documentation.
| File | Description |
|---|---|
wolfcrypt/test/test.c |
Adds raw-signature tests, guards ASN-only tests and enlarges callback buffers. |
wolfcrypt/src/wc_pkcs11.c |
Enlarges private-key export storage and cleanup. |
wolfcrypt/src/ecc.c |
Corrects blinding widths, export sizing, imports and raw signatures. |
wolfcrypt/src/asn.c |
Updates DER sizing and private-key buffers. |
wolfcrypt/src/asn_orig.c |
Updates legacy DER encoding for wider private keys. |
tests/api/test_ecc.h |
Registers the order-width regression test. |
tests/api/test_ecc.c |
Tests ECDH, exports, imports, DER round trips and length queries. |
tests/api.c |
Enlarges crypto-callback private-key buffers. |
doc/dox_comments/header_files/ecc.h |
Documents export sizes and length-query behavior. |
.github/configs/os-check-linux.json |
Adds small-curve blinding and ASN configuration coverage. |
Review effort: Balanced
e2a3424 to
9d0a7d9
Compare
With WOLFSSL_ECC_BLIND_K the private scalar is stored XORed with a random mask, and the XORs were dp->size bytes wide. On secp160r1, secp160r2, secp160k1 and secp224k1 the order has one bit more than the prime, so a valid scalar of 2^(8*size) or more lost its top bit when blinded at import or key generation. The key then acted as d - 2^(8*size): the public key, ECDH results and signatures were all wrong. Size the mask from ECC_KEY_MAX_BITS, which already includes the extra order bit. These curves are compiled out with FP_ECC, which --enable-all turns on, so the existing BLIND_K CI entry never reached them. Add an os-check entry with BLIND_K, FP_ECC off and a 160-bit minimum key size, and a test that uses d = n - 1, whose ECDH result with G must be Gx. Fixes F-16894.
9d0a7d9 to
c2776b6
Compare
wc_ecc_export_private_only() wrote the private scalar at dp->size bytes. On secp160r1, secp160r2, secp160k1 and secp224k1 the order has one bit more than the prime, so a valid scalar of 2^(8*size) or more failed to export with BUFFER_E, while *outLen claimed dp->size was enough. The DER encoders failed with it. Export the scalar zero-padded to the size of the order instead, as RFC 5915 specifies and OpenSSL does. Private keys on these curves are now one byte longer for every key, so a caller sizing the buffer with wc_ecc_size() gets BUFFER_E and the needed size. The order size comes from the curve's order string, with no allocation or failure path. The component exports, wc_ecc_export_ex() and wc_ecc_export_private_raw(), keep d at dp->size. They pair with wc_ecc_import_unsigned(), which reads d at that size with no length, so a wider d would load the wrong key. The ECC white-box test, which calls _ecc_export_ex() directly, passes the new argument. wc_ecc_export_private_only() with a NULL buffer now returns LENGTH_ONLY_E and the size the export needs: the order size, or the black-key size for CAAM. It does not read the private key, and a key exported through a crypto callback need not hold the private part locally. The DER encoders use it, with the private key read unlocked as for the export itself, instead of assuming a size. They fall back to dp->size, plus the MAC size for a QNX CAAM black key in asn_orig.c, when it is not supported, so they stay correct with a FIPS module's ecc.c. wc_CheckPrivateKey(), the PKCS#11 key import and the crypto callback test handlers size their buffers with MAX_ECC_BYTES + 1, which a FIPS module's ecc.h also has, and the STM32 CCB key generation falls back to software up front on these curves, as it does for a curve the CCB cannot wrap. The PKCS#11 key import zeroes the buffer it allocated rather than the size a failed export reports. Importing a private key without a curve id picks the curve by size, and the longer export matched no curve. A size one byte longer than every curve now selects the curve whose order has that size, so an export from secp160r1 still imports. Without ASN.1, signatures are raw r || s, also written at dp->size, so an r or s of 2^(8*size) or more was truncated with the error ignored. Write them at the size of the order, check the conversions, set *outlen only when they succeed, and check *outlen before signing, which was never done. Verification accepts both sizes, so signatures from older versions still verify, initializes r and s for asynchronous builds and checks their reads, and no longer leaks them on a wrong size. Crypto- callback-only builds, which never use the raw signature size, no longer declare it, so they build again without ASN.1. Add a test of both sizes to wolfcrypt/test, guard its explicit-curve decode tests so the NO_ASN build compiles, and add os-check entries for NO_ASN and for the original ASN.1 code with these curves. Fixes F-16898.
c2776b6 to
d4af6fa
Compare
|
Jenkins retest this please |
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| *outLen = ecc_order_size(key->dp); | ||
| #if defined(WOLFSSL_QNX_CAAM) || defined(WOLFSSL_IMXRT1170_CAAM) | ||
| if ((key->blackKey > 0) && (key->blackKey != CAAM_BLACK_KEY_ECB)) { | ||
| *outLen = (word32)key->dp->size + WC_CAAM_MAC_SZ; | ||
| } | ||
| #endif | ||
| return WC_NO_ERR_TRACE(LENGTH_ONLY_E); | ||
| } | ||
|
|
||
| return ecc_export_ex(key, NULL, NULL, NULL, NULL, out, outLen, encType, 1); |
There was a problem hiding this comment.
🔴 Code that re-imports wc_ecc_export_private_only() output with wc_ecc_import_unsigned() now silently loads the wrong private key on secp160r1/r2/k1 and secp224k1. The export is now one byte longer, at the order size. wc_ecc_import_unsigned() still reads exactly key->dp->size bytes of d (ecc.c:13170). So it gets the leading 0x00 plus all but the last byte, which is d >> 8, and returns no error. Fix: keep the WOLFSSL_ABI export at the prime size and give the DER encoders an internal order-size export, or otherwise make this size mismatch fail instead of corrupting the key.
Why this was flagged
The trigger is an out-of-tree caller that exports with wc_ecc_export_private_only() and re-imports through wc_ecc_import_unsigned(key, qx, qy, d, curveId) on one of the four curves. The PR names wolfHSM; other applications cannot be enumerated from this checkout. ecc.c:12520 passes dOrderSz=1, so _ecc_export_ex (ecc.c:12386) writes ecc_order_size() bytes (21 or 29). Because n exceeds 2^160 or 2^224 only slightly, almost every key starts with a 0x00 byte. _ecc_import_raw_private reads (word32)key->dp->size bytes at ecc.c:13170, so k becomes d >> 8. That value is still in [1, n-1], so the range check passes, and only WOLFSSL_VALIDATE_ECC_IMPORT would compare it with Q. Signatures and ECDH secrets then come from the wrong key, with no error. On the base branch the 20- or 28-byte export round-trips exactly. The PR description treats this as a wolfHSM deploy-order note. It does not say the failure is a silently wrong key, and it applies to every such caller. Meanwhile, the d >= 2^(8*size) failure that the raw-export change fixes occurs with probability about 2^-80 for generated keys.
Verification: wc_ecc_export_private_only() now writes d at ecc_order_size(key->dp) (ecc.c:~12385), 21 bytes on the 160-bit curves and 29 on secp224k1. On the base it was keySz = dp->size (20 or 28). _ecc_import_raw_private still reads key->dp->size bytes (ecc.c:13170-13171), so it loads d >> 8 and returns no error. No in-repo caller pairs the two functions.
| static int _ecc_export_ex(ecc_key* key, byte* qx, word32* qxLen, | ||
| byte* qy, word32* qyLen, byte* d, word32* dLen, | ||
| int encType) | ||
| int encType, int dOrderSz) | ||
| { | ||
| int err = 0; | ||
| word32 keySz; |
There was a problem hiding this comment.
🔴 Builds with the Microchip ATECC508A/608A or TA100 ports (--enable-microchip) would likely stop compiling under the default -Werror. The new dOrderSz parameter of _ecc_export_ex() is only read in the #else branch at ecc.c:12385. Those ports compile that branch out and return NOT_COMPILED_IN at ecc.c:12346. With -Wall -Wextra, gcc then reports an unused parameter. Fix: add (void)dOrderSz; in the ATECC/TA100 branch, or once at the top of _ecc_export_ex(), so every port configuration still compiles.
Why this was flagged
The trigger is a build with WOLFSSL_ATECC508A, WOLFSSL_ATECC608A or WOLFSSL_MICROCHIP_TA100 defined, which configure.ac:3964-3972 sets for --enable-microchip=508/608/100. In _ecc_export_ex() (ecc.c:12321), the only read of the new int dOrderSz parameter is inside the #else of #if defined(WOLFSSL_ATECC508A) || ... WOLFSSL_MICROCHIP_TA100 (ecc.c:12385-12386). That #if branch returns NOT_COMPILED_IN at ecc.c:12346 and never references dOrderSz. m4/ax_harden_compiler_flags.m4 adds -Wall, -Wextra and -Wunused (lines 121, 122, 157), which together enable -Wunused-parameter. It also adds -Werror in a VCS checkout (line 107), so ecc.c fails to compile for these ports. On the base branch every parameter of _ecc_export_ex() was also used in the ATECC branch, because encType is still read by the qx/qy exports. Nothing in the function casts dOrderSz to void.
Verification: The diff adds int dOrderSz to _ecc_export_ex() (ecc.c:12321-12323). Its only read is at ecc.c:12385-12386, inside the #else of the ATECC/TA100 #if (ecc.c:12343-12347). That #if branch only does return NOT_COMPILED_IN;. No (void)dOrderSz; exists in ecc.c. -Werror is added in a VCS checkout (m4/ax_harden_compiler_flags.m4:106-108). So the warning is new with this diff.
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #12037
Scan targets checked: wolfcrypt-src, wolfcrypt-bugs, wolfssl-bugs
Not reviewed (no scan target covers them): .github/configs/os-check-linux.json, doc/dox_comments/header_files/ecc.h, tests/api/test_ecc.h
Coverage: 7 of 8 in-scope changed file(s) opened by the reviewer; not opened: tests/unit-mcdc/test_ecc_whitebox.c
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
Description
On secp160r1/r2/k1 and secp224k1 the order has one bit more than the prime.
WOLFSSL_ECC_BLIND_Kmasked only the prime's width of the private key, and private keys and raw signatures were written at the prime's size, failing or truncating for large values.Fixes F-16894 and F-16898.
WOLFSSL_ECC_BLIND_Kblinds the private key across the full order widthwc_ecc_export_private_only()and the DER encoders write the key at the order size, as RFC 5915 and OpenSSL dowc_ecc_export_private_only()with a NULL buffer returnsLENGTH_ONLY_Eand the size*outlenis checked before signing; verify accepts both sizeswc_ecc_import_unsigned()readsBreaking changes
wc_ecc_export_private_only()output on these curves is one byte longer, so a buffer sized withwc_ecc_size()getsBUFFER_Ewc_ecc_import_unsigned(); the fix on branchFrauschi:ecc-private-only-update-raw-exportshould land firstTesting
test_wc_ecc_order_wider_than_field: ECDH with d = n-1, export sizes, re-import and DER round trips on all four curvestest_wc_ecc_export_private_onlycovers the length queryecc-blind-k-small-curves,ecc-asn-original-small-curvesandecc-no-asn-small-curves