Opened 6 years ago

Closed 6 years ago

#13998 closed defect (fixed)

Tor Browser needs to handle changes in NoScript 2.6.9.8+

Reported by: mikeperry Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Keywords: TorBrowserTeam201501R, tbb-4.5-alpha-3, MikePerry201501R
Cc: gk Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

In NoScript 2.6.9.8, Giorgio landed several fixes for us that will result in changes to how Tor Browser interacts with NoScript. Here are the ones I'm aware of:

  1. We no longer need to add https: to the whitelist for the "Medium-High" security slider position due to fixes to noscript.globalHttpsWhitelist.
  2. Temporary permissions are no longer stored in the noscript.temp pref (which gets written to disk). Instead, they are stored in a new memory-only data structure. This means New Identity needs to change how it clears NoScript permissions.
  3. The pref noscript.volatilePrivatePermissions (which governs if temporary permissions are used for Private Browsing Mode) is false by default. We probably want to set it to true if disk records are disabled, but false if they are enabled. We will also need to ensure "New Identity" properly clears the permissions in both cases.

I think that is all, but we should also be sure to have people test NoScript thoroughly in the next TBBs that include NoScript 2.6.9.8+, to make sure there are no other surprise issues.

Child Tickets

Change History (4)

comment:1 Changed 6 years ago by mikeperry

Keywords: TorBrowserTeam201501 added; TorBrowserTeam201412 removed

comment:2 Changed 6 years ago by mikeperry

Keywords: TorBrowserTeam201501R tbb-4.5-alpha-3 MikePerry201501R added; TorBrowserTeam201501 removed

comment:3 Changed 6 years ago by gk

Status: newneeds_review

Replying to mikeperry:

In NoScript 2.6.9.8, Giorgio landed several fixes for us that will result in changes to how Tor Browser interacts with NoScript. Here are the ones I'm aware of:

  1. We no longer need to add https: to the whitelist for the "Medium-High" security slider position due to fixes to noscript.globalHttpsWhitelist.

Should be fixed in my bug_13998 in my public Torbutton repo. Looking at NoScript code I found a more serious bug that is fixed now as well: we need to disable NoScript in the Medium/High mode. Otherwise any JavaScript resource is loaded as the HTTPS checks are only considered if JavaScript is disabled. See: isJSEnabled() in noscriptService.js.

It works fine, e.g. with https://taz.de or https://www.spiegel.de. Interestingly, the latter won't load any JS over https://. The reason is that www.spiegel.de does not support TLS and the internal JavaScript on that side is trying to load all the https:// resources which is obviously failing which is fine I think (although maybe a bit confusing in the first place).

  1. Temporary permissions are no longer stored in the noscript.temp pref (which gets written to disk). Instead, they are stored in a new memory-only data structure. This means New Identity needs to change how it clears NoScript permissions.

What do you mean by "new memory-only data structure"? As far as I can see there is no such thing. tempSites and gTempSites are already available in 2.6.9.6 and cleared if we call eraseTemp. (The same happens still with 2.6.9.10 which is why we don't have to change anything wrt temporary permissions, I think)

  1. The pref noscript.volatilePrivatePermissions (which governs if temporary permissions are used for Private Browsing Mode) is false by default. We probably want to set it to true if disk records are disabled, but false if they are enabled. We will also need to ensure "New Identity" properly clears the permissions in both cases.

Looking at the code this pref is true by default beginning from 2.6.9.7, if I see that correctly. Grepping gives me something like:

./defaults/preferences/noscript.js:pref("noscript.volatilePrivatePermissions", true); 

And updating a freshly downloaded 4.5-alpha-2 to 2.6.9.10 shows true as well. Did you have something else here in mind?

volatilePrivatePermissions should now be bound to no-disk/disk but as in the previous point you mentioned fixed there is no change in handling temporary permissions. This pref is more used to hide non-temporary menuitems and not to tamper with the nature of the permissions itself. I tested it a bit though and think we don't need to implement changes here unless we want to erase the permanent permissions as well on New Identity.

comment:4 in reply to:  3 Changed 6 years ago by mikeperry

Resolution: fixed
Status: needs_reviewclosed

Replying to gk:

Replying to mikeperry:

In NoScript 2.6.9.8, Giorgio landed several fixes for us that will result in changes to how Tor Browser interacts with NoScript. Here are the ones I'm aware of:

  1. We no longer need to add https: to the whitelist for the "Medium-High" security slider position due to fixes to noscript.globalHttpsWhitelist.

Should be fixed in my bug_13998 in my public Torbutton repo. Looking at NoScript code I found a more serious bug that is fixed now as well: we need to disable NoScript in the Medium/High mode. Otherwise any JavaScript resource is loaded as the HTTPS checks are only considered if JavaScript is disabled. See: isJSEnabled() in noscriptService.js.

Ah yes, I mentioned this in my comment on the security slider ticket: https://trac.torproject.org/projects/tor/ticket/9387#comment:70

I also think I found the "custom settings" flapping bug I mentioned there. I think we actually need to inspect all of the prefs upon update, to see if they actually match the expected values before setting (or unsetting) custom. I noted this in a comment, and also removed the pref observer for the NoScript whitelist pref, so that simply touching that pref doesn't cause custom settings. I pushed this in an extra commit on top of your work back to origin/master.

  1. Temporary permissions are no longer stored in the noscript.temp pref (which gets written to disk). Instead, they are stored in a new memory-only data structure. This means New Identity needs to change how it clears NoScript permissions.

What do you mean by "new memory-only data structure"? As far as I can see there is no such thing. tempSites and gTempSites are already available in 2.6.9.6 and cleared if we call eraseTemp. (The same happens still with 2.6.9.10 which is why we don't have to change anything wrt temporary permissions, I think)

Ah, I wasn't sure the noscript function properly dealt with this new mechanism. Sounds good.

  1. The pref noscript.volatilePrivatePermissions (which governs if temporary permissions are used for Private Browsing Mode) is false by default. We probably want to set it to true if disk records are disabled, but false if they are enabled. We will also need to ensure "New Identity" properly clears the permissions in both cases.

Looking at the code this pref is true by default beginning from 2.6.9.7, if I see that correctly. Grepping gives me something like:

./defaults/preferences/noscript.js:pref("noscript.volatilePrivatePermissions", true); 

And updating a freshly downloaded 4.5-alpha-2 to 2.6.9.10 shows true as well. Did you have something else here in mind?

Giorgio told me he was going to change this default in 2.6.9.8. I guess he changed his mind? Or delayed the change?

volatilePrivatePermissions should now be bound to no-disk/disk but as in the previous point you mentioned fixed there is no change in handling temporary permissions. This pref is more used to hide non-temporary menuitems and not to tamper with the nature of the permissions itself. I tested it a bit though and think we don't need to implement changes here unless we want to erase the permanent permissions as well on New Identity.

This looks good. As I said, I've merged your work, but you should perhaps look at my latest commit and comment. We don't need to rush that fix for the release, but we should fix that eventually.

Note: See TracTickets for help on using tickets.