Make security slider work with NoScript for ESR60

added TorBrowserTeam201806R component::applications/tor browser ff60-esr owner::tbb-team priority::very high resolution::fixed severity::normal status::closed type::defect labels

Trac:
Priority: Medium to Very High
Keywords: N/A deleted, TorBrowserTeam201805 added

Trac:
Cc: N/A to sukhbir

Here's a patch for torbutton that talks to the WebExtensions version of NoScript:

https://github.com/arthuredelstein/torbutton/commit/26128 (7656b587d13aa6b0f90f0149d884aafa1cc65570)

This patch uses three tricks:

Using a LegacyExtensionContext (defined in LegacyExtensionsUtils.jsm) to send JSON objects to NoScript via sendMessage.
Taking advantage of an existing invocation of browser.runtime.onMessage.addListener(...) in NoScript's code that accepts a JSON object for updating NoScript's settings.
Providing NoScript with settings for a "site" whose "domain" is "http:", which causes NoScript to match non-https sites.

We may decide to tweak the capabilities for each security slider level; I tried to make them as close to the previous behavior as possible, but not sure if they're exactly as we want.

One problem I ran into is that, even if I set NoScript 10.1.8.2 only "script" and "fetch" content while disallowing "object", "media", "frame", "font", "webgl", and "other", I can still watch videos on YouTube. So I think this is a NoScript bug rather than a problem with this patch.

(Thanks to Sukhbir for help with this!)

Trac:
Keywords: TorBrowserTeam201805 deleted, TorBrowserTeam201806R added
Status: new to needs_review

mcs/brade: can you have a look at that patch?

Trac:
Cc: sukhbir to sukhbir, mcs, brade

Kathy and I reviewed these changes. Awesome work! Of course we have a few comments:

There is a comma missing after the second array within untrusted_caps (before the // medium: http comment).
Please add some details to the check in comment, e.g., some of the things you mentioned in comment:3.
To match the slider UI, please use Standard/Safer/Safest within comments (rather than Low/Medium/High).
If the user adds an exception (e.g., adding a site to the Trusted list), the changes will be lost at startup and each time the slider is adjusted. That will surprise people; we should decide what behavior we want and try to implement it.
Do we have any kind of commitment from the NoScript author (Giorgio) that the IPC message we are using will continue to work with future releases of NoScript?
Kathy and I think the capability groupings you selected make sense, but we will probably need to adjust some of the wording within the security slider window (i.e., the text which describes the levels).

Trac:
Status: needs_review to needs_revision

Replying to mcs:

Kathy and I reviewed these changes. Awesome work! Of course we have a few comments:

Thanks for the review! My revised branch is at: https://github.com/arthuredelstein/torbutton/commit/26128+1

There is a comma missing after the second array within untrusted_caps (before the // medium: http comment).

Fixed.

Please add some details to the check in comment, e.g., some of the things you mentioned in comment:3.

Done.

To match the slider UI, please use Standard/Safer/Safest within comments (rather than Low/Medium/High).

Done.

If the user adds an exception (e.g., adding a site to the Trusted list), the changes will be lost at startup and each time the slider is adjusted. That will surprise people; we should decide what behavior we want and try to implement it.

That's true. In principle we can also listen to the settings objects broadcast by NoScript. As a demonstration, this code prints all Settings changes from NoScript.

const { LegacyExtensionContext } =
      Cu.import("resource://gre/modules/LegacyExtensionsUtils.jsm", {});
const noscriptID = "{73a6fe31-595d-460b-a920-fcc0f8843232}";
let extensionContext = new LegacyExtensionContext({ id : noscriptID });
extensionContext.api.browser.runtime.onMessage.addListener((a,b,c) => console.log(a,b,c))

To preserve user custom settings, we would need to use these messages to maintain a mirror of the NoScript's Settings object and pass back a modified version of the mirrored Settings whenever the Security Slider is altered.

A simpler approach could be to patch NoScript to accept diffs rather than the entire Settings object.

However, I'm not sure we want to permanently preserve custom user settings in NoScript at all. Such a feature might be a privacy footgun for users.

[Snipped IPC question for a separate comment, below.]

Kathy and I think the capability groupings you selected make sense, but we will probably need to adjust some of the wording within the security slider window (i.e., the text which describes the levels).

Good point. I'm still inclined to consider the possibility of simplifying the capability groupings -- the different treatment of media, fonts, and scripts seems somewhat arbitrary to me and I think it would be useful to come up with a more systematic rationale for the settings we have chosen for the "Safer" (medium) level. It's kind of a separate issue (#22981 (moved)), but worth considering now given that in transitioning to the new NoScript, some behaviors are going to change.

Trac:
Cc: sukhbir, mcs, brade to sukhbir, mcs, brade, ma1

Replying to mcs:

Do we have any kind of commitment from the NoScript author (Giorgio) that the IPC message we are using will continue to work with future releases of NoScript?

Giorgio, what do you think? My impression is that the message listener and the Settings objects are central to NoScript's architecture. But it would be better if we could define the protocol more formally. And what do you think about idea of NoScript accepting diffs to Settings (see comment:7)? Is that already possible in some way or could we add this capability to NoScript?

Also, I used a hack to treat http and https domain differently (using a "site" whose "domain" is the string "http:". Is there a cleaner way to do this?

Thanks in advance!

Trac:
Status: needs_revision to needs_information

Replying to arthuredelstein:

Replying to mcs:

Do we have any kind of commitment from the NoScript author (Giorgio) that the IPC message we are using will continue to work with future releases of NoScript?

Yes, the updateSettings responder is going to work for the foreseeable future.

But it would be better if we could define the protocol more formally. And what do you think about idea of NoScript accepting diffs to Settings (see comment:7)?

If I understand correctly you need it to accept diffs to the default Policy object, rather than to the updateSettings argument (which already expects a subset or just one of the supported settings to be actually defined, and therefore modified).

Is that already possible in some way or could we add this capability to NoScript? It's not possible yet from the UI or the content script, and so far I didn't have a compelling reason to do it.

Also, I used a hack to treat http and https domain differently (using a "site" whose "domain" is the string "http:". Is there a cleaner way to do this?

I don't consider it a hack: "just by protocol" is explicitly supported by the Policy site matching algorithm. However if by "cleaner" you mean an ad-hoc switch, no there's none.

Regarding both the enhancements, if they're really needed, have you got any design proposal?

Thanks!

One problem I ran into is that, even if I set NoScript 10.1.8.2 only "script" and "fetch" content while disallowing "object", "media", "frame", "font", "webgl", and "other", I can still watch videos on YouTube. So I think this is a NoScript bug rather than a problem with this patch.

That was NoScript Quantum not handling MSE (scripts using MediaSource and blob: URLs to feed media elements) yet.

This feature has been added in 10.1.8.3rc1, https://noscript.net/getit#devel

ma1: Thanks!

I merged 26128+1 to master (commit 1e43e9c6ed448d9ca91235c38f5ea0b4d43c71cb). Arthur: Could you file the follow-up tickets you had in mind?

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

This seems to break on a fresh 8.0a9-build3 linux64 installation whenever TOR_SKIP_LAUNCH=1 is set (with system tor running on the usual ports). JavaScript is effectively disabled and the browser console says:

[06-25 18:36:50] Torbutton NOTE: security-prefs.js initialization complete
Error: Could not establish connection. Receiving end does not exist. ExtensionCommon.jsm:456:12

Some sort of extension startup race?

Trac:
Username: rustybird
Cc: sukhbir, mcs, brade, ma1 to sukhbir, mcs, brade, ma1, rustybird@net-c.com
Status: closed to reopened
Resolution: fixed to N/A

Replying to rustybird:

This seems to break on a fresh 8.0a9-build3 linux64 installation whenever TOR_SKIP_LAUNCH=1 is set (with system tor running on the usual ports). JavaScript is effectively disabled and the browser console says:

{{{ [06-25 18:36:50] Torbutton NOTE: security-prefs.js initialization complete Error: Could not establish connection. Receiving end does not exist. ExtensionCommon.jsm:456:12 }}}

Some sort of extension startup race?

Could be. Let's track this in a new ticket, though. I created #26520 (moved).

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

Please notice that NoScript 10.1.8.17 does change its message handling to slightly abstract it, simplify it and fix some subtle bugs.

As a consequence, the property by which all the messages (including updateSettings)identify themselves is not called "type" anymore, but "_messageName".

In order to work with 10.1.8.1.17 and above, the Security Slider must therefore send this the updated policy in an object with a "_messageName": "updateSettings" property (alone, or in addition to the existent "type": "updateSettings" one for backward compatibility).

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

Thanks, Giorgio. I opened #27276 (moved)

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

closed

mentioned in issue #26407 (moved)

moved to tpo/applications/tor-browser#26128 (closed)

mentioned in issue tpo/applications/torbutton#26407 (moved)

mentioned in issue tpo/applications/tor-browser#40973

Make security slider work with NoScript for ESR60

Child items ...

Activity