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

Unable to resolve A when record points to CNAME containing asterisk #42171

Closed
rb-cohen opened this issue Mar 1, 2022 · 8 comments
Closed

Unable to resolve A when record points to CNAME containing asterisk #42171

rb-cohen opened this issue Mar 1, 2022 · 8 comments
Labels
dns Issues and PRs related to the dns subsystem.

Comments

@rb-cohen
Copy link

rb-cohen commented Mar 1, 2022

Version

16.14.0

Platform

20.5.0 Darwin Kernel Version 20.5.0: Sat May 8 05:10:33 PDT 2021; root:xnu-7195.121.3~9/RELEASE_X86_64 x86_64

Subsystem

No response

What steps will reproduce the bug?

Perform a dns.lookup() against a domain that resolves to a CNAME containing a wildcard asterisk (*).

Domains pointing to CloudFlare seem to use this approach in particular.

Sample code (will try to find a domain that breaks that I can share publicly):

const dns = require('dns');
dns.setServers([ "8.8.8.8", "8.8.4.4" ]);

domain="test.tld";

dns.resolve(domain, function (error, addresses) {
    console.log("Error: ", error);  // null
    console.log("DNS servers: ", dns.getServers());  // [ '8.8.8.8', '8.8.4.4' ]
    console.log(domain + " resolves to: ", addresses);  // [ '192.168.0.10' ]
});

Using nslookup or dig does return an A record. Using a version of node <16.6.2 also works.

Probably related to this similar issue:
#39780

How often does it reproduce? Is there a required condition?

Can be reproduced every time.

What is the expected behavior?

dns.resolve() returns a successful result (A record addresses).

What do you see instead?

Error:  Error: queryA EBADRESP test.tld
    at QueryReqWrap.onresolve [as oncomplete] (node:dns:213:19) {
  errno: undefined,
  code: 'EBADRESP',
  syscall: 'queryA',
  hostname: 'test.tld'
}

Additional information

Seems related to:

@rb-cohen
Copy link
Author

rb-cohen commented Mar 1, 2022

Here's a dig with the domain redacted too:

dig test.tld

; <<>> DiG 9.10.6 <<>> test.tld
;; global options: +cmd
;; Got answer:
;; ->>HEADER<<- opcode: QUERY, status: NOERROR, id: 3826
;; flags: qr rd ra; QUERY: 1, ANSWER: 3, AUTHORITY: 0, ADDITIONAL: 1

;; OPT PSEUDOSECTION:
; EDNS: version: 0, flags:; udp: 512
;; QUESTION SECTION:
;test.tld.		IN	A

;; ANSWER SECTION:
test.tld.	6529	IN	CNAME	*.test.tld.cdn.cloudflare.net.
*.test.tld.cdn.cloudflare.net. 300 IN A	1.1.1.1
*.test.tld.cdn.cloudflare.net. 300 IN A	1.1.1.1

;; Query time: 67 msec
;; SERVER: 192.168.0.1#53(192.168.0.1)
;; WHEN: Tue Mar 01 18:54:45 GMT 2022
;; MSG SIZE  rcvd: 128

@VoltrexKeyva VoltrexKeyva added the dns Issues and PRs related to the dns subsystem. label Mar 1, 2022
@benjamingr
Copy link
Member

@nodejs/dns

@bnoordhuis
Copy link
Member

I don't think *.test.tld.cdn.cloudflare.net. is a valid CNAME. I'm not even sure what you'd do with it if it was.

FWIW, musl libc rejects it too.

@rb-cohen
Copy link
Author

rb-cohen commented Mar 2, 2022

I don't think *.test.tld.cdn.cloudflare.net. is a valid CNAME. I'm not even sure what you'd do with it if it was.

I was trying to check, but didn't find anything conclusive. There's https://www.ietf.org/rfc/rfc4592.txt, but, at first glance didn't get a definitive answer.

To clarify, the CNAME record does not contain a wildcard, just the value (answer). It is valid to have a wildcard A record, so it would seem weird that you couldn't CNAME to it. Stranger things have happened.

Browsers, dig, nslookup, and older versions of node seem OK with it, fwiw?

@richardlau
Copy link
Member

The validation is done in c-ares so any changes would have to be made there.

@rb-cohen
Copy link
Author

rb-cohen commented Mar 2, 2022

The validation is done in c-ares so any changes would have to be made there.

Thank you, have raised an issue on c-ares. 🙏
c-ares/c-ares#457

bradh352 added a commit to c-ares/c-ares that referenced this issue Mar 2, 2022
…e wildcard domains

CloudFlare appears to use this logic in CNAMEs as per
nodejs/node#42171

Fixes: #457
Fix By: Brad House (@bradh352)
@bnoordhuis
Copy link
Member

Fixed in c-ares/c-ares@b5a3d96. Someone want to open a cherry-pick PR? It's trivial enough that I don't think it can do harm.

rb-cohen pushed a commit to rb-cohen/node that referenced this issue Mar 4, 2022
Original commit message:

Asterisks should be allowed in host validation as CNAMEs may reference wildcard domains

CloudFlare appears to use this logic in CNAMEs as per
nodejs#42171

Fixes: nodejs#457
Fix By: Brad House (@bradh352)
rb-cohen pushed a commit to rb-cohen/node that referenced this issue Mar 4, 2022
Original commit message:

Asterisks should be allowed in host validation as CNAMEs may reference
wildcard domains

CloudFlare appears to use this logic in CNAMEs as per
nodejs#42171

Fixes: nodejs#457
Fix By: Brad House (@bradh352)
rb-cohen pushed a commit to rb-cohen/node that referenced this issue Mar 4, 2022
Original commit message:

Asterisks should be allowed in host validation as CNAMEs may reference
wildcard domains

CloudFlare appears to use this logic in CNAMEs as per
nodejs#42171

Fixes: c-ares/c-ares#457
Fix By: Brad House (@bradh352)
@rb-cohen
Copy link
Author

rb-cohen commented Mar 4, 2022

There's a cherry-pick PR ready for review. I had a battle with the commit message linter, it won.

bengl pushed a commit that referenced this issue Mar 21, 2022
Original commit message:

Asterisks should be allowed in host validation as CNAMEs may reference
wildcard domains

CloudFlare appears to use this logic in CNAMEs as per
#42171

Fixes: c-ares/c-ares#457
Fix By: Brad House (@bradh352)

PR-URL: #42216
Fixes: #42171
Fixes: #457
Refs: c-ares/c-ares#457
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
danielleadams pushed a commit to danielleadams/node that referenced this issue Apr 21, 2022
Original commit message:

Asterisks should be allowed in host validation as CNAMEs may reference
wildcard domains

CloudFlare appears to use this logic in CNAMEs as per
nodejs#42171

Fixes: c-ares/c-ares#457
Fix By: Brad House (@bradh352)

PR-URL: nodejs#42216
Fixes: nodejs#42171
Fixes: nodejs#457
Refs: c-ares/c-ares#457
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
danielleadams pushed a commit that referenced this issue Apr 24, 2022
Original commit message:

Asterisks should be allowed in host validation as CNAMEs may reference
wildcard domains

CloudFlare appears to use this logic in CNAMEs as per
#42171

Fixes: c-ares/c-ares#457
Fix By: Brad House (@bradh352)

PR-URL: #42216
Fixes: #42171
Fixes: #457
Refs: c-ares/c-ares#457
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
danielleadams pushed a commit that referenced this issue Apr 24, 2022
Original commit message:

Asterisks should be allowed in host validation as CNAMEs may reference
wildcard domains

CloudFlare appears to use this logic in CNAMEs as per
#42171

Fixes: c-ares/c-ares#457
Fix By: Brad House (@bradh352)

PR-URL: #42216
Fixes: #42171
Fixes: #457
Refs: c-ares/c-ares#457
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
danielleadams pushed a commit that referenced this issue Apr 24, 2022
Original commit message:

Asterisks should be allowed in host validation as CNAMEs may reference
wildcard domains

CloudFlare appears to use this logic in CNAMEs as per
#42171

Fixes: c-ares/c-ares#457
Fix By: Brad House (@bradh352)

PR-URL: #42216
Fixes: #42171
Fixes: #457
Refs: c-ares/c-ares#457
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
xtx1130 pushed a commit to xtx1130/node that referenced this issue Apr 25, 2022
Original commit message:

Asterisks should be allowed in host validation as CNAMEs may reference
wildcard domains

CloudFlare appears to use this logic in CNAMEs as per
nodejs#42171

Fixes: c-ares/c-ares#457
Fix By: Brad House (@bradh352)

PR-URL: nodejs#42216
Fixes: nodejs#42171
Fixes: nodejs#457
Refs: c-ares/c-ares#457
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this issue Jan 31, 2023
Version 1.19.0 (18 Jan 2023)

bradh352 (18 Jan 2023)
- Prep for 1.19.0 release

- Fix inverted logic in 25523e2

  Fix .localhost. handling in prior commit

  Fix By: Brad House (@bradh352)

- RFC6761 localhost definition includes subdomains

  RFC6761 6.3 states:
    The domain "localhost." and any names falling within ".localhost."

  We were only honoring "localhost".

  Fixes: #477
  Fix By: Brad House (@bradh352)

- docs: ARES_OPT_UDP_PORT and ARES_OPT_TCP_PORT docs wrong byte order

  As per #487, documentation states the port should be in network byte
  order, but we can see from the test cases using MockServers on
  different ports that this is not the case, it is definitely in host
  byte order.

  Fix By: Brad House (@bradh352)

GitHub (18 Jan 2023)
- [hopper-vul brought this change]

  Add str len check in config_sortlist to avoid stack overflow (#497)

  In ares_set_sortlist, it calls config_sortlist(..., sortstr) to parse
  the input str and initialize a sortlist configuration.

  However, ares_set_sortlist has not any checks about the validity of the input str.
  It is very easy to create an arbitrary length stack overflow with the unchecked
  `memcpy(ipbuf, str, q-str);` and `memcpy(ipbufpfx, str, q-str);`
  statements in the config_sortlist call, which could potentially cause severe
  security impact in practical programs.

  This commit add necessary check for `ipbuf` and `ipbufpfx` which avoid the
  potential stack overflows.

  fixes #496

  Fix By: @hopper-vul

bradh352 (18 Jan 2023)
- Fix build due to str-split sed gone wrong

  Fix By: Brad House (@bradh352)

- cirrus-ci: switch to scan-build-py for MacOS

  MacOS seems to work better with scan-build-py

  Fix By: Brad House (@bradh352)

- ares_strsplit* -> ares__strsplit* to comply with internal function naming

  Inspired by #495, but was missing test cases and would failed to build.

  Fix By: Brad House (@bradh352), Daniel Stenberg (@bagder)

- Cirrus-CI: MacOS Homebrew has changed from /usr/local/opt to /opt/homebrew

  Fix paths for homebrew.

  Fix By: Brad House (@bradh352)

- cirrus-ci: iOS build needs to use ARM MacOS image

  CirrusCI removed Intel-based MacOS images.  Need to switch
  iOS builds to use new ARM images as well.

  Fix By: Brad House (@bradh352)

- cirrus-ci: new MacOS image

  Cirrus-CI has recently EOL'd Intel MacOS VMs, switch to the latest
  ARM-based image.

  Fix By: Brad House (@bradh352)

- acountry was passing stack variable to callback

  Recent ASAN versions picked up that acountry was passing stack
  variables to ares_gethostbyname() then leaving the stack context.
  We will now allocate a buffer for this.

  Fix By: Brad House (@bradh352)

GitHub (13 Dec 2022)
- [Daniel Stenberg brought this change]

  docs: reformat/cleanup man pages SYNOPSIS sections (#494)

  To make them render "nicer" in both terminals and on the website.

  - Removes the bold
  - Removes .PP lines
  - Indents them more like proper code style

  Fix By: Daniel Stenberg (@bagder)

- [Nikolaos Chatzikonstantinou brought this change]

  bug fix: new ares_strsplit (#492)

  * add ares_strsplit unit test

  The test reveals a bug in the implementation of ares_strsplit when the
  make_set parameter is set to 1, as distinct domains are confused for
  equal:

    out = ares_strsplit("example.com, example.co", ", ", 1, &n);

  evaluates to n = 1 with out = { "example.com" }.

  * bugfix and cleanup of ares_strsplit

  The purpose of ares_strsplit in c-ares is to split a comma-delimited
  string of unique (up to letter case) domains. However, because the
  terminating NUL byte was not checked in the substrings when comparing
  for uniqueness, the function would sometimes drop domains it should
  not. For example,

      ares_strsplit("example.com, example.co", ",")

  would only result in a single domain "example.com".

  Aside from this bugfix, the following cleanup is performed:

  1. The tokenization now happens with the help of strcspn instead of the
     custom function is_delim.
  2. The function list_contains has been inlined.
  3. The interface of ares_strsplit has been simplified by removing the
     parameter make_set since in practice it was always 1.
  4. There are fewer passes over the input string.
  5. We resize the table using realloc() down to its minimum size.
  6. The docstring of ares_strsplit is updated and also a couple typos
     are fixed.

  There occurs a single use of ares_strsplit and since the make_set
  parameter has been removed, the call in ares_init.c is modified
  accordingly. The unit test for ares_strsplit is also updated.

  Fix By: Nikolaos Chatzikonstantinou (@createyourpersonalaccount)

bradh352 (23 Oct 2022)
- CirrusCI: update freebsd image

  Old FreeBSD image for CirrusCI has issues with newer symbols, update to later one.

  Fix By: Brad House (@bradh352)

GitHub (23 Oct 2022)
- [Stephen Sachs brought this change]

  Fix Intel compiler deprecated options (#485)

  Options `-we ###` and `-wd ###` should not include a whitespace. They are also deprecated and `-diag-error` and `-diag-disable` are their replacements.

  Intel compiler 2021.6 is not able to be used in configure without the proposed patch.

  Fix By: Stephen Sachs (@stephenmsachs)

- [Jonathan Ringer brought this change]

  Allow for CMake to use absolute install paths (#486)

  Generated libcares.pc could have bad paths when using absolute paths.

  Fix By: Jonathan Ringer (@jonringer)

- [Thomas Dreibholz brought this change]

  Fix for issue #488: ensure that the number of iovec entries does not exceed system limits. (#489)

  c-ares could try to exceed maximum number of iovec entries supported by system.

  Fix By: Thomas Dreibholz (@dreibh)

- [bsergean brought this change]

  Add include guards to ares_data.h (#491)

  All the other header files in the src/lib folder do have an include guard so it look like an overthought.

  Fix By: @bsergean

- [Brad Spencer brought this change]

  Fix typo in docs for ares_process_fd (#490)

  A single letter was missing

  Fix By: Brad Spencer (@b-spencer)

- [lifenjoiner brought this change]

  tools: refine help (#481)

  fix invalid help options and documentation typos

  Fix By: @lifenjoiner

- [lifenjoiner brought this change]

  Git: ignore CMake temporary files (#480)

  exclude more files from git

  Fix By: @lifenjoiner

- [lifenjoiner brought this change]

  adig: fix `-T` option (#479)

  Helper was missing flag to enable TCP mode of operation.

  Fix By: @lifenjoiner

- [Frank brought this change]

  Add vcpkg installation instructions (#478)

  Update to include vcpkg installation instructions

  Fix By: @FrankXie05

- [marc-groundctl brought this change]

  Convert total timeout to per-query (#467)

  On Apple platforms, libresolv reports the total timeout in retrans, not the per-query time. This patch undoes that math to get the per-query time, which is what c-ares expects. This is not perfect because libresolv is inconsistent on whether the timeout is multiplied by retry or retry+1, but I don't see any way to distinguish these cases.

  Fix By: Marc Aldorasi (@marc-groundctl)

- [marc-groundctl brought this change]

  Don't include version info in the static library (#468)

  The static library should not contain version info, since it would be linked into an executable or dll with its own version info.

  Fix By: @marc-groundctl

- [Ridge Kennedy brought this change]

  Fix ares_getaddrinfo() numerical address fast path with AF_UNSPEC (#469)

  The conversion of numeric IPv4 addresses in fake_addrinfo() is broken when
  the family is AF_UNSPEC. The initial call to ares_inet_pton with AF_INET
  will succeed, but the subsequent call using AF_INET6 will fail. This results
  in the fake_addrinfo() fast path failing, and ares_getaddrinfo() making a
  query when none should be required.

  Resolve this by only attempting the call to ares_inet_pton with AF_INET6
  if the initial call with AF_INET was unsuccessful.

  Fix By: Ridge Kennedy (@ridgek)

- [Manish Mehra brought this change]

  Configurable hosts path for file_lookup (#465)

  This changeset adds support for configurable hosts file
  ARES_OPT_HOSTS_FILE (similar to ARES_OPT_RESOLVCONF).

  Co-authored-by: Manish Mehra (@mmehra)

bradh352 (27 Apr 2022)
- CMake: Windows DLLs lack version information

  The cares.rc was not included in the build for CMake.  Conditionally
  add it when building for Windows.

  Fix By: Brad House (@bradh352)
  Fixes Bug: #460

GitHub (27 Apr 2022)
- [Kai Pastor brought this change]

  CMake: Guard target creation in exported config (#464)

  User projects may call 'find_package(c-ares)' multiple times (e.g.
  via dependencies), but targets must be created only once.
  Shared and static target must be treated independently.

  Fix By: Kai Pastor (@dg0yt)

bradh352 (27 Apr 2022)
- Honor valid DNS result even if other class returned an error

  When using ares_getaddrinfo() with PF_UNSPEC, if a DNS server returned
  good data on an A record, followed by bad data on an AAAA record, the
  good record would be thrown away and an error returned.

  If we got a good response from one of the two queries, regardless of
  the order returned, we should honor that.

  Fix By: Dmitry Karpov ([email protected])
  Signed Off By: Brad House (@bradh352)

GitHub (2 Apr 2022)
- [Sam James brought this change]

  configure.ac: fix STDC_HEADERS typo (#459)

  There is no autoconf macro called STDC_HEADERS. AC_HEADER_STDC however does
  exist and it defines the STDC_HEADERS macro for use.

  Not clear that STDC_HEADERS from its use in the repo is needed but
  would rather not meddle with it for now.

  Fixes an annoying warning on `./configure`:
  ```
  /var/tmp/portage/net-dns/c-ares-1.18.1/work/c-ares-1.18.1/configure: 24546: STDC_HEADERS: not found
  ```

  Signed-off-by: Sam James <[email protected]>

bradh352 (2 Mar 2022)
- Asterisks should be allowed in host validation as CNAMEs may reference wildcard domains

  CloudFlare appears to use this logic in CNAMEs as per
  nodejs/node#42171

  Fixes: #457
  Fix By: Brad House (@bradh352)

- Don't return on file lookup failure, set status

  When resolving a host via /etc/hosts, don't return with a predefined
  error as there may be other tries.

  Fix By: Brad House (@bradh352)

- 'localhost' special treatment enhancement

  Since localhost is special-cased, any errors should be ignored when
  reading /etc/hosts as otherwise we could return an error if there
  were for instance an invalidly formatted /etc/hosts or if /etc/hosts
  had a permissions error while reading.

  This exact behavior appears to have been seen on OS/400 PASE
  environments which allows AIX binares to run.

  Fix By: Brad House (@bradh352)

- If chain building c-ares as part of another project, detect of res_servicename could fail (#451)

  If libresolv is already included with the build, c-ares wouldn't properly detect its use.

  May fix: #451
  Fix by: Brad House (@bradh352)

- no analyze capability on ios

- attempt to use scan-build on ios

- disable tests on ios

- fix switch statement

- code coverage had gotten disabled

- looks like shell expansion doesn't work with cirrus-ci, lets do it another way

- attempt to autobuild for iOS

GitHub (8 Dec 2021)
- [Brad House brought this change]

  Windows: rework/simplify initialization code, drop long EOL systems (#445)

  There was a lot of windows initialization code specific to the era that predates Windows Vista such as reading DNS configuration from the registry, and dynamically loading libraries to get access to functions that didn't exist in XP or earlier releases.

  Vista was released in January 2007, and was EOL'd in 2017, and support for Vista is still maintained with this patch set.

  XP was EOL'd in Apr 8 2014.

  I believe the last OS based on something earlier than Vista was POSReady 2009, as it was XP based for some reason, and that was EOL'd in January 2019. Considering any POS system falls under the PCI-DSS rules, they aren't allow to run POSReady 2009 any more so there is no reason to try to continue supporting such systems.

  We have also targeted with our build system Vista support for the last few years, and while developers could change the target, we haven't had any reports that they have.

bradh352 (9 Nov 2021)
- Fix memory leak in reading /etc/hosts

  When an /etc/hosts lookup is performed, but fails with ENOTFOUND, and
  a valid RFC6761 Section 6.3 fallback is performed, it could overwrite
  variables that were already set and therefore leave the pointers
  dangling, never to be cleaned up.

  Clean up explicitly on ENOTFOUND when returning from the file parser.

  Fixes: #439
  Fix By: Brad House (@bradh352)

GitHub (2 Nov 2021)
- [Bobby Reynolds brought this change]

  Fix cross-compilation from Windows to Linux due to CPACK logic (#436)

  When determining value for CPACK_PACKAGE_ARCHITECTURE, prefer to use
  value from CMAKE_SYSTEM_PROCESSOR before falling back to uname output.

  Additionally, if building from a Windows host, emit a fatal error
  instead of attempting to call uname.

  Fix By: Bobby Reynolds (@reynoldsbd)

bradh352 (1 Nov 2021)
- fix coveralls link

- coveralls needs token

- coveralls appears to require git

- fix a couple of coveralls vars

- more coveralls fixes

- add code coverage libs to LDADD instead of _LIBS

- make verbose

- try to fix code coverage building

- need -y for install

- try to fix asan/ubsan/lsan when built with clang. try to support code coverage properly.

- try another path

- fix pip

- attempt to enable some other build types that travis supported
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dns Issues and PRs related to the dns subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants