Opened 7 years ago

Closed 6 years ago

Last modified 18 months ago

#5477 closed defect (fixed)

Surprising DOM origins before HTTPS-E/NoScript redirects have completed

Reported by: Drugoy Owned by: ma1
Priority: Immediate Milestone:
Component: HTTPS Everywhere/EFF-HTTPS Everywhere Version:
Severity: Normal Keywords: MikePerry201207
Cc: mikeperry, schoen, theo.chevalier11@…, g.maone@… Actual Points: 20 (pde: 6? mikeperry: 14)
Parent ID: Points: 20
Reviewer: Sponsor:

Description

http://majorsecurity.net/html5/ios51-demo.html

^ Here is the demo of address spoofing.

With HTTPS-Everywhere enabled in latest Nightly - clicking the button opens a new tab with "apple.com" address. But this is a spoofed address, press CTRL+U to watch the source code of that page.

Child Tickets

Attachments (7)

0001-Merge-NoScript-2.3.7-s-ChannelReplacement-stuff.patch (15.8 KB) - added by ma1 7 years ago.
Patch: Merges NoScript 2.3.7's ChannelReplacement and seems to fix this issue
0002-2nd-take-to-NoScript-2.3.7-merge-use-CC-CI-oops.patch (7.2 KB) - added by ma1 7 years ago.
Patch: Merges NoScript 2.3.7's ChannelReplacement and seems to fix this issue, 2nd take with legacy CC and CI constants convention
noscript-merge.sh (1008 bytes) - added by ma1 7 years ago.
Merging bash/perl script to sync with current stable NoScript
download.php?i=t8KvVQJr (613 bytes) - added by rransom 7 years ago.
AsyncRedirect.patch (5.4 KB) - added by mikeperry 7 years ago.
Redirect API patch (against Firefox 13).
UseRedirectAPI.patch (742 bytes) - added by mikeperry 7 years ago.
Patch for HTTPS-Everywhere to use the new redirect API (if it exists).
UseRedirectApi.patch (730 bytes) - added by mikeperry 7 years ago.
Updated version of the quick hack to use the new API

Download all attachments as: .zip

Change History (58)

comment:1 Changed 7 years ago by pde

Cc: mikeperry schoen added

Hi Drugoy, thanks for reporting this!

Mike + Seth, I'm going to be offline for the next 5+ hours unfortunately, do you think you can take a start at pushing a fix?

comment:2 Changed 7 years ago by tchevalier

Hi guys,
just to be sure, is anyone currently working on this?

comment:3 Changed 7 years ago by tchevalier

Cc: theo.chevalier11@… added

comment:4 Changed 7 years ago by Drugoy

tchevalier,

Obviously no one. The extension remains critically vulnerable and many users don't even know about it.
3+ weeks have passed and it's still not fixed.
I think I will delete this extension at all, since while it is disabled it has no use, and if it's enabled - then you can't be sure anymore that the page you are viewing is the one you think it is.

comment:5 in reply to:  4 Changed 7 years ago by tchevalier

Drugoy, thanks for the reply.

3+ weeks without any activity, is a long time. At Mozilla they decided not to blocklist the extension since we were thinking someone was working to fix it. I guess we will rethink about that, and blocklist HTTPS-Everywhere while this is not fixed. I will ask for that.

comment:6 Changed 7 years ago by pde

tchevalier, a couple of our developers looked at it, and concluded that the exploitability of the bug was low. But if you think this is not the case, I'm happy to take a closer look.

comment:7 Changed 7 years ago by pde

So at this URL is a modified version of Drugoy's page:

http://ww2.cs.mu.oz.au/~pde/bugs/5477-tst.html

It does the same thing when you click the button, with the addition of an alert that says "frogs". Visited without HTTPS Everywhere, the alert goes off. With HTTPS Everywhere, the iframe appears to replace the whole window, despite what one sees after "view source". In particular, there is no frogs alert.

comment:8 Changed 7 years ago by pde

Priority: criticalmajor
Summary: Critical security vulnerability is caused by HTTPS-Everywhere enabledHTTPS Everywhere sometimes causes iframes to behave strangely (take over their window?)

So I am tentatively downgrading and retitling this bug.  However I want to stress that if Drugoy or others can show a clearer proof-of-concept, or suggest patches for the underlying bug, we remain very interested in those.

comment:9 Changed 7 years ago by mikeperry

From reading the source of the exploit, my conclusion is that this is a race condition brought about by the HTTPS-E synthetic redirect that somehow allows the document.write to bypass the same origin policy (http frame is able to write to an "https" origin).

My opinion that the reference to the window should become invalid after our redirect (or the rendered window should be cleared). For some reason neither happens...

Can we send Giorgio another wizard robe? Or do we owe him several already?

comment:10 Changed 7 years ago by pde

Re: exploitability, on one hand, the fact that I can briefly see Drugoy's page with the Apple HTTPS  decoration, does make me quite nervous.  On the other hand, I still can't get an alert with the apple.com cookies to fire.  So it's possible that none of the code is actually executing in the real https://apple.com origin.

comment:11 Changed 7 years ago by pde

Summary: HTTPS Everywhere sometimes causes iframes to behave strangely (take over their window?)Surprising DOM origins before HTTPS-E/NoScript redirects have completed

comment:12 Changed 7 years ago by mikeperry

pde: One reason you might not be able to snag apple.com cookies is that the cookie origin checks are independent from the document.write origin checks. However, the ability to spoof a login page from a false https origin is bad enough to warrant investigation, I think. The default user behavior for a convincing login is to enter their password, after all.

As far as how to solve this: to avoid wading through both NoScript and the corresponding XPCOM objects in C++, I motion that we first ask Giorgio if he has any clues as to what is going on. if Giorgio times out, I think we should ask Mozilla why normal redirects can't do this attack. Or perhaps in the reverse order..

In either case, it seems a suspicious enough violation of same-origin policy to make me feel like we need not be first in line to spend deep IQ on this problem.

comment:13 Changed 7 years ago by pde

mikeperry: the way I'm trying to snag a cookie is by document.writing an alert(document.cookie) script into the apple page.  It doesn't seem to work: I think that write only goes to Apple's window, not Apple's DOM, and only until the redirect has completed.  So some way of stopping the redirect halfway would be necessary to make the fake login page work. 

(In my testing, if it gets any cookies, they're from the attack page, not the victim page:

http://ww2.cs.mu.oz.au/~pde/bugs/5477-tst-cookies.html

screenshot of (transient) cookie alert:

http://ww2.cs.mu.oz.au/~pde/bugs/5477-screenshot.png

)

comment:14 Changed 7 years ago by pde

Cc: g.maone@… added

Adding a CC to Giorgio, since at minimum he should know about its existence.

comment:15 Changed 7 years ago by ma1

From a first cursory look, latest stable NoScript (2.3.7) on latest Nightly does not seem affected (while I could reproduce with stable HTTPS Everywhere).

Tested with default configuration + NoScript Options|General|Scripts globally allowed + NoScript Options|Advanced|HTTPS with apple.com forced to HTTPS (i.e. apple.com and subdomains).

I apparently get the same behavior as a clean profile with no extensions (i.e. the load gets early aborted by document.write(), which therefore outputs in a window which still displays the origin of its opener.

Hence, if these observations are confirmed, the question is: where does HTTPS-Everywhere's HTTPS enforcement implementation diverge from NoScript's?

comment:16 Changed 7 years ago by ma1

Owner: changed from pde to ma1
Status: newaccepted

OK, I thing I've found it.

HTTPS Everywhere uses a very old version of NoScript's ChannelReplacement machinery, which has several known issues with Gecko >= 2.

Patch coming in minutes...

Changed 7 years ago by ma1

Patch: Merges NoScript 2.3.7's ChannelReplacement and seems to fix this issue

Changed 7 years ago by ma1

Patch: Merges NoScript 2.3.7's ChannelReplacement and seems to fix this issue, 2nd take with legacy CC and CI constants convention

comment:17 Changed 7 years ago by ma1

Status: acceptedneeds_review

Mike, could you please apply *both* those patches in sequence (or just merge the ChannelReplacement.js file from a NoScript 2.3.7 inside IOUtil.js turning Cc/Ci into CC/CI) and check if everything works as expected?

What I could check is that the bug doesn't happen and the frame's URL gets swapped from http to https as expected, but I can't tell for sure the merge doesn't break other stuff of yours.

Hope it helps.

comment:18 Changed 7 years ago by mikeperry

Status: needs_reviewneeds_revision

ma1: We're running into some issues getting those patches to apply with 'git am' for some reason.. I first suspected dos line breaks as the issue, but removing them didn't seem to help. Applying the patch manually with 'patch' causes an exception to be generated: http://pastebin.com/t8KvVQJr

Also, I think I'd prefer it if we can keep our codebases closer in sync. I am assuming that if I just rename IOUtil.js to ChannelReplacement.js, I should be able to re-produce that patch, right? (Note the last time we synced with NoScript was noscript-2.0.9.8rc1).

I'm going to give that a shot right now.

comment:19 Changed 7 years ago by mikeperry

Ok, I'm almost done with this merge. I've noticed that IOUtil.js and Thread.js got folded into the main NoScript service component file. I've broken them back out again and updated our copies.

I've also updated us to use the ChannelReplacement.js directly from NS 2.3.7.

I've pushed this to mikeperry/ns-2.3.7-merge. I still need to test it and describe the merging process in my diff script, and then push that.

Changed 7 years ago by ma1

Attachment: noscript-merge.sh added

Merging bash/perl script to sync with current stable NoScript

comment:20 Changed 7 years ago by ma1

I've just attached a script (depending on bash and perl) to be put at the root of the tree (like noscript-diff.sh), which downloads and unpacks latest NoScript stable (optionally, pass "-n" to skip) then extracts the relevant bits from noscriptService.js and ChannelReplacement.js, merges them into IOUtil.js and makes the relevant changes to ensure compatibility with HTTPSEverywhere (e.g. removes dependencies on ABE/NS and switches back to CC/CI convention).

Hope it helps.

comment:21 Changed 7 years ago by mikeperry

I fixed the ABE issue by making a gimp ABE object in our main service with a log function. After some more cleanup, I'm still having issues that probably will require me to mess around in the JSD for a while... There are a couple exceptions being thrown due to missing notificationCallbacks and a possibly a couple other issues too.. :/. I think I need to put this down for today.

On an unrelated note, I've been meaning to ask you if this is an OK hack for a while now:
https://gitweb.torproject.org/mikeperry/https-everywhere.git/commitdiff/7b29ac0b95a466bb180012d11c2db28d9c5c09ad

comment:22 Changed 7 years ago by pde

Giorgio, we haven't yet succeeded in getting a clean, updated IOUtils.js, either by patching or by merging from the NoScript tree.  If you have a working (ie, rewrites HTTP->HTTPS) .xpi, do you want to paste that here so we can prod at it?

comment:23 Changed 7 years ago by pde

Hmmm, actually perhaps we have a branch that's working for me but not mikeperry.  Further investigation underway.

comment:24 Changed 7 years ago by pde

Solved.  Thanks for your help Giorgio!  But we do have a suggested patch for you:

https://gitweb.torproject.org/https-everywhere.git/commitdiff/eb911ff0517cb497d9bca3ef72e2d66ac8be6738

(but actually mike tells me that that component might not exist at all...)

comment:25 Changed 7 years ago by mikeperry

Also Giorgio, let me know what you think about that commit url I pasted up there removing the ABE channel check.

Changed 7 years ago by rransom

Attachment: download.php?i=t8KvVQJr added

comment:26 Changed 7 years ago by ma1

@mikeperry:
The channel/window matching hack is there because some extensions which use hidden browsers (usually for prerendering) caused new windows to be spawned instead of frame navigations and similar unpleasant issues. As you can see, my noscript-merge.sh bash script attached above takes care of replacing the ABE dependency with equivalent self-contained code.

@pde:
Sorry, I have to rejected your patch ;)
The ClassID in my code is correct (without the trailing ";1"), but it's there for Fx 3.x legacy compatibility: Safe Browsing needed to be invoked explicitly that way in Gecko 1.9.1, now (Gecko >= 2) it's hardcoded and the component is gone.

comment:27 Changed 7 years ago by mikeperry

Actual Points: 7
Keywords: MikePerry201204 added; address spoofing critical vulnerability removed
Points: 7
Resolution: fixed
Status: needs_revisionclosed

Thanks Giorgio! I merged both fixes for your comment. They should go out in HTTPS-Everywhere 2.0.3, I assume.

comment:28 Changed 7 years ago by tchevalier

Thank you guys for your work! I'm happy to see this fixed :)

comment:29 Changed 7 years ago by pde

@ma1 : okay, in that case could it be wrapped in a try/catch so that it doesn't look like a bug in the JavaScript debugger?

comment:30 Changed 7 years ago by ma1

@pde: I'd be very surprised if that code actually threw any exception you can catch.
Anyway, if it's a lint warning you want to avoid, change it into

const k = "@mozilla.org/channelclassifier";
return this.proto._classifierClass = (k in Cc) && Cc[k];

comment:31 Changed 7 years ago by Drugoy

Priority: majorblocker
Resolution: fixed
Status: closedreopened

This is a critical security vulnerability. 1.5 months have passed since I reported it. It is still not fixed (neither in 2.0.3 nor in 3.0.2.), and there are thousands of users that may get hacked.
I think you don't give a fcare about users of your products and such critical vulnerabilities if you don't even test whether the bug is fixed or not.
Well, you obviously are not that good as you pretend to be.

comment:32 Changed 7 years ago by pde

You seem to be right Drugoy.  I needed to clear my cache to make the intermediate display state actually render on my screen, but it seems to still be there :(.

I wonder what our options here are.  Is there some way we can forcibly snip the old DOM out of the window once we know we're starting the HTTPS redirect?  If we make window.document point to a new blank DOM, will we break the redirect that's in progress?

comment:33 Changed 7 years ago by mikeperry

FTR: The behavior is quite different if you run http://ww2.cs.mu.oz.au/~pde/bugs/5477-tst.html in Tor Browser. For me, the url bar in the popup goes through three states:

  1. I click Demo, and the popup has a url of http://ww2.cs.mu.oz.au/~pde/bugs/5477-tst.html and the frogs popup appears immediately.
  1. A second goes by, and the url bar turns to https://www.apple.com, with the content of the popup still in place (yes this is bad, but keep reading)
  1. Another second or two goes by, and the redirect completes, and as far as I can tell, I'm now on the real https://www.apple.com url with valid content.

It's possible that when we tested this, step 2 happened very quickly for us (perhaps because both Peter and I were testing the fix on vanilla Firefox without Tor), and we didn't notice the interim state.

Am I seeing the same thing everyone else is seeing? Is the blocker that is causing so many users to get hacked really this brief interim state in 2? Because if so, I'm very surprised that so many users are getting hacked so quickly.

Not that the brief interim state isn't something that should be prevented if possible.. I'm just surprised at all the screaming. Seems a bit unnecessary.

comment:34 Changed 7 years ago by pde

Mike, what I observe is the same as what you observe, and I believe the vulnerability, if there is one, is state number 2.

On its face that does not look easily exploitable, but I think there are at least theoretical grounds for concern.  For example, an active network attacker could induce state 2 and then drop a whole lot of packets or slow them down to an absolute trickle.  That might cause state 2 to persist for 30 seconds or longer, perhaps enough to trick a user into completing a password dialog.

comment:35 Changed 7 years ago by mikeperry

We've discovered that for some reason both codepaths of URL rewriting (HTTPS.forceURI and HTTPS.replaceChannel) are applying to this PoC exploit. That could explain the intermediate step 2, due to a race condition between them. Next step is disabling each one and seeing what the behavior is.

comment:36 Changed 7 years ago by mikeperry

Actual Points: 7
Keywords: MikePerry201204 removed
Points: 7

comment:37 Changed 7 years ago by pde

In parallel, I've been discussing this with the Mozilla security team.  One horrible workaround option (assuming we don't learn anything actionable from the approach mikeperry mentions) would be to redirect to about:blank straightaway, and then on to the real destination.  It would probably have to be about:blank#token in order to keep track of what we're doing.

I also tested to see if this problem exists with Mozilla's native HSTS implementation.  It doesn't.  Unfortunately, all of that machinery is asynchronous native code that's not available to scripts.

comment:38 Changed 7 years ago by pde

I performed the experiment that mikeperry suggested, testing each of our rewriting paths separately.

Disabling the shouldLoad / forceURI path did not stop the bug.

Disabling the http-on-modify-request / replaceChannel path did stop the bug.  So unsuprisingly, we're looking at a bug in replaceChannel.

comment:39 Changed 7 years ago by pde

Aside from the horrible about:blank#id hack solution discussed above, another horrible approach would be to try to get a callback when the malicious code alters the content of the http://www.apple.com window.  If someone changes the content of a window that we're replaceChanneling, we could try to abort the channel replacement.  Although that does sound a little racy.

comment:40 Changed 7 years ago by pde

A summary of possible solution strategies:

  1. Make every redirect via about:blank#rewrite-id. Advantages: quick. Disadvantages: extremely janky, will make our code much uglier; hard to know whether requests will ever mutate if we do this; structures for tracking the rewrite-ids will be a likely source of memory leaks.
  1. Try to deny the malicious code access to the window once we're rewriting inside it. Advantages: unknown. Disadvtanges: we don't know whether this is possible, or how to do it.
  1. Use the HSTS machinery. Advantages: will probably work. Disadvantages: will require a Firefox patch (!!!) to expose those mechanisms to JavaScript; the HSTS paths have probably never been tested with cross-domain rewrites.

Mike Perry is looking into the feasibility of 3.

comment:41 in reply to:  40 Changed 7 years ago by mikeperry

Replying to pde:

  1. Use the HSTS machinery. Advantages: will probably work. Disadvantages: will require a Firefox patch (!!!) to expose those mechanisms to JavaScript; the HSTS paths have probably never been tested with cross-domain rewrites.

Turns out it was actually pretty straightforward to adapt the HSTS machinery into a general URL rewriting XPCOM API. I created this API in my Tor Browser-patched Firefox 13 source and tested using it with a patched HTTPS-Everywhere to use the API, and it worked. It also blocks the url spoofing exploit by way of the url bar always remaining as http://ww2.cs.mu.oz.au/~pde/bugs/5477-tst.html (ie steps 2 and 3 of my description above now never happen).

Will attach both patches. The Firefox patch will need review from some Mozilla people as well as some more extensive testing, as there are a few questions I have:

  1. Should this API be its own interface instead of getting added to nsIHTTPChannel?
  2. Do I need to do anything special wrt refcounting the nsIURI?
  3. Are there any weird edgecases like webfonts, favicons, spdy, post requests, etc etc?
  4. What about other channel types? Should we keep the NoScript machinery for them?

Changed 7 years ago by mikeperry

Attachment: AsyncRedirect.patch added

Redirect API patch (against Firefox 13).

Changed 7 years ago by mikeperry

Attachment: UseRedirectAPI.patch added

Patch for HTTPS-Everywhere to use the new redirect API (if it exists).

comment:42 Changed 7 years ago by mikeperry

I created a Mozilla bug for the Firefox patch here: https://bugzilla.mozilla.org/show_bug.cgi?id=765934.

Once it gets a bit of review, we can deploy a prototype in TBB-alpha.

comment:43 Changed 7 years ago by mikeperry

Keywords: MikePerry201207 added

I need to keep an eye on this one. I have about a half dozen questions about the patch that need to be answered still. I'll see about (re)-posting them to the Mozilla bug.

Changed 7 years ago by mikeperry

Attachment: UseRedirectApi.patch added

Updated version of the quick hack to use the new API

comment:44 Changed 7 years ago by mikeperry

I updated the patch on https://bugzilla.mozilla.org/show_bug.cgi?id=765934. I'm feeling a bit better about the new version.

comment:45 Changed 7 years ago by mikeperry

Actual Points: 20 (pde: 6? mikeperry: 14)
Points: 20
Resolution: fixed
Status: reopenedclosed

The Firefox patch will appear in the next Tor Browser 2.3.x-alpha series and will be active when used in combination with HTTPS-Everywhere 3.0dev6 and above. If that works out, we may backport to Tor Browser 2.2.x.

As for when the Firefox patch gets merged for normal HTTPS-Everywhere users, please keep an eye on the bugzilla bug.

I'm guessing pde spent about a day and a half going back and forth with Mozilla on the unit tests and making them work right.

comment:46 Changed 6 years ago by pde

Resolution: fixed
Status: closedreopened

Reopening until we actually have a fix for this landed in mainline Firefox.

comment:47 Changed 6 years ago by pde

The fix just landed in Mozilla Central (Firefox v. 21 nightly)

comment:48 Changed 6 years ago by pde

The fix has landed in Firefox 20 but seems to be stopping there :/

comment:49 Changed 6 years ago by Drugoy

Fx 20 released, but demo page is gone, could someone please somehow test the presence of this bug on Fx 20?

comment:50 Changed 6 years ago by pde

Resolution: fixed
Status: reopenedclosed

Now fixed in the wild.

(Drugoy, if you'd like to confirm that the demo page is back online at http://reworld.org/old/bugs/5477-tst.html )

comment:51 Changed 18 months ago by teor

Severity: Normal

Set all tickets without a severity to "Normal"

Note: See TracTickets for help on using tickets.