1401895 - Block top-level navigations to data: URIs

Status: RESOLVED FIXED

(Core :: DOM: Security, enhancement)

Description

[Christoph Kerschbaumer [:ckerschb]]

Within Bug 1380959 we flipped that behavior on for Nightly and Early Beta. Within this bug we should switch it on for real.

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1401895_block_toplevel_data_uris.patch

Boris, now that we only block data URIs that are opened in the browser (see Bug 1403814) and also got Bug 1407891 (view-image problem) out of the way, can we ship this behavioral change of blocking top-level data URI navigations?

Please note that we already enabled this change in Nightly and Early Beta within the 57 cycle (see Bug 1380959).

What do you think?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8926437 [details] [diff] [review]
bug_1401895_block_toplevel_data_uris.patch

The comment on this pref needs to be updated to match reality.

It looks like we block data:text/plain in toplevel.  Why?  In general, I would think we would want to whitelist everything in nsContentUtils::IsPlainTextType, not just application/json.  It's particularly weird to whitelist application/json but not text/json...

r=me if we whitelist all the plaintext types, I think.  If we don't want to do that, I'd like to understand why.

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #3)
> Comment on attachment 8926437 [details] [diff] [review]
> bug_1401895_block_toplevel_data_uris.patch
> 
> The comment on this pref needs to be updated to match reality.
> 
> It looks like we block data:text/plain in toplevel.  Why?  In general, I
> would think we would want to whitelist everything in
> nsContentUtils::IsPlainTextType, not just application/json.  It's
> particularly weird to whitelist application/json but not text/json...

I guess that makes sense. Thanks for the hint. I filed Bug 1415612 to get that done and to not pollute this bug with those changes.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/479f3105ad3b
Block top-level navigations to data: URIs. r=bz

Comment 6

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/479f3105ad3b

Comment 7

[Kohei Yoshino [:kohei]]

Posted the site compatibility note: https://www.fxsitecompat.com/en-CA/docs/2017/data-url-navigations-on-top-level-window-will-now-be-blocked/

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Kohei Yoshino [:kohei] from comment #7)
> Posted the site compatibility note:
> https://www.fxsitecompat.com/en-CA/docs/2017/data-url-navigations-on-top-
> level-window-will-now-be-blocked/

Thanks!

Comment 9

[Chris Mills [:cmills]]

I've documented this by adding a note to our browser compat data (see https://github.com/mdn/browser-compat-data/pull/857), and a note to the Fx59 rel notes:

https://developer.mozilla.org/en-US/Firefox/Releases/59#Security

Let me know if that is OK. Thanks!

Comment 10

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #9)
> I've documented this by adding a note to our browser compat data (see
> https://github.com/mdn/browser-compat-data/pull/857), and a note to the Fx59
> rel notes:
> 
> https://developer.mozilla.org/en-US/Firefox/Releases/59#Security
> 
> Let me know if that is OK. Thanks!

That looks good, thanks. Can you also link to the blogpost, which in my opinion is more comprehensive:
https://blog.mozilla.org/security/2017/11/27/blocking-top-level-navigations-data-urls-firefox-58/

Thanks!

Comment 11

[Chris Mills [:cmills]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #10)
> (In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #9)
> > I've documented this by adding a note to our browser compat data (see
> > https://github.com/mdn/browser-compat-data/pull/857), and a note to the Fx59
> > rel notes:
> > 
> > https://developer.mozilla.org/en-US/Firefox/Releases/59#Security
> > 
> > Let me know if that is OK. Thanks!
> 
> That looks good, thanks. Can you also link to the blogpost, which in my
> opinion is more comprehensive:
> https://blog.mozilla.org/security/2017/11/27/blocking-top-level-navigations-
> data-urls-firefox-58/
> 
> Thanks!

OK, I've updated the link.

Comment 12

[Martin S.]

I've been using top-level document data URI as a workaround for the https://bugzilla.mozilla.org/show_bug.cgi?id=1344465 bug (POST does not work in webextensions). Disabling the top-level data URI kills this workaround.

Comment 13

[:Gijs (he/him)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #10)
> (In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #9)
> > I've documented this by adding a note to our browser compat data (see
> > https://github.com/mdn/browser-compat-data/pull/857), and a note to the Fx59
> > rel notes:
> > 
> > https://developer.mozilla.org/en-US/Firefox/Releases/59#Security
> > 
> > Let me know if that is OK. Thanks!
> 
> That looks good, thanks. Can you also link to the blogpost, which in my
> opinion is more comprehensive:
> https://blog.mozilla.org/security/2017/11/27/blocking-top-level-navigations-
> data-urls-firefox-58/
> 
> Thanks!

This blogpost suggests we did this in 58. Can we update it to say 59 instead? I was very confused when a reporter pointed out that toplevel data: URI navigation still worked in bug 1441819.

Comment 14

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Martin S. from comment #12)
> This blogpost suggests we did this in 58. Can we update it to say 59
> instead? I was very confused when a reporter pointed out that toplevel data:
> URI navigation still worked in bug 1441819.

Heh, interesting that no one of my reviewers caught that. Thanks for pointing it out - I already updated it [1].

[1] https://blog.mozilla.org/security/2017/11/27/blocking-top-level-navigations-data-urls-firefox-59/