Closed Bug 799952 (CVE-2012-4192) Opened 12 years ago Closed 12 years ago

Cross domain access to the location object

Categories

(Core :: DOM: Core & HTML, defect)

16 Branch
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla18
Tracking Status
firefox15 --- unaffected
firefox16 + verified
firefox17 + verified
firefox18 --- unaffected
firefox19 --- unaffected
firefox-esr10 --- unaffected

People

(Reporter: ehsan.akhgari, Assigned: ejpbruel)

References

()

Details

(5 keywords, Whiteboard: fixed by bug 720619)

Attachments

(5 files, 2 obsolete files)

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.
This also affects 17 (at least 10/8's Aurora) but not trunk.
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.
Note that the comments on the linked blog also say that both
  alert(document.getElementById(‘x’).contentWindow.location)
and
  alert(win.location)
work.
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.
Blocks: 754202
We should probably pull 16 off the wire.
This actually hits a different exception in 15 than the "stupid testcase" does, for what it's worth.
One thing I can't understand is how we could possibly not have had a test for this.
OS: Mac OS X → All
Hardware: x86 → All
Version: Trunk → 16 Branch
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...
I'm landing this over in bug 720619. Marking the dep.
Depends on: CVE-2012-4193
Whiteboard: fixed by bug 720619
We're checking in testcases this time, right?
Flags: in-testsuite?
I've tested to make sure that ESR10 is unaffected here.
I'm working with Boris and will be able to check in test cases very soon.
Keywords: verifyme
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.
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.
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'"
I've verified using the test cases here and in bug 720619 on Windows XP and Fx16.0.1 candidates.
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'.
(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)
(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).
* 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
Alias: CVE-2012-4192
Since this is public, are we intending to do a chemspill to fix this one? its sg-critical from what i see.
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?
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
(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.
Status: RESOLVED → VERIFIED
Thanks Kyle and Jesse. Indeed, my test had the false positive for the case where [Object Location] was returned. Thanks for tweaking it.
These testcases all report PASS in the 10.0.9esr candidate builds.
Can this bug report be made public?
Group: core-security
Can we clear the in-testsuite flag given that we got a testcase with the fix on bug 720619?
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 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!
Attachment #673487 - Flags: review?(bzbarsky) → review+
Incorporated Boris' changes.
Attachment #673487 - Attachment is obsolete: true
Attachment #674414 - Flags: review?(bzbarsky)
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.
Attachment #674414 - Flags: review?(bzbarsky) → review+
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.
> 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?
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.
Attachment #674414 - Attachment is obsolete: true
Attachment #674785 - Flags: review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8cf15921d03
Assignee: nobody → ejpbruel
Flags: in-testsuite? → in-testsuite+
Keywords: checkin-needed
Target Milestone: --- → mozilla18
(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.
Flags: in-testsuite+ → in-testsuite?
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?
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.
Flags: sec-bounty+
Component: DOM → DOM: Core & HTML
Flags: sec-bounty-hof-
You need to log in before you can comment on or make changes to this bug.