Security

(public)

User Story

Buffer11::NativeBuffer11::map (gfx\angle\src\libGLESv2\renderer\d3d\d3d11\Buffer11.cpp) can cause its callers to use a wild pointer to read and/or write data to memory. This is because it calls ID3D11DeviceContext::Map without checking the return code (in release builds). It then returns the resulting mappedResource.pData to its callers, which use it to read and/or write data into memory that (on success) would be mapped to a GPU buffer. But mappedResource is an automatic variable with no explicit initialization, and it isa D3D11_MAPPED_SUBRESOURCE, which lacks a constructor and so does not initialize itself. Thus, on failure (which is possible: see https://msdn.microsoft.com/en-us/library/windows/desktop/ff476457%28v=vs.85%29.aspx ) callers use a wild pointer to read and/or write data to memory.
788: void *Buffer11::NativeBuffer11::map(size_t offset, size_t length, GLbitfield access)
789: {
790: ASSERT(mUsage == BUFFER_USAGE_STAGING);
791:
792: D3D11_MAPPED_SUBRESOURCE mappedResource;
793: ID3D11DeviceContext *context = mRenderer->getDeviceContext();
794: D3D11_MAP d3dMapType = gl_d3d11::GetD3DMapTypeFromBits(access);
795: UINT d3dMapFlag = ((access & GL_MAP_UNSYNCHRONIZED_BIT) != 0 ? D3D11_MAP_FLAG_DO_NOT_WAIT : 0);
796:
797: HRESULT result = context->Map(mNativeBuffer, 0, d3dMapType, d3dMapFlag, &mappedResource);
798: UNUSED_ASSERTION_VARIABLE(result);
799: ASSERT(SUCCEEDED(result));
800:
801: return static_cast<GLubyte*>(mappedResource.pData) + offset;
802: }
There are reports of ID3D11DeviceContext::Map returning error in other applications in OOM situations (e.g, https://community.amd.com/thread/128535 ) , and when there are underlying graphics driver bugs (e.g., http://answers.flyppdevportal.com/categories/metro/gameswithdirectx.aspx?ID=7ff91f34-f8e0-4197-a5d7-991e136ae939 ).
*** There is also a similar bug in Buffer11::NativeBuffer11::copyFromStorage in the same module, but other code in the same module, e.g., Buffer11::getData, handles this issue correctly. ***

[Tracking Requested - why for this release]: This is a sec-high bug fixed in an upstream library (and possibly disclosed by them). We need to cherry-pick this patch (not the whole ANGLE update) for ESR 38 when it ships on mainline, and we should consider pulling this up to Firefox 43.

(In reply to Milan Sreckovic [:milan] from comment #15)
> The timing for this is not optimal. We don't have the attack vector, right?
I don't know of a way to exploit this bug, but I haven't explored it extensively. The data that would be read and/or written from/into the wild pointer source/destination is the kind of data that ordinarily would be exchanged between D3D and a GPU. That data is mostly derived from web-provided data, so an attacker has good (but not perfect) control over it. Where it goes in memory depends on what's on the stack at the time of the attack. That might be pretty predictable -- or not. I also don't know how easy it is for an attacker to cause the hingepin fault -- ID3D11DeviceContext::Map failure.
FWIW, the Angle team published their fix for this bug in https://github.com/google/angle/commit/ab4fd98fac0cbc30cfc578a06c068519ee2df2e1 , on 5/15/2015, with the commit label "Small fixes for passing code analysis tools", so someone who has been following Angle and FF commits would know about it.

Is this still an issue in 43? I think we will have to mark this as wontfix.
If there is a very low risk and well tested option to uplift a fix to 43.0.2 next week, that's not impossible, but I'd prefer not to take any more changes at this point.

We need to decide for ESR 38.6 whether to cherry-pick the fix in comment 16 or to WONTFIX. Setting esr tracking to 44+ as a decision point -- either fix it the same time as trunk or not, let's not just accidentally miss it.

Thanks Daniel. I did miss this as I did not have time for esr triage. Milan do you have feedback here? I am planning to go to build for esr 38.6.0 on Thursday. If you think that you can come up with a low risk fix for ESR we could still fix this. If not then please wontfix for esr38.

That's a scary change to cherry pick, there have been too many changes in the code interacting with ANGLE on our side, to just pick it up. It'd probably be safe, but not worth a risk for 38. WontFix for ESR38.