1504159 - "Save Image As.." doesn't work for mixed content images

Status: VERIFIED FIXED

(Core :: DOM: Security, defect)

Description

[heidi]

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:62.0) Gecko/20100101 Firefox/62.0

Steps to reproduce:

In Firefox 63, saving images with right-click consistently does not work on some sites. Clicking "retry" under about:download successfully downloads the file. Rolling back to a previous version of Firefox resolves the issue.

To reproduce on a specific site:

1. Visit any image page on https://e-hentai.org/, such as https://e-hentai.org/s/872cd95d14/1308601-1 (warning: link is SFW but site is not)

2. Right-click image, select "Save Image As...", then save to some location.


Actual results:

File is not saved and shows up as "Failed" under about:download. Clicking "retry" will successfully download the file. A mixed-content warning is displayed in the console for the initial pageload, but no errors are logged for the actual download.


Expected results:

File should be saved successfully.

Comment 1

[Kershaw Chang [:kershaw]]

I saw NS_ERROR_CONTENT_BLOCKED returned from nsContentSecurityManager::doContentSecurityCheck.
I think this has something todo with mixed content, since the image's scheme is http, not https.

The interesting thing is that I can download the image by chrome.

Comment 2

[Daniel Veditz [:dveditz]]

Looks like downloaded files are using load type TYPE_OTHER, which is blocked by the mixed content blocker unlike images which are allowed

https://searchfox.org/mozilla-central/source/dom/webbrowserpersist/nsWebBrowserPersist.cpp#1361

There's probably a bunch of things we _do_ want to block for TYPE_OTHER so maybe we need a new specific TYPE_DOWNLOAD type for this? If the user explicitly asks for it we should make it work. Probably got broken by some of the triggeringPrincipal work (like bug 1469916 perhaps, according to jkt).


A workaround is to use the context menu to View Image, and then save it from there. But folks will not be happy at the extra work.

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

Probably we should just use TYPE_SAVEAS_DOWNLOAD in nsWebBrowserPersist.cpp

Comment 5

[:Gijs (he/him)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #3)
> Probably we should just use TYPE_SAVEAS_DOWNLOAD in nsWebBrowserPersist.cpp

If we always use that, won't that lead to things that are normally blocked by mixed content blocking or other mechanisms being allowed?

Seems like in theory:

1) toplevel callers need to provide the correct type (in this case, that'd be the 'save image as' implementation), or maybe the toplevel should default to the TYPE_SAVEAS_DOWNLOAD type
2) for all subresources, we should store the correct type when we parse it out of the document (so when we save html, complete, and download an image, we should be using the TYPE_IMAGE or whatever type that is, and TYPE_SCRIPT for JS, etc. etc.)

Not sure how straightforward that'd be to implement. What's the risk of the bandaid TYPE_SAVEAS_DOWNLOAD here, and/or is there a better bandaid? Maybe TYPE_SAVEAS_DOWNLOAD for the top request, and keep TYPE_OTHER for all the subresources for now, assuming that's easier? Christoph, thoughts?


(In reply to heidi from comment #0)
> 1. Visit any image page on https://e-hentai.org/, such as
> https://e-hentai.org/s/872cd95d14/1308601-1 (warning: link is SFW but site
> is not)

FTR, without an ad-blocker that link is also not SFW, the link in the dupe (bug 1504609) is probably better for verification and testing.

Though in any case we should probably add an automated test for this.

Comment 6

[Jonathan Kingston [:jkt] he/him]

> Not sure how straightforward that'd be to implement.

Yeah when we triaged this last night, it seemed like there are a lot of codepaths here which would suffer from a similar problem as the triggeringPrincipal work where it's turtles all the way up.

Perhaps the function could initially take an optional type argument that the "save as" interface could pass and when the type isn't passed it would revert back to the safer TYPE_OTHER existing default.

This way we would have a secure interim and be able to fix individual cases as they are noticed. I'm a little concerned we are using these functions within content and an overly permissive change might introduce some mixed content blocker escapes.

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

(In reply to :Gijs (he/him) from comment #5)
> (In reply to Christoph Kerschbaumer [:ckerschb] from comment #3)
> > Probably we should just use TYPE_SAVEAS_DOWNLOAD in nsWebBrowserPersist.cpp
> 
> If we always use that, won't that lead to things that are normally blocked
> by mixed content blocking or other mechanisms being allowed?

So, before Bug 1469916, we used the systemPrincipal as the triggeringPrincipal which bypassed all of the security checks anyway. Using TYPE_SAVEAS_DOWNLOAD could work as a temporary workaround which seems small enough to be uplifted. Ultimately we should file a follow up and pass the right content type and us that type for loading the channel.

Reasonable?

Comment 8

[Jonathan Kingston [:jkt] he/him]

> I'm a little concerned we are using these functions within content and an overly permissive change might introduce some mixed content blocker escapes.

Just to clarify, I don't actually know if we use this function within content however verifying this isn't the case isn't a quick task.
I think Christoph has a fair point that it's certainly better than what was there before.

Comment 9

[heidi]

> FTR, without an ad-blocker that link is also not SFW, the link in the dupe
> (bug 1504609) is probably better for verification and testing.

Sorry about that. It doesn't look like I can edit the initial comment for this report myself, but you can append ?noads=1 to the URL to disable the ad-loading script. https://e-hentai.org/s/872cd95d14/1308601-1?noads=1 should be actually work-safe.

Comment 11

[:Gijs (he/him)]

[Tracking Requested - why for this release]:
Apparently this breaks direct downloads from google images (for http: images).

Comment 12

[:Gijs (he/him)]

Attachment: Bug 1504159 - use TYPE_SAVEAS_DOWNLOAD for data saved through nsIWebBrowserPersist, r?ckerschb

Comment 13

[:Gijs (he/him)]

Attachment: Bug 1504159 - add test to verify we can save a mixed content image from the context menu, r?jkt

Depends on D11411

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/93217373996c
use TYPE_SAVEAS_DOWNLOAD for data saved through nsIWebBrowserPersist, r=jkt
https://hg.mozilla.org/integration/autoland/rev/abe810b08dc6
add test to verify we can save a mixed content image from the context menu, r=jkt

Comment 15

[:Gijs (he/him)]

Comment on attachment 9023823 [details]
Bug 1504159 - use TYPE_SAVEAS_DOWNLOAD for data saved through nsIWebBrowserPersist, r?ckerschb

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1469916

User impact if declined: The context menu to save images (and possibly other content like videos) is broken for mixed content images/media/...

This affects http (not https) results in google images

User Workaround: click the 'retry' button in the downloads panel for the failed download that appears there.

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: No

Needs manual test from QE?: Yes

If yes, steps to reproduce: See bug 1505879

List of other uplifts needed: n/a

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Single line change that makes downloads more permissive than they are today, but keeps them less permissive than they were prior to bug 1469916.

String changes made/needed: none

Comment 16

[:Gijs (he/him)]

(approval request for both patches)

Comment 17

[:Gijs (he/him)]

(In reply to :Gijs (he/him) from comment #15)
> Is this code covered by automated tests?: Yes

Just to be clearer here (the newfangled form for these requests doesn't allow for a more elaborate answer...):

The patches include an automated test for this specific issue.

However, I would say overall test coverage of our "save thing X from web to disk" for various values of X is abysmal. It exists, but when I landed bug 1469916 there were several crashes that went undetected by automated tests, and now there's both this (+ dupes) and the local pdf re-saving issue (bug 1502448) that were only found when this went to release.

While this change doesn't touch any public APIs and therefore I can't see how it'd cause crashes (unlike bug 1469916), I'd be on the fence about this going into release - it's a trivial change, but also this code is not that well-understood or tested in general, even if I'm adding tests as I'm going along and fixing various aspects of it. There's also a workaround, and we're about a month from the 64 release, and I'm not sure I'm convinced this issue alone is worth spinning a(nother) dot release for. But I figured I'd give relman the opportunity to make a final call. Hopefully this discussion helps making that call.

Comment 18

[Julien Cristau [:jcristau]]

Comment on attachment 9023823 [details]
Bug 1504159 - use TYPE_SAVEAS_DOWNLOAD for data saved through nsIWebBrowserPersist, r?ckerschb

approved for 64.0b9, thanks

Comment 19

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/93217373996c
https://hg.mozilla.org/mozilla-central/rev/abe810b08dc6

Comment 20

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a9a646d4a158
https://hg.mozilla.org/releases/mozilla-beta/rev/1c5b308487dc

Comment 21

[Catalin Sasca, Desktop Test Engineering [:csasca]]

I successfully reproduced the issue on Firefox Nightly 65.0a1 (2018-11-02) under Windows 10 (x64) using the STR from Comment 0.

The issue is no longer reproducible on latest Nightly 65.0a1 (2018-11-12) and Firefox Beta 64.0b9 under Windows 10 (x64), Ubuntu 18.04 (x64) and macOS 10.12.

Comment 22

[Pascal Chevrel:pascalc]

Comment on attachment 9023823 [details]
Bug 1504159 - use TYPE_SAVEAS_DOWNLOAD for data saved through nsIWebBrowserPersist, r?ckerschb

Looking at SUMO reports, feedback on social media and end user forums for Firefox in the languages I understand, I didn't find complaints from users about this bug. There is also a simple workaround easily discoverable by the end users which is clicking on the reload icon next to the failed download line in the download panel. Given that Gijs is expressing concerns about the quality of our tests for the code handling file downloads, that it just got uplifted to beta and that 64 is in 4 weeks, I will not take the patch into our next dot release as I prefer to have a well known regression with a limited scope and easily discoverable workarounds than potentially causing new regressions related to file handling, thanks.