Fix X509_NAME_print_ex flag handling and unsafe value copying - #11395
Fix X509_NAME_print_ex flag handling and unsafe value copying#11395julek-wolfssl wants to merge 3 commits into
Conversation
XN_FLAG_RFC2253 was defined as 1, the same bit as ASN1_STRFLGS_ESC_2253, so a caller asking only for RFC 2253 escaping also got the reversed DN order. OpenVPN master now calls X509_NAME_print_ex() with XN_FLAG_SEP_CPLUS_SPC | XN_FLAG_FN_SN | ASN1_STRFLGS_ESC_2253 | ASN1_STRFLGS_UTF8_CONVERT (fix for CVE-2026-84790) and its ssl_testdriver failed against wolfSSL. - Define XN_FLAG_RFC2253 and XN_FLAG_MULTILINE as OpenSSL does. - Reverse the order only on XN_FLAG_DN_REV. - Implement ASN1_STRFLGS_ESC_2253, ASN1_STRFLGS_ESC_CTRL and ASN1_STRFLGS_ESC_MSB escaping as OpenSSL does. - Write values byte for byte. An embedded NUL previously truncated the value and copied uninitialized heap bytes into the BIO. - Escaping can triple the value length. Reject lengths where the buffer size would overflow. Entries are still always separated by ", " and printed with their short name. XN_FLAG_MULTILINE was 0xFFFF and behaved like XN_FLAG_RFC2253; it now escapes control and non-ASCII bytes and does not reverse the order.
There was a problem hiding this comment.
🟡 Changes recommended
A couple of small but concrete maintainability/documentation issues remain in the touched code (stored as PR comments) and should be corrected before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR improves OpenSSL-compatibility and safety in X509_NAME_print_ex() output formatting by fixing flag semantics and implementing stricter, byte-accurate value escaping/printing (including embedded NUL handling) to avoid truncation and potential heap data disclosure.
Changes:
- Re-define
XN_FLAG_RFC2253/XN_FLAG_MULTILINEto align with OpenSSL’s bit layout and ensure DN order is only reversed whenXN_FLAG_DN_REVis set. - Update
wolfSSL_X509_NAME_print_ex()to escape values byte-for-byte (supportingASN1_STRFLGS_ESC_2253,ASN1_STRFLGS_ESC_CTRL,ASN1_STRFLGS_ESC_MSB) and to size buffers defensively to avoid integer overflow. - Extend API tests to cover RFC2253 escaping behavior, DN reversal behavior, embedded NULs, control-byte escaping, and MSB escaping.
File summaries
| File | Description |
|---|---|
wolfssl/openssl/x509.h |
Updates XN flag macro definitions to match OpenSSL behavior and avoid bit collisions. |
src/x509.c |
Reworks wolfSSL_X509_NAME_print_ex() to do length-based, byte-accurate escaping and output writing with overflow checks. |
tests/api/test_ossl_x509_name.c |
Adds targeted regression tests for flag semantics and value escaping (including embedded NUL/control/MSB bytes). |
Review details
- Files reviewed: 3/3 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.
| const unsigned long escFlags = WOLFSSL_ASN1_STRFLGS_ESC_2253 | | ||
| WOLFSSL_ASN1_STRFLGS_ESC_CTRL | WOLFSSL_ASN1_STRFLGS_ESC_MSB | | ||
| WOLFSSL_ASN1_STRFLGS_ESC_QUOTE; |
| } | ||
| else if (bio->type != WOLFSSL_BIO_FILE && | ||
| bio->type != WOLFSSL_BIO_MEMORY) { | ||
| /* include the terminating null when not writing to a file */ |
|
There was a problem hiding this comment.
🔵 Needs a closer look
wolfSSL_X509_NAME_print_ex() still conditionally writes a trailing NUL byte for some BIO types, producing inconsistent output that is unexpected for an OpenSSL-compat print API.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/x509.c:15774
- The last-entry path conditionally appends a terminating NUL byte for non-file/non-memory BIOs and then includes it in the write length. This means the output differs by BIO type and can send an extra '\0' byte over e.g. socket/custom BIOs, which is unexpected for a print function (and contradicts the function header comment stating the output does not include a null terminator).
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Lite
The NUL was once written for every BIO type and later limited to BIOs that are not files or memory. It made the output depend on the BIO type and contradicted the function comment.
There was a problem hiding this comment.
🟢 Approval recommended
The changes are narrowly scoped, add strong regression tests for key compatibility/security edge cases, and the updated implementation avoids prior truncation/leak hazards while maintaining consistent output formatting.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Lite
Frauschi
left a comment
Frauschi
left a comment
There was a problem hiding this comment.
Looks good overall - the byte-for-byte value copy fixes a real uninitialized-heap disclosure for DN values with an embedded NUL, and the flag bits no longer collide. One comment on the header side of XN_FLAG_RFC2253.
| #define WOLFSSL_XN_FLAG_FN_ALIGN (1 << 25) | ||
|
|
||
| #define WOLFSSL_XN_FLAG_MULTILINE 0xFFFF | ||
| #define WOLFSSL_XN_FLAG_RFC2253 (WOLFSSL_ASN1_STRFLGS_RFC2253 | \ |
There was a problem hiding this comment.
XN_FLAG_RFC2253 now expands to OpenSSL's exact bit pattern, including XN_FLAG_SEP_COMMA_PLUS and XN_FLAG_FN_SN, but wolfSSL_X509_NAME_print_ex never reads the SEP_/FN_ selectors - it always emits ", " between entries. OpenSSL and RFC 2253 2.2 use ",", so X509_NAME_print_ex(bio, name, 0, XN_FLAG_RFC2253) gives CN=wolfssl.com, C=US where OpenSSL gives CN=wolfssl.com,C=US. Same gap for SEP_MULTILINE and FN_LN in the new XN_FLAG_MULTILINE: output stays on one line with short names.
That matters for anyone porting from OpenSSL who renders a DN and compares it against a string generated there. Could you either honour XN_FLAG_SEP_MASK, or note the limitation here in x509.h? The caveat is currently only in the doxygen block on wolfSSL_X509_NAME_print_ex, and the header is what a caller reads when picking the flag.
XN_FLAG_RFC2253shared bit 1 withASN1_STRFLGS_ESC_2253, so requesting only RFC 2253 escaping also reversed the DN order.XN_FLAG_RFC2253andXN_FLAG_MULTILINEare now defined as OpenSSL does, and DN order is only reversed whenXN_FLAG_DN_REVis set.ASN1_STRFLGS_ESC_2253,ASN1_STRFLGS_ESC_CTRLandASN1_STRFLGS_ESC_MSBescaping to match OpenSSL behavior.XN_FLAG_MULTILINE(previously0xFFFF, behaving likeXN_FLAG_RFC2253) now escapes control and non-ASCII bytes without reversing order.This fixes OpenVPN master's
ssl_testdriver, which callsX509_NAME_print_ex()withXN_FLAG_SEP_CPLUS_SPC | XN_FLAG_FN_SN | ASN1_STRFLGS_ESC_2253 | ASN1_STRFLGS_UTF8_CONVERT(fix for CVE-2026-84790) and previously failed against wolfSSL.