1396798 - Don't block top level data: URIs when loading an image

Status: RESOLVED FIXED

(Core :: DOM: Security, enhancement, P2)

Description

[Christoph Kerschbaumer [:ckerschb]]

Currently, all toplevel data: URI navigations are blocked when the pref |security.data_uri.block_toplevel_data_uri_navigations| is set to true.

I think we can exclude data images from that restriction.

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1396798_do_not_block_toplevel_data_img.patch

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1396798_test_toplevel_data_img.patch

Comment 3

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

Comment on attachment 8904991 [details] [diff] [review]
bug_1396798_do_not_block_toplevel_data_img.patch

Oddly simple.

Comment 4

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

Comment on attachment 8904992 [details] [diff] [review]
bug_1396798_test_toplevel_data_img.patch

The svg part here is wrong. SVG is loaded as a document, so there wouldn't be any img elements.
I guess you could check for example that 
wrappedWin2.document.documentElement.localName != "svg"

With that, r+

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Olli Pettay [:smaug] from comment #4)
> Comment on attachment 8904992 [details] [diff] [review]
> bug_1396798_test_toplevel_data_img.patch
> 
> The svg part here is wrong. SVG is loaded as a document, so there wouldn't
> be any img elements.
> I guess you could check for example that 
> wrappedWin2.document.documentElement.localName != "svg"
> 
> With that, r+

Thanks, I just checked that we spit out the warning and don't load the svg image. I'll fix that.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8904991 [details] [diff] [review]
bug_1396798_do_not_block_toplevel_data_img.patch

Given how large image data: URIs can be, it might be nice to not do a copy of the entire URI here if we can avoid it.  Since we know the scheme is "data" already, could we GetFilePath instead of GetSpec?  That will just bump the stringbuffer refcount on mPath instead of copying...

Comment 7

[Boris Zbarsky [:bzbarsky]]

Also, what is the "spec" used for right now?  If this is the code added in bug 1394554, then it looks like it's dead code, just doing a copy of a large string for no reason.

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Boris Zbarsky [:bz] (still digging out from vacation mail) from comment #7)
> Also, what is the "spec" used for right now?  If this is the code added in
> bug 1394554, then it looks like it's dead code, just doing a copy of a large
> string for no reason.

Thanks Boris - Good catch - I'll get both things fixed.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff412c116b9b
Do not block toplevel data: navigation to image (except svgs). r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/79bf8a92a0ea
Test toplevel data: URI navigation to images. r=smaug

Comment 10

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

https://hg.mozilla.org/mozilla-central/rev/ff412c116b9b
https://hg.mozilla.org/mozilla-central/rev/79bf8a92a0ea