Last Comment Bug 737559 - "Assertion failure: !proto->getClass()->ext.outerObject"
: "Assertion failure: !proto->getClass()->ext.outerObject"
Status: VERIFIED FIXED
[sg:moderate][advisory-tracking+] don...
: assertion, privacy, sec-moderate, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86_64 Mac OS X
: -- critical (vote)
: ---
Assigned To: Blake Kaplan (:mrbkap)
: Anthony Hughes (:ashughes) [GFX][QA][Mentor]
Mentors:
Depends on: CVE-2012-1959
Blocks: 326633 735281 735280 753622
  Show dependency treegraph
 
Reported: 2012-03-20 11:48 PDT by Jesse Ruderman
Modified: 2013-03-11 16:59 PDT (History)
15 users (show)
mrbkap: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
verified
verified
14+
verified


Attachments
testcase (asserts fatally when loaded) (77 bytes, text/html)
2012-03-20 11:48 PDT, Jesse Ruderman
no flags Details
stack trace (6.41 KB, text/plain)
2012-03-20 11:48 PDT, Jesse Ruderman
no flags Details
Mochitest (1.97 KB, patch)
2012-04-03 06:57 PDT, Blake Kaplan (:mrbkap)
bobbyholley: review+
Details | Diff | Splinter Review
Possible fix (998 bytes, patch)
2012-04-03 07:01 PDT, Blake Kaplan (:mrbkap)
bobbyholley: review+
Details | Diff | Splinter Review

Description Jesse Ruderman 2012-03-20 11:48:25 PDT
Created attachment 607646 [details]
testcase (asserts fatally when loaded)

Assertion failure: !proto->getClass()->ext.outerObject, at js/src/jsinferinlines.h:1162

This assertion was added in bug 719841.
Comment 1 Jesse Ruderman 2012-03-20 11:48:58 PDT
Created attachment 607647 [details]
stack trace
Comment 2 David Mandelin [:dmandelin] 2012-04-02 13:46:52 PDT
Bobby, you added this assertion--could you comment on how serious the bug is?
Comment 3 Bobby Holley (:bholley) (busy with Stylo) 2012-04-02 14:13:16 PDT
(In reply to David Mandelin from comment #2)
> Bobby, you added this assertion--could you comment on how serious the bug is?

It was added at jorendorff's instructions. But I think it's bad. Maybe jorendorff can comment further?
Comment 4 Blake Kaplan (:mrbkap) 2012-04-03 06:55:22 PDT
This is pretty bad. The potential at this point depends on how important the security checks around native anonymous content are: if there's an sg:crit bug depending on that, then this is sg:crit. However, in the absence of any of those, the most I can make of this bug is an sg:moderate (tracking the location object).

The assertion here is pointing out that an inner window has been exposed directly to JS. That bug in and of itself is not exploitable: the inner window will be same origin for all time and the wrapper that goes away has no security characteristics in and of itself. The behavior would be odd, though.

The reason this is at least sg:moderate is because this bug allows you to remove the security wrapper around the location object. I have a patch that fixes the issue pointed out by Jesse's testcase, but I wonder if this means that we should tell the JS engine somehow about same-compartment wrappers so that JS_WrapObject can do the Right Thing.
Comment 5 Blake Kaplan (:mrbkap) 2012-04-03 06:57:58 PDT
Created attachment 611793 [details] [diff] [review]
Mochitest
Comment 6 Blake Kaplan (:mrbkap) 2012-04-03 07:01:24 PDT
Created attachment 611794 [details] [diff] [review]
Possible fix

As I attach this patch, I wonder if the proper solution to this bug wouldn't be to get rid of the XPCNativeWrapper constructor in content altogether... That would be bug 575002, already filed by jorendorff.
Comment 7 Bobby Holley (:bholley) (busy with Stylo) 2012-04-03 19:44:48 PDT
Comment on attachment 611794 [details] [diff] [review]
Possible fix

I would much prefer to teach the JS engine about same-compartment wrappers somehow. It seems like our whole setup here is pretty weak and we don't have a great way of enforcing invariants.

If you have the time, I'd be totally down to sit down and brainstorm something. If you're busy at the moment though (as I am), r=bholley for the mean time.
Comment 8 Blake Kaplan (:mrbkap) 2012-04-05 09:10:59 PDT
I'm a little queasy checking this patch in. While it isn't critical, the patch points pretty directly to the problem and how to exploit it.

Dan, should I check this in anyway?
Comment 11 Jason Orendorff [:jorendorff] 2012-05-01 14:42:38 PDT
Did the brainstorming of comment 7 ever happen?

Assuming no one has time to do violence to the JS engine at this moment, here are some other possible plans.

- We could fix it via bug 575002, but IIUC that un-exposes the bug rather than
  actually fixing it.

- We could create some busywork bug to do some API cleanup (make a jsfriendapi
  function that does this particular kind of unwrapping) and fix it there, and
  hope no one notices.

- We could do some API cleanup in this area and fold it into the direct proxies
  patch in bug 703537. No bad guy is going to read that whole patch. If we remove
  the 3-line comment from this patch, it should blend in pretty well there.

- We can sit on this fix indefinitely.

Thoughts?
Comment 12 Daniel Veditz [:dveditz] 2012-05-09 18:35:59 PDT
Or land as-is on May 21 on all the branches, shipping on June 5. Or even over the next weekend (may 25-27) to make sure it at least makes it into Beta 6 (building may 28) for some sanity testing.

Neither API cleanup nor implementing Harmony proxies are going to fly on the beta branch or ESR. The patch is going to be checked-in relatively exposed for a week or two no matter what we do so don't twist yourselves into a knot worrying about mozilla-central (unless there's a big gap between m-c and beta/esr check-ins, so don't do that).
Comment 13 Daniel Veditz [:dveditz] 2012-05-09 18:40:53 PDT
Comment on attachment 611794 [details] [diff] [review]
Possible fix

[Approval Request Comment]
Regression caused by (bug #): Firefox 4 wrapper/js rewrites
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: users can be tracked on the web. Possibly worse exploits depending on whether add-ons using XPCNativeWrapper() can be abused
Fix Landed on Version: none yet, exploit fairly obvious from patch
Risk to taking this patch (and alternatives if risky): remote possibility of breaking an add-on that was inadvertently relying on the security hole.
String changes made by this patch: none
Comment 14 Daniel Veditz [:dveditz] 2012-05-09 18:43:09 PDT
Don't check in the test until after this ships though.
Comment 15 Bobby Holley (:bholley) (busy with Stylo) 2012-05-10 14:30:17 PDT
This is kind of just a special case of bug 754044, and this fix does no good in the general case. I think we might want to avoid drawing attention to this until we have a story over there.
Comment 16 Alex Keybl [:akeybl] 2012-05-11 16:31:50 PDT
Holding in the queue till May 21. We want this for our fifth beta, so that we have the opportunity to back out and test before release if necessary.
Comment 17 Bobby Holley (:bholley) (busy with Stylo) 2012-05-12 15:36:08 PDT
I have patches over in bug 754044, though they're bigger than this fix. This fix doesn't do us much good in light of that bug. So we need to figure out how to proceed on all this stuff.
Comment 18 Alex Keybl [:akeybl] 2012-05-16 11:28:21 PDT
(In reply to Bobby Holley (:bholley) from comment #17)
> I have patches over in bug 754044, though they're bigger than this fix. This
> fix doesn't do us much good in light of that bug. So we need to figure out
> how to proceed on all this stuff.

It sounds like our options are

1) Land this fix for FF13, but it doesn't cover the general case and draws attention to the issue
2) Land bug 754044 for FF13, take additional risk since it's a 
3) Land bug 754044 for FF14, let it ride the train

This bug is sg:moderate, and bug 754044 is sg:crit. Option 1 doesn't seem like a very good option given what you've said, so it really comes down to Options 2/3 which depend upon a risk evaluation of bug 754044. I'll comment there.
Comment 19 Bobby Holley (:bholley) (busy with Stylo) 2012-05-16 12:15:13 PDT
I agree that we should not land this. It only draws attention to the more general problem.
Comment 20 Johnny Stenback (:jst, jst@mozilla.com) 2012-06-13 12:31:38 PDT
Closing bug, this was fixed by bug 754044.
Comment 21 Daniel Veditz [:dveditz] 2012-06-21 16:54:43 PDT
The fix in this bug is obsolete, right? maybe we should mark it that way so someone doesn't come along and use it.
Comment 22 Lukas Blakk [:lsblakk] use ?needinfo 2012-06-22 14:53:50 PDT
as jst mentions in comment 20, this is fixed for esr in bug 754044 as well.
Comment 23 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-07-11 10:51:38 PDT
Verified fixed using the attached testcase in Firefox 10.0.6esrpre Debug 2012-07-11.
Comment 24 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-08-21 15:05:50 PDT
Confirmed the attach testcase crashes with this assertion in Firefox 14.0a1 Nightly Debug 2012-03-20. Testcase does not crash/assert Firefox 14.0 Beta Debug 2012-06-14, nor Firefox 15.0 Aurora Debug 2012-06-14.
Comment 25 Christian Holler (:decoder) 2013-03-11 10:31:54 PDT
I tried adding the attached mochitest, but it failed in a try run:

https://tbpl.mozilla.org/php/getParsedLog.php?id=20526319&tree=Try&full=1#error0

mrbkap, can you check what's wrong and commit the test? Thanks!
Comment 26 Blake Kaplan (:mrbkap) 2013-03-11 16:59:51 PDT
The mochitest here no longer makes sense. We don't need to land it.

Note You need to log in before you can comment on or make changes to this bug.