crypto: implement OpenSSL-compatible X509Certificate.checkHost/checkEmail options - #33676
crypto: implement OpenSSL-compatible X509Certificate.checkHost/checkEmail options#33676robobun wants to merge 7 commits into
Conversation
…mail options BoringSSL defines X509_CHECK_FLAG_ALWAYS_CHECK_SUBJECT, X509_CHECK_FLAG_NO_PARTIAL_WILDCARDS, X509_CHECK_FLAG_MULTI_LABEL_WILDCARDS and X509_CHECK_FLAG_SINGLE_LABEL_SUBDOMAINS as 0, so the checkHost/checkEmail option parser compiled to 'flags |= 0' and the options were silently ignored. Port OpenSSL's do_x509_check and helpers into ncrypto so the documented options (subject:'always', partialWildcards, multiLabelWildcards, singleLabelSubdomains) behave like Node, and stop rejecting an empty options object.
|
Updated 11:40 AM PT - Jul 26th, 2026
❌ @robobun, your commit 87e98e6 has 1 failures in
Add 🧪 To try this PR locally: bunx bun-pr 33676That installs a local version of the PR into your bun-33676 --bun |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
WalkthroughReplaces OpenSSL/BoringSSL-based hostname and email certificate matching in ChangesX509 certificate check flags and matching
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs, but this ports ~250 lines of OpenSSL's certificate name-matching logic and replaces BoringSSL's X509_check_host/email — security-sensitive enough that a human should sign off.
Extended reasoning...
Overview
This PR ports OpenSSL's do_x509_check and its helpers (valid_star, wildcard_match, equal_nocase/equal_case/equal_email, skip_prefix) from crypto/x509/v3_utl.c into ncrypto.cpp, replacing the previous delegation to BoringSSL's X509_check_host/X509_check_email. It defines local X509View::CheckFlags constants with OpenSSL's real bit values (BoringSSL #defines several of these to 0), wires getFlags() to use them, and removes the incorrect "options must have at least one property" throw. checkIp and the QUIC path in bun-usockets remain on BoringSSL.
Security risks
This is certificate hostname/email matching — the code that decides whether a given name is covered by an X.509 certificate. A defect here could either reject valid names (breakage) or, worse, accept names that shouldn't match (e.g., an over-permissive wildcard, mishandled IDNA label, or a bounds slip in skip_prefix/wildcard_match). The port also introduces manual buffer handling (ASN1_STRING_to_UTF8 + OPENSSL_free, OPENSSL_strndup, raw pointer arithmetic in valid_star/wildcard_match). The change deliberately loosens matching relative to BoringSSL's stricter behaviour (partial wildcards, multi-label wildcards, subject fallback with SANs present), which is correct for Node compat but is exactly the direction that warrants careful review.
Level of scrutiny
High. ~250 lines of new C++ reimplementing security-relevant string/wildcard matching, with pointer arithmetic on ASN.1-derived data. Even though it's a line-for-line port of well-audited OpenSSL code and the PR verified byte-identical output against Node v26.3.0 across a purpose-built cert matrix, the CLAUDE.md guidance is explicit: never auto-approve changes to crypto / certificate-verification paths.
Other factors
- The port looks faithful to OpenSSL
v3_utl.c; memory ownership (free ofastron the OOM path indo_check_string,GENERAL_NAMES_free, null-initialisedpeername) appears correct. - Embedded-NUL rejection is preserved by an explicit
memchrguard now that BoringSSL's wrapper (which did that check) is bypassed. - Test coverage is thorough (9 new tests, 3 purpose-built certs, every option exercised, negative cases, NUL rejection) and the description reports ASAN-clean under a 60k-iteration loop.
- No prior reviews or outstanding comments on the PR.
do_x509_check only returns -1/0/1; the null and embedded-NUL inputs that BoringSSL's X509_check_host/email reported as -2 are now rejected before the switch.
There was a problem hiding this comment.
No further issues from me — the dead case -2: cleanup landed. This is a ~260-line port of OpenSSL's X509 host/email matching (wildcard parsing, subject-DN fallback) into ncrypto.cpp, so it warrants a human pass over the pointer arithmetic and flag semantics before merge.
Extended reasoning...
Overview
This PR ports OpenSSL's do_x509_check and its helpers (valid_star, wildcard_match, equal_nocase/equal_case/equal_email, skip_prefix, do_check_string) — roughly 260 lines of new C++ — into src/jsc/bindings/ncrypto.cpp, replacing the BoringSSL X509_check_host/X509_check_email calls that silently ignored four of the five documented checkHost/checkEmail options. It adds an X509View::CheckFlags struct in ncrypto.h with OpenSSL's real bit values, wires getFlags() in JSX509CertificatePrototype.cpp to use them, removes the bogus empty-options error, and adds a thorough test matrix in test/js/node/crypto/x509.test.ts cross-checked against Node.js.
Security risks
The new code is X.509 hostname/email matching — squarely security-adjacent. It intentionally loosens matching relative to BoringSSL when the caller opts in (partial wildcards by default, subject: 'always' fallback, multi-label wildcards, .suffix inputs). Per the PR description this only backs the user-facing X509Certificate.checkHost/checkEmail Node-compat API; the actual TLS/QUIC verification path (packages/bun-usockets/src/quic.c) still uses BoringSSL's stricter X509_check_host. That scoping is important and looks correct, but the ported string-matching code itself has non-trivial pointer arithmetic over certificate-derived ASN.1 strings and deserves a careful human read for bounds correctness and fidelity to upstream v3_utl.c.
Level of scrutiny
High. This is hand-ported crypto library code with manual buffer walking, an OPENSSL_strndup/OPENSSL_free ownership pair, and flag-driven branching that changes what certificates a name will match. Even though it's a faithful-looking port with strong test coverage and an ASAN loop, ported security code in this repo's conventions calls for a maintainer to diff it against the OpenSSL reference.
Other factors
My earlier inline comment (unreachable case -2: arms) was addressed in 071be66 and the thread is resolved. The bug-hunting pass found nothing on the current revision. Test coverage is excellent — nine new tests exercising every flag against purpose-built certs, with expected values taken from Node — and the PR description documents ASAN verification. None of that changes the fact that this is outside the "simple/mechanical" bar for auto-approval.
|
All test lanes green on build 82498 (26/26 in That delta is not from this diff. The baseline is main Review threads: all five resolved (one dead-code finding addressed in 071be66, four comment-cop threads addressed in 3c3ee2b / 87e98e6). Ready for a maintainer. |
…eckhost-openssl-flags
…elevant type OpenSSL's X509_check_host/X509_check_email default rule falls back to the subject CN/emailAddress when the certificate has no SAN of the relevant type, not only when the SAN extension is absent. BoringSSL's implementation skips the subject as soon as any SAN extension is present. The ported do_x509_check already handles this via san_present; add explicit coverage for certs whose SANs are all of a different type (or an empty sequence).
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs, but this is a ~250-line hand-port of OpenSSL's do_x509_check / valid_star / wildcard_match into a security-sensitive path (X509 hostname/email matching), and it strictly widens what matches relative to the previous BoringSSL-backed behaviour. That warrants a human diff against crypto/x509/v3_utl.c before landing.
What was reviewed:
- Traced
do_x509_checkreturn values →{-1, 0, 1};-1correctly maps toOPERATION_FAILEDand the earlier deadcase -2:labels are gone. - Checked memory ownership:
peername(OPENSSL_strndup) is adopted byDataPointer,GENERAL_NAMES_free(gens)runs on every exit,astris freed on both success and thestrndup-failure path. - Confirmed the new
CheckFlagsbit values match OpenSSL'sX509_CHECK_FLAG_*and don't collide withkDotSubdomainsFlag = 0x8000;checkIpstill routes to BoringSSL and QUIC'sX509_check_hostis untouched.
Extended reasoning...
Overview
The PR replaces BoringSSL's X509_check_host / X509_check_email with a local port of OpenSSL's do_x509_check (and its helpers skip_prefix, equal_nocase/equal_case/equal_email, wildcard_match, valid_star, do_check_string) inside src/jsc/bindings/ncrypto.cpp. A new X509View::CheckFlags struct in ncrypto.h carries OpenSSL's real bit values, and getFlags() in JSX509CertificatePrototype.cpp is rewired to use them (the bogus empty-options TypeError is also removed). ~150 lines of new tests exercise every documented option against six purpose-built certificates.
Security risks
This is X509 certificate name matching — the change makes checkHost/checkEmail more permissive than before: partial-label wildcards (a*.example.com), multi-label wildcards, .suffix subdomain matching, and subject-CN fallback in the presence of unrelated SAN types now succeed where they previously returned undefined. That is the documented Node.js/OpenSSL behaviour and the PR is careful to leave TLS/QUIC verification on BoringSSL's stricter path, so this only affects users who call X509Certificate#checkHost/checkEmail directly. Still, any transcription error in the ~250-line port could accept a hostname OpenSSL would reject, so this is squarely security-sensitive.
Level of scrutiny
High. Per the repo guidance, hand-rolled security-sensitive parsing needs to "replicate the FULL verification path" of the reference implementation, and ported code should be diffed line-for-line against upstream. The port looks faithful (I spot-checked valid_star's star/label/IDNA state machine, wildcard_match's IDNA guard and hostname-char loop, and do_x509_check's san_present fallback rule against OpenSSL's v3_utl.c), and memory handling is clean (GENERAL_NAMES_free, OPENSSL_free(astr), OPENSSL_strndup → DataPointer). But a maintainer should independently confirm the port matches upstream and is comfortable with the compat-vs-strictness tradeoff.
Other factors
Test coverage is thorough — every option is exercised in both directions with expected values taken from Node v26.3.0, plus the per-type SAN fallback rule and empty-SAN edge case. My earlier inline note about unreachable case -2: was addressed; the comment-cop nits were resolved by trimming to single-line provenance comments. No outstanding threads. Deferring because this is complex, security-adjacent native code that a human should sign off on.
|
Closing this since #36252 (tls: share one native certificate-name matcher across fetch and X509Certificate#checkHost) merged and covers the same ground. Thank you @robobun for the PR — if there's a piece of this that #36252 didn't pick up, please say so and we'll take another look. (This comment was written by Claude, on behalf of the Bun team.) |
|
Ran this PR's test matrix against current main (a7838c5, with #36252 merged): all Three // SAN email:san.first@..., email:UPPER@... subject emailAddress=Subject.Mail@x509.sysfuzz.test
E.checkEmail("Subject.Mail@x509.sysfuzz.test", { subject: "always" });
// node: "Subject.Mail@x509.sysfuzz.test" bun main: undefined
// SAN DNS:host.b.test only (no email SAN) subject emailAddress=hidden@b.test
dnsOnlySan.checkEmail("hidden@b.test");
// node: "hidden@b.test" bun main: undefined
// SAN present but empty sequence subject emailAddress=subj@c.test
emptySan.checkEmail("subj@c.test");
// node: "subj@c.test" bun main: undefinedThe first is the |
2 participants
What
X509Certificate.checkHost()andcheckEmail()accept five documented options. Four of them were type-validated and then silently ignored, andpartialWildcardsbeing the Node default meant partial-wildcard SAN entries likeDNS:a*.example.comnever matched at all. Passing{}as the options object also threw instead of using defaults.Separately, the no-options default subject-fallback rule also diverged: Node falls back to the subject CN/emailAddress whenever the certificate has no SAN of the relevant type (so a cert with only email SANs still matches
checkHost()by CN, and a cert with only DNS SANs still matchescheckEmail()by subject emailAddress), but Bun only fell back when the SAN extension was absent altogether.Repro
Cause
getFlags()ORs inX509_CHECK_FLAG_ALWAYS_CHECK_SUBJECT/NO_PARTIAL_WILDCARDS/MULTI_LABEL_WILDCARDS/SINGLE_LABEL_SUBDOMAINSand passes them to BoringSSL'sX509_check_host/X509_check_email. BoringSSL#defines all four of those to0and itsdo_x509_checknever implements the OpenSSL behaviour behind them (no subject fallback when SANs of the right type exist, no partial or multi-label wildcards, no.suffixmatching), so the whole options block compiled toflags |= 0.BoringSSL's
do_x509_checkalso returns immediately after walking the SAN list whenever the extension is present at all, whereas OpenSSL trackssan_presentper type and only suppresses the subject fallback when at least one SAN of the requested type was seen.Fix
Port OpenSSL
crypto/x509/v3_utl.cdo_x509_checkand its helpers (valid_star,wildcard_match,equal_nocase/equal_case/equal_email,skip_prefix) intoncrypto.cppand drive them with locally definedX509View::CheckFlagsthat carry OpenSSL's real bit values.X509View::checkHost/checkEmailnow call the port instead of BoringSSL, andgetFlags()ORs in the new constants. The bogus "options must have at least one property" check is removed to match Node.checkIpis unchanged (no flags apply to IP matching).packages/bun-usockets/src/quic.cstill calls BoringSSL'sX509_check_host, which keeps QUIC certificate verification on BoringSSL's stricter rules.Verification
test/js/node/crypto/x509.test.tsgains two describe blocks: one covering every option against three purpose-built certificates (full-label wildcard + CN, partial-wildcard SANs, email SAN + subject emailAddress), and one covering the default subject-fallback rule against certs whose SANs are entirely of a different type or an empty sequence. Every expected value was taken from Node.js v26.3.0; the debug build's output is byte-identical to Node for the full repro matrix. 9 of the 12 new tests fail onmain, all 26 pass with this change.test-crypto-x509.js,test-tls-getcertificate-x509.jsandx509-subclass.test.tsstill pass, and a 60k-iteration loop under ASAN shows bounded RSS.This follows up on #33299, which noted these options as BoringSSL limitations.
[review] gate passed · iteration 6 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 6
evidence per changed file