1099437 - (CVE-2015-0806) Negative-size-param memset in mozilla::layers::BufferTextureClient::AllocateForSurface

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Abhishek Arya]

Attachment: test.html

DOMFuzzHelper created
=================================================================
==29802==ERROR: AddressSanitizer: negative-size-param: (size=-827354608)
    #0 0x48c76e in __asan_memset _asan_rtl_
    #1 0x7fbe864815f9 in mozilla::layers::BufferTextureClient::AllocateForSurface(mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits>, mozilla::layers::TextureAllocationFlags) gfx/layers/client/TextureClient.cpp:677:12
    #2 0x7fbe86470ea8 in mozilla::layers::TextureClient::CreateForDrawing(mozilla::layers::ISurfaceAllocator*, mozilla::gfx::SurfaceFormat, mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits>, mozilla::gfx::BackendType, mozilla::layers::TextureFlags, mozilla::layers::TextureAllocationFlags) gfx/layers/client/TextureClient.cpp:350:8
    #3 0x7fbe86473045 in mozilla::layers::ContentClientRemoteBuffer::CreateBackBuffer(nsIntRect const&) gfx/layers/client/CompositableClient.cpp:210:10
    #4 0x7fbe864735ef in mozilla::layers::ContentClientRemoteBuffer::CreateBuffer(gfxContentType, nsIntRect const&, unsigned int, mozilla::RefPtr<mozilla::gfx::DrawTarget>*, mozilla::RefPtr<mozilla::gfx::DrawTarget>*) gfx/layers/client/ContentClient.cpp:295:3
    #5 0x7fbe863d32b8 in mozilla::layers::RotatedContentBuffer::BeginPaint(mozilla::layers::PaintedLayer*, unsigned int) gfx/layers/RotatedBuffer.cpp:648:5
    #6 0x7fbe86484291 in mozilla::layers::ContentClientRemoteBuffer::BeginPaintBuffer(mozilla::layers::PaintedLayer*, unsigned int) objdir-ff-asan/dist/include/mozilla/layers/ContentClient.h:214:12
    #7 0x7fbe8646823c in mozilla::layers::ClientPaintedLayer::PaintThebes() gfx/layers/client/ClientPaintedLayer.cpp:54:5
    #8 0x7fbe86468e68 in mozilla::layers::ClientPaintedLayer::RenderLayerWithReadback(mozilla::layers::ReadbackProcessor*) gfx/layers/client/ClientPaintedLayer.cpp:131:3
    #9 0x7fbe8648e3d7 in mozilla::layers::ClientContainerLayer::RenderLayer() gfx/layers/client/ClientContainerLayer.h:69:7
    #10 0x7fbe8648e3d7 in mozilla::layers::ClientContainerLayer::RenderLayer() gfx/layers/client/ClientContainerLayer.h:69:7
    #11 0x7fbe8648e3d7 in mozilla::layers::ClientContainerLayer::RenderLayer() gfx/layers/client/ClientContainerLayer.h:69:7
    #12 0x7fbe8648e3d7 in mozilla::layers::ClientContainerLayer::RenderLayer() gfx/layers/client/ClientContainerLayer.h:69:7
    #13 0x7fbe8648e3d7 in mozilla::layers::ClientContainerLayer::RenderLayer() gfx/layers/client/ClientContainerLayer.h:69:7
    #14 0x7fbe864639b4 in mozilla::layers::ClientLayerManager::EndTransactionInternal(void (*)(mozilla::layers::PaintedLayer*, gfxContext*, nsIntRegion const&, mozilla::layers::DrawRegionClip, nsIntRegion const&, void*), void*, mozilla::layers::LayerManager::EndTransactionFlags) gfx/layers/client/ClientLayerManager.cpp:268:3
    #15 0x7fbe86463efb in mozilla::layers::ClientLayerManager::EndTransaction(void (*)(mozilla::layers::PaintedLayer*, gfxContext*, nsIntRegion const&, mozilla::layers::DrawRegionClip, nsIntRegion const&, void*), void*, mozilla::layers::LayerManager::EndTransactionFlags) gfx/layers/client/ClientLayerManager.cpp:298:3
    #16 0x7fbe89cac4a7 in nsDisplayList::PaintRoot(nsDisplayListBuilder*, nsRenderingContext*, unsigned int) layout/base/nsDisplayList.cpp:1435:3
    #17 0x7fbe89d2d9db in nsLayoutUtils::PaintFrame(nsRenderingContext*, nsIFrame*, nsRegion const&, unsigned int, unsigned int) layout/base/nsLayoutUtils.cpp:3128:5
    #18 0x7fbe89dad133 in PresShell::Paint(nsView*, nsRegion const&, unsigned int) layout/base/nsPresShell.cpp:6337:5
    #19 0x7fbe89532422 in nsViewManager::ProcessPendingUpdatesPaint(nsIWidget*) view/nsViewManager.cpp:443:7
    #20 0x7fbe89531c6e in nsViewManager::ProcessPendingUpdatesForView(nsView*, bool) view/nsViewManager.cpp:384:9
    #21 0x7fbe89b53642 in nsRefreshDriver::Tick(long, mozilla::TimeStamp) layout/base/nsRefreshDriver.cpp:1354:5
    #22 0x7fbe89b59716 in mozilla::RefreshDriverTimer::Tick() layout/base/nsRefreshDriver.cpp:173:5
    #23 0x7fbe84b154e5 in nsTimerImpl::Fire() xpcom/threads/nsTimerImpl.cpp:621:7
    #24 0x7fbe84b160c0 in nsTimerEvent::Run() xpcom/threads/nsTimerImpl.cpp:714:3
    #25 0x7fbe84b0c106 in nsThread::ProcessNextEvent(bool, bool*) xpcom/threads/nsThread.cpp:830:7
    #26 0x7fbe84b62536 in NS_ProcessNextEvent(nsIThread*, bool) xpcom/glue/nsThreadUtils.cpp:265:10
    #27 0x7fbe853a0daf in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) ipc/glue/MessagePump.cpp:99:21
    #28 0x7fbe85353761 in MessageLoop::Run() ipc/chromium/src/base/message_loop.cc:233:3
    #29 0x7fbe89563a8f in nsBaseAppShell::Run() widget/nsBaseAppShell.cpp:164:3
    #30 0x7fbe8afcc762 in XRE_RunAppShell toolkit/xre/nsEmbedFunctions.cpp:713:12
    #31 0x7fbe85353761 in MessageLoop::Run() ipc/chromium/src/base/message_loop.cc:233:3
    #32 0x7fbe8afcbc72 in XRE_InitChildProcess toolkit/xre/nsEmbedFunctions.cpp:550:7
    #33 0x4ba94f in main ipc/contentproc/plugin-container.cpp:158:19
    #34 0x7fbe820d6de4 in __libc_start_main /build/buildd/eglibc-2.17/csu/libc-start.c:260
AddressSanitizer can not describe address in more detail (wild memory access suspected).
SUMMARY: AddressSanitizer: negative-size-param ??:0 ??

Comment 1

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

Mike, any thoughts here?

Comment 2

[Mike Hommey [:glandium]]

Seems like nothing in ImageDataSerializer::ComputeMinBufferSize does overflow detection when computing the buffer size. Someone from gfx should look into this.

Comment 3

[Andrew McCreight [:mccr8]]

(In reply to Mike Hommey [:glandium] from comment #2)
> Seems like nothing in ImageDataSerializer::ComputeMinBufferSize does
> overflow detection when computing the buffer size. Someone from gfx should
> look into this.

Our actual question, which Al did not actually spell out, is just wondering how bad it would be with our jemalloc if somebody passes in a negative value to memset, or whatever is happening in comment 0.

Comment 4

[Mike Hommey [:glandium]]

memset is a libc function, not part of the allocator. And at least for gcc, it is likely to be replaced with an intrinsic. I think that's also the case with MSVC. I don't know if their implementation is safe, but note the function signature uses a (unsigned) size_t length, which means that bufSize (signed) int is casted to (unsigned) size_t, which means bad things will happen.

Comment 5

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

Attachment: Check for invalid sizes in the obvious places. r=nical

Let's see if this helps.

Comment 6

[Mike Hommey [:glandium]]

Comment on attachment 8525493 [details] [diff] [review]
Check for invalid sizes in the obvious places. r=nical

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

::: gfx/layers/ImageDataSerializer.cpp
@@ +83,1 @@
>    uint32_t bufsize = aSize.height * ComputeStride(aFormat, aSize.width);

Checking that the two ints you multiply are greater than 0 is not enough to know whether they will fit in a uint32_t.
Example: 1 << 24 and 1 << 24. Why not use CheckedInt?

Comment 7

[Nicolas Silva [:nical]]

Comment on attachment 8525493 [details] [diff] [review]
Check for invalid sizes in the obvious places. r=nical

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

I agree with Mike, let's use CheckedInt to be on the safe side here.

Comment 8

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

Attachment: Part 1. Check for negative or overflow sizes. r=nical

Comment 9

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

Attachment: Part 2. Clean up a bit of int vs. uint usage. r=nical

Not sure we want this, but there is a lot of casting, and Moz2D actually uses int32_t for size limits.

Comment 10

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

Comment on attachment 8526270 [details] [diff] [review]
Part 2. Clean up a bit of int vs. uint usage. r=nical

Sounds like you don't mind the idea of some int/uint cleanup, so let's just do the review.  Unchanged patch from f+.

Comment 12

[Andrew McCreight [:mccr8]]

This needs a security rating and possible sec-approval (depending on the rating) before it lands.

Comment 13

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

I'm assuming with sec-moderate, we don't need the approval?

Comment 15

[Andrew McCreight [:mccr8]]

(In reply to Milan Sreckovic [:milan] from comment #13)
> I'm assuming with sec-moderate, we don't need the approval?

That is correct.

Comment 16

[Andrew McCreight [:mccr8]]

Thanks for rating it.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/55252aebf52d
https://hg.mozilla.org/integration/mozilla-inbound/rev/258ac2909d6e

Do we need this on any other branches?

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Backed out for bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/a770fca02a0e

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=4230931&repo=mozilla-inbound

Comment 19

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

Should be OK riding the trains.

Sorry about the bustage.  I was convinced I had a try run, but obviously not.

Comment 20

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

Attachment: Part 1. Check for negative or overflow sizes. Carry r=nical

Carry r=nical.  Added missing includes (problems on some platforms), fixed a typo, and rebased.

Comment 21

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

Attachment: Part 2. Clean up a bit of int vs. uint usage. Carry r=nical

Fixed the compile error.  Carry r=nical

Comment 22

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

How do I convince treeherder to actually tell me how the try run went? https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=5eb79b37bfdc

Comment 23

[Andrew McCreight [:mccr8]]

(In reply to Milan Sreckovic [:milan] from comment #22)
> How do I convince treeherder to actually tell me how the try run went?
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=5eb79b37bfdc

Weird.  I'd kill all those jobs and repush it to try.  It shouldn't still be running tests many days later...

Comment 24

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

Let's see if this one has more luck:  https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a1f4fa4a55fa

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

There's no way all of those reftest failures could be because of this patch, right?

Comment 26

[Ryan VanderMeulen [:RyanVM]]

They look real to me.

Comment 27

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

Attachment: Part 1. Check for negative or overflow sizes. Carry r=nical

Yeah, try failures were real.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=0890258269d8

Comment 28

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

Not sure if I quite believe this run, what with no intermittent oranges, but could be just luck...

Comment 29

[Ryan VanderMeulen [:RyanVM]]

SHIP IT!

https://hg.mozilla.org/integration/mozilla-inbound/rev/5597611e315f
https://hg.mozilla.org/integration/mozilla-inbound/rev/c15637e90d9a

Setting the branches to wontfix per comment 19.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5597611e315f
https://hg.mozilla.org/mozilla-central/rev/c15637e90d9a

Comment 31

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

Since this is not being uplifted, when do we mark it resolved?

Comment 32

[Andrew McCreight [:mccr8]]

It should actually get closed when it lands on central, whether or not it was getting uplifting.  Ryan probably just forgot.

Comment 33

[Raymond Forbes[:rforbes]]

this is a dupe of 1097725 so this is not eligible for a bounty as 1097725 came first.  Please let me know if you think this is mistaken.