1167393 - DataTextureSourceD3D11::Update using uninitialized memory

Status: RESOLVED DUPLICATE of bug 1167356

(Core :: Graphics: Layers, 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, DataTextureSourceD3D11::Update uses uninitialized memory at gfx\layers\d3d11\TextureD3D11.cpp line 644 et seq.

The problem is that DataTextureSourceD3D11::Update does not check the return value from DataSourceSurface::Map at line 630, thus (on failure and for some derived classes of DataSourceSurface, e.g., DataSourceSurfaceD2D) leaving untouched whatever was contained in the uninitialized variable map:

629:    DataSourceSurface::MappedSurface map;
630:    aSurface->Map(DataSourceSurface::MapType::READ, &map);
631:
632:    if (aDestRegion) {
      ...
644:      void* data = map.mData + map.mStride * iterRect->y + BytesPerPixel(aSurface->GetFormat()) * iterRect->x;
645:
646:      mCompositor->GetDC()->UpdateSubresource(mTexture, 0, &box, data, map.mStride, map.mStride * mSize.height);

Since map can contain anything, this bug could make it possible to read data from anywhere in Firefox's address space. Depending on how Update is used, this bug might be exploitable to disclose sensitive data.

I notice the following note at the beginning of Update indicating that this might not currently be a significant bug, but it's still worth fixing:

  // Incremental update with a source offset is only used on Mac so it is not
  // clear that we ever will need to support it for D3D.

This bug seems unrelated to the problem reported in https://bugzilla.mozilla.org/show_bug.cgi?id=1092167 .

Comment 1

[Andrew Comminos [:acomminos]]

Attachment: Check return value of mapping in DataTextureSourceD3D11. r=bas

Comment 2

[Andrew Comminos [:acomminos]]

Attachment: Check return value of mapping in DataTextureSourceD3D11.

Comment 3

[Bas Schouten (:bas.schouten)]

Comment on attachment 8610778 [details] [diff] [review]
Check return value of mapping in DataTextureSourceD3D11.

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

::: gfx/layers/d3d11/TextureD3D11.cpp
@@ +848,5 @@
>      DataSourceSurface::MappedSurface map;
> +    if (!aSurface->Map(DataSourceSurface::MapType::READ, &map)) {
> +      Reset();
> +      return false;
> +    }

gfxCriticalError() here too.

Comment 4

[Andrew Comminos [:acomminos]]

Attachment: Check return value of mapping in DataTextureSourceD3D11. Carry r=bas

Comment 5

[Andrew McCreight [:mccr8]]

Please remember to get sec-approval before landing on this and other patches that are sec-high or sec-critical and affect more than mozilla-central.

Comment 6

[Andrew Comminos [:acomminos]]

Attachment: Check return value of mapping in DataTextureSourceD3D11. Carry r=bas

Fixed incorrect result check.

Comment 7

[Andrew Comminos [:acomminos]]

Attachment: Check return value of mapping in DataTextureSourceD3D11. Carry r=bas

Fixed incorrect result check.

Comment 8

[Andrew Comminos [:acomminos]]

Comment on attachment 8611338 [details] [diff] [review]
Check return value of mapping in DataTextureSourceD3D11. Carry r=bas

[Security approval request comment]

>How easily could an exploit be constructed based on the patch?

On certain data source surface implementations (i.e. D2D) if the mapping fails, map is left untouched, potentially pointing anywhere. The triviality of developing an exploit for this is dependent on whether or not it is possible to cause some implementations of the source surface to fail mapping the backing texture.

>Do comments in the patch, the check-in comment, or tests included in the patch >paint a bulls-eye on the security problem?

No, the commit message is fairly unspecific.

>Which older supported branches are affected by this flaw?

All supported branches with DataTextureSourceD3D11.

>Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

No backported patch, it would be quite trivial to make one.

>How likely is this patch to cause regressions; how much testing does it need?

Unlikely to cause regressions. Shouldn't need any additional testing.

Comment 9

[Daniel Veditz [:dveditz]]

Sorry, Firefox OS branches won't have D3D11.

Comment 10

[Daniel Veditz [:dveditz]]

Comment on attachment 8611338 [details] [diff] [review]
Check return value of mapping in DataTextureSourceD3D11. Carry r=bas

Holding off on this until we can fix all the similar problems at the same time (noted in the other bug).

Comment 11

[Andrew Comminos [:acomminos]]

Fixes for DataSourceSurface::Map related bugs are handled in my patch for bug 1167356.

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Given that bug 1167356 is resolved as fixed and the fix is in all the streams, updating status-affected as "Fixed".