Revert 278655 "Include loaded modules in safe browsing client in..."

> Include loaded modules in safe browsing client incident reports.
> 
> This includes the sanitized paths of all loaded modules and an
> indication for those that host active LSPs.
> 
> BUG=386156
> 
> Review URL: https://codereview.chromium.org/323953002

TBR=pmonette@google.com,grt@chromium.org

Review URL: https://codereview.chromium.org/347123002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@278721 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/safe_browsing/environment_data_collection_win.cc

@@ -4,94 +4,13 @@
 
 #include "chrome/browser/safe_browsing/environment_data_collection_win.h"
 
-#include <windows.h>
-#include <set>
-
-#include "base/strings/string16.h"
-#include "base/strings/string_util.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/path_sanitizer.h"
 #include "chrome/common/safe_browsing/csd.pb.h"
 
 namespace safe_browsing {
 
-namespace {
-
-// Helper function for expanding all environment variables in |path|.
-base::string16 ExpandEnvironmentVariables(const base::string16& path) {
-  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;
-}
-
-}  // namespace
-
-bool CollectDlls(ClientIncidentReport_EnvironmentData_Process* process) {
-  // Retrieve the module list.
-  std::set<ModuleInfo> loaded_modules;
-  if (!GetLoadedModules(&loaded_modules))
-    return false;
-
-  // Sanitize path of each module and add it to the incident report.
-  PathSanitizer path_sanitizer;
-  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);
-
-    ClientIncidentReport_EnvironmentData_Process_Dll* dll = process->add_dll();
-    dll->set_path(base::WideToUTF8(dll_path.value()));
-    dll->set_base_address(it->base_address);
-    dll->set_length(it->size);
-  }
-
-  return true;
-}
-
-void RecordLspFeature(ClientIncidentReport_EnvironmentData_Process* process) {
-  WinsockLayeredServiceProviderList lsp_list;
-  GetWinsockLayeredServiceProviders(&lsp_list);
-
-  // For each LSP, we extract and sanitize the path.
-  PathSanitizer path_sanitizer;
-  std::set<base::string16> lsp_paths;
-  for (size_t i = 0; i < lsp_list.size(); ++i) {
-    base::FilePath lsp_path(ExpandEnvironmentVariables(lsp_list[i].path));
-    path_sanitizer.StripHomeDirectory(&lsp_path);
-    lsp_paths.insert(lsp_path.value());
-  }
-
-  // Look for a match between LSPs and loaded dlls.
-  for (int i = 0; i < process->dll_size(); ++i) {
-    if (lsp_paths.count(base::UTF8ToWide(process->dll(i).path()))) {
-      process->mutable_dll(i)
-          ->add_feature(ClientIncidentReport_EnvironmentData_Process_Dll::LSP);
-    }
-  }
-}
-
 void CollectPlatformProcessData(
     ClientIncidentReport_EnvironmentData_Process* process) {
-  CollectDlls(process);
-  RecordLspFeature(process);
+  // TODO(pmonette): collect dlls and lsps.
 }
 
 }  // namespace safe_browsing

chrome/browser/safe_browsing/environment_data_collection_win.h

@@ -7,16 +7,7 @@
 
 namespace safe_browsing {
 
-class ClientIncidentReport_EnvironmentData_Process;
-
-// Collects then populates |process| with the sanitized paths of all DLLs
-// loaded in the current process. Return false if an error occurred while
-// querying for the loaded dlls.
-bool CollectDlls(ClientIncidentReport_EnvironmentData_Process* process);
-
-// For each of the dlls in this already populated incident report,
-// check one of them is a registered LSP.
-void RecordLspFeature(ClientIncidentReport_EnvironmentData_Process* process);
+// TODO(pmonette): put DLL and LSP collection declarations here.
 
 }  // namespace safe_browsing
 

chrome/browser/safe_browsing/environment_data_collection_win_unittest.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/environment_data_collection_win.h"
-
-#include <string>
-
-#include "base/files/file_path.h"
-#include "base/scoped_native_library.h"
-#include "chrome/common/safe_browsing/csd.pb.h"
-#include "testing/gtest/include/gtest/gtest.h"
-
-namespace {
-
-// Helper function that returns true if a dll with filename |dll_name| is
-// found in |process_report|.
-bool ProcessReportContainsDll(
-    const safe_browsing::ClientIncidentReport_EnvironmentData_Process&
-        process_report,
-    const base::FilePath& dll_name) {
-  for (int i = 0; i < process_report.dll_size(); ++i) {
-    base::FilePath current_dll =
-        base::FilePath::FromUTF8Unsafe(process_report.dll(i).path());
-
-    if (current_dll.BaseName() == dll_name)
-      return true;
-  }
-
-  return false;
-}
-
-}  // namespace
-
-TEST(SafeBrowsingEnvironmentDataCollectionWinTest, CollectDlls) {
-  // This test will check if the CollectDlls method works by loading
-  // a dll and then checking if we can find it within the process report.
-  // Pick msvidc32.dll as it is present from WinXP to Win8 and yet rarely used.
-  // msvidc32.dll exists in both 32 and 64 bit versions.
-  base::FilePath msvdc32_dll(L"msvidc32.dll");
-
-  safe_browsing::ClientIncidentReport_EnvironmentData_Process process_report;
-  safe_browsing::CollectDlls(&process_report);
-
-  ASSERT_FALSE(ProcessReportContainsDll(process_report, msvdc32_dll));
-
-  // Redo the same verification after loading a new dll.
-  base::ScopedNativeLibrary library(msvdc32_dll);
-
-  process_report.clear_dll();
-  safe_browsing::CollectDlls(&process_report);
-
-  ASSERT_TRUE(ProcessReportContainsDll(process_report, msvdc32_dll));
-}
-
-TEST(SafeBrowsingEnvironmentDataCollectionWinTest, RecordLspFeature) {
-  // Populate our incident report with loaded modules.
-  safe_browsing::ClientIncidentReport_EnvironmentData_Process process_report;
-  safe_browsing::CollectDlls(&process_report);
-
-  // We'll test RecordLspFeatures against a real dll registered as a LSP.
-  std::string lsp_path = "C:\\Windows\\system32\\mswsock.dll";
-  int base_address = 0x77770000;
-  int length = 0x180000;
-
-  // Look for dlls that are registered as a LSP.
-  safe_browsing::RecordLspFeature(&process_report);
-
-  // Look through dll entries and check that none contains the LSP feature.
-  bool lsp_feature_found = false;
-  for (int i = 0; i < process_report.dll_size(); ++i) {
-    if (process_report.dll(i).path() == lsp_path) {
-      // Look for ClientIncidentReport_EnvironmentData_Process_DLL_Feature_LSP
-      // through the features of each dll.
-      for (int j = 0; j < process_report.dll(i).feature_size(); ++j) {
-        if (process_report.dll(i).feature(j) ==
-            safe_browsing::ClientIncidentReport_EnvironmentData_Process_Dll::
-                LSP)
-          lsp_feature_found = true;
-      }
-    }
-  }
-
-  ASSERT_FALSE(lsp_feature_found);
-
-  // Manually add an entry to the process report that will get marked as a LSP.
-  safe_browsing::ClientIncidentReport_EnvironmentData_Process_Dll* dll =
-      process_report.add_dll();
-  dll->set_path(lsp_path);
-  dll->set_base_address(base_address);
-  dll->set_length(length);
-
-  // Look for dlls that are registered as a LSP.
-  safe_browsing::RecordLspFeature(&process_report);
-
-  // Look through dll entries and check if the one we added contains the LSP
-  // feature.
-  lsp_feature_found = false;
-  for (int i = 0; i < process_report.dll_size(); ++i) {
-    if (process_report.dll(i).path() == lsp_path) {
-      // Look for ClientIncidentReport_EnvironmentData_Process_DLL_Feature_LSP
-      // through the features of each dll.
-      for (int j = 0; j < process_report.dll(i).feature_size(); ++j) {
-        if (process_report.dll(i).feature(j) ==
-            safe_browsing::ClientIncidentReport_EnvironmentData_Process_Dll::
-                LSP)
-          lsp_feature_found = true;
-      }
-    }
-  }
-
-  ASSERT_TRUE(lsp_feature_found);
-}

chrome/browser/safe_browsing/path_sanitizer.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/path_sanitizer.h"
-
-#include "base/logging.h"
-#include "base/path_service.h"
-
-namespace safe_browsing {
-
-PathSanitizer::PathSanitizer() {
-  // Get the home directory path.
-  if (!PathService::Get(base::DIR_HOME, &home_path_))
-    NOTREACHED();
-}
-
-const base::FilePath& PathSanitizer::GetHomeDirectory() const {
-  return home_path_;
-}
-
-void PathSanitizer::StripHomeDirectory(base::FilePath* file_path) const {
-  base::FilePath sanitized_path(FILE_PATH_LITERAL("~"));
-
-  // The |file_path| is overwritten only if a relative path is found.
-  if (home_path_.AppendRelativePath(*file_path, &sanitized_path))
-    *file_path = sanitized_path;
-}
-
-}  // namespace safe_browsing

chrome/browser/safe_browsing/path_sanitizer.h

-// 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.
-
-#ifndef CHROME_BROWSER_SAFE_BROWSING_PATH_SANITIZER_H_
-#define CHROME_BROWSER_SAFE_BROWSING_PATH_SANITIZER_H_
-
-#include "base/files/file_path.h"
-#include "base/macros.h"
-
-namespace safe_browsing {
-
-// A helper class to sanitize any number of file paths by replacing the portion
-// that represents the current user's home directory with "~".
-class PathSanitizer {
- public:
-  PathSanitizer();
-
-  const base::FilePath& GetHomeDirectory() const;
-
-  void StripHomeDirectory(base::FilePath* file_path) const;
-
- private:
-  base::FilePath home_path_;
-
-  DISALLOW_COPY_AND_ASSIGN(PathSanitizer);
-};
-
-}  // namespace safe_browsing
-
-#endif  // CHROME_BROWSER_SAFE_BROWSING_PATH_SANITIZER_H_

chrome/browser/safe_browsing/path_sanitizer_unittest.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/path_sanitizer.h"
-
-#include <vector>
-
-#include "base/logging.h"
-#include "base/path_service.h"
-#include "testing/gtest/include/gtest/gtest.h"
-
-namespace {
-
-// Returns the root directory with a trailing separator. Works on all platforms.
-base::FilePath GetRootDirectory() {
-  base::FilePath dir_temp;
-  if (!PathService::Get(base::DIR_TEMP, &dir_temp))
-    NOTREACHED();
-
-  std::vector<base::FilePath::StringType> components;
-  dir_temp.GetComponents(&components);
-
-  return base::FilePath(components[0]).AsEndingWithSeparator();
-}
-
-}  // namespace
-
-TEST(SafeBrowsingPathSanitizerTest, HomeDirectoryIsNotEmpty) {
-  safe_browsing::PathSanitizer path_sanitizer;
-
-  ASSERT_FALSE(path_sanitizer.GetHomeDirectory().empty());
-}
-
-TEST(SafeBrowsingPathSanitizerTest, DontStripHomeDirectoryTest) {
-  // Test with path not in home directory.
-  base::FilePath path =
-      GetRootDirectory().Append(FILE_PATH_LITERAL("not_in_home_directory.ext"));
-  base::FilePath path_expected = path;
-
-  safe_browsing::PathSanitizer path_sanitizer;
-  path_sanitizer.StripHomeDirectory(&path);
-
-  ASSERT_EQ(path.value(), path_expected.value());
-}
-
-TEST(SafeBrowsingPathSanitizerTest, DoStripHomeDirectoryTest) {
-  // Test with path in home directory.
-  safe_browsing::PathSanitizer path_sanitizer;
-
-  base::FilePath path = path_sanitizer.GetHomeDirectory().Append(
-      FILE_PATH_LITERAL("in_home_directory.ext"));
-  base::FilePath path_expected = base::FilePath(FILE_PATH_LITERAL("~")).Append(
-      FILE_PATH_LITERAL("in_home_directory.ext"));
-
-  path_sanitizer.StripHomeDirectory(&path);
-
-  ASSERT_EQ(path.value(), path_expected.value());
-}

chrome/chrome_browser.gypi

@@ -1876,8 +1876,6 @@
         'browser/safe_browsing/malware_details_cache.h',
         'browser/safe_browsing/malware_details_history.cc',
         'browser/safe_browsing/malware_details_history.h',
-        'browser/safe_browsing/path_sanitizer.cc',
-        'browser/safe_browsing/path_sanitizer.h',
         'browser/safe_browsing/pe_image_reader_win.cc',
         'browser/safe_browsing/pe_image_reader_win.h',
         'browser/safe_browsing/ping_manager.cc',

chrome/chrome_tests_unit.gypi

@@ -1249,12 +1249,10 @@
         'browser/safe_browsing/download_feedback_service_unittest.cc',
         'browser/safe_browsing/download_feedback_unittest.cc',
         'browser/safe_browsing/download_protection_service_unittest.cc',
-        'browser/safe_browsing/environment_data_collection_win_unittest.cc',
         'browser/safe_browsing/incident_report_uploader_impl_unittest.cc',
         'browser/safe_browsing/incident_reporting_service_unittest.cc',
         'browser/safe_browsing/local_two_phase_testserver.cc',
         'browser/safe_browsing/malware_details_unittest.cc',
-        'browser/safe_browsing/path_sanitizer_unittest.cc',
         'browser/safe_browsing/pe_image_reader_win_unittest.cc',
         'browser/safe_browsing/ping_manager_unittest.cc',
         'browser/safe_browsing/preference_validation_delegate_unittest.cc',

chrome/common/safe_browsing/csd.proto

@@ -110,7 +110,7 @@ message ClientMalwareRequest {
     required string url = 2;
     optional string method = 3;
     optional string referrer = 4;
-    // Resource type, the int value is a direct cast from the Type enum
+    // Resource type, the int value is a direct cast from the Type enum 
     // of ResourceType class defined in //src/webkit/commom/resource_type.h
     optional int32 resource_type = 5;
   }
@@ -380,7 +380,7 @@ message ClientIncidentReport {
     optional Machine machine = 2;
     message Process {
       optional string version = 1;
-      repeated string OBSOLETE_dlls = 2;
+      repeated string dlls = 2;
       message Patch {
         optional string function = 1;
         optional string target_dll = 2;
@@ -399,17 +399,6 @@ message ClientIncidentReport {
       optional int64 uptime_msec = 6;
       optional bool metrics_consent = 7;
       optional bool extended_consent = 8;
-      message Dll {
-        enum Feature {
-          UNKNOWN = 0;
-          LSP = 1;
-        }
-        optional string path = 1;
-        optional uint64 base_address = 2;
-        optional uint32 length = 3;
-        repeated Feature feature = 4;
-      }
-      repeated Dll dll = 9;
     }
     optional Process process = 3;
   }