1018524 - (CVE-2014-1563) Heap-use-after-free in mozilla::DOMSVGLength::GetTearOff

Status: VERIFIED FIXED

(Core :: SVG, defect)

Description

[Abhishek Arya]

Attachment: Testcase

Need fuzzPriv extension. https://www.squarefree.com/extensions/domFuzzLite3.xpi

==6521==ERROR: AddressSanitizer: heap-use-after-free on address 0x60700026a968 at pc 0x7f779d09e97a bp 0x7fff11d30870 sp 0x7fff11d30868
READ of size 8 at 0x60700026a968 thread T0
    #0 0x7f779d09e979 in mozilla::DOMSVGLength::GetTearOff(nsSVGLength2*, nsSVGElement*, bool) objdir-ff-asan/content/svg/content/src/../../../../dist/include/nsISupportsImpl.h:51
    #1 0x7f779d1e232f in nsSVGLength2::ToDOMAnimVal(mozilla::DOMSVGLength**, nsSVGElement*) content/svg/content/src/nsSVGLength2.cpp:336
    #2 0x7f779d108850 in mozilla::dom::SVGAnimatedLength::AnimVal() content/svg/content/src/SVGAnimatedLength.cpp:37
    #3 0x7f779b446989 in mozilla::dom::SVGAnimatedLengthBinding::get_animVal(JSContext*, JS::Handle<JSObject*>, mozilla::dom::SVGAnimatedLength*, JSJitGetterCallArgs) objdir-ff-asan/dom/bindings/./SVGAnimatedLengthBinding.cpp:48
    #4 0x7f779b95bdfc in mozilla::dom::GenericBindingGetter(JSContext*, unsigned int, JS::Value*) dom/bindings/BindingUtils.cpp:2280
    #5 0x7f77a06bd843 in js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) js/src/jscntxtinlines.h:239
    #6 0x7f77a0659b1e 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
    #7 0x7f77a06bf5fd in js::InvokeGetterOrSetter(JSContext*, JSObject*, JS::Value, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) js/src/vm/Interpreter.cpp:603
    #8 0x7f77a04799ff in js::baseops::GetProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>) js/src/vm/Shape-inl.h:46
    #9 0x7f77a06b0e19 in Interpret(JSContext*, js::RunState&) js/src/jsobj.h:984
    #10 0x7f77a069615c in js::RunScript(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:422
    #11 0x7f77a06bdd91 in js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) js/src/vm/Interpreter.cpp:494
    #12 0x7f77a0659b1e 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 0x7f77a034bdb5 in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) js/src/jsapi.cpp:5214
    #14 0x7f779ad89131 in mozilla::dom::EventHandlerNonNull::Call(JSContext*, JS::Handle<JS::Value>, mozilla::dom::Event&, mozilla::ErrorResult&) objdir-ff-asan/dom/bindings/./EventHandlerBinding.cpp:35
    #15 0x7f779c2a96ef in JS::Value mozilla::dom::EventHandlerNonNull::Call<nsISupports*>(nsISupports* const&, mozilla::dom::Event&, mozilla::ErrorResult&, mozilla::dom::CallbackObject::ExceptionHandling) objdir-ff-asan/dom/events/../../dist/include/mozilla/dom/EventHandlerBinding.h:62
    #16 0x7f779c2a7c0f in mozilla::JSEventHandler::HandleEvent(nsIDOMEvent*) dom/events/JSEventHandler.cpp:214
    #17 0x7f779c2767d0 in mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, nsIDOMEvent*, mozilla::dom::EventTarget*) dom/events/EventListenerManager.cpp:950
    #18 0x7f779c277b40 in mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent**, mozilla::dom::EventTarget*, nsEventStatus*) dom/events/EventListenerManager.cpp:1011
    #19 0x7f779c268a01 in mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) dom/events/EventDispatcher.cpp:287
    #20 0x7f779c26cb26 in mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsCOMArray<mozilla::dom::EventTarget>*) dom/events/EventDispatcher.cpp:597
    #21 0x7f779d7ad394 in nsDocumentViewer::LoadComplete(tag_nsresult) layout/base/nsDocumentViewer.cpp:1004
    #22 0x7f779e42970b in nsDocShell::EndPageLoad(nsIWebProgress*, nsIChannel*, tag_nsresult) docshell/base/nsDocShell.cpp:6963
    #23 0x7f779e426899 in nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, tag_nsresult) docshell/base/nsDocShell.cpp:6762
    #24 0x7f779e426cff in non-virtual thunk to nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, tag_nsresult) docshell/base/nsDocShell.cpp:6769
    #25 0x7f779a2d0abb in nsDocLoader::DoFireOnStateChange(nsIWebProgress*, nsIRequest*, int&, tag_nsresult) uriloader/base/nsDocLoader.cpp:1268
    #26 0x7f779a2cfe02 in nsDocLoader::doStopDocumentLoad(nsIRequest*, tag_nsresult) uriloader/base/nsDocLoader.cpp:850
    #27 0x7f779a2cd9a3 in nsDocLoader::DocLoaderIsEmpty(bool) uriloader/base/nsDocLoader.cpp:740
    #28 0x7f779a2cf202 in nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, tag_nsresult) uriloader/base/nsDocLoader.cpp:624
    #29 0x7f779a2cf8ac in non-virtual thunk to nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, tag_nsresult) objdir-ff-asan/uriloader/base/Unified_cpp_uriloader_base0.cpp:628
    #30 0x7f77991dda38 in nsLoadGroup::RemoveRequest(nsIRequest*, nsISupports*, tag_nsresult) netwerk/base/src/nsLoadGroup.cpp:689
    #31 0x7f779c85397c in nsDocument::DoUnblockOnload() content/base/src/nsDocument.cpp:8706
    #32 0x7f779c85362a in nsDocument::UnblockOnload(bool) content/base/src/nsDocument.cpp:8634
    #33 0x7f779c8288c8 in nsDocument::DispatchContentLoadedEvents() content/base/src/nsDocument.cpp:4917
    #34 0x7f779c878da0 in nsRunnableMethodImpl<void (nsDocument::*)(), void, true>::Run() objdir-ff-asan/content/base/src/../../../dist/include/nsThreadUtils.h:387
    #35 0x7f77990765b5 in nsThread::ProcessNextEvent(bool, bool*) xpcom/threads/nsThread.cpp:766
    #36 0x7f7798f3622a in NS_ProcessNextEvent(nsIThread*, bool) xpcom/glue/nsThreadUtils.cpp:263
    #37 0x7f77998704a9 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) ipc/glue/MessagePump.cpp:95
    #38 0x7f7799818920 in MessageLoop::Run() ipc/chromium/src/base/message_loop.cc:229
    #39 0x7f779bb979d7 in nsBaseAppShell::Run() widget/xpwidgets/nsBaseAppShell.cpp:164
    #40 0x7f779ea75628 in nsAppStartup::Run() toolkit/components/startup/nsAppStartup.cpp:278
    #41 0x7f779e8ef0d3 in XREMain::XRE_mainRun() toolkit/xre/nsAppRunner.cpp:4012
    #42 0x7f779e8effaf in XREMain::XRE_main(int, char**, nsXREAppData const*) toolkit/xre/nsAppRunner.cpp:4081
    #43 0x7f779e8f0dfd in XRE_main toolkit/xre/nsAppRunner.cpp:4293
    #44 0x48c757 in main browser/app/nsBrowserApp.cpp:282
    #45 0x7f77a79d1de4 in __libc_start_main /build/buildd/eglibc-2.17/csu/libc-start.c:260
0x60700026a968 is located 40 bytes inside of 80-byte region [0x60700026a940,0x60700026a990)
freed by thread T0 here:
    #0 0x473bf1 in __interceptor_free _asan_rtl_
    #1 0x7f7798f87d1a in SnowWhiteKiller::~SnowWhiteKiller() xpcom/base/nsCycleCollector.cpp:2611
    #2 0x7f7798f87929 in nsCycleCollector::FreeSnowWhite(bool) xpcom/base/nsCycleCollector.cpp:2778
    #3 0x7f7798f8d67f in nsCycleCollector::BeginCollection(ccType, nsICycleCollectorListener*) xpcom/base/nsCycleCollector.cpp:3705
    #4 0x7f7798f8cdb6 in nsCycleCollector::Collect(ccType, js::SliceBudget&, nsICycleCollectorListener*) xpcom/base/nsCycleCollector.cpp:3546
    #5 0x7f7798f90d0c in nsCycleCollector_collect(nsICycleCollectorListener*) xpcom/base/nsCycleCollector.cpp:4127
    #6 0x7f779c049263 in nsJSContext::CycleCollectNow(nsICycleCollectorListener*, int) dom/base/nsJSEnvironment.cpp:1943
    #7 0x7f779c04895f in nsJSEnvironmentObserver::Observe(nsISupports*, char const*, char16_t const*) dom/base/nsJSEnvironment.cpp:283
    #8 0x7f7798fd7329 in nsObserverService::NotifyObservers(nsISupports*, char const*, char16_t const*) xpcom/ds/nsObserverList.cpp:96
    #9 0x7f779908b331 in NS_InvokeByIndex xpcom/reflect/xptcall/src/md/unix/xptcinvoke_x86_64_unix.cpp:162
    #10 0x7f779bd882c7 in XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) js/xpconnect/src/XPCWrappedNative.cpp:2395
    #11 0x7f779bd8e0ae in XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1273
    #12 0x7f77a06bd843 in js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) js/src/jscntxtinlines.h:239
    #13 0x7f77a06b1774 in Interpret(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:2581
    #14 0x7f77a069615c in js::RunScript(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:422
    #15 0x7f77a06bdd91 in js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) js/src/vm/Interpreter.cpp:494
    #16 0x7f77a03ca040 in js::CallOrConstructBoundFunction(JSContext*, unsigned int, JS::Value*) js/src/jsfun.cpp:1401
    #17 0x7f77a06bd843 in js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) js/src/jscntxtinlines.h:239
    #18 0x7f77a0659b1e 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
    #19 0x7f77a05ba765 in js::CrossCompartmentWrapper::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) js/src/jsproxy.cpp:448
    #20 0x7f77a0538195 in js::Proxy::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) js/src/jsproxy.cpp:2663
    #21 0x7f77a053e354 in js::proxy_Call(JSContext*, unsigned int, JS::Value*) js/src/jsproxy.cpp:3066
    #22 0x7f77a06bd843 in js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) js/src/jscntxtinlines.h:239
    #23 0x7f77a06b1774 in Interpret(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:2581
    #24 0x7f77a069615c in js::RunScript(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:422
    #25 0x7f77a06bdd91 in js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) js/src/vm/Interpreter.cpp:494
    #26 0x7f77a0659b1e 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
    #27 0x7f77a034bdb5 in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) js/src/jsapi.cpp:5214
    #28 0x7f779ad89131 in mozilla::dom::EventHandlerNonNull::Call(JSContext*, JS::Handle<JS::Value>, mozilla::dom::Event&, mozilla::ErrorResult&) objdir-ff-asan/dom/bindings/./EventHandlerBinding.cpp:35
    #29 0x7f779c2a96ef in JS::Value mozilla::dom::EventHandlerNonNull::Call<nsISupports*>(nsISupports* const&, mozilla::dom::Event&, mozilla::ErrorResult&, mozilla::dom::CallbackObject::ExceptionHandling) objdir-ff-asan/dom/events/../../dist/include/mozilla/dom/EventHandlerBinding.h:62

previously allocated by thread T0 here:
    #0 0x473df1 in malloc _asan_rtl_
    #1 0x7f77a3bc6a4d in moz_xmalloc memory/mozalloc/mozalloc.cpp:52
    #2 0x7f779d09e654 in mozilla::DOMSVGLength::GetTearOff(nsSVGLength2*, nsSVGElement*, bool) objdir-ff-asan/content/svg/content/src/../../../../dist/include/mozilla/mozalloc.h:201
    #3 0x7f779d1e232f in nsSVGLength2::ToDOMAnimVal(mozilla::DOMSVGLength**, nsSVGElement*) content/svg/content/src/nsSVGLength2.cpp:336
    #4 0x7f779d108850 in mozilla::dom::SVGAnimatedLength::AnimVal() content/svg/content/src/SVGAnimatedLength.cpp:37
    #5 0x7f779b446989 in mozilla::dom::SVGAnimatedLengthBinding::get_animVal(JSContext*, JS::Handle<JSObject*>, mozilla::dom::SVGAnimatedLength*, JSJitGetterCallArgs) objdir-ff-asan/dom/bindings/./SVGAnimatedLengthBinding.cpp:48
    #6 0x7f779b95bdfc in mozilla::dom::GenericBindingGetter(JSContext*, unsigned int, JS::Value*) dom/bindings/BindingUtils.cpp:2280
    #7 0x7f77a06bd843 in js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) js/src/jscntxtinlines.h:239
    #8 0x7f77a0659b1e 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
    #9 0x7f77a06bf5fd in js::InvokeGetterOrSetter(JSContext*, JSObject*, JS::Value, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) js/src/vm/Interpreter.cpp:603
    #10 0x7f77a04799ff in js::baseops::GetProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>) js/src/vm/Shape-inl.h:46
    #11 0x7f77a06b0e19 in Interpret(JSContext*, js::RunState&) js/src/jsobj.h:984
    #12 0x7f77a069615c in js::RunScript(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:422
    #13 0x7f77a06bdd91 in js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) js/src/vm/Interpreter.cpp:494
    #14 0x7f77a0659b1e 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
    #15 0x7f77a034bdb5 in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) js/src/jsapi.cpp:5214
    #16 0x7f779ad89131 in mozilla::dom::EventHandlerNonNull::Call(JSContext*, JS::Handle<JS::Value>, mozilla::dom::Event&, mozilla::ErrorResult&) objdir-ff-asan/dom/bindings/./EventHandlerBinding.cpp:35
    #17 0x7f779c2a96ef in JS::Value mozilla::dom::EventHandlerNonNull::Call<nsISupports*>(nsISupports* const&, mozilla::dom::Event&, mozilla::ErrorResult&, mozilla::dom::CallbackObject::ExceptionHandling) objdir-ff-asan/dom/events/../../dist/include/mozilla/dom/EventHandlerBinding.h:62
    #18 0x7f779c2a7c0f in mozilla::JSEventHandler::HandleEvent(nsIDOMEvent*) dom/events/JSEventHandler.cpp:214
    #19 0x7f779c2767d0 in mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, nsIDOMEvent*, mozilla::dom::EventTarget*) dom/events/EventListenerManager.cpp:950
    #20 0x7f779c277b40 in mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent**, mozilla::dom::EventTarget*, nsEventStatus*) dom/events/EventListenerManager.cpp:1011
    #21 0x7f779c268a01 in mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) dom/events/EventDispatcher.cpp:287
    #22 0x7f779c26cb26 in mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsCOMArray<mozilla::dom::EventTarget>*) dom/events/EventDispatcher.cpp:597
    #23 0x7f779d7ad394 in nsDocumentViewer::LoadComplete(tag_nsresult) layout/base/nsDocumentViewer.cpp:1004
    #24 0x7f779e42970b in nsDocShell::EndPageLoad(nsIWebProgress*, nsIChannel*, tag_nsresult) docshell/base/nsDocShell.cpp:6963
    #25 0x7f779e426899 in nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, tag_nsresult) docshell/base/nsDocShell.cpp:6762
    #26 0x7f779e426cff in non-virtual thunk to nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, tag_nsresult) docshell/base/nsDocShell.cpp:6769
    #27 0x7f779a2d0abb in nsDocLoader::DoFireOnStateChange(nsIWebProgress*, nsIRequest*, int&, tag_nsresult) uriloader/base/nsDocLoader.cpp:1268
    #28 0x7f779a2cfe02 in nsDocLoader::doStopDocumentLoad(nsIRequest*, tag_nsresult) uriloader/base/nsDocLoader.cpp:850
    #29 0x7f779a2cd9a3 in nsDocLoader::DocLoaderIsEmpty(bool) uriloader/base/nsDocLoader.cpp:740

SUMMARY: AddressSanitizer: heap-use-after-free ??:0 ??
Shadow bytes around the buggy address:
  0x0c0e800454d0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c0e800454e0: fa fa 00 00 00 00 00 00 00 00 00 00 fa fa fa fa
  0x0c0e800454f0: fd fd fd fd fd fd fd fd fd fd fa fa fa fa 00 00
  0x0c0e80045500: 00 00 00 00 00 00 00 00 fa fa fa fa fd fd fd fd
  0x0c0e80045510: fd fd fd fd fd fd fa fa fa fa fd fd fd fd fd fd
=>0x0c0e80045520: fd fd fd fd fa fa fa fa fd fd fd fd fd[fd]fd fd
  0x0c0e80045530: fd fd fa fa fa fa 00 00 00 00 00 00 00 00 00 fa
  0x0c0e80045540: fa fa fa fa 00 00 00 00 00 00 00 00 00 00 fa fa
  0x0c0e80045550: fa fa 00 00 00 00 00 00 00 00 00 00 fa fa fa fa
  0x0c0e80045560: fd fd fd fd fd fd fd fd fd fd fa fa fa fa 00 00
  0x0c0e80045570: 00 00 00 00 00 00 00 00 fa fa fa fa 00 00 00 00
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
==6521==ABORTING

Comment 1

[Daniel Holbert [:dholbert]]

I loaded this 4 times in a normal debug build (no ASAN). On 3/4 of the runs, I  this fatal assertion:
>Assertion failure: int32_t(mRefCnt) >= 0 (illegal refcnt), at content/svg/content/src/DOMSVGLength.cpp:53

And in the other run, I hit this MOZ_CRASH instead:
>Hit MOZ_CRASH(DOMSVGLength not thread-safe) at content/svg/content/src/DOMSVGLength.cpp:53


Both of those are part of the NS_IMPL_CYCLE_COLLECTING_ADDREF(DOMSVGLength) macro (that's what's at line 53 in that .cpp file). And in both cases, this is inside of an addref in the GetTearoff() call that's shown in comment 0's backtrace.

(I suspect the thread-safety crash is a red herring -- it probably just indicates that we're dealing with a deleted object whose "mOwningThread" member is bogus.)

Comment 2

[Andrew McCreight [:mccr8]]

Is this something you can investigate Daniel or should I look for somebody else?  Thanks.

Comment 3

[Daniel Holbert [:dholbert]]

Sorry for going quiet -- I did poke around a bit after comment 1, and saw the DOMSVGLength's refcount going to 0, but wasn't sure where exactly we should still be owning a reference.  I started setting up a 32-bit linux VM to run rr so that I could replay the execution to see what the earlier owners were of that object, but I got stalled on setting up that VM, & since then I've been working on other P2-goal-oriented bugs [and had a week of planned vacation].

jwatt knows SVG DOM tearoff lifetime-management better than I do, so if he has cycles to take this, he'd probably be able to get to the bottom of it quicker than I can. jwatt, any chance you can take a look?

(If not, I can circle back to it soonish.)

Comment 4

[Jonathan Watt [:jwatt]]

I'm guessing this was caused by Ehsan's patch for bug 886416. I've not dug into this yet though.

Comment 5

[Robert Longson [:longsonr]]

bug 886416 may have also caused bug 1033301. It's possible that looking at that bug's testcase may provide additional clues as to how to fix this.

Comment 6

[Jonathan Watt [:jwatt]]

Ehsan, bz, I've not managed to get to this yet and now I'm about to be away for two weeks. Could you guys take a look and see what went wrong in the patch for bug 886416 (I'm assuming it's to blame since it seems very likely - I haven't confirmed that though).

Comment 7

[(no longer active)]

I'll take a look.

Comment 8

[(no longer active)]

The problem is that after we put the DOMSVGLength object into the tear-off table, DOMSVGLength::RemovingFromList ends up being called, which changes mIsAnimValItem to be false, which causes the code in the destructor which removes the tear-off entry to look at the wrong table, so the anim value tear-off table keeps holding on to the freed reference.

Comment 9

[(no longer active)]

I think this is pretty exploitable!

Comment 10

[(no longer active)]

Attachment: Patch (v1)

Comment 11

[Boris Zbarsky [:bzbarsky]]

So I think the real bug here is that the list code allows changing the readonlyness of the thing being inserted.  Consider:

    var oSVGAltGlyphElement = document.createElementNS("http://www.w3.org/2000/svg",
                                                       "altGlyph");
    var oSvgMaskElement = document.createElementNS("http://www.w3.org/2000/svg", "mask");
    var oSvgLengthList = oSVGAltGlyphElement.dy.baseVal;
    var anim = oSvgMaskElement.width.animVal;
    alert(anim.value);
    try { anim.value = 42; } catch (e) { alert(e); }
    alert(anim.value);
    oSvgLengthList.appendItem(anim);
    try { anim.value = 42; } catch (e) { alert(e); }
    alert(anim.value);

this alerts some value other than 42, then an exception message, then the same other number, then 42.  So just appending the animated value to a base value list suddenly made it no longer readonly!

We should either not allow such appends or Copy() on such appends like we do if the arg has an owner.

That said, shouldn't an SVGAnimatedLength count as an owner too?  That is, why does oSvgMaskElement.width.animVal not have an owner?

Comment 12

[Boris Zbarsky [:bzbarsky]]

Cameron, any feedback on what this should do in spec terms would be really useful.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8454670 [details] [diff] [review]
Patch (v1)

I don't think this is the right thing to do, though making mIsAnimValItem const _is_ a good idea.  I don't think it should ever change.

Comment 14

[Cameron McCormack (:heycam)]

(In reply to Boris Zbarsky [:bz] from comment #11)
> We should either not allow such appends or Copy() on such appends like we do
> if the arg has an owner.

I haven't noticed that copying behaviour before.  The spec says that the SVGLength will be removed from its original list before being inserted into the new one.  Is the copying something that other implementations do, and we should update the spec?

I don't think we should allow an SVGLength that came from reflecting a non-list attribute to be inserted into any SVGLengthList.  The spec doesn't say anything about this case currently, so I think a literal reading would not allow 42 to be assigned to the SVGLength, as the object is in an animVal list somewhere.  If implementations agree on the copying behaviour, then this should copy here too.  I think I prefer copying to throwing, anyway.

Comment 15

[Boris Zbarsky [:bzbarsky]]

> Is the copying something that other implementations do, and we should update the spec?

Iirc there are open spec and/or Gecko issues for figuring out what to do there.

> I don't think we should allow an SVGLength that came from reflecting a non-list attribute
> to be inserted into any SVGLengthList.

Even a baseval into a baseval list?

If so, we should probably just treat being a baseval/animval (not in a list) as having an owner for now...  But it might be easier to just allow inserting basevals into baseval lists for the time being.

I'll try to write up a patch here on Monday, I guess, if no one else gets to it before then.

Comment 16

[Cameron McCormack (:heycam)]

(In reply to Boris Zbarsky [:bz] from comment #15)
> Even a baseval into a baseval list?

Yes I don't think we should try to make having an SVGLength reflect two separate baseVals (one a single value, say rectElement.x.baseVal, and one an item of a list, say textElement.x.baseVal[0]) work.

> If so, we should probably just treat being a baseval/animval (not in a list)
> as having an owner for now...  But it might be easier to just allow
> inserting basevals into baseval lists for the time being.

Are we sure there will be no issues from having that single SVGLength object reflecting more than one thing from the DOM?

Comment 17

[Robert Longson [:longsonr]]

See bug 515116 comment 25 and the response bug 515116 comment 45. I'll paste the relevant parts so you don't need to wade through the irrelevant bits

> Does this handle removing the item from its previous list? Not sure if I see
> where that happens. Are there tests for removal from previous list?

Deliberately, no. After several conversations going over the arguments for and against inserting vs copying with roc and other implementers, we concluded that a good compromise to meet user expectations while minimizing unexpected behavior is to insert the object itself only if it doesn't belong to a list, and to insert a copy of it if it does. That way anyone using createSVGLength() gets what they expect, but anyone trying to copy lengths from an existing list doesn't have the existing list mutating from under them.

In reply to comment 11. I would expect that oSvgMaskElement.width.animVal should have an owner

Note that we do want svgElement.createLength() (see http://www.w3.org/TR/SVG/struct.html#__svg__SVGSVGElement__createSVGLength) to be able to be inserted into a list.

Comment 18

[Boris Zbarsky [:bzbarsky]]

> I would expect that oSvgMaskElement.width.animVal should have an owner

That would help things here, yes.  DOMSVGLength has:

124   /**
125    * In future, if this class is used for non-list lengths, this will be
126    * different to IsInList().
127    */
128   bool HasOwner() const {
129     return !!mList;
130   }

Clearly we're using this for non-list lengths.  Should this return mList || mVal, perhaps?

Are there other things like this, which can appear both in lists and as anim/base values?  My initial skim of the SVG code isn't finding any, but I don't really know this stuff that well.

Comment 19

[Robert Longson [:longsonr]]

(In reply to Boris Zbarsky [:bz] from comment #18)
> > I would expect that oSvgMaskElement.width.animVal should have an owner
> 
> That would help things here, yes.  DOMSVGLength has:

I've looked at this more and I've changed my mind. The whole code assumes that owner == list owner.

I think the way to go is somehow to disallow assigning/ inserting with animvals. They are meant to be read only so

 1    var oSVGAltGlyphElement = document.createElementNS("http://www.w3.org/2000/svg",
 2                                                       "altGlyph");
 3    var oSvgMaskElement = document.createElementNS("http://www.w3.org/2000/svg", "mask");
 4    var oSvgLengthList = oSVGAltGlyphElement.dy.baseVal;
 5    var anim = oSvgMaskElement.width.animVal;
 6    alert(anim.value);
 7    try { anim.value = 42; } catch (e) { alert(e); }
 8    alert(anim.value);
 9    oSvgLengthList.appendItem(anim);
10    try { anim.value = 42; } catch (e) { alert(e); }
11    alert(anim.value);

We should make this throw at line 9.

The SVG spec says of this method...

a DOMException with code NO_MODIFICATION_ALLOWED_ERR is raised when the list corresponds to a read only attribute or when the object itself is read only.

So that's what we should do on any insert of any animVal into a list as the object itself is read only in this case and that's what we're not checking for but should. So the spec is clear and we're not following it.

Same for initialize, insertItemBefore, replaceItem.

We currently call IsAnimValList() and throw if that's true which is the first half of the check but the other half is missing so...


already_AddRefed<SVGTransform>
DOMSVGTransformList::Initialize(SVGTransform& newItem, ErrorResult& error)
{
  if (IsAnimValList() || newItem.IsAnimVal()) {
    error.Throw(NS_ERROR_DOM_NO_MODIFICATION_ALLOWED_ERR);
    return nullptr;
  }
... 

etc for all methods that take a list item in all list types except StringList.

Comment 20

[Robert Longson [:longsonr]]

Attachment: like so

I've not tested this, but it does compile.

Comment 21

[Cameron McCormack (:heycam)]

> (In reply to Boris Zbarsky [:bz] from comment #18)
> > > I would expect that oSvgMaskElement.width.animVal should have an owner
> > 
> > That would help things here, yes.  DOMSVGLength has:
> 
> I've looked at this more and I've changed my mind. The whole code assumes
> that owner == list owner.
> 
> I think the way to go is somehow to disallow assigning/ inserting with
> animvals. They are meant to be read only so
> 
>  1    var oSVGAltGlyphElement =
> document.createElementNS("http://www.w3.org/2000/svg",
>  2                                                       "altGlyph");
>  3    var oSvgMaskElement =
> document.createElementNS("http://www.w3.org/2000/svg", "mask");
>  4    var oSvgLengthList = oSVGAltGlyphElement.dy.baseVal;
>  5    var anim = oSvgMaskElement.width.animVal;
>  6    alert(anim.value);
>  7    try { anim.value = 42; } catch (e) { alert(e); }
>  8    alert(anim.value);
>  9    oSvgLengthList.appendItem(anim);
> 10    try { anim.value = 42; } catch (e) { alert(e); }
> 11    alert(anim.value);
> 
> We should make this throw at line 9.
> 
> The SVG spec says of this method...
> 
> a DOMException with code NO_MODIFICATION_ALLOWED_ERR is raised when the list
> corresponds to a read only attribute or when the object itself is read only.

I am pretty sure that "when the object itself is read only" is talking about the DOM node, not the item passed in to it.  This was meant to handle the case of an SVG DOM element being a descendant of an Entity node, which are read only -- see http://www.w3.org/TR/DOM-Level-3-Core/core.html#ID-527DCFF2 -- but in practice nobody implemented DOM access to Entity objects.  The wording for the clear() method, which does not take an argument, also talks of "the object itself", so that suggests the initialize() method's mention of it is not talking about the argument.  Note that the SVG 2 draft removed the wording about "the object itself" being read only, since that case does not come up.

If we are copying an item when we try to insert it into a list and it's already in a modifiable list, why wouldn't we do the same when it is in an unmodifiable list?

Comment 22

[Cameron McCormack (:heycam)]

Sorry, I meant to say that the "when the list corresponds to a read only attribute" bit is talking about the read only DOM node, while the "object itself is read only" is talking about the list object.

Comment 23

[Robert Longson [:longsonr]]

We don't copy if it isn't in a list at all. That's to try to have a length created by createSVGLength which is then put in a list and the original object modified work.

Comment 24

[Cameron McCormack (:heycam)]

I guess my point is why should we have two different behaviours when the item we're attempting to insert already lives somewhere else? If it's helpful to the author to copy when the item lives in a modifiable list, why not when it's in an unmodifiable list?

Comment 25

[Robert Longson [:longsonr]]

Regarding comment 11.

Opera 12 displays 0, 42, 42
IE 9 displays 0, Exception, 0, Exception 0

Comment 26

[Robert Longson [:longsonr]]

(In reply to Cameron McCormack (:heycam) from comment #24)
> I guess my point is why should we have two different behaviours when the
> item we're attempting to insert already lives somewhere else? If it's
> helpful to the author to copy when the item lives in a modifiable list, why
> not when it's in an unmodifiable list?

Cam, can you discuss this with roc? Perhaps he can remember what he and jwatt discussed in bug 515116 in more detail?

Comment 27

[Robert Longson [:longsonr]]

roc do you have an opinion regarding copying vs moving when inserting into SVG lists. Comment 19 onwards pretty much.

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Cameron McCormack (:heycam) from comment #24)
> I guess my point is why should we have two different behaviours when the
> item we're attempting to insert already lives somewhere else? If it's
> helpful to the author to copy when the item lives in a modifiable list, why
> not when it's in an unmodifiable list?

I agree with this. We should just copy in this situation.

Comment 29

[(no longer active)]

I don't know much about what's going on here any more, so I'm not a good assignee.

Comment 30

[Cameron McCormack (:heycam)]

I brought this up in yesterday's SVG WG teleconference and we resolved on the copying behaviour.

Comment 31

[Cameron McCormack (:heycam)]

The tearoff tables in DOMSVGLength.cpp are only used for when we are reflecting a single length-typed attribute (so that's when mVal is not null).  So we shouldn't need to remove the DOMSVGLength from the tearoff table from RemovingFromList, as we'll never have an mVal-backed DOMSVGLength in an SVGLengthList -- at least, once we start copying.

Comment 32

[Cameron McCormack (:heycam)]

Attachment: patch v2

Comment 33

[Cameron McCormack (:heycam)]

I went with a separate IsReflectingAttribute method rather than adding the mVal check to HasOwner, since HasOwner is used in a bunch of places to really mean "are we in a list".

Comment 34

[Cameron McCormack (:heycam)]

Attachment: patch v2.1

Forgot to refresh my patch with some additional comments.

Comment 35

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8458389 [details] [diff] [review]
patch v2.1

Makes sense to me.

Comment 36

[Sylvestre Ledru [:Sylvestre]]

Too late for 31 but we will take a patch for ESR 31.

Comment 37

[Robert Longson [:longsonr]]

Comment on attachment 8458389 [details] [diff] [review]
patch v2.1

r=longsonr, but a test with an unowned length (something created by svg.createSVGLength() would be nice.

Comment 38

[Robert Longson [:longsonr]]

But... Don't you need to make the same changes in some of the other list types e.g. DOMSVGPoint, DOMSVGPathSeg

Comment 39

[Cameron McCormack (:heycam)]

(In reply to Robert Longson from comment #38)
> But... Don't you need to make the same changes in some of the other list
> types e.g. DOMSVGPoint, DOMSVGPathSeg

No, since these other list item types don't use tear-off tables and they are not used to reflect single attribute values.  The only other case I found where you can append an item that isn't unowned or in another list is bug 1040575.

Comment 40

[Cameron McCormack (:heycam)]

Attachment: patch v2.1 + additional test

Comment 41

[Cameron McCormack (:heycam)]

Comment on attachment 8459099 [details] [diff] [review]
patch v2.1 + additional test

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Not that easily, but SVG list code has traditionally been security bug prone, so noticing changes to this area might prompt someone to take a closer look.

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?

Firefox 31 onwards.

If not all supported branches, which bug introduced the flaw?

Bug 886416.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

Patch ought to apply to the older branches.

How likely is this patch to cause regressions; how much testing does it need?

Unlikely; patch adds regression test for the behaviour we want.

Comment 42

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

Comment on attachment 8459099 [details] [diff] [review]
patch v2.1 + additional test

Sec-approval+ for checkin on 8/5/14 or later.

Comment 43

[Andrew McCreight [:mccr8]]

This is okay to check in now.

Comment 44

[Cameron McCormack (:heycam)]

Should I land this on inbound, wait for it to bake a day or so, then land on branches (after I've requested approval-mozilla-blah) or should I get everything lined up to land all at once?  I ask because this is the first time I've been asked to wait for a particular date to land, and I'm not sure what that was for. :)

Comment 45

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think you're good to land on inbound now, and then request aurora/beta approval.

Comment 46

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/109af7e5adbf

Comment 47

[Cameron McCormack (:heycam)]

Comment on attachment 8459099 [details] [diff] [review]
patch v2.1 + additional test

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: potentially exploitable security bug remains?
Fix Landed on Version: 34
Risk to taking this patch (and alternatives if risky): low; the patch simplifies SVG list processing a little, and we have a regression test
String or UUID changes made by this patch: -

Comment 48

[Andrew McCreight [:mccr8]]

Yeah, you can just follow the regular landing process now, of putting it on inbound, filing for branch approval, etc.  Thanks!

Comment 49

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/109af7e5adbf

Comment 50

[Robert Longson [:longsonr]]

you should really land bug 1040575 now that this has landed.

Comment 51

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

Comment on attachment 8459099 [details] [diff] [review]
patch v2.1 + additional test

Approved for branches: aurora, beta, esr31.

Comment 52

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e7f718f619ba
https://hg.mozilla.org/releases/mozilla-beta/rev/3315c53c4bb7
https://hg.mozilla.org/releases/mozilla-esr31/rev/e56441aca60d

Comment 53

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/3315c53c4bb7

Comment 54

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

Changing tracking-firefox-esr31 (again) to reflect release policy of marking it for the release of Firefox mainline which is shipping the same time as this fix on ESR. 

Given how many times Release Management has gotten on me for making stuff "+" instead, it would be good if their own team didn't do it. :-)

Comment 55

[Daniel Veditz [:dveditz] (OOO: back Aug 26)]

Adding sec-bounty nomination per reporter's request

Comment 57

[Kamil Jozwiak [:kjozwiak]]

Reproduced the original issue using the following builds + fuzzPriv from comment #0:
- http://inbound-archive.pub.build.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux64-asan/1399310558/
- http://inbound-archive.pub.build.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-aurora-linux64-asan/1399322743/

Went through verification using the following builds:

fx 34.0a1: [PASSED]
- http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux64-asan/1408998354/

fx 33.0a2: [PASSED]
- http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-aurora-linux64-asan/1408992832/

fx 32: [PASSED]
- http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-beta-linux64-asan/1409002734/

Used the following test cases for each of the builds listed above:

- opened the poc code in 10 different tabs/windows
- opened the poc code in 10 different private window tabs/windows
- opened the poc code in 10 different e10s tabs/windows (m-c buil only)

Comment 58

[Kamil Jozwiak [:kjozwiak]]

fx 31.1.0esrpre: [PASSED]
- Built from: http://hg.mozilla.org/releases/mozilla-esr31/rev/d736074cabbf

Configure arguments:

--enable-address-sanitizer --disable-jemalloc --disable-crashreporter --disable-elf-hack --enable-debug-symbols --disable-install-strip --enable-optimize --enable-debug

Went through the test cases from comment #57 without any issues.