Bug 1191463 (CVE-2015-7180)

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

RESOLVED FIXED in Firefox 41

Status

()

defect
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: q1, Assigned: jnicol)

Tracking

({csectype-uninitialized, sec-high})

39 Branch
mozilla43
Points:
---
Bug Flags:
sec-bounty +
in-testsuite ?

Firefox Tracking Flags

(firefox40 wontfix, firefox41+ fixed, firefox42+ fixed, firefox43+ fixed, firefox-esr3841+ 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)

Details

(Whiteboard: [post-critsmash-triage][adv-main41+][adv-esr38.3+])

Attachments

(1 attachment, 1 obsolete attachment)

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
Posted 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-
Posted 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+
https://hg.mozilla.org/mozilla-central/rev/91fe6c306c8a
Status: NEW → RESOLVED
Closed: 4 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.