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

@@ -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));
+    RecordReason(nsHostRecord::TRR_RCODE_FAIL);
-    return NS_ERROR_FAILURE;
   }
 
   uint16_t questionRecords = get16bit(mResponse, 4);  // qdcount

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
     }
 

netwerk/dns/nsHostResolver.h

@@ -120,6 +120,7 @@ class nsHostRecord : public mozilla::LinkedListElement<RefPtr<nsHostRecord>>,
     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.

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 || [],

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`
+  );
+});

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')