686581 - Disable theming in SVG images

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Daniel Holbert [:dholbert]]

roc pointed out that it's probably possible to detect what platform a user is on (and their GTK theme, etc) by sticking a button into an SVG image.

To prevent this data leakage, we should disable theming in SVG images.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: testcase

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Mats, can you take this? We probably just need to add a "-moz-appearance:none ! important" rule to the style sheets for all SVG images.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Hmm, another thing we need to look at is whether links in SVG images get colored according to visitedness. If they do, then we're leaking visitedness information.

Comment 4

[Robert Longson [:longsonr]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #3)
> Hmm, another thing we need to look at is whether links in SVG images get
> colored according to visitedness. If they do, then we're leaking visitedness
> information.

We've got bug 641731 for that.

Comment 5

[Mats Palmgren (inactive)]

Attachment: fix

Comment 6

[Mats Palmgren (inactive)]

Do we restrict theme media queries to chrome?
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsGkAtomList.h#1876

Comment 7

[Robert Longson [:longsonr]]

The test and code does not seem to be for svg within an image tag. We're OK with SVG having themed as long as it's not <image src="something.svg/>  aren't we? That's what the standalone testcase tests - you have an image tag and the svg is within that.

I think you should have tests that svg not in an image tag is still themed.

It's a bit tricky to read the tests as they seem to be all on one line too.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It looks like this patch will apply to all SVG, which isn't what we want. We want it to only apply to the content of SVG used in an image context.

Comment 9

[Mats Palmgren (inactive)]

Sorry, I thought that's what you wanted...
As far as I know it's NOT possible to do this using only CSS currently.

Maybe we could introduce a :-moz-root-of-resource-document that would match
the root element of a resource document to differentiate the two cases?

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

A media query feature would probably be slightly better.

Whether a document is a resource document can't change dynamically so it should be quite simple to implement.

One thing that just occurred to me is that it appears an <iframe> inside an SVG image document might not set its child document to return true for IsBeingUsedAsImage ... but it should.

Comment 11

[Mats Palmgren (inactive)]

I can't get an <iframe> inside a SVG image to render at all.  It works when I load the
SVG directly, but when using it in a <img src=...> all I see is the <iframe>
borders, nothing inside.

Comment 12

[Mats Palmgren (inactive)]

Attachment: part 1, Implement -moz-is-resource-document media query

Comment 13

[Mats Palmgren (inactive)]

Attachment: part 2, Use it to disable theming in SVG images

Comment 14

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c764918036ab
https://hg.mozilla.org/integration/mozilla-inbound/rev/8068ef5b4dcb

Comment 15

[Dão Gottwald [:dao]]

Comment on attachment 562052 [details] [diff] [review]
part 2, Use it to disable theming in SVG images

>+@media all and (-moz-is-resource-document) {
>+ foreignObject *|* {
>+   -moz-appearance: none;
>+ }
>+}

Does this prevent people from explicitly setting -moz-appearance:button? It looks like it doesn't.

Besides, CSS platform colors also still work and expose pretty much the same information, right? I don't think this is worth fighting against...

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

That's a good point about CSS platform colors, although I think those are already leaked through canvas drawing. I still think this is worth doing.

(In reply to Dão Gottwald [:dao] from comment #15)
> >+@media all and (-moz-is-resource-document) {
> >+ foreignObject *|* {
> >+   -moz-appearance: none;
> >+ }
> >+}
> 
> Does this prevent people from explicitly setting -moz-appearance:button? It
> looks like it doesn't.

Sigh, I missed that. It needs !important.

Comment 17

[Dão Gottwald [:dao]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #16)
> That's a good point about CSS platform colors, although I think those are
> already leaked through canvas drawing. I still think this is worth doing.

You need neither canvas nor SVG for it, just CSS and getComputedStyle.

Comment 18

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/ec384fc33769

Comment 19

[Matt Brubeck (:mbrubeck)]

I pasted the wrong changeset above.
https://hg.mozilla.org/mozilla-central/rev/c764918036ab
https://hg.mozilla.org/mozilla-central/rev/8068ef5b4dcb

Comment 20

[Mats Palmgren (inactive)]

(In reply to Dão Gottwald [:dao] from comment #15)
> Does this prevent people from explicitly setting -moz-appearance:button? 

Good catch, thanks!

Comment 21

[Mats Palmgren (inactive)]

Attachment: part 3, add the missing !important

Comment 22

[Dão Gottwald [:dao]]

I still think this bug is pointless...

Comment 23

[Robert Longson [:longsonr]]

Comment on attachment 562423 [details] [diff] [review]
part 3, add the missing !important

Stealing review so it can land before the aurora merge if necessary.

Comment 24

[Mats Palmgren (inactive)]

Thanks.
https://hg.mozilla.org/mozilla-central/rev/368b4a870769