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)
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)
1.46 KB,
patch
|
bas.schouten
:
review+
dveditz
:
approval-mozilla-aurora+
dveditz
:
approval-mozilla-beta+
dveditz
:
approval-mozilla-esr38+
dveditz
:
sec-approval+
|
Details | Diff | Splinter Review |
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.
Updated•9 years ago
|
Flags: sec-bounty?
Updated•9 years ago
|
Assignee: nobody → jnicol
Updated•9 years ago
|
Keywords: sec-vector
Updated•9 years ago
|
Keywords: sec-vector
Assignee | ||
Comment 1•9 years ago
|
||
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 2•9 years ago
|
||
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-
Assignee | ||
Comment 3•9 years ago
|
||
Return NS_OK instead of NS_ERROR_FAILURE.
Attachment #8649369 -
Flags: review?(bas)
Assignee | ||
Updated•9 years ago
|
Attachment #8649250 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8649369 -
Attachment description: Handle possible ID3D10Texture2D::Map() failure → Patch v2
Updated•9 years ago
|
Attachment #8649369 -
Flags: review?(bas) → review+
Assignee | ||
Comment 4•9 years ago
|
||
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?
Comment 5•9 years ago
|
||
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.
Keywords: csectype-uninitialized,
sec-high
Updated•9 years ago
|
status-firefox40:
--- → wontfix
status-firefox41:
--- → affected
status-firefox42:
--- → affected
status-firefox43:
--- → affected
status-firefox-esr38:
--- → affected
tracking-firefox41:
--- → +
tracking-firefox42:
--- → +
tracking-firefox43:
--- → +
Comment 6•9 years ago
|
||
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+
Comment 7•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/91fe6c306c8a
https://hg.mozilla.org/releases/mozilla-aurora/rev/9294d377bbc5
https://hg.mozilla.org/releases/mozilla-beta/rev/2a38ca9cbfad
https://hg.mozilla.org/releases/mozilla-esr38/rev/aa2ecb8673b1
status-b2g-v2.0:
--- → unaffected
status-b2g-v2.0M:
--- → unaffected
status-b2g-v2.1:
--- → unaffected
status-b2g-v2.1S:
--- → unaffected
status-b2g-v2.2:
--- → unaffected
status-b2g-v2.2r:
--- → unaffected
status-b2g-master:
--- → unaffected
tracking-firefox-esr38:
--- → ?
Comment 8•9 years ago
|
||
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Updated•9 years ago
|
Flags: sec-bounty? → sec-bounty+
Updated•9 years ago
|
Updated•9 years ago
|
Group: core-security → core-security-release
Updated•9 years ago
|
Whiteboard: [post-critsmash-triage]
Updated•9 years ago
|
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main41+][adv-esr38.3+]
Updated•9 years ago
|
Alias: CVE-2015-7180
Updated•9 years ago
|
Group: core-security-release
Updated•8 months ago
|
Keywords: reporter-external
You need to log in
before you can comment on or make changes to this bug.
Description
•