NoScript is broken with TOR_SKIP_LAUNCH=1 in ESR 60-based Tor Browser

added AffectsTails TorBrowserTeam201808R component::applications/tor browser ff60-esr noscript owner::pospeselr priority::high resolution::fixed severity::normal status::closed type::defect labels

Just to clarify: NoScript itself doesn't appear to be broken - only the communication between Torbutton and NoScript.

Trac:
Username: rustybird

Thanks, rustybird! I can reproduce this. The cause of the reported error is that NoScript starts (or at least starts listening for messages) later than torbutton tries to send initial settings. Unfortunately I haven't figured out yet how to determine when NoScript is ready to receive messages.

Trac:
Cc: arthuredelstein, mcs, brade, rustybird@net-c.com to arthuredelstein, mcs, brade, rustybird@net-c.com, intrigeri

Moving first batch of tickets to July 2018

Trac:
Keywords: TorBrowserTeam201806 deleted, TorBrowserTeam201807 added

Here's a patch for review: https://github.com/arthuredelstein/torbutton/commit/26520

Trac:
Status: new to needs_review
Keywords: TorBrowserTeam201807 deleted, TorBrowserTeam201807R added

Here's a patch for review: https://github.com/arthuredelstein/torbutton/commit/26520

Now it seems to fire too late - on first site load (not including about:tor). If that first loaded site contains JavaScript, it will be disabled there. Tested on e.g. https://enable-javascript.com

I tried with a line log(5, "XXX sending"); added inside the sendNoScriptSettings function and noticed that both the first site load and moving the slider resulted in //multiple// messages to NoScript - not sure if that's a problem. (Slightly related, noscript-control.js has a guard against the initialized variable, but it's never set to true.)

log(e.message); -> log(5, e.message);

Trac:
Username: rustybird

Replying to rustybird:

Thanks for testing! Addressing your comments in rearranged order:

(Slightly related, noscript-control.js has a guard against the initialized variable, but it's never set to true.)

log(e.message); -> log(5, e.message);

Thanks for catching these two mistakes. Here's a revised patch that includes fixes for those: https://github.com/arthuredelstein/torbutton/commit/26520+1

Now it seems to fire too late - on first site load (not including about:tor). If that first loaded site contains JavaScript, it will be disabled there. Tested on e.g. https://enable-javascript.com

I tried with a line log(5, "XXX sending"); added inside the sendNoScriptSettings function and noticed that both the first site load and moving the slider resulted in //multiple// messages to NoScript - not sure if that's a problem.

I wasn't able to reproduce these two issues. I even tried setting https://enable-javascript.com as the browser's homepage, but in every case the NoScript state seems to be set correctly. Do you still see these issues with the revised patch?

I wasn't able to reproduce these two issues.

The "multiple messages to NoScript" problem is gone with the revised patch. I still have the "first site load" problem on a freshly extracted and patched TB - sorry if it's tediously verbose:

Run system tor on SocksPort 9150, ControlPort 9151
Extract tor-browser-linux64-8.0a9_en-US.tar.xz to a new directory
In the extensions directory, unzip -d torbutton@torproject.org/ torbutton@torproject.org.xpi and delete the .xpi
In torbutton@torproject.org/, apply the revised patch with -p2
Make a backup of the pristine patched tor-browser_en-US/ directory
Run TOR_SKIP_LAUNCH=1 ./start-tor-browser.desktop
Go to https://enable-javascript.com

It shows "JavaScript is disabled in your web browser" until I refresh the site, at which point it switches to "JavaScript is enabled". Same for other websites that depend on JavaScript, e.g. https://twitter.com/torproject (after restarting with a new copy of the backed up directory from step 5).

Trac:
Username: rustybird

Trac:
Status: needs_review to needs_revision
Keywords: TorBrowserTeam201807R deleted, TorBrowserTeam201807 added

Move our tickets to August.

Trac:
Keywords: TorBrowserTeam201807 deleted, TorBrowserTeam201808 added

Trac:
Username: rustybird

debug-retry.diff

Trac:
Username: rustybird

@arthuredelstein: As an experiment, I added a patch (debug-retry.diff) on top of your second patch. It reverts to the previous approach of immediately calling sendMessage() instead of waiting for a message from NoScript first, and it also retries on failure. Weirdly, after logging about ten failures and then a success to the Browser Console, JavaScript is still disabled! (On a fresh TB installation.) Maybe there's a second race condition somewhere, e.g. NoScript ignoring a message delivered successfully but very early?

BTW should this ticket have the AffectsTails keyword?

Trac:
Username: rustybird

Replying to rustybird:

BTW should this ticket have the AffectsTails keyword?

Thanks for caring! Tails does not use TOR_SKIP_LAUNCH=1. Instead we set TOR_CONFIGURE_ONLY=1 before starting Tor Launcher (as an independent XUL application). I've tested on my current build of Tails with Tor Browser 8.0a10 and:

NoScript seems to work just fine
I see JavaScript error: re[/gre/modules/ExtensionCommon.jsm,](/gre/modules/ExtensionCommon.jsm,) line 456: Error: Could not establish connection. Receiving end does not exist. in the logs.
If I raise the security slider to "Safest" then JavaScript is correctly disabled.
I don't know how to test whether "the communication between Torbutton and NoScript" works.

Let me know if there's any other info I can provide, happy to help :)

Replying to intrigeri:

Tails does not use TOR_SKIP_LAUNCH=1. Instead we set TOR_CONFIGURE_ONLY=1 before starting Tor Launcher (as an independent XUL application).

I think it's equivalent for this bug (except with maybe a slightly different timing in the race?) to use TOR_SKIP_LAUNCH=1, or extensions.torlauncher.start_tor=false, or to completely disable/remove the TorLauncher extension in the main Tor Browser profile (e.g. because you're starting it independently).

I see JavaScript error: re[/gre/modules/ExtensionCommon.jsm,](/gre/modules/ExtensionCommon.jsm,) line 456: Error: Could not establish connection. Receiving end does not exist. in the logs.

Yup, there it is. :\

If I raise the security slider to "Safest" then JavaScript is correctly disabled.

I don't know how to test whether "the communication between Torbutton and NoScript" works.

The symptom to look out for is JavaScript //not// working on websites even though it should be enabled, especially in the default state after installation. If you start a fresh browser and don't open the Torbutton Security Settings dialog at all, the slider level defaults to Standard, so JavaScript should be enabled - but:

situation on vanilla 8.0a9 and 8.0a10: You can only make JavaScript work if you move the slider away from Standard to Safer or Safest, confirm with OK, and then move it back to Standard again
situation with the proposed second patch: JavaScript only works (and the slider only becomes functional) after the first website has been loaded

Trac:
Username: rustybird

noscript-NoScript.started.diff

Trac:
Username: rustybird

torbutton-NoScript.started.diff

Trac:
Username: rustybird

Replying to rustybird:

Maybe there's a second race condition somewhere, e.g. NoScript ignoring a message delivered successfully but very early?

Looks like my debug-retry.diff sends the settings so early that NoScript will clobber them during its own startup. I've attached two new patches that ensure the message is sent at the right time, neither too soon nor too late:

noscript-NoScript.started.diff, if applied to NoScript (unix2dos < noscript-NoScript.started.diff | patch --binary -p2 inside an unpacked NoScript extension directory) makes it broadcast a NoScript.started type message after it has initialized. If someone wants to test it, don't forget to disable signature verification for the modified NoScript extension, e.g. by flipping the xpinstall.signatures.required pref to false.
torbutton-NoScript.started.diff, if applied to Torbutton (on top of arthuredelstein's second patch), waits for exactly that message.

Thoughts?

Trac:
Username: rustybird

Replying to rustybird:

Replying to intrigeri:

I don't know how to test whether "the communication between Torbutton and NoScript" works.

The symptom to look out for is JavaScript //not// working on websites even though it should be enabled, especially in the default state after installation. If you start a fresh browser and don't open the Torbutton Security Settings dialog at all, the slider level defaults to Standard, so JavaScript should be enabled - but:

situation on vanilla 8.0a9 and 8.0a10: You can only make JavaScript work if you move the slider away from Standard to Safer or Safest, confirm with OK, and then move it back to Standard again

Thanks for the detailed explanation :)

OK, so in Tails the browser homepage is https://tails.boum.org/home/. It does use JavaScript sometimes, and thankfully for the purpose of debugging this problem, we're currently in the (rather rare) situation where it does run some JS in a way that can trivially be noticed. So I could confirm that this piece of JS does run just fine on our homepage when starting Tor Browser 8.0a10 in Tails. Then, FWIW, I immediately set the security slider to "safest", reloaded the home page, and confirmed that our JS was not run this time. So I think the use cases we care about at Tails are not affected => no need for an AffectsTails keyword.

Replying to intrigeri:

So I could confirm that this piece of JS does run just fine on our homepage when starting Tor Browser 8.0a10 in Tails.

And the Could not establish connection. Receiving end does not exist. error from ExtensionCommon.jsm still appeared when you observed working JS? Sorry for pestering you, it's just baffling.

Trac:
Username: rustybird

Replying to rustybird:

Replying to intrigeri: And the Could not establish connection. Receiving end does not exist. error from ExtensionCommon.jsm still appeared when you observed working JS?

Yes (I've just tested again just to be sure).

Sorry for pestering you, it's just baffling.

No problem!

Trac:
Status: needs_revision to assigned
Owner: tbb-team to pospeselr

Trac:
Cc: arthuredelstein, mcs, brade, rustybird@net-c.com, intrigeri to arthuredelstein, mcs, brade, rustybird@net-c.com, intrigeri, tbb-team

Trac:
Keywords: N/A deleted, noscript added

rustybird: Are things still broken for you using the latest Tor Browser nightly (http://f4amtbsowhix7rrf.onion/tor-browser-builds/)? (make sure to update NoScript first, before doing your testing)

Trac:
Priority: Very High to High
Status: assigned to needs_information

Replying to gk:

rustybird: Are things still broken for you using the latest Tor Browser nightly (http://f4amtbsowhix7rrf.onion/tor-browser-builds/)? (make sure to update NoScript first, before doing your testing)

Yes, the 2018-08-25 Tor Browser nightly, with NoScript manually updated to 10.1.8.23 before first start and Torbutton patched with the fix for #27276 (moved), is broken in the same way as before.

Trac:
Username: rustybird

Replying to intrigeri:

Replying to rustybird:

And the Could not establish connection. Receiving end does not exist. error from ExtensionCommon.jsm still appeared when you observed working JS?

Yes (I've just tested again just to be sure).

I tried it out with tails-amd64-feature_15803-tor-browser-8.0a10+force-all-tests-3.9-20180819T1720Z-575956dc62+testing@b05edb901d.iso (apparently deleted from https://nightly.tails.boum.org/ in the meantime):

Torbutton 2.0.2 from Tor Browser 8.0a10 does not have the initialized = true fix yet, so sendMessage() happens to be called twice on init. The first time it fails with the error message, the second time it succeeds for me. Same as you, apparently. Maybe my old test notebook causes delays in just the right order before the second sendMessage(), giving NoScript time to become ready? Whereas with vanilla 8.0a10 in my Fedora 28 test VM, both sendMessage() calls fail and I see the error twice.

With the Torbutton fix (included in Torbutton >= 2.0.4), the bug does seem to consistently affect Tails in my testing.

Trac:
Username: rustybird

Replying to rustybird:

I tried it out with tails-amd64-feature_15803-tor-browser-8.0a10+force-all-tests-3.9-20180819T1720Z-575956dc62+testing@b05edb901d.iso (apparently deleted from https://nightly.tails.boum.org/ in the meantime):

FYI that's because the corresponding topic branch was merged into our testing branch so our CI stopped building it and GC'ed the now obsolete artifacts. Until Tails 3.9 is out, https://nightly.tails.boum.org/build_Tails_ISO_testing/lastSuccessful/archive/build-artifacts/ is the closest thing to what we'll release.

With the Torbutton fix (included in Torbutton >= 2.0.4), the bug does seem to consistently affect Tails in my testing.

Thanks for testing & reporting! I'll reproduce this and will assess how problematic this is for Tails (hopefully today or tomorrow).

Replying to intrigeri:

Replying to rustybird: Thanks for testing & reporting! I'll reproduce this and will assess how problematic this is for Tails (hopefully today or tomorrow).

Here are my test results on current Tails testing branch. Thanks to gk for the guidance!

no change: cannot reproduce
upgrade Torbutton to a XPI built from commit c93f06417e0bdbd6d709ec5109938347d1d38123 with ./makexpi.sh: reproduced the bug; all pages are affected, not only the 1st loaded one
upgrade Torbutton to a XPI built from commit c93f06417e0bdbd6d709ec5109938347d1d38123 with ./makexpi.sh, upgrade NoScript to https://secure.informaction.com/download/releases/noscript-10.1.8.23.xpi: reproduced the bug; all pages are affected (not only the 1st loaded one) until I do the trick gk suggested i.e. set the security slider to safer, apply, set the security slider back to standard, apply.

Anyone who wants to reproduce this or try to fix it in the context of Tails:

grab an ISO from https://nightly.tails.boum.org/build_Tails_ISO_testing/lastSuccessful/archive/build-artifacts/
boot that ISO in a x86-64 VM with at least 2GB of RAM
in the Tails Greeter ("Welcome to Tails"), set an administration password so you can use sudo in the next step: click the "+" button, click "Administration Password", etc.
replace whatever XPI you want: they live in /usr/local/share/tor-browser-extensions
start tor-browser from a terminal to see its stdout (otherwise it's sent to the systemd Journal that you can query with sudo journalctl).

I'm happy to provide more guidance or to test stuff myself in the context of Tails :)

Okay, I've updated my one-liner NoScript patch (that makes it send a "started" message) to its new messaging system - this is v2-noscript-started.diff and it supersedes noscript-NoScript.started.diff. See comment:14 for how to apply it to an unpacked NoScript extension directory. I'm going to open a NoScript pull request if nobody objects, maybe it can land in time for the Tor Browser 8.0 release.

As for my other patch, torbutton-NoScript.started.diff, on second thought I think it shouldn't be applied. Because in the unlikely case that the NoScript WebExtension finishes startup sooner than the unbootstrapped XPCOM Torbutton extension, which would mean that Torbutton (patched with arthuredelstein's proposed +1 patch rebased on top of aa379dc) misses the "started" message, it would be a more graceful failure mode to at least submit the slider settings after the first webpage has loaded and NoScript has sent a non-"started" message (instead of never submitting the slider settings at all). NoScript currently doesn't seem to send any messages //before// its own initialization phase has finished, so it wouldn't clobber the configuration (see comment:11) if Torbutton just reacts to the first message, whatever it may be.

To summarize, IMHO Tor Browser 8.0 final should ship with:

Torbutton with arthuredelstein's proposed +1 patch rebased on top of aa379dc
NoScript with v2-noscript-started.diff, hopefully as an upstream release

As far as I can tell, this allows an escape from the maze of race conditions...

Trac:
Username: rustybird

v2-noscript-started.diff

supersedes noscript-NoScript.started.diff [and added semicolon]

Trac:
Username: rustybird

Giorgio, does comment:26 sound like a good idea to you?

Trac:
Cc: arthuredelstein, mcs, brade, rustybird@net-c.com, intrigeri, tbb-team to arthuredelstein, mcs, brade, rustybird@net-c.com, intrigeri, tbb-team, ma1

Replying to gk:

Giorgio, does comment:26 sound like a good idea to you?

Yes, it makes sense. I'm gonna accept the PR as soon as I get it notified.

https://github.com/hackademix/noscript/pull/16 (merged)

Trac:
Username: rustybird

Replying to rustybird:

https://github.com/hackademix/noscript/pull/16

Merged, thanks.

Replying to ma1:

Replying to rustybird:

https://github.com/hackademix/noscript/pull/16

Merged, thanks.

Great, thanks. Giorgio, could you get a new NoScript version out, let's say on Wednesday european time, including this fix? We thought about starting building Tor Browser 8 on that evening and Tails needs that one as they don't have extensions updates enabled.

Trac:
Status: needs_information to new

Trac:
Username: rustybird

arthuredelstein-26520_1-rebased.patch

Trac:
Username: rustybird

Replying to gk:

Replying to ma1:

Replying to rustybird:

https://github.com/hackademix/noscript/pull/16

Merged, thanks.

Great, thanks. Giorgio, could you get a new NoScript version out, let's say on Wednesday european time, including this fix?

All right, can you start testing 10.1.9rc2, which includes this patch.

Many thanks to rustybird, Giorgio and intrigeri for your work on this!

Replying to rustybird:

in the unlikely case that the NoScript WebExtension finishes startup sooner than the unbootstrapped XPCOM Torbutton extension

Do we know for sure that NoScript loading first is unlikely? I'm not actually sure about what determines the order of XPIs loading.

If we wanted to be absolutely sure, NoScript could be patched to listen for a "ping" and reply with a "pong". And then torbutton could repeatedly send "ping" (say, once a second) until it receives a "pong", and then proceed by sending the first updateSettings message.

Giorgio, would that sort of patch sound reasonable to you? If so, I can send a pull request.

Replying to arthuredelstein:

Do we know for sure that NoScript loading first is unlikely? I'm not actually sure about what determines the order of XPIs loading.

If your XPI is a bootstrapped (legacy) extensions and does all its initialization in its startup callback yes, it's impossible for NoScript to be initialized first: WebExtensions are loaded asynchronously, most if not all the WebExtensions APIs are asynchronous as well. As a matter of facts, NoScript needs some ugly hacks (like blocking and reloading pages retrieved from session restore) in order not reliably enforce its policy on startup (see deferredWebTraffic.js).

If we wanted to be absolutely sure, NoScript could be patched to listen for a "ping" and reply with a "pong". And then torbutton could repeatedly send "ping" (say, once a second) until it receives a "pong", and then proceed by sending the first updateSettings message.

Giorgio, would that sort of patch sound reasonable to you? If so, I can send a pull request.

Not knowing what kind of (possibly asynchronous) work you're doing on your side before you're ready, I'm willing to check a patch, provided that it doesn't break other clients different than the Tor Browser.

FWIW I've tested Tails (testing branch i.e. 8.0a10) + Torbutton built from commit c93f06417e0bdbd6d709ec5109938347d1d38123 + NoScript 10.1.9rc2 and I can still reproduce the bug for all pages (not just the 1st loaded one). The security slider back'n'forth trick "fixes" it.

Alright, I uploaded a Torbutton .xpi containing the fix on top of master for easier testing:

https://people.torproject.org/~gk/misc/torbutton-2.0.4.xpi https://people.torproject.org/~gk/misc/torbutton-2.0.4.xpi.asc

Tails (testing branch i.e. 8.0a10) + the Torbutton 2.0.4 XPI uploaded by gk + NoScript 10.1.9rc2: the bug is fixed! \o/ :)

Replying to ma1:

If your XPI is a bootstrapped (legacy) extensions and does all its initialization in its startup callback yes, it's impossible for NoScript to be initialized first

Torbutton as an unbootstrapped / XUL overlay flavor of legacy extension should load even earlier, right? Although I'm unfamiliar with Torbutton's startup and don't know about the potential async initialization work you mentioned.

Replying to arthuredelstein:

If we wanted to be absolutely sure, NoScript could be patched to listen for a "ping" and reply with a "pong". And then torbutton could repeatedly send "ping" (say, once a second) until it receives a "pong", and then proceed by sending the first updateSettings message.

Sounds like an improvement. And if Torbutton accepts both "start" and the first "pong", there wouldn't even be an up to 1 second delay (which might be a problem if a URL is loaded quickly after browser startup, e.g. a non-about: homepage). Edit: Although even with both "start" and "ping"/"pong", AFAICT it's still not 100% certain that the updateSettings message is sent and handled before the first page is loaded...

Trac:
Username: rustybird

Replying to rustybird:

Although even with both "start" and "ping"/"pong", AFAICT it's still not 100% certain that the updateSettings message is sent and handled before the first page is loaded...

Well, IMHO the most reliable way to do it, no matter how long initialization takes on both sides, could be this:

NoScript needs a way to know very early (at the beginning of its execution) that it's running in the Tor Browser. Ideally, the browser.runtime.getBrowserInfo() API should return something unique, like {name: "Tor Browser", vendor: "The Tor Project"} (not sure if this requires a patch to the WebExtensions API or just setting some preference).
If it knows it's hosted by TB, NoScript will run its initialization code, including the part where it defers all the content network activity until ready, but won't unblock until a specific "updateSettings" message is received from the Tor Browser.

Anyway, if all the Tor Button initialization happens synchronously in a profile-after-change observer, like it was usually done in XUL extensions, it will definitely finish before NoScript starts.

Therefore, if this is the case, NoScript waiting for the "start" message to be answered (like in the original patch) should suffice, provided that this happens before we resolve the init() promise and give the green light to deferWebTraffic: hence my just released 10.1.9rc3 which reorders NoScript's startup sequence to guarantee this. Please test it.

NoScript is broken with TOR_SKIP_LAUNCH=1 in ESR 60-based Tor Browser

Child items ...

Activity