Bug 1167332 (CVE-2015-2737)

rx::d3d11::SetBufferData using uninitialized memory

RESOLVED FIXED in Firefox 39

Status

()

defect
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: q1, Assigned: lsalzman)

Tracking

({csectype-uninitialized, sec-critical})

38 Branch
mozilla41
Points:
---
Bug Flags:
sec-bounty +

Firefox Tracking Flags

(firefox38 wontfix, firefox39+ fixed, firefox38.0.5 wontfix, firefox40+ fixed, firefox41+ fixed, firefox-esr31 unaffected, firefox-esr3839+ fixed, b2g-v2.0 unaffected, b2g-v2.0M unaffected, b2g-v2.1 unaffected, b2g-v2.1S unaffected, b2g-v2.2 unaffected, b2g-master unaffected)

Details

(Whiteboard: [adv-main39+][adv-esr38.1+])

Attachments

(1 attachment, 2 obsolete attachments)

Reporter

Description

4 years ago
User Agent: Mozilla/5.0 (Windows; rv:26.0) Gecko/20100101 Firefox/26.0
Build ID: 20150305021524

Steps to reproduce:

In Firefox 38.0.1, rx::d3d11::SetBufferData uses uninitialized memory at gfx\angle\src\libGLESv2\renderer\d3d\d3d11\renderer11_utils.h line 176.

The problem is that SetBufferData does not check the return value from the Windows function ID3D11DeviceContext::Map at line 174, thus (on failure) leaving untouched whatever was contained in the uninitialized variable mappedResource:

173:    D3D11_MAPPED_SUBRESOURCE mappedResource;
174:    context->Map(constantBuffer, 0, D3D11_MAP_WRITE_DISCARD, 0, &mappedResource);
175:
176:    memcpy(mappedResource.pData, &value, sizeof(T));

Since mappedResource can contain anything, this bug could make it possible to write data into anywhere in Firefox's address space. Depending on how SetBufferData is used, this bug might be exploitable to disclose sensitive data and/or cause execution of attacker-selected code.

Comment 1

4 years ago
We know Map can fail in OOM situations, which makes it likely that an attacker can force that to fail.

mappedResource is stack memory, which could make this very easy or hard to exploit depending on generated assembly and call patterns, but it's easier to call this a sg-crit and fix it.
Component: Untriaged → Graphics: Layers
Flags: needinfo?(bas)
Product: Firefox → Core
Assignee: nobody → lsalzman
Assignee

Comment 2

4 years ago
Attachment #8611346 - Flags: review?(jgilbert)
Component: Graphics: Layers → Canvas: WebGL
Comment on attachment 8611346 [details] [diff] [review]
Check result of Map in SetBufferData

Review of attachment 8611346 [details] [diff] [review]:
-----------------------------------------------------------------

I think this the right path, but that we need more than this. This could expose uninitialized data still, since it doesn't bail with an error or force context loss.
Attachment #8611346 - Flags: review?(jgilbert) → review-
Assignee

Comment 4

4 years ago
Re-run of patch. This version passes the failure out of SetBufferData so that the caller can then determine the best way to pass the failure up the chain. I also moved the call to SetBufferData before a lot of state on the device context is manipulated, so as to potentially leave the device in a nicer state on failure when a GL error is passed out of PixelTransfer11::copyBufferToTexture.
Attachment #8611346 - Attachment is obsolete: true
Attachment #8612866 - Flags: review?(jgilbert)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: sec-bounty?
Comment on attachment 8612866 [details] [diff] [review]
Pass failures out of SetBufferData

Review of attachment 8612866 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/angle/src/libGLESv2/renderer/d3d/d3d11/PixelTransfer11.cpp
@@ +211,5 @@
> +        {
> +             return gl::Error(GL_OUT_OF_MEMORY, "Failed to set shader parameters, result: 0x%X.", result);
> +        }
> +        mParamsData = shaderParams;
> +    }

Why did you move this around?
Flags: needinfo?(lsalzman)
Assignee

Comment 6

4 years ago
(In reply to Jeff Gilbert [:jgilbert] from comment #5)
> Comment on attachment 8612866 [details] [diff] [review]
> Pass failures out of SetBufferData
> 
> Review of attachment 8612866 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/angle/src/libGLESv2/renderer/d3d/d3d11/PixelTransfer11.cpp
> @@ +211,5 @@
> > +        {
> > +             return gl::Error(GL_OUT_OF_MEMORY, "Failed to set shader parameters, result: 0x%X.", result);
> > +        }
> > +        mParamsData = shaderParams;
> > +    }
> 
> Why did you move this around?

To leave the context in a cleaner state, so that if the error is triggered, it bails out before the other state is munged. The problem arises from updating this buffer unrelated to all the other state, so if that fails, it would theoretically be better to know as soon as possible...
Flags: needinfo?(lsalzman)
Tracking this for all affected versions since it's sec-critical.
Comment on attachment 8612866 [details] [diff] [review]
Pass failures out of SetBufferData

Review of attachment 8612866 [details] [diff] [review]:
-----------------------------------------------------------------

Cool.
Attachment #8612866 - Flags: review?(jgilbert) → review+
Assignee

Comment 10

4 years ago
Fix compilation error relating to deviceContext var needing to be assigned a bit earlier

Re-do of try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a1715a978378
Attachment #8612866 - Attachment is obsolete: true
Attachment #8617490 - Flags: review+
Assignee

Updated

4 years ago
Keywords: checkin-needed
Assignee

Updated

4 years ago
Status: NEW → ASSIGNED
Keywords: checkin-needed
Needs security approval before it can land since it's a sec-crit affecting multiple release.
https://wiki.mozilla.org/Security/Bug_Approval_Process
Keywords: checkin-needed
Assignee

Comment 12

4 years ago
Comment on attachment 8617490 [details] [diff] [review]
Pass failures out of SetBufferData

[Security approval request comment]
> 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?
Any in which we were using angle with the D3D 11 backend, presumably for canvas, but would be only confined to our Windows builds. Unsure exactly when this was introduced.

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Patch should work in all such branches.

> How likely is this patch to cause regressions; how much testing does it need?
Extremely unlikely. This patch only deals with failure cases that were already dubious, but leaves success-ful behavior essentially untouched.
Attachment #8617490 - Flags: sec-approval?
(In reply to Lee Salzman [:eihrul] from comment #12)
> > Which older supported branches are affected by this flaw?
> Any in which we were using angle with the D3D 11 backend, presumably for
> canvas, but would be only confined to our Windows builds. Unsure exactly
> when this was introduced.

Bug 1036068 (Gecko 35) if my hg archaeology is correct.
Comment on attachment 8617490 [details] [diff] [review]
Pass failures out of SetBufferData

sec-approval+ for trunk. We'll want this on affected branches including ESR ones as well.
Attachment #8617490 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/mozilla-central/rev/29cc4d75104b
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Lee can you request uplift? I'd like to get this into the next beta build on Monday morning if we can. Thanks!
Flags: needinfo?(lsalzman)
Assignee

Comment 18

4 years ago
Comment on attachment 8617490 [details] [diff] [review]
Pass failures out of SetBufferData

Approval Request Comment
[Feature/regressing bug #]: 1167332
[User impact if declined]: See bug
[Describe test coverage new/current, TreeHerder]: mochitests
[Risks and why]: See bug
[String/UUID change made/needed]:
Flags: needinfo?(lsalzman)
Attachment #8617490 - Flags: approval-mozilla-beta?
Attachment #8617490 - Flags: approval-mozilla-aurora?
(In reply to Lee Salzman [:eihrul] from comment #18)
> [Risks and why]: See bug
It seems that the bug number is missing here?!
Flags: needinfo?(lsalzman)
Assignee

Comment 20

4 years ago
(In reply to Sylvestre Ledru [:sylvestre] from comment #19)
> (In reply to Lee Salzman [:eihrul] from comment #18)
> > [Risks and why]: See bug
> It seems that the bug number is missing here?!

I just meant that the information here in this thread is contained in the sec approval request. But, that is to say, the impact is that the security bug remains in release if not uplifted, and the risk is a potential security exploit that allows writing arbitrary data to arbitrary addresses in memory.
Flags: needinfo?(lsalzman)
Comment on attachment 8617490 [details] [diff] [review]
Pass failures out of SetBufferData

Approved for uplift to aurora and beta since this is sec-critical. This didn't make it to beta 6 but will be in beta 7 later in the week.
Attachment #8617490 - Flags: approval-mozilla-beta?
Attachment #8617490 - Flags: approval-mozilla-beta+
Attachment #8617490 - Flags: approval-mozilla-aurora?
Attachment #8617490 - Flags: approval-mozilla-aurora+
Comment on attachment 8617490 [details] [diff] [review]
Pass failures out of SetBufferData

This is needed for esr38 as well.
Attachment #8617490 - Flags: approval-mozilla-esr38?
Flags: sec-bounty? → sec-bounty+
Keywords: checkin-needed
Attachment #8617490 - Flags: approval-mozilla-esr38? → approval-mozilla-esr38+
Whiteboard: [adv-main39+][adv-esr38.1+]
Alias: CVE-2015-2737

Updated

4 years ago
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.