dns: improve setServers() errors and performance

davisjam · targos · commit 65b9c427ac33 · 2018-06-13T08:49:29.000+02:00
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>
diff --git a/lib/dns.js b/lib/dns.js
@@ -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) => {
@@ -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;
 
-    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);
diff --git a/test/parallel/test-dns.js b/test/parallel/test-dns.js
@@ -62,6 +62,31 @@ assert(existing.length > 0);
   ]);
 }
 
+{
+  // Various invalidities, all of which should throw a clean error.
+  const invalidServers = [
+    ' ',
+    '\n',
+    '\0',
+    '1'.repeat(3 * 4),
+    // Check for REDOS issues.
+    ':'.repeat(100000),
+    '['.repeat(100000),
+    '['.repeat(100000) + ']'.repeat(100000) + 'a'
+  ];
+  invalidServers.forEach((serv) => {
+    assert.throws(
+      () => {
+        dns.setServers([serv]);
+      },
+      {
+        name: 'TypeError [ERR_INVALID_IP_ADDRESS]',
+        code: 'ERR_INVALID_IP_ADDRESS'
+      }
+    );
+  });
+}
+
 const goog = [
   '8.8.8.8',
   '8.8.4.4',
