Strengthen renderer autherization in ContentPasswordManagerDriver.

- The code is refactored. CheckChildProcessSecurityPolicy is to be used for
autofill::mojom::PasswordManagerClient too.
- PasswordForm::signon_realm is now checked together with origin.

Bug: 467150
Change-Id: I8e696c4c904c72621a3410a8ec8f4d00cb4d6236
Reviewed-on: https://chromium-review.googlesource.com/1109966
Commit-Queue: Vasilii Sukhanov <vasilii@chromium.org>
Reviewed-by: Dominic Battré <battre@chromium.org>
Cr-Commit-Position: refs/heads/master@{#569331}

components/password_manager/content/browser/bad_message.cc

@@ -6,11 +6,19 @@
 
 #include "base/logging.h"
 #include "base/metrics/histogram_functions.h"
+#include "base/syslog_logging.h"
+#include "components/autofill/core/common/password_form.h"
+#include "content/public/browser/child_process_security_policy.h"
+#include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/render_process_host.h"
 
 namespace password_manager {
 namespace bad_message {
+namespace {
 
+// Called when the browser receives a bad IPC message from a renderer process on
+// the UI thread. Logs the event, records a histogram metric for the |reason|,
+// and terminates the process for |host|.
 void ReceivedBadMessage(content::RenderProcessHost* host,
                         BadMessageReason reason) {
   LOG(ERROR)
@@ -22,5 +30,53 @@ void ReceivedBadMessage(content::RenderProcessHost* host,
       content::RenderProcessHost::CrashReportMode::GENERATE_CRASH_DUMP);
 }
 
+bool CheckChildProcessSecurityPolicyForURL(content::RenderFrameHost* frame,
+                                           const GURL& url,
+                                           BadMessageReason reason) {
+  // Renderer-side logic should prevent any password manager usage for
+  // about:blank frames as well as data URLs.  If that's not the case, kill the
+  // renderer, as it might be exploited.
+  if (url.SchemeIs(url::kAboutScheme) || url.SchemeIs(url::kDataScheme)) {
+    SYSLOG(WARNING) << "Killing renderer: illegal password access from about: "
+                    << "or data: URL. Reason: " << static_cast<int>(reason);
+    bad_message::ReceivedBadMessage(frame->GetProcess(), reason);
+    return false;
+  }
+
+  content::ChildProcessSecurityPolicy* policy =
+      content::ChildProcessSecurityPolicy::GetInstance();
+  if (!policy->CanAccessDataForOrigin(frame->GetProcess()->GetID(), url)) {
+    SYSLOG(WARNING) << "Killing renderer: illegal password access. Reason: "
+                    << static_cast<int>(reason);
+    bad_message::ReceivedBadMessage(frame->GetProcess(), reason);
+    return false;
+  }
+
+  return true;
+}
+
+}  // namespace
+
+bool CheckChildProcessSecurityPolicy(
+    content::RenderFrameHost* frame,
+    const autofill::PasswordForm& password_form,
+    BadMessageReason reason) {
+  return CheckChildProcessSecurityPolicyForURL(frame, password_form.origin,
+                                               reason) &&
+         CheckChildProcessSecurityPolicyForURL(
+             frame, GURL(password_form.signon_realm), reason);
+}
+
+bool CheckChildProcessSecurityPolicy(
+    content::RenderFrameHost* frame,
+    const std::vector<autofill::PasswordForm>& forms,
+    BadMessageReason reason) {
+  for (const auto& form : forms) {
+    if (!bad_message::CheckChildProcessSecurityPolicy(frame, form, reason))
+      return false;
+  }
+  return true;
+}
+
 }  // namespace bad_message
 }  // namespace password_manager

components/password_manager/content/browser/bad_message.h

@@ -5,8 +5,14 @@
 #ifndef COMPONENTS_PASSWORD_MANAGER_CONTENT_BROWSER_BAD_MESSAGE_H_
 #define COMPONENTS_PASSWORD_MANAGER_CONTENT_BROWSER_BAD_MESSAGE_H_
 
+#include <vector>
+
+namespace autofill {
+struct PasswordForm;
+}
+
 namespace content {
-class RenderProcessHost;
+class RenderFrameHost;
 }
 
 namespace password_manager {
@@ -38,10 +44,18 @@ enum class BadMessageReason {
 };
 
 namespace bad_message {
-// Called when the browser receives a bad IPC message from a renderer process on
-// the UI thread. Logs the event, records a histogram metric for the |reason|,
-// and terminates the process for |host|.
-void ReceivedBadMessage(content::RenderProcessHost* host,
+// Returns true if the renderer for |frame| is allowed to perform an operation
+// on |password_form|. If the origin mismatches, the process for |frame| is
+// terminated and the function returns false.
+bool CheckChildProcessSecurityPolicy(
+    content::RenderFrameHost* frame,
+    const autofill::PasswordForm& password_form,
+    BadMessageReason reason);
+
+// Same as above but checks every form in |forms|.
+bool CheckChildProcessSecurityPolicy(
+    content::RenderFrameHost* frame,
+    const std::vector<autofill::PasswordForm>& forms,
     BadMessageReason reason);
 
 }  // namespace bad_message

components/password_manager/content/browser/content_password_manager_driver.cc

@@ -6,7 +6,6 @@
 
 #include <utility>
 
-#include "base/syslog_logging.h"
 #include "components/autofill/content/browser/content_autofill_driver.h"
 #include "components/autofill/core/common/form_data.h"
 #include "components/autofill/core/common/password_form.h"
@@ -17,14 +16,9 @@
 #include "components/password_manager/core/browser/password_manager_client.h"
 #include "components/password_manager/core/browser/password_manager_metrics_recorder.h"
 #include "content/public/browser/browser_context.h"
-#include "content/public/browser/child_process_security_policy.h"
 #include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/navigation_handle.h"
-#include "content/public/browser/render_frame_host.h"
-#include "content/public/browser/render_process_host.h"
-#include "content/public/browser/render_view_host.h"
 #include "content/public/browser/render_widget_host_view.h"
-#include "content/public/browser/site_instance.h"
 #include "content/public/browser/ssl_status.h"
 #include "content/public/browser/web_contents.h"
 #include "net/cert/cert_status_flags.h"
@@ -142,21 +136,22 @@ void ContentPasswordManagerDriver::ForceSavePassword() {
                  weak_factory_.GetWeakPtr()));
 }
 
-void ContentPasswordManagerDriver::ShowManualFallbackForSaving(
-    const autofill::PasswordForm& password_form) {
-  if (!CheckChildProcessSecurityPolicy(
-          password_form.origin,
-          BadMessageReason::CPMD_BAD_ORIGIN_SHOW_FALLBACK_FOR_SAVING))
-    return;
-  GetPasswordManager()->ShowManualFallbackForSaving(this, password_form);
+void ContentPasswordManagerDriver::GeneratePassword() {
+  GetPasswordGenerationAgent()->UserTriggeredGeneratePassword();
 }
 
-void ContentPasswordManagerDriver::HideManualFallbackForSaving() {
-  GetPasswordManager()->HideManualFallbackForSaving();
+PasswordGenerationManager*
+ContentPasswordManagerDriver::GetPasswordGenerationManager() {
+  return &password_generation_manager_;
 }
 
-void ContentPasswordManagerDriver::GeneratePassword() {
-  GetPasswordGenerationAgent()->UserTriggeredGeneratePassword();
+PasswordManager* ContentPasswordManagerDriver::GetPasswordManager() {
+  return client_->GetPasswordManager();
+}
+
+PasswordAutofillManager*
+ContentPasswordManagerDriver::GetPasswordAutofillManager() {
+  return &password_autofill_manager_;
 }
 
 void ContentPasswordManagerDriver::SendLoggingAvailability() {
@@ -181,32 +176,24 @@ void ContentPasswordManagerDriver::MatchingBlacklistedFormFound() {
   GetPasswordAutofillAgent()->BlacklistedFormFound();
 }
 
-PasswordGenerationManager*
-ContentPasswordManagerDriver::GetPasswordGenerationManager() {
-  return &password_generation_manager_;
-}
-
-PasswordManager* ContentPasswordManagerDriver::GetPasswordManager() {
-  return client_->GetPasswordManager();
+void ContentPasswordManagerDriver::DidNavigateFrame(
+    content::NavigationHandle* navigation_handle) {
+  // Clear page specific data after main frame navigation.
+  if (navigation_handle->IsInMainFrame() &&
+      !navigation_handle->IsSameDocument()) {
+    GetPasswordManager()->DidNavigateMainFrame();
+    GetPasswordAutofillManager()->DidNavigateMainFrame();
+  }
 }
 
-PasswordAutofillManager*
-ContentPasswordManagerDriver::GetPasswordAutofillManager() {
-  return &password_autofill_manager_;
-}
 
 void ContentPasswordManagerDriver::PasswordFormsParsed(
     const std::vector<autofill::PasswordForm>& forms) {
-  for (const auto& form : forms)
-    if (!CheckChildProcessSecurityPolicy(
-            form.origin, BadMessageReason::CPMD_BAD_ORIGIN_FORMS_PARSED))
+  if (!bad_message::CheckChildProcessSecurityPolicy(
+          render_frame_host_, forms,
+          BadMessageReason::CPMD_BAD_ORIGIN_FORMS_PARSED))
     return;
 
-  OnPasswordFormsParsedNoRenderCheck(forms);
-}
-
-void ContentPasswordManagerDriver::OnPasswordFormsParsedNoRenderCheck(
-    const std::vector<autofill::PasswordForm>& forms) {
   GetPasswordManager()->OnPasswordFormsParsed(this, forms);
   GetPasswordGenerationManager()->CheckIfFormClassifierShouldRun();
 }
@@ -214,71 +201,42 @@ void ContentPasswordManagerDriver::OnPasswordFormsParsedNoRenderCheck(
 void ContentPasswordManagerDriver::PasswordFormsRendered(
     const std::vector<autofill::PasswordForm>& visible_forms,
     bool did_stop_loading) {
-  for (const auto& form : visible_forms)
-    if (!CheckChildProcessSecurityPolicy(
-            form.origin, BadMessageReason::CPMD_BAD_ORIGIN_FORMS_RENDERED))
+  if (!bad_message::CheckChildProcessSecurityPolicy(
+          render_frame_host_, visible_forms,
+          BadMessageReason::CPMD_BAD_ORIGIN_FORMS_RENDERED))
     return;
+
   GetPasswordManager()->OnPasswordFormsRendered(this, visible_forms,
                                                 did_stop_loading);
 }
 
 void ContentPasswordManagerDriver::PasswordFormSubmitted(
     const autofill::PasswordForm& password_form) {
-  if (!CheckChildProcessSecurityPolicy(
-          password_form.origin,
+  if (!bad_message::CheckChildProcessSecurityPolicy(
+          render_frame_host_, password_form,
           BadMessageReason::CPMD_BAD_ORIGIN_FORM_SUBMITTED))
     return;
   GetPasswordManager()->OnPasswordFormSubmitted(this, password_form);
 }
 
-void ContentPasswordManagerDriver::OnFocusedPasswordFormFound(
+void ContentPasswordManagerDriver::ShowManualFallbackForSaving(
     const autofill::PasswordForm& password_form) {
-  if (!CheckChildProcessSecurityPolicy(
-          password_form.origin,
-          BadMessageReason::CPMD_BAD_ORIGIN_FOCUSED_PASSWORD_FORM_FOUND))
+  if (!bad_message::CheckChildProcessSecurityPolicy(
+          render_frame_host_, password_form,
+          BadMessageReason::CPMD_BAD_ORIGIN_SHOW_FALLBACK_FOR_SAVING))
     return;
-  GetPasswordManager()->OnPasswordFormForceSaveRequested(this, password_form);
-}
-
-void ContentPasswordManagerDriver::DidNavigateFrame(
-    content::NavigationHandle* navigation_handle) {
-  // Clear page specific data after main frame navigation.
-  if (navigation_handle->IsInMainFrame() &&
-      !navigation_handle->IsSameDocument()) {
-    GetPasswordManager()->DidNavigateMainFrame();
-    GetPasswordAutofillManager()->DidNavigateMainFrame();
-  }
+  GetPasswordManager()->ShowManualFallbackForSaving(this, password_form);
 }
 
 void ContentPasswordManagerDriver::SameDocumentNavigation(
     const autofill::PasswordForm& password_form) {
-  if (!CheckChildProcessSecurityPolicy(
-          password_form.origin,
+  if (!bad_message::CheckChildProcessSecurityPolicy(
+          render_frame_host_, password_form,
           BadMessageReason::CPMD_BAD_ORIGIN_IN_PAGE_NAVIGATION))
     return;
   GetPasswordManager()->OnSameDocumentNavigation(this, password_form);
 }
 
-void ContentPasswordManagerDriver::SaveGenerationFieldDetectedByClassifier(
-    const autofill::PasswordForm& password_form,
-    const base::string16& generation_field) {
-  if (!CheckChildProcessSecurityPolicy(
-          password_form.origin,
-          BadMessageReason::
-              CPMD_BAD_ORIGIN_SAVE_GENERATION_FIELD_DETECTED_BY_CLASSIFIER))
-    return;
-  GetPasswordManager()->SaveGenerationFieldDetectedByClassifier(
-      password_form, generation_field);
-}
-
-void ContentPasswordManagerDriver::CheckSafeBrowsingReputation(
-    const GURL& form_action,
-    const GURL& frame_url) {
-#if defined(SAFE_BROWSING_DB_LOCAL)
-  client_->CheckSafeBrowsingReputation(form_action, frame_url);
-#endif
-}
-
 void ContentPasswordManagerDriver::ShowPasswordSuggestions(
     int key,
     base::i18n::TextDirection text_direction,
@@ -297,6 +255,10 @@ void ContentPasswordManagerDriver::ShowManualFallbackSuggestion(
       text_direction, TransformToRootCoordinates(bounds));
 }
 
+void ContentPasswordManagerDriver::HideManualFallbackForSaving() {
+  GetPasswordManager()->HideManualFallbackForSaving();
+}
+
 void ContentPasswordManagerDriver::RecordSavePasswordProgress(
     const std::string& log) {
   client_->GetLogManager()->LogSavePasswordProgress(log);
@@ -306,30 +268,33 @@ void ContentPasswordManagerDriver::UserModifiedPasswordField() {
   client_->GetMetricsRecorder().RecordUserModifiedPasswordField();
 }
 
-bool ContentPasswordManagerDriver::CheckChildProcessSecurityPolicy(
-    const GURL& url,
-    BadMessageReason reason) {
-  // Renderer-side logic should prevent any password manager usage for
-  // about:blank frames as well as data URLs.  If that's not the case, kill the
-  // renderer, as it might be exploited.
-  if (url.SchemeIs(url::kAboutScheme) || url.SchemeIs(url::kDataScheme)) {
-    SYSLOG(WARNING) << "Killing renderer: illegal password access from about: "
-                    << " or data: URL. Reason: " << static_cast<int>(reason);
-    bad_message::ReceivedBadMessage(render_frame_host_->GetProcess(), reason);
-    return false;
-  }
+void ContentPasswordManagerDriver::SaveGenerationFieldDetectedByClassifier(
+    const autofill::PasswordForm& password_form,
+    const base::string16& generation_field) {
+  if (!bad_message::CheckChildProcessSecurityPolicy(
+          render_frame_host_, password_form,
+          BadMessageReason::
+              CPMD_BAD_ORIGIN_SAVE_GENERATION_FIELD_DETECTED_BY_CLASSIFIER))
+    return;
+  GetPasswordManager()->SaveGenerationFieldDetectedByClassifier(
+      password_form, generation_field);
+}
 
-  content::ChildProcessSecurityPolicy* policy =
-      content::ChildProcessSecurityPolicy::GetInstance();
-  if (!policy->CanAccessDataForOrigin(render_frame_host_->GetProcess()->GetID(),
-                                      url)) {
-    SYSLOG(WARNING) << "Killing renderer: illegal password access. Reason: "
-                    << static_cast<int>(reason);
-    bad_message::ReceivedBadMessage(render_frame_host_->GetProcess(), reason);
-    return false;
-  }
+void ContentPasswordManagerDriver::CheckSafeBrowsingReputation(
+    const GURL& form_action,
+    const GURL& frame_url) {
+#if defined(SAFE_BROWSING_DB_LOCAL)
+  client_->CheckSafeBrowsingReputation(form_action, frame_url);
+#endif
+}
 
-  return true;
+void ContentPasswordManagerDriver::OnFocusedPasswordFormFound(
+    const autofill::PasswordForm& password_form) {
+  if (!bad_message::CheckChildProcessSecurityPolicy(
+          render_frame_host_, password_form,
+          BadMessageReason::CPMD_BAD_ORIGIN_FOCUSED_PASSWORD_FORM_FOUND))
+    return;
+  GetPasswordManager()->OnPasswordFormForceSaveRequested(this, password_form);
 }
 
 const autofill::mojom::AutofillAgentPtr&

components/password_manager/content/browser/content_password_manager_driver.h

@@ -72,22 +72,22 @@ class ContentPasswordManagerDriver
       const autofill::PasswordFormFillData& form_data) override;
   void ClearPreviewedForm() override;
   void ForceSavePassword() override;
-  void ShowManualFallbackForSaving(const autofill::PasswordForm& form) override;
-  void HideManualFallbackForSaving() override;
   void GeneratePassword() override;
+  PasswordGenerationManager* GetPasswordGenerationManager() override;
+  PasswordManager* GetPasswordManager() override;
+  PasswordAutofillManager* GetPasswordAutofillManager() override;
   void SendLoggingAvailability() override;
   void AllowToRunFormClassifier() override;
   autofill::AutofillDriver* GetAutofillDriver() override;
   bool IsMainFrame() const override;
   void MatchingBlacklistedFormFound() override;
 
-  PasswordGenerationManager* GetPasswordGenerationManager() override;
-  PasswordManager* GetPasswordManager() override;
-  PasswordAutofillManager* GetPasswordAutofillManager() override;
-
   void DidNavigateFrame(content::NavigationHandle* navigation_handle);
 
   // autofill::mojom::PasswordManagerDriver:
+  // Note that these messages received from a potentially compromised renderer.
+  // For that reason, any access to form data should be validated via
+  // bad_message::CheckChildProcessSecurityPolicy.
   void PasswordFormsParsed(
       const std::vector<autofill::PasswordForm>& forms) override;
   void PasswordFormsRendered(
@@ -95,6 +95,8 @@ class ContentPasswordManagerDriver
       bool did_stop_loading) override;
   void PasswordFormSubmitted(
       const autofill::PasswordForm& password_form) override;
+  void ShowManualFallbackForSaving(const autofill::PasswordForm& form) override;
+  void HideManualFallbackForSaving() override;
   void SameDocumentNavigation(
       const autofill::PasswordForm& password_form) override;
   void ShowPasswordSuggestions(int key,
@@ -112,13 +114,8 @@ class ContentPasswordManagerDriver
   void CheckSafeBrowsingReputation(const GURL& form_action,
                                    const GURL& frame_url) override;
 
-  void OnPasswordFormsParsedNoRenderCheck(
-      const std::vector<autofill::PasswordForm>& forms);
-  void OnFocusedPasswordFormFound(const autofill::PasswordForm& password_form);
-
  private:
-  bool CheckChildProcessSecurityPolicy(const GURL& url,
-                                       BadMessageReason reason);
+  void OnFocusedPasswordFormFound(const autofill::PasswordForm& password_form);
 
   const autofill::mojom::AutofillAgentPtr& GetAutofillAgent();
 

components/password_manager/core/browser/password_manager_driver.h

@@ -86,14 +86,6 @@ class PasswordManagerDriver
   // the corresponding password form, so that it can be saved.
   virtual void ForceSavePassword() {}
 
-  // Tells the driver to show the manual fallback for password saving, i.e. to
-  // show the omnibox icon with anchored hidden save prompt.
-  virtual void ShowManualFallbackForSaving(const autofill::PasswordForm& form) {
-  }
-
-  // Tells the driver to hide the manual fallback for saving.
-  virtual void HideManualFallbackForSaving() {}
-
   // Tells the driver to find the focused password field and to show generation
   // popup at it.
   virtual void GeneratePassword() {}