Last Comment Bug 1292443 - (CVE-2016-5296) Heap-buffer-overflow WRITE in rasterize_edges_1
(CVE-2016-5296)
: Heap-buffer-overflow WRITE in rasterize_edges_1
Status: VERIFIED FIXED
[post-critsmash-triage][adv-main50+][...
: crash, csectype-bounds, csectype-undefined, sec-critical
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: Unspecified Unspecified
-- critical (vote)
: mozilla52
Assigned To: Jeff Muizelaar [:jrmuizel]
:
: Milan Sreckovic [:milan]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2016-08-04 22:40 PDT by Abhishek Arya
Modified: 2017-02-09 08:03 PST (History)
16 users (show)
dveditz: sec‑bounty+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
verified
+
verified
+
verified
50+
verified


Attachments
Testcase (4.65 KB, image/svg+xml)
2016-08-04 22:40 PDT, Abhishek Arya
no flags Details
testcase 2 (reduced) (413 bytes, application/xhtml+xml)
2016-08-10 14:34 PDT, Daniel Holbert [:dholbert] (vacation, returning 2/27)
no flags Details
testcase 3 (interactive -- hover blue area to test) (489 bytes, application/xhtml+xml)
2016-08-10 15:43 PDT, Daniel Holbert [:dholbert] (vacation, returning 2/27)
no flags Details
debugging patch v1 (2.62 KB, patch)
2016-08-10 15:49 PDT, Daniel Holbert [:dholbert] (vacation, returning 2/27)
no flags Details | Diff | Splinter Review
snippet of annotated disassembly (2.17 KB, text/plain)
2016-08-27 15:34 PDT, Daniel Holbert [:dholbert] (vacation, returning 2/27)
no flags Details
snippet of annotated disassembly #2 (3.16 KB, text/plain)
2016-08-29 14:29 PDT, Mats Palmgren (:mats)
no flags Details
Check for overflow. (1.07 KB, patch)
2016-10-06 10:20 PDT, Milan Sreckovic [:milan]
no flags Details | Diff | Splinter Review
Cast to unsigned for arithmetic (25.18 KB, patch)
2016-10-11 15:58 PDT, Jeff Muizelaar [:jrmuizel]
no flags Details | Diff | Splinter Review
Cast to unsigned for arithmetic (667 bytes, patch)
2016-10-13 14:06 PDT, Jeff Muizelaar [:jrmuizel]
dholbert: review+
dveditz: approval‑mozilla‑aurora+
dveditz: approval‑mozilla‑beta+
dveditz: approval‑mozilla‑esr45+
dveditz: sec‑approval+
Details | Diff | Splinter Review

Description User image Abhishek Arya 2016-08-04 22:40:57 PDT
Created attachment 8778096 [details]
Testcase

==13447==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6230000601a8 at pc 0x0000004bea3c bp 0x7ffeffd80c70 sp 0x7ffeffd80420
WRITE of size 4112 at 0x6230000601a8 thread T0 (Web Content)
SCARINESS: 55 (multi-byte-write-heap-buffer-overflow-far-from-bounds)
    #0 0x4bea3b in __asan_memset _asan_rtl_
    #1 0x7fd8de8e2b4c in rasterize_edges_1 gfx/cairo/libpixman/src/pixman-edge-imp.h:125:7
    #2 0x7fd8de8e2b4c in pixman_rasterize_edges_no_accessors gfx/cairo/libpixman/src/pixman-edge.c:351
    #3 0x7fd8de8e2b4c in _moz_pixman_rasterize_edges gfx/cairo/libpixman/src/pixman-edge.c:382
    #4 0x7fd8de99d824 in _moz_pixman_rasterize_trapezoid gfx/cairo/libpixman/src/pixman-trap.c:386:2
    #5 0x7fd8de7b1392 in _pixman_image_add_traps gfx/cairo/cairo/src/cairo-image-surface.c:2466:2
    #6 0x7fd8de7b0701 in _composite_traps gfx/cairo/cairo/src/cairo-image-surface.c:2516:5
    #7 0x7fd8de7b5b18 in _clip_and_composite gfx/cairo/cairo/src/cairo-image-surface.c:2359:15
    #8 0x7fd8de7b891f in _clip_and_composite_trapezoids gfx/cairo/cairo/src/cairo-image-surface.c:3258:12
    #9 0x7fd8de7b7d7d in _clip_and_composite_polygon gfx/cairo/cairo/src/cairo-image-surface.c:3625:15
    #10 0x7fd8de7a3ceb in _cairo_image_surface_stroke gfx/cairo/cairo/src/cairo-image-surface.c:3717:15
    #11 0x7fd8de81bce1 in _cairo_surface_stroke gfx/cairo/cairo/src/cairo-surface.c:2296:11
    #12 0x7fd8de796646 in _cairo_gstate_stroke gfx/cairo/cairo/src/cairo-gstate.c:1166:14
    #13 0x7fd8de83ea7d in _moz_cairo_stroke_preserve gfx/cairo/cairo/src/cairo.c:2430:14
    #14 0x7fd8d75bfbcf in mozilla::gfx::DrawTargetCairo::DrawPattern(mozilla::gfx::Pattern const&, mozilla::gfx::StrokeOptions const&, mozilla::gfx::DrawOptions const&, mozilla::gfx::DrawTargetCairo::DrawPatternType, bool) gfx/2d/DrawTargetCairo.cpp:1024:7
    #15 0x7fd8d75c4f81 in mozilla::gfx::DrawTargetCairo::Stroke(mozilla::gfx::Path const*, mozilla::gfx::Pattern const&, mozilla::gfx::StrokeOptions const&, mozilla::gfx::DrawOptions const&) gfx/2d/DrawTargetCairo.cpp:1255:3
    #16 0x7fd8dcd8bca6 in nsSVGPathGeometryFrame::Render(gfxContext*, unsigned int, gfxMatrix const&) layout/svg/nsSVGPathGeometryFrame.cpp:867:21
    #17 0x7fd8dcd8a20d in nsSVGPathGeometryFrame::PaintSVG(gfxContext&, gfxMatrix const&, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const*) layout/svg/nsSVGPathGeometryFrame.cpp:293:5
    #18 0x7fd8dcd88726 in nsDisplaySVGPathGeometry::Paint(nsDisplayListBuilder*, nsRenderingContext*) layout/svg/nsSVGPathGeometryFrame.cpp:125:51
    #19 0x7fd8dc5f5f98 in mozilla::FrameLayerBuilder::PaintItems(nsTArray<mozilla::FrameLayerBuilder::ClippedDisplayItem>&, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&, gfxContext*, nsRenderingContext*, nsDisplayListBuilder*, nsPresContext*, mozilla::gfx::IntPointTyped<mozilla::gfx::UnknownUnits> const&, float, float, int) layout/base/FrameLayerBuilder.cpp:5710:21
    #20 0x7fd8dc5f9433 in mozilla::FrameLayerBuilder::DrawPaintedLayer(mozilla::layers::PaintedLayer*, gfxContext*, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::layers::DrawRegionClip, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, void*) layout/base/FrameLayerBuilder.cpp:5885:19
    #21 0x7fd8d7940a51 in mozilla::layers::BasicPaintedLayer::PaintThebes(gfxContext*, mozilla::layers::Layer*, void (*)(mozilla::layers::PaintedLayer*, gfxContext*, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::layers::DrawRegionClip, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, void*), void*) gfx/layers/basic/BasicPaintedLayer.cpp:95:9
    #22 0x7fd8d793b3ec in mozilla::layers::BasicLayerManager::PaintSelfOrChildren(mozilla::layers::PaintLayerContext&, gfxContext*) gfx/layers/basic/BasicLayerManager.cpp:714:13
    #23 0x7fd8d7939c49 in mozilla::layers::BasicLayerManager::PaintLayer(gfxContext*, mozilla::layers::Layer*, void (*)(mozilla::layers::PaintedLayer*, gfxContext*, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::layers::DrawRegionClip, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, void*), void*) gfx/layers/basic/BasicLayerManager.cpp:883:5
    #24 0x7fd8d793b0b2 in mozilla::layers::BasicLayerManager::PaintSelfOrChildren(mozilla::layers::PaintLayerContext&, gfxContext*) gfx/layers/basic/BasicLayerManager.cpp:735:7
    #25 0x7fd8d7939c49 in mozilla::layers::BasicLayerManager::PaintLayer(gfxContext*, mozilla::layers::Layer*, void (*)(mozilla::layers::PaintedLayer*, gfxContext*, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::layers::DrawRegionClip, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, void*), void*) gfx/layers/basic/BasicLayerManager.cpp:883:5
    #26 0x7fd8d7934f1d in mozilla::layers::BasicLayerManager::EndTransactionInternal(void (*)(mozilla::layers::PaintedLayer*, gfxContext*, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::layers::DrawRegionClip, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, void*), void*, mozilla::layers::LayerManager::EndTransactionFlags) gfx/layers/basic/BasicLayerManager.cpp:623:5
    #27 0x7fd8dc5f5fb6 in PaintInactiveLayer layout/base/FrameLayerBuilder.cpp:3605:12
    #28 0x7fd8dc5f5fb6 in mozilla::FrameLayerBuilder::PaintItems(nsTArray<mozilla::FrameLayerBuilder::ClippedDisplayItem>&, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&, gfxContext*, nsRenderingContext*, nsDisplayListBuilder*, nsPresContext*, mozilla::gfx::IntPointTyped<mozilla::gfx::UnknownUnits> const&, float, float, int) layout/base/FrameLayerBuilder.cpp:5696
    #29 0x7fd8dc5f9433 in mozilla::FrameLayerBuilder::DrawPaintedLayer(mozilla::layers::PaintedLayer*, gfxContext*, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::layers::DrawRegionClip, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, void*) layout/base/FrameLayerBuilder.cpp:5885:19
    #30 0x7fd8d79536e2 in mozilla::layers::ClientPaintedLayer::PaintThebes() gfx/layers/client/ClientPaintedLayer.cpp:94:5
    #31 0x7fd8d795437d in mozilla::layers::ClientPaintedLayer::RenderLayerWithReadback(mozilla::layers::ReadbackProcessor*) gfx/layers/client/ClientPaintedLayer.cpp:148:3
    #32 0x7fd8d796dd8c in mozilla::layers::ClientContainerLayer::RenderLayer() gfx/layers/client/ClientContainerLayer.h:65:29
    #33 0x7fd8d796dd8c in mozilla::layers::ClientContainerLayer::RenderLayer() gfx/layers/client/ClientContainerLayer.h:65:29
    #34 0x7fd8d796dd8c in mozilla::layers::ClientContainerLayer::RenderLayer() gfx/layers/client/ClientContainerLayer.h:65:29
    #35 0x7fd8d796dd8c in mozilla::layers::ClientContainerLayer::RenderLayer() gfx/layers/client/ClientContainerLayer.h:65:29
    #36 0x7fd8d796dd8c in mozilla::layers::ClientContainerLayer::RenderLayer() gfx/layers/client/ClientContainerLayer.h:65:29
    #37 0x7fd8d796dd8c in mozilla::layers::ClientContainerLayer::RenderLayer() gfx/layers/client/ClientContainerLayer.h:65:29
    #38 0x7fd8d796dd8c in mozilla::layers::ClientContainerLayer::RenderLayer() gfx/layers/client/ClientContainerLayer.h:65:29
    #39 0x7fd8d794e89e in mozilla::layers::ClientLayerManager::EndTransactionInternal(void (*)(mozilla::layers::PaintedLayer*, gfxContext*, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::layers::DrawRegionClip, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, void*), void*, mozilla::layers::LayerManager::EndTransactionFlags) gfx/layers/client/ClientLayerManager.cpp:296:9
    #40 0x7fd8d794ee7c in mozilla::layers::ClientLayerManager::EndTransaction(void (*)(mozilla::layers::PaintedLayer*, gfxContext*, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::layers::DrawRegionClip, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, void*), void*, mozilla::layers::LayerManager::EndTransactionFlags) gfx/layers/client/ClientLayerManager.cpp:339:3
    #41 0x7fd8dc74a3db in nsDisplayList::PaintRoot(nsDisplayListBuilder*, nsRenderingContext*, unsigned int) layout/base/nsDisplayList.cpp:1927:17
    #42 0x7fd8dc7fb0d0 in nsLayoutUtils::PaintFrame(nsRenderingContext*, nsIFrame*, nsRegion const&, unsigned int, nsDisplayListBuilderMode, nsLayoutUtils::PaintFrameFlags) layout/base/nsLayoutUtils.cpp:3606:10
    #43 0x7fd8dc8835ad in PresShell::Paint(nsView*, nsRegion const&, unsigned int) layout/base/nsPresShell.cpp:6636:5
    #44 0x7fd8dbe4bf5c in nsViewManager::ProcessPendingUpdatesPaint(nsIWidget*) view/nsViewManager.cpp:484:19
    #45 0x7fd8dbe4ad31 in nsViewManager::ProcessPendingUpdatesForView(nsView*, bool) view/nsViewManager.cpp:415:33
    #46 0x7fd8dbe4ecc9 in nsViewManager::ProcessPendingUpdates() view/nsViewManager.cpp:1119:5
    #47 0x7fd8dc57654f in nsRefreshDriver::Tick(long, mozilla::TimeStamp) layout/base/nsRefreshDriver.cpp:1911:9
    #48 0x7fd8dc58109d in TickDriver layout/base/nsRefreshDriver.cpp:279:13
    #49 0x7fd8dc58109d in mozilla::RefreshDriverTimer::TickRefreshDrivers(long, mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&) layout/base/nsRefreshDriver.cpp:251
    #50 0x7fd8dc580cf9 in mozilla::RefreshDriverTimer::Tick(long, mozilla::TimeStamp) layout/base/nsRefreshDriver.cpp:270:5
    #51 0x7fd8dc582c94 in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::NotifyVsync(mozilla::TimeStamp) layout/base/nsRefreshDriver.cpp:430:9
    #52 0x7fd8dcf0c2c4 in mozilla::layout::VsyncChild::RecvNotify(mozilla::TimeStamp const&) layout/ipc/VsyncChild.cpp:64:16
    #53 0x7fd8d6823059 in mozilla::layout::PVsyncChild::OnMessageReceived(IPC::Message const&) objdir-ff-asan/ipc/ipdl/PVsyncChild.cpp:240:20
    #54 0x7fd8d62a1814 in mozilla::ipc::PBackgroundChild::OnMessageReceived(IPC::Message const&) objdir-ff-asan/ipc/ipdl/PBackgroundChild.cpp:2133:28
    #55 0x7fd8d61cd566 in mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&) ipc/glue/MessageChannel.cpp:1661:25
    #56 0x7fd8d61c9c87 in mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&) ipc/glue/MessageChannel.cpp:1599:17
    #57 0x7fd8d61b6dff in mozilla::ipc::MessageChannel::OnMaybeDequeueOne() ipc/glue/MessageChannel.cpp:1566:5
    #58 0x7fd8d61e8152 in applyImpl<mozilla::ipc::MessageChannel, bool (mozilla::ipc::MessageChannel::*)()> objdir-ff-asan/dist/include/nsThreadUtils.h:729:12
    #59 0x7fd8d61e8152 in apply<mozilla::ipc::MessageChannel, bool (mozilla::ipc::MessageChannel::*)()> objdir-ff-asan/dist/include/nsThreadUtils.h:735
    #60 0x7fd8d61e8152 in mozilla::detail::RunnableMethodImpl<bool (mozilla::ipc::MessageChannel::*)(), false, true>::Run() objdir-ff-asan/dist/include/nsThreadUtils.h:764
    #61 0x7fd8d61e773f in Run objdir-ff-asan/dist/include/mozilla/ipc/MessageChannel.h:550:29
    #62 0x7fd8d61e773f in mozilla::ipc::MessageChannel::DequeueTask::Run() objdir-ff-asan/dist/include/mozilla/ipc/MessageChannel.h:569
    #63 0x7fd8d53e8802 in nsThread::ProcessNextEvent(bool, bool*) xpcom/threads/nsThread.cpp:1068:14
    #64 0x7fd8d54684d8 in NS_ProcessNextEvent(nsIThread*, bool) xpcom/glue/nsThreadUtils.cpp:290:10
    #65 0x7fd8d61d4d51 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) ipc/glue/MessagePump.cpp:100:21
    #66 0x7fd8d61478f0 in RunInternal ipc/chromium/src/base/message_loop.cc:232:10
    #67 0x7fd8d61478f0 in RunHandler ipc/chromium/src/base/message_loop.cc:225
    #68 0x7fd8d61478f0 in MessageLoop::Run() ipc/chromium/src/base/message_loop.cc:205
    #69 0x7fd8dbec0e2f in nsBaseAppShell::Run() widget/nsBaseAppShell.cpp:156:27
    #70 0x7fd8ddfe49c7 in XRE_RunAppShell toolkit/xre/nsEmbedFunctions.cpp:851:22
    #71 0x7fd8d61478f0 in RunInternal ipc/chromium/src/base/message_loop.cc:232:10
    #72 0x7fd8d61478f0 in RunHandler ipc/chromium/src/base/message_loop.cc:225
    #73 0x7fd8d61478f0 in MessageLoop::Run() ipc/chromium/src/base/message_loop.cc:205
    #74 0x7fd8ddfe4088 in XRE_InitChildProcess toolkit/xre/nsEmbedFunctions.cpp:681:34
    #75 0x510ae1 in content_process_main ipc/contentproc/plugin-container.cpp:224:19
    #76 0x510ae1 in main browser/app/nsBrowserApp.cpp:357
    #77 0x7fd8ef154f44 in __libc_start_main /build/eglibc-oGUzwX/eglibc-2.19/csu/libc-start.c:287
AddressSanitizer can not describe address in more detail (wild memory access suspected).
SUMMARY: AddressSanitizer: heap-buffer-overflow (/mnt/scratch0/clusterfuzz/slave-bot/builds/linux_asan_firefox/custom/firefox/firefox-bin+0x4bea3b)
Shadow bytes around the buggy address:
  0x0c4680003fe0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c4680003ff0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c4680004000: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c4680004010: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c4680004020: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
=>0x0c4680004030: fa fa fa fa fa[fa]fa fa fa fa fa fa fa fa fa fa
  0x0c4680004040: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c4680004050: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c4680004060: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c4680004070: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c4680004080: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:       fa
  Heap right redzone:      fb
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack partial redzone:   f4
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==13447==ABORTING
Comment 1 User image Daniel Veditz [:dveditz] 2016-08-08 09:57:43 PDT
Daniel: please direct this bug to someone in the SVG team that can work on it.
Comment 2 User image Daniel Holbert [:dholbert] (vacation, returning 2/27) 2016-08-08 23:44:43 PDT
I'll take a look to start out... Abishek, could you let me know what build/OS you're using?

I can't reproduce any ASAN issues on linux, using the latest inbound ASAN build:
http://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-inbound-linux64-asan-debug/1470715521/

I just get this when loading the attached file:
> JavaScript error: file:///...path-to-file.../test.svg, line 15: SyntaxError: expected expression, got '}'
but no ASAN errors.
Comment 3 User image Abhishek Arya 2016-08-09 17:15:01 PDT
20160807213804 was previous build i used. i just tried with tip-of-tree trunk (6cf0089510fa) and easily reproduces with that svg file. you need to use release build. also my clang revision is r277962. and this was on linux.
Comment 4 User image Daniel Holbert [:dholbert] (vacation, returning 2/27) 2016-08-10 10:20:20 PDT
(In reply to Abhishek Arya from comment #3)
> you need to use release build.

(I assume you mean an optimized build [not a build with release branding] -- gotcha.)

> also my clang revision is r277962. and this was on linux.

OK, thanks. Have you tested with a Mozilla-provided ASAN-enabled optimized Firefox build, like this one from today:
https://ftp.mozilla.org/pub/firefox/tinderbox-builds/mozilla-central-linux64-asan/1470837316/firefox-51.0a1.en-US.linux-x86_64-asan.tar.bz2
?

I just tried that one and still can't reproduce.  I'm building locally as well, but given that I've failed to repro with 2 different types of ASAN builds so far, I'm wondering if there's another factor that I'm missing. (And if you could test [or let me know if you've already tested] a build like the FTP one above, that would rule out the possibility of something special in your build environment.)
Comment 5 User image Daniel Holbert [:dholbert] (vacation, returning 2/27) 2016-08-10 10:28:55 PDT
I just finished & tested my own local [enable-debug,enable-opt] ASAN Firefox build, as well -- couldn't repro any ASAN issues. I happen to be using clang from r278176 (~trunk), and mozilla-central c12bb83ad278 (also ~trunk).

I'll try Abhishek's clang/m-c revision next, to rule out the possibility of any differences there. But I suspect there's some other factor that I'm missing here.
Comment 6 User image Daniel Holbert [:dholbert] (vacation, returning 2/27) 2016-08-10 11:53:10 PDT
(In reply to Daniel Holbert [:dholbert] (recovering from vacation reviews/bugmail) from comment #5)
> I'll try Abhishek's clang/m-c revision next, to rule out the possibility of
> any differences there.

OK, I've found that I can reproduce with those revisions.
Comment 7 User image Daniel Holbert [:dholbert] (vacation, returning 2/27) 2016-08-10 14:34:18 PDT
Created attachment 8779869 [details]
testcase 2 (reduced)

Here's a reduced / cleaned-up testcase that still triggers the crash for me (using a debug+opt ASAN-enabled build with the mozilla-central/clang revisions noted in comment 3).
Comment 8 User image Daniel Holbert [:dholbert] (vacation, returning 2/27) 2016-08-10 14:34:38 PDT
er, s/still triggers the crash/still triggers the ASAN report/abort.
Comment 9 User image Daniel Holbert [:dholbert] (vacation, returning 2/27) 2016-08-10 15:43:47 PDT
Created attachment 8779903 [details]
testcase 3 (interactive -- hover blue area to test)
Comment 10 User image Daniel Holbert [:dholbert] (vacation, returning 2/27) 2016-08-10 15:49:13 PDT
Created attachment 8779906 [details] [diff] [review]
debugging patch v1

Here's a debugging/diagnostic patch which demonstrates what the problem is, on my machine.

It seems the compiler is assuming (incorrectly in this case) that signed integer overflow will never happen. IIRC, that is something that compilers are allowed to assume.  We end up with an (overflowed) hugely-negative number, which the code *purportedly* clamps to be nonnegative -- BUT in actual fact, we skip the "lx < 0" check (and the clamping code) because the compiler is mistakenly assuming the value is still positive.

Here's the tail end of the printf output from this patch, leading up to where we get killed by ASAN:
{
Initial lx: 2147183239
Adding 32767 to lx
Do we think lx=2147216006 is negative & needs clamping? No.
Initial lx: 2147254082
Adding 32767 to lx
Do we think lx=2147286849 is negative & needs clamping? No.
Initial lx: 2147324924
Adding 32767 to lx
Do we think lx=2147357691 is negative & needs clamping? No.
Initial lx: 2147395766
Adding 32767 to lx
Do we think lx=2147428533 is negative & needs clamping? No.
Initial lx: 2147466609
Adding 32767 to lx
OH NO, we're about to wrap lx past INT32_MAX! Upcoming clamp might fail to catch this...
Do we think lx=-2147467920 is negative & needs clamping? No.
***dholbert Calling WRITE with nmiddle=1029 (x=0, width=32964, startmask=0, endmask=15) (rx=12865573, lx=-2147467920) (rxi=196, lxi=-32768)
=================================================================
==12873==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x623000122378 at pc 0x7f5c6c3238c5 bp 0x7fffa9076e30 sp 0x7fffa9076e28
}

Note in particular the final "Do we think lx=-2147467920 is negative...No" report there. :-/  That's the problem.
Comment 11 User image Daniel Holbert [:dholbert] (vacation, returning 2/27) 2016-08-10 16:14:26 PDT
This particular chunk of code (the addition to & clamping of "lx") hasn't changed since a pixman update that happened in bug 562087 (back in 2010), BTW.  And it looks like we had a version of this bug (potential for overflow & failure-to-clamp) before that, too.

This bug exists in the latest version of the pixman code that I can find, too  (i.e. it may trigger signed-overflow and then fail to clamp a negative |lx| value):
> 	lx += X_FRAC_FIRST(1) - pixman_fixed_e;
> 	rx += X_FRAC_FIRST(1) - pixman_fixed_e;
> #endif
> 	/* clip X */
> 	if (lx < 0)
> 	    lx = 0;
https://cgit.freedesktop.org/pixman/tree/pixman/pixman-edge-imp.h#n58

Ideally this should get fixed in pixman, but that's perhaps pretty fragile... Perhaps we can also add some sanity-checking in the code that produces such large |lx| values, to clamp it such that it can never get this close to INT32_MAX.
Comment 12 User image Daniel Holbert [:dholbert] (vacation, returning 2/27) 2016-08-19 14:57:14 PDT
Jeff, maybe this falls under your wheelhouse, as being a bug in cairo... do you have any ideas on what we should do here?  Perhaps we should add a spot fix here to check that lx and rx aren't going to overflow *before* we do the addition? (since checking for after the overflow already happened isn't guaranteed to work, due to compiler optimizations)

See my description of the bug in comment 10. As shown in the output there, this value |lx| is overflowing into negative territory, and it's used to determine where we write some memory, so we end up writing into hugely negative memory.
Comment 13 User image Daniel Holbert [:dholbert] (vacation, returning 2/27) 2016-08-23 07:55:40 PDT
>so we end up writing into hugely negative memory.

(By which I meant to say: we end up writing at a hugely negative offset from some pointer, i.e. at a bogus location.)
Comment 14 User image Jeff Muizelaar [:jrmuizel] 2016-08-25 15:50:13 PDT
(In reply to Daniel Holbert [:dholbert] from comment #11)
I'd like to understand how the compiler is dropping the check. Can you perhaps post the annotated assembly?
Comment 15 User image Daniel Holbert [:dholbert] (vacation, returning 2/27) 2016-08-27 15:34:24 PDT
Created attachment 8785631 [details]
snippet of annotated disassembly

Here's a snippet of the annotated disassembly of "rasterize_edges_1".

The source annotation is missing some source code -- it just shows us assigning |lx| and then checking if it's less than 0, and it's missing the lines of addition that I quoted in comment 11 (adding X_FRAC_FIRST(1) - pixman_fixed_e)

But I think that addition is present in the annotation - I do see an "add" instruction in between the bunch of assembly between the assignment & the less-than-zero-check, at least.
Comment 16 User image Mats Palmgren (:mats) 2016-08-29 14:29:16 PDT
Created attachment 8786096 [details]
snippet of annotated disassembly #2

I see different assembler from Daniel's.
It appears this code essentially does this instead:

	if ("original lx before the increment" < -32767)
	    lx = 0;

which clearly ignores any overflow from the addition.
(this is an Opt ASAN build on 64-bit Linux with clang 3.8.0)
Comment 17 User image Mats Palmgren (:mats) 2016-08-29 14:52:08 PDT
If I use an unsigned type to do the addition then it seems to work,
presumably because unsigned overflow is well-defined:

-       lx += X_FRAC_FIRST(1) - pixman_fixed_e;
-       rx += X_FRAC_FIRST(1) - pixman_fixed_e;
+       uint32_t lx_unsigned = lx;
+       lx = lx_unsigned + (X_FRAC_FIRST(1) - pixman_fixed_e);
+       uint32_t rx_unsigned = rx;
+       rx = rx_unsigned + (X_FRAC_FIRST(1) - pixman_fixed_e);

I'm not sure how we typically fix stuff like this though.
Any suggestions?
Comment 18 User image Mats Palmgren (:mats) 2016-08-31 11:29:54 PDT
Reassigning to Graphics since the underlying bug is in Cairo.
Comment 19 User image Andrew McCreight [:mccr8] 2016-09-07 10:16:35 PDT
Milan is there somebody who can look at this? This externally reported sec-critical bug is about a month old. Thanks!
Comment 20 User image Milan Sreckovic [:milan] 2016-09-07 11:19:50 PDT
Does this actually happen with non-ASAN builds?  I couldn't quite tell from the different comments.
Comment 21 User image Ryan VanderMeulen [:RyanVM] 2016-09-07 11:53:22 PDT
On Windows, the attached testcases show rendering issues with hardware acceleration disabled, but they don't crash. No asserts with debug builds. However, the switch to the Skia backend for unaccelerated layers (bug 1007702) has made this go away on m-c tip, i.e. rendering now matches the accelerated rendering.

Per dholbert on IRC, the root pixman issue has probably been around since ever, so removing the regressionwindow-wanted keyword.
Comment 22 User image Daniel Holbert [:dholbert] (vacation, returning 2/27) 2016-09-07 12:00:46 PDT
(In reply to Milan Sreckovic [:milan] from comment #20)
> Does this actually happen with non-ASAN builds?  I couldn't quite tell from
> the different comments.

I'm pretty sure. Since it depends on a compiler optimization, this does require optimizations to be enabled (i.e. not the sort of build we normally compile/test locally).  I don't have an enable-optimize build handy, so I can't immediately check, but I'm rebuilding & can confirm soon.

My attached diagnostic patch should make it easy to detect if/when this bug happens, in any build (particularly opt builds).  The key "printf" output to look for is something like this:
> OH NO, we're about to wrap lx past INT32_MAX! Upcoming clamp might fail to catch this...
> Do we think lx=-2147467920 is negative & needs clamping? No.

"lx" is used as an assumed-to-be-positive offset for a write shortly after that point (and that's the dangerous/exploitable part).
Comment 23 User image Daniel Holbert [:dholbert] (vacation, returning 2/27) 2016-09-07 13:02:30 PDT
OK -- Milan, I can confirm that normal (non-ASAN) optimized builds **are indeed affected by this bug**, using my diagnostic patch.   (I compiled my local opt build using Ubuntu 16.04's stock clang 3.8 version, if it matters.)

With my diagnostic patch applied, I see this go by in my terminal output when I load & hover testcase 3 (after grepping for "OH NO" to narrow in on the bad part):
> Adding 32767 to lx
> OH NO, we're about to wrap lx past INT32_MAX! Upcoming clamp might fail to catch this...
> Do we think lx=-2147467920 is negative & needs clamping? No.
> ***dholbert Calling WRITE with nmiddle=1029 (x=0, width=32964, startmask=0, endmask=15) (rx=12865573, lx=-2147467920) (rxi=196, lxi=-32768)

Note that I had to manually enable cairo as my rendering backend here (by setting gfx.content.azure.backends=cairo) in order to enable this codepath. We recently switched to prefer/mandate skia on trunk, in bug 1278957.  But we're still shipping cairo in version 49 and 50, and as a fallback renderer on Windows, so we do still need to care about it I think.
Comment 24 User image Milan Sreckovic [:milan] 2016-09-15 12:29:22 PDT
I'm particularly enjoying this from comment 23:

Do we think lx=-2147467920 is negative & needs clamping? No.

Yikes.
Comment 25 User image Daniel Holbert [:dholbert] (vacation, returning 2/27) 2016-09-15 12:34:17 PDT
Yeah, that's the problem. And then we use that as an (assumed-to-be-nonnegative) offset into a buffer, and we write data into that location.

Mats' suggestion in comment 17 seems like a reasonable workaround to me, FWIW (just doing the arithmetic in unsigned space and then casting back to signed).
Comment 26 User image Milan Sreckovic [:milan] 2016-09-15 14:30:13 PDT
I don't mind changing the code, in order to fix this particular bug, but what are the chances of this being the only place where something like that happens?

It'd be interesting to try if -fwrapv option changes the behaviour at all.
Comment 27 User image Daniel Holbert [:dholbert] (vacation, returning 2/27) 2016-09-15 14:59:21 PDT
(In reply to Milan Sreckovic [:milan] from comment #26)
> I don't mind changing the code, in order to fix this particular bug, but
> what are the chances of this being the only place where something like that
> happens?

I'm worried about that, too. :/  I was hoping someone who knows the cairo code (Jeff?) might have a guess about (or might be able to quickly audit for) the presence of other instances of this pattern that we'd need to fix at the same time.

We do probably want to report this bug to upstream cairo developers, too -- do we have a contact who works on cairo who we could CC here for thoughts/expertise?
Comment 28 User image Milan Sreckovic [:milan] 2016-09-16 11:10:56 PDT
On the upstream note - this code is still the same in the latest pixman from what I can see.  Also, does this https://gcc.gnu.org/onlinedocs/gcc/Integer-Overflow-Builtins.html apply here?  We're not currently using it, and it could be GCC only.
Comment 29 User image Liz Henry (:lizzard) (needinfo? me) 2016-10-05 10:14:44 PDT
Milan, what do you think should happen going forward? Looks like there is some ambiguity about how to best approach this. But it is a sec critical issue so we don't want to let it sit too long.
Comment 30 User image Milan Sreckovic [:milan] 2016-10-06 10:06:56 PDT
Yes, we're not completely sure what to do with this as a general issue, though perhaps could deal with it in this instance.  Also, as of 52, we're moving to Skia where the problem goes away.  Still, let me see if we can do something.
Comment 31 User image Milan Sreckovic [:milan] 2016-10-06 10:20:25 PDT
Created attachment 8798497 [details] [diff] [review]
Check for overflow.

Not sure this is worth it (see my previous comment), but should take care of this particular instance of a problem.  Don't know what kind of performance impact it may have.
Comment 32 User image Jeff Muizelaar [:jrmuizel] 2016-10-11 15:58:00 PDT
Created attachment 8800006 [details] [diff] [review]
Cast to unsigned for arithmetic

I'd prefer this as this code is somewhat performance sensitive and this seems like it might be more palatable to upstream.
Comment 33 User image Milan Sreckovic [:milan] 2016-10-13 11:57:08 PDT
(In reply to Jeff Muizelaar [:jrmuizel] from comment #32)
> Created attachment 8800006 [details] [diff] [review]
> Cast to unsigned for arithmetic
> 
> I'd prefer this as this code is somewhat performance sensitive and this
> seems like it might be more palatable to upstream.

Pretty sure you attached a wrong patch.
Comment 34 User image Jeff Muizelaar [:jrmuizel] 2016-10-13 14:06:26 PDT
Created attachment 8800854 [details] [diff] [review]
Cast to unsigned for arithmetic
Comment 35 User image Al Billings [:abillings] 2016-10-19 10:14:49 PDT
Liz or Ritu, will this be able to make beta if we have a reviewed patch?
Comment 36 User image Ritu Kothari (:ritu) 2016-10-19 11:01:49 PDT
(In reply to Al Billings [:abillings] from comment #35)
> Liz or Ritu, will this be able to make beta if we have a reviewed patch?

Hi Al, given that this is a sec-crit, yes we can take it in Beta. I hope it lands today/tomm am PST so it gets included in 50.0b9 (gtb Thursday noon PST).
Comment 37 User image Daniel Holbert [:dholbert] (vacation, returning 2/27) 2016-10-19 12:28:08 PDT
Comment on attachment 8800854 [details] [diff] [review]
Cast to unsigned for arithmetic

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

Not sure if you meant to request review on this latest patch, but in the interests of getting this landed, I'll jump in & mark this r=me, assuming:
 - you've tested it locally & made sure it does fix the overflow.
 - you add a sane commit message that doesn't give away too much.
Comment 38 User image Daniel Holbert [:dholbert] (vacation, returning 2/27) 2016-10-19 12:29:18 PDT
Comment on attachment 8798497 [details] [diff] [review]
Check for overflow.

(I think this earlier patch is obsolete, too --> marking as such.)
Comment 39 User image Mats Palmgren (:mats) 2016-10-19 13:37:43 PDT
Comment on attachment 8800854 [details] [diff] [review]
Cast to unsigned for arithmetic

Looks like Jeff isn't online currently, so I'll jump-start the approvals...

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

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

Which older supported branches are affected by this flaw?
All, most likely.

If not all supported branches, which bug introduced the flaw?
N/A

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
This patch should apply to all branches.

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


Approval Request Comment
[Feature/regressing bug #]: -
[User impact if declined]: sec-critical
[Describe test coverage new/current, TreeHerder]: I suspect this code is covered by a lot of tests in general (although not with the extreme input values that the testcases on this bug creates).  As usual, we'll wait with landing these tests until the bug is public.
[Risks and why]: very low risc
[String/UUID change made/needed]: none
Comment 40 User image Ritu Kothari (:ritu) 2016-10-19 15:44:49 PDT
I'll wait for a sec-approval+ before taking it in beta50 and aurora51. 

If someone can assess whether esr45 is affected or not, that would be great!
Comment 41 User image Ryan VanderMeulen [:RyanVM] 2016-10-19 15:47:09 PDT
It's a really old issue in pixman, so I'm about 99% sure that esr45 is also affected.
Comment 42 User image Daniel Holbert [:dholbert] (vacation, returning 2/27) 2016-10-19 16:03:05 PDT
Yup -- per comment 11, we've had this bug (via this chunk of pixman code) since 2010 or earlier, so esr45 is definitely affected.
Comment 43 User image Ritu Kothari (:ritu) 2016-10-20 10:30:45 PDT
Thanks Dan and Ryan. Hello Mats, could you please nominate a patch for uplift to ESR45 as well? Thanks!
Comment 44 User image Daniel Holbert [:dholbert] (vacation, returning 2/27) 2016-10-20 10:40:01 PDT
(nominated. I would've done so sooner, in comment 42, but I misread the flags that RyanVM had tweaked & thought he'd already nominated it.)
Comment 45 User image Daniel Veditz [:dveditz] 2016-10-20 11:22:12 PDT
Comment on attachment 8800854 [details] [diff] [review]
Cast to unsigned for arithmetic

sec-approval+
a=dveditz/ritu for aurora, beta, and esr45
Comment 48 User image Ryan VanderMeulen [:RyanVM] 2016-10-21 14:29:35 PDT
https://hg.mozilla.org/mozilla-central/rev/fbd443ce8fb5
Comment 49 User image Ciprian Georgiu, QA [:ciprian_georgiu] 2016-11-08 02:56:46 PST
I've managed to reproduce this bug with an inbound ASAN build (2016-08-04) and using testcase 3 on Ubuntu 16.04 x64 LTS.

This issue is verified fixed on latest Nightly 52.0a1 (2016-11-07) and latest Aurora 51.a2 (2016-11-07) using the same platform mentioned above. I can no longer reproduce the crash and I don't see any rendering issues.

 -- On 45.5.0 esr (20161031153904) I cannot see the testcase 3 (interactive -- hover blue area to test), the page appears blank when I load the testcase. You can see here: https://goo.gl/zNp9xF  
 -- On 50.0-build2 (20161104212021) there are some rendering issues but the tab is not crashing. See here: https://goo.gl/J6G0YX


Jeff, Wes what do you think about this results in 45.5.0 esr and 50 RC2?
Comment 50 User image Ciprian Georgiu, QA [:ciprian_georgiu] 2016-11-08 23:42:19 PST
 > Jeff, Wes what do you think about this results in 45.5.0 esr and 50 RC2? (comment 49)
Comment 51 User image Milan Sreckovic [:milan] 2016-11-10 10:39:03 PST
I'm not worried about 45.5 and 50 results in the context of this bug.  Looks like the crash got fixed.  If those problems happen in those versions, we should probably open another bug.
Comment 52 User image Ciprian Georgiu, QA [:ciprian_georgiu] 2016-11-23 04:07:03 PST
Thank you, Milan. I will close this issue as verified fixed since I can no longer reproduce the crashes on 45.5.0 esr, 50, 51 beta 2 and 52 latest Aurora.

Note You need to log in before you can comment on or make changes to this bug.