Bug 1167356 (CVE-2015-2738)

YCbCrImageDataDeserializer::ToDataSourceSurface using uninitialized memory

RESOLVED FIXED in Firefox 39, Firefox OS master

Status

()

Core
Graphics: Layers
RESOLVED FIXED
3 years ago
10 months ago

People

(Reporter: q1, Assigned: acomminos)

Tracking

({csectype-uninitialized, sec-high})

38 Branch
mozilla41
csectype-uninitialized, sec-high
Points:
---
Bug Flags:
sec-bounty +

Firefox Tracking Flags

(firefox38 wontfix, firefox38.0.5 wontfix, firefox39+ fixed, firefox40+ fixed, firefox41+ fixed, firefox-esr3139+ fixed, 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 fixed)

Details

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

Attachments

(6 attachments, 2 obsolete attachments)

11.21 KB, patch
bas
: review+
abillings
: sec-approval+
Details | Diff | Splinter Review
8.30 KB, patch
Details | Diff | Splinter Review
3.33 KB, patch
Details | Diff | Splinter Review
1.25 KB, patch
Details | Diff | Splinter Review
11.85 KB, patch
Details | Diff | Splinter Review
7.95 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

3 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, 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.

Updated

3 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(milan)
Keywords: csectype-uninitialized, sec-critical
Flags: needinfo?(milan)
Assignee: nobody → acomminos
(Assignee)

Comment 1

3 years ago
Created attachment 8609529 [details] [diff] [review]
Don't modify surface if mapping fails in YCbCrImageDataDeserializer.
(Assignee)

Updated

3 years ago
Attachment #8609529 - Flags: review?(gavin.sharp)
Attachment #8609529 - Flags: review?(gavin.sharp) → review?(nical.bugzilla)

Updated

3 years ago
Attachment #8609529 - Flags: review?(nical.bugzilla) → review+
status-firefox38: --- → wontfix
status-firefox38.0.5: --- → affected
status-firefox39: --- → affected
status-firefox40: --- → affected
status-firefox41: --- → affected
status-firefox-esr38: --- → affected
Component: Untriaged → Graphics: Layers
Product: Firefox → Core
(Assignee)

Comment 2

3 years ago
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.
Attachment #8609529 - Flags: sec-approval?
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.
Attachment #8609529 - Flags: sec-approval? → feedback-
Keywords: sec-critical → sec-high
status-b2g-v2.0: --- → affected
status-b2g-v2.0M: --- → affected
status-b2g-v2.1: --- → affected
status-b2g-v2.1S: --- → affected
status-b2g-v2.2: --- → affected
status-b2g-master: --- → affected
status-firefox-esr31: --- → affected
tracking-firefox39: --- → +
tracking-firefox40: --- → +
tracking-firefox41: --- → +
tracking-firefox-esr31: --- → 39+
tracking-firefox-esr38: --- → 39+
I like it, let's see if we can catch all the cases.
Flags: sec-bounty?
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?
Flags: needinfo?(milan)
Flags: needinfo?(acomminos)
Let's get the full patch first, and we'll let you know tomorrow.
(Assignee)

Comment 7

3 years ago
Created attachment 8617449 [details] [diff] [review]
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?
Attachment #8609529 - Attachment is obsolete: true
Attachment #8617449 - Flags: review?(bas)
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?
(Assignee)

Comment 9

3 years ago
(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.
Flags: needinfo?(acomminos)
Oh, I see - there are two different Map calls, the bool and the HRESULT?
Flags: needinfo?(milan)
(Assignee)

Comment 11

3 years ago
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.
(Reporter)

Comment 12

3 years ago
(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 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.
Attachment #8617449 - Flags: review?(bas) → review+
(Assignee)

Comment 14

3 years ago
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.
Attachment #8617449 - Flags: sec-approval?
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.
Attachment #8617449 - Flags: sec-approval? → sec-approval+
If this can land on m-c as soon as possible then I'd like to get it into the beta 5 build today.
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2bb563092098
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox41: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Can you request uplift? Thanks!
Flags: needinfo?(acomminos)
(Assignee)

Comment 20

3 years ago
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!
Flags: needinfo?(acomminos)
Attachment #8617449 - Flags: approval-mozilla-beta?
(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?
Flags: needinfo?(milan)
Comment on attachment 8617449 [details] [diff] [review]
Handle return value of DataSourceSurface::Map wherever possible.

Uplift to aurora. Not sure about beta.
Attachment #8617449 - Flags: approval-mozilla-aurora+
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.
Attachment #8617449 - Flags: approval-mozilla-esr38?
Attachment #8617449 - Flags: approval-mozilla-esr31?
status-b2g-master: affected → fixed
status-firefox38.0.5: affected → wontfix
(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.
Flags: needinfo?(milan)
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.
Attachment #8617449 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: sec-bounty? → sec-bounty+
Attachment #8617449 - Flags: approval-mozilla-esr38?
Attachment #8617449 - Flags: approval-mozilla-esr38+
Attachment #8617449 - Flags: approval-mozilla-esr31?
Attachment #8617449 - Flags: approval-mozilla-esr31+
Keywords: checkin-needed
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.
Flags: needinfo?(acomminos)
Keywords: checkin-needed
(Assignee)

Comment 28

3 years ago
Created attachment 8623059 [details] [diff] [review]
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).
(Assignee)

Updated

3 years ago
Flags: needinfo?(acomminos)
Keywords: checkin-needed
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1167320
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1167393
Thanks for confirming :)
Keywords: checkin-needed
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.
status-b2g-v2.0: affected → unaffected
status-b2g-v2.0M: affected → unaffected
status-b2g-v2.1: affected → unaffected
status-b2g-v2.1S: affected → unaffected
status-b2g-v2.2: affected → unaffected
(Assignee)

Comment 34

3 years ago
Created attachment 8623671 [details] [diff] [review]
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.
(Assignee)

Comment 35

3 years ago
Created attachment 8623679 [details] [diff] [review]
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.
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 :)
Flags: needinfo?(acomminos)
(Assignee)

Comment 37

3 years ago
Yup, I'll get on it momentarily.
Flags: needinfo?(acomminos)
(Assignee)

Comment 40

3 years ago
Created attachment 8624226 [details] [diff] [review]
Handle return value of DataSourceSurface::Map wherever possible. (ESR31)

Backported to ESR31.
(Assignee)

Comment 41

3 years ago
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.
Attachment #8624226 - Attachment is obsolete: true
(Assignee)

Comment 42

3 years ago
Created attachment 8624238 [details] [diff] [review]
Handle return value of DataSourceSurface::Map wherever possible. (ESR38)

Patch for ESR38.
(Assignee)

Comment 43

3 years ago
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.
Attachment #8624226 - Attachment is obsolete: false
Thanks for doing the rebases :)

https://hg.mozilla.org/releases/mozilla-esr38/rev/7dcf2a400da0
https://hg.mozilla.org/releases/mozilla-esr31/rev/6d81ec591658
status-firefox-esr31: affected → fixed
status-firefox-esr38: affected → fixed
(Assignee)

Comment 45

3 years ago
Created attachment 8624572 [details] [diff] [review]
Handle return value of DataSourceSurface::Map wherever possible. (ESR31)

Updated ESR31 with logging include.
Attachment #8624226 - Attachment is obsolete: true
Whiteboard: [adv-main39+][adv-esr38.1+][adv-esr31.8+]
Alias: CVE-2015-2738

Updated

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