about:preferences is broken with security slider set to "High"

If one sets the security slider to "High" there is no way anymore to configure the browser preferences on the about:preferences page. First mentioned in https://blog.torproject.org/blog/tor-browser-50-released#comment-100830.

added TorBrowserTeam201509R in Legacy / Trac component::applications/tor browser in Legacy / Trac owner::tbb-team in Legacy / Trac priority::medium in Legacy / Trac resolution::fixed in Legacy / Trac status::closed in Legacy / Trac tbb-5.0-regression in Legacy / Trac tbb-usability in Legacy / Trac type::defect in Legacy / Trac labels

To workaround it change the security level to a lower one, restart, make the changes and set it back to "high".

Trac:
Cc: N/A to brade, mcs

Another workaround: set "browser.preferences.inContent" to false in about:config to restore the old Preferences UI that works well, then use (Tools->)Options in the menu (bar).

The underlying preference value that causes this bug is svg.in-content.enabled = false.

A related ticket: legacy/trac#16607 (moved) (maybe a similar solution can be used to solve both problems).

Kathy and I have concluded that a whitelisting mechanism is needed. The most straightforward solution is to enable SVG when the URI associated with a document has one of the following schemes: about: chrome: resource:

Doing so will fix this ticket as well as legacy/trac#16607 (moved). The only downside is that chrome: and resource: URIs can be loaded by remote web pages, which means they would be able to trigger execution of SVG code in a limited way. Maybe we should have another ticket to disallow that kind of load, but overall the risk seems acceptable.

Before we proceed with a fix, Kathy and I would like opinions from other people as to whether whitelisting is safe. gk? mikeperry?

Trac:
Cc: brade, mcs to brade, mcs, gk, mikeperry

Trac:
Status: new to needs_information

We only need to allow the about: scheme for this bug, right? If so, this is fine to me. Generally, I am very hesitant to water down our "High Security" mode. We know that things are breaking in this mode and users ought to do the same. I don't feel the usability in this mode is so bad at the moment that we should jump off the cliff allowing chrome: and resource: (too) and see what happens.

Replying to gk:

We only need to allow the about: scheme for this bug, right? If so, this is fine to me. Generally, I am very hesitant to water down our "High Security" mode. We know that things are breaking in this mode and users ought to do the same. I don't feel the usability in this mode is so bad at the moment that we should jump off the cliff allowing chrome: and resource: (too) and see what happens.

OK. Whitelisting based on the top-level page turned out to be somewhat messy (for example, some SVG images are loaded as CSS background images, and therefore we need to extract the top document from the channel's load context). We also added some debug printfs because we found them useful for verifying correct behavior. Here is the patch:

https://gitweb.torproject.org/user/brade/tor-browser.git/commit/?h=bug16775-01&id=0b4c8b589077d054b60a609871dd023bc4d9c444

Trac:
Status: needs_information to needs_review
Keywords: N/A deleted, TorBrowserTeam201508R added

Hm... about:tor is not loading for me on Linux anymore and I can't open the hamburger menu nor the browser console with Ctrl + Shift + J. After managing to get the menubar visible trying to open the browser console gives me

console.error: 
  Message: TypeError: devtools.TargetFactory is undefined
  Stack:
    getTarget@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/webconsole/hudservice.js:219:7
Handler.prototype.process@re[/gre/modules/Promise.jsm](/gre/modules/Promise.jsm) -> re[/gre/modules/Promise-backend.js:867:23](/gre/modules/Promise-backend.js:867:23)
this.PromiseWalker.walkerLoop@re[/gre/modules/Promise.jsm](/gre/modules/Promise.jsm) -> re[/gre/modules/Promise-backend.js:746:7](/gre/modules/Promise-backend.js:746:7)
this.PromiseWalker.scheduleWalkerLoop/<@re[/gre/modules/Promise.jsm](/gre/modules/Promise.jsm) -> re[/gre/modules/Promise-backend.js:688:37](/gre/modules/Promise-backend.js:688:37)

Looking at my branch I built with your patch is the only one atop of the 5.5 branch. Seems like we need some revision here.

Trac:
Status: needs_review to needs_revision

Btw: this happens with the default settings, choosing the "High" security level changes nothing.

Replying to gk:

Hm... about:tor is not loading for me on Linux anymore and I can't open the hamburger menu nor the browser console with Ctrl + Shift + J.

Strange. Kathy and I did standalone (non-gitian) builds for both Mac OS and Linux64 this morning and did not encounter the problems you found. I just started a gitian-based build to see if that makes a difference (but that build will take a while to complete even though I am only building for Linux).

Replying to mcs:

Replying to gk:

Hm... about:tor is not loading for me on Linux anymore and I can't open the hamburger menu nor the browser console with Ctrl + Shift + J.

Strange. Kathy and I did standalone (non-gitian) builds for both Mac OS and Linux64 this morning and did not encounter the problems you found. I just started a gitian-based build to see if that makes a difference (but that build will take a while to complete even though I am only building for Linux).

It looked again and these problems seemed to be caused by me not having a recent Torbutton in the profile (for whatever reason). Sorry for the noise. Testing it it looks good.

Trac:
Status: needs_revision to needs_review

The code looks good to me.

Replying to gk:

The code looks good to me.

Thanks. Since these are C++ changes, another review would be very much appreciated.

Trac:
Cc: brade, mcs, gk, mikeperry to brade, mcs, gk, mikeperry, arthuredelstein

Replying to mcs:

Replying to gk:

The code looks good to me.

Thanks. Since these are C++ changes, another review would be very much appreciated.

It looks good to me as well.

Transfer review tickets to Sept.

Trac:
Keywords: TorBrowserTeam201508R deleted, TorBrowserTeam201509R added

Fixed on tor-browser-38.2.1esr-5.5-2 (commit d8419037) and on tor-browser-38.2.1esr-5.0-2 (commit 191176cf), thanks.

Trac:
Status: needs_review to closed
Resolution: N/A to fixed

closed

moved from legacy/trac#16775 (moved)

mentioned in commit 191176cf

mentioned in commit d8419037

mentioned in commit 4fc327d6

mentioned in commit e5101c9e

added Bug UX labels and removed 1 deleted label

removed 1 deleted label