Bug 1910593 - Don't prefetch HTTPS RR if proxyDNS is enabled, r=necko-reviewers,valentin

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

dom/chrome-webidl/NetDashboard.webidl

@@ -68,6 +68,7 @@ dictionary DnsCacheEntry {
   boolean trr = false;
   DOMString originAttributesSuffix = "";
   DOMString flags = "";
+  unsigned short type = 0;
 };
 
 [GenerateConversionToJS]

netwerk/base/Dashboard.cpp

@@ -912,10 +912,13 @@ nsresult Dashboard::GetDNSCacheEntries(DnsData* dnsData) {
       CopyASCIItoUTF16(dnsData->mData[i].hostaddr[j], *addr);
     }
 
-    if (dnsData->mData[i].family == PR_AF_INET6) {
-      entry.mFamily.AssignLiteral(u"ipv6");
-    } else {
-      entry.mFamily.AssignLiteral(u"ipv4");
+    entry.mType = dnsData->mData[i].resolveType;
+    if (entry.mType == nsIDNSService::RESOLVE_TYPE_DEFAULT) {
+      if (dnsData->mData[i].family == PR_AF_INET6) {
+        entry.mFamily.AssignLiteral(u"ipv6");
+      } else {
+        entry.mFamily.AssignLiteral(u"ipv4");
+      }
     }
 
     entry.mOriginAttributesSuffix =

netwerk/base/DashboardTypes.h

@@ -35,12 +35,12 @@ struct DnsAndConnectSockets {
 struct DNSCacheEntries {
   nsCString hostname;
   nsTArray<nsCString> hostaddr;
-  uint16_t family;
-  int64_t expiration;
-  nsCString netInterface;
-  bool TRR;
+  uint16_t family{0};
+  int64_t expiration{0};
+  bool TRR{false};
   nsCString originAttributesSuffix;
   nsCString flags;
+  uint16_t resolveType{0};
 };
 
 struct HttpConnInfo {
@@ -99,8 +99,10 @@ struct ParamTraits<mozilla::net::DNSCacheEntries> {
     WriteParam(aWriter, aParam.hostaddr);
     WriteParam(aWriter, aParam.family);
     WriteParam(aWriter, aParam.expiration);
-    WriteParam(aWriter, aParam.netInterface);
     WriteParam(aWriter, aParam.TRR);
+    WriteParam(aWriter, aParam.originAttributesSuffix);
+    WriteParam(aWriter, aParam.flags);
+    WriteParam(aWriter, aParam.resolveType);
   }
 
   static bool Read(MessageReader* aReader, paramType* aResult) {
@@ -108,8 +110,10 @@ struct ParamTraits<mozilla::net::DNSCacheEntries> {
            ReadParam(aReader, &aResult->hostaddr) &&
            ReadParam(aReader, &aResult->family) &&
            ReadParam(aReader, &aResult->expiration) &&
-           ReadParam(aReader, &aResult->netInterface) &&
-           ReadParam(aReader, &aResult->TRR);
+           ReadParam(aReader, &aResult->TRR) &&
+           ReadParam(aReader, &aResult->originAttributesSuffix) &&
+           ReadParam(aReader, &aResult->flags) &&
+           ReadParam(aReader, &aResult->resolveType);
   }
 };
 

netwerk/dns/nsHostResolver.cpp

@@ -2035,20 +2035,13 @@ void nsHostResolver::GetDNSCacheEntries(nsTArray<DNSCacheEntries>* args) {
       continue;
     }
 
-    // For now we only show A/AAAA records.
-    if (!rec->IsAddrRecord()) {
-      continue;
-    }
-
-    RefPtr<AddrHostRecord> addrRec = do_QueryObject(rec);
-    MOZ_ASSERT(addrRec);
-    if (!addrRec || !addrRec->addr_info) {
-      continue;
-    }
-
     DNSCacheEntries info;
+    info.resolveType = rec->type;
     info.hostname = rec->host;
     info.family = rec->af;
+    if (rec->mValidEnd.IsNull()) {
+      continue;
+    }
     info.expiration =
         (int64_t)(rec->mValidEnd - TimeStamp::NowLoRes()).ToSeconds();
     if (info.expiration <= 0) {
@@ -2056,7 +2049,12 @@ void nsHostResolver::GetDNSCacheEntries(nsTArray<DNSCacheEntries>* args) {
       continue;
     }
 
-    {
+    info.originAttributesSuffix = recordEntry.GetKey().originSuffix;
+    info.flags = nsPrintfCString("%u|0x%x|%u|%d|%s", rec->type, rec->flags,
+                                 rec->af, rec->pb, rec->mTrrServer.get());
+
+    RefPtr<AddrHostRecord> addrRec = do_QueryObject(rec);
+    if (addrRec && addrRec->addr_info) {
       MutexAutoLock lock(addrRec->addr_info_lock);
       for (const auto& addr : addrRec->addr_info->Addresses()) {
         char buf[kIPv6CStrBufSize];
@@ -2067,10 +2065,6 @@ void nsHostResolver::GetDNSCacheEntries(nsTArray<DNSCacheEntries>* args) {
       info.TRR = addrRec->addr_info->IsTRR();
     }
 
-    info.originAttributesSuffix = recordEntry.GetKey().originSuffix;
-    info.flags = nsPrintfCString("%u|0x%x|%u|%d|%s", rec->type, rec->flags,
-                                 rec->af, rec->pb, rec->mTrrServer.get());
-
     args->AppendElement(std::move(info));
   }
 }

netwerk/protocol/http/nsHttp.cpp

@@ -35,6 +35,8 @@
 namespace mozilla {
 namespace net {
 
+extern const char kProxyType_SOCKS[];
+
 const uint32_t kHttp3VersionCount = 5;
 const nsCString kHttp3Versions[] = {"h3-29"_ns, "h3-30"_ns, "h3-31"_ns,
                                     "h3-32"_ns, "h3"_ns};
@@ -1178,5 +1180,19 @@ nsLiteralCString HttpVersionToTelemetryLabel(HttpVersion version) {
   return "unknown"_ns;
 }
 
+ProxyDNSStrategy GetProxyDNSStrategyHelper(const char* aType, uint32_t aFlag) {
+  if (!aType) {
+    return ProxyDNSStrategy::ORIGIN;
+  }
+
+  if (!(aFlag & nsIProxyInfo::TRANSPARENT_PROXY_RESOLVES_HOST)) {
+    if (aType == kProxyType_SOCKS) {
+      return ProxyDNSStrategy::ORIGIN;
+    }
+  }
+
+  return ProxyDNSStrategy::PROXY;
+}
+
 }  // namespace net
 }  // namespace mozilla

netwerk/protocol/http/nsHttp.h

@@ -529,6 +529,16 @@ void DisallowHTTPSRR(uint32_t& aCaps);
 
 nsLiteralCString HttpVersionToTelemetryLabel(HttpVersion version);
 
+enum class ProxyDNSStrategy : uint8_t {
+  // To resolve the origin of the end server we are connecting
+  // to.
+  ORIGIN = 1 << 0,
+  // To resolve the host name of the proxy.
+  PROXY = 1 << 1
+};
+
+ProxyDNSStrategy GetProxyDNSStrategyHelper(const char* aType, uint32_t aFlag);
+
 }  // namespace net
 }  // namespace mozilla
 

netwerk/protocol/http/nsHttpChannel.cpp

@@ -825,7 +825,7 @@ nsresult nsHttpChannel::MaybeUseHTTPSRRForUpgrade(bool aShouldUpgrade,
   }
 
   auto dnsStrategy = GetProxyDNSStrategy();
-  if (!(dnsStrategy & DNS_PREFETCH_ORIGIN)) {
+  if (dnsStrategy != ProxyDNSStrategy::ORIGIN) {
     return ContinueOnBeforeConnect(aShouldUpgrade, aStatus);
   }
 
@@ -6773,29 +6773,16 @@ nsHttpChannel::GetOrCreateChannelClassifier() {
   return classifier.forget();
 }
 
-uint16_t nsHttpChannel::GetProxyDNSStrategy() {
-  // This function currently only supports returning DNS_PREFETCH_ORIGIN.
-  // Support for the rest of the DNS_* flags will be added later.
-
+ProxyDNSStrategy nsHttpChannel::GetProxyDNSStrategy() {
   // When network_dns_force_use_https_rr is true, return DNS_PREFETCH_ORIGIN.
   // This ensures that we always perform HTTPS RR query.
-  if (!mProxyInfo || StaticPrefs::network_dns_force_use_https_rr()) {
-    return DNS_PREFETCH_ORIGIN;
+  nsCOMPtr<nsProxyInfo> proxyInfo(static_cast<nsProxyInfo*>(mProxyInfo.get()));
+  if (!proxyInfo || StaticPrefs::network_dns_force_use_https_rr()) {
+    return ProxyDNSStrategy::ORIGIN;
   }
 
-  uint32_t flags = 0;
-  nsAutoCString type;
-  mProxyInfo->GetFlags(&flags);
-  mProxyInfo->GetType(type);
-
   // If the proxy is not to perform name resolution itself.
-  if (!(flags & nsIProxyInfo::TRANSPARENT_PROXY_RESOLVES_HOST)) {
-    if (type.EqualsLiteral("socks")) {
-      return DNS_PREFETCH_ORIGIN;
-    }
-  }
-
-  return 0;
+  return GetProxyDNSStrategyHelper(proxyInfo->Type(), proxyInfo->Flags());
 }
 
 // BeginConnect() SHOULD NOT call AsyncAbort(). AsyncAbort will be called by
@@ -7096,16 +7083,7 @@ nsresult nsHttpChannel::BeginConnect() {
     ReEvaluateReferrerAfterTrackingStatusIsKnown();
   }
 
-  rv = MaybeStartDNSPrefetch();
-  if (NS_FAILED(rv)) {
-    auto dnsStrategy = GetProxyDNSStrategy();
-    if (dnsStrategy & DNS_BLOCK_ON_ORIGIN_RESOLVE) {
-      // TODO: Should this be fatal?
-      return rv;
-    }
-    // Otherwise this shouldn't be fatal.
-    return NS_OK;
-  }
+  MaybeStartDNSPrefetch();
 
   rv = CallOrWaitForResume(
       [](nsHttpChannel* self) { return self->PrepareToConnect(); });
@@ -7125,7 +7103,7 @@ nsresult nsHttpChannel::BeginConnect() {
   return NS_OK;
 }
 
-nsresult nsHttpChannel::MaybeStartDNSPrefetch() {
+void nsHttpChannel::MaybeStartDNSPrefetch() {
   // Start a DNS lookup very early in case the real open is queued the DNS can
   // happen in parallel. Do not do so in the presence of an HTTP proxy as
   // all lookups other than for the proxy itself are done by the proxy.
@@ -7141,7 +7119,7 @@ nsresult nsHttpChannel::MaybeStartDNSPrefetch() {
   // timing we used.
   if ((mLoadFlags & (LOAD_NO_NETWORK_IO | LOAD_ONLY_FROM_CACHE)) ||
       LoadAuthRedirectedChannel()) {
-    return NS_OK;
+    return;
   }
 
   auto dnsStrategy = GetProxyDNSStrategy();
@@ -7149,10 +7127,10 @@ nsresult nsHttpChannel::MaybeStartDNSPrefetch() {
   LOG(
       ("nsHttpChannel::MaybeStartDNSPrefetch [this=%p, strategy=%u] "
        "prefetching%s\n",
-       this, dnsStrategy,
+       this, static_cast<uint32_t>(dnsStrategy),
        mCaps & NS_HTTP_REFRESH_DNS ? ", refresh requested" : ""));
 
-  if (dnsStrategy & DNS_PREFETCH_ORIGIN) {
+  if (dnsStrategy == ProxyDNSStrategy::ORIGIN) {
     OriginAttributes originAttributes;
     StoragePrincipalHelper::GetOriginAttributesForNetworkState(
         this, originAttributes);
@@ -7164,20 +7142,8 @@ nsresult nsHttpChannel::MaybeStartDNSPrefetch() {
     if (mCaps & NS_HTTP_REFRESH_DNS) {
       dnsFlags |= nsIDNSService::RESOLVE_BYPASS_CACHE;
     }
-    nsresult rv = mDNSPrefetch->PrefetchHigh(dnsFlags);
 
-    if (dnsStrategy & DNS_BLOCK_ON_ORIGIN_RESOLVE) {
-      LOG(("  blocking on prefetching origin"));
-
-      if (NS_WARN_IF(NS_FAILED(rv))) {
-        LOG(("  lookup failed with 0x%08" PRIx32 ", aborting request",
-             static_cast<uint32_t>(rv)));
-        return rv;
-      }
-
-      // Resolved in OnLookupComplete.
-      mDNSBlockingThenable = mDNSBlockingPromise.Ensure(__func__);
-    }
+    Unused << mDNSPrefetch->PrefetchHigh(dnsFlags);
 
     if (StaticPrefs::network_dns_use_https_rr_as_altsvc() && !mHTTPSSVCRecord &&
         !(mCaps & NS_HTTP_DISALLOW_HTTPS_RR) && canUseHTTPSRRonNetwork()) {
@@ -7195,8 +7161,6 @@ nsresult nsHttpChannel::MaybeStartDNSPrefetch() {
                                         });
     }
   }
-
-  return NS_OK;
 }
 
 NS_IMETHODIMP

netwerk/protocol/http/nsHttpChannel.h

@@ -307,23 +307,11 @@ class nsHttpChannel final : public HttpBaseChannel,
   // Connections will only be established in this function.
   // (including DNS prefetch and speculative connection.)
   void MaybeResolveProxyAndBeginConnect();
-  nsresult MaybeStartDNSPrefetch();
-
-  // Tells the channel to resolve the origin of the end server we are connecting
-  // to.
-  static uint16_t const DNS_PREFETCH_ORIGIN = 1 << 0;
-  // Tells the channel to resolve the host name of the proxy.
-  static uint16_t const DNS_PREFETCH_PROXY = 1 << 1;
-  // Will be set if the current channel uses an HTTP/HTTPS proxy.
-  static uint16_t const DNS_PROXY_IS_HTTP = 1 << 2;
-  // Tells the channel to wait for the result of the origin server resolution
-  // before any connection attempts are made.
-  static uint16_t const DNS_BLOCK_ON_ORIGIN_RESOLVE = 1 << 3;
+  void MaybeStartDNSPrefetch();
 
   // Based on the proxy configuration determine the strategy for resolving the
   // end server host name.
-  // Returns a combination of the above flags.
-  uint16_t GetProxyDNSStrategy();
+  ProxyDNSStrategy GetProxyDNSStrategy();
 
   // We might synchronously or asynchronously call BeginConnect,
   // which includes DNS prefetch and speculative connection, according to

netwerk/protocol/http/nsHttpConnectionInfo.h

@@ -127,6 +127,13 @@ class nsHttpConnectionInfo final : public ARefBase {
   const char* ProxyPassword() const {
     return mProxyInfo ? mProxyInfo->Password().get() : nullptr;
   }
+  uint32_t ProxyFlag() const {
+    uint32_t flags = 0;
+    if (mProxyInfo) {
+      mProxyInfo->GetFlags(&flags);
+    }
+    return flags;
+  }
 
   const nsCString& ProxyAuthorizationHeader() const {
     return mProxyInfo ? mProxyInfo->ProxyAuthorizationHeader() : EmptyCString();

netwerk/protocol/http/nsHttpConnectionMgr.cpp

@@ -3573,7 +3573,10 @@ void nsHttpConnectionMgr::DoSpeculativeConnectionInternal(
     return;
   }
 
-  if (aFetchHTTPSRR && NS_SUCCEEDED(aTrans->FetchHTTPSRR())) {
+  ProxyDNSStrategy strategy = GetProxyDNSStrategyHelper(
+      aEnt->mConnInfo->ProxyType(), aEnt->mConnInfo->ProxyFlag());
+  if (aFetchHTTPSRR && strategy == ProxyDNSStrategy::ORIGIN &&
+      NS_SUCCEEDED(aTrans->FetchHTTPSRR())) {
     // nsHttpConnectionMgr::DoSpeculativeConnection will be called again when
     // HTTPS RR is available.
     return;

netwerk/protocol/http/nsHttpTransaction.cpp

@@ -369,8 +369,11 @@ nsresult nsHttpTransaction::Init(
   }
 
   bool forceUseHTTPSRR = StaticPrefs::network_dns_force_use_https_rr();
+  ProxyDNSStrategy strategy =
+      GetProxyDNSStrategyHelper(mConnInfo->ProxyType(), mConnInfo->ProxyFlag());
   if ((StaticPrefs::network_dns_use_https_rr_as_altsvc() &&
-       !(mCaps & NS_HTTP_DISALLOW_HTTPS_RR)) ||
+       !(mCaps & NS_HTTP_DISALLOW_HTTPS_RR) &&
+       strategy == ProxyDNSStrategy::ORIGIN) ||
       forceUseHTTPSRR) {
     nsCOMPtr<nsIEventTarget> target;
     Unused << gHttpHandler->GetSocketThreadTarget(getter_AddRefs(target));

netwerk/test/unit/test_proxyDNS_leak.js

@@ -0,0 +1,110 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+// Test when socks proxy is registered, we don't try to resolve HTTPS record.
+// Steps:
+// 1. Use addHTTPSRecordOverride to add an override for service.com.
+// 2. Add a proxy filter to use socks proxy.
+// 3. Create a request to load service.com.
+// 4. See if the HTTPS record is in DNS cache entries.
+
+"use strict";
+
+const gDashboard = Cc["@mozilla.org/network/dashboard;1"].getService(
+  Ci.nsIDashboard
+);
+const pps = Cc["@mozilla.org/network/protocol-proxy-service;1"].getService();
+
+add_task(async function setup() {
+  Services.prefs.setBoolPref("network.dns.native_https_query", true);
+  Services.prefs.setBoolPref("network.dns.native_https_query_win10", true);
+  const override = Cc["@mozilla.org/network/native-dns-override;1"].getService(
+    Ci.nsINativeDNSResolverOverride
+  );
+
+  let rawBuffer = [
+    0, 0, 128, 0, 0, 0, 0, 1, 0, 0, 0, 0, 7, 115, 101, 114, 118, 105, 99, 101,
+    3, 99, 111, 109, 0, 0, 65, 0, 1, 0, 0, 0, 55, 0, 13, 0, 1, 0, 0, 1, 0, 6, 2,
+    104, 50, 2, 104, 51,
+  ];
+  override.addHTTPSRecordOverride("service.com", rawBuffer, rawBuffer.length);
+  override.addIPOverride("service.com", "127.0.0.1");
+  registerCleanupFunction(() => {
+    Services.prefs.clearUserPref("network.dns.native_https_query");
+    Services.prefs.clearUserPref("network.dns.native_https_query_win10");
+    Services.prefs.clearUserPref("network.dns.localDomains");
+    override.clearOverrides();
+  });
+});
+
+function makeChan(uri) {
+  let chan = NetUtil.newChannel({
+    uri,
+    loadUsingSystemPrincipal: true,
+  }).QueryInterface(Ci.nsIHttpChannel);
+  chan.loadFlags = Ci.nsIChannel.LOAD_INITIAL_DOCUMENT_URI;
+  return chan;
+}
+
+function channelOpenPromise(chan, flags) {
+  return new Promise(resolve => {
+    function finish(req, buffer) {
+      resolve([req, buffer]);
+    }
+    chan.asyncOpen(new ChannelListener(finish, null, flags));
+  });
+}
+
+async function isRecordFound(hostname) {
+  return new Promise(resolve => {
+    gDashboard.requestDNSInfo(function (data) {
+      let found = false;
+      for (let i = 0; i < data.entries.length; i++) {
+        if (
+          data.entries[i].hostname == hostname &&
+          data.entries[i].type == Ci.nsIDNSService.RESOLVE_TYPE_HTTPSSVC
+        ) {
+          found = true;
+          break;
+        }
+      }
+      resolve(found);
+    });
+  });
+}
+
+async function do_test_with_proxy_filter(filter) {
+  pps.registerFilter(filter, 10);
+
+  let chan = makeChan(`https://service.com/`);
+  await channelOpenPromise(chan, CL_EXPECT_LATE_FAILURE | CL_ALLOW_UNKNOWN_CL);
+
+  let found = await isRecordFound("service.com");
+  pps.unregisterFilter(filter);
+
+  return found;
+}
+
+add_task(async function test_proxyDNS_do_leak() {
+  let filter = new NodeProxyFilter("socks", "localhost", 443, 0);
+
+  let res = await do_test_with_proxy_filter(filter);
+
+  Assert.ok(res, "Should find a DNS entry");
+});
+
+add_task(async function test_proxyDNS_do_leak() {
+  Services.dns.clearCache(false);
+
+  let filter = new NodeProxyFilter(
+    "socks",
+    "localhost",
+    443,
+    Ci.nsIProxyInfo.TRANSPARENT_PROXY_RESOLVES_HOST
+  );
+
+  let res = await do_test_with_proxy_filter(filter);
+
+  Assert.ok(!res, "Should not find a DNS entry");
+});

netwerk/test/unit/xpcshell.toml

@@ -987,6 +987,12 @@ run-sequentially = "node server exceptions dont replay well"
 
 ["test_proxy_pac.js"]
 
+["test_proxyDNS_leak.js"]
+skip-if = [
+  "os == 'android'",
+  "socketprocess_networking",
+]
+
 ["test_proxyconnect.js"]
 skip-if = [
   "tsan",

toolkit/content/aboutNetworking.js

@@ -116,6 +116,11 @@ function displayDns(data) {
   new_cont.setAttribute("id", "dns_content");
 
   for (let i = 0; i < data.entries.length; i++) {
+    // TODO: Will be supported in bug 1889387.
+    if (data.entries[i].type != Ci.nsIDNSService.RESOLVE_TYPE_DEFAULT) {
+      continue;
+    }
+
     let row = document.createElement("tr");
     row.appendChild(col(data.entries[i].hostname));
     row.appendChild(col(data.entries[i].family));