Closed Bug 1401895 Opened 2 years ago Closed 2 years ago

Block top-level navigations to data: URIs

Categories

(Core :: DOM: Security, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: ckerschb, Assigned: ckerschb)

References

Details

(Keywords: dev-doc-complete, site-compat, Whiteboard: [domsecurity-active])

Attachments

(1 file)

Within Bug 1380959 we flipped that behavior on for Nightly and Early Beta. Within this bug we should switch it on for real.
Assignee: nobody → ckerschb
Depends on: 1380959
Whiteboard: [domsecurity-active]
Status: NEW → ASSIGNED
Duplicate of this bug: 1402623
Depends on: 1403814
Depends on: 1407891
Depends on: 1408400
Depends on: 1408451
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?
Attachment #8926437 - Flags: review?(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.
Attachment #8926437 - Flags: review?(bzbarsky) → review+
Depends on: 1415612
(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.
https://hg.mozilla.org/mozilla-central/rev/479f3105ad3b
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
(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!
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!
Flags: needinfo?(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!
Flags: needinfo?(ckerschb)
(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.
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.
(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.
Flags: needinfo?(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/
Flags: needinfo?(ckerschb)
You need to log in before you can comment on or make changes to this bug.