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: use IDNA 2008 to encode non-ascii hostnames #25679

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
9 participants
@bnoordhuis
Copy link
Member

bnoordhuis commented Jan 24, 2019

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: nodejs-private/security#97

cc @santigimeno - I'm not too hung up on whether this PR lands or #25559 but this way at least you can compare the two. :)

@bnoordhuis bnoordhuis referenced this pull request Jan 24, 2019

Closed

dns: idna encode hostnames before resolution #25559

2 of 4 tasks complete
@santigimeno
Copy link
Member

santigimeno left a comment

LGTM with one comment.
Should we be lenient to toASCII() errors and call it with the 2nd parameter set to true and catching possible exceptions like in here?

@saghul

saghul approved these changes Jan 24, 2019

@bnoordhuis

This comment has been minimized.

Copy link
Member Author

bnoordhuis commented Jan 24, 2019

Should we be lenient to toASCII() errors

I saw that in your PR. Are there legitimate inputs where it fails on?

@santigimeno

This comment has been minimized.

Copy link
Member

santigimeno commented Jan 24, 2019

I saw that in your PR. Are there legitimate inputs where it fails on?

I don't know. I just saw its usage in https://github.com/nodejs/node/blob/master/lib/url.js#L385 and that the punycode implementation could potentially throw. If you don't think that's an issue I'm fine.

// 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.

This comment has been minimized.

@TimothyGu

TimothyGu Jan 24, 2019

Member

nit: one too many leading space

dns: use IDNA 2008 to encode non-ascii hostnames
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: nodejs-private/security#97
Fixes: #25558

@bnoordhuis bnoordhuis force-pushed the bnoordhuis:fix25558 branch from 2409ae2 to e3ddd72 Jan 28, 2019

@bnoordhuis

This comment has been minimized.

Copy link
Member Author

bnoordhuis commented Jan 28, 2019

@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented Jan 28, 2019

Landed in f399b01

@addaleax addaleax closed this Jan 28, 2019

addaleax added a commit that referenced this pull request Jan 28, 2019

dns: use IDNA 2008 to encode non-ascii hostnames
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: nodejs-private/security#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>

addaleax added a commit that referenced this pull request Jan 28, 2019

dns: use IDNA 2008 to encode non-ascii hostnames
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: nodejs-private/security#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>

@targos targos referenced this pull request Jan 29, 2019

Merged

v11.9.0 proposal #25802

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Feb 1, 2019

test/internet/test-dns-idna2008.js (added in this PR four days ago) failed in CI a few hours ago:

https://ci.nodejs.org/job/node-test-commit-custom-suites/855/default/console

00:01:48 not ok 9 internet/test-dns-idna2008
00:01:48   ---
00:01:48   duration_ms: 1.14
00:01:48   severity: fail
00:01:48   exitcode: 1
00:01:48   stack: |-
00:01:48     (node:4568) ExperimentalWarning: The dns.promises API is experimental
00:01:48     assert.js:753
00:01:48         throw newErr;
00:01:48         ^
00:01:48     
00:01:48     AssertionError [ERR_ASSERTION]: ifError got unwanted exception: queryA ESERVFAIL straße.de
00:01:48         at QueryReqWrap.dns.resolve4.mustCall (/home/iojs/build/workspace/node-test-commit-custom-suites/default/test/internet/test-dns-idna2008.js:27:10)
00:01:48         at QueryReqWrap.callback (/home/iojs/build/workspace/node-test-commit-custom-suites/default/test/common/index.js:368:15)
00:01:48         at QueryReqWrap.onresolve [as oncomplete] (dns.js:199:10)
00:01:48         at QueryReqWrap.onresolve [as oncomplete] (dns.js:199:19)
00:01:48   ...

It had previously succeeded in CI (see https://ci.nodejs.org/job/node-test-commit-custom-suites/852/default/console).

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Feb 1, 2019

I guess ESERVFAIL is a DNS problem likely unrelated to the test itself? Although other DNS tests in internet ran just fine, so maybe it is in fact specific to this test?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment