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 6 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.
45803a0 to
e2265a7
Compare
… 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.
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); | ||
| } |
| WOLFSSL_XN_FLAG_FN_LN | \ | ||
| WOLFSSL_XN_FLAG_FN_ALIGN) | ||
| #define WOLFSSL_XN_FLAG_ONELINE (WOLFSSL_ASN1_STRFLGS_ESC_CTRL | \ | ||
| WOLFSSL_ASN1_STRFLGS_RFC2253 | \ |
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
Description
Partially fixes zd#22448
Testing
Built in + added tests, reproducers
Checklist