Skip to content

Remove carry over of old buffers/sizes in EVP and X509 ext functions. #11021

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
kareem-wolfssl wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Remove carry over of old buffers/sizes in EVP and X509 ext functions. #11021

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0325699
Remove carry over of old buffers/sizes in EVP and X509 ext functions.
kareem-wolfssl Jul 30, 2026
b8acee2
Code review feedback
kareem-wolfssl Aug 3, 2026
900c5ab
Fix missing include in EVP test files.
kareem-wolfssl Aug 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions certs/test/catalog.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,3 +12,9 @@ digsigku.pem:
set.
cert-ext-joi.pem:
Simple certificate that includes OIDs for JurisdictionOfIncorporation
cert-ext-oid-collide.der:

@dgarske dgarske Aug 5, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🔵 [Low] New test certificate has no generation recipe alongside the other cert-ext- certs*

Every other certs/test/cert-ext-* artifact is produced by certs/test/gen-ext-certs.sh from a committed .cfg. cert-ext-oid-collide.der is committed as an opaque blob with no recipe; catalog.txt documents what it is (a 4-byte OID that hashes to the prime256v1 NID, invalid signature, parse-only) but not how to rebuild it. If wc_oid_sum() ever changes, no one can regenerate a colliding OID without redoing the analysis. (The certificate itself is fine for the test: wolfSSL_X509_load_certificate_file() parses with NO_VERIFY, so neither the bogus signature nor the 2029 notAfter matters.)

Fix: Add a generation recipe or document how the colliding OID was derived so the artifact is reproducible.

Certificate carrying a non-critical extension whose 4-byte OID hashes to the
same wc_oid_sum() as prime256v1, so it maps to a NID whose canonical OID is
longer. Used to test that wolfSSL_X509_set_ext() sizes the ASN1_OBJECT
buffer from the certificate's OID rather than the mapped one. The signature
is not valid; the certificate is only ever parsed, never verified.
Binary file added certs/test/cert-ext-oid-collide.der
View file
Open in desktop
Binary file not shown.
1 change: 1 addition & 0 deletions certs/test/include.am
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ EXTRA_DIST += \
certs/test/cert-ext-ndir-exc.cfg \
certs/test/cert-ext-ndir-exc.der \
certs/test/cert-ext-ndir-exc.pem \
certs/test/cert-ext-oid-collide.der \
certs/test/gen-ext-certs.sh \
certs/test/server-duplicate-policy.pem \
certs/test/cert-ext-joi.der \
Expand Down
10 changes: 5 additions & 5 deletions src/x509.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1115,12 +1115,12 @@ WOLFSSL_X509_EXTENSION* wolfSSL_X509_set_ext(WOLFSSL_X509* x509, int loc)
(ext->obj->obj == NULL)) {
byte* tmp;
#ifdef WOLFSSL_NO_REALLOC
/* Don't carry the old OID over: the buffer is rewritten in full
* from oidBuf below, and ext->obj->objSz is the length of the
* canonical OID that OBJ_nid2obj() produced for the mapped NID,
* which may be longer than the OID in the certificate. */
tmp = (byte*)XMALLOC(objSz, NULL, DYNAMIC_TYPE_ASN1);
if (tmp != NULL && ext->obj->obj != NULL) {
XMEMCPY(tmp, ext->obj->obj, ext->obj->objSz);
XFREE((byte*)ext->obj->obj, NULL, DYNAMIC_TYPE_ASN1);
}
else if (tmp == NULL) {
if (ext->obj->obj != NULL) {
XFREE((byte*)ext->obj->obj, NULL, DYNAMIC_TYPE_ASN1);
}
ext->obj->obj = tmp;
Comment thread
kareem-wolfssl marked this conversation as resolved.
Expand Down
1 change: 1 addition & 0 deletions tests/api/test_evp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
#include <wolfssl/wolfcrypt/error-crypt.h>

#include <wolfssl/openssl/evp.h>
#include <wolfssl/openssl/pem.h>
#include <tests/api/test_evp.h>

/* Test functions for base64 encode/decode */
Expand Down
124 changes: 124 additions & 0 deletions tests/api/test_evp_pkey.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@

#include <wolfssl/openssl/evp.h>
#include <wolfssl/openssl/kdf.h>
#include <wolfssl/openssl/pem.h>
#include <tests/api/api.h>
#include <tests/api/test_evp_pkey.h>

Expand Down Expand Up @@ -3112,3 +3113,126 @@ int test_wolfSSL_EVP_PKEY_encoded_public_key(void)
return EXPECT_RESULT();
}



int test_wolfSSL_EVP_PKEY_populate_shrink(void)
{
EXPECT_DECLS;
#if defined(OPENSSL_EXTRA) && defined(WOLFSSL_KEY_TO_DER) && \
!defined(NO_FILESYSTEM) && !defined(NO_BIO)
#ifndef NO_RSA
{
/* 2048-bit key first, then a 1024-bit one into the same EVP_PKEY. */
EVP_PKEY* pkey = NULL;
RSA* big = NULL;
RSA* small = NULL;
BIO* bio = NULL;
int bigSz = 0;

ExpectNotNull(bio = BIO_new_file("./certs/server-key.pem", "rb"));
ExpectNotNull(big = PEM_read_bio_RSAPrivateKey(bio, NULL, NULL, NULL));
BIO_free(bio);
bio = NULL;
ExpectNotNull(bio = BIO_new_file("./certs/1024/server-key.pem", "rb"));
ExpectNotNull(small = PEM_read_bio_RSAPrivateKey(bio, NULL, NULL,
NULL));
BIO_free(bio);
bio = NULL;

ExpectNotNull(pkey = EVP_PKEY_new());
ExpectIntEQ(EVP_PKEY_set1_RSA(pkey, big), 1);
if (pkey != NULL)
bigSz = pkey->pkey_sz;
ExpectIntGT(bigSz, 0);

ExpectIntEQ(EVP_PKEY_set1_RSA(pkey, small), 1);
/* The cached DER must now describe the smaller key. */
if (pkey != NULL) {
ExpectIntGT(pkey->pkey_sz, 0);
ExpectIntLT(pkey->pkey_sz, bigSz);
}

RSA_free(big);
RSA_free(small);
EVP_PKEY_free(pkey);
}
#if defined(WOLFSSL_KEY_GEN) && !defined(NO_TLS) && !defined(NO_WOLFSSL_CLIENT)
{
/* A PKCS#8-wrapped key: pkey_sz includes the wrapper, while
* PopulateRSAEvpPkeyDer() sizes its buffer from the unwrapped key. */
EVP_PKEY* pkey = NULL;
WOLFSSL_CTX* ctx = NULL;
BIO* bio = NULL;

ExpectNotNull(bio = BIO_new_file("./certs/server-keyPkcs8.pem", "rb"));
ExpectNotNull(pkey = PEM_read_bio_PrivateKey(bio, NULL, NULL, NULL));
ExpectNotNull(ctx = wolfSSL_CTX_new(wolfSSLv23_client_method()));
ExpectIntEQ(wolfSSL_CTX_use_PrivateKey(ctx, pkey), 1);
ExpectIntGT(pkey == NULL ? 0 : pkey->pkey_sz, 0);

wolfSSL_CTX_free(ctx);
EVP_PKEY_free(pkey);
BIO_free(bio);
}
#endif /* WOLFSSL_KEY_GEN && !NO_TLS && !NO_WOLFSSL_CLIENT */
#endif /* !NO_RSA */
#if defined(HAVE_ECC) && \
(defined(HAVE_ECC521) || defined(HAVE_ALL_CURVES)) && \
ECC_MIN_KEY_SZ <= 521 && \
(defined(HAVE_ECC256) || defined(HAVE_ALL_CURVES))
{
/* A public-only EC key takes the ECC_PUBLICKEY branch. The P-521
* private DER is longer than a P-256 SubjectPublicKeyInfo. */
EVP_PKEY* pkey = NULL;
EC_KEY* big = NULL;
EC_KEY* small = NULL;
unsigned char* der = NULL;
unsigned char* p;
const unsigned char* q;
int derSz = 0;
int bigSz = 0;

ExpectNotNull(pkey = EVP_PKEY_new());
ExpectNotNull(big = EC_KEY_new_by_curve_name(NID_secp521r1));
ExpectIntEQ(EC_KEY_generate_key(big), 1);
ExpectIntEQ(EVP_PKEY_set1_EC_KEY(pkey, big), 1);
if (pkey != NULL)
bigSz = pkey->pkey_sz;
ExpectIntGT(bigSz, 0);

/* Build a public-only P-256 key by round-tripping the point. */
ExpectNotNull(small = EC_KEY_new_by_curve_name(NID_X9_62_prime256v1));
ExpectIntEQ(EC_KEY_generate_key(small), 1);
ExpectIntGT((derSz = wolfSSL_i2o_ECPublicKey(small, NULL)), 0);
ExpectNotNull(der = (unsigned char*)XMALLOC((size_t)derSz, NULL,
DYNAMIC_TYPE_TMP_BUFFER));
if (der != NULL) {
p = der;
ExpectIntEQ(wolfSSL_i2o_ECPublicKey(small, &p), derSz);
}
EC_KEY_free(small);
small = NULL;
ExpectNotNull(small = EC_KEY_new_by_curve_name(NID_X9_62_prime256v1));
if (der != NULL) {
q = der;
ExpectNotNull(wolfSSL_o2i_ECPublicKey(&small, &q, derSz));
}
/* Push the decoded point into the internal key so that it is typed
* ECC_PUBLICKEY, which is the branch under test. */
ExpectIntEQ(EC_KEY_check_key(small), 1);

ExpectIntEQ(EVP_PKEY_set1_EC_KEY(pkey, small), 1);
if (pkey != NULL) {
ExpectIntGT(pkey->pkey_sz, 0);
ExpectIntLT(pkey->pkey_sz, bigSz);
}

XFREE(der, NULL, DYNAMIC_TYPE_TMP_BUFFER);
EC_KEY_free(big);
EC_KEY_free(small);
EVP_PKEY_free(pkey);
}
#endif /* HAVE_ECC && P-521 && P-256 */
#endif /* OPENSSL_EXTRA && WOLFSSL_KEY_TO_DER && !NO_FILESYSTEM && !NO_BIO */
return EXPECT_RESULT();
}
4 changes: 3 additions & 1 deletion tests/api/test_evp_pkey.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ int test_wolfSSL_EVP_PKEY_ed448(void);
int test_wolfSSL_EVP_PKEY_x25519(void);
int test_wolfSSL_EVP_PKEY_x448(void);
int test_wolfSSL_EVP_PKEY_encoded_public_key(void);
int test_wolfSSL_EVP_PKEY_populate_shrink(void);

#define TEST_EVP_PKEY_DECLS \
TEST_DECL_GROUP("evp_pkey", test_wolfSSL_EVP_PKEY_CTX_new_id), \
Expand Down Expand Up @@ -116,6 +117,7 @@ int test_wolfSSL_EVP_PKEY_encoded_public_key(void);
TEST_DECL_GROUP("evp_pkey", test_wolfSSL_EVP_PKEY_ed448), \
TEST_DECL_GROUP("evp_pkey", test_wolfSSL_EVP_PKEY_x25519), \
TEST_DECL_GROUP("evp_pkey", test_wolfSSL_EVP_PKEY_x448), \
TEST_DECL_GROUP("evp_pkey", test_wolfSSL_EVP_PKEY_encoded_public_key)
TEST_DECL_GROUP("evp_pkey", test_wolfSSL_EVP_PKEY_encoded_public_key), \
TEST_DECL_GROUP("evp_pkey", test_wolfSSL_EVP_PKEY_populate_shrink)

#endif /* WOLFCRYPT_TEST_EVP_PKEY_H */
30 changes: 30 additions & 0 deletions tests/api/test_ossl_x509_ext.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2639,3 +2639,33 @@ int test_wolfSSL_NAME_CONSTRAINTS_excluded(void)
* !IGNORE_NAME_CONSTRAINTS */
return EXPECT_RESULT();
}

int test_wolfSSL_X509_set_ext_oid_collision(void)
{
EXPECT_DECLS;
#if defined(OPENSSL_EXTRA) && !defined(NO_FILESYSTEM) && !defined(NO_CERTS) && \
defined(HAVE_ECC)
X509* x509 = NULL;
int count = 0;
int i;

ExpectNotNull(x509 = wolfSSL_X509_load_certificate_file(
"./certs/test/cert-ext-oid-collide.der", WOLFSSL_FILETYPE_ASN1));
ExpectIntGT((count = X509_get_ext_count(x509)), 0);

/* The returned object's OID has to be the one in the certificate, not
* the longer canonical OID of the NID it happens to hash to. */
for (i = 0; i < count; i++) {
X509_EXTENSION* ext = NULL;
ASN1_OBJECT* obj = NULL;

ExpectNotNull(ext = X509_get_ext(x509, i));
ExpectNotNull(obj = X509_EXTENSION_get_object(ext));
/* 4 OID content bytes plus tag and length. */
ExpectIntEQ(obj == NULL ? -1 : (int)obj->objSz, 6);
}

X509_free(x509);
#endif /* OPENSSL_EXTRA && !NO_FILESYSTEM && !NO_CERTS && HAVE_ECC */
return EXPECT_RESULT();
}
5 changes: 4 additions & 1 deletion tests/api/test_ossl_x509_ext.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ int test_wolfSSL_NAME_CONSTRAINTS_check_name(void);
int test_wolfSSL_NAME_CONSTRAINTS_manual_paths(void);
int test_wolfSSL_NAME_CONSTRAINTS_dns(void);
int test_wolfSSL_NAME_CONSTRAINTS_excluded(void);
int test_wolfSSL_X509_set_ext_oid_collision(void);

#define TEST_OSSL_X509_EXT_DECLS \
TEST_DECL_GROUP("ossl_x509_ext", test_wolfSSL_X509_get_extension_flags), \
Expand Down Expand Up @@ -96,6 +97,8 @@ int test_wolfSSL_NAME_CONSTRAINTS_excluded(void);
TEST_DECL_GROUP("ossl_x509_ext", test_wolfSSL_NAME_CONSTRAINTS_check_name),\
TEST_DECL_GROUP("ossl_x509_ext", test_wolfSSL_NAME_CONSTRAINTS_manual_paths),\
TEST_DECL_GROUP("ossl_x509_ext", test_wolfSSL_NAME_CONSTRAINTS_dns), \
TEST_DECL_GROUP("ossl_x509_ext", test_wolfSSL_NAME_CONSTRAINTS_excluded)
TEST_DECL_GROUP("ossl_x509_ext", test_wolfSSL_NAME_CONSTRAINTS_excluded), \
TEST_DECL_GROUP("ossl_x509_ext", \
test_wolfSSL_X509_set_ext_oid_collision)

#endif /* WOLFCRYPT_TEST_OSSL_X509_EXT_H */
33 changes: 16 additions & 17 deletions wolfcrypt/src/evp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9282,9 +9282,13 @@ static int PopulateRSAEvpPkeyDer(WOLFSSL_EVP_PKEY *pkey)
#ifdef WOLFSSL_NO_REALLOC
derBuf = (byte*)XMALLOC((size_t)derSz, pkey->heap, DYNAMIC_TYPE_DER);
if (derBuf != NULL) {
XMEMCPY(derBuf, pkey->pkey.ptr, (size_t)pkey->pkey_sz);
XFREE(pkey->pkey.ptr, pkey->heap, DYNAMIC_TYPE_DER);
pkey->pkey.ptr = NULL;
/* Don't carry the cached DER over: the whole buffer is re-serialized
* below, and pkey_sz describes the previous key, which may be longer
* than derSz. */
if (pkey->pkey.ptr != NULL) {
XFREE(pkey->pkey.ptr, pkey->heap, DYNAMIC_TYPE_DER);
pkey->pkey.ptr = NULL;
}
}
#else
derBuf = (byte*)XREALLOC(pkey->pkey.ptr, (size_t)derSz,
Expand All @@ -9297,6 +9301,9 @@ static int PopulateRSAEvpPkeyDer(WOLFSSL_EVP_PKEY *pkey)

/* Old pointer is invalid from this point on */
pkey->pkey.ptr = (char*)derBuf;
/* pkey_sz still describes the old buffer; clear it so an error below
* cannot leave a length that overruns the new one. */
pkey->pkey_sz = 0;

if (rsa->type == RSA_PRIVATE) {
ret = wc_RsaKeyToDer(rsa, derBuf, (word32)derSz);
Expand Down Expand Up @@ -9840,25 +9847,17 @@ static int ECC_populate_EVP_PKEY(WOLFSSL_EVP_PKEY* pkey, WOLFSSL_EC_KEY *key)
}
else if (ecc->type == ECC_PUBLICKEY) {

@dgarske dgarske Aug 5, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🟠 [Medium] ECC_populate_EVP_PKEY now keeps the previous key's cached DER on the serialization error path

The rewritten ECC_PUBLICKEY branch changes the error-path semantics. Previously, when wc_EccPublicKeyToDer() failed the code did pkey->pkey.ptr = NULL; pkey->pkey_sz = 0;, so the EVP_PKEY was left with no cached DER. After this change, on that same failure only the newly allocated derBuf is freed and the old buffer/size are left in place. wolfSSL_EVP_PKEY_set1_EC_KEY() calls clearEVPPkeyKeys() (which does not touch pkey->pkey.ptr) and installs pkey->ecc = key before calling this function, so on failure the EVP_PKEY ends up with the NEW EC key but the OLD key's DER still cached in pkey->pkey.ptr/pkey->pkey_sz. Any caller that ignores the WOLFSSL_FAILURE return (or that later reads the cached DER via wolfSSL_EVP_PKEY_get_der(), i2d_*, EVP_PKEY_cmp(), wolfSSL_CTX_use_PrivateKey()) then silently operates on a different key rather than getting an error. It is not a memory-safety issue (size still matches the retained buffer), but it is exactly the stale-carry-over class of state this PR sets out to eliminate. Note also that this block does not reset pkey->pkcs8HeaderSz (pre-existing), so a PKCS#8-wrapped key followed by a public-only EC key leaves a stale non-zero header size next to a DER that has no PKCS#8 wrapper - worth handling while the block is being rewritten.

Fix: Free and NULL pkey->pkey.ptr (and zero pkey->pkey_sz) before/when the serialization fails so a failed EVP_PKEY_set1_EC_KEY() cannot leave a DER that decodes to the previously installed key. Mirrors the pkey->pkey_sz = 0; that was added to PopulateRSAEvpPkeyDer() in this same PR.

if ((derSz = wc_EccPublicKeyDerSize(ecc, 1)) > 0) {
#ifdef WOLFSSL_NO_REALLOC
derBuf = (byte*)XMALLOC((size_t)derSz, pkey->heap, DYNAMIC_TYPE_OPENSSL);
if (derBuf != NULL) {
XMEMCPY(derBuf, pkey->pkey.ptr, (size_t)pkey->pkey_sz);
XFREE(pkey->pkey.ptr, pkey->heap, DYNAMIC_TYPE_OPENSSL);
pkey->pkey.ptr = NULL;
}
#else
derBuf = (byte*)XREALLOC(pkey->pkey.ptr, (size_t)derSz, pkey->heap,
DYNAMIC_TYPE_OPENSSL);
#endif
derBuf = (byte*)XMALLOC((size_t)derSz, pkey->heap,
DYNAMIC_TYPE_OPENSSL);
if (derBuf != NULL) {
pkey->pkey.ptr = (char*)derBuf;
if ((derSz = wc_EccPublicKeyToDer(ecc, derBuf, (word32)derSz,
1)) < 0) {
XFREE(derBuf, pkey->heap, DYNAMIC_TYPE_OPENSSL);
derBuf = NULL;
pkey->pkey.ptr = NULL;
pkey->pkey_sz = 0;
}
else {
XFREE(pkey->pkey.ptr, pkey->heap, DYNAMIC_TYPE_OPENSSL);
pkey->pkey.ptr = (char*)derBuf;
}
}
}
Expand Down
Loading