New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 160803 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Dec 2012
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug-Security



Sign in to add a comment

Security: ugly crash with history.replaceState() while the window displays HTTPS interstitial

Project Member Reported by lcamtuf@google.com, Nov 13 2012

Issue description

This works for me on Linux and Windows:

http://lcamtuf.coredump.cx/httpsinter/index2.html

(1dc8.1a30): Access violation - code c0000005 (first chance)
chrome_5b630000:
5bb46a8b ff5010          call    dword ptr [eax+10h]  ds:002b:f62ff8f7=????????

Stumbled upon it while doing something completely unrelated but seems vaguely bad.

 
Cc: abarth@chromium.org creis@chromium.org agl@chromium.org
Heh. That address starts with "f" which implies our new tcmalloc freelist poisioning may have hit, which implies a critical use-after-free in the browser :)

Charlie, who's the best to fix?

Comment 2 by agl@chromium.org, Nov 13 2012

In debug we hit a DCHECK:

content/browser/web_contents/navigation_controller_impl.cc:780
// The active entry's SiteInstance should match our SiteInstance.
DCHECK(active_entry->site_instance() == web_contents_->GetSiteInstance());

(The LHS is NULL in that equality.)

If we remove that DCHECK, we crash in chrome/browser/ui/constrained_window_tab_helper.cc:96

With a freed details.entry:
print *details.entry 
$3 = {_vptr.NavigationEntry = 0xffff80028e7c54ed}

I'd suggest Avi, but I don't think he's around at the moment.

Comment 3 by jsc...@chromium.org, Nov 13 2012

Labels: -Area-Undefined Area-Internals SecSeverity-Critical OS-All SecImpacts-Stable SecImpacts-Beta Mstone-23
Status: Available
Legitimate critical, so it's a p0.

P.S.  This is why no one likes Michal... err... I mean "thanks Michal."

Comment 4 by creis@chromium.org, Nov 14 2012

Owner: creis@chromium.org
Status: Assigned
That DCHECK in NavigationController is something I should look at, though the crash itself is in ConstrainedWindowTabHelper, which I'm not familiar with:
https://crash.corp.google.com/reportdetail?reportid=62c6c39fadb64a86

I'll see if I can take a look tomorrow, unless anyone recognizes that class.

Comment 5 by creis@chromium.org, Nov 14 2012

Cc: jcivelli@chromium.org
Status: Started
Ah, I get it.  InterstitialPage::DontProceed is getting called as one of the listeners to NotifyNavigationEntryCommitted.  It triggers a DiscardTransientEntry, which deletes the NavigationEntry.  That entry is still on the details object being sent to other listeners, so they end up seeing garbage when they read details.entry.  The crash could end up in any number of places (ConstrainedWindowTabHelper, InfoBarDelegate, etc).

I haven't looked closely enough to figure out the fix yet.

Comment 6 by creis@chromium.org, Nov 14 2012

Looks like NavigationController correctly classifies the replaceState as an in-page navigation for an existing entry, but it's not clearing the transient entry for the interstitial page.  As a result, the |details| object gets the interstitial page's entry from GetActiveEntry().

I have a one-line change to RendererDidNavigateInPage that discards the non-committed entries even if there's no pending_entry_, and that prevents the crash.  I want to look a little closer to be sure it's right, but it seems like a good fix.  Hope to have a CL up this afternoon.

Comment 7 by creis@chromium.org, Nov 14 2012

Cc: brettw@chromium.org
CL up for review: https://codereview.chromium.org/11377169/
CC'ing Brett for context.
Project Member

Comment 8 by bugdroid1@chromium.org, Nov 15 2012

The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=167856

------------------------------------------------------------------------
r167856 | creis@chromium.org | 2012-11-15T04:39:49.039470Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/web_contents/navigation_controller_impl_unittest.cc?r1=167856&r2=167855&pathrev=167856
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/web_contents/navigation_controller_impl.cc?r1=167856&r2=167855&pathrev=167856

Ensure a transient entry is discarded on in-page navigations.

BUG= 160803 
TEST=See bug for repro steps.


Review URL: https://chromiumcodereview.appspot.com/11377169
------------------------------------------------------------------------
Labels: -Restrict-View-SecurityTeam -Pri-0 Restrict-View-SecurityNotify Pri-1 Merge-Approved
Status: FixUnreleased

Comment 10 by lcamtuf@google.com, Nov 15 2012

Well that was quick :-)
Well, of course. This is a genuine critical.
Didn't know you had it in you, Michal :P :P
Project Member

Comment 12 by bugdroid1@chromium.org, Nov 30 2012

Labels: -Merge-Approved merge-merged-1271
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=170536

------------------------------------------------------------------------
r170536 | cevans@chromium.org | 2012-11-30T21:08:53.261336Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/branches/1271/src/content/browser/web_contents/navigation_controller_impl_unittest.cc?r1=170536&r2=170535&pathrev=170536
   M http://src.chromium.org/viewvc/chrome/branches/1271/src/content/browser/web_contents/navigation_controller_impl.cc?r1=170536&r2=170535&pathrev=170536

Merge 167856 - Ensure a transient entry is discarded on in-page navigations.

BUG= 160803 
TEST=See bug for repro steps.


Review URL: https://chromiumcodereview.appspot.com/11377169

TBR=creis@chromium.org
Review URL: https://codereview.chromium.org/11428119
------------------------------------------------------------------------
Project Member

Comment 13 by bugdroid1@chromium.org, Nov 30 2012

Labels: merge-merged-1312
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=170538

------------------------------------------------------------------------
r170538 | cevans@chromium.org | 2012-11-30T21:11:07.240022Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/branches/1312/src/content/browser/web_contents/navigation_controller_impl.cc?r1=170538&r2=170537&pathrev=170538
   M http://src.chromium.org/viewvc/chrome/branches/1312/src/content/browser/web_contents/navigation_controller_impl_unittest.cc?r1=170538&r2=170537&pathrev=170538

Merge 167856 - Ensure a transient entry is discarded on in-page navigations.

BUG= 160803 
TEST=See bug for repro steps.


Review URL: https://chromiumcodereview.appspot.com/11377169

TBR=creis@chromium.org
Review URL: https://codereview.chromium.org/11308297
------------------------------------------------------------------------
Labels: CVE-2012-5142
Status: Fixed
Project Member

Comment 16 by bugdroid1@chromium.org, Mar 10 2013

Labels: -Type-Security -Area-Internals -SecSeverity-Critical -SecImpacts-Stable -SecImpacts-Beta -Mstone-23 Security-Impact-Stable Security-Impact-Beta Cr-Internals M-23 Type-Bug-Security Security-Severity-Critical
Labels: -Restrict-View-SecurityNotify
Project Member

Comment 18 by bugdroid1@chromium.org, Mar 21 2013

Labels: -Security-Impact-Stable Security_Impact-Stable
Project Member

Comment 19 by bugdroid1@chromium.org, Mar 21 2013

Labels: -Security-Impact-Beta Security_Impact-Beta
Project Member

Comment 20 by bugdroid1@chromium.org, Mar 21 2013

Labels: -Security-Severity-Critical Security_Severity-Critical
Project Member

Comment 21 by sheriffbot@chromium.org, Jun 14 2016

Labels: -security_impact-beta
Project Member

Comment 22 by sheriffbot@chromium.org, Oct 1 2016

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 23 by sheriffbot@chromium.org, Oct 2 2016

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
Labels: CVE_description-submitted

Sign in to add a comment