Add a ModuleLoadAnalyzer which checks modules against a whitelist

chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win_unittest.cc

Index: chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win_unittest.cc
diff --git a/chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win_unittest.cc b/chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..f3212d4f4f4ff6a4938a4d49d95d8a3c44c8c30c
--- /dev/null
+++ b/chrome/browser/safe_browsing/incident_reporting/module_load_analyzer_win_unittest.cc
@@ -0,0 +1,121 @@
+// Copyright 2014 The Chromium Authors. All rights reserved.

[grt (UTC plus 2), 2016/02/15 16:46:51]

2016

[proberge, 2016/02/16 16:56:23]

Done.

+// 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 <vector>

[grt (UTC plus 2), 2016/02/15 16:46:50]

unused

[proberge, 2016/02/16 16:56:23]

Done.

+
+#include "base/files/file_path.h"
+#include "base/memory/scoped_vector.h"
+#include "base/path_service.h"
+#include "base/run_loop.h"
+#include "base/scoped_native_library.h"
+#include "base/strings/string_util.h"
+#include "chrome/browser/safe_browsing/incident_reporting/incident.h"
+#include "chrome/browser/safe_browsing/incident_reporting/incident_receiver.h"
+#include "chrome/browser/safe_browsing/incident_reporting/mock_incident_receiver.h"
+#include "chrome/common/safe_browsing/csd.pb.h"
+#include "components/safe_browsing_db/database_manager.h"
+#include "components/safe_browsing_db/test_database_manager.h"
+#include "content/public/test/test_browser_thread_bundle.h"
+#include "content/public/test/test_utils.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using ::testing::IsNull;

[grt (UTC plus 2), 2016/02/15 16:46:50]

unused

[proberge, 2016/02/16 16:56:23]

Done.

+using ::testing::Return;
+using ::testing::StrictMock;
+using ::testing::_;
+
+namespace safe_browsing {
+
+namespace {

[grt (UTC plus 2), 2016/02/15 16:46:50]

nit: newline after this

[proberge, 2016/02/16 16:56:23]

Done.

+const char kWhitelistedModuleName[] = "USER32.dll";
+
+const char kNonWhitelistedModuleName[] = "blacklist_test_dll_1.dll";
+
+class MockSafeBrowsingDatabaseManager : public TestSafeBrowsingDatabaseManager {
+ public:
+  MockSafeBrowsingDatabaseManager() {}
+
+  MOCK_METHOD1(MatchModuleWhitelistString, bool(const std::string&));
+
+ private:
+  virtual ~MockSafeBrowsingDatabaseManager() {}

[grt (UTC plus 2), 2016/02/15 16:46:50]

is this needed? if so, it should be "~MockSafeBrowsingDatabaseManager() override
{}"

[proberge, 2016/02/16 16:56:23]

Done.

+  DISALLOW_COPY_AND_ASSIGN(MockSafeBrowsingDatabaseManager);
+};
+
+class ModuleLoadAnalayzerTest : public testing::Test {
+ protected:
+  ModuleLoadAnalayzerTest()
+      : mock_incident_receiver_(
+            new StrictMock<safe_browsing::MockIncidentReceiver>()),
+        mock_safe_browsing_database_manager_(
+            new MockSafeBrowsingDatabaseManager()) {
+    // Accept all dlls except kNonWhitelistedModuleName.
+    EXPECT_CALL(*mock_safe_browsing_database_manager_,

[grt (UTC plus 2), 2016/02/15 16:46:50]

if this is meant to be "return true by default", then you can say this
explicitly with:
    ON_CALL(...).WillByDefault(Return(true));
this is the better choice if you really don't need test expectations for these
calls.

[proberge, 2016/02/16 16:56:23]

ON_CALL(...).WillByDefault and EXPECT_CALL(...).WillOnce don't work well
together; the expect_call gets saturated by the wrong argument.

Using ON_CALL for both works, but it spams the logs so I'm keeping the two
EXPECT_CALL.

[grt (UTC plus 2), 2016/02/17 18:04:59]

Why? I have used the two in other tests with success.
Spams them with what?

[proberge, 2016/02/17 21:16:32]

I'm probably doing something wrong, but I'm seeing the following:

unknown file: error:
Unexpected mock function call - taking default action specified at:
[...]chrome\browser\safe_browsing\incident_reporting\module_load_analyzer_win_unittest.cc(57):
    Function call: MatchModuleWhitelistString(@0018F140 "user32.dll")
          Returns: true
Google Mock tried the following 1 expectation, but it didn't match:
[...]chrome\browser\safe_browsing\incident_reporting\module_load_analyzer_win_unittest.cc(115):
EXPECT_CALL
(*mock_safe_browsing_database_manager_,
MatchModuleWhitelistString(kNonWhitelistedModuleName))...
  Expected arg #0: is equal to "blacklist_test_dll_1.dll"
           Actual: "user32.dll"
         Expected: to be called once
           Actual: called once - saturated and active
The following message (once for every dll on the system):

GMOCK WARNING:
Uninteresting mock function call - taking default action specified at:
c:\src\chrome\src\chrome\browser\safe_browsing\incident_reporting\module_load_analyzer_win_unittest.cc(57):
    Function call: MatchModuleWhitelistString(@0018F280 "user32.dll")
          Returns: true
NOTE: You can safely ignore the above warning unless this call should not
happen.  Do not suppress it by blindly adding
an EXPECT_CALL() if you don't mean to enforce the call.  See
http://code.google.com/p/googlemock/wiki/CookBook#Knowing_W
hen_to_Expect for details.

[grt (UTC plus 2), 2016/02/18 16:06:55]

Riiiiight. I see (2nd paragraph of
https://github.com/google/googletest/blob/master/googlemock/docs/CookBook.md#...).
Apologies for the mis-direction.
Oh, yeah. NiceMock is good for suppressing those, but in this case I think you
need the expectations as you had them.

+                MatchModuleWhitelistString(_))
+        .WillRepeatedly(Return(true));
+    EXPECT_CALL(*mock_safe_browsing_database_manager_,

[grt (UTC plus 2), 2016/02/15 16:46:50]

this seems like a valid test expectation for TestNonWhitelistedDLLs. how about
moving it down there? if it should only be called once, you could make it a
WillOnce(Return(false)). in this case, you may be able to make it a StrictMock
as well.

[proberge, 2016/02/16 16:56:23]

Can't mock SafeBrowsingDatabaseManager directly, as it's an abstract class.

[grt (UTC plus 2), 2016/02/17 18:04:59]

I don't understand what you're saying here. What's wrong with the mock you have?
You can change it to a StrictMock with "new
StrictMock<MockSafeBrowsingDatabaseManager>()" on line 54, no?

[proberge, 2016/02/17 21:16:32]

Does it make sense to StrictMock a Mock class? That seemed like a weird thing to
do, so I assumed you were suggesting to use
StrictMock<SafeBrowsingDatabaseManager> instead of
MockSafeBrowsingDatabaseManager.

[grt (UTC plus 2), 2016/02/18 16:06:55]

Yup, they are wrappers around any mock:
https://github.com/google/googletest/blob/master/googlemock/docs/CookBook.md#...

+                MatchModuleWhitelistString(kNonWhitelistedModuleName))
+        .WillRepeatedly(Return(false));
+  }
+
+  void ExpectIncident(const std::string& module_to_load) {

[grt (UTC plus 2), 2016/02/15 16:46:50]

#include <string>

[proberge, 2016/02/16 16:56:23]

Done.

+    base::FilePath current_dir;
+    ASSERT_TRUE(PathService::Get(base::DIR_EXE, &current_dir));
+    base::ScopedNativeLibrary dll1(current_dir.AppendASCII(module_to_load));
+
+    scoped_ptr<Incident> incident;
+    EXPECT_CALL(*mock_incident_receiver_, DoAddIncidentForProcess(_))
+        .WillOnce(TakeIncident(&incident));
+
+    VerifyModuleLoadState(mock_safe_browsing_database_manager_,
+                          make_scoped_ptr(mock_incident_receiver_));
+
+    base::RunLoop().RunUntilIdle();
+    content::RunAllBlockingPoolTasksUntilIdle();
+
+    ASSERT_TRUE(incident);
+    scoped_ptr<ClientIncidentReport_IncidentData> incident_data =
+        incident->TakePayload();
+    ASSERT_TRUE(incident_data->has_suspicious_module());
+    const ClientIncidentReport_IncidentData_SuspiciousModuleIncident&
+        suspicious_module_incident = incident_data->suspicious_module();
+    EXPECT_TRUE(suspicious_module_incident.has_digest());
+    EXPECT_TRUE(base::EndsWith(suspicious_module_incident.path(),
+                               module_to_load, base::CompareCase::SENSITIVE));
+  }
+
+  void ExpectNoIncident(const std::string& module_to_load) {
+    base::FilePath current_dir;
+    ASSERT_TRUE(PathService::Get(base::DIR_EXE, &current_dir));
+    base::ScopedNativeLibrary dll1(current_dir.AppendASCII(module_to_load));
+
+    EXPECT_CALL(*mock_incident_receiver_, DoAddIncidentForProcess(_)).Times(0);
+
+    VerifyModuleLoadState(mock_safe_browsing_database_manager_,
+                          make_scoped_ptr(mock_incident_receiver_));
+
+    base::RunLoop().RunUntilIdle();
+    content::RunAllBlockingPoolTasksUntilIdle();
+  }
+
+  content::TestBrowserThreadBundle browser_thread_bundle_;
+  StrictMock<safe_browsing::MockIncidentReceiver>* mock_incident_receiver_;
+  scoped_refptr<MockSafeBrowsingDatabaseManager>
+      mock_safe_browsing_database_manager_;
+};
+
+}  // namespace
+
+TEST_F(ModuleLoadAnalayzerTest, TestWhitelistedDLLs) {
+  ExpectNoIncident(kWhitelistedModuleName);
+}
+
+TEST_F(ModuleLoadAnalayzerTest, TestNonWhitelistedDLLs) {
+  ExpectIncident(kNonWhitelistedModuleName);
+}
+
+}  // namespace safe_browsing