Project: chromium Issues People Development process History Sign in
New issue
Advanced search Search tips
Starred by 2 users
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 Back to list
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 clusterf...@chromium.org, Dec 18 2015
Labels: Pri-1
Project Member Comment 4 by clusterf...@chromium.org, 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 clusterf...@chromium.org, 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
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 clusterf...@chromium.org, Mar 10 2016
Status: Fixed
Please mark security bugs as fixed as soon as the fix lands, and before requesting merges.

- Your friendly ClusterFuzz
Project Member Comment 24 by clusterf...@chromium.org, 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
Sign in to add a comment