Persist phished saved password bit.

In order to be able to keep track of which saved password and associated domains are phished, we to save a row to the compromised credentials table for the password and each domain. Bug: 1010764 Change-Id: Ie9232c22066be3766ca3d3ba30938224be9912b7 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1898175 Reviewed-by: Varun Khaneja <vakh@chromium.org> Commit-Queue: Bettina Dea <bdea@chromium.org> Cr-Commit-Position: refs/heads/master@{#716211}
Denger-Network · Nov 18, 2019 · 0013b4d · 0013b4d
1 parent aab1e96
commit 0013b4d
Show file tree

Hide file tree

Showing 7 changed files with 96 additions and 18 deletions.
diff --git a/chrome/browser/safe_browsing/chrome_password_protection_service.cc b/chrome/browser/safe_browsing/chrome_password_protection_service.cc
@@ -33,6 +33,7 @@
 #include "chrome/common/url_constants.h"
 #include "components/content_settings/core/browser/host_content_settings_map.h"
 #include "components/google/core/common/google_util.h"
+#include "components/password_manager/core/browser/compromised_credentials_table.h"
 #include "components/prefs/pref_change_registrar.h"
 #include "components/prefs/pref_service.h"
 #include "components/prefs/scoped_user_pref_update.h"
@@ -230,9 +231,7 @@ ChromePasswordProtectionService::ChromePasswordProtectionService(
 
 #if defined(SYNC_PASSWORD_REUSE_WARNING_ENABLED)
   scoped_refptr<password_manager::PasswordStore> password_store =
-      PasswordStoreFactory::GetForProfile(profile_,
-                                          ServiceAccessType::EXPLICIT_ACCESS)
-          .get();
+      GetProfilePasswordStore();
   // Password store can be null in tests.
   if (password_store) {
     // Subscribe to gaia hash password changes change notifications.
@@ -1131,17 +1130,14 @@ void ChromePasswordProtectionService::OnWarningTriggerChanged() {
 
   // Clears captured enterprise password hashes or GSuite sync password hashes.
   scoped_refptr<password_manager::PasswordStore> password_store =
-      PasswordStoreFactory::GetForProfile(profile_,
-                                          ServiceAccessType::EXPLICIT_ACCESS);
+      GetProfilePasswordStore();
 
   password_store->ClearAllNonGmailPasswordHash();
   password_store->ClearAllEnterprisePasswordHash();
 }
 
 void ChromePasswordProtectionService::OnEnterprisePasswordUrlChanged() {
-  PasswordStoreFactory::GetForProfile(profile_,
-                                      ServiceAccessType::EXPLICIT_ACCESS)
-      ->ScheduleEnterprisePasswordURLUpdate();
+  GetProfilePasswordStore()->ScheduleEnterprisePasswordURLUpdate();
 }
 
 bool ChromePasswordProtectionService::CanShowInterstitial(
@@ -1543,6 +1539,35 @@ bool ChromePasswordProtectionService::IsURLWhitelistedForPasswordEntry(
   return false;
 }
 
+void ChromePasswordProtectionService::PersistPhishedSavedPasswordCredential(
+    const std::string& username,
+    const std::vector<std::string>& matching_domains) {
+  if (!profile_)
+    return;
+  scoped_refptr<password_manager::PasswordStore> password_store =
+      GetProfilePasswordStore();
+
+  // Password store can be null in tests.
+  if (!password_store) {
+    return;
+  }
+  for (const std::string& domain : matching_domains) {
+    password_store->AddCompromisedCredentials(
+        password_manager::CompromisedCredentials(
+            GURL(domain), base::ASCIIToUTF16(username), base::Time::Now(),
+            password_manager::CompromiseType::kPhished));
+  }
+}
+
+password_manager::PasswordStore*
+ChromePasswordProtectionService::GetProfilePasswordStore() const {
+  // Always use EXPLICIT_ACCESS as the password manager checks IsIncognito
+  // itself when it shouldn't access the PasswordStore.
+  return PasswordStoreFactory::GetForProfile(profile_,
+                                             ServiceAccessType::EXPLICIT_ACCESS)
+      .get();
+}
+
 void ChromePasswordProtectionService::SanitizeReferrerChain(
     ReferrerChain* referrer_chain) {
   SafeBrowsingNavigationObserverManager::SanitizeReferrerChain(referrer_chain);

diff --git a/chrome/browser/safe_browsing/chrome_password_protection_service.h b/chrome/browser/safe_browsing/chrome_password_protection_service.h
@@ -215,6 +215,16 @@ class ChromePasswordProtectionService : public PasswordProtectionService {
   bool IsURLWhitelistedForPasswordEntry(const GURL& url,
                                         RequestOutcome* reason) const override;
 
+  // Persist the phished saved password credential in the "compromised
+  // credentials" table. Calls the password store to add a row for each domain
+  // where the phished saved password is used on.
+  void PersistPhishedSavedPasswordCredential(
+      const std::string& username,
+      const std::vector<std::string>& matching_domains) override;
+
+  // Returns the profile PasswordStore associated with this instance.
+  password_manager::PasswordStore* GetProfilePasswordStore() const;
+
   // Gets the type of sync account associated with current profile or
   // |NOT_SIGNED_IN|.
   LoginReputationClientRequest::PasswordReuseEvent::SyncAccountType
@@ -397,6 +407,8 @@ class ChromePasswordProtectionService : public PasswordProtectionService {
                            VerifyPasswordCaptureEventRecorded);
   FRIEND_TEST_ALL_PREFIXES(ChromePasswordProtectionServiceTest,
                            VerifyPasswordReuseDetectedSecurityEventRecorded);
+  FRIEND_TEST_ALL_PREFIXES(ChromePasswordProtectionServiceTest,
+                           VerifyPersistPhishedSavedPasswordCredential);
   // Browser tests
   FRIEND_TEST_ALL_PREFIXES(ChromePasswordProtectionServiceBrowserTest,
                            VerifyCheckGaiaPasswordChange);
@@ -509,6 +521,9 @@ class ChromePasswordProtectionService : public PasswordProtectionService {
   // Bypasses the check for probability when sending sample pings.
   bool bypass_probability_for_tests_ = false;
 
+  // Can be set for testing.
+  base::Clock* clock_;
+
   // Used to inject a different password hash, for testing. It's done as a
   // member callback rather than a virtual function because it's needed in the
   // constructor.

diff --git a/chrome/browser/safe_browsing/chrome_password_protection_service_unittest.cc b/chrome/browser/safe_browsing/chrome_password_protection_service_unittest.cc
@@ -25,10 +25,11 @@
 #include "chrome/test/base/chrome_render_view_host_test_harness.h"
 #include "chrome/test/base/testing_profile.h"
 #include "components/content_settings/core/browser/host_content_settings_map.h"
+#include "components/password_manager/core/browser/compromised_credentials_table.h"
 #include "components/password_manager/core/browser/hash_password_manager.h"
+#include "components/password_manager/core/browser/mock_password_store.h"
 #include "components/password_manager/core/browser/password_manager_metrics_util.h"
 #include "components/password_manager/core/browser/password_manager_test_utils.h"
-#include "components/password_manager/core/browser/test_password_store.h"
 #include "components/prefs/pref_service.h"
 #include "components/prefs/scoped_user_pref_update.h"
 #include "components/safe_browsing/common/utils.h"
@@ -44,7 +45,6 @@
 #include "components/sync_user_events/fake_user_event_service.h"
 #include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/web_contents.h"
-#include "content/public/test/browser_task_environment.h"
 #include "content/public/test/mock_navigation_handle.h"
 #include "extensions/browser/test_event_router.h"
 #include "net/http/http_util.h"
@@ -205,16 +205,20 @@ class ChromePasswordProtectionServiceTest
   void SetUp() override {
     ChromeRenderViewHostTestHarness::SetUp();
 
-    // Use TestPasswordStore to remove a possible race. Normally the
+    // Use MockPasswordStore to remove a possible race. Normally the
     // PasswordStore does its database manipulation on the DB thread, which
     // creates a possible race during navigation. Specifically the
     // PasswordManager will ignore any forms in a page if the load from the
     // PasswordStore has not completed.
-    PasswordStoreFactory::GetInstance()->SetTestingFactory(
-        profile(),
-        base::BindRepeating(
-            &password_manager::BuildPasswordStore<
-                content::BrowserContext, password_manager::TestPasswordStore>));
+    password_store_ =
+        base::WrapRefCounted(static_cast<password_manager::MockPasswordStore*>(
+            PasswordStoreFactory::GetInstance()
+                ->SetTestingFactoryAndUse(
+                    profile(),
+                    base::BindRepeating(&password_manager::BuildPasswordStore<
+                                        content::BrowserContext,
+                                        password_manager::MockPasswordStore>))
+                .get()));
 
     profile()->GetPrefs()->SetBoolean(prefs::kSafeBrowsingEnabled, true);
     profile()->GetPrefs()->SetInteger(
@@ -255,6 +259,7 @@ class ChromePasswordProtectionServiceTest
     base::RunLoop().RunUntilIdle();
     service_.reset();
     request_ = nullptr;
+    password_store_->ShutdownOnUIThread();
     identity_test_env_profile_adaptor_.reset();
     cache_manager_.reset();
     content_setting_map_->ShutdownOnUIThread();
@@ -366,6 +371,7 @@ class ChromePasswordProtectionServiceTest
   std::unique_ptr<IdentityTestEnvironmentProfileAdaptor>
       identity_test_env_profile_adaptor_;
   MockSecurityEventRecorder* security_event_recorder_;
+  scoped_refptr<password_manager::MockPasswordStore> password_store_;
   // Owned by KeyedServiceFactory.
   syncer::FakeUserEventService* fake_user_event_service_;
   extensions::TestEventRouter* test_event_router_;
@@ -543,6 +549,17 @@ TEST_F(ChromePasswordProtectionServiceTest,
   EXPECT_EQ(RequestOutcome::MATCHED_ENTERPRISE_LOGIN_URL, reason);
 }
 
+TEST_F(ChromePasswordProtectionServiceTest,
+       VerifyPersistPhishedSavedPasswordCredential) {
+  service_->ConfigService(/*is_incognito=*/false,
+                          /*is_extended_reporting=*/true);
+
+  std::vector<std::string> domains{"http://example.com",
+                                   "https://2.example.com"};
+  EXPECT_CALL(*password_store_, AddCompromisedCredentialsImpl(_)).Times(2);
+  service_->PersistPhishedSavedPasswordCredential("username", domains);
+}
+
 TEST_F(ChromePasswordProtectionServiceTest, VerifyCanSendSamplePing) {
   // If experiment is not enabled, do not send ping.
   service_->ConfigService(/*is_incognito=*/false,

diff --git a/components/safe_browsing/password_protection/mock_password_protection_service.h b/components/safe_browsing/password_protection/mock_password_protection_service.h
@@ -50,6 +50,8 @@ class MockPasswordProtectionService : public PasswordProtectionService {
   MOCK_METHOD1(SanitizeReferrerChain, void(ReferrerChain*));
   MOCK_METHOD2(ShowInterstitial,
                void(content::WebContents*, ReusedPasswordAccountType));
+  MOCK_METHOD2(PersistPhishedSavedPasswordCredential,
+               void(const std::string&, const std::vector<std::string>&));
   MOCK_METHOD3(IsPingingEnabled,
                bool(LoginReputationClientRequest::TriggerType,
                     ReusedPasswordAccountType,

diff --git a/components/safe_browsing/password_protection/password_protection_request.h b/components/safe_browsing/password_protection/password_protection_request.h
@@ -101,6 +101,10 @@ class PasswordProtectionRequest : public base::RefCountedThreadSafe<
 
   PasswordType password_type() const { return password_type_; }
 
+  const std::vector<std::string> matching_domains() const& {
+    return matching_domains_;
+  }
+
   bool is_modal_warning_showing() const { return is_modal_warning_showing_; }
 
   void set_is_modal_warning_showing(bool is_warning_showing) {

diff --git a/components/safe_browsing/password_protection/password_protection_service.cc b/components/safe_browsing/password_protection/password_protection_service.cc
@@ -313,20 +313,28 @@ void PasswordProtectionService::RequestFinished(
 
   request->HandleDeferredNavigations();
 
-#if defined(SYNC_PASSWORD_REUSE_WARNING_ENABLED)
   // If the request is canceled, the PasswordProtectionService is already
   // partially destroyed, and we won't be able to log accurate metrics.
   if (outcome != RequestOutcome::CANCELED) {
     auto verdict =
         response ? response->verdict_type()
                  : LoginReputationClientResponse::VERDICT_TYPE_UNSPECIFIED;
     auto is_phishing_url = verdict == LoginReputationClientResponse::PHISHING;
+
+#if defined(SYNC_PASSWORD_REUSE_WARNING_ENABLED)
     MaybeReportPasswordReuseDetected(request->web_contents(),
                                      request->username(),
                                      request->password_type(), is_phishing_url);
-  }
 #endif
 
+    // Persist a bit in CompromisedCredentials table when saved password is
+    // reused on a phishing site.
+    if (is_phishing_url) {
+      PersistPhishedSavedPasswordCredential(request->username(),
+                                            request->matching_domains());
+    }
+  }
+
   // Remove request from |pending_requests_| list. If it triggers warning, add
   // it into the !warning_reqeusts_| list.
   for (auto it = pending_requests_.begin(); it != pending_requests_.end();

diff --git a/components/safe_browsing/password_protection/password_protection_service.h b/components/safe_browsing/password_protection/password_protection_service.h
@@ -201,6 +201,13 @@ class PasswordProtectionService : public history::HistoryServiceObserver {
       const GURL& url,
       RequestOutcome* reason) const = 0;
 
+  // Persist the phished saved password credential in the "compromised
+  // credentials" table. Calls the password store to add a row for each domain
+  // where the phished saved password is used on.
+  virtual void PersistPhishedSavedPasswordCredential(
+      const std::string& username,
+      const std::vector<std::string>& matching_domains) = 0;
+
   // Converts from password::metrics_util::PasswordType to
   // LoginReputationClientRequest::PasswordReuseEvent::ReusedPasswordType.
   static ReusedPasswordType GetPasswordProtectionReusedPasswordType(