1005584 - (CVE-2014-1538) Heap-use-after-free in nsTextEditRules::CreateMozBR

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect)

Description

[Abhishek Arya]

Attachment: Testcase

Install fuzzPriv extension before running test.html

>==16549==ERROR: AddressSanitizer: heap-use-after-free on address 0x6190009a83a0 at pc 0x7f2757bcb74a bp 0x7fff5c52fb50 sp 0x7fff5c52fb48
>READ of size 8 at 0x6190009a83a0 thread T0
>    #0 0x7f2757bcb749 in nsTextEditRules::CreateMozBR(nsIDOMNode*, int, nsIDOMNode**) editor/libeditor/text/nsTextEditRules.cpp:1347
>    #1 0x7f2757c65f33 in nsHTMLEditRules::InsertMozBRIfNeeded(nsIDOMNode*) editor/libeditor/html/nsHTMLEditRules.cpp:8607
>    #2 0x7f2757c38455 in nsHTMLEditRules::DidDoAction(nsISelection*, nsRulesInfo*, tag_nsresult) editor/libeditor/html/nsHTMLEditRules.cpp:3648
>    #3 0x7f2757c9627c in nsHTMLEditor::InsertBasicBlock(nsAString_internal const&) editor/libeditor/html/nsHTMLEditor.cpp:2165
>    #4 0x7f2757c94b5f in nsHTMLEditor::SetParagraphFormat(nsAString_internal const&) editor/libeditor/html/nsHTMLEditor.cpp:1725
>    #5 0x7f2756434871 in nsParagraphStateCommand::SetState(nsIEditor*, nsString&) editor/composer/src/nsComposerCommands.cpp:654
>    #6 0x7f2756433d8e in nsMultiStateCommand::DoCommandParams(char const*, nsICommandParams*, nsISupports*) editor/composer/src/nsComposerCommands.cpp:599
>    #7 0x7f2758cebd3f in nsControllerCommandTable::DoCommandParams(char const*, nsICommandParams*, nsISupports*) embedding/components/commandhandler/src/nsControllerCommandTable.cpp:171
>    #8 0x7f2758ce20aa in non-virtual thunk to nsBaseCommandController::DoCommandWithParams(char const*, nsICommandParams*) embedding/components/commandhandler/src/nsBaseCommandController.cpp:152
>    #9 0x7f2758ce8026 in nsCommandManager::DoCommand(char const*, nsICommandParams*, nsIDOMWindow*) embedding/components/commandhandler/src/nsCommandManager.cpp:233
>    #10 0x7f275761d2f0 in nsHTMLDocument::ExecCommand(nsAString_internal const&, bool, nsAString_internal const&, mozilla::ErrorResult&) content/html/document/src/nsHTMLDocument.cpp:3242
>    #11 0x7f275574160f in mozilla::dom::HTMLDocumentBinding::execCommand(JSContext*, JS::Handle<JSObject*>, nsHTMLDocument*, JSJitMethodCallArgs const&) objdir-ff-asan/dom/bindings/./HTMLDocumentBinding.cpp:802
>    #12 0x7f27561a4ae7 in mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) dom/bindings/BindingUtils.cpp:2300
>    #13 0x7f275ae8fea0 in js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) js/src/jscntxtinlines.h:239
>    #14 0x7f275ae83cc8 in Interpret(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:2620
>    #15 0x7f275ae67599 in js::RunScript(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:422
>    #16 0x7f275ae92dcb in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::ExecuteType, js::AbstractFramePtr, JS::Value*) js/src/vm/Interpreter.cpp:630
>    #17 0x7f275ae9336e in js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) js/src/vm/Interpreter.cpp:666
>    #18 0x7f275aae699b in Evaluate(JSContext*, JS::Handle<JSObject*>, JS::ReadOnlyCompileOptions const&, JS::SourceBufferHolder&, JS::Value*) js/src/jsapi.cpp:4984
>    #19 0x7f275691e0d0 in nsJSUtils::EvaluateString(JSContext*, JS::SourceBufferHolder&, JS::Handle<JSObject*>, JS::CompileOptions&, nsJSUtils::EvaluateOptions const&, JS::MutableHandle<JS::Value>, void**) dom/base/nsJSUtils.cpp:254
>    #20 0x7f275691d6ea in nsJSUtils::EvaluateString(JSContext*, nsAString_internal const&, JS::Handle<JSObject*>, JS::CompileOptions&, nsJSUtils::EvaluateOptions const&, JS::MutableHandle<JS::Value>, void**) dom/base/nsJSUtils.cpp:186
>    #21 0x7f275691ed00 in nsJSUtils::EvaluateString(JSContext*, nsAString_internal const&, JS::Handle<JSObject*>, JS::CompileOptions&, void**) dom/base/nsJSUtils.cpp:306
>    #22 0x7f27567fccb5 in nsGlobalWindow::RunTimeoutHandler(nsTimeout*, nsIScriptContext*) dom/base/nsGlobalWindow.cpp:11936
>    #23 0x7f27567da69c in nsGlobalWindow::RunTimeout(nsTimeout*) dom/base/nsGlobalWindow.cpp:12166
>    #24 0x7f27567fbd91 in nsGlobalWindow::TimerCallback(nsITimer*, void*) dom/base/nsGlobalWindow.cpp:12412
>    #25 0x7f27539860d4 in nsTimerImpl::Fire() xpcom/threads/nsTimerImpl.cpp:555
>    #26 0x7f275398675e in nsTimerEvent::Run() xpcom/threads/nsTimerImpl.cpp:639
>    #27 0x7f275397d7e0 in nsThread::ProcessNextEvent(bool, bool*) xpcom/threads/nsThread.cpp:715
>    #28 0x7f275383db1a in NS_ProcessNextEvent(nsIThread*, bool) xpcom/glue/nsThreadUtils.cpp:263
>    #29 0x7f2754165f98 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) ipc/glue/MessagePump.cpp:136
>    #30 0x7f275410e5f0 in MessageLoop::Run() ipc/chromium/src/base/message_loop.cc:229
>    #31 0x7f27563e0007 in nsBaseAppShell::Run() widget/xpwidgets/nsBaseAppShell.cpp:164
>    #32 0x7f275925c5f8 in nsAppStartup::Run() toolkit/components/startup/nsAppStartup.cpp:278
>    #33 0x7f27590d81a3 in XREMain::XRE_mainRun() toolkit/xre/nsAppRunner.cpp:4019
>    #34 0x7f27590d9085 in XREMain::XRE_main(int, char**, nsXREAppData const*) toolkit/xre/nsAppRunner.cpp:4088
>    #35 0x7f27590d9edd in XRE_main toolkit/xre/nsAppRunner.cpp:4300
>    #36 0x48c6e7 in main browser/app/nsBrowserApp.cpp:282
>    #37 0x7f27621ae76c in __libc_start_main /build/buildd/eglibc-2.15/csu/libc-start.c:226
>    #38 0x48bbbc in _start
>
>0x6190009a83a0 is located 32 bytes inside of 1104-byte region [0x6190009a8380,0x6190009a87d0)
>freed by thread T0 here:
>    #0 0x473b81 in __interceptor_free _asan_rtl_
>    #1 0x7f275388f10a in SnowWhiteKiller::~SnowWhiteKiller() xpcom/base/nsCycleCollector.cpp:2403
>    #2 0x7f275388ed19 in nsCycleCollector::FreeSnowWhite(bool) xpcom/base/nsCycleCollector.cpp:2569
>    #3 0x7f2753894be1 in nsCycleCollector::BeginCollection(ccType, nsICycleCollectorListener*) xpcom/base/nsCycleCollector.cpp:3445
>    #4 0x7f27538942e6 in nsCycleCollector::Collect(ccType, js::SliceBudget&, nsICycleCollectorListener*) xpcom/base/nsCycleCollector.cpp:3307
>    #5 0x7f2753897ea3 in nsCycleCollector_collect(nsICycleCollectorListener*) xpcom/base/nsCycleCollector.cpp:3860
>    #6 0x7f275684c5a6 in nsJSContext::CycleCollectNow(nsICycleCollectorListener*, int) dom/base/nsJSEnvironment.cpp:1940
>    #7 0x7f2756756408 in nsDOMWindowUtils::CycleCollect(nsICycleCollectorListener*, int) dom/base/nsDOMWindowUtils.cpp:1485
>    #8 0x7f27539909e1 in NS_InvokeByIndex xpcom/reflect/xptcall/src/md/unix/xptcinvoke_x86_64_unix.cpp:162
>    #9 0x7f2756580977 in XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) js/xpconnect/src/XPCWrappedNative.cpp:2399
>    #10 0x7f27565865fe in XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1277
>    #11 0x7f275ae8fea0 in js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) js/src/jscntxtinlines.h:239
>    #12 0x7f275ae83cc8 in Interpret(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:2620
>    #13 0x7f275ae67599 in js::RunScript(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:422
>    #14 0x7f275ae903ee in js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) js/src/vm/Interpreter.cpp:494
>    #15 0x7f275ae9149e in js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value const*, JS::MutableHandle<JS::Value>) js/src/vm/Interpreter.cpp:531
>    #16 0x7f275ac74f24 in js::DirectProxyHandler::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) js/src/jsproxy.cpp:448
>    #17 0x7f275ad7a6f6 in js::CrossCompartmentWrapper::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) js/src/jswrapper.cpp:464
>    #18 0x7f275ac97da5 in js::Proxy::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) js/src/jsproxy.cpp:2659
>    #19 0x7f275ac9d2c4 in js::proxy_Call(JSContext*, unsigned int, JS::Value*) js/src/jsproxy.cpp:3062
>    #20 0x7f275ae8fea0 in js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) js/src/jscntxtinlines.h:239
>    #21 0x7f275ae83cc8 in Interpret(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:2620
>    #22 0x7f275ae67599 in js::RunScript(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:422
>    #23 0x7f275ae903ee in js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) js/src/vm/Interpreter.cpp:494
>    #24 0x7f275ae9149e in js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value const*, JS::MutableHandle<JS::Value>) js/src/vm/Interpreter.cpp:531
>    #25 0x7f275aae9aaf in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) js/src/jsapi.cpp:5196
>    #26 0x7f275561872a in mozilla::dom::EventListener::HandleEvent(JSContext*, JS::Handle<JS::Value>, mozilla::dom::Event&, mozilla::ErrorResult&) objdir-ff-asan/dom/bindings/./EventListenerBinding.cpp:43
>    #27 0x7f2756a46c8a in void mozilla::dom::EventListener::HandleEvent<mozilla::dom::EventTarget*>(mozilla::dom::EventTarget* const&, mozilla::dom::Event&, mozilla::ErrorResult&, mozilla::dom::CallbackObject::ExceptionHandling) objdir-ff-asan/dom/events/../../dist/include/mozilla/dom/EventListenerBinding.h:53
>    #28 0x7f2756a464e8 in mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, nsIDOMEvent*, mozilla::dom::EventTarget*) dom/events/EventListenerManager.cpp:946
>    #29 0x7f2756a478a0 in mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent**, mozilla::dom::EventTarget*, nsEventStatus*) dom/events/EventListenerManager.cpp:1011
>
>previously allocated by thread T0 here:
>    #0 0x473d81 in malloc _asan_rtl_
>    #1 0x7f275e324a4d in moz_xmalloc memory/mozalloc/mozalloc.cpp:52
>    #2 0x7f2757c8907d in nsHTMLEditor::InitRules() objdir-ff-asan/editor/libeditor/html/../../../dist/include/mozilla/mozalloc.h:201
>    #3 0x7f2757bcbf3a in nsAutoEditInitRulesTrigger::~nsAutoEditInitRulesTrigger() editor/libeditor/text/nsPlaintextEditor.cpp:208
>    #4 0x7f2757c86fdc in nsHTMLEditor::Init(nsIDOMDocument*, nsIContent*, nsISelectionController*, unsigned int, nsAString_internal const&) editor/libeditor/html/nsHTMLEditor.cpp:299
>    #5 0x7f275644be09 in nsEditingSession::SetupEditorOnWindow(nsIDOMWindow*) editor/composer/src/nsEditingSession.cpp:450
>    #6 0x7f275644904f in nsEditingSession::MakeWindowEditable(nsIDOMWindow*, char const*, bool, bool, bool) editor/composer/src/nsEditingSession.cpp:168
>    #7 0x7f27576074b9 in nsHTMLDocument::EditingStateChanged() content/html/document/src/nsHTMLDocument.cpp:2734
>    #8 0x7f275761baf5 in nsHTMLDocument::SetDesignMode(nsAString_internal const&, mozilla::ErrorResult&) content/html/document/src/nsHTMLDocument.cpp:2877
>    #9 0x7f275573b791 in mozilla::dom::HTMLDocumentBinding::set_designMode(JSContext*, JS::Handle<JSObject*>, nsHTMLDocument*, JSJitSetterCallArgs) objdir-ff-asan/dom/bindings/./HTMLDocumentBinding.cpp:734
>    #10 0x7f27561a43a5 in mozilla::dom::GenericBindingSetter(JSContext*, unsigned int, JS::Value*) dom/bindings/BindingUtils.cpp:2269
>    #11 0x7f275ae8fea0 in js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) js/src/jscntxtinlines.h:239
>    #12 0x7f275ae9149e in js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value const*, JS::MutableHandle<JS::Value>) js/src/vm/Interpreter.cpp:531
>    #13 0x7f275ae9283d in js::InvokeGetterOrSetter(JSContext*, JSObject*, JS::Value, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) js/src/vm/Interpreter.cpp:603
>    #14 0x7f275ac71cce in js::BaseProxyHandler::set(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>, JS::Handle<jsid>, bool, JS::MutableHandle<JS::Value>) js/src/jscntxtinlines.h:342
>    #15 0x7f27561a9c8b in mozilla::dom::DOMProxyHandler::set(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>, JS::Handle<jsid>, bool, JS::MutableHandle<JS::Value>) dom/bindings/DOMJSProxyHandler.cpp:228
>    #16 0x7f275ac96b49 in js::Proxy::set(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>, JS::Handle<jsid>, bool, JS::MutableHandle<JS::Value>) js/src/jsproxy.cpp:2572
>    #17 0x7f275ac0fa4e in JSObject::nonNativeSetProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>, bool) js/src/jsobj.cpp:1668
>    #18 0x7f275ae81125 in Interpret(JSContext*, js::RunState&) js/src/jsobj.h:1022
>    #19 0x7f275ae67599 in js::RunScript(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:422
>    #20 0x7f275ae903ee in js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) js/src/vm/Interpreter.cpp:494
>    #21 0x7f275ae9149e in js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value const*, JS::MutableHandle<JS::Value>) js/src/vm/Interpreter.cpp:531
>    #22 0x7f275aae9aaf in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) js/src/jsapi.cpp:5196
>    #23 0x7f275561872a in mozilla::dom::EventListener::HandleEvent(JSContext*, JS::Handle<JS::Value>, mozilla::dom::Event&, mozilla::ErrorResult&) objdir-ff-asan/dom/bindings/./EventListenerBinding.cpp:43
>    #24 0x7f2756a46c8a in void mozilla::dom::EventListener::HandleEvent<mozilla::dom::EventTarget*>(mozilla::dom::EventTarget* const&, mozilla::dom::Event&, mozilla::ErrorResult&, mozilla::dom::CallbackObject::ExceptionHandling) objdir-ff-asan/dom/events/../../dist/include/mozilla/dom/EventListenerBinding.h:53
>    #25 0x7f2756a464e8 in mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, nsIDOMEvent*, mozilla::dom::EventTarget*) dom/events/EventListenerManager.cpp:946
>    #26 0x7f2756a478a0 in mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent**, mozilla::dom::EventTarget*, nsEventStatus*) dom/events/EventListenerManager.cpp:1011
>    #27 0x7f2756a382b1 in mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) dom/events/EventDispatcher.cpp:287
>    #28 0x7f2756a3c3d6 in mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsCOMArray<mozilla::dom::EventTarget>*) dom/events/EventDispatcher.cpp:597
>    #29 0x7f2756a023f5 in mozilla::EventDispatcher::DispatchDOMEvent(nsISupports*, mozilla::WidgetEvent*, nsIDOMEvent*, nsPresContext*, nsEventStatus*) dom/events/EventDispatcher.cpp:661
>
>SUMMARY: AddressSanitizer: heap-use-after-free ??:0 ??
>Shadow bytes around the buggy address:
>  0x0c328012d020: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>  0x0c328012d030: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>  0x0c328012d040: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>  0x0c328012d050: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>  0x0c328012d060: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>=>0x0c328012d070: fd fd fd fd[fd]fd fd fd fd fd fd fd fd fd fd fd
>  0x0c328012d080: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
>  0x0c328012d090: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
>  0x0c328012d0a0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
>  0x0c328012d0b0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
>  0x0c328012d0c0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
>Shadow byte legend (one shadow byte represents 8 application bytes):
>  Addressable:           00
>  Partially addressable: 01 02 03 04 05 06 07
>  Heap left redzone:       fa
>  Heap right redzone:      fb
>  Freed heap region:       fd
>  Stack left redzone:      f1
>  Stack mid redzone:       f2
>  Stack right redzone:     f3
>  Stack partial redzone:   f4
>  Stack after return:      f5
>  Stack use after scope:   f8
>  Global redzone:          f9
>  Global init order:       f6
>  Poisoned by user:        f7
>  Contiguous container OOB:fc
>  ASan internal:           fe
>==16549==ABORTING
>
>

Comment 1

[Jeff Walden [:Waldo]]

smaug, looks like snow-white cycle collector funtimes.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

No. SnowWhite just deletes some object, and something else ends up using it after that.

http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/text/nsTextEditRules.h?rev=be6f50f0f583&mark=224-224#224

Comment 3

[(no longer active)]

Aryeh, can you take a look, please?

Comment 4

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

I don't have time for new bugs until August.  If it's still an issue then (hope not!), I'll look.

Comment 5

[David Bolter [:davidb] (NeedInfo me for attention)]

Ehsan, any other victim suggestions?

Comment 6

[(no longer active)]

Sigh, I guess I'll take this :(

Jesse, where do I get the fuzzPriv extension from?

Comment 7

[(no longer active)]

https://www.squarefree.com/extensions/domFuzzLite3.xpi

Comment 8

[(no longer active)]

We have a kungfuDeathGrip in nsHTMLEditor::InsertBasicBlock, but that's insufficient, since when we run the mutation observer in this test case, we end up clobbering mRules *twice*.  So at this point we have three rules objects around, the first being kept alive by the kungFuDeathGrip, the third being kept alive by the mRules reference, but the second object is not kept alive by anything and is collected by the CC during the mutation event.

And here's how this is triggered: we have a pattern like this: we have an object that might get destoryed and we're trying to keep it alive with a kungFuDeathGrip (like <http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/html/nsHTMLEditor.cpp#2087>).  We call one method on it first <http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/html/nsHTMLEditor.cpp#2099> which drops all refs to it except for the one coming from the kungFuDeathGrip and then we call a second method on it <http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/html/nsHTMLEditor.cpp#2146>, but the second method is now called on the second object (which will get destroyed in a sec) and we'll UAF when we unwind our stack back to the first method.

So, all of these kungFuDeathGrips in the editor/ code must be audited, but I'm not sure what other things may be affected by this.  CCing a bunch of other people for ideas there.

Comment 9

[(no longer active)]

So does anyone know if grep -i kungFuDeathGrip gives us a list of all of these objects?  Any other known naming patterns?

Comment 10

[(no longer active)]

Olli, mccr8 tells me that with your SnowWhiteKiller stuff, we only delete things off of the event loop.  In this test case, the object is killed like:

#0  nsTextEditRules::~nsTextEditRules (this=0x125765000) at nsTextEditRules.cpp:75
#1  0x00000001042ed228 in nsHTMLEditRules::~nsHTMLEditRules (this=0x125765000) at nsHTMLEditRules.cpp:215
#2  0x00000001042ed105 in nsHTMLEditRules::~nsHTMLEditRules (this=0x125765000) at nsHTMLEditRules.cpp:207
#3  0x00000001042ed0d9 in nsHTMLEditRules::~nsHTMLEditRules (this=0x125765000) at nsHTMLEditRules.cpp:207
#4  0x00000001042bf301 in nsTextEditRules::DeleteCycleCollectable (this=0x125765000) at nsTextEditRules.cpp:95
#5  0x00000001042c7cd2 in nsTextEditRules::cycleCollection::DeleteCycleCollectable (this=0x10908c478, p=0x125765000) at nsTextEditRules.h:45
#6  0x000000010160db1f in SnowWhiteKiller::~SnowWhiteKiller (this=0x7fff5fbeebe0) at nsCycleCollector.cpp:2403
#7  0x00000001015f4705 in SnowWhiteKiller::~SnowWhiteKiller (this=0x7fff5fbeebe0) at nsCycleCollector.cpp:2396
#8  0x00000001015dceed in nsCycleCollector::FreeSnowWhite (this=0x10066a000, aUntilNoSWInPurpleBuffer=true) at nsCycleCollector.cpp:2569
#9  0x00000001015dffa2 in nsCycleCollector::BeginCollection (this=0x10066a000, aCCType=ManualCC, aManualListener=0x0) at nsCycleCollector.cpp:3445
#10 0x00000001015dfa13 in nsCycleCollector::Collect (this=0x10066a000, aCCType=ManualCC, aBudget=@0x7fff5fbeee08, aManualListener=0x114daa670) at nsCycleCollector.cpp:3307
#11 0x00000001015e1ace in nsCycleCollector_collect (aManualListener=0x114daa670) at nsCycleCollector.cpp:3860
#12 0x00000001035fc12e in nsJSContext::CycleCollectNow (aListener=0x114daa670, aExtraForgetSkippableCalls=0) at /Users/ehsan/moz/src/dom/base/nsJSEnvironment.cpp:1940
#13 0x000000010358020f in nsDOMWindowUtils::CycleCollect (this=0x12be54520, aListener=0x114daa670, aExtraForgetSkippableCalls=0) at /Users/ehsan/moz/src/dom/base/nsDOMWindowUtils.c

Now, if SnowWhiteKiller is *only* ever called from the event loop, then this won't be exploitable, but do we protect against nested event loops from things like sync xhr or alert()?  And is this a guarantee or just a best effort?

Comment 11

[(no longer active)]

Here is a list of the instances of this bug in editor/:

=== methods accessing mRules directly ===
* nsHTMLEditor::AbsolutePositionSelection
* nsHTMLEditor::RelativeChangeZIndex
* nsHTMLEditor::LoadHTML
* nsHTMLEditor::DoInsertHTMLWithContext
* nsHTMLEditor::InsertElementAtSelection
* nsHTMLEditor::MakeOrChangeList
* nsHTMLEditor::RemoveList
* nsHTMLEditor::MakeDefinitionItem
* nsHTMLEditor::InsertBasicBlock
* nsHTMLEditor::Indent
* nsHTMLEditor::Align
* nsHTMLEditor::SetCSSBackgroundColor
* nsHTMLEditor::SetInlineProperty
* nsHTMLEditor::RemoveInlinePropertyImpl
* nsPlaintextEditor::DeleteSelection
* nsPlaintextEditor::InsertText
* nsPlaintextEditor::InsertLineBreak
* nsPlaintextEditor::Undo
* nsPlaintextEditor::Redo
* nsHTMLEditor::AbsolutelyPositionElement (possibly)
* nsHTMLEditor::PasteAsCitedQuotation (possibly... the fact that this function doesn't call DidDoAction is probably another bug!)
* nsHTMLEditor::InsertAsPlaintextQuotation (same as above)
* nsHTMLEditor::InsertAsCitedQuotation (same as above)
* nsPlaintextEditor::OutputToString (same as above)
* nsPlaintextEditor::InsertAsQuotation (same as above)

I also suspect all of the 50 (literally) callsites of nsAutoRules, because I can't convince myself that all of the things that get triggered from either BeforeEdit and AfterEdit are safe.  We should probably treat them as unsafe.

Now this brings us to talking about how we want to fix this.  Simply kungFuDeathGripping twice won't be an answer since WillDoAction/DidDoAction might be called on different rules objects, which is incorrect.  Ideally we'd stop this business of re-creating the rules object when the editor is reinitialized, but I don't know if that is going to be easy, and is going to be very regression prone.  And I can't think of another good way to protect against this object getting destroyed like this...  So right now I don't have a good solution in mind.

Comment 12

[(no longer active)]

Here's what really scares me about this bug.

Bug 848644 was our only pwn2own bug for 2013.  That bug has a full analysis of this kind of issue, and it even has a patch posted by Boris (see bug 848644 comment 84) which we moved to bug 848895 because we deemed it not safe enough.  The latter bug is still closed, but unfortunately bug 848644 is open.  I think going from the analysis there to finding this bug is not going to be too much work (and I'm really ashamed to not have done this sooner).  If someone has found the link (and I think we should assume that they have), this might be exploited in the wild...

My first urge is to close bug 848644 right now until we figure out what we're going to do with this bug.  Dan, is that OK?

Comment 13

[(no longer active)]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #12)
> If someone has found the link (and I think we should assume that
> they have), this might be exploited in the wild...

Ideas for proving/disproving this theory highly welcome.

Comment 14

[(no longer active)]

Jesse, please see comment 8 specifically.  We should modify our DOM fuzzer to do the "reset design mode" dance twice or more in mutation events.  Should I file a bug about that?

Comment 15

[Boris Zbarsky [:bzbarsky]]

The right general fix seems to me to be that if you feel you have to hold a kungFuDeathGrip, you should only call methods on the kungFuDeathGrip. :(

Comment 16

[(no longer active)]

(In reply to Boris Zbarsky [:bz] from comment #15)
> The right general fix seems to me to be that if you feel you have to hold a
> kungFuDeathGrip, you should only call methods on the kungFuDeathGrip. :(

But I'm not yet sure if that preserves correctness...

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #10)
> Now, if SnowWhiteKiller is *only* ever called from the event loop, then this
> won't be exploitable, but do we protect against nested event loops from
> things like sync xhr or alert()?  And is this a guarantee or just a best
> effort?

Based on the comment 0 we access deleted object after snowwhite has done its job.
So why wouldn't this be exploitable?
And Snowwhite can't protect against nested event loops.
(So it fixed only a subclass of this kinds of crashes.)

And yes, comment 15 sounds right.
So change nsCOMPtr<nsIEditRules> kungFuDeathGrip(mRules); to
nsCOMPtr<nsIEditRules> rules(mRules); and use that in the method?

Comment 18

[Andrew McCreight [:mccr8]]

> So why wouldn't this be exploitable?
Well, this particular test cases uses an extension to enable pages to manually force a CC via a JS call.  But as you say, it sounds like you could have a similar thing happen via a nested event loop.

Comment 19

[(no longer active)]

(In reply to Olli Pettay [:smaug] from comment #17)
> (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> #10)
> > Now, if SnowWhiteKiller is *only* ever called from the event loop, then this
> > won't be exploitable, but do we protect against nested event loops from
> > things like sync xhr or alert()?  And is this a guarantee or just a best
> > effort?
> 
> Based on the comment 0 we access deleted object after snowwhite has done its
> job.
> So why wouldn't this be exploitable?
> And Snowwhite can't protect against nested event loops.
> (So it fixed only a subclass of this kinds of crashes.)

Yeah, so like Andrew said, we should continue to assume that this is exploitable.

> And yes, comment 15 sounds right.
> So change nsCOMPtr<nsIEditRules> kungFuDeathGrip(mRules); to
> nsCOMPtr<nsIEditRules> rules(mRules); and use that in the method?

Well, that means that we'd end up calling DidDoAction on a rules object which will no longer be attached to the editor, and I'm not sure if that's correct, or safe.

I'm trying to see if we can just avoid this whole business of replacing the rules object.

Comment 20

[(no longer active)]

So I have a fix which I've sent to the try server for now, where instead of recreating the rules object when the editor gets reattached to a document, I just reset all of its fields, and leave mRules intact.  That way, the underlying issue in comment 8 goes away, and we can remove all of these kungFuDeathGrips for mRules.  I have yet to convince myself that this is quite the right fix...

https://tbpl.mozilla.org/?tree=Try&rev=e9d409d3c772

Comment 21

[Daniel Veditz [:dveditz]]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #12)
> My first urge is to close bug 848644 right now until we figure out what
> we're going to do with this bug.  Dan, is that OK?

Done. we can always reopen it.

Comment 22

[(no longer active)]

This does affect 29, even if we don't want to fix it there.

Comment 23

[(no longer active)]

So I chatted with dveditz on what to do with this bug.  tl;dr, I don't think we'll have a fix that I would feel comfortable with, so I'm going to file another bug, pretend that my patch wins some perf etc, and post the patch there, land it, wait for a day or two on trunk, and then backport it to aurora and beta to get some real testing there...

ehsan_
so
ehsan_
I assume you've seen https://bugzilla.mozilla.org/show_bug.cgi?id=1005584 ?
dveditz
yeah, I re-hid the 84* bug a little while ago and then got pinged before I could comment back in the original bug
dveditz
just did that
ehsan_
thanks
ehsan_
so I have a question here
ehsan_
well, before I ask that
dveditz
feel free to re-hide any bug you like -- we can always change our minds
dveditz
I mean, hide first, ask later
ehsan_
(thanks, didn't know what we usually do in these cases -- makes sense)
ehsan_
so
dveditz
if there's a problem that's better, if there isn't we just undo it
ehsan_
unfortunately I'm beginning to think that we cannot get a fix which I feel 100% comfortable with
dveditz
welcome to engineering :-)
ehsan_
which means that I'd probably want to land something on trunk, let it bake for some weeks, and then decide to backport
ehsan_
we have almost 0 test coverage on the interesting parts of this code :(
ehsan_
so it will have to be tested in the wild
dveditz
isn't it always "the best we can do, given the constraints"?
ehsan_
well
ehsan_
the issue here is that the cause of this bug (editor being reattached) doesn't happen "normally"
ehsan_
and I'm basically rewriting what happens in that case
ehsan_
and so far my local tests are happy
ehsan_
and try is happy
ehsan_
but I can't convince myself that my patch is correct :(
ehsan_
so my question is, how long can we stall backporting this bug, given the fact that it was discovered in pwn2own etc
dveditz
can we not reattach? "Sorry, you can only edit once"
ehsan_
and can we know for a fact whether this is being exploited in the wild somehow?
ehsan_
nope, unfortunately we can't do that
ehsan_
that behavior is controlled from content script
ehsan_
and there are good reasons why you would want to do that, for example to show an inline preview of a blog post or something
dveditz
sure.
ehsan_
so, how long can we stall backporting this?
dveditz
how long do you need? are we talking a couple weeks, or a version or two?
dveditz
if we think this is too risky we can skip beta (30) and wait until 31 (but then backport to the b2g30 branch at that time)
dveditz
if we're going to try for beta then we should land at the same time to get the most feedback, so we have time to back it out if we need to
dveditz
rather than land it in the last beta
ehsan_
hmm, it's hard to say how long we need to bake this for, ideally as long as possible, but that's obviously not a useful answer
ehsan_
so I have an idea
ehsan_
I can file a new bug, masquerade my patch as a perf improvement or something, and get it landed on trunk, wait a week or so, and backport it to aurora and beta if we don't find a regression to get more testing
ehsan_
and then by the time we need to make the last call on beta, if this has stuck we can backport to esr etc
dveditz
when's the next release? early June?
ehsan_
let me check
dveditz
yeah, june 10
ehsan_
yep
dveditz
so /maybe/ a week of m-c bake, no more if we're going to land on beta
ehsan_
so that way we may get to a decision by June 1st
ehsan_
yeah
dveditz
but really, I'd give it a couple days and then go. like, if you land tomorrow maybe try to land on beta on Tuesday
dveditz
not sure when they do beta builds. I think there are two a week
17:51 ehsan_
sure
ehsan_
if we don't find something disasterous on trunk, I don't expect to get a lot of useful testing there anyways
dveditz
right
dveditz
beta is where the people are
ehsan_
and we should know of a disaster in a day or so!
dveditz
nightly and aurora folks do mostly the same things we do -- boring :-)
ehsan_
indeed!
ehsan_
ok great, so can I put this log on the bug?
dveditz
beta starts to hit the wierd stuff
dveditz
yeah
ehsan_
awesome
ehsan_
thanks

Comment 24

[(no longer active)]

Bug 1007940 is the decoy bug.

Comment 25

[(no longer active)]

Attachment: placeholder for attachment 8419724 in bug 1007940

Comment 26

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8419727 [details]
placeholder for attachment 8419724 [details] [diff] [review] in bug 1007940

r=games

Comment 27

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

I'm happy to give this sec-approval+ as soon as we want it to go in.

Comment 28

[(no longer active)]

Comment on attachment 8419727 [details]
placeholder for attachment 8419724 [details] [diff] [review] in bug 1007940

[Security approval request comment]
How easily could an exploit be constructed based on the patch? If they know what they're looking for, fairly easily.  In order to mitigate this, I'm going to pretend to land this as a perf improvement in an open bug (bug 1007940) in order to get more testing.  See comment 23.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No.

Which older supported branches are affected by this flaw? All.

If not all supported branches, which bug introduced the flaw? Has been there for years if not decades.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?  Not yet, but the patch should be landable on branches as it is right now.

How likely is this patch to cause regressions; how much testing does it need? I have very little confidence in this patch unfortunately, which is why I want to land this as soon as possible.  Explained the logic in comment 23.

Comment 29

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

Comment on attachment 8419727 [details]
placeholder for attachment 8419724 [details] [diff] [review] in bug 1007940

sec-approval+.

Once it lands on trunk, we should nominate it for Aurora and Beta. I'm not sure when we'll want to land on ESR24 since, once we do, that's a big flag for "Probably important security bug here!"

Comment 30

[(no longer active)]

I'd like to land it on Aurora and Beta first for more testing.  Once we determine that the fix is good and doesn't regress anything, we should probably land it on ESR24/etc immediately before shipping dot releases.

Comment 31

[(no longer active)]

Landed on inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/1100671a94ba

Comment 32

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/mozilla-central/rev/1100671a94ba

Comment 33

[(no longer active)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d66cfabe38e5
https://hg.mozilla.org/releases/mozilla-beta/rev/d3ccc8465e4b

Comment 35

[Lawrence Mandel [:lmandel] (use needinfo)]

We're about at that time to land on ESR24 before shipping. Smoke test e-mail goes out tomorrow and all fixes should be in this week. 

Ehsan - Are you confident enough in the patch at this point to land on ESR24?

Comment 36

[(no longer active)]

So far this has baked on beta for about one week without any report for regressions, which makes me feel a bit more confident in this patch.  What is the last deadline for the ESR uplift?

Comment 37

[Lawrence Mandel [:lmandel] (use needinfo)]

Monday, June 2, if you want to leave it right to the last minute. If you want the patch included in the enterprise smoke test pass (I can't comment on how many enterprises actually exercise this option), the patch needs to land today.

Comment 38

[(no longer active)]

I don't know how effective those tests are, but thinking about this, I don't have a good reason to not land this, so I'll ask for approval.

Comment 39

[(no longer active)]

Comment on attachment 8419727 [details]
placeholder for attachment 8419724 [details] [diff] [review] in bug 1007940

Please see comment 23 and the previous approval requests for the landing plan here.

Comment 40

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8419727 [details]
placeholder for attachment 8419724 [details] [diff] [review] in bug 1007940

ESR 24 approval granted. Please land ASAP. We are unlikely to identify issues through usage as we have missed the smoke test window.

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr24/rev/1e2f9d32e218

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/517d4f4aac4f
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/f19a72c9d1c8

Comment 43

[(no longer active)]

Do we want to backport this anywhere else?

Comment 44

[(no longer active)]

Maybe Tarako?

Comment 45

[[:fabrice] Fabrice Desré]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #44)
> Maybe Tarako?

We will get that from the merge of mozilla-b2g28_v1_3 to mozilla-b2g28_v1_3t.

Comment 46

[(no longer active)]

Filed bug 1018486 in order to audit the rest of Gecko.

Comment 48

[Matt Wobensmith [:mwobensmith][:matt:]]

Confirmed crash on 2014-05-12, Fx30.
Verified fixed on 2014-06-03, ASan Fx24.6.0esr, Fx30, Fx31 and Fx32.

Comment 49

[Justin Wood (:Callek)]

fixed in SeaMonkey 2.26.1 with:

https://hg.mozilla.org/releases/mozilla-release/rev/384d0e944ba5