Skip to content

Commit

Permalink
dns: use IDNA 2008 to encode non-ascii hostnames
Browse files Browse the repository at this point in the history
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: #25558

PR-URL: #25679
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
  • Loading branch information
bnoordhuis authored and addaleax committed Jan 28, 2019
1 parent 6d9af41 commit c82f244
Show file tree
Hide file tree
Showing 7 changed files with 51 additions and 9 deletions.
5 changes: 3 additions & 2 deletions lib/dns.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down Expand Up @@ -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 {};
Expand Down Expand Up @@ -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;
}
Expand Down
5 changes: 3 additions & 2 deletions lib/internal/dns/promises.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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));
Expand Down Expand Up @@ -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));
Expand Down
9 changes: 9 additions & 0 deletions lib/internal/idna.js
Original file line number Diff line number Diff line change
@@ -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 };
}
5 changes: 1 addition & 4 deletions lib/url.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
1 change: 1 addition & 0 deletions node.gyp
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down
33 changes: 33 additions & 0 deletions test/internet/test-dns-idna2008.js
Original file line number Diff line number Diff line change
@@ -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]);
}));
2 changes: 1 addition & 1 deletion test/parallel/test-bootstrap-modules.js
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand Down

0 comments on commit c82f244

Please sign in to comment.