Uh oh!
There was an error while loading. Please reload this page.
Validate EC_PUB_X/EC_PUB_Y on import by routine them through the X9.63 point import - #467
Validate EC_PUB_X/EC_PUB_Y on import by routine them through the X9.63 point import#467gasbytes wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens ECC public key imports by validating raw EC_PUB_X/EC_PUB_Y coordinates via the same X9.63 point import path already used for encoded public keys, ensuring points are on-curve and that X/Y are provided together. It also adds regression tests to cover off-curve and incomplete-coordinate edge cases.
Changes:
- Route
EC_PUB_X/EC_PUB_Yimports throughwc_ecc_import_x963_ex()by constructing an uncompressed X9.63 point, enforcing on-curve validation and proper point initialization. - Reject public key imports that provide only one ordinate (X without Y, or vice versa).
- Add regression tests for off-curve XY import rejection, X-only rejection, and an ECDH derive check with an off-curve peer key.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| test/test_ecc.c | Adds regression tests for off-curve and incomplete-coordinate public key imports, plus an ECDH-path guard test. |
| src/wp_ecc_kmgmt.c | Validates EC_PUB_X/EC_PUB_Y imports by converting to an X9.63 point and importing via wc_ecc_import_x963_ex(). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Uh oh!
There was an error while loading. Please reload this page.
41a341d to
6c5beb7Compare| ok = 0; | ||
| } | ||
| if (ok) { | ||
| /* wc_ecc_import_x963_ex only checks the point against the curve |
There was a problem hiding this comment.
According to the logic in this comment, we should also apply the check below in wp_ecc_set_params_enc_pub_key. Maybe add a helper and call from both?
iiuc, WOLFSSL_VALIDATE_ECC_IMPORT is fairly new (or at least the checking is), so we want to keep this check for older wolfSSL versions.
| int rc; | ||
| int origType; | ||
| rc = wc_ecc_import_x963_ex(point, 1 + (2 * (word32)size), &ecc->key, |
There was a problem hiding this comment.
AI tells me this call can overwrite/zero existing fields within the ecc key, which is fine for import, but potentially dangerous when called from wp_ecc_set_params. Can you investigate?
| /* wc_ecc_import_x963_ex only checks the point against the curve | ||
| * when wolfSSL is built with WOLFSSL_VALIDATE_ECC_IMPORT, so check | ||
| * it here instead of relying on the build options. */ | ||
| origType = ecc->key.type; |
There was a problem hiding this comment.
AI says ecc->key.type is always ECC_PUBLICKEY so the save+restore is not needed. But zooming out, do we need to check the key we just imported?
| static const OSSL_PARAM wp_ecc_supported_settable_params[] = { | ||
| OSSL_PARAM_int(OSSL_PKEY_PARAM_USE_COFACTOR_ECDH, NULL), | ||
| OSSL_PARAM_octet_string(OSSL_PKEY_PARAM_ENCODED_PUBLIC_KEY, NULL, 0), | ||
| OSSL_PARAM_octet_string(OSSL_PKEY_PARAM_EC_PUB_X, NULL, 0), |
There was a problem hiding this comment.
Not directly related, but stumbled upon this block where X and Y are octet_strings. I think they should be OSSL_PARAM_BN
Route EC_PUB_x/EC_PUB_Y through wc_ecc_import_x963_ex, the same validated import the encoded public key path uses, so the point is checked to be onn the curve,
both ordinates are required together, and the projective z ordinate is set.
Also, added some 3 regression tests for the off-curve and X-without-Y cases to test the edge cases of this behaviour.
Fixes F-4694.