Closed Bug 867771 Opened 7 years ago Closed 6 years ago

Compartment Mismatch in DebuggerObject_getClass

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24
Tracking Status
firefox22 --- unaffected
firefox23 + fixed
firefox24 + fixed
b2g18 --- unaffected
b2g-v1.1hd --- unaffected
b2g-v1.2 --- unaffected

People

(Reporter: bholley, Assigned: jimb)

References

Details

(Keywords: sec-moderate, Whiteboard: [adv-main23-])

Attachments

(2 files, 1 obsolete file)

I jut hit this when poking around in the web console. I just did $('iframeid').contentWindow and it crashed. I'll attach a stack.
Attached file stack
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
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.
QA Contact: 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?
Flags: needinfo?(bobbyholley+bmo)
Ah! Reproduced! I just needed a newer build.
Flags: needinfo?(bobbyholley+bmo)
Duplicate of this bug: 868823
Attached patch enter refobj compartment (obsolete) — Splinter Review
We should just enter the refobj compartment before trying to get the className.
Attachment #745706 - Flags: review?(jorendorff)
Duplicate of this bug: 869567
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. :)
Assignee: general → evilpies
Keywords: sec-moderate
QA Contact: 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?
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);
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.
Maybe we should also assert that in assertEnteredPolicy?
(In reply to Tom Schuster [:evilpie] from comment #13)
> Maybe we should also assert that in assertEnteredPolicy?

That looks reasonable; give it a try!
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?
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.
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?
Flags: needinfo?(jimb)
: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
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).
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 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.
Attachment #745706 - Flags: review?(jorendorff)
Flags: needinfo?(jimb)
Jimb are you on this?
Duplicate of this bug: 881291
Hey, can we move on this?
Flags: needinfo?(jimb)
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.
Assignee: evilpies → jimb
Attachment #745706 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #761139 - Flags: review?(evilpies)
Flags: needinfo?(jimb)
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.
Attachment #761139 - Flags: review?(evilpies) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1384f7cc8f9
Flags: in-testsuite+
OS: Mac OS X → All
Hardware: x86 → All
Target Milestone: --- → mozilla24
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.
Attachment #761139 - Flags: approval-mozilla-aurora?
Attachment #761139 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/d1384f7cc8f9
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
As a regression from bug 862531, this should be Firefox 22 unaffected, right?
Yes, that's right.
Whiteboard: [adv-main23-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.