Commit 65b9c42
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 <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>1 parent b044256 commit 65b9c42
2 files changed
Lines changed: 35 additions & 5 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -289,7 +289,7 @@ function setServers(servers) { | |||
| 289 | 289 | // servers cares won't have any servers available for resolution | |
| 290 | 290 | const orig = this._handle.getServers(); | |
| 291 | 291 | const newSet = []; | |
| 292 | - const IPv6RE = /\[(.*)\]/; | ||
| 292 | + const IPv6RE = /^\[([^[\]]*)\]/; | ||
| 293 | 293 | const addrSplitRE = /(^.+?)(?::(\d+))?$/; | |
| 294 | 294 | ||
| 295 | 295 | servers.forEach((serv) => { | |
@@ -309,11 +309,16 @@ function setServers(servers) { | |||
| 309 | 309 | } | |
| 310 | 310 | } | |
| 311 | 311 | ||
| 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; | ||
| 314 | 317 | ||
| 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 | + } | ||
| 317 | 322 | } | |
| 318 | 323 | ||
| 319 | 324 | throw new ERR_INVALID_IP_ADDRESS(serv); | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -62,6 +62,31 @@ assert(existing.length > 0); | |||
| 62 | 62 | ]); | |
| 63 | 63 | } | |
| 64 | 64 | ||
| 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 | + | ||
| 65 | 90 | const goog = [ | |
| 66 | 91 | '8.8.8.8', | |
| 67 | 92 | '8.8.4.4', | |
0 commit comments