Last Comment Bug 839745 - (CVE-2013-1678) Invalid write in _cairo_xlib_surface_add_glyph
(CVE-2013-1678)
: Invalid write in _cairo_xlib_surface_add_glyph
Status: RESOLVED FIXED
[adv-main21+]
: sec-moderate
Product: Core
Classification: Components
Component: Graphics: Text (show other bugs)
: Trunk
: x86_64 All
: -- normal (vote)
: mozilla22
Assigned To: Cameron McCormack (:heycam) (away Jun 25 – Jul 10)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-02-08 21:51 PST by Abhishek Arya
Modified: 2014-11-19 19:39 PST (History)
7 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
wontfix
fixed
fixed
wontfix
unaffected


Attachments
Testcase (560 bytes, text/plain)
2013-02-08 21:51 PST, Abhishek Arya
no flags Details
patch (1.40 KB, patch)
2013-02-13 00:13 PST, Cameron McCormack (:heycam) (away Jun 25 – Jul 10)
jmuizelaar: review+
bajaj.bhavana: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta-
lukasblakk+bugs: approval‑mozilla‑release-
Details | Diff | Review

Description Abhishek Arya 2013-02-08 21:51:13 PST
Created attachment 712073 [details]
Testcase

>==10987== ERROR: AddressSanitizer: unknown-crash on address 0x60920001abfc at pc 0x411d13 bp 0x7fffcc9bcb00 sp 0x7fffcc9bcad8
>WRITE of size 8 at 0x60920001abfc thread T0
>    #0 0x411d12 in __interceptor_memmove
>    #1 0x7f85d2e03e8d in
>    #2 0x7f85d7d44ab5 in _cairo_xlib_surface_add_glyph src/gfx/cairo/cairo/src/cairo-xlib-surface.c:4321
>    #3 0x7f85d7d44ab5 in _cairo_xlib_surface_emit_glyphs src/gfx/cairo/cairo/src/cairo-xlib-surface.c:4577
>    #4 0x7f85d7d44ab5 in _cairo_xlib_surface_show_glyphs src/gfx/cairo/cairo/src/cairo-xlib-surface.c:4904
>    #5 0x7f85d7ccd8ff in _cairo_surface_show_text_glyphs src/gfx/cairo/cairo/src/cairo-surface.c:2763
>    #6 0x7f85d7c5a40c in _cairo_gstate_show_text_glyphs src/gfx/cairo/cairo/src/cairo-gstate.c:1998
>    #7 0x7f85d7c2d056 in _moz_cairo_show_glyphs src/gfx/cairo/cairo/src/cairo.c:3523
>    #8 0x7f85d796b3d8 in GlyphBuffer::Flush(_cairo*, gfxFont::DrawMode, bool, gfxTextObjectPaint*, gfxMatrix const&, bool) src/gfx/thebes/gfxFont.cpp:1576
>    #9 0x7f85d79680f7 in gfxFont::Draw(gfxTextRun*, unsigned int, unsigned int, gfxContext*, gfxFont::DrawMode, gfxPoint*, gfxFont::Spacing*, gfxTextObjectPaint*) src/gfx/thebes/gfxFont.cpp:1967
>    #10 0x7f85d797ec4b in gfxTextRun::DrawGlyphs(gfxFont*, gfxContext*, gfxFont::DrawMode, gfxPoint*, gfxTextObjectPaint*, unsigned int, unsigned int, gfxTextRun::PropertyProvider*, unsigned int, unsigned int) src/gfx/thebes/gfxFont.cpp:4950
>    #11 0x7f85d797fe06 in gfxTextRun::Draw(gfxContext*, gfxPoint, gfxFont::DrawMode, unsigned int, unsigned int, gfxTextRun::PropertyProvider*, double*, gfxTextObjectPaint*, gfxTextRun::DrawCallbacks*) src/gfx/thebes/gfxFont.cpp:5151
>    #12 0x7f85d5eea3ea in nsSVGGlyphFrame::DrawCharacters(CharacterIterator*, gfxContext*, gfxFont::DrawMode, gfxTextObjectPaint*) src/layout/svg/nsSVGGlyphFrame.cpp:613
>    #13 0x7f85d5ee9d4d in nsSVGGlyphFrame::PaintSVG(nsRenderingContext*, nsIntRect const*) src/layout/svg/nsSVGGlyphFrame.cpp:441
>    #14 0x7f85d5ee7fc9 in nsDisplaySVGGlyphs::Paint(nsDisplayListBuilder*, nsRenderingContext*) src/layout/svg/nsSVGGlyphFrame.cpp:254
>    #15 0x7f85d4301d3c in mozilla::FrameLayerBuilder::DrawThebesLayer(mozilla::layers::ThebesLayer*, gfxContext*, nsIntRegion const&, nsIntRegion const&, void*) src/layout/base/FrameLayerBuilder.cpp:3341
>    #16 0x7f85d7a25232 in PaintBuffer src/gfx/layers/basic/BasicThebesLayer.h:94
>    #17 0x7f85d7a25232 in mozilla::layers::BasicShadowableThebesLayer::PaintBuffer(gfxContext*, nsIntRegion const&, nsIntRegion const&, nsIntRegion const&, bool, void (*)(mozilla::layers::ThebesLayer*, gfxContext*, nsIntRegion const&, nsIntRegion const&, void*), void*) src/gfx/layers/basic/BasicThebesLayer.cpp:400
>    #18 0x7f85d7a21b9e in mozilla::layers::BasicThebesLayer::PaintThebes(gfxContext*, mozilla::layers::Layer*, void (*)(mozilla::layers::ThebesLayer*, gfxContext*, nsIntRegion const&, nsIntRegion const&, void*), void*, mozilla::layers::ReadbackProcessor*) src/gfx/layers/basic/BasicThebesLayer.cpp:187
>    #19 0x7f85d7a2472b in mozilla::layers::BasicShadowableThebesLayer::PaintThebes(gfxContext*, mozilla::layers::Layer*, void (*)(mozilla::layers::ThebesLayer*, gfxContext*, nsIntRegion const&, nsIntRegion const&, void*), void*, mozilla::layers::ReadbackProcessor*) src/gfx/layers/basic/BasicThebesLayer.cpp:301
>    #20 0x7f85d7a02b35 in mozilla::layers::BasicLayerManager::PaintSelfOrChildren(mozilla::layers::PaintContext&, gfxContext*) src/gfx/layers/basic/BasicLayerManager.cpp:819
>    #21 0x7f85d7a005fa in mozilla::layers::BasicLayerManager::PaintLayer(gfxContext*, mozilla::layers::Layer*, void (*)(mozilla::layers::ThebesLayer*, gfxContext*, nsIntRegion const&, nsIntRegion const&, void*), void*, mozilla::layers::ReadbackProcessor*) src/gfx/layers/basic/BasicLayerManager.cpp:934
>    #22 0x7f85d7a02975 in mozilla::layers::BasicLayerManager::PaintSelfOrChildren(mozilla::layers::PaintContext&, gfxContext*) src/gfx/layers/basic/BasicLayerManager.cpp:835
>    #23 0x7f85d7a005fa in mozilla::layers::BasicLayerManager::PaintLayer(gfxContext*, mozilla::layers::Layer*, void (*)(mozilla::layers::ThebesLayer*, gfxContext*, nsIntRegion const&, nsIntRegion const&, void*), void*, mozilla::layers::ReadbackProcessor*) src/gfx/layers/basic/BasicLayerManager.cpp:934
>    #24 0x7f85d7a02975 in mozilla::layers::BasicLayerManager::PaintSelfOrChildren(mozilla::layers::PaintContext&, gfxContext*) src/gfx/layers/basic/BasicLayerManager.cpp:835
>    #25 0x7f85d7a005fa in mozilla::layers::BasicLayerManager::PaintLayer(gfxContext*, mozilla::layers::Layer*, void (*)(mozilla::layers::ThebesLayer*, gfxContext*, nsIntRegion const&, nsIntRegion const&, void*), void*, mozilla::layers::ReadbackProcessor*) src/gfx/layers/basic/BasicLayerManager.cpp:934
>    #26 0x7f85d79fd825 in mozilla::layers::BasicLayerManager::EndTransactionInternal(void (*)(mozilla::layers::ThebesLayer*, gfxContext*, nsIntRegion const&, nsIntRegion const&, void*), void*, mozilla::layers::LayerManager::EndTransactionFlags) src/gfx/layers/basic/BasicLayerManager.cpp:590
>    #27 0x7f85d7a06125 in EndTransaction src/gfx/layers/basic/BasicLayerManager.cpp:509
>    #28 0x7f85d7a06125 in mozilla::layers::BasicShadowLayerManager::EndTransaction(void (*)(mozilla::layers::ThebesLayer*, gfxContext*, nsIntRegion const&, nsIntRegion const&, void*), void*, mozilla::layers::LayerManager::EndTransactionFlags) src/gfx/layers/basic/BasicLayerManager.cpp:1144
>    #29 0x7f85d43c4970 in nsDisplayList::PaintForFrame(nsDisplayListBuilder*, nsRenderingContext*, nsIFrame*, unsigned int) const src/layout/base/nsDisplayList.cpp:1168
>    #30 0x7f85d43c36db in nsDisplayList::PaintRoot(nsDisplayListBuilder*, nsRenderingContext*, unsigned int) const src/layout/base/nsDisplayList.cpp:1030
>    #31 0x7f85d443df42 in nsLayoutUtils::PaintFrame(nsRenderingContext*, nsIFrame*, nsRegion const&, unsigned int, unsigned int) src/layout/base/nsLayoutUtils.cpp:2014
>    #32 0x7f85d449d7d7 in PresShell::Paint(nsView*, nsRegion const&, unsigned int) src/layout/base/nsPresShell.cpp:5414
>    #33 0x7f85d5500bcb in nsViewManager::ProcessPendingUpdatesForView(nsView*, bool) src/view/src/nsViewManager.cpp:399
>    #34 0x7f85d44bc175 in nsRefreshDriver::Tick(long, mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:955
>    #35 0x7f85d44beaf3 in TickDriver src/layout/base/nsRefreshDriver.cpp:164
>    #36 0x7f85d44beaf3 in mozilla::RefreshDriverTimer::Tick() src/layout/base/nsRefreshDriver.cpp:156
>    #37 0x7f85d784b194 in nsTimerImpl::Fire() src/xpcom/threads/nsTimerImpl.cpp:482
>    #38 0x7f85d784b762 in nsTimerEvent::Run() src/xpcom/threads/nsTimerImpl.cpp:565
>    #39 0x7f85d783fdee in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:627
>    #40 0x7f85d7777ce2 in NS_ProcessNextEvent_P(nsIThread*, bool) src/objdir-ff-asan/xpcom/build/nsThreadUtils.cpp:238
>    #41 0x7f85d6f00bec in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:82
>    #42 0x7f85d78d1aa8 in RunInternal src/ipc/chromium/src/base/message_loop.cc:215
>    #43 0x7f85d78d1aa8 in RunHandler src/ipc/chromium/src/base/message_loop.cc:208
>    #44 0x7f85d78d1aa8 in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:182
>    #45 0x7f85d6bf69dd in nsBaseAppShell::Run() src/widget/xpwidgets/nsBaseAppShell.cpp:163
>    #46 0x7f85d6679340 in nsAppStartup::Run() src/toolkit/components/startup/nsAppStartup.cpp:288
>    #47 0x7f85d3bb93ac in XREMain::XRE_mainRun() src/toolkit/xre/nsAppRunner.cpp:3826
>    #48 0x7f85d3bba23f in XREMain::XRE_main(int, char**, nsXREAppData const*) src/toolkit/xre/nsAppRunner.cpp:3893
>    #49 0x7f85d3bbb201 in XRE_main src/toolkit/xre/nsAppRunner.cpp:4096
>    #50 0x41fd9b in do_main src/browser/app/nsBrowserApp.cpp:185
>    #51 0x41fd9b in main src/browser/app/nsBrowserApp.cpp:377
>    #52 0x7f85dd8d276c in
>    #53 0x41f114 in
>0x60920001ac00 is located 0 bytes to the right of 16384-byte region [0x609200016c00,0x60920001ac00)
>allocated by thread T0 here:
>    #0 0x413e83 in __interceptor_calloc
>    #1 0x7f85d1e5ee2d in
>Shadow bytes around the buggy address:
>  0x1c1240003520: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>  0x1c1240003530: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>  0x1c1240003540: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>  0x1c1240003550: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>  0x1c1240003560: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>=>0x1c1240003570: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00[00]
>  0x1c1240003580:fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>  0x1c1240003590: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>  0x1c12400035a0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>  0x1c12400035b0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>  0x1c12400035c0: 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 righ 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
>  ASan internal:         fe
>==10987== ABORTING
>
>
Comment 1 Cameron McCormack (:heycam) (away Jun 25 – Jul 10) 2013-02-11 16:58:23 PST
Abishek, I can't seem to reproduce this with the most recent Linux ASAN build, which I took from https://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux64-dbg-asan/1360616788/.  Was there a particular build you used?
Comment 2 Abhishek Arya 2013-02-11 19:30:59 PST
(In reply to Cameron McCormack (:heycam) from comment #1)
> Abishek, I can't seem to reproduce this with the most recent Linux ASAN
> build, which I took from
> https://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-
> central-linux64-dbg-asan/1360616788/.  Was there a particular build you used?

Ah i forgot to put in c#0, you need to run it on Xvfb screen [Xvfb :1 -screen 0 1280x1024x24] and then set env as DISPLAY=:1 ASAN_OPTIONS=redzone=1024
Comment 3 Cameron McCormack (:heycam) (away Jun 25 – Jul 10) 2013-02-12 22:38:17 PST
Thanks, I can reproduce now.

I don't know anything about X11 programming, but it looks like the invalid write is due to the following:

  1. Cairo wants to send a glyph image using XRenderAddGlyphs.  The glyph image
     is kind of large -- 336608 bytes' worth.
  2. XRenderAddGlyphs grabs an initial request buffer from the display using
     GetReq.

http://cgit.freedesktop.org/xorg/lib/libXrender/tree/src/Glyph.c?id=libXrender-0.9.7#n101

  3. The SetReqLen is called with 84156 words == 336624 bytes.
  4. Inside SetReqLen, it finds that the request length is >= 64KB, so it attempts
     to upgrade the request packet to a big request:

http://cgit.freedesktop.org/xorg/lib/libX11/tree/include/X11/Xlibint.h?id=libX11-1.5.0#n546

  5. The MakeBigReq call just assumes that it can shift the contents of the request
     over with memmove to make way for the extra four bytes used to store the
     big request length:

http://cgit.freedesktop.org/xorg/lib/libX11/tree/include/X11/Xlibint.h?id=libX11-1.5.0#n518

  6. We write off the end of the display's buffer in the memmove.


As I said I don't really know how X11 is meant to work, but it seems like MakeBigReq is assuming that it can write to just after the end of the request in the buffer.  Maybe the X client is meant to keep a bit of extra space at the end for just this situation?

I think the situation here is that there was enough space to allocate the empty RenderAddGlyphs request in the GetReq call, and thus in GetReq it didn't need to call _XFlush inside _XGetRequest:

http://cgit.freedesktop.org/xorg/lib/libX11/tree/src/XlibInt.c?id=libX11-1.5.0#n1966

but the allocated request ran right up against the end of the buffer, and there was insufficient space to upgrade it to a big request.
Comment 4 Cameron McCormack (:heycam) (away Jun 25 – Jul 10) 2013-02-12 22:44:41 PST
(In reply to Cameron McCormack (:heycam) from comment #3)
>   4. Inside SetReqLen, it finds that the request length is >= 64KB, so it

That should be ">= 64K 32-bit words, i.e. 262140 bytes".
Comment 5 Cameron McCormack (:heycam) (away Jun 25 – Jul 10) 2013-02-13 00:13:46 PST
Created attachment 713306 [details] [diff] [review]
patch

If we wanted to work around the bug, we could do something like this, and call XFlush() if there's not enough space for the xRenderAddGlyphsReq struct plus the four bytes for the big request size.  It does avoid the ASAN error with the test file.

I'm not sure how bad it is to use Xlibint.h.  I suppose the change should also go in a patch file instead of changing cairo-xlib-surface.c itself?
Comment 6 Daniel Veditz [:dveditz] 2013-02-13 10:23:35 PST
(In reply to Cameron McCormack (:heycam) from comment #3)
> As I said I don't really know how X11 is meant to work, but it seems like
> MakeBigReq is assuming that it can write to just after the end of the
> request in the buffer.  Maybe the X client is meant to keep a bit of extra
> space at the end for just this situation?

How much past the end can this write? If it's just one pointer's worth that might only be sec-moderate, but if it's an arbitrary amount that's sec-critical.
Comment 7 Cameron McCormack (:heycam) (away Jun 25 – Jul 10) 2013-02-13 15:11:24 PST
I believe it is just four bytes' worth.  The size of the display's buffer is checked against the desired length of the request before the attempt to upgrade it to a big request, and flushed if that would not fit.  The actual glyph payload data is sent with the Data() call, which correctly checks the buffer size.
Comment 8 Milan Sreckovic [:milan] 2013-02-13 15:13:11 PST
(In reply to Daniel Veditz [:dveditz] from comment #6)
> (In reply to Cameron McCormack (:heycam) from comment #3)
> 
> How much past the end can this write? If it's just one pointer's worth that
> might only be sec-moderate, but if it's an arbitrary amount that's
> sec-critical.

Based on this and comment 7, tagging as sec-moderate.
Comment 9 Jeff Muizelaar [:jrmuizel] 2013-02-27 13:08:23 PST
Comment on attachment 713306 [details] [diff] [review]
patch

I guess we should do something to get this fixed upstream too. Can you do that Cam?
Comment 10 Cameron McCormack (:heycam) (away Jun 25 – Jul 10) 2013-02-27 19:54:17 PST
(In reply to Jeff Muizelaar [:jrmuizel] from comment #9)
> I guess we should do something to get this fixed upstream too. Can you do
> that Cam?

Yes I'll get on that when I'm back.
Comment 11 Cameron McCormack (:heycam) (away Jun 25 – Jul 10) 2013-02-27 20:25:13 PST
(Actually checkin-needed might not be right, if we need to add the cairo patch file too.)
Comment 12 Cameron McCormack (:heycam) (away Jun 25 – Jul 10) 2013-03-24 20:47:57 PDT
Comment on attachment 713306 [details] [diff] [review]
patch

Not really sure which branches this should land on; a? for the non-long-term ones.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): N/A
User impact if declined: content could cause data to be written one word past a buffer
Testing completed (on m-c, etc.): tested locally only
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: none
Comment 13 Lukas Blakk [:lsblakk] use ?needinfo 2013-03-25 09:15:32 PDT
Since this is only sec-moderate, not yet landed to trunk, and untracked we'll leave this out of FF20 which is going to build today.
Comment 14 Cameron McCormack (:heycam) (away Jun 25 – Jul 10) 2013-03-25 15:13:29 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2038e23a282
Comment 15 Cameron McCormack (:heycam) (away Jun 25 – Jul 10) 2013-03-26 02:58:03 PDT
Turns out Karl reported the underlying xlib bug a few months ago:

  https://bugs.freedesktop.org/show_bug.cgi?id=56508

and it has been fixed in the latest RC:

  http://cgit.freedesktop.org/xorg/lib/libX11/commit/?id=39547d600a13713e15429f49768e54c3173c828d

The Cairo xlib maintainer didn't seem interested in taking the workaround patch.
Comment 16 Ryan VanderMeulen [:RyanVM] 2013-03-26 07:31:26 PDT
https://hg.mozilla.org/mozilla-central/rev/f2038e23a282

Can we land the test for this?
Comment 17 Ryan VanderMeulen [:RyanVM] 2013-03-26 12:57:53 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/9c5670e1683c
Comment 18 Cameron McCormack (:heycam) (away Jun 25 – Jul 10) 2013-03-26 15:20:53 PDT
(In reply to Ryan VanderMeulen [:RyanVM] from comment #16)
> Can we land the test for this?

The crash would only happen in an ASAN build; we don't have ASAN builds running do we?
Comment 19 Daniel Veditz [:dveditz] 2013-04-30 18:13:42 PDT
(In reply to Cameron McCormack (:heycam) from comment #18)
> The crash would only happen in an ASAN build; we don't have ASAN builds
> running do we?

We run them manually on a regular basis and are working toward having them run as regular RelEng/tbpl test runs. A regression test would still be worthwhile.
Comment 20 Cameron McCormack (:heycam) (away Jun 25 – Jul 10) 2013-05-15 17:53:46 PDT
I landed the crashtest: https://hg.mozilla.org/integration/mozilla-inbound/rev/b5ca9f068320
Comment 21 Ryan VanderMeulen [:RyanVM] 2013-05-16 14:09:55 PDT
https://hg.mozilla.org/mozilla-central/rev/b5ca9f068320

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