Support for recording registered LSPs

chrome/browser/safe_browsing/environment_data_collection_win.cc

Index: chrome/browser/safe_browsing/environment_data_collection_win.cc
diff --git a/chrome/browser/safe_browsing/environment_data_collection_win.cc b/chrome/browser/safe_browsing/environment_data_collection_win.cc
index ef962358909e5a2af2d26ac45e1ee7c7fac7e2dd..d44ebd2252608d3dd6ef81fb358e1797f43d3bab 100644
--- a/chrome/browser/safe_browsing/environment_data_collection_win.cc
+++ b/chrome/browser/safe_browsing/environment_data_collection_win.cc
@@ -1,16 +1,97 @@
-// 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/environment_data_collection_win.h"
-
-#include "chrome/common/safe_browsing/csd.pb.h"
-
-namespace safe_browsing {
-
-void CollectPlatformProcessData(
-    ClientIncidentReport_EnvironmentData_Process* process) {
-  // TODO(pmonette): collect dlls and lsps.
-}
-
-}  // namespace safe_browsing
+// 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/environment_data_collection_win.h"
+
+#include <windows.h>
+#include <set>
+#include <string>
+#include <vector>
+
+#include "base/files/file_path.h"
+#include "base/strings/utf_string_conversions.h"
+#include "chrome/browser/install_verification/win/module_info.h"
+#include "chrome/browser/install_verification/win/module_verification_common.h"
+#include "chrome/browser/net/service_providers_win.h"
+#include "chrome/browser/safe_browsing/environment_data_collection.h"
+#include "chrome/browser/safe_browsing/path_sanitizer.h"
+#include "chrome/common/safe_browsing/csd.pb.h"
+
+namespace safe_browsing {
+
+bool CollectDlls(ClientIncidentReport_EnvironmentData_Process* process) {
+  // Retrieve the module list.
+  std::set<ModuleInfo> loaded_modules;
+  if (!GetLoadedModules(&loaded_modules))
+    return false;
+
+  PathSanitizer path_sanitizer;
+
+  // Sanitize path of each module and add it to the incident report.
+  for (std::set<ModuleInfo>::const_iterator it = loaded_modules.begin();
+       it != loaded_modules.end();
+       ++it) {
+    base::FilePath dll_path(it->name);
+    path_sanitizer.StripHomeDirectory(&dll_path);
+
+    AddDll(base::WideToUTF8(dll_path.value()),
+           it->base_address,
+           it->size,
+           process);
+  }
+
+  return true;
+}
+
+void VerifyLSP(
+    safe_browsing::ClientIncidentReport_EnvironmentData_Process* process) {

[grt (UTC plus 2), 2014/06/10 13:47:53]

"safe_browsing::" is not needed here

[pmonette_google.com, 2014/06/10 19:34:38]

Done.

+  WinsockLayeredServiceProviderList lsp_list;
+  GetWinsockLayeredServiceProviders(&lsp_list);
+
+  // For each LSP, we extract and sanitize the path.
+  PathSanitizer path_sanitizer;
+  std::vector<std::wstring> lsp_paths(lsp_list.size());

[grt (UTC plus 2), 2014/06/10 13:47:53]

use base::string16 rather than std::wstring

[pmonette_google.com, 2014/06/10 19:34:38]

Done.

+  for (unsigned int i = 0; i < lsp_list.size(); ++i) {

[grt (UTC plus 2), 2014/06/10 13:47:53]

unsigned int -> size_t (use size_t for the size_type of all STL containers)

[pmonette_google.com, 2014/06/10 19:34:39]

Done.

+    lsp_paths[i] = ExpandEnvironmentVariables(lsp_list[i].path);
+    path_sanitizer.StripHomeDirectory(&lsp_paths[i]);
+  }
+
+  // Remove duplicates.
+  lsp_paths.erase(unique(lsp_paths.begin(), lsp_paths.end()), lsp_paths.end());

[grt (UTC plus 2), 2014/06/10 13:47:53]

unique -> std::unique
std::unique requires that lsp_paths be sorted. it's not obvious that it is.
consider using a std::set for lsp_paths so you don't need to worry about the
duplicate removal.

[pmonette_google.com, 2014/06/10 19:34:38]

Done. I just went for std::set.

+
+  // Look for a match between LSPs and loaded dlls.
+  for (unsigned int i = 0; i < lsp_paths.size(); ++i) {

[grt (UTC plus 2), 2014/06/10 13:47:53]

if you use a std::set for lsp_paths, you can make this O(MlogN) rather than
O(MN) by making the outer loop over process->dlls and then check for
lsp_paths.count(process->dlls(i).path()). when that's non-zero, you've found a
match.

[grt (UTC plus 2), 2014/06/10 13:47:53]

unsigned int -> size_t

[pmonette_google.com, 2014/06/10 19:34:39]

Yes. My intent was to add a break in the if statement but somehow I forgot.
Anyway, using count() makes the code way easier to read. Do you think that
lsp_paths.find() != lsp_paths.end() would be even more descriptive?

+    for (int j = 0; j < process->dlls_size(); ++j) {
+      if (base::WideToUTF8(lsp_paths[i]) == process->dlls(j).path())
+        process->mutable_dlls(j)->add_features(
+            ClientIncidentReport_EnvironmentData_Process_DLL_Feature_LSP);
+    }

[grt (UTC plus 2), 2014/06/10 13:47:53]

if there is no match, that means that the LSP was not present in the browser
process. do we still want to report those LSPs somehow? perhaps this iteration
should remove the LSPs from lsp_paths as they are logged in the protobuf, and
then do something interesting with those that are left over. minimally, we could
send the count up by UMA so that we know this is happening. let's discuss in
person.

[pmonette_google.com, 2014/06/10 19:34:39]

We talked about this. No match is an expected behavior when no tampering has
been done. The "leftovers" are just LSPs that are registered in Windows but
aren't loaded in Chrome.

+  }
+}
+
+void CollectPlatformProcessData(
+    ClientIncidentReport_EnvironmentData_Process* process) {
+  CollectDlls(process);
+  VerifyLSP(process);
+}
+
+base::string16 ExpandEnvironmentVariables(const base::string16& path) {
+  wchar_t path_expanded[MAX_PATH + 1] = {0};

[grt (UTC plus 2), 2014/06/10 13:47:53]

nit: {0} -> {}

[pmonette_google.com, 2014/06/10 19:34:38]

Done.

+  ExpandEnvironmentStrings(path.c_str(), path_expanded, MAX_PATH);

[grt (UTC plus 2), 2014/06/10 13:47:53]

you need to check the return value here since there are failure modes. also, the
paths could be longer than MAX_PATH. i suggest something like this:
  static const DWORD kMaxBuffer = 32 * 1024;  // Max according to MSDN.
  base::string16 path_expanded;
  DWORD path_len = MAX_PATH;
  do {
    DWORD result = ExpandEnvironmentStrings(
        path.c_str(), WriteInto(&path_expanded, path_len), path_len);
    if (!result) {
      // Failed to expand variables. Return the original string.
      DPLOG(ERROR) << path;
      break;
    }
    if (result <= path_len)
      return path_expanded.substr(0, result - 1);
    path_len = result;
  } while (path_len < kMaxBuffer);

  return path;

[pmonette_google.com, 2014/06/10 19:34:38]

Done.

WriteInto is handy!

+
+  return base::string16(path_expanded);
+}
+
+void AddDll(const std::string& path,
+            int base_address,
+            int length,
+            ClientIncidentReport_EnvironmentData_Process* process) {
+  ClientIncidentReport_EnvironmentData_Process_DLL* dll = process->add_dlls();
+
+  dll->set_path(path);
+  dll->set_base_address(base_address);
+  dll->set_length(length);
+}
+
+}  // namespace safe_browsing