New issue
Advanced search Search tips
Starred by 2 users
Status: Fixed
Owner:
Closed: Jun 23
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 1
Type: Bug-Security



Sign in to add a comment
Chrome broker PP_Instance overwrite in IPC handler OnMsgDidCreateInProcessInstance
Reported by scdengy...@gmail.com, Jun 15 Back to list
UserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/58.0.3029.110 Safari/537.36

Steps to reproduce the problem:
The sandbox process can send the following IPC message to browser process:
IPC_MESSAGE_HANDLER(FrameHostMsg_DidCreateInProcessInstance,
                        OnMsgDidCreateInProcessInstance)

The implementation of PepperRendererConnection::OnMsgDidCreateInProcessInstance fails however to verify that the instance is actually an instance to a message it sent:

void PepperRendererConnection::OnMsgDidCreateInProcessInstance(
    PP_Instance instance,
    const PepperRendererInstanceData& instance_data) {
  PepperRendererInstanceData data = instance_data;
  data.render_process_id = render_process_id_;
  in_process_host_->AddInstance(instance, data);
}

If the instance (which is controlled by the sandbox process) is in the instance_map_ the we can overwrite the instance with fake data.

What is the expected behavior?

What went wrong?
privilege elevation

Did this work before? N/A 

Chrome version: 61.0.3128.0  Channel: dev
OS Version: OS X 10.12.5
Flash Version: Shockwave Flash 26.0 r0

Credit to Yuan Deng, Yu Zhou of Ant-financial Light-Year Security Lab(蚂蚁金服巴斯光年安全实验室)
 
patch_add.diff
758 bytes Download
void BrowserPpapiHostImpl::AddInstance(
    PP_Instance instance,
    const PepperRendererInstanceData& renderer_instance_data) {
  DCHECK(instance_map_.find(instance) == instance_map_.end());  <---DCHECK not compiled in release version
  instance_map_[instance] =
      base::MakeUnique<InstanceData>(renderer_instance_data);
}
Cc: raymes@chromium.org
Components: Internals>Plugins>Pepper
Labels: Security_Impact-Stable
Owner: bbudge@chromium.org
Status: Assigned
bbudge, could you please take a look and help assess severity? I'm not sure what the security implications of this bug might be. Thanks!
This seems to allow a malicious plugin to add non-existent PP_Instance's to the map too. It's hard to say how bad this would be. It seems prudent to check the PP_Instance. However, I don't know how to do this offhand, as nobody has been working on Pepper much.

Perhaps Raymes can provide more in-depth analysis.
Labels: Security_Severity-High
Tentatively assigning high severity out of an abundance of caution.
Project Member Comment 5 by sheriffbot@chromium.org, Jun 16
Labels: M-59
Project Member Comment 6 by sheriffbot@chromium.org, Jun 16
Labels: -Pri-2 Pri-1
Status: Started
Labels: Merge-Request-60
Project Member Comment 10 by sheriffbot@chromium.org, Jun 22
Labels: -Merge-Request-60 Hotlist-Merge-Review Merge-Review-60
This bug requires manual review: M60 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), josafat@(ChromeOS), bustamante@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member Comment 11 by sheriffbot@chromium.org, Jun 23
Status: Fixed
Please mark security bugs as fixed as soon as the fix lands, and before requesting merges. This update is based on the merge- labels applied to this issue. Please reopen if this update was incorrect.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Can you confirm if this has landed in canary and verified the fix?
It is in Canary. I have no way to reproduce the issue, but the fix is very simple. The fix is by the same CL as for  issue 733549 .
Project Member Comment 14 by bugdroid1@chromium.org, Jun 23
Labels: merge-merged-3112
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2e98fb826886c18acdd55890ee0b52d2aee57394

commit 2e98fb826886c18acdd55890ee0b52d2aee57394
Author: Bill Budge <bbudge@chromium.org>
Date: Fri Jun 23 22:23:17 2017

Validate in-process plugin instance messages.

Bug:  733548 ,  733549 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_site_isolation
Change-Id: Ie5572c7bcafa05399b09c44425ddd5ce9b9e4cba
Reviewed-on: https://chromium-review.googlesource.com/538908
Commit-Queue: Bill Budge <bbudge@chromium.org>
Reviewed-by: Raymes Khoury <raymes@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#480696}
Review-Url: https://codereview.chromium.org/2955703002 .
Cr-Commit-Position: refs/branch-heads/3112@{#456}
Cr-Branched-From: b6460e24cf59f429d69de255538d0fc7a425ccf9-refs/heads/master@{#474897}

[modify] https://crrev.com/2e98fb826886c18acdd55890ee0b52d2aee57394/content/browser/renderer_host/pepper/browser_ppapi_host_impl.cc
[modify] https://crrev.com/2e98fb826886c18acdd55890ee0b52d2aee57394/content/browser/renderer_host/pepper/pepper_renderer_connection.cc

Project Member Comment 15 by sheriffbot@chromium.org, Jun 24
Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Labels: -Merge-Review-60 Merge-Approved-60
Approving merge to M60. 
Labels: reward-topanel
Hi scdengyuan@. The VRP panel has looked at this, and ask if you could provide a proof of concept, or details about how this could be triggered?
run the code below in a malicious plugin, then you can overwrite an existent PP_Instance in the broker, or add non-existent PP_Instance's to the map.

Send(new FrameHostMsg_DidCreateInProcessInstance(
      fake_instance,
      PepperRendererInstanceData(0x41414141, 0x41414141, "AAAAA", "AAAA", true)));
Project Member Comment 20 by sheriffbot@chromium.org, Jun 30
Cc: abdulsyed@chromium.org
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

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: -Merge-Approved-60
Removing merge-approved label.
Labels: -M-59 M-60 M-61
Labels: Release-0-M60
Labels: -reward-topanel reward-0
Per our email conversation, we're covering the reward payment for this issue with 733549.  Thanks!
Labels: reward-unpaid
Labels: -reward-unpaid
Labels: CVE-2017-5099
Project Member Comment 28 by sheriffbot@chromium.org, Sep 30
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
Sign in to add a comment