Closed Bug 1187459 Opened 9 years ago Closed 8 years ago

Memory-safety bug in Image11::map

Categories

(Core :: Graphics, defect)

39 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox45 --- fixed
firefox46 --- fixed
firefox-esr38 --- wontfix
firefox-esr45 --- fixed

People

(Reporter: q1, Assigned: jgilbert)

References

Details

(Keywords: sec-high)

Image11::map (gfx\angle\src\libGLESv2\renderer\d3d\d3d11\image11.cpp) does not properly report error status if its call to deviceContext -> Map fails due to a "device lost" error. Instead, it returns GL_NO_ERROR. This can cause a caller to use an uninitialized D3D11_MAPPED_SUBRESOURCE object. One such caller is Image11::loadData, which uses an uninitialized D3D11_MAPPED_SUBRESOURCE to determine where to copy the data specified by its |input| argument. This can cause loadData to overwrite an arbitrary (and possibly quite large) block of memory with data from |input|.

The bug is at line 613, which should be followed by something like |return result;|

588:gl::Error Image11::map(D3D11_MAP mapType, D3D11_MAPPED_SUBRESOURCE *map)
... [*map is not initialized]
608:    HRESULT result = deviceContext->Map(stagingTexture, subresourceIndex, mapType, 0, map);
609:
610:    // this can fail if the device is removed (from TDR)
611:    if (d3d11::isDeviceLostError(result))
612:    {
613:        mRenderer->notifyDeviceLost();
614:    }
615:    else if (FAILED(result))
616:    {
617:        return gl::Error(GL_OUT_OF_MEMORY, "Failed to map staging texture, result: 0x%X.", result);
618:    }
619:
620:    mDirty = true;
621:
622:    return gl::Error(GL_NO_ERROR);
623:}

The bug is activated by, e.g., loadData:

...
264:    D3D11_MAPPED_SUBRESOURCE mappedImage;
265:    gl::Error error = map(D3D11_MAP_WRITE, &mappedImage);
266:    if (error.isError())
267:    {
268:        return error;
269:    }
270:
271:    uint8_t* offsetMappedData = (reinterpret_cast<uint8_t*>(mappedImage.pData) + (yoffset * mappedImage.RowPitch + xoffset * outputPixelSize + zoffset * mappedImage.DepthPitch));
272:    loadFunction(width, height, depth,
273:                 reinterpret_cast<const uint8_t*>(input), inputRowPitch, inputDepthPitch,
274:                 offsetMappedData, mappedImage.RowPitch, mappedImage.DepthPitch);
275:
276:    unmap();
277:
278:    return gl::Error(GL_NO_ERROR);
279:}

mappedImage is neither explicitly nor implicitly initialized before line 265 passes it to map. Line 271 then uses it to compute the destination address for a copy operation, and line 272 calls loadFunction to do the copy (which it does without testing for the error status set by map at line 613).

I have verified the above by using the debugger to to skip line 608 and force isDeviceLostError on line 611 to return |true|.

Additionally, unmap() on line 276 probably does something bad if line 608 fails.
Flags: sec-bounty?
I'm not sure how hard it is to force a device-lost error, but this sounds pretty bad.
Keywords: sec-high
Just disable the graphics card in the system device panel should force a device lost error.  Chances are we'll crash before we can do a lot of harm, but perhaps not quickly enough.  We should check if this is still the case in the trunk ANGLE.
Flags: needinfo?(jgilbert)
(In reply to Milan Sreckovic [:milan] from comment #2)
> Just disable the graphics card in the system device panel should force a
> device lost error.  Chances are we'll crash before we can do a lot of harm,
> but perhaps not quickly enough.  We should check if this is still the case
> in the trunk ANGLE.

It's still there: github.com/google/angle/blob/master/src/libANGLE/renderer/d3d/d3d11/Image11.cpp . I will report this to Angle and post the bug number here.
Added kbrussel@alum.mit.edu to CC list at the request of the Angle team.
Thanks for CC'ing me. Do you have a test case which provokes this bug?
(In reply to Kenneth Russell from comment #6)
> Thanks for CC'ing me. Do you have a test case which provokes this bug?

No. I suppose that an attacker might cause the GPU driver to lose its mind and reset the device (= possible device-lost error) by using up all available texture memory, or possibly by causing the GPU to compute something very lengthy. Or she might leverage another bug in the GPU driver or OS that spuriously generates a device-lost error, as apparently is occurring in, e.g., http://steamcommunity.com/app/223470/discussions/1/882964760964387159/ and http://www.gamedev.net/topic/451670-persistent-d3derr_devicelost-problem-solved/ .
> Or she might leverage another bug in the GPU driver
> or OS that spuriously generates a device-lost error, as apparently is
> occurring in, e.g.,
> http://steamcommunity.com/app/223470/discussions/1/882964760964387159/ and
> http://www.gamedev.net/topic/451670-persistent-d3derr_devicelost-problem-
> solved/ .

What's occurring there is some bug that's generating a device-lost error, not an exploitation of this bug.
Flags: needinfo?(jgilbert)
Group: core-security → gfx-core-security
Flags: sec-bounty? → sec-bounty+
Ron, if you find a testcase for this, we can re-evaluate the bounty payment higher.
(In reply to Al Billings [:abillings] from comment #9)
> Ron, if you find a testcase for this, we can re-evaluate the bounty payment
> higher.

Thanks!
Assignee: nobody → jgilbert
Jeff, are you still working on this or can you help find an owner?
Flags: needinfo?(jgilbert)
Man, this fell through the cracks, sorry!

I see the bug, but it seems hard to act on. It's generally tricky to deterministically trigger a TDR, and I'm not sure how possible it would be to race to act on it before our internal mechanisms recognize the TDR occurred and forbid further WebGL calls.

This seems an impractical vulnerability to act on, though I won't rule it out.

This was fixed in a previous ANGLE update we did (bug 1211774):
https://hg.mozilla.org/mozilla-central/diff/0c0c6ebfa5a5/gfx/angle/src/libANGLE/renderer/d3d/d3d11/Image11.cpp#l639
Status: NEW → RESOLVED
Closed: 8 years ago
Flags: needinfo?(jgilbert)
Resolution: --- → FIXED
Group: gfx-core-security → core-security-release
Can't see us updating ANGLE on ESR 38 at this point given the lack of testing on that branch.
Depends on: 1211774
Target Milestone: --- → mozilla44
Marking qe-verify- based on comment 12 and complexity of testing without PoC or test.
Flags: qe-verify-
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.