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

RESOLVED FIXED in Firefox 58

Status

()

defect
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: zdnexnet, Assigned: ckerschb)

Tracking

({regression, site-compat})

57 Branch
mozilla58
All
Unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox55 unaffected, firefox56 unaffected, firefox57 disabled, firefox58 fixed)

Details

(Whiteboard: [tor][domsecurity-active])

Attachments

(3 attachments, 2 obsolete attachments)

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
Blocks: 1401895
Keywords: regression
Hardware: Unspecified → All
Version: 55 Branch → 57 Branch
Blocks: 1380959
Status: UNCONFIRMED → NEW
Component: Untriaged → DOM: Security
Ever confirmed: true
Product: Firefox → Core
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.
Assignee: nobody → ckerschb
Status: NEW → ASSIGNED
Priority: -- → P1
Summary: window.open data: URI is broken in FF57 → Potentially allow navigting to data:text/csv
Whiteboard: [tor][domsecurity-active]
Summary: Potentially allow navigting to data:text/csv → Potentially allow navigations to data:text/csv
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
(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.
We could probably allow all base64-encoded data URLs regardless of the MIME type?
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.
Attachment #8917556 - Flags: review?(bugs)
Could you explain why we want this, and why only text/csv and not also some other text/*?
I kind of like comment 4. And only allowing text/csv feels a bit hacky.
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?
Attachment #8917556 - Flags: review?(bugs)
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.
Attachment #8917557 - Flags: review?(bugs) → review+
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?
Attachment #8917556 - Flags: review?(bugs)
Should we allow all the data: loads which won't be loaded in browser?
(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 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.
Attachment #8917556 - Flags: review?(bugs) → review-
(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.
I filed Bug 1408400 to consider the blacklist approach.
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
Flags: needinfo?(bugs)
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.
Flags: needinfo?(bugs)
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?
(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?
Flags: needinfo?(bugs)
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.
(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.
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).
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.
(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.
Flags: needinfo?(bugs)
Summary: Potentially allow navigations to data:text/csv → Block toplevel data: URI navigations only if openend in the browser
I guess there is still Bug 1403870. But I think we should manually whitelist data:application/json.
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.
Attachment #8917556 - Attachment is obsolete: true
Attachment #8921458 - Flags: review?(bugs)
Blocks: 1403870
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.
Attachment #8921459 - Flags: review?(bugs) → review+
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 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)
(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;
}
> 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...
(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.
The refactoring should _definitely_ be in a separate bug from this one.
But we don't want to leave windows open, so that window closing needs to be handled while making the other changes.
Attachment #8921458 - Flags: review?(bugs)
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.
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.
See Also: → 1412824
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.
Attachment #8921458 - Attachment is obsolete: true
Attachment #8924533 - Flags: review?(bugs)
Attachment #8924533 - Flags: review?(bugs) → review+
Do we need some tests... like, target load to a new window and then redirect to data: or something weird like that?
(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.
Pushed by mozilla@christophkerschbaumer.com:
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
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.
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...
Depends on: 1419902
You need to log in before you can comment on or make changes to this bug.