New issue
Advanced search Search tips
Starred by 1 user

Issue metadata

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



Sign in to add a comment

Security: Bad cast to ChromeDownloadManagerDelegate* from DevToolsDownloadManagerDelegate*

Project Member Reported by rob@robwu.nl, Jan 25

Issue description

VULNERABILITY DETAILS
DownloadPrefs::FromDownloadManager is reachable from several places (e.g. when a download is started) and implemented as follows [1]:
  DownloadPrefs::FromDownloadManager(
      DownloadManager* download_manager) {
    ChromeDownloadManagerDelegate* delegate =
        static_cast<ChromeDownloadManagerDelegate*>( // <---- WARNING
            download_manager->GetDelegate());
    return delegate->download_prefs();               // <---- undefined behavior after bad cast
  }

At the place marked "WARNING", a bad cast occurs:
download_manager->GetDelegate() is cast from content::DownloadManagerDelegate* to ChromeDownloadManagerDelegate*.
But it can also be a DevToolsDownloadManagerDelegate*, when the DevToolsDownloadManagerDelegate::TakeOver is called
(via the "Page.setDownloadBehavior" DevTools protocol method [2]) [3].

This can be exploited via Chrome extensions and the remote debugging protocol.

VERSION
Chrome Version: 64.0.3282.119 (stable)

REPRODUCTION CASE
Download and unpack the attached extension and start Chrome:

$ ./chromium --user-data-dir=/tmp/whatever --load-extension=/tmp/downloadBehavior-bad-cast

Received signal 11 SEGV_MAPERR ffffd86fb80ead74
#0 0x559f72545b77 base::debug::StackTrace::StackTrace()
#1 0x559f725456df base::debug::(anonymous namespace)::StackDumpSignalHandler()
#2 0x7f33a9b63da0 <unknown>
#3 0x559f735e81f7 subtle::PrefMemberBase::VerifyPref()
#4 0x559f722dd70d DownloadPrefs::PromptForDownload()       <--- The memory here is *not* a DownloadPrefs*
#5 0x559f722fbc7d DownloadFilePicker::DownloadFilePicker() <--- Calls DownloadPrefs::FromBrowserContext, which calls FromDownloadManager
#6 0x559f722fe0b6 DownloadTargetDeterminer::DoRequestConfirmation()
#7 0x559f722fd6d3 DownloadTargetDeterminer::DoLoop()


PS. I find it peculiar that PageHandler::SetDownloadBehavior changes the behavior for BrowserContext* (which affects all tabs in the same profile), even though the "Page.setDownloadBehavior" DevTools command is only supposed to alter the behavior for a single target (page).


[1] https://chromium.googlesource.com/chromium/src/+/6fd2b931db44eadcf5c2d33409d8b4e6bf4f6ad7/chrome/browser/download/download_prefs.cc#247
[2] https://chromium.googlesource.com/chromium/src/+/9fa5fa7e7c5aa6f67ee721cd4022de491dc255d1/content/browser/devtools/protocol/page_handler.cc#724
[3] https://chromium.googlesource.com/chromium/src/+/6fd2b931db44eadcf5c2d33409d8b4e6bf4f6ad7/content/browser/devtools/protocol/devtools_download_manager_delegate.cc#57
 
downloadBehavior-bad-cast.zip
1.9 KB Download

Comment 1 by rob@robwu.nl, Jan 25

For clarity, content::DownloadManagerDelegate is the base class of
ChromeDownloadManagerDelegate [4] and
DevToolsDownloadManagerDelegate [5].

But download_prefs() is part of ChromeDownloadManagerDelegate (not of the base class) (and both classes have completely different members).


[4] https://chromium.googlesource.com/chromium/src/+/ca91e7d0fceae42b489bb5e09d17e0782917e43e/chrome/browser/download/chrome_download_manager_delegate.h#49
[5] https://chromium.googlesource.com/chromium/src/+/ca91e7d0fceae42b489bb5e09d17e0782917e43e/content/browser/devtools/protocol/devtools_download_manager_delegate.h#27
Owner: dgozman@chromium.org
Status: Assigned (was: Untriaged)
dgozman: PTAL?

FWIW I seem to be unable to get a crash on Linux with a local and asan builds (even after enabling the auto download). Perhaps I'm missing a step.
Cc: dgozman@chromium.org
Owner: dvallet@chromium.org
David, mind taking a look?

Comment 4 by rob@robwu.nl, Jan 25

@#2

Odd. I got the crash on my local release build (v63), but not with an ASAN or Debug build, nor the official build from ArchLinux.
Here is another way to trigger the vulnerability (manually because it was the first caller that I picked. There are other extension APIs (e.g. chrome.browsingData and chrome.downloads) that can be used to automate the exploitation).

1. Modify the extension's background.js and delete (or comment out) the line with "a.click();" (so that the DevToolsDownloadManagerDelegate is still installed, but the code path is not yet triggered. Then trigger one of the other code paths, e.g. at https://chromium.googlesource.com/chromium/src/+/0fc0e49c81e22c768f9e1d3e8fe55a401425907d/chrome/browser/devtools/devtools_file_helper.cc#261
2. (Re)load the modified extension.
3. Visit chrome-devtools://devtools/blank in a new tab (not in tab created by the PoC extension because otherwise the DevTools session gets terminated).
4. Open the console and run the following snippet:
DevToolsHost.sendMessageToEmbedder('{"id":1, "method": "save", "params": ["data:,url", "content", true]}');

Chromium 66.0.3330.0 from chromium-browser-continuous crashes after these steps.
The crash of my local ASAN build (Chromium 63.0.3239.84) is detected by ASAN; I have attached the symbolized ASAN trace.

Comment 5 by rob@robwu.nl, Jan 25

Here is the stack trace.

(and also a minimal extension for the manual test)
asan-manual-repro-63.0.3239.84.log
17.7 KB View Download
background.js
981 bytes View Download
manifest.json
294 bytes View Download

Comment 6 by rob@robwu.nl, Jan 26

Here's the ASAN report for Chromium 64.0.3282.119, built from source, using the repro from #4 and #5.

It is virtually identical to the one from 63.0.3239.84 (in comment 5).
asan-manual-repro-64.0.3282.119.log
14.8 KB View Download
Thanks for the report. 
That part of the code indeed looks problematic.
I could add add check in DevToolsFileHelper to see if the DevToolsDownloadDelegate has taken over, but that wouldn't prevent other code from calling 
DownloadPrefs::FromDownloadManager (which is the one that does the static cast) 

To be sure that this doesn't happen, I'm thinking on adding a isChromeDownloadDelegate method in the DownloadManagerDelegate class to protect all static casts

wdyt? 

Comment 8 by rob@robwu.nl, Jan 29

That suggestion looks like band-aid and not fix the root cause.

At the end of my initial report, I referred to some peculiarity. Namely that the DevTools API affects the whole BrowserContext (profile) instead of the inspected page. The sole purpose of DevToolsDownloadManagerDelegate is to override the download path via DevToolsDownloadManagerDelegate::DetermineDownloadTarget.

That only makes sense if a DevToolsDownloadManagerHelper is installed for the WebContents (i.e. when Page.setDownloadBehavior is called for a target). In all other cases the default (Chrome)DownloadManagerDelegate should be activated. Considering this, DownloadPrefs::FromDownloadManager can be rewritten to (recursively!!) use DevToolsDownloadManagerDelegate::proxy_download_delegate_ to ultimately retrieve a DownloadPrefs* if the class is not a ChromeDownloadManagerDelegate.

An alternative, much easier solution is to reject Page.setDownloadBehavior for non-headless commands. The primary purpose of this method is to support headless Chrome ( bug 696481 ), so that should not be impacted. Another advantage of this solution  bug 805445  will be fixed too.
Labels: Security_Severity-Medium Pri-1
I can repro with the steps in comment #4. Security-medium as this is a memory corruption that requires a specific extension to be installed.
Labels: Security_Impact-Stable
Owner: pfeldman@chromium.org
Project Member

Comment 12 by sheriffbot@chromium.org, Jan 30

Labels: M-64
Cc: dvallet@chromium.org
Cc: qin...@chromium.org
Project Member

Comment 15 by bugdroid1@chromium.org, Feb 1

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

commit cbb2c0940d4e3914ccd74f6466ff4cb9e50e0e86
Author: Pavel Feldman <pfeldman@chromium.org>
Date: Thu Feb 01 01:36:25 2018

Don't downcast DownloadManagerDelegate to ChromeDownloadManagerDelegate.

DownloadManager has public SetDelegate method and tests and or other subsystems
can install their own implementations of the delegate.

Bug:  805905 
Change-Id: Iecf1e0aceada0e1048bed1e2d2ceb29ca64295b8
TBR: tests updated to follow the API change.
Reviewed-on: https://chromium-review.googlesource.com/894702
Reviewed-by: David Vallet <dvallet@chromium.org>
Reviewed-by: Min Qin <qinmin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#533515}
[modify] https://crrev.com/cbb2c0940d4e3914ccd74f6466ff4cb9e50e0e86/chrome/browser/browsing_data/chrome_browsing_data_remover_delegate_unittest.cc
[modify] https://crrev.com/cbb2c0940d4e3914ccd74f6466ff4cb9e50e0e86/chrome/browser/download/download_core_service_impl.cc
[modify] https://crrev.com/cbb2c0940d4e3914ccd74f6466ff4cb9e50e0e86/chrome/browser/download/download_prefs.cc
[modify] https://crrev.com/cbb2c0940d4e3914ccd74f6466ff4cb9e50e0e86/chrome/browser/ui/webui/settings/downloads_handler_unittest.cc

Project Member

Comment 16 by sheriffbot@chromium.org, Feb 14

pfeldman: 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
pfeldman: Can this be closed now? Thanks.
Project Member

Comment 18 by sheriffbot@chromium.org, Feb 28

pfeldman: 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
Status: Fixed (was: Assigned)
Labels: -M-64 M-66
Project Member

Comment 21 by sheriffbot@chromium.org, Mar 6

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

Comment 23 by sheriffbot@chromium.org, Mar 16

Labels: Merge-Request-66
Project Member

Comment 24 by sheriffbot@chromium.org, Mar 16

Labels: -Merge-Request-66 Merge-Review-66 Hotlist-Merge-Review
This bug requires manual review: M66 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), josafat@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: -Hotlist-Merge-Review -M-66 -Merge-Review-66
This was fixed before M66 branch point.
Labels: -reward-topanel reward-unpaid reward-500
*** 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 eligible 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.
*********************************
The VRP panel decided to award $500 for this reporting, that since the delegate isn't attacker controlled, it seems like it might not be exploitable, coupled with the extension/devtools precondition. Happy to reconsider for a higher amount if exploitability can be shown.
Labels: -reward-unpaid reward-inprocess
Project Member

Comment 29 by sheriffbot@chromium.org, Jun 12

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: M-66
(note that even though this was fixed before M66 branch point, it should have retained the M-66 label)
Labels: Release-0-M66
Labels: CVE-2018-6151
Labels: CVE_description-missing

Sign in to add a comment