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 490297 c3e09876ab13c1b27355f2d0680b0e34a40bbcf4
parent 490296 ced08ec47ace994bf5c5f1e4052052e7cb9bab30
child 490298 6e0c72599e0b63fe2e4957184c40520c2fa6339c
push id247
push userfmarier@mozilla.com
push dateSat, 27 Oct 2018 01:06:44 +0000
reviewersfrancois
bugs1356427, 1434741
milestone64.0a1
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));
   }