754044 - (CVE-2012-1959) Same-compartment security wrappers can be trivially bypassed by passing them to another compartment

Status: VERIFIED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

Sigh. They're totally fucked. I don't know how we didn't think of this before.

JS wrapping doesn't know about SCSWs. So our strategy is just inject them in places like NativeInterface2JSObject, and hope that nobody can actually touch the object.

This works, mostly. Occasionally we find a hole (see bug 737559). But while debugging CPG wrapper reparenting fallout, I realized that there's a simple workaround. Cross-compartment wrappers don't go through SCSWs in most cases, but rather have the filtering policy built directly into them. So when an object is wrapped cross-compartment, the SCSW is stripped off. But when the object is read _back_, the JS engine has no idea that there was ever supposed to be a SCSW there. So it just says "hey, the underlying object here is in the same compartment we're wrapping for. No wrappers needed!"

With compartment-per-global (i.e. Nightly), this is super easy make happen. Just load up a trivial iframe, define the security-checked property on its contentWindow, and read it back again. Boom. I wrote a testcase to confirm that this works.

Without compartment-per-global (i.e. Aurora, Beta, ESR, and Release), it's a tiny bit trickier, since normally an origin only gets one compartment to play with. But this can be easily defeated by making the trivial iframe same-origin via document.domain (rather than by its true origin), which puts it in a separate compartment. I didn't write a testcase for this because it's 11pm and I'm tired, but I don't see why it wouldn't work just as well.

On Nightly, we recently upped the ante by making the Components object protected by SCSWs (bug 735280). So this would in theory give full Components access to web content. But thankfully, I don't think gabor ever got around to removing the old-style CAPS check (did you, gabor?). So I think we're safe there for now. But we're still sitting ducks as far as Location and SOWs are concerned.

So yeah, we need a story here. Pronto.

Comment 1

[Bobby Holley (:bholley)]

Some more thoughts:

Modulo moving all SCSW-ed objects into a "compartment of forbidden things", I think our only answer here is to add another wrap callback that lets spidermonkey apply SCSWs when the underlying object turns out to be same-compartment as the context. This shouldn't be too hard, and would prevent us from having to apply SCSWs manually (though we'd still need to remember to call wrap in the appropriate places).

The annoying part, though, is that we don't get the easy checking of invariants that assertSameCompartment gives us. I wonder if there's some way we could assert (in debug builds) that certain objects are only accessed through our special proxies. Maybe we trigger them to blow up by default, and have the SCSWs disarm()/rearm() them? I wonder if jorendorff has any ideas.

Comment 2

[Bobby Holley (:bholley)]

I think this probably should have the same security rating as bug 737559. I kind of think that one is under-rated, though. There are probably nasty things that can be done to native anonymous content. Also, can't you sometimes read cookies out of window.location?

Comment 3

[Luke Wagner [:luke]]

Actually, the "compartment of forbidden things" strategy... sounds pretty good -- going with the flow.  New compartment means new global but perhaps that wouldn't break the things in this forbidden compartment or perhaps we could create a special "proxy global".  Anyway, I know there is a timeliness to the present problem, but longer term a new compartment sounds attractive.  <meme>Compartmentalize all the things!</meme>

Comment 4

[Gabor Krizsanits (INACTIVE)]

(In reply to Bobby Holley (:bholley) from comment #0)
> But thankfully, I don't think gabor ever
> got around to removing the old-style CAPS check (did you, gabor?). 

Nope. It's Bug 735281 and it's not very urgent to fix.

Comment 5

[Bobby Holley (:bholley)]

I've got patches for this. Uploading.

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 1 - Pass stopAtOuter=false in GetWrappedNativeOfJSObject, because that's what we mean. v1

Comment 7

[Bobby Holley (:bholley)]

Attachment: Part 2 - Alter the semantics of UnwrapObject{,Checked} to check for outer windows on the initial wrapper passed. v1

This distinction is the only reason for writing the code in this confusing manner. And I don't think we need it now that we have stopAtOuter.

Comment 8

[Bobby Holley (:bholley)]

Attachment: Part 3 - Simplify the logic surrounding the prewrap callback in jscompartment.cpp. v1

Comment 9

[Bobby Holley (:bholley)]

Attachment: Part 4 - Introduce sameCompartmentWrapObjectCallback. v1

We leave it null for gecko for the time being to keep this patch small.

Comment 10

[Bobby Holley (:bholley)]

Attachment: Part 5 - Apply same-compartment security wrappers in same-compartment wrapping callback. v1

Comment 11

[Bobby Holley (:bholley)]

Attachment: Part 6 - Remove manual injection of same-compartment security wrappers, and make sure to call JS_Wrap* instead. v1

Comment 12

[Bobby Holley (:bholley)]

Collectively, these fix one of the bugs causing crashes in bug 752309.

Comment 13

[Bobby Holley (:bholley)]

Pushed this all to try, along with bug 754311:
https://tbpl.mozilla.org/?tree=Try&rev=52e05025cd4d

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 623170 [details] [diff] [review]
Part 2 - Alter the semantics of UnwrapObject{,Checked} to check for outer windows on the initial wrapper passed. v1

I assume that you've audited the other callers of UnwrapObject to make sure that they work wit this change?

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 623172 [details] [diff] [review]
Part 4 - Introduce sameCompartmentWrapObjectCallback. v1

I was going to ask if it made sense to simply use the preWrap hook (after moving the call above the compartment check), but realized that overloads the return value too much, so this is clearly the best strategy.

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 623174 [details] [diff] [review]
Part 6 - Remove manual injection of same-compartment security wrappers, and make sure to call JS_Wrap* instead. v1

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

::: js/xpconnect/src/XPCConvert.cpp
@@ +1084,5 @@
>  
> +    // Outerize if necessary.
> +    // XXXbholley - This could probably also go in the same-compartment wrapping
> +    // callback. But it's a bigger change than I want to make right now.
> +    flat = JS_ObjectToOuterObject(cx, flat);

I'm not sure this change is as big as you think... WrapperFactory::PrepareForWrapping already outerizes, so any caller that needs an inner window here is already broken. That being said, if you do want to do this in a followup, please replace the XXXbholley with a TODO (bug #): and file a followup.

Comment 17

[Bobby Holley (:bholley)]

Pushed again to try with another patch to move outerization into the callback:

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

Comment 18

[Bobby Holley (:bholley)]

I've thought about this a bit more, and I don't think it makes sense to outerize in the same-compartment wrapping hook. We really just need it in that one place, and so it just adds overhead to do it on every wrap.

Another thing I realized is that the contract for same-compartment wrapping hooks is kind of crappy. We _might_ call them with an unwrapped object, but we might not (so the 'rewrap' part is a misnomer). Unfortunately, I don't think there's a great alternative. If we always unwrap before calling that hook, we won't be able to reuse any cross-compartment wrappers we've already computed (a rather common case, I suspect).

It doesn't affect us for now though (since the same-compartment wrapping hook just checks for WNs). I'm going to mention it in a comment though.

Comment 19

[Bobby Holley (:bholley)]

Pushed to m-i:

http://hg.mozilla.org/integration/mozilla-inbound/rev/59f29fffe03f
http://hg.mozilla.org/integration/mozilla-inbound/rev/dbe0dd266065
http://hg.mozilla.org/integration/mozilla-inbound/rev/5567ff3506ae
http://hg.mozilla.org/integration/mozilla-inbound/rev/e4fcbb84474f
http://hg.mozilla.org/integration/mozilla-inbound/rev/3be962df706a
http://hg.mozilla.org/integration/mozilla-inbound/rev/e66591a43a5b

Flagging for tracking on all branches. This is a large cumulative diff, which sucks, but it also has the advantage that it doesn't point as obviously to the exploit as bug 737559 does. We might be able to get away with just landing this everywhere without drawing too much attention to it.

Comment 20

[Ed Morley [:emorley]]

http://hg.mozilla.org/mozilla-central/rev/59f29fffe03f
http://hg.mozilla.org/mozilla-central/rev/dbe0dd266065
http://hg.mozilla.org/mozilla-central/rev/5567ff3506ae
http://hg.mozilla.org/mozilla-central/rev/e4fcbb84474f
http://hg.mozilla.org/mozilla-central/rev/3be962df706a
http://hg.mozilla.org/mozilla-central/rev/e66591a43a5b

Comment 21

[Alex Keybl [:akeybl]]

Would somebody mind addressing the risk of this bug? If

* the patch is lowish risk or fallout will be easily discoverable through crash-stats/input feedback
* we land the fix on mozilla-beta on or before 5/21

we can still take the fix for FF13.

Comment 22

[Bobby Holley (:bholley)]

What do you think, Blake and/or Johnny? Is this the kind of thing we want to land on beta? It's a nontrivial patch, but not the scariest thing in the world...

Comment 23

[Alex Keybl [:akeybl]]

(In reply to Alex Keybl [:akeybl] from comment #21)
> Would somebody mind addressing the risk of this bug? If
> 
> * the patch is lowish risk or fallout will be easily discoverable through
> crash-stats/input feedback
> * we land the fix on mozilla-beta on or before 5/21
> 
> we can still take the fix for FF13.

One other requirement for landing on FF13 is that we backport to the ESR in a low-risk fashion prior to our 5/1 final go-to-build.

Comment 24

[Alex Keybl [:akeybl]]

5/1->6/1

Comment 25

[Bobby Holley (:bholley)]

So, I've thought about this a bit more.

Backporting to beta is tricky, because this bug also depends on the work that was done in bug 739796. And I'm not 100% sure that the work over there doesn't depend on anything else that I was doing in the flurry of patches I landed in march. That stuff is all on aurora now, which should make backporting easier, but it's not on beta.

I also think the security rating isn't entirely correct. The biggest threat here is that it allows the unwrapping of SOWs, which gives untrusted content access to the XBL that implements <video> controls and such. And given that we generally turned that off for the web, there are almost certainly sg:crits lurking there. But this isn't an sg:crit directly. The worst this is on its own is that it allows URIs to be read cross-origin, which allows attackers to read cookie strings, passwords, etc that happen to be in GET parameters.

So I'm going to compromise on sg:high, and suggest that we shouldn't take this for beta. I'm going to prepare an aurora patch now.

Comment 26

[Bobby Holley (:bholley)]

Oh hm, there's still the ESR10 issue.

1 - Let it ride for ESR10.
2 - Port the refactor of same-compartment security wrappers: https://bugzilla.mozilla.org/attachment.cgi?id=609924&action=diff
3 - Write a new version of the wrapping callback that just manually applies the wrappers.

Comment 27

[Daniel Veditz [:dveditz]]

wontfix for Beta/Fx13 but we should fix this on 14 and then figure out what the hell to do with ESR-10.

Comment 28

[Bobby Holley (:bholley)]

These patches apply to aurora verbatim. Requesting approval for all of them.

Comment 29

[Bobby Holley (:bholley)]

Note that these patches are associated with a very intermittent crash in bug 758572. I'm pretty sure that this just exposes an existing bug, but I'm not sure.

Comment 30

[Bobby Holley (:bholley)]

Pushed aurora version to try: https://tbpl.mozilla.org/?tree=Try&rev=97c1c322fe87

Comment 31

[Alex Keybl [:akeybl]]

Comment on attachment 623169 [details] [diff] [review]
Part 1 - Pass stopAtOuter=false in GetWrappedNativeOfJSObject, because that's what we mean. v1

[Triage Comment]
sg:high, approving for Aurora 14.

Comment 32

[Bobby Holley (:bholley)]

Something went funny with the try push - trying again to see if it's me: https://tbpl.mozilla.org/?tree=Try&rev=d36b801c4160

Comment 33

[Bobby Holley (:bholley)]

Ok, I get the same funny try behavior with an empty patch from aurora. Must be infra-related: https://tbpl.mozilla.org/?tree=Try&rev=a5f3d01d8686

Pushed to m-a:
http://hg.mozilla.org/releases/mozilla-aurora/rev/9d86c4735ae7
http://hg.mozilla.org/releases/mozilla-aurora/rev/f86cce5d95c8
http://hg.mozilla.org/releases/mozilla-aurora/rev/fd813603c89e
http://hg.mozilla.org/releases/mozilla-aurora/rev/cc780c6ba8ef
http://hg.mozilla.org/releases/mozilla-aurora/rev/fec931ade719
http://hg.mozilla.org/releases/mozilla-aurora/rev/de05eff84e4c

Comment 34

[Daniel Veditz [:dveditz]]

We've got 5 more ESR-10 releases, would it be better to have code that's more like what's going on the trunk or a one-off tweak? Happy to have this fixed either way.

Comment 35

[Al Billings [:abillings - ex-MoCo]]

Do we have a testcase or repro steps to demonstrate the fixed issue here that can be used by folks to verify the fix on the various releases?

Comment 36

[Bobby Holley (:bholley)]

(In reply to Al Billings [:abillings] from comment #35)
> Do we have a testcase or repro steps to demonstrate the fixed issue here
> that can be used by folks to verify the fix on the various releases?

Use the mochitest over in bug 737559.

Comment 37

[Bobby Holley (:bholley)]

(In reply to Daniel Veditz [:dveditz] from comment #34)
> We've got 5 more ESR-10 releases, would it be better to have code that's
> more like what's going on the trunk or a one-off tweak? Happy to have this
> fixed either way.

I rebased the necessary patches (underlying, and ones from this bug) onto esr10 and pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=45b9c16cb862

Comment 38

[Al Billings [:abillings - ex-MoCo]]

(In reply to Bobby Holley (:bholley) from comment #36)
> (In reply to Al Billings [:abillings] from comment #35)
> > Do we have a testcase or repro steps to demonstrate the fixed issue here
> > that can be used by folks to verify the fix on the various releases?
> 
> Use the mochitest over in bug 737559.

Verified using that mochitest on my own mozilla-central build. It passes.

Comment 39

[Bobby Holley (:bholley)]

Attachment: esr10 rollup patch v1

Try results are hard to interpret because it looks like the try builders aren't set up to deal with esr10. But I _think_ it looks ok.

Uploading a rollup patch and flagging for esr10 approval given comment 34.

Comment 40

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 634057 [details] [diff] [review]
esr10 rollup patch v1

Yes, unfortunately we don't have a solid way to get try ESR we can only rely on the builds/results post-landing.

Comment 41

[Bobby Holley (:bholley)]

Pushed to mozilla-esr10:

http://hg.mozilla.org/releases/mozilla-esr10/rev/a18a80b8c741
(and ancestors)

Comment 42

[Ed Morley [:emorley]]

(In reply to Bobby Holley (:bholley) from comment #41)
> Pushed to mozilla-esr10:
> 
> http://hg.mozilla.org/releases/mozilla-esr10/rev/a18a80b8c741
> (and ancestors)

This caused make-check failures on multiple platforms:
https://tbpl.mozilla.org/php/getParsedLog.php?id=12891191&tree=Mozilla-Esr10

Comment 43

[Bobby Holley (:bholley)]

(In reply to Ed Morley [:edmorley] from comment #42)

> This caused make-check failures on multiple platforms:
> https://tbpl.mozilla.org/php/getParsedLog.php?id=12891191&tree=Mozilla-Esr10

Attempted fix: http://hg.mozilla.org/releases/mozilla-esr10/rev/85a40497d54a

Comment 44

[u279076]

(In reply to Al Billings [:abillings] from comment #38)
> > Use the mochitest over in bug 737559.
> 
> Verified using that mochitest on my own mozilla-central build. It passes.

Can someone please help me do the same for Firefox 14, 15, and ESR? I've never run mochitests before.

Comment 45

[Al Billings [:abillings - ex-MoCo]]

I did it by building my own Firefox and running the test after building it. Are you interested in building 14, 15, and ESR from hg?

Comment 46

[u279076]

(In reply to Al Billings [:abillings] from comment #45)
> I did it by building my own Firefox and running the test after building it.
> Are you interested in building 14, 15, and ESR from hg?

That's not a problem for me, but I've never run mochitests before. Would you be willing to give me some direction or point me to some docs?

Comment 47

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

http://lmgtfy.com/?q=mochitests

Comment 48

[u279076]

Seems straightforward enough...I'll give it a try, thought it will take me some time.