1045977 - (CVE-2014-1564) Apparent info leak caused by uninitialized memory with malformed GIFs

Status: VERIFIED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Michal Zalewski]

Check this out:

http://lcamtuf.coredump.cx/ffgif/

The code repeatedly loads the same image via <img> and then puts it on <canvas>, comparing results. At least on my system, this generates somewhere between 4 and 8 distinct bitmaps. The differences are also apparent visually.

This feels awfully like the use of uninitialized memory; if that's the case, it sounds like a potential security-relevant info leak.

The image in question is:
http://lcamtuf.coredump.cx/ffgif/id:000110,src:000023.gif

Comment 1

[Andrew McCreight [:mccr8]]

johns is going to check if this test case does anything interesting with ASan.

Comment 2

[John Schoenick [:johns]]

This doesn't seem to reproduce in asan builds, with or without optimize/debug. In a non-asan build one interesting thing I saw was this variant of the image:

> data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAACAAAAAgCAYAAABzenr0AAAAR0lEQVRYhe3OoREAIAxD0WzAMGzTHZmtAlEZLArBVf7cfZt7kiTbsecqXfNjmTlsR0fqOgIAAAAAAAAAAAAAAAAAAAAA/HYA0VZS42M4bB0AAAAASUVORK5CYII=

Which has some random pixels in addition to the odd shade of grey, which definitely looks worrying

Comment 3

[John Schoenick [:johns]]

This also doesn't seem to reproduce in a debug/no-opt build without asan

Comment 4

[Andrew McCreight [:mccr8]]

johns was running on Linux and it seemed to happen fairly frequently. For me on OSX, I was only able to reproduce it once in a half dozen times, and there it only seemed to be a small difference.

Comment 5

[John Schoenick [:johns]]

Attachment: memorygif.png

So, this is the result of --disable-jemalloc, which also disables poisoning. ASAN does its own more thorough poisoning, so that could explain why the images were uniform in those builds.

Comment 6

[Andrew McCreight [:mccr8]]

Nick, if you wouldn't mind, could you see what this test case does in Valgrind?  Thanks.

Comment 7

[Nicholas Nethercote [inactive]]

Are we only supposed to see one variant?

Comment 8

[Nicholas Nethercote [inactive]]

Attachment: Valgrind errors

I got 163 variants(!) and heaps of undefined value error reports when running under Valgrind. They all seem to involve VolatileBuffers. See the attachment.

Note that each error has two parts:

- the "Conditional jump or move depends on uninitialised value(s)" part, which indicates where the undefined value was used in a dangerous way, and

- the "Uninitialised value was created by a heap allocation" part, which indicates where the undefined value came from.

For posterity, here are the Valgrind flags I used:

> valgrind --smc-check=all-non-file --vex-iropt-register-updates=allregs-at-mem-access --num-callers=36 --leak-check=no --track-o
rigins=yes

Finally, ASan won't be able to detect these errors. Undefined value errors are the main class of errors that Valgrind can detect but ASan cannot. (MSan is supposed to detect them, but I think it's still experimental.)

Comment 9

[Andrew McCreight [:mccr8]]

Thanks, Nick!

Comment 10

[Andrew McCreight [:mccr8]]

Could you find somebody to look into this, Milan?  Thanks.

Comment 11

[Jeff Muizelaar [:jrmuizel]]

I started looking at this a bit.

The image is truncated after the GIF_IMAGE_SEPARATOR. I suspect we are getting a little confused about whether we have a frame of the image or not.

Comment 12

[Jeff Muizelaar [:jrmuizel]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #11)
> I started looking at this a bit.
> 
> The image is truncated after the GIF_IMAGE_SEPARATOR. I suspect we are
> getting a little confused about whether we have a frame of the image or not.

It looks like the problem shown by valgrind is that VolatileBufferFallback::Init does not initialize it's buffer to 0

Comment 13

[Michael Wu [:mwu]]

Attachment: Clear heap allocated buffers

Not initializing the buffer to zero is fine - malloc does the same thing, but imgFrame isn't clearing the buffer when it needs to. This code used to have gfxImageSurface allocate the buffer, which would memset the data to zero. I missed this when switching the code to managing its own buffers.

Buffers do not need to be cleared when not heap allocated, since the memory comes directly from the kernel which zeros it for us.

Comment 14

[Michael Wu [:mwu]]

Comment on attachment 8471069 [details] [diff] [review]
Clear heap allocated buffers

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

I was looking into making the VolatileBuffer api provide a InitZeroed function, but that makes the fix a bit more complicated. I'll look into doing that after fixing this since I don't think exposing OnHeap() is good.

Comment 15

[Julian Seward [:jseward]]

(In reply to Michael Wu [:mwu] from comment #14)
> Comment on attachment 8471069 [details] [diff] [review]
> Clear heap allocated buffers

Is it possible for such buffers to be allocated on the stack instead?
If so, won't we need to clear those too?

Comment 16

[Michael Wu [:mwu]]

(In reply to Julian Seward [:jseward] from comment #15)
> Is it possible for such buffers to be allocated on the stack instead?
> If so, won't we need to clear those too?

VolatileBuffers are always allocated using system calls or a heap allocator.

Comment 17

[Nicholas Nethercote [inactive]]

Comment on attachment 8471069 [details] [diff] [review]
Clear heap allocated buffers

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

r=me for fixing the problem, but yes, a nicer follow-up is needed.

::: image/src/imgFrame.cpp
@@ +185,5 @@
> +      if (mVBuf->OnHeap()) {
> +        int32_t stride = VolatileSurfaceStride(mSize, mFormat);
> +        VolatileBufferPtr<uint8_t> ptr(mVBuf);
> +        memset(ptr, 0, stride * mSize.height);
> +      }

Move this new code before the CreateLockedSurface() call? Seems better to allocate the buffer and then immediately zero it.

Comment 18

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

Comment on attachment 8471069 [details] [diff] [review]
Clear heap allocated buffers

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

I agree with njn's comment, but otherwise looks good to me.

I am pretty sure I just hit this in another patch, bug 1054079, which makes us rasterize SVG images into imgFrame's instead of using SourceSurface's directly. The oranges are the result of uninitialized memory causing random noise in parts of the image where the SVG was totally transparent (and thus didn't draw anything).

Comment 19

[Michael Wu [:mwu]]

Is there any specific way I should land this, given that it's a security bug?

Comment 20

[Reed Loden [:reed]]

(In reply to Michael Wu [:mwu] from comment #19)
> Is there any specific way I should land this, given that it's a security bug?

Yes, follow the directions outlined at https://wiki.mozilla.org/Security/Bug_Approval_Process

Comment 21

[Michael Wu [:mwu]]

Attachment: Clear heap allocated buffers, v2

Review comment addressed.

Comment 22

[Michael Wu [:mwu]]

Attachment: Clear heap allocated buffers (For esr31/b2g30)

Comment 23

[Michael Wu [:mwu]]

Comment on attachment 8474064 [details] [diff] [review]
Clear heap allocated buffers, v2

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

It's relatively easy to get access to the uninitialized data through canvas, and canvas is the most obvious way to grab raw image data. The only non-obvious part is using a corrupt image to prevent the buffer from being overridden with decoded image data.

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

The check-in comment could be rephrased a bit if needed.

Which older supported branches are affected by this flaw?

ESR31, B2G30

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

Bug 962670

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

A different patch is required for Gecko 31/30. It's attached in the bug, and should work as long as it compiles.

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

Regressions unlikely, though it may affect performance in certain cases.

Comment 24

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

Comment on attachment 8474064 [details] [diff] [review]
Clear heap allocated buffers, v2

sec-approval+ for trunk.

Please nominate this for Aurora, Beta, and ESR31.

Comment 25

[Michael Wu [:mwu]]

[Blocking Requested - why for this release]: Security bug.

Comment 26

[Michael Wu [:mwu]]

Comment on attachment 8474064 [details] [diff] [review]
Clear heap allocated buffers, v2

[Approval Request Comment]
This is a sec-high bug. See comment 23 for details.

Comment 27

[Michael Wu [:mwu]]

Clearing blocking request - going to use the approval flag on the patch instead.

Comment 28

[Michael Wu [:mwu]]

Comment on attachment 8474724 [details] [diff] [review]
Clear heap allocated buffers (For esr31/b2g30)

Apparently imgFrame.cpp diverged a bit between 30 and 31, so I'm going to have a separate patch for b2g30.

Comment 29

[Michael Wu [:mwu]]

Comment on attachment 8474724 [details] [diff] [review]
Clear heap allocated buffers (For esr31/b2g30)

Nevermind - I mixed up the release branches. This does apply to b2g30.

Comment 30

[Michael Wu [:mwu]]

Comment on attachment 8474724 [details] [diff] [review]
Clear heap allocated buffers (For esr31/b2g30)

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:

This is a sec-high bug. See comment 23 for details.

Comment 31

[Lawrence Mandel [:lmandel] (use needinfo)]

We're late in beta. If this should land concurrently on all branches, we can try to land today in time for beta8. If not, this should get in beta9, which goes to build on Thu.

Comment 32

[Michael Wu [:mwu]]

Landing in beta 9 sounds good to me.

Comment 33

[Lawrence Mandel [:lmandel] (use needinfo)]

Does this need to land concurrently or can this land on m-c Tue and bake for a day or two before uplift?

Comment 34

[Andrew McCreight [:mccr8]]

It should be fine to land on trunk first.  That's what we usually do.

Comment 35

[Michael Wu [:mwu]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9833f4970436

Comment 36

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9833f4970436

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ab8b62a75e77
https://hg.mozilla.org/releases/mozilla-beta/rev/bff13e7445c5
https://hg.mozilla.org/releases/mozilla-esr31/rev/2437605df381
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/64ed70a322c9

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/bff13e7445c5

Comment 39

[Michael Wu [:mwu]]

This actually affect 1.3t, since volatile buffer images were backported there.

Comment 40

[Michael Wu [:mwu]]

Comment on attachment 8474724 [details] [diff] [review]
Clear heap allocated buffers (For esr31/b2g30)

[Approval Request Comment]
This is a sec-high bug that also affects 1.3t (but *not* 1.3). See comment 23 for details.

Comment 42

[Matt Wobensmith [:mwobensmith][:matt:]]

On builds from around the time this was reported, I get between 2-4 variants. However, using builds post-fix from 2014-08-20, I consistently get 1 variant.

While it's an improvement, does this mean we've successfully mitigated this? FWIW the image itself does not appear visually different to my eye.

Comment 43

[Michael Wu [:mwu]]

I'm pretty sure this is fixed for the test case posted here if you're only getting one variant. If you want to be more thorough, test on Windows(8.1 and pre-8.1)/Linux/OSX/Android, because there are slightly different volatile buffer code paths in each one. Getting one variant consistently is the important part.

Comment 44

[Kamil Jozwiak [:kjozwiak]]

As mentioned in comment #43, only a single variant should always appear when loading the POC from comment #0, went through the following verification:

Reproduced the original issue using the following build:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-07-29-03-02-02-mozilla-central/
- Received anywhere between 2-6 variants until it finally reach 1 varriant
- Uninstalled/Installed several times and kept receiving different variaent sizes

Win 8.1 x64 (VM): PASSED
Win 7 Pro SP2 x64 (VM): PASSED
Ubuntu 13.10 x64 (VM): PASSED
OSX 10.9.4 x64: PASSED

For each of the above OS's, I went through the following test cases:

fx 34.0a1:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-08-26-03-02-03-mozilla-central/
- loaded the poc 20x (uninstalling/re-installing fx once I reached 10x) and always received 1 variant

fx 33.0a2:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-08-26-00-40-03-mozilla-aurora/
- loaded the poc 20x (uninstalling/re-installing fx once I reached 10x) and always received 1 variant

fx 32:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/releases/latest-beta/
- loaded the poc 20x (uninstalling/re-installing fx once I reached 10x) and always received 1 variant

fx 31.0esrpre:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-08-26-00-05-03-mozilla-esr31/
- loaded the poc 20x (uninstalling/re-installing fx once I reached 10x) and always received 1 variant

Comment 45

[Michael Wu [:mwu]]

Comment on attachment 8474724 [details] [diff] [review]
Clear heap allocated buffers (For esr31/b2g30)

Clearing request since it seems too late for b2g28.