Last Comment Bug 799952 - (CVE-2012-4192) Cross domain access to the location object
(CVE-2012-4192)
: Cross domain access to the location object
Status: VERIFIED FIXED
fixed by bug 720619
: csectype-disclosure, csectype-sop, regression, sec-high, testcase
Product: Core
Classification: Components
Component: DOM (show other bugs)
: 16 Branch
: All All
: -- normal (vote)
: mozilla18
Assigned To: Eddy Bruel [:ejpbruel]
:
Mentors:
https://blog.mozilla.org/security/201...
: 800029 (view as bug list)
Depends on: CVE-2012-4193
Blocks: 754202
  Show dependency treegraph
 
Reported: 2012-10-10 07:42 PDT by :Ehsan Akhgari
Modified: 2013-10-14 10:09 PDT (History)
46 users (show)
rforbes: sec‑bounty+
gary: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
+
verified
+
verified
unaffected
unaffected
unaffected


Attachments
Stupid testcase showing complete lack of even rudimentary security checks here (196 bytes, text/html)
2012-10-10 08:03 PDT, Boris Zbarsky [:bz]
no flags Details
Testcase that shows that for the exec() case we tried to call toString() in 15 and failed (212 bytes, text/html)
2012-10-10 08:10 PDT, Boris Zbarsky [:bz]
no flags Details
Positive/negative test cases for accessing cross- and same-domain location objects (2.63 KB, text/html)
2012-10-10 18:43 PDT, Matt Wobensmith [:mwobensmith][:matt:]
no flags Details
tweaked version of Matt's testcase (3.44 KB, text/html)
2012-10-10 19:30 PDT, Jesse Ruderman
no flags Details
testcase to bug 799952 - x-domain access to location objects (5.15 KB, patch)
2012-10-19 17:41 PDT, Matt Wobensmith [:mwobensmith][:matt:]
bzbarsky: review+
Details | Diff | Splinter Review
Updated test case for bug #799952 (5.24 KB, patch)
2012-10-23 15:50 PDT, Matt Wobensmith [:mwobensmith][:matt:]
bzbarsky: review+
Details | Diff | Splinter Review
(Again) updated testcase for #799952 (5.24 KB, patch)
2012-10-24 12:17 PDT, Matt Wobensmith [:mwobensmith][:matt:]
mwobensmith: review+
Details | Diff | Splinter Review

Description :Ehsan Akhgari 2012-10-10 07:42:47 PDT
http://www.thespanner.co.uk/2012/10/10/firefox-knows-what-your-friends-did-last-summer/

function poc() {
var win = window.open('https://twitter.com/lists/', 'newWin', 'width=200,height=200');
setTimeout(function(){
alert('Hello '+/^https:\/\/twitter.com\/([^/]+)/.exec(win.location)[1])
}, 5000);
}

I can't reproduce in 15 or trunk, but 16 is affected.
Comment 1 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-10-10 07:50:26 PDT
This also affects 17 (at least 10/8's Aurora) but not trunk.
Comment 2 Bobby Holley (:bholley) (busy with Stylo) 2012-10-10 07:56:05 PDT
See also bug 730431 and bug 720619.
Comment 3 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-10-10 07:59:14 PDT
This doesn't involve the web console.  Turn off popups and visit http://people.mozilla.org/~khuey/test_location.html

It's also not the same as Bug 720619 because it doesn't happen in the copy of ESR that I have, but the fix for that has not been backported to ESR yet.
Comment 4 Mike Hommey [:glandium] 2012-10-10 08:00:20 PDT
Note that the comments on the linked blog also say that both
  alert(document.getElementById(‘x’).contentWindow.location)
and
  alert(win.location)
work.
Comment 5 Boris Zbarsky [:bz] 2012-10-10 08:01:11 PDT
I'll attach a minimal testcase in a sec that does not involve popups.

This regressed on trunk in the range http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=bf8f2961d0cc&tochange=4a8e0d5fc954 so presumably from bug 754202.  It then got fixed in bug 720619, I assume.
Comment 6 Boris Zbarsky [:bz] 2012-10-10 08:03:38 PDT
Created attachment 669975 [details]
Stupid testcase showing complete lack of even rudimentary security checks here
Comment 7 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-10-10 08:05:26 PDT
We should probably pull 16 off the wire.
Comment 8 Boris Zbarsky [:bz] 2012-10-10 08:10:48 PDT
Created attachment 669980 [details]
Testcase that shows that for the exec() case we tried to call toString() in 15 and failed

This actually hits a different exception in 15 than the "stupid testcase" does, for what it's worth.
Comment 9 Boris Zbarsky [:bz] 2012-10-10 08:23:40 PDT
One thing I can't understand is how we could possibly not have had a test for this.
Comment 10 Boris Zbarsky [:bz] 2012-10-10 09:05:42 PDT
And specifically, this regressed with http://hg.mozilla.org/mozilla-central/rev/6f70fe755b10 which is what actually switched from pulling principals off the stackframe to pulling them off the compartment...
Comment 11 Bobby Holley (:bholley) (busy with Stylo) 2012-10-10 09:47:38 PDT
I'm landing this over in bug 720619. Marking the dep.
Comment 12 Daniel Veditz [:dveditz] 2012-10-10 11:20:37 PDT
We're checking in testcases this time, right?
Comment 13 Alex Keybl [:akeybl] 2012-10-10 13:22:49 PDT
I've tested to make sure that ESR10 is unaffected here.
Comment 14 Matt Wobensmith [:mwobensmith][:matt:] 2012-10-10 15:32:54 PDT
I'm working with Boris and will be able to check in test cases very soon.
Comment 15 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-10-10 17:03:15 PDT
Matt Wobensmith and I have confirmed the testcases in this bug and bug 720619 reproduce in 17b1#1 and do not reproduce in 17b1#2. We only have candidates for Linux and Mac so far so we can't call this verified yet, but at least Linux & Mac are taken care of.
Comment 16 juan becerra [:juanb] 2012-10-10 17:44:05 PDT
I've tested this on Linux and Mac on builds prior to the fix (where I can see the problem), and with the Fx16.0.1 candidates where I do not see the problem. I'll check Windows when the builds are out.
Comment 17 Aaron Train [:aaronmt] 2012-10-10 17:54:52 PDT
Verified Fixed against Firefox for Android (mozilla-release, 16.0.1)

Test-case: public PoC
Result: No alert; console error — "Permission denied to access property 'toString'"

Test-case: first attached test-case
Result: Alert — NS_ERROR_XPC_BAD_CONVERT on argument 0

Test-case: second attached test-case
Result:  Alert — Error: "Permission denied to access property 'toString'"
Comment 18 juan becerra [:juanb] 2012-10-10 18:38:07 PDT
I've verified using the test cases here and in bug 720619 on Windows XP and Fx16.0.1 candidates.
Comment 19 Matt Wobensmith [:mwobensmith][:matt:] 2012-10-10 18:43:01 PDT
Created attachment 670212 [details]
Positive/negative test cases for accessing cross- and same-domain location objects

As per discussion with Boris, this test fleshes out the issue a bit more and adds a few more cases not covered by simple access to the location object. Also includes some positive test cases in case we broke same-domain location object access.

All tests pass in FF15. Some break in 17.0b1 build 1 but then are all fixed in 17.0b1 build 2.

Also worth noting: try running this in Safari and Chrome. Just sayin'.
Comment 20 Aaron Train [:aaronmt] 2012-10-10 18:57:08 PDT
(In reply to Matt Wobensmith from comment #19)
> Created attachment 670212 [details]
> Positive/negative test cases for accessing cross- and same-domain location objects
> [...] 

'All tests pass' on Firefox for Android (mozilla-release, 16.0.1)
Comment 21 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-10-10 19:03:08 PDT
(In reply to Matt Wobensmith from comment #19)
> Created attachment 670212 [details]
> Positive/negative test cases for accessing cross- and same-domain location
> objects
> 
> As per discussion with Boris, this test fleshes out the issue a bit more and
> adds a few more cases not covered by simple access to the location object.
> Also includes some positive test cases in case we broke same-domain location
> object access.
> 
> All tests pass in FF15. Some break in 17.0b1 build 1 but then are all fixed
> in 17.0b1 build 2.
> 
> Also worth noting: try running this in Safari and Chrome. Just sayin'.

On Chrome and Opera I see "Fail: X-domain access to iframe contentWindow.location object via Object.prototype.toString.call should be disallowed"

Using Object.prototype.toString.call just returns "[Object Location]" though, so I don't think there's a security issue here in Chrome (or Opera).
Comment 22 Jesse Ruderman 2012-10-10 19:30:49 PDT
Created attachment 670221 [details]
tweaked version of Matt's testcase

* More verbose output (to show that Chrome's behavior is probably okay)
* Use https://blog.mozilla.com/ instead of http://web.mit.edu/ so we don't hit mixed content blocking when loading the testcase from Bugzilla
* setTimeout instead of setInterval
Comment 23 Huzaifa Sidhpurwala 2012-10-10 20:17:54 PDT
Since this is public, are we intending to do a chemspill to fix this one? its sg-critical from what i see.
Comment 24 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-10-10 20:55:30 PDT
Verified fixed with all attached testcases in Firefox 17.0b1#2 on Ubuntu 12.04, Mac OSX 10.7, and Windows 7. Any reason to keep this bug NEW?
Comment 25 Daniel Veditz [:dveditz] 2012-10-10 21:22:14 PDT
*** Bug 800029 has been marked as a duplicate of this bug. ***
Comment 26 Bobby Holley (:bholley) (busy with Stylo) 2012-10-11 03:09:19 PDT
(In reply to Huzaifa Sidhpurwala from comment #23)
> Since this is public, are we intending to do a chemspill to fix this one?
> its sg-critical from what i see.

It is sg-high (there's no remote code execution). We are indeed chemspilling.
Comment 27 Matt Wobensmith [:mwobensmith][:matt:] 2012-10-11 12:07:09 PDT
Thanks Kyle and Jesse. Indeed, my test had the false positive for the case where [Object Location] was returned. Thanks for tweaking it.
Comment 28 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-10-11 19:02:59 PDT
These testcases all report PASS in the 10.0.9esr candidate builds.
Comment 29 Jesse Ruderman 2012-10-12 18:12:34 PDT
Can this bug report be made public?
Comment 30 Henrik Skupin (:whimboo) 2012-10-15 05:13:40 PDT
Can we clear the in-testsuite flag given that we got a testcase with the fix on bug 720619?
Comment 31 Boris Zbarsky [:bz] 2012-10-15 05:53:50 PDT
The testcase in bug 720619 does not test the web-visible codepaths.  The testcase in this bug does.  What needs to happen is for the testcase in this bug to be added to our automated tests.
Comment 33 Matt Wobensmith [:mwobensmith][:matt:] 2012-10-19 17:41:36 PDT
Created attachment 673487 [details] [diff] [review]
testcase to bug 799952 - x-domain access to location objects
Comment 34 Boris Zbarsky [:bz] 2012-10-19 18:12:02 PDT
Comment on attachment 673487 [details] [diff] [review]
testcase to bug 799952 - x-domain access to location objects

>+++ b/dom/tests/mochitest/bugs/Makefile.in
>+		test_bug799952.html \
		child_bug799952.html \

This is broken.  You have a CR in there after the backslash, not a newline.  Please fix that.

>+++ b/dom/tests/mochitest/bugs/child_bug799952.html
>@@ -0,0 +1,1 @@
>+#799952

How about:

<!DOCTYPE html>

at least?  Or just name the file child_bug799952.txt, perhaps?

>+++ b/dom/tests/mochitest/bugs/test_bug799952.html
>+setTimeout (start, 2000);

So as a general rule, use of setTimeout in a test indicates a bug.

In this case, nothing actually ensures that after two seconds the frame has even been parsed, much less finished loading.  It would be better to do:

  addLoadEvent(start);

which will make sure start() runs after onload.

>+// the following are error cases. If they do not throw, they are failures.

Probably good to indent everything inside the function by two spaces.

Also, the try/catch indentation is slightly odd.  Normally I think it would be:

  try {
    something;
  } catch (e) {
    whatever;
  }

>+	foo = String (/(.*)/.exec(window[0].location)[1]);

Perhaps replace window[0] with $("win").contentWindow like you have for the other tests here?


>+is ( result, "Error: Permission denied to access property \'toString\'", "Access to xdomain location via regexp must throw exception" )

Please nix the spaces around the '(' and before the ')'.

>+<iframe id="win" name="win" src="https://example.com:443/tests/dom/tests/mochitest/bugs/child_bug799952.html"></iframe> 

You don't need a "name" attribute there.

>+<iframe id="sameDomainContent" name="sameDomainContent" src="../child_bug799952.html"></iframe>

Likewise.  And also, why the "../" bit?

r=me with the above addressed.  Thank you for writing these up!
Comment 35 Matt Wobensmith [:mwobensmith][:matt:] 2012-10-23 15:50:08 PDT
Created attachment 674414 [details] [diff] [review]
Updated test case for bug #799952

Incorporated Boris' changes.
Comment 36 Boris Zbarsky [:bz] 2012-10-23 22:09:49 PDT
Comment on attachment 674414 [details] [diff] [review]
Updated test case for bug #799952

> +<iframe id="sameDomainContent" src="/tests/dom/tests/mochitest/bugs/child_bug799952.html"></iframe>

Can't this just be:

  <iframe d="sameDomainContent" src="child_bug799952.html"></iframe>

?

All the is() calls should have the third argument indented even with the first argument, not with the is() itself.

r=me with those nits picked.
Comment 37 Matt Wobensmith [:mwobensmith][:matt:] 2012-10-24 09:55:42 PDT
Problem with referencing simply "child_bug799952.html" is that the Mochitest test suite - for whatever reason - tries to resolve that as "test_bug799952.html/child_bug799952.html" which returns a 404.

Previously I used a workaround of "../" but you mentioned that wasn't kosher.

Any clues about why relative URLs are resolved like this? I'm pretty baffled. The above solution works and assures us that we have a same-domain URL, but it does feel like I'm missing something.
Comment 38 Boris Zbarsky [:bz] 2012-10-24 11:49:06 PDT
> is that the Mochitest test suite - for whatever reason - tries to resolve that as
> "test_bug799952.html/child_bug799952.html"

That's.... quite unexpected.  How exactly are you running the test?
Comment 39 Matt Wobensmith [:mwobensmith][:matt:] 2012-10-24 12:17:54 PDT
Created attachment 674785 [details] [diff] [review]
(Again) updated testcase for #799952

Fixed URL issue. Looks like I was adding an extra slash to the file name when I ran the test locally, causing the 404 badness. Mea culpa and thanks for having patience.
Comment 40 Ryan VanderMeulen [:RyanVM] 2012-10-24 13:55:52 PDT
Pushed to Try:
https://tbpl.mozilla.org/?tree=Try&rev=aa7a833ce345
Comment 41 Ryan VanderMeulen [:RyanVM] 2012-10-24 19:11:04 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8cf15921d03
Comment 43 Gary Kwong [:gkw] [:nth10sd] 2012-10-24 21:11:01 PDT
(In reply to Phil Ringnalda (:philor) from comment #42)
> Sorry, backed out in
> http://hg.mozilla.org/integration/mozilla-inbound/rev/305471f75b59 - it's
> only run a few times, and it's already failed in
> https://tbpl.mozilla.org/php/getParsedLog.php?id=16441596&tree=Mozilla-
> Inbound and
> https://tbpl.mozilla.org/php/getParsedLog.php?id=16441901&tree=Mozilla-
> Inbound and
> https://tbpl.mozilla.org/php/getParsedLog.php?id=16442191&tree=Mozilla-
> Inbound

Resetting in-testsuite flag.
Comment 44 Boris Zbarsky [:bz] 2012-10-24 21:33:15 PDT
That's .... really odd.  Those failures look like what I would expect if the test were running before the cross-origin iframe has loaded, but we're explicitly running it off the load event!  What the heck?
Comment 45 Boris Zbarsky [:bz] 2012-10-24 21:45:20 PDT
Maybe add a test that makes sure we can't read the DOM of the cross-origin iframe and push to try with that?  See whether that test is also failing when the location part fails?

Alternately, perhaps log the actual location string when we get it, so we can see what page we _think_ we have loaded in there.

Note You need to log in before you can comment on or make changes to this bug.