1167332 - (CVE-2015-2737) rx::d3d11::SetBufferData using uninitialized memory

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[q1]

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

[Benjamin Smedberg]

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.

Comment 2

[Lee Salzman [:lsalzman]]

Attachment: Check result of Map in SetBufferData

Comment 3

[Kelsey Gilbert [:jgilbert]]

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.

Comment 4

[Lee Salzman [:lsalzman]]

Attachment: Pass failures out of SetBufferData

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.

Comment 5

[Kelsey Gilbert [:jgilbert]]

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?

Comment 6

[Lee Salzman [:lsalzman]]

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

Comment 7

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking this for all affected versions since it's sec-critical.

Comment 8

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8612866 [details] [diff] [review]
Pass failures out of SetBufferData

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

Cool.

Comment 9

[Lee Salzman [:lsalzman]]

Try run for patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c5f69d6c0642

Comment 10

[Lee Salzman [:lsalzman]]

Attachment: Pass failures out of SetBufferData

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Needs security approval before it can land since it's a sec-crit affecting multiple release.
https://wiki.mozilla.org/Security/Bug_Approval_Process

Comment 12

[Lee Salzman [:lsalzman]]

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.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/29cc4d75104b

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/29cc4d75104b

Comment 17

[Liz Henry (:lizzard) (relman/hg->git project)]

Lee can you request uplift? I'd like to get this into the next beta build on Monday morning if we can. Thanks!

Comment 18

[Lee Salzman [:lsalzman]]

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

Comment 19

[Sylvestre Ledru [:Sylvestre]]

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

Comment 20

[Lee Salzman [:lsalzman]]

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

Comment 21

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

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

This is needed for esr38 as well.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/970295c94145
https://hg.mozilla.org/releases/mozilla-beta/rev/9060127fc5ae

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr38/rev/bbac190ad1e7