Bug 1443092 (CVE-2018-5154)

heap-use-after-free in nsIFrame::GetClipPropClipRect

VERIFIED FIXED in Firefox -esr52

Status

()

VERIFIED FIXED
a year ago
6 months ago

People

(Reporter: nils, Assigned: botond, NeedInfo)

Tracking

({csectype-uaf, sec-high})

60 Branch
mozilla61
csectype-uaf, sec-high
Points:
---
Bug Flags:
sec-bounty +
in-testsuite +

Firefox Tracking Flags

(firefox-esr5260+ verified, firefox59 wontfix, firefox60+ verified, firefox61+ verified)

Details

(Whiteboard: [adv-main60+][adv-esr52.8+])

Attachments

(7 attachments)

(Reporter)

Description

a year ago
Created attachment 8955997 [details]
crash.html (minimised testcase)

The following testcase crashes the latest ASAN build of Firefox 60.0a1 (SourceStamp=21f7f94a2c18dc8010ac2f300a36cc4ddd16081c). It requires the attached test.svg in the same directory.

crash.html:
<script>
function start() {
	o7=document.createElement('div');
	o7.innerHTML='<style>@keyframes key7{ from{ transform: rotate(-536870911deg)}}\n*{ animation-name: key7; animation-duration: 0.001s';
	o17=document.createElement('div');
	o17.innerHTML='<svg><style>@font-face{}\n*{ outline-style: dotted</style><style>@font-face{ font-family: font3; src: url(';
	o18=o17.firstChild.getElementsByTagName('*');
	o20=o18[0];
	o23=o18[1];
	o114=document.createElement('iframe');
	o114.src='test.svg';
	o114.addEventListener('load', fun0,false);
	document.body.appendChild(o114);
}
function fun0() {
	o117=o114.contentDocument;
	document.replaceChild(o117.documentElement,document.documentElement);
	o124=document.createElement('iframe');
	document.documentElement.appendChild(o124);
	o145=document.createElement('div');
	o145.innerHTML='<svg><set attributeName="text-decoration">';
	document.documentElement.appendChild(o20);
	document.documentElement.appendChild(o23);
	document.documentElement.appendChild(o7);
	o124.src='x';
	document.documentElement.appendChild(o145);
	o264=document.createElement('style');
	o265=document.createTextNode('*{ float: left');
	o264.appendChild(o265);
	o23.appendChild(o264);
	setTimeout("location.reload()",40);
}
</script>
<body onload="start()"></body>

test.svg:
<svg class='class2' xmlns='http://www.w3.org/2000/svg'>
<clipPath clipPathUnits='userSpaceOnUse' id='id9' lighting-color='pink' class='class0 class1' >
</clipPath>
<a id='id0' clip-path='url(#id9)' >
</a>
</svg>

ASAN output:
=================================================================
==5408==ERROR: AddressSanitizer: heap-use-after-free on address 0x6040000e7ebc at pc 0x7fdacf128a80 bp 0x7fff70c73a20 sp 0x7fff70c73a18
READ of size 1 at 0x6040000e7ebc thread T0 (file:// Content)
    #0 0x7fdacf128a7f in nsIFrame::GetClipPropClipRect(nsStyleDisplay const*, nsStyleEffects const*, nsSize const&) const /builds/worker/workspace/build/src/layout/generic/nsFrame.cpp:2426:19
    #1 0x7fdacf133156 in nsIFrame::BuildDisplayListForStackingContext(nsDisplayListBuilder*, nsDisplayList*, bool*) /builds/worker/workspace/build/src/layout/generic/nsFrame.cpp:3013:7
    #2 0x7fdacf0596ed in nsIFrame::BuildDisplayListForChild(nsDisplayListBuilder*, nsIFrame*, nsDisplayListSet const&, unsigned int) /builds/worker/workspace/build/src/layout/generic/nsFrame.cpp:3755:12
    #3 0x7fdacf0eb3fd in nsContainerFrame::BuildDisplayListForNonBlockChildren(nsDisplayListBuilder*, nsDisplayListSet const&, unsigned int) /builds/worker/workspace/build/src/layout/generic/nsContainerFrame.cpp:388:5
    #4 0x7fdacf05a27c in nsIFrame::BuildDisplayListForChild(nsDisplayListBuilder*, nsIFrame*, nsDisplayListSet const&, unsigned int) /builds/worker/workspace/build/src/layout/generic/nsFrame.cpp:3815:14
    #5 0x7fdacf0eb3fd in nsContainerFrame::BuildDisplayListForNonBlockChildren(nsDisplayListBuilder*, nsDisplayListSet const&, unsigned int) /builds/worker/workspace/build/src/layout/generic/nsContainerFrame.cpp:388:5
    #6 0x7fdacf513f93 in nsSVGOuterSVGFrame::BuildDisplayList(nsDisplayListBuilder*, nsDisplayListSet const&) /builds/worker/workspace/build/src/layout/svg/nsSVGOuterSVGFrame.cpp:788:5
    #7 0x7fdacf133e19 in nsIFrame::BuildDisplayListForStackingContext(nsDisplayListBuilder*, nsDisplayList*, bool*) /builds/worker/workspace/build/src/layout/generic/nsFrame.cpp:3047:5
    #8 0x7fdacf0596ed in nsIFrame::BuildDisplayListForChild(nsDisplayListBuilder*, nsIFrame*, nsDisplayListSet const&, unsigned int) /builds/worker/workspace/build/src/layout/generic/nsFrame.cpp:3755:12
    #9 0x7fdacf0da52d in nsCanvasFrame::BuildDisplayList(nsDisplayListBuilder*, nsDisplayListSet const&) /builds/worker/workspace/build/src/layout/generic/nsCanvasFrame.cpp:568:5
    #10 0x7fdacf05a27c in nsIFrame::BuildDisplayListForChild(nsDisplayListBuilder*, nsIFrame*, nsDisplayListSet const&, unsigned int) /builds/worker/workspace/build/src/layout/generic/nsFrame.cpp:3815:14
    #11 0x7fdacf1dd8ee in mozilla::ScrollFrameHelper::BuildDisplayList(nsDisplayListBuilder*, nsDisplayListSet const&) /builds/worker/workspace/build/src/layout/generic/nsGfxScrollFrame.cpp:3617:15
    #12 0x7fdacf05a27c in nsIFrame::BuildDisplayListForChild(nsDisplayListBuilder*, nsIFrame*, nsDisplayListSet const&, unsigned int) /builds/worker/workspace/build/src/layout/generic/nsFrame.cpp:3815:14
    #13 0x7fdacf055c2f in mozilla::ViewportFrame::BuildDisplayList(nsDisplayListBuilder*, nsDisplayListSet const&) /builds/worker/workspace/build/src/layout/generic/ViewportFrame.cpp:66:5
    #14 0x7fdacf133e19 in nsIFrame::BuildDisplayListForStackingContext(nsDisplayListBuilder*, nsDisplayList*, bool*) /builds/worker/workspace/build/src/layout/generic/nsFrame.cpp:3047:5
    #15 0x7fdacef75433 in nsLayoutUtils::GetFramesForArea(nsIFrame*, nsRect const&, nsTArray<nsIFrame*>&, unsigned int) /builds/worker/workspace/build/src/layout/base/nsLayoutUtils.cpp:3330:11
    #16 0x7fdacef74afb in nsLayoutUtils::GetFrameForPoint(nsIFrame*, nsPoint, unsigned int) /builds/worker/workspace/build/src/layout/base/nsLayoutUtils.cpp:3291:8
    #17 0x7fdacee1c1a3 in mozilla::FindFrameTargetedByInputEvent(mozilla::WidgetGUIEvent*, nsIFrame*, nsPoint const&, unsigned int) /builds/worker/workspace/build/src/layout/base/PositionedEventTargeting.cpp:548:5
    #18 0x7fdacee68d0c in mozilla::PresShell::HandleEvent(nsIFrame*, mozilla::WidgetGUIEvent*, bool, nsEventStatus*) /builds/worker/workspace/build/src/layout/base/PresShell.cpp:7191:11
    #19 0x7fdace5b2c1a in nsViewManager::DispatchEvent(mozilla::WidgetGUIEvent*, nsView*, nsEventStatus*) /builds/worker/workspace/build/src/view/nsViewManager.cpp:812:14
    #20 0x7fdace5b2302 in nsView::HandleEvent(mozilla::WidgetGUIEvent*, bool) /builds/worker/workspace/build/src/view/nsView.cpp:1139:9
    #21 0x7fdace61584c in mozilla::widget::PuppetWidget::DispatchEvent(mozilla::WidgetGUIEvent*, nsEventStatus&) /builds/worker/workspace/build/src/widget/PuppetWidget.cpp:410:35
    #22 0x7fdac932ed5c in mozilla::layers::APZCCallbackHelper::DispatchWidgetEvent(mozilla::WidgetGUIEvent&) /builds/worker/workspace/build/src/gfx/layers/apz/util/APZCCallbackHelper.cpp:499:21
    #23 0x7fdacdf08847 in DispatchWidgetEventViaAPZ /builds/worker/workspace/build/src/dom/ipc/TabChild.cpp:1794:10
    #24 0x7fdacdf08847 in mozilla::dom::TabChild::HandleRealMouseButtonEvent(mozilla::WidgetMouseEvent const&, mozilla::layers::ScrollableLayerGuid const&, unsigned long const&) /builds/worker/workspace/build/src/dom/ipc/TabChild.cpp:1734
    #25 0x7fdacdf0984e in mozilla::dom::TabChild::RecvRealMouseButtonEvent(mozilla::WidgetMouseEvent const&, mozilla::layers::ScrollableLayerGuid const&, unsigned long const&) /builds/worker/workspace/build/src/dom/ipc/TabChild.cpp:1701:3
    #26 0x7fdacdf09a24 in RecvSynthMouseMoveEvent /builds/worker/workspace/build/src/dom/ipc/TabChild.cpp:1662:8
    #27 0x7fdacdf09a24 in non-virtual thunk to mozilla::dom::TabChild::RecvSynthMouseMoveEvent(mozilla::WidgetMouseEvent const&, mozilla::layers::ScrollableLayerGuid const&, unsigned long const&) /builds/worker/workspace/build/src/dom/ipc/TabChild.cpp
    #28 0x7fdac81b2ab5 in mozilla::dom::PBrowserChild::OnMessageReceived(IPC::Message const&) /builds/worker/workspace/build/src/obj-firefox/ipc/ipdl/PBrowserChild.cpp:3362:20
    #29 0x7fdac83b1604 in mozilla::dom::PContentChild::OnMessageReceived(IPC::Message const&) /builds/worker/workspace/build/src/obj-firefox/ipc/ipdl/PContentChild.cpp:5023:28
    #30 0x7fdac7ab5f3e in mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&) /builds/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:2133:25
    #31 0x7fdac7ab2fb7 in mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&) /builds/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:2063:17
    #32 0x7fdac7ab46bc in mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::MessageChannel::MessageTask&) /builds/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:1909:5
    #33 0x7fdac7ab4d18 in mozilla::ipc::MessageChannel::MessageTask::Run() /builds/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:1942:15
    #34 0x7fdac6bbc4a0 in mozilla::SchedulerGroup::Runnable::Run() /builds/worker/workspace/build/src/xpcom/threads/SchedulerGroup.cpp:413:25
    #35 0x7fdac6be4a84 in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1040:14
    #36 0x7fdac6c00a40 in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:517:10
    #37 0x7fdac7abe05a in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/worker/workspace/build/src/ipc/glue/MessagePump.cpp:97:21
    #38 0x7fdac7a115a9 in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:326:10
    #39 0x7fdac7a115a9 in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:319
    #40 0x7fdac7a115a9 in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:299
    #41 0x7fdace63f08a in nsBaseAppShell::Run() /builds/worker/workspace/build/src/widget/nsBaseAppShell.cpp:157:27
    #42 0x7fdad2ad29fb in XRE_RunAppShell() /builds/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:892:22
    #43 0x7fdac7a115a9 in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:326:10
    #44 0x7fdac7a115a9 in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:319
    #45 0x7fdac7a115a9 in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:299
    #46 0x7fdad2ad23da in XRE_InitChildProcess(int, char**, XREChildData const*) /builds/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:718:34
    #47 0x4f1875 in content_process_main /builds/worker/workspace/build/src/browser/app/../../ipc/contentproc/plugin-container.cpp:50:30
    #48 0x4f1875 in main /builds/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:280
    #49 0x7fdae611c82f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)
    #50 0x420f48 in _start (/fuzzer3/firefox/firefox+0x420f48)

0x6040000e7ebc is located 44 bytes inside of 48-byte region [0x6040000e7e90,0x6040000e7ec0)
freed by thread T0 (file:// Content) here:
    #0 0x4c1952 in __interceptor_free /builds/worker/workspace/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:68:3
    #1 0x7fdad4a70510 in _$LT$servo_arc..Arc$LT$T$GT$$u20$as$u20$core..ops..drop..Drop$GT$::drop::h9d5951288e1541e5 /builds/worker/workspace/build/src/servo/components/servo_arc/lib.rs:390
    #2 0x7fdad4a70510 in core::ptr::drop_in_place::h79927eef08a236c1 /checkout/src/libcore/ptr.rs:59
    #3 0x7fdad4a70510 in _$LT$servo_arc..RawOffsetArc$LT$T$GT$$u20$as$u20$core..ops..drop..Drop$GT$::drop::h8efc1cbf95802962 /builds/worker/workspace/build/src/servo/components/servo_arc/lib.rs:796
    #4 0x7fdad4a70510 in core::ptr::drop_in_place::h544d4bbc90b52c9b /checkout/src/libcore/ptr.rs:59
    #5 0x7fdad4a70510 in core::ptr::drop_in_place::h7d2987951e4a2458 /checkout/src/libcore/ptr.rs:59
    #6 0x7fdad4a70510 in core::ptr::drop_in_place::h901136eb87f307ed /checkout/src/libcore/ptr.rs:59
    #7 0x7fdad4a70510 in core::ptr::drop_in_place::h1697b417c29b53e8 /checkout/src/libcore/ptr.rs:59
    #8 0x7fdad4a70510 in core::ptr::drop_in_place::h88fe9c96fb8c5fb6 /checkout/src/libcore/ptr.rs:59
    #9 0x7fdad4a70510 in core::ptr::drop_in_place::h962af92429c8852c /checkout/src/libcore/ptr.rs:59
    #10 0x7fdad4a70510 in _$LT$servo_arc..Arc$LT$T$GT$$GT$::drop_slow::h051a8ccf0a675b69 /builds/worker/workspace/build/src/servo/components/servo_arc/lib.rs:256
    #11 0x7fdad4a74552 in _$LT$servo_arc..Arc$LT$T$GT$$u20$as$u20$core..ops..drop..Drop$GT$::drop::h27083b96d3340537 /builds/worker/workspace/build/src/servo/components/servo_arc/lib.rs:390
    #12 0x7fdad4a74552 in core::ptr::drop_in_place::hbba63d62ce0b4882 /checkout/src/libcore/ptr.rs:59
    #13 0x7fdad4a74552 in style::gecko::arc_types::Servo_StyleContext_Release::_$u7b$$u7b$closure$u7d$$u7d$::haa79a1639c6d1f99 /builds/worker/workspace/build/src/servo/components/style/gecko/arc_types.rs:133
    #14 0x7fdad4a74552 in _$LT$servo_arc..ArcBorrow$LT$$u27$a$C$$u20$T$GT$$GT$::with_arc::h5bede05e74d34eef /builds/worker/workspace/build/src/servo/components/servo_arc/lib.rs:939
    #15 0x7fdad4a74552 in Servo_StyleContext_Release /builds/worker/workspace/build/src/servo/components/style/gecko/arc_types.rs:132
    #16 0x7fdaceed3265 in Release /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/ServoStyleContext.h:37:20
    #17 0x7fdaceed3265 in Release /builds/worker/workspace/build/src/layout/style/nsStyleContextInlines.h:54
    #18 0x7fdaceed3265 in Release /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/RefPtr.h:41
    #19 0x7fdaceed3265 in Release /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/RefPtr.h:398
    #20 0x7fdaceed3265 in assign_assuming_AddRef /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/RefPtr.h:66
    #21 0x7fdaceed3265 in assign_with_AddRef /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/RefPtr.h:57
    #22 0x7fdaceed3265 in operator= /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/RefPtr.h:193
    #23 0x7fdaceed3265 in SetStyleContextWithoutNotification /builds/worker/workspace/build/src/layout/generic/nsIFrame.h:821
    #24 0x7fdaceed3265 in nsCSSFrameConstructor::ConstructDocElementFrame(mozilla::dom::Element*, nsILayoutHistoryState*) /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:2526
    #25 0x7fdaceef7afb in nsCSSFrameConstructor::ContentRangeInserted(nsIContent*, nsIContent*, nsIContent*, nsILayoutHistoryState*, nsCSSFrameConstructor::InsertionKind, TreeMatchContext*) /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:8064:9
    #26 0x7fdaceef15cd in nsCSSFrameConstructor::RecreateFramesForContent(nsIContent*, nsCSSFrameConstructor::InsertionKind) /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:9857:9
    #27 0x7fdacee1196d in mozilla::RestyleManager::ProcessRestyledFrames(nsStyleChangeList&) /builds/worker/workspace/build/src/layout/base/RestyleManager.cpp:1658:25
    #28 0x7fdacee8d4a5 in mozilla::ServoRestyleManager::DoProcessPendingRestyles(mozilla::ServoTraversalFlags) /builds/worker/workspace/build/src/layout/base/ServoRestyleManager.cpp:1188:9
    #29 0x7fdacee46836 in ProcessPendingRestyles /builds/worker/workspace/build/src/layout/base/ServoRestyleManager.cpp:1264:3
    #30 0x7fdacee46836 in ProcessPendingRestyles /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/RestyleManagerInlines.h:46
    #31 0x7fdacee46836 in mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush) /builds/worker/workspace/build/src/layout/base/PresShell.cpp:4211
    #32 0x7fdac9e43b05 in FlushPendingNotifications /builds/worker/workspace/build/src/obj-firefox/dist/include/nsIPresShell.h:572:5
    #33 0x7fdac9e43b05 in nsDocument::FlushPendingNotifications(mozilla::FlushType, mozilla::FlushTarget) /builds/worker/workspace/build/src/dom/base/nsDocument.cpp:7602
    #34 0x7fdace036903 in nsSMILAnimationController::DoSample(bool) /builds/worker/workspace/build/src/dom/smil/nsSMILAnimationController.cpp:440:15
    #35 0x7fdacdb8aecb in Resample /builds/worker/workspace/build/src/dom/smil/nsSMILAnimationController.h:74:21
    #36 0x7fdacdb8aecb in FlushResampleRequests /builds/worker/workspace/build/src/dom/smil/nsSMILAnimationController.h:90
    #37 0x7fdacdb8aecb in FlushAnimations /builds/worker/workspace/build/src/dom/svg/nsSVGElement.cpp:2726
    #38 0x7fdacdb8aecb in nsSVGEnum::DOMAnimatedEnum::AnimVal() /builds/worker/workspace/build/src/dom/svg/nsSVGEnum.h:97
    #39 0x7fdacf5241f3 in nsSVGUtils::GetBBox(nsIFrame*, unsigned int, mozilla::gfx::BaseMatrix<double> const*) /builds/worker/workspace/build/src/layout/svg/nsSVGUtils.cpp:1187:20
    #40 0x7fdacf131a1c in ComputeClipForMaskItem /builds/worker/workspace/build/src/layout/generic/nsFrame.cpp:2686:22
    #41 0x7fdacf131a1c in nsIFrame::BuildDisplayListForStackingContext(nsDisplayListBuilder*, nsDisplayList*, bool*) /builds/worker/workspace/build/src/layout/generic/nsFrame.cpp:2997
    #42 0x7fdacf0596ed in nsIFrame::BuildDisplayListForChild(nsDisplayListBuilder*, nsIFrame*, nsDisplayListSet const&, unsigned int) /builds/worker/workspace/build/src/layout/generic/nsFrame.cpp:3755:12
    #43 0x7fdacf0eb3fd in nsContainerFrame::BuildDisplayListForNonBlockChildren(nsDisplayListBuilder*, nsDisplayListSet const&, unsigned int) /builds/worker/workspace/build/src/layout/generic/nsContainerFrame.cpp:388:5
    #44 0x7fdacf05a27c in nsIFrame::BuildDisplayListForChild(nsDisplayListBuilder*, nsIFrame*, nsDisplayListSet const&, unsigned int) /builds/worker/workspace/build/src/layout/generic/nsFrame.cpp:3815:14
    #45 0x7fdacf0eb3fd in nsContainerFrame::BuildDisplayListForNonBlockChildren(nsDisplayListBuilder*, nsDisplayListSet const&, unsigned int) /builds/worker/workspace/build/src/layout/generic/nsContainerFrame.cpp:388:5
    #46 0x7fdacf513f93 in nsSVGOuterSVGFrame::BuildDisplayList(nsDisplayListBuilder*, nsDisplayListSet const&) /builds/worker/workspace/build/src/layout/svg/nsSVGOuterSVGFrame.cpp:788:5
    #47 0x7fdacf133e19 in nsIFrame::BuildDisplayListForStackingContext(nsDisplayListBuilder*, nsDisplayList*, bool*) /builds/worker/workspace/build/src/layout/generic/nsFrame.cpp:3047:5
    #48 0x7fdacf0596ed in nsIFrame::BuildDisplayListForChild(nsDisplayListBuilder*, nsIFrame*, nsDisplayListSet const&, unsigned int) /builds/worker/workspace/build/src/layout/generic/nsFrame.cpp:3755:12
    #49 0x7fdacf0da52d in nsCanvasFrame::BuildDisplayList(nsDisplayListBuilder*, nsDisplayListSet const&) /builds/worker/workspace/build/src/layout/generic/nsCanvasFrame.cpp:568:5
    #50 0x7fdacf05a27c in nsIFrame::BuildDisplayListForChild(nsDisplayListBuilder*, nsIFrame*, nsDisplayListSet const&, unsigned int) /builds/worker/workspace/build/src/layout/generic/nsFrame.cpp:3815:14
    #51 0x7fdacf1dd8ee in mozilla::ScrollFrameHelper::BuildDisplayList(nsDisplayListBuilder*, nsDisplayListSet const&) /builds/worker/workspace/build/src/layout/generic/nsGfxScrollFrame.cpp:3617:15
    #52 0x7fdacf05a27c in nsIFrame::BuildDisplayListForChild(nsDisplayListBuilder*, nsIFrame*, nsDisplayListSet const&, unsigned int) /builds/worker/workspace/build/src/layout/generic/nsFrame.cpp:3815:14
    #53 0x7fdacf055c2f in mozilla::ViewportFrame::BuildDisplayList(nsDisplayListBuilder*, nsDisplayListSet const&) /builds/worker/workspace/build/src/layout/generic/ViewportFrame.cpp:66:5
    #54 0x7fdacf133e19 in nsIFrame::BuildDisplayListForStackingContext(nsDisplayListBuilder*, nsDisplayList*, bool*) /builds/worker/workspace/build/src/layout/generic/nsFrame.cpp:3047:5
    #55 0x7fdacef75433 in nsLayoutUtils::GetFramesForArea(nsIFrame*, nsRect const&, nsTArray<nsIFrame*>&, unsigned int) /builds/worker/workspace/build/src/layout/base/nsLayoutUtils.cpp:3330:11
    #56 0x7fdacef74afb in nsLayoutUtils::GetFrameForPoint(nsIFrame*, nsPoint, unsigned int) /builds/worker/workspace/build/src/layout/base/nsLayoutUtils.cpp:3291:8
    #57 0x7fdacee1c1a3 in mozilla::FindFrameTargetedByInputEvent(mozilla::WidgetGUIEvent*, nsIFrame*, nsPoint const&, unsigned int) /builds/worker/workspace/build/src/layout/base/PositionedEventTargeting.cpp:548:5

previously allocated by thread T0 (file:// Content) here:
    #0 0x4c1c93 in malloc /builds/worker/workspace/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:88:3
    #1 0x7fdad4a79db2 in alloc_system::platform::_$LT$impl$u20$alloc..allocator..Alloc$u20$for$u20$$RF$$u27$a$u20$alloc_system..System$GT$::alloc::h8cd93390057ad93d /checkout/src/liballoc_system/lib.rs:135
    #2 0x7fdad4a79db2 in _$LT$alloc_system..System$u20$as$u20$alloc..allocator..Alloc$GT$::alloc::haa725be1f48230e6 /checkout/src/liballoc_system/lib.rs:57
    #3 0x7fdad4a79db2 in __rdl_alloc /checkout/src/libstd/heap.rs:37
    #4 0x7fdad4a79db2 in _$LT$alloc..heap..Heap$u20$as$u20$alloc..allocator..Alloc$GT$::alloc::h05b55476c9a26516 /checkout/src/liballoc/heap.rs:84
    #5 0x7fdad4a79db2 in alloc::heap::exchange_malloc::h72e689b70b59f4d6 /checkout/src/liballoc/heap.rs:249
    #6 0x7fdad4a79db2 in _$LT$alloc..boxed..Box$LT$T$GT$$GT$::new::ha611249e2e31b711 /checkout/src/liballoc/boxed.rs:242
    #7 0x7fdad4a79db2 in _$LT$servo_arc..Arc$LT$T$GT$$GT$::new::h751c71290f83200a /builds/worker/workspace/build/src/servo/components/servo_arc/lib.rs:185
    #8 0x7fdad4a79db2 in style::gecko_properties::_$LT$impl$u20$style..gecko_bindings..structs..root..mozilla..GeckoEffects$GT$::default::h45253e246a1803b9 /builds/worker/workspace/build/src/obj-firefox/toolkit/library/x86_64-unknown-linux-gnu/release/build/style-6201c7cf09cdb4fe/out/gecko_properties.rs:23318
    #9 0x7fdad4a79db2 in style::gecko_properties::ComputedValues::default_values::h79759018a37c1c59 /builds/worker/workspace/build/src/obj-firefox/toolkit/library/x86_64-unknown-linux-gnu/release/build/style-6201c7cf09cdb4fe/out/gecko_properties.rs:269
    #10 0x7fdad49e66e9 in style::gecko::media_queries::Device::reset_computed_values::hc53a9fc1b6719b31 /builds/worker/workspace/build/src/servo/components/style/gecko/media_queries.rs:145
    #11 0x7fdad49e66e9 in style::gecko::media_queries::Device::rebuild_cached_data::h8f194ecbe0a2ebdf /builds/worker/workspace/build/src/servo/components/style/gecko/media_queries.rs:150
    #12 0x7fdad49e66e9 in Servo_StyleSet_RebuildCachedData /builds/worker/workspace/build/src/servo/ports/geckolib/glue.rs:2600
    #13 0x7fdacee8803a in mozilla::ServoRestyleManager::PostRebuildAllStyleDataEvent(nsChangeHint, nsRestyleHint) /builds/worker/workspace/build/src/layout/base/ServoRestyleManager.cpp:424:15
    #14 0x7fdac9e71a15 in nsIDocument::FlushUserFontSet() /builds/worker/workspace/build/src/dom/base/nsDocument.cpp:12656:22
    #15 0x7fdacee4653a in mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush) /builds/worker/workspace/build/src/layout/base/PresShell.cpp:4187:18
    #16 0x7fdac9e43b05 in FlushPendingNotifications /builds/worker/workspace/build/src/obj-firefox/dist/include/nsIPresShell.h:572:5
    #17 0x7fdac9e43b05 in nsDocument::FlushPendingNotifications(mozilla::FlushType, mozilla::FlushTarget) /builds/worker/workspace/build/src/dom/base/nsDocument.cpp:7602
    #18 0x7fdac9e439eb in nsDocument::FlushPendingNotifications(mozilla::FlushType, mozilla::FlushTarget) /builds/worker/workspace/build/src/dom/base/nsDocument.cpp:7597:22
    #19 0x7fdac8b46b47 in nsDocLoader::DocLoaderIsEmpty(bool) /builds/worker/workspace/build/src/uriloader/base/nsDocLoader.cpp:700:14
    #20 0x7fdac8b48f7c in nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, nsresult) /builds/worker/workspace/build/src/uriloader/base/nsDocLoader.cpp:632:5
    #21 0x7fdac8b49f9c in non-virtual thunk to nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, nsresult) /builds/worker/workspace/build/src/uriloader/base/nsDocLoader.cpp
    #22 0x7fdac6daee0a in mozilla::net::nsLoadGroup::RemoveRequest(nsIRequest*, nsISupports*, nsresult) /builds/worker/workspace/build/src/netwerk/base/nsLoadGroup.cpp:629:28
    #23 0x7fdac6dab4df in mozilla::net::nsLoadGroup::Cancel(nsresult) /builds/worker/workspace/build/src/netwerk/base/nsLoadGroup.cpp:258:15
    #24 0x7fdac8b46669 in nsDocLoader::Stop() /builds/worker/workspace/build/src/uriloader/base/nsDocLoader.cpp:246:22
    #25 0x7fdad1fcc6f9 in Stop /builds/worker/workspace/build/src/docshell/base/nsDocShell.h:202:25
    #26 0x7fdad1fcc6f9 in nsDocShell::Stop(unsigned int) /builds/worker/workspace/build/src/docshell/base/nsDocShell.cpp:5208
    #27 0x7fdad1ff1c03 in nsDocShell::InternalLoad(nsIURI*, nsIURI*, mozilla::Maybe<nsCOMPtr<nsIURI> > const&, bool, nsIURI*, unsigned int, nsIPrincipal*, nsIPrincipal*, unsigned int, nsTSubstring<char16_t> const&, char const*, nsTSubstring<char16_t> const&, nsIInputStream*, long, nsIInputStream*, unsigned int, nsISHEntry*, bool, nsTSubstring<char16_t> const&, nsIDocShell*, nsIURI*, nsIDocShell**, nsIRequest**) /builds/worker/workspace/build/src/docshell/base/nsDocShell.cpp
    #28 0x7fdad1fe8615 in nsDocShell::LoadURI(nsIURI*, nsIDocShellLoadInfo*, unsigned int, bool) /builds/worker/workspace/build/src/docshell/base/nsDocShell.cpp:1017:10
    #29 0x7fdac9ed3e76 in nsFrameLoader::ReallyStartLoadingInternal() /builds/worker/workspace/build/src/dom/base/nsFrameLoader.cpp:597:19
    #30 0x7fdac9ed2a4b in nsFrameLoader::ReallyStartLoading() /builds/worker/workspace/build/src/dom/base/nsFrameLoader.cpp:474:17
    #31 0x7fdac9e29cb9 in nsDocument::MaybeInitializeFinalizeFrameLoaders() /builds/worker/workspace/build/src/dom/base/nsDocument.cpp:6696:13
    #32 0x7fdac9e2956b in nsDocument::EndUpdate(unsigned int) /builds/worker/workspace/build/src/dom/base/nsDocument.cpp:5151:3
    #33 0x7fdacce2af6c in nsHTMLDocument::EndUpdate(unsigned int) /builds/worker/workspace/build/src/dom/html/nsHTMLDocument.cpp:2265:15
    #34 0x7fdac9c1e011 in ~mozAutoDocUpdate /builds/worker/workspace/build/src/dom/base/mozAutoDocUpdate.h:40:18
    #35 0x7fdac9c1e011 in mozilla::dom::Element::SetAttr(int, nsAtom*, nsAtom*, nsTSubstring<char16_t> const&, nsIPrincipal*, bool) /builds/worker/workspace/build/src/dom/base/Element.cpp:2588
    #36 0x7fdacbf2a937 in SetAttr /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/Element.h:895:12
    #37 0x7fdacbf2a937 in SetAttr /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/Element.h:1648
    #38 0x7fdacbf2a937 in SetHTMLAttr /builds/worker/workspace/build/src/dom/html/nsGenericHTMLElement.h:795
    #39 0x7fdacbf2a937 in SetSrc /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/HTMLIFrameElement.h:56
    #40 0x7fdacbf2a937 in mozilla::dom::HTMLIFrameElementBinding::set_src(JSContext*, JS::Handle<JSObject*>, mozilla::dom::HTMLIFrameElement*, JSJitSetterCallArgs) /builds/worker/workspace/build/src/obj-firefox/dom/bindings/HTMLIFrameElementBinding.cpp:86
    #41 0x7fdacc2fa513 in mozilla::dom::GenericBindingSetter(JSContext*, unsigned int, JS::Value*) /builds/worker/workspace/build/src/dom/bindings/BindingUtils.cpp:2992:8
    #42 0x7fdad2d94598 in CallJSNative /builds/worker/workspace/build/src/js/src/vm/JSContext-inl.h:290:15
    #43 0x7fdad2d94598 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:467
    #44 0x7fdad2d96d26 in InternalCall /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:516:12
    #45 0x7fdad2d96d26 in Call /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:535
    #46 0x7fdad2d96d26 in js::CallSetter(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::Handle<JS::Value>) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:664
    #47 0x7fdad3d008c0 in SetExistingProperty(JSContext*, JS::Handle<js::NativeObject*>, JS::Handle<jsid>, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::Handle<js::NativeObject*>, JS::Handle<JS::PropertyResult>, JS::ObjectOpResult&) /builds/worker/workspace/build/src/js/src/vm/NativeObject.cpp:2765:10
    #48 0x7fdad3cf91d6 in bool js::NativeSetProperty<(js::QualifiedBool)1>(JSContext*, JS::Handle<js::NativeObject*>, JS::Handle<jsid>, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::ObjectOpResult&) /builds/worker/workspace/build/src/js/src/vm/NativeObject.cpp:2793:20
    #49 0x7fdad2d74a59 in SetProperty /builds/worker/workspace/build/src/js/src/vm/NativeObject.h:1646:12
    #50 0x7fdad2d74a59 in SetPropertyOperation /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:264
    #51 0x7fdad2d74a59 in Interpret(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:2882

SUMMARY: AddressSanitizer: heap-use-after-free /builds/worker/workspace/build/src/layout/generic/nsFrame.cpp:2426:19 in nsIFrame::GetClipPropClipRect(nsStyleDisplay const*, nsStyleEffects const*, nsSize const&) const
Shadow bytes around the buggy address:
  0x0c0880014f80: fa fa 00 00 00 00 00 fa fa fa fd fd fd fd fd fa
  0x0c0880014f90: fa fa 00 00 00 00 00 fa fa fa 00 00 00 00 00 fa
  0x0c0880014fa0: fa fa fd fd fd fd fd fd fa fa 00 00 00 00 00 fa
  0x0c0880014fb0: fa fa 00 00 00 00 00 fa fa fa fd fd fd fd fd fa
  0x0c0880014fc0: fa fa 00 00 00 00 00 fa fa fa 00 00 00 00 00 00
=>0x0c0880014fd0: fa fa fd fd fd fd fd[fd]fa fa fd fd fd fd fd fd
  0x0c0880014fe0: fa fa fd fd fd fd fd fd fa fa fd fd fd fd fd fa
  0x0c0880014ff0: fa fa fd fd fd fd fd fa fa fa fd fd fd fd fd fa
  0x0c0880015000: fa fa fd fd fd fd fd fa fa fa fd fd fd fd fd fa
  0x0c0880015010: fa fa fd fd fd fd fd fa fa fa fd fd fd fd fd fa
  0x0c0880015020: fa fa fd fd fd fd fd fa fa fa fd fd fd fd fd 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
  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
==5408==ABORTING
(Reporter)

Comment 1

a year ago
Created attachment 8955998 [details]
test.svg
(Reporter)

Comment 2

a year ago
Created attachment 8955999 [details]
ASAN output
Group: core-security → layout-core-security
Keywords: csectype-uaf, sec-high
status-firefox59: --- → ?
Component: Layout → Layout: Web Painting
This is pretty bad, we're restyling in the middle of display list building (hits an assert for exactly that in debug builds).

Relevant bit of the free callstack is this:

#33 0x7fdac9e43b05 in nsDocument::FlushPendingNotifications(mozilla::FlushType, mozilla::FlushTarget) /builds/worker/workspace/build/src/dom/base/nsDocument.cpp:7602
#34 0x7fdace036903 in nsSMILAnimationController::DoSample(bool) /builds/worker/workspace/build/src/dom/smil/nsSMILAnimationController.cpp:440:15
#35 0x7fdacdb8aecb in Resample /builds/worker/workspace/build/src/dom/smil/nsSMILAnimationController.h:74:21
#36 0x7fdacdb8aecb in FlushResampleRequests /builds/worker/workspace/build/src/dom/smil/nsSMILAnimationController.h:90
#37 0x7fdacdb8aecb in FlushAnimations /builds/worker/workspace/build/src/dom/svg/nsSVGElement.cpp:2726
#38 0x7fdacdb8aecb in nsSVGEnum::DOMAnimatedEnum::AnimVal() /builds/worker/workspace/build/src/dom/svg/nsSVGEnum.h:97
#39 0x7fdacf5241f3 in nsSVGUtils::GetBBox(nsIFrame*, unsigned int, mozilla::gfx::BaseMatrix<double> const*) /builds/worker/workspace/build/src/layout/svg/nsSVGUtils.cpp:1187:20
#40 0x7fdacf131a1c in ComputeClipForMaskItem /builds/worker/workspace/build/src/layout/generic/nsFrame.cpp:2686:22
#41 0x7fdacf131a1c in nsIFrame::BuildDisplayListForStackingContext(nsDisplayListBuilder*, nsDisplayList*, bool*) /builds/worker/workspace/build/src/layout/generic/nsFrame.cpp:2997

Looks like botond wrote the code in question (ComputeClipForMaskItem), cc'ing him.

We should have flushed before display list building, so maybe we need to make sure we don't try to flush animations from this caller?
Flags: needinfo?(botond)
(In reply to Matt Woodrow (:mattwoodrow) from comment #3)
> This is pretty bad, we're restyling in the middle of display list building
> (hits an assert for exactly that in debug builds).
> 
> Relevant bit of the free callstack is this:
> 
> #33 0x7fdac9e43b05 in
> nsDocument::FlushPendingNotifications(mozilla::FlushType,
> mozilla::FlushTarget)
> /builds/worker/workspace/build/src/dom/base/nsDocument.cpp:7602
> #34 0x7fdace036903 in nsSMILAnimationController::DoSample(bool)
> /builds/worker/workspace/build/src/dom/smil/nsSMILAnimationController.cpp:
> 440:15
> #35 0x7fdacdb8aecb in Resample
> /builds/worker/workspace/build/src/dom/smil/nsSMILAnimationController.h:74:21
> #36 0x7fdacdb8aecb in FlushResampleRequests
> /builds/worker/workspace/build/src/dom/smil/nsSMILAnimationController.h:90
> #37 0x7fdacdb8aecb in FlushAnimations
> /builds/worker/workspace/build/src/dom/svg/nsSVGElement.cpp:2726
> #38 0x7fdacdb8aecb in nsSVGEnum::DOMAnimatedEnum::AnimVal()
> /builds/worker/workspace/build/src/dom/svg/nsSVGEnum.h:97
> #39 0x7fdacf5241f3 in nsSVGUtils::GetBBox(nsIFrame*, unsigned int,
> mozilla::gfx::BaseMatrix<double> const*)
> /builds/worker/workspace/build/src/layout/svg/nsSVGUtils.cpp:1187:20
> #40 0x7fdacf131a1c in ComputeClipForMaskItem
> /builds/worker/workspace/build/src/layout/generic/nsFrame.cpp:2686:22
> #41 0x7fdacf131a1c in
> nsIFrame::BuildDisplayListForStackingContext(nsDisplayListBuilder*,
> nsDisplayList*, bool*)
> /builds/worker/workspace/build/src/layout/generic/nsFrame.cpp:2997
> 
> Looks like botond wrote the code in question (ComputeClipForMaskItem),
> cc'ing him.
> 
> We should have flushed before display list building, so maybe we need to
> make sure we don't try to flush animations from this caller?

Ugh, we should really annotate the whole codebase with MOZ_CAN_RUN_SCRIPT :(
While I introduced this particular call to nsSVGUtils::GetBBox() in display list building code, it has existing call sites in FrameLayerBuilder (where a restyle is similarly bad); for example, nsDisplayMask::PaintMask() -> ComputeMaskGeometry() -> nsSVGMaskFrame::GetMaskArea() -> nsSVGUtils::GetBBox().

So, if nsSVGUtils::GetBBox() can trigger a restyle (as the stack trace in comment 3 indicates), then we have a pre-existing problem.
Flags: needinfo?(botond)
The way nsSVGUtils::GetBBox() triggers a restyle is that it calls SVGAnimatedEnumeration::AnimVal() [1], whose implementation (in DOMAnimatedEnum::AnimVal()) calls nsSVGElement::FlushAnimations() [2].

I note that a related security bug, bug 547333, was fixed in 2010, with the fix approach behind that "DOM getters" flush animations while "internal getters" do not.

Based on the change made in that patch, DOMAnimatedEnum::AnimVal() is a "DOM getter", while the function nsSVGEnum::GetAnimValue() that it delegates to, is an "internal getter".

So perhaps nsSVGUtils::GetBBox() should be using the internal getter rather than the DOM getter.

[1] https://searchfox.org/mozilla-central/rev/44fa24847e4e73ce8932de4c8cf47559302e4ba2/layout/svg/nsSVGUtils.cpp#1187
[2] https://searchfox.org/mozilla-central/rev/44fa24847e4e73ce8932de4c8cf47559302e4ba2/dom/svg/nsSVGEnum.h#97
(In reply to Botond Ballo [:botond] from comment #6)
> So perhaps nsSVGUtils::GetBBox() should be using the internal getter rather
> than the DOM getter.

Jonathan, does this sound reasonable?
Flags: needinfo?(jwatt)
(In reply to Botond Ballo [:botond] from comment #6)
> So perhaps nsSVGUtils::GetBBox() should be using the internal getter rather
> than the DOM getter.

That makes sense to me, yeah.

Specifically, we probably shouldn't be calling "clipContent->ClipPathUnits()" here (which returns a DOM-flavored object, with the AnimVal() "DOM getter").

We probably need to expose some way of getting at clipContent's mEnumAttributes[CLIPPATHUNITS] member-variable directly (the thing with type nsSVGEnum).

(We already directly grab this member-var for some usages -- e.g. in nsSVGClipPathFrame.cpp [1] -- though that requires "friend" access.)

Maybe we should just add a "bool SVGClipPathElement::IsUnitsObjectBoundingBox()", and call that here?  (And have it internally check mEnumAttributes[CLIPPATHUNITS].GetAnimValue() == SVG_UNIT_TYPE_OBJECTBOUNDINGBOX, and note that it does not flush style so it might be out of date if there's a pending style flush.)

[1] https://dxr.mozilla.org/mozilla-central/rev/f1965cf7425fe422c9e9c78018f11b97e0a0f229/layout/svg/nsSVGClipPathFrame.cpp#463-464
Created attachment 8957726 [details] [diff] [review]
Part 1 - Avoid calling SVGAnimatedEnumeration::AnimVal() from nsSVGUtils::GetBBox()

Thanks for the suggestion, :dholbert! This patch implements it.
Flags: needinfo?(jwatt)
Attachment #8957726 - Flags: review?(jwatt)
Created attachment 8957727 [details] [diff] [review]
Part 2 - Crashtest
Assignee: nobody → botond
Attachment #8957727 - Flags: review?(jwatt)
This change looks good in and of itself, but we should do some wide auditing here.
Flags: needinfo?(jwatt)
Attachment #8957726 - Flags: review?(jwatt) → review+
Attachment #8957727 - Flags: review?(jwatt) → review+
Comment on attachment 8957726 [details] [diff] [review]
Part 1 - Avoid calling SVGAnimatedEnumeration::AnimVal() from nsSVGUtils::GetBBox()

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

It's a UAF with a reproducing testcase, so I assume fairly easily.

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

The check-in comment provides some explanation, without spelling out the issue; I tried to find a middle ground between motivating the change and pinpointing the security issue.

If you prefer, I could remove the explanation from the check-in comment entirely, and require people to look at the comments in this bug instead.

> Which older supported branches are affected by this flaw?
> If not all supported branches, which bug introduced the flaw?

The issue appears to have been introduced in bug 999964, which landed in 2014. It may have been made more prevalent by bug 1382534, which landed in 58.

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

Not yet, but I can prepare backports easily; they would be the same as the fix for mainline.

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

There is a possibility of nsSVGUtils::GetBBox() now producing a stale value because it didn't flush styles. I don't expect that to be an issue for call sites during display list building or FrameLayerBuilder, since we flush styles earlier in the refresh driver tick. But it's conceivable some other call sites may have been (inadvertently) relying on this behaviour. I think that's fairly unlikely, and would be satisfied with seeing a green automation run, but :jwatt may have a more informed opinion here.
Attachment #8957726 - Flags: sec-approval?
Comment on attachment 8957727 [details] [diff] [review]
Part 2 - Crashtest

(See previous comment.)
Attachment #8957727 - Flags: sec-approval?
Flags: sec-bounty?
Sec-approval to land on March 27, two weeks into the next release cycle. We'll want to backport it to 60 and ESR52 at that time as well so we'll need nominations for patches after it lands.

Do not land before that. This is *not* approval for the test. That needs to land after we ship a release with this fix so we don't 0day ourselves with our test. I'll clear the sec-approval on that patch. You should probably set in-testsuite? as a reminder to land it later (or ni? yourself).
status-firefox59: ? → wontfix
status-firefox-esr52: --- → affected
tracking-firefox60: --- → +
tracking-firefox-esr52: --- → 60+
Whiteboard: [checkin on 3/27]
Attachment #8957726 - Flags: sec-approval? → sec-approval+
Attachment #8957727 - Flags: sec-approval?
(In reply to Botond Ballo [:botond] from comment #12)
> > How likely is this patch to cause regressions; how much testing does it need?
> 
> There is a possibility of nsSVGUtils::GetBBox() now producing a stale value
> because it didn't flush styles. I don't expect that to be an issue for call
> sites during display list building or FrameLayerBuilder, since we flush
> styles earlier in the refresh driver tick. But it's conceivable some other
> call sites may have been (inadvertently) relying on this behaviour. I think
> that's fairly unlikely, and would be satisfied with seeing a green
> automation run, but :jwatt may have a more informed opinion here.

I would be quite surprised if this causes any issues.
Flags: needinfo?(jwatt)
Needinfo'ing myself again to do some wider auditing.
Flags: needinfo?(jwatt)
https://hg.mozilla.org/integration/mozilla-inbound/rev/dcf29f1d4c4dda9b684ecdebf2557bb4a463a7f0
status-firefox61: --- → affected
tracking-firefox61: --- → +
Flags: in-testsuite?
Whiteboard: [checkin on 3/27]
Please request Beta approval on this when you get a chance. It grafts cleanly as-landed. It needs a rebased patch for ESR52, however.
https://hg.mozilla.org/mozilla-central/rev/dcf29f1d4c4d
Status: NEW → RESOLVED
Last Resolved: 11 months ago
status-firefox61: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
(In reply to Jonathan Watt [:jwatt] (back April 2nd) from comment #16)
> Needinfo'ing myself again to do some wider auditing.

I did that in bug 1448774. That bug contains patches for the only other two instances I could find.
Group: layout-core-security → core-security-release
Created attachment 8963236 [details] [diff] [review]
Uplift patch for beta

Approval Request Comment
[Feature/Bug causing the regression]:
  Bug 999964, made more prevalent by bug 1382534.

[User impact if declined]:
  Security risk (potentially exploitable UAF)

[Is this code covered by automated tests?]:
  Yes (but as per comment 14, the test will not land until 
  Firefox 61 is released).

[Has the fix been verified in Nightly?]:
  Yes (I downloaded ASAN builds from Treeherder to test).

[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?]:
  No.

[Why is the change risky/not risky?]:
  It's a small, targeted fix. See also comment 12.

[String changes made/needed]:
  None.
Attachment #8963236 - Flags: approval-mozilla-beta?
Created attachment 8963239 [details] [diff] [review]
Uplift patch for esr52

[Approval Request Comment]
See previous comment.
Attachment #8963239 - Flags: approval-mozilla-esr52?
Comment on attachment 8963236 [details] [diff] [review]
Uplift patch for beta

Approved for 60.0b8 and ESR 52.8.
Attachment #8963236 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8963239 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+

Comment 25

11 months ago
uplift
https://hg.mozilla.org/releases/mozilla-esr52/rev/b8c430253efd
status-firefox-esr52: affected → fixed
Flags: qe-verify+
I managed to reproduce the bug using Ubuntu 16.04 x 64 and an older version of ASAN build of Firefox 60.0a1 (2018-03-05). I got an error in the terminal and the tab crashed.

I verified the fix on the same platform using the latest ASAN build of Firefox 61.0a1, latest ASAN build of esr52, latest ASAN build of beta 60.0b9, latest Nightly 61.0a1 and beta 60.0b8 and the bug is not reproducing anymore. The tab doesn't crash and there are no errors thrown in the terminal.
Status: RESOLVED → VERIFIED
status-firefox60: fixed → verified
status-firefox61: fixed → verified
status-firefox-esr52: fixed → verified
Flags: qe-verify+
Flags: sec-bounty? → sec-bounty+
Whiteboard: [adv-main60+][adv-esr52.8+]
Alias: CVE-2018-5154
Comment on attachment 8957727 [details] [diff] [review]
Part 2 - Crashtest

Re-requesting sec-approval as per comment 14 (the fix was released today as part of Firefox 60).
Attachment #8957727 - Flags: sec-approval?
Comment on attachment 8957727 [details] [diff] [review]
Part 2 - Crashtest

sec-approval for landing on May 23 (two weeks into the cycle) so we have time to get uptake on the current release.
Attachment #8957727 - Flags: sec-approval? → sec-approval+
Whiteboard: [adv-main60+][adv-esr52.8+] → [adv-main60+][adv-esr52.8+][checkin on 5/23]
I believe the test patch is ready for landing as per comment 28.

(Do we usually uplift test patches to Beta and ESR too in a situation like this?)
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/24cfa380890604a502afaa388bee7e6587407fd2
Flags: needinfo?(jwatt)
Flags: in-testsuite?
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [adv-main60+][adv-esr52.8+][checkin on 5/23] → [adv-main60+][adv-esr52.8+]
(Restoring needinfo on :jwatt since, as per comment 16, that's for doing additional auditing as a follow-up.)
Flags: needinfo?(jwatt)
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.