bug 1501302 - TRR: pass on correct originSuffix for NS checks r=valentin
authorDaniel Stenberg <daniel@haxx.se>
Tue, 30 Oct 2018 13:06:24 +0000
changeset 443520 c17a9a39185206ec340920b8165387ddebc760d2
parent 443519 5d017b5af700c51b674a32933074b11cddb5a5cb
child 443521 399cd7842104f310dd0c87120052650ab59aee68
push id34961
push useraciure@mozilla.com
push dateTue, 30 Oct 2018 22:06:02 +0000
treeherdermozilla-central@a9bd3dd38b19 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersvalentin
bugs1501302
milestone65.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
bug 1501302 - TRR: pass on correct originSuffix for NS checks r=valentin ... when that NS check is used to check the "parent" domain of a blacklisted host. Previously, additional TRRblacklist entries due to this would always be added with the originSuffix "" which was incorrect for all uses of other suffxes. MozReview-Commit-ID: EeorQuuRCRX Differential Revision: https://phabricator.services.mozilla.com/D10192
netwerk/dns/TRR.cpp
netwerk/dns/TRR.h
netwerk/dns/TRRService.cpp
netwerk/dns/TRRService.h
netwerk/dns/nsHostResolver.cpp
netwerk/dns/nsHostResolver.h
--- a/netwerk/dns/TRR.cpp
+++ b/netwerk/dns/TRR.cpp
@@ -174,17 +174,17 @@ TRR::SendHTTPRequest()
     // these types
     return NS_ERROR_FAILURE;
   }
 
   if ((mType == TRRTYPE_A) || (mType == TRRTYPE_AAAA)) {
     // let NS resolves skip the blacklist check
     MOZ_ASSERT(mRec);
 
-    if (gTRRService->IsTRRBlacklisted(mHost, mRec->originSuffix, mPB, true)) {
+    if (gTRRService->IsTRRBlacklisted(mHost, mOriginSuffix, mPB, true)) {
       if (mType == TRRTYPE_A) {
         // count only blacklist for A records to avoid double counts
         Telemetry::Accumulate(Telemetry::DNS_TRR_BLACKLISTED, true);
       }
       // not really an error but no TRR is issued
       return NS_ERROR_UNKNOWN_HOST;
     } else {
       if (mType == TRRTYPE_A) {
@@ -908,17 +908,18 @@ TRR::ReturnData()
         // lowest number.
         ttl = item->mTtl;
       }
     }
     ai->ttl = ttl;
     if (!mHostResolver) {
       return NS_ERROR_FAILURE;
     }
-    (void)mHostResolver->CompleteLookup(mRec, NS_OK, ai.forget(), mPB);
+    (void)mHostResolver->CompleteLookup(mRec, NS_OK, ai.forget(), mPB,
+                                        mOriginSuffix);
     mHostResolver = nullptr;
     mRec = nullptr;
   } else {
     (void)mHostResolver->CompleteLookupByType(mRec, NS_OK, &mTxt, mTxtTtl, mPB);
   }
   return NS_OK;
 }
 
@@ -932,17 +933,17 @@ TRR::FailData(nsresult error)
   if (mType == TRRTYPE_TXT) {
     (void)mHostResolver->CompleteLookupByType(mRec, error,
                                               nullptr, 0, mPB);
   } else {
     // create and populate an TRR AddrInfo instance to pass on to signal that
     // this comes from TRR
     AddrInfo *ai = new AddrInfo(mHost, mType);
 
-    (void)mHostResolver->CompleteLookup(mRec, error, ai, mPB);
+    (void)mHostResolver->CompleteLookup(mRec, error, ai, mPB, mOriginSuffix);
   }
 
   mHostResolver = nullptr;
   mRec = nullptr;
   return NS_OK;
 }
 
 nsresult
--- a/netwerk/dns/TRR.h
+++ b/netwerk/dns/TRR.h
@@ -75,16 +75,17 @@ public:
     , mRec(aRec)
     , mHostResolver(aResolver)
     , mType(aType)
     , mBodySize(0)
     , mFailed(false)
     , mCnameLoop(kCnameChaseMax)
     , mAllowRFC1918(false)
     , mTxtTtl(UINT32_MAX)
+    , mOriginSuffix(aRec->originSuffix)
   {
     mHost = aRec->host;
     mPB = aRec->pb;
   }
 
   // when following CNAMEs
   explicit TRR(AHostResolver *aResolver,
                nsHostRecord *aRec,
@@ -98,16 +99,17 @@ public:
     , mHostResolver(aResolver)
     , mType(aType)
     , mBodySize(0)
     , mFailed(false)
     , mPB(aPB)
     , mCnameLoop(aLoopCount)
     , mAllowRFC1918(false)
     , mTxtTtl(UINT32_MAX)
+    , mOriginSuffix(aRec->originSuffix)
   {
 
   }
 
   // used on push
   explicit TRR(AHostResolver *aResolver, bool aPB)
     : mozilla::Runnable("TRR")
     , mHostResolver(aResolver)
@@ -119,27 +121,30 @@ public:
     , mAllowRFC1918(false)
     , mTxtTtl(UINT32_MAX)
   { }
 
   // to verify a domain
   explicit TRR(AHostResolver *aResolver,
                nsACString &aHost,
                enum TrrType aType,
+               const nsACString &aOriginSuffix,
                bool aPB)
     : mozilla::Runnable("TRR")
     , mHost(aHost)
+    , mRec(nullptr)
     , mHostResolver(aResolver)
     , mType(aType)
     , mBodySize(0)
     , mFailed(false)
     , mPB(aPB)
     , mCnameLoop(kCnameChaseMax)
     , mAllowRFC1918(false)
     , mTxtTtl(UINT32_MAX)
+    , mOriginSuffix(aOriginSuffix)
   { }
 
   NS_IMETHOD Run() override;
   void Cancel();
   enum TrrType Type() { return mType; }
   nsCString mHost;
   RefPtr<nsHostRecord> mRec;
   RefPtr<AHostResolver> mHostResolver;
@@ -174,14 +179,17 @@ private:
   bool mPB;
   DOHresp mDNS;
   nsCOMPtr<nsITimer> mTimeout;
   nsCString mCname;
   uint32_t mCnameLoop; // loop detection counter
   bool mAllowRFC1918;
   nsTArray<nsCString> mTxt;
   uint32_t mTxtTtl;
+
+  // keep a copy of the originSuffix for the cases where mRec == nullptr */
+  const nsCString mOriginSuffix;
 };
 
 } // namespace net
 } // namespace mozilla
 
 #endif // include guard
--- a/netwerk/dns/TRRService.cpp
+++ b/netwerk/dns/TRRService.cpp
@@ -388,17 +388,17 @@ TRRService::MaybeConfirm()
   }
   if (host.Equals("skip")) {
     LOG(("TRRService starting confirmation test %s SKIPPED\n",
          mPrivateURI.get()));
     mConfirmationState = CONFIRM_OK;
   } else {
     LOG(("TRRService starting confirmation test %s %s\n",
          mPrivateURI.get(), host.get()));
-    mConfirmer = new TRR(this, host, TRRTYPE_NS, false);
+    mConfirmer = new TRR(this, host, TRRTYPE_NS, EmptyCString(), false);
     NS_DispatchToMainThread(mConfirmer);
   }
 }
 
 bool
 TRRService::MaybeBootstrap(const nsACString &aPossible, nsACString &aResult)
 {
   MutexAutoLock lock(mLock);
@@ -577,17 +577,18 @@ TRRService::TRRBlacklist(const nsACStrin
       if (IsTRRBlacklisted(check, aOriginSuffix, privateBrowsing, false)) {
         // the domain part is already blacklisted, no need to add this entry
         return;
       }
       // verify 'check' over TRR
       LOG(("TRR: verify if '%s' resolves as NS\n", check.get()));
 
       // check if there's an NS entry for this name
-      RefPtr<TRR> trr = new TRR(this, check, TRRTYPE_NS, privateBrowsing);
+      RefPtr<TRR> trr = new TRR(this, check, TRRTYPE_NS, aOriginSuffix,
+                                privateBrowsing);
       NS_DispatchToMainThread(trr);
     }
   }
 }
 
 NS_IMETHODIMP
 TRRService::Notify(nsITimer *aTimer)
 {
@@ -627,17 +628,18 @@ TRRService::TRRIsOkay(enum TrrOkay aReas
                               this, mRetryConfirmInterval,
                               nsITimer::TYPE_ONE_SHOT);
       mTRRFailures = 0; // clear it again
     }
   }
 }
 
 AHostResolver::LookupStatus
-TRRService::CompleteLookup(nsHostRecord *rec, nsresult status, AddrInfo *aNewRRSet, bool pb)
+TRRService::CompleteLookup(nsHostRecord *rec, nsresult status, AddrInfo *aNewRRSet, bool pb,
+                           const nsACString &aOriginSuffix)
 {
   // this is an NS check for the TRR blacklist or confirmationNS check
 
   MOZ_ASSERT(NS_IsMainThread());
   MOZ_ASSERT(!rec);
 
   nsAutoPtr<AddrInfo> newRRSet(aNewRRSet);
   MOZ_ASSERT(newRRSet && newRRSet->IsTRR() == TRRTYPE_NS);
@@ -671,17 +673,17 @@ TRRService::CompleteLookup(nsHostRecord 
     return LOOKUP_OK;
   }
 
   // when called without a host record, this is a domain name check response.
   if (NS_SUCCEEDED(status)) {
     LOG(("TRR verified %s to be fine!\n", newRRSet->mHostName.get()));
   } else {
     LOG(("TRR says %s doesn't resolve as NS!\n", newRRSet->mHostName.get()));
-    TRRBlacklist(newRRSet->mHostName, nsCString(""), pb, false);
+    TRRBlacklist(newRRSet->mHostName, aOriginSuffix, pb, false);
   }
   return LOOKUP_OK;
 }
 
 AHostResolver::LookupStatus
 TRRService::CompleteLookupByType(nsHostRecord *, nsresult,
                                  const nsTArray<nsCString> *aResult,
                                  uint32_t aTtl,
--- a/netwerk/dns/TRRService.h
+++ b/netwerk/dns/TRRService.h
@@ -38,17 +38,18 @@ public:
   bool UseGET() { return mUseGET; }
   bool EarlyAAAA() { return mEarlyAAAA; }
   bool DisableIPv6() { return mDisableIPv6; }
   bool DisableECS() { return mDisableECS; }
   nsresult GetURI(nsCString &result);
   nsresult GetCredentials(nsCString &result);
   uint32_t GetRequestTimeout() { return mTRRTimeout; }
 
-  LookupStatus CompleteLookup(nsHostRecord *, nsresult, mozilla::net::AddrInfo *, bool pb) override;
+  LookupStatus CompleteLookup(nsHostRecord *, nsresult, mozilla::net::AddrInfo *, bool pb,
+                              const nsACString &aOriginSuffix) override;
   LookupStatus CompleteLookupByType(nsHostRecord *, nsresult, const nsTArray<nsCString> *, uint32_t, bool pb) override;
   void TRRBlacklist(const nsACString &host, const nsACString &originSuffix,
                     bool privateBrowsing, bool aParentsToo);
   bool IsTRRBlacklisted(const nsACString &aHost, const nsACString &aOriginSuffix,
                         bool aPrivateBrowsing, bool aParentsToo);
 
   bool MaybeBootstrap(const nsACString &possible, nsACString &result);
   enum TrrOkay {
--- a/netwerk/dns/nsHostResolver.cpp
+++ b/netwerk/dns/nsHostResolver.cpp
@@ -738,17 +738,18 @@ nsHostResolver::Init()
 void
 nsHostResolver::ClearPendingQueue(LinkedList<RefPtr<nsHostRecord>>& aPendingQ)
 {
     // loop through pending queue, erroring out pending lookups.
     if (!aPendingQ.isEmpty()) {
         for (RefPtr<nsHostRecord> rec : aPendingQ) {
             rec->Cancel();
             if (rec->IsAddrRecord()) {
-                CompleteLookup(rec, NS_ERROR_ABORT, nullptr, rec->pb);
+                CompleteLookup(rec, NS_ERROR_ABORT, nullptr, rec->pb,
+                               rec->originSuffix);
             } else {
                 CompleteLookupByType(rec, NS_ERROR_ABORT, nullptr, 0, rec->pb);
             }
         }
     }
 }
 
 //
@@ -1781,17 +1782,18 @@ nsHostResolver::AddToEvictionQ(nsHostRec
     }
 }
 
 //
 // CompleteLookup() checks if the resolving should be redone and if so it
 // returns LOOKUP_RESOLVEAGAIN, but only if 'status' is not NS_ERROR_ABORT.
 // takes ownership of AddrInfo parameter
 nsHostResolver::LookupStatus
-nsHostResolver::CompleteLookup(nsHostRecord* rec, nsresult status, AddrInfo* aNewRRSet, bool pb)
+nsHostResolver::CompleteLookup(nsHostRecord* rec, nsresult status, AddrInfo* aNewRRSet, bool pb,
+                               const nsACString & aOriginsuffix)
 {
     MutexAutoLock lock(mLock);
     MOZ_ASSERT(rec);
     MOZ_ASSERT(rec->pb == pb);
     MOZ_ASSERT(rec->IsAddrRecord());
 
     nsCOMPtr<AddrHostRecord> addrRec = do_QueryInterface(rec);
     MOZ_ASSERT(addrRec);
@@ -2214,17 +2216,18 @@ nsHostResolver::ThreadFunc()
                 }
             }
         }
 
         LOG(("DNS lookup thread - lookup completed for host [%s]: %s.\n",
              rec->host.get(),
              ai ? "success" : "failure: unknown host"));
 
-        if (LOOKUP_RESOLVEAGAIN == CompleteLookup(rec, status, ai, rec->pb)) {
+        if (LOOKUP_RESOLVEAGAIN == CompleteLookup(rec, status, ai, rec->pb,
+                                                  rec->originSuffix)) {
             // leave 'rec' assigned and loop to make a renewed host resolve
             LOG(("DNS lookup thread - Re-resolving host [%s].\n", rec->host.get()));
         } else {
             rec = nullptr;
         }
     } while(true);
 
     mActiveTaskCount--;
--- a/netwerk/dns/nsHostResolver.h
+++ b/netwerk/dns/nsHostResolver.h
@@ -363,17 +363,18 @@ public:
     virtual ~AHostResolver() = default;
     NS_INLINE_DECL_PURE_VIRTUAL_REFCOUNTING
 
      enum LookupStatus {
         LOOKUP_OK,
         LOOKUP_RESOLVEAGAIN,
     };
 
-    virtual LookupStatus CompleteLookup(nsHostRecord *, nsresult, mozilla::net::AddrInfo *, bool pb) = 0;
+    virtual LookupStatus CompleteLookup(nsHostRecord *, nsresult, mozilla::net::AddrInfo *, bool pb,
+                                        const nsACString &aOriginsuffix) = 0;
     virtual LookupStatus CompleteLookupByType(nsHostRecord *, nsresult,
                                               const nsTArray<nsCString> *aResult,
                                               uint32_t aTtl, bool pb) = 0;
     virtual nsresult GetHostRecord(const nsACString &host, uint16_t type,
                                    uint16_t flags, uint16_t af, bool pb,
                                    const nsCString &originSuffix,
                                    nsHostRecord **result)
     {
@@ -482,17 +483,18 @@ public:
 
     size_t SizeOfIncludingThis(mozilla::MallocSizeOf mallocSizeOf) const;
 
     /**
      * Flush the DNS cache.
      */
     void FlushCache();
 
-    LookupStatus CompleteLookup(nsHostRecord *, nsresult, mozilla::net::AddrInfo *, bool pb) override;
+    LookupStatus CompleteLookup(nsHostRecord *, nsresult, mozilla::net::AddrInfo *, bool pb,
+                                const nsACString &aOriginsuffix) override;
     LookupStatus CompleteLookupByType(nsHostRecord *, nsresult,
                                       const nsTArray<nsCString> *aResult,
                                       uint32_t aTtl, bool pb) override;
     nsresult GetHostRecord(const nsACString &host, uint16_t type,
                            uint16_t flags, uint16_t af, bool pb,
                            const nsCString &originSuffix,
                            nsHostRecord **result) override;
     nsresult TrrLookup_unlocked(nsHostRecord *, mozilla::net::TRR *pushedTRR = nullptr) override;