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.

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: setServers has super-linear regex #20443

Closed
davisjam opened this issue May 1, 2018 · 0 comments
Closed

dns: setServers has super-linear regex #20443

davisjam opened this issue May 1, 2018 · 0 comments

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

I recently scanned lib/ with my vuln-regex-detector tools and found a super-linear regex in lib/dns.js.

Here is a demo:

(20:20:05) jamie@woody ~ $ cat /tmp/node-dns-poc.js
#!/usr/bin/env node

const dns = require('dns');

const badServer = '['.repeat(100000);
console.log('Setting badServer');
try {
    dns.setServers([badServer]);
} catch (e) {}
console.log('Done');

(20:20:06) jamie@woody ~ $ time /tmp/node-dns-poc.js
Setting badServer
Done

real    0m6.330s <------- Ouch.
user    0m6.326s
sys    0m0.004s

The long execution time is due to this unsafe regex:

Resolver.prototype.setServers = setServers;
function setServers(servers) {
  // cache the original servers because in the event of an error setting the
  // servers cares won't have any servers available for resolution
  const orig = this._handle.getServers();
  const newSet = [];
  const IPv6RE = /\[(.*)\]/; <---------- Quadratic complexity

I submitted this via HackerOne for assessment and it was deemed not a likely threat vector. I agree with this assessment, since I really hope most users aren't letting clients set their DNS servers.

However, this is still a potential performance bug. I am working on a PR.

davisjam added a commit to davisjam/node that referenced this issue May 31, 2018
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: nodejs#20443
@Trott Trott closed this as completed in 872c331 Jun 9, 2018
targos pushed a commit that referenced this issue Jun 10, 2018
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
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
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant