New issue
Advanced search Search tips

Issue 647560 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Oct 2016
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Feature
Team-Security-UX

Blocked on:
issue 654079
issue 653240
issue 654082

Blocking:
issue 647754
issue 648838



Sign in to add a comment

Update SSLStatus when passwords are detected in a page [Desktop]

Project Member Reported by f...@chromium.org, Sep 16 2016

Issue description

Create a new class, HttpDowngradeHandler:
This class will be responsible for handling events, checking whether the top-level frame is displaying a non-secure origin, and (if so) updating the SecurityStateModel.
It will implement OnPasswordFormsRendered.
It will require access to a WebContentsImpl.
This class will have //content dependencies, but it should not have any //chrome dependencies.

Update the ContentPasswordManagerDriver:
It should create a HttpDowngradeHandler, passing it a WebContents or WebContentsImpl. (It can get this from content::WebContents::FromRenderFrameHost(render_frame_host_).
When PasswordFormsRendered is called, it will  call http_downgrade_handler_->OnPasswordFormsRendered.

The detection code should record the event on the NavigationEntry’s SSLStatus.
Extend SSLStatus to be aware of HTTP bad detection events. Specifically, we’ll add new flags to the ContentStatusFlags enum: PASSWORD_DETECTED and CC_DETECTED. These flags should be set on the current NavigationEntry’s SSLStatus whenever the detection logic identifies a password or credit card form field.

The detection code should trigger a refresh of the security UI elements by calling WebContentsImpl::DidChangeVisibleSSLState. This will cause the security UI elements to request updated security information from the SSM.

https://docs.google.com/document/d/1xno6g6OnA7strcyzE-o_drevW8L0Mb6ZBEkjsiwa6x0/edit#heading=h.ea8pa1mzsvg8
 

Comment 1 by f...@chromium.org, Sep 16 2016

Blocking: 647754

Comment 2 by est...@chromium.org, Sep 21 2016

Owner: est...@chromium.org
Status: Assigned (was: Available)
Updated plan from the doc (replacing HttpDowngradeHandler with a WebContents method):

Create a new WebContentsImpl method, OnSensitiveInputShownOnHttp:
This method should be called whenever a top-level frame is displaying a non-secure origin and displays a password or credit card input. It should take the input type (password or credit card) as an argument.
When called, this method should set a member on the WebContentsImpl, displayed_sensitive_input_on_http_. This flag should be cleared when the main frame navigates. (This parallels the handling of passive mixed content.)

After setting the flag, the method should call SSLManager::NotifySSLInternalStateChanged(), which triggers a UI refresh via DidChangeVisibleSSLState().
SSLPolicy::UpdateEntry should check the member on the WebContentsImpl in order to set the new SSLStatus flags (defined below).

Update the ContentPasswordManagerDriver:
When PasswordFormsRendered is called, it should retrieve the WebContentsImpl from content::WebContents::FromRenderFrameHost(render_frame_host_) and call OnSensitiveInputShownOnHttp.

Comment 3 by est...@chromium.org, Sep 21 2016

Blocking: 648838
Blockedon: 653240
Blockedon: 654082
Blockedon: 654079
Project Member

Comment 7 by bugdroid1@chromium.org, Oct 10 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/703f1b43a576ef25b5e267beaaba5f6ef70df08f

commit 703f1b43a576ef25b5e267beaaba5f6ef70df08f
Author: estark <estark@chromium.org>
Date: Mon Oct 10 21:37:15 2016

Add (some) password detection for HTTP-bad

This CL adds WebContents methods for setting the new SSLStatus flags to
record a password or credit card field on an HTTP page. It also
(roughly) integrates with the ContentPasswordManagerDriver code to call
the new WebContents methods. The integration as written at present will
only set the SSLStatus flags whenever a password field is parsed;
eventually we will refine that to set the flags whenever a password field
is visible on the page (https://codereview.chromium.org/2378503002/).

BUG= 647560 

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

[modify] https://crrev.com/703f1b43a576ef25b5e267beaaba5f6ef70df08f/chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc
[modify] https://crrev.com/703f1b43a576ef25b5e267beaaba5f6ef70df08f/chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm
[modify] https://crrev.com/703f1b43a576ef25b5e267beaaba5f6ef70df08f/chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm
[add] https://crrev.com/703f1b43a576ef25b5e267beaaba5f6ef70df08f/chrome/test/data/password/invisible_password.html
[add] https://crrev.com/703f1b43a576ef25b5e267beaaba5f6ef70df08f/chrome/test/data/password/simple_password.html
[add] https://crrev.com/703f1b43a576ef25b5e267beaaba5f6ef70df08f/chrome/test/data/password/simple_password_in_https_iframe.html
[add] https://crrev.com/703f1b43a576ef25b5e267beaaba5f6ef70df08f/chrome/test/data/password/simple_password_in_iframe.html
[modify] https://crrev.com/703f1b43a576ef25b5e267beaaba5f6ef70df08f/components/password_manager/content/browser/content_password_manager_driver.cc
[modify] https://crrev.com/703f1b43a576ef25b5e267beaaba5f6ef70df08f/content/browser/ssl/ssl_manager.cc
[modify] https://crrev.com/703f1b43a576ef25b5e267beaaba5f6ef70df08f/content/browser/web_contents/web_contents_impl.cc
[modify] https://crrev.com/703f1b43a576ef25b5e267beaaba5f6ef70df08f/content/browser/web_contents/web_contents_impl.h
[modify] https://crrev.com/703f1b43a576ef25b5e267beaaba5f6ef70df08f/content/public/browser/web_contents.h

Project Member

Comment 8 by bugdroid1@chromium.org, Oct 12 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b3f84073ec1cb1222b32e7dc8d796d231a25efec

commit b3f84073ec1cb1222b32e7dc8d796d231a25efec
Author: estark <estark@chromium.org>
Date: Wed Oct 12 19:00:47 2016

Observe visibility of password inputs, for HTTP-bad phase 1

This CL is a step along the way to putting a warning in the omnibox when
an insecure HTTP page displays a visible password field. When a password
input field becomes visible, a Mojo IPC is sent to the browser process
and handled by the ContentPasswordManagerDriver. At the moment
ContentPasswordManagerDriver does nothing with this message, but in a
follow-up CL, it will notify the WebContents which will cause a warning
to appear in the omnibox when an HTTP page displays a visible password field.

Design doc: https://docs.google.com/document/d/1xno6g6OnA7strcyzE-o_drevW8L0Mb6ZBEkjsiwa6x0/edit#

BUG= 647560 

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

[modify] https://crrev.com/b3f84073ec1cb1222b32e7dc8d796d231a25efec/chrome/browser/chrome_content_browser_client.cc
[modify] https://crrev.com/b3f84073ec1cb1222b32e7dc8d796d231a25efec/components/password_manager/content/browser/DEPS
[modify] https://crrev.com/b3f84073ec1cb1222b32e7dc8d796d231a25efec/components/password_manager/content/browser/content_password_manager_driver.cc
[modify] https://crrev.com/b3f84073ec1cb1222b32e7dc8d796d231a25efec/components/password_manager/content/browser/content_password_manager_driver.h
[modify] https://crrev.com/b3f84073ec1cb1222b32e7dc8d796d231a25efec/components/password_manager/content/browser/content_password_manager_driver_factory.cc
[modify] https://crrev.com/b3f84073ec1cb1222b32e7dc8d796d231a25efec/components/password_manager/content/browser/content_password_manager_driver_factory.h
[modify] https://crrev.com/b3f84073ec1cb1222b32e7dc8d796d231a25efec/third_party/WebKit/Source/core/BUILD.gn
[modify] https://crrev.com/b3f84073ec1cb1222b32e7dc8d796d231a25efec/third_party/WebKit/Source/core/DEPS
[modify] https://crrev.com/b3f84073ec1cb1222b32e7dc8d796d231a25efec/third_party/WebKit/Source/core/html/forms/PasswordInputType.cpp
[modify] https://crrev.com/b3f84073ec1cb1222b32e7dc8d796d231a25efec/third_party/WebKit/Source/core/html/forms/PasswordInputType.h
[add] https://crrev.com/b3f84073ec1cb1222b32e7dc8d796d231a25efec/third_party/WebKit/Source/core/html/forms/PasswordInputTypeTest.cpp
[modify] https://crrev.com/b3f84073ec1cb1222b32e7dc8d796d231a25efec/third_party/WebKit/Source/core/html/forms/TextFieldInputType.h
[modify] https://crrev.com/b3f84073ec1cb1222b32e7dc8d796d231a25efec/third_party/WebKit/Source/core/testing/DummyPageHolder.cpp
[modify] https://crrev.com/b3f84073ec1cb1222b32e7dc8d796d231a25efec/third_party/WebKit/Source/core/testing/DummyPageHolder.h
[modify] https://crrev.com/b3f84073ec1cb1222b32e7dc8d796d231a25efec/third_party/WebKit/public/BUILD.gn
[add] https://crrev.com/b3f84073ec1cb1222b32e7dc8d796d231a25efec/third_party/WebKit/public/platform/modules/sensitive_input_visibility/OWNERS
[add] https://crrev.com/b3f84073ec1cb1222b32e7dc8d796d231a25efec/third_party/WebKit/public/platform/modules/sensitive_input_visibility/sensitive_input_visibility_service.mojom

Project Member

Comment 9 by bugdroid1@chromium.org, Oct 13 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/79b7352086134008f856ac78753aeee3dc52a166

commit 79b7352086134008f856ac78753aeee3dc52a166
Author: estark <estark@chromium.org>
Date: Thu Oct 13 01:35:54 2016

Add unit test for notifying WebContents when SSLStatus changes due to HTTP-bad

We had a bug where we failed to notify that the SSL state has changed
when there are events that trigger a "not secure" warning on HTTP pages
(specifically, password and credit card fields). We were setting the
appropriate flags on SSLStatus, but, due to an early return, we were
never notifying the WebContents which meant the omnibox would never
redraw (among other things).

This bug was fixed somewhat unintentionally in
https://codereview.chromium.org/2408393003/. This CL adds a unit test
for it, and also includes some style fixes in SSLManager::UpdateEntry().

BUG= 647560 

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

[modify] https://crrev.com/79b7352086134008f856ac78753aeee3dc52a166/content/browser/ssl/ssl_manager.cc
[add] https://crrev.com/79b7352086134008f856ac78753aeee3dc52a166/content/browser/ssl/ssl_manager_unittest.cc
[modify] https://crrev.com/79b7352086134008f856ac78753aeee3dc52a166/content/test/BUILD.gn

Project Member

Comment 10 by bugdroid1@chromium.org, Oct 13 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/98182cd39250bf946e34c5da63eaf4d5afcd77af

commit 98182cd39250bf946e34c5da63eaf4d5afcd77af
Author: estark <estark@chromium.org>
Date: Thu Oct 13 15:55:10 2016

Inform WebContents when a password field is visible

Previously, ContentPasswordManagerDriver was notifying the WebContents
whenever a password field was parsed. Instead, we want to notify the
WebContents whenever a password field becomes visible. This CL moves the
WebContents notification from PasswordFormsParsed() to
PasswordFieldVisibleInInsecureContext().

BUG= 647560 
TEST=New unit test checks that ContentPasswordManagerDriver correctly
passes the password visibility notification to the
WebContents. Pre-existing browser tests in
chrome_security_state_model_client_browsertest.cc check that everything
is hooked up correctly (i.e. when Blink sends a password visibility
message, it is received by ContentPasswordManagerDriver, passed on to
the WebContents, which records it on the SSLStatus).

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

[modify] https://crrev.com/98182cd39250bf946e34c5da63eaf4d5afcd77af/chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc
[modify] https://crrev.com/98182cd39250bf946e34c5da63eaf4d5afcd77af/components/password_manager/content/browser/content_password_manager_driver.cc
[modify] https://crrev.com/98182cd39250bf946e34c5da63eaf4d5afcd77af/components/password_manager/content/browser/content_password_manager_driver_unittest.cc

Status: Fixed (was: Assigned)
Components: -Security>UX Internals>PageSecurityState
Components: Internals>Permissions>CrowdConsent
Components: -Internals>Permissions>CrowdConsent

Sign in to add a comment