From 032569996c11a5ba7ede8e819262de892078ca5a Mon Sep 17 00:00:00 2001 From: Kareem Date: Thu, 30 Jul 2026 15:46:40 -0700 Subject: [PATCH 1/3] Remove carry over of old buffers/sizes in EVP and X509 ext functions. Thanks to Kushal Khemka & Mayank Jangid (OpenSec Intelligence) for the report. --- certs/test/catalog.txt | 6 ++ certs/test/cert-ext-oid-collide.der | Bin 0 -> 584 bytes certs/test/include.am | 1 + src/x509.c | 12 ++- tests/api/test_evp_pkey.c | 123 ++++++++++++++++++++++++++++ tests/api/test_evp_pkey.h | 4 +- tests/api/test_ossl_x509_ext.c | 30 +++++++ tests/api/test_ossl_x509_ext.h | 5 +- wolfcrypt/src/evp.c | 27 +++--- 9 files changed, 184 insertions(+), 24 deletions(-) create mode 100644 certs/test/cert-ext-oid-collide.der diff --git a/certs/test/catalog.txt b/certs/test/catalog.txt index 9fc982697a9..3b8390860ff 100644 --- a/certs/test/catalog.txt +++ b/certs/test/catalog.txt @@ -12,3 +12,9 @@ digsigku.pem: set. cert-ext-joi.pem: Simple certificate that includes OIDs for JurisdictionOfIncorporation +cert-ext-oid-collide.der: + 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. diff --git a/certs/test/cert-ext-oid-collide.der b/certs/test/cert-ext-oid-collide.der new file mode 100644 index 0000000000000000000000000000000000000000..3f15fda7a28ad72d7485edf48419d9e027d80419 GIT binary patch literal 584 zcmXqLVsbHPVtloLnTe5!Nkn+9_Gx8-*&gz7zD)bSeBe)8HCxqyi;Y98&EuRc3p0~J z;|xP?15P&PP!={}rqEzRVFN)Bhl7VJJh3<JD>0NX5Ce%a^YE9Km+OJl78mE}CFkcF zDj3MIaRQyu_MMlJlTl1CEit*I*a1m}ft)z6k(q&+p`nqXiHVU#6p(9aU~FIxW9)N1v2%jq(*r(_=dIPWn6XRX+DlHYhLw8a zT06V;8#lW>p342C++nM=v#T+GVsMSQ%fZus8cw}%ezkltFfI&u*jQe)Zu4YeVPs-v zL5@yl4+aBQCIwruExC@s8+ hdv;glwUsx`{$4)mm-C9fnX~iA*Vu@ZHS?}M1OVgMtUCYz literal 0 HcmV?d00001 diff --git a/certs/test/include.am b/certs/test/include.am index e5b06eebf58..d41fb467846 100644 --- a/certs/test/include.am +++ b/certs/test/include.am @@ -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 \ diff --git a/src/x509.c b/src/x509.c index 839f4317e8d..1dd2657cb20 100644 --- a/src/x509.c +++ b/src/x509.c @@ -1115,14 +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) { - XFREE((byte*)ext->obj->obj, NULL, DYNAMIC_TYPE_ASN1); - } + XFREE((byte*)ext->obj->obj, NULL, DYNAMIC_TYPE_ASN1); ext->obj->obj = tmp; #else tmp = (byte*)XREALLOC((byte*)ext->obj->obj, objSz, NULL, diff --git a/tests/api/test_evp_pkey.c b/tests/api/test_evp_pkey.c index 6fbfb281c2a..05f99e05796 100644 --- a/tests/api/test_evp_pkey.c +++ b/tests/api/test_evp_pkey.c @@ -3112,3 +3112,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(); +} diff --git a/tests/api/test_evp_pkey.h b/tests/api/test_evp_pkey.h index 60647ac0490..9d1527ecae7 100644 --- a/tests/api/test_evp_pkey.h +++ b/tests/api/test_evp_pkey.h @@ -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), \ @@ -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 */ diff --git a/tests/api/test_ossl_x509_ext.c b/tests/api/test_ossl_x509_ext.c index 18849b4fd69..401901b9bd1 100644 --- a/tests/api/test_ossl_x509_ext.c +++ b/tests/api/test_ossl_x509_ext.c @@ -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(); +} diff --git a/tests/api/test_ossl_x509_ext.h b/tests/api/test_ossl_x509_ext.h index 13ab76484c5..44484a4f3f1 100644 --- a/tests/api/test_ossl_x509_ext.h +++ b/tests/api/test_ossl_x509_ext.h @@ -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), \ @@ -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 */ diff --git a/wolfcrypt/src/evp.c b/wolfcrypt/src/evp.c index ced8033430f..d976716872f 100644 --- a/wolfcrypt/src/evp.c +++ b/wolfcrypt/src/evp.c @@ -9282,7 +9282,9 @@ 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); + /* 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. */ XFREE(pkey->pkey.ptr, pkey->heap, DYNAMIC_TYPE_DER); pkey->pkey.ptr = NULL; } @@ -9297,6 +9299,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); @@ -9840,25 +9845,17 @@ static int ECC_populate_EVP_PKEY(WOLFSSL_EVP_PKEY* pkey, WOLFSSL_EC_KEY *key) } else if (ecc->type == ECC_PUBLICKEY) { 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; } } } From b8acee2a0ae247b43a81316d2d9696beacb0696d Mon Sep 17 00:00:00 2001 From: Kareem Date: Mon, 3 Aug 2026 11:35:48 -0700 Subject: [PATCH 2/3] Code review feedback --- src/x509.c | 4 +++- wolfcrypt/src/evp.c | 6 ++++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/src/x509.c b/src/x509.c index 1dd2657cb20..2dce680f895 100644 --- a/src/x509.c +++ b/src/x509.c @@ -1120,7 +1120,9 @@ WOLFSSL_X509_EXTENSION* wolfSSL_X509_set_ext(WOLFSSL_X509* x509, int loc) * 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); - XFREE((byte*)ext->obj->obj, NULL, DYNAMIC_TYPE_ASN1); + if (ext->obj->obj != NULL) { + XFREE((byte*)ext->obj->obj, NULL, DYNAMIC_TYPE_ASN1); + } ext->obj->obj = tmp; #else tmp = (byte*)XREALLOC((byte*)ext->obj->obj, objSz, NULL, diff --git a/wolfcrypt/src/evp.c b/wolfcrypt/src/evp.c index d976716872f..685ef66ba0c 100644 --- a/wolfcrypt/src/evp.c +++ b/wolfcrypt/src/evp.c @@ -9285,8 +9285,10 @@ static int PopulateRSAEvpPkeyDer(WOLFSSL_EVP_PKEY *pkey) /* 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. */ - XFREE(pkey->pkey.ptr, pkey->heap, DYNAMIC_TYPE_DER); - pkey->pkey.ptr = NULL; + 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, From 900c5ab541dd39c02f63fe8c201d857c2c51058e Mon Sep 17 00:00:00 2001 From: Kareem Date: Mon, 3 Aug 2026 11:46:33 -0700 Subject: [PATCH 3/3] Fix missing include in EVP test files. --- tests/api/test_evp.c | 1 + tests/api/test_evp_pkey.c | 1 + 2 files changed, 2 insertions(+) diff --git a/tests/api/test_evp.c b/tests/api/test_evp.c index 354d0566600..6ee7e61fba4 100644 --- a/tests/api/test_evp.c +++ b/tests/api/test_evp.c @@ -24,6 +24,7 @@ #include #include +#include #include /* Test functions for base64 encode/decode */ diff --git a/tests/api/test_evp_pkey.c b/tests/api/test_evp_pkey.c index 05f99e05796..ecae753a37b 100644 --- a/tests/api/test_evp_pkey.c +++ b/tests/api/test_evp_pkey.c @@ -30,6 +30,7 @@ #include #include +#include #include #include