Conversation
|
|
Jenkins retest this please |
|
Retest this please Jenkins. The history is no longer available. |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Fixes a regression in ECC point validation during X9.63 public key import by ensuring the legacy import wrapper treats inputs as untrusted, and adds a regression test to catch off-curve points in the future.
Changes:
- Update
wc_ecc_import_x963_exto passuntrusted=1so imported points are validated as on-curve. - Add an API test that imports a deliberately off-curve P-256 point and expects rejection.
- Register the new test in the ECC API test header.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| wolfcrypt/src/ecc.c | Restores on-curve validation by treating X9.63 imports as untrusted in the wrapper. |
| tests/api/test_ecc.h | Declares and registers a new ECC X9.63 off-curve regression test. |
| tests/api/test_ecc.c | Adds a regression test asserting off-curve X9.63 points are rejected. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * Regression coverage for the invalid-curve attack: the legacy wrapper | ||
| * wc_ecc_import_x963_ex must pass untrusted=1 to wc_ecc_import_x963_ex2 | ||
| * so that ECIES, PKCS#7 KARI, and EVP ECDH callers validate that the | ||
| * imported point actually lies on the curve. Without that, an attacker | ||
| * can feed a point from a weak twist and leak the victim's private | ||
| * scalar modulo small primes (Biehl-Meyer-Mueller). |
There was a problem hiding this comment.
The comment is internally inconsistent: it says it’s testing wc_ecc_import_x963(), but then describes the regression as being in the wc_ecc_import_x963_ex wrapper. Please update the comment to match the function/path actually being tested (or update the test to exercise the wrapper that regressed) so future readers understand what regression this test is guarding.
| * Regression coverage for the invalid-curve attack: the legacy wrapper | |
| * wc_ecc_import_x963_ex must pass untrusted=1 to wc_ecc_import_x963_ex2 | |
| * so that ECIES, PKCS#7 KARI, and EVP ECDH callers validate that the | |
| * imported point actually lies on the curve. Without that, an attacker | |
| * can feed a point from a weak twist and leak the victim's private | |
| * scalar modulo small primes (Biehl-Meyer-Mueller). | |
| * Regression coverage for the invalid-curve attack in the public-point | |
| * import validation path. This test directly exercises | |
| * wc_ecc_import_x963(), which must reject points that do not lie on the | |
| * selected curve. A related historical regression was in the legacy | |
| * wc_ecc_import_x963_ex wrapper, which must pass untrusted=1 to | |
| * wc_ecc_import_x963_ex2 so higher-level callers validate imported | |
| * public points before use. |
| ExpectIntNE(wc_ecc_import_x963(offCurveX963, (word32)sizeof(offCurveX963), | ||
| &pubKey), 0); |
There was a problem hiding this comment.
This test currently calls wc_ecc_import_x963(...), but the regression described in the PR is in the legacy wrapper wc_ecc_import_x963_ex(...) passing the untrusted flag. To ensure the test actually covers the fixed code path, consider calling wc_ecc_import_x963_ex(offCurveX963, sizeof(offCurveX963), &pubKey, ECC_SECP256R1) (or the appropriate curve id constant used elsewhere in this test suite). If wc_ecc_import_x963 already routes through _ex, please make that explicit in the test comment to avoid ambiguity.
Fix a regression accidentally added in #9851 due to rebasing after #10133 has landed. Added a test to catch this in the future.
Fixes zd#21654.