Remove carry over of old buffers/sizes in EVP and X509 ext functions. - #11021
Remove carry over of old buffers/sizes in EVP and X509 ext functions.#11021kareem-wolfssl wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes issues where cached buffers/lengths from prior operations could be carried over in EVP_PKEY DER caching and X509 extension OID handling, potentially leading to incorrect sizing/overreads. It also adds targeted regression tests and a new certificate fixture to cover the reported edge cases (zd#22223).
Changes:
- EVP: stop carrying forward prior cached DER data/length when repopulating RSA/ECC EVP_PKEY DER buffers.
- X509 ext: stop carrying forward the canonical OID buffer when the extension’s OID in the certificate is shorter than the mapped NID’s canonical OID.
- Add regression tests plus a new
cert-ext-oid-collide.derfixture and catalog entry.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| wolfcrypt/src/evp.c | Clears/stops reusing stale cached DER buffers/sizes when repopulating EVP_PKEY DER for RSA/ECC. |
| src/x509.c | Avoids copying/reusing old ASN1_OBJECT OID buffers when rebuilding extension OIDs from the certificate. |
| tests/api/test_ossl_x509_ext.h | Registers new X509 extension regression test. |
| tests/api/test_ossl_x509_ext.c | Adds regression test ensuring returned extension OIDs reflect the certificate’s OID encoding/size. |
| tests/api/test_evp_pkey.h | Registers new EVP_PKEY DER “shrink” regression test. |
| tests/api/test_evp_pkey.c | Adds regression coverage for DER cache resizing across RSA/PKCS#8/ECC public-only scenarios. |
| certs/test/include.am | Distributes the new DER fixture in test artifacts. |
| certs/test/catalog.txt | Documents the new fixture and its purpose. |
Comments suppressed due to low confidence (1)
wolfcrypt/src/evp.c:9859
- XFREE(pkey->pkey.ptr, ...) is unconditionally called here, unlike other branches in ECC_populate_EVP_PKEY which guard the free with a NULL check. Adding the guard improves portability in case the underlying allocator does not accept NULL frees.
else {
XFREE(pkey->pkey.ptr, pkey->heap, DYNAMIC_TYPE_OPENSSL);
pkey->pkey.ptr = (char*)derBuf;
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Thanks to Kushal Khemka & Mayank Jangid (OpenSec Intelligence) for the report.
dgarske
left a comment
dgarske
left a comment
There was a problem hiding this comment.
Skoll Code Review
Scan type: reviewOverall recommendation: COMMENT
Findings: 7 total — 2 posted, 5 skipped
2 finding(s) posted as inline comments (see file-level comments below)
Posted findings
- [Medium] ECC_populate_EVP_PKEY now keeps the previous key's cached DER on the serialization error path —
wolfcrypt/src/evp.c:9848-9868 - [Low] New test certificate has no generation recipe alongside the other cert-ext- certs* —
certs/test/catalog.txt:15-20
Skipped findings
- [Medium]
New EC test arm is gated on the wrong curve macros (P-256 arm guarded by ECC_MIN_KEY_SZ = 521 / HAVE_ECC256) - [Medium]
X509 OID-collision test asserts a value that also holds when the collision does not occur - [Low]
Unused pem.h include added to tests/api/test_evp.c - [Low]
Inconsistent NULL-check-before-XFREE across the three sites touched by this PR - [Info]
Macro continuation formatting in TEST_OSSL_X509_EXT_DECLS
Review generated by Skoll
| @@ -9840,25 +9847,17 @@ static int ECC_populate_EVP_PKEY(WOLFSSL_EVP_PKEY* pkey, WOLFSSL_EC_KEY *key) | |||
| } | |||
| else if (ecc->type == ECC_PUBLICKEY) { | |||
There was a problem hiding this comment.
🟠 [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.
| set. | ||
| cert-ext-joi.pem: | ||
| Simple certificate that includes OIDs for JurisdictionOfIncorporation | ||
| cert-ext-oid-collide.der: |
There was a problem hiding this comment.
🔵 [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.
Description
Fixes zd#22223
Testing
Built in tests, added tests, provided reproducer
Checklist