Closed Bug 714590 Opened 8 years ago Closed 8 years ago
"ASSERTION: no self" with treewalker filter, modified __proto
404 bytes, text/html
407 bytes, text/html
6.36 KB, patch
|Details | Diff | Splinter Review|
4.05 KB, patch
|Details | Diff | Splinter Review|
4.10 KB, patch
|Details | Diff | Splinter Review|
###!!! ASSERTION: no self: 'self', file e:/builds/moz2_slave/m-cen-w32-dbg/build/xpcom/reflect/xptcall/src/md/win32/xptcstubs.cpp, line 68
Happens on Windows but not on Mac??
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
This variant crashes [@ nsIDOMElementCSSInlineStyle_GetStyle] on Linux. And on Windows, I think.
The initial [@ nsIDOMElementCSSInlineStyle_GetStyle] crash report on Windows looked exploitable, with "Crash address: 0xfffffffffdfdfe0d".
Bobby, can you dig in here?
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.
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.
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)
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
Adding an aurora patch. This skips the rename and is thus significantly slimmer. Flagging mrbkap for review.
Attachment #598316 - Flags: review?(mrbkap)
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+
[Approval Request Comment] Same patch on beta, carrying over review. See comment 16 for risk assessment. Flagging for beta approval.
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?
[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.
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.
Neither testcase crashes in Firefox 10.0.3 ESR -- marking verified.
Whiteboard: [sg:critical] → [sg:critical][qa+]
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.
You need to log in before you can comment on or make changes to this bug.