Bug 1714182 - Don't fallback from DoH to native in cases of request failure. r=necko-reviewers,dragana

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

modules/libpref/init/StaticPrefList.yaml

@@ -9649,6 +9649,11 @@
   value: "https://mozilla.cloudflare-dns.com/dns-query"
   mirror: never
 
+- name: network.trr.strict_native_fallback
+  type: RelaxedAtomicBool
+  value: false
+  mirror: always
+
 # Single TRR request timeout, in milliseconds
 - name: network.trr.request_timeout_ms
   type: RelaxedAtomicUint32

netwerk/dns/nsHostResolver.cpp

@@ -1338,9 +1338,14 @@ nsHostResolver::LookupStatus nsHostResolver::CompleteLookupLocked(
       addrRec->RecordReason(TRRSkippedReason::TRR_OK);
     }
 
+    bool shouldAttemptNative =
+        !StaticPrefs::network_trr_strict_native_fallback() ||
+        aReason == TRRSkippedReason::TRR_NXDOMAIN ||
+        aReason == TRRSkippedReason::TRR_DISABLED_FLAG;
+
     if (NS_FAILED(status) &&
         addrRec->mEffectiveTRRMode == nsIRequest::TRR_FIRST_MODE &&
-        status != NS_ERROR_DEFINITIVE_UNKNOWN_HOST) {
+        status != NS_ERROR_DEFINITIVE_UNKNOWN_HOST && shouldAttemptNative) {
       MOZ_ASSERT(!addrRec->mResolving);
       NativeLookup(addrRec, aLock);
       MOZ_ASSERT(addrRec->mResolving);

netwerk/test/unit/test_odoh.js

@@ -217,6 +217,8 @@ add_task(test_GET_ECS);
 
 add_task(test_timeout_mode3);
 
+add_task(test_strict_native_fallback);
+
 add_task(test_no_answers_fallback);
 
 add_task(test_404_fallback);

netwerk/test/unit/test_trr.js

@@ -149,6 +149,8 @@ add_task(test_GET_ECS);
 
 add_task(test_timeout_mode3);
 
+add_task(test_strict_native_fallback).only();
+
 add_task(test_no_answers_fallback);
 
 add_task(test_404_fallback);

netwerk/test/unit/trr_common.js

@@ -225,6 +225,80 @@ async function test_timeout_mode3() {
   Services.prefs.clearUserPref("network.trr.request_timeout_mode_trronly_ms");
 }
 
+async function test_strict_native_fallback() {
+  dns.clearCache(true);
+  Services.prefs.setBoolPref("network.trr.strict_native_fallback", true);
+
+  info("First a timeout case");
+  setModeAndURI(2, "doh?noResponse=true");
+  Services.prefs.setIntPref("network.trr.request_timeout_ms", 10);
+  Services.prefs.setIntPref("network.trr.request_timeout_mode_trronly_ms", 10);
+
+  let [, , inStatus] = await new TRRDNSListener(
+    "timeout.example.com",
+    undefined,
+    false
+  );
+  Assert.ok(
+    !Components.isSuccessCode(inStatus),
+    `${inStatus} should be an error code`
+  );
+
+  info("Now a connection error");
+  dns.clearCache(true);
+  setModeAndURI(2, "doh?responseIP=2.2.2.2");
+  Services.prefs.clearUserPref("network.trr.request_timeout_ms");
+  Services.prefs.clearUserPref("network.trr.request_timeout_mode_trronly_ms");
+  [, , inStatus] = await new TRRDNSListener("closeme.com", undefined, false);
+  Assert.ok(
+    !Components.isSuccessCode(inStatus),
+    `${inStatus} should be an error code`
+  );
+
+  info("Now a decode error");
+  dns.clearCache(true);
+  setModeAndURI(2, "doh?responseIP=2.2.2.2&corruptedAnswer=true");
+  [, , inStatus] = await new TRRDNSListener(
+    "bar.example.com",
+    undefined,
+    false
+  );
+  Assert.ok(
+    !Components.isSuccessCode(inStatus),
+    `${inStatus} should be an error code`
+  );
+
+  info("Now a successful case.");
+  dns.clearCache(true);
+  setModeAndURI(2, "doh?responseIP=2.2.2.2");
+  await new TRRDNSListener("bar.example.com", "2.2.2.2");
+
+  info("Now without strict fallback mode, timeout case");
+  dns.clearCache(true);
+  setModeAndURI(2, "doh?noResponse=true");
+  Services.prefs.setIntPref("network.trr.request_timeout_ms", 10);
+  Services.prefs.setIntPref("network.trr.request_timeout_mode_trronly_ms", 10);
+  Services.prefs.setBoolPref("network.trr.strict_native_fallback", false);
+
+  await new TRRDNSListener("timeout.example.com", "127.0.0.1"); // Should fallback
+
+  info("Now a connection error");
+  dns.clearCache(true);
+  setModeAndURI(2, "doh?responseIP=2.2.2.2");
+  Services.prefs.clearUserPref("network.trr.request_timeout_ms");
+  Services.prefs.clearUserPref("network.trr.request_timeout_mode_trronly_ms");
+  await new TRRDNSListener("closeme.com", "127.0.0.1"); // Should fallback
+
+  info("Now a decode error");
+  dns.clearCache(true);
+  setModeAndURI(2, "doh?responseIP=2.2.2.2&corruptedAnswer=true");
+  await new TRRDNSListener("bar.example.com", "127.0.0.1"); // Should fallback
+
+  Services.prefs.clearUserPref("network.trr.strict_native_fallback");
+  Services.prefs.clearUserPref("network.trr.request_timeout_ms");
+  Services.prefs.clearUserPref("network.trr.request_timeout_mode_trronly_ms");
+}
+
 async function test_no_answers_fallback() {
   info("Verfiying that we correctly fallback to Do53 when no answers from DoH");
   dns.clearCache(true);

testing/xpcshell/moz-http2/moz-http2.js

@@ -346,6 +346,13 @@ function handleRequest(req, res) {
       return null;
     }
 
+    if (u.query.corruptedAnswer) {
+      // DNS response header is 12 bytes, we check for this minimum length
+      // at the start of decoding so this is the simplest way to force
+      // a decode error.
+      return "<12bytes";
+    }
+
     function responseData() {
       if (
         packet.questions.length > 0 &&