[PATCH] Fix NoScript IPC for about:blank by whitelisting messages

If about:blank is the homepage (or has been passed as a command line
parameter), NoScript will send a message named "fetchChildPolicy"
_before_ "started". Torbutton would then send its "updateSettings" too
soon, resulting in the dreaded error "Could not establish connection.
Receiving end does not exist" (see bug 26520).

Fix this by whitelisting the relevant messages from NoScript: "started"
and also "pageshow" for a slightly more graceful failure mode in case
Torbutton somehow misses NoScript startup.

Trac:
Username: rustybird

added TorBrowserTeam201809R component::applications/tor browser owner::arthuredelstein priority::very high reporter::rustybird resolution::fixed severity::normal status::closed tbb-8.0.1-can type::defect labels

Trac:
Username: rustybird

Fix-NoScript-IPC-for-about-blank-by-whitelisting-messages.patch

Trac:
Username: rustybird

Thanks for the patch rustybird. I assume Tor Browser as we ship it is not affected by this bug and neither is Tails? If that's true what use case does this break? We are way too late with our release candidates and I am a little hesitant to rebundle/rebuild yet another time. In other words: why does it have prio "Immediate"?

Trac:
Cc: tbb-team, gk, arthuredelstein, mc1 to tbb-team, gk, arthuredelstein, ma1

Trac:
Keywords: N/A deleted, TorBrowserTeam201809 added

Replying to gk:

Thanks for the patch rustybird. I assume Tor Browser as we ship it is not affected by this bug and neither is Tails? If that's true what use case does this break? We are way too late with our release candidates and I am a little hesitant to rebundle/rebuild yet another time. In other words: why does it have prio "Immediate"?

I guess it's only a problem for people who configure "Show a blank page" in "When Firefox starts" preferences. Unless there's some other situation in which NoScript sends very early messages?

Feel free to downgrade the priority. I just chose Immediate on the chance that you'd want to include it in the final. (To me it looks like a low risk patch that makes the approach more robust, but obv the constant rebuilding must be a giant PITA.)

Trac:
Username: rustybird

Thanks. I am inclined to move this to a fixup release which we likely release before the end of September, because we are running short on time to prepare Tor Browser 8. If that bug is indeed affecting default Tor Browser 8 as we ship it or Tails then we should reconsider.

Trac:
Priority: Immediate to Very High
Keywords: TorBrowserTeam201809 deleted, TorBrowserTeam201809R added
Status: new to needs_review

For any fellow blank homepage enthusiast stumbling upon this ticket: An ok workaround is to set the homepage address to data:, which does not trigger the bug.

Trac:
Username: rustybird

tbb-8.0.1-can

Trac:
Keywords: N/A deleted, tbb-8.0.1-can added

Replying to rustybird:

For any fellow blank homepage enthusiast stumbling upon this ticket: An ok workaround is to set the homepage address to data:, which does not trigger the bug. Thanks, I have always used about:blank. But what exactly is the failure mode here? Torbutton failing to re-configure NoScript?

Thanks, I have always used about:blank. But what exactly is the failure mode here? Torbutton failing to re-configure NoScript?

Yup. On browser startup with about:blank, Torbutton's intended JavaScript configuration is not transmitted to NoScript, and Tor Browser logs a "Could not establish connection. Receiving end does not exist" error in the Browser Console and also in the terminal if it's running with --verbose. (This is most noticable on first start of a freshly installed browser, when the Torbutton defaults and the NoScript defaults are very different. Once you manually change the security slider position and confirm, both of their settings sync up, and then NoScript will save them to disk and the problem should sort of disappear. That is, unless a system crash or something causes a new desynchronization.)

Interestingly, this bug recently triggered for me //once// with data:,. I'm running a setup where Tor Browser is started "freshly installed" dozens of times a day, so I figure that using data:, instead of about:blank just makes the race much less likely to be "won" by the bug, but not impossible. Which could mean that it occasionally affects real websites as well. Hopefully, the patch fixes all of those cases.

Trac:
Username: rustybird

Trac:
Status: needs_review to assigned
Owner: tbb-team to arthuredelstein

After testing in Tor Browser, Rusty Bird's patch looks good to me. I changed it to use the better JS equality operator, but otherwise it's the same:

Here it is rebased to origin/master:

https://github.com/arthuredelstein/torbutton/commit/27427

Trac:
Status: assigned to needs_review

Replying to rustybird:

the race much less likely to be "won" by the bug, but not impossible. Which could mean that it occasionally affects real websites as well. Hopefully, the patch fixes all of those cases.

Ok. And your fix is to ignore the message? Isn't it evident that there's a concurrency bug in NoScript that should be fixed? Just quickly skimming over the code I can see that handling a fetchChildPolicy message involves objects that are mutated (I suppose initialised) in init, the function that sends started when completes. Do you see?

Replying to arthuredelstein:

After testing in Tor Browser, Rusty Bird's patch looks good to me. I changed it to use the better JS equality operator, but otherwise it's the same:

Here it is rebased to origin/master:

https://github.com/arthuredelstein/torbutton/commit/27427

Please notice that this patch, using message._messageName, will work on NoScript 10.1.9.6rc3 (or 10.1.9.6 stable) and above, which gracefully handles "legacy" message recipients by adding this property for them: https://github.com/hackademix/noscript/commit/71c82b50849d8d70bb5bc410fcdf814837358143.

Currently all the message metadata, also the name, is meant to be confined in a __meta (notice the double underscore) object, which always includes a name property, like

message.__meta = {
  name: "start",
  recipientInfo: {},
  //... possibly other properties in future
}

Replying to ma1:

Currently all the message metadata, also the name, is meant to be confined in a __meta (notice the double underscore) object, which always includes a "name" property, like

{{{ message.__meta = { name: "start", recipientInfo: {}, //... possibly other properties in future } }}}

Of course, since this is an extensible mechanism which will allow us to pack as much metadata as we need in one single place, it's meant to be stable and not to change in any foreseeable future.

So, if the Tor Browser can start using __meta.name both on the receiving and the sending end, I'm gonna get rid of the "legacy" redundant _messageName property in one of the next releases.

Replying to cypherpunks3:

Replying to rustybird:

Which could mean that it occasionally affects real websites as well.

Ok. And your fix is to ignore the [fetchChildPolicy] message? Isn't it evident that there's a concurrency bug in NoScript that should be fixed?

Hmm, now that you mention it... If this race hypothetically affects real websites (i.e. not just about:blank and empty data: pages), then it seems like the NoScript policy would not be applied correctly for such a page, with or without the proposed Torbutton patch.

Trac:
Username: rustybird

Replying to rustybird:

Hmm, now that you mention it... If this race hypothetically affects real websites (i.e. not just about:blank and empty data: pages),

It should not: NoScript defers all the HTTP(S) traffic until its policy is configured and ready to be enforced. about:blank, data: and file: URLs are those which might suffer of this problem, because NoScript has no means to prevent them from loading before it's initialized.

Trac:
Username: rustybird

v3-Fix-NoScript-IPC-for-about-blank-by-whitelisting-messages.patch

Trac:
Username: rustybird

Trac:
Username: rustybird

Send-updateSettings-message-using-NoScript-10.1.9.2-protocol.patch

Trac:
Username: rustybird

Replying to ma1:

Replying to rustybird:

If this race hypothetically affects real websites (i.e. not just about:blank and empty data: pages),

It should not: NoScript defers all the HTTP(S) traffic until its policy is configured and ready to be enforced. about:blank, data: and file: URLs are those which might suffer of this problem, because NoScript has no means to prevent them from loading before it's initialized.

Thanks, that makes sense.

Replying to ma1:

So, if the Tor Browser can start using __meta.name both on the receiving and the sending end, I'm gonna get rid of the "legacy" redundant _messageName property in one of the next releases.

I've uploaded a v3 patch for the receiving end and a patch for the sending end.

Assuming that these patches land in Tor Browser 8.0.1, maybe NoScript could keep the legacy code for a little while, e.g. until Tor Browser 8.0.2 is released. This would be a grace period for Tor Browser 8.0**(.0)** users, so they don't automatically receive a NoScript extension update to an incompatible version.

Replying to arthuredelstein:

I changed it to use the better JS equality operator

Whoops yes, == is crappy. The v3 patch uses Array.prototype.includes() to make it shorter, so it's like === except that NaN would be considered equal to itself. Hope that's okay, I can change it if not.

Trac:
Username: rustybird

Replying to ma1:

It should not: NoScript defers all the HTTP(S) traffic until its policy is configured and ready to be enforced. Ok, so let's say it only breaks in harmless cases. Regardless, it still looks like a bug to me: the handler for fetchChildPolicy is running before making sure the state is properly initialised; for example, the object ns.policy is used and dereferenced in getForDocument even though it could still be null. Or maybe I'm wrong, I'm just reading this code now.

about:blank, data: and file: URLs are those which might suffer of this problem, because NoScript has no means to prevent them from loading before it's initialized. Does that mean that the approach mentioned there [ticket:27553#comment:3] is unreliable because of this race?

Replying to cypherpunks3:

Replying to ma1:

It should not: NoScript defers all the HTTP(S) traffic until its policy is configured and ready to be enforced. Ok, so let's say it only breaks in harmless cases. Regardless, it still looks like a bug to me: the handler for fetchChildPolicy is running before making sure the state is properly initialised; for example, the object ns.policy is used and dereferenced in getForDocument even though it could still be null. Or maybe I'm wrong, I'm just reading this code now.

I think you're wrong, indeed. The message handler gets enabled only after initialization is completed.

about:blank, data: and file: URLs are those which might suffer of this problem, because NoScript has no means to prevent them from loading before it's initialized. Does that mean that the approach mentioned there [ticket:27553#comment:3] is unreliable because of this race?

That approach is reliable, except on the very first load of a local (bypassing webRequest) page. I.e. if you set a file:, data: or about: URL as your home page (or, regrettably, if you're using session restore and that's the last active tab) NoScript won't be able to affect it on first load.

A work-around would be detecting some in the static content scripts (which appear to run anyway) that the dynamic content scripts didn't (i.e. NoScript's background script has not been initialized yet), stopping the load and triggering a (delayed?) reload. Maybe this could be helped by waiting for a response to a dummy HTTP fetch() request, relying on the web traffic deferral, in order to prevent reload loops in case something fails.

Replying to ma1:

I think you're wrong, indeed. The message handler gets enabled only after initialization is completed. Yep, I saw that. I conflated evidence of the message with evidence of the handling. That approach is reliable, except on the very first load of a local (bypassing webRequest) page. I.e. if you set a file:, data: or about: URL as your home page (or, regrettably, if you're using session restore and that's the last active tab) NoScript won't be able to affect it on first load. Thanks for the explanation. This seems reasonable to me (but probably it should be documented). A work-around would be detecting some in the static content scripts (which appear to run anyway) that the dynamic content scripts didn't (i.e. NoScript's background script has not been initialized yet), stopping the load and triggering a (delayed?) reload. Maybe this could be helped by waiting for a response to a dummy HTTP fetch() request, relying on the web traffic deferral, in order to prevent reload loops in case something fails. I don't understand it well, but it seems convoluted. Porbably webext should grow explicit API to cover these cases.

Okay, I think I take rustybird's original patch (slightly enhanced by Arthur) for the stable (8.0.1) and would take the new patches with the new protocol for the alpha to test (I'll open a new bug for that to not get confused). Barring any issues the stable after 8.0.1 would get the alpha patch backported. Does that work for you, ma1?

Replying to gk:

Okay, I think I take rustybird's original patch (slightly enhanced by Arthur) for the stable (8.0.1) and would take the new patches with the new protocol for the alpha to test (I'll open a new bug for that to not get confused). Barring any issues the stable after 8.0.1 would get the alpha patch backported. Does that work for you, ma1? That's OK, provided that the "stable" patch goes with NoScript 10.1.9.6 or above (which I suppose is gonna happen anyway).

I tested both of rustybird's patches from comment:16. They worked well and look good to me. So I think we're good to go for the alpha. For convenience here's a branch with both patches:

https://github.com/arthuredelstein/torbutton/commits/27427_alpha

Trac:
Status: needs_review to merge_ready

Replying to arthuredelstein:

After testing in Tor Browser, Rusty Bird's patch looks good to me. I changed it to use the better JS equality operator, but otherwise it's the same:

Here it is rebased to origin/master:

https://github.com/arthuredelstein/torbutton/commit/27427

Thanks. Cherry-picked to master (commit 8ff3b44e478ebddf5a067d796f57a353beae2af0). This will make it into 8.0.1.

I opened #27760 (moved) for using message.__meta in the communication with NoScript.

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

closed

mentioned in issue #27760 (moved)

mentioned in issue #32019 (moved)

mentioned in issue #34250 (moved)

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

mentioned in issue tpo/applications/tor-browser#27760 (closed)

mentioned in issue tpo/applications/tor-browser#32019 (closed)

mentioned in issue tpo/applications/tor-browser#34250 (closed)