714590 - "ASSERTION: no self" with treewalker filter, modified __proto__

Status: VERIFIED FIXED

(Core :: XPConnect, defect)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

###!!! ASSERTION: no self: 'self', file e:/builds/moz2_slave/m-cen-w32-dbg/build/xpcom/reflect/xptcall/src/md/win32/xptcstubs.cpp, line 68

Comment 1

[Jesse Ruderman]

Happens on Windows but not on Mac??

Comment 2

[Jesse Ruderman]

Fails in a different way on Linux:

###!!! ASSERTION: bad param: 'Error', file /builds/slave/m-cen-lnx64-dbg/build/xpcom/reflect/xptinfo/src/xptiInterfaceInfo.cpp, line 227

###!!! ASSERTION: no method info: 'info', file /builds/slave/m-cen-lnx64-dbg/build/xpcom/reflect/xptcall/src/md/unix/xptcstubs_x86_64_linux.cpp, line 79

Comment 3

[Jesse Ruderman]

Attachment: testcase 2 (crashes Firefox when loaded)

This variant crashes [@ nsIDOMElementCSSInlineStyle_GetStyle] on Linux. And on Windows, I think.

Comment 4

[Jesse Ruderman]

The initial [@ nsIDOMElementCSSInlineStyle_GetStyle] crash report on Windows looked exploitable, with "Crash address: 0xfffffffffdfdfe0d".

Comment 5

[Johnny Stenback (:jst)]

Bobby, can you dig in here?

Comment 6

[Bobby Holley (:bholley)]

On my linux VM, I get a crash with the builtin browser (FF4), but not on a recent nightly. When I run a debug nightly, boom() throws NS_ERROR_XPC_BAD_CONVERT_JS, which presumably means that we've started catching this, whatever it is.

Jesse, can you confirm that this isn't reproducible on trunk? The next step is probably finding a fix window to figure out if the fix is easily backportable.

Comment 7

[Jesse Ruderman]

I still hit both crashes on Windows.

Comment 8

[Bobby Holley (:bholley)]

(In reply to Jesse Ruderman from comment #7)
> I still hit both crashes on Windows.

Ok. My current windows VM is kind of borked at the moment, so I'll spin up another one and give it a try.

Comment 9

[Bobby Holley (:bholley)]

Ok, so this shouldn't actually be going through xptcall at all. We appear to be getting vtable corruption that's sending us an xpt stub method (184, if you were curious), which calls into PrepareAndDispatch. PrepareAndDispatch crashes, because the arguments aren't where it expects.

Presumably, this is quite exploitable. I'm going to try to figure out where the memory corruption is happening.

Comment 10

[Bobby Holley (:bholley)]

Ignore comment 9 - I was wrong. Here's what's happening:

We make a callback object, pass it into the DOM, and take it out again. This gives us a double-wrapped object. That's the only important part - the rest of the treewalker stuff is irrelevant.

Next, we apply a quickstubbed DOM getter for nsGenericHTMLElement::GetId (this is done by setting __proto__ to a new div element, but could also be done with a simple call/apply). This lands us in the generated function nsIDOMHTMLElement_GetId().

Soon enough, we end up in the this-unwrapping code. The common path goes through getWrapper. In the old world, this would examine the parent of the function and invoke the method on the div. After my patches in bug 622301, we just throw. Either way, no problems there.

However, nsINode is one of those special castable interfaces, which takes us through a different path: castNativeFromWrapper. This method does some checks to make sure the object is a wrapped native, and then casts the object class to XPCNativeScriptableSharedJSClass, which is just a JSClass plus an interface bitmap.

Therein lies the rub. While most JSClasses in XPConnect have this interface bitmap, the NoHelper variety doesn't. And this is precisely what we get in the double-wrapped case. So we check a garbage bit, and if we're unlucky, the check passes and we proceed.

So we call a virtual method on the object, thinking it's GetId when it's in fact an xptcall stub method funneling into PrepareAndDispatch. The arguments are junk, so we crash in PrepareAndDispatch.

This is probably exploitable, and I think it's straightforward to fix. I'll look at it again first thing in the morning and see what the best approach is.

Comment 11

[Bobby Holley (:bholley)]

Attachment: Use the helper jsclass format everywhere. v1

Added a patch for this, flagging mrbkap for review. I took some liberties with the indentation on the POD declaration to keep the patch smaller.

This fixes the crash on the first testcase on windows, which is the only one I could reliably reproduce. The testcase isn't guaranteed to cause a crash though, because it depends on the value of garbage memory. It's also a pretty revealing testcase, so I'd be in favor of not landing it (at least for the moment).

This patch will probably need to be backported onto just about everything. We can probably slim the patch down a little bit by eliminating the rename.

Comment 12

[Bobby Holley (:bholley)]

Pushed this to try: https://tbpl.mozilla.org/?tree=Try&rev=65f7b041c326

Comment 13

[Bobby Holley (:bholley)]

Pushed this to mozilla-inbound:

http://hg.mozilla.org/integration/mozilla-inbound/rev/2e93b93bd8a8

We'll want this on the branches as well. Does that mean this bug should stay open after the merge to m-c?

Comment 14

[Ed Morley [:emorley]]

(In reply to Bobby Holley (:bholley) from comment #13)
> We'll want this on the branches as well. Does that mean this bug should stay
> open after the merge to m-c?

Seems to vary from person to person for s-g bugs, I'll leave this open for now in case.

https://hg.mozilla.org/mozilla-central/rev/2e93b93bd8a8

Comment 15

[Bobby Holley (:bholley)]

Attachment: Use the helper jsclass format everywhere on aurora. v1

Adding an aurora patch. This skips the rename and is thus significantly slimmer.

Flagging mrbkap for review.

Comment 16

[Bobby Holley (:bholley)]

Comment on attachment 598316 [details] [diff] [review]
Use the helper jsclass format everywhere on aurora. v1

[Approval Request Comment]
Regression caused by (bug #): bug 533637

User impact if declined: Potential remote code execution. 

Testing completed (on m-c, etc.): Baked on m-c. A reliable testcase isn't possible, since the issue at hand involves reading garbage memory.

Risk to taking this patch (and alternatives if risky): Low. We're essentially just reserving 4 bytes more of space for one static data structure, and zero-ing those extra bytes. This means that, if someone happens to confuse this data structure with one that uses the slightly-expanded format, it won't read garbage bits. If the bits are all zero, we'll always throw in the problem case. 
 
String changes made by this patch: None

Comment 17

[Alex Keybl [:akeybl]]

Comment on attachment 598316 [details] [diff] [review]
Use the helper jsclass format everywhere on aurora. v1

[Triage Comment]
Low risk, sg:crit fix. Approved for Aurora 12.

Comment 18

[Bobby Holley (:bholley)]

Landed to m-a:

http://hg.mozilla.org/releases/mozilla-aurora/rev/77a5d4052be1

Comment 19

[Bobby Holley (:bholley)]

Attachment: Use the helper jsclass format everywhere on beta.

[Approval Request Comment]

Same patch on beta, carrying over review. See comment 16 for risk assessment.

Flagging for beta approval.

Comment 20

[Alex Keybl [:akeybl]]

Comment on attachment 599407 [details] [diff] [review]
Use the helper jsclass format everywhere on beta.

[Triage Comment]
Approved for Beta 11 given the sg:crit rating and low risk assessment.

Comment 21

[Bobby Holley (:bholley)]

pushed to m-b: http://hg.mozilla.org/releases/mozilla-beta/rev/09063407efb3

dveditz, do we want this on ESR?

Comment 22

[Lukas Blakk [:lsblakk] use ?needinfo]

[Triage comment]

This bug is being tracked for landing on ESR branch.  Please land patches on http://hg.mozilla.org/releases/mozilla-esr10/by Thursday March 1, 2012 in order to be ready for go-to-build on Friday March 2, 2012.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more information.

Comment 23

[Bobby Holley (:bholley)]

Pushed this to mozilla-esr10:

http://hg.mozilla.org/releases/mozilla-esr10/rev/49a14bdb34be

Marking fixed on esr10, and resolving fixed entirely, since this patch has now landed on every branch ever.

Comment 24

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

Neither testcase crashes 3.6.27 on XP SP3. Marking as unaffected.

Comment 25

[u279076]

Neither testcase crashes in Firefox 10.0.3 ESR -- marking verified.

Comment 26

[u279076]

Verified fixed in Firefox 11.0b6

Comment 27

[Paul Silaghi, QA [:pauly]]

Neither testcase crashes in FF 13b4 and FF 2012-05-18-mozilla-beta-debug on Win 7, Ubuntu 12.04 and Mac OS X 10.6. Marking verified.