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

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


(Core :: Graphics, defect)

39 Branch
Not set



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


(Reporter: q1, Assigned: jnicol)


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


(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:

42:  {
43:    D3D10_TEXTURE2D_DESC desc;
44:    mTask->mReadbackTexture->GetDesc(&desc);
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);
51:    if (FAILED(hr)) {
52:      mTask->mSink->ProcessReadback(nullptr);
53:    }
55:    {
56:      RefPtr<DataSourceSurface> surf =
57:        Factory::CreateWrappingDataSourceSurface((uint8_t*)mappedTex.pData, mappedTex.RowPitch,
58:                                                 IntSize(desc.Width, desc.Height),
59:                                                 SurfaceFormat::B8G8R8X8);
61:      mTask->mSink->ProcessReadback(surf);
63:      MOZ_ASSERT(surf->hasOneRef());
64:    }
66:    mTask->mReadbackTexture->Unmap(0);
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?


> Which older supported branches are affected by this flaw?


> 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+
Closed: 5 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.