https://github.com/brave/browser-laptop/issues/10644#issuecomment-324467012 notes that this "attack" generally does not work in Chrome.

In Chrome, only a File URL is allowed to request another File URL, which means that this behavior is only possible if you're viewing a HTML file that has already been downloaded.

Are you able to reproduce this from a non-local HTML page?

Thanks for your report. There is a comment in the GitHub issue you mentioned:

"In chrome browser the file download is blocked here.
https://cs.chromium.org/chromium/src/content/browser/child_process_security_policy_impl.cc?gsn=CanRequestURL&l=226
it returns false here and hence its blocked"

link to the comment: https://github.com/brave/browser-laptop/issues/10644#issuecomment-324467012

I've just verified, and that appears to be true in Version 65.0.3325.181 (Official Build) (64-bit):

poc.html:1 Not allowed to load local resource: file:///C:/Windows/System32/Drivers/etc/hosts


You can download local files when you open a page via file:// protocol. However, if you try serving html page via HTTP (e.g. python -m SimpleHTTPServer), you'll see that download is blocked.

This bug came up in the chain of another bug, so let's reopen. It might be worthwhile fixing. 

dtrainor: Could you please take a look and reassign if necessary? Thanks.

Bumping this to Pri-1 to avoid priority inversion: it's blocking  issue 848123 , which is Pri-1 and Medium severity.

Asanka is there any historical discussion on this for why we allow file downloads from other files?  Could we just do something like always prompt in this scenario?

There appears to be a real bug somewhere in here were we honor the suggested filename in a download attribute in an anchor where the document and the target are both file:// URLs. This is incorrect since file:// to file:// is considered cross origin. Let's make sure that issue is addressed.

Regarding downloading of file:// URLs, we've considered blocking it entirely but instead resigned into blocking non-file:// schemes from downloading file:// schemes. file:// downloading file:// is still allowed and is a side-effect of file:// being allowed to navigate to file://. No additional work, as I recall, was done to specifically disable downloading in this case.

One thing that was considered, but not implemented or discussed further was to make file:// URL downloads trivially succeed. I.e. downloading a file:// resource just creates a DownloadItem with a FullPath corresponding to the file:// URL. The target would then be available on the download shelf for the user's convenience, but Chrome won't copy the file to the downloads folder, nor attempt to rename anything. This would preserve use cases like some enterprise having documentation in HTML that points to -- say an archive or some other random document -- elsewhere in a network drive.

Another thing we would like to consider is blocking local file:// URLs from navigating or downloading remote file:// URLs. That's not been explored yet, and the security wins there are also debatable.

+jochen@ it looks like this CL (https://chromium-review.googlesource.com/#/c/847594/) should in theory have blocked the download attribute if we treated file:// to file:// as cross origin.  jochen@ do you know where the right place in the stack is to detect/flag the cross origin state?

Related bug:  714373 

I'm using this method in the renderer: https://cs.chromium.org/chromium/src/third_party/blink/renderer/platform/weborigin/security_origin.h?rcl=a2a5386474feb62d5b8a84244b8657f09209fb22&l=125

jochen@ do you know why we set the suggested filename regardless of whether or not it's a cross origin request?  Alt clicks still download through another path and the suggested filename gets used there.  Is that expected?

hum, yeah, that's kinda odd.

i'll put it on my backlog of things to look at

i suspect that something creates a ResourceRequest with the suggested filename and passes that to DownloadURL(). fixing would require location that, and stripping the suggested filename.

we could also just block downloading non-web files from web origins..

Yeah it looks like frame_loader.cc calls DownloadURL with the ResourceRequest.  Is there a reason we can't just move setting the suggested filename into the if block that checks if we can read the content?

i think that makes sense

Hi Team, Thank you for looking into this issue again :)

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

commit 280c895af0a1d870df902bf06e2fc5c44036421a
Author: Jochen Eisinger <jochen@chromium.org>
Date: Tue Jun 26 07:20:59 2018

Ignore suggested filename for URLs we can't download

BUG= 827083 
R=dtrainor@chromium.org

Change-Id: Iffaa3adff0bb82cdeabf9af70a5952e6a50c7079
Reviewed-on: https://chromium-review.googlesource.com/1113447
Reviewed-by: David Trainor <dtrainor@chromium.org>
Commit-Queue: Jochen Eisinger <jochen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570342}
[modify] https://crrev.com/280c895af0a1d870df902bf06e2fc5c44036421a/third_party/blink/renderer/core/html/html_anchor_element.cc

Able to reproduce this issue on Windows 10, Mac OS 10.13.3 and Ubuntu 17.10 on the reported version 65.0.3325.181 and the issue is fixed on the latest Canary 69.0.3474.0.
On clicking on the Download Local file link, can observe that the file is not downloaded.
Attached is the screen cast for reference.

Hence adding TE verified labels as the fix is working as intended.

Thanks..

Deleted: 827083-M69.mp4 325 KB

	827083-M69.mp4 325 KB View Download

This bug requires manual review: M68 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), kariahda@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Why is this critical for M68? My recommendation is to wait until M69 if this is not absolutely critical, since we only have 2 more M68 beta cycles remaining. 

Critical is a strong word... I'm fine either way

Since the security severity is low, I suggest waiting for M69.

Let's wait until M69. Thanks for the fix and raising this! Please re-add merge-request label if you think we should reconsider it for M68. Thanks!

Thank you team for the fix and re-considering this :)

dtrainor, jochen: I think this should be merged to M68 as this fix will break the attack chain on  Issue 848123 . If this needs more time on canary then maybe the change mentioned on #31 should be merged instead as it has been on canary for some time and will also break the attack chain.

meacer@ can you look at comments 20-26 please?

sorry for not updating this issue. meacer@ agreed that we shouldn't merge