New issue
Advanced search Search tips

Issue 636986 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Aug 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug
Team-Security-UX

Blocked on:
issue 634171



Sign in to add a comment

DevTools security panel shows mixed content after clicking through a certificate error

Project Member Reported by est...@chromium.org, Aug 11 2016

Issue description

This is a recent regression, caused by https://codereview.chromium.org/2191113002/. That commit causes the renderer to send messages to the browser any time a subresource with a cert error is loaded, whereas before we were only sending messages to the browser for subresources with cert errors that are different than the main resource's cert error. For some reason I convinced myself that this didn't affect any browser UI, but apparently I completely forgot about DevTools, because that change means that we now display the mixed content warning in the security panel for any page with a certificate error that loads same-origin subresources.

The real issue here is that the browser treats subresources with cert errors the same as mixed content, which isn't correct and causes problems like this. I have a series of CLs ready for review to fix that ( issue 634171 ).

The other real issue is that we don't have any way of end-to-end testing for issues like this in devtools, as far as I know. :(
 
Project Member

Comment 1 by bugdroid1@chromium.org, Aug 12 2016

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

commit 54f191f54bfac1f2400a42ece329516e225a2a75
Author: estark <estark@chromium.org>
Date: Fri Aug 12 00:27:32 2016

Rename SecurityStateModel::MIXED_CONTENT_STATUS enum values

I'm planning to add information about subresources with certificate
errors to SecurityStateModel. This CL renames the MixedContentStatus
enum values to be more generic, so that we can use the same
passive/displayed, active/ran distinction for subresources with cert
errors that we use for mixed content.

#1: https://codereview.chromium.org/2224193003/
  (this CL)
#2: https://codereview.chromium.org/2224023003/
  Teach SecurityStateModel about subresources with cert errors
#3: https://codereview.chromium.org/2225213004/
  Teach SSLHostStateDelegate about subresources with cert errors
#4: https://codereview.chromium.org/2226363002/
  Track subresources with cert errors separately from mixed content

BUG= 634171 , 636986 

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

[modify] https://crrev.com/54f191f54bfac1f2400a42ece329516e225a2a75/chrome/browser/ssl/chrome_security_state_model_client.cc
[modify] https://crrev.com/54f191f54bfac1f2400a42ece329516e225a2a75/chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc
[modify] https://crrev.com/54f191f54bfac1f2400a42ece329516e225a2a75/chrome/browser/ssl/security_state_model_android.cc
[modify] https://crrev.com/54f191f54bfac1f2400a42ece329516e225a2a75/chrome/browser/ui/website_settings/website_settings.cc
[modify] https://crrev.com/54f191f54bfac1f2400a42ece329516e225a2a75/chrome/browser/ui/website_settings/website_settings_unittest.cc
[modify] https://crrev.com/54f191f54bfac1f2400a42ece329516e225a2a75/components/security_state/security_state_model.cc
[modify] https://crrev.com/54f191f54bfac1f2400a42ece329516e225a2a75/components/security_state/security_state_model.h
[modify] https://crrev.com/54f191f54bfac1f2400a42ece329516e225a2a75/components/security_state/security_state_model_unittest.cc

Project Member

Comment 2 by bugdroid1@chromium.org, Aug 12 2016

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

commit 61ab9d4706f6887539b9097bdbbc40ec6e671c85
Author: estark <estark@chromium.org>
Date: Fri Aug 12 02:08:57 2016

Teach SecurityStateModel about subresources with cert errors

This is the second in a series of CLs to create dedicated plumbing for
subresources with certificate errors, instead of treating them like
mixed content from the browser's perspective.

This CL adds flags for subresources with cert errors to
SecurityStateModel and SSLStatus. ChromeSecurityStateModelClient sets
these flags on the VisibleSecurityState based on the NavigationEntry's
SSLStatus, and SecurityStateModel downgrades the security level
appropriately.

Nothing yet sets the flags on the SSLStatus.

#1: https://codereview.chromium.org/2224193003/
  Rename SecurityStateModel::MIXED_CONTENT_STATUS enum values
#2: https://codereview.chromium.org/2224023003/
  (this CL)
#3: https://codereview.chromium.org/2225213004/
  Teach SSLHostStateDelegate about subresources with cert errors
#4: https://codereview.chromium.org/2226363002/
  Track subresources with cert errors separately from mixed content

BUG= 634171 , 636986 

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

[modify] https://crrev.com/61ab9d4706f6887539b9097bdbbc40ec6e671c85/chrome/browser/ssl/chrome_security_state_model_client.cc
[modify] https://crrev.com/61ab9d4706f6887539b9097bdbbc40ec6e671c85/chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc
[modify] https://crrev.com/61ab9d4706f6887539b9097bdbbc40ec6e671c85/components/security_state/security_state_model.cc
[modify] https://crrev.com/61ab9d4706f6887539b9097bdbbc40ec6e671c85/components/security_state/security_state_model.h
[modify] https://crrev.com/61ab9d4706f6887539b9097bdbbc40ec6e671c85/content/public/common/ssl_status.h

Project Member

Comment 3 by bugdroid1@chromium.org, Aug 12 2016

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

commit 854d78b541e9a8132f50985fc434d4d43c7417b1
Author: estark <estark@chromium.org>
Date: Fri Aug 12 03:30:33 2016

Teach SSLHostStateDelegate about subresources with cert errors

This is the third in a series of CLs to create dedicated plumbing for
subresources with certificate errors, instead of treating them like
mixed content from the browser's perspective.

This CL teaches SSLHostStateDelegate (and its implementation
ChromeSSLHostStateDelegate) about subresources with certificate errors,
as distinct from mixed content. ChromeSSLHostStateDelegate keeps two
maps of broken hosts, one for mixed content and one for subresources
with cert errors. HostRanInsecureContent() and
DidHostRunInsecureContent() take an argument to indicate whether the
subresource in question was mixed or had cert errors. (An alternative
would be to have a separate set of methods for cert errors; I could be
convinced either way.)

#1: https://codereview.chromium.org/2224193003/
  Rename SecurityStateModel::MIXED_CONTENT_STATUS enum values
#2: https://codereview.chromium.org/2224023003/
  Teach SecurityStateModel about subresources with cert errors
#3: https://codereview.chromium.org/2225213004/
  (this CL)
#4: https://codereview.chromium.org/2226363002/
  Track subresources with cert errors separately from mixed content

BUG= 634171 , 636986 

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

[modify] https://crrev.com/854d78b541e9a8132f50985fc434d4d43c7417b1/android_webview/browser/aw_ssl_host_state_delegate.cc
[modify] https://crrev.com/854d78b541e9a8132f50985fc434d4d43c7417b1/android_webview/browser/aw_ssl_host_state_delegate.h
[modify] https://crrev.com/854d78b541e9a8132f50985fc434d4d43c7417b1/chrome/browser/ssl/chrome_ssl_host_state_delegate.cc
[modify] https://crrev.com/854d78b541e9a8132f50985fc434d4d43c7417b1/chrome/browser/ssl/chrome_ssl_host_state_delegate.h
[modify] https://crrev.com/854d78b541e9a8132f50985fc434d4d43c7417b1/chrome/browser/ssl/chrome_ssl_host_state_delegate_test.cc
[modify] https://crrev.com/854d78b541e9a8132f50985fc434d4d43c7417b1/content/browser/ssl/ssl_policy_backend.cc
[modify] https://crrev.com/854d78b541e9a8132f50985fc434d4d43c7417b1/content/public/browser/ssl_host_state_delegate.h
[modify] https://crrev.com/854d78b541e9a8132f50985fc434d4d43c7417b1/content/test/mock_ssl_host_state_delegate.cc
[modify] https://crrev.com/854d78b541e9a8132f50985fc434d4d43c7417b1/content/test/mock_ssl_host_state_delegate.h

Project Member

Comment 4 by bugdroid1@chromium.org, Aug 12 2016

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

commit cd2e30cd6679e2249af296807d7270087879919d
Author: estark <estark@chromium.org>
Date: Fri Aug 12 06:51:15 2016

Track subresources with cert errors separately from mixed content

This is the final in a series of CLs to create dedicated plumbing for
subresources with certificate errors, instead of treating them like
mixed content from the browser's perspective.

Previously, when WebContentsImpl learned about a subresource with
certificate errors, it set its own DisplayedInsecureContent() flag for a
passive resource, or notified SSLManager::HostRanInsecureContent() about
an active subresource. This is the same set of mechanisms that was used
for subresources loaded over HTTP on HTTPS pages (mixed content).

In this CL, SSLManager, SSLPolicy, and SSLPolicyBackend get mechanisms
for tracking subresources with cert errors separately from mixed
content. WebContentsImpl also gets a DisplayedContentWithCertErrors()
flag separate from DisplayedInsecureContent() which it uses for mixed
content.

This separate set of plumbing allows browser UI to distinguish mixed
content from subresources with cert errors; for example, the DevTools
security panel should use a separate message to describe these two
different situations. (This CL does not actually make aforementioned
UI changes, but rather just sets up the plumbing to do so.)

#1: https://codereview.chromium.org/2224193003/
  Rename SecurityStateModel::MIXED_CONTENT_STATUS enum values
#2: https://codereview.chromium.org/2224023003/
  Teach SecurityStateModel about subresources with cert errors
#3: https://codereview.chromium.org/2225213004/
  Teach SSLHostStateDelegate about subresources with cert errors
#4: https://codereview.chromium.org/2226363002/
  (this CL)

BUG= 634171 , 636986 

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

[modify] https://crrev.com/cd2e30cd6679e2249af296807d7270087879919d/chrome/browser/ssl/ssl_browser_tests.cc
[modify] https://crrev.com/cd2e30cd6679e2249af296807d7270087879919d/content/browser/service_worker/service_worker_browsertest.cc
[modify] https://crrev.com/cd2e30cd6679e2249af296807d7270087879919d/content/browser/site_per_process_browsertest.cc
[modify] https://crrev.com/cd2e30cd6679e2249af296807d7270087879919d/content/browser/ssl/ssl_manager.cc
[modify] https://crrev.com/cd2e30cd6679e2249af296807d7270087879919d/content/browser/ssl/ssl_manager.h
[modify] https://crrev.com/cd2e30cd6679e2249af296807d7270087879919d/content/browser/ssl/ssl_policy.cc
[modify] https://crrev.com/cd2e30cd6679e2249af296807d7270087879919d/content/browser/ssl/ssl_policy.h
[modify] https://crrev.com/cd2e30cd6679e2249af296807d7270087879919d/content/browser/ssl/ssl_policy_backend.cc
[modify] https://crrev.com/cd2e30cd6679e2249af296807d7270087879919d/content/browser/ssl/ssl_policy_backend.h
[modify] https://crrev.com/cd2e30cd6679e2249af296807d7270087879919d/content/browser/web_contents/web_contents_impl.cc
[modify] https://crrev.com/cd2e30cd6679e2249af296807d7270087879919d/content/browser/web_contents/web_contents_impl.h
[modify] https://crrev.com/cd2e30cd6679e2249af296807d7270087879919d/content/public/browser/web_contents.h

Comment 5 by est...@chromium.org, Aug 12 2016

Status: Fixed (was: Started)
Should be fixed now. DevTools now won't say anything in particular about subresources with cert errors. It should have an explanation for them, which I'm tracking in  issue 587172 .
Components: -Security>UX

Sign in to add a comment