Add security feature to ProvisionalSavePassword

chrome/browser/password_manager/password_manager_browsertest.cc

 // Copyright 2013 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 <string>
 #include <utility>
 
 #include "base/command_line.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
@@ skipping 1394 matching lines @@
   std::unique_ptr<BubbleObserver> prompt_observer(
       new BubbleObserver(WebContents()));
   ui_test_utils::NavigateToURL(browser(), https_url);
 
   observer.SetPathToWaitFor("/password/done_and_separate_login_form.html");
   observer.Wait();
 
   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 =
       static_cast<password_manager::TestPasswordStore*>(
           PasswordStoreFactory::GetForProfile(
               browser()->profile(), ServiceAccessType::IMPLICIT_ACCESS).get());
 
   EXPECT_TRUE(password_store->IsEmpty());
 
   NavigateToFile("/password/form_with_only_password_field.html");
@@ skipping 1652 matching lines @@
   // about all frames, not just the main one. The factories should receive
   // messages for non-main frames, in particular
   // AutofillHostMsg_PasswordFormsParsed. If that were the first time the
   // factories hear about such frames, this would crash.
   tab_strip_model->AddWebContents(detached_web_contents.release(), -1,
                                   ::ui::PAGE_TRANSITION_AUTO_TOPLEVEL,
                                   TabStripModel::ADD_ACTIVE);
 }
 
 }  // namespace password_manager