Closed Bug 1191463 (CVE-2015-7180) Opened 9 years ago Closed 9 years ago

Mishandling return status in ReadbackResultWriterD3D11::Run might cause memory-safety bug

Categories

(Core :: Graphics, defect)

39 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox40 --- wontfix
firefox41 + fixed
firefox42 + fixed
firefox43 + fixed
firefox-esr38 41+ fixed
b2g-v2.0 --- unaffected
b2g-v2.0M --- unaffected
b2g-v2.1 --- unaffected
b2g-v2.1S --- unaffected
b2g-v2.2 --- unaffected
b2g-v2.2r --- unaffected
b2g-master --- unaffected

People

(Reporter: q1, Assigned: jnicol)

Details

(Keywords: csectype-uninitialized, reporter-external, sec-high, Whiteboard: [post-critsmash-triage][adv-main41+][adv-esr38.3+])

Attachments

(1 file, 1 obsolete file)

ReadbackResultWriterD3D11::Run (gfx\layers\d3d11\ReadbackManagerD3D11.cpp) mishandles failure status from ID3D10Texture2D::Map. This might cause it to use a pointer and other data from uninitialized memory to create a surface, which is then used to read (and possibly write?) data from/to memory: 41: NS_IMETHODIMP Run() 42: { 43: D3D10_TEXTURE2D_DESC desc; 44: mTask->mReadbackTexture->GetDesc(&desc); 45: 46: D3D10_MAPPED_TEXTURE2D mappedTex; 47: // We know this map will immediately succeed, as we've already mapped this 48: // copied data on our task thread. 49: HRESULT hr = mTask->mReadbackTexture->Map(0, D3D10_MAP_READ, 0, &mappedTex); 50: 51: if (FAILED(hr)) { 52: mTask->mSink->ProcessReadback(nullptr); 53: } 54: 55: { 56: RefPtr<DataSourceSurface> surf = 57: Factory::CreateWrappingDataSourceSurface((uint8_t*)mappedTex.pData, mappedTex.RowPitch, 58: IntSize(desc.Width, desc.Height), 59: SurfaceFormat::B8G8R8X8); 60: 61: mTask->mSink->ProcessReadback(surf); 62: 63: MOZ_ASSERT(surf->hasOneRef()); 64: } 65: 66: mTask->mReadbackTexture->Unmap(0); 67: 68: return NS_OK; 69: } First lines 47-8 assert that Map cannot fail, but then lines 51-3 attempt to handle failure. However, they do so imcompletely (should there be a "return <some status>;" following line 52? Or has an "else" accidentally been elided from line 54?) leaving lines 57-9 using the uninitialized struct mappedTex. Despite the comments on line 47-8, probably OOM, device failure, and possibly other conditions can cause Map to fail even under the conditions under which Run runs.
Flags: sec-bounty?
Assignee: nobody → jnicol
Attached patch Patch v1 (obsolete) — Splinter Review
Bas, are you okay to review this? Attached patch just adds a early return if the map fails, and updates the comment. Is it a problem that we do not handle the potential Map failure in ReadbackManagerD3D11::ProcessTasks()? In its case we only call Unmap() rather than read from mappedTex.
Attachment #8649250 - Flags: review?(bas)
Comment on attachment 8649250 [details] [diff] [review] Patch v1 Review of attachment 8649250 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/d3d11/ReadbackManagerD3D11.cpp @@ +49,5 @@ > HRESULT hr = mTask->mReadbackTexture->Map(0, D3D10_MAP_READ, 0, &mappedTex); > > if (FAILED(hr)) { > mTask->mSink->ProcessReadback(nullptr); > + return NS_ERROR_FAILURE; You need to return NS_OK here, the Run() function returning an error is only for very critical failures, this is potentially a situation we can deal with.
Attachment #8649250 - Flags: review?(bas) → review-
Attached patch Patch v2Splinter Review
Return NS_OK instead of NS_ERROR_FAILURE.
Attachment #8649369 - Flags: review?(bas)
Attachment #8649250 - Attachment is obsolete: true
Attachment #8649369 - Attachment description: Handle possible ID3D10Texture2D::Map() failure → Patch v2
Attachment #8649369 - Flags: review?(bas) → review+
Comment on attachment 8649369 [details] [diff] [review] Patch v2 I wasn't sure if this needs sec approval, so asking just in case. Sorry if it doesn't. [Security approval request comment] > How easily could an exploit be constructed based on the patch? Not easily > Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No > Which older supported branches are affected by this flaw? All > If not all supported branches, which bug introduced the flaw? > Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Patch applies cleanly to all supported branches > How likely is this patch to cause regressions; how much testing does it need? Very unlikely, very little
Attachment #8649369 - Flags: sec-approval?
It sounds like the analysis in comment 0 is accurate, and we're dereferencing an uninitialized value, so I'm going to mark this sec-high. Thanks for the patch and the bug report.
Comment on attachment 8649369 [details] [diff] [review] Patch v2 a=dveditz for sec-approval and older branches. Please also check this into aurora, beta, and ESR38
Attachment #8649369 - Flags: sec-approval?
Attachment #8649369 - Flags: sec-approval+
Attachment #8649369 - Flags: approval-mozilla-esr38+
Attachment #8649369 - Flags: approval-mozilla-beta+
Attachment #8649369 - Flags: approval-mozilla-aurora+
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Flags: sec-bounty? → sec-bounty+
Group: core-security → core-security-release
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main41+][adv-esr38.3+]
Alias: CVE-2015-7180
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: