Repository navigation
Fix read length in ASN1_STRING_print_ex/to_UTF8. Refactor freeing logic in wolfSSL_X509_get0_extensions. Only read GENERAL_NAME types with strings in wolfSSL_X509V3_EXT_print. Properly handle NULLs inside of DNs in X509_NAME_oneline. - #11627
kareem-wolfssl wants to merge 7 commits into
Conversation
|
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The NUL escape remains ambiguous with literal \x00 input, allowing distinct names to compare equal.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Improves OpenSSL compatibility and safety for ASN.1 strings, X.509 extensions, general names, and embedded NULs in distinguished names.
Changes:
- Uses explicit ASN.1 lengths and ensures UTF-8 output termination.
- Stabilizes cached extension stacks and safely handles GENERAL_NAME variants.
- Escapes embedded NULs in one-line X.509 names and adds regression tests.
| File | Description |
|---|---|
wolfssl/openssl/x509.h |
Aligns one-line name flags with OpenSSL. |
src/x509.c |
Updates extension caching, parsing, printing, and DN rendering. |
src/ssl_asn1.c |
Fixes length-based ASN.1 copying and printing. |
tests/api/test_ossl_asn1.c |
Updates escaped-string length coverage. |
tests/api/test_ossl_x509_ext.c |
Tests extension stability and GENERAL_NAME safety. |
tests/api/test_ossl_x509_ext.h |
Registers the extension stability test. |
tests/api/test_ossl_x509_name.c |
Tests embedded-NUL distinguished names. |
tests/api/test_ossl_x509_name.h |
Registers the embedded-NUL test. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
e2265a7 to
9d8a250
Compare
|
@kareem-wolfssl, while reviewing #11446, I saw that this PR touches similar logic. I think we should merge that one first, then roll the new NUL check into X509CertEscapeName I had mistakenly posted this comment on a different PR earlier, my bad. |
There was a problem hiding this comment.
🟡 Changes recommended
Extension-generation failures can permanently cache incomplete results, while XN_FLAG_ONELINE incorrectly escapes UTF-8 bytes, causing a compatibility regression.
2 open issues
1 resolved since last review
Review effort: Balanced
| if (x509->ext_sk_full == NULL) { | ||
| x509->ext_sk_full = generateExtStack(x); | ||
| } |
There was a problem hiding this comment.
This was already considered and declined. wolfssl_dns_entry_othername_to_gn() fails for any otherName type it can’t re-encode, so returning NULL on partial failure would make X509_get0_extensions() drop every extension rather than one. Handling that needs wolfSSL_X509_set_ext() to keep the generic entry when only the SAN decoding fails, which also changes X509_get_ext(). This is out of scope for this PR.
| WOLFSSL_XN_FLAG_FN_LN | \ | ||
| WOLFSSL_XN_FLAG_FN_ALIGN) | ||
| #define WOLFSSL_XN_FLAG_ONELINE (WOLFSSL_ASN1_STRFLGS_ESC_CTRL | \ | ||
| WOLFSSL_ASN1_STRFLGS_RFC2253 | \ |
There was a problem hiding this comment.
I was unable to confirm this. OSSL 3.5.5 and 1.1.1 both appear to be using the full RFC2253 including ESC_MSB.
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11627
Scan targets checked: wolfssl-src, wolfssl-bugs
Not reviewed (no scan target covers them): tests/api/test_ossl_x509_ext.h, tests/api/test_ossl_x509_name.h
Coverage: 3 of 6 in-scope changed file(s) opened by the reviewer; not opened: tests/api/test_ossl_x509_ext.c, tests/api/test_ossl_x509_name.c, wolfssl/openssl/x509.h
Fenrir result: Approved ✅
No new issues found in the changed files.
Advisory only — this automated result does not count as a GitHub approval.
Review tier: Lite
… in wolfSSL_X509_set_ext. Thanks to Anthropic for the report.
…ing the extension stack unless it is NULL. Allow non critical extensions and extensions with zero-length OCTET STRING to match the core DER parser behavior. Thanks to Anthropic for the report.
Thanks to Anthropic for the report.
Thanks to Anthropic for the report.
…NAME_ENTRY_get_object.
4968ee5 to
8c2190d
Compare
Description
Partially fixes zd#22448
Testing
Built in + added tests, reproducers
Checklist