Closed Bug 1502936 Opened 6 years ago Closed 6 years ago

use-after-poison in [@ nsSVGUseFrame::PositionAttributeChanged]

Categories

(Core :: SVG, defect)

Product:
Core
Component:
SVG
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla65
Tracking Flags:
Tracking Status
firefox-esr60 --- unaffected
firefox63 --- unaffected
firefox64 + fixed
firefox65 + fixed

People

(Reporter: tsmith, Assigned: emilio)

References

(Blocks 1 open bug)

Details

(5 keywords, Whiteboard: [post-critsmash-triage])

Attachments

(3 files)

testcase.html
204 bytes, text/html
Details
segv_log.txt
9.41 KB, text/plain
Details
Bug 1502936 - Fix SVGUseElement::GetFrame to not get confused.
47 bytes, text/x-phabricator-request
dveditz
: approval-mozilla-beta+
dveditz
: sec-approval+
Details | Review
Attached file testcase.html — 
Reduced with m-c:
BuildID=20181029092600
SourceStamp=f7a97b344fa59bd3b01ea81ebd5b150aa63bfb12

==89611==ERROR: AddressSanitizer: use-after-poison on address 0x6250002c4748 at pc 0x7f68bcd3647e bp 0x7ffd75b86280 sp 0x7ffd75b86278
READ of size 8 at 0x6250002c4748 thread T0 (file:// Content)
    #0 0x7f68bcd3647d in assign src/obj-firefox/dist/include/nsAutoPtr.h:38:17
    #1 0x7f68bcd3647d in operator= src/obj-firefox/dist/include/nsAutoPtr.h:129
    #2 0x7f68bcd3647d in nsSVGUseFrame::PositionAttributeChanged() src/layout/svg/nsSVGUseFrame.cpp:64
    #3 0x7f68baee2210 in mozilla::dom::SVGUseElement::ProcessAttributeChange(int, nsAtom*) src/dom/svg/SVGUseElement.cpp:101:16
    #4 0x7f68baee3674 in mozilla::dom::SVGUseElement::AfterSetAttr(int, nsAtom*, nsAttrValue const*, nsAttrValue const*, nsIPrincipal*, bool) src/dom/svg/SVGUseElement.cpp:138:3
    #5 0x7f68b5b0b303 in mozilla::dom::Element::SetAttrAndNotify(int, nsAtom*, nsAtom*, nsAttrValue const*, nsAttrValue&, nsIPrincipal*, unsigned char, bool, bool, bool, nsIDocument*, mozAutoDocUpdate const&) src/dom/base/Element.cpp:2774:10
    #6 0x7f68b5b012f4 in mozilla::dom::Element::SetAttr(int, nsAtom*, nsAtom*, nsTSubstring<char16_t> const&, nsIPrincipal*, bool) src/dom/base/Element.cpp:2621:10
    #7 0x7f68b5b0090e in SetAttr src/obj-firefox/dist/include/mozilla/dom/Element.h:870:12
    #8 0x7f68b5b0090e in mozilla::dom::Element::SetAttribute(nsTSubstring<char16_t> const&, nsTSubstring<char16_t> const&, nsIPrincipal*, mozilla::ErrorResult&) src/dom/base/Element.cpp:1415
    #9 0x7f68b8773c9f in mozilla::dom::Element_Binding::setAttribute(JSContext*, JS::Handle<JSObject*>, mozilla::dom::Element*, JSJitMethodCallArgs const&) src/obj-firefox/dom/bindings/ElementBinding.cpp:1289:9
    #10 0x7f68b9073454 in bool mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*) src/dom/bindings/BindingUtils.cpp:3314:13
    #11 0x7f68c1fec658 in CallJSNative src/js/src/vm/Interpreter.cpp:468:15
    #12 0x7f68c1fec658 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) src/js/src/vm/Interpreter.cpp:560
    #13 0x7f68c1fd3478 in CallFromStack src/js/src/vm/Interpreter.cpp:620:12
    #14 0x7f68c1fd3478 in Interpret(JSContext*, js::RunState&) src/js/src/vm/Interpreter.cpp:3462
    #15 0x7f68c1fb80c8 in js::RunScript(JSContext*, js::RunState&) src/js/src/vm/Interpreter.cpp:447:12
    #16 0x7f68c1fed066 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) src/js/src/vm/Interpreter.cpp:587:15
    #17 0x7f68c1feed12 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) src/js/src/vm/Interpreter.cpp:633:10
    #18 0x7f68c07722b6 in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) src/js/src/jsapi.cpp:2975:12
    #19 0x7f68b85f94a6 in mozilla::dom::EventHandlerNonNull::Call(JSContext*, JS::Handle<JS::Value>, mozilla::dom::Event&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&) src/obj-firefox/dom/bindings/EventHandlerBinding.cpp:265:37
    #20 0x7f68b9904979 in void mozilla::dom::EventHandlerNonNull::Call<nsISupports*>(nsISupports* const&, mozilla::dom::Event&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&, char const*, mozilla::dom::CallbackObject::ExceptionHandling, JS::Realm*) src/obj-firefox/dist/include/mozilla/dom/EventHandlerBinding.h:363:12
    #21 0x7f68b9901c09 in mozilla::JSEventHandler::HandleEvent(mozilla::dom::Event*) src/dom/events/JSEventHandler.cpp:214:12
    #22 0x7f68b98b5c6a in mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, mozilla::dom::Event*, mozilla::dom::EventTarget*) src/dom/events/EventListenerManager.cpp:1107:52
    #23 0x7f68b98b8267 in mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event**, mozilla::dom::EventTarget*, nsEventStatus*, bool) src/dom/events/EventListenerManager.cpp:1309:15
    #24 0x7f68b989a9d6 in HandleEvent src/obj-firefox/dist/include/mozilla/EventListenerManager.h:390:5
    #25 0x7f68b989a9d6 in mozilla::EventTargetChainItem::HandleEvent(mozilla::EventChainPostVisitor&, mozilla::ELMCreationDetector&) src/dom/events/EventDispatcher.cpp:424
    #26 0x7f68b9898c58 in mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) src/dom/events/EventDispatcher.cpp:641:16
    #27 0x7f68b989f717 in mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*) src/dom/events/EventDispatcher.cpp:1156:11
    #28 0x7f68bc560b8e in nsDocumentViewer::LoadComplete(nsresult) src/layout/base/nsDocumentViewer.cpp:1167:7
    #29 0x7f68bf46cc63 in nsDocShell::EndPageLoad(nsIWebProgress*, nsIChannel*, nsresult) src/docshell/base/nsDocShell.cpp:7009:21
    #30 0x7f68bf468489 in nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, nsresult) src/docshell/base/nsDocShell.cpp:6800:7
    #31 0x7f68bf471217 in non-virtual thunk to nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, nsresult) src/docshell/base/nsDocShell.cpp
    #32 0x7f68b4456ba5 in nsDocLoader::DoFireOnStateChange(nsIWebProgress*, nsIRequest*, int&, nsresult) src/uriloader/base/nsDocLoader.cpp:1307:3
    #33 0x7f68b445578c in nsDocLoader::doStopDocumentLoad(nsIRequest*, nsresult) src/uriloader/base/nsDocLoader.cpp:850:14
    #34 0x7f68b44510c8 in nsDocLoader::DocLoaderIsEmpty(bool) src/uriloader/base/nsDocLoader.cpp:739:9
    #35 0x7f68b4453a1e in nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, nsresult) src/uriloader/base/nsDocLoader.cpp:628:5
    #36 0x7f68b44552b4 in non-virtual thunk to nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, nsresult) src/uriloader/base/nsDocLoader.cpp
    #37 0x7f68b1e46897 in mozilla::net::nsLoadGroup::RemoveRequest(nsIRequest*, nsISupports*, nsresult) src/netwerk/base/nsLoadGroup.cpp:630:28
    #38 0x7f68b5db80c7 in DoUnblockOnload src/dom/base/nsDocument.cpp:8505:18
    #39 0x7f68b5db80c7 in nsDocument::UnblockOnload(bool) src/dom/base/nsDocument.cpp:8427
    #40 0x7f68b5d92142 in nsIDocument::DispatchContentLoadedEvents() src/dom/base/nsDocument.cpp:5320:3
    #41 0x7f68b5efbb1b in applyImpl<nsIDocument, void (nsIDocument::*)()> src/obj-firefox/dist/include/nsThreadUtils.h:1191:12
    #42 0x7f68b5efbb1b in apply<nsIDocument, void (nsIDocument::*)()> src/obj-firefox/dist/include/nsThreadUtils.h:1197
    #43 0x7f68b5efbb1b in mozilla::detail::RunnableMethodImpl<nsIDocument*, void (nsIDocument::*)(), true, (mozilla::RunnableKind)0>::Run() src/obj-firefox/dist/include/nsThreadUtils.h:1242
    #44 0x7f68b1b7e8a5 in mozilla::SchedulerGroup::Runnable::Run() src/xpcom/threads/SchedulerGroup.cpp:337:32
    #45 0x7f68b1bbbe81 in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:1245:14
    #46 0x7f68b1bc4bcd in NS_ProcessNextEvent(nsIThread*, bool) src/xpcom/threads/nsThreadUtils.cpp:530:10
    #47 0x7f68b2e0db4f in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:97:21
    #48 0x7f68b2d09eae in RunInternal src/ipc/chromium/src/base/message_loop.cc:325:10
    #49 0x7f68b2d09eae in RunHandler src/ipc/chromium/src/base/message_loop.cc:318
    #50 0x7f68b2d09eae in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:298
    #51 0x7f68bbce8ba3 in nsBaseAppShell::Run() src/widget/nsBaseAppShell.cpp:158:27
    #52 0x7f68c024cbee in XRE_RunAppShell() src/toolkit/xre/nsEmbedFunctions.cpp:939:22
    #53 0x7f68b2d09eae in RunInternal src/ipc/chromium/src/base/message_loop.cc:325:10
    #54 0x7f68b2d09eae in RunHandler src/ipc/chromium/src/base/message_loop.cc:318
    #55 0x7f68b2d09eae in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:298
    #56 0x7f68c024bc1a in XRE_InitChildProcess(int, char**, XREChildData const*) src/toolkit/xre/nsEmbedFunctions.cpp:765:34
    #57 0x5610e25e5864 in content_process_main src/browser/app/../../ipc/contentproc/plugin-container.cpp:50:30
    #58 0x5610e25e5864 in main src/browser/app/nsBrowserApp.cpp:301
    #59 0x7f68d518a82f in __libc_start_main /build/glibc-Cl5G7W/glibc-2.23/csu/../csu/libc-start.c:291
    #60 0x5610e250aeec in _start (firefox+0x2deec)

0x6250002c4748 is located 3656 bytes inside of 8192-byte region [0x6250002c3900,0x6250002c5900)
allocated by thread T0 (file:// Content) here:
    #0 0x5610e25b2d93 in malloc /builds/worker/workspace/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:146:3
    #1 0x7f68b1b59010 in mozilla::ArenaAllocator<8192ul, 8ul>::AllocateChunk(unsigned long) src/obj-firefox/dist/include/mozilla/ArenaAllocator.h:193:15
    #2 0x7f68b1b4e888 in InternalAllocate src/obj-firefox/dist/include/mozilla/ArenaAllocator.h:228:25
    #3 0x7f68b1b4e888 in Allocate src/obj-firefox/dist/include/mozilla/ArenaAllocator.h:75
    #4 0x7f68b1b4e888 in mozilla::ArenaAllocator<8192ul, 8ul>::Allocate(unsigned long) src/obj-firefox/dist/include/mozilla/ArenaAllocator.h:80
    #5 0x7f68bc6d7f4a in AllocateByFrameID src/layout/base/nsPresArena.h:39:12
    #6 0x7f68bc6d7f4a in AllocateFrame src/layout/base/nsIPresShell.h:207
    #7 0x7f68bc6d7f4a in operator new src/layout/generic/ViewportFrame.cpp:34
    #8 0x7f68bc6d7f4a in NS_NewViewportFrame(nsIPresShell*, mozilla::ComputedStyle*) src/layout/generic/ViewportFrame.cpp:31
    #9 0x7f68bc4f58ea in nsCSSFrameConstructor::ConstructRootFrame() src/layout/base/nsCSSFrameConstructor.cpp:2651:5
    #10 0x7f68bc422b22 in mozilla::PresShell::Initialize() src/layout/base/PresShell.cpp:1795:36
    #11 0x7f68b5cd9a18 in nsContentSink::StartLayout(bool) src/dom/base/nsContentSink.cpp:1276:26
    #12 0x7f68b4677b02 in nsHtml5TreeOpExecutor::StartLayout(bool*) src/parser/html/nsHtml5TreeOpExecutor.cpp:673:18
    #13 0x7f68b467313b in nsHtml5TreeOperation::Perform(nsHtml5TreeOpExecutor*, nsIContent**, bool*, bool*) src/parser/html/nsHtml5TreeOperation.cpp:1204:17
    #14 0x7f68b467000a in nsHtml5TreeOpExecutor::RunFlushLoop() src/parser/html/nsHtml5TreeOpExecutor.cpp:489:17
    #15 0x7f68b467c3bb in nsHtml5ExecutorFlusher::Run() src/parser/html/nsHtml5StreamParser.cpp:123:18
    #16 0x7f68b1b7e8a5 in mozilla::SchedulerGroup::Runnable::Run() src/xpcom/threads/SchedulerGroup.cpp:337:32
    #17 0x7f68b1bbbe81 in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:1245:14
    #18 0x7f68b1bc4bcd in NS_ProcessNextEvent(nsIThread*, bool) src/xpcom/threads/nsThreadUtils.cpp:530:10
    #19 0x7f68b2e0db4f in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:97:21
    #20 0x7f68b2d09eae in RunInternal src/ipc/chromium/src/base/message_loop.cc:325:10
    #21 0x7f68b2d09eae in RunHandler src/ipc/chromium/src/base/message_loop.cc:318
    #22 0x7f68b2d09eae in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:298
    #23 0x7f68bbce8ba3 in nsBaseAppShell::Run() src/widget/nsBaseAppShell.cpp:158:27
    #24 0x7f68c024cbee in XRE_RunAppShell() src/toolkit/xre/nsEmbedFunctions.cpp:939:22
    #25 0x7f68b2d09eae in RunInternal src/ipc/chromium/src/base/message_loop.cc:325:10
    #26 0x7f68b2d09eae in RunHandler src/ipc/chromium/src/base/message_loop.cc:318
    #27 0x7f68b2d09eae in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:298
    #28 0x7f68c024bc1a in XRE_InitChildProcess(int, char**, XREChildData const*) src/toolkit/xre/nsEmbedFunctions.cpp:765:34
Flags: in-testsuite?
Attached file segv_log.txt — 
I also came across this crash while reducing the test case.
Stack trace suggests this is the recently landed bug 1502658
This is a pre-existing issue to some extent... In a debug build this asserts with:

  Assertion failure: frame->IsSVGUseFrame(), at /home/emilio/src/moz/gecko/dom/svg/SVGUseElement.cpp:566

So something that other code already asumed is clearly not right.
Assignee: nobody → emilio
Fuzzing also started hitting other similar traces. If these aren't dups let me know and I will log them with test cases.

src/layout/svg/nsSVGUseFrame.cpp:77:7: runtime error: load of value 88, which is not a valid value for type 'bool'
    #0 0x7f3a3a9e0adb in nsSVGUseFrame::DimensionAttributeChanged(bool, bool) src/layout/svg/nsSVGUseFrame.cpp:77:7
    #1 0x7f3a38b8c5d8 in mozilla::dom::SVGUseElement::ProcessAttributeChange(int, nsAtom*) src/dom/svg/SVGUseElement.cpp:112:16
    #2 0x7f3a38b8d674 in mozilla::dom::SVGUseElement::AfterSetAttr(int, nsAtom*, nsAttrValue const*, nsAttrValue const*, nsIPrincipal*, bool) src/dom/svg/SVGUseElement.cpp:138:3
    #3 0x7f3a337b5303 in mozilla::dom::Element::SetAttrAndNotify(int, nsAtom*, nsAtom*, nsAttrValue const*, nsAttrValue&, nsIPrincipal*, unsigned char, bool, bool, bool, nsIDocument*, mozAutoDocUpdate const&) src/dom/base/Element.cpp:2774:10
    #4 0x7f3a38bb6408 in nsSVGElement::DidChangeValue(nsAtom*, nsAttrValue const&, nsAttrValue&) src/dom/svg/nsSVGElement.cpp:1432:3
    #5 0x7f3a38bd188b in DidChangeLength src/dom/svg/nsSVGElement.cpp:1543:3
    ...


==11402==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x625001751900 at pc 0x7fd18f8c3ab6 bp 0x7ffeb024b580 sp 0x7ffeb024b578
READ of size 1 at 0x625001751900 thread T0
    #0 0x7fd18f8c3ab5 in nsSVGUseFrame::DimensionAttributeChanged(bool, bool) src/layout/svg/nsSVGUseFrame.cpp:77:7
    #1 0x7fd18da6f5d8 in mozilla::dom::SVGUseElement::ProcessAttributeChange(int, nsAtom*) src/dom/svg/SVGUseElement.cpp:112:16
    #2 0x7fd18da70674 in mozilla::dom::SVGUseElement::AfterSetAttr(int, nsAtom*, nsAttrValue const*, nsAttrValue const*, nsIPrincipal*, bool) src/dom/svg/SVGUseElement.cpp:138:3
    #3 0x7fd188698303 in mozilla::dom::Element::SetAttrAndNotify(int, nsAtom*, nsAtom*, nsAttrValue const*, nsAttrValue&, nsIPrincipal*, unsigned char, bool, bool, bool, nsIDocument*, mozAutoDocUpdate const&) src/dom/base/Element.cpp:2774:10
    #4 0x7fd18da99408 in nsSVGElement::DidChangeValue(nsAtom*, nsAttrValue const&, nsAttrValue&) src/dom/svg/nsSVGElement.cpp:1432:3
    #5 0x7fd18dab488b in DidChangeLength src/dom/svg/nsSVGElement.cpp:1543:3
    #6 0x7fd18dab488b in nsSVGLength2::SetBaseValueInSpecifiedUnits(float, nsSVGElement*, bool) src/dom/svg/nsSVGLength2.cpp:293
    #7 0x7fd18d909f5c in mozilla::DOMSVGLength::SetValueInSpecifiedUnits(float, mozilla::ErrorResult&) src/dom/svg/DOMSVGLength.cpp:313:11
    #8 0x7fd189ed3960 in mozilla::dom::SVGLength_Binding::set_valueInSpecifiedUnits(JSContext*, JS::Handle<JSObject*>, mozilla::DOMSVGLength*, JSJitSetterCallArgs) src/obj-firefox/dom/bindings/SVGLengthBinding.cpp:153:9
    #9 0x7fd18bbfcea0 in bool mozilla::dom::binding_detail::GenericSetter<mozilla::dom::binding_detail::NormalThisPolicy>(JSContext*, unsigned int, JS::Value*) src/dom/bindings/BindingUtils.cpp:3258:8
    #10 0x7fd194b79658 in CallJSNative src/js/src/vm/Interpreter.cpp:468:15
    ...
Yeah, all those are dups... Sorry for the breakage, patch incoming.
Blocks: 1502658
Should've noticed in bug 1502658 that GetFrame() was dead, to verify its
assumptions... Oh well.
Comment on attachment 9020888 [details]
Bug 1502936 - Fix SVGUseElement::GetFrame to not get confused.

Hi Daniel, could I ask for sec-rating / sec-approval for this? It's type confusion, which makes me a bit less sure about it being sec-low / sec-moderate, and the regressing patch has ended up in b5.

[Security Approval Request]

How easily could an exploit be constructed based on the patch?: Unsure, it's use-after-poison, but the underlying issue is type confusion, which makes me unsure about the severity.

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

Which older supported branches are affected by this flaw?: None, only latest beta build which includes the regressing bug.

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

Do you have backports for the affected branches?: Yes

If not, how different, hard to create, and risky will they be?: 

How likely is this patch to cause regressions; how much testing does it need?: Very hard, it fixes a function which was dead before bug 1502658 and I assumed (wrongly, as it can be seen) to be right.
Flags: needinfo?(dveditz)
Attachment #9020888 - Flags: sec-approval?
nsSVGUseFrame isn't an nsIFrame so we can't say this is safe "framepoisoning". I guess sec-high looking at what could come later if this pointer keeps being used? Normally wouldn't need sec-approval for a regression from yesterday, but since this got super fast beta uplift technically we do.
Flags: needinfo?(dveditz)
Keywords: csectype-uaf, regression, sec-high
Comment on attachment 9020888 [details]
Bug 1502936 - Fix SVGUseElement::GetFrame to not get confused.

sec-approval+ and beta approval a=dveditz
Attachment #9020888 - Flags: sec-approval?
Attachment #9020888 - Flags: sec-approval+
Attachment #9020888 - Flags: approval-mozilla-beta+
(In reply to Daniel Veditz [:dveditz] from comment #8)
> nsSVGUseFrame isn't an nsIFrame so we can't say this is safe

it is an nsIFrame, fwiw.
https://hg.mozilla.org/mozilla-central/rev/8ff29f3d64b7
Group: layout-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox65: affected → fixed
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: