1403814 - Block toplevel data: URI navigations only if openend in the browser

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P1)

Description

[zdnex]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:55.0) Gecko/20100101 Firefox/55.0
Build ID: 20170824053622

Steps to reproduce:

1) Allow opening of popups on page
2) Open console and write simple one liner which should create CSV file

window.open('data:text/csv;base64,' + btoa(unescape(encodeURIComponent('my text;with;values'))));
3) Observe if it is saved and presented to user


Actual results:

In current 57 this is not the case > it opens empty blocked page leading user to think that it did not produce any result


Expected results:

It should save file like in happens in FF55 or  Chrome

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

I think Chrome is blocking that as well because they block any toplevel data: URI navigations. We can discuss if we potentially want to allow data:text/csv.

Comment 2

[zdnex]

I am not sure why you think that Chrome is blocking this, because it normally works in Chrome I have tried it before and also now on 62.0.3202.38. This is used on internal company IS for getting reports in CSV from HTML <table>

You can directly see it used by several users on https://stackoverflow.com/questions/14964035/how-to-export-javascript-array-info-to-csv-on-client-side

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

(In reply to zdnex from comment #2)
> I am not sure why you think that Chrome is blocking this

Because Chrome is blocking all toplevel data: URI navigations. The reason this works in Chrome is because they don't navigate to it, they open 'save as' dialog prompt. SO I assume the reason it works is because they go down a different code path, similar to Bug 1403641.

Comment 4

[Kohei Yoshino]

We could probably allow all base64-encoded data URLs regardless of the MIME type?

Comment 5

[zdnex]

I guess that if base64 is allowed for every content type it will solve some usecases. Not sure if every normally used one as json is most of time not encoded for example.

Comment 6

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Same reason as mentioned https://bugzilla.mozilla.org/show_bug.cgi?id=1403641#c13

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1403814_allow_data_text_csv.patch

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1403814_test_data_text_csv.patch

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Could you explain why we want this, and why only text/csv and not also some other text/*?

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

I kind of like comment 4. And only allowing text/csv feels a bit hacky.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8917556 [details] [diff] [review]
bug_1403814_allow_data_text_csv.patch

Ask review again if you explain why special case text/csv.
Are there other cases which should let the browser to trigger download UI?

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8917557 [details] [diff] [review]
bug_1403814_test_data_text_csv.patch

r+, and this could be extended to support other mimetypes too.

Comment 13

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8917556 [details] [diff] [review]
bug_1403814_allow_data_text_csv.patch

(In reply to Olli Pettay [:smaug] from comment #11)
> Ask review again if you explain why special case text/csv.
> Are there other cases which should let the browser to trigger download UI?

Sorry for not mentioning this the first time around. Based on the bug filed it seems there is a usecase to allow data:text/csv. Since I am mostly worried about phishing attacks and since data:text/csv doesn't seem to be troublesome to me, I think we can allow that usecase. Please note that we also allow data: URI downloads in general (see bug 1403641).

Ultimately, I am not sure if we want to allow more data: URI navigations or evaluate on a per usecase basis, like for this bug.

What do you think?

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Should we allow all the data: loads which won't be loaded in browser?

Comment 15

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Olli Pettay [:smaug] from comment #14)
> Should we allow all the data: loads which won't be loaded in browser?

I would be fine with that. In fact, I think that makes sense. How would we do that? What is the indicator?

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8917556 [details] [diff] [review]
bug_1403814_allow_data_text_csv.patch

I think we really should fix this in more generic way. We don't know for what all data: is being used.
I don't know immediately recall a simple call whether browser may handle certain mimetype (in which case we could block it, unless it is image, or I guess pdf).

Or should we black list? Would it be enough to blacklist text/html and other html/svg mimetypes, since those are the ones which may run scripts.

Comment 17

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Olli Pettay [:smaug] from comment #16)
> Or should we black list? Would it be enough to blacklist text/html and other
> html/svg mimetypes, since those are the ones which may run scripts.

I think we could do a blacklist. We definitely want to block:
* text/html
* text/svg
* image/svg

And that would then also allow application/json and friends.

Comment 18

[Christoph Kerschbaumer [:ckerschb]]

I filed Bug 1408400 to consider the blacklist approach.

Comment 19

[Christoph Kerschbaumer [:ckerschb]]

Hey Smaug, given the discussion within Bug 1408400 [1] it seems that blacklisting is hard (or at least more time consuming). Can we whitelist data:text/csv within this bug for now?

Asked differently, how would you like to move forward with blocking data URI navigations? Is it worth adding such an API as suggested with [1] so we can add a blacklist? I would rather whitelist excemptions as we go along. What do you think?

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1408400#c2

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Why you think whitelisting csv is enough? Can't there be any random mimetypes similarly broken?
If we use whitelisting, the setup becomes rather surprising: some mimetypes which the browser doesn't handle itself can be used, but not others.

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

Is there some reason to go with the whitelisting approach?

FWIW, I am currently rather worried that blocking data: on top level navigations will break real world sites.

What does Chrome do?

Comment 22

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Olli Pettay [:smaug] (work week Oct 16-20) from comment #20)
> Why you think whitelisting csv is enough? Can't there be any random
> mimetypes similarly broken?

There might be mimetypes that are similarly broken. Since we are using a whitelist approach we are adding one after the other as we encounter problems. Please note that we already allow all 'downloads' of data: URIs (see Bug 1403641). But agreed, that does not account for mime types that the browser can't handle.

> If we use whitelisting, the setup becomes rather surprising: some mimetypes
> which the browser doesn't handle itself can be used, but not others.

I agree, but blacklisting seems hard. If you could suggest a way forward I am happy to implement a blacklist. The reason it's easier for Chrome is that they have different codepaths for things the browser is going to display and things that will trigger the download panel. In Firefox we don't know that within docshell. Potentially we could move the check to a different location in the code?

Comment 23

[Boris Zbarsky [:bzbarsky]]

Fundamentally, Chrome is doing their check _after_ they "get the response", while you're doing it before you "send the request".

If you want to do the check after we "get the response", that's certainly doable.

Comment 24

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #23)
> If you want to do the check after we "get the response", that's certainly
> doable.

I guess my main question is: where do we decide whether to load a data URI in the browser or whether we prompt the download panel? Maybe it makes more sense to do the blocking later if we can do it more accurately at a different place in the code.

Comment 25

[Boris Zbarsky [:bzbarsky]]

That decision is made via the logic in nsURILoader and the things it calls.

So for example, you could have nsDSURIContentListener::DoContent do whatever checks you want and set *aAbortProcess to true as needed (and cancel aRequest; see the existing code that sets *aAbortProcess to true).

Comment 26

[Boris Zbarsky [:bzbarsky]]

And to be clear, nsDSURIContentListener::DoContent is the point where we have decided that we'd doing the load in a docshell and decided what docshell we're doing the load in, so now we're telling it to do the load.

Comment 27

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #26)
> And to be clear, nsDSURIContentListener::DoContent is the point where we
> have decided that we'd doing the load in a docshell and decided what
> docshell we're doing the load in, so now we're telling it to do the load.

Thanks for the explanation Boris. Overall that makes sense to me. Changing title of this bug and also marking Bug 1408400 (blacklist approach) as invalid.

Comment 28

[Christoph Kerschbaumer [:ckerschb]]

I guess there is still Bug 1403870. But I think we should manually whitelist data:application/json.

Comment 29

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1403814_block_data_if_opnend_in_browser.patch

Smaug, as discussed, we should block toplevel data: URI navigations within nsDSURIContentListener::DoContent, because at this point we know that the data URI will be openend in the browser. In turn, this change allows us to remove the argument aIsDownLoad because all downloads of data URIs will not go through this codepath. The only update we have to do is, we have to store the loadFromExternal flag within the loadinfo, so we know whether a load started from an external application.

Finally, we have to update test_block_toplevel_data_navigation.html because now we block later in the loading cycle.

Comment 30

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1403814_test_updates.patch

Comment 31

[Christoph Kerschbaumer [:ckerschb]]

 https://treeherder.mozilla.org/#/jobs?repo=try&revision=7de695f4267533706a5627cce912347be075548c

Comment 32

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8921459 [details] [diff] [review]
bug_1403814_test_updates.patch

1000 timeout is a tad worrisome, but what can one do when trying to detect that something does not happen.

Comment 33

[Olli Pettay [:smaug][[e-mail]]]

So how does the new approach work with window.open("data:...")? We open a new window, realize that it is data: and close it? I guess that is similar to downloading.

Comment 34

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8921458 [details] [diff] [review]
bug_1403814_block_data_if_opnend_in_browser.patch

>+++ b/netwerk/base/nsILoadInfo.idl
>@@ -594,16 +594,23 @@ interface nsILoadInfo : nsISupports
>    * Please note, once the flag is set to true it must remain true
>    * throughout the lifetime of the channel. Trying to set it
>    * to anything else than true will be discarded.
>    *
>    */
>   [infallible] attribute boolean initialSecurityCheckDone;
> 
>   /**
>+   * Returns true if the load was triggered from an external application
>+   * (e.g. Thunderbird). Please note that this flag will only ever be true
>+   * if the load is of TYPE_DOCUMENT. 
>+   */
>+  [infallible] attribute boolean loadTriggeredFromExternal;
>+
Well, then you should assert that load is TYPE_DOCUMENT when one calls SetLoadTriggeredFromExternal(true)

(Need to review still the main part of the patch)

Comment 35

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Olli Pettay [:smaug] from comment #32)
> 1000 timeout is a tad worrisome, but what can one do when trying to detect
> that something does not happen.
I agree, that's suboptimal. Happy to change the entire test if someone can tell me a better way to test that a load does not occur.

(In reply to Olli Pettay [:smaug] from comment #33)
> So how does the new approach work with window.open("data:...")? We open a
> new window, realize that it is data: and close it? I guess that is similar
> to downloading.
Well, in fact we are not closing it at this point. E.g. <a href="data:bla" target="_blank"> also opens the new window, and does not close it. Can we even fix that within nsDSURIContentListener? I know we had that discussion before that's why we initially added the check early within docshell so the window does not even get openend. The tradeoff to that old design was that we were not able to distinguish between data types that are openend in the browser and those who open the download panel.

(In reply to Olli Pettay [:smaug] from comment #34)
> >+  [infallible] attribute boolean loadTriggeredFromExternal;
> >+
> Well, then you should assert that load is TYPE_DOCUMENT when one calls
> SetLoadTriggeredFromExternal(true)

Yes, that makes sense, I'll extend:
NS_IMETHODIMP
LoadInfo::SetLoadTriggeredFromExternal(bool aLoadTriggeredFromExternal)
{
  MOZ_ASSERT(!aLoadTriggeredFromExternal ||
             mInternalContentPolicyType == nsIContentPolicy::TYPE_DOCUMENT,
             "can only set load triggered from external for TYPE_DOCUMENT");
  mLoadTriggeredFromExternal = aLoadTriggeredFromExternal;
  return NS_OK;
}

Comment 36

[Boris Zbarsky [:bzbarsky]]

> Can we even fix that within nsDSURIContentListener?

Yes.  We could factor out the logic from http://searchfox.org/mozilla-central/rev/1285ae3e810e2dbf2652c49ee539e3199fcfe820/uriloader/exthandler/nsExternalHelperAppService.cpp#1591-1598,1608-1619 and http://searchfox.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp#2526-2560 into a helper "close this window async and return its opener if it says it was just opened" thing...

Comment 37

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #36)
> > Can we even fix that within nsDSURIContentListener?
> 
> Yes.  We could factor out the logic from
> http://searchfox.org/mozilla-central/rev/
> 1285ae3e810e2dbf2652c49ee539e3199fcfe820/uriloader/exthandler/
> nsExternalHelperAppService.cpp#1591-1598,1608-1619 and
> http://searchfox.org/mozilla-central/source/uriloader/exthandler/
> nsExternalHelperAppService.cpp#2526-2560 into a helper "close this window
> async and return its opener if it says it was just opened" thing...

That sounds good to me. Since all of that data URI blocking is still only enabled within nightly and early beta, would you be fine with refactoring that in a follow up? If you think it belongs into this bug, that's fine with me too.

Comment 38

[Boris Zbarsky [:bzbarsky]]

The refactoring should _definitely_ be in a separate bug from this one.

Comment 39

[Olli Pettay [:smaug][[e-mail]]]

But we don't want to leave windows open, so that window closing needs to be handled while making the other changes.

Comment 40

[Olli Pettay [:smaug][[e-mail]]]

Or, I guess I could review the patch here, if you open another bug and deal with window closing there. But I'd prefer looking that all at the same time.

Comment 41

[Christoph Kerschbaumer [:ckerschb]]

I filed Bug 1412824, will do the refactoring that allows us to close the window within that bug and will flag you for review for both bugs at the same time.

Comment 42

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1403814_block_data_if_opnend_in_browser.patch

Smaug, this patch blocks the data navigation after we have received a response. As discussed, the downside of this approach is that we manually have to close the window that has already been opened at this point.

As suggested by Boris in comment 36, I refactored the window closing code from nsExternalHelperAppService so we can reuse the same functionality -> see Bug 1412824.

Please note that this Bug and Bug 1412824 need to land together at the same time.

Comment 43

[Olli Pettay [:smaug][[e-mail]]]

Do we need some tests... like, target load to a new window and then redirect to data: or something weird like that?

Comment 44

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Olli Pettay [:smaug] from comment #43)
> Do we need some tests... like, target load to a new window and then redirect
> to data: or something weird like that?

We have test_toplevel_data_navigation.html which includes that redirect case.

Comment 45

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/acbbffb64a06
Test navigation to data:text/csv. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d25efb36d34
Update tests for toplevel data URI blocking because we know block after we have received the response. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c4ecb840463
Block toplevel data: URI navigations only if openend in the browser. r=smaug

Comment 46

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/acbbffb64a06
https://hg.mozilla.org/mozilla-central/rev/5d25efb36d34
https://hg.mozilla.org/mozilla-central/rev/0c4ecb840463

Comment 47

[zdnex]

Hello,

Is this supposed to work in FF58 beta1? As on some machines FF58 developer version was installed now and it looks like data: uris are still broken.

Thanks for info.

Comment 48

[Boris Zbarsky [:bzbarsky]]

This is certainly supposed to be fixed in 58 beta1, which doesn't exist yet as far as I know.

I just checked developer edition, and it seems to be on 57 for me, not 58, with all updates applied.  In that developer edition build, the testcase from this bug seems to work...