Closed Bug 1276688 Opened 3 years ago Closed 3 years ago

heap-buffer-overflow in BuildSegmentsFromValueEntries

Categories

(Core :: DOM: Animation, defect)

49 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox47 --- unaffected
firefox48 + fixed
firefox49 + fixed
firefox-esr45 --- unaffected
firefox50 + fixed

People

(Reporter: nils, Assigned: birtles)

References

Details

(Keywords: csectype-bounds, regression, sec-high)

Attachments

(3 files, 5 obsolete files)

The following testcase crashes the latest ASAN build of Firefox (buildId 20160523171639).

Testcase:

<script>
function start() {
        o0=window.document;
        o1=window.document.documentElement;
        o32=document.createElementNS('http://www.w3.org/1999/xhtml','video');
        o1.animate([{right: 'auto',strokeDasharray: 'none',rubyPosition: 'inter-character',stopOpacity: '1'},{right: '19px',strokeDasharray: 'none',rubyPosition: 'inter-character',stopOpacity: '13'}],1);
        o32.innerHTML="<svg><desc>";
        o39=o32.querySelectorAll('*')[0];
        o59=o32.querySelectorAll('*')[1];
        document.documentElement.appendChild(o39);
        o59.animate([{dominantBaseline: 'use-script !important',all: 'initial'},{dominantBaseline: 'mathematical',all: 'unset'}],1);
}
</script>
<body onload="start()"></body>


ASAN crash:

=================================================================
==2854==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x62d001246400 at pc 0x7f71fd965b72 bp 0x7ffcfce7ec90 sp 0x7ffcfce7ec88
READ of size 4 at 0x62d001246400 thread T0 (Web Content)
    #0 0x7f71fd965b71 in BuildSegmentsFromValueEntries /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/animation/KeyframeUtils.cpp:959
    #1 0x7f71fd965b71 in mozilla::KeyframeUtils::GetAnimationPropertiesFromKeyframes(nsStyleContext*, mozilla::dom::Element*, mozilla::CSSPseudoElementType, nsTArray<mozilla::Keyframe> const&) /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/animation/KeyframeUtils.cpp:553
    #2 0x7f71fd95336b in mozilla::dom::KeyframeEffectReadOnly::UpdateProperties(nsStyleContext*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/animation/KeyframeEffect.cpp:514
    #3 0x7f71fd95fc8e in mozilla::dom::KeyframeEffectReadOnly::SetKeyframes(JSContext*, JS::Handle<JSObject*>, mozilla::ErrorResult&) /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/animation/KeyframeEffect.cpp:454
    #4 0x7f71fd96e39a in already_AddRefed<mozilla::dom::KeyframeEffect> mozilla::dom::KeyframeEffectReadOnly::ConstructKeyframeEffect<mozilla::dom::KeyframeEffect, mozilla::dom::UnrestrictedDoubleOrKeyframeAnimationOptions>(mozilla::dom::GlobalObject const&, mozilla::dom::Nullable<mozilla::dom::ElementOrCSSPseudoElement> const&, JS::Handle<JSObject*>, mozilla::dom::UnrestrictedDoubleOrKeyframeAnimationOptions const&, mozilla::ErrorResult&) /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/animation/KeyframeEffect.cpp:753
    #5 0x7f71fd96dd4d in mozilla::dom::KeyframeEffect::Constructor(mozilla::dom::GlobalObject const&, mozilla::dom::Nullable<mozilla::dom::ElementOrCSSPseudoElement> const&, JS::Handle<JSObject*>, mozilla::dom::UnrestrictedDoubleOrKeyframeAnimationOptions const&, mozilla::ErrorResult&) /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/animation/KeyframeEffect.cpp:1358
    #6 0x7f71fdb9ad44 in mozilla::dom::Element::Animate(mozilla::dom::Nullable<mozilla::dom::ElementOrCSSPseudoElement> const&, JSContext*, JS::Handle<JSObject*>, mozilla::dom::UnrestrictedDoubleOrKeyframeAnimationOptions const&, mozilla::ErrorResult&) /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/base/Element.cpp:3356
    #7 0x7f71fdb9a68e in mozilla::dom::Element::Animate(JSContext*, JS::Handle<JSObject*>, mozilla::dom::UnrestrictedDoubleOrKeyframeAnimationOptions const&, mozilla::ErrorResult&) /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/base/Element.cpp:3313
    #8 0x7f71ff63fdc5 in mozilla::dom::ElementBinding::animate(JSContext*, JS::Handle<JSObject*>, mozilla::dom::Element*, JSJitMethodCallArgs const&) /builds/slave/m-cen-l64-asan-000000000000000/build/src/obj-firefox/dom/bindings/ElementBinding.cpp:3414
    #9 0x7f71ffaf96b5 in mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/bindings/BindingUtils.cpp:2782
    #10 0x7f7205619b1e in CallJSNative /builds/slave/m-cen-l64-asan-000000000000000/build/src/js/src/jscntxtinlines.h:235
    #11 0x7f7205619b1e in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/slave/m-cen-l64-asan-000000000000000/build/src/js/src/vm/Interpreter.cpp:480
    #12 0x7f720560877c in CallFromStack /builds/slave/m-cen-l64-asan-000000000000000/build/src/js/src/vm/Interpreter.cpp:531
    #13 0x7f720560877c in Interpret(JSContext*, js::RunState&) /builds/slave/m-cen-l64-asan-000000000000000/build/src/js/src/vm/Interpreter.cpp:2868
    #14 0x7f72055e9b9e in js::RunScript(JSContext*, js::RunState&) /builds/slave/m-cen-l64-asan-000000000000000/build/src/js/src/vm/Interpreter.cpp:426
    #15 0x7f720561a26b in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/slave/m-cen-l64-asan-000000000000000/build/src/js/src/vm/Interpreter.cpp:498
    #16 0x7f720561a801 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) /builds/slave/m-cen-l64-asan-000000000000000/build/src/js/src/vm/Interpreter.cpp:544
    #17 0x7f72051c4e77 in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) /builds/slave/m-cen-l64-asan-000000000000000/build/src/js/src/jsapi.cpp:2926
    #18 0x7f71ff5bb923 in mozilla::dom::EventHandlerNonNull::Call(JSContext*, JS::Handle<JS::Value>, mozilla::dom::Event&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&) /builds/slave/m-cen-l64-asan-000000000000000/build/src/obj-firefox/dom/bindings/EventHandlerBinding.cpp:259
    #19 0x7f71ffef7e89 in Call<nsISupports *> /builds/slave/m-cen-l64-asan-000000000000000/build/src/obj-firefox/dist/include/mozilla/dom/EventHandlerBinding.h:356
    #20 0x7f71ffef7e89 in mozilla::JSEventHandler::HandleEvent(nsIDOMEvent*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/events/JSEventHandler.cpp:214
    #21 0x7f71ffec28b4 in mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, nsIDOMEvent*, mozilla::dom::EventTarget*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/events/EventListenerManager.cpp:1111
    #22 0x7f71ffec4813 in mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent**, mozilla::dom::EventTarget*, nsEventStatus*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/events/EventListenerManager.cpp:1283
    #23 0x7f71ffea1921 in mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/events/EventDispatcher.cpp:379
    #24 0x7f71ffea5d3c in mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/events/EventDispatcher.cpp:710
    #25 0x7f7201e791c0 in nsDocumentViewer::LoadComplete(nsresult) /builds/slave/m-cen-l64-asan-000000000000000/build/src/layout/base/nsDocumentViewer.cpp:993
    #26 0x7f7202bbf49d in nsDocShell::EndPageLoad(nsIWebProgress*, nsIChannel*, nsresult) /builds/slave/m-cen-l64-asan-000000000000000/build/src/docshell/base/nsDocShell.cpp:7533
    #27 0x7f7202bbb8a4 in nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, nsresult) /builds/slave/m-cen-l64-asan-000000000000000/build/src/docshell/base/nsDocShell.cpp:7334
    #28 0x7f7202bc26cf in non-virtual thunk to nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, nsresult) /builds/slave/m-cen-l64-asan-000000000000000/build/src/obj-firefox/docshell/base/Unified_cpp_docshell_base0.cpp:7341
    #29 0x7f71fcf2ce8b in nsDocLoader::DoFireOnStateChange(nsIWebProgress*, nsIRequest*, int&, nsresult) /builds/slave/m-cen-l64-asan-000000000000000/build/src/uriloader/base/nsDocLoader.cpp:1250
    #30 0x7f71fcf2c173 in nsDocLoader::doStopDocumentLoad(nsIRequest*, nsresult) /builds/slave/m-cen-l64-asan-000000000000000/build/src/uriloader/base/nsDocLoader.cpp:834
    #31 0x7f71fcf28e53 in nsDocLoader::DocLoaderIsEmpty(bool) /builds/slave/m-cen-l64-asan-000000000000000/build/src/uriloader/base/nsDocLoader.cpp:724
    #32 0x7f71fcf2b12f in nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, nsresult) /builds/slave/m-cen-l64-asan-000000000000000/build/src/uriloader/base/nsDocLoader.cpp:608
    #33 0x7f71fcf2bb3c in non-virtual thunk to nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, nsresult) /builds/slave/m-cen-l64-asan-000000000000000/build/src/obj-firefox/uriloader/base/Unified_cpp_uriloader_base0.cpp:612
    #34 0x7f71fb394594 in nsLoadGroup::RemoveRequest(nsIRequest*, nsISupports*, nsresult) /builds/slave/m-cen-l64-asan-000000000000000/build/src/netwerk/base/nsLoadGroup.cpp:633
    #35 0x7f71fddb399c in nsDocument::DoUnblockOnload() /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/base/nsDocument.cpp:9229
    #36 0x7f71fde5d02f in nsUnblockOnloadEvent::Run() /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/base/nsDocument.cpp:9182
    #37 0x7f71fb1c78db in nsThread::ProcessNextEvent(bool, bool*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/xpcom/threads/nsThread.cpp:1073
    #38 0x7f71fb241e2a in NS_ProcessNextEvent(nsIThread*, bool) /builds/slave/m-cen-l64-asan-000000000000000/build/src/xpcom/glue/nsThreadUtils.cpp:290
    #39 0x7f71fbf5156e in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/ipc/glue/MessagePump.cpp:98
    #40 0x7f71fbec622c in RunInternal /builds/slave/m-cen-l64-asan-000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:235
    #41 0x7f71fbec622c in RunHandler /builds/slave/m-cen-l64-asan-000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:228
    #42 0x7f71fbec622c in MessageLoop::Run() /builds/slave/m-cen-l64-asan-000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:208
    #43 0x7f72015da447 in nsBaseAppShell::Run() /builds/slave/m-cen-l64-asan-000000000000000/build/src/widget/nsBaseAppShell.cpp:156
    #44 0x7f72035f5342 in XRE_RunAppShell /builds/slave/m-cen-l64-asan-000000000000000/build/src/toolkit/xre/nsEmbedFunctions.cpp:809
    #45 0x7f71fbec622c in RunInternal /builds/slave/m-cen-l64-asan-000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:235
    #46 0x7f71fbec622c in RunHandler /builds/slave/m-cen-l64-asan-000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:228
    #47 0x7f71fbec622c in MessageLoop::Run() /builds/slave/m-cen-l64-asan-000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:208
    #48 0x7f72035f4a21 in XRE_InitChildProcess /builds/slave/m-cen-l64-asan-000000000000000/build/src/toolkit/xre/nsEmbedFunctions.cpp:644
    #49 0x48df67 in content_process_main(int, char**) /builds/slave/m-cen-l64-asan-000000000000000/build/src/ipc/app/../contentproc/plugin-container.cpp:231
    #50 0x7f71f8a0282f in __libc_start_main /build/glibc-GKVZIf/glibc-2.23/csu/../csu/libc-start.c:291
    #51 0x48cb3c in _start (/home/nils/fuzzer3/firefox/plugin-container+0x48cb3c)

0x62d001246400 is located 0 bytes to the right of 32768-byte region [0x62d00123e400,0x62d001246400)
allocated by thread T0 (Web Content) here:
    #0 0x4753cb in realloc /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:95
    #1 0x48e9fd in moz_xrealloc /builds/slave/m-cen-l64-asan-000000000000000/build/src/memory/mozalloc/mozalloc.cpp:105
    #2 0x7f71faff63bf in Realloc /builds/slave/m-cen-l64-asan-000000000000000/build/src/obj-firefox/dist/include/nsTArray.h:181
    #3 0x7f71faff63bf in nsTArrayInfallibleAllocator::ResultTypeProxy nsTArray_base<nsTArrayInfallibleAllocator, nsTArray_CopyWithMemutils>::EnsureCapacity<nsTArrayInfallibleAllocator>(unsigned long, unsigned long) /builds/slave/m-cen-l64-asan-000000000000000/build/src/obj-firefox/dist/include/nsTArray-inl.h:183
    #4 0x7f71fd9648d3 in AppendElements<nsTArrayInfallibleAllocator> /builds/slave/m-cen-l64-asan-000000000000000/build/src/obj-firefox/dist/include/nsTArray.h:1609
    #5 0x7f71fd9648d3 in AppendElement<nsTArrayInfallibleAllocator> /builds/slave/m-cen-l64-asan-000000000000000/build/src/obj-firefox/dist/include/nsTArray.h:1637
    #6 0x7f71fd9648d3 in mozilla::KeyframeUtils::GetAnimationPropertiesFromKeyframes(nsStyleContext*, mozilla::dom::Element*, mozilla::CSSPseudoElementType, nsTArray<mozilla::Keyframe> const&) /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/animation/KeyframeUtils.cpp:541
    #7 0x7f71fd95336b in mozilla::dom::KeyframeEffectReadOnly::UpdateProperties(nsStyleContext*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/animation/KeyframeEffect.cpp:514
    #8 0x7f71fd95fc8e in mozilla::dom::KeyframeEffectReadOnly::SetKeyframes(JSContext*, JS::Handle<JSObject*>, mozilla::ErrorResult&) /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/animation/KeyframeEffect.cpp:454
    #9 0x7f71fd96e39a in already_AddRefed<mozilla::dom::KeyframeEffect> mozilla::dom::KeyframeEffectReadOnly::ConstructKeyframeEffect<mozilla::dom::KeyframeEffect, mozilla::dom::UnrestrictedDoubleOrKeyframeAnimationOptions>(mozilla::dom::GlobalObject const&, mozilla::dom::Nullable<mozilla::dom::ElementOrCSSPseudoElement> const&, JS::Handle<JSObject*>, mozilla::dom::UnrestrictedDoubleOrKeyframeAnimationOptions const&, mozilla::ErrorResult&) /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/animation/KeyframeEffect.cpp:753
    #10 0x7f71fd96dd4d in mozilla::dom::KeyframeEffect::Constructor(mozilla::dom::GlobalObject const&, mozilla::dom::Nullable<mozilla::dom::ElementOrCSSPseudoElement> const&, JS::Handle<JSObject*>, mozilla::dom::UnrestrictedDoubleOrKeyframeAnimationOptions const&, mozilla::ErrorResult&) /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/animation/KeyframeEffect.cpp:1358
    #11 0x7f71fdb9ad44 in mozilla::dom::Element::Animate(mozilla::dom::Nullable<mozilla::dom::ElementOrCSSPseudoElement> const&, JSContext*, JS::Handle<JSObject*>, mozilla::dom::UnrestrictedDoubleOrKeyframeAnimationOptions const&, mozilla::ErrorResult&) /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/base/Element.cpp:3356
    #12 0x7f71fdb9a68e in mozilla::dom::Element::Animate(JSContext*, JS::Handle<JSObject*>, mozilla::dom::UnrestrictedDoubleOrKeyframeAnimationOptions const&, mozilla::ErrorResult&) /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/base/Element.cpp:3313
    #13 0x7f71ff63fdc5 in mozilla::dom::ElementBinding::animate(JSContext*, JS::Handle<JSObject*>, mozilla::dom::Element*, JSJitMethodCallArgs const&) /builds/slave/m-cen-l64-asan-000000000000000/build/src/obj-firefox/dom/bindings/ElementBinding.cpp:3414
    #14 0x7f71ffaf96b5 in mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/bindings/BindingUtils.cpp:2782
    #15 0x7f7205619b1e in CallJSNative /builds/slave/m-cen-l64-asan-000000000000000/build/src/js/src/jscntxtinlines.h:235
    #16 0x7f7205619b1e in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/slave/m-cen-l64-asan-000000000000000/build/src/js/src/vm/Interpreter.cpp:480
    #17 0x7f720560877c in CallFromStack /builds/slave/m-cen-l64-asan-000000000000000/build/src/js/src/vm/Interpreter.cpp:531
    #18 0x7f720560877c in Interpret(JSContext*, js::RunState&) /builds/slave/m-cen-l64-asan-000000000000000/build/src/js/src/vm/Interpreter.cpp:2868
    #19 0x7f72055e9b9e in js::RunScript(JSContext*, js::RunState&) /builds/slave/m-cen-l64-asan-000000000000000/build/src/js/src/vm/Interpreter.cpp:426
    #20 0x7f720561a26b in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/slave/m-cen-l64-asan-000000000000000/build/src/js/src/vm/Interpreter.cpp:498
    #21 0x7f720561a801 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) /builds/slave/m-cen-l64-asan-000000000000000/build/src/js/src/vm/Interpreter.cpp:544
    #22 0x7f72051c4e77 in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) /builds/slave/m-cen-l64-asan-000000000000000/build/src/js/src/jsapi.cpp:2926
    #23 0x7f71ff5bb923 in mozilla::dom::EventHandlerNonNull::Call(JSContext*, JS::Handle<JS::Value>, mozilla::dom::Event&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&) /builds/slave/m-cen-l64-asan-000000000000000/build/src/obj-firefox/dom/bindings/EventHandlerBinding.cpp:259
    #24 0x7f71ffef7e89 in Call<nsISupports *> /builds/slave/m-cen-l64-asan-000000000000000/build/src/obj-firefox/dist/include/mozilla/dom/EventHandlerBinding.h:356
    #25 0x7f71ffef7e89 in mozilla::JSEventHandler::HandleEvent(nsIDOMEvent*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/events/JSEventHandler.cpp:214
    #26 0x7f71ffec28b4 in mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, nsIDOMEvent*, mozilla::dom::EventTarget*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/events/EventListenerManager.cpp:1111
    #27 0x7f71ffec4813 in mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent**, mozilla::dom::EventTarget*, nsEventStatus*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/events/EventListenerManager.cpp:1283
    #28 0x7f71ffea1921 in mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/events/EventDispatcher.cpp:379
    #29 0x7f71ffea5d3c in mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/events/EventDispatcher.cpp:710
    #30 0x7f7201e791c0 in nsDocumentViewer::LoadComplete(nsresult) /builds/slave/m-cen-l64-asan-000000000000000/build/src/layout/base/nsDocumentViewer.cpp:993
    #31 0x7f7202bbf49d in nsDocShell::EndPageLoad(nsIWebProgress*, nsIChannel*, nsresult) /builds/slave/m-cen-l64-asan-000000000000000/build/src/docshell/base/nsDocShell.cpp:7533
    #32 0x7f7202bbb8a4 in nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, nsresult) /builds/slave/m-cen-l64-asan-000000000000000/build/src/docshell/base/nsDocShell.cpp:7334
    #33 0x7f7202bc26cf in non-virtual thunk to nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, nsresult) /builds/slave/m-cen-l64-asan-000000000000000/build/src/obj-firefox/docshell/base/Unified_cpp_docshell_base0.cpp:7341
    #34 0x7f71fcf2ce8b in nsDocLoader::DoFireOnStateChange(nsIWebProgress*, nsIRequest*, int&, nsresult) /builds/slave/m-cen-l64-asan-000000000000000/build/src/uriloader/base/nsDocLoader.cpp:1250
    #35 0x7f71fcf2c173 in nsDocLoader::doStopDocumentLoad(nsIRequest*, nsresult) /builds/slave/m-cen-l64-asan-000000000000000/build/src/uriloader/base/nsDocLoader.cpp:834

SUMMARY: AddressSanitizer: heap-buffer-overflow /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/animation/KeyframeUtils.cpp:959 BuildSegmentsFromValueEntries
Shadow bytes around the buggy address:
  0x0c5a80240c30: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c5a80240c40: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c5a80240c50: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c5a80240c60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c5a80240c70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x0c5a80240c80:[fa]fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c5a80240c90: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c5a80240ca0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c5a80240cb0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c5a80240cc0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c5a80240cd0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
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
==2854==ABORTING
Brian, do you know who might be able to look into this?
Flags: needinfo?(bbirtles)
Group: core-security → layout-core-security
I'll look into this for now.

I notice that we're hitting an assertion in StyleAnimationValue::ExtractComputedValue where we get back a null unit for the stroke-dasharray property (strokeDasharray: 'none' in the test).

I wonder if that's going to leave us with a values array that violates some assumptions in BuildSegmentsFromValueEntries.

For my own reference, the stack in comment 0 points the finger at this line (which has shifted in current trunk m-c):

  https://hg.mozilla.org/mozilla-central/file/8d85de1a5c24/dom/animation/KeyframeUtils.cpp#l959

Looking at that line, I'm not entirely sure why we assume it's safe to access aEntries[i + 2]. I'll look into it a little more in a moment.
Assignee: nobody → bbirtles
Status: NEW → ASSIGNED
Flags: needinfo?(bbirtles)
(In reply to Brian Birtles (:birtles) from comment #2)
> Looking at that line, I'm not entirely sure why we assume it's safe to
> access aEntries[i + 2]. I'll look into it a little more in a moment.

So it looks like it's based on the assertion that our first element has offset 0 and our last element has offset 1. We know that this point that aEntries[i + 1].mOffset == 0.0f so, based on the assertion about the last element, we must have at *least* one more frame at i + 2 (and if it is the last frame, it will have offset 1.0).

So perhaps we're getting into a situation where we have a keyframe at offset 0, but none at offset 1, hence we read past the end of the array. That probably wouldn't have happened when we checked the result of StyleAnimationValue::ExtractComputedValue earlier in the process, but now that we store nsCSSValues and convert to StyleAnimationValues at a later point I guess it does.

I know I ran some checks that even if StyleAnimationValue::ExtractComputedValue returned false we wouldn't do anything bad, but perhaps I forgot to check for the case of 0.0 being filled-in but 1.0 being left off.

That's all just a guess though, I've yet to step through and check. If it's correct, this probably affects aurora too (but not beta, I think).
Oh, it looks like the problem is actually with:

  o59.animate([{dominantBaseline: 'use-script !important',all: 'initial'},{dominantBaseline: 'mathematical',all: 'unset'}],1);

For 'all', we compute the value for 'initial' ok, but when we get to the second keyframe and encounter 'unset', StyleAnimationValue::ComputeValues returns false so we don't add keyframes at offset 1.0 for those values.

So we have a situation where:

* We *try* to detect cases like this in advance in RequiresAdditiveAnimation but that explicitly does not catch all cases since it is called in situations where we don't have a context element so we can't check if all values can be computed sensibly or not. In this case, we check the 'all:unset' can be parsed correctly but don't check if it can be computed into suitable StyleAnimationValues and hence we don't detect this case in advance.

* We sort the KeyframeValueEntry objects by property and then offset

* In this case, although we have one keyframe with an offset of 1.0 (for dominant-baseline), it doesn't sort last since we sort first by property name so we fail to satisfy the condition that the last keyframe has offset 1.0.

So I guess we need to make BuildSegmentsFromValueEntries more robust to drop properties if we don't have a 0.0 or 1.0 keyframe.

(For what it's worth, I'm not sure if 'all:unset' is supposed to be use-able in keyframes but I guess it is.)
It turns out that it's not as simple as all: unset always causing ComputeValues to fail. Rather, after dropping the assertion generated by strokeDasharray: 'none', we hit can get ComputeValues to fail (and generate a sequence of properties where we don't have a value at offset 1.0 for some properties) using the following reduced test case:

  <div id=div></div>
  <script>
  function start() {
    document.documentElement.animate(
      [ { strokeDasharray: 'none' },
        { strokeDasharray: 'none' } ], 1);
    div.animate([{ all: 'initial' },{ all: 'unset' }], 1);
  }
  </script>
  <body onload="start()"></body>

It doesn't reproduce with other properties that I've tried, only strokeDasharray. It doesn't reproduce if the two animations are applied to the same element.

It does reproduce if I replace 'unset' with 'inherit'.

I guess we go to inherit the 'none' value but, in ComputeValuesFromStyleRule, we will get false back StyleAnimationValue::ExtractComputedValue. I'll have to check tomorrow why we recover successfully if we just do:

   document.documentElement.animate(
    [ { strokeDasharray: 'none' },
      { strokeDasharray: '5, 5' } ], 1);

But not if we inherit 'none' through a shorthand.
So it looks like part of the issue with stroke-dasharray is that as of [1] we represent stroke-dasharray: none" with single-value nsCSSValueList. However, when we go to inherit that we don't expect 'none' values in the list.

That's a simple fix but I'm concerned there will be other cases similar to this and we need a more general solution.

As far as I can see, we can do two things:

1) Make BuildSegmentsFromValueEntries robust enough to entry lists where not all properties have 0.0 and 1.0 keyframes. This is easy enough but I don't think we can write an automated test for this. I mean, we can write one using the stroke-dasharray but once we fix that the test will no longer work. Likewise for any other property:value combinations that trigger ComputeValues to fail.

2) Make StyleAnimationValue::ComputeValues fill in all values with 'none' values or something suitable even in the failure case. That seems difficult, however, since there are a bunch of reasons we might fail in that method and for all of them we'd have to still make sure we go through all the component properties and set up 'none' values. It's also not clear what we should do if the passed-in property is disabled.

I'm going to go ahead and do (1) even if we can't write an automated test (or at least not one that we can check-in -- I'll write one locally). That might be ok since we hope within about 6 months to implement additive animation which should provide the fallback behavior to use in this case.

[1] https://hg.mozilla.org/mozilla-central/rev/1c1825d6960d
Looking into this further (and trying to write a test case), I'm starting to suspect bug 1269175 for this (https://hg.mozilla.org/mozilla-central/rev/1c1825d6960d). I'm hitting assertions where the nsStyleCoord has a null unit which makes me suspect we're not managing the memory of those dasharray arrays correctly.
(In reply to Brian Birtles (:birtles) from comment #7)
> Looking into this further (and trying to write a test case), I'm starting to
> suspect bug 1269175 for this
> (https://hg.mozilla.org/mozilla-central/rev/1c1825d6960d)

Nope, that's not it. I can reproduce the assertion on the parent of that changeset.
More likely it's the fact that nsRuleNode::ComputeSVGData doesn't recognize that we can have none values in the list so it leaves the coord initialized to none. However, as of bug 524539 we represent none values as nsCSSValueList objects where the first value is none.[1]

[1] https://hg.mozilla.org/mozilla-central/rev/1c1825d6960d

So we can either try to emulate that in mStrokeDasharray, or probably better, recognize that pattern (a list with a single element whose value is none), and leave the array empty.

It's probably not necessary to fix this bug, however.
This can probably be landed in a separate public bug but I'm just putting it here now since I don't have anywhere else to put it
Comment on attachment 8759545 [details] [diff] [review]
Handle entries arrays which don't have values at 0.0/1.0 offset

Hi Cameron, what do you think of this? This is not as trivial as I hoped. I've written tests (attachment 8759546 [details] [diff] [review]) but they're not in a state they could be checked-in. I'm not even sure how we could make them landable short of adding some fairly invasive test API. I'll have a think about it though.

Comment 6 explains a bit of background. Basically, I'm trying to add code to safely handle when ComputeValues fails.
Attachment #8759545 - Flags: feedback?(cam)
(In reply to Brian Birtles (:birtles) from comment #13)
> Hi Cameron, what do you think of this? This is not as trivial as I hoped.
> I've written tests (attachment 8759546 [details] [diff] [review]) but
> they're not in a state they could be checked-in. I'm not even sure how we
> could make them landable short of adding some fairly invasive test API. I'll
> have a think about it though.

Actually, I reckon if I look a bit harder I can probably find another example of a shorthand value that parses but doesn't compute. It might even be as simple as using variables. Then we could write tests for this.
(In reply to Brian Birtles (:birtles) from comment #14)
> Actually, I reckon if I look a bit harder I can probably find another
> example of a shorthand value that parses but doesn't compute. It might even
> be as simple as using variables. Then we could write tests for this.

So far I haven't been able to succeed in doing this. Even if I use variables and stuff them with all sorts of invalid values, we don't fail to compute the value because we'll just ignore the invalid token stream value and return a style context with the default values filled-in. At this stage I can't think of any way to reliably reproduce this situation once we fix the stroke-dasharray bug.
Comment on attachment 8759545 [details] [diff] [review]
Handle entries arrays which don't have values at 0.0/1.0 offset

I can't think of anything better to do at this stage. I don't like landing this without automated tests we can land with it (or shortly after it) but it still seems better to land something locally tested that is known to fix a security bug, than do nothing.

Perhaps we could add a chrome-only API that lets us toggle a kind of test mode where we can do something like attachment 8759546 [details] [diff] [review]? i.e. recognize a certain value and fail when we see it?
Attachment #8759545 - Flags: feedback?(cam) → review?(cam)
Comment on attachment 8759545 [details] [diff] [review]
Handle entries arrays which don't have values at 0.0/1.0 offset

Review of attachment 8759545 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/animation/KeyframeUtils.cpp
@@ +977,5 @@
>  
>    size_t i = 0, n = aEntries.Length();
>  
>    while (i + 1 < n) {
> +    // Skip properties that don't have an entry with offset 0.

We should have a comment somewhere that says we have to handle properties with missing t=0/1 values in this way due to the possibility of ComputeValues failing earlier.

@@ +989,5 @@
> +    }
> +
> +    // Drop properties that don't end with an entry with offset 1.
> +    if (aEntries[i].mProperty != aEntries[i + 1].mProperty &&
> +        aEntries[i].mOffset != 1.0f) {

Because the loop goes up to the second last element, I think we'll miss the case where we are missing an mOffset == 1.0f entry at the very end of the aEntries array.

@@ +1006,5 @@
>        // We need to generate an initial zero-length segment.
>        MOZ_ASSERT(aEntries[i].mProperty == aEntries[i + 1].mProperty);
>        j = i + 1;
> +      while (aEntries[j + 1].mOffset == 0.0f &&
> +             aEntries[j + 1].mProperty == aEntries[j].mProperty) {

Let's ensure we have a test for this case, entries like:

  (property = X, offset = 0.0)
  (property = Y, offset = 0.0)
  (property = Y, offset = 1.0)

@@ +1011,5 @@
>          ++j;
>        }
>      } else if (aEntries[i].mOffset == 1.0f) {
> +      if (aEntries[i + 1].mOffset == 1.0f &&
> +          aEntries[i + 1].mProperty == aEntries[i].mProperty) {

And for this:

  (property = X, offset = 0.0)
  (property = X, offset = 1.0)
  (property = Y, offset = 1.0)

@@ +1021,5 @@
>            ++j;
>          }
>        } else {
>          // New property.
> +        animationProperty = nullptr;

With this and the other |animationProperty = nullptr| assignment earlier, will it always be null once we're about to move on to a new property?  If so (and I think it is) please assert just before we assign the new appended AnimationProperty to it below.

I wonder if we should also be asserting that the mProperty of the entries are never eCSSProperty_UNKNOWN, since we use that as a dummy value just before the loop to detect the first time to append a new AnimationProperty.

@@ -1003,5 @@
>          }
>        } else {
>          // New property.
> -        MOZ_ASSERT(aEntries[i + 1].mOffset == 0.0f);
> -        MOZ_ASSERT(aEntries[i].mProperty != aEntries[i + 1].mProperty);

I think it's safe to leave this second assertion.  If the current entry has mOffset == 1.0f, and the following entry has mOffset != 1.0f, then it must be for a different property (even if we consider missing offsets due to failed ComputedValues calls).
Attachment #8759545 - Flags: review?(cam)
(In reply to Brian Birtles (:birtles) from comment #16)
> Perhaps we could add a chrome-only API that lets us toggle a kind of test
> mode where we can do something like attachment 8759546 [details] [diff] [review]
> [review]? i.e. recognize a certain value and fail when we see it?

I'm fine with a [ChromeOnly] thing that can cause ComputeValues to fail.

From IRC discussion, can we have it be a [ChromeOnly] dictionary member on one of the animate() arguments, which makes a value at a specific index fail to compute?  I think that would be better than hard coding in a special failing value like 77%.
(In reply to Cameron McCormack (:heycam) from comment #18)
> (In reply to Brian Birtles (:birtles) from comment #16)
> > Perhaps we could add a chrome-only API that lets us toggle a kind of test
> > mode where we can do something like attachment 8759546 [details] [diff] [review]
> > [review]? i.e. recognize a certain value and fail when we see it?
> 
> I'm fine with a [ChromeOnly] thing that can cause ComputeValues to fail.
> 
> From IRC discussion, can we have it be a [ChromeOnly] dictionary member on
> one of the animate() arguments, which makes a value at a specific index fail
> to compute?  I think that would be better than hard coding in a special
> failing value like 77%.

I'm having a hard time doing this without introducing an extra bool member on Keyframe (since the point where we read this value off the dictionary is quite separate from the point where we go to call ComputeValues on the Keyframe object we generate as a result). It seems unfortunate to swell the size of all Keyframes for the sake if this check. Perhaps I can exploit some of the other members of the nsCSSTokenStream class to indicate a value we should reject.
Hmm, can you just use a valid CSS value with invalid variable references that are guaranteed to be invalid at computed value time, like "var(--invalid)" without that variable existing?  Or does that not cause ComputeValues to fail.  If you need to store something additional on nsCSSTokenStream there's free space after mLevel.
(In reply to Cameron McCormack (:heycam) from comment #20)
> Hmm, can you just use a valid CSS value with invalid variable references
> that are guaranteed to be invalid at computed value time, like
> "var(--invalid)" without that variable existing?  Or does that not cause
> ComputeValues to fail.  If you need to store something additional on
> nsCSSTokenStream there's free space after mLevel.

Yeah, I'm pretty sure invalid variables don't do the trick -- we just skip over them.

However, we only really need to test failures on shorthands, I think. Or at least, that should be sufficient. And we currently represent shorthands as token streams where we just leave all the fields at their default values (e.g. eCSSProperty_UNKNOWN) and stick the string in the mTokenStream member. We pull out the mTokenStream string and pass that along when we actually go to compute the value.

So I think we could repurpose any of those other members to indicate "shorthand value that we should fail on". As long as we carefully document it and encapsulate that behavior in KeyframeUtils I think it's ok. We already check the value of the mProperty fields etc. to distinguish between token streams that represent:

* Shorthands
* Longhands that are valid token streams (e.g. have variable references)
* Longhands that failed to parse but which we stuffed in a token stream so we can return the original values from getKeyframes()

So we'd just be extending that logic to differentiate between two types of shorthand values.
Sounds good.
(In reply to Cameron McCormack (:heycam) from comment #17)
> ::: dom/animation/KeyframeUtils.cpp
> @@ +977,5 @@
> >  
> >    size_t i = 0, n = aEntries.Length();
> >  
> >    while (i + 1 < n) {
> > +    // Skip properties that don't have an entry with offset 0.
> 
> We should have a comment somewhere that says we have to handle properties
> with missing t=0/1 values in this way due to the possibility of
> ComputeValues failing earlier.

Added.

> @@ +989,5 @@
> > +    }
> > +
> > +    // Drop properties that don't end with an entry with offset 1.
> > +    if (aEntries[i].mProperty != aEntries[i + 1].mProperty &&
> > +        aEntries[i].mOffset != 1.0f) {
> 
> Because the loop goes up to the second last element, I think we'll miss the
> case where we are missing an mOffset == 1.0f entry at the very end of the
> aEntries array.

Fixed and added test.

> @@ +1006,5 @@
> >        // We need to generate an initial zero-length segment.
> >        MOZ_ASSERT(aEntries[i].mProperty == aEntries[i + 1].mProperty);
> >        j = i + 1;
> > +      while (aEntries[j + 1].mOffset == 0.0f &&
> > +             aEntries[j + 1].mProperty == aEntries[j].mProperty) {
> 
> Let's ensure we have a test for this case, entries like:
> 
>   (property = X, offset = 0.0)
>   (property = Y, offset = 0.0)
>   (property = Y, offset = 1.0)

That should be covered by the following (newly-added) case:

    frames:   [ { margin: '5px', right: '10px' },
                { margin: '5px', right: '20px',
                  simulateComputeValuesFailure: true } ],

> @@ +1011,5 @@
> >          ++j;
> >        }
> >      } else if (aEntries[i].mOffset == 1.0f) {
> > +      if (aEntries[i + 1].mOffset == 1.0f &&
> > +          aEntries[i + 1].mProperty == aEntries[i].mProperty) {
> 
> And for this:
> 
>   (property = X, offset = 0.0)
>   (property = X, offset = 1.0)
>   (property = Y, offset = 1.0)

That should be covered by the (existing) case:

    frames:   [ { margin: '77%', left: '10px',
                  simulateComputeValuesFailure: true },
                { margin: '5px', left: '20px' } ],

> @@ +1021,5 @@
> >            ++j;
> >          }
> >        } else {
> >          // New property.
> > +        animationProperty = nullptr;
> 
> With this and the other |animationProperty = nullptr| assignment earlier,
> will it always be null once we're about to move on to a new property?  If so
> (and I think it is) please assert just before we assign the new appended
> AnimationProperty to it below.

Added.

> I wonder if we should also be asserting that the mProperty of the entries
> are never eCSSProperty_UNKNOWN, since we use that as a dummy value just
> before the loop to detect the first time to append a new AnimationProperty.

Added.

> @@ -1003,5 @@
> >          }
> >        } else {
> >          // New property.
> > -        MOZ_ASSERT(aEntries[i + 1].mOffset == 0.0f);
> > -        MOZ_ASSERT(aEntries[i].mProperty != aEntries[i + 1].mProperty);
> 
> I think it's safe to leave this second assertion.  If the current entry has
> mOffset == 1.0f, and the following entry has mOffset != 1.0f, then it must
> be for a different property (even if we consider missing offsets due to
> failed ComputedValues calls).

You're right, I've added it back. I originally dropped it before changing the condition before it.

Patches forthcoming once I split them up correctly.
MozReview-Commit-ID: GCCF4taOC6z
Attachment #8760599 - Flags: review?(cam)
Attachment #8759545 - Attachment is obsolete: true
MozReview-Commit-ID: DIQyt7f91an
Attachment #8760604 - Flags: review?(cam)
Attachment #8759546 - Attachment is obsolete: true
Comment on attachment 8760599 [details] [diff] [review]
part 1 - Handle entries arrays where we don't have a property value at the 0.0/1.0 offset

Review of attachment 8760599 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/animation/KeyframeUtils.cpp
@@ +1004,5 @@
> +    // Drop properties that don't end with an entry with offset 1.
> +    if ((aEntries[i].mProperty != aEntries[i + 1].mProperty &&
> +         aEntries[i].mOffset != 1.0f) ||
> +        // Including when this is the last segment in the list
> +        (i == n - 2 && aEntries[i + 1].mOffset != 1.0f)) {

Is this right?  What about an array that has these three entries:

  [0] mProperty = P1, mOffset = 0.0f
  [1] mProperty = P1, mOffset = 1.0f
  [2] mProperty = P2, mOffset = 0.0f

The first time around the loop, i = 0 and we create a new AnimationProperty and add the first segment to it.  The next time around, i = 1 and this condition will be true, and we'll remove that sole segment from aResult.
(In reply to Cameron McCormack (:heycam) from comment #26)
> Comment on attachment 8760599 [details] [diff] [review]
> part 1 - Handle entries arrays where we don't have a property value at the
> 0.0/1.0 offset
> 
> Review of attachment 8760599 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/animation/KeyframeUtils.cpp
> @@ +1004,5 @@
> > +    // Drop properties that don't end with an entry with offset 1.
> > +    if ((aEntries[i].mProperty != aEntries[i + 1].mProperty &&
> > +         aEntries[i].mOffset != 1.0f) ||
> > +        // Including when this is the last segment in the list
> > +        (i == n - 2 && aEntries[i + 1].mOffset != 1.0f)) {
> 
> Is this right?  What about an array that has these three entries:
> 
>   [0] mProperty = P1, mOffset = 0.0f
>   [1] mProperty = P1, mOffset = 1.0f
>   [2] mProperty = P2, mOffset = 0.0f
> 
> The first time around the loop, i = 0 and we create a new AnimationProperty
> and add the first segment to it.  The next time around, i = 1 and this
> condition will be true, and we'll remove that sole segment from aResult.

Ah, I guess we can't actually reproduce that case, hence why none of the tests fail. The closest we can reproduce is:

>   [0] mProperty = P1, mOffset = 0.0f
>   [1] mProperty = P1, mOffset = 1.0f
>   [2] mProperty = P2, mOffset = 0.0f
>   [3] mProperty = P3, mOffset = 0.0f
>   [4] mProperty = P4, mOffset = 0.0f
>   [5] mProperty = P5, mOffset = 0.0f

I guess we need a separate branch to deal with the last segment case.
Comment on attachment 8760604 [details] [diff] [review]
part 2 - Add tests for entries array handling when ComputeValues fails

Review of attachment 8760604 [details] [diff] [review]:
-----------------------------------------------------------------

Please add some brief comments in nsCSSValue.h mentioning the different kinds of special values we can store in nsCSSTokenStreams.  r=me with that and if you add a test for the case that I mention in the part 1 review.  You'll need to get a DOM peer to review the .webidl change.
Attachment #8760604 - Flags: review?(cam) → review+
Attachment #8760599 - Attachment is obsolete: true
Attachment #8760599 - Flags: review?(cam)
Olli, would you mind checking the webidl changes here? Thanks
Attachment #8760615 - Flags: review?(bugs)
Attachment #8760604 - Attachment is obsolete: true
Comment on attachment 8760614 [details] [diff] [review]
part 1 - Handle entries arrays where we don't have a property value at the 0.0/1.0 offset

Review of attachment 8760614 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks.
Attachment #8760614 - Flags: review?(cam) → review+
Comment on attachment 8760615 [details] [diff] [review]
part 2 - Add tests for entries array handling when ComputeValues fails

r+ for .webidl
Attachment #8760615 - Flags: review?(bugs) → review+
Comment on attachment 8760614 [details] [diff] [review]
part 1 - Handle entries arrays where we don't have a property value at the 0.0/1.0 offset

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
See below--somewhat easily.

> Do comments in the patch, the check-in comment, or tests included in the
> patch paint a bulls-eye on the security problem?
Not a bulls-eye, but pretty close. There's a comment that specifically mentions that this patch makes us do the right thing when StyleAnimationValue::ComputeValues fails. So basically it tells you that if you can find a way to get ComputeValues to fail (non-trivial, but possible) then you could reproduce the problem. Shall I drop the comment?

> Which older supported branches are affected by this flaw?
Beta and Aurora (i.e. 48 and 49)

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

> Do you have backports for the affected branches? If not, how different,
> hard to create, and risky will they be?
This patch appears to apply cleanly to aurora and beta.

> How likely is this patch to cause regressions; how much testing does it need?
It's not likely to cause functional regressions--it simply makes us do something sensible in a particular edge case that normally should not happen. Part 2 (which I plan to land later, perhaps after we've opened up this bug) includes tests for quite a large combination of inputs, however it's possible that there is some combination we've overlooked. If that's the case, is's likely to be a further edge case rather than a new regression. I haven't run these tests on try but have verified that the tests that exercise this code under dom/animation/test/, testing/web-platform/tests/web-animations, and layout/style/test/test_{animation|transition}* pass locally on Windows with this patch applied.
Attachment #8760614 - Flags: sec-approval?
Comment on attachment 8760614 [details] [diff] [review]
part 1 - Handle entries arrays where we don't have a property value at the 0.0/1.0 offset

sec-approval+ for trunk since it only affects trunk, beta, and aurora.

Please make and nominate 48 and 49 patches too.
Attachment #8760614 - Flags: sec-approval? → sec-approval+
Comment on attachment 8760614 [details] [diff] [review]
part 1 - Handle entries arrays where we don't have a property value at the 0.0/1.0 offset

Approval Request Comment
[Feature/regressing bug #]: Bug 1245748
[User impact if declined]: Potentially exploitable security bug
[Describe test coverage new/current, TreeHerder]: Test written (attachment 8760615 [details] [diff] [review]) but won't be landed until the patch has landed on affected branches.
[Risks and why]: I haven't run the patch through try because I didn't want to expose the vulnerability. I'll land this on trunk momentarily and so we'll find out pretty soon if there are any major problems. Otherwise, see comment 33.
[String/UUID change made/needed]: None.
Attachment #8760614 - Flags: approval-mozilla-beta?
Attachment #8760614 - Flags: approval-mozilla-aurora?
(In reply to Cameron McCormack (:heycam) from comment #28)
> Please add some brief comments in nsCSSValue.h mentioning the different
> kinds of special values we can store in nsCSSTokenStreams.

I went to add this comment but I'm not sure if it's necessary since, for the most part, the special values are encapsulted within KeyframeUtils.

Here's the comment I drafted:

// A string value used primarily to represent variable references.
//
// Animation code, specifically the KeyframeUtils class, also uses this
// type as a container for various string values including:
//
// * Shorthand property values
// * Shorthand sentinel values used for testing failure conditions
// * Invalid longhand property values
//
// For the most part, the above values are not passed to functions that
// manipulate nsCSSValue objects in a generic fashion. Instead KeyframeUtils
// extracts the string from the nsCSSValueTokenStream and passes that around
// instead. The single exception is nsCSSValue::AppendToString which we use
// to serialize the string contained in the nsCSSValueTokenStream by ensuring
// the mShorthandPropertyID is set to eCSSProperty_UNKNOWN.

What do you think?
Thanks, I think this sounds good.
Updated comment. I'm not planning to land this until part 1 has been applied to beta and aurora branches, however.
Attachment #8760615 - Attachment is obsolete: true
Liz, I'd like to land this on beta/aurora fairly quickly since the fix that landed on trunk gives some hints about how to exploit this. What do you think? (approval request in comment 35)
Flags: needinfo?(lhenry)
Flags: in-testsuite?
Closing since it's only tests ad backports that are yet to land (I didn't realize until know that we closed bugs when only tests had yet to land).
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Keywords: leave-open
Resolution: --- → FIXED
Comment on attachment 8760614 [details] [diff] [review]
part 1 - Handle entries arrays where we don't have a property value at the 0.0/1.0 offset

sec-high, looks low risk, ok for aurora and beta uplift.
Flags: needinfo?(lhenry)
Attachment #8760614 - Flags: approval-mozilla-beta?
Attachment #8760614 - Flags: approval-mozilla-beta+
Attachment #8760614 - Flags: approval-mozilla-aurora?
Attachment #8760614 - Flags: approval-mozilla-aurora+
Duplicate of this bug: 1277392
Blocks: 1245748
Flags: sec-bounty?
Keywords: regression
Flags: sec-bounty? → sec-bounty+
Group: layout-core-security → core-security-release
I believe the fix for this bug has now shipped on all affected branches so it should be safe to open.
Pushed test case to inbound because I believe the fix has shipped on all affected branches:

https://hg.mozilla.org/integration/mozilla-inbound/rev/ccb47df5ed6a947150a7c786eab419137fde296c
Flags: in-testsuite? → in-testsuite+
Daniel, I think it's safe to make this bug public now since the fix has shipped on all affected branches.
Flags: needinfo?(dveditz)
Group: core-security-release
Flags: needinfo?(dveditz)
You need to log in before you can comment on or make changes to this bug.