Fix peer key masking - #11283
Conversation
Sorry, something went wrong.
|
Can one of the admins verify this patch? |
Sorry, something went wrong.
There was a problem hiding this comment.
Pull request overview
This PR updates wolfSSL’s TLS handshake processing to comply with RFC 7748 §5 by masking (clearing) the reserved high bit in received 32-byte X25519 u-coordinates instead of rejecting them, while preserving the existing opt-in strict behavior when WOLFSSL_X25519_NO_MASK_PEER is defined. It also adds a TLS 1.3 regression test to ensure a tampered key_share with the reserved bit set no longer fails the handshake with ECC_PEERKEY_ERROR.
Changes:
- Mask the reserved high bit on received X25519 peer public values before calling wc_curve25519_check_public() and before importing the key (TLS 1.3 KeyShare and TLS 1.2 key exchange paths).
- Add a TLS 1.3 memio regression test that flips the reserved bit in the ClientHello X25519 key_share and confirms the server continues the handshake.
- Register the new test in the TLS 1.3 API test declarations.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/api/test_tls13.h | Declares and registers the new TLS 1.3 regression test. |
| tests/api/test_tls13.c | Adds a memio-based TLS 1.3 test that tampers the X25519 key_share reserved bit and asserts the server proceeds. |
| src/tls.c | Masks the reserved high bit of the peer’s X25519 key_share value before validation/import in the TLS 1.3 path (unless WOLFSSL_X25519_NO_MASK_PEER). |
| src/internal.c | Masks the reserved high bit before validation/import in the TLS 1.2 peer public key parsing/import paths (unless WOLFSSL_X25519_NO_MASK_PEER). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Sorry, something went wrong.
|
Sorry, something went wrong.
test_tls13_x25519_keyshare_masks_reserved_bit always expected WOLFSSL_ERROR_WANT_READ, but with WOLFSSL_X25519_NO_MASK_PEER defined the masking in tls.c/internal.c is compiled out and the server correctly rejects the key with ECC_PEERKEY_ERROR instead. Branch the expectation on that macro so the test passes in both configurations.
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11283
Scan targets checked: wolfcrypt-rs-bugs, 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.
Sorry, something went wrong.
| * the server masks it and proceeds, instead of aborting the handshake with | ||
| * ECC_PEERKEY_ERROR the way wc_curve25519_check_public() alone would. | ||
| */ | ||
| int test_tls13_x25519_keyshare_masks_reserved_bit(void) |
There was a problem hiding this comment.
New masking branches in the TLS 1.2 X25519 paths are untested · Missing edge-case coverage on a function the PR also changed
The PR adds the same maskedPub/&= 0x7f branch to three peer-key parsers, but only the TLS 1.3 KeyShare path (TLSX_KeyShare_ProcessX25519_ex) is exercised. The new branches in GetEcDiffieHellmanKea (src/internal.c:35558) and ImportPeerECCKey (src/internal.c:44436) have no coverage, so a wrong offset or length guard there passes CI silently.
Fix: Add TLS 1.2 tests that set the reserved high bit in the ServerKeyExchange and ClientKeyExchange X25519 public values.
Sorry, something went wrong.
Reviewers
Requested changes must be addressed to merge this pull request.
Summary(F9972)
RFC 7748 Section 5 requires that a receiver of a 32-byte X25519 u-coordinate mask (clear) the reserved high bit of the final byte rather than reject it. wolfSSL's TLS 1.2/1.3 handshake code instead rejected any peer X25519 public value with that bit set, aborting the handshake with ECC_PEERKEY_ERROR before the key was ever imported.
This affected all three places a peer's X25519 public value is parsed off the wire:
Fix
Each site now copies the received 32-byte value into a local buffer, clears bit 7 of the last byte (&= 0x7f), and passes the masked copy to both wc_curve25519_check_public() and the import call. Behavior is unchanged when WOLFSSL_X25519_NO_MASK_PEER is defined (existing opt-in strict rejection is preserved).
wc_curve25519_check_public() itself is untouched — masking is a TLS-layer concern per RFC 7748; the primitive's existing reject-on-high-bit behavior and its unit tests (tests/api/test_curve25519.c) remain correct as-is.
Testing
Added test_tls13_x25519_keyshare_masks_reserved_bit (tests/api/test_tls13.c): drives a real TLS 1.3 memio handshake, tampers the client's ClientHello key_share entry to set the reserved bit, and confirms the server now masks it and proceeds (WOLFSSL_ERROR_WANT_READ) instead of aborting with ECC_PEERKEY_ERROR.
Verified the test fails with the expected -352 (ECC_PEERKEY_ERROR) when run against the code without this fix, confirming it catches the regression.
TLS 1.2 paths (GetEcDiffieHellmanKea, ImportPeerECCKey) share the same masking logic but are not yet covered by a dedicated test.
Checklist