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

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.

David, mind taking a look?

@#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.

Here is the stack trace.

(and also a minimal extension for the manual test)

Deleted: asan-manual-repro-63.0.3239.84.log 17.7 KB

asan-manual-repro-63.0.3239.84.log 17.7 KB View Download

Deleted: background.js 981 bytes

background.js 981 bytes View Download

Deleted: manifest.json 294 bytes

manifest.json 294 bytes View Download

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).

Deleted: asan-manual-repro-64.0.3282.119.log 14.8 KB

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? 

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.

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.

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

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.

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

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

This was fixed before M66 branch point.

*** 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.

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

(note that even though this was fixed before M66 branch point, it should have retained the M-66 label)