Bug 1434741 - Only check final download URL against the application reputation whitelist. r?gcp draft
authorFrancois Marier <francois@mozilla.com>
Mon, 05 Feb 2018 18:11:56 -0800
changeset 753231 8ab23c0e0bcc5fe325da72bf83c96d133b1a3e89
parent 752153 65133e49fbfd5306632301f74be7cd15890bdf9f
push id98530
push userfmarier@mozilla.com
push dateFri, 09 Feb 2018 21:17:14 +0000
reviewersgcp
bugs1434741
milestone60.0a1
Bug 1434741 - Only check final download URL against the application reputation whitelist. r?gcp MozReview-Commit-ID: QCaStgteko
toolkit/components/reputationservice/ApplicationReputation.cpp
toolkit/components/reputationservice/test/unit/test_app_rep.js
--- a/toolkit/components/reputationservice/ApplicationReputation.cpp
+++ b/toolkit/components/reputationservice/ApplicationReputation.cpp
@@ -83,16 +83,18 @@ using safe_browsing::ClientDownloadReque
 #define PREF_BLOCK_POTENTIALLY_UNWANTED "browser.safebrowsing.downloads.remote.block_potentially_unwanted"
 #define PREF_BLOCK_UNCOMMON             "browser.safebrowsing.downloads.remote.block_uncommon"
 
 // MOZ_LOG=ApplicationReputation:5
 mozilla::LazyLogModule ApplicationReputationService::prlog("ApplicationReputation");
 #define LOG(args) MOZ_LOG(ApplicationReputationService::prlog, mozilla::LogLevel::Debug, args)
 #define LOG_ENABLED() MOZ_LOG_TEST(ApplicationReputationService::prlog, mozilla::LogLevel::Debug)
 
+enum class LookupType { AllowlistOnly, BlocklistOnly, BothLists };
+
 class PendingDBLookup;
 
 // A single use class private to ApplicationReputationService encapsulating an
 // nsIApplicationReputationQuery and an nsIApplicationReputationCallback. Once
 // created by ApplicationReputationService, it is guaranteed to call mCallback.
 // This class is private to ApplicationReputationService.
 class PendingLookup final : public nsIStreamListener,
                             public nsITimerCallback,
@@ -138,18 +140,20 @@ private:
   nsCOMPtr<nsIApplicationReputationQuery> mQuery;
 
   // The callback with which to report the verdict.
   nsCOMPtr<nsIApplicationReputationCallback> mCallback;
 
   // An array of strings created from certificate information used to whitelist
   // the downloaded file.
   nsTArray<nsCString> mAllowlistSpecs;
-  // The source URI of the download, the referrer and possibly any redirects.
+  // The source URI of the download (i.e. final URI after any redirects).
   nsTArray<nsCString> mAnylistSpecs;
+  // The referrer and possibly any redirects.
+  nsTArray<nsCString> mBlocklistSpecs;
 
   // When we started this query
   TimeStamp mStartTime;
 
   // The channel used to talk to the remote lookup server
   nsCOMPtr<nsIChannel> mChannel;
 
   // Timer to abort this lookup if it takes too long
@@ -214,17 +218,17 @@ private:
   // for each (cert, issuer) pair in each chain of certificates that is
   // associated with the binary.
   nsresult GenerateWhitelistStrings();
 
   // Parse the XPCOM certificate lists and stick them into the protocol buffer
   // version.
   nsresult ParseCertificates(nsIArray* aSigArray);
 
-  // Adds the redirects to mAnylistSpecs to be looked up.
+  // Adds the redirects to mBlocklistSpecs to be looked up.
   nsresult AddRedirects(nsIArray* aRedirects);
 
   // Helper function to ensure that we call PendingLookup::LookupNext or
   // PendingLookup::OnComplete.
   nsresult DoLookupInternal();
 
   // Looks up all the URIs that may be responsible for allowlisting or
   // blocklisting the downloaded file. These URIs may include whitelist strings
@@ -249,58 +253,58 @@ public:
   NS_DECL_NSIURLCLASSIFIERCALLBACK
 
   // Constructor and destructor
   explicit PendingDBLookup(PendingLookup* aPendingLookup);
 
   // Look up the given URI in the safebrowsing DBs, optionally on both the allow
   // list and the blocklist. If there is a match, call
   // PendingLookup::OnComplete. Otherwise, call PendingLookup::LookupNext.
-  nsresult LookupSpec(const nsACString& aSpec, bool aAllowlistOnly);
+  nsresult LookupSpec(const nsACString& aSpec, const LookupType& aLookupType);
 
 private:
   ~PendingDBLookup();
 
   // The download appeared on the allowlist, blocklist, or no list (and thus
   // could trigger a remote query.
   enum LIST_TYPES {
     ALLOW_LIST = 0,
     BLOCK_LIST = 1,
     NO_LIST = 2,
   };
 
   nsCString mSpec;
-  bool mAllowlistOnly;
+  LookupType mLookupType;
   RefPtr<PendingLookup> mPendingLookup;
   nsresult LookupSpecInternal(const nsACString& aSpec);
 };
 
 NS_IMPL_ISUPPORTS(PendingDBLookup,
                   nsIUrlClassifierCallback)
 
 PendingDBLookup::PendingDBLookup(PendingLookup* aPendingLookup) :
-  mAllowlistOnly(false),
+  mLookupType(LookupType::BothLists),
   mPendingLookup(aPendingLookup)
 {
   LOG(("Created pending DB lookup [this = %p]", this));
 }
 
 PendingDBLookup::~PendingDBLookup()
 {
   LOG(("Destroying pending DB lookup [this = %p]", this));
   mPendingLookup = nullptr;
 }
 
 nsresult
 PendingDBLookup::LookupSpec(const nsACString& aSpec,
-                            bool aAllowlistOnly)
+                            const LookupType& aLookupType)
 {
   LOG(("Checking principal %s [this=%p]", aSpec.Data(), this));
   mSpec = aSpec;
-  mAllowlistOnly = aAllowlistOnly;
+  mLookupType = aLookupType;
   nsresult rv = LookupSpecInternal(aSpec);
   if (NS_FAILED(rv)) {
     nsAutoCString errorName;
     mozilla::GetErrorName(rv, errorName);
     LOG(("Error in LookupSpecInternal() [rv = %s, this = %p]",
          errorName.get(), this));
     return mPendingLookup->LookupNext(); // ignore this lookup and move to next
   }
@@ -331,57 +335,59 @@ PendingDBLookup::LookupSpecInternal(cons
   LOG(("Checking DB service for principal %s [this = %p]", mSpec.get(), this));
   nsCOMPtr<nsIUrlClassifierDBService> dbService =
     do_GetService(NS_URLCLASSIFIERDBSERVICE_CONTRACTID, &rv);
   NS_ENSURE_SUCCESS(rv, rv);
 
   nsAutoCString tables;
   nsAutoCString allowlist;
   Preferences::GetCString(PREF_DOWNLOAD_ALLOW_TABLE, allowlist);
-  if (!allowlist.IsEmpty()) {
+  if ((mLookupType != LookupType::BlocklistOnly) && !allowlist.IsEmpty()) {
     tables.Append(allowlist);
   }
   nsAutoCString blocklist;
   Preferences::GetCString(PREF_DOWNLOAD_BLOCK_TABLE, blocklist);
-  if (!mAllowlistOnly && !blocklist.IsEmpty()) {
-    tables.Append(',');
+  if ((mLookupType != LookupType::AllowlistOnly) && !blocklist.IsEmpty()) {
+    if (!tables.IsEmpty()) {
+      tables.Append(',');
+    }
     tables.Append(blocklist);
   }
   return dbService->Lookup(principal, tables, this);
 }
 
 NS_IMETHODIMP
 PendingDBLookup::HandleEvent(const nsACString& tables)
 {
   // HandleEvent is guaranteed to call either:
   // 1) PendingLookup::OnComplete if the URL matches the blocklist, or
   // 2) PendingLookup::LookupNext if the URL does not match the blocklist.
   // Blocklisting trumps allowlisting.
   nsAutoCString blockList;
   Preferences::GetCString(PREF_DOWNLOAD_BLOCK_TABLE, blockList);
-  if (!mAllowlistOnly && FindInReadable(blockList, tables)) {
+  if ((mLookupType != LookupType::AllowlistOnly) && FindInReadable(blockList, tables)) {
     mPendingLookup->mBlocklistCount++;
     Accumulate(mozilla::Telemetry::APPLICATION_REPUTATION_LOCAL, BLOCK_LIST);
     LOG(("Found principal %s on blocklist [this = %p]", mSpec.get(), this));
     return mPendingLookup->OnComplete(true, NS_OK,
       nsIApplicationReputationService::VERDICT_DANGEROUS);
   }
 
   nsAutoCString allowList;
   Preferences::GetCString(PREF_DOWNLOAD_ALLOW_TABLE, allowList);
-  if (FindInReadable(allowList, tables)) {
+  if ((mLookupType != LookupType::BlocklistOnly) && FindInReadable(allowList, tables)) {
     mPendingLookup->mAllowlistCount++;
     Accumulate(mozilla::Telemetry::APPLICATION_REPUTATION_LOCAL, ALLOW_LIST);
     LOG(("Found principal %s on allowlist [this = %p]", mSpec.get(), this));
     // Don't call onComplete, since blocklisting trumps allowlisting
-  } else {
-    LOG(("Didn't find principal %s on any list [this = %p]", mSpec.get(),
-         this));
-    Accumulate(mozilla::Telemetry::APPLICATION_REPUTATION_LOCAL, NO_LIST);
+    return mPendingLookup->LookupNext();
   }
+
+  LOG(("Didn't find principal %s on any list [this = %p]", mSpec.get(), this));
+  Accumulate(mozilla::Telemetry::APPLICATION_REPUTATION_LOCAL, NO_LIST);
   return mPendingLookup->LookupNext();
 }
 
 NS_IMPL_ISUPPORTS(PendingLookup,
                   nsIStreamListener,
                   nsIRequestObserver,
                   nsIObserver,
                   nsISupportsWeakReference)
@@ -768,47 +774,60 @@ PendingLookup::GetDownloadType(const nsA
 }
 
 nsresult
 PendingLookup::LookupNext()
 {
   // We must call LookupNext or SendRemoteQuery upon return.
   // Look up all of the URLs that could allow or block this download.
   // Blocklist first.
+
+  // If any of mAnylistSpecs or mBlocklistSpecs matched the blocklist,
+  // go ahead and block.
   if (mBlocklistCount > 0) {
     return OnComplete(true, NS_OK,
                       nsIApplicationReputationService::VERDICT_DANGEROUS);
   }
+
   int index = mAnylistSpecs.Length() - 1;
   nsCString spec;
   if (index >= 0) {
-    // Check the source URI, referrer and redirect chain.
+    // Check the source URI only.
     spec = mAnylistSpecs[index];
     mAnylistSpecs.RemoveElementAt(index);
     RefPtr<PendingDBLookup> lookup(new PendingDBLookup(this));
-    return lookup->LookupSpec(spec, false);
+    return lookup->LookupSpec(spec, LookupType::BothLists);
   }
-  // If any of mAnylistSpecs matched the blocklist, go ahead and block.
-  if (mBlocklistCount > 0) {
-    return OnComplete(true, NS_OK,
-                      nsIApplicationReputationService::VERDICT_DANGEROUS);
+
+  index = mBlocklistSpecs.Length() - 1;
+  if (index >= 0) {
+    // Check the referrer and redirect chain.
+    spec = mBlocklistSpecs[index];
+    mBlocklistSpecs.RemoveElementAt(index);
+    RefPtr<PendingDBLookup> lookup(new PendingDBLookup(this));
+    return lookup->LookupSpec(spec, LookupType::BlocklistOnly);
   }
-  // If any of mAnylistSpecs matched the allowlist, go ahead and pass.
+
+  // Now that we've looked up all of the URIs against the blocklist,
+  // if any of mAnylistSpecs or mAllowlistSpecs matched the allowlist,
+  // go ahead and pass.
   if (mAllowlistCount > 0) {
     return OnComplete(false, NS_OK);
   }
+
   // Only binary signatures remain.
   index = mAllowlistSpecs.Length() - 1;
   if (index >= 0) {
     spec = mAllowlistSpecs[index];
     LOG(("PendingLookup::LookupNext: checking %s on allowlist", spec.get()));
     mAllowlistSpecs.RemoveElementAt(index);
     RefPtr<PendingDBLookup> lookup(new PendingDBLookup(this));
-    return lookup->LookupSpec(spec, true);
+    return lookup->LookupSpec(spec, LookupType::AllowlistOnly);
   }
+
   // There are no more URIs to check against local list. If the file is
   // not eligible for remote lookup, bail.
   if (!IsBinaryFile()) {
     LOG(("Not eligible for remote lookups [this=%p]", this));
     return OnComplete(false, NS_OK);
   }
   nsresult rv = SendRemoteQuery();
   if (NS_FAILED(rv)) {
@@ -981,17 +1000,17 @@ PendingLookup::AddRedirects(nsIArray* aR
     rv = principal->GetURI(getter_AddRefs(uri));
     NS_ENSURE_SUCCESS(rv, rv);
 
     // Add the spec to our list of local lookups. The most recent redirect is
     // the last element.
     nsCString spec;
     rv = GetStrippedSpec(uri, spec);
     NS_ENSURE_SUCCESS(rv, rv);
-    mAnylistSpecs.AppendElement(spec);
+    mBlocklistSpecs.AppendElement(spec);
     LOG(("ApplicationReputation: Appending redirect %s\n", spec.get()));
 
     // Store the redirect information in the remote request.
     ClientDownloadRequest_Resource* resource = mRequest.add_resources();
     resource->set_url(spec.get());
     resource->set_type(ClientDownloadRequest::DOWNLOAD_REDIRECT);
 
     rv = iter->HasMoreElements(&hasMoreRedirects);
@@ -1127,17 +1146,17 @@ PendingLookup::DoLookupInternal()
   resource->set_type(ClientDownloadRequest::DOWNLOAD_URL);
 
   nsCOMPtr<nsIURI> referrer = nullptr;
   rv = mQuery->GetReferrerURI(getter_AddRefs(referrer));
   if (referrer) {
     nsCString referrerSpec;
     rv = GetStrippedSpec(referrer, referrerSpec);
     NS_ENSURE_SUCCESS(rv, rv);
-    mAnylistSpecs.AppendElement(referrerSpec);
+    mBlocklistSpecs.AppendElement(referrerSpec);
     resource->set_referrer(referrerSpec.get());
   }
   nsCOMPtr<nsIArray> redirects;
   rv = mQuery->GetRedirects(getter_AddRefs(redirects));
   if (redirects) {
     AddRedirects(redirects);
   } else {
     LOG(("ApplicationReputation: Got no redirects [this=%p]", this));
--- a/toolkit/components/reputationservice/test/unit/test_app_rep.js
+++ b/toolkit/components/reputationservice/test/unit/test_app_rep.js
@@ -264,16 +264,17 @@ add_test(function test_local_blacklist()
 });
 
 add_test(function test_referer_blacklist() {
   Services.prefs.setCharPref(appRepURLPref,
                              "http://localhost:4444/download");
   let counts = get_telemetry_counts();
   let listCounts = counts.listCounts;
   listCounts[BLOCK_LIST]++;
+  listCounts[NO_LIST]++;
   gAppRep.queryReputation({
     sourceURI: exampleURI,
     referrerURI: blocklistedURI,
     fileSize: 12,
   }, function onComplete(aShouldBlock, aStatus) {
     Assert.equal(Cr.NS_OK, aStatus);
     Assert.ok(aShouldBlock);
     check_telemetry(counts.shouldBlock + 1, listCounts);
@@ -282,16 +283,17 @@ add_test(function test_referer_blacklist
 });
 
 add_test(function test_blocklist_trumps_allowlist() {
   Services.prefs.setCharPref(appRepURLPref,
                              "http://localhost:4444/download");
   let counts = get_telemetry_counts();
   let listCounts = counts.listCounts;
   listCounts[BLOCK_LIST]++;
+  listCounts[ALLOW_LIST]++;
   gAppRep.queryReputation({
     sourceURI: whitelistedURI,
     referrerURI: blocklistedURI,
     fileSize: 12,
   }, function onComplete(aShouldBlock, aStatus) {
     Assert.equal(Cr.NS_OK, aStatus);
     Assert.ok(aShouldBlock);
     check_telemetry(counts.shouldBlock + 1, listCounts);
@@ -301,32 +303,35 @@ add_test(function test_blocklist_trumps_
 
 add_test(function test_redirect_on_blocklist() {
   Services.prefs.setCharPref(appRepURLPref,
                              "http://localhost:4444/download");
   let counts = get_telemetry_counts();
   let listCounts = counts.listCounts;
   listCounts[BLOCK_LIST]++;
   listCounts[ALLOW_LIST]++;
+  listCounts[NO_LIST]++;
   let secman = Services.scriptSecurityManager;
   let badRedirects = Cc["@mozilla.org/array;1"]
                        .createInstance(Ci.nsIMutableArray);
 
   let redirect1 = {
     QueryInterface: XPCOMUtils.generateQI([Ci.nsIRedirectHistoryEntry]),
     principal: secman.createCodebasePrincipal(exampleURI, {}),
   };
   badRedirects.appendElement(redirect1);
 
   let redirect2 = {
     QueryInterface: XPCOMUtils.generateQI([Ci.nsIRedirectHistoryEntry]),
     principal: secman.createCodebasePrincipal(blocklistedURI, {}),
   };
   badRedirects.appendElement(redirect2);
 
+  // Add a whitelisted URI that will not be looked up against the
+  // whitelist (i.e. it will match NO_LIST).
   let redirect3 = {
     QueryInterface: XPCOMUtils.generateQI([Ci.nsIRedirectHistoryEntry]),
     principal: secman.createCodebasePrincipal(whitelistedURI, {}),
   };
   badRedirects.appendElement(redirect3);
 
   gAppRep.queryReputation({
     sourceURI: whitelistedURI,
@@ -335,8 +340,79 @@ add_test(function test_redirect_on_block
     fileSize: 12,
   }, function onComplete(aShouldBlock, aStatus) {
     Assert.equal(Cr.NS_OK, aStatus);
     Assert.ok(aShouldBlock);
     check_telemetry(counts.shouldBlock + 1, listCounts);
     run_next_test();
   });
 });
+
+add_test(function test_whitelisted_source() {
+  Services.prefs.setCharPref(appRepURLPref,
+                             "http://localhost:4444/download");
+  let counts = get_telemetry_counts();
+  let listCounts = counts.listCounts;
+  listCounts[ALLOW_LIST]++;
+  gAppRep.queryReputation({
+    sourceURI: whitelistedURI,
+    fileSize: 12,
+  }, function onComplete(aShouldBlock, aStatus) {
+    Assert.equal(Cr.NS_OK, aStatus);
+    Assert.ok(!aShouldBlock);
+    check_telemetry(counts.shouldBlock, listCounts);
+    run_next_test();
+  });
+});
+
+add_test(function test_whitelisted_referrer() {
+  Services.prefs.setCharPref(appRepURLPref,
+                             "http://localhost:4444/download");
+  let counts = get_telemetry_counts();
+  let listCounts = counts.listCounts;
+  listCounts[NO_LIST] += 2;
+  gAppRep.queryReputation({
+    sourceURI: exampleURI,
+    referrerURI: whitelistedURI,
+    fileSize: 12,
+  }, function onComplete(aShouldBlock, aStatus) {
+    Assert.equal(Cr.NS_OK, aStatus);
+    Assert.ok(!aShouldBlock);
+    check_telemetry(counts.shouldBlock, listCounts);
+    run_next_test();
+  });
+});
+
+add_test(function test_whitelisted_redirect() {
+  Services.prefs.setCharPref(appRepURLPref,
+                             "http://localhost:4444/download");
+  let counts = get_telemetry_counts();
+  let listCounts = counts.listCounts;
+  listCounts[NO_LIST] += 3;
+  let secman = Services.scriptSecurityManager;
+  let okayRedirects = Cc["@mozilla.org/array;1"]
+                       .createInstance(Ci.nsIMutableArray);
+
+  let redirect1 = {
+    QueryInterface: XPCOMUtils.generateQI([Ci.nsIRedirectHistoryEntry]),
+    principal: secman.createCodebasePrincipal(exampleURI, {}),
+  };
+  okayRedirects.appendElement(redirect1);
+
+  // Add a whitelisted URI that will not be looked up against the
+  // whitelist (i.e. it will match NO_LIST).
+  let redirect2 = {
+    QueryInterface: XPCOMUtils.generateQI([Ci.nsIRedirectHistoryEntry]),
+    principal: secman.createCodebasePrincipal(whitelistedURI, {}),
+  };
+  okayRedirects.appendElement(redirect2);
+
+  gAppRep.queryReputation({
+    sourceURI: exampleURI,
+    redirects: okayRedirects,
+    fileSize: 12,
+  }, function onComplete(aShouldBlock, aStatus) {
+    Assert.equal(Cr.NS_OK, aStatus);
+    Assert.ok(!aShouldBlock);
+    check_telemetry(counts.shouldBlock, listCounts);
+    run_next_test();
+  });
+});