Skip to content

evp: fix EVP_PKEY_cmp for EC keys after DER deserialization#9987

Open
MarkAtwood wants to merge 5 commits intowolfSSL:masterfrom
MarkAtwood:fix/evp-pkey-cmp-after-der-roundtrip
Open

evp: fix EVP_PKEY_cmp for EC keys after DER deserialization#9987
MarkAtwood wants to merge 5 commits intowolfSSL:masterfrom
MarkAtwood:fix/evp-pkey-cmp-after-der-roundtrip

Conversation

@MarkAtwood
Copy link

Summary

wolfSSL_EVP_PKEY_cmp returns "not equal" for EC keys that are identical but one was deserialized from DER. This breaks the OpenSSL API contract that EVP_PKEY_cmp compares key material regardless of how the EVP_PKEY objects were constructed.

Reproducer

// Generate an EC P-256 key
EVP_PKEY_CTX *ctx = EVP_PKEY_CTX_new_id(EVP_PKEY_EC, NULL);
EVP_PKEY_keygen_init(ctx);
EVP_PKEY_CTX_set_ec_paramgen_curve_nid(ctx, NID_X9_62_prime256v1);
EVP_PKEY *original = NULL;
EVP_PKEY_keygen(ctx, &original);

// Serialize to RFC 5915 DER (ECPrivateKey without optional public key)
EC_KEY *ec = EVP_PKEY_get0_EC_KEY(original);
unsigned char *der = NULL;
int der_len = i2d_ECPrivateKey(ec, &der);

// Deserialize back
const unsigned char *p = der;
EC_KEY *ec2 = d2i_ECPrivateKey(NULL, &p, der_len);
EVP_PKEY *deserialized = EVP_PKEY_new();
EVP_PKEY_assign_EC_KEY(deserialized, ec2);

// Compare — returns -1 (not equal) instead of 0 (equal)
int result = wolfSSL_EVP_PKEY_cmp(original, deserialized);
// Expected: 0 (match), Actual: -1 (no match)

Root Cause

Two issues compound:

1. inSet not checked before accessing ecc->internal

After d2i_PrivateKey or d2i_ECPrivateKey, the external WOLFSSL_EC_KEY fields (BIGNUMs for private key, WOLFSSL_EC_POINT for public key) are populated, but the internal wolfCrypt ecc_key struct may not be synced. The inSet flag tracks this. wolfSSL_EVP_PKEY_cmp accessed ecc->internal without checking inSet or calling SetECKeyInternal to sync.

2. Private-only keys have empty pubkey point

When wc_EccPrivateKeyDecode parses an RFC 5915 ECPrivateKey that omits the optional public key field (which is valid per the RFC), it calls wc_ecc_import_private_key_ex with pub=NULL, pubSz=0. This sets ecc_key.type = ECC_PRIVATEKEY_ONLY and leaves ecc_key.pubkey as all zeros. The comparison then compared a populated pubkey against zeros → "not equal".

Fix

In the WC_EVP_PKEY_EC case of wolfSSL_EVP_PKEY_cmp:

  1. Call SetECKeyInternal() if inSet == 0 to sync external → internal representation
  2. If ecc_key.type == ECC_PRIVATEKEY_ONLY, call wc_ecc_make_pub() to derive the public key from the private key before comparing

This ensures the comparison works regardless of how the key was constructed — generated, imported with public key, or imported private-only.

Test Plan

  • Generate EC key, serialize to PKCS#8, deserialize, EVP_PKEY_cmp returns match
  • Generate EC key, serialize to RFC 5915 (without optional public key), deserialize, EVP_PKEY_cmp returns match
  • Two different EC keys still return "not equal"
  • RSA key comparison still works (unchanged code path)
  • Run existing wolfSSL test suite

🤖 Generated with Claude Code

MarkAtwood and others added 5 commits March 16, 2026 11:13
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>
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>
Copilot AI review requested due to automatic review settings March 16, 2026 18:48
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_key internal state (inSet) before comparing in wolfSSL_EVP_PKEY_cmp.
  • For private-only EC imports, derive the public key before point comparison.
  • Harden wc_ecc_import_point_der_ex by 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);
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants