Skip to content
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.

Sign up for GitHub

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

Jump to bottom

dns: consistency; performance on malicious input #20445

Closed
wants to merge 5 commits into from
Closed

dns: consistency; performance on malicious input #20445

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
cd73836
dns: make invalid setServers yield uniform error
davisjam May 1, 2018
fc93b59
dns: revise quadratic regex in setServers
davisjam May 31, 2018
03451b4
address review comments
davisjam Jun 1, 2018
ae34731
add missing semicolon caught by linter
davisjam Jun 2, 2018
260ee07
Update test-dns.js
Trott Jun 7, 2018
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
15 changes: 10 additions & 5 deletions lib/dns.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,7 +289,7 @@ function setServers(servers) {
// servers cares won't have any servers available for resolution
const orig = this._handle.getServers();
const newSet = [];
const IPv6RE = /\[(.*)\]/;
const IPv6RE = /^\[([^[\]]*)\]/;
const addrSplitRE = /(^.+?)(?::(\d+))?$/;

servers.forEach((serv) => {
Expand All @@ -309,11 +309,16 @@ function setServers(servers) {
}
}

const [, s, p] = serv.match(addrSplitRE);
ipVersion = isIP(s);
// addr::port
const addrSplitMatch = serv.match(addrSplitRE);
if (addrSplitMatch) {
const hostIP = addrSplitMatch[1];
const port = addrSplitMatch[2] || IANA_DNS_PORT;
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain the default value here? That is something unexpected for me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addrSplitMatch[2] might be undefined (see #20441), in which case I figured the port should be 53 (DNS port).

I think this is consistent with existing cases where a default port is selected, e.g. here and here.

Copy link
Member

Choose a reason for hiding this comment

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

@bnoordhuis @apapirovski PTAL

Copy link
Member

Choose a reason for hiding this comment

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

I think it should be fine. parseInt('') works out to 0 when coerced to int32 and c-ares uses the default port in that case. IOW, there should be no observable change in behavior, it'll use port 53 either way.


if (ipVersion !== 0) {
return newSet.push([ipVersion, s, parseInt(p)]);
ipVersion = isIP(hostIP);
if (ipVersion !== 0) {
return newSet.push([ipVersion, hostIP, parseInt(port)]);
}
}

throw new ERR_INVALID_IP_ADDRESS(serv);
Expand Down
26 changes: 26 additions & 0 deletions test/parallel/test-dns.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,32 @@ assert(existing.length > 0);
]);
}

{
// Various invalidities, all of which should throw a clean error.
const invalidServers = [
' ',
'\n',
'\0',
'1'.repeat(3 * 4),
// Check for REDOS issues.
':'.repeat(100000),
'['.repeat(100000),
'['.repeat(100000) + ']'.repeat(100000) + 'a'
];
invalidServers.forEach((serv) => {
assert.throws(
() => {
dns.setServers([serv])
},
{
name: 'TypeError [ERR_INVALID_IP_ADDRESS]',
code: 'ERR_INVALID_IP_ADDRESS'
},
`Unexpected error thrown for serv '${serv}'`
Copy link
Member

@Trott Trott Jun 3, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Nit: It would seem that the most likely case for this assertion to fire is that dns.setServers() does not throw here, rather than it throws the wrong error type. In that case, the output of the AssertionError will be:

AssertionError [ERR_ASSERTION]: Missing expected exception: Unexpected error thrown for serv

Someone seeing this message may be misled by "Unexpected error thrown" if the reason for the AssertionError is that no error was in fact thrown by dns.setServers().

Especially given that a few of serv values are whitespace, I think maybe just omitting the third argument to assert.throws() altogether might be better. If it were just this:

assert.throws(
    () => {
      dns.setServers([serv]);
    },
    {
      name: 'TypeError [ERR_INVALID_IP_ADDRESS]',
      code: 'ERR_INVALID_IP_ADDRESS'
    }
);

...then the default message would display and that one is pretty clear I think:

AssertionError [ERR_ASSERTION]: Missing expected exception.

It also improves the output when the wrong error is thrown:

AssertionError [ERR_ASSERTION]: Input A expected to strictly deep-equal input B:
+ expected - actual

  Comparison {
-   name: 'RangeError',
-   code: 'ERR_OOPS'
+   name: 'TypeError [ERR_INVALID_IP_ADDRESS]',
+   code: 'ERR_INVALID_IP_ADDRESS'
  }

With the code currently in this branch/PR, the user would get this instead:

AssertionError [ERR_ASSERTION]: Unexpected error thrown for serv

);
});
}

const goog = [
'8.8.8.8',
'8.8.4.4',
Expand Down