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

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 2 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
16 changes: 16 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,22 @@ assert(existing.length > 0);
]);
}

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

Choose a reason for hiding this comment

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

This line is too long, but I'm not sure how best to format it. I tried the obvious indentation but eslint complained that the TypeError ought to be indented 18 spaces (??).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The correct error type is Error, not TypeError.

Copy link
Contributor

Choose a reason for hiding this comment

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

This should exceed the max line number :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes I recall a comment about that earlier. I need to fix this.

});
}

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