Opened 11 months ago

Closed 9 months ago

Last modified 9 months ago

#24421 closed defect (fixed)

"Temporarily allow all this page" get inherited when New Identity is chosen.

Reported by: cypherpunks Owned by: tbb-team
Priority: High Milestone:
Component: Applications/Tor Browser Version:
Severity: Major Keywords: tbb-newnym, TorBrowserTeam201801
Cc: gk, mcs, brade, ma1 Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

How to reproduce:

  1. Open Tor Browser.
  2. Select "High" Security Setting.
  3. Go to https://github.com/
  4. Click on NoScript icon and choose "Temporarilly allow all this page".
  5. You can note that when the page gets refreshed the canvas prompt is displayed.
  6. Select New Identity in the Torbutton.
  7. Go to https://github.com/
  8. You can notice that the canvas prompt is displayed (meaning JS is enabled, which in turn implies that "Temporarilly allow all this page" gets inherited when New Identity is chosen.)

Child Tickets

Change History (26)

comment:1 Changed 11 months ago by gk

Keywords: TorBrowserTeam201711 tbb-newnym added
Priority: MediumHigh
Severity: NormalMajor

comment:2 Changed 11 months ago by gk

Moving tickets to December 2017

comment:3 Changed 11 months ago by gk

Keywords: TorBrowserTeam201712 added; TorBrowserTeam201711 removed

Moving tickets to December 2017, for realz.

comment:4 Changed 10 months ago by cypherpunks

Summary: "Temporarilly allow all this page" gets inherited when New Identity is chosen."Temporarily allow all this page" gets inherited when New Identity is chosen.

Workaround: click "Temporarily allow all this page" after New Identity to update settings.

comment:5 Changed 10 months ago by cypherpunks

I think there's something far more serious under the hood: I was uploading some files to some onion services using the Tor Browser, they (the uploads) timed out, so I refreshed each one and the upload was going again. I then closed the tabs where the upload was happening and I noticed that the upload speed in the Gnome System Monitor was at its max--meaning that the upload was still happening! When I clicked on New Identity, I saw yet again that the upload was still going!! (I had the Gnome System Monitor open while looking at the upload speed) It's only when I closed the Tor Browser that the upload speed returned to its idle value. So it's seems it's not just JS whitelists that get inherited.

comment:6 Changed 10 months ago by cypherpunks

Steps to reproduce for comment:5:

  1. Set High security setting (I was using that one, didn't bother to see if it affects other security settings).
  2. Go to the Bezos Washington Post Secure Drop: jcw5q6uyjioupxcc.onion (you can verify it here: securedrop.org/directory )
  3. Click on "Submit Documents".
  4. Click on "Use New Codename".
  5. Select some large file (we don't want the upload to finish so we don't waste the journalists' time wasting Bezos' money) like a Tor Browser tar.gz
  6. Open your Gnome System Monitor and go to "Resources" and watch the network graph. You should see the upload at its peek values.
  7. Close the tab, you can see that the upload is still going.
  8. Click on New Identity, again, upload is still going.
  9. Close the Tor Browser, upload halts.

It works for clearnet websites as well, you can test with https://catbox.moe/ following essentially the same procedure as the last ones above (i.e. 5-9).

comment:7 Changed 10 months ago by cypherpunks

Summary: "Temporarily allow all this page" gets inherited when New Identity is chosen."Temporarily allow all this page" and uploads get inherited when New Identity is chosen.

comment:8 Changed 10 months ago by gk

Cc: mcs brade added

Some weird things I learned so far:

1) This got introduced with commit 32f9cf56c89ccd2e24924975dc5515e4198d28c3 which fixes #18913.

2) This issue (the one in comment:description) is *not* reproducible during first start with a clean new bundle. However, starting with the second start the STR work for me. Digging deeper what prevents the bug to occur with a clean, new Tor Browser is

            // TBB 5.0a3 users had all the necessary data cached in
            // directoryLinks.json. This meant that resetting the pref above
            // alone was not sufficient as the tiles features uses the cache
            // even if the pref indicates that feature should be disabled.
            // We flip the preference below as this forces a refetching which
            // effectively results in an empty JSON file due to our spoofed
            // URLs.
            let matchOS = m_tb_prefs.getBoolPref("intl.locale.matchOS");
            m_tb_prefs.setBoolPref("intl.locale.matchOS", !matchOS);
            m_tb_prefs.setBoolPref("intl.locale.matchOS", matchOS);

which is run in torbutton.js on first start.

I am not sure yet how 1) and 2) fit together.

comment:9 Changed 10 months ago by cypherpunks

#24784 is a duplicate.

comment:10 Changed 9 months ago by gk

Keywords: TorBrowserTeam201801 added; TorBrowserTeam201712 removed

Moving tickets to 2018.

comment:11 Changed 9 months ago by gk

mcs, brade do you have some cycles to look at this bug? I probably won't, alas. :(

comment:12 in reply to:  11 Changed 9 months ago by mcs

Replying to gk:

mcs, brade do you have some cycles to look at this bug? I probably won't, alas. :(

Kathy and I spent some time debugging this today, but we did not find the root cause. Due to other committments, we may not have much time to work on this until next week, so I am posting what we learned so far here:

  • Running with e10s disabled (MOZ_FORCE_DISABLE_E10S=1) avoids the problem.
  • I am not convinced it is the cause of this bug because NoScript appears to catch the errors and ignore them, but when running with a debug build when e10s is enabled, several messages like the following are emitted right after the call to nsSvc.eraseTemp() (called from Torbutton's torbutton_do_new_identity() function):
    [Child 23233] ###!!! ASSERTION: ENSURE_MAIN_PROCESS failed. Cannot SetCharPref from content process: temp: 'Error', file /.../dev/tor/tor-browser/modules/libpref/nsPrefBranch.cpp, line 195
    
  • I don't know anything about the NoScript internals, but when this bug occurs it seems that eraseTemp() (inside NoScript's Main.js) is called twice: once directly by Torbutton and once in response to a browser:purge-session-history observer notification. But I don't know if the fact that eraseTemp() is called twice is causing a problem.

comment:13 Changed 9 months ago by mcs

Another idea came to me while I was doing something else: maybe there are actually two copies of the NoScript code running, and *that* is causing problems. A quick dump() added to the end of NoScript's Main.js shows it is being loaded twice, but I am not sure if that is by design or not.

comment:14 Changed 9 months ago by gk

Cc: ma1 added

Giorgio: Any ideas on that one?

comment:15 in reply to:  13 Changed 9 months ago by ma1

Replying to mcs:

A quick dump() added to the end of NoScript's Main.js shows it is being loaded twice, but I am not sure if that is by design or not.

By Design. Both MainParent and MainChild are built as mix-ins with Main (which is common to both), and this gets loaded twice in the same process if you've got e10s disabled, because of IPC "emulation" (the MessageManager APIs try to show you the same multi-process view of the environment even if e10s is off and everything lives in a single actual process).

comment:16 Changed 9 months ago by mcs

After some more debugging, Kathy and I have learned that the root of the problem is that some NoScript data structures are not correctly synchronized between the chrome process and the content process when e10s is enabled. Specifically, after "Temporarily allow all this page" is done for a site, one of the Main.js instances does not record the site inside this.tempSites. Later when eraseTemp() is called to remove all temporary sites, the site is not removed.

I don't know if it is the correct fix, but this bug disappeared after we changed ns.bootstrap to include setTemp in the list of functions included in the IPC.autoSync() call:

    try {
      IPC.autoSync(this, "Main", ["setJSEnabled", "setTemp", "eraseTemp", "allowObject", "resetAllowedObjects", "shutdown"]);
    } catch (e) {

Giorgio, can you take it from here?

comment:17 in reply to:  16 ; Changed 9 months ago by ma1

Replying to mcs:

    try {
      IPC.autoSync(this, "Main", ["setJSEnabled", "setTemp", "eraseTemp", "allowObject", "resetAllowedObjects", "shutdown"]);
    } catch (e) {

Giorgio, can you take it from here?

It makes sense, indeed, thank you for your finding.
I'm putting it in next 5.x, to be released ASAP.

comment:18 Changed 9 months ago by ma1

Please check 5.1.6.4rc1 from https://noscript.net/getit#devel, thanks.

comment:19 in reply to:  18 Changed 9 months ago by mcs

Replying to ma1:

Please check 5.1.6.4rc1 from https://noscript.net/getit#devel, thanks.

You meant 5.1.8.4rc1, correct?
Kathy and I tested it and it fixes this bug. We will leave it up to Georg to decide when to include this version of NoScript in Tor Browser.

comment:20 in reply to:  17 ; Changed 9 months ago by gk

Replying to ma1:

Replying to mcs:

    try {
      IPC.autoSync(this, "Main", ["setJSEnabled", "setTemp", "eraseTemp", "allowObject", "resetAllowedObjects", "shutdown"]);
    } catch (e) {

Giorgio, can you take it from here?

It makes sense, indeed, thank you for your finding.
I'm putting it in next 5.x, to be released ASAP.

Giorgio, how's the release going? I don't think we want to put a devel version of NoScript into Tor Browser...

comment:21 in reply to:  20 Changed 9 months ago by cypherpunks

Replying to gk:

I don't think we want to put a devel version of NoScript into Tor Browser...

devel? It's on AMO's beta channel and marked as rc by the author, so it's more than suitable for alphas and nightlies.

comment:22 Changed 9 months ago by ma1

Just released 5.1.8.4 as stable.
Please also integrate [System+Principal] in your noscript.mandatory preference in order to fix #25000, thanks.

comment:23 Changed 9 months ago by gk

Resolution: fixed
Status: newclosed

Thanks. Closing this ticket then.

comment:24 Changed 9 months ago by cypherpunks

Georg, did you test as well the upload issue?

comment:25 in reply to:  24 Changed 9 months ago by gk

Summary: "Temporarily allow all this page" and uploads get inherited when New Identity is chosen."Temporarily allow all this page" get inherited when New Identity is chosen.

Replying to cypherpunks:

Georg, did you test as well the upload issue?

No. It is not related this this issue. And I am not sure how we should deal with that. See for instance #10426 which deals with the download case where users are complaining that New Identity does indeed abort the in-progress download.

comment:26 Changed 9 months ago by gk

I've opened #25082 for the upload issue.

Note: See TracTickets for help on using tickets.