Last Comment Bug 801576 - Align Gecko and the spec on cross-origin access to window.history
: Align Gecko and the spec on cross-origin access to window.history
Status: RESOLVED FIXED
: dev-doc-complete, relnote
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla19
Assigned To: Bobby Holley (:bholley) (busy with Stylo)
:
Mentors:
Depends on: 809318
Blocks: CVE-2013-0793
  Show dependency treegraph
 
Reported: 2012-10-15 03:31 PDT by Bobby Holley (:bholley) (busy with Stylo)
Modified: 2014-12-08 19:02 PST (History)
14 users (show)
bobbyholley: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed
fixed
wontfix
fixed


Attachments
Forbid cross-origin access to the History object. v1 (1.35 KB, patch)
2012-10-16 07:23 PDT, Bobby Holley (:bholley) (busy with Stylo)
mrbkap: review+
akeybl: approval‑mozilla‑esr17+
lukasblakk+bugs: approval‑mozilla‑b2g18+
Details | Diff | Splinter Review
Fix sandbox test. v1 (1020 bytes, patch)
2012-10-16 07:24 PDT, Bobby Holley (:bholley) (busy with Stylo)
mrbkap: review+
Details | Diff | Splinter Review
Fix {push,pop}State tests. v1 (2.36 KB, patch)
2012-10-16 07:24 PDT, Bobby Holley (:bholley) (busy with Stylo)
justin.lebar+bug: review-
Details | Diff | Splinter Review
Fix {push,pop}State tests. v2 (2.72 KB, patch)
2012-10-16 07:46 PDT, Bobby Holley (:bholley) (busy with Stylo)
bobbyholley: review+
Details | Diff | Splinter Review
Add tests for the same-origin policy. v1 (3.13 KB, patch)
2012-10-16 08:59 PDT, Bobby Holley (:bholley) (busy with Stylo)
mrbkap: review+
Details | Diff | Splinter Review

Description Bobby Holley (:bholley) (busy with Stylo) 2012-10-15 03:31:24 PDT
The only two objects that are cross-origin accessible per spec are Window and Location. However, Gecko currently also allows access to the back(), forward(), and go() methods on window.history. jst says this is probably just heritage from the netscape 4 days.

Cross-origin security policy is pretty important, so I think we should either change Gecko or change the spec. In particular, we should figure out what other UAs do and whether restricting access to window.history is web-compatible.
Comment 1 Bobby Holley (:bholley) (busy with Stylo) 2012-10-15 03:44:06 PDT
So I put together the following testcase, and evaluated it with hixie's live DOM viewer:

http://pastebin.mozilla.org/1867437

It looks like Gecko navigates the iframe back, Chrome/Safari naviate the entire top-level window back, and Opera and IE deny access to the history object entirely.

Given that, I think it makes the most sense for us to kill cross-origin access to the history object and align with the spec here.
Comment 2 Bobby Holley (:bholley) (busy with Stylo) 2012-10-15 03:58:42 PDT
https://tbpl.mozilla.org/?tree=Try&rev=25043303c42f
Comment 3 Bobby Holley (:bholley) (busy with Stylo) 2012-10-15 08:34:24 PDT
CCing smaug in case he has any thoughts about the weird WebKit navigation behavior or my proposed behavior change here.
Comment 4 Olli Pettay [:smaug] 2012-10-15 08:48:03 PDT
Note, Webkit's session history is very broken even without cross-origin frames or anything like that,
so we should not align our behavior with that.

But yes, killing cross-origin access to the the history object sounds ok, especially
if IE and Opera have that behavior.
Comment 5 Bobby Holley (:bholley) (busy with Stylo) 2012-10-16 01:55:10 PDT
Fixed a few tests and pushed again to try:
https://tbpl.mozilla.org/?tree=Try&rev=e2bfdf119790
Comment 6 Olli Pettay [:smaug] 2012-10-16 02:03:29 PDT
(In reply to Bobby Holley (:bholley) from comment #1)
> and Opera and IE deny access to the history
> object entirely.

What does this actually mean? Do they throw an exception?  or what?
Changing this all is very regression risky, so if we decide to go with the behavior IE has,
better to make sure we behave the same way in all the cases.
Comment 7 Bobby Holley (:bholley) (busy with Stylo) 2012-10-16 02:10:27 PDT
(In reply to Olli Pettay [:smaug] from comment #6)
> (In reply to Bobby Holley (:bholley) from comment #1)
> > and Opera and IE deny access to the history
> > object entirely.
> 
> What does this actually mean? Do they throw an exception?  or what?

Yes. The throw an exception when touching the property, just like any cross-origin access to non cross-origin-accessible properties. That's why my patch does here.
Comment 8 Bobby Holley (:bholley) (busy with Stylo) 2012-10-16 07:23:42 PDT
Created attachment 671841 [details] [diff] [review]
Forbid cross-origin access to the History object. v1
Comment 9 Bobby Holley (:bholley) (busy with Stylo) 2012-10-16 07:24:04 PDT
Created attachment 671842 [details] [diff] [review]
Fix sandbox test. v1
Comment 10 Bobby Holley (:bholley) (busy with Stylo) 2012-10-16 07:24:20 PDT
Created attachment 671843 [details] [diff] [review]
Fix {push,pop}State tests. v1
Comment 11 Justin Lebar (not reading bugmail) 2012-10-16 07:29:21 PDT
Comment on attachment 671843 [details] [diff] [review]
Fix {push,pop}State tests. v1

Yay for this bug, but r- because this patch removes important tests.  You'll need to fix this (e.g. by s/example.com/mochi.test:8000/).
Comment 12 Bobby Holley (:bholley) (busy with Stylo) 2012-10-16 07:46:26 PDT
Created attachment 671855 [details] [diff] [review]
Fix {push,pop}State tests. v2
Comment 13 Justin Lebar (not reading bugmail) 2012-10-16 07:53:51 PDT
Comment on attachment 671855 [details] [diff] [review]
Fix {push,pop}State tests. v2

> +  tryBadPushAndReplaceState("http://foo.mochitest:8000");

foo.mochi.test, right?

I wish I knew why I went through all this effort to run the tests at example.com.  Maybe it was because I didn't want to rely on the default mochitest domain not changing (it did change, I believe!).

Anyway, this looks good to me.  Thanks.
Comment 14 Bobby Holley (:bholley) (busy with Stylo) 2012-10-16 08:59:30 PDT
Created attachment 671886 [details] [diff] [review]
Add tests for the same-origin policy. v1

Given our terrible track record of testing very basic stuff like this, I think this is worthwhile.
Comment 15 Bobby Holley (:bholley) (busy with Stylo) 2012-10-17 08:28:28 PDT
Comment on attachment 671855 [details] [diff] [review]
Fix {push,pop}State tests. v2

Doh, it's actually mochi.test:8888, not mochi.test:8000. Canceling review so that I remember to fix this before landing.
Comment 18 Blake Kaplan (:mrbkap) 2012-10-22 14:28:30 PDT
Comment on attachment 671886 [details] [diff] [review]
Add tests for the same-origin policy. v1

Review of attachment 671886 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good. It would be good to test that we correctly distinguish between setting/getting for the props that are sensitive to that distinction (and checking that we throw correctly if someone tries to set a property that's only gettable.
Comment 20 Bobby Holley (:bholley) (busy with Stylo) 2012-10-24 03:07:21 PDT
Not sure if this is relnote-worthy, but I'll set the flag and let other folks decide.
Comment 21 Bobby Holley (:bholley) (busy with Stylo) 2012-10-24 03:08:38 PDT
Comment on attachment 671855 [details] [diff] [review]
Fix {push,pop}State tests. v2

I fixed the :8000 thing. Reapplying review for posterity.
Comment 22 Bobby Holley (:bholley) (busy with Stylo) 2012-10-24 04:20:06 PDT
Quick bustage fix: https://hg.mozilla.org/integration/mozilla-inbound/rev/ab316048d5e1
Comment 23 Bobby Holley (:bholley) (busy with Stylo) 2012-10-24 05:19:29 PDT
Annnnd, another bustage fix for the inexplicable android issue:

https://hg.mozilla.org/integration/mozilla-inbound/rev/5e3b672c303a
Comment 25 Jesper Kristensen 2013-01-29 10:32:34 PST
I have recently done some research on the same origin policy. I have tried to document what I found at https://developer.mozilla.org/en-US/docs/JavaScript/Same_origin_policy_for_JavaScript . I just came across this bug today, and I added a link on the documentation page to the source file defining the policy.

It looks like there are still a number of properties, which are allowed cross-origin in Firefox, but not according to the spec. Namely location.hash, window.blur, window.close, window.closed, window.focus and window.opener. Are there any plans on aligning Firefox and the HTML spec on these additional properties?
Comment 26 Bobby Holley (:bholley) (busy with Stylo) 2013-01-29 11:28:22 PST
(In reply to Jesper Kristensen from comment #25)
> It looks like there are still a number of properties, which are allowed
> cross-origin in Firefox, but not according to the spec. Namely
> location.hash, window.blur, window.close, window.closed, window.focus and
> window.opener. Are there any plans on aligning Firefox and the HTML spec on
> these additional properties?

I would love to. Making that call, however, requires doing research into what other UAs do, which isn't something I've had the time to do. If you're interested in helping out here, it would be _very_ helpful to make a table listing which of these non-standard properties are available in which UAs (we'd want to test Safari, Chrome, Opera, IE9, and IE10 (in the various compat modes)). If the data looks compelling, I'm happy to land a fix to bring us more in line with the spec.
Comment 27 Jesper Kristensen 2013-02-10 03:44:53 PST
(In reply to Bobby Holley (:bholley) from comment #26)
I have filed bug 839867
Comment 28 Bobby Holley (:bholley) (busy with Stylo) 2013-02-22 08:07:21 PST
Comment on attachment 671841 [details] [diff] [review]
Forbid cross-origin access to the History object. v1

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
blocks bug 803870.
User impact if declined: bug 803870. 
Fix Landed on Version: mozilla19
Risk to taking this patch (and alternatives if risky): Somewhat risky in that this is a web-visible change. However, the old behavior was gecko-specific anyway, so it's not clear that sites would have been depending on it. And the release of this bug in mozilla19 should force sites to adapt anyway (though there's a risk for internal intranet apps).
String or UUID changes made by this patch:  None

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Comment 29 Alex Keybl [:akeybl] 2013-02-28 16:38:43 PST
Comment on attachment 671841 [details] [diff] [review]
Forbid cross-origin access to the History object. v1

When given the choice of not fixing bug 803870, investigating a new fix (without testing elsewhere), or taking this patch, triage thinks this is the best route.
Comment 30 Ryan VanderMeulen [:RyanVM] 2013-03-03 15:21:59 PST
Sorry, I didn't see your whiteboard comment after I'd pushed. My fault entirely :(

It applied cleanly (just one makefile fixup for the tests was required) and it seems to have landed OK, though. If you want me to backout, I will of course.

FWIW, I rolled the tests and bustage fixes into one commit for esr17. Also, is this something we should consider for b2g18 uplift as well?

https://hg.mozilla.org/releases/mozilla-esr17/rev/d286e7431abb
Comment 31 Bobby Holley (:bholley) (busy with Stylo) 2013-03-03 15:24:30 PST
No, that's fine. I was just worried that it might need some rebasing and didn't want to waste your time. If the test passed, we should be ok. :-)

Whether or not we uplift to b2g18 depends on what we decide for bug 803870.
Comment 32 Bobby Holley (:bholley) (busy with Stylo) 2013-03-06 08:48:31 PST
Comment on attachment 671841 [details] [diff] [review]
Forbid cross-origin access to the History object. v1

Nominating for b2g18 per comment 28.
Comment 33 Lukas Blakk [:lsblakk] use ?needinfo 2013-03-08 07:51:52 PST
Comment on attachment 671841 [details] [diff] [review]
Forbid cross-origin access to the History object. v1

approving for uplift to b2g18 since we do want the bug it's blocking.
Comment 34 Ryan VanderMeulen [:RyanVM] 2013-03-08 07:54:11 PST
https://hg.mozilla.org/releases/mozilla-b2g18/rev/ef5bc6848172
Comment 35 Ryan VanderMeulen [:RyanVM] 2013-04-02 18:44:56 PDT
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/a1a5c516b32f
Comment 36 Will Bamberg [:wbamberg] 2014-12-08 19:02:28 PST
I'm removing dev-doc-needed, because I think the docs added in bug 965898, comment 48, cover this, too. If anyone disagrees please reinstate dev-doc-needed and explain what else is needed.

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