Adding a new delayed analysis that verify binaries signature.

chrome/browser/safe_browsing/binary_integrity_service_win.cc

+// Copyright 2014 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/binary_integrity_service.h"
+
+#include "base/callback.h"
+#include "base/files/file_path.h"
+#include "base/file_util.h"
+#include "base/logging.h"
+#include "base/path_service.h"
+#include "base/strings/stringprintf.h"
+#include "base/strings/utf_string_conversions.h"
+#include "chrome/browser/safe_browsing/binary_feature_extractor.h"
+#include "chrome/common/safe_browsing/csd.pb.h"
+
+namespace safe_browsing {
+
+void VerifyBinaryIntegrity(const AddIncidentCallback& callback) {
+  scoped_refptr<BinaryFeatureExtractor> binary_feature_extractor(
+      new BinaryFeatureExtractor());
+
+  // Hold to the same signature info instance until we use it in a report.

[robertshield, 2014/08/07 14:57:24]

s/Hold/Hold on/

[pmonette_google.com, 2014/08/07 21:29:29]

Done.

+  scoped_ptr<ClientDownloadRequest_SignatureInfo> signature_info(
+      new ClientDownloadRequest_SignatureInfo());

[mattm, 2014/08/07 20:16:01]

I think a new SignatureInfo needs to be created for each call to CheckSignature,
or it needs to be cleared between calls, otherwise it looks like data from
multiple calls could get mashed together.

[pmonette_google.com, 2014/08/07 21:29:29]

That is true. Done.

+
+  std::vector<base::FilePath> critical_binaries = GetCriticalBinariesPath();
+  for (size_t i = 0; i < critical_binaries.size(); ++i) {
+    base::FilePath binary_path(critical_binaries[i]);
+    if (!base::PathExists(binary_path))
+      continue;
+
+    binary_feature_extractor->CheckSignature(binary_path, signature_info.get());
+
+    // Only create a report if the signature is untrusted.
+    if (!signature_info->trusted()) {
+      scoped_ptr<ClientIncidentReport_IncidentData> incident_data(
+          new ClientIncidentReport_IncidentData());
+      ClientIncidentReport_IncidentData_BinaryIntegrityIncident*
+          binary_integrity = incident_data->mutable_binary_integrity();
+
+      binary_integrity->set_file(base::WideToUTF8(binary_path.value()));

[grt (UTC plus 2), 2014/08/07 02:12:46]

you can use binary_path.AsUTF8Unsafe() here

[pmonette_google.com, 2014/08/07 21:29:29]

Done.

+      binary_integrity->set_allocated_signature(signature_info.release());
+
+      // Send the report.
+      callback.Run(incident_data.Pass());
+
+      signature_info.reset(new ClientDownloadRequest_SignatureInfo());
+    }
+  }
+}
+
+std::vector<base::FilePath> GetCriticalBinariesPath() {
+  const wchar_t* const critical_binary_name[] = {

[grt (UTC plus 2), 2014/08/07 02:12:46]

static

[pmonette_google.com, 2014/08/07 21:29:29]

Done.

+      L"%ls\\chrome.dll", L"%ls\\chrome_elf.dll", L"chrome.exe",
+  };
+
+  // Find where chrome.exe is installed.
+  base::FilePath chrome_exe_dir;
+  if (!PathService::Get(base::DIR_EXE, &chrome_exe_dir))
+    NOTREACHED();
+
+  std::vector<base::FilePath> critical_binary;

[mattm, 2014/08/07 20:16:01]

critical_binaries?

[pmonette_google.com, 2014/08/07 21:29:29]

Done.

+
+  for (size_t i = 0; i < arraysize(critical_binary_name); ++i) {

[grt (UTC plus 2), 2014/08/07 02:12:46]

i like the use of the array to build the output, but i don't like that the array
contains different kinds of items (some in the version dir, one that isn't) for
which a single sprintf is used. i think it'd be cleaner to have something like:

  static const base::FilePath::CharType* const kUnversionedFiles[] = {
    FILE_PATH_LITERAL("chrome.exe"),
  };
  static const base::FilePath::CharType* const kVersionedFIles[] = {
    FILE_PATH_LITERAL("chrome.dll"),
    FILE_PATH_LITERAL("chrome_elf.dll"),
  };

and then process these separately:

  critical_binary.reserve(arraysize(kUnversionedFIles) +
arraysize(kVersionedFIles));
  // append each unversioned file to chrome_exe_dir and push_back
  base::FilePath version_dir(chrome_exe_dir.Append(chrome_version));
  // append each versioned file to version_dir and push_back

[pmonette_google.com, 2014/08/07 21:29:28]

Done.

+    base::string16 binary_name =
+        base::StringPrintf(critical_binary_name[i], GetChromeVersion().c_str());

[grt (UTC plus 2), 2014/08/07 02:12:46]

remove GetChromeVersion altogether and either use chrome::VersionInfo outside of
the loop to get the version string, or #include "version.h" and use the macro
CHROME_VERSION_STRING, which expands to a string literal.

[pmonette_google.com, 2014/08/07 21:29:29]

Went with the second suggestion.

+    critical_binary.push_back(chrome_exe_dir.Append(binary_name));
+  }
+
+  return critical_binary;
+}
+
+}  // namespace safe_browsing