1063733 - (CVE-2014-1580) Apparent use of uninitialized memory when rendering truncated GIFs to <canvas>

Status: VERIFIED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Michal Zalewski]

Hey,

This seems reminiscent of bug 1045977, but affects 32.0:

http://lcamtuf.coredump.cx/ffgif2/

The page creates multiple Image() instances of a truncated GIF (./id:000158,orig:000158,src:000140.gif) repeatedly and then renders them on <canvas>. In my testing, this results in the missing (transparent) region being rendered in 2-8 distinct ways, suggesting the use of uninitialized memory (and the risk of leaking secrets across domains).

/mz

Comment 1

[Michal Zalewski]

Attachment: variants.png

Screenshot from a run

Comment 2

[Michael Wu [:mwu]]

This comes from SourceSurfaceAlignedRawData which doesn't zero its buffer when initialized. RasterImage::CopyFrame calls Factory::CreateDataSourceSurface which setups a SourceSurfaceAlignedRawData. I believe CreateDataSourceSurface should start with a zeroed buffer. The API that it replaces (gfxImageSurface) always zeroed its buffer.

Comment 3

[Michael Wu [:mwu]]

Bas, do you think comment 2 is reasonable?

Comment 4

[Bas Schouten (:bas.schouten)]

(In reply to Michael Wu [:mwu] from comment #3)
> Bas, do you think comment 2 is reasonable?

It does. But I disagree we should be zeroing it. The thing is that it's a complete waste to do that when the calling API might very well be going to write to every single pixel, there's no need to spend that memory bandwidth :). Callers should be responsible for zero-ing, in my opinion.

Comment 5

[Michael Wu [:mwu]]

The thing is, the caller *can't* efficiently zero it. On large allocations, the OS gives us zeroed pages, but there's no easy way to determine this on the caller side. calloc is the most efficient way to get zeroed buffers.

Comment 6

[Michael Wu [:mwu]]

Attachment: Add support for using calloc when allocating surfaces

I was thinking of making a small change to zero out the surface on the imagelib side, but this makes the code more efficient and obscures what's going on a little better. I also found another path in the image that doesn't zero when it needs to, but I haven't checked how one would hit that path.

About why calloc is more efficient than memsetting - allocations that are larger than half a page are very likely to be allocated through mmap which always returns zeroed pages. jemalloc will not memset that memory when allocating through calloc. Half a page on most systems is 2048 bytes, which can store a 32 x 16 pixel RGBA image. So for most reasonably sized images, we get zeroing for free.

Also worth mentioning - I tried value initialization to zero the array, but g++ didn't seem to use calloc.

Comment 7

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8486801 [details] [diff] [review]
Add support for using calloc when allocating surfaces

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

Looks good!

Note that I only reviewed RasterImage.cpp, imgTools.cpp, and enough of the other files to understand what the new parameter to CreateDataSourceSurface() does.

::: image/src/RasterImage.cpp
@@ +864,5 @@
>    // rect, implicitly padding the frame out to the image's size.
>  
>    IntSize size(mSize.width, mSize.height);
>    RefPtr<DataSourceSurface> surf =
> +    Factory::CreateDataSourceSurface(size, SurfaceFormat::B8G8R8A8, true);

Nit: it's preferable to state what boolean parameters mean with a comment, like so:

Factory::CreateDataSourceSurface(size, SurfaceFormat::B8G8R8A8, /* aZero = */ true);

Comment 8

[Bas Schouten (:bas.schouten)]

Comment on attachment 8486801 [details] [diff] [review]
Add support for using calloc when allocating surfaces

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

::: gfx/2d/Tools.h
@@ +150,5 @@
>        return;
>      }
>      // We don't create an array of T here, since we don't want ctors to be
>      // invoked at the wrong places if we realign below.
> +    if (aZero) {

I find this a little iffy, since now we sort of have magic in here that doesn't really work and breaks down when you try to use zero and use a non-POD type. I'd prefer if the callers just memset rather than passing a bool to realloc.

Comment 9

[Bas Schouten (:bas.schouten)]

(In reply to Bas Schouten (:bas.schouten) from comment #8)
> Comment on attachment 8486801 [details] [diff] [review]
> Add support for using calloc when allocating surfaces
> 
> Review of attachment 8486801 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/2d/Tools.h
> @@ +150,5 @@
> >        return;
> >      }
> >      // We don't create an array of T here, since we don't want ctors to be
> >      // invoked at the wrong places if we realign below.
> > +    if (aZero) {
> 
> I find this a little iffy, since now we sort of have magic in here that
> doesn't really work and breaks down when you try to use zero and use a
> non-POD type. I'd prefer if the callers just memset rather than passing a
> bool to realloc.

Note that I understand in some cases, memset implies additional cost, but I feel unless we actually run into that being an issue(I've , it's fine to do it explicitly. Alternatively we could add some specialized method that only compiles for POD types for this if you feel strongly about it.

Comment 10

[Michael Wu [:mwu]]

I think in most cases, memset implies additional cost. FWIW I really rather use calloc.

I'm not sure what you mean by magic that doesn't work when using a non-POD type - initialization and allocation is separate, so it would work. Maybe if you expected zeroing to occur after the object is initialized, it would be strange, but I can't think of any situation where you'd want to memset after objects are initialized..

Comment 11

[Bas Schouten (:bas.schouten)]

Comment on attachment 8486801 [details] [diff] [review]
Add support for using calloc when allocating surfaces

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

Please split off the patch for the gfx/2d  portions from the portions in the rest of the codebase.

::: gfx/2d/Tools.h
@@ +102,5 @@
>      , mStorage(nullptr)
>    {
>    }
>  
> +  explicit MOZ_ALWAYS_INLINE AlignedArray(size_t aCount, bool aZero)

I'd prefer false as a default param here personally.

@@ +150,5 @@
>        return;
>      }
>      // We don't create an array of T here, since we don't want ctors to be
>      // invoked at the wrong places if we realign below.
> +    if (aZero) {

I am plain wrong, this will be wasteful for non-POD types, but not broken. So it's fine. Sorry.

Comment 12

[Michael Wu [:mwu]]

(In reply to Bas Schouten (:bas.schouten) from comment #11)
> Please split off the patch for the gfx/2d  portions from the portions in the
> rest of the codebase.
> 

I intentionally mixed in a bunch of different changes in here so the change in imglib would be less visible. Could even pass as an optimization only fix. But if we don't care about that, I'll split the patch.

> ::: gfx/2d/Tools.h
> @@ +102,5 @@
> >      , mStorage(nullptr)
> >    {
> >    }
> >  
> > +  explicit MOZ_ALWAYS_INLINE AlignedArray(size_t aCount, bool aZero)
> 
> I'd prefer false as a default param here personally.
> 

I've added false as the default param to the constructor and Realloc().

> @@ +150,5 @@
> >        return;
> >      }
> >      // We don't create an array of T here, since we don't want ctors to be
> >      // invoked at the wrong places if we realign below.
> > +    if (aZero) {
> 
> I am plain wrong, this will be wasteful for non-POD types, but not broken.
> So it's fine. Sorry.

No problem, thanks for the review!

Comment 13

[Michael Wu [:mwu]]

Attachment: Add support for using calloc when allocating surfaces, v2 (for Aurora / Gecko 34)

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

It may take a bit of work to figure out that this occurs when drawing to a canvas, and a bit more work to figure out how to generate an image to trigger this. Even then, the memory is very likely to be poisoned. If the image is too large, it's likely to be zeroed. The current testcase doesn't appear to produce anything but poisoned memory or zeroed memory, but I expect it to be possible when using a smaller image size.

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

I made this patch look like just an optimization with most changes focused on gfx/2d. I can open a bug for this optimization if it helps.

Which older supported branches are affected by this flaw?

B2G 2.0 / Gecko 32 is also affected.

If not all supported branches, which bug introduced the flaw?

This was introduced by bug 994081 which landed in gecko 32.

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

This patch applies cleanly to m-c and aurora.

I've ported the patch to beta - will attach.

It's more difficult to port this patch to B2G 2.0 / Gecko 32 - this patch depends on bug 1015785. It's possible to create a simple fix for this branch, but that would directly point to the problem.

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

I don't think it will cause regressions.

Comment 14

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

Bug 994081 didn't land on ESR31. Is there any reason to think ESR31 would be affected? (I ask because of the ambiguous status flag.)

sec-approval+ for trunk. We should take this on Aurora and Beta as well.

Comment 15

[Michael Wu [:mwu]]

Attachment: Add support for using calloc when allocating surfaces, v2 (for Beta / Gecko 33)

Comment 16

[Michael Wu [:mwu]]

Attachment: Add support for using calloc when allocating surfaces, v2 (unbitrotted, for m-c)

Got bitrotted.

Comment 17

[Michael Wu [:mwu]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f5ad5364a2df

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f5ad5364a2df

Comment 19

[Sylvestre Ledru [:Sylvestre]]

Michael, could you fill the uplift request for aurora & beta?
Thanks

Comment 20

[Michael Wu [:mwu]]

Comment on attachment 8487538 [details] [diff] [review]
Add support for using calloc when allocating surfaces, v2 (for Aurora / Gecko 34)

Approval Request Comment
[Feature/regressing bug #]: Regressed by bug 994081.
[User impact if declined]: This is a security bug that can potentially leak data.
[Describe test coverage new/current, TBPL]: None at the moment - will followup with a simple test after landing everywhere.
[Risks and why]: Low - we're zeroing buffers in more cases now.
[String/UUID change made/needed]: None

Comment 21

[Michael Wu [:mwu]]

Comment on attachment 8488084 [details] [diff] [review]
Add support for using calloc when allocating surfaces, v2 (for Beta / Gecko 33)

Approval Request Comment
See comment 20.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c775b7328ab0
https://hg.mozilla.org/releases/mozilla-beta/rev/5982da7a1215

Please post a rebased patch for b2g32 uplift as well.

Comment 23

[Michael Wu [:mwu]]

I've requested approval for bug 1015785 and bug 1028802 which will make it possible to land the beta patch on b2g32.

Comment 25

[Aaron Train [:aaronmt]]

Verified FIXED on m-c Nightly (09/23) for Android based off the test-case in comment #0. Over to Anthony for desktop.

Comment 26

[Aaron Train [:aaronmt]]

(In reply to Aaron Train [:aaronmt] from comment #25)
> Verified FIXED on m-c Nightly (09/23) for Android based off the test-case in
> comment #0. Over to Anthony for desktop.

also verified fixed on mozilla-beta (33.0b6) & mozilla-aurora (09/23) for Android too

Comment 27

[bhavana bajaj [:bajaj]]

(In reply to Michael Wu [:mwu] from comment #23)
> I've requested approval for bug 1015785 and bug 1028802 which will make it
> possible to land the beta patch on b2g32.

I've approved the dependencies for b2g32, so this should be good to land now.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/4eaddf6ea12b

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/4eaddf6ea12b

Comment 30

[Kamil Jozwiak [:kjozwiak]]

Reproduced the original issue using the following build:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-09-11-03-02-04-mozilla-central/
- http://ftp.mozilla.org/pub/mozilla.org/firefox/releases/32.0.1/

- Visited the website mentioned in comment #0 over 10 times and received anywhere between 2-7 variants

OS's used for each channel:
- Win 8.1 x64 (VM) - Passed
- Ubuntu 13.10 x64 (VM) - Passed
- OSX 10.9.5 x64 - Passed (only tested m-c due to the v2 signing not being uplifted yet)

fx35:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-10-02-03-02-02-mozilla-central/

fx34:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-10-02-00-40-01-mozilla-aurora/

fx33:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/releases/33.0b8/

Test Cases Used:

- opened the website from comment #0 15 different times and ensured that the "The image has just one variant when rendered. You're probabky OK." prompt appears every single time.