Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NoScript + uMatrix = cookie leak #554

Closed
scratchyleet opened this Issue May 24, 2016 · 32 comments

Comments

Projects
None yet
5 participants
@scratchyleet
Copy link

scratchyleet commented May 24, 2016

Requirements:

  • Firefox 46.0.1.
  • uMatrix 0.9.3.3.
  • NoScript 2.9.0.11.

Reproducing steps:

  • Create a new profile.
  • Install uMatrix.
  • Install NoScript.
  • Close a browser.
  • Open a browser.
  • Close a browser.
  • Open a browser.
  • Open page https://github.com/gorhill/httpswitchboard/wiki
  • Find string "Are cookies blocked?". Open its link in a new tab.
  • Press uMatrix icon. Press lower half block under "cookie" opposite to the right from "raymondhill.net". (or just block cookies to raymondhill.net) Press "lock" icon. ("Save all temporary changes for this scope.")
  • Close a browser.
  • Open a browser.
  • Open page https://github.com/gorhill/httpswitchboard/wiki
  • Find string "Are cookies blocked?". Open its link in a new tab.
  • Press "Reload current page" icon.

Marvelously works at first browser start. If you close a tab and open it again it won't happen. Unless you restart a browser.

Temporary (known) workaround:

  • Disable NoScript. Completely.
  • Close a browser.
  • Open a browser.
  • Close a browser.
  • Open a browser.
  • Open page https://github.com/gorhill/httpswitchboard/wiki
  • Find string "Are cookies blocked?" and open its link in a new tab.
  • Press "Reload current page" icon to check if it worked.

Good luck fixing that.

@gorhill

This comment has been minimized.

Copy link
Owner

gorhill commented May 24, 2016

Everything works fine here.

@gorhill gorhill closed this May 24, 2016

@gorhill gorhill added the invalid label May 24, 2016

@gorhill gorhill reopened this May 24, 2016

@gorhill

This comment has been minimized.

Copy link
Owner

gorhill commented May 24, 2016

I could reproduce, somehow, need to figure out how.

@gorhill

This comment has been minimized.

Copy link
Owner

gorhill commented May 24, 2016

Finding so far:

With NoScript enabled, the data uBO attaches to the channel instance in its http observer[1] can no longer be found. This could be because this data is trashed somehow, or because NoScript creates an entirely new channel instance, causing the older one to be discarded. Will need insights from NoScript people.

In the current state, uMatrix is incompatible with NoScript: NoScript appears to clobber uMatrix's internal data.

[1] Line 2048 of vapi-background.js

@gorhill gorhill removed the invalid label May 24, 2016

@gorhill

This comment has been minimized.

Copy link
Owner

gorhill commented May 24, 2016

By the way, "cookie leak" is just one symptom of the issue here, the bigger problem is that other parts of uMatrix are crippled by the loss of contextual data when NoScript is enabled.

@gorhill

This comment has been minimized.

Copy link
Owner

gorhill commented May 24, 2016

@laniakea64 Any quick insight about why the channel data is lost (or why the channel at http-on-modify-request time is not the same as the one at http-on-opening-request)?

@laniakea64

This comment has been minimized.

Copy link

laniakea64 commented May 24, 2016

Now that you mention it I seem to recall something about NoScript cloning channels but can't remember exactly what (or when). Any suggestions what to search for in the NoScript code?

@gorhill

This comment has been minimized.

Copy link
Owner

gorhill commented May 24, 2016

I am currently looking here, it seems this is called for every http-on-opening-request.

@gorhill

This comment has been minimized.

Copy link
Owner

gorhill commented May 24, 2016

If I un-check NoScript's "Enable ABE", the issue goes away.

@laniakea64

This comment has been minimized.

Copy link

laniakea64 commented May 24, 2016

Unfortunately for debugging, that "Enable ABE" checkbox in practice disables more than just ABE itself...

Just looking through the code I'm not seeing what would cause this. I'll play around with this some and post back if I find anything useful.

@gorhill

This comment has been minimized.

Copy link
Owner

gorhill commented May 24, 2016

ABE.js uses ChannelReplacement objects, defined here.

@laniakea64

This comment has been minimized.

Copy link

laniakea64 commented May 24, 2016

I don't seem to be able to reproduce the issue either with SeaMonkey or with Firefox 46.0.1. Can please clarify the instructions "Close a browser" and "Open a browser" - does "browser" refer to a browser tab, browser window, the whole application, or..? Also, is there something special about the cookie testing page on raymondhill.net and/or accessing from Github that makes this impossible to reproduce on a local server?

(In case it could be platform-specific, I'm running Linux x86_64)

@gorhill

This comment has been minimized.

Copy link
Owner

gorhill commented May 24, 2016

The repro steps for me are these:

From uMatrix's point of view, the properties it sets on the channel at http-on-opening-request time are gone once http-on-modify-request is fired.

@laniakea64

This comment has been minimized.

Copy link

laniakea64 commented May 24, 2016

Thanks gorhill, I can reproduce the issue in SeaMonkey with that set of steps-to-reproduce (on the last step, forcing reload from uMatrix).

Apparently this is not due to a recent change in NoScript - I'm getting the same result with every NoScript version that actually works with a modern browser. I'll try debugging NoScript a bit, see if I can find where exactly the data is getting lost...

@laniakea64

This comment has been minimized.

Copy link

laniakea64 commented May 25, 2016

Been trying to see if it's possible to resolve this with a patch to NoScript, haven't cracked this one so far although I did find out that if I cause an error in the initialization code for ABERequest objects then the issue goes away (but probably at the expense of borking NoScript, despite the total lack of any error messages)

I'll see if we can bring Giorgio in this if I don't solve it soon.

@gorhill

This comment has been minimized.

Copy link
Owner

gorhill commented May 26, 2016

How do you debug NoScript? It does not show up in the list at about:debugging#addons on my side.

@laniakea64

This comment has been minimized.

Copy link

laniakea64 commented May 26, 2016

Oh, I'm not doing that kind of debugging, I just modify the xpi and insert dump() calls in possibly-relevant places in the code and hope something means anything? 😕
Was trying to see if there's a point in the NoScript code where the channel still has its umatrixreqdata and where it doesn't, and go from there. Now I'm trying to work backwards from the fact that disabling ABE gets it working.

@laniakea64

This comment has been minimized.

Copy link

laniakea64 commented May 26, 2016

OK here is what I think is going on. (Testing NoScript 2.9.0.12rc1)
NoScript uses redirectTo() method of HTTP channels and it is redirectTo() itself which is causing uMatrix data to be dropped. If I comment out this line in NoScript's ChannelReplacement.js (making no other changes) then uMatrix works as expected:

      chan.redirectTo(this.newURI);

I'm thinking it's not a NoScript bug per se but maybe Giorgio can at least work around it.

@laniakea64

This comment has been minimized.

@hackademix

This comment has been minimized.

Copy link

hackademix commented May 26, 2016

Giorgio here.
What kind of properties does uMatrix set on the channel?
Depending on whether they're expected to be persisted across "normal" redirections (as it happens for nsIPropertyBag ones) we could either file (and possibly fix) a Mozilla Necko bug to make redirectTo() mimic more closely HTTP redirections, or uMatrix could use a ChannelEventSink to set them on the new, redirected channel.

There's actually a 3rd solution/work-around: if a proper fix in redirectTo() is considered too risky, NoScript itself could use its own old, pre-redirectTo() redirection emulation trickery to "augment" Necko with property copy...

@gorhill

This comment has been minimized.

Copy link
Owner

gorhill commented May 27, 2016

uMatrix could use a ChannelEventSink to set them on the new, redirected channel.

That's what uMatrix does already (see here).

The properties are the tab id (string), request type (number) (see here -- the comment is not accurate, the attached data is also useful for other phases of the request: http-on-modify-request, http-on-examine-response).

@gorhill

This comment has been minimized.

Copy link
Owner

gorhill commented May 27, 2016

I don't have time to investigate more for now, but I just found that this line seems to throw an exception. So that could be the problem? (or just another symptom of whatever the reason it is that umtrixreqdata can no longer be found).

@gorhill

This comment has been minimized.

Copy link
Owner

gorhill commented May 27, 2016

Correction to my comment above: there is no exception, it seems to just be a debugger quirk, giving the impression that an exception is throw.

Something else I found: uMatrix properties are not attached on the channel in the first place because http-on-opening-request is never fired if the debugger is to be believed. I see only http-on-modify-request and http-on-examine-... notifications.

So at this point it comes down to find out why http-on-opening-request is not fired for uMatrix in the current scenario.

@gorhill

This comment has been minimized.

Copy link
Owner

gorhill commented May 29, 2016

Latest finding is that http-on-opening-request is not fired for uMatrix in the reported problematic scenario. When I force a refresh of the page, this is what uMatrix is notified about with NoScript enabled:

uMatrix: 69 observe() url="http://raymondhill.net/httpsb/httpsb-test-cookie-1.php" topic="http-on-modify-request"
uMatrix: 70 asyncOnChannelRedirect() url="http://raymondhill.net/httpsb/httpsb-test-cookie-1.php"
uMatrix: 71 observe() url="http://raymondhill.net/httpsb/httpsb-test-cookie-1.php" topic="http-on-modify-request"
uMatrix: 72 observe() url="http://raymondhill.net/httpsb/httpsb-test-cookie-1.php" topic="http-on-examine-response"

The number following "uMatrix" is a simple counter to be sure to understand the sequence of event in time.

If I disable NoScript, http-on-opening-request is fired every time I force a refresh

uMatrix: 84 observe() url="http://raymondhill.net/httpsb/httpsb-test-cookie-1.php" topic="http-on-opening-request"
uMatrix: 85 observe() url="http://raymondhill.net/httpsb/httpsb-test-cookie-1.php" topic="http-on-modify-request"
uMatrix: 86 observe() url="http://raymondhill.net/httpsb/httpsb-test-cookie-1.php" topic="http-on-examine-response"
@hackademix

This comment has been minimized.

Copy link

hackademix commented May 29, 2016

That's expected, because NoScript uses this internal redirection to delay anti-CSRF protection processing until DNS resolution is complete to mitigate DNS rebinding attacks.
As I suggested earlier, the problem appears to be caused by

  1. redirectTo() not copying the extra properties from the old channel to the new one (opposite to what happens for "organic" redirections) and
  2. http-on-opening-request never being called for any redirection (by design).

An easy work-around would be assigning your extra properties in http-on-modify-request, I guess.

@gorhill

This comment has been minimized.

Copy link
Owner

gorhill commented Jun 2, 2016

An easy work-around would be assigning your extra properties in http-on-modify-request, I guess.

This also mean that any extension which relies on http-on-opening-request would have to implement such workaround just for the sake of working properly with NoScript specifically.

I could probably add a code path to uBO/uMatrix for the sake of working around http-on-modify-request, but that does not feel right to have such a workaround just in case one other specific extension is installed.

@hackademix

This comment has been minimized.

Copy link

hackademix commented Jun 2, 2016

This also mean that any extension which relies on http-on-modify-request would have to implement such workaround just for the sake of working properly with NoScript specifically.

Not quite. It just means that you should not modify requests (e.g. by adding headers or properties) in "http-on-opening-request": as the name suggests, the proper place is "http-on-modify-request".

Furthermore, this bug is not triggered specifically by NoScript, but by any add-on (or Gecko internal code) which uses nsIHttpChannel.redirectTo() (and this is gonna be quite common in the brave new WebExtensions world, since webRequest's redirectUrl implementation does exactly that).

@gorhill

This comment has been minimized.

Copy link
Owner

gorhill commented Jun 2, 2016

you should not modify requests (e.g. by adding headers or properties) in "http-on-opening-request"

Neither uBlock or uMatrix do this. Headers are modified strictly at http-on-modify-request and/or http-on-examine-response time.

this is gonna be quite common in the brave new WebExtensions world, since webRequest's redirectUrl implementation does exactly that

With Chrome API, never does the use of redirection prevent onBeforeRequest from being properly fired to all listening extensions.

I strongly believe that it is reasonable for an extension subscribing to http-on-opening-request to always be notified through http-on-opening-request for every new request. This is the issue here, the http-on-opening-request event is being "eaten" from the point of view of those extensions which listen to http-on-opening-request events.

@hackademix

This comment has been minimized.

Copy link

hackademix commented Jun 2, 2016

You should not modify requests (e.g. by adding headers or properties) in "http-on-opening-request"

Neither uBlock or uMatrix do this. Headers are modified strictly at http-on-modify-request and/or http-on-examine-response time.

Maybe I misunderstood the whole issue, then: do or do not your add-ons modify the request by adding custom properties in "http-on-opening-request"? If they actually do, please move that modification code in "http-on-modify-request".

With Chrome API, never does the use of redirection prevent onBeforeRequest from being properly fired to all listening extensions.

In facts, Firefox's onBeforeRequest implementation relies on "http-on-modify-request", not on "http-on-opening-request": therefore WebExtensions themselves will work just fine, but legacy add-ons which use http-on-opening-request will get confused by WebExtensions' redirections.

I strongly believe that it is reasonable for an extension subscribing to http-on-opening-request to always be notified through http-on-opening-request for every new request.

No, i think it's not. The docs do not guarantee this, and discourage its usage unless you've got a very narrow use case for it:

http-on-opening-request - Similar to http-on-modify-request, but called earlier (synchronously during the channel's asyncOpen() call), and some channel atttributes (proxyInfo) may be missing. Use only if your observer must be called before asyncOpen returns.

At any rate, http-on-opening-request observers are not notified for redirections, there's specific Necko code in nsHttpChannel.cpp preventing this, so if you need to process redirected channels just do not rely on that notification.

@gorhill

This comment has been minimized.

Copy link
Owner

gorhill commented Jun 2, 2016

In facts, Firefox's onBeforeRequest implementation relies on "http-on-modify-request", not on "http-on-opening-request"

Thanks for the information. I really want to be sure I understand properly all this. The doc on chrome.onBeforeRequest says this (my emphasis):

onBeforeRequest (optionally synchronous)
Fires when a request is about to occur. This event is sent before any TCP connection is made and can be used to cancel or redirect requests.

Is "sent before any TCP connection is made" also true for Firefox's http-on-modify-request?

@hackademix

This comment has been minimized.

Copy link

hackademix commented Jun 3, 2016

Is "sent before any TCP connection is made" also true for Firefox's http-on-modify-request?

Yes, unless of course it's a previously established persistent connection (same host, Connection: Keep-Alive), which I'm pretty sure happens also for Chrome's onBeforeRequest, despite the ambiguous promise made by the doc (but I believe it's not relevant for our own use cases, is it?)

@gorhill

This comment has been minimized.

Copy link
Owner

gorhill commented Jun 3, 2016

Yes

Well then, I should be able to move everything at http-on-modify-request with little modifications for both uMatrix/uBO.

@gorhill gorhill closed this in 73646ed Jun 3, 2016

gorhill added a commit to gorhill/uBlock that referenced this issue Jun 3, 2016

ahmadassaf added a commit to ahmadassaf/uBlock that referenced this issue Jun 3, 2016

Merge branch 'master' of https://github.com/gorhill/uBlock
* 'master' of https://github.com/gorhill/uBlock:
  fix gorhill/uMatrix#554
  fix uBlock-LLC#1673
  fix typo in comment
  fix uBlock-LLC#1662
  fix gorhill/uBlock#1660
  fix uBlockOrigin/uAssets#50
  fix uBlock-LLC#1629
  fix the undue discarding of logger events in edge cases
  EasyList and EasyPrivacy have moved (uBlock-LLC#1612)
  fix uBlock-LLC#1607

gorhill added a commit to gorhill/uBlock that referenced this issue Jun 6, 2016

code review re. gorhill/uMatrix#554
No longer need to evaluate within asyncOnChannelRedirect() since
all is now evaluated at `http-on-modify-request` time.

gorhill added a commit that referenced this issue Jun 6, 2016

code review re. #554
No longer need to evaluate within asyncOnChannelRedirect() since
all is now evaluated at `http-on-modify-request` time.

gorhill added a commit that referenced this issue Jun 10, 2016

gorhill added a commit to gorhill/uBlock that referenced this issue Jun 10, 2016

@davidhedlund

This comment has been minimized.

Copy link

davidhedlund commented Jun 23, 2016

@gorhill

uBlock Origin ("uBO") and uMatrix must be considered incompatible with NoScript.

ABP can be used with NoScript without any problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.