Closed Bug 1186264 Opened 4 years ago Closed 4 years ago

Uninitialized variable in GrallocTextureHostOGL::GetAsSurface causes memory-safety bug

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(firefox42 fixed)

RESOLVED FIXED
Tracking Status
firefox42 --- fixed

People

(Reporter: q1, Assigned: sotaro)

Details

Attachments

(1 file)

User Agent: Mozilla/5.0 (Windows NT 5.1; rv:39.0) Gecko/20100101 Firefox/39.0
Build ID: 20150630154324

Steps to reproduce:

GrallocTextureHostOGL::GetAsSurface (gfx\layers\opengl\GrallocTextureHost.cpp) attempts to allocate and lock a graphics buffer, but then uses a pointer to the locked buffer without checking for failure. Since it does not initialize the pointer, and the pointer is an automatic variable, it is uninitialized, unless android::GraphicBuffer::lock (which appears to be undocumented, along with the rest of android::GraphicBuffer) sets it to nullptr on failure.

Apparently the buffer is only read, so potential damage may be limited to leakage of sensitive information and/or a crash.

231:  TemporaryRef<gfx::DataSourceSurface>
232:  GrallocTextureHostOGL::GetAsSurface() {
233:    android::GraphicBuffer* graphicBuffer = GetGraphicBufferFromDesc(mGrallocHandle).get();
234:    uint8_t* grallocData;
235:    graphicBuffer->lock(GRALLOC_USAGE_SW_READ_OFTEN, reinterpret_cast<void**>(&grallocData));
236:    RefPtr<gfx::DataSourceSurface> grallocTempSurf =
237:      gfx::Factory::CreateWrappingDataSourceSurface(grallocData,
238:                                                    graphicBuffer->getStride() * android::bytesPerPixel(graphicBuffer->getPixelFormat()),
239:                                                    GetSize(), GetFormat());
240:    RefPtr<gfx::DataSourceSurface> surf = CreateDataSourceSurfaceByCloning(grallocTempSurf);
241: 
242:    graphicBuffer->unlock();
243:
244:    return surf.forget();
245:  }
Flags: sec-bounty?
Assignee: nobody → sotaro.ikeda.g
Yea, error check to GrallocTextureHostOGL::GetAsSurface() is necessary. GrallocTextureHostOGL::GetAsSurface() is used when MOZ_DUMP_PAINTING is enabled.
Attachment #8640016 - Flags: review?(nical.bugzilla)
Comment on attachment 8640016 [details] [diff] [review]
patch - Add error check to GrallocTextureHostOGL::GetAsSurface()

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

As Sotaro said, this code is only used in debugging tools, so no need to flag the bug as security sensitive.
Attachment #8640016 - Flags: review?(nical.bugzilla) → review+
Group: core-security
Keywords: sec-moderate
https://hg.mozilla.org/mozilla-central/rev/eafffe750ebb
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Flags: sec-bounty? → sec-bounty-
You need to log in before you can comment on or make changes to this bug.