Last Comment Bug 718202 - Crash @GetCollectionForType with __proto__
: Crash @GetCollectionForType with __proto__
: crash, testcase
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
-- critical (vote)
: ---
Assigned To: Ben Turner (not reading bugmail, use the needinfo flag!)
: Andrew Overholt [:overholt]
Depends on:
  Show dependency treegraph
Reported: 2012-01-14 09:50 PST by Nils
Modified: 2012-05-07 14:23 PDT (History)
9 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Patch, v1 (6.19 KB, patch)
2012-01-17 16:44 PST, Ben Turner (not reading bugmail, use the needinfo flag!)
jst: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
lukasblakk+bugs: approval‑mozilla‑esr10+
Details | Diff | Splinter Review

Description User image Nils 2012-01-14 09:50:32 PST
This crash seems to be related to the use of __proto__, the Worker object and event calls on the object. It's marked as a security issue, as it crashes on accessing non-mapped memory addresses which vary and could potentially be controlled by an attacker.

Tested and Affected Versions:
	Firefox 9.0.1 (Windows)
        Firefox trunk 12.0a (Linux)
        Ubuntu Firefox 9.0.1

The following testcase trigger the bug and shows exploitable behaviour on Linux. Null deref on windows.

o2=new ArrayBuffer(1337);
o4=new Worker(null);
o2.__proto__ = o4.__proto__;

This testcase shows potentially exploitable behaviour on both Windows and Linux:

o4=new Worker(null);
o2.__proto__ = o4.__proto__;

Stack Backtrace:
Following stack backtraces have been observed during testing.


(gdb) bt 10
#0  GetCollectionForType (aTypeId=140736937735104, aHead=0x7fffc31563e8)
    at /build/buildd/firefox-9.0.1+build1/build-tree/mozilla/dom/workers/ListenerManager.cpp:132
#1  mozilla::dom::workers::events::ListenerManager::Add (this=0x7fffc31563e8, aCx=0x7fffc72d0800, aType=0x7fffdf2e33c0, 
    aListenerVal=..., aPhase=mozilla::dom::workers::events::ListenerManager::Bubbling, aWantsUntrusted=false)
    at /build/buildd/firefox-9.0.1+build1/build-tree/mozilla/dom/workers/ListenerManager.cpp:204
#2  0x00007ffff40b295a in AddEventListener (aWantsUntrusted=<optimized out>, aCapturing=<optimized out>, 
    aListener=<optimized out>, aType=<optimized out>, aCx=0x7fffc72d0800, this=0x7fffc31563e8)
    at /build/buildd/firefox-9.0.1+build1/build-tree/mozilla/dom/workers/ListenerManager.h:98
#3  mozilla::dom::workers::events::EventTarget::AddEventListener (aCx=0x7fffc72d0800, aArgc=3, aVp=0x7fffdf9fe148)
    at /build/buildd/firefox-9.0.1+build1/build-tree/mozilla/dom/workers/EventTarget.cpp:162
#4  0x00007ffff485d12e in CallJSNative (args=..., native=<optimized out>, cx=0x7fffc72d0800)
    at /build/buildd/firefox-9.0.1+build1/build-tree/mozilla/js/src/jscntxtinlines.h:296
#5  js::InvokeKernel (cx=0x7fffc72d0800, argsRef=<optimized out>, construct=js::NO_CONSTRUCT)
    at /build/buildd/firefox-9.0.1+build1/build-tree/mozilla/js/src/jsinterp.cpp:660
#6  0x00007ffff4853e02 in js::Interpret (cx=0x7fffc72d0800, entryFrame=0x7fffdf9fe0d0, interpMode=js::JSINTERP_NORMAL)
    at /build/buildd/firefox-9.0.1+build1/build-tree/mozilla/js/src/jsinterp.cpp:4036
#7  0x00007ffff485cedf in js::ExecuteKernel (cx=0x7fffc72d0800, script=0x7fffdf774ac0, scopeChain=<optimized out>, 
    thisv=<optimized out>, type=<optimized out>, evalInFrame=<optimized out>, result=0x7fffdf9fe0a8)
    at /build/buildd/firefox-9.0.1+build1/build-tree/mozilla/js/src/jsinterp.cpp:814
#8  0x00007ffff486f30d in EvalKernel (cx=0x7fffc72d0800, call=..., evalType=DIRECT_EVAL, caller=0x7fffdf9fe030, 
    scopeobj=...) at /build/buildd/firefox-9.0.1+build1/build-tree/mozilla/js/src/jsobj.cpp:1283
#9  0x00007ffff4854993 in js::Interpret (cx=0x7fffc72d0800, entryFrame=0x7fffdf9fe030, interpMode=js::JSINTERP_NORMAL)
    at /build/buildd/firefox-9.0.1+build1/build-tree/mozilla/js/src/jsinterp.cpp:4005
(More stack frames follow...)


xul!`anonymous namespace'::GetCollectionForType+0x8
xul!nsRunnableMethodImpl<void (__thiscall mozilla::dom::indexedDB::LazyIdleThread::*)(void),0>::Run+0x13

It crashed on following instruction:

xul!`anonymous namespace'::GetCollectionForType:
70e532b0 8b4c2404        mov     ecx,dword ptr [esp+4]
70e532b4 8b01            mov     eax,dword ptr [ecx]
70e532b6 eb0b            jmp     xul!`anonymous namespace'::GetCollectionForType+0x13 (70e532c3)
70e532b8 8b5008          mov     edx,dword ptr [eax+8] ds:002b:24748b5e=????????
70e532bb 3b542408        cmp     edx,dword ptr [esp+8]
70e532bf 7408            je      xul!`anonymous namespace'::GetCollectionForType+0x19 (70e532c9)
70e532c1 8b00            mov     eax,dword ptr [eax]
70e532c3 3bc1            cmp     eax,ecx
70e532c5 75f1            jne     xul!`anonymous namespace'::GetCollectionForType+0x8 (70e532b8)
70e532c7 33c0            xor     eax,eax
70e532c9 c3              ret

reported by nils of vulndev ltd.
Comment 1 User image Daniel Veditz [:dveditz] 2012-01-16 10:01:14 PST
Second testcase looks bad on Mac, too

This is probably more a Workers problem than JS Engine
Comment 2 User image Ben Turner (not reading bugmail, use the needinfo flag!) 2012-01-17 16:44:46 PST
Created attachment 589351 [details] [diff] [review]
Patch, v1

This is a little ugly but I don't think there's a nicer way at the moment. The new auto-generated worker bindings should make this prettier.
Comment 3 User image Ben Turner (not reading bugmail, use the needinfo flag!) 2012-01-18 10:11:59 PST
Comment 4 User image Ben Turner (not reading bugmail, use the needinfo flag!) 2012-01-19 08:10:21 PST
Comment 5 User image Ben Turner (not reading bugmail, use the needinfo flag!) 2012-01-19 08:13:23 PST
Comment on attachment 589351 [details] [diff] [review]
Patch, v1

[Approval Request Comment]
Regression caused by (bug #): Bug 649537
User impact if declined: Sometimes crashing, sometimes something that could be exploitable.
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): None. This simply throws an exception rather than derefing an unknown pointer. The code changes are very simple.
Comment 6 User image Alex Keybl [:akeybl] 2012-01-19 16:27:34 PST
We're going to hold this in the beta approval queue to see how it fares on m-c.
Comment 7 User image Alex Keybl [:akeybl] 2012-01-22 18:49:28 PST
Comment on attachment 589351 [details] [diff] [review]
Patch, v1

[Triage Comment]
Based upon the risk evaluation above, the fact that we haven't heard of any regressions on m-c, and the security team's recommendation, please land on Aurora 11 and Beta 10 tomorrow before end-of-day PT.
Comment 9 User image Daniel Veditz [:dveditz] 2012-03-02 11:31:58 PST
In Firefox 10.0.2 on Mac entering the first testcase on the web console results in
"TypeError: EventTarget.prototype.AddEventListener called on incompatible Proxy"

The second example crashes with bp-a3ed6257-f3ba-4290-b8eb-5e6622120302
If the crash is pointing at the right line (and optimization hasn't moved things around) looks like "collection" is or contains garbage in GetCollectionForType().

-> Firefox 10 not fixed.

In Firefox 11 (beta whatever rev/8c9e4873d419 is) I get the same results for the first testcase, and the second gives me "TypeError: EventTarget.prototype.RemoveEventListener called on incompatible HTMLBodyElement" in the web console. Ditto Firefox 12/Aurora

-> Firefox 11 fix verified
-> Firefox 12 fix verified
Comment 10 User image Lukas Blakk [:lsblakk] use ?needinfo 2012-03-02 14:22:34 PST
Comment on attachment 589351 [details] [diff] [review]
Patch, v1

Please land this immediately on the mozilla-esr10 branch so we may go to build today with this fix in.  Let me or Alex Keybl know if there are any issues or concerns with cleanly landing this patch or if you need someone else to do it.
Comment 11 User image Lukas Blakk [:lsblakk] use ?needinfo 2012-03-02 14:30:19 PST
This is already in esr10, sorry for the bug spam - just had to move the status-firefox10 flag to 'fixed'.
Comment 12 User image Al Billings [:abillings] 2012-03-02 15:12:55 PST
On Win 7 SP1 32-bit, first testcase results in error:

TypeError: EventTarget.prototype.AddEventListener called on incompatible Proxy

Second testcase results in crash: bp-9d513a4b-32b6-4f48-ba29-42c052120302
Comment 13 User image Al Billings [:abillings] 2012-03-02 15:13:46 PST
This is in 10.0.2. Sorry.
Comment 14 User image Daniel Veditz [:dveditz] 2012-03-02 15:21:18 PST
(In reply to Lukas Blakk [:lsblakk] from comment #11)
> This is already in esr10, sorry for the bug spam - just had to move the
> status-firefox10 flag to 'fixed'.

It was not mis-marked. It was marked fixed when the patch landed but not verified, and I changed it back based on my investigation in comment 9 (see also confirmation in comment 12).

To reduce confusion Ben has asked for a separate ESR10 branch bug. Since one of the two testcases was fixed in 10/esr10 I guess we can leave it marked that way.
Comment 15 User image Daniel Veditz [:dveditz] 2012-03-02 15:25:08 PST
Ben identified a buggy merge, should be easy to fix and is clearly related rather than a separate bug. We should keep it here.
Comment 16 User image Daniel Veditz [:dveditz] 2012-03-02 15:37:02 PST

Should be fixed for real, now
Comment 17 User image Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-03-05 13:53:50 PST
Verified fixed in Firefox 10.0.3esr.
Comment 18 User image Al Billings [:abillings] 2012-05-07 14:23:59 PDT
With Firefox 9 on Windows 7, the first testcase gives this crash, bp-e6e95b94-8d14-4da4-a65a-484212120507

The second gives bp-81d9d33f-d016-45e7-90a3-f27302120507

In Firefox 12, first testcase gives:

Error: EventTarget.prototype.AddEventListener called on incompatible ArrayBuffer

and no crash.

Second testcase gives: 

Error: EventTarget.prototype.RemoveEventListener called on incompatible HTMLBodyElement

and no crash. 

Marking this as verified fixed.

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