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

Issue metadata

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



Sign in to add a comment
link

Issue 613266: Security: Universal XSS via reentrancy in FrameLoader::startLoad

Reported by marius.mlynski@gmail.com, May 19 2016

Issue description

VULNERABILITY DETAILS
From /third_party/WebKit/Source/core/loader/FrameLoader.cpp:
----------------
void FrameLoader::startLoad(...)
{
    ASSERT(client()->hasWebView());
    if (m_frame->document()->pageDismissalEventBeingDispatched() != Document::NoDismissal)
        return;
    (...)
    m_frame->document()->cancelParsing();
    detachDocumentLoader(m_provisionalDocumentLoader);

    // beforeunload fired above, and detaching a DocumentLoader can fire
    // events, which can detach this frame.
    if (!m_frame->host())
        return;

    m_provisionalDocumentLoader = client()->createDocumentLoader(m_frame, request, (...));
    (...)
    m_provisionalDocumentLoader->startLoadingMainResource();
    (...)
}
----------------

Detaching the provisional document loader can trigger load event handlers that may reenter FrameLoader::startLoad and attach a new provisional loader. After |detachDocumentLoader| returns, the pointer to the new loader is immediately overwritten. As a result, the abandoned loader will remain attached to the frame for the duration of its lifetime, which allows an attacker to escape from FrameLoader::prepareForCommit without the risk of canceling the load.

VERSION
Chrome 51.0.2704.54 (Beta)
Chrome 52.0.2739.0 (Dev)
Chromium 52.0.2742.0 (Release build compiled today)
 
exploit.html
1.2 KB View Download

Comment 1 by marius.mlynski@gmail.com, May 19 2016

Please note that this exploit doesn't work in the stable version, where this code looks a bit different (that's before https://crrev.com/243c3522db). I have yet to check if I can accommodate the stable channel.

Comment 2 by lgar...@chromium.org, May 19 2016

Components: Blink>HTML>Frame
Labels: Security_Severity-High Security_Impact-Beta
Owner: japhet@chromium.org
Status: Assigned (was: Unconfirmed)
Whew, neat! I can verify on OSX on Chrome 51.0.2704.29.

japhet@, it seems you wrote a lot of the code in this function. Could you take a look at this security bug?

Comment 3 by sheriffbot@chromium.org, May 20 2016

Project Member
Labels: M-51

Comment 4 by sheriffbot@chromium.org, May 20 2016

Project Member
Labels: ReleaseBlock-Stable

Comment 5 by sheriffbot@chromium.org, May 20 2016

Project Member
Labels: Pri-1

Comment 6 by marius.mlynski@gmail.com, May 21 2016

This is exploitable in the stable channel as well, some adjustments were needed to take a different timing of load events into account. Tested versions:

Chrome 50.0.2661.102 (Stable)
Chrome 51.0.2704.54 (Beta)
Chrome 52.0.2739.0 (Dev)
Chromium 53.0.2745.0 (Release build compiled today)
exploit.html
1.0 KB View Download

Comment 7 by sheriffbot@chromium.org, May 21 2016

Project Member
Labels: ReleaseBlock-Stable

Comment 8 by gov...@chromium.org, May 23 2016

Cc: sshruthi@chromium.org timwillis@chromium.org
Is this bug applicable to specific OS or all os?

Also We're getting closer to M51 Stable launch. pls make sure to land the fix and get it merged ASAP. All changes MUST be merged into the release branch by 5pm on May 23, Monday to make into the desktop Stable final build cut.

+timwillis@ (Security TPM)

Comment 9 by mmoroz@chromium.org, May 23 2016

Cc: mmoroz@chromium.org

Comment 10 by amineer@chromium.org, May 23 2016

Labels: OS-All
Smells like OS-All to me.

Comment 11 by timwillis@google.com, May 23 2016

Labels: -Security_Impact-Beta Security_Impact-Stable reward-topanel
This shouldn't block the release of M-51 that triggers today, as any fix will need bake time. We can consider a fix here to ride along with a post-stable update.

Comment 12 by japhet@chromium.org, May 23 2016

I've yet to reproduce this locally on trunk with either repro. The alert() appears to be reporting the correct location, and I've added a "CHECK(!m_provisionalDocuemntLoader);" after detachDocumentLoader() that hasn't failed. Am I missing something?

Comment 13 by marius.mlynski@gmail.com, May 23 2016

I can't get the exploit to fail, no matter what I try. The original version was sort of rushed, but I can't imagine what could fail in the revised one. Maybe if http://www.apple.com was handled faster than data:text/xml,? Can you try a different URL that responds slower? In a real-life scenario, an attacker would probably use a dedicated web server guaranteed not to handle the HTTP request too fast. Also, can you try increasing both timeouts significantly? Does it work in stable/beta/dev? I'll recompile the trunk to verify nothing has changed recently and get back to you with a debug version and a set of assertions to pinpoint the exact problem.

> and I've added a "CHECK(!m_provisionalDocuemntLoader);" after detachDocumentLoader() that hasn't failed.

Sorry, this appears to be a mistake in the original bug description: "After |detachDocumentLoader| returns" should have read "After |loader->detachFromFrame()| in detachDocumentLoader returns". |m_provisionalDocumentLoader| is passed as a reference, and it's already cleared in detachDocumentLoader. The proper check would be to set aside |m_provisionalDocumentLoader| when FrameLoader::detachDocumentLoader is entered and verify that it hasn't changed before |loader = nullptr;|. Apologies for the confusion, this was a rushed report and it shows.

Comment 14 by japhet@chromium.org, May 23 2016

Ah, of course. I probably should've been able to figure that out :)

Comment 15 by japhet@chromium.org, May 24 2016

Cc: dcheng@chromium.org

Comment 16 by dcheng@chromium.org, May 25 2016

Cc: csharrison@chromium.org

Comment 17 by csharrison@chromium.org, May 25 2016

Hm I can't repro either (getting abc.xyz with abc.xyz alert). This causes the renderer to crash in canary 52.0.2721.0 (old version) though (every couple of tries).

I tried replacing apple.com with some other urls but still no dice.

I haven't added any CHECKs in the renderer yet.

Comment 18 by marius.mlynski@gmail.com, May 25 2016

@csharrison: This is the expected result, abc.xyz should be protected by the same-origin policy. Swapping abc.xyz and apple.com URLs should yield XSS in apple.com.

Comment 19 by csharrison@chromium.org, May 25 2016

Ah okay I misread the source. Looks like we might be able to do a similar trick like we do in prepareForCommit wrt provisional document loader.

Comment 20 by japhet@chromium.org, May 25 2016

I've got a tentative solution up at https://codereview.chromium.org/2006033002/, but given that I made the same mistake as Comment #17 in my first read of the repro, I want to walk through the repro a bit more and make sure I'm not missing anything.

Comment 21 by mmoroz@google.com, May 25 2016

Status: Started (was: Assigned)

Comment 23 by aarya@google.com, May 27 2016

Status: Fixed (was: Started)
Marking fixed for now based on c#22. If any more work shows up as a result of c#20, please feel free to reopen bug or file a new security bug for that variant.

Comment 24 by sheriffbot@chromium.org, May 27 2016

Project Member
Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify

Comment 25 by ClusterFuzz, May 27 2016

Project Member
Labels: Merge-Triage M-52
Adding Merge-Triage label for tracking purposes.

Once your fix had sufficient bake time (on canary, dev as appropriate), please nominate your fix for merge by adding the Merge-Request-XX label, where XX is the Chrome milestone.

When your merge is approved by the release manager, please start merging with higher milestone label first. Make sure to re-request merge for every milestone in the label list. You can get branch information on omahaproxy.appspot.com.

- Your friendly ClusterFuzz

Comment 26 by japhet@chromium.org, May 31 2016

Labels: -Merge-Triage Merge-Request-51 Merge-Request-52
I haven't found any new, relevant crashes in recent canaries, so requesting merge to beta and stable.

Comment 27 by tin...@google.com, May 31 2016

Labels: -Merge-Request-52 Merge-Approved-52 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M52 (branch: 2743)

Comment 28 by tin...@google.com, May 31 2016

Labels: -Merge-Request-51 Merge-Review-51 Hotlist-Merge-Review
[Automated comment] Request affecting a post-stable build (M51), manual review required.

Comment 29 by gov...@chromium.org, May 31 2016

Labels: -Merge-Request-51 Merge-Approved-51
Approving merge to M51 branch 2704 based on comment #26. Please merge ASAP before 4:00 PM PST if possible so we take it for this week Stable release. Thank you.

Comment 32 by timwillis@google.com, May 31 2016

Labels: Release-1-M51

Comment 33 by timwillis@google.com, Jun 6 2016

Labels: -reward-topanel reward-7500 CVE-2016-1697 reward-unpaid
As noted in the release notes - $7500 here. Congrats!

Comment 34 by timwillis@google.com, Jun 17 2016

Labels: reward-inprocess

Comment 35 by timwillis@google.com, Jun 17 2016

Labels: -reward-unpaid

Comment 36 by sheriffbot@chromium.org, Sep 2 2016

Project Member
Labels: -Restrict-View-SecurityNotify
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

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

Project Member
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

Comment 38 by sheriffbot@chromium.org, Oct 2 2016

Project Member
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

Comment 39 by mbarbe...@chromium.org, Oct 2 2016

Labels: allpublic

Comment 40 by awhalley@chromium.org, Apr 25 2018

Labels: CVE_description-submitted

Sign in to add a comment