Validate X25519 public key length before reading input buffer - #457
Open
yosuke-wolfssl wants to merge 1 commit into
Open
Validate X25519 public key length before reading input buffer#457yosuke-wolfssl wants to merge 1 commit into
yosuke-wolfssl wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request fixes a reachable out-of-bounds read when importing X25519 public keys by validating the input length before any dereference/copy of the caller-provided buffer, and adds a regression test that would previously trip ASan.
Changes:
- Add an early guard in
wp_x25519_import_public()to rejectNULLinput or a non-32-byte public key length before readingin[31]or copying 32 bytes. - Add a new unit test that exercises both X25519 public-key import entry points with a short (heap) buffer to ensure the over-read cannot occur.
- Register the new test in the unit test declarations list.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/wp_ecx_kmgmt.c |
Adds pre-read length validation in X25519 public key import to prevent OOB reads. |
test/test_ecx.c |
Adds ASan-visible regression test for undersized X25519 public key import via both OpenSSL entry points. |
test/unit.c |
Registers the new ECX test in the unit test table. |
test/unit.h |
Declares the new test function prototype. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #457
Scan targets checked: wolfprovider-bugs, wolfprovider-src
Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
ColtonWilley
approved these changes
Aug 5, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
wp_x25519_import_public()dereferencesin[CURVE25519_KEYSIZE - 1]and, when that byte's top bit is set, does a 32-byteXMEMCPY(data, in, CURVE25519_KEYSIZE)— all beforeinLenis ever examined. The length is only validated later, insidewc_curve25519_import_public_ex().Neither caller bounds the length first:
wp_ecx_import()takespubData/lenstraight fromOSSL_PKEY_PARAM_PUB_KEY, andwp_ecx_set_params()does the same forOSSL_PKEY_PARAM_ENCODED_PUBLIC_KEY. Any X25519 public key shorter than 32 bytes causes a read of up to 31 bytes past the caller's buffer, reachable fromEVP_PKEY_fromdata()orEVP_PKEY_set1_encoded_public_key().Closes f-7489.
Fix (
src/wp_ecx_kmgmt.c)Reject a bad length before
inis touched:BAD_FUNC_ARGis what the wolfCrypt imports already return for this condition, so callers see no new error class. X25519 is the only affected type —wc_curve448_import_public_ex(),wc_ed25519_import_public_ex()andwc_ed448_import_public_ex()all validate length before reading, and the Ed25519/Ed448 wrappers delegate immediately.Test harness (
test/test_ecx.c)test_ecx_import_short_pubdrives a 4-byte heap buffer (filled0xffso the top-bit branch is taken) through both entry points:EVP_PKEY_fromdata()withOSSL_PKEY_PARAM_PUB_KEY, andEVP_PKEY_set1_encoded_public_key(). Modelled on the neighbouringtest_ecx_import_zero_priv. The heap allocation is what lets ASan see the over-read; the Sanitizers workflow runsmake testunder ASan+UBSan with-fno-sanitize-recover=allon every PR.Verification
heap-buffer-overflowREAD atwp_x25519_import_publicviawp_ecx_import←EVP_PKEY_fromdata.