dns.resolveSoa returns EBADRESP if hostname has a CNAME record

[jlchristi]

• Version(s): v12.16.3 / v12.18.3 / v12.18.0
• Platform: Windows / Windows Subsystem for Linux (Debian) / Red Hat Enterprise Linux Server release 7.8
• Subsystem: DNS

What steps will reproduce the bug?

const dns = require('dns');
const resolver = new dns.promises.Resolver();

// **************************************************************

async function test(hostname){
 let data = '';

 console.log('hostname:', hostname);

 try{
 data = await resolver.resolveCname(hostname);
 console.log('CNAME result:', data);
 } catch (error){
 console.log('CNAME result:', error.message);
 }

 try{
 data = await resolver.resolveSoa(hostname);
 console.log('SOA result:', JSON.stringify(data));
 } catch (error){
 console.log('SOA result:', error.message);
 }

 console.log();
}

// **************************************************************

(async function main(){
 await test('support.microsoft.com');
})();

Actual Results

hostname: support.microsoft.com
CNAME result: [ 'ev.support.microsoft.com.edgekey.net' ]
SOA result: querySoa EBADRESP support.microsoft.com

Expected Results

hostname: support.microsoft.com
CNAME result: [ 'ev.support.microsoft.com.edgekey.net' ]
SOA result: querySoa ENODATA support.microsoft.com

Additional information

This seems to happen for any hostname with a CNAME record.

Another example:

hostname: store.gocomics.com
CNAME result: [ 'gocomicsstore.wpengine.com' ]
SOA result: querySoa EBADRESP store.gocomics.com

I would expect to get an 'ENODATA' instead of 'EBADRESP', as with the other resolveXXX() calls.

For a hostname with an SOA record but no CNAME, you get:

hostname: microsoft.com
CNAME result: queryCname ENODATA microsoft.com
SOA result:{"nsname":"ns1-205.azure-dns.com","hostmaster":"azuredns-
hostmaster.microsoft.com","serial":1,"refresh":3600,"retry":300,"expire":2419200,"minttl":300}

[bnoordhuis]

Thanks for the bug report. At first glance this appears to be a bug in our response parsing logic.

---

node/src/cares_wrap.cc

Lines 1078 to 1080 in 74df749

	if (ptr + temp_len + NS_QFIXEDSZ > buf + len){
	return ARES_EBADRESP;
	}

@addaleax @XadillaX Maybe not the root cause but those bounds checks look like UB to me.

Pointers in C and C++ are allowed to point one element past the end of an array and no more. The check should look like this:

diff --git a/src/cares_wrap.cc b/src/cares_wrap.cc
index 73a0ac6b33..778e0821d7 100644
--- a/src/cares_wrap.cc
+++ b/src/cares_wrap.cc
@@ -1075,7 +1075,7 @@ int ParseSoaReply(Environment* env,
 return status == ARES_EBADNAME ? ARES_EBADRESP : status;
 }
 
- if (ptr + temp_len + NS_QFIXEDSZ > buf + len){
+ if (temp_len > LONG_MAX - NS_QFIXEDSZ || temp_len + NS_QFIXEDSZ > len){
 return ARES_EBADRESP;
 }
 ptr += temp_len + NS_QFIXEDSZ;

[AasthaGupta]

@bnoordhuis I'd like to work on this.

[AasthaGupta]

@bnoordhuis -
I tried tracing down function calls and I don't think the problem exists in parseSoaReply. Instead, the Parse function of QuerySoaWrap is invoked.

This Parse function handles the parsing when the ares_query invokes its callback with a success status.
In the case when SOA response doesn't exist, the ares_query function invokes the callback with a success status instead of ENODATA.

At this point, I think the problem exists in the ares_query library function itself.

Also while working on this, I discovered a bug related to free call on garbage pointer. #35502

[nordluf]

v20.11.0 the bug is still there

[santigimeno]

This issue (if so) comes indeed from this condition in cares and should be reported upstream in case they want to change it. Should we keep this open or just close it?