Fix X509 extension handling (X509V3_ADD_*, extKeyUsage, EC point form) and cipher suite test - #11375
Open
julek-wolfssl wants to merge 11 commits into
Open
Fix X509 extension handling (X509V3_ADD_*, extKeyUsage, EC point form) and cipher suite test#11375julek-wolfssl wants to merge 11 commits into
julek-wolfssl wants to merge 11 commits into
Conversation
cipher_names[] only holds the IANA suite name when error strings are built in, so lean builds could not match "TLS_AES_128_GCM_SHA256". Gate the test on BUILD_TLS_AES_128_GCM_SHA256 and fall back to the wolfSSL suite name. Also build the basicConstraints ASN1_OBJECT with OBJ_nid2obj() so it carries a real OID encoding, not just a hand-set type.
Removing an extension left state behind: the freed altName list was still referenced by the altNamesNext retrieval hint (use after free through X509_get_next_altname()), and the cached subject key ID string, the encoded extended key usage and the per-extension set flags kept the removed extension observable. X509V3_ADD_SILENT only suppresses error reporting in OpenSSL, so X509V3_ADD_DEFAULT on an existing extension fails either way. REPLACE and REPLACE_EXISTING now encode the new extension before dropping the old one, so a failure no longer destroys the existing extension.
OpenSSL saves the encoding form on the key so that a later i2o_ECPublicKey() re-emits it. Without this a compressed point silently round-tripped back as uncompressed.
wolfSSL_X509V3_EXT_i2d() encodes extKeyUsage as a stack of ASN1_OBJECTs, which wolfSSL_X509_add_ext() could not use at all. Map each KeyPurposeId OID to its EXTKEYUSE_* bit and store the list as DER, the layout a parsed certificate carries, so X509_get_ext_d2i() and X509_get_extended_key_usage() both read it back. OIDs wolfSSL has no bit for stay in the list, matching the certificate parser. Entries that are not a single OBJECT IDENTIFIER are rejected before anything is stored.
wolfssl_x509_ext_is_set() answered "absent" for every NID it does not know, so DELETE and REPLACE_EXISTING failed with a reason indistinguishable from a real absence, and DEFAULT and KEEP_EXISTING could not detect a duplicate. It now reports "unknown" separately and only X509V3_ADD_APPEND, which is defined as not checking, is attempted for such a NID. X509V3_ADD_SILENT only gated WOLFSSL_MSG() calls that are compiled out without DEBUG_WOLFSSL, and this API pushes nothing to an error queue. Drop the three dead conditionals and document the flag as accepted but inert.
wolfSSL_X509V3_EXT_i2d() builds the authorityKeyIdentifier object from the caller's issuer name when no key ID is given, and wolfSSL_X509_add_ext() dispatches on that object rather than on the NID that was asked for. A REPLACE therefore removed the certificate's authority key identifier and then failed, or wrote into a different extension's storage. Check the encoded NID before anything is removed.
wolfSSL_X509V3_EXT_i2d() copies the object stack without inspecting it, so a REPLACE dropped the certificate's extKeyUsage and only then discovered that the new stack could not be stored. Split the per-entry check out of wolfssl_x509_add_ext_key_usage_sk() and run it before the removal.
2 tasks
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Two correctness issues were found in the new code/tests (EC point conversion-form handling in wolfSSL_EC_KEY_oct2key() and a length/reporting bug in the new SAN test helper).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR extends wolfSSL’s OpenSSL-compat layer and X.509 extension machinery by adding X509_add1_ext_i2d() support (including X509V3_ADD_* operation flags), improving handling of extKeyUsage and certain extension removal semantics, and adding EC_KEY_oct2key() with a corresponding cipher-suite and API test coverage.
Changes:
- Add OpenSSL-compat
X509V3_ADD_*flags and wire upX509_add1_ext_i2d()in both public and compat headers. - Extend X.509 extension add/remove behavior (notably
extKeyUsagestack support and typed-state clearing on delete/replace). - Add
EC_KEY_oct2key()(including conversion-form behavior) and new/expanded API tests for the added compatibility macros and behaviors.
File summaries
| File | Description |
|---|---|
| wolfssl/ssl.h | Exposes wolfSSL_X509_add1_ext_i2d() in the OpenSSL-extra/all API surface. |
| wolfssl/openssl/x509v3.h | Introduces WOLFSSL_X509V3_ADD_* and maps OpenSSL X509V3_ADD_* flags for compat callers. |
| wolfssl/openssl/ssl.h | Adds OpenSSL-compat macros for SSL_set_ciphersuites, X509_add1_ext_i2d, and sk_GENERAL_NAME_new_null. |
| wolfssl/openssl/ec.h | Declares wolfSSL_EC_KEY_oct2key() and maps EC_KEY_oct2key for OpenSSL compat. |
| tests/api.c | Adds new tests for SSL_set_ciphersuites, sk_GENERAL_NAME_new_null, X509_add1_ext_i2d (+ flags/edge cases), and EC_KEY_oct2key. |
| src/x509.c | Implements wolfSSL_X509_add1_ext_i2d() and extends internal typed extension storage/removal, including extKeyUsage stack ingestion. |
| src/pk_ec.c | Implements wolfSSL_EC_KEY_oct2key() with conversion-form tracking. |
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+4809
to
+4813
| if (ret == 1) { | ||
| /* SEC 1: 0x02/0x03 compressed, 0x04 uncompressed. Clearing the low | ||
| * bit turns the leading byte into the conversion form. */ | ||
| wolfSSL_EC_KEY_set_conv_form(key, buf[0] & ~0x01); | ||
| } |
Comment on lines
+23138
to
+23140
| *out = dnsBuf; | ||
| *outLen = gn->d.dNSName->length; | ||
| } |
|
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.
Continuation of #10520