Fix IDNA matching#10331
Conversation
There was a problem hiding this comment.
Pull request overview
Updates wolfSSL’s hostname matching to properly handle IDNA (punycode “xn--” A-label) cases by tightening wildcard behavior and adding targeted regression tests.
Changes:
- Add A-label detection and block wildcard matching when IDNA A-labels are involved (per RFC 6125 / RFC 9525 guidance).
- Introduce a helper to validate whether an input is a well-formed FQDN and use it to gate wildcard behavior under
WOLFSSL_LEFT_MOST_WILDCARD_ONLY. - Add new API tests covering IDNA wildcard edge cases and FQDN validation boundaries.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/internal.c |
Adds IDNA A-label gating logic to MatchDomainName() and integrates FQDN validation for stricter wildcard-only mode. |
wolfcrypt/src/asn.c |
Adds wolfssl_local_IsValidFQDN() helper used by domain matching logic. |
wolfssl/wolfcrypt/asn.h |
Exposes wolfssl_local_IsValidFQDN() for in-tree testing via WOLFSSL_TEST_VIS. |
wolfssl/internal.h |
Changes MatchDomainName() declaration visibility to WOLFSSL_TEST_VIS to support direct test calls. |
tests/api/test_ossl_x509.c |
Adds new unit tests for wolfssl_local_IsValidFQDN() and IDNA wildcard matching; strengthens existing IP/hostname regression coverage. |
tests/api/test_ossl_x509.h |
Registers the newly added tests in the ossl_x509 test group. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| EXPECT_DECLS; | ||
| #if !defined(NO_CERTS) | ||
| static const struct { |
There was a problem hiding this comment.
test_wolfSSL_MatchDomainName_idn is currently only gated on !NO_CERTS, but it calls MatchDomainName() with WOLFSSL_LEFT_MOST_WILDCARD_ONLY, which (after this change) depends on wolfssl_local_IsValidFQDN() from ASN code. If there are configurations that build with NO_ASN (or without BUILD_ASN) but still compile this test, it will fail to build/link. Consider guarding this test with !defined(NO_ASN) (or the same feature macro that guarantees wolfssl_local_IsValidFQDN is available).
| if (leftWildcardOnly && (! wolfssl_local_IsValidFQDN(str, strLen))) { | ||
| /* Not a valid FQDN -- require byte-exact match, no case folding, no | ||
| * wildcard interpretation. This is appropriate for an IPv4 match, for | ||
| * example, but also matches improvised names like "localhost", albeit | ||
| * case-sensitively. |
There was a problem hiding this comment.
MatchDomainName() now calls wolfssl_local_IsValidFQDN(), which is only declared when !NO_ASN (via wolfcrypt/asn.h) and implemented in wolfcrypt/src/asn.c (built only with ASN). If NO_ASN is enabled without also defining NO_CERTS, this introduces a compile/link failure. Consider moving wolfssl_local_IsValidFQDN() into src/internal.c (or otherwise ensuring it’s available whenever MatchDomainName() is built), or wrap the call with #ifndef NO_ASN and provide an alternate path when ASN is disabled.
| test_cases[i].is_FQDN); | ||
| if (! EXPECT_SUCCESS()) { | ||
| fprintf(stderr, "wolfssl_local_IsValidFQDN() wrong result for " | ||
| "case %d \"%s\"\n", i, test_cases[i].str); |
There was a problem hiding this comment.
In test_wolfssl_local_IsValidFQDN, the failure diagnostic uses "%s" with test_cases[i].str, but one test case intentionally sets str = NULL. If the NULL-case ever fails (or any future NULL case is added), this error path can crash/UB. Print a placeholder when str == NULL (or use %p) to keep the diagnostic safe.
| "case %d \"%s\"\n", i, test_cases[i].str); | |
| "case %d \"%s\"\n", i, | |
| test_cases[i].str ? test_cases[i].str : "(null)"); |
| * MatchDomainName() is WOLFSSL_LOCAL but visible to the test binary because | ||
| * tests link against the in-tree library. */ |
There was a problem hiding this comment.
The comment above test_wolfSSL_MatchDomainName_idn says MatchDomainName() is WOLFSSL_LOCAL, but in this PR the declaration was changed to WOLFSSL_TEST_VIS in wolfssl/internal.h. Update the comment to reflect the current visibility/export mechanism (or remove the visibility claim) to avoid misleading future readers.
| * MatchDomainName() is WOLFSSL_LOCAL but visible to the test binary because | |
| * tests link against the in-tree library. */ | |
| * MatchDomainName() is exposed for testing via the visibility mechanism | |
| * declared in wolfssl/internal.h. */ |
| /* Returns 1 if name is a syntactically valid DNS FQDN per RFC 952/1123. | ||
| * | ||
| * Rules enforced: | ||
| * - Total effective length (excluding optional trailing dot) in [1, 253] | ||
| * - Each label is 1-63 octets of [a-zA-Z0-9-], with _ allowed in all but | ||
| * the last label. | ||
| * - No label starts or ends with '-' |
There was a problem hiding this comment.
The doc comment claims this validates an FQDN “per RFC 952/1123”, but the implementation explicitly allows underscores in non-final labels. RFC 952/1123 host name syntax doesn’t permit underscores, so the reference is misleading. Either tighten the validation to match the RFC claim, or adjust the comment to describe this as a pragmatic DNS-label check that allows underscores in non-TLD labels.
|
Description
Handle IDNA wildcards
Requires #10183 (will need to be rebased)
Fixes zd21706
Testing
Added
test_wolfSSL_MatchDomainName_idnChecklist