-
Notifications
You must be signed in to change notification settings - Fork 1k
Fix ML-DSA MakePublicKey derivation, verify guards, and ASN derivation testing #10985
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
39ca493
b97f92b
e453e2a
bc2c6f2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33090,10 +33090,13 @@ static int DecodePrivateKey_ex(WOLFSSL *ssl, byte keyType, const DerBuffer* key, | |
|
|
||
| /* Set start of data to beginning of buffer. */ | ||
| idx = 0; | ||
| /* Decode the key assuming it is an ECC private key. */ | ||
| ret = wc_EccPrivateKeyDecode(key->buffer, &idx, | ||
| /* Decode the key assuming it is an ECC private key. Skip the | ||
| * best-effort public point derivation: this key is only ever used to | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ⚪ [Info] Comment claims handshake ECC key is sign-only; it also serves static ECDH · Logic errors The new comment justifying Fix: Extend the comment to mention the static-ECDH use and that |
||
| * sign (CertificateVerify / ServerKeyExchange), which needs the | ||
| * private scalar alone, and this runs once per handshake. */ | ||
| ret = EccPrivateKeyDecodeEx(key->buffer, &idx, | ||
| (ecc_key*)*hsKey, | ||
| key->length); | ||
| key->length, 0); | ||
| #ifdef WOLF_PRIVATE_KEY_ID | ||
| /* if using external key then allow using a public key */ | ||
| if (ret != 0 && (ssl->devId != INVALID_DEVID | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2290,6 +2290,10 @@ int wolfSSL_StaticEphemeralKeyLoad(WOLFSSL* ssl, int keyAlgo, void* keyPtr) | |
| if (der != NULL) { | ||
| ecc_key* key = (ecc_key*)keyPtr; | ||
| WOLFSSL_MSG("Using static ECDH key"); | ||
| /* Keep the best-effort public point derivation: the caller | ||
| * (TLSX_KeyShare_GenEccKey) exports this key's public point | ||
| * with wc_ecc_export_x963(), which fails ECC_PRIVATEONLY_E on | ||
| * an ECC_PRIVATEKEY_ONLY key. */ | ||
| ret = wc_EccPrivateKeyDecode(der->buffer, &idx, key, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 [Low] Static ephemeral ECDH public-point derivation runs without scalar-mult blinding · Constant-time violations This branch deliberately keeps the new decode-time Fix: Call |
||
| der->length); | ||
| } | ||
|
|
@@ -2370,7 +2374,9 @@ static int DetectStaticEphemeralKeyType(const byte* keyBuf, unsigned int keySz, | |
| ret = wc_ecc_init_ex(eccKey, heap, INVALID_DEVID); | ||
| } | ||
| if (ret == 0) { | ||
| ret = wc_EccPrivateKeyDecode(keyBuf, &idx, eccKey, keySz); | ||
| /* Pure type probe - key is freed right below, so skip the | ||
| * best-effort public point derivation done on decode. */ | ||
| ret = EccPrivateKeyDecodeEx(keyBuf, &idx, eccKey, keySz, 0); | ||
| if (ret == 0) { | ||
| *keyAlgo = WC_PK_TYPE_ECDH; | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔵 [Low] Doxygen for the new/changed ML-DSA APIs is inconsistent with the implemented return codes
🔧 NIT
styleTwo small doc mismatches. (1) The new
wc_MlDsaKey_MakePublicKeyblock lists0 / BAD_FUNC_ARG / MEMORY_E / Other negativebut omitsPUBLIC_KEY_E, which is the function's most interesting documented failure (t0 ortrmismatch, wc_mldsa.c:11634 and :11650) — and every export function's block in the same file does list it. (2) Thewc_MlDsaKey_PrivateKeyDecodeaddition says the public key is "derived best-effort on demand by export functions", but the export functions now hard-fail withPUBLIC_KEY_E/MEMORY_Ewhen derivation fails (as those same doc blocks state); "best-effort" describes the ECC path, not this one, and reads as a contradiction.Suggestion:
Recommendation: Add PUBLIC_KEY_E to the MakePublicKey return list, and reword the PrivateKeyDecode note to "public key left unset; derived on demand by the export functions, which fail if derivation fails."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed as recommended