dns: use IDNA 2008 to encode non-ascii hostnames #25679
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nodejs-github-bot
added
the
lib / src
4 tasks
santigimeno
approved these changes
Jan 24, 2019
There was a problem hiding this 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
approved these changes
Jan 24, 2019
addaleax
approved these changes
Jan 24, 2019
I saw that in your PR. Are there legitimate inputs where it fails on? |
BridgeAR
approved these changes
Jan 24, 2019
cjihrig
approved these changes
Jan 24, 2019
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. |
TimothyGu
approved these changes
Jan 24, 2019
test/internet/test-dns-idna2008.js
Outdated
| // 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. |
There was a problem hiding this comment.
nit: one too many leading space
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: nodejs#25558
|
Nit addressed and rebased: https://ci.nodejs.org/job/node-test-pull-request/20370/ |
|
Landed in f399b01 |
addaleax
pushed a commit
that referenced
this pull request
Jan 28, 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: 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>
addaleax
pushed a commit
that referenced
this pull request
Jan 28, 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: 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>
Merged
|
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). |
|
I guess |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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:
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
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. :)