Skip to content
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
85 changes: 85 additions & 0 deletions src/internal.c
Original file line number Diff line number Diff line change
Expand Up @@ -13272,6 +13272,78 @@ static int MatchIPv6(const char* pattern, int patternLen,
}
#endif /* WOLFSSL_IP_ALT_NAME && !WOLFSSL_USER_IO */

/* 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-]
* - No label starts or ends with '-'
* - At least two labels (single-label names are not "fully qualified")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 [Low] Header comment says 'at least two labels' but code intentionally allows single labels
🔧 NIT style

The function header comment states "At least two labels (single-label names are not 'fully qualified')" as a rule that is enforced, but the code at line 13344 returns (labelCount > 0) && tldHasAlpha, deliberately allowing single-label names (as the inline comment explains: "we allow single labels, so that ad hoc 'localhost' CN is allowed"). The header comment should match the actual behavior to avoid confusing future readers.

Suggestion:

Suggested change
* - At least two labels (single-label names are not "fully qualified")
* - Labels must have at least one alpha character in the final label (TLD)
* - Single-label names are allowed (for ad-hoc names like "localhost")

* - Final label (TLD) contains at least one letter (rejects all-numeric
* strings that could be confused with IPv4 literals, and matches the
* ICANN constraint that TLDs are alphabetic)
* - Optional trailing dot is accepted (absolute FQDN form)
* - Internationalized names are valid in their ACE/punycode (xn--) form
*/
static int IsValidFQDN(const char* name, word32 nameSz)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 [Medium] No unit tests for IsValidFQDN or the new MatchDomainName early-return behavior
💡 SUGGEST test

IsValidFQDN is a new 57-line function with complex validation logic covering multiple edge cases (empty labels, leading/trailing hyphens, label length limits, trailing dots, TLD alpha requirement, total length limit). There are no direct unit tests for this function. The existing test_wolfSSL_X509_name_match* and test_wolfSSL_check_domain* tests exercise MatchDomainName indirectly, but they don't specifically test the new FQDN-vs-non-FQDN branching behavior or the full range of IsValidFQDN edge cases. Key scenarios that should be tested:

  • IPv4 address ("192.168.1.1") → exact match path
  • Valid FQDN ("foo.example.com") → wildcard/case-fold path
  • Trailing dot FQDN ("example.com.") → valid FQDN, dot stripped
  • Single label with alpha ("localhost") → valid FQDN path
  • All-numeric TLD ("123.456") → exact match path
  • Label starting/ending with hyphen → exact match path
  • 253-byte boundary cases
  • Underscore-containing names → exact match path

Recommendation: Add unit tests that call MatchDomainName with both FQDN and non-FQDN str values to verify the branching behavior. If IsValidFQDN cannot be tested directly (it's static), consider adding a wrapper or testing through MatchDomainName's observable behavior.

{
word32 i;
int labelLen = 0;
int labelCount = 0;
int tldHasAlpha = 0; /* tracks alpha presence in the *current* label */

if (name == NULL || nameSz <= 0)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 [Medium] Unsigned nameSz <= 0 comparison is tautologically equivalent to == 0
💡 SUGGEST bug

nameSz is declared as word32, which is typedef unsigned int word32 (or unsigned long). The comparison nameSz <= 0 can never be true for the < 0 branch — it is always equivalent to nameSz == 0. Clang may emit -Wtautological-compare warnings under -Wall, and GCC will warn with -Wtype-limits (part of -Wextra). The PR mentions testing with clang-tidy-all-sp-all, so this might already be caught, but it should be fixed to avoid surprise warnings in downstream builds.

Suggestion:

Suggested change
if (name == NULL || nameSz <= 0)
if (name == NULL || nameSz == 0)
return 0;

return 0;

/* Strip a single optional trailing dot before measuring. "example.com."
* is the absolute form of the same FQDN.
*/
if (name[nameSz - 1] == '.')
--nameSz;

if (nameSz < 1 || nameSz > 253)
return 0;

for (i = 0; i < nameSz; i++) {
byte c = (byte)name[i];

if (c == '.') {
if (labelLen == 0 || name[i - 1] == '-')
return 0; /* empty label or label ending in '-' */
++labelCount;
labelLen = 0;
tldHasAlpha = 0; /* reset for next label */
continue;
}

if (++labelLen > 63)
return 0;

if (c == '-') {
if (labelLen == 1) /* label starts with '-' */
return 0;
}
else if (((c | 0x20) >= 'a') && ((c | 0x20) <= 'z')) {
tldHasAlpha = 1;
}
else if ((c < '0') || (c > '9')) {
return 0; /* character outside [a-zA-Z0-9-] */
}
}

/* Final label (no trailing dot in the effective range to close it) */
if ((labelLen == 0) || (name[nameSz - 1] == '-'))
return 0;
++labelCount;

/* Technically, "Fully qualified" requires at least two labels, and the TLD
* must not be purely numeric (no such TLD exists; also rejects bare IPv4).
* We enforce the not-purely-numeric rule, but we allow single labels, so
* that ad hoc "localhost" CN is allowed.
*/
return ((labelCount > 0) && tldHasAlpha);
}

/* Match names with wildcards, each wildcard can represent a single name
component or fragment but not multiple names, i.e.,
*.z.com matches y.z.com but not x.y.z.com
Expand All @@ -13296,6 +13368,19 @@ int MatchDomainName(const char* pattern, int patternLen, const char* str,
return 1;
#endif

if (! IsValidFQDN(str, strLen)) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 [Medium] Underscore-containing DNS names lose case-insensitive matching
💡 SUGGEST question

DNS names containing underscores (e.g. _dmarc.example.com, _sip._tcp.example.com) are used in practice for SRV, DKIM, and DMARC records. While RFC 952/1123 does not allow underscores, RFC 4592/5280 and real-world certificates sometimes contain them. IsValidFQDN rejects such names (underscore falls through the [a-zA-Z0-9-] check), causing MatchDomainName to use the byte-exact XMEMCMP fallback — which means case-insensitive matching and wildcard matching are lost for these names. If a certificate has _DMARC.EXAMPLE.COM and the user checks _dmarc.example.com, the match will fail where it previously would have succeeded. This is likely acceptable given the PR's goals, but the behavioral change should be documented.

Recommendation: Consider whether IsValidFQDN should also accept underscores (expand the character set to [a-zA-Z0-9_-]), or document this as an intentional restriction. Underscored hostnames are uncommon in TLS certificates but not impossible.

/* Not a valid FQDN or IPv6 address -- require byte-exact match, no case
* folding, no wildcard interpretation. This is appropriate for an IPv4
* match, for example.
*/
return (((word32)patternLen == strLen) &&
(XMEMCMP(pattern, str, patternLen) == 0));
}

/* strip trailing dot if necessary (FQDN designator). */
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 [Medium] Trailing dot stripped from str but not from pattern — asymmetric matching
💡 SUGGEST question

After the IsValidFQDN check, the trailing dot is stripped from str but not from pattern. This means str="example.com." will match pattern="example.com" (good), but str="example.com" will NOT match pattern="example.com." because the pattern's trailing dot persists and exhausts the character-by-character loop after str is consumed. While certificates with trailing dots in SANs/CNs are unusual, RFC 5280 does not explicitly forbid them, and some CAs may produce them. This creates an asymmetry that could cause surprising match failures.

Suggestion:

Suggested change
/* strip trailing dot if necessary (FQDN designator). */
/* strip trailing dot if necessary (FQDN designator). */
if (str[strLen-1] == '.')
--strLen;
if (patternLen > 0 && pattern[patternLen-1] == '.')
--patternLen;

if (str[strLen-1] == '.')
--strLen;

while (patternLen > 0) {
/* Get the next pattern char to evaluate */
char p = (char)XTOLOWER((unsigned char)*pattern);
Expand Down
Loading