From a3a123bcf377edff9aca191cc2ece37c30e7ca71 Mon Sep 17 00:00:00 2001 From: Valentin Gosu Date: Thu, 1 Oct 2020 10:05:03 +0000 Subject: [PATCH] Bug 1667975 - Don't check rcode before parsing packet r=dragana,necko-reviewers Differential Revision: https://phabricator.services.mozilla.com/D92040 --- netwerk/dns/TRR.cpp | 4 +-- netwerk/dns/nsHostResolver.cpp | 10 +++++-- netwerk/dns/nsHostResolver.h | 1 + netwerk/test/unit/head_trr.js | 10 ++++++- netwerk/test/unit/test_trr_extended_error.js | 30 ++++++++++++++++++++ testing/xpcshell/dns-packet/index.js | 1 + 6 files changed, 51 insertions(+), 5 deletions(-) diff --git a/netwerk/dns/TRR.cpp b/netwerk/dns/TRR.cpp index 2db16c1c88f6..b32fb849230c 100644 --- a/netwerk/dns/TRR.cpp +++ b/netwerk/dns/TRR.cpp @@ -823,9 +823,9 @@ nsresult TRR::DohDecode(nsCString& aHost) { return NS_ERROR_ILLEGAL_VALUE; } uint8_t rcode = mResponse[3] & 0x0F; + LOG(("TRR Decode %s RCODE %d\n", aHost.get(), rcode)); if (rcode) { - LOG(("TRR Decode %s RCODE %d\n", aHost.get(), rcode)); - return NS_ERROR_FAILURE; + RecordReason(nsHostRecord::TRR_RCODE_FAIL); } uint16_t questionRecords = get16bit(mResponse, 4); // qdcount diff --git a/netwerk/dns/nsHostResolver.cpp b/netwerk/dns/nsHostResolver.cpp index 00542702a441..1a9235837021 100644 --- a/netwerk/dns/nsHostResolver.cpp +++ b/netwerk/dns/nsHostResolver.cpp @@ -1473,6 +1473,7 @@ nsresult nsHostResolver::NativeLookup(nsHostRecord* aRec) { if (StaticPrefs::network_dns_disabled()) { return NS_ERROR_UNKNOWN_HOST; } + LOG(("NativeLookup host:%s af:%" PRId16, aRec->host.get(), aRec->af)); // Only A/AAAA request are resolve natively. MOZ_ASSERT(aRec->IsAddrRecord()); @@ -2023,7 +2024,6 @@ nsHostResolver::LookupStatus nsHostResolver::CompleteLookup( if (!addrRec->mTRRSuccess) { // no TRR success newRRSet = nullptr; - status = NS_ERROR_UNKNOWN_HOST; // At least one of them was a failure. If the IPv4 response has a // recorded reason, we use that (we also care about ipv4 more). @@ -2041,7 +2041,8 @@ nsHostResolver::LookupStatus nsHostResolver::CompleteLookup( if (!addrRec->mTRRSuccess && addrRec->mEffectiveTRRMode == nsIRequest::TRR_FIRST_MODE && - addrRec->mFirstTRRresult != NS_ERROR_DEFINITIVE_UNKNOWN_HOST) { + addrRec->mFirstTRRresult != NS_ERROR_DEFINITIVE_UNKNOWN_HOST && + status != NS_ERROR_DEFINITIVE_UNKNOWN_HOST) { MOZ_ASSERT(!addrRec->mResolving); NativeLookup(addrRec); MOZ_ASSERT(addrRec->mResolving); @@ -2053,6 +2054,11 @@ nsHostResolver::LookupStatus nsHostResolver::CompleteLookup( newRRSet = mNCS->MapNAT64IPs(newRRSet); } + if (NS_FAILED(status)) { + // This is the error that consumers expect. + status = NS_ERROR_UNKNOWN_HOST; + } + // continue } diff --git a/netwerk/dns/nsHostResolver.h b/netwerk/dns/nsHostResolver.h index a92e279ce20d..4424d8da5a68 100644 --- a/netwerk/dns/nsHostResolver.h +++ b/netwerk/dns/nsHostResolver.h @@ -120,6 +120,7 @@ class nsHostRecord : public mozilla::LinkedListElement>, TRR_DECODE_FAILED = 25, // DohDecode failed TRR_EXCLUDED = 26, // ExcludedFromTRR TRR_SERVER_RESPONSE_ERR = 27, // Server responded with non-200 code + TRR_RCODE_FAIL = 28, // DNS response contains a non-NOERROR rcode }; // Records the first reason that caused TRR to be skipped or to fail. diff --git a/netwerk/test/unit/head_trr.js b/netwerk/test/unit/head_trr.js index 2e17dac60b46..bc488ec6cc0d 100644 --- a/netwerk/test/unit/head_trr.js +++ b/netwerk/test/unit/head_trr.js @@ -269,10 +269,18 @@ function trrQueryHandler(req, resp, url) { `${dnsQuery.questions[0].name}/${dnsQuery.questions[0].type}` ] || {}; + let flags = global.dnsPacket.RECURSION_DESIRED; + if ( + (!response.answers || !response.answers.length) && + response.additionals && + response.additionals.length > 0 + ) { + flags |= global.dnsPacket.rcodes.toRcode("SERVFAIL"); + } let buf = global.dnsPacket.encode({ type: "response", id: dnsQuery.id, - flags: global.dnsPacket.RECURSION_DESIRED, + flags, questions: dnsQuery.questions, answers: response.answers || [], additionals: response.additionals || [], diff --git a/netwerk/test/unit/test_trr_extended_error.js b/netwerk/test/unit/test_trr_extended_error.js index c9ec16078b8d..4bfe237383b4 100644 --- a/netwerk/test/unit/test_trr_extended_error.js +++ b/netwerk/test/unit/test_trr_extended_error.js @@ -306,3 +306,33 @@ add_task(async function delayed_ipv4_answer_and_ipv6_error() { // Check that we don't fall back to DNS await new TRRDNSListener("delay4.com", { expectedAnswer: "1.2.3.4" }); }); + +add_task(async function test_only_ipv4_extended_error() { + Services.prefs.setBoolPref("network.dns.disableIPv6", true); + await trrServer.registerDoHAnswers( + "only.com", + "A", + [], + [ + { + name: ".", + type: "OPT", + class: "IN", + options: [ + { + code: "EDNS_ERROR", + extended_error: 17, // Filtered + text: "Filtered", + }, + ], + }, + ] + ); + let [, , inStatus] = await new TRRDNSListener("only.com", { + expectedSuccess: false, + }); + Assert.ok( + !Components.isSuccessCode(inStatus), + `${inStatus} should be an error code` + ); +}); diff --git a/testing/xpcshell/dns-packet/index.js b/testing/xpcshell/dns-packet/index.js index 09aa0e4fd16a..263b3f08f6a1 100644 --- a/testing/xpcshell/dns-packet/index.js +++ b/testing/xpcshell/dns-packet/index.js @@ -2,6 +2,7 @@ const types = require('./types') const rcodes = require('./rcodes') +exports.rcodes = rcodes; const opcodes = require('./opcodes') const classes = require('./classes') const optioncodes = require('./optioncodes')