733984 - Prepare Location object wrappers for a compartment-per-global world

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

Location object security is special, so it's no surprise that it breaks in various places when compartment-per-global is turned on. This bug is about fixing those places.

The patches here apply on top of the miscellaneous cosmetic changes I made to Location wrapper handling in bug 667388 (ie, SameOriginOrCrossOriginAccessiblePropertiesOnly -> LocationPolicy).

Comment 1

[Bobby Holley (:bholley)]

Attachment: part 1 - Stop specializing createHolder, and simplify holder creation in WrapperFactory::Rewrap. v1

Comment 2

[Bobby Holley (:bholley)]

Attachment: part 2 - Apply Location wrappers for same-origin cross-compartment wrapping. v1

This isn't an issue right now, since it can't ever happen outside of sandboxes, which content can't use. But if it could, it could get a pure CrossCompartmentWrapper to a Location object, which is bad.

Comment 3

[Bobby Holley (:bholley)]

There's a third patch that fixes another case, but I'm a bit too fried to do it properly right now. I'll hopefully get it done tomorrow.

Also, just to reiterate, these patches go on top of those in bug 667388.

Comment 4

[Bobby Holley (:bholley)]

Attachment: part 3 - Generalize partially-transparent wrappers and use them for Location object wrapping as well. v1

Here's the third and final patch. Flagging mrbkap for review.

Note that this is all more or less covered by existing tests that would otherwise break when we turn on compartment-per-global.

Comment 5

[Bobby Holley (:bholley)]

I've moved the dependent Location patch out of bug 667388 and into this one. So the patches here should be reviewed first.

Comment 6

[Bobby Holley (:bholley)]

Attachment: part 1 - Stop specializing createHolder, and simplify holder creation in WrapperFactory::Rewrap. v1

Comment 7

[Bobby Holley (:bholley)]

Attachment: part 2 - Clarify the security characteristics of Location objects. v1

Comment 8

[Bobby Holley (:bholley)]

Attachment: part 3 - Use the Location security policy even for content accessing chrome. v1

I'm adding asserts about when we do and don't have a Location object behind the wrapper, and this case was hitting them. What we do here doesn't so much matter given how this stuff all works. On the one hand, statically using a restrictive policy is slightly more defense-in-depth. On the other hand, if this stuff is broken we're screwed in much more serious ways than content reading chrome locations, and using a consistent wrapper scheme allows us to make stronger asserts and assumptions.

I opted for stronger assumptions and more understandable security code. If Blake feels strongly though, I could go the other way and sprinkle '|| isChrome(obj)' throughout the asserts though.

Comment 9

[Bobby Holley (:bholley)]

Attachment: part 4 - Apply Location wrappers for same-origin cross-compartment wrapping. v1

Comment 10

[Bobby Holley (:bholley)]

Attachment: part 5 - Explicitly disallow expandos on location wrappers. v1

We'll soon be treating Location objects as first-class partially-transparent Xray wrappers. However, we need to prevent them from defining properties on the holder and on the underlying object in order to avoid confusing plugins. This wasn't a problem before for various reasons, but we'll need it soon.

Comment 11

[Bobby Holley (:bholley)]

Attachment: part 6 - Generalize partially-transparent wrappers and use them for Location object wrapping as well. v1

Comment 12

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (:bholley) from comment #10)
> Created attachment 605228 [details] [diff] [review]
> part 5 - Explicitly disallow expandos on location wrappers. v1

OK, I think this was too draconian. In particular, some of our mochitests rely on putting expandos on location. Maybe we should just forbid shadowing, instead.

Comment 13

[Bobby Holley (:bholley)]

Attachment: part 5 - Explicitly disallow shadowing on location wrappers. v2

Switching part 5 to disable shadowing (rather than all expandos), per a discussion on IRC with mrkbap.

Comment 14

[Bobby Holley (:bholley)]

Attachment: part 6 - Generalize partially-transparent wrappers and use them for Location object wrapping as well. v2

Rebasing part 6.

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 605224 [details] [diff] [review]
part 2 - Clarify the security characteristics of Location objects. v1

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

::: js/xpconnect/wrappers/AccessCheck.h
@@ +106,5 @@
>  struct CrossOriginAccessiblePropertiesOnly : public Policy {
>      static bool check(JSContext *cx, JSObject *wrapper, jsid id, js::Wrapper::Action act,
>                        Permission &perm) {
> +
> +        // Location objects should always use LocationPolicy.

Uber-nit: why the newline before the comment?

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 605225 [details] [diff] [review]
part 3 - Use the Location security policy even for content accessing chrome. v1

This is fine. It was mostly an oversight on my part.

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 605635 [details] [diff] [review]
part 5 - Explicitly disallow shadowing on location wrappers. v2

This can't happen with this patch since location doesn't do this, but it seems like this will prevent scriptable helpers from defining properties on wrappers that have this flag (I'm not entirely sure, though). Might be worth commenting on that somewhere.

Comment 18

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 605223 [details] [diff] [review]
part 1 - Stop specializing createHolder, and simplify holder creation in WrapperFactory::Rewrap. v1

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

::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ +123,5 @@
> +    if (!holder)
> +        return nsnull;
> +
> +    CompartmentPrivate *priv =
> +        (CompartmentPrivate *)JS_GetCompartmentPrivate(js::GetObjectCompartment(holder));

If you're touching this, static_cast, please. Also use NULL consistently

Comment 19

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 605636 [details] [diff] [review]
part 6 - Generalize partially-transparent wrappers and use them for Location object wrapping as well. v2

I was with you until here. Why is this necessary? It seems like it adds complication for no benefit, but maybe I'm missing something?

Comment 20

[Bobby Holley (:bholley)]

Comment on attachment 605636 [details] [diff] [review]
part 6 - Generalize partially-transparent wrappers and use them for Location object wrapping as well. v2

> I was with you until here. Why is this necessary? It seems like it adds
> complication for no benefit, but maybe I'm missing something?

Sorry, I should have explained more.

This patch is necessary for correctness because it allows us to piggyback on the existing IsTransparent machinery in XrayWrapper.

This is necessary because same-origin scripts expect to be able to see each others' expandos. In the old world, they were in the same compartment, so everyone could just stick things on the LW and it would be visible in the right places. But now, that doesn't work anymore. So we need a consistent place for these things to live. As such, the actual object seemed appropriate.

Unfortunately, I just thought of a potential security concern. Suppose the outer window is an iframe navigated to foo.com, inside a bar.com page. Within the iframe, foo.com script puts expandos on it. The bar.com page then grabs the location object of the iframe, and subsequently navigates the iframes to bar.com. This makes the dynamic access check pass, so that bar.com can see foo.com's expandos.

So I guess we need another approach here. Fundamentally, we've now got two non-overlapping security considerations - the Location DOM object, and expandos. Perhaps we need a CrossCompartmentLW in addition to XLW?

Comment 21

[Bobby Holley (:bholley)]

Blake and I talked about this on IRC. Sounds like we're going to have to do something more involved for patch 6, so I'm going to land patches 1-5 in the mean time to unblock other stuff (like bug 667388).

Pushed patches 1-5 to try: https://tbpl.mozilla.org/?tree=Try&rev=d57c78e0795f

Comment 22

[Bobby Holley (:bholley)]

I pushed the first 4 patches to inbound. I'll work on getting 5 and 6 sorted out.

http://hg.mozilla.org/integration/mozilla-inbound/rev/6519fd0616e2
http://hg.mozilla.org/integration/mozilla-inbound/rev/ae71e6cdc6c4
http://hg.mozilla.org/integration/mozilla-inbound/rev/c8464d25e40d
http://hg.mozilla.org/integration/mozilla-inbound/rev/2b113540e75a

Please leave this bug open after merging, since there are still 2 patches to land.

Comment 23

[Bobby Holley (:bholley)]

Pushed part 5 after resolving test failures: http://hg.mozilla.org/integration/mozilla-inbound/rev/8794d663b5f4

Still need to write and land part 6.

Comment 24

[Bobby Holley (:bholley)]

Attachment: part 6 - Make same-origin cross-compartment Location object access go through the LW in the host compartment. v1

Uploading a new part 6 patch. Flagging blake for review.

Blake, I realized after our conversation that putting expandos on the location object itself probably allows clever callers to bypass the LW. I'm guessing that they can add an expando method to the LW and do something clever with |this| to get access to the underlying object. My understanding is that our LW strategy is all a capability thing, so this would kind of hose it.

On the bright side, the double-wrapping approach seems to work out pretty well.

This undoes some work from a previous patch, but that work has already landed.

Comment 25

[Ed Morley [:emorley]]

Almost missed comment 22, for ease of spotting in the future, please can you put [leave open] in the whiteboard, along the lines of http://blog.bonardo.net/2012/03/23/how-you-can-help-mozilla-inbound-sheriffs-when-pushing - thank you :-)

Parts 1-5:
https://hg.mozilla.org/mozilla-central/rev/2b113540e75a
https://hg.mozilla.org/mozilla-central/rev/c8464d25e40d
https://hg.mozilla.org/mozilla-central/rev/ae71e6cdc6c4
https://hg.mozilla.org/mozilla-central/rev/6519fd0616e2
https://hg.mozilla.org/mozilla-central/rev/8794d663b5f4

Comment 26

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 608932 [details] [diff] [review]
part 6 - Make same-origin cross-compartment Location object access go through the LW in the host compartment. v1

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

::: js/xpconnect/wrappers/WrapperFactory.cpp
@@ +393,5 @@
> +            if (IsLocationObject(obj)) {
> +                JSAutoEnterCompartment ac;
> +                if (!ac.enter(cx, obj))
> +                    return nsnull;
> +                obj = WrapLocationObject(cx, obj);

Won't this create a location object wrapper for each time we go through this code?

Comment 27

[Bobby Holley (:bholley)]

(In reply to Blake Kaplan (:mrbkap) from comment #26)
> Won't this create a location object wrapper for each time we go through this
> code?

Good catch!

I've split the patch 6 stuff into bug 733984 to avoid confusion. Resolving this bug FIXED.