1245264 - (CVE-2016-1965) address bar spoofing using location.protocol and history.back

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect)

Description

[Tsubasa Iinuma]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_3) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/48.0.2564.97 Safari/537.36

Steps to reproduce:

1.
Please access the following page.
http://poc.vuln.jp/firefox/trunk/spoofing/

2.
Click the 'CLICK ME' button.
Wait for about two seconds.

3.
Click the 'back' button.


Actual results:

test.vuln.jp is shown to an address bar.
However, document.domain is poc.vuln.jp.
Address Bar Spoofing was successful.


Expected results:

Must be displayed same domain in both.

Comment 1

[Daniel Veditz [:dveditz]]

I cannot reproduce this. tested using a Mac (as you appear to use based on your Chrome user agent) on Nightly version 47 (as in the bug version field), Release 44, and ESR 38.6. In all 3 cases the final URL was data://poc.vuln.jp -- which is an invalid URL and WRONG -- but not the spoofing http://test.vuln.jp as claimed.

Can you be more specific on what version you used, or confirm that you saw this behavior is Firefox 47 nightly running on a Mac?

I don't know why we even let location.protocol be writable.

Comment 2

[Tsubasa Iinuma]

(In reply to Daniel Veditz [:dveditz] from comment #1)
> I cannot reproduce this. tested using a Mac (as you appear to use based on
> your Chrome user agent) on Nightly version 47 (as in the bug version field),
> Release 44, and ESR 38.6. In all 3 cases the final URL was
> data://poc.vuln.jp -- which is an invalid URL and WRONG -- but not the
> spoofing http://test.vuln.jp as claimed.
> 
> Can you be more specific on what version you used, or confirm that you saw
> this behavior is Firefox 47 nightly running on a Mac?
> 
> I don't know why we even let location.protocol be writable.

Umm...

I tested using Mac and Windows on Nightly version 47.0a1 (2016-02-02), it was successful on both.
Therefore, I tried to take a video at that test.
http://1drv.ms/20okF2z

After that, I tested the following two plans, but Nightly returned the same result (Address Bar Spoofing was successful)...
1. Reinstalling Nightly.
2. Create a new profile for Nightly.

Comment 3

[Tsubasa Iinuma]

Today, I tried the following test.
1. Install Windows 8.1 on VMware fusion.
2. Install Firefox Nightly on Windows 8.1.
3. Execute my PoC on Nightly.

This test is successful at Address Bar Spoofing.

Comment 4

[Daniel Veditz [:dveditz]]

Not terribly concerning if this spoofs one of your own sites, and if it's not your own site then how do you get the user to hit the back button (or put the button on the site)? BUT -- maybe w.history.back() will work to accomplish the same thing. that would be more effective.

Comment 5

[Tsubasa Iinuma]

Yes, this bug must have been provided the back button to the site.
I think the following case exists.

1. 
A user access the following page.
http://poc.vuln.jp/firefox/trunk/spoofing/testcase/

The attacker will create any login page that was installed Facebook authentication button on this page. (At this time, a link to the Facebook error page has been fixed to the Facebook authentication button.)

2.
A user click the 'Sign in with Facebook' button.
Wait for about two seconds.

3.
Facebook error page is displayed.
A user click the 'Go back to the previous page' button.

4.
Address Bar Spoofing was successful.
The attacker will create a page that was disguised as a Facebook probably here.

------

I tried to test w.history.back().
The result, it did not work... (I think this is the correct behavior.)

Error Message:
>> w.history.back()
Error: Permission denied to access property "history"

Comment 6

[Tsubasa Iinuma]

I think the following movement should be prohibited as fundamental correction.

location.protocol = "data";

It's because this behavior has become the beginning of various bugs.

For example:
- Bug 1228950
- New cross-origin restriction bypass PoC (Incomplete, Unpublished)
http://poc.vuln.jp/firefox/trunk/bypass/
(It works with Firefox release version, it does not work with nightly.)
- This spoofing bug.

Comment 7

[:Gijs (he/him)]

(In reply to Tsubasa Iinuma from comment #6)
> I think the following movement should be prohibited as fundamental
> correction.
> 
> location.protocol = "data";

Note that this isn't prohibited on any other browser. We just don't seem to be doing the right thing.


(In reply to Daniel Veditz [:dveditz] from comment #1)
> I cannot reproduce this. tested using a Mac (as you appear to use based on
> your Chrome user agent) on Nightly version 47 (as in the bug version field),
> Release 44, and ESR 38.6. In all 3 cases the final URL was
> data://poc.vuln.jp -- which is an invalid URL and WRONG -- but not the
> spoofing http://test.vuln.jp as claimed.
> 
> Can you be more specific on what version you used, or confirm that you saw
> this behavior is Firefox 47 nightly running on a Mac?

I reproduced this as well, and I believe you get the results as in comment #0 in e10s, and the results you described in non-e10s.

Comment 8

[:Gijs (he/him)]

Attachment: Possible patch

I believe this addresses things.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8718778 [details] [diff] [review]
Possible patch

This is probably ok conceptually.  In an ideal world of immutable URIs, this is how SetScheme would work in general...

That said, calling SetHref is wrong, because it does all sorts of weirdness the other setters are not supposed to do (e.g. converting to replace loads automatically).  Please NS_NewURI the newSpec, with a comment explaining that this is needed because the new scheme may mean we want a different URI class, and then SetURI the result.

Comment 11

[:Gijs (he/him)]

Attachment: Patch v2

I'm aware that this is currently rated sec-moderate and as such doesn't need sec-approval, but it seems like the steps in comment 5 might potentially make this more serious.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
It's not entirely trivial, as you'd need to combine the location.protocol issue with something else to get reasonable spoofing, but I think comment 6 also has a point, so yeah. :-\

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
See previous point.

Which older supported branches are affected by this flaw?
Everything at least up to current 43, probably ESR as well, but I've not checked.

If not all supported branches, which bug introduced the flaw?
n/a

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Not in hand, but it doesn't look like this code changed a lot recently, so it shouldn't be hard to backport this even to 38, based on code inspection.

How likely is this patch to cause regressions; how much testing does it need?
I wouldn't expect this to cause regressions on non-malicious websites, but equally I'd be hesitant to dump a web-facing change into ESR or late beta without at least a bit of baking on other branches or spare time.

bz, I think this is what you meant, but I also realized that I wasn't checking results from these method calls, and that the scheme comes as a cstring and NS_NewURI takes a wide string, so can you just doublecheck that what I've done makes sense? Thanks!

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8718868 [details] [diff] [review]
Patch v2

>+  rv = NS_NewURI(getter_AddRefs(uri), NS_ConvertUTF8toUTF16(newSpec));

Just:

  rv = NS_NewURI(getter_AddRefs(uri), newSpec);

it has overloads that take UTF-8 (and in fact the overload that takes UTF-16 just converts right back to UTF-8 and calls the UTF-8 overload).

r=me with that

Comment 13

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8718868 [details] [diff] [review]
Patch v2

sec-approval+. We probably want this on Aurora and Beta as well. Can you see if ESR38 is vulnerable?

Dan, should we re-rate this higher?

Comment 14

[Daniel Veditz [:dveditz]]

The rating still seems appropriate but that doesn't mean we shouldn't take it on ESR. This would be good to get there.

Comment 15

[:Gijs (he/him)]

https://hg.mozilla.org/integration/fx-team/rev/29a35142a813

Comment 16

[:Gijs (he/him)]

Comment on attachment 8718868 [details] [diff] [review]
Patch v2

Approval Request Comment
[Feature/regressing bug #]: n/a
[User impact if declined]: security issue
[Describe test coverage new/current, TreeHerder]: pretty much none. :-\
[Risks and why]: Fairly low. It's a focused change to a single method. I wouldn't expect anything to break that wasn't trying to abuse the current implementation.
[String/UUID change made/needed]: no

Comment 17

[:Gijs (he/him)]

Attachment: Patch for ESR

I'll request approval for this once we're on beta and aurora and it's had at least some baking on other branches.

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/29a35142a813

Comment 19

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Gijs, if you were able to use the STR from comment 0 and verify that it's fixed, I would feel more comfortable taking it to Aurora. "no test coverage" sounds scary! :(

Comment 20

[:Gijs (he/him)]

(In reply to Ritu Kothari (:ritu) from comment #19)
> Gijs, if you were able to use the STR from comment 0 and verify that it's
> fixed, I would feel more comfortable taking it to Aurora. "no test coverage"
> sounds scary! :(

Normally assignees should not verify their own bugfixes, but, OK. I checked on Nightly from 2 days ago and could reproduce the issue there. I can't reproduce on yesterday's nightly (which included the fix) -- today's nightly hasn't been built yet. Tested on Windows. Marking verified for nightly -- we should let QA verify this on aurora and beta when it lands there.

Comment 21

[:Gijs (he/him)]

(note for QA: the text on the page will always say "address bar spoofing successful" - the issue is whether the location bar's URI shows the domain "poc.vuln.jp" (correct) or the domain "test.vuln.jp" (incorrect), and whether or not it has "data://" in the URL bar (also incorrect). Note that the end result differed before the fix between e10s and non-e10s, and it would be good to verify both for aurora, at least (I don't think it's currently easily possible to toggle e10s on beta.))

Comment 22

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8718868 [details] [diff] [review]
Patch v2

Gijs locally tested to make sure the fix works, approving for uplift to Aurora46 and Beta45.

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/55d25967b804
https://hg.mozilla.org/releases/mozilla-beta/rev/89743488eb2a

Comment 24

[Petruta Horea [:phorea], Desktop QA]

Reproduced e10s/non e10s behavior on Nightly 47.0a1 2016-02-02.

Url is now the same with document.domain (poc.vuln.jp) in both e10s and non e10s windows.
Tested under Win 7 64-bit, Ubuntu 14.04 64-bit and Mac OS X 10.9.5 using Firefox 45 beta 8 and latest Aurora 46.0a2 2016-02-23.

Comment 25

[:Gijs (he/him)]

Comment on attachment 8719470 [details] [diff] [review]
Patch for ESR

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: see comment 14. This (setting location.protocol to something) has been involved in several security issues now.
User impact if declined: security issues
Fix Landed on Version: 47, uplifted to 45 and 46 and verified there
Risk to taking this patch (and alternatives if risky): pretty low. The alternative would be waiting until 38 is EOL.
String or UUID changes made by this patch: no.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 26

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8719470 [details] [diff] [review]
Patch for ESR

sec-high fix that has been in beta45 and aurora46 for a week now. Let's uplift to esr38.7

Comment 27

[Carsten Book [:Tomcat]]

 https://hg.mozilla.org/releases/mozilla-esr38/rev/b4467681abd6

Comment 28

[Petruta Horea [:phorea], Desktop QA]

Verified as fixed using Firefox 38.7 esr under Win 7 64-bit and Ubuntu 14.04 64-bit.