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

RESOLVED FIXED in mozilla14

Status

()

Core
XPConnect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

unspecified
mozilla14
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 6 obsolete attachments)

12.27 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
11.39 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
1.50 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
2.28 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
8.71 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
4.00 KB, patch
Details | Diff | Splinter Review
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).
Created attachment 603955 [details] [diff] [review]
part 1 - Stop specializing createHolder, and simplify holder creation in WrapperFactory::Rewrap. v1
Attachment #603955 - Flags: review?(mrbkap)
Created attachment 603956 [details] [diff] [review]
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.
Attachment #603956 - Flags: review?(mrbkap)
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.
Created attachment 604133 [details] [diff] [review]
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.
Attachment #604133 - Flags: review?(mrbkap)
I've moved the dependent Location patch out of bug 667388 and into this one. So the patches here should be reviewed first.
Attachment #603955 - Flags: review?(mrbkap)
Attachment #603956 - Flags: review?(mrbkap)
Attachment #604133 - Flags: review?(mrbkap)
Attachment #603955 - Attachment is obsolete: true
Attachment #603956 - Attachment is obsolete: true
Attachment #604133 - Attachment is obsolete: true
Created attachment 605223 [details] [diff] [review]
part 1 - Stop specializing createHolder, and simplify holder creation in WrapperFactory::Rewrap. v1
Attachment #605223 - Flags: review?(mrbkap)
Created attachment 605224 [details] [diff] [review]
part 2 - Clarify the security characteristics of Location objects. v1
Attachment #605224 - Flags: review?(mrbkap)
Created attachment 605225 [details] [diff] [review]
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.
Attachment #605225 - Flags: review?(mrbkap)
Created attachment 605226 [details] [diff] [review]
part 4 - Apply Location wrappers for same-origin cross-compartment wrapping. v1
Attachment #605226 - Flags: review?(mrbkap)
Created attachment 605228 [details] [diff] [review]
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.
Attachment #605228 - Flags: review?(mrbkap)
Created attachment 605229 [details] [diff] [review]
part 6 - Generalize partially-transparent wrappers and use them for Location object wrapping as well. v1
Attachment #605229 - Flags: review?(mrbkap)
(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.
Created attachment 605635 [details] [diff] [review]
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.
Attachment #605228 - Attachment is obsolete: true
Attachment #605228 - Flags: review?(mrbkap)
Attachment #605635 - Flags: review?(mrbkap)
Created attachment 605636 [details] [diff] [review]
part 6 - Generalize partially-transparent wrappers and use them for Location object wrapping as well. v2

Rebasing part 6.
Attachment #605229 - Attachment is obsolete: true
Attachment #605636 - Flags: review?(mrbkap)
Attachment #605229 - Flags: review?(mrbkap)

Updated

5 years ago
Attachment #605223 - Flags: review?(mrbkap) → review+
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?
Attachment #605224 - Flags: review?(mrbkap) → review+
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.
Attachment #605225 - Flags: review?(mrbkap) → review+

Updated

5 years ago
Attachment #605226 - Flags: review?(mrbkap) → review+
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.
Attachment #605635 - Flags: review?(mrbkap) → review+
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 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?
Attachment #605636 - Flags: review?(mrbkap)
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?
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
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.
Pushed part 5 after resolving test failures: http://hg.mozilla.org/integration/mozilla-inbound/rev/8794d663b5f4

Still need to write and land part 6.
Created attachment 608932 [details] [diff] [review]
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.
Attachment #605636 - Attachment is obsolete: true
Attachment #608932 - Flags: review?(mrbkap)
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
Target Milestone: --- → mozilla14
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?
Attachment #608932 - Flags: review?(mrbkap)
(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.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.