Opened 8 months ago

Closed 7 months ago

#27427 closed defect (fixed)

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

Reported by: rustybird Owned by: arthuredelstein
Priority: Very High Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: TorBrowserTeam201809R, tbb-8.0.1-can
Cc: tbb-team, gk, arthuredelstein, ma1 Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

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.

Child Tickets

Change History (26)

comment:1 Changed 8 months ago by gk

Cc: ma1 added; mc1 removed

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"?

Last edited 8 months ago by gk (previous) (diff)

comment:2 Changed 8 months ago by gk

Keywords: TorBrowserTeam201809 added

comment:3 in reply to:  1 Changed 8 months ago by rustybird

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

comment:4 Changed 8 months ago by gk

Keywords: TorBrowserTeam201809R added; TorBrowserTeam201809 removed
Priority: ImmediateVery High
Status: newneeds_review

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.

comment:5 Changed 8 months ago by 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.

comment:6 Changed 7 months ago by gk

Keywords: tbb-8.0.1-can added

tbb-8.0.1-can

comment:7 in reply to:  5 Changed 7 months ago by cypherpunks3

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?

comment:8 Changed 7 months ago by rustybird

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.

Last edited 7 months ago by rustybird (previous) (diff)

comment:9 Changed 7 months ago by arthuredelstein

Owner: changed from tbb-team to arthuredelstein
Status: needs_reviewassigned

comment:10 Changed 7 months ago by arthuredelstein

Status: assignedneeds_review

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

comment:11 in reply to:  8 ; Changed 7 months ago by cypherpunks3

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?

comment:12 in reply to:  10 ; Changed 7 months ago by ma1

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
}
Last edited 7 months ago by ma1 (previous) (diff)

comment:13 in reply to:  12 ; Changed 7 months ago by ma1

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.

Last edited 7 months ago by ma1 (previous) (diff)

comment:14 in reply to:  11 ; Changed 7 months ago by rustybird

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.

comment:15 in reply to:  14 ; Changed 7 months ago by ma1

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.

comment:16 in reply to:  13 Changed 7 months ago by 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.

Last edited 7 months ago by rustybird (previous) (diff)

comment:17 in reply to:  15 ; Changed 7 months ago by 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.

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 27553#comment:3 is unreliable because of this race?

comment:18 in reply to:  17 ; Changed 7 months ago by ma1

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

comment:19 in reply to:  18 Changed 7 months ago by cypherpunks3

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.

comment:20 Changed 7 months ago by 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?

comment:21 in reply to:  20 Changed 7 months ago by 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).

comment:22 Changed 7 months ago by arthuredelstein

Status: needs_reviewmerge_ready

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

comment:23 in reply to:  10 Changed 7 months ago by gk

Resolution: fixed
Status: merge_readyclosed

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 for using message.__meta in the communication with NoScript.

Note: See TracTickets for help on using tickets.