Last Comment Bug 714590 - "ASSERTION: no self" with treewalker filter, modified __proto__
: "ASSERTION: no self" with treewalker filter, modified __proto__
Status: VERIFIED FIXED
[sg:critical][qa!]
: assertion, crash, testcase, verified-beta
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: Trunk
: x86_64 Windows 7
: -- critical (vote)
: mozilla13
Assigned To: Bobby Holley (:bholley) (busy with Stylo)
:
Mentors:
Depends on:
Blocks: 326633
  Show dependency treegraph
 
Reported: 2012-01-02 01:05 PST by Jesse Ruderman
Modified: 2012-05-21 00:45 PDT (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
wontfix
+
wontfix
+
verified
+
verified
verified
11+
verified
unaffected


Attachments
testcase (crashes Firefox when loaded) (404 bytes, text/html)
2012-01-02 01:05 PST, Jesse Ruderman
no flags Details
testcase 2 (crashes Firefox when loaded) (407 bytes, text/html)
2012-01-02 12:12 PST, Jesse Ruderman
no flags Details
Use the helper jsclass format everywhere. v1 (6.36 KB, patch)
2012-02-15 11:15 PST, Bobby Holley (:bholley) (busy with Stylo)
mrbkap: review+
Details | Diff | Splinter Review
Use the helper jsclass format everywhere on aurora. v1 (4.05 KB, patch)
2012-02-17 11:51 PST, Bobby Holley (:bholley) (busy with Stylo)
mrbkap: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
Use the helper jsclass format everywhere on beta. (4.10 KB, patch)
2012-02-21 16:29 PST, Bobby Holley (:bholley) (busy with Stylo)
bobbyholley: review+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Jesse Ruderman 2012-01-02 01:05:49 PST
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
Comment 1 Jesse Ruderman 2012-01-02 01:09:03 PST
Happens on Windows but not on Mac??
Comment 2 Jesse Ruderman 2012-01-02 12:04:54 PST
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 2012-01-02 12:12:23 PST
Created attachment 585319 [details]
testcase 2 (crashes Firefox when loaded)

This variant crashes [@ nsIDOMElementCSSInlineStyle_GetStyle] on Linux. And on Windows, I think.
Comment 4 Jesse Ruderman 2012-01-02 12:14:00 PST
The initial [@ nsIDOMElementCSSInlineStyle_GetStyle] crash report on Windows looked exploitable, with "Crash address: 0xfffffffffdfdfe0d".
Comment 5 Johnny Stenback (:jst, jst@mozilla.com) 2012-01-05 13:13:44 PST
Bobby, can you dig in here?
Comment 6 Bobby Holley (:bholley) (busy with Stylo) 2012-01-20 15:38:31 PST
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 2012-01-21 06:22:24 PST
I still hit both crashes on Windows.
Comment 8 Bobby Holley (:bholley) (busy with Stylo) 2012-01-21 07:38:04 PST
(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) (busy with Stylo) 2012-02-14 18:16:51 PST
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) (busy with Stylo) 2012-02-14 22:05:16 PST
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) (busy with Stylo) 2012-02-15 11:15:32 PST
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.
Comment 12 Bobby Holley (:bholley) (busy with Stylo) 2012-02-16 08:58:52 PST
Pushed this to try: https://tbpl.mozilla.org/?tree=Try&rev=65f7b041c326
Comment 13 Bobby Holley (:bholley) (busy with Stylo) 2012-02-16 12:23:44 PST
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] 2012-02-17 05:20:28 PST
(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) (busy with Stylo) 2012-02-17 11:51:24 PST
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.
Comment 16 Bobby Holley (:bholley) (busy with Stylo) 2012-02-19 11:03:41 PST
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] 2012-02-21 11:46:43 PST
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) (busy with Stylo) 2012-02-21 11:58:59 PST
Landed to m-a:

http://hg.mozilla.org/releases/mozilla-aurora/rev/77a5d4052be1
Comment 19 Bobby Holley (:bholley) (busy with Stylo) 2012-02-21 16:29:15 PST
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.
Comment 20 Alex Keybl [:akeybl] 2012-02-23 15:15:11 PST
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) (busy with Stylo) 2012-02-23 15:35:07 PST
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 2012-02-23 16:46:30 PST
[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) (busy with Stylo) 2012-02-23 18:55:21 PST
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] 2012-03-01 17:33:41 PST
Neither testcase crashes 3.6.27 on XP SP3. Marking as unaffected.
Comment 25 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-03-05 09:45:21 PST
Neither testcase crashes in Firefox 10.0.3 ESR -- marking verified.
Comment 26 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-03-06 15:32:42 PST
Verified fixed in Firefox 11.0b6
Comment 27 Paul Silaghi, QA [:pauly] 2012-05-21 00:45:34 PDT
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.

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