650273 - "Assertion failure: compartment mismatched" with setTimeout

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[moz_bug_r_a4]

Attachment: testcase (crashes Firefox when loaded)

Assertion failure: compartment mismatched, at js/src/jscntxtinlines.h:542

Comment 1

[Daniel Veditz [:dveditz]]

missed macaw, we'll look at triaging into possible chemspills when we know the cause and fix.

Comment 2

[Daniel Veditz [:dveditz]]

What build does this happen in? mozilla-central, Firefox 4? regression between 4 and 4.0.1pre?

Comment 3

[moz_bug_r_a4]

I can reproduce this in mozilla-central and Firefox 4, and this is not a
regression between 4 and 4.0.1pre.

Comment 4

[Daniel Veditz [:dveditz]]

mrbkap: is this likely an sg:high same-origin violation, or an sg:critical worm-into-chrome problem? (or neither?)

Minus for mozilla-2.0 for now, but if we have a ff5 fix in hand and do a chemspill we can pick it up then.

Comment 5

[Johnny Stenback (:jst)]

Blake, can you have a look here?

Comment 6

[Johnny Stenback (:jst)]

Blake says this bug in and of itself is not exploitable, but he's got a fix in the works that will fix this bug and also bug 650275.

Comment 7

[Marcia Knous [:marcia]]

Depends on Bug 650275 - Sheila commented in that bug asking for status.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: wip

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix

Comment 11

[Luke Wagner [:luke]]

Comment on attachment 531794 [details] [diff] [review]
Fix

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

::: js/src/jswrapper.cpp
@@ +375,5 @@
> +    if (!context->stack.pushDummyFrame(context, *scopeChain, &frame))
> +        return false;
> +
> +    if (context->isExceptionPending())
> +        context->wrapPendingException();

As discussed, I think this can be dropped since the compartment isn't changing.

Comment 12

[Luke Wagner [:luke]]

(That is, the wrapPendingException, not the first two lines that splinter included.  We need a way to control how much splinter quotes.)

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/tracemonkey/rev/ab172b4ca00d

Comment 14

[Asa Dotzler [:asa]]

Luke, Blake, what's the risk/reward trade-off here for potentially taking into Firefox 5?

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

Asa, this fixes a few security bugs, and adds belts-and-braces for other security bugs that we fixed in another way. It also maintains some invariants that were otherwise not well maintained in the face of malicious scripts.

On the other hand, this isn't the safest patch, as it does add a couple of new cases that didn't exist before (and required at least one fix, in bug 656460). I'd say it's worth it, overall.

Comment 16

[Chris Leary [:cdleary] (not checking bugmail)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/ab172b4ca00d

Comment 17

[Boris Zbarsky [:bzbarsky]]

This caused bug 659207.

I would expect that to have a good chance of causing this to not build if pushed for fx5 as-is.

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 531794 [details] [diff] [review]
Fix

I think this should go into Firefox 5. It hasn't had fallout except for bug 659207 and that's a minor change to the patch.

Comment 19

[Asa Dotzler [:asa]]

Blake, is the patch at bug 650273 strictly necessary if we agree to take the patch here? Or is that something "only" of concern for our developers?

Comment 20

[Blake Kaplan (:mrbkap) (inactive)]

I don't think "'only' of concern for our developers is a valid phrase. If all you care about is cutting down on the number of patches pushed to Aurora, then I'm happy to fold the patches together. I don't want to split hairs over a compile fix with no effect on actual code. There is no advantage to taking this patch without the compile fix in bug 659207.

Comment 21

[Asa Dotzler [:asa]]

(In reply to comment #20)
> I don't think "'only' of concern for our developers is a valid phrase. If
> all you care about is cutting down on the number of patches pushed to
> Aurora, then I'm happy to fold the patches together. I don't want to split
> hairs over a compile fix with no effect on actual code. There is no
> advantage to taking this patch without the compile fix in bug 659207.

I'm not interested in numbers of patches at all. What I'm trying to figure out here is what part of these three issues, this bug, bug 650275 and bug 659207 are necessary to solve the security issue that I presumed was a concern for our users. That's why release drivers are tracking these bugs.

Comment 22

[Blake Kaplan (:mrbkap) (inactive)]

Well, except that release drivers are pretty explicitly not tracking bug 659207. For the record:

This bug (bug 650273) requires bug 650275 (i.e. you can't land this bug without that one). bug 650275 only has an effect with this bug checked in (the problem that it fixes doesn't exist without the patch in this bug). bug 659207 fixes bz's build bustage as fallout from the patch here.

Comment 23

[Johnny Stenback (:jst)]

Per further discussion with mrbkap etc we've decided that it's too late to take this for 5, but this landed in time for 6 already, so it'll be fixed there.

Comment 24

[Asa Dotzler [:asa]]

Comment on attachment 531794 [details] [diff] [review]
Fix

removing approval since this is no longer intended for beta for 5.

Comment 25

[Jesse Ruderman]

I took some ideas from the testcase and added them to my DOM fuzzer. Thanks moz_bug_r_a4 and mrbkap. (7ea4f291dde2, 68b6a7a21708, 8958f6b556c7, 10570e474f13)