Fix incorrect SSL state being shown for client redirects.

Fix incorrect SSL state being shown for client redirects.

BUG=643173
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/0576b13b74ef273fe311a95cbbb9e1a3bc8045c5
Cr-Commit-Position: refs/heads/master@{#416849}

[jam, 2016-09-02 21:35:19]

Description was changed from

==========
Fix incorrect SSL state being shown for client redirects.

BUG=643173
==========

to

==========
Fix incorrect SSL state being shown for client redirects.

BUG=643173
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[jam, 2016-09-02 21:35:31]

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

[commit-bot: I haz the power, 2016-09-02 21:36:13]

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

[jam, 2016-09-02 21:36:42]

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

[jam, 2016-09-02 21:42:07]

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

[commit-bot: I haz the power, 2016-09-02 21:42:40]

Dry run: 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-09-02 22:30:17]

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

[commit-bot: I haz the power, 2016-09-02 22:30:18]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jam, 2016-09-02 22:38:58]

hmm, looking into test failures

[jam, 2016-09-03 01:21:47]

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

[commit-bot: I haz the power, 2016-09-03 01:22:10]

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

[jam, 2016-09-03 01:22:23]

ptal, the failing tests are now fixed

[commit-bot: I haz the power, 2016-09-03 02:52:39]

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

[commit-bot: I haz the power, 2016-09-03 02:52:39]

Dry run: This issue passed the CQ dry run.

[jam, 2016-09-03 07:25:06]

Patchset #4 (id:60001) has been deleted

[jam, 2016-09-06 15:24:42]

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

[commit-bot: I haz the power, 2016-09-06 15:24:59]

Dry run: 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-09-06 16:41:04]

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

[commit-bot: I haz the power, 2016-09-06 16:41:05]

Dry run: This issue passed the CQ dry run.

[felt, 2016-09-06 20:09:55]

https://codereview.chromium.org/2305093002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/2305093002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:1205: if
(params.url.GetOrigin() != entry->GetURL().GetOrigin())
Oof. This fixes the obvious common case but I am concerned it does not really
fix the whole issue (corner cases).

The tricky thing here is that navigating within the same origin could
theoretically still change the state, right? For example, if the new page has
mixed content or if there are malicious subresources on the new page.

[jam, 2016-09-06 20:20:37]

https://codereview.chromium.org/2305093002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/2305093002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:1205: if
(params.url.GetOrigin() != entry->GetURL().GetOrigin())
On 2016/09/06 20:09:55, felt wrote:
> Oof. This fixes the obvious common case but I am concerned it does not really
> fix the whole issue (corner cases).
> 
> The tricky thing here is that navigating within the same origin could
> theoretically still change the state, right? For example, if the new page has
> mixed content or if there are malicious subresources on the new page.
> 

If these events occur (mixed content or malicious subresources), the relevant
notifications will still be fired from the renderer and the SSLStatus will be
updated. They'll fire after the did commit provisional load IPC which is what
calls this method.

[felt, 2016-09-06 20:28:19]

lgtm

[jam, 2016-09-06 20:28:54]

https://codereview.chromium.org/2305093002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/2305093002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:1205: if
(params.url.GetOrigin() != entry->GetURL().GetOrigin())
On 2016/09/06 20:20:37, jam wrote:
> On 2016/09/06 20:09:55, felt wrote:
> > Oof. This fixes the obvious common case but I am concerned it does not
really
> > fix the whole issue (corner cases).
> > 
> > The tricky thing here is that navigating within the same origin could
> > theoretically still change the state, right? For example, if the new page
has
> > mixed content or if there are malicious subresources on the new page.
> > 
> 
> If these events occur (mixed content or malicious subresources), the relevant
> notifications will still be fired from the renderer and the SSLStatus will be
> updated. They'll fire after the did commit provisional load IPC which is what
> calls this method.

ok thinking some more about this, one issue which I think you bring up is:
-user visits https://foo.com/mixedcontent
-the SSLStatus is later updated because of mixed content from that page
-page uses window.location.replace to go to https://foo.com/goodpage
-we don't create a new SSLStatus and use the old bad one

so it seems we would end up in a situation where a bad icon is used when it
should be good. the other way around shouldn't be possible (because of my
earlier reply). i'll look some more into differentiating this case.

[felt, 2016-09-06 20:31:38]

On 2016/09/06 20:28:54, jam wrote:
>
https://codereview.chromium.org/2305093002/diff/40001/content/browser/frame_h...
> File content/browser/frame_host/navigation_controller_impl.cc (right):
> 
>
https://codereview.chromium.org/2305093002/diff/40001/content/browser/frame_h...
> content/browser/frame_host/navigation_controller_impl.cc:1205: if
> (params.url.GetOrigin() != entry->GetURL().GetOrigin())
> On 2016/09/06 20:20:37, jam wrote:
> > On 2016/09/06 20:09:55, felt wrote:
> > > Oof. This fixes the obvious common case but I am concerned it does not
> really
> > > fix the whole issue (corner cases).
> > > 
> > > The tricky thing here is that navigating within the same origin could
> > > theoretically still change the state, right? For example, if the new page
> has
> > > mixed content or if there are malicious subresources on the new page.
> > > 
> > 
> > If these events occur (mixed content or malicious subresources), the
relevant
> > notifications will still be fired from the renderer and the SSLStatus will
be
> > updated. They'll fire after the did commit provisional load IPC which is
what
> > calls this method.
> 
> ok thinking some more about this, one issue which I think you bring up is:
> -user visits https://foo.com/mixedcontent
> -the SSLStatus is later updated because of mixed content from that page
> -page uses window.location.replace to go to https://foo.com/goodpage
> -we don't create a new SSLStatus and use the old bad one
> 
> so it seems we would end up in a situation where a bad icon is used when it
> should be good. the other way around shouldn't be possible (because of my
> earlier reply). i'll look some more into differentiating this case.

(not lgtm)

you're right, I hadn't thought of it the other way around and that does still
seem like a problem.

[jam, 2016-09-06 20:56:47]

On 2016/09/06 20:31:38, felt wrote:
> On 2016/09/06 20:28:54, jam wrote:
> >
>
https://codereview.chromium.org/2305093002/diff/40001/content/browser/frame_h...
> > File content/browser/frame_host/navigation_controller_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/2305093002/diff/40001/content/browser/frame_h...
> > content/browser/frame_host/navigation_controller_impl.cc:1205: if
> > (params.url.GetOrigin() != entry->GetURL().GetOrigin())
> > On 2016/09/06 20:20:37, jam wrote:
> > > On 2016/09/06 20:09:55, felt wrote:
> > > > Oof. This fixes the obvious common case but I am concerned it does not
> > really
> > > > fix the whole issue (corner cases).
> > > > 
> > > > The tricky thing here is that navigating within the same origin could
> > > > theoretically still change the state, right? For example, if the new
page
> > has
> > > > mixed content or if there are malicious subresources on the new page.
> > > > 
> > > 
> > > If these events occur (mixed content or malicious subresources), the
> relevant
> > > notifications will still be fired from the renderer and the SSLStatus will
> be
> > > updated. They'll fire after the did commit provisional load IPC which is
> what
> > > calls this method.
> > 
> > ok thinking some more about this, one issue which I think you bring up is:
> > -user visits https://foo.com/mixedcontent
> > -the SSLStatus is later updated because of mixed content from that page
> > -page uses window.location.replace to go to https://foo.com/goodpage
> > -we don't create a new SSLStatus and use the old bad one
> > 
> > so it seems we would end up in a situation where a bad icon is used when it
> > should be good. the other way around shouldn't be possible (because of my
> > earlier reply). i'll look some more into differentiating this case.
> 
> (not lgtm)
> 
> you're right, I hadn't thought of it the other way around and that does still
> seem like a problem.

ok I just tested this manually and it actually works fine. i visited
https://mixed.badssl.com/ which shows the lock icon displaying mixed icon.

i then did window.location.replace("https://mixed.badssl.com/image.jpg"). while
the codepath that I added didn't reset the SSLStatus, it turns out that in the
same callstack (below) WebContentsImpl::DidNavigateMainFramePostCommit is called
and this codepath:

  if (!details.is_in_page) {
    // Once the main frame is navigated, we're no longer considered to have
    // displayed insecure content.
    displayed_insecure_content_ = false;
    displayed_content_with_cert_errors_ = false;
    SSLManager::NotifySSLInternalStateChanged(
        GetController().GetBrowserContext());
  }

resets the SSLState for real navigations.



 	content.dll!content::SSLPolicy::UpdateEntry(content::NavigationEntryImpl *
entry, content::WebContents * web_contents) Line 204	C++
 	content.dll!content::SSLManager::UpdateEntry(content::NavigationEntryImpl *
entry) Line 194	C++

	content.dll!content::SSLManager::NotifySSLInternalStateChanged(content::BrowserContext
* context) Line 121	C++
>	content.dll!content::WebContentsImpl::DidNavigateMainFramePostCommit(content::RenderFrameHostImpl
* render_frame_host, const content::LoadCommittedDetails & details, const
FrameHostMsg_DidCommitProvisionalLoad_Params & params) Line 3384	C++
 	content.dll!content::NavigatorImpl::DidNavigate(content::RenderFrameHostImpl *
render_frame_host, const FrameHostMsg_DidCommitProvisionalLoad_Params & params)
Line 651	C++
 	content.dll!content::RenderFrameHostImpl::OnDidCommitProvisionalLoad(const
IPC::Message & msg) Line 1209	C++

[jam, 2016-09-06 21:24:24]

On 2016/09/06 20:56:47, jam wrote:
> On 2016/09/06 20:31:38, felt wrote:
> > On 2016/09/06 20:28:54, jam wrote:
> > >
> >
>
https://codereview.chromium.org/2305093002/diff/40001/content/browser/frame_h...
> > > File content/browser/frame_host/navigation_controller_impl.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2305093002/diff/40001/content/browser/frame_h...
> > > content/browser/frame_host/navigation_controller_impl.cc:1205: if
> > > (params.url.GetOrigin() != entry->GetURL().GetOrigin())
> > > On 2016/09/06 20:20:37, jam wrote:
> > > > On 2016/09/06 20:09:55, felt wrote:
> > > > > Oof. This fixes the obvious common case but I am concerned it does not
> > > really
> > > > > fix the whole issue (corner cases).
> > > > > 
> > > > > The tricky thing here is that navigating within the same origin could
> > > > > theoretically still change the state, right? For example, if the new
> page
> > > has
> > > > > mixed content or if there are malicious subresources on the new page.
> > > > > 
> > > > 
> > > > If these events occur (mixed content or malicious subresources), the
> > relevant
> > > > notifications will still be fired from the renderer and the SSLStatus
will
> > be
> > > > updated. They'll fire after the did commit provisional load IPC which is
> > what
> > > > calls this method.
> > > 
> > > ok thinking some more about this, one issue which I think you bring up is:
> > > -user visits https://foo.com/mixedcontent
> > > -the SSLStatus is later updated because of mixed content from that page
> > > -page uses window.location.replace to go to https://foo.com/goodpage
> > > -we don't create a new SSLStatus and use the old bad one
> > > 
> > > so it seems we would end up in a situation where a bad icon is used when
it
> > > should be good. the other way around shouldn't be possible (because of my
> > > earlier reply). i'll look some more into differentiating this case.
> > 
> > (not lgtm)
> > 
> > you're right, I hadn't thought of it the other way around and that does
still
> > seem like a problem.
> 
> ok I just tested this manually and it actually works fine. i visited
> https://mixed.badssl.com/ which shows the lock icon displaying mixed icon.
> 
> i then did window.location.replace("https://mixed.badssl.com/image.jpg").
while
> the codepath that I added didn't reset the SSLStatus, it turns out that in the
> same callstack (below) WebContentsImpl::DidNavigateMainFramePostCommit is
called
> and this codepath:
> 
>   if (!details.is_in_page) {
>     // Once the main frame is navigated, we're no longer considered to have
>     // displayed insecure content.
>     displayed_insecure_content_ = false;
>     displayed_content_with_cert_errors_ = false;
>     SSLManager::NotifySSLInternalStateChanged(
>         GetController().GetBrowserContext());
>   }
> 
> resets the SSLState for real navigations.
> 
> 
> 
>  	content.dll!content::SSLPolicy::UpdateEntry(content::NavigationEntryImpl *
> entry, content::WebContents * web_contents) Line 204	C++
>  	content.dll!content::SSLManager::UpdateEntry(content::NavigationEntryImpl *
> entry) Line 194	C++
> 
>
	content.dll!content::SSLManager::NotifySSLInternalStateChanged(content::BrowserContext
> * context) Line 121	C++
>
>	content.dll!content::WebContentsImpl::DidNavigateMainFramePostCommit(content::RenderFrameHostImpl
> * render_frame_host, const content::LoadCommittedDetails & details, const
> FrameHostMsg_DidCommitProvisionalLoad_Params & params) Line 3384	C++
>  	content.dll!content::NavigatorImpl::DidNavigate(content::RenderFrameHostImpl
*
> render_frame_host, const FrameHostMsg_DidCommitProvisionalLoad_Params &
params)
> Line 651	C++
>  	content.dll!content::RenderFrameHostImpl::OnDidCommitProvisionalLoad(const
> IPC::Message & msg) Line 1209	C++

thinking about this some more, is_in_page can be keyed off to figure out if we
shoudl replace SSLStatus or not, and then the hacky origin check isn't needed.
i'll update the cl

[jam, 2016-09-06 21:34:57]

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

[commit-bot: I haz the power, 2016-09-06 21:35:30]

Dry run: 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-09-06 22:49:17]

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

[commit-bot: I haz the power, 2016-09-06 22:49:17]

Dry run: This issue passed the CQ dry run.

[Charlie Reis, 2016-09-06 23:43:13]

creis@chromium.org changed reviewers:
+ creis@chromium.org

[Charlie Reis, 2016-09-06 23:43:14]

I'm not opposed to the is_in_page check, but I don't understand how it relates
to the failing tests yet.

https://codereview.chromium.org/2305093002/diff/80001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/2305093002/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:1205: // a request and
so we need to update the SSLStatus.
This comment seems stale-- it's referring to the old origin check.  Can you
update it to explain why the SSLStatus shouldn't be updated for in page
navigations?  (I'm not sure why that failed in the MixedContentWithBrokenSHA1
and MixedContent tests, since those don't appear to have in-page navigations.)

[felt, 2016-09-07 00:03:29]

https://codereview.chromium.org/2305093002/diff/80001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_browser_tests.cc (right):

https://codereview.chromium.org/2305093002/diff/80001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_browser_tests.cc:3203: IN_PROC_BROWSER_TEST_F(SSLUITest,
ClientRedirectSSLState) {
Can you add more variants of this test to trigger the corner cases we were
discussing?

* An in-page navigation
* Valid HTTPS -> HTTPS with mixed content
* HTTPS with mixed content -> valid HTTPS

[jam, 2016-09-07 00:03:54]

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

[jam, 2016-09-07 00:03:56]

https://codereview.chromium.org/2305093002/diff/80001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/2305093002/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:1205: // a request and
so we need to update the SSLStatus.
On 2016/09/06 23:43:14, Charlie Reis wrote:
> This comment seems stale-- it's referring to the old origin check.  Can you
> update it to explain why the SSLStatus shouldn't be updated for in page
> navigations?

Done

>  (I'm not sure why that failed in the MixedContentWithBrokenSHA1
> and MixedContent tests, since those don't appear to have in-page navigations.)


As an example, they loaded
chrome/test/data/ssl/page_runs_insecure_content_in_iframe.html which includes
iframe_with_insecure_content.html and that runs randomize_hash.js:

document.body.innerHTML = "I'm content from an HTTP script." 
location.hash = Math.random();



The location.hash call ended up going to line 1200 above. There was no actual
navigation (i.e. is_in_page), and so blindly resetting  entry->GetSSL() =
handle->ssl_status(); meant that we lost the ssl state (since the
navigationhandle had an empty SSLStatus)

[commit-bot: I haz the power, 2016-09-07 00:04:17]

Dry run: 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-09-07 01:32:15]

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

[commit-bot: I haz the power, 2016-09-07 01:32:16]

Dry run: This issue passed the CQ dry run.

[jam, 2016-09-07 01:49:56]

Patchset #6 (id:120001) has been deleted

[jam, 2016-09-07 01:50:49]

https://codereview.chromium.org/2305093002/diff/80001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_browser_tests.cc (right):

https://codereview.chromium.org/2305093002/diff/80001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_browser_tests.cc:3203: IN_PROC_BROWSER_TEST_F(SSLUITest,
ClientRedirectSSLState) {
On 2016/09/07 00:03:29, felt wrote:
> Can you add more variants of this test to trigger the corner cases we were
> discussing?
> 
> * An in-page navigation

hmm this should have been covered by an existing test
(TestRunsInsecuredContentRandomizeHash) but that seems to not do anything now.
I've commented on https://codereview.chromium.org/1931063004/ and I'll add a new
test in the meantime.

this was also covered by the existing tests I broke in patchset 1, but I guess
better to have tests that check for this directly instead of as a side-effect.

Done


> * Valid HTTPS -> HTTPS with mixed content
> * HTTPS with mixed content -> valid HTTPS

Done

[jam, 2016-09-07 01:51:29]

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

[commit-bot: I haz the power, 2016-09-07 01:51:54]

Dry run: 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-09-07 02:05:34]

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

[commit-bot: I haz the power, 2016-09-07 02:05:34]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jam, 2016-09-07 02:31:46]

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

[jam, 2016-09-07 02:32:00]

Patchset #6 (id:140001) has been deleted

[commit-bot: I haz the power, 2016-09-07 02:32:01]

Dry run: 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-09-07 04:27:58]

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

[commit-bot: I haz the power, 2016-09-07 04:27:59]

Dry run: This issue passed the CQ dry run.

[felt, 2016-09-07 04:58:53]

lgtm

[felt, 2016-09-07 04:59:14]

The CQ bit was checked by felt@chromium.org

[commit-bot: I haz the power, 2016-09-07 04:59:33]

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-09-07 05:13:28]

Committed patchset #6 (id:160001)

[commit-bot: I haz the power, 2016-09-07 05:15:53]

Description was changed from

==========
Fix incorrect SSL state being shown for client redirects.

BUG=643173
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Fix incorrect SSL state being shown for client redirects.

BUG=643173
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/0576b13b74ef273fe311a95cbbb9e1a3bc8045c5
Cr-Commit-Position: refs/heads/master@{#416849}
==========

[commit-bot: I haz the power, 2016-09-07 05:15:54]

Patchset 6 (id:??) landed as
https://crrev.com/0576b13b74ef273fe311a95cbbb9e1a3bc8045c5
Cr-Commit-Position: refs/heads/master@{#416849}