Bobby Holley (:bholley) Assignee
	Description • 12 years ago

In the wake of all the recent hubub about the Location object, we started to brainstorm how to make our implementation more solid.  The basic problem right no is that nsLocation is per-inner-window, but it describes (and thus inherits security characteristics from) the outer window, which jumps around between compartments. So Location is the one DOM object whose security characteristics cannot be inferred from its compartment. We currently handle this by wrapping all Location objects in same-compartment security wrappers and always doing dynamic checks, but this has proven to be brittle (not to mention a pain in the neck).

The current draft of the spec states that Location is a per-inner-window object that describes the inner window (not the outer window). Matt Wobensmith wrote some tests [1] that demonstrate that all major engines (Gecko, WebKit, Presto, and Trident) do the contrary (Location describes the outer window - the WindowProxy in spec terms). So the spec here is fiction, and probably not web-compatible.

Since the spec's approach seems unlikely to stick, the other approach is to have one Location object per outer window. For us, this would mean transplanting the Location object during navigation (along with the window). Matt's tests indicate that Trident and Presto already have one Location object per browsing context, so this is likely web-compatible. Matt is currently investigating what happens to expandos when this happens, and will report back shortly (hopefully in this bug).

So this bug is about implementing the second approach. I've already got some mostly-working patches for this, but there's more work to be done.

[1] http://people.mozilla.org/~mwobensmith/window_test/doc_win.htm

	Boris Zbarsky [:bzbarsky]
	Comment 1 • 12 years ago

Please raise the spec issue here!

	Bobby Holley (:bholley) Assignee
	Comment 2 • 12 years ago

(In reply to Boris Zbarsky (:bz) from comment #1)
> Please raise the spec issue here!

I will. I want to make sure this implementation is feasible first, though.

	Matt Wobensmith [:mwobensmith][:matt:]
	Comment 3 • 12 years ago

I've updated the tests at the link above to include a few checks for "expando" (or dynamic) property additions to the location object. As expected, there exists variance across browsers. I'll leave it up to you to say if this should be cause for  concern.

Also happy to add more tests in this area, or any others.

	Bobby Holley (:bholley) Assignee
	Comment 4 • 12 years ago

Thanks Matt!

Looks like expando stuff is indeed all over the place. I'm guessing we can probably just get away with clearing expandos on navigation.

Matt, can you add the ability to navigat the iframe back/forward? I'd like to see whether the engines that do Location per Browsing Context restore the old expandos when navigating back. I'm hoping they don't.

	Matt Wobensmith [:mwobensmith][:matt:]
	Comment 5 • 12 years ago

As far as going back/forward, I just click links A and B repeatedly to observe their expando behavior. 

Did you need something different w/r/t navigation? Just tell me and I'll do it, just not sure what's needed.

	Matt Wobensmith [:mwobensmith][:matt:]
	Comment 6 • 12 years ago

Actually, on further thought, I'm inferring that what you need is a way to navigate the page w/o resetting the expando values each time, to observe whether those values are preserved on their own by the browser.

I added back/forward links to do just this, give it a try.

	Bobby Holley (:bholley) Assignee
	Comment 7 • 12 years ago

(In reply to Matt Wobensmith from comment #6)
> Actually, on further thought, I'm inferring that what you need is a way to
> navigate the page w/o resetting the expando values each time, to observe
> whether those values are preserved on their own by the browser.

That's part of it. The other part is that navigating a->b->a is not the same thing as navigating a->b and then going back to a. The former creates 3 Windows/Documents, and the latter creates two (and restores the first one). So if we move the Location object from one Window to another, and then back again, we need to decide what should happen with the expandos that were placed on Location by the first Window.

	Bobby Holley (:bholley) Assignee
	Comment 8 • 12 years ago

Interesting. Here's the behavior when navigating a->b and then going back:

Engines with Location object per Window:
* Gecko: Expandos on both Location objects are preserved.
* WebKit: Expando on first window's Location is gone. Second one is preserved.

Engines with Location object per Browsing Context:
* Presto: Second Window's expandos remain, even when navigated back. Weird.
* Trident: Same as above.

Given that, I think it's probably safe for us to just forget expandos on navigation, which simplifies things a lot.

	Bobby Holley (:bholley) Assignee
	Comment 9 • 12 years ago

https://tbpl.mozilla.org/?tree=Try&rev=2592f9cf244f

	Bobby Holley (:bholley) Assignee
	Comment 10 • 12 years ago

https://tbpl.mozilla.org/?tree=Try&rev=d9e38ad1b03e

	Bobby Holley (:bholley) Assignee
	Comment 11 • 12 years ago

Fixed the last remaining orange (b-c). Uploading patches and flagging for review.

	Bobby Holley (:bholley) Assignee
	Comment 12 • 12 years ago

Right now the code holds a ref to the per-inner-window Location object of the
Window associated with the sandbox, and uses that to determine if the page has
navigated. This breaks when we have one Location object per docshell, because
the check always succeeds. Just use the window ID instead, which is also less
likely to leak.

	Bobby Holley (:bholley) Assignee
	Comment 13 • 12 years ago

We have a very nice and robust infrastructure for per-origin expando sharing
for Xrays. Unfortunately, the only way to currently exercise it is with
Location objects, since those are the only objects with same-origin Xrays
(cross-origin Xrays don't allow expandos at all). Sandbox wantXrays would
almost work here, but we actually make an explicit exception for sandboxes
so that they never share expandos (the 'exlusive global' stuff).

I think the infrastructure is nice and that we may want it in the future, so
I don't really want to back it out. But I also don't want to leave it in the
tree untested. So I'm adding an explicit Cu API to force DOM compartments to
use same-origin Xrays. This allows us to keep testing this stuff, which I think
is important.

	Bobby Holley (:bholley) Assignee
	Comment 14 • 12 years ago

This allows us to remove the same-compartment Location wrappers. This can
go away when we move Location to the new bindings and get access to
[Unforgeable].

	Bobby Holley (:bholley) Assignee
	Comment 16 • 12 years ago

Note that Part 5 is one logical change (the pieces aren't independently correct).
I'll land them as a single changeset, but I think it's easier to review the pieces
separately.

	Bobby Holley (:bholley) Assignee
	Comment 18 • 12 years ago

\o/

	Bobby Holley (:bholley) Assignee
	Comment 20 • 12 years ago

This is now unused.

	Bobby Holley (:bholley) Assignee
	Comment 22 • 12 years ago

Comment on attachment 679717 [details] [diff] [review]
Part 5 - The Deed (Part D) - Fix up tests to work with new behavior. v1

I'd like bz to mull this bug in his head for a little bit, so I'll flag him for review on patch to update our test suite to the new behavior. :-)

	Blake Kaplan (:mrbkap) (inactive)
	Comment 23 • 12 years ago

Comment on attachment 679711 [details] [diff] [review]
Part 2 - Rescue expando sharing tests. v1

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

::: js/xpconnect/tests/chrome/test_expandosharing.xul
@@ +29,5 @@
>  
>    // Wait for all child frames to load.
>    var gLoadCount = 0;
>    function frameLoaded() {
> +    if (++gLoadCount == 5)

Can we future-proof this hardcoded 5 with 'window.frames.length'?

	Blake Kaplan (:mrbkap) (inactive)
	Comment 24 • 12 years ago

Comment on attachment 679712 [details] [diff] [review]
Part 3 - Implement shadowing protection in nsDOMClassInfo. v1

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

::: js/xpconnect/src/XPCWrappedNative.cpp
@@ +3208,5 @@
>      return NS_OK;
>  }
>  
> +/* [notxpcom] bool HasNativeMember (in jsval name); */
> +bool

I think this has to be NS_IMETHODIMP_(bool) for windows calling convention magic.

	Blake Kaplan (:mrbkap) (inactive)
	Comment 25 • 12 years ago

Comment on attachment 679713 [details] [diff] [review]
Part 4 - Add the option to forget expandos during wrapper reparenting, and remove unnecessary outparam. v1

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

::: js/xpconnect/idl/nsIXPConnect.idl
@@ +523,5 @@
>      reparentWrappedNativeIfFound(in JSContextPtr aJSContext,
>                                   in JSObjectPtr  aScope,
>                                   in JSObjectPtr  aNewParent,
> +                                 in nsISupports  aCOMObj,
> +                                 in boolean      aForgetExpandos);

This needs an IID bump.

::: js/xpconnect/src/XPCWrappedNative.cpp
@@ +1498,1 @@
>          return NS_OK;

Nit: Nuke the braces here.

@@ +1554,5 @@
> +            // Cloning the object copies the reserved slots, including the expando
> +            // chain. We may decide to clone the expando chain below, but we
> +            // certainly don't want to carry the reference over verbatim. Null it
> +            // out.
> +            SetWNExpandoChain(newobj, nullptr);

This isn't currently a bug because we always CloneExpandoChain right now, right?

	Blake Kaplan (:mrbkap) (inactive)
	Comment 26 • 12 years ago

Comment on attachment 679716 [details] [diff] [review]
Part 5 - The Deed (Part C) - Remove specialized Location security wrappers. v1

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

*So* *much* *cleaner*!

	Blake Kaplan (:mrbkap) (inactive)
	Comment 27 • 12 years ago

Comment on attachment 679720 [details] [diff] [review]
Part 7 - Tests. v1

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

Holding my breath that the expando stuff doesn't come back to bite us.

	Bobby Holley (:bholley) Assignee
	Comment 28 • 12 years ago

All ready to go except for bz reviewing those test changes. I think this is a big enough deal that it warrants a full try push:

https://tbpl.mozilla.org/?tree=Try&rev=50fdb43f1732

	Bobby Holley (:bholley) Assignee
	Comment 29 • 12 years ago

One orange, from that lovely Location test I added.

====

There are a number of fixes to this important tests, so this warrants a separate
patch.

First of all, the boundTo machinery goes away, because we no longer have same-
compartment Xrays giving us the weird bound method machinery.

Furthermore, now that the sensitive methods are just regular old methods
off the prototype, after transplanting they'll just fail in the
GetWrappedNativeOfJSObject rat's nest (when they can't unwrap the security
wrapper), so we'll just get a generic XPConnect error instead of a security
exception. I want to fix this soon, so I changed the skipMessageCheck stuff
to use todo_is.

However, _that_ caused an UNEXPECTED-PASS for the DefaultValue test (which
was the only one of the array of tests that was throwing a security exception
in step 2). So I added an annotation for that in item[2].

	Boris Zbarsky [:bzbarsky]
	Comment 30 • 12 years ago

Comment on attachment 679717 [details] [diff] [review]
Part 5 - The Deed (Part D) - Fix up tests to work with new behavior. v1

>-// Now make sure we can't watch location.

Hmm.  It would be good to keep testing that, if at all possible...  Or do we no longer care about being able to watch it?  If not, why not?

r=me modulo that bit

	Bobby Holley (:bholley) Assignee
	Comment 31 • 12 years ago

(In reply to Boris Zbarsky (:bz) from comment #30)
> Comment on attachment 679717 [details] [diff] [review]
> Part 5 - The Deed (Part D) - Fix up tests to work with new behavior. v1
> 
> >-// Now make sure we can't watch location.
> 
> Hmm.  It would be good to keep testing that, if at all possible...  Or do we
> no longer care about being able to watch it?  If not, why not?

Implementing the watchpoint protection here turned out to be a rats nest, and Jorendorff concluded that it doesn't seem like there's a compelling reason to prevent watching it anymore. Can you think of one?

	Boris Zbarsky [:bzbarsky]
	Comment 32 • 12 years ago

Say page A loads page B in a subframe.  Then page A navigates the subframe to URL C (which may not be same-origin with B) by setting location.href on the subframe's location.  Allowing page B to get access to URL C is a security problem, imo.

	Boris Zbarsky [:bzbarsky]
	Comment 33 • 12 years ago

Comment on attachment 681363 [details] [diff] [review]
Bug 808608 - The Deed (Part E) - Fix test_bug802557. v1

r=me, I think.  I assume we still don't allow shadowing the proto props on the object, right?

	Bobby Holley (:bholley) Assignee
	Comment 34 • 12 years ago

(In reply to Boris Zbarsky (:bz) from comment #32)
> Say page A loads page B in a subframe.  Then page A navigates the subframe
> to URL C (which may not be same-origin with B) by setting location.href on
> the subframe's location.  Allowing page B to get access to URL C is a
> security problem, imo.

Are you concerned about the point at which A does subframe.location.href = X;? If A and B are same-origin, then it's not a security issue IMO. If they're not same-origin, then subframe.location will be an Xray wrapper, and won't enter the compartment of B at all, and thus won't trigger any of its (per-compartment) watchpoints.

Once the navigation happens, the Location object is transplanted and all that's left behind is a wrapper. Presumably the watchpoint will still be in effect on the wrapper, but its effect should be inherently limited to its home compartment.

CCing jorendorff just to make sure I'm speaking the truth about the watchpoint implementation here.

	Bobby Holley (:bholley) Assignee
	Comment 35 • 12 years ago

It is true, however, that watchpoints could potentially interfere with plugins trying to set window.location (gets should be safe). Is that ok? It effectively allows content to shadow Location setters, but not getters.

Flagging for sec review on any case, both for this issue and for all the other issues that might be at play with this major change.

I'm planning on landing this after the branch so that it has maximal bake time.

	Boris Zbarsky [:bzbarsky]
	Comment 36 • 12 years ago

> Are you concerned about the point at which A does subframe.location.href = X;?

Yes.

> If they're not same-origin, then subframe.location will be an Xray wrapper, and won't
> enter the compartment of B at all, and thus won't trigger any of its (per-compartment)
> watchpoints.

OK, good.  Can we add a test for that?

> Is that ok?

I want to say yes... but not sure.

	Bobby Holley (:bholley) Assignee
	Comment 37 • 12 years ago

(I'm waiting to land this until after the branch).

	Jason Orendorff [:jorendorff]
	Comment 38 • 12 years ago

(In reply to Bobby Holley (:bholley) from comment #34)
> CCing jorendorff just to make sure I'm speaking the truth about the
> watchpoint implementation here.

I know about watchpoints, but I don't know enough about the location object or transplanting to be able to answer this. Ping me on IRC?

	Bobby Holley (:bholley) Assignee
	Comment 39 • 12 years ago

Jorendorff and I just talked about this on IRC.

When the Location object is transplanted, its guts are swapped with a cross-compartment wrapper. Since watchpoints are stored in per-compartment map, this means that we end up getting a cross-compartment wrapper in that map. This is a bit weird, since the watchpoint machinery actually forbids adding non-native objects to the map, but jorendorff is pretty sure it's harmless.

Long story short - setting watchpoints on Location should be ok.

	Bobby Holley (:bholley) Assignee
	Comment 40 • 12 years ago

So, the spec discussion is kind of in flux right now, in this thread:

http://lists.w3.org/Archives/Public/public-whatwg-archive/2012Nov/0083.html

It looks like the approach used in this bug (transplanting the Location object itself) isn't going to fly. Hixie proposed the possibility of making Location more like Window (that is to say, having a LocationProxy), which we'd like, but it would happen in a separate bug.

Now that we have C++ security checks in nsLocation, we don't need the same-compartment Location security wrappers anymore, which are 90% of the pain associated with our current approach. So I'm morphing this bug into a bug to remove those. I'll be landing some of the patches (but not all), with various test munging.

	Bobby Holley (:bholley) Assignee
	Comment 41 • 12 years ago

Removing test coverage isn't great. But the only reason this test is doing all
this funny stuff with Location is that it thinks that it's always wrapped in
an Xray wrapper and that we always do a dynamic security check, which is no
longer true. Moreover, it checks for very specific error messages, which are
kind of in flux right now as I'm fixing up GWNOJO. The calls are never going
to actually succeed (since location isn't a Node), so it's not really clear
how to fix up this test to do something uniquely useful in a readable way.
I've added enough Location test coverage recently that I'm comfortable removing
this part.

	Bobby Holley (:bholley) Assignee
	Comment 42 • 12 years ago

https://tbpl.mozilla.org/?tree=Try&rev=f32576c32ba0

	Bobby Holley (:bholley) Assignee
	Comment 43 • 12 years ago

This is green. The one outstanding review needed is a little bit of test munging that doesn't need to block the landing. Blake can review it ex-post-facto.

	Bobby Holley (:bholley) Assignee
	Comment 44 • 12 years ago

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4b01f63ea852
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3a0c93f1b34c
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/58b5ab333166
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f630faad853a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/0f3cad59a963
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6b62f09b6f94
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/10bfd3601af1
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/783573d542df
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/81bea5f51d63

	Ed Morley [:emorley]
	Comment 45 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/4b01f63ea852
https://hg.mozilla.org/mozilla-central/rev/3a0c93f1b34c
https://hg.mozilla.org/mozilla-central/rev/58b5ab333166
https://hg.mozilla.org/mozilla-central/rev/f630faad853a
https://hg.mozilla.org/mozilla-central/rev/0f3cad59a963
https://hg.mozilla.org/mozilla-central/rev/6b62f09b6f94
https://hg.mozilla.org/mozilla-central/rev/10bfd3601af1
https://hg.mozilla.org/mozilla-central/rev/783573d542df
https://hg.mozilla.org/mozilla-central/rev/81bea5f51d63

	Bobby Holley (:bholley) Assignee
	Comment 46 • 10 years ago

I think this ship has sailed.