New issue
Advanced search Search tips
Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 1
Type: Bug-Security



Sign in to add a comment

Security: use-after-free in AttachFilteredEvent on event_bindings.cc

Reported by jinmot...@gmail.com, Jul 2 2016

Issue description

VULNERABILITY DETAILS
When processing smart pointers in AddEventMatcher(event_filters.cc), it don't std::move the smart pointer on certain condition, like (see <--)
On line 56
EventFilter::MatcherID EventFilter::AddEventMatcher(
    const std::string& event_name,
    std::unique_ptr<EventMatcher> matcher <-- unique_ptr) {
  MatcherID id = next_id_++;
  URLMatcherConditionSet::Vector condition_sets;
  if (!CreateConditionSets(id, matcher.get(), &condition_sets))
    return -1; <-- frees the EventMatcher matcher object

  for (URLMatcherConditionSet::Vector::iterator it = condition_sets.begin();
       it != condition_sets.end(); it++) {
    condition_set_id_to_event_matcher_id_.insert(
        std::make_pair((*it)->id(), id));
  }
  id_to_event_name_[id] = event_name;
  event_matchers_[event_name][id] = linked_ptr<EventMatcherEntry>(
      new EventMatcherEntry(std::move(matcher) <-- correctly moved, &url_matcher_, condition_sets));
  return id;
}

This is the partial definition of EventMatcher class (event_matcher.h).
On line 23
class EventMatcher {
...
  std::unique_ptr<base::DictionaryValue> filter_;
...
};

The filter_ is wrapped in unique_ptr, so freed when EventMatcher object frees.
It results to use-after-free in AttachFilteredEvent on event_bindings.cc.

On line 262
  std::unique_ptr<base::DictionaryValue> filter;
...
On line 277
  base::DictionaryValue* filter_weak = filter.get();
  int id = g_event_filter.Get().AddEventMatcher(
      event_name, ParseEventMatcher(std::move(filter))); <-- Actually, ParseEventMatcher creates EventMatcher object mentioned above, with the filter_ member provided to the argument. AddEventMatcher frees filter_weak.
...
On line 283
  std::string extension_id = context()->GetExtensionID();
  if (AddFilter(event_name, extension_id, *filter_weak)) { <-- boom 1
    bool lazy = ExtensionFrameHelper::IsContextForEventPage(context());
    content::RenderThread::Get()->Send(new ExtensionHostMsg_AddFilteredListener(
        extension_id, event_name, *filter_weak, lazy)); <-- boom 2
  }

Actually, the Event object itself is exposed in Port object, which is returned by chrome.runtime.connect. WEB_PAGE context can call it. Like,

var Event = chrome.runtime.connect('yo').onDisconnect.constructor;

And we can provide a invalid URL matcher.

var event = new Event('app.app', [], {supportsFilters: true}); <-- Abstraction of the event binding interface.
event.addListener(function() {}, {url: [1]}); <-- Calls the AttachFilteredEvent method.

VERSION
Chrome Version: 51.0.2704.84 stable 32bit
Operating System: Windows 10 x64

REPRODUCTION CASE

I've attached the PoC as a HTML file.
 
chrome_new1.html
321 bytes View Download
FYI, it can cause heap BOF or changing EIP if it's sprayed by IPC message between free/use(used in Value::DeepCopy and sending IPC message on line 286).

This case was little for stable build but on canary build, and I don't know why..

8^(
Cc: rdevlin....@chromium.org
Components: Platform>Extensions>API
Labels: Security_Impact-Stable M-53 OS-Chrome OS-Linux OS-Mac OS-Windows
Owner: mea...@chromium.org
Status: Assigned (was: Unconfirmed)
Labels: Security_Severity-Medium Pri-1
Project Member

Comment 4 by sheriffbot@chromium.org, Jul 17 2016

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

Comment 5 by sheriffbot@chromium.org, Jul 31 2016

meacer: Uh oh! This issue still open and hasn't been updated in the last 28 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
Labels: -Security_Severity-Medium Security_Severity-High
jinmoteam@: Thanks for the report and apologies for the late response.

I'm bumping up the severity to high as this doesn't require an extension to be installed. 
meacer@: thanks!

Comment 8 by mea...@chromium.org, Aug 11 2016

Status: Started (was: Assigned)
https://codereview.chromium.org/2236133002/
Project Member

Comment 9 by bugdroid1@chromium.org, Aug 12 2016

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

commit ba011d9f8322c62633a069a59c2c5525e3ff46cc
Author: meacer <meacer@chromium.org>
Date: Fri Aug 12 00:23:40 2016

Ignore filtered event if an event matcher cannot be added.

BUG= 625404 

Review-Url: https://codereview.chromium.org/2236133002
Cr-Commit-Position: refs/heads/master@{#411472}

[modify] https://crrev.com/ba011d9f8322c62633a069a59c2c5525e3ff46cc/extensions/renderer/event_bindings.cc

Status: Fixed (was: Started)
Project Member

Comment 11 by sheriffbot@chromium.org, Aug 12 2016

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Labels: Merge-Request-53

Comment 13 by dimu@chromium.org, Aug 12 2016

Labels: -Merge-Request-53 Merge-Approved-53 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M53 (branch: 2785)
Please merge your change by Monday (08/15) 5:00 PM PT so we can take it in for next week Beta release. Thank you.

Project Member

Comment 15 by bugdroid1@chromium.org, Aug 15 2016

Labels: -merge-approved-53 merge-merged-2785
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7de974664bfeefa2310ee0f350437f3fdf466b91

commit 7de974664bfeefa2310ee0f350437f3fdf466b91
Author: Mustafa Acer <meacer@chromium.org>
Date: Mon Aug 15 19:05:41 2016

Ignore filtered event if an event matcher cannot be added.

BUG= 625404 

Review-Url: https://codereview.chromium.org/2236133002
Cr-Commit-Position: refs/heads/master@{#411472}
(cherry picked from commit ba011d9f8322c62633a069a59c2c5525e3ff46cc)

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

Cr-Commit-Position: refs/branch-heads/2785@{#601}
Cr-Branched-From: 68623971be0cfc492a2cb0427d7f478e7b214c24-refs/heads/master@{#403382}

[modify] https://crrev.com/7de974664bfeefa2310ee0f350437f3fdf466b91/extensions/renderer/event_bindings.cc

Labels: reward-topanel
Labels: Release-0-M53
Labels: -reward-topanel reward-unpaid reward-3000
Congratulations! The panel decided to award $3,000 for this bug.  A member of our finance team will be in touch shortly with next steps.

*** Boilerplate reminders! ***
Please do NOT publicly disclose details until a fix has been released to all our users. Early public disclosure may cancel the provisional reward. Also, please be considerate about disclosure when the bug affects a core library that may be used by other products. Please do NOT share this information with third parties who are not directly involved in fixing the bug. Doing so may cancel the provisional reward. Please be honest if you have already disclosed anything publicly or to third parties. Lastly, we understand that some of you are not interested in money. We offer the option to donate your reward to an established charity. If you prefer this option, let us know and we will also match your donation - subject to our discretion. Any rewards that are unclaimed after 12 months will be donated to a charity of our choosing.
*********************************
Labels: -reward-unpaid reward-inprocess
Labels: CVE-2016-5156
btw can I disclose this publicly?
Project Member

Comment 23 by sheriffbot@chromium.org, Nov 18 2016

Labels: -Restrict-View-SecurityNotify allpublic
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: CVE_description-submitted

Sign in to add a comment