Project: chromium Issues People Development process History Sign in
New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 1 user
Status: Fixed
Owner:
Closed: Feb 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android, Chrome
Pri: 1
Type: Bug-Security



Sign in to add a comment
Directory traversal on file:// via escaped slashes
Reported by jannhorn@googlemail.com, Feb 12 2016 Back to list
Steps to reproduce the problem:
On Android, in the Chrome browser, navigate to <file:///sdcard/..%2f..%2f../data/data/com.android.chrome/app_chrome/Default/>.

What is the expected behavior?
The request should either fail because a slash can't be part of a directory or file name or be blocked by ChromeNetworkDelegate::OnCanAccessFile() because the resolved path is outside /sdcard and /mnt/sdcard.

What went wrong?
The request succeeds and a file listing is shown.

FileProtocolHandler::MaybeCreateJob() first calls FileURLToFilePath(), which decodes the encoded slashes (%2f) using UnescapeURLComponent() and collapses consecutive slashes, but doesn't normalize the path or check it for directory traversal again. FileProtocolHandler::MaybeCreateJob() then performs the path whitelisting check using network_delegate->CanAccessFile(), which calls ChromeNetworkDelegate::OnCanAccessFile(). OnCanAccessFile() compares the path to the whitelisted directories using the IsParent() function, but again without normalizing the path first - and as the documentation of IsParent() says: "Does not convert paths to absolute, follow symlinks or directory navigation (e.g. "..")."

Did this work before? N/A 

Chrome version: 48.0.2564.95  Channel: n/a
OS Version: 6.0.1
Flash Version: 

Actually accessing the files doesn't seem to work directly - I'm not yet sure why or whether this can be circumvented.

The same mechanism is also used on Chrome OS to restrict file:// access to a few directories, so it might be interesting to look into what behavior this causes on Chrome OS.

I don't think I have demonstrated significant security impact in this report, but I'm filing it as a security bug in case someone figures out how to actually grab files with this.
 
Screenshot_20160213-000015.png
261 KB View Download
Well, I don't have a Chromebook, but in some ages-old Chromium OS ISO build I installed, this works. So it might be more relevant for Chromium OS than for Android.
Comment 2 by rickyz@chromium.org, Feb 13 2016
Cc: dcheng@chromium.org
Labels: Cr-Internals-Network OS-Chrome Security_Severity-Medium Security_Impact-Stable
I marked this as medium out of caution for now, since I do not know how much we rely on file:/// whitelisting for security after a renderer has been compromised.

Adding dcheng@, who might be more familiar.
Comment 3 by rickyz@chromium.org, Feb 13 2016
Labels: M-50
Status: Available
Project Member Comment 4 by clusterf...@chromium.org, Feb 13 2016
Labels: -Pri-2 Pri-1
Comment 5 by raymes@chromium.org, Feb 15 2016
Owner: rsleevi@chromium.org
Status: Assigned
On the network side, assigning to rsleevi for triage.
Cc: rsleevi@chromium.org
Owner: mmenke@chromium.org
Hoping mmenke@ can triage as I'm at an event this week.
Comment 7 by mmenke@chromium.org, Feb 15 2016
file:///sdcard/../..%2f../data/data/com.android.chrome/app_chrome/Default/ is normalized to file:///..%2f../data/data/com.android.chrome/app_chrome/Default/, so presumably the issue is with the normalization logic in FileURLToFilePath() - presumably we should either not unescape %2f, or we should unescape it before path normalization.

I'll dig into it more tomorrow.
Comment 8 by mmenke@chromium.org, Feb 16 2016
So GURL's constructor collapses "foo/../" in URLs somewhere in url::DoPath, long before the network stack gets a shot at urls.  It also collapses things like "foo/..\".  However, it leaves %2f and %5c alone, escaped.  FileURLToFilePath, on the other hand, unescapes these to references to parent directory.  However, on other desktop platforms, URLRequestFileJob creates a FileStream::Context, which runs a FilePath::ReferencesParent check, and then fails the request because that returns true.

On Android, however, file_stream_context has conditionally compiled code that skips this check.

I think we should do two things:

1)  Fix file_stream_context on Android.
2)  Make FileURLToFilePath and GURL consistent in terms of unescaping %2f and %5c in file URLs.  It looks to me like both FireFox and IE unescape those characters in file URLs, so I'd tend to suggest we go that route in GURL (Though all else being equal, I'd prefer to keep them escaped).
Comment 9 by mmenke@chromium.org, Feb 16 2016
Cc: qin...@chromium.org
[+qinmin]:  Looks like you landed the code that introduced the bug in file_stream_context.h.  Care to take a swing at fixing it?

I'm happy to take a swing at GURL changes (Which will independently fix this specific bug, but I suspect there are other problems with not doing the parent check).
On desktop chrome, on all platforms, file URLs that are directories are *also* missing this check.  Think it makes sense to move a check like this to a higher level, to catch all problems here....  Which I should probably be the one to do, rather than qinmin.  I'm not sure if we need a similar check for content:// URLs on Android.
Content:// URLs doesn't suffer from this as the URI will need to go through content resolver.

Cc: eroman@chromium.org
Cc: brettw@chromium.org abarth@chromium.org mkwst@chromium.org
Project Member Comment 14 by bugdroid1@chromium.org, Feb 23 2016
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/30408ae67a9f6aea074b2883ba861613f52bd246

commit 30408ae67a9f6aea074b2883ba861613f52bd246
Author: mmenke <mmenke@chromium.org>
Date: Tue Feb 23 17:28:17 2016

FileURLToFilePath:  Don't unescape '/' and '\\'.

GURL leaves these escaped, and unescaping them in paths changes the
meaning of the path.

Added two values to the UnescapeRule enumeration:
PATH_SEPARATORS and URL_SPECIAL_CHARS_EXCEPT_PATH_SEPARATORS.

In followup CLs, I intend to replace all uses of URL_SPECIAL_CHARS,
in favor of one or both the two new values, and eventually remove
the value, as it's easily to use in an unsafe manner.

BUG= 586657 

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

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

[modify] https://crrev.com/30408ae67a9f6aea074b2883ba861613f52bd246/net/base/escape.cc
[modify] https://crrev.com/30408ae67a9f6aea074b2883ba861613f52bd246/net/base/escape.h
[modify] https://crrev.com/30408ae67a9f6aea074b2883ba861613f52bd246/net/base/escape_unittest.cc
[modify] https://crrev.com/30408ae67a9f6aea074b2883ba861613f52bd246/net/base/filename_util.cc
[modify] https://crrev.com/30408ae67a9f6aea074b2883ba861613f52bd246/net/base/filename_util_unittest.cc

Status: Fixed
I'll let the fix bake a couple days, and then request a merge.
Labels: Merge-Request-49
Actually, not that far away from release, and this is labelled as a medium severity security issue, so I'll just request the merge now.
Comment 17 by shey...@google.com, Feb 23 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.
Cc: sshruthi@chromium.org
Labels: reward-topanel
Shruthi - can you please review this merge request? We'd like to get this in with the final beta tomorrow.
Labels: -Hotlist-Merge-review -Merge-Review-49 Merge-Triage
Removing merge request - don't want this to land right in the middle of some android stuff. Adding Merge-Triage to revisit this.
Project Member Comment 20 by clusterf...@chromium.org, Mar 10 2016
Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Labels: -Merge-Triage merge-na Release-3-M50
This actually shipped with M50 stable based on revision number, so let's note this with 3-M50 notes so Jann get's his credit syndicated.
Comment 22 Deleted
Congratulations - $500 for this report! (Deleted comment was me copypasting the wrong panel notes to this bug) 
Project Member Comment 24 by sheriffbot@chromium.org, Jun 1 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-unpaid reward-inprocess
Project Member Comment 26 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 27 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