Opened 3 years ago

Closed 3 years ago

#12827 closed enhancement (fixed)

Create preference to disable SVG

Reported by: mikeperry Owned by: mcs
Priority: High Milestone:
Component: Applications/Tor Browser Version:
Severity: Keywords: tbb-security, tbb-isec-report, tbb-4.5-alpha, TorBrowserTeam201503R, tbb-testcase
Cc: gk, intrigeri, arthuredelstein, boklm, brade, mcs Actual Points:
Parent ID: #9387 Points:
Reviewer: Sponsor:

Description

We should have a way to disable SVG suport in Firefox for the security slider. There currently is no pref in Firefox for this, so we will need to create one.

Child Tickets

Attachments (1)

svgtest.html.gz (1.8 KB) - added by mcs 3 years ago.
SVG test file (gzipped)

Download all attachments as: .zip

Change History (19)

comment:1 Changed 3 years ago by gk

Cc: gk added

comment:2 Changed 3 years ago by mikeperry

Keywords: tbb-isec-report added; isec-audit removed

comment:3 Changed 3 years ago by intrigeri

Cc: intrigeri added

comment:4 Changed 3 years ago by gk

Parent ID: #9387

comment:5 Changed 3 years ago by cypherpunks

No image "lol.svg" attached to Ticket #12827

comment:6 Changed 3 years ago by cYphErPUNKs

No image "lollollol.svg" attached to Ticket #12827

comment:7 Changed 3 years ago by arthuredelstein

Cc: arthuredelstein added

comment:8 Changed 3 years ago by boklm

Cc: boklm added

comment:9 Changed 3 years ago by gk

Keywords: tbb-4.5-alpha added

comment:10 Changed 3 years ago by mcs

Cc: brade mcs added
Owner: changed from tbb-team to mcs
Status: newassigned

Kathy and I started working on this yesterday. When support for SVG was first added by Mozilla, and for some years afterward, Firefox included support for a Boolean pref named svg.enabled. It has been helpful to look at how that pref was implemented; here is the change set that removed support for it:

http://hg.mozilla.org/mozilla-central/rev/9e3b27acb9ef

We are in the process of reintroducing that pref using as similar approach to the old, removed Mozilla code as possible. In fact, we were able to get the basic "turn SVG on/off" feature working with not too much difficulty. We are now in the process of making changes so that SVG is enabled for chrome but not for content (the new pref. should probably be named something like svg.inContent.enabled or svg.inContent.disabled).

Using nsContentUtils::IsChromeDoc(doc) to decide whether to allow SVG seems like an OK approach but unfortunately for some internal uses of SVG there is no associated document. Specifically, this seems to be the case for portions of the browser tab images. We should have a solution in a day or two.

comment:11 Changed 3 years ago by mcs

Keywords: TorBrowserTeam201503 added

comment:12 Changed 3 years ago by mcs

Keywords: TorBrowserTeam201503R added; TorBrowserTeam201503 removed
Status: assignedneeds_review

OK, this took a lot longer than expected, and developing the patch was somewhat painful. But Kathy and I now have something that is ready for other people to review:

https://gitweb.torproject.org/user/brade/tor-browser.git/commit/?h=bug12827-01&id=7dd093abb5c68728b5ba6c940cca224345d89f3e

Please see the commit message for more details about the patch.
We tested the new svg.inContent.enabled pref. on Linux, Windows, and Mac.

comment:13 Changed 3 years ago by mikeperry

Ok, I looked over this for general safety and it seems good to me, so I went ahead and merged it in the 4.5 branch. I am not sure it covers all of the cases, or how I would even tell that for sure. Did you test things like SVG images encoded data URIs, for example?

I also changed the name of the pref to be svg.in-content.enabled, to be consistent with the other existing svg prefs already in about:config. I did this in a fixup commit on the 4.5 branch.

Finally, out of curiosity, do you know why the ImageFactory code has this bizarre pattern of returning already_addReffed pointers, while *also* calling .forget() before returning anything? I saw your cut+paste of BadImage() and was about to get worried, before I realized that it was a pre-existing utility function that followed the same pattern as the rest of the ImageFactory code...

comment:14 Changed 3 years ago by gk

Do we expect to get the external file type download dialog if SVG can't get rendered? See: https://upload.wikimedia.org/wikipedia/commons/6/6c/Trajans-Column-lower-animated.svg. If so, what happens if there are numerous of these things embedded in websites? Do users get then dozens of such dialogs? That would be suboptimal, I guess.

comment:15 in reply to:  13 Changed 3 years ago by mcs

Replying to mikeperry:

Ok, I looked over this for general safety and it seems good to me, so I went ahead and merged it in the 4.5 branch. I am not sure it covers all of the cases, or how I would even tell that for sure. Did you test things like SVG images encoded data URIs, for example?

Yes. We made a messy HTML file that tests a bunch of scenarios. One of us will attach it this ticket soon.

I also changed the name of the pref to be svg.in-content.enabled, to be consistent with the other existing svg prefs already in about:config. I did this in a fixup commit on the 4.5 branch.

I think that is a good change. We found some other pref that used inContent but consistency with other SVG prefs is more important.

Finally, out of curiosity, do you know why the ImageFactory code has this bizarre pattern of returning already_addReffed pointers, while *also* calling .forget() before returning anything? I saw your cut+paste of BadImage() and was about to get worried, before I realized that it was a pre-existing utility function that followed the same pattern as the rest of the ImageFactory code...

I guess it is just thought of as the safest / most clear way to do things. Kathy and I found this old bug where forget() was introduced that explains some of the motivation: https://bugzilla.mozilla.org/show_bug.cgi?id=392493
Or maybe I misunderstand your question?

comment:16 in reply to:  14 Changed 3 years ago by mcs

Replying to gk:

Do we expect to get the external file type download dialog if SVG can't get rendered? See: https://upload.wikimedia.org/wikipedia/commons/6/6c/Trajans-Column-lower-animated.svg. If so, what happens if there are numerous of these things embedded in websites? Do users get then dozens of such dialogs? That would be suboptimal, I guess.

Yes, for each top-level SVG document that is loaded and for each that is in an iframe, an external file download prompt will be displayed. I don't think it is common for web pages to use a lot of SVG images in that way, but I do not know for sure. Use of <svg> tags inline will not cause a prompt to be displayed.

Changed 3 years ago by mcs

Attachment: svgtest.html.gz added

SVG test file (gzipped)

comment:17 Changed 3 years ago by mcs

I could not directly attach svgtest.html because it contains too many occurrences of "http:". Then, when I attached a gzip'd file, Trac gzip'd it a second time. Anyway, if you dig deep enough, there is an HTML file inside svgtest.html.gz.

comment:18 Changed 3 years ago by mikeperry

Keywords: tbb-testcase added
Resolution: fixed
Status: needs_reviewclosed

Ok, I am going to call this fixed, then. We can deal with the popup issue in a separate usability ticket if it turns out to be common. I added the tbb-testcase tag so we can pick this up later otherwise. It may be better done as an in-tree test (perhaps when we rebase to ff38), also. boklm, if you want to create a new ticket for the tests, feel free.

mcs - thanks for that old bugzilla ticket. That did answer my question.

Note: See TracTickets for help on using tickets.