Skip to content

Commit

Permalink
wireaddr: is_dnsaddr allow underscore in hostname
Browse files Browse the repository at this point in the history 
The hostname part of a DNS FQDN can allow for additional characters
other than specified in `man 7 hostname`.

This extends is_dnsaddr and the test issue #5657.
Also fixes a typo in a comment.

Changelog-Fixed: wireaddr: #5657 allow '_' underscore in hostname part of DNS FQDN
  • Loading branch information
@m-schmoock @cdecker
m-schmoock authored and cdecker committed Dec 6, 2022
1 parent a96ff3b commit 29f81ba
Show file tree
Hide file tree
Showing 2 changed files with 17 additions and 3 deletions.
3 changes: 3 additions & 0 deletions common/test/run-ip_port_parsing.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -126,11 +126,14 @@ int main(int argc, char *argv[])
assert(is_dnsaddr("123example.com"));
assert(is_dnsaddr("example123.com"));
assert(is_dnsaddr("is-valid.3hostname123.com"));
assert(is_dnsaddr("just-a-hostname-with-dashes"));
assert(is_dnsaddr("lightningd_dest.underscore.allowed.in.hostname.part.com"));
assert(!is_dnsaddr("UPPERCASE.invalid.com"));
assert(!is_dnsaddr("-.invalid.com"));
assert(!is_dnsaddr("invalid.-example.com"));
assert(!is_dnsaddr("invalid.example-.com"));
assert(!is_dnsaddr("invalid..example.com"));
assert(!is_dnsaddr("underscore.not.allowed.in.domain_name.com"));

/* Grossly invalid. */
assert(!separate_address_and_port(tmpctx, "[", &ip, &port));
Expand Down
17 changes: 14 additions & 3 deletions common/wireaddr.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -375,18 +375,24 @@ bool is_wildcardaddr(const char *arg)
return streq(arg, "");
}

/* Rules:
/* The rules to check for DNS FQDNs, see `man 7 hostname`
*
* - not longer than 255
* - segments are separated with . dot
* - segments do not start or end with - hyphen
* - segments must be longer thant zero
* - lowercase a-z and digits 0-9 and - hyphen
* - segments must be longer than zero
* - allow lowercase a-z and digits 0-9 and - hyphen
* - additionall we allow for an '_' underscore in the hostname part.
*
* See issue #5657
* https://github.com/ElementsProject/lightning/issues/5657
* https://en.wikipedia.org/wiki/Hostname
*/
bool is_dnsaddr(const char *arg)
{
size_t i, arglen;
int lastdot;
int numdot;

if (is_ipaddr(arg) || is_toraddr(arg) || is_wildcardaddr(arg))
return false;
Expand All @@ -396,8 +402,10 @@ bool is_dnsaddr(const char *arg)
if (arglen > 255)
return false;
lastdot = -1;
numdot = 0;
for (i = 0; i < arglen; i++) {
if (arg[i] == '.') {
numdot++;
/* segment must be longer than zero */
if (i - lastdot == 1)
return false;
Expand All @@ -416,6 +424,9 @@ bool is_dnsaddr(const char *arg)
continue;
if (arg[i] == '-')
continue;
/* allow for _ underscores in the first hostname part */
if (arg[i] == '_' && numdot == 0)

This comment has been minimized.

Sign in to view

Copy link
@whitslack

whitslack Dec 6, 2022

Collaborator

@m-schmoock: Underscores can appear anywhere in a DNS name, not just in the first label (what you're calling a segment). While it's true that you cannot register a domain name containing an underscore, you can still have a FQDN like one_two.three_four.five_six that's resolvable via some alternative name server ecosystem apart from the default global root name servers.

Is there any good reason you're being so restrictive? You should follow the robustness principle and be maximally forgiving in what you'll accept. If the adminstrator tells you to advertise abc@xyz_123-.9 as a DNS name, you should obey. Warn if you think they're wrong, but don't refuse. You don't know better than the admin.

This comment has been minimized.

Sign in to view

Copy link
@m-schmoock

m-schmoock Dec 7, 2022

Author Collaborator

Well, same reason your not allowed to enter an IP address like 394.854.176.128:9735
or a malformed TOR type. There could be alternative internet layers or further TOR versions would allow for that and then we patch, first the bolt protocol, then the code here.

Your example abc@xyz_123-.9 looks like email and that could, in theory, be a future address type. It's not 'robust' to allow the user broadcasting invalid typo or otherwise unusable data to other nodes that will not understand this. It's sane to abort and tell him that upfront. Be conservative in what you send, be liberal in what you accept means exaclty that we should not accept any arbitrary input from the user here and send that via P2P to other node, still we can be liberal in what we accept from other nodes, which we do by passing the string to the operating system using getaddrinfo.

In any case I will submit a PR solving the uppercase character issue, I just hadn't hat the time yet.
continue;
return false;
}
return true;
Expand Down

0 comments on commit 29f81ba

Please sign in to comment.