From c82f2445e5f5e8b387992e4e0e0ed99ab3e93012 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Wed, 10 Oct 2018 20:23:48 +0200 Subject: [PATCH] dns: use IDNA 2008 to encode non-ascii hostnames MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Before this commit, Node.js left it up to the system resolver or c-ares. Leaving it to the system resolver introduces platform differences because: * some support IDNA 2008 * some only IDNA 2003 (glibc until 2.28), and * some don't support IDNA at all (musl libc) c-ares doesn't support IDNA either although curl does, by virtue of linking against libidn2. Upgrading from libidn1 to libidn2 in order to get proper IDNA 2008 support was the fix for curl's CVE-2016-8625. libidn2 is not an option (incompatible license) but ICU has an IDNA API and we already use that in one place. For non-ICU builds, we fall back to the bundled punycode.js that also supports IDNA 2008. Fixes: https://github.com/nodejs-private/security/issues/97 Fixes: https://github.com/nodejs/node/issues/25558 PR-URL: https://github.com/nodejs/node/pull/25679 Reviewed-By: Santiago Gimeno Reviewed-By: Saúl Ibarra Corretgé Reviewed-By: Anna Henningsen Reviewed-By: Ruben Bridgewater Reviewed-By: Colin Ihrig Reviewed-By: Tiancheng "Timothy" Gu --- lib/dns.js | 5 ++-- lib/internal/dns/promises.js | 5 ++-- lib/internal/idna.js | 9 +++++++ lib/url.js | 5 +--- node.gyp | 1 + test/internet/test-dns-idna2008.js | 33 +++++++++++++++++++++++++ test/parallel/test-bootstrap-modules.js | 2 +- 7 files changed, 51 insertions(+), 9 deletions(-) create mode 100644 lib/internal/idna.js create mode 100644 test/internet/test-dns-idna2008.js diff --git a/lib/dns.js b/lib/dns.js index f68409f2ee6743..952ef39006c8e4 100644 --- a/lib/dns.js +++ b/lib/dns.js @@ -22,6 +22,7 @@ 'use strict'; const cares = internalBinding('cares_wrap'); +const { toASCII } = require('internal/idna'); const { isIP, isIPv4, isLegalPort } = require('internal/net'); const { customPromisifyArgs } = require('internal/util'); const errors = require('internal/errors'); @@ -139,7 +140,7 @@ function lookup(hostname, options, callback) { req.hostname = hostname; req.oncomplete = all ? onlookupall : onlookup; - var err = cares.getaddrinfo(req, hostname, family, hints, verbatim); + var err = cares.getaddrinfo(req, toASCII(hostname), family, hints, verbatim); if (err) { process.nextTick(callback, dnsException(err, 'getaddrinfo', hostname)); return {}; @@ -219,7 +220,7 @@ function resolver(bindingName) { req.hostname = name; req.oncomplete = onresolve; req.ttl = !!(options && options.ttl); - var err = this._handle[bindingName](req, name); + var err = this._handle[bindingName](req, toASCII(name)); if (err) throw dnsException(err, bindingName, name); return req; } diff --git a/lib/internal/dns/promises.js b/lib/internal/dns/promises.js index be49ebf2106d7e..25696bf2228b64 100644 --- a/lib/internal/dns/promises.js +++ b/lib/internal/dns/promises.js @@ -6,6 +6,7 @@ const { emitInvalidHostnameWarning, } = require('internal/dns/utils'); const { codes, dnsException } = require('internal/errors'); +const { toASCII } = require('internal/idna'); const { isIP, isIPv4, isLegalPort } = require('internal/net'); const { getaddrinfo, @@ -86,7 +87,7 @@ function createLookupPromise(family, hostname, all, hints, verbatim) { req.resolve = resolve; req.reject = reject; - const err = getaddrinfo(req, hostname, family, hints, verbatim); + const err = getaddrinfo(req, toASCII(hostname), family, hints, verbatim); if (err) { reject(dnsException(err, 'getaddrinfo', hostname)); @@ -184,7 +185,7 @@ function createResolverPromise(resolver, bindingName, hostname, ttl) { req.reject = reject; req.ttl = ttl; - const err = resolver._handle[bindingName](req, hostname); + const err = resolver._handle[bindingName](req, toASCII(hostname)); if (err) reject(dnsException(err, bindingName, hostname)); diff --git a/lib/internal/idna.js b/lib/internal/idna.js new file mode 100644 index 00000000000000..409cabedf10d1a --- /dev/null +++ b/lib/internal/idna.js @@ -0,0 +1,9 @@ +'use strict'; + +if (process.binding('config').hasIntl) { + const { toASCII, toUnicode } = internalBinding('icu'); + module.exports = { toASCII, toUnicode }; +} else { + const { toASCII, toUnicode } = require('punycode'); + module.exports = { toASCII, toUnicode }; +} diff --git a/lib/url.js b/lib/url.js index 9755cf430af924..569733bfc4b0d7 100644 --- a/lib/url.js +++ b/lib/url.js @@ -21,11 +21,8 @@ 'use strict'; -const { toASCII } = internalBinding('config').hasIntl ? - internalBinding('icu') : require('punycode'); - +const { toASCII } = require('internal/idna'); const { hexTable } = require('internal/querystring'); - const { SafeSet } = require('internal/safe_globals'); const { diff --git a/node.gyp b/node.gyp index 013ecb8e8330cd..d90da3c2d3c750 100644 --- a/node.gyp +++ b/node.gyp @@ -127,6 +127,7 @@ 'lib/internal/fs/utils.js', 'lib/internal/fs/watchers.js', 'lib/internal/http.js', + 'lib/internal/idna.js', 'lib/internal/inspector_async_hook.js', 'lib/internal/js_stream_socket.js', 'lib/internal/linkedlist.js', diff --git a/test/internet/test-dns-idna2008.js b/test/internet/test-dns-idna2008.js new file mode 100644 index 00000000000000..ebabe322f0db0d --- /dev/null +++ b/test/internet/test-dns-idna2008.js @@ -0,0 +1,33 @@ +'use strict'; + +// Verify that non-ASCII hostnames are handled correctly as IDNA 2008. +// +// * Tests will fail with NXDOMAIN when UTF-8 leaks through to a getaddrinfo() +// that doesn't support IDNA at all. +// +// * "straße.de" will resolve to the wrong address when the resolver supports +// only IDNA 2003 (e.g., glibc until 2.28) because it encodes it wrong. + +const { mustCall } = require('../common'); +const assert = require('assert'); +const dns = require('dns'); + +const [host, expectedAddress] = ['straße.de', '81.169.145.78']; + +dns.lookup(host, mustCall((err, address) => { + assert.ifError(err); + assert.strictEqual(address, expectedAddress); +})); + +dns.promises.lookup(host).then(mustCall(({ address }) => { + assert.strictEqual(address, expectedAddress); +})); + +dns.resolve4(host, mustCall((err, addresses) => { + assert.ifError(err); + assert.deepStrictEqual(addresses, [expectedAddress]); +})); + +new dns.promises.Resolver().resolve4(host).then(mustCall((addresses) => { + assert.deepStrictEqual(addresses, [expectedAddress]); +})); diff --git a/test/parallel/test-bootstrap-modules.js b/test/parallel/test-bootstrap-modules.js index 19ca44ec463297..6bfd5d2d769f2a 100644 --- a/test/parallel/test-bootstrap-modules.js +++ b/test/parallel/test-bootstrap-modules.js @@ -10,7 +10,7 @@ const assert = require('assert'); const isMainThread = common.isMainThread; const kCoverageModuleCount = process.env.NODE_V8_COVERAGE ? 1 : 0; -const kMaxModuleCount = (isMainThread ? 64 : 84) + kCoverageModuleCount; +const kMaxModuleCount = (isMainThread ? 65 : 85) + kCoverageModuleCount; assert(list.length <= kMaxModuleCount, `Total length: ${list.length}\n` + list.join('\n')