Disable theming in SVG images

RESOLVED FIXED in mozilla9

Status

()

Core
SVG
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: dholbert, Assigned: mats)

Tracking

Trunk
mozilla9
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
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.
(Reporter)

Updated

6 years ago
OS: Linux → All
Hardware: x86_64 → All
(Reporter)

Comment 1

6 years ago
Created attachment 560071 [details]
testcase
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.
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

6 years ago
(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.
(Assignee)

Comment 5

6 years ago
Created attachment 561821 [details] [diff] [review]
fix
Assignee: nobody → matspal
Attachment #561821 - Flags: review?(roc)
(Assignee)

Comment 6

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

Comment 7

6 years ago
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.
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.
(Assignee)

Comment 9

6 years ago
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?
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.
(Assignee)

Comment 11

6 years ago
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.
(Assignee)

Comment 12

6 years ago
Created attachment 562051 [details] [diff] [review]
part 1, Implement -moz-is-resource-document media query
Attachment #561821 - Attachment is obsolete: true
Attachment #561821 - Flags: review?(roc)
Attachment #562051 - Flags: review?(roc)
(Assignee)

Comment 13

6 years ago
Created attachment 562052 [details] [diff] [review]
part 2, Use it to disable theming in SVG images
Attachment #562052 - Flags: review?(roc)
Attachment #562051 - Flags: review?(roc) → review+
Attachment #562052 - Flags: review?(roc) → review+
(Assignee)

Comment 14

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/c764918036ab
https://hg.mozilla.org/integration/mozilla-inbound/rev/8068ef5b4dcb
Flags: in-testsuite+
Whiteboard: [inbound]
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...
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.
(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.
https://hg.mozilla.org/mozilla-central/rev/ec384fc33769
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla9
I pasted the wrong changeset above.
https://hg.mozilla.org/mozilla-central/rev/c764918036ab
https://hg.mozilla.org/mozilla-central/rev/8068ef5b4dcb
(Assignee)

Comment 20

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

Good catch, thanks!
(Assignee)

Comment 21

6 years ago
Created attachment 562423 [details] [diff] [review]
part 3, add the missing !important
Attachment #562423 - Flags: review?(roc)
I still think this bug is pointless...

Comment 23

6 years ago
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.
Attachment #562423 - Flags: review?(roc) → review+
(Assignee)

Comment 24

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

Updated

6 years ago
Depends on: 689224
You need to log in before you can comment on or make changes to this bug.