Bug 1779110 - Do not follow CNAME for NODATA records r=necko-reviewers,kershaw

If the response has the RA flag set, that means the recursive
resolver has probably followed the CNAME chain already, so
issuing other requests would be pointless.

Differential Revision: https://phabricator.services.mozilla.com/D153512

netwerk/dns/DNSPacket.cpp

@@ -490,6 +490,15 @@ Result<uint8_t, nsresult> DNSPacket::GetRCode() const {
   return mResponse[3] & 0x0F;
 }
 
+Result<bool, nsresult> DNSPacket::RecursionAvailable() const {
+  if (mBodySize < 12) {
+    LOG(("DNSPacket::GetRCode - packet too small"));
+    return Err(NS_ERROR_ILLEGAL_VALUE);
+  }
+
+  return mResponse[3] & 0x80;
+}
+
 nsresult DNSPacket::DecodeInternal(
     nsCString& aHost, enum TrrType aType, nsCString& aCname, bool aAllowRFC1918,
     DOHresp& aResp, TypeRecordResultType& aTypeResult,

netwerk/dns/DNSPacket.h

@@ -53,6 +53,7 @@ class DNSPacket {
   virtual ~DNSPacket() = default;
 
   Result<uint8_t, nsresult> GetRCode() const;
+  Result<bool, nsresult> RecursionAvailable() const;
 
   // Called in order to feed data into the buffer.
   nsresult OnDataAvailable(nsIRequest* aRequest, nsIInputStream* aInputStream,

netwerk/dns/TRR.cpp

@@ -794,6 +794,19 @@ void TRR::HandleDecodeError(nsresult aStatusCode) {
   }
 }
 
+bool TRR::HasUsableResponse() {
+  if (mType == TRRTYPE_A || mType == TRRTYPE_AAAA) {
+    return !mDNS.mAddresses.IsEmpty();
+  }
+  if (mType == TRRTYPE_TXT) {
+    return mResult.is<TypeRecordTxt>();
+  }
+  if (mType == TRRTYPE_HTTPSSVC) {
+    return mResult.is<TypeRecordHTTPSSVC>();
+  }
+  return false;
+}
+
 nsresult TRR::FollowCname(nsIChannel* aChannel) {
   nsresult rv = NS_OK;
   nsAutoCString cname;
@@ -819,11 +832,21 @@ nsresult TRR::FollowCname(nsIChannel* aChannel) {
 
   // restore mCname as DohDecode() change it
   mCname = cname;
-  if (NS_SUCCEEDED(rv) && !mDNS.mAddresses.IsEmpty()) {
+  if (NS_SUCCEEDED(rv) && HasUsableResponse()) {
     ReturnData(aChannel);
     return NS_OK;
   }
 
+  bool ra = mPacket && mPacket->RecursionAvailable().unwrapOr(false);
+  LOG(("ra = %d", ra));
+  if (rv == NS_ERROR_UNKNOWN_HOST && ra) {
+    // If recursion is available, but no addresses have been returned,
+    // we can just return a failure here.
+    LOG(("TRR::FollowCname not sending another request as RA flag is set."));
+    FailData(NS_ERROR_UNKNOWN_HOST);
+    return NS_OK;
+  }
+
   if (!mCnameLoop) {
     LOG(("TRR::On200Response CNAME loop, eject!\n"));
     return NS_ERROR_REDIRECT_LOOP;

netwerk/dns/TRR.h

@@ -105,6 +105,8 @@ class TRR : public Runnable,
   nsresult On200Response(nsIChannel* aChannel);
   nsresult FollowCname(nsIChannel* aChannel);
 
+  bool HasUsableResponse();
+
   bool UseDefaultServer();
   void SaveAdditionalRecords(
       const nsClassHashtable<nsCStringHashKey, DOHresp>& aRecords);

netwerk/test/unit/test_trr_cname_chain.js

@@ -7,11 +7,7 @@
 const dns = Cc["@mozilla.org/network/dns-service;1"].getService(
   Ci.nsIDNSService
 );
-
-trr_test_setup();
-registerCleanupFunction(async () => {
-  trr_clear_prefs();
-});
+let trrServer;
 
 function makeChan(url) {
   let chan = NetUtil.newChannel({
@@ -30,8 +26,13 @@ function channelOpenPromise(chan) {
   });
 }
 
-add_task(async function test_follow_cnames_same_response() {
-  let trrServer = new TRRServer();
+add_setup(async function setup() {
+  trr_test_setup();
+  registerCleanupFunction(async () => {
+    trr_clear_prefs();
+  });
+
+  trrServer = new TRRServer();
   registerCleanupFunction(async () => {
     await trrServer.stop();
   });
@@ -47,7 +48,9 @@ add_task(async function test_follow_cnames_same_response() {
     "network.trr.uri",
     `https://foo.example.com:${trrServer.port}/dns-query`
   );
+});
 
+add_task(async function test_follow_cnames_same_response() {
   await trrServer.registerDoHAnswers("something.foo", "A", {
     answers: [
       {
@@ -109,6 +112,123 @@ add_task(async function test_follow_cnames_same_response() {
     ],
   });
   await new TRRDNSListener("a.foo", { expectedAnswer: "2.3.4.5" });
-
-  await trrServer.stop();
+});
+
+add_task(async function test_cname_nodata() {
+  // Test that we don't needlessly follow cname chains when the RA flag is set
+  // on the response.
+
+  await trrServer.registerDoHAnswers("first.foo", "A", {
+    flags: 0x80,
+    answers: [
+      {
+        name: "first.foo",
+        ttl: 55,
+        type: "CNAME",
+        flush: false,
+        data: "second.foo",
+      },
+      {
+        name: "second.foo",
+        ttl: 55,
+        type: "A",
+        flush: false,
+        data: "1.2.3.4",
+      },
+    ],
+  });
+  await trrServer.registerDoHAnswers("first.foo", "AAAA", {
+    flags: 0x80,
+    answers: [
+      {
+        name: "first.foo",
+        ttl: 55,
+        type: "CNAME",
+        flush: false,
+        data: "second.foo",
+      },
+    ],
+  });
+
+  await new TRRDNSListener("first.foo", { expectedAnswer: "1.2.3.4" });
+  equal(await trrServer.requestCount("first.foo", "A"), 1);
+  equal(await trrServer.requestCount("first.foo", "AAAA"), 1);
+  equal(await trrServer.requestCount("second.foo", "A"), 0);
+  equal(await trrServer.requestCount("second.foo", "AAAA"), 0);
+
+  await trrServer.registerDoHAnswers("first.bar", "A", {
+    answers: [
+      {
+        name: "first.bar",
+        ttl: 55,
+        type: "CNAME",
+        flush: false,
+        data: "second.bar",
+      },
+      {
+        name: "second.bar",
+        ttl: 55,
+        type: "A",
+        flush: false,
+        data: "1.2.3.4",
+      },
+    ],
+  });
+  await trrServer.registerDoHAnswers("first.bar", "AAAA", {
+    answers: [
+      {
+        name: "first.bar",
+        ttl: 55,
+        type: "CNAME",
+        flush: false,
+        data: "second.bar",
+      },
+    ],
+  });
+
+  await new TRRDNSListener("first.bar", { expectedAnswer: "1.2.3.4" });
+  equal(await trrServer.requestCount("first.bar", "A"), 1);
+  equal(await trrServer.requestCount("first.bar", "AAAA"), 1);
+  equal(await trrServer.requestCount("second.bar", "A"), 0); // addr included in first response
+  equal(await trrServer.requestCount("second.bar", "AAAA"), 1); // will follow cname because no flag is set
+
+  // Check that it also works for HTTPS records
+
+  await trrServer.registerDoHAnswers("first.bar", "HTTPS", {
+    flags: 0x80,
+    answers: [
+      {
+        name: "second.bar",
+        ttl: 55,
+        type: "HTTPS",
+        flush: false,
+        data: {
+          priority: 1,
+          name: "h3pool",
+          values: [
+            { key: "alpn", value: ["h2", "h3"] },
+            { key: "no-default-alpn" },
+            { key: "port", value: 8888 },
+            { key: "ipv4hint", value: "1.2.3.4" },
+            { key: "echconfig", value: "123..." },
+            { key: "ipv6hint", value: "::1" },
+          ],
+        },
+      },
+      {
+        name: "first.bar",
+        ttl: 55,
+        type: "CNAME",
+        flush: false,
+        data: "second.bar",
+      },
+    ],
+  });
+
+  let { inStatus } = await new TRRDNSListener("first.bar", {
+    type: dns.RESOLVE_TYPE_HTTPSSVC,
+  });
+  Assert.ok(Components.isSuccessCode(inStatus), `${inStatus} should work`);
+  equal(await trrServer.requestCount("first.bar", "HTTPS"), 1);
+  equal(await trrServer.requestCount("second.bar", "HTTPS"), 0);
 });