Repository navigation
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The existing P-256 tests cannot detect this regression; a test using differing prime and order lengths must verify that the private-only verify/update path preserves the scalar under wolfSSL’s order-sized export behavior.
1 open issue
What changed in this PR
This PR preserves ECC private scalars during private-only key updates as wolfSSL moves private-only exports to order-sized encoding.
Changes:
- Uses
wc_ecc_export_private_raw()to retain the prime-sized encoding expected bywc_ecc_import_unsigned(). - Adds a comment explaining the import size requirement.
| File | Description |
|---|---|
src/wh_crypto.c |
Changes private-scalar export to match the importer’s prime-size requirement. |
Review effort: Balanced
wh_Crypto_EccUpdatePrivateOnlyKeyDer() exports the private scalar with wc_ecc_export_private_only() and re-imports it, with the public key from the server's response, through wc_ecc_import_unsigned(). That import reads d at the size of the prime and takes no length. wolfSSL is changing wc_ecc_export_private_only() to write the scalar zero-padded to the size of the curve order, as RFC 5915 and OpenSSL do. On secp160r1, secp160r2, secp160k1 and secp224k1 the order is one byte longer than the prime, so the export would be 21 or 29 bytes and the import would read the first 20 or 28: after a client verify the private-only key would silently hold a different scalar. Export with wc_ecc_export_private_raw() instead. It writes d at the size of the prime in current wolfSSL and in every release wolfHSM builds with, so the import always gets the layout it expects. Nothing tested this path: no test verified with a private-only key. Add one in test-refactor that does so on P-256 and on secp224k1, whose order is a byte longer than its prime, and checks that the key gains its public point and keeps its scalar. The test configuration enables Koblitz curves for it.
b20bf9a to
6ecf87c
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #560
Scan targets checked: wolfhsm-crypto-bugs, wolfhsm-src
Not reviewed (no scan target covers them): test/config/user_settings.h
Coverage: 2 of 2 in-scope changed file(s) opened by the reviewer
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.
Review tier: Max
wh_Crypto_EccUpdatePrivateOnlyKeyDer()exports the private scalar withwc_ecc_export_private_only()and re-imports it withwc_ecc_import_unsigned(), which reads d at the size of the prime with no length. wolfSSL is movingwc_ecc_export_private_only()to the size of the curve order, as RFC 5915 and OpenSSL do, so on secp160r1/r2/k1 and secp224k1, whose order is a byte longer than the prime, a private-only key would come back as a different key after a client verify.Changes:
wc_ecc_export_private_raw(), which writes it at the size of the prime in current and older wolfSSL releases.Testing:
test-refactortest verifies with a private-only key on P-256 and secp224k1 and checks the scalar is unchanged; the test config enables Koblitz curves for it.