-
Notifications
You must be signed in to change notification settings - Fork 959
20260409-IsValidFQDN #10183
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
20260409-IsValidFQDN #10183
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -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") | ||||||||||||||
| * - 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) | ||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 [Medium] No unit tests for
Recommendation: Add unit tests that call |
||||||||||||||
| { | ||||||||||||||
| word32 i; | ||||||||||||||
| int labelLen = 0; | ||||||||||||||
| int labelCount = 0; | ||||||||||||||
| int tldHasAlpha = 0; /* tracks alpha presence in the *current* label */ | ||||||||||||||
|
|
||||||||||||||
| if (name == NULL || nameSz <= 0) | ||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 [Medium] Unsigned
Suggestion:
Suggested change
|
||||||||||||||
| 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 | ||||||||||||||
douzzer marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||
|
|
@@ -13296,6 +13368,19 @@ int MatchDomainName(const char* pattern, int patternLen, const char* str, | |||||||||||||
| return 1; | ||||||||||||||
| #endif | ||||||||||||||
|
|
||||||||||||||
| if (! IsValidFQDN(str, strLen)) { | ||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 [Medium] Underscore-containing DNS names lose case-insensitive matching DNS names containing underscores (e.g. Recommendation: Consider whether |
||||||||||||||
| /* 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). */ | ||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 [Medium] Trailing dot stripped from After the Suggestion:
Suggested change
|
||||||||||||||
| if (str[strLen-1] == '.') | ||||||||||||||
| --strLen; | ||||||||||||||
|
|
||||||||||||||
| while (patternLen > 0) { | ||||||||||||||
| /* Get the next pattern char to evaluate */ | ||||||||||||||
| char p = (char)XTOLOWER((unsigned char)*pattern); | ||||||||||||||
|
|
||||||||||||||
There was a problem hiding this comment.
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
styleThe 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: