Bug 1063733 (CVE-2014-1580)

Apparent use of uninitialized memory when rendering truncated GIFs to <canvas>

VERIFIED FIXED in Firefox 33

Status

()

defect
VERIFIED FIXED
5 years ago
2 years ago

People

(Reporter: lcamtuf, Assigned: mwu)

Tracking

({sec-high})

32 Branch
mozilla35
x86_64
Windows 7
Points:
---
Bug Flags:
sec-bounty +
in-testsuite ?
qe-verify +

Firefox Tracking Flags

(firefox32 wontfix, firefox33+ verified, firefox34+ verified, firefox35+ verified, firefox-esr31 unaffected, b2g-v1.4 unaffected, b2g-v2.0 fixed, b2g-v2.0M fixed, b2g-v2.1 fixed, b2g-v2.2 fixed)

Details

(Whiteboard: [adv-main33+])

Attachments

(4 attachments, 1 obsolete attachment)

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
Posted image variants.png
Screenshot from a run
Assignee: nobody → mwu
Component: Security → ImageLib
Product: Firefox → Core
See Also: → CVE-2014-1564
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.
Bas, do you think comment 2 is reasonable?
Flags: needinfo?(bas)
(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.
Flags: needinfo?(bas)
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.
Keywords: sec-high
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.
Attachment #8486801 - Flags: review?(seth)
Attachment #8486801 - Flags: review?(bas)
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);
Attachment #8486801 - Flags: review?(seth) → review+
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.
(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.
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 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.
Attachment #8486801 - Flags: review?(bas) → review+
(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!
[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.
Attachment #8486801 - Attachment is obsolete: true
Attachment #8487538 - Flags: sec-approval?
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.
Attachment #8487538 - Flags: sec-approval? → sec-approval+
Attachment #8487538 - Attachment description: Add support for using calloc when allocating surfaces, v2 → Add support for using calloc when allocating surfaces, v2 (for Aurora / Gecko 34)
https://hg.mozilla.org/mozilla-central/rev/f5ad5364a2df
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Michael, could you fill the uplift request for aurora & beta?
Thanks
Flags: needinfo?(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
Attachment #8487538 - Flags: approval-mozilla-aurora?
Flags: needinfo?(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.
Attachment #8488084 - Flags: approval-mozilla-beta?
Attachment #8488084 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8487538 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
I've requested approval for bug 1015785 and bug 1028802 which will make it possible to land the beta patch on b2g32.
Flags: needinfo?(mwu)
Duplicate of this bug: 1067982
Verified FIXED on m-c Nightly (09/23) for Android based off the test-case in comment #0. Over to Anthony for desktop.
(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
(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.
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Whiteboard: [adv-main33+]
Alias: CVE-2014-1580
Flags: sec-bounty?
Flags: sec-bounty? → sec-bounty+
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.