My favorites | Sign in
Project Home Downloads Wiki Issues Code Search
New issue   Search
for
  Advanced search   Search tips   Subscriptions
Issue 234809: URL spoof or renderer kill when committing prerendered/instant page with a pending entry
5 people starred this issue and may be notified of changes. Back to list
 
Project Member Reported by creis@chromium.org, Apr 23, 2013
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.
Apr 23, 2013
#1 creis@chromium.org
(No comment was entered for this change.)
Blocking: chromium:9682
Apr 24, 2013
#2 creis@chromium.org
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.
Apr 24, 2013
#3 creis@chromium.org
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.
Apr 25, 2013
#4 creis@chromium.org
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.

Summary: URL spoof or renderer kill when committing prerendered/instant page with a pending entry (was: Renderer kill via RenderThreadImpl::OnTempCrashWithData)
Cc: cbentzel@chromium.org sreeram@chromium.org nasko@chromium.org
Labels: -Type-Bug Type-Bug-Security Cr-Internals-Preload Cr-UI-Browser-Instant
Apr 25, 2013
#5 creis@chromium.org
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.
Apr 25, 2013
#6 sreeram@chromium.org
> 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.
Apr 25, 2013
#7 creis@chromium.org
Great.  Sounds like the impact is limited to prerendering, then.
Labels: -Cr-UI-Browser-Instant
Apr 26, 2013
#8 creis@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.
Cc: tburk...@chromium.org
Apr 26, 2013
#9 creis@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.
Cc: brettw@chromium.org
Apr 26, 2013
#10 creis@chromium.org
+jochen for FYI.
Cc: jochen@chromium.org
Apr 26, 2013
#11 cbentzel@chromium.org
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.
Apr 26, 2013
#12 creis@chromium.org
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.
Apr 26, 2013
#13 cbentzel@chromium.org
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.
Apr 26, 2013
#14 creis@chromium.org
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.
Apr 26, 2013
#15 creis@chromium.org
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.
May 7, 2013
#16 pal...@google.com
Security_Impact-None, since it's HEAD churn, right?
Labels: Security_Impact-None
May 7, 2013
#17 creis@chromium.org
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.
May 8, 2013
#18 pal...@chromium.org
creis, any thoughts on Security_Severity? -Medium?
May 9, 2013
#19 creis@chromium.org
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_Severity-Medium
May 10, 2013
#20 infe...@chromium.org
Please do read Mark's email titled "Calling a Code 28 for Security Bugs" on chrome-team mailing list.
Labels: Security-Code28
May 13, 2013
#21 infe...@chromium.org
very friendly ping :).
May 13, 2013
#22 creis@chromium.org
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.
May 22, 2013
#23 infe...@chromium.org
Can you please post the cl link here.
May 22, 2013
#24 creis@chromium.org
Sure: https://codereview.chromium.org/15041004/

Still trying to work through the implications for tests, and one issue for Instant's local page.
May 22, 2013
#25 creis@chromium.org
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.
Labels: -M-28 M-29
May 31, 2013
#26 bugdro...@chromium.org
------------------------------------------------------------------------
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
------------------------------------------------------------------------
May 31, 2013
#27 bugdro...@chromium.org
------------------------------------------------------------------------
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
------------------------------------------------------------------------
May 31, 2013
#28 infe...@chromium.org
(No comment was entered for this change.)
Status: Fixed
Labels: Merge-Approved Restrict-View-SecurityNotify
Jun 7, 2013
#29 scarybea...@gmail.com
(No comment was entered for this change.)
Labels: -Security_Impact-None -Merge-Approved Security_Impact-Beta Release-0 Security_Impact-Stable
Oct 16, 2013
#30 creis@chromium.org
(No comment was entered for this change.)
Cc: davidben@chromium.org
Nov 18, 2013
#31 jschuh@chromium.org
Bulk release of old security bug reports.

Labels: -Restrict-View-SecurityNotify
Sign in to add a comment

Powered by Google Project Hosting