1167356 - (CVE-2015-2738) YCbCrImageDataDeserializer::ToDataSourceSurface using uninitialized memory

Status: RESOLVED FIXED

(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, YCbCrImageDataDeserializer::ToDataSourceSurface uses uninitialized memory at gfx\layers\YCbCrImageDataSerializer.cpp line 292 et seq.

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

  DataSourceSurface::MappedSurface map;
  result->Map(DataSourceSurface::MapType::WRITE, &map);

  gfx::ConvertYCbCrToRGB32(GetYData(), GetCbData(), GetCrData(),
                           map.mData,
                           0, 0, //pic x and y
                           GetYSize().width, GetYSize().height,
                           GetYStride(), GetCbCrStride(),
                           map.mStride,
                           gfx::YV12);
  result->Unmap();

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

Comment 1

[Andrew Comminos [:acomminos]]

Attachment: Don't modify surface if mapping fails in YCbCrImageDataDeserializer.

Comment 2

[Andrew Comminos [:acomminos]]

Comment on attachment 8609529 [details] [diff] [review]
Don't modify surface if mapping fails in YCbCrImageDataDeserializer.

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?

Again, depends on how consistently one can cause D2D to fail mapping a bitmap. If so, quite 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 supported branches.

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

No, but they would be trivial to make.

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

Unlikely to cause regressions or need any additional testing.

Comment 3

[Daniel Veditz [:dveditz]]

Comment on attachment 8609529 [details] [diff] [review]
Don't modify surface if mapping fails in YCbCrImageDataDeserializer.

I'm concerned that when we check this in that could inspire a hunt for us making the same mistake elsewhere. Do we? Oh yes... We need to fix the pattern, not just this bug. Other occurrences I found with a quick dxr search (not guaranteed to be complete):

https://dxr.mozilla.org/mozilla-central/source/gfx/2d/DrawTargetTiled.h#180
https://dxr.mozilla.org/mozilla-central/source/gfx/2d/FilterNodeSoftware.cpp#1098

https://dxr.mozilla.org/mozilla-central/source/gfx/gl/GLReadTexImageHelper.cpp#220
https://dxr.mozilla.org/mozilla-central/source/gfx/gl/GLReadTexImageHelper.cpp#291
https://dxr.mozilla.org/mozilla-central/source/gfx/gl/GLReadTexImageHelper.cpp#292
(above 3 -- MOZ_ALWAYS_TRUE asserts in debug, what protects release builds?)

https://dxr.mozilla.org/mozilla-central/source/gfx/layers/composite/TextRenderer.cpp#93
https://dxr.mozilla.org/mozilla-central/source/gfx/layers/composite/TextRenderer.cpp#154
https://dxr.mozilla.org/mozilla-central/source/gfx/layers/d3d11/TextureD3D11.cpp#849

https://dxr.mozilla.org/mozilla-central/source/gfx/2d/SourceSurfaceD2D1.cpp#185
https://dxr.mozilla.org/mozilla-central/source/gfx/2d/SourceSurfaceD2D1.cpp#218
https://dxr.mozilla.org/mozilla-central/source/gfx/layers/d3d11/CompositorD3D11.cpp#712
(different kind of Map() but Windows APIs also fail)

The TextureD3D11 one appears to also be bug 1167393. Let's be smart and not require a dozen separate bugs to fix all these.

Comment 4

[Milan Sreckovic [:milan] (needinfo for best results)]

I like it, let's see if we can catch all the cases.

Comment 5

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

This can still make it into beta 5 if a fix lands before Thursday morning. Are you aiming to fix this in 39 or in 40?

Comment 6

[Milan Sreckovic [:milan] (needinfo for best results)]

Let's get the full patch first, and we'll let you know tomorrow.

Comment 7

[Andrew Comminos [:acomminos]]

Attachment: Handle return value of DataSourceSurface::Map wherever possible.

This should properly check for the return result of DataSourceSurface::Map everywhere, including areas covered by bugs 1167320 and 1167393. Perhaps we should merge them into one big DataSourceSurface::Map bug?

Comment 8

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8617449 [details] [diff] [review]
Handle return value of DataSourceSurface::Map wherever possible.

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

::: gfx/2d/SourceSurfaceD2D1.cpp
@@ +181,5 @@
>      MOZ_CRASH("No support for Write maps on D2D1 DataSourceSurfaces yet!");
>    }
>  
>    D2D1_MAPPED_RECT map;
> +  if (FAILED(mBitmap->Map(D2D1_MAP_OPTIONS_READ, &map))) {

Is using FAILED macro correct thing to do here?

@@ +217,5 @@
>    MOZ_ASSERT(!mIsMapped);
>    if (mMapped) {
>      return;
>    }
> +  if (FAILED(mBitmap->Map(D2D1_MAP_OPTIONS_READ, &mMap))) {

Same question here?

Comment 9

[Andrew Comminos [:acomminos]]

(In reply to Milan Sreckovic [:milan] from comment #8)
> Is using FAILED macro correct thing to do here?

I believe so; it's recommended in the D2D docs for handling HRESULT errors, and we use it for various other checks when dealing with COM APIs.

Comment 10

[Milan Sreckovic [:milan] (needinfo for best results)]

Oh, I see - there are two different Map calls, the bool and the HRESULT?

Comment 11

[Andrew Comminos [:acomminos]]

Yup; both of which can fail to map. It isn't clear in Microsoft's documentation whether or not the rect gets set to null on failure, but I'd imagine it's better to be safe in any case.

Comment 12

[q1]

(In reply to Andrew Comminos [:acomminos] from comment #11)
> Yup; both of which can fail to map. It isn't clear in Microsoft's
> documentation whether or not the rect gets set to null on failure, but I'd
> imagine it's better to be safe in any case.

Yes. I can say that the similar API LockRect does not null the rect on failure under at least Windows XP, hence https://bugzilla.mozilla.org/show_bug.cgi?id=1166082 . In any case it's perilous to depend upon undocumented behavior.

Comment 13

[Bas Schouten (:bas.schouten)]

Comment on attachment 8617449 [details] [diff] [review]
Handle return value of DataSourceSurface::Map wherever possible.

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

As an added measure I wonder if we should add a constructor to MappedSurface that initializes it's members to 0.

::: gfx/2d/SourceSurfaceD2D1.cpp
@@ +181,5 @@
>      MOZ_CRASH("No support for Write maps on D2D1 DataSourceSurfaces yet!");
>    }
>  
>    D2D1_MAPPED_RECT map;
> +  if (FAILED(mBitmap->Map(D2D1_MAP_OPTIONS_READ, &map))) {

It is.

Comment 14

[Andrew Comminos [:acomminos]]

Comment on attachment 8617449 [details] [diff] [review]
Handle return value of DataSourceSurface::Map wherever possible.

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

>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 15

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

Comment on attachment 8617449 [details] [diff] [review]
Handle return value of DataSourceSurface::Map wherever possible.

sec-approval+ for trunk.

Once it is in, please nominate for affected branches (if it applies cleanly) or make branch specific patches and nominate them.

Comment 16

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

If this can land on m-c as soon as possible then I'd like to get it into the beta 5 build today.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2bb563092098

Comment 18

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

https://hg.mozilla.org/mozilla-central/rev/2bb563092098

Comment 19

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

Can you request uplift? Thanks!

Comment 20

[Andrew Comminos [:acomminos]]

Comment on attachment 8617449 [details] [diff] [review]
Handle return value of DataSourceSurface::Map wherever possible.

Approval Request Comment
[Feature/regressing bug #]: N/A
[User impact if declined]: Security issue, reading and writing to a potentially uninitialized location in memory.
[Describe test coverage new/current, TreeHerder]: Manual testing, mainly sanity checks and ensuring builds work.
[Risks and why]: Low risk, should only introduce problems if we accidentally depended on touching memory pointed to by uninitialized data on the stack.
[String/UUID change made/needed]: none

Thanks!

Comment 21

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

(In reply to Andrew Comminos [:acomminos] from comment #20)

> [Risks and why]: Low risk, should only introduce problems if we accidentally
> depended on touching memory pointed to by uninitialized data on the stack.

OK, maybe we could check on that. Let's uplift this to aurora then and let it have some time there. We may not get this into 39 but I'd like to know that we aren't causing regressions.

Milan what do you think here?

Comment 22

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

Comment on attachment 8617449 [details] [diff] [review]
Handle return value of DataSourceSurface::Map wherever possible.

Uplift to aurora. Not sure about beta.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b43df791dbaf

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8617449 [details] [diff] [review]
Handle return value of DataSourceSurface::Map wherever possible.

See comment 20. Note that if this is denied for Beta39, the ESR 39+ tracking status will need to be re-evaluated (and likely changed to 40+). And at that point, these nominations should wait to be considered until the next cycle.

Comment 25

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Liz Henry (:lizzard) from comment #21)
> (In reply to Andrew Comminos [:acomminos] from comment #20)
> 
> > [Risks and why]: Low risk, should only introduce problems if we accidentally
> > depended on touching memory pointed to by uninitialized data on the stack.
> 
> OK, maybe we could check on that. Let's uplift this to aurora then and let
> it have some time there. We may not get this into 39 but I'd like to know
> that we aren't causing regressions.
> 
> Milan what do you think here?

Not worried about the risk that Andrew came up with.  Vanishingly small chance of that.

Comment 26

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

Comment on attachment 8617449 [details] [diff] [review]
Handle return value of DataSourceSurface::Map wherever possible.

Approved for uplift to beta. Thanks Milan and Andrew for the extra info.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

This is hitting some trivial-looking conflicts on Beta, but it would be good for you to take a look since there's also the possibility of there being other branch-specific differences that need addressing.

Comment 28

[Andrew Comminos [:acomminos]]

Attachment: Handle return value of DataSourceSurface::Map wherever possible. (rebased on mozilla-beta)

Doesn't look like there are any additional calls to DataSourceSurface::Map on the beta branch. Here's a patch rebased onto mozilla-beta (missing one change to the Map patch on central).

Comment 31

[Ryan VanderMeulen [:RyanVM]]

Thanks for confirming :)

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c7499d2cdf32

Comment 33

[Ryan VanderMeulen [:RyanVM]]

Yesterday, Andrew and I discussed some possibly-missed cases on the beta uplift, so he's going to double-check that. Also, esr38/esr31 will need their own patches. He also said that D2D is the only case really affected by this bug, so setting the B2G branches to unaffected.

Comment 34

[Andrew Comminos [:acomminos]]

Attachment: Add return value checking for more instances of DataSourceSurface::Map. (mozilla-beta)

This patch for beta includes changes in the original patch lost in the imgFrame move, as well as a missed check for RasterImage.cpp. I'll post an additional patch for m-c in a moment that covers the RasterImage call.

Comment 35

[Andrew Comminos [:acomminos]]

Attachment: Check return value of DataSourceSurface::Map in RasterImage::CopyFrame.

This is an additional patch for m-c that covers the RasterImage change in the last commit.

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e3703bb3d64b
https://hg.mozilla.org/releases/mozilla-aurora/rev/8bb9305de24e
https://hg.mozilla.org/releases/mozilla-beta/rev/2a55a06804f5

You're still good doing the esr38/esr31 rebases? Just to catch any other branch-specific ones floating around from days of Gecko past :)

Comment 37

[Andrew Comminos [:acomminos]]

Yup, I'll get on it momentarily.

Comment 38

[Ryan VanderMeulen [:RyanVM]]

The trunk/aurora follow-up required a bustage fix:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d2ea0b17116
https://hg.mozilla.org/releases/mozilla-aurora/rev/421bb3bea4d7

Comment 39

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e3703bb3d64b
https://hg.mozilla.org/mozilla-central/rev/2d2ea0b17116

Comment 40

[Andrew Comminos [:acomminos]]

Attachment: Handle return value of DataSourceSurface::Map wherever possible. (ESR31)

Backported to ESR31.

Comment 41

[Andrew Comminos [:acomminos]]

Comment on attachment 8624226 [details] [diff] [review]
Handle return value of DataSourceSurface::Map wherever possible. (ESR31)

Ignore this, gfxCriticalError is not defined in the ESRs.

Comment 42

[Andrew Comminos [:acomminos]]

Attachment: Handle return value of DataSourceSurface::Map wherever possible. (ESR38)

Patch for ESR38.

Comment 43

[Andrew Comminos [:acomminos]]

Comment on attachment 8624226 [details] [diff] [review]
Handle return value of DataSourceSurface::Map wherever possible. (ESR31)

This should be fine for ESR31, actually; Logging.h is included by HelpersD2D.h. False alarm from building esr38 locally.

Comment 44

[Ryan VanderMeulen [:RyanVM]]

Thanks for doing the rebases :)

https://hg.mozilla.org/releases/mozilla-esr38/rev/7dcf2a400da0
https://hg.mozilla.org/releases/mozilla-esr31/rev/6d81ec591658

Comment 45

[Andrew Comminos [:acomminos]]

Attachment: Handle return value of DataSourceSurface::Map wherever possible. (ESR31)

Updated ESR31 with logging include.

Comment 46

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr31/rev/3290ae4fe9f3