Add a ModuleLoadAnalyzer which checks modules against a whitelist

chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win.cc

+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/safe_browsing/incident_reporting/module_load_analyzer.h"
+
+#include "base/file_version_info.h"
+#include "base/files/file_path.h"
+#include "base/logging.h"
+#include "base/metrics/histogram.h"
+#include "base/strings/string_number_conversions.h"
+#include "base/strings/string_util.h"
+#include "base/strings/utf_string_conversions.h"
+#include "chrome/browser/browser_process.h"
+#include "chrome/browser/install_verification/win/module_info.h"
+#include "chrome/browser/install_verification/win/module_verification_common.h"
+#include "chrome/browser/safe_browsing/incident_reporting/incident_receiver.h"
+#include "chrome/browser/safe_browsing/incident_reporting/suspicious_module_incident.h"
+#include "chrome/browser/safe_browsing/path_sanitizer.h"
+#include "chrome/browser/safe_browsing/safe_browsing_service.h"
+#include "chrome/common/safe_browsing/binary_feature_extractor.h"
+#include "chrome/common/safe_browsing/csd.pb.h"
+#include "crypto/sha2.h"
+
+#if defined(SAFE_BROWSING_DB_LOCAL)
+#include "chrome/browser/safe_browsing/local_database_manager.h"
+#elif defined(SAFE_BROWSING_DB_REMOTE)
+#include "chrome/browser/safe_browsing/remote_database_manager.h"
+#endif
+
+namespace safe_browsing {
+
+// Retrieves the set of suspicious modules that are loaded in the process.
+void ModuleLoadAnalyzer::GetLoadedSuspiciousModulesOnIOThread() {
+  std::set<ModuleInfo> module_info_set;
+  if (!GetLoadedModules(&module_info_set))
+    return;
+
+  std::set<base::string16>* suspicious_names = new std::set<base::string16>();

[robertshield, 2016/02/05 21:20:32]

could you stick this right away in a scoped_ptr? base::Passed() below will do
the right thing given a scoped_ptr since it is a movable type. 

[proberge, 2016/02/05 22:26:10]

Great catch, curently I would leak the set if suspicious_names->empty().

+
+  std::set<ModuleInfo>::const_iterator module_iter(module_info_set.begin());
+  for (; module_iter != module_info_set.end(); ++module_iter) {
+    base::string16 module_file_name(base::ToLowerASCII(
+        base::FilePath(module_iter->name).BaseName().value()));
+
+    // If not whitelisted.
+    if (!database_manager_->MatchModuleWhitelistString(
+            base::UTF16ToUTF8(module_file_name)))
+      suspicious_names->insert(module_iter->name);
+  }
+
+  if (!suspicious_names->empty()) {
+    scoped_refptr<base::TaskRunner> task_runner =
+        content::BrowserThread::GetBlockingPool()
+            ->GetTaskRunnerWithShutdownBehavior(
+                base::SequencedWorkerPool::SKIP_ON_SHUTDOWN);
+
+    task_runner->PostTask(
+        FROM_HERE,
+        base::Bind(&ModuleLoadAnalyzer::ReportIncidentsForSuspiciousModules,
+                   base::Unretained(this),
+                   base::Passed(make_scoped_ptr(suspicious_names))));
+  }
+}
+
+void ModuleLoadAnalyzer::ReportIncidentsForSuspiciousModules(
+    scoped_ptr<std::set<base::string16>> module_names) {
+  PathSanitizer path_sanitizer;
+  scoped_ptr<IncidentReceiver> incident_receiver =
+      std::move(incident_receiver_);
+
+  for (const auto& module_name : *module_names) {
+    if (abort_reporting_)
+      return;
+
+    // TODO(proberge): Skip over modules that have already been reported.
+
+    scoped_ptr<ClientIncidentReport_IncidentData_SuspiciousModuleIncident>
+        suspicious_module(
+            new ClientIncidentReport_IncidentData_SuspiciousModuleIncident());
+
+    const base::FilePath module_path(module_name);
+
+    // Sanitized path.
+    base::FilePath sanitized_path(module_path);
+    path_sanitizer.StripHomeDirectory(&sanitized_path);
+    suspicious_module->set_path(base::WideToUTF8(sanitized_path.value()));
+
+    // Digest.
+    scoped_refptr<BinaryFeatureExtractor> binary_feature_extractor(
+        new BinaryFeatureExtractor());
+    base::TimeTicks start_time = base::TimeTicks::Now();
+    binary_feature_extractor->ExtractDigest(
+        module_path, suspicious_module->mutable_digest());
+    UMA_HISTOGRAM_TIMES("SBIRS.BLAHashTime",

[robertshield, 2016/02/05 21:20:32]

This will fire a histogram for each item in the module list, would it be better
to accumulate the total time spent and report a histogram once?

[proberge, 2016/02/05 22:26:10]

Yes. Removed the existing histograms (used by the blacklist incident) and
replaced with 3 different histograms.

+                        base::TimeTicks::Now() - start_time);
+
+    // Version.
+    scoped_ptr<FileVersionInfo> version_info(
+        FileVersionInfo::CreateFileVersionInfo(module_path));
+    if (version_info) {
+      std::wstring file_version = version_info->file_version();
+      if (!file_version.empty())
+        suspicious_module->set_version(base::WideToUTF8(file_version));
+    }
+
+    // Signature.
+    start_time = base::TimeTicks::Now();
+    binary_feature_extractor->CheckSignature(
+        module_path, suspicious_module->mutable_signature());
+    UMA_HISTOGRAM_TIMES("SBIRS.BLASignatureTime",
+                        base::TimeTicks::Now() - start_time);
+
+    // Image headers.
+    if (!binary_feature_extractor->ExtractImageFeatures(
+            module_path, BinaryFeatureExtractor::kDefaultOptions,
+            suspicious_module->mutable_image_headers(),
+            nullptr /* signed_data */)) {
+      suspicious_module->clear_image_headers();
+    }
+
+    // Send the report.
+    incident_receiver->AddIncidentForProcess(make_scoped_ptr(
+        new SuspiciousModuleIncident(std::move(suspicious_module))));
+  }
+}
+
+void ModuleLoadAnalyzer::VerifyModuleLoadState(
+    scoped_ptr<IncidentReceiver> incident_receiver) {
+  incident_receiver_ = std::move(incident_receiver);

[robertshield, 2016/02/05 21:20:32]

keeping the incident_receiver_ in a local var that is cleared during this
process makes me wonder about what will happen if VerifyModuleLoadState is
called twice in succession. Seems like there are a number of permutations of
callbacks that make that a little hard to predict. If this is correct, might be
worth adding a DCHECK here to guard against multiple calls to
VerifyModuleLoadState on the same instance of a ModuleLoadAnalyzer.

[proberge, 2016/02/05 22:26:10]

Done.

+
+  // PostTaskAndReply doesn't work here because we're in a sequenced blocking
+  // thread pool.
+  content::BrowserThread::PostTask(
+      content::BrowserThread::IO, FROM_HERE,
+      base::Bind(&ModuleLoadAnalyzer::GetLoadedSuspiciousModulesOnIOThread,
+                 base::Unretained(this)));
+}
+
+}  // namespace safe_browsing