867771 - Compartment Mismatch in DebuggerObject_getClass

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bobby Holley (:bholley)]

I jut hit this when poking around in the web console. I just did $('iframeid').contentWindow and it crashed. I'll attach a stack.

Comment 1

[Bobby Holley (:bholley)]

Attachment: stack

Comment 2

[Bobby Holley (:bholley)]

It appears that refobj here is not same-compartment as cx, but gets passed to JSObject::className:

http://mxr.mozilla.org/mozilla-central/source/js/src/vm/Debugger.cpp#4175

Comment 3

[Jim Blandy :jimb]

This looks very much like a regression introduced by bug 862531. I think it will be faster to fix than to write a test case for.

I don't believe it's a serious cross-compartment problem, as none of our extant className Proxy handlers do anything non-trivial. At most, they forward to the className handler of some other object.

Comment 4

[Jim Blandy :jimb]

I thought I was going to be able to reproduce this easily, but I'm not able to; I don't think I actually understand the conditions under which it occurs. Could you provide specific reproduction instructions?

Comment 5

[Jim Blandy :jimb]

Ah! Reproduced! I just needed a newer build.

Comment 7

[Tom S [:evilpie]]

Attachment: enter refobj compartment

We should just enter the refobj compartment before trying to get the className.

Comment 9

[Andrew McCreight [:mccr8]]

This is a compartment mismatch, which is bad, but all the testcases we've seen involve entering things into the developer console, so it seems like a limited problem in practice.  You can probably do much worse if you can get somebody to enter something in the console. :)

Comment 10

[Jim Blandy :jimb]

Comment on attachment 745706 [details] [diff] [review]
enter refobj compartment

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

Yes, this looks like exactly the right thing --- thanks, Tom!

But --- can we get a regression test case?

Comment 11

[Tom S [:evilpie]]

Dammit can't get it to crash. Should something like this work:
var g = newGlobal('new-compartment');
var hits = 0;
var dbg = new Debugger(g);
dbg.onDebuggerStatement = function (frame) {
    assertEq(frame.arguments[0].class, 'Object');
    hits++;
};
g.eval("function f(arg) { debugger; }");
g.eval("var g2 = newGlobal('new-compartment'); f(g2.eval('({})'));");
assertEq(hits, 1);

Comment 12

[Jim Blandy :jimb]

From the attached stack, it seems like only the xray wrapper proxy handler is bothering to check compartment consistency. Since there are no xray wrappers in the JS shell, this may not be reproducible in the JS shell as it stands.

However, what if we added:

assertSameCompartment(cx, proxy);

to BaseProxyHandler::className? That's a sound assertion, and I *think* that would make your test case assert.

Comment 13

[Tom S [:evilpie]]

Maybe we should also assert that in assertEnteredPolicy?

Comment 14

[Jim Blandy :jimb]

(In reply to Tom Schuster [:evilpie] from comment #13)
> Maybe we should also assert that in assertEnteredPolicy?

That looks reasonable; give it a try!

Comment 15

[Tom S [:evilpie]]

I better checkin in the fix now. We can worry about the test later. I am however not certain about landing s-s stuff. Did we miss a branch or would this go on trunk?

Comment 16

[Andrew McCreight [:mccr8]]

If you agree with this being sec-moderate, then you can just go ahead and land.  The s-s stuff only kicks in for high and crit.

Comment 17

[Andrew McCreight [:mccr8]]

This has been waiting for review for almost 20 days, and people keep hitting this crash.  Is there somebody else who could review it?  Maybe jimb?

Comment 18

[Andrew McCreight [:mccr8]]

:kats also is hitting this:
I can reproduce a crash with this signature. Crash report at:

https://crash-stats.mozilla.com/report/index/bp-2c64cb36-f2c9-45e9-af25-144cc2130524

STR:
1) Install latest desktop nightly for mac os x with a clean profile
2) Load http://www.muho-mannheim.de/frame.php?path=/veranstaltungen/index.htm
3) In the navbar on the left, click on the link that says "Alle Veranstaltungen"
4) Cmd+option+k to bring up the developer console
5) Execute the expression "window[2]"

A few more crash reports from my Aurora build that I originally encountered this on:
https://crash-stats.mozilla.com/report/index/bp-a5a7f540-a6b6-4008-965e-30f772130524
https://crash-stats.mozilla.com/report/index/bp-fc36e592-a23e-4428-a08a-714f62130524

Comment 19

[Andrew McCreight [:mccr8]]

Marking for tracking not for the security aspects, but because this is something people seem to hit quite often, and might cause crashes (it always causes crashes in Nightly/Aurora but in Beta/release it will just cause unsafe conditions that may or may not crash).

Comment 20

[Andrew McCreight [:mccr8]]

Well, this is something that only arises with devtools, which mitigates how bad it is.  But for people using devtools, it has come up at least 4 times.

Comment 21

[Jim Blandy :jimb]

Comment on attachment 745706 [details] [diff] [review]
enter refobj compartment

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

(Stealing review.)

This needs a test. I'll see if I can get one in there.

Comment 22

[Tom S [:evilpie]]

Jimb are you on this?

Comment 24

[Bobby Holley (:bholley)]

Hey, can we move on this?

Comment 25

[Jim Blandy :jimb]

Attachment: Make Debugger.Object.prototype.getClass switch compartments correctly.

I found a place to add the assertion that makes several existing Debugger tests die; the fix addresses them all. So, with the assertion added, this change is covered by existing tests.

Comment 26

[Jim Blandy :jimb]

Try: https://tbpl.mozilla.org/?tree=Try&rev=5017e312c533

Comment 27

[Tom S [:evilpie]]

Comment on attachment 761139 [details] [diff] [review]
Make Debugger.Object.prototype.getClass switch compartments correctly.

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

I feel like I test this place, but who knows :) I think Jason should have quick look, because this is mostly my patch.

Comment 28

[Jim Blandy :jimb]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d1384f7cc8f9

Comment 29

[Jim Blandy :jimb]

Comment on attachment 761139 [details] [diff] [review]
Make Debugger.Object.prototype.getClass switch compartments correctly.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 862531
User impact if declined: crashes while using devtools
Testing completed (on m-c, etc.): Yes.
Risk to taking this patch (and alternatives if risky): None.
String or IDL/UUID changes made by this patch: None.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d1384f7cc8f9

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5276fa7da911

Comment 32

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

As a regression from bug 862531, this should be Firefox 22 unaffected, right?

Comment 33

[Jim Blandy :jimb]

Yes, that's right.