Add security feature to ProvisionalSavePassword

chrome/browser/password_manager/password_manager_browsertest.cc

Index: chrome/browser/password_manager/password_manager_browsertest.cc
diff --git a/chrome/browser/password_manager/password_manager_browsertest.cc b/chrome/browser/password_manager/password_manager_browsertest.cc
index a17d1f5b00776ca754c6640f3e636d550018bcfb..ba373454490666fd7b7be0327ccf4f4ed4ba5b43 100644
--- a/chrome/browser/password_manager/password_manager_browsertest.cc
+++ b/chrome/browser/password_manager/password_manager_browsertest.cc
@@ -1412,6 +1412,91 @@ IN_PROC_BROWSER_TEST_F(
   EXPECT_FALSE(prompt_observer->IsShowingSavePrompt());
 }
 
+IN_PROC_BROWSER_TEST_F(
+    PasswordManagerBrowserTestBase,
+    NoPromptForSeperateLoginFormWhenSwitchingFromHttpsToHttp) {
+  base::CommandLine::ForCurrentProcess()->AppendSwitch(
+      ::switches::kAllowRunningInsecureContent);
+  base::CommandLine::ForCurrentProcess()->AppendSwitch(
+      ::switches::kIgnoreCertificateErrors);
+  const base::FilePath::CharType kDocRoot[] =
+      FILE_PATH_LITERAL("chrome/test/data");
+  net::EmbeddedTestServer https_test_server(
+      net::EmbeddedTestServer::TYPE_HTTPS);
+  https_test_server.ServeFilesFromSourceDirectory(base::FilePath(kDocRoot));
+  ASSERT_TRUE(https_test_server.Start());
+
+  // Setup the mock host resolver
+  host_resolver()->AddRule("*", "127.0.0.1");
+
+  std::string path = "/password/password_form.html";
+  GURL https_url(https_test_server.GetURL(path));
+  ASSERT_TRUE(https_url.SchemeIs(url::kHttpsScheme));
+
+  NavigationObserver form_observer(WebContents());
+  ui_test_utils::NavigateToURL(browser(), https_url);
+  form_observer.Wait();
+
+  std::string fill_and_submit_redirect =
+      "document.getElementById('username_redirect').value = 'user';"
+      "document.getElementById('password_redirect').value = 'password';"
+      "document.getElementById('submit_redirect').click()";
+  ASSERT_TRUE(
+      content::ExecuteScript(RenderViewHost(), fill_and_submit_redirect));
+
+  NavigationObserver redirect_observer(WebContents());
+  redirect_observer.SetPathToWaitFor("/password/redirect.html");
+  redirect_observer.Wait();
+
+  GURL http_url(embedded_test_server()->GetURL("/password/done.html"));
+  std::string http_redirect =
+      "window.location.href = '" + http_url.spec() + "';";
+  ASSERT_TRUE(content::ExecuteScript(RenderViewHost(), http_redirect));
+
+  NavigationObserver first_done_observer(WebContents());
+  first_done_observer.SetPathToWaitFor("/password/done.html");

[vasilii, 2017/01/05 11:26:11]

Looks like an unnecessary step. You can land on simple_password.html directly.

[jdoerrie, 2017/01/05 18:11:30]

You are right. My intention was to simulate an attack, where the user is
normally redirected to done.html before the attacker intervenes, who then also
redirects to done.html after submitting the malicious credential. This might be
overkill for this test though.

[vasilii, 2017/01/09 14:16:55]

The attacker don't need to redirect to simple_password.html. He can just inject
the content to done.html. Thus, it's indeed an overkill.

+  first_done_observer.Wait();
+
+  std::string attacker_redirect =
+      "window.location.href = 'simple_password.html'";
+  ASSERT_TRUE(content::ExecuteScript(RenderViewHost(), attacker_redirect));
+
+  NavigationObserver attacker_observer(WebContents());
+  attacker_observer.SetPathToWaitFor("/password/simple_password.html");
+  attacker_observer.Wait();
+
+  std::string fill_and_submit_attacker_form =
+      "document.getElementById('username_field').value = 'attacker_username';"
+      "document.getElementById('password_field').value = 'attacker_password';"
+      "document.getElementById('input_submit_button').click()";
+  ASSERT_TRUE(
+      content::ExecuteScript(RenderViewHost(), fill_and_submit_attacker_form));
+
+  NavigationObserver second_done_observer(WebContents());
+  second_done_observer.SetPathToWaitFor("/password/done.html");
+  second_done_observer.Wait();
+
+  BubbleObserver prompt_observer(WebContents());
+  EXPECT_TRUE(prompt_observer.IsShowingSavePrompt());
+  prompt_observer.AcceptSavePrompt();
+
+  // Check that credentials are stored.
+  scoped_refptr<password_manager::TestPasswordStore> password_store =
+      static_cast<password_manager::TestPasswordStore*>(
+          PasswordStoreFactory::GetForProfile(
+              browser()->profile(), ServiceAccessType::IMPLICIT_ACCESS)
+              .get());
+
+  // Spin the message loop to make sure the password store had a chance to save
+  // the password.
+  base::RunLoop run_loop;
+  run_loop.RunUntilIdle();

[vasilii, 2017/01/05 11:26:11]

It's dangerous in the browser test. Use WaitForPasswordStore().

[jdoerrie, 2017/01/05 18:11:30]

WaitForPasswordStore() is only implemented for CredentialManagerBrowserTest, not
PasswordManagerBrowserTestBase. Should I move it into that class instead? Other
tests in this file use |base::RunLoop::RunUntilIdle()|, are they all unsafe?

[vasilii, 2017/01/09 14:16:55]

They are all unsafe. The documentation says clearly that RunUntilIdle() may
never return. You can move WaitForPasswordStore() here.

+  EXPECT_FALSE(password_store->IsEmpty());
+
+  CheckThatCredentialsStored(password_store.get(), base::ASCIIToUTF16("user"),

[jdoerrie, 2017/01/04 16:55:18]

This test currently fails, because
|ShouldBlockPasswordForSameOriginButDifferentScheme| returns false due to
different ports. During an example run of the test requests were served from the
following origins: 

https://127.0.0.1:45407/
http://127.0.0.1:45779/

I would like to avoid the port mismatch here, but I'm not quite sure how I would
do this. Dropping the port would work, or setting them to their default values
of 80 and 443.

Unfortunately, I do not think that is possible. Is there any other possible
approach?

[vasilii, 2017/01/05 11:26:11]

Sounds like a bug in production. In the real world the ports will be different
(80 vs. 443). The attacker can redirect the user to any port.

[jdoerrie, 2017/01/05 18:11:30]

Acknowledged, dropped the check for port equality.

+                             base::ASCIIToUTF16("password"));
+}
+
 IN_PROC_BROWSER_TEST_F(PasswordManagerBrowserTestBase,
                        PromptWhenPasswordFormWithoutUsernameFieldSubmitted) {
   scoped_refptr<password_manager::TestPasswordStore> password_store =