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

VERIFIED FIXED in Firefox 11

Status

()

Core
XPConnect
--
critical
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: Jesse Ruderman, Assigned: bholley)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
mozilla13
x86_64
Windows 7
assertion, crash, testcase, verified-beta
Points:
---

Firefox Tracking Flags

(firefox9- wontfix, firefox10+ wontfix, firefox11+ verified, firefox12+ verified, firefox13 verified, firefox-esr1011+ verified, status1.9.2 unaffected)

Details

([sg:critical][qa!])

Attachments

(5 attachments)

(Reporter)

Description

5 years ago
Created attachment 585258 [details]
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
(Reporter)

Comment 1

5 years ago
Happens on Windows but not on Mac??
(Reporter)

Comment 2

5 years ago
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
(Reporter)

Comment 3

5 years ago
Created attachment 585319 [details]
testcase 2 (crashes Firefox when loaded)

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

Comment 4

5 years ago
The initial [@ nsIDOMElementCSSInlineStyle_GetStyle] crash report on Windows looked exploitable, with "Crash address: 0xfffffffffdfdfe0d".
Group: core-security
(Reporter)

Updated

5 years ago
Whiteboard: [sg:critical]
Bobby, can you dig in here?
Assignee: nobody → bobbyholley+bmo
status-firefox10: --- → affected
status-firefox11: --- → affected
status-firefox12: --- → affected
status-firefox9: --- → wontfix
tracking-firefox10: --- → +
tracking-firefox11: --- → +
tracking-firefox12: --- → +
tracking-firefox9: --- → -
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.
(Reporter)

Comment 7

5 years ago
I still hit both crashes on Windows.
(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.
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.
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.
Created attachment 597492 [details] [diff] [review]
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.
Attachment #597492 - Flags: review?(mrbkap)

Updated

5 years ago
Attachment #597492 - Flags: review?(mrbkap) → review+
Pushed this to try: https://tbpl.mozilla.org/?tree=Try&rev=65f7b041c326
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?
Target Milestone: --- → mozilla13
(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
Status: NEW → ASSIGNED
Created attachment 598316 [details] [diff] [review]
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.
Attachment #598316 - Flags: review?(mrbkap)

Updated

5 years ago
Attachment #598316 - Flags: review?(mrbkap) → review+
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
Attachment #598316 - Flags: approval-mozilla-aurora?
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.
Attachment #598316 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Landed to m-a:

http://hg.mozilla.org/releases/mozilla-aurora/rev/77a5d4052be1
Created attachment 599407 [details] [diff] [review]
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.
Attachment #599407 - Flags: review+
Attachment #599407 - Flags: approval-mozilla-beta?
status-firefox-esr10: --- → affected
status-firefox10: affected → wontfix
status-firefox13: --- → affected
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.
Attachment #599407 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
pushed to m-b: http://hg.mozilla.org/releases/mozilla-beta/rev/09063407efb3

dveditz, do we want this on ESR?
status-firefox11: affected → fixed
status-firefox12: affected → fixed
status-firefox13: affected → fixed
[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.
tracking-firefox-esr10: --- → 11+
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
status-firefox-esr10: affected → fixed
Resolution: --- → FIXED
Neither testcase crashes 3.6.27 on XP SP3. Marking as unaffected.
status1.9.2: --- → unaffected
Neither testcase crashes in Firefox 10.0.3 ESR -- marking verified.
status-firefox-esr10: fixed → verified
Whiteboard: [sg:critical] → [sg:critical][qa+]
Verified fixed in Firefox 11.0b6
status-firefox11: fixed → verified
Keywords: verified-beta
Group: core-security

Updated

5 years ago
status-firefox12: fixed → verified
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.
Status: RESOLVED → VERIFIED
status-firefox13: fixed → verified
Whiteboard: [sg:critical][qa+] → [sg:critical][qa!]
You need to log in before you can comment on or make changes to this bug.