New issue
Advanced search Search tips
Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: Bug-Security
Nag



Sign in to add a comment

Security: Android Chrome download files into arbitrary sdcard directory

Reported by dmitry.l...@gmail.com, Dec 17 2015

Issue description

VULNERABILITY DETAILS
Android Chrome can download files into arbitrary sdcard path instead of default download path "/sdcard/Download/". User won't be informed that file will be downloaded into arbitrary sdcard path.

VERSION
Chrome Version: 47.0.2526.83 stable from Google Play
Operating System: non rooted Android 6.0.1, M8974A-2.0.50.2.28

REPRODUCTION CASE
Server "download.php":
<?
	$name = "../chrome/default.txt";	
	header("Content-Type: application/octet-stream; name=\"readme.txt\"");	
	header("Content-Disposition: attachment; filename=\"".$name."\"");	
	echo "hello, world";
?>
Load url like "server/download.php". File will be downloaded into "/sdcard/chrome/" directory instead of "/sdcard/Download/".
I attach PoC with possible cases of file name (encoded, utf8 encoded). 

Video of PoC:
https://youtu.be/_CPra2qLqm0 (access only by link)

Looks like DownloadManager (http://developer.android.com/intl/ru/reference/android/app/DownloadManager.html) receives subpath argument with traversal path(which must be checked in the application)

 
poc.zip
837 bytes Download

Comment 1 by rsesek@chromium.org, Dec 17 2015

Labels: Cr-UI-Browser-Downloads Security_Impact-Stable Security_Severity-Medium M-49
Owner: qin...@chromium.org
Status: Assigned
qinmin: Could you please take a look at this issue?

Comment 2 by rsesek@chromium.org, Dec 17 2015

Labels: OS-Android
Project Member

Comment 3 by ClusterFuzz, Dec 18 2015

Labels: Pri-1
Project Member

Comment 4 by ClusterFuzz, Jan 8 2016

Labels: Nag
qinmin@: Uh oh! This issue is still open and hasn't been updated in the last 21 days. Since this is a serious security vulnerability, we want to make sure progress is happening. Can you update the bug with current status, and what, if anything, is blocking?

If you are not the right Owner for this bug, please find someone else to own it as soon as possible and remove yourself as Owner.

If the issue is already fixed or you are to unable to reproduce it, please close the bug. (And thanks for fixing the bug!).

These nags can be disabled by adding a 'WIP' label and an optional codereview link.

- Your friendly ClusterFuzz
Stable version the same and problem still exists.
I've checked Chrome Beta v 48.0.2564.71, Android 4.4.2, and problem reproduced too.
Hi, any news? Looks that this is not serious problem?
Project Member

Comment 7 by ClusterFuzz, Jan 29 2016

qinmin@: Uh oh! This issue is still open and hasn't been updated in the last 42 days. Since this is a serious security vulnerability, we want to make sure progress is happening. Can you update the bug with current status, and what, if anything, is blocking?

If you are not the right Owner for this bug, please find someone else to own it as soon as possible and remove yourself as Owner.

If the issue is already fixed or you are to unable to reproduce it, please close the bug. (And thanks for fixing the bug!).

These nags can be disabled by adding a 'WIP' label and an optional codereview link.

- Your friendly ClusterFuzz

Comment 8 by raymes@chromium.org, Feb 18 2016

Cc: asanka@chromium.org klo...@chromium.org tedc...@chromium.org mmenke@chromium.org
qinmin@ - are you able to take a look at this? 

I wonder if this is related to: https://code.google.com/p/chromium/issues/detail?id=586657 ?


Comment 9 by mmenke@chromium.org, Feb 18 2016

Hrm...  586657 is about FileURLToFilePath, which is solely for decoding local paths for file URLs.  I believe GenerateSafeFileName, GetSuggestedFilename, and GenerateSafeFileName all have protection against this (Checked some of them as a part of that bug, at least their handling of the URL itself).
qinmin: InterceptDownloadResourceThrottle doesn't do any sanitization of the filename extracted from the Content-Disposition header. :-(
Looks like we pass the raw arguments (Content-Disposition, url) to Java, and then have some Java code that picks the file name:  https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/java/src/org/chromium/chrome/browser/download/ChromeDownloadDelegate.java&q=requestHttpGetDownload&sq=package:chromium&type=cs&l=231

Instead, it should use net's methods to get the file name, and just pass that to Java instead.
Seems like we could add it here:
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/android/download_controller_android_impl.cc&l=394

Is that path used both when we use the chrome download stack and the android one?  Do we have to handle those separately?
#12: Chrome downloads stack already has sanitization for filenames.
So the java side uses URLUtil.guessFileName(String url, String contentDisposition, String mimeType), that function doesn't seem to provide sanitization
StartAndroidDownloadInternal just uses net::HttpContentDisposition to extract the filename from the Content-Disposition header. This method just returns the decoded filename received from the network without any sanitization. It then passes this filename through to DownloadController.newHttpGetDownload(). The filename then passes through the stack as a member of the DownloadInfo structure. I don't see it getting sanitized at any point from there on.

Where is ChromeDownloadDelegate.fileName() called?
it is called if the content disposition don't have filename specified.

A simple fix is to call net::GetSuggestedFilename() before passing the download to java side, that should give us a non-null file name and it should be sanitized 
#16: Sure, but whether URLUtil.guessFileName() provides sanitization doesn't matter when the attack vector is the Content-Disposition header.

I think in addition to deterministically generating a filename in native code, we should eliminate the guessFileName() call and associated logic on the Java side. That way it's not confusing where the filename is coming from and whether it's sanitized.

Yes, that's what I am planning to do. 
uploaded https://codereview.chromium.org/1717783002/ to fix the problem
Project Member

Comment 19 by bugdroid1@chromium.org, Feb 25 2016

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

commit 168f723d0f0ce60d92a6307c754181f6d8644583
Author: qinmin <qinmin@chromium.org>
Date: Thu Feb 25 01:10:38 2016

Fix an issue that download filename from content disposition is not sanitized

The filename from content disposition may not be sanitized.
This change uses net::GetSuggestedFileName() to sanitize the filename
before passing it to java.
Also, there is no need to call URLUtil.guessFileName() since
GetSuggestedFileName() will always return a non-null value.

BUG= 570750 

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

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

[modify] https://crrev.com/168f723d0f0ce60d92a6307c754181f6d8644583/chrome/android/java/src/org/chromium/chrome/browser/download/ChromeDownloadDelegate.java
[modify] https://crrev.com/168f723d0f0ce60d92a6307c754181f6d8644583/chrome/android/javatests/src/org/chromium/chrome/browser/download/ChromeDownloadDelegateTest.java
[modify] https://crrev.com/168f723d0f0ce60d92a6307c754181f6d8644583/chrome/browser/android/download/download_manager_service.cc
[modify] https://crrev.com/168f723d0f0ce60d92a6307c754181f6d8644583/content/browser/android/download_controller_android_impl.cc
[modify] https://crrev.com/168f723d0f0ce60d92a6307c754181f6d8644583/content/browser/android/download_controller_android_impl.h
[modify] https://crrev.com/168f723d0f0ce60d92a6307c754181f6d8644583/content/public/browser/android/download_controller_android.h

Labels: Merge-Request-49
Status: Started (was: Assigned)

Comment 21 by tin...@google.com, Feb 25 2016

Labels: -Merge-Request-49 Merge-Review-49 Hotlist-Merge-Review
[Automated comment] Less than 2 weeks to go before stable on M49, manual review required.

Comment 22 by k...@google.com, Mar 9 2016

Labels: -M-49 -Merge-Review-49 M-50
49 is wrapped, moving to 50.
Project Member

Comment 23 by ClusterFuzz, Mar 10 2016

Status: Fixed (was: Started)
Please mark security bugs as fixed as soon as the fix lands, and before requesting merges.

- Your friendly ClusterFuzz
Project Member

Comment 24 by ClusterFuzz, Mar 10 2016

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Labels: reward-topanel Release-0-M50
Labels: CVE-2016-1656
Thanks for the report. How would you like to be credited when we mention this in Chrome's release notes?
Hi. Dzmitry Lukyanenko. If possible to add links than I would like to provide my LinkedIn: www.linkedin.com/in/dzima

Thanks:)
Labels: -reward-topanel reward-unpaid Reward-500
Thanks again for the report! This one qualified for a $500 reward.
Labels: -reward-unpaid reward-inprocess
Project Member

Comment 31 by sheriffbot@chromium.org, Jun 17 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-500 -Hotlist-Merge-review reward-500
Project Member

Comment 33 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 34 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
Labels: CVE_description-submitted

Sign in to add a comment