New issue
Advanced search Search tips

Issue 601706 link

Starred by 1 user

Issue metadata

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



Sign in to add a comment

Security: Universal XSS using a flaw in the load deferral logic

Reported by marius.mlynski@gmail.com, Apr 8 2016

Issue description

VULNERABILITY DETAILS
This is a regression from https://crrev.com/f92a1f3b9 . Previously, ResourceLoader::start bailed out if ResourceLoader::m_defersLoading was true. Now, it calls setDefersLoading on the associated WebURLLoader instead:

----------------
void ResourceLoader::start(ResourceRequest& request)
{
    (...)
    m_loader = adoptPtr(Platform::current()->createURLLoader());
    m_loader->setDefersLoading(m_fetcher->defersLoading());
    ASSERT(m_loader);
    m_loader->setLoadingTaskRunner(m_fetcher->loadingTaskRunner());

    if (m_resource->options().synchronousPolicy == RequestSynchronously)
        requestSynchronously(request);
    else
        m_loader->loadAsynchronously(WrappedResourceRequest(request), this);
}
----------------
void WebURLLoaderImpl::setDefersLoading(bool value) {
  context_->SetDefersLoading(value);
}
----------------
void WebURLLoaderImpl::Context::SetDefersLoading(bool value) {
  if (request_id_ != -1)
    resource_dispatcher_->SetDefersLoading(request_id_, value);
  (...)
}
----------------

Note that |resource_dispatcher_->SetDefersLoading(request_id_, value)| isn't called because |request_id_| isn't set until after the |m_loader->loadAsynchronously| call in ResourceLoader::start. Therefore, if a load is started after instantiating a ScopedPageLoadDeferrer, the pending request is never marked as deferred and it's allowed to proceed regardless of the deferral state of the fetcher. This allows an attacker to load cross-origin documents in numerous unexpected circumstances.

VERSION
Chrome 51.0.2700.0 (Dev)
Chromium 51.0.2703.0 + Pepper Flash (Release build compiled today)
 
exploit.zip
2.0 KB Download

Comment 1 by kenrb@chromium.org, Apr 8 2016

Cc: dcheng@chromium.org
Components: Blink>Loader
Labels: M-51 Security_Severity-High Security_Impact-Head OS-All Pri-1
Owner: japhet@chromium.org
Status: Assigned (was: Unconfirmed)
Thanks for the report.

japhet@: Can you please take a look?
Project Member

Comment 2 by sheriffbot@chromium.org, Apr 14 2016

Labels: -Security_Impact-Head Security_Impact-Beta
Project Member

Comment 3 by ClusterFuzz, Apr 14 2016

Labels: ReleaseBlock-Stable

Comment 4 by dcheng@chromium.org, Apr 16 2016

Cc: yhirano@chromium.org
Project Member

Comment 5 by bugdroid1@chromium.org, Apr 21 2016

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

commit c5a3e99a81bee5f325b81be3d21f5daf1854b572
Author: japhet <japhet@chromium.org>
Date: Thu Apr 21 21:44:06 2016

Enable setting deferral state on ResourceDispatcher at request start

In https://chromium.googlesource.com/chromium/src/+/f92a1f3b9,
blink's ResourceLoader stopped handling load deferrals,
instead leaving it to WebURLLoaderImpl. However,
WebURLLoaderImpl can't quite do everything it needs to, as
ResourceDispatcher also needs to note that the load is
deferred, and it can't do that until a PendingRequest has
been created. Give ResourceDispatcher a way to immediately
mark a load a deferred on start.

BUG= 601706 

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

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

[modify] https://crrev.com/c5a3e99a81bee5f325b81be3d21f5daf1854b572/content/child/resource_dispatcher.h
[modify] https://crrev.com/c5a3e99a81bee5f325b81be3d21f5daf1854b572/content/child/web_url_loader_impl.cc
[modify] https://crrev.com/c5a3e99a81bee5f325b81be3d21f5daf1854b572/content/child/web_url_loader_impl_unittest.cc

Project Member

Comment 6 by sheriffbot@chromium.org, Apr 22 2016

japhet: Uh oh! This issue still open and hasn't been updated in the last 14 days. This is a serious vulnerability, and we want to ensure that there's progress. Could you please leave an update with the current status and any potential blockers?

If you're not the right owner for this issue, could you please remove yourself as soon as possible or help us find the right one?

If the issue is fixed or you can't reproduce it, please close the bug. If you've started working on a fix, please set the status to Started.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 7 by japhet@chromium.org, Apr 22 2016

Labels: Merge-Request-51

Comment 8 by tin...@google.com, Apr 22 2016

Labels: -Merge-Request-51 Merge-Approved-51 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M51 (branch: 2704)

Comment 9 by gov...@chromium.org, Apr 22 2016

Please merge your change to M51 branch 2704 before 5:00 PM PST Monday (04/25/16) so we can take it for next week M51 Beta candidate cut. Thank you.
Project Member

Comment 10 by bugdroid1@chromium.org, Apr 22 2016

Labels: -merge-approved-51 merge-merged-2704
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cdb8e78779913eca8c5860c8cb8642752b2b9a83

commit cdb8e78779913eca8c5860c8cb8642752b2b9a83
Author: Nate Chapin <japhet@chromium.org>
Date: Fri Apr 22 21:02:07 2016

Enable setting deferral state on ResourceDispatcher at request start

In https://chromium.googlesource.com/chromium/src/+/f92a1f3b9,
blink's ResourceLoader stopped handling load deferrals,
instead leaving it to WebURLLoaderImpl. However,
WebURLLoaderImpl can't quite do everything it needs to, as
ResourceDispatcher also needs to note that the load is
deferred, and it can't do that until a PendingRequest has
been created. Give ResourceDispatcher a way to immediately
mark a load a deferred on start.

BUG= 601706 

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

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

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

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

[modify] https://crrev.com/cdb8e78779913eca8c5860c8cb8642752b2b9a83/content/child/resource_dispatcher.h
[modify] https://crrev.com/cdb8e78779913eca8c5860c8cb8642752b2b9a83/content/child/web_url_loader_impl.cc
[modify] https://crrev.com/cdb8e78779913eca8c5860c8cb8642752b2b9a83/content/child/web_url_loader_impl_unittest.cc

Status: Fixed (was: Assigned)
Project Member

Comment 12 by ClusterFuzz, Apr 23 2016

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Labels: reward-topanel
Labels: -reward-topanel reward-7500 reward-unpaid
Another $7,500 for the tab!
Project Member

Comment 15 by sheriffbot@chromium.org, Jul 30 2016

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
Labels: -reward-unpaid reward-inprocess
Project Member

Comment 17 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 18 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

Sign in to add a comment