Issue 599951  has been merged into this issue.

Hello, could one of you help confirm if this is a bug or not (and assign if so).

Many thanks.

This doesn't seem to work for me.  The testcase tries and fails to open a popup window, clicking the link in the popup blocker opens a tab to Google, pressing the button in the testcase does nothing.

IIUC, this is basically complaining that the address bar state does not update when showing an interstitial?  If so that doesn't seem like an exploitable security issue.

Well, you can repro this bug easily with this testcase. Press on the button and wait 3 seconds, the address bar will update to google.com.

Deleted: testcase.html 273 bytes

testcase.html 273 bytes View Download

Deleted: result.png 84.3 KB

	result.png 84.3 KB View Download

Doesn't seem to repro for me on 51.0.2693.2, Win 10 x64.  My address bar shows ianfette.org.

I can half-reproduce. (Note: you need to enable popups from the testcase window.) The interstitial page will show "google.com" for a second but then go back to showing the correct URL for a second. I'm guessing there is some issue with how the virtual URL is updated when x.location changes.

I can repro using the steps in comment 6 in 51.0.2695.1 and 51.0.2699.0 (no need to enable popups).  Agreed that it doesn't repro in 51.0.2693.2.  Probably a recent regression.

This looks like a bug where we're discarding the pending entry without discarding the interstitial.  It's ugly and we should fix it, but I don't see much of a security implication.  Seems like you'd just be able to make the user think a victim site was untrustworthy, since the attacker doesn't have control over the content of the interstitial.

I'm familiar with this logic, but I don't have a lot of time today.  Nasko, would you be able to take a look?

Yes, I'll take a look.

Nasko confirmed this is due to https://codereview.chromium.org/1683593002.  I'm reverting until we can track down how it's happening.

Reverted in r385235.  We should confirm that this fixes the spoof in tomorrow's canary.

Unfortunately, the revert didn't help. It bisected very well to that revision and local revert didn't repro anymore, but today's canary (51.0.2701.0) still reproduces the issue. It is a race condition and the CL we reverted probably increased the probability of it happening, but wasn't the root cause.

Is this a security vulnerability? What's the attack scenario? Can wrong.host.badssl.com script google.com, for example?

Dumping my current understanding of what is happening with the repro. The window.open step is basically to allow JS to control the navigations. Being renderer initiated, they are not going to swap processes by default.

The first navigation to google.com is establishing a session history entry for googlg.com and a successful commit. The second navigation is to a page that causes an interstitial page to show up. What I'm observing is that the navigation starts, but we don't reach RenderFrameImpl::didFailProvisionalLoad. The interstitial page is correctly loaded in a different process and displayed.

The timeout value for the second navigation should be big enough for all the steps above to complete. The second navigation is what tickles enough state in the renderer to get to RenderFrameImpl::didFailProvisionalLoad for the first navigation, then hit RenderFrameImpl::didStartProvisionalLoad for the second navigation. It is then again stuck and makes no forward progress. 

At this stage we update the omnibox and it shows an incorrect URL. I haven't traced why that is the case, but my hypothesis is that these navigations being renderer-initiated (therefore not as trusted), we end up falling back and displaying the last committed URL (google.com in the repro).

Here is a short log of some logging I've put in to help diagnose (tests.netsekure.org being google.com and wrong.host.badssl.com causing the interstitial):

// New window navigated to localhost, which serves the repro page
[67396:67396:0406/142932:ERROR:render_frame_host_impl.cc(217)] RFH[0x61800012cc80]::RFH:  routing_id:4 instance:2:http://localhost/
[67432:67432:0406/142932:ERROR:render_frame_impl.cc(1033)] RF[0x61900005af80]::RF:  routing_id:4
[67432:67432:0406/142932:ERROR:render_frame_impl.cc(2998)] RF[0x61900005af80]::didStartProvisionalLoad:  url:http://tests.netsekure.org/
[67396:67396:0406/142932:ERROR:render_frame_host_impl.cc(908)] RFH[0x61800012cc80]::DidStartProvisionalLoad:  url:http://tests.netsekure.org/
[67432:67432:0406/142932:ERROR:render_frame_impl.cc(3127)] RF[0x61900005af80]::didCommitProvisionalLoad:  url:http://tests.netsekure.org/
[67396:67396:0406/142932:ERROR:render_frame_host_impl.cc(976)] RFH[0x61800012cc80]::DidCommitProvisionalLoad:  url:http://tests.netsekure.org/
[67432:67432:0406/142932:ERROR:render_frame_impl.cc(3385)] RF[0x61900005af80]::didFinishDocumentLoad:  url:http://tests.netsekure.org/

// First navigation to badssl starts here.
[67432:67432:0406/142933:ERROR:render_frame_impl.cc(2998)] RF[0x61900005af80]::didStartProvisionalLoad:  url:https://wrong.host.badssl.com/
[67396:67396:0406/142933:ERROR:render_frame_host_impl.cc(908)] RFH[0x61800012cc80]::DidStartProvisionalLoad:  url:https://wrong.host.badssl.com/
[67396:67396:0406/142933:ERROR:render_frame_host_impl.cc(217)] RFH[0x61800012ac80]::RFH:  routing_id:2 instance:3:

// Not sure what is causing this LaunchProcess error or whether it matters.
LaunchProcess: failed to execvp:

// This is loading the interstitial page into the new renderer.
[67396:67396:0406/142933:ERROR:render_frame_host_impl.cc(2037)] RFH[0x61800012ac80]::Navigate:  url:data:text/html;charset=utf-8,%3C!doctype%20html%3E%0A%3Chtml%20i18n-values=%22dir%3Atextdirection;lang%3Alanguage%22%3E%0A%3Chead%3E%0A%20%20%3Cmeta%20charset=%22utf-8%22%3E%0A%20%20%3Cmeta%20name=%22viewport%22%0A%20%20%20%20%20%20content=%22initial
[67457:67457:0406/142934:ERROR:render_frame_impl.cc(1033)] RF[0x61900004f180]::RF:  routing_id:2
[67457:67457:0406/142934:ERROR:render_frame_impl.cc(2998)] RF[0x61900004f180]::didStartProvisionalLoad:  url:data:text/html;charset=utf-8,%3C!doctype%20html%3E%0A%3Chtml%20i18n-values=%22dir%3Atextdirection;lang%3Alanguage%22%3E%0A%3Chead%3E%0A%20%20%3Cmeta%20charset=%22utf-8%22%3E%0A%20%20%3Cmeta%20name=%22viewport%22%0A%20%20%20%20%20%20content=%22initial
[67396:67396:0406/142934:ERROR:render_frame_host_impl.cc(908)] RFH[0x61800012ac80]::DidStartProvisionalLoad:  url:data:text/html;charset=utf-8,%3C!doctype%20html%3E%0A%3Chtml%20i18n-values=%22dir%3Atextdirection;lang%3Alanguage%22%3E%0A%3Chead%3E%0A%20%20%3Cmeta%20charset=%22utf-8%22%3E%0A%20%20%3Cmeta%20name=%22viewport%22%0A%20%20%20%20%20%20content=%22initial
[67457:67457:0406/142934:ERROR:render_frame_impl.cc(3127)] RF[0x61900004f180]::didCommitProvisionalLoad:  url:data:text/html;charset=utf-8,%3C!doctype%20html%3E%0A%3Chtml%20i18n-values=%22dir%3Atextdirection;lang%3Alanguage%22%3E%0A%3Chead%3E%0A%20%20%3Cmeta%20charset=%22utf-8%22%3E%0A%20%20%3Cmeta%20name=%22viewport%22%0A%20%20%20%20%20%20content=%22initial
[67396:67396:0406/142934:ERROR:render_frame_host_impl.cc(976)] RFH[0x61800012ac80]::DidCommitProvisionalLoad:  url:data:text/html;charset=utf-8,%3C!doctype%20html%3E%0A%3Chtml%20i18n-values=%22dir%3Atextdirection;lang%3Alanguage%22%3E%0A%3Chead%3E%0A%20%20%3Cmeta%20charset=%22utf-8%22%3E%0A%20%20%3Cmeta%20name=%22viewport%22%0A%20%20%20%20%20%20content=%22initial
[67457:67457:0406/142934:ERROR:render_frame_impl.cc(3385)] RF[0x61900004f180]::didFinishDocumentLoad:  url:data:text/html;charset=utf-8,%3C!doctype%20html%3E%0A%3Chtml%20i18n-values=%22dir%3Atextdirection;lang%3Alanguage%22%3E%0A%3Chead%3E%0A%20%20%3Cmeta%20charset=%22utf-8%22%3E%0A%20%20%3Cmeta%20name=%22viewport%22%0A%20%20%20%20%20%20content=%22initial

// Some time passes, the second navigation is kicking in here, causing the fail for the previous one and starting the second one.
[67432:67432:0406/142937:ERROR:render_frame_impl.cc(3059)] RF[0x61900005af80]::didFailProvisionalLoad:  url:https://wrong.host.badssl.com/
[67432:67432:0406/142937:ERROR:render_frame_impl.cc(2998)] RF[0x61900005af80]::didStartProvisionalLoad:  url:https://wrong.host.badssl.com/
[67396:67396:0406/142937:ERROR:render_frame_host_impl.cc(920)] RFH[0x61800012cc80]::DidFailProvisionalLoadWithError:  error:-3
[67396:67396:0406/142937:ERROR:web_contents_impl.cc(3726)] WC[0x61c0000af080]::LoadingStateChanged: ignoring due to interstitial
[67396:67396:0406/142937:ERROR:web_contents_impl.cc(3726)] WC[0x61c0000af080]::LoadingStateChanged: ignoring due to interstitial

// This IPC handler is causing the URL in the omnibox to be updated.
[67396:67396:0406/142937:ERROR:render_frame_host_impl.cc(908)] RFH[0x61800012cc80]::DidStartProvisionalLoad:  url:https://wrong.host.badssl.com/


In some cases, I've even hit this check (though rarely):
[50048:50048:0406/141409:FATAL:ssl_error_handler.cc(159)] Check failed: !FromWebContents(web_contents).

So my hypothesis is that we are having a bug somewhere in routing SSL or request/response information or it is being deferred when it is not expected.
I will continue digging further and post updates as I find out more.

A bit more information. I was able to repro a variation of this on Chrome Mac stable (49.0.2623.110). It still shows the committed page URL in the omnibox, but the spinner is spinning and the reload button is showing X. Clicking on it to stop the load leaves the URL in the omnibox and the interstitial page still showing for the not-committed site. A quick two iteration bisect shows that it is an old issue, as I was able to repro this behavior in build based on r318289.

The current ToT behavior is worse, as there is no spinner and the reload button does not indicate loading state. It is possible this regressed in https://codereview.chromium.org/1690653003/, but haven't yet verified.

Bisected to range containing a different reland of the same CL - https://codereview.chromium.org/1693353002.

https://codereview.chromium.org/1693353002 has introduced slight changes in loading behavior when showing an interstitial.
Before: we would register whether WebContents was loading or not and fire DidStopLoading when the interstitial was shown. When the interstitial goes away or we navigate away via a browser-initiated navigation, if WebContents was previously loading we fire DidStartLoading. If a DidStartLoading comes from a renderer when the interstitial is showing, we may fire a DidStartLoading.
After: when the interstitial is shown we fire DidStopLoading. When we go away, we check if any frame in the tree is loading. If yes, we fire DidStartLoading again. If any DidStartLoading comes in the meantime we don't fire DidStartLoading.

So basically the patch ensures that you will never fire a DidStartLoading while the interstitial page is showing and the user has not attempted do do a browser-initiated navigation away from the interstitial. That's what I understood the behavior with an interstitial should be when reading the comments in InterstitialPageImpl.

Is this a security bug? if so what is the severity of this bug?

After some discussion, I am removing security flags from this bug. URL spoofing can be a serious issue but in this case a hypothetical attacker wouldn't have any control over the content of the page being displayed underneath the URL, and we can't think of any way this could plausibly be used to harm a user.

I think I have full understanding of what is going on at this point. There are couple of "right things" in play that end up with incorrect behavior when interacting.

1. Back in https://codereview.chromium.org/22622003, we've made change such that a pending navigation will update the URL in the omnibox, to improve user experience and show correctly where we are navigating to. The logic in NavController::GetVisibleURL (used by the omnibox to display URLs) takes into account how "trustworthy" the navigation is. When the navigation is renderer-initiated and is not the initial one of window.open(), we consider it less trusted and we don't use its URL to display, rather we keep the last committed URL in the omnibox.

2. When displaying an interstitial page, the old page is still around and can execute. However, since we would ideally have it completely blocked and we can't, we block all of its outgoing network requests at the network stack.

Even with the behavior in 2, the renderer sends a DidStartProvisionalLoad IPC to the browser. Since there are no browser-initiated navigations, we create a pending NavEntry and overwrite any existing one. We also notify the omnibox that there is a change of navigation state, so it should update itself. Due to 1, we return the last committed URL (google.com). Since we've blocked all network requests for the last committed page, it will never complete, so we are stuck with the last committed URL in the omnibox, while displaying an interstitial page.

The CL I pointed out earlier (#c20) only changes the visible behavior of the repro. Before that change, the spinner is spinning and the reload button is (X), whereas after the change it is not showing any loading behavior.

I'll think if we can mitigate this short term with an easy fix. Otherwise I will let it be fixed by the long standing plan to refactor interstitial pages to be committed session history items (issue 392354), which will completely remove this type of bug.

Attaching the file I've used to repro this. It has both the original repro and the separate steps broken apart to allow me to have more controlled repro.

Deleted: badssl-testcase.html 789 bytes

badssl-testcase.html 789 bytes View Download

Good find, Nasko!  It sounds like the first navigation to the bad URL creates a transient entry (when the interstitial page is created).  Then the second navigation to the bad URL causes NavigatorImpl::DidStartMainFrameNavigation to call SetPendingEntry.  In addition to clearing the previous pending entry, SetPendingEntry *also* clears the interstitial page's transient entry.  That changes the result of GetVisibleEntry from the transient entry to the last committed entry, causing this bug.

It seems to me that we don't need to call SetPendingEntry from DidStartMainFrameNavigation if there's an interstitial showing.  We only create a pending entry there for renderer-initiated navigations to handle the cases that it's safe to show that URL, and here we know that the transient entry will be displayed instead *and* the navigation likely won't succeed (since we'll block it in the network stack).  It's possible the interstitial will be canceled after that, but then we just fall back to the last committed, which isn't bad at that point.

(We could consider going further and making sure any call to DiscardTransientEntry verifies that no interstitial page is showing, but that's more of a second line of defense against bugs like this.  Perhaps we can land that separately.)

I have a solution in mind that I've discussed with creis@ and prototyped it. I need to write a test for this and will send out a CL for fixing it.

The CL with a fix for this is up at https://codereview.chromium.org/1881023003/. It should land in the next few days.

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

commit b2f82b84617fd2c1645a4a185f628f61f7741627
Author: nasko <nasko@chromium.org>
Date: Fri Apr 15 07:09:07 2016

Do not create pending NavigationEntry when a transient one is present.

When an interestitial page is displayed, network requests from the existing
RenderFrameHost are blocked. However, this still allows the renderer to
send a DidStartProvisionalLoad IPC. This CL adds check whether when
processing the IPC a transient entry is present and skips creating a
pending entry. Otherwise the pending entry creation deletes the transient
entry and leades to URL spoof.

BUG= 600046 
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Review URL: https://codereview.chromium.org/1881023003

Cr-Commit-Position: refs/heads/master@{#387545}

[modify] https://crrev.com/b2f82b84617fd2c1645a4a185f628f61f7741627/chrome/browser/OWNERS
[add] https://crrev.com/b2f82b84617fd2c1645a4a185f628f61f7741627/chrome/browser/chrome_navigation_browsertest.cc
[modify] https://crrev.com/b2f82b84617fd2c1645a4a185f628f61f7741627/chrome/chrome_tests.gypi
[add] https://crrev.com/b2f82b84617fd2c1645a4a185f628f61f7741627/chrome/test/data/window_open_and_navigate.html
[modify] https://crrev.com/b2f82b84617fd2c1645a4a185f628f61f7741627/content/browser/frame_host/navigator_impl.cc

Verified on 52.0.2710.0 and seems like fixed. Thanks nasko@!

Thanks for verifying chromium.khalil@! Marking as fixed.

Do we want this merged in beta or stable?

Your change meets the bar and is auto-approved for M51 (branch: 2704)

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

commit 5cb7488cf703179cea5af8ce5dff08fb4b9e2be4
Author: Nasko Oskov <nasko@chromium.org>
Date: Mon Apr 18 17:02:13 2016

Do not create pending NavigationEntry when a transient one is present.

When an interestitial page is displayed, network requests from the existing
RenderFrameHost are blocked. However, this still allows the renderer to
send a DidStartProvisionalLoad IPC. This CL adds check whether when
processing the IPC a transient entry is present and skips creating a
pending entry. Otherwise the pending entry creation deletes the transient
entry and leades to URL spoof.

BUG= 600046 
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Review URL: https://codereview.chromium.org/1881023003

Cr-Commit-Position: refs/heads/master@{#387545}
(cherry picked from commit b2f82b84617fd2c1645a4a185f628f61f7741627)

Review URL: https://codereview.chromium.org/1902433002 .

Cr-Commit-Position: refs/branch-heads/2704@{#92}
Cr-Branched-From: 6e53600def8f60d8c632fadc70d7c1939ccea347-refs/heads/master@{#386251}

[modify] https://crrev.com/5cb7488cf703179cea5af8ce5dff08fb4b9e2be4/chrome/browser/OWNERS
[add] https://crrev.com/5cb7488cf703179cea5af8ce5dff08fb4b9e2be4/chrome/browser/chrome_navigation_browsertest.cc
[modify] https://crrev.com/5cb7488cf703179cea5af8ce5dff08fb4b9e2be4/chrome/chrome_tests.gypi
[add] https://crrev.com/5cb7488cf703179cea5af8ce5dff08fb4b9e2be4/chrome/test/data/window_open_and_navigate.html
[modify] https://crrev.com/5cb7488cf703179cea5af8ce5dff08fb4b9e2be4/content/browser/frame_host/navigator_impl.cc

Security>UX component is deprecated in favor of the Team-Security-UX label