Bug 1356427 - P2. Only check binary files against the Application Reputation whitelist, r=francois
authorDimi Lee <dlee@mozilla.com>
Thu, 18 Oct 2018 14:40:56 +0000
changeset 500392 c3e09876ab13c1b27355f2d0680b0e34a40bbcf4
parent 500391 ced08ec47ace994bf5c5f1e4052052e7cb9bab30
child 500393 6e0c72599e0b63fe2e4957184c40520c2fa6339c
push id1864
push userffxbld-merge
push dateMon, 03 Dec 2018 15:51:40 +0000
treeherdermozilla-release@f040763d99ad [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersfrancois
bugs1356427, 1434741
milestone64.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 1356427 - P2. Only check binary files against the Application Reputation whitelist, r=francois We already skip checking whitelist for redirects and referrer in Bug 1434741 So this patch just does the following to remove redundant whitelist check: 1. Skip checking whitelist for the final URL if it is not a binary 2. Skip parsing certificate for non-binary files After applying this patch, whitelist will only be used for final URL of binary file and certificates of binary file Differential Revision: https://phabricator.services.mozilla.com/D8687
toolkit/components/reputationservice/ApplicationReputation.cpp
--- a/toolkit/components/reputationservice/ApplicationReputation.cpp
+++ b/toolkit/components/reputationservice/ApplicationReputation.cpp
@@ -130,16 +130,23 @@ private:
   // Telemetry states.
   // Status of the remote response (valid or not).
   enum SERVER_RESPONSE_TYPES {
     SERVER_RESPONSE_VALID = 0,
     SERVER_RESPONSE_FAILED = 1,
     SERVER_RESPONSE_INVALID = 2,
   };
 
+  // The target filename for the downloaded file.
+  nsCString mFileName;
+
+  // True if extension of this file matches any extension in the
+  // kBinaryFileExtensions list.
+  bool mIsBinaryFile;
+
   // Number of blocklist and allowlist hits we have seen.
   uint32_t mBlocklistCount;
   uint32_t mAllowlistCount;
 
   // The query containing metadata about the downloaded file.
   nsCOMPtr<nsIApplicationReputationQuery> mQuery;
 
   // The callback with which to report the verdict.
@@ -392,16 +399,17 @@ PendingDBLookup::HandleEvent(const nsACS
 NS_IMPL_ISUPPORTS(PendingLookup,
                   nsIStreamListener,
                   nsIRequestObserver,
                   nsIObserver,
                   nsISupportsWeakReference)
 
 PendingLookup::PendingLookup(nsIApplicationReputationQuery* aQuery,
                              nsIApplicationReputationCallback* aCallback) :
+  mIsBinaryFile(false),
   mBlocklistCount(0),
   mAllowlistCount(0),
   mQuery(aQuery),
   mCallback(aCallback)
 {
   LOG(("Created pending lookup [this = %p]", this));
 }
 
@@ -854,17 +862,20 @@ PendingLookup::LookupNext()
 
   int index = mAnylistSpecs.Length() - 1;
   nsCString spec;
   if (index >= 0) {
     // Check the source URI only.
     spec = mAnylistSpecs[index];
     mAnylistSpecs.RemoveElementAt(index);
     RefPtr<PendingDBLookup> lookup(new PendingDBLookup(this));
-    return lookup->LookupSpec(spec, LookupType::BothLists);
+
+    // We don't need to check whitelist if the file is not a binary file.
+    auto type = mIsBinaryFile ? LookupType::BothLists : LookupType::BlocklistOnly;
+    return lookup->LookupSpec(spec, type);
   }
 
   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));
@@ -873,63 +884,57 @@ PendingLookup::LookupNext()
 
   // 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);
   }
 
+  MOZ_ASSERT_IF(!mIsBinaryFile, mAllowlistSpecs.Length() == 0);
+
   // 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, LookupType::AllowlistOnly);
   }
 
-  // Check whether or not the file is eligible for remote lookups.
-  bool isBinaryFile = false;
-  nsAutoCString fileName;
-  nsresult rv = mQuery->GetSuggestedFileName(fileName);
-  if (NS_SUCCEEDED(rv) && !fileName.IsEmpty()) {
-    LOG(("Suggested filename: %s [this = %p]", fileName.get(), this));
-    isBinaryFile = IsFileType(fileName, kBinaryFileExtensions,
-                              ArrayLength(kBinaryFileExtensions));
-    AccumulateCategorical(isBinaryFile ?
+  if (!mFileName.IsEmpty()) {
+    AccumulateCategorical(mIsBinaryFile ?
                           mozilla::Telemetry::LABELS_APPLICATION_REPUTATION_BINARY::BinaryFile :
                           mozilla::Telemetry::LABELS_APPLICATION_REPUTATION_BINARY::NonBinaryFile);
   } else {
-    LOG(("No suggested filename [this = %p]", this));
     AccumulateCategorical(mozilla::Telemetry::LABELS_APPLICATION_REPUTATION_BINARY::MissingFilename);
   }
 
-  if (IsFileType(fileName, kDmgFileExtensions,
+  if (IsFileType(mFileName, kDmgFileExtensions,
                  ArrayLength(kDmgFileExtensions))) {
     AccumulateCategorical(mozilla::Telemetry::LABELS_APPLICATION_REPUTATION_BINARY_ARCHIVE::DmgFile);
-  } else if (IsFileType(fileName, kRarFileExtensions,
+  } else if (IsFileType(mFileName, kRarFileExtensions,
                         ArrayLength(kRarFileExtensions))) {
     AccumulateCategorical(mozilla::Telemetry::LABELS_APPLICATION_REPUTATION_BINARY_ARCHIVE::RarFile);
-  } else if (IsFileType(fileName, kZipFileExtensions,
+  } else if (IsFileType(mFileName, kZipFileExtensions,
                         ArrayLength(kZipFileExtensions))) {
     AccumulateCategorical(mozilla::Telemetry::LABELS_APPLICATION_REPUTATION_BINARY_ARCHIVE::ZipFile);
-  } else if (isBinaryFile) {
+  } else if (mIsBinaryFile) {
     AccumulateCategorical(mozilla::Telemetry::LABELS_APPLICATION_REPUTATION_BINARY_ARCHIVE::OtherBinaryFile);
   }
 
   // There are no more URIs to check against local list. If the file is
   // not eligible for remote lookup, bail.
-  if (!isBinaryFile) {
+  if (!mIsBinaryFile) {
     LOG(("Not eligible for remote lookups [this=%p]", this));
     return OnComplete(false, NS_OK);
   }
 
-  rv = SendRemoteQuery();
+  nsresult rv = SendRemoteQuery();
   if (NS_FAILED(rv)) {
     return OnComplete(false, rv);
   }
   return NS_OK;
 }
 
 nsCString
 PendingLookup::EscapeCertificateAttribute(const nsACString& aAttribute)
@@ -1252,30 +1257,45 @@ PendingLookup::DoLookupInternal()
   nsCOMPtr<nsIArray> redirects;
   rv = mQuery->GetRedirects(getter_AddRefs(redirects));
   if (redirects) {
     AddRedirects(redirects);
   } else {
     LOG(("ApplicationReputation: Got no redirects [this=%p]", this));
   }
 
-  // Extract the signature and parse certificates so we can use it to check
-  // whitelists.
-  nsCOMPtr<nsIArray> sigArray;
-  rv = mQuery->GetSignatureInfo(getter_AddRefs(sigArray));
-  NS_ENSURE_SUCCESS(rv, rv);
+  rv = mQuery->GetSuggestedFileName(mFileName);
+  if (NS_SUCCEEDED(rv) && !mFileName.IsEmpty()) {
+    mIsBinaryFile = IsFileType(mFileName, kBinaryFileExtensions,
+                               ArrayLength(kBinaryFileExtensions));
+    LOG(("Suggested filename: %s [binary = %d, this = %p]",
+         mFileName.get(), mIsBinaryFile, this));
+  } else {
+    nsAutoCString errorName;
+    mozilla::GetErrorName(rv, errorName);
+    LOG(("No suggested filename [rv = %s, this = %p]", errorName.get(), this));
+    mFileName = EmptyCString();
+  }
 
-  if (sigArray) {
-    rv = ParseCertificates(sigArray);
+  // We can skip parsing certificate for non-binary files because we only
+  // check local block list for them.
+  if (mIsBinaryFile) {
+    nsCOMPtr<nsIArray> sigArray;
+    rv = mQuery->GetSignatureInfo(getter_AddRefs(sigArray));
+    NS_ENSURE_SUCCESS(rv, rv);
+
+    if (sigArray) {
+      rv = ParseCertificates(sigArray);
+      NS_ENSURE_SUCCESS(rv, rv);
+    }
+
+    rv = GenerateWhitelistStrings();
     NS_ENSURE_SUCCESS(rv, rv);
   }
 
-  rv = GenerateWhitelistStrings();
-  NS_ENSURE_SUCCESS(rv, rv);
-
   // Start the call chain.
   return LookupNext();
 }
 
 nsresult
 PendingLookup::OnComplete(bool shouldBlock, nsresult rv, uint32_t verdict)
 {
   MOZ_ASSERT(!shouldBlock ||
@@ -1447,21 +1467,18 @@ PendingLookup::SendRemoteQueryInternal()
   nsCString locale;
   rv = LocaleService::GetInstance()->GetAppLocaleAsLangTag(locale);
   NS_ENSURE_SUCCESS(rv, rv);
   mRequest.set_locale(locale.get());
   nsCString sha256Hash;
   rv = mQuery->GetSha256Hash(sha256Hash);
   NS_ENSURE_SUCCESS(rv, rv);
   mRequest.mutable_digests()->set_sha256(sha256Hash.Data());
-  nsCString fileName;
-  rv = mQuery->GetSuggestedFileName(fileName);
-  NS_ENSURE_SUCCESS(rv, rv);
-  mRequest.set_file_basename(fileName.get());
-  mRequest.set_download_type(GetDownloadType(fileName));
+  mRequest.set_file_basename(mFileName.get());
+  mRequest.set_download_type(GetDownloadType(mFileName));
 
   if (mRequest.signature().trusted()) {
     LOG(("Got signed binary for remote application reputation check "
          "[this = %p]", this));
   } else {
     LOG(("Got unsigned binary for remote application reputation check "
          "[this = %p]", this));
   }