Crash @GetCollectionForType with __proto__

VERIFIED FIXED

Status

()

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

People

(Reporter: Nils, Assigned: Ben Turner (not reading bugmail, use the needinfo flag!))

Tracking

({crash, testcase})

Trunk
crash, testcase
Points:
---

Firefox Tracking Flags

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

Details

(Whiteboard: [sg:critical][qa!], crash signature)

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
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.
Second testcase looks bad on Mac, too
bp-f0529c55-9130-4bba-a097-f4ba12120116

This is probably more a Workers problem than JS Engine
Status: UNCONFIRMED → NEW
Component: Untriaged → DOM
Ever confirmed: true
Product: Firefox → Core
QA Contact: untriaged → general
Whiteboard: [sg:critical]
Crash Signature: [@ mozilla::dom::workers::events::ListenerManager::Remove ]
Keywords: crash, testcase

Updated

5 years ago
Assignee: nobody → bent.mozilla
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.
Attachment #589351 - Flags: review?(jst)

Updated

5 years ago
Attachment #589351 - Flags: review?(jst) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/be734df2dcab
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/be734df2dcab
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
status-firefox10: --- → affected
status-firefox11: --- → affected
status-firefox12: --- → fixed
status-firefox9: --- → affected
tracking-firefox10: --- → ?
tracking-firefox11: --- → ?
Resolution: --- → FIXED
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.
Attachment #589351 - Flags: approval-mozilla-beta?
Attachment #589351 - Flags: approval-mozilla-aurora?

Comment 6

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

Comment 7

5 years ago
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.
Attachment #589351 - Flags: approval-mozilla-beta?
Attachment #589351 - Flags: approval-mozilla-beta+
Attachment #589351 - Flags: approval-mozilla-aurora?
Attachment #589351 - Flags: approval-mozilla-aurora+

Updated

5 years ago
tracking-firefox10: ? → +
tracking-firefox11: ? → +
https://hg.mozilla.org/releases/mozilla-aurora/rev/e44cc659b759
https://hg.mozilla.org/releases/mozilla-beta/rev/9ce530c3cf52
status-firefox10: affected → fixed
status-firefox11: affected → fixed
Whiteboard: [sg:critical] → [sg:critical][qa+]
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
status1.9.2: --- → unaffected
status-firefox-esr10: --- → affected
status-firefox10: fixed → affected
status-firefox11: fixed → verified
status-firefox12: fixed → verified
tracking-firefox-esr10: --- → ?
tracking-firefox-esr10: ? → 11+
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.
Attachment #589351 - Flags: approval-mozilla-esr10+
This is already in esr10, sorry for the bug spam - just had to move the status-firefox10 flag to 'fixed'.
status-firefox-esr10: affected → fixed
status-firefox10: affected → fixed
tracking-firefox-esr10: 11+ → -
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
This is in 10.0.2. Sorry.
(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.
Ben identified a buggy merge, should be easy to fix and is clearly related rather than a separate bug. We should keep it here.
status-firefox-esr10: fixed → affected
status-firefox10: fixed → wontfix
tracking-firefox-esr10: - → 11+
https://hg.mozilla.org/releases/mozilla-esr10/rev/039e4d5bb2ce

Should be fixed for real, now
status-firefox-esr10: affected → fixed
Verified fixed in Firefox 10.0.3esr.
status-firefox-esr10: fixed → verified
Whiteboard: [sg:critical][qa+] → [sg:critical][qa!]
Group: core-security
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.