839745 - (CVE-2013-1678) Invalid write in _cairo_xlib_surface_add_glyph

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect)

Description

[Abhishek Arya]

Attachment: 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)]

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]

(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)]

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)]

(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)]

Attachment: 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]]

(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)]

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] (needinfo for best results)]

(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]]

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)]

(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)]

(Actually checkin-needed might not be right, if we need to add the cairo patch file too.)

Comment 12

[Cameron McCormack (:heycam)]

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]

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)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f2038e23a282

Comment 15

[Cameron McCormack (:heycam)]

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]]

https://hg.mozilla.org/mozilla-central/rev/f2038e23a282

Can we land the test for this?

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9c5670e1683c

Comment 18

[Cameron McCormack (:heycam)]

(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]]

(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)]

I landed the crashtest: https://hg.mozilla.org/integration/mozilla-inbound/rev/b5ca9f068320

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b5ca9f068320