Closed Bug 1186621 (CVE-2016-1944) Opened 9 years ago Closed 9 years ago

Memory-safety bug in Buffer11::NativeBuffer11::map

Categories

(Core :: Graphics, defect)

39 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
firefox42 --- wontfix
firefox43 + wontfix
firefox44 + fixed
firefox-esr38 44+ wontfix

People

(Reporter: q1, Assigned: jgilbert)

References

Details

(Keywords: csectype-wildptr, sec-high, Whiteboard: [b2g-adv-main2.5?][post-critsmash-triage][adv-main44+] fixed by ANGLE update bug 1179280)

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. ***
Flags: sec-bounty?
Marking sec-high. Adjust as needed.
I'll report this one to the Angle team.
The Angle team fixed this one in https://github.com/google/angle/commit/ab4fd98fac0cbc30cfc578a06c068519ee2df2e1 .
Let's mark this bug accordingly when that ANGLE fix shows up in Firefox.
Assignee: nobody → jgilbert
Group: core-security → gfx-core-security
Jeff, when are we planning on taking an ANGLE update?
Flags: needinfo?(jgilbert)
(In reply to Al Billings [:abillings] from comment #5)
> Jeff, when are we planning on taking an ANGLE update?

Some time in the next month. We should cherry-pick this.
Flags: needinfo?(jgilbert)
This was fixed with the ANGLE version that landed in bug 1179280 (train 44)
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Ron, this was previously reported to Angle and we were on track to take an update to Angle before this was filed. Thank you for flagging this issue for us though.
Flags: sec-bounty? → sec-bounty+
Group: gfx-core-security → core-security-release
Al, any idea how far back this goes? Looks like this might affect 38 and earlier, and thus previous b2g releases.
Flags: needinfo?(abillings)
Whiteboard: [b2g-adv-main2.5?]
No idea. That's a question for q1 or whomever fixes it. Ron?
Flags: needinfo?(abillings) → needinfo?(q1)
(In reply to Al Billings [:abillings] from comment #10)
> No idea. That's a question for q1 or whomever fixes it. Ron?

It goes back at least to http://hg.mozilla.org/mozilla-central/file/72a70862ee28/gfx/angle/src/libGLESv2/renderer/d3d11/BufferStorage11.cpp , which has this description:

72a70862ee28: b=1010371; Update ANGLE; r=upstream
Vladimir Vukicevic <vladimir@pobox.com> - Fri, 04 Jul 2014 11:49:25 -0400 - rev 192432
b=1010371; Update ANGLE; r=upstream

That changeset also includes the similar bug in copyFromStorage that I noted in comment 0.
Flags: needinfo?(q1)
[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.
Tracking for 43+. It is late to uplift this for 43 (the fix in bug 1179280). Milan what do you think?
Flags: needinfo?(milan)
If there is something we can do to mitigate the security risk we could uplift it for an RC build next week. If not, I don't think we should uplift the fix from 1179280 as it caused many regressions.
The timing for this is not optimal.  We don't have the attack vector, right?
Flags: needinfo?(milan)
(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.
Flags: needinfo?(milan)
Agreed.
Flags: needinfo?(milan)
Thank you Milan :)  The occasional bugzilla smiley has to happen....
Whiteboard: [b2g-adv-main2.5?] → [b2g-adv-main2.5?][post-critsmash-triage]
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.
Depends on: 1179280
Whiteboard: [b2g-adv-main2.5?][post-critsmash-triage] → [b2g-adv-main2.5?][post-critsmash-triage] fixed by ANGLE update bug 1179280
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.
Flags: needinfo?(milan)
Whiteboard: [b2g-adv-main2.5?][post-critsmash-triage] fixed by ANGLE update bug 1179280 → [b2g-adv-main2.5?][post-critsmash-triage][adv-main44] fixed by ANGLE update bug 1179280
Whiteboard: [b2g-adv-main2.5?][post-critsmash-triage][adv-main44] fixed by ANGLE update bug 1179280 → [b2g-adv-main2.5?][post-critsmash-triage][adv-main44+] fixed by ANGLE update bug 1179280
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.
Flags: needinfo?(milan)
Alias: CVE-2016-1944
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.