Bug 754044 (CVE-2012-1959)

Same-compartment security wrappers can be trivially bypassed by passing them to another compartment

VERIFIED FIXED in Firefox 14

Status

()

defect
VERIFIED FIXED
7 years ago
6 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

({sec-high})

unspecified
mozilla15
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox13 wontfix, firefox14+ fixed, firefox15 fixed, firefox-esr1014+ fixed)

Details

(Whiteboard: [sg:high][advisory-tracking+][qa?])

Attachments

(7 attachments)

1.05 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
1.77 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
2.30 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
9.07 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
3.68 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
3.73 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
27.38 KB, patch
Details | Diff | Splinter Review
Assignee

Description

7 years ago
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.
Assignee

Comment 1

7 years ago
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.
Keywords: sec-critical
Whiteboard: [sg:critical]
Assignee

Comment 2

7 years ago
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?
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>
(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.
Assignee

Comment 5

7 years ago
I've got patches for this. Uploading.
Assignee

Comment 7

7 years ago
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.
Attachment #623170 - Flags: review?(mrbkap)
Assignee

Comment 9

7 years ago
We leave it null for gecko for the time being to keep this patch small.
Attachment #623172 - Flags: review?(mrbkap)
Assignee

Comment 12

7 years ago
Collectively, these fix one of the bugs causing crashes in bug 752309.
Assignee

Comment 13

7 years ago
Pushed this all to try, along with bug 754311:
https://tbpl.mozilla.org/?tree=Try&rev=52e05025cd4d
Attachment #623169 - Flags: review?(mrbkap) → review+
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?
Attachment #623170 - Flags: review?(mrbkap) → review+
Attachment #623171 - Flags: review?(mrbkap) → review+
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.
Attachment #623172 - Flags: review?(mrbkap) → review+
Attachment #623173 - Flags: review?(mrbkap) → review+
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.
Attachment #623174 - Flags: review?(mrbkap) → review+
Assignee

Comment 17

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

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

Comment 18

7 years ago
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.
Assignee

Comment 19

7 years ago
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.
Assignee: nobody → bobbyholley+bmo
Target Milestone: --- → mozilla15
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.
Assignee

Comment 22

7 years ago
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...
(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.
Blocks: 737559
5/1->6/1
Assignee

Comment 25

7 years ago
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.
Keywords: sec-criticalsec-high
Whiteboard: [sg:critical] → [sg:high]
Assignee

Comment 26

7 years ago
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.
wontfix for Beta/Fx13 but we should fix this on 14 and then figure out what the hell to do with ESR-10.
Depends on: 758572
Assignee

Comment 28

7 years ago
These patches apply to aurora verbatim. Requesting approval for all of them.
Assignee

Updated

7 years ago
Attachment #623169 - Flags: approval-mozilla-aurora?
Assignee

Updated

7 years ago
Attachment #623170 - Flags: approval-mozilla-aurora?
Assignee

Updated

7 years ago
Attachment #623171 - Flags: approval-mozilla-aurora?
Assignee

Updated

7 years ago
Attachment #623172 - Flags: approval-mozilla-aurora?
Assignee

Updated

7 years ago
Attachment #623173 - Flags: approval-mozilla-aurora?
Assignee

Updated

7 years ago
Attachment #623174 - Flags: approval-mozilla-aurora?
Assignee

Comment 29

7 years ago
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.
Assignee

Comment 30

7 years ago
Pushed aurora version to try: https://tbpl.mozilla.org/?tree=Try&rev=97c1c322fe87
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.
Attachment #623169 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #623170 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #623171 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #623172 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #623173 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #623174 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee

Comment 32

7 years ago
Something went funny with the try push - trying again to see if it's me: https://tbpl.mozilla.org/?tree=Try&rev=d36b801c4160
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.
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?
Assignee

Comment 36

7 years ago
(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.
Assignee

Comment 37

7 years ago
(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
(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.
Status: RESOLVED → VERIFIED
Assignee

Comment 39

7 years ago
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.
Attachment #634057 - Flags: approval-mozilla-esr10?
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.
Attachment #634057 - Flags: approval-mozilla-esr10? → approval-mozilla-esr10+
(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
Assignee

Comment 43

7 years ago
(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
Whiteboard: [sg:high] → [sg:high][advisory-tracking+]
Alias: CVE-2012-1959
(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.
Whiteboard: [sg:high][advisory-tracking+] → [sg:high][advisory-tracking+][qa?]
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?
(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?
Seems straightforward enough...I'll give it a try, thought it will take me some time.
Group: core-security
You need to log in before you can comment on or make changes to this bug.