From cd73836591e38ae5a31dabe8a779edb5a466189b Mon Sep 17 00:00:00 2001 From: Jamie Davis Date: Mon, 30 Apr 2018 21:27:18 -0400 Subject: [PATCH 1/5] dns: 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. Fixes: https://github.com/nodejs/node/issues/20441 --- lib/dns.js | 13 +++++++++---- test/parallel/test-dns.js | 13 +++++++++++++ 2 files changed, 22 insertions(+), 4 deletions(-) diff --git a/lib/dns.js b/lib/dns.js index 62faf32b690735..cbde0af2c0ac3d 100644 --- a/lib/dns.js +++ b/lib/dns.js @@ -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 index fb648544b95469..27f6f4a126d096 100644 --- a/test/parallel/test-dns.js +++ b/test/parallel/test-dns.js @@ -62,6 +62,19 @@ assert(existing.length > 0); ]); } +{ + // Various invalidities, all of which should throw a clean error. + const invalidServers = [ + ' ', + '\n', + '\0', + '1'.repeat(3 * 4) + ]; + invalidServers.forEach((serv) => { + assert.throws(() => dns.setServers([serv]), /TypeError.*ERR_INVALID_IP_ADDRESS/, `Unexpected error thrown for ${serv}`); + }); +} + const goog = [ '8.8.8.8', '8.8.4.4', From fc93b598c0aebc4e80ee98208d89b548777986f8 Mon Sep 17 00:00:00 2001 From: Jamie Davis Date: Thu, 31 May 2018 00:02:55 -0400 Subject: [PATCH 2/5] dns: 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: https://github.com/nodejs/node/issues/20443 --- lib/dns.js | 2 +- test/parallel/test-dns.js | 5 ++++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/lib/dns.js b/lib/dns.js index cbde0af2c0ac3d..1f6994ba16fd21 100644 --- 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) => { diff --git a/test/parallel/test-dns.js b/test/parallel/test-dns.js index 27f6f4a126d096..27356e5104c71b 100644 --- a/test/parallel/test-dns.js +++ b/test/parallel/test-dns.js @@ -68,7 +68,10 @@ assert(existing.length > 0); ' ', '\n', '\0', - '1'.repeat(3 * 4) + '1'.repeat(3 * 4), + ':'.repeat(100000), + '['.repeat(100000), + '['.repeat(100000) + ']'.repeat(100000) + 'a' ]; invalidServers.forEach((serv) => { assert.throws(() => dns.setServers([serv]), /TypeError.*ERR_INVALID_IP_ADDRESS/, `Unexpected error thrown for ${serv}`); From 03451b444326da15aa97793e314eea2afd5341ed Mon Sep 17 00:00:00 2001 From: Jamie Davis Date: Thu, 31 May 2018 23:56:13 -0400 Subject: [PATCH 3/5] address review comments --- test/parallel/test-dns.js | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/test/parallel/test-dns.js b/test/parallel/test-dns.js index 27356e5104c71b..c48afab8843419 100644 --- a/test/parallel/test-dns.js +++ b/test/parallel/test-dns.js @@ -69,12 +69,22 @@ assert(existing.length > 0); '\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]), /TypeError.*ERR_INVALID_IP_ADDRESS/, `Unexpected error thrown for ${serv}`); + assert.throws( + () => { + dns.setServers([serv]) + }, + { + name: 'TypeError [ERR_INVALID_IP_ADDRESS]', + code: 'ERR_INVALID_IP_ADDRESS' + }, + `Unexpected error thrown for serv '${serv}'` + ); }); } From ae347312fe2d44ef00ca9e2e8ff4b7cfd29d9fa5 Mon Sep 17 00:00:00 2001 From: Jamie Davis Date: Sat, 2 Jun 2018 14:07:07 -0400 Subject: [PATCH 4/5] add missing semicolon caught by linter --- test/parallel/test-dns.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/parallel/test-dns.js b/test/parallel/test-dns.js index c48afab8843419..edeedfb7bf6d9c 100644 --- a/test/parallel/test-dns.js +++ b/test/parallel/test-dns.js @@ -77,7 +77,7 @@ assert(existing.length > 0); invalidServers.forEach((serv) => { assert.throws( () => { - dns.setServers([serv]) + dns.setServers([serv]); }, { name: 'TypeError [ERR_INVALID_IP_ADDRESS]', From 260ee0725b206f5a4d8bbd6d9e1634f488a13fd4 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Thu, 7 Jun 2018 16:53:57 -0700 Subject: [PATCH 5/5] Update test-dns.js --- test/parallel/test-dns.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test/parallel/test-dns.js b/test/parallel/test-dns.js index edeedfb7bf6d9c..f50b9464f102cc 100644 --- a/test/parallel/test-dns.js +++ b/test/parallel/test-dns.js @@ -82,8 +82,7 @@ assert(existing.length > 0); { name: 'TypeError [ERR_INVALID_IP_ADDRESS]', code: 'ERR_INVALID_IP_ADDRESS' - }, - `Unexpected error thrown for serv '${serv}'` + } ); }); }