20260409-IsValidFQDN

[douzzer]

src/internal.c: add IsValidFQDN(), and in MatchDomainName(), do pattern matching and case folding only if target string IsValidFQDN().

tested with

wolfssl-multi-test.sh ...
check-source-text
clang-tidy-all-sp-all

see #10169 and ZD #21565

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #10183

Scan targets checked: wolfssl-bugs, wolfssl-compliance, wolfssl-consttime, wolfssl-defaults, wolfssl-mutation, wolfssl-proptest, wolfssl-src, wolfssl-zeroize

Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)

This review was generated automatically by Fenrir. Findings are non-blocking.

[dgarske]

🐺 Skoll Code Review

Overall recommendation: APPROVE
Findings: 5 total — 5 posted, 0 skipped

Posted findings

• [Medium] Unsigned nameSz <= 0 comparison is tautologically equivalent to == 0 — src/internal.c:13295
• [Low] Header comment says 'at least two labels' but code intentionally allows single labels — src/internal.c:13281
• [Medium] Trailing dot stripped from str but not from pattern — asymmetric matching — src/internal.c:13380-13382
• [Medium] Underscore-containing DNS names lose case-insensitive matching — src/internal.c:13371-13378
• [Medium] No unit tests for IsValidFQDN or the new MatchDomainName early-return behavior — src/internal.c:13288-13345

Review generated by Skoll via openclaw

[dgarske]

🟡 [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;

[dgarske]

🔵 [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")

[dgarske]

🟡 [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;

[dgarske]

🟡 [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.

[dgarske]

🟡 [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.