Opened 7 months ago

Closed 4 months ago

#26128 closed defect (fixed)

Make security slider work with NoScript for ESR60

Reported by: arthuredelstein Owned by: tbb-team
Priority: Very High Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: ff60-esr, TorBrowserTeam201806R
Cc: sukhbir, mcs, brade, ma1, rustybird@… Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

NoScript is now rewritten based on WebExtensions and as such no longer uses prefs. So we need to find another way to control it. Here are the prefs we used:

  "noscript.forbidMedia" :                    [,  true,  true,  true,  false],
  "noscript.global" :                         [,  false, false, false, true ],
  "noscript.globalHttpsWhitelist" :           [,  false, true,  true,  false],
  "noscript.forbidFonts" :                    [,  true,  false, false, false],

Child Tickets

Change History (16)

comment:1 Changed 7 months ago by gk

Keywords: TorBrowserTeam201805 added
Priority: MediumVery High

comment:2 Changed 7 months ago by sukhbir

Cc: sukhbir added

comment:3 Changed 6 months ago by arthuredelstein

Keywords: TorBrowserTeam201806R added; TorBrowserTeam201805 removed
Status: newneeds_review

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:

  1. Using a LegacyExtensionContext (defined in LegacyExtensionsUtils.jsm) to send JSON objects to NoScript via sendMessage.
  2. 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.
  3. 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!)

comment:4 Changed 6 months ago by gk

Cc: mcs brade added

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

comment:5 Changed 6 months ago by mcs

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

comment:6 Changed 6 months ago by mcs

Status: needs_reviewneeds_revision

comment:7 in reply to:  5 Changed 6 months ago by arthuredelstein

Cc: ma1 added

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), but worth considering now given that in transitioning to the new NoScript, some behaviors are going to change.

comment:8 in reply to:  5 ; Changed 6 months ago by 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?

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!

comment:9 Changed 6 months ago by arthuredelstein

Status: needs_revisionneeds_information

comment:10 in reply to:  8 Changed 6 months ago by ma1

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!

comment:11 in reply to:  3 Changed 6 months ago by ma1

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

comment:12 Changed 6 months ago by gk

Resolution: fixed
Status: needs_informationclosed

ma1: Thanks!

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

comment:13 Changed 6 months ago by rustybird

Cc: rustybird@… added
Resolution: fixed
Status: closedreopened

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?

comment:14 in reply to:  13 Changed 6 months ago by gk

Resolution: fixed
Status: reopenedclosed

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.

comment:15 Changed 4 months ago by ma1

Resolution: fixed
Status: closedreopened

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

comment:16 Changed 4 months ago by arthuredelstein

Resolution: fixed
Status: reopenedclosed

Thanks, Giorgio. I opened #27276

Note: See TracTickets for help on using tickets.