1652427 - Pref for Including DHCP suffixes to TRR DoH

Status: RESOLVED FIXED

(Core :: Networking: DNS, enhancement, P3)

Description

[mcccs]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0) Gecko/20100101 Firefox/78.0

Steps to reproduce:

Related to: Bug 1642623
Connected to Wifi with DHCP, betrayed by TRR.

Actual results:

https://hg.mozilla.org/mozilla-central/rev/e8c352c1055e Excludes DHCP Search Suffixes from TRR. System resolver is used

Expected results:

In about:config there should be net.trr.enforce_all that keeps Firefox away from the system resolver. Its effects should be:

• Domains with DHCP suffixes are resolved with TRR DoH
• net.trr.excluded-domains is ignored

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug should belong to this component, but please revert this change in case of error.

Comment 2

[Junior [inactive]]

What do you think, Valentin?

Comment 3

[mcccs]

Hi @valentin,

Firstly thank you for assigning yourself, in addition to confirming it!

Secondly, I noticed that I couldn't define clearly the problem in the original report, so I'll rephrase it:

When the users set the network.trr.mode to 3, they expect that only TRR is used when they make network requests.
What I propose adding is a "pedantic mode", disabled by default. With it enabled, FF should not make any unencrypted DNS queries but fail
to connect instead of continuing with system DNS, to avoid giving users a false impression of "network.trr.mode: 3 means TRR-only!
Everything is encrypted and private!🌈". Due to some factors (IsExcludedFromTRR), sometimes it's fine to use the system DNS.
But if their network is misconfigured, they might be unknowingly always using the system DNS. With this pref, it becomes possible
to detect DNS leakages and misconfigurations (e.g. in Windows Registry.) by turning DNS leaks into connection errors.

Likewise, related to: https://redd.it/h8v5sh People actually believed network.trr.mode is TRR only. This line (https://searchfox.org/mozilla-central/rev/dcd9c2d2bc19d96d487825eb70c2333a4d60994e/modules/libpref/init/all.js#4052) is misleading, but it's the perfect balance between safety and convenience. This pref is the fix.

Lastly, is the patch below OK?

diff -r 73ee67c9a90d modules/libpref/init/all.js
--- a/modules/libpref/init/all.js	Tue Jul 21 11:42:44 2020 +0000
+++ b/modules/libpref/init/all.js	Thu Jul 23 12:45:03 2020 +0000
@@ -4094,6 +4094,9 @@
 // When true, the DNS+TRR cache will be cleared when a relevant TRR pref
 // changes. (uri, bootstrapAddress, excluded-domains)
 pref("network.trr.clear-cache-on-pref-change", true);
+// When used with mode 3, never use the system resolver as a fallback
+// for normal requests.
+pref("network.trr.disable-system-resolver", false);
 
 pref("captivedetect.canonicalURL", "http://detectportal.firefox.com/success.txt");
 pref("captivedetect.canonicalContent", "success\n");
diff -r 73ee67c9a90d netwerk/dns/TRRService.cpp
--- a/netwerk/dns/TRRService.cpp	Tue Jul 21 11:42:44 2020 +0000
+++ b/netwerk/dns/TRRService.cpp	Thu Jul 23 12:45:03 2020 +0000
@@ -61,7 +61,8 @@
       mConfirmationState(CONFIRM_INIT),
       mRetryConfirmInterval(1000),
       mTRRFailures(0),
-      mParentalControlEnabled(false) {
+      mParentalControlEnabled(false),
+      mDisableSystemResolver(false) {
   MOZ_ASSERT(NS_IsMainThread(), "wrong thread");
 }
 
@@ -350,6 +351,12 @@
       mDisableECS = tmp;
     }
   }
+  if (!name || !strcmp(name, TRR_PREF("disable-system-resolver"))) {
+    bool tmp;
+    if (NS_SUCCEEDED(Preferences::GetBool(TRR_PREF("disable-system-resolver"), &tmp))) {
+      mDisableSystemResolver = tmp;
+    }
+  }
   if (!name || !strcmp(name, TRR_PREF("max-fails"))) {
     uint32_t fails;
     if (NS_SUCCEEDED(Preferences::GetUint(TRR_PREF("max-fails"), &fails))) {
@@ -735,6 +742,10 @@
     mLock.AssertCurrentThreadOwns();
   }
 
+  if (mDisableSystemResolver) {
+    return false;
+  }
+
   if (mPlatformDisabledTRR) {
     LOG(("%s is excluded from TRR because of platform indications",
          aHost.BeginReading()));
diff -r 73ee67c9a90d netwerk/dns/TRRService.h
--- a/netwerk/dns/TRRService.h	Tue Jul 21 11:42:44 2020 +0000
+++ b/netwerk/dns/TRRService.h	Thu Jul 23 12:45:03 2020 +0000
@@ -49,6 +49,7 @@
   bool SkipTRRWhenParentalControlEnabled() {
     return mSkipTRRWhenParentalControlEnabled;
   }
+  bool DisableSystemResolver() { return mDisableSystemResolver; }
   nsresult GetURI(nsACString& result);
   nsresult GetCredentials(nsCString& result);
   uint32_t GetRequestTimeout();
@@ -136,6 +137,7 @@
   Atomic<uint32_t, Relaxed>
       mDisableAfterFails;  // this many fails in a row means failed TRR service
   Atomic<bool, Relaxed> mPlatformDisabledTRR;
+  Atomic<bool, Relaxed> mDisableSystemResolver;
 
   // TRR Blocklist storage
   // mTRRBLStorage is only modified on the main thread, but we query whether it

Comment 4

[mcccs]

I have remade the patch. Now it honors excluded-domains.

(But @valentin please review my other patch first, it fixes high latency IPv4 for those running IPv6 on DS-Lite such as Bug 1538480 : patch at https://bugzilla.mozilla.org/show_bug.cgi?id=1623302#c4 and it would increase global IPv6 usage statistics. That only needs clang-format then it doesn't seem to me that further design improvements are possible)

Does network.trr.disable-system-resolver have to be in sync with mDisableSystemResolver? Every time mDisableSystemResolver's value is read, it's read as !(mDisableSystemResolver && mMode == MODE_TRRONLY), so may I change the pref reader (adding mMode == MODE_TRRONLY):

+    if (mMode == MODE_TRRONLY && NS_SUCCEEDED(
+            Preferences::GetBool(TRR_PREF("disable-system-resolver"), &tmp))) {
+      mDisableSystemResolver = tmp;
+    }

New diff:

diff -r 73ee67c9a90d modules/libpref/init/all.js
--- a/modules/libpref/init/all.js	Tue Jul 21 11:42:44 2020 +0000
+++ b/modules/libpref/init/all.js	Fri Jul 24 11:08:26 2020 +0000
@@ -4094,6 +4094,10 @@
 // When true, the DNS+TRR cache will be cleared when a relevant TRR pref
 // changes. (uri, bootstrapAddress, excluded-domains)
 pref("network.trr.clear-cache-on-pref-change", true);
+// When used with mode 3, never use the system resolver as a fallback
+// for normal requests, with the only exception of
+// domains specified with `network.trr.excluded-domains`.
+pref("network.trr.disable-system-resolver", false);
 
 pref("captivedetect.canonicalURL", "http://detectportal.firefox.com/success.txt");
 pref("captivedetect.canonicalContent", "success\n");
diff -r 73ee67c9a90d netwerk/dns/TRRService.cpp
--- a/netwerk/dns/TRRService.cpp	Tue Jul 21 11:42:44 2020 +0000
+++ b/netwerk/dns/TRRService.cpp	Fri Jul 24 11:08:26 2020 +0000
@@ -57,6 +57,7 @@
       mSkipTRRWhenParentalControlEnabled(true),
       mDisableAfterFails(5),
       mPlatformDisabledTRR(false),
+      mDisableSystemResolver(false),
       mTRRBLStorage("DataMutex::TRRBlocklist"),
       mConfirmationState(CONFIRM_INIT),
       mRetryConfirmInterval(1000),
@@ -350,6 +351,13 @@
       mDisableECS = tmp;
     }
   }
+  if (!name || !strcmp(name, TRR_PREF("disable-system-resolver"))) {
+    bool tmp;
+    if (NS_SUCCEEDED(
+            Preferences::GetBool(TRR_PREF("disable-system-resolver"), &tmp))) {
+      mDisableSystemResolver = tmp;
+    }
+  }
   if (!name || !strcmp(name, TRR_PREF("max-fails"))) {
     uint32_t fails;
     if (NS_SUCCEEDED(Preferences::GetUint(TRR_PREF("max-fails"), &fails))) {
@@ -735,7 +743,8 @@
     mLock.AssertCurrentThreadOwns();
   }
 
-  if (mPlatformDisabledTRR) {
+  if (mPlatformDisabledTRR &&
+      !(mDisableSystemResolver && mMode == MODE_TRRONLY)) {
     LOG(("%s is excluded from TRR because of platform indications",
          aHost.BeginReading()));
     return true;
@@ -752,8 +761,9 @@
            subdomain.BeginReading(), aHost.BeginReading()));
       return true;
     }
-    if (mDNSSuffixDomains.GetEntry(subdomain)) {
-      LOG(("Subdomain [%s] of host [%s] Is Excluded From TRR via pref\n",
+    if (mDNSSuffixDomains.GetEntry(subdomain) &&
+        !(mDisableSystemResolver && mMode == MODE_TRRONLY)) {
+      LOG(("Subdomain [%s] of host [%s] Is Excluded From TRR dns suffix\n",
            subdomain.BeginReading(), aHost.BeginReading()));
       return true;
     }
diff -r 73ee67c9a90d netwerk/dns/TRRService.h
--- a/netwerk/dns/TRRService.h	Tue Jul 21 11:42:44 2020 +0000
+++ b/netwerk/dns/TRRService.h	Fri Jul 24 11:08:26 2020 +0000
@@ -49,6 +49,7 @@
   bool SkipTRRWhenParentalControlEnabled() {
     return mSkipTRRWhenParentalControlEnabled;
   }
+  bool DisableSystemResolver() { return mDisableSystemResolver; }
   nsresult GetURI(nsACString& result);
   nsresult GetCredentials(nsCString& result);
   uint32_t GetRequestTimeout();
@@ -136,6 +137,7 @@
   Atomic<uint32_t, Relaxed>
       mDisableAfterFails;  // this many fails in a row means failed TRR service
   Atomic<bool, Relaxed> mPlatformDisabledTRR;
+  Atomic<bool, Relaxed> mDisableSystemResolver;
 
   // TRR Blocklist storage
   // mTRRBLStorage is only modified on the main thread, but we query whether it

Comment 5

[mcccs]

I'll post a cleaner patch to the correct reviewing place after D85010 is merged, sorry for the wrong needinfo @valentin.

Comment 6

[Vernon Van Steenkist]

+// When used with mode 3, never use the system resolver as a fallback
+// for normal requests, with the only exception of
+// domains specified with network.trr.excluded-domains.
+pref("network.trr.disable-system-resolver", false);

So let me understand this correctly, when network.trr.mode 3, network.trr.disable-system-resolver will now need to be set to true as well in order for Firefox to never use the system resolver as a fallback with the only exception of domains specified with network.trr.excluded-domains ?

Comment 7

[mcccs]

An updated patch can be found at https://phabricator.services.mozilla.com/D85607 but I doubt it will make into production.(In reply to Vernon Van Steenkist from comment #6)

+// When used with mode 3, never use the system resolver as a fallback
+// for normal requests, with the only exception of
+// domains specified with network.trr.excluded-domains.
+pref("network.trr.disable-system-resolver", false);

So let me understand this correctly, when network.trr.mode 3, network.trr.disable-system-resolver will now need to be set to true as well in order for Firefox to never use the system resolver as a fallback with the only exception of domains specified with network.trr.excluded-domains ?

An updated patch can be found at https://phabricator.services.mozilla.com/D85607 but I doubt these prefs will make into production.

Yes, there are certain cases that TRR is not used in TRR-only mode causing information to leak (It's trivial to write a spyware that manipulates those registry keys: https://searchfox.org/mozilla-central/rev/3434a9df60373a997263107e6f124fb164ddebf2/netwerk/system/win32/nsNotifyAddrListener.cpp#591 then we can log user's DNS request even though Firefox is in the TRR-only mode). This pref helps finding those situations.

Comment 8

[mcccs]

*** but there are other prefs doing the same, such as "network.trr.enable_when_nrpt_detected" (this one for the example above). So my patch sucked at design since it disabled things that could be disabled with some other pref. The advantage of my pref is that, if for example NRPT is detected, the TRR request fails and no DNS request is failed instead of not checking for it and doing the TRR request (the pref above). In this case, it helps you notice that something is wrong with your configuration, the cause of which should still be found instead of being ignored.

Comment 9

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1652427 - Add pref for disabling TRR split horizon mitigations r=#necko

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c656168bb863
Add pref for disabling TRR split horizon mitigations r=necko-reviewers,kershaw

Comment 11

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/c656168bb863