718202 - Crash @GetCollectionForType with __proto__

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Nils]

Description:
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

Testcase:
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__;
o2.addEventListener(0,0);

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

o2=document.body;
o4=new Worker(null);
o2.__proto__ = o4.__proto__;
o2.removeEventListener(0,0);


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

Linux:

(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...)
}}}


Windows:

xul!`anonymous namespace'::GetCollectionForType+0x8
xul!mozilla::dom::workers::events::ListenerManager::Remove+0x23
xul!mozilla::dom::workers::events::ListenerManager::RemoveEventListener+0x2f
xul!mozilla::dom::workers::events::EventTarget::RemoveEventListener+0x74
mozjs!js::InvokeKernel+0xcf
mozjs!js::Interpret+0x583e
mozjs!js::RunScript+0x98
mozjs!js::ExecuteKernel+0x169
mozjs!EvalKernel+0x2a9
mozjs!js::DirectEval+0x6b
mozjs!js::Interpret+0x5783
mozjs!js::RunScript+0x98
mozjs!js::ExecuteKernel+0x169
mozjs!js::Execute+0x9e
mozjs!EvaluateUCScriptForPrincipalsCommon+0x84
mozjs!JS_EvaluateUCScriptForPrincipalsVersion+0x55
xul!nsJSContext::EvaluateString+0x1ea
xul!nsJSThunk::EvaluateScript+0x64b
xul!nsJSChannel::EvaluateScript+0x2c
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

[Daniel Veditz [:dveditz]]

Second testcase looks bad on Mac, too
bp-f0529c55-9130-4bba-a097-f4ba12120116

This is probably more a Workers problem than JS Engine

Comment 2

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: 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

[Ben Turner (not reading bugmail, use the needinfo flag!)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/be734df2dcab

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

https://hg.mozilla.org/mozilla-central/rev/be734df2dcab

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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

[Alex Keybl [:akeybl]]

We're going to hold this in the beta approval queue to see how it fares on m-c.

Comment 7

[Alex Keybl [:akeybl]]

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 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e44cc659b759
https://hg.mozilla.org/releases/mozilla-beta/rev/9ce530c3cf52

Comment 9

[Daniel Veditz [:dveditz]]

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

[Lukas Blakk [:lsblakk] use ?needinfo]

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

[Lukas Blakk [:lsblakk] use ?needinfo]

This is already in esr10, sorry for the bug spam - just had to move the status-firefox10 flag to 'fixed'.

Comment 12

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

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

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

This is in 10.0.2. Sorry.

Comment 14

[Daniel Veditz [:dveditz]]

(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

[Daniel Veditz [:dveditz]]

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

[Daniel Veditz [:dveditz]]

https://hg.mozilla.org/releases/mozilla-esr10/rev/039e4d5bb2ce

Should be fixed for real, now

Comment 17

[u279076]

Verified fixed in Firefox 10.0.3esr.

Comment 18

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

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.