Skip to content
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 resolution fails for internationalized domain names #25558

Closed
saghul opened this issue Jan 18, 2019 · 5 comments
Closed

DNS resolution fails for internationalized domain names #25558

saghul opened this issue Jan 18, 2019 · 5 comments
Labels
confirmed-bug Issues with confirmed bugs. dns Issues and PRs related to the dns subsystem.

Comments

@saghul
Copy link
Member

saghul commented Jan 18, 2019

  • Version: v10.14.2
  • Platform: Darwin november.local 18.2.0 Darwin Kernel Version 18.2.0: Mon Nov 12 20:24:46 PST 2018; root:xnu-4903.231.4~2/RELEASE_X86_64 x86_64
  • Subsystem: dns, cares_wrap

Currently the DNS module seems to utf-8 encode the names passed to c-ares for DNS resolution:

node::Utf8Value name(env->isolate(), string);

That doesn't work for internationalized domain names, which need to be IDNA encoded.

DNS is pretty much all ASCII, so IMHO Node shouldn't utf-8 encode on input.

Here is a list of international domain names to test.

This will produce an error:

> dns.resolve('españa.icom.museum', function(r, e) { console.log(r, e) })`
> { Error: queryA ENOTFOUND españa.icom.museum
    at QueryReqWrap.onresolve [as oncomplete] (dns.js:197:19)
  errno: 'ENOTFOUND',
  code: 'ENOTFOUND',
  syscall: 'queryA',
  hostname: 'españa.icom.museum' } undefined

Whereas the proper IDNA encoded version works:

> dns.resolve('xn--espaa-rta.icom.museum', function(e, r) { console.log(e, r) })
> null [ '91.194.60.138' ]
@saghul saghul added the dns Issues and PRs related to the dns subsystem. label Jan 18, 2019
@targos targos self-assigned this Jan 18, 2019
@targos
Copy link
Member

targos commented Jan 18, 2019

All supported versions of Node are affected. I'm looking into it.

@santigimeno
Copy link
Member

Possible fix in #25559

@targos targos removed their assignment Jan 18, 2019
@silverwind
Copy link
Contributor

I'm quite surprised that this affects so many version. I'm pretty sure IDN did work at least at some point in time.

@bnoordhuis
Copy link
Member

@santigimeno and I opened PRs but now that I think about it, this issue should be fixed once v10.x upgrades to libuv v1.24.0 because that's the first libuv release that includes libuv/libuv#2046.

Landing the PRs would still be useful for:

  1. v6.x (and v8.x?), because libuv isn't upgraded on that branch, and
  2. z/os, because EBCDIC

@bnoordhuis
Copy link
Member

I'm pretty sure IDN did work at least at some point in time.

@silverwind If you'll allow me to self-quote from #25679 (comment):

[..] 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)

In other words, Node's behavior is currently platform-dependent (bad.)

bnoordhuis added a commit to bnoordhuis/io.js that referenced this issue 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: nodejs-private/security#97
Fixes: nodejs#25558
addaleax pushed a commit that referenced this issue 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>
abhishekumar-tyagi pushed a commit to abhishekumar-tyagi/node that referenced this issue May 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. dns Issues and PRs related to the dns subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants