Validate EC_PUB_X/EC_PUB_Y on import by routine them through the X9.63 point import by gasbytes · Pull Request #467 · wolfSSL/wolfProvider

gasbytes · 2026-08-13T12:41:14Z

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.

Copilot

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_Y imports through wc_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.

padelsbach · 2026-08-14T05:15:29Z

            ok = 0;
        }
        if (ok) {
+            /* wc_ecc_import_x963_ex only checks the point against the curve


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.

yup you are right, will make it a helper and will apply the check there too.
thanks!

padelsbach · 2026-08-14T05:21:56Z

+        int rc;
+        int origType;
+
+        rc = wc_ecc_import_x963_ex(point, 1 + (2 * (word32)size), &ecc->key,


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?

yeah it's truthful but not really dangerous in my opinion, since the worst case is an error. the key is marked public-only, so wolfssl refuses to sign or derive with it and master did the same thing here already.
I fixed it anyway since it's cleaner anyway to go through a scratch key, and the private key is left alone instead.

padelsbach · 2026-08-14T05:25:12Z

+            /* 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;


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?

padelsbach · 2026-08-14T05:28:18Z

Not directly related, but stumbled upon this block where X and Y are octet_strings. I think they should be OSSL_PARAM_BN

wolfSSL-Bot · 2026-08-17T18:44:17Z

Horray! No issues found.
No test results found.

padelsbach

More findings, sorry for another big batch

padelsbach · 2026-08-17T19:48:35Z

+
+    rc = wc_ecc_import_x963_ex(data, len, &ecc->key, ecc->curveId);
+    if (rc != 0) {
+        WOLFPROV_MSG_DEBUG_RETCODE(WP_LOG_LEVEL_DEBUG, "wc_ecc_import_x963_ex",


I think we need to clear hasPub on this failure path

fixed in previous commit, now squashed.

padelsbach · 2026-08-17T20:02:01Z

+    if (!wp_ecc_set_params_pub_xy(ecc, params)) {
        ok = 0;
    }
    if (wp_ecc_set_params_enc_pub_key(ecc, params,


For completeness, can you add the (ok == 1) condition to the other checks in this function?

padelsbach · 2026-08-17T20:14:33Z

+        err = EVP_PKEY_fromdata_init(ctx) != 1;
+    }
+    if (err == 0) {
+        ret = EVP_PKEY_fromdata(ctx, pkey, EVP_PKEY_PUBLIC_KEY, params) == 1;


AI tells me we don't have a test which builds an off-curve public key, per the scenario in the Fenrir finding. Can you please check?

Also, is there a positive/valid test case for public import?

AI tells me we don't have a test which builds an off-curve public key, per the scenario in the Fenrir finding. Can you please check?

there is one which is test_ec_import_off_curve_xy(), it takes the valid p-256 point and flips a bit in Y, then feeds it in as x/y.

Also, is there a positive/valid test case for public import?

adding one that imports valid x/y and checks the result matches and it's correct. thanks.

padelsbach · 2026-08-17T20:17:12Z

+
+    WOLFPROV_ENTER(WP_LOG_COMP_ECC, "wp_ecc_import_pub_x963");
+
+    rc = wc_ecc_import_x963_ex(data, len, &ecc->key, ecc->curveId);


AI says we should use a local ecc_key instead of ecc->key here since wc_ecc_import_x963_ex can succeed and write the point into the struct, then remain there after a failure below.

fixed in previous commit, now squashed.

padelsbach · 2026-08-17T20:21:29Z

+        ok = 0;
+    }
+    if (ok) {
+        rc = wc_ecc_check_key(&ecc->key);


Thinking we should add this check to the other pub key decode calls in this file, wp_ecc_decode_spki and wp_ecc_decode_x963_pub

added the missing checks.

padelsbach · 2026-08-17T20:23:56Z

+    int ok = 1;
+    int rc;
+
+    WOLFPROV_ENTER(WP_LOG_COMP_ECC, "wp_ecc_import_pub_x963");


AI says we need a lock around the two steps in this function (import and check). Can you investigate?

looked into it, I don't think that's needed. since both steps operate on pub which is local and there's nothing shared that would cause a race as far as I can tell.

…3 point import

gasbytes self-assigned this Aug 13, 2026

Copilot AI lite review requested due to automatic review settings August 13, 2026 12:41

Copilot started reviewing on behalf of gasbytes August 13, 2026 12:41 View session

gasbytes marked this pull request as ready for review August 13, 2026 12:44

Copilot AI reviewed Aug 13, 2026

View reviewed changes

Comment thread test/test_ecc.c

gasbytes force-pushed the wp_ecc_set_params_pub-fix branch from 41a341d to 6c5beb7 Compare August 13, 2026 13:23

gasbytes assigned padelsbach and unassigned gasbytes Aug 13, 2026

gasbytes requested a review from padelsbach August 13, 2026 14:45

padelsbach requested changes Aug 14, 2026

View reviewed changes

gasbytes force-pushed the wp_ecc_set_params_pub-fix branch from 6c5beb7 to a41af83 Compare August 17, 2026 18:01

padelsbach requested changes Aug 17, 2026

View reviewed changes

gasbytes force-pushed the wp_ecc_set_params_pub-fix branch from a41af83 to 3be1a89 Compare August 18, 2026 10:28

validate EC_PUB_X/EC_PUB_Y on import by routine them through the X9.6…

e5663ab

…3 point import

gasbytes force-pushed the wp_ecc_set_params_pub-fix branch from 3be1a89 to e5663ab Compare August 18, 2026 12:13

gasbytes requested a review from padelsbach August 18, 2026 13:12


		WOLFPROV_ENTER(WP_LOG_COMP_ECC, "wp_ecc_import_pub_x963");

		rc = wc_ecc_import_x963_ex(data, len, &ecc->key, ecc->curveId);

Uh oh!

Conversation

gasbytes commented Aug 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gasbytes Aug 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wolfSSL-Bot commented Aug 17, 2026

Uh oh!

padelsbach left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gasbytes Aug 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gasbytes Aug 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gasbytes commented Aug 13, 2026 •

edited

Loading

gasbytes Aug 18, 2026 •

edited

Loading

gasbytes Aug 18, 2026 •

edited

Loading

gasbytes Aug 18, 2026 •

edited

Loading