Create preference to disable SVG

added TorBrowserTeam201503R component::applications/tor browser owner::mcs parent::9387 priority::high resolution::fixed status::closed tbb-4.5-alpha tbb-isec-report tbb-security tbb-testcase type::enhancement labels

Trac:
Cc: N/A to gk

Trac:
Keywords: isec-audit deleted, tbb-isec-report added

Trac:
Cc: gk to gk, intrigeri

Trac:
Parent: N/A to #9387 (moved)

Trac:
Cc: gk, intrigeri to gk, intrigeri, arthuredelstein

Trac:
Cc: gk, intrigeri, arthuredelstein to gk, intrigeri, arthuredelstein, boklm

Trac:
Keywords: N/A deleted, tbb-4.5-alpha added

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.

Trac:
Cc: gk, intrigeri, arthuredelstein, boklm to gk, intrigeri, arthuredelstein, boklm, brade, mcs
Status: new to assigned
Owner: tbb-team to mcs

Trac:
Keywords: N/A deleted, TorBrowserTeam201503 added

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.

Trac:
Keywords: TorBrowserTeam201503 deleted, TorBrowserTeam201503R added
Status: assigned to needs_review

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...

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.

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?

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 tags inline will not cause a prompt to be displayed.

Trac:
svgtest.html.gz

SVG test file (gzipped)

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.

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.

Trac:
Resolution: N/A to fixed
Keywords: N/A deleted, tbb-testcase added
Status: needs_review to closed