1268854 - (CVE-2016-5252) stack-buffer-overflow in mozilla::gfx::BasePoint4d

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Georg Koppen]

Attachment: gfx_stackbuffer

While visiting team-cymru.org and having the website open for a while I get a stack-buffer-overflow in gfx code with my ASan build. This is reproducible for me using an 45.1.0esr based Tor Browser on a Linux 64bit machine. We don't have patches for that part of the code, thus I guess this should be reproducible with a vanilla Firefox 45.1.0 as well at least. Attached is the stack trace.

Comment 1

[Georg Koppen]

I just verified that a vanilla Firefox based on the esr45 branch is affected as well. Steps to reproduce:

1) Build Firefox 45.1.0esr (I used commit 2747353bc358b8ee234fea20821b0372f5312d5a from the gecko-dev repo) using the following .mozconfig:

. $topsrcdir/browser/config/mozconfig

export CFLAGS="-fsanitize=address -Dxmalloc=myxmalloc -fwrapv"
export CXXFLAGS="-fsanitize=address -Dxmalloc=myxmalloc -fwrapv"
# We need to add -ldl explicitely due to bug 1213698
export LDFLAGS="-fsanitize=address -ldl"

mk_add_options MOZ_OBJDIR=@TOPSRCDIR@/obj-@CONFIG_GUESS@
mk_add_options MOZ_APP_DISPLAYNAME="Tor Browser"
mk_add_options MOZILLA_OFFICIAL=1
mk_add_options BUILD_OFFICIAL=1

ac_add_options --enable-address-sanitizer
ac_add_options --disable-jemalloc
ac_add_options --disable-elf-hack

ac_add_options --enable-optimize
ac_add_options --enable-official-branding

ac_add_options --disable-strip
ac_add_options --disable-install-strip
ac_add_options --disable-tests
ac_add_options --disable-debug
ac_add_options --disable-maintenance-service
ac_add_options --disable-crashreporter
ac_add_options --disable-webrtc
ac_add_options --disable-eme

I did this on an up-to-date Debian testing system

2) Start it with a new profile and visit team-cymru.org

3) Wait a while (I just hovered from time to time over the menu items but did not click anything)

4) Your ASan build will crash giving you a stacktrace like the following (looks nicer than the one I attached):

=================================================================
==2563==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7ffd1aa481b0 at pc 0x7f42bed958b0 bp 0x7ffd1aa480a0 sp 0x7ffd1aa48098
READ of size 8 at 0x7ffd1aa481b0 thread T0
    #0 0x7f42bed958af in mozilla::gfx::BasePoint4D<double, mozilla::gfx::Point4DTyped<mozilla::gfx::UnknownUnits, double> >::DotProduct(mozilla::gfx::Point4DTyped<mozilla::gfx::UnknownUnits, double> const&) const ../../dist/include/mozilla/gfx/BasePoint4D.h:101
    #1 0x7f42bed958af in unsigned long mozilla::gfx::Matrix4x4Typed<mozilla::gfx::UnknownUnits, mozilla::gfx::UnknownUnits>::TransformAndClipRect<double>(mozilla::gfx::RectTyped<mozilla::gfx::UnknownUnits, double> const&, mozilla::gfx::RectTyped<mozilla::gfx::UnknownUnits, double> const&, mozilla::gfx::PointTyped<mozilla::gfx::UnknownUnits, double>*) const ../../dist/include/mozilla/gfx/Matrix.h:720
    #2 0x7f42bee8e583 in mozilla::gfx::RectTyped<mozilla::gfx::UnknownUnits, double> mozilla::gfx::Matrix4x4Typed<mozilla::gfx::UnknownUnits, mozilla::gfx::UnknownUnits>::TransformAndClipBounds<double>(mozilla::gfx::RectTyped<mozilla::gfx::UnknownUnits, double> const&, mozilla::gfx::RectTyped<mozilla::gfx::UnknownUnits, double> const&) const ../../dist/include/mozilla/gfx/Matrix.h:657
    #3 0x7f42bee8e583 in Transform3D /home/thomas/Arbeit/Tor/tor-browser/gfx/layers/basic/BasicLayerManager.cpp:859
    #4 0x7f42bee8e583 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*) /home/thomas/Arbeit/Tor/tor-browser/gfx/layers/basic/BasicLayerManager.cpp:1054
    #5 0x7f42bee8ff37 in mozilla::layers::BasicLayerManager::PaintSelfOrChildren(mozilla::layers::PaintLayerContext&, gfxContext*) /home/thomas/Arbeit/Tor/tor-browser/gfx/layers/basic/BasicLayerManager.cpp:907
    #6 0x7f42bee8e2da 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*) /home/thomas/Arbeit/Tor/tor-browser/gfx/layers/basic/BasicLayerManager.cpp:1020
    #7 0x7f42bee8f7f6 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) /home/thomas/Arbeit/Tor/tor-browser/gfx/layers/basic/BasicLayerManager.cpp:624
    #8 0x7f42c0ec61b5 in PaintInactiveLayer /home/thomas/Arbeit/Tor/tor-browser/layout/base/FrameLayerBuilder.cpp:3643
    #9 0x7f42c0ec61b5 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) /home/thomas/Arbeit/Tor/tor-browser/layout/base/FrameLayerBuilder.cpp:5652
    #10 0x7f42c0ec6e40 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*) /home/thomas/Arbeit/Tor/tor-browser/layout/base/FrameLayerBuilder.cpp:5839
    #11 0x7f42beea3559 in mozilla::layers::ClientPaintedLayer::PaintThebes() /home/thomas/Arbeit/Tor/tor-browser/gfx/layers/client/ClientPaintedLayer.cpp:100
    #12 0x7f42beea8011 in mozilla::layers::ClientPaintedLayer::RenderLayerWithReadback(mozilla::layers::ReadbackProcessor*) /home/thomas/Arbeit/Tor/tor-browser/gfx/layers/client/ClientPaintedLayer.cpp:148
    #13 0x7f42beeb9447 in mozilla::layers::ClientContainerLayer::RenderLayer() /home/thomas/Arbeit/Tor/tor-browser/gfx/layers/client/ClientContainerLayer.h:65
    #14 0x7f42beeb9447 in mozilla::layers::ClientContainerLayer::RenderLayer() /home/thomas/Arbeit/Tor/tor-browser/gfx/layers/client/ClientContainerLayer.h:65
    #15 0x7f42bee9d11c 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) /home/thomas/Arbeit/Tor/tor-browser/gfx/layers/client/ClientLayerManager.cpp:282
    #16 0x7f42beeb4b34 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) /home/thomas/Arbeit/Tor/tor-browser/gfx/layers/client/ClientLayerManager.cpp:325
    #17 0x7f42c0f52641 in nsDisplayList::PaintRoot(nsDisplayListBuilder*, nsRenderingContext*, unsigned int) /home/thomas/Arbeit/Tor/tor-browser/layout/base/nsDisplayList.cpp:1754
    #18 0x7f42c0fc5260 in nsLayoutUtils::PaintFrame(nsRenderingContext*, nsIFrame*, nsRegion const&, unsigned int, unsigned int) /home/thomas/Arbeit/Tor/tor-browser/layout/base/nsLayoutUtils.cpp:3389
    #19 0x7f42c0fdf579 in PresShell::Paint(nsView*, nsRegion const&, unsigned int) /home/thomas/Arbeit/Tor/tor-browser/layout/base/nsPresShell.cpp:6105
    #20 0x7f42c0b4807d in nsViewManager::ProcessPendingUpdatesPaint(nsIWidget*) /home/thomas/Arbeit/Tor/tor-browser/view/nsViewManager.cpp:467
    #21 0x7f42c0b487e6 in nsViewManager::ProcessPendingUpdatesForView(nsView*, bool) /home/thomas/Arbeit/Tor/tor-browser/view/nsViewManager.cpp:398
    #22 0x7f42c0e84ad0 in nsRefreshDriver::Tick(long, mozilla::TimeStamp) /home/thomas/Arbeit/Tor/tor-browser/layout/base/nsRefreshDriver.cpp:1857
    #23 0x7f42c0e8a385 in mozilla::RefreshDriverTimer::TickRefreshDrivers(long, mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&) /home/thomas/Arbeit/Tor/tor-browser/layout/base/nsRefreshDriver.cpp:236
    #24 0x7f42c0e8a4cf in mozilla::RefreshDriverTimer::Tick(long, mozilla::TimeStamp) /home/thomas/Arbeit/Tor/tor-browser/layout/base/nsRefreshDriver.cpp:255
    #25 0x7f42c0e8a8bf in mozilla::VsyncRefreshDriverTimer::RunRefreshDrivers(mozilla::TimeStamp) /home/thomas/Arbeit/Tor/tor-browser/layout/base/nsRefreshDriver.cpp:566
    #26 0x7f42c0e8a8bf in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver(mozilla::TimeStamp) /home/thomas/Arbeit/Tor/tor-browser/layout/base/nsRefreshDriver.cpp:486
    #27 0x7f42c0e85b67 in void nsRunnableMethodArguments<mozilla::TimeStamp>::apply<mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver, void (mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::*)(mozilla::TimeStamp)>(mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver*, void (mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::*)(mozilla::TimeStamp)) ../../dist/include/nsThreadUtils.h:676
    #28 0x7f42c0e85b67 in nsRunnableMethodImpl<void (mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::*)(mozilla::TimeStamp), true, mozilla::TimeStamp>::Run() ../../dist/include/nsThreadUtils.h:870
    #29 0x7f42bdfbda6e in nsThread::ProcessNextEvent(bool, bool*) /home/thomas/Arbeit/Tor/tor-browser/xpcom/threads/nsThread.cpp:972
    #30 0x7f42be007d1c in NS_ProcessNextEvent(nsIThread*, bool) /home/thomas/Arbeit/Tor/tor-browser/xpcom/glue/nsThreadUtils.cpp:297
    #31 0x7f42be436e03 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /home/thomas/Arbeit/Tor/tor-browser/ipc/glue/MessagePump.cpp:127
    #32 0x7f42be3a44f5 in MessageLoop::RunHandler() /home/thomas/Arbeit/Tor/tor-browser/ipc/chromium/src/base/message_loop.cc:227
    #33 0x7f42be3a44f5 in MessageLoop::Run() /home/thomas/Arbeit/Tor/tor-browser/ipc/chromium/src/base/message_loop.cc:201
    #34 0x7f42c0b7f490 in nsBaseAppShell::Run() /home/thomas/Arbeit/Tor/tor-browser/widget/nsBaseAppShell.cpp:156
    #35 0x7f42c180c882 in nsAppStartup::Run() /home/thomas/Arbeit/Tor/tor-browser/toolkit/components/startup/nsAppStartup.cpp:281
    #36 0x7f42c1887d8a in XREMain::XRE_mainRun() /home/thomas/Arbeit/Tor/tor-browser/toolkit/xre/nsAppRunner.cpp:4285
    #37 0x7f42c188861f in XREMain::XRE_main(int, char**, nsXREAppData const*) /home/thomas/Arbeit/Tor/tor-browser/toolkit/xre/nsAppRunner.cpp:4382
    #38 0x7f42c1888a47 in XRE_main /home/thomas/Arbeit/Tor/tor-browser/toolkit/xre/nsAppRunner.cpp:4484
    #39 0x405d26 in do_main /home/thomas/Arbeit/Tor/tor-browser/browser/app/nsBrowserApp.cpp:212
    #40 0x404cdc in main /home/thomas/Arbeit/Tor/tor-browser/browser/app/nsBrowserApp.cpp:352
    #41 0x7f42ca07360f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2060f)
    #42 0x405138 in _start (/home/thomas/Arbeit/Tor/tor-browser/obj-x86_64-unknown-linux-gnu/dist/bin/firefox+0x405138)

Address 0x7ffd1aa481b0 is located in stack of thread T0 at offset 160 in frame
    #0 0x7f42bed95199 in unsigned long mozilla::gfx::Matrix4x4Typed<mozilla::gfx::UnknownUnits, mozilla::gfx::UnknownUnits>::TransformAndClipRect<double>(mozilla::gfx::RectTyped<mozilla::gfx::UnknownUnits, double> const&, mozilla::gfx::RectTyped<mozilla::gfx::UnknownUnits, double> const&, mozilla::gfx::PointTyped<mozilla::gfx::UnknownUnits, double>*) const ../../dist/include/mozilla/gfx/Matrix.h:688

  This frame has 2 object(s):
    [32, 160) 'planeNormals' <== Memory access at offset 160 overflows this variable
    [192, 2240) 'points'
HINT: this may be a false positive if your program uses some custom stack unwind mechanism or swapcontext
      (longjmp and C++ exceptions *are* supported)
SUMMARY: AddressSanitizer: stack-buffer-overflow ../../dist/include/mozilla/gfx/BasePoint4D.h:101 mozilla::gfx::BasePoint4D<double, mozilla::gfx::Point4DTyped<mozilla::gfx::UnknownUnits, double> >::DotProduct(mozilla::gfx::Point4DTyped<mozilla::gfx::UnknownUnits, double> const&) const
Shadow bytes around the buggy address:
  0x100023540fe0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x100023540ff0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x100023541000: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x100023541010: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x100023541020: 00 00 f1 f1 f1 f1 00 00 00 00 00 00 00 00 00 00
=>0x100023541030: 00 00 00 00 00 00[f2]f2 f2 f2 00 00 00 00 00 00
  0x100023541040: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x100023541050: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x100023541060: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x100023541070: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x100023541080: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  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
==2563==ABORTING

Comment 2

[Georg Koppen]

I investigated a bit further. ESR 38 is not affected and a much simpler example to reproduce the issue is https://desandro.github.io/3dtransforms/examples/box-01-steps.html. Clicking on button 4 or 2 several times if needed works for me. This works even by loading the example via file:/// from a local storage medium.

Comment 3

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

Milan, can you find someone to look into this?

Comment 4

[Milan Sreckovic [:milan] (needinfo for best results)]

Mason, can you take a look?

Comment 5

[Mason Chang [Inactive] [:mchang]]

Attachment: Startup failure

I actually can't reproduce this, but I also can't even start Firefox with an address sanitizer build. I keep getting this crash. This is also explicitly forcing a clang build rather than a gcc build. GCC doesn't seem to build ASAN at all and the general ASAN docs also point to using LLVM. Should I file a new bug with this or is this a known thing?

Comment 6

[Mason Chang [Inactive] [:mchang]]

What compiler are you using? Can you output clang --version or gcc -- version depending on what you're building? Thanks!

Comment 7

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

I have no idea if GCC works with Clang. Everyone at Mozilla does it with LLVM.

Comment 8

[Christian Holler (:decoder)]

Mason, can you show me the configuration you used to build that leads to this startup crash? Or did you use one of our existing builds? Please also include your Clang version/revision if you built this yourself.

Comment 9

[Georg Koppen]

FWIW, if you follow the steps outlined in comment 1) you should get working a ASan build with GCC. That is at least what I used to reproduce the bug with a vanilla esr45. The GCC on my system is 5.3.1.

Comment 10

[Georg Koppen]

I tried to reproduce the problem using the last official 45.0a1 ASan build I could find (https://ftp.mozilla.org/pub/firefox/tinderbox-builds/mozilla-central-linux64-asan/1450090940/) but it seems I can't. I am not sure why, yet, though. I guess I'll try clang on my Debian to test whether the different compiler is responsible for that. Meanwhile using a somewhat reduced .mozconfig (without the clutter we have for Tor Browser) triggers the problem as well (using GCC):

. $topsrcdir/browser/config/mozconfig

export CFLAGS="-fsanitize=address -Dxmalloc=myxmalloc"
export CXXFLAGS="-fsanitize=address -Dxmalloc=myxmalloc"
# We need to add -ldl explicitely due to bug 1213698
export LDFLAGS="-fsanitize=address -ldl"

ac_add_options --enable-address-sanitizer
ac_add_options --disable-jemalloc
ac_add_options --disable-elf-hack

Comment 11

[Georg Koppen]

Okay, this is interesting. Using clang 3.6.2 on the same machine results in a Firefox that does not show any sign of this buffer overflow. I used the following .mozconfig for this:

. $topsrcdir/browser/config/mozconfig

export LLVM_HOME="/usr/lib/llvm-3.6"
export CC="$LLVM_HOME/bin/clang"
export CXX="$LLVM_HOME/bin/clang++"
export LLVM_SYMBOLIZER="$LLVM_HOME/bin/llvm-symbolizer"

export CFLAGS="-fsanitize=address -Dxmalloc=myxmalloc -fPIC"
export CXXFLAGS="-fsanitize=address -Dxmalloc=myxmalloc -fPIC"
export LDFLAGS="-fsanitize=address"

ac_add_options --enable-address-sanitizer
ac_add_options --disable-jemalloc
ac_add_options --disable-elf-hack

Comment 12

[Christian Holler (:decoder)]

Georg, does this reproduce for you on Nightly as well? I tried Nightly on Linux with LLVM trunk and couldn't reproduce it. So either it is something specific to your setup, or something GCC-specific. LLVM 3.6 is certainly too old to compare though.

Comment 13

[Georg Koppen]

My current bet is this is related to GCC, somehow. I can actually reproduce the bug with different setups (the one we use for Tor Browser differs considerably from the one on my Linux box I used for testing vanilla Firefox) which probably only have GCC in common (but not even the same version).

I don't think LLVM 3.6 is too old compared to GCC 5.3.1. If I got the revisions right (+ do not account for the cherry-picking the GCC devs did) then ASan in LLVM 3.6 might even be slightly newer than the one GCC 5.3.1 ships (or 5.2 for that matter which we used in the browser I hit the problem with in the first place).

I did not test nightly with clang as esr45 already failed to show the same problem for me. I tried building nightly with GCC + ASan but the compilation breaks and I did not have time to investigate what happens yet.

Comment 14

[Mason Chang [Inactive] [:mchang]]

(In reply to Georg Koppen from comment #9)
> FWIW, if you follow the steps outlined in comment 1) you should get working
> a ASan build with GCC. That is at least what I used to reproduce the bug
> with a vanilla esr45. The GCC on my system is 5.3.1.

I used the mozconfig from comment 1, including the ESR 45 revision, but I could not build with gcc. Configure kept blowing up with different libraries missing, etc. I had to add export CC/CXX to clang/clang++ to build an ASAN version, which cannot start firefox. My GCC version is:

g++ (Ubuntu 5.3.1-14ubuntu2) 5.3.1 20160413

Clang:
clang version 3.8.0-2ubuntu3 (tags/RELEASE_380/final)

I'm on ubuntu 16.04 LTS xenial.

Comment 15

[Mason Chang [Inactive] [:mchang]]

On today's master from gecko-dev, revision fc68be72fbc43f091d450c9b52b5162cbf3e9e10, using clang to build, I was able to start Firefox as normal. I loaded the page team-cymru.org, just let it sit there for a 3 minutes while occasionally scrolling / hovering over links, but I was unable to reproduce the crash.

Comment 16

[Mason Chang [Inactive] [:mchang]]

So far this seems to be related to GCC from comment 11, 12, 13, and 15. Can we resolve as WFM then?

Comment 17

[Georg Koppen]

It seems related to builds using GCC (which Mozilla ships as well fwiw), yes. I wonder whether this really implies a bug in GCC's ASan code (which is basically incorporated LLVM ASan code).

Comment 18

[Mason Chang [Inactive] [:mchang]]

Resolving as WFM since this seems related to gcc and not gfx code.

Comment 19

[Georg Koppen]

Attachment: 1157984_ASan_stacktrace

FWIW https://hg.mozilla.org/mozilla-central/rev/9d8134f02763 (rev 260434, bug 1157984) is the first revision that introduces these problems for me. The log of the ASan crash is slightly different, though. The issue is still visible with an up-to-date mozilla-central repo.

Comment 20

[Georg Koppen]

This is not a GCC bug. It is reproducible with clang as well, see: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=71291. So, it seems this is indeed an issue in Mozilla code. Note: that GCC bug is public. I assumed you were right that this is no Mozilla bug and the GCC bugtracker has no option to mark bugs as security sensitive (at least I did not find it). I am sorry about that.

Comment 21

[Andrew McCreight [:mccr8]]

Could you take a look at this again, Mason? Thanks.

Comment 22

[:kip (Kearwood Gilbert)]

I haven't reproduced this yet; however, perhaps this overflow could be possible if "dstPoint" in TransformAndClipRect was incremented more than 32 times (kTransformAndClipRectMaxVerts):

https://dxr.mozilla.org/mozilla-central/source/gfx/2d/Matrix.h#713

It should not normally be possible to emit more than 32 vertices from this function; however, perhaps a sanity check could be added there to limit the number of vertices in the event of a floating point precision related error or other things I may not have considered.

Comment 23

[:kip (Kearwood Gilbert)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=95c5a5814dc3

Comment 24

[:kip (Kearwood Gilbert)]

Attachment: Bug 1268854 - Prevent buffer overflow in Matrix4x4Typed::TransformAndClipRect

Could someone who has reproduced this bug try this patch?

Comment 25

[Mason Chang [Inactive] [:mchang]]

I have been unable to reproduce this on any of the builds I've had.

(In reply to :kip (Kearwood Gilbert) from comment #24)
> Created attachment 8758418 [details] [diff] [review]
> Bug 1268854 - Prevent buffer overflow in Matrix4x4Typed::TransformAndClipRect
> 
> Could someone who has reproduced this bug try this patch?

Can you try this? Thanks!

Comment 26

[:kip (Kearwood Gilbert)]

FYI - I've kicked off a try push for that patch:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=63594067f7b3

Comment 27

[Georg Koppen]

(In reply to Mason Chang [:mchang] from comment #25)
> I have been unable to reproduce this on any of the builds I've had.
> 
> (In reply to :kip (Kearwood Gilbert) from comment #24)
> > Created attachment 8758418 [details] [diff] [review]
> > Bug 1268854 - Prevent buffer overflow in Matrix4x4Typed::TransformAndClipRect
> > 
> > Could someone who has reproduced this bug try this patch?
> 
> Can you try this? Thanks!

I can give it a try tomorrow. Not sure how you built Firefox but at least with CFLAGS="-fsanitize=address -fsanitize-recover=address -O0" is seems to be reproducible. (see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=71291#c10)

Comment 28

[Georg Koppen]

Nope, the proposed fix does not solve the issue. I still see the same crash after testing a build based on r299668 + the patch in this ticket (+ a patch for bug 1272498).

Comment 29

[Mason Chang [Inactive] [:mchang]]

Attachment: ASAN .mozconfig

I couldn't reproduce on today's master with Ubuntu 16.04 LTS, the attached ASAN mozconfig and today's master from the git gecko-dev repo c2494b58832076c918460a93623d75552b7551b3, on both lab.hakim.se/meny and team-cymru.org.

clang version 3.8.0-2ubuntu3 (tags/RELEASE_380/final)

Comment 30

[Mason Chang [Inactive] [:mchang]]

Attachment: Stack Trace

Comment 31

[Mason Chang [Inactive] [:mchang]]

Attachment: Crashing Mozconfig

Comment 32

[Georg Koppen]

FWIW: I just confirmed that the issue did not get triggered by clang due to compiling with -O2 in the official ASan builds instead of using -O0.

Comment 33

[Mason Chang [Inactive] [:mchang]]

Attachment: Break out of loop if we don't have any points intersecting or are on the positive side of the clipping plane

I'm not sure this is right, but we get a stack underflow here - https://dxr.mozilla.org/mozilla-central/source/gfx/2d/Matrix.h#737. This happens if we never move dstPoint during the loop below and so dstPoint == Points[0|1] and we read at Points[0|1][-1], causing the stack underflow. This patch incorporates Kip's checks along with a break if we don't move dstPoint at all. I'm not quite sure this is the right fix here, but I verified this fixes the issue for me on lab.hakim.se/meny.

Comment 34

[:kip (Kearwood Gilbert)]

Comment on attachment 8758851 [details] [diff] [review]
Break out of loop if we don't have any points intersecting or are on the positive side of the clipping plane

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

Looks good to me

Comment 35

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8758851 [details] [diff] [review]
Break out of loop if we don't have any points intersecting or are on the positive side of the clipping plane

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
I'm not really sure, but from the original bug report, random websites can trigger this bug.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
I don't think so.

Which older supported branches are affected by this flaw?
All branches up to ESR 45.

If not all supported branches, which bug introduced the flaw?
Bug 1157984.
Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

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

Comment 36

[Mason Chang [Inactive] [:mchang]]

So once this gets sec-approval, do I land it or does the security team do it after approval?

Comment 37

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

Normally you or the sheriffs land stuff. There is no "security team" actually. There are security people on different teams. :-)

Once it is time to land, you can use the "checkin-needed" keyword to get it landed on trunk.

I'm giving this sec-approval but only for landing on June 21 or later (on trunk). We just build Firefox 47 so this is too late for this release and we try to keep the window of exposure narrow on security bugs.

We'll want Aurora, Beta, and ESR45 patches made and nominated after this lands on trunk.

Comment 38

[Mason Chang [Inactive] [:mchang]]

NI on myself to remember

Comment 39

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8758851 [details] [diff] [review]
Break out of loop if we don't have any points intersecting or are on the positive side of the clipping plane

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

::: gfx/2d/Matrix.h
@@ +721,4 @@
>      // the input rectangle, aRect.
>      Point4DTyped<UnknownUnits, F> points[2][kTransformAndClipRectMaxVerts];
>      Point4DTyped<UnknownUnits, F>* dstPoint = points[0];
> +    Point4DTyped<UnknownUnits, F>* dstPointStart = dstPoint;

You don't need this one with the variable of the same name also defined in line 748, right?

Comment 40

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

We'll want branch patches for affected branches nominated after this goes into trunk.

Comment 41

[Mason Chang [Inactive] [:mchang]]

Attachment: Break out of loop if no points intersecting or are on positive side of the clipping plane.

Approval Request Comment
[Feature/regressing bug #]: N/A
[User impact if declined]: Security high bug.
[Describe test coverage new/current, TreeHerder]: Treeherder
[Risks and why]: Low, this just breaks out of a loop early if we had the condition that caused the stack underflow. 
[String/UUID change made/needed]: None

This patch applies on all branches so far. I'll land on nightly once we get beta/aurora approval.

Comment 42

[Mason Chang [Inactive] [:mchang]]

Can I get a sec+ again on the updated patch in comment 41? Thanks!

Comment 43

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8764294 [details] [diff] [review]
Break out of loop if no points intersecting or are on positive side of the clipping plane.

You don't need a sec-approval for an uplift. Al gave you one, it is enough for uplift.

Taking it to esr45 as it is a sec-high impacting esr.
Should be in 48 beta 3 and 45.3.0!

Comment 44

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/200070acb240df4253ef7a5be1d7ea376de14892

Comment 45

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/200070acb240

Comment 46

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/afcdd8539963
 https://hg.mozilla.org/releases/mozilla-beta/rev/cc7b9052413a

Comment 47

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-esr45/rev/910b8f21e777

Comment 48

[Daniel Veditz [:dveditz]]

Georg: Did this patch resolve the problem for you? Since we've had trouble reproducing would like a sanity check from your end.

Comment 49

[Georg Koppen]

(In reply to Daniel Veditz [:dveditz] from comment #48)
> Georg: Did this patch resolve the problem for you? Since we've had trouble
> reproducing would like a sanity check from your end.

Yes, I've built our current Tor Browser code with the patch on top of it and I don't get it crashed anymore.