New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 586657 link

Starred by 1 user

Issue metadata

Status: Fixed
Closed: Feb 2016
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, Feb 12 2016

Issue description

Steps to reproduce the problem:
On Android, in the Chrome browser, navigate to <file:///sdcard/..%2f..%2f../data/data/>.

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.
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, Feb 13 2016

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, Feb 13 2016

Labels: M-50
Status: Available
Project Member

Comment 4 by ClusterFuzz, Feb 13 2016

Labels: -Pri-2 Pri-1

Comment 5 by, Feb 15 2016

Status: Assigned
On the network side, assigning to rsleevi for triage.
Hoping mmenke@ can triage as I'm at an event this week.

Comment 7 by, Feb 15 2016

file:///sdcard/../..%2f../data/data/ is normalized to file:///..%2f../data/data/, 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, 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, Feb 16 2016

[+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.

Project Member

Comment 14 by, Feb 23 2016

The following revision refers to this bug:

commit 30408ae67a9f6aea074b2883ba861613f52bd246
Author: mmenke <>
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:

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:

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


Status: Fixed (was: Assigned)
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, 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.
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 ClusterFuzz, 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, 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 - Your friendly Sheriffbot
Labels: -reward-unpaid reward-inprocess
Project Member

Comment 26 by, Oct 1 2016

This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit - Your friendly Sheriffbot
Project Member

Comment 27 by, Oct 2 2016

This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit - Your friendly Sheriffbot
Labels: allpublic
Labels: CVE_description-submitted

Sign in to add a comment