Preparation for removal of sending content::SSLStatus to the renderer.

Preparation for removal of sending content::SSLStatus to the renderer.

In this particular path (WebFrameClient::didDisplayContentWithCertificateErrors and didRunContentWithCertificateErrors), the information needed is mostly from net's SSLInfo. I removed content::SecurityStyle from the IsContentWithCertificateErrorsRelevantToUI check since it's based on other members from SSLInfo that are compared. This allows future changes to not send content::SSLStatus to the renderer process.

Also stop sending the serialized security_info string to the browser since it's not used.

BUG=598073
Committed: https://crrev.com/a385e11086b4b17f926a4fe95a22a00e9918bbdb
Cr-Commit-Position: refs/heads/master@{#407001}

[jam, 2016-07-20 18:21:48]

The CQ bit was checked by jam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-20 18:22:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jam, 2016-07-20 18:26:53]

jam@chromium.org changed reviewers:
+ estark@chromium.org

[jam, 2016-07-20 18:26:54]

I first tried to see if we can stop sending this data (even a reduced version of
what's in content::SSLStatus) to the renderer. That didn't appear possible for
this case, since we depend on the callbacks from Blink which are per-resource
related certificate error and the browser doesn't save the SSL state for random
responses.

If the IsContentWithCertificateErrorsRelevantToUI call is just an optimization,
and we can remove it completely (i.e. don't need to run it in the renderer OR
browser), then that would be even better because then we can remove sending the
SSL info data to the renderer.

[commit-bot: I haz the power, 2016-07-20 20:14:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-20 20:15:00]

Dry run: This issue passed the CQ dry run.

[jam, 2016-07-20 20:25:33]

btw I'm curious if there are test cases for the iframe case? I couldn't find any
on badssl.com.

I'm looking for a case where both the main frame has a bad cert and also an
iframe.

[estark, 2016-07-21 01:09:47]

lgtm

> If the IsContentWithCertificateErrorsRelevantToUI call is just an
optimization,
> and we can remove it completely (i.e. don't need to run it in the renderer OR
> browser), then that would be even better because then we can remove sending
the
> SSL info data to the renderer.

I don't think that's the case. :( It's an optimization to do the check in the
renderer (to avoid sending an unnecessary IPC to the browser), but the UI would
get into a weird state if we didn't do the check at all. If you click through an
interstitial on a page with certificate errors, we'd end up treating all
same-origin subresources on that page as mixed content. IIRC this would have
been a problem for Chrome but an even bigger one for Opera security UI.

> btw I'm curious if there are test cases for the iframe case? I couldn't find
any
> on http://badssl.com.
> 
> I'm looking for a case where both the main frame has a bad cert and also an
> iframe.

Hmm, there are browser tests in ssl_browser_tests.cc but it looks like they test
images and scripts rather than iframes. I'd probably just hack it together by
using devtools to add an iframe to a main frame with a bad cert. Or poke
lgarron@ to add this case to badssl.com. :) (I've cc'ed him.)

[jam, 2016-07-21 15:56:05]

The CQ bit was checked by jam@chromium.org

[commit-bot: I haz the power, 2016-07-21 15:56:27]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jam, 2016-07-21 15:56:28]

On 2016/07/21 01:09:47, estark wrote:
> lgtm
> 
> > If the IsContentWithCertificateErrorsRelevantToUI call is just an
> optimization,
> > and we can remove it completely (i.e. don't need to run it in the renderer
OR
> > browser), then that would be even better because then we can remove sending
> the
> > SSL info data to the renderer.
> 
> I don't think that's the case. :( It's an optimization to do the check in the
> renderer (to avoid sending an unnecessary IPC to the browser), but the UI
would
> get into a weird state if we didn't do the check at all. If you click through
an
> interstitial on a page with certificate errors, we'd end up treating all
> same-origin subresources on that page as mixed content. IIRC this would have
> been a problem for Chrome but an even bigger one for Opera security UI.

Ah, ok thanks for the explanation.

[commit-bot: I haz the power, 2016-07-21 16:01:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-21 16:01:59]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[lgarron, 2016-07-21 16:18:07]

Description was changed from

==========
Prepartion for removal of sending content::SSLStatus to the renderer.

In this particular path (WebFrameClient::didDisplayContentWithCertificateErrors
and didRunContentWithCertificateErrors), the information needed is mostly from
net's SSLInfo. I removed content::SecurityStyle from the
IsContentWithCertificateErrorsRelevantToUI check since it's based on other
members from SSLInfo that are compared. This allows future changes to not send
content::SSLStatus to the renderer process.

Also stop sending the serialized security_info string to the browser since it's
not used.

BUG=598073
==========

to

==========
Preparation for removal of sending content::SSLStatus to the renderer.

In this particular path (WebFrameClient::didDisplayContentWithCertificateErrors
and didRunContentWithCertificateErrors), the information needed is mostly from
net's SSLInfo. I removed content::SecurityStyle from the
IsContentWithCertificateErrorsRelevantToUI check since it's based on other
members from SSLInfo that are compared. This allows future changes to not send
content::SSLStatus to the renderer process.

Also stop sending the serialized security_info string to the browser since it's
not used.

BUG=598073
==========

[lgarron, 2016-07-21 16:19:34]

On 2016/07/21 at 16:18:07, lgarron wrote:
> Description was changed from
> 
> ==========
> Prepartion for removal of sending content::SSLStatus to the renderer.
> 
> In this particular path
(WebFrameClient::didDisplayContentWithCertificateErrors and
didRunContentWithCertificateErrors), the information needed is mostly from net's
SSLInfo. I removed content::SecurityStyle from the
IsContentWithCertificateErrorsRelevantToUI check since it's based on other
members from SSLInfo that are compared. This allows future changes to not send
content::SSLStatus to the renderer process.
> 
> Also stop sending the serialized security_info string to the browser since
it's not used.
> 
> BUG=598073
> ==========
> 
> to
> 
> ==========
> Preparation for removal of sending content::SSLStatus to the renderer.
> 
> In this particular path
(WebFrameClient::didDisplayContentWithCertificateErrors and
didRunContentWithCertificateErrors), the information needed is mostly from net's
SSLInfo. I removed content::SecurityStyle from the
IsContentWithCertificateErrorsRelevantToUI check since it's based on other
members from SSLInfo that are compared. This allows future changes to not send
content::SSLStatus to the renderer process.
> 
> Also stop sending the serialized security_info string to the browser since
it's not used.
> 
> BUG=598073
> ==========

You can get an HTTP frame at https://expired.badssl.com/mixed/iframe

Is that sufficient?

[estark, 2016-07-21 16:45:52]

On 2016/07/21 16:19:34, lgarron wrote:
> On 2016/07/21 at 16:18:07, lgarron wrote:
> > Description was changed from
> > 
> > ==========
> > Prepartion for removal of sending content::SSLStatus to the renderer.
> > 
> > In this particular path
> (WebFrameClient::didDisplayContentWithCertificateErrors and
> didRunContentWithCertificateErrors), the information needed is mostly from
net's
> SSLInfo. I removed content::SecurityStyle from the
> IsContentWithCertificateErrorsRelevantToUI check since it's based on other
> members from SSLInfo that are compared. This allows future changes to not send
> content::SSLStatus to the renderer process.
> > 
> > Also stop sending the serialized security_info string to the browser since
> it's not used.
> > 
> > BUG=598073
> > ==========
> > 
> > to
> > 
> > ==========
> > Preparation for removal of sending content::SSLStatus to the renderer.
> > 
> > In this particular path
> (WebFrameClient::didDisplayContentWithCertificateErrors and
> didRunContentWithCertificateErrors), the information needed is mostly from
net's
> SSLInfo. I removed content::SecurityStyle from the
> IsContentWithCertificateErrorsRelevantToUI check since it's based on other
> members from SSLInfo that are compared. This allows future changes to not send
> content::SSLStatus to the renderer process.
> > 
> > Also stop sending the serialized security_info string to the browser since
> it's not used.
> > 
> > BUG=598073
> > ==========
> 
> You can get an HTTP frame at https://expired.badssl.com/mixed/iframe
> 
> Is that sufficient?

I believe jam was looking for a broken-HTTPS frame, e.g.
https://expired.badssl.com framing https://self-signed.badssl.com.

[jam, 2016-07-21 21:40:40]

jam@chromium.org changed reviewers:
+ nasko@chromium.org

[jam, 2016-07-21 21:40:53]

+nasko for ipc change

[jam, 2016-07-21 21:42:33]

On 2016/07/21 16:45:52, estark wrote:
> On 2016/07/21 16:19:34, lgarron wrote:
> > On 2016/07/21 at 16:18:07, lgarron wrote:
> > > Description was changed from
> > > 
> > > ==========
> > > Prepartion for removal of sending content::SSLStatus to the renderer.
> > > 
> > > In this particular path
> > (WebFrameClient::didDisplayContentWithCertificateErrors and
> > didRunContentWithCertificateErrors), the information needed is mostly from
> net's
> > SSLInfo. I removed content::SecurityStyle from the
> > IsContentWithCertificateErrorsRelevantToUI check since it's based on other
> > members from SSLInfo that are compared. This allows future changes to not
send
> > content::SSLStatus to the renderer process.
> > > 
> > > Also stop sending the serialized security_info string to the browser since
> > it's not used.
> > > 
> > > BUG=598073
> > > ==========
> > > 
> > > to
> > > 
> > > ==========
> > > Preparation for removal of sending content::SSLStatus to the renderer.
> > > 
> > > In this particular path
> > (WebFrameClient::didDisplayContentWithCertificateErrors and
> > didRunContentWithCertificateErrors), the information needed is mostly from
> net's
> > SSLInfo. I removed content::SecurityStyle from the
> > IsContentWithCertificateErrorsRelevantToUI check since it's based on other
> > members from SSLInfo that are compared. This allows future changes to not
send
> > content::SSLStatus to the renderer process.
> > > 
> > > Also stop sending the serialized security_info string to the browser since
> > it's not used.
> > > 
> > > BUG=598073
> > > ==========
> > 
> > You can get an HTTP frame at https://expired.badssl.com/mixed/iframe
> > 
> > Is that sufficient?
> 
> I believe jam was looking for a broken-HTTPS frame, e.g.
> https://expired.badssl.com framing https://self-signed.badssl.com.

right

[nasko, 2016-07-21 21:52:09]

Passing less untrusted data to the browser -> LGTM :)

https://codereview.chromium.org/2167773002/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_impl.cc (left):

https://codereview.chromium.org/2167773002/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:3500:
bad_message::WC_CONTENT_WITH_CERT_ERRORS_BAD_SECURITY_INFO);
nit: Let's mark this value as no longer used in the header and histograms.xml.

[jam, 2016-07-21 22:44:05]

https://codereview.chromium.org/2167773002/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_impl.cc (left):

https://codereview.chromium.org/2167773002/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:3500:
bad_message::WC_CONTENT_WITH_CERT_ERRORS_BAD_SECURITY_INFO);
On 2016/07/21 21:52:09, nasko wrote:
> nit: Let's mark this value as no longer used in the header and histograms.xml.

I've updated the header per the other example there (leaving a comment). That
didn't end up updating histograms.xml, even after running
update_bad_message_reasons.py

[jam, 2016-07-21 22:44:13]

The CQ bit was checked by jam@chromium.org

[jam, 2016-07-21 22:44:14]

The patchset sent to the CQ was uploaded after l-g-t-m from nasko@chromium.org,
estark@chromium.org
Link to the patchset: https://codereview.chromium.org/2167773002/#ps20001
(title: "review comment")

[commit-bot: I haz the power, 2016-07-21 22:45:06]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-22 00:08:37]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-07-22 00:11:54]

Description was changed from

==========
Preparation for removal of sending content::SSLStatus to the renderer.

In this particular path (WebFrameClient::didDisplayContentWithCertificateErrors
and didRunContentWithCertificateErrors), the information needed is mostly from
net's SSLInfo. I removed content::SecurityStyle from the
IsContentWithCertificateErrorsRelevantToUI check since it's based on other
members from SSLInfo that are compared. This allows future changes to not send
content::SSLStatus to the renderer process.

Also stop sending the serialized security_info string to the browser since it's
not used.

BUG=598073
==========

to

==========
Preparation for removal of sending content::SSLStatus to the renderer.

In this particular path (WebFrameClient::didDisplayContentWithCertificateErrors
and didRunContentWithCertificateErrors), the information needed is mostly from
net's SSLInfo. I removed content::SecurityStyle from the
IsContentWithCertificateErrorsRelevantToUI check since it's based on other
members from SSLInfo that are compared. This allows future changes to not send
content::SSLStatus to the renderer process.

Also stop sending the serialized security_info string to the browser since it's
not used.

BUG=598073

Committed: https://crrev.com/a385e11086b4b17f926a4fe95a22a00e9918bbdb
Cr-Commit-Position: refs/heads/master@{#407001}
==========

[commit-bot: I haz the power, 2016-07-22 00:11:55]

Patchset 2 (id:??) landed as
https://crrev.com/a385e11086b4b17f926a4fe95a22a00e9918bbdb
Cr-Commit-Position: refs/heads/master@{#407001}