Skip to content

Commit 65b9c42

Browse files
davisjamtargos
authored andcommitted
dns: improve setServers() errors and performance
Issue 1: make invalid setServers yield uniform error Behavior: dns.setServers throws a null pointer dereference on some inputs. Expected behavior was the more pleasant TypeError [ERR_INVALID_IP_ADDRESS] ... Root cause(s?): - Dereferencing the result of a regex match without confirming that there was a match. - assuming the capture of an optional group (?) Solution: Confirm the match, and handle a missing port cleanly. Tests: I added tests for various unusual inputs. Issue 2: revise quadratic regex in setServers Problem: The IPv6 regex was quadratic. On long malicious input the event loop could block. The security team did not deem it a security risk, but said a PR was welcome. Solution: Revise the regex to a linear-complexity version. Tests: I added REDOS tests to the "oddities" section. Fixes: #20441 Fixes: #20443 PR-URL: #20445 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]>
1 parent b044256 commit 65b9c42

File tree

2 files changed

+35
-5
lines changed

2 files changed

+35
-5
lines changed

lib/dns.js

+10-5
Original file line numberDiff line numberDiff line change
@@ -289,7 +289,7 @@ function setServers(servers) {
289289
// servers cares won't have any servers available for resolution
290290
const orig = this._handle.getServers();
291291
const newSet = [];
292-
const IPv6RE = /\[(.*)\]/;
292+
const IPv6RE = /^\[([^[\]]*)\]/;
293293
const addrSplitRE = /(^.+?)(?::(\d+))?$/;
294294

295295
servers.forEach((serv) => {
@@ -309,11 +309,16 @@ function setServers(servers) {
309309
}
310310
}
311311

312-
const [, s, p] = serv.match(addrSplitRE);
313-
ipVersion = isIP(s);
312+
// addr::port
313+
const addrSplitMatch = serv.match(addrSplitRE);
314+
if (addrSplitMatch) {
315+
const hostIP = addrSplitMatch[1];
316+
const port = addrSplitMatch[2] || IANA_DNS_PORT;
314317

315-
if (ipVersion !== 0) {
316-
return newSet.push([ipVersion, s, parseInt(p)]);
318+
ipVersion = isIP(hostIP);
319+
if (ipVersion !== 0) {
320+
return newSet.push([ipVersion, hostIP, parseInt(port)]);
321+
}
317322
}
318323

319324
throw new ERR_INVALID_IP_ADDRESS(serv);

test/parallel/test-dns.js

+25
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,31 @@ assert(existing.length > 0);
6262
]);
6363
}
6464

65+
{
66+
// Various invalidities, all of which should throw a clean error.
67+
const invalidServers = [
68+
' ',
69+
'\n',
70+
'\0',
71+
'1'.repeat(3 * 4),
72+
// Check for REDOS issues.
73+
':'.repeat(100000),
74+
'['.repeat(100000),
75+
'['.repeat(100000) + ']'.repeat(100000) + 'a'
76+
];
77+
invalidServers.forEach((serv) => {
78+
assert.throws(
79+
() => {
80+
dns.setServers([serv]);
81+
},
82+
{
83+
name: 'TypeError [ERR_INVALID_IP_ADDRESS]',
84+
code: 'ERR_INVALID_IP_ADDRESS'
85+
}
86+
);
87+
});
88+
}
89+
6590
const goog = [
6691
'8.8.8.8',
6792
'8.8.4.4',

0 commit comments

Comments
 (0)