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

--HG--
extra : moz-landing-system : lando

netwerk/dns/TRR.cpp

@@ -179,7 +179,7 @@ TRR::SendHTTPRequest()
     // 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);
@@ -913,7 +913,8 @@ TRR::ReturnData()
     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 {
@@ -937,7 +938,7 @@ TRR::FailData(nsresult error)
     // 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;

netwerk/dns/TRR.h

@@ -80,6 +80,7 @@ public:
     , mCnameLoop(kCnameChaseMax)
     , mAllowRFC1918(false)
     , mTxtTtl(UINT32_MAX)
+    , mOriginSuffix(aRec->originSuffix)
   {
     mHost = aRec->host;
     mPB = aRec->pb;
@@ -103,6 +104,7 @@ public:
     , mCnameLoop(aLoopCount)
     , mAllowRFC1918(false)
     , mTxtTtl(UINT32_MAX)
+    , mOriginSuffix(aRec->originSuffix)
   {
 
   }
@@ -124,9 +126,11 @@ public:
   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)
@@ -135,6 +139,7 @@ public:
     , mCnameLoop(kCnameChaseMax)
     , mAllowRFC1918(false)
     , mTxtTtl(UINT32_MAX)
+    , mOriginSuffix(aOriginSuffix)
   { }
 
   NS_IMETHOD Run() override;
@@ -179,6 +184,9 @@ private:
   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

netwerk/dns/TRRService.cpp

@@ -393,7 +393,7 @@ TRRService::MaybeConfirm()
   } 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);
   }
 }
@@ -582,7 +582,8 @@ TRRService::TRRBlacklist(const nsACString &aHost, const nsACString &aOriginSuffi
       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);
     }
   }
@@ -632,7 +633,8 @@ TRRService::TRRIsOkay(enum TrrOkay aReason)
 }
 
 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
 
@@ -676,7 +678,7 @@ TRRService::CompleteLookup(nsHostRecord *rec, nsresult status, AddrInfo *aNewRRS
     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;
 }

netwerk/dns/TRRService.h

@@ -43,7 +43,8 @@ public:
   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);

netwerk/dns/nsHostResolver.cpp

@@ -743,7 +743,8 @@ nsHostResolver::ClearPendingQueue(LinkedList<RefPtr<nsHostRecord>>& aPendingQ)
         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);
             }
@@ -1786,7 +1787,8 @@ nsHostResolver::AddToEvictionQ(nsHostRecord* rec)
 // 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);
@@ -2219,7 +2221,8 @@ nsHostResolver::ThreadFunc()
              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 {

netwerk/dns/nsHostResolver.h

@@ -368,7 +368,8 @@ public:
         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;
@@ -487,7 +488,8 @@ public:
      */
     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;