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: setServers throws inconsistent TypeError #20441

Closed
davisjam opened this issue May 1, 2018 · 1 comment
Closed

dns: setServers throws inconsistent TypeError #20441

davisjam opened this issue May 1, 2018 · 1 comment

Comments

@davisjam
Copy link
Contributor

davisjam commented May 1, 2018

  • Version
./out/Release/node  --version
v11.0.0-pre
  • Platform
(20:24:36) jamie@woody ~/Desktop/floss/node $ uname -a
Linux woody 4.13.0-38-generic #43~16.04.1-Ubuntu SMP Wed Mar 14 17:48:43 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux
  • Subsystem

DNS

Problematic behavior

The dns.setServers API responds inconsistently on different invalid input.

Good behavior:

dns.setServers([' '])
TypeError [ERR_INVALID_IP_ADDRESS]: Invalid IP address:  
    at servers.forEach (dns.js:319:11)
    at Array.forEach (<anonymous>)
    at Resolver.setServers (dns.js:295:11)
    at Object.defaultResolverSetServers [as setServers] (dns.js:360:12)

Bad behavior:

> dns.setServers(['\n'])
TypeError: Cannot read property 'Symbol(Symbol.iterator)' of null
    at servers.forEach (dns.js:312:27)
    at Array.forEach (<anonymous>)
    at Resolver.setServers (dns.js:295:11)
    at Object.defaultResolverSetServers [as setServers] (dns.js:360:12)

A null pointer dereference is rather less nice than an error like ERR_INVALID_IP_ADDRESS.

I believe the problem is in dns.js:

...
  servers.forEach((serv) => {
    ...
    const match = serv.match(IPv6RE);
    // we have an IPv6 in brackets
    if (match) {
       ....
    }
    ...
    const [, s, p] = serv.match(addrSplitRE);

While the IPv6 code only accesses match if there was a match, the addrSplitRE code assumes there is a match, which can lead to this unsavory TypeError.
@davisjam
Copy link
Contributor Author

davisjam commented May 1, 2018

Working on a PR

davisjam added a commit to davisjam/node that referenced this issue May 1, 2018
@davisjam
dns: make invalid setServers yield uniform error
cd73836 
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.

Fixes: nodejs#20441
@davisjam davisjam mentioned this issue May 1, 2018
Closed
3 tasks
@Trott Trott closed this as completed in 872c331 Jun 9, 2018
targos pushed a commit that referenced this issue Jun 10, 2018
@davisjam @targos
dns: improve setServers() errors and performance
ab554d1 
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>
targos pushed a commit that referenced this issue Jun 13, 2018
@davisjam @targos
dns: improve setServers() errors and performance
65b9c42 
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@davisjam