Bug 1045977 (CVE-2014-1564)

Apparent info leak caused by uninitialized memory with malformed GIFs

VERIFIED FIXED in Firefox 32

Status

()

defect
VERIFIED FIXED
5 years ago
2 years ago

People

(Reporter: lcamtuf, Assigned: mwu)

Tracking

({csectype-uninitialized, sec-high})

31 Branch
mozilla34
x86_64
All
Points:
---
Bug Flags:
sec-bounty +
qe-verify +

Firefox Tracking Flags

(firefox31 wontfix, firefox32+ verified, firefox33+ verified, firefox34+ verified, firefox-esr24 unaffected, firefox-esr3132+ verified, b2g-v1.3 unaffected, b2g-v1.3T affected, b2g-v1.4 fixed, b2g-v2.0 fixed, b2g-v2.1 fixed)

Details

(Whiteboard: [reporter-external][adv-main32+][adv-esr31.1+])

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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
Component: Security → ImageLib
Product: Firefox → Core
Flags: sec-bounty?
Whiteboard: [reporter-external]
johns is going to check if this test case does anything interesting with ASan.
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
This also doesn't seem to reproduce in a debug/no-opt build without asan
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.
OS: Windows 7 → All
Posted image 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.
Nick, if you wouldn't mind, could you see what this test case does in Valgrind?  Thanks.
Flags: needinfo?(n.nethercote)
Are we only supposed to see one variant?
Flags: needinfo?(n.nethercote)
Summary: Apparent uninitialized memory leak with malformed GIFs → Apparent info leak caused by uninitialized memory with malformed GIFs
Posted file 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.)
Thanks, Nick!
Could you find somebody to look into this, Milan?  Thanks.
Flags: needinfo?(milan)
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.
Assignee: nobody → jmuizelaar
(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
Assignee: jmuizelaar → mwu
(Assignee)

Comment 13

5 years ago
Posted patch Clear heap allocated buffers (obsolete) — Splinter Review
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.
(Assignee)

Comment 14

5 years ago
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.
Attachment #8471069 - Flags: review?(seth)
Attachment #8471069 - Flags: review?(n.nethercote)
(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?
(Assignee)

Comment 16

5 years ago
(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 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.
Attachment #8471069 - Flags: review?(n.nethercote) → review+
Flags: needinfo?(milan)
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).
Attachment #8471069 - Flags: review?(seth) → review+
(Assignee)

Comment 19

5 years ago
Is there any specific way I should land this, given that it's a security bug?
(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
(Assignee)

Comment 21

5 years ago
Review comment addressed.
Attachment #8471069 - Attachment is obsolete: true
(Assignee)

Comment 23

5 years ago
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.
Attachment #8474064 - Flags: sec-approval?
Comment on attachment 8474064 [details] [diff] [review]
Clear heap allocated buffers, v2

sec-approval+ for trunk.

Please nominate this for Aurora, Beta, and ESR31.
Attachment #8474064 - Flags: sec-approval? → sec-approval+
(Assignee)

Comment 25

5 years ago
[Blocking Requested - why for this release]: Security bug.
blocking-b2g: --- → 2.0?
(Assignee)

Comment 26

5 years ago
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.
Attachment #8474064 - Flags: approval-mozilla-beta?
Attachment #8474064 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 27

5 years ago
Clearing blocking request - going to use the approval flag on the patch instead.
blocking-b2g: 2.0? → ---
(Assignee)

Comment 28

5 years ago
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.
Attachment #8474724 - Attachment description: Clear heap allocated buffers (For esr31/b2g30) → Clear heap allocated buffers (For esr31)
(Assignee)

Comment 29

5 years ago
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.
Attachment #8474724 - Attachment description: Clear heap allocated buffers (For esr31) → Clear heap allocated buffers (For esr31/b2g30)
(Assignee)

Comment 30

5 years ago
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.
Attachment #8474724 - Flags: approval-mozilla-esr31?
Attachment #8474724 - Flags: approval-mozilla-b2g30?
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.
(Assignee)

Comment 32

5 years ago
Landing in beta 9 sounds good to me.
Does this need to land concurrently or can this land on m-c Tue and bake for a day or two before uplift?
It should be fine to land on trunk first.  That's what we usually do.
https://hg.mozilla.org/mozilla-central/rev/9833f4970436
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Attachment #8474724 - Flags: approval-mozilla-esr31? → approval-mozilla-esr31+
Attachment #8474064 - Flags: approval-mozilla-beta?
Attachment #8474064 - Flags: approval-mozilla-beta+
Attachment #8474064 - Flags: approval-mozilla-aurora?
Attachment #8474064 - Flags: approval-mozilla-aurora+
Attachment #8474724 - Flags: approval-mozilla-b2g30?
(Assignee)

Comment 39

5 years ago
This actually affect 1.3t, since volatile buffer images were backported there.
(Assignee)

Comment 40

5 years ago
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.
Attachment #8474724 - Flags: approval-mozilla-b2g28?
Flags: sec-bounty? → sec-bounty+
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.
Flags: needinfo?(mwu)
(Assignee)

Comment 43

5 years ago
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.
Flags: needinfo?(mwu)
Whiteboard: [reporter-external] → [reporter-external][adv-main32+][adv-esr31.1+]
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
Alias: CVE-2014-1564
See Also: → CVE-2014-1580
(Assignee)

Comment 45

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

Clearing request since it seems too late for b2g28.
Attachment #8474724 - Flags: approval-mozilla-b2g28?

Updated

4 years ago
Group: core-security → core-security-release
Group: core-security-release

Updated

2 years ago
Duplicate of this bug: 1420168
You need to log in before you can comment on or make changes to this bug.