Project: chromium Issues People Development process History Sign in
New issue
Advanced search Search tips
Issue 234809 URL spoof or renderer kill when committing prerendered/instant page with a pending entry
Starred by 5 users Project Member Reported by creis@chromium.org, Apr 23 2013 Back to list
Status: Fixed
Owner:
Closed: May 2013
Cc:
Components:
NextAction: ----
OS: All
Pri: 1
Type: Bug-Security

Blocking:
issue 9682


Sign in to add a comment
Version: 28.0.1487.0
OS: All

http://go/crash/reportdetail?reportid=9b12c0418469b8f7

This is a renderer kill that occurs when the NavigationController sees a commit for an existing entry that it can't find.  We report diagnostic info in the OnTempCrashWithData URL, and all the crashes seem to be similar to:

[url]#page1#max1#frame1#ids1_2x,2_2

I haven't found repro steps yet, but it looks like a new navigation is being misclassified because the max page ID is 1 instead of -1.

It's very likely this is fallout from r195553, which fixed issue 9682.  I'm actively trying to fix this to avoid having to revert that CL.
 
Comment 1 by creis@chromium.org, Apr 24 2013
Blocking: chromium:9682
Comment 2 by creis@chromium.org, Apr 24 2013
The offending CL was reverted in r195970, but I'm still trying to track down the cause so that I can re-land it.

This looks like a new navigation in a new process because the page ID is 1, the frame ID is 1, and the array of existing entries contain no entries from the same SiteInstance.  That's very consistent with navigating to a site in a new process.

If that's true, the bug is that max_page_id is 1 instead of -1.  As a result, NavigationControllerImpl::ClassifyNavigation thinks it's an existing entry and not a new entry.  We don't find the existing entry, so we kill the renderer.

I don't see where my CL would have affected max_page_id, but I'm investigating.  There's a potential lead in issue 234720.
Comment 3 by creis@chromium.org, Apr 24 2013
Yes, I'm able to (inconsistently) repro the crash using the steps in issue 234720.  I'll post an updated set of steps here if I can get them to be more consistent.

There's a good chance that prerendering is involved, which could explain the max page ID issue.  It's definitely happening on a new cross-process navigation.
Comment 4 by creis@chromium.org, Apr 25 2013
Cc: nasko@chromium.org sreeram@chromium.org cbentzel@chromium.org
Labels: -Type-Bug Type-Bug-Security Cr-Internals-Preload Cr-UI-Browser-Instant
Summary: URL spoof or renderer kill when committing prerendered/instant page with a pending entry (was: Renderer kill via RenderThreadImpl::OnTempCrashWithData)
Got it.  It's a bug with prerendering when a pending_entry_ is present in the prerendered NavigationController.  That was much more common after my CL to fix 9682, but I'm almost certain it's a real issue even on stable.

Conceptual repro steps:
1) Start prerendering a "foo.html" that does a client redirect to a slow hosted app URL.  (The hosted app navigation will require a process swap and thus create a pending_entry_ within the prerendering NavigationController.)
2) Swap in the prerendered page after foo.html commits but before the redirect to the app completes.

At this point, if you cancel the pending entry (or if it turns out to be a download or 204 and thus never commits), we will be left showing the prerendered foo.html, but there won't be any entry in the NavigationController corresponding to it!  Instead, the address bar will be left showing the URL you started on (before step 1).

If instead you let the pending entry commit, it's possible for the renderer to be killed.  This will happen if the pending entry is in the same Siteinstance as foo.html, and it happens because we try to use the max_page_id from the last committed entry but we delete the last committed entry in PruneAllButActiveInternal.  (This is very common after my fix for 9682, because we create pending entries for all renderer-initiated navigations.  It's harder to repro without that CL, but it's still possible: all renderer-initiated navigations in WebUI renderers create pending entries, even when they don't swap processes.)

I'm having trouble putting together a repro case for the spoof, though, because the prerendering gets canceled whenever I do a redirect in the prerendered page.  That's clearly not an effective defense, because Gmail is able to stay prerendered after doing client redirects (as in the repro steps from issue 234720).  I haven't yet been able to figure out what the difference is, and how to prevent the prerender in my repro case from being canceled.

Comment 5 by creis@chromium.org, Apr 25 2013
The easy fix for the renderer kill is to just reset max_page_id if we end up with nothing in entries_ during CopyStateFromAndPrune.

That doesn't solve the URL spoof, though.  Fundamentally, you can't prune everything but the pending_entry_ (while there's a visible last committed entry), because the pending entry might not commit.  It could be a download or 204.  You have to leave the last committed entry in place, because that page will remain visible.

I'm a bit concerned about the impact such a change will have on prerendering and instant code, since I'm not sure what assumptions that will break.  I'm also not sure what would happen if there were no committed entry at all (only a pending entry), and the pending entry didn't commit.
> the impact such a change will have on prerendering and instant code

For Instant, it's always fine to keep the last committed entry in PruneAllButActive(). I haven't thought through every possible corner case, but at the worst, this means there'll be an unexpected Google homepage or search results page in the nav stack. No big deal (the Google homepage will be more unexpected than the search results page, and even so, it'll probably only be a mild irritation).

> what would happen if there were no committed entry at all (only a pending entry)

I don't think this is possible with Instant at all. Instant's WebContents is used only after the initial page load has completed (and we have determined that it "supports instant"), which is well past "commit", so we should have at least one committed entry.
Comment 7 by creis@chromium.org, Apr 25 2013
Labels: -Cr-UI-Browser-Instant
Great.  Sounds like the impact is limited to prerendering, then.
Comment 8 by creis@chromium.org, Apr 26 2013
Cc: tburkard@chromium.org
Ok, I've looked a little closer, and here's some repro steps for the prerendering issue:

1) Start prerendering a slow URL.
2) Swap it in by hitting enter in the omnibox, before the URL commits.
3) Stop the slow load (or have it otherwise time out or fail).

We are now showing the blank initial page of the prerendered WebContents, with the title of the previous page.  Technically the slow URL is in the address bar, but if you hit escape to clear the failed pending entry, the address bar shows the previous URL above the blank page.  (Also, if the previous page was the NTP, you can't go back or forward.)

I still can't figure out how to get prerendering to work with my test page that does a redirect (though it works for Gmail), but this would make it an exploitable URL spoof.  The prerendered redirect page would show fake content, but its navigation entry wouldn't exist anymore.

I think this has two implications:
1) We can't use a prerendered WebContents that only has a pending entry and no last committed entry.
2) If the prerendered WebContents has a last committed entry and a pending entry, we have to keep them both.

The invariant is that PruneAllButActiveInternal has to leave a last committed entry in place, because this is the one that's visible.

Chris or Timo, do you have a sense for how much impact this will have on prerendering?  I'll try to start putting together a CL.
Comment 9 by creis@chromium.org, Apr 26 2013
Cc: brettw@chromium.org
We may have to go further and cancel any pending or transient entries as well.  I just don't see how things like a pending entry at an existing index (e.g., back navigation) makes sense if we keep the last committed entry.

These changes will make the callers meet more preconditions, but they should vastly simplify the crazy logic in the prune functions, hopefully clearing up several bugs.
Comment 10 by creis@chromium.org, Apr 26 2013
Cc: jochen@chromium.org
+jochen for FYI.
I'm a bit worried about the requirement that there is a committed entry - if the prerendered page has a 3 second server response time and the user navigates to it in 1.5 seconds, it seems like a good idea to still preserve the same navigation rather than cancel it and navigate again because the prerender can't be swapped in.
Comment 12 by creis@chromium.org, Apr 26 2013
I disagree-- I don't think that's safe.  If we swap it in, we end up showing a blank page with the wrong navigation entry corresponding to it.  That slow navigation might fail or turn out to be a download, in which case the user is left with a blank tab and a confused NavigationController.  See my comment 8 about that case.
Could we special case the initial blank prerendered page and replace it after the pending prerendered navigation commits, or erase it after it fails and go back to the entry-before-initial-blank page? We'd probably also need to clear the title.

I do think that this is pretty convoluted and perhaps not worth it, but just wondering if it is a possibility.
Comment 14 by creis@chromium.org, Apr 26 2013
What if it wasn't blank, though?  It's still a URL spoof if the visible prerendered page has no entry because there was a slow pending entry.

I think it would also be a strange user experience to have even a blank page while the slow load is in progress and suddenly reload the previous page if the slow load fails.
Comment 15 by creis@chromium.org, Apr 26 2013
Jochen brought up an important concern with my suggested approach in comments 8 and 9.

Even if we cancel the pending entry, the renderer might have just committed it, with the commit message in-flight to the browser.

What happens if a slow back navigation commits and all we've remembered is the last committed entry?  We can't ignore it.  Would it become a new entry at the end of the history?  Not sure whether there's really a sensible thing to do.
Comment 16 by palmer@google.com, May 7 2013
Labels: Security_Impact-None
Security_Impact-None, since it's HEAD churn, right?
Yeah.  Technically the problem exists in all builds of Chrome, but I haven't found a way to repro it without the fix for 9682 in place.  If I find any repro steps, I'll post them and update the security impact.
creis, any thoughts on Security_Severity? -Medium?
Labels: Security_Severity-Medium
Medium sounds right.  A URL spoof is usually considered High, but this would currently require user interaction to create a pending entry that cancels while a prerender is in progress, unless we're able to find other repro steps.
Labels: Security-Code28
Please do read Mark's email titled "Calling a Code 28 for Security Bugs" on chrome-team mailing list.
very friendly ping :).
Comment 22 by creis@chromium.org, May 13 2013
I've started a CL for this, but it's a little disruptive, so it's going to take some work to update tests.

I think we've settled on an approach that should work.  In order to safely call PruneAllButActive, we need the following invariants:
1) There must be a last committed entry.  Otherwise merging in other entries via CopyStateFromAndPrune will cause confusion in NavigationController, and show a blank page under a newly merged entry.
2) There must not be a pending entry to an existing entry.  We have to keep the last committed entry in case the pending navigation fails, and the existing pending entry could get pruned.  (We can't keep both in any sane order.)
3) There can be a pending entry for a new navigation.  We can't prevent this anyway, because the renderer can commit a new navigation at any time.
4) There cannot be a transient entry.  There's no reason to support swapping in a prerendered or instant page when it's showing an interstitial, and there's too many ways for it to go wrong.

I have a CL that does this, renaming PruneAllButActive to PruneAllButVisible.  I've updated most of the affected tests, but still have some work to do.
Can you please post the cl link here.
Comment 24 by creis@chromium.org, May 22 2013
Sure: https://codereview.chromium.org/15041004/

Still trying to work through the implications for tests, and one issue for Instant's local page.
Comment 25 by creis@chromium.org, May 22 2013
Labels: -M-28 M-29
Also, I don't think this will be something we can merge to M28.  I'll mark it as M29, but let me know if that's an issue.
Project Member Comment 26 by bugdroid1@chromium.org, May 31 2013
------------------------------------------------------------------------
r203420 | mmenke@chromium.org | 2013-05-31T16:45:07.407273Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/prerender/prerender_contents.cc?r1=203420&r2=203419&pathrev=203420
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/test/data/webui/net_internals/prerender_view.js?r1=203420&r2=203419&pathrev=203420
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/resources/net_internals/prerender_view.html?r1=203420&r2=203419&pathrev=203420

about:net-internals#prerender:  Add information
about whether a prerender has finished loading or not.

Also wait until a prerender has finished loading in the
browser test before swapping in the prerender.  This is
needed before https://codereview.chromium.org/15041004/
can be landed (Which prevents a prerender from being
swapped in before commit, to fix a bug).

BUG= 234809 

Review URL: https://chromiumcodereview.appspot.com/16194008
------------------------------------------------------------------------
Project Member Comment 27 by bugdroid1@chromium.org, May 31 2013
------------------------------------------------------------------------
r203492 | creis@chromium.org | 2013-05-31T22:31:16.442972Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/prerender/prerender_manager.cc?r1=203492&r2=203491&pathrev=203492
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/web_contents/navigation_controller_impl_unittest.cc?r1=203492&r2=203491&pathrev=203492
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/android/content_view_core_impl.cc?r1=203492&r2=203491&pathrev=203492
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/web_contents/navigation_controller_impl.cc?r1=203492&r2=203491&pathrev=203492
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/web_contents/navigation_controller_impl.h?r1=203492&r2=203491&pathrev=203492
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/prerender/prerender_browsertest.cc?r1=203492&r2=203491&pathrev=203492
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/public/browser/navigation_controller.h?r1=203492&r2=203491&pathrev=203492
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ui/browser_commands.cc?r1=203492&r2=203491&pathrev=203492
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/web_contents/web_contents_impl_unittest.cc?r1=203492&r2=203491&pathrev=203492
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ui/browser_instant_controller.cc?r1=203492&r2=203491&pathrev=203492
   D http://src.chromium.org/viewvc/chrome/trunk/src/chrome/test/data/prerender/prerender_visibility_quick.html?r1=203492&r2=203491&pathrev=203492
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ui/search/instant_controller.cc?r1=203492&r2=203491&pathrev=203492

Replace PruneAllButActive with PruneAllButVisible.

Before we would forget the entry for the visible page, so we now require there
to be a last committed entry, no transient entry, and optionally a new pending
entry (not an existing pending entry).

BUG= 234809 
TEST=Prerendering should not swap in without a last committed entry.

Review URL: https://chromiumcodereview.appspot.com/15041004
------------------------------------------------------------------------
Labels: Merge-Approved Restrict-View-SecurityNotify
Status: Fixed
Labels: -Security_Impact-None -Merge-Approved Security_Impact-Beta Release-0 Security_Impact-Stable
Comment 30 by creis@chromium.org, Oct 16 2013
Cc: davidben@chromium.org
Labels: -Restrict-View-SecurityNotify
Bulk release of old security bug reports.

Project Member Comment 32 by clusterf...@chromium.org, Feb 2 2016
Labels: -Security_Impact-Beta
Project Member Comment 33 by sheriffbot@chromium.org, Oct 1
This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member Comment 34 by sheriffbot@chromium.org, Oct 2
This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: allpublic
Sign in to add a comment