Closed Bug 1099437 (CVE-2015-0806) Opened 5 years ago Closed 5 years ago

Negative-size-param memset in mozilla::layers::BufferTextureClient::AllocateForSurface

Categories

(Core :: Graphics: Layers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37
Tracking Status
firefox35 --- wontfix
firefox36 --- wontfix
firefox37 --- fixed
firefox-esr31 --- wontfix
b2g-v2.1 --- wontfix
b2g-v2.2 --- fixed

People

(Reporter: inferno, Assigned: milan)

References

Details

(Keywords: sec-moderate, Whiteboard: [adv-main37+][b2g-adv-main2.2+])

Attachments

(3 files, 4 obsolete files)

Attached file 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 ??
Flags: sec-bounty?
Mike, any thoughts here?
Seems like nothing in ImageDataSerializer::ComputeMinBufferSize does overflow detection when computing the buffer size. Someone from gfx should look into this.
(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.
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.
Assignee: nobody → milan
Attachment #8525493 - Flags: review?(nical.bugzilla)
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 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.
Attachment #8525493 - Flags: review?(nical.bugzilla) → review-
Attachment #8525493 - Attachment is obsolete: true
Attachment #8526269 - Flags: review?(nical.bugzilla)
Not sure we want this, but there is a lot of casting, and Moz2D actually uses int32_t for size limits.
Attachment #8526270 - Flags: feedback?(nical.bugzilla)
Attachment #8526270 - Flags: feedback?(nical.bugzilla) → feedback+
Attachment #8526269 - Flags: review?(nical.bugzilla) → review+
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+.
Attachment #8526270 - Flags: review?(nical.bugzilla)
Attachment #8526270 - Flags: review?(nical.bugzilla) → review+
Duplicate of this bug: 1104254
This needs a security rating and possible sec-approval (depending on the rating) before it lands.
I'm assuming with sec-moderate, we don't need the approval?
Duplicate of this bug: 1097725
(In reply to Milan Sreckovic [:milan] from comment #13)
> I'm assuming with sec-moderate, we don't need the approval?

That is correct.
Thanks for rating it.
Should be OK riding the trains.

Sorry about the bustage.  I was convinced I had a try run, but obviously not.
Flags: needinfo?(milan)
Carry r=nical.  Added missing includes (problems on some platforms), fixed a typo, and rebased.
Attachment #8526269 - Attachment is obsolete: true
Attachment #8529439 - Flags: review+
Fixed the compile error.  Carry r=nical
Attachment #8526270 - Attachment is obsolete: true
Attachment #8529441 - Flags: review+
How do I convince treeherder to actually tell me how the try run went? https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=5eb79b37bfdc
(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...
There's no way all of those reftest failures could be because of this patch, right?
Flags: needinfo?(milan)
They look real to me.
Not sure if I quite believe this run, what with no intermittent oranges, but could be just luck...
Keywords: checkin-needed
Since this is not being uplifted, when do we mark it resolved?
It should actually get closed when it lands on central, whether or not it was getting uplifting.  Ryan probably just forgot.
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
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.
Flags: sec-bounty? → sec-bounty-
Whiteboard: [adv-main37+]
Alias: CVE-2015-0806
Whiteboard: [adv-main37+] → [adv-main37+][b2g-adv-main2.2+]
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.