Filter out credentials with non-matching schemes

PasswordFormManager::ProcessMatches currently happily accepts credentials from
PasswordStore with a different PasswordForm::Scheme than the observed form has.
However, it still has a DCHECK against it later (in the Autofill* methods), so
it is clearly not expecting these, rather than mixing the schemes being by
design.

And it should not be by design. Especially, if the saved credential is a
non-HTML one, and should be filled in a HTML form. Mixing them makes the
non-HTML credential vulnerable against (injected attacker's) JavaScript
accessing them.

This CL filters out credentials with non-matching scheme from the batch coming
from the PasswordStore. Given the absence of DCHECKs in release builds, this
actually changes the behaviour for Chrome users, but the change is a desired
one.

BUG=640897

Review-Url: https://codereview.chromium.org/2298733002
Cr-Commit-Position: refs/heads/master@{#415622}

components/password_manager/core/browser/password_form_manager.cc

@@ -481,16 +481,25 @@ void PasswordFormManager::ProcessMatches(
   // Create a copy of |non_federated| which we can reorder and prune.
   std::vector<const PasswordForm*> all_matches(non_federated);
 
-  // The next step is to reorder |all_matches| into three consequent blocks:
+  // The next step is to reorder |all_matches| into four consequent blocks:
   // (A) relevant blacklist entries
   // (B) non-relevant blacklist entries
-  // (C) non-blacklisted matches
+  // (C) relevant non-blacklisted matches
+  // (D) non-relevant non-blacklisted matches
 
   auto begin_nonblacklisted =  // start of block (C)
       std::partition(
           all_matches.begin(), all_matches.end(),
           [](const PasswordForm* form) { return form->blacklisted_by_user; });
 
+  // Block (D) can be removed immediately.
+  const PasswordForm::Scheme observed_scheme = observed_form_.scheme;
+  all_matches.erase(std::remove_if(begin_nonblacklisted, all_matches.end(),
+                                   [observed_scheme](const PasswordForm* form) {
+                                     return form->scheme != observed_scheme;
+                                   }),
+                    all_matches.end());
+
   auto begin_nonrelevant =  // start of block (B)
       std::partition(
           all_matches.begin(), begin_nonblacklisted,

components/password_manager/core/browser/password_form_manager_unittest.cc

@@ -3016,4 +3016,54 @@ TEST_F(PasswordFormManagerTest, ReportProcessingUpdate) {
   EXPECT_EQ(1, tester.GetActionCount("PasswordManager_LoginFollowingAutofill"));
 }
 
+// For all combinations of PasswordForm schemes, test that ProcessMatches
+// filters out forms with schemes not matching the observed form.
+TEST_F(PasswordFormManagerTest, RemoveResultsWithWrongScheme_ObservingHTML) {
+  for (int correct = 0; correct <= PasswordForm::SCHEME_LAST; ++correct) {
+    for (int wrong = 0; wrong <= PasswordForm::SCHEME_LAST; ++wrong) {
+      if (correct == wrong)
+        continue;
+
+      const PasswordForm::Scheme kCorrectScheme =
+          static_cast<PasswordForm::Scheme>(correct);
+      const PasswordForm::Scheme kWrongScheme =
+          static_cast<PasswordForm::Scheme>(wrong);
+      SCOPED_TRACE(testing::Message() << "Correct scheme = " << kCorrectScheme
+                                      << ", wrong scheme = " << kWrongScheme);
+
+      PasswordForm observed = *observed_form();
+      observed.scheme = kCorrectScheme;
+      PasswordFormManager form_manager(
+          password_manager(), client(),
+          (kCorrectScheme == PasswordForm::SCHEME_HTML ? client()->driver()
+                                                       : nullptr),
+          observed, base::MakeUnique<NiceMock<MockFormSaver>>());
+
+      PasswordForm match = *saved_match();
+      match.scheme = kCorrectScheme;
+
+      PasswordForm non_match = match;
+      non_match.scheme = kWrongScheme;
+
+      // First try putting the correct scheme first in returned matches.
+      std::vector<const PasswordForm*> all_matches = {&match, &non_match};
+      static_cast<FormFetcher::Consumer*>(&form_manager)
+          ->ProcessMatches(all_matches, 0u);
+
+      EXPECT_EQ(1u, form_manager.best_matches().size());
+      EXPECT_EQ(kCorrectScheme,
+                form_manager.best_matches().begin()->second->scheme);
+
+      // Now try putting the correct scheme last in returned matches.
+      all_matches = {&non_match, &match};
+      static_cast<FormFetcher::Consumer*>(&form_manager)
+          ->ProcessMatches(all_matches, 0u);
+
+      EXPECT_EQ(1u, form_manager.best_matches().size());
+      EXPECT_EQ(kCorrectScheme,
+                form_manager.best_matches().begin()->second->scheme);
+    }
+  }
+}
+
 }  // namespace password_manager