If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.
Bug 1099437 (CVE-2015-0806)

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

RESOLVED FIXED in Firefox 37, Firefox OS v2.2

Status

()

Core
Graphics: Layers
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: Abhishek Arya, Assigned: milan)

Tracking

({sec-moderate})

Trunk
mozilla37
sec-moderate
Points:
---
Bug Flags:
sec-bounty -

Firefox Tracking Flags

(firefox35 wontfix, firefox36 wontfix, firefox37 fixed, firefox-esr31 wontfix, b2g-v2.1 wontfix, b2g-v2.2 fixed)

Details

(Whiteboard: [adv-main37+][b2g-adv-main2.2+])

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

3 years ago
Created attachment 8523262 [details]
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?
status-firefox36: --- → affected
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)

Updated

3 years ago
Assignee: nobody → milan
(Assignee)

Comment 5

3 years ago
Created attachment 8525493 [details] [diff] [review]
Check for invalid sizes in the obvious places. r=nical

Let's see if this helps.
(Assignee)

Updated

3 years ago
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-
(Assignee)

Comment 8

3 years ago
Created attachment 8526269 [details] [diff] [review]
Part 1. Check for negative or overflow sizes. r=nical
Attachment #8525493 - Attachment is obsolete: true
Attachment #8526269 - Flags: review?(nical.bugzilla)
(Assignee)

Comment 9

3 years ago
Created attachment 8526270 [details] [diff] [review]
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.
Attachment #8526270 - Flags: feedback?(nical.bugzilla)

Updated

3 years ago
Attachment #8526270 - Flags: feedback?(nical.bugzilla) → feedback+

Updated

3 years ago
Attachment #8526269 - Flags: review?(nical.bugzilla) → review+
(Assignee)

Comment 10

3 years ago
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)

Updated

3 years ago
Attachment #8526270 - Flags: review?(nical.bugzilla) → review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1104254
This needs a security rating and possible sec-approval (depending on the rating) before it lands.
(Assignee)

Updated

3 years ago
Keywords: sec-moderate
(Assignee)

Comment 13

3 years ago
I'm assuming with sec-moderate, we don't need the approval?
(Assignee)

Updated

3 years ago
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.
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?
Flags: needinfo?(milan)
Keywords: checkin-needed
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
(Assignee)

Comment 19

3 years ago
Should be OK riding the trains.

Sorry about the bustage.  I was convinced I had a try run, but obviously not.
Flags: needinfo?(milan)
(Assignee)

Comment 20

3 years ago
Created attachment 8529439 [details] [diff] [review]
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.
Attachment #8526269 - Attachment is obsolete: true
Attachment #8529439 - Flags: review+
(Assignee)

Comment 21

3 years ago
Created attachment 8529441 [details] [diff] [review]
Part 2. Clean up a bit of int vs. uint usage. Carry r=nical

Fixed the compile error.  Carry r=nical
Attachment #8526270 - Attachment is obsolete: true
Attachment #8529441 - Flags: review+
(Assignee)

Comment 22

3 years ago
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...
(Assignee)

Comment 24

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

Updated

3 years ago
Keywords: checkin-needed
There's no way all of those reftest failures could be because of this patch, right?
Flags: needinfo?(milan)
They look real to me.
Keywords: checkin-needed, sec-moderate
Keywords: sec-moderate
(Assignee)

Comment 27

3 years ago
Created attachment 8533837 [details] [diff] [review]
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
Attachment #8529439 - Attachment is obsolete: true
Flags: needinfo?(milan)
Attachment #8533837 - Flags: review+
(Assignee)

Comment 28

3 years ago
Not sure if I quite believe this run, what with no intermittent oranges, but could be just luck...
Keywords: checkin-needed
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.
status-b2g-v2.1: --- → wontfix
status-b2g-v2.2: --- → affected
status-firefox35: --- → wontfix
status-firefox36: affected → wontfix
status-firefox37: --- → affected
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5597611e315f
https://hg.mozilla.org/mozilla-central/rev/c15637e90d9a
status-b2g-v2.2: affected → fixed
status-firefox37: affected → fixed
Target Milestone: --- → mozilla37
(Assignee)

Comment 31

3 years ago
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
Last Resolved: 3 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-
status-firefox-esr31: --- → wontfix
Whiteboard: [adv-main37+]
Alias: CVE-2015-0806
Whiteboard: [adv-main37+] → [adv-main37+][b2g-adv-main2.2+]

Updated

2 years ago
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.