Fix : accept non-canonical public values per RFC 7748 - #11272
Conversation
|
Can one of the admins verify this patch? 8000 |
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Expands validation and shared-secret test coverage for X25519/X448 around RFC 7748 non-canonical encodings and low-order public values, and updates public key validation logic to accept required non-canonical inputs while rejecting cheaply detectable low-order points.
Changes:
- Update
wc_curve25519_check_public()/wc_curve448_check_public()to reject only specific low-order (and non-canonical low-order) encodings while accepting the RFC-mandated non-canonical ranges. - Add new known-answer tests and edge-case tests for shared secret derivation and public key validation (canonical vs non-canonical equivalence).
- Add a TLS 1.3 server-side test that feeds a canned ClientHello to verify X25519 non-canonical key shares are accepted while low-order points are rejected.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| wolfcrypt/test/test.c | Adds non-canonical accept/reject vectors and non-canonical shared-secret KATs for Curve25519/Curve448 in the wolfcrypt test suite. |
| wolfcrypt/src/curve448.c | Refines Curve448 public-key validation to reject p-1/p/p+1 (and canonical 0/1) while accepting other non-canonical values per RFC 7748. |
| wolfcrypt/src/curve25519.c | Refines Curve25519 public-key validation to reject only p-1/p/p+1 (and 0/1) while accepting p+2..p+18 per RFC 7748. |
| tests/api/test_tls13.h | Registers a new TLS 1.3 X25519 non-canonical key share API test. |
| tests/api/test_tls13.c | Adds the TLS 1.3 canned-ClientHello test to exercise X25519 non-canonical key shares and low-order rejection behavior. |
| tests/api/test_curve448.h | Registers a new Curve448 non-canonical shared-secret KAT API test. |
| tests/api/test_curve448.c | Adds Curve448 non-canonical shared-secret KATs and clarifies/extends check_public tests for non-canonical cases. |
| tests/api/test_curve25519.h | Registers a new Curve25519 non-canonical shared-secret KAT API test. |
| tests/api/test_curve25519.c | Extends Curve25519 zero-check tests for canonical/non-canonical low-order points and adds non-canonical shared-secret KATs. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| /* Other cheaply encoded low-order points, canonical and non-canonical: | ||
| * 1, p-1 (u = -1), p (== 0) and p+1 (== 1), p = 2^255-19. None may | ||
| * yield a shared secret, independent of wc_curve25519_check_public. */ | ||
| { | ||
| static const byte kLowByte[4] = { 0x01, 0xec, 0xed, 0xee }; | ||
| byte low_pub[CURVE25519_KEYSIZE]; | ||
| int i; | ||
|
|
||
| for (i = 0; i < 4; i++) { | ||
| if (i == 0) { | ||
| XMEMSET(low_pub, 0, sizeof(low_pub)); | ||
| } | ||
| else { | ||
| XMEMSET(low_pub, 0xff, sizeof(low_pub)); | ||
| low_pub[CURVE25519_KEYSIZE - 1] = 0x7f; | ||
| } | ||
| low_pub[0] = kLowByte[i]; | ||
| ExpectIntEQ(wc_curve25519_import_public_ex(low_pub, | ||
| sizeof(low_pub), &public_key, EC25519_LITTLE_ENDIAN), 0); | ||
| outLen = sizeof(out); | ||
| ExpectIntEQ(wc_curve25519_shared_secret_ex(&private_key, | ||
| &public_key, out, &outLen, EC25519_LITTLE_ENDIAN), | ||
| WC_NO_ERR_TRACE(ECC_OUT_OF_RANGE_E)); | ||
| } | ||
| } |
| const int ksOffset = 97; | ||
| byte ch[sizeof(chTemplate)]; |
| testCertFile = eccCertFile; | ||
| testKeyFile = eccKeyFile; | ||
| #endif | ||
|
|
|
retest this please |
There was a problem hiding this comment.
Fenrir Automated Review — PR #11272
Scan targets checked: wolfcrypt-bugs, wolfcrypt-port-bugs, wolfcrypt-rs-bugs, wolfcrypt-src, wolfssl-bugs, wolfssl-src
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.
| ERROR_OUT(WC_TEST_RET_ENC_I(i), done); | ||
| sharedSz = sizeof(shared); | ||
| XMEMSET(shared, 0, sizeof(shared)); | ||
| ret = wc_curve25519_shared_secret_ex(&userA, &userB, shared, &sharedSz, |
There was a problem hiding this comment.
New curve25519 KAT in test.c does not handle WC_PENDING_E from async shared-secret · Incorrect error handling
wc_curve25519_shared_secret_ex() returns WC_PENDING_E (curve25519.c:831) when built with WOLFSSL_ASYNC_CRYPT + WC_ASYNC_ENABLE_X25519 + WC_X25519_NONBLOCK and a valid devId (the combination in examples/async/user_settings.h). The new loop treats that as a hard failure, so wolfcrypt_test() fails spuriously; every other X25519 shared-secret call in test.c retries via wc_AsyncWait.
Related known finding #3755 (similar but distinct): Both involve Curve25519 error handling, but the candidate mishandles WC_PENDING_E from wc_curve25519_shared_secret_ex in a test KAT, while issue 3755 writes public-key output after wc_curve25519_make_pub fails in wc_curve25519_export_public_ex. The operations and root causes differ, and each needs a separate patch.
Fix: Add the file's #if defined(WOLFSSL_ASYNC_CRYPT) WC_PENDING_E / wc_AsyncWait(ret, &userA.asyncDev, WC_ASYNC_FLAG_NONE) retry, as at test.c:48326.
|
This pull request significantly expands and clarifies the test coverage for Curve25519 and Curve448 public key validation and shared secret derivation, especially around edge cases involving non-canonical and low-order public values as specified by RFC 7748. The changes ensure that the implementation strictly adheres to the RFC's requirements for accepting or rejecting specific encodings, and adds known-answer tests to confirm correct shared secret computation for non-canonical inputs.
The most important changes are:
Curve25519 Test Enhancements:
test_wc_curve25519_check_public_leandtest_wc_curve25519_check_public_beto cover non-canonical encodings of low-order values (e.g., p, p+1, p+2...p+18), verifying that they are accepted or rejected according to RFC 7748 and that verdicts match canonical encodings. [1] [2]test_wc_curve25519_shared_secret_noncanonical_kat, which validates that shared secrets derived from non-canonical public values (p+2, p+9, p+18) match those from their canonical counterparts, using expected results from OpenSSL. This function is registered in the test suite. [1] [2] [3]test_wc_curve25519_shared_secret_zero_checkto test additional low-order points (both canonical and non-canonical encodings) to ensure they are properly rejected during shared secret computation, regardless of public key validation.Curve448 Test Clarifications:
test_wc_curve448_check_public_leandtest_wc_curve448_check_public_beto clarify the handling of non-canonical and low-order public values, ensuring acceptance or rejection aligns with RFC 7748. This includes explicit tests for p+1 (non-canonical 1, rejected), p+2 (accepted), and p-1 (rejected). [1] [2] [3]These changes improve the robustness of the test suite, ensure standards compliance, and clarify the expected behavior for edge-case public key encodings.