evp: fix EVP_PKEY_cmp for EC keys after DER deserialization#9987
Open
MarkAtwood wants to merge 5 commits intowolfSSL:masterfrom
Open
evp: fix EVP_PKEY_cmp for EC keys after DER deserialization#9987MarkAtwood wants to merge 5 commits intowolfSSL:masterfrom
MarkAtwood wants to merge 5 commits intowolfSSL:masterfrom
Conversation
wc_ecc_import_point_der_ex accepted a single byte 0x02 or 0x03 as a valid compressed EC point, treating the missing X coordinate bytes as zeros. This could allow ECDH with a crafted peer public key. Add length validation: compressed points must be exactly 1 + field_element_size bytes. Reject anything shorter or longer. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…byte wc_ecc_import_point_der_ex crashed (double-free/SIGABRT) when given a full-length EC point blob with an invalid first byte (0x01, 0x05, 0xFF, etc.). The function fell through to code paths that partially initialized state, then the cleanup path freed already-freed memory. Add early validation of the format byte and fix cleanup paths to prevent double-free on error. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
wc_ecc_import_point_der_ex accepts a single byte 0x02 or 0x03 as a valid compressed EC point. It treats the missing X coordinate as zero, decompresses it (producing a valid on-curve point), and wc_ecc_check_key passes. This allows ECDH key agreement with a crafted 1-byte peer public key. Add length validation for compressed points: after identifying 0x02/0x03 format byte, verify that inLen == ecc_sets[curve_idx].size + 1 using unsigned comparison to avoid underflow. Only set compressed = 1 after the length check passes, keeping state consistent on the error path. Reproducer: call EC_POINT_oct2point with a 1-byte buffer containing 0x02 for any NIST curve. Before this fix it succeeds; after, it returns ECC_BAD_ARG_E. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… length check style)
wolfSSL_EVP_PKEY_cmp returned 'not equal' for EC keys that were serialized to DER and deserialized back, even though the key material was identical. This happened because keys imported via RFC 5915 (ECPrivateKey) without the optional public key field had type ECC_PRIVATEKEY_ONLY, meaning the internal ecc_key.pubkey was not populated. The point comparison then failed against a key that did have a populated pubkey. Fix by deriving the public key from the private key via wc_ecc_make_pub() when the ecc_key type is ECC_PRIVATEKEY_ONLY before comparing. Also ensure SetECKeyInternal() is called when the internal representation is not yet synced from external BIGNUMs. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes wolfSSL_EVP_PKEY_cmp returning “not equal” for identical EC keys when one key was DER-deserialized without an embedded public key, aligning behavior with the expected OpenSSL-style key-material comparison semantics.
Changes:
- Sync EC
ecc_keyinternal state (inSet) before comparing inwolfSSL_EVP_PKEY_cmp. - For private-only EC imports, derive the public key before point comparison.
- Harden
wc_ecc_import_point_der_exby validating point type early and rejecting truncated compressed-point encodings.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| wolfcrypt/src/evp.c | Synces EC internal state and derives missing public key to make EC key comparisons succeed after DER deserialization. |
| wolfcrypt/src/ecc.c | Adds earlier input validation for EC point import and rejects invalid/truncated compressed encodings. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Comment on lines
+4025
to
+4034
| if (a->ecc->inSet == 0) { | ||
| if (SetECKeyInternal((WOLFSSL_EC_KEY*)a->ecc) != 1) { | ||
| return ret; | ||
| } | ||
| } | ||
| if (b->ecc->inSet == 0) { | ||
| if (SetECKeyInternal((WOLFSSL_EC_KEY*)b->ecc) != 1) { | ||
| return ret; | ||
| } | ||
| } |
Comment on lines
+4046
to
+4058
| * optional public key), the pubkey point will not be populated. | ||
| * Derive it from the private key so the comparison can succeed. */ | ||
| if (ecc_key_a->type == ECC_PRIVATEKEY_ONLY) { | ||
| if (wc_ecc_make_pub(ecc_key_a, NULL) != MP_OKAY) { | ||
| return ret; | ||
| } | ||
| ecc_key_a->type = ECC_PRIVATEKEY; | ||
| } | ||
| if (ecc_key_b->type == ECC_PRIVATEKEY_ONLY) { | ||
| if (wc_ecc_make_pub(ecc_key_b, NULL) != MP_OKAY) { | ||
| return ret; | ||
| } | ||
| ecc_key_b->type = ECC_PRIVATEKEY; |
Comment on lines
+9442
to
+9448
| /* validate point format byte before any memory operations */ | ||
| pointType = in[0]; | ||
| if (pointType != ECC_POINT_UNCOMP && | ||
| pointType != ECC_POINT_COMP_EVEN && | ||
| pointType != ECC_POINT_COMP_ODD) { | ||
| return ASN_PARSE_E; | ||
| } |
| compressed = 1; | ||
| } | ||
| else { | ||
| err = ECC_BAD_ARG_E; |
Comment on lines
+9442
to
9452
| /* validate point format byte before any memory operations */ | ||
| pointType = in[0]; | ||
| if (pointType != ECC_POINT_UNCOMP && | ||
| pointType != ECC_POINT_COMP_EVEN && | ||
| pointType != ECC_POINT_COMP_ODD) { | ||
| return ASN_PARSE_E; | ||
| } | ||
|
|
||
| /* clear if previously allocated */ | ||
| mp_clear(point->x); | ||
| mp_clear(point->y); |
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.
Summary
wolfSSL_EVP_PKEY_cmpreturns "not equal" for EC keys that are identical but one was deserialized from DER. This breaks the OpenSSL API contract thatEVP_PKEY_cmpcompares key material regardless of how theEVP_PKEYobjects were constructed.Reproducer
Root Cause
Two issues compound:
1.
inSetnot checked before accessingecc->internalAfter
d2i_PrivateKeyord2i_ECPrivateKey, the external WOLFSSL_EC_KEY fields (BIGNUMs for private key, WOLFSSL_EC_POINT for public key) are populated, but the internal wolfCryptecc_keystruct may not be synced. TheinSetflag tracks this.wolfSSL_EVP_PKEY_cmpaccessedecc->internalwithout checkinginSetor callingSetECKeyInternalto sync.2. Private-only keys have empty
pubkeypointWhen
wc_EccPrivateKeyDecodeparses an RFC 5915ECPrivateKeythat omits the optional public key field (which is valid per the RFC), it callswc_ecc_import_private_key_exwithpub=NULL, pubSz=0. This setsecc_key.type = ECC_PRIVATEKEY_ONLYand leavesecc_key.pubkeyas all zeros. The comparison then compared a populated pubkey against zeros → "not equal".Fix
In the
WC_EVP_PKEY_ECcase ofwolfSSL_EVP_PKEY_cmp:SetECKeyInternal()ifinSet == 0to sync external → internal representationecc_key.type == ECC_PRIVATEKEY_ONLY, callwc_ecc_make_pub()to derive the public key from the private key before comparingThis ensures the comparison works regardless of how the key was constructed — generated, imported with public key, or imported private-only.
Test Plan
EVP_PKEY_cmpreturns matchEVP_PKEY_cmpreturns match🤖 Generated with Claude Code