737559 - "Assertion failure: !proto->getClass()->ext.outerObject"

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

Attachment: 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]

Attachment: stack trace

Comment 2

[David Mandelin [:dmandelin]]

Bobby, you added this assertion--could you comment on how serious the bug is?

Comment 3

[Bobby Holley (:bholley)]

(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) (inactive)]

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) (inactive)]

Attachment: Mochitest

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: 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)]

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) (inactive)]

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]]

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]]

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]]

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]]

Don't check in the test until after this ships though.

Comment 15

[Bobby Holley (:bholley)]

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]]

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)]

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]]

(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)]

I agree that we should not land this. It only draws attention to the more general problem.

Comment 20

[Johnny Stenback (:jst)]

Closing bug, this was fixed by bug 754044.

Comment 21

[Daniel Veditz [:dveditz]]

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]

as jst mentions in comment 20, this is fixed for esr in bug 754044 as well.

Comment 23

[u279076]

Verified fixed using the attached testcase in Firefox 10.0.6esrpre Debug 2012-07-11.

Comment 24

[u279076]

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)]

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) (inactive)]

The mochitest here no longer makes sense. We don't need to land it.