710079 - (CVE-2012-0447) When using imgITools.encodeImage to encode image/vnd.microsoft.icon as png, output is always the same size and contains uninitialized bytes

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Reproducer: Encodes a file as image/vnd.microsoft.icon

I've been using the attached javascript to encode various images as image/vnd.microsoft.icon and write them to (.ico) files.  I've noticed a couple things:
   1) The output is always the exact same number of bytes (8214)
   2) After the end of the valid data, the rest of the file is full of 0xcd bytes in debug builds and (apparently) uninitialized data in release builds - I think this data is uninitialized because converting the same image multiple times gives different results

Comment 1

[Brian R. Bondy [:bbondy]]

Thanks Tim.  
This is only with PNG ICOs right?  And the extra bytes only appear when debug? From your example it seems it uses the default encoding options which would only be PNG ICOs.

Comment 2

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

(In reply to Brian R. Bondy [:bbondy] from comment #1)
> Thanks Tim.  
> This is only with PNG ICOs right?

Yup this is only occurring with PNG ICOs.  I also used this same javascript to test outputting as "image/png" but the png files didn't have the extra bytes.

> And the extra bytes only appear when
> debug?

The extra bytes appear in both debug and release builds, always producing a file that is 8214 bytes long.  In debug the extra bytes are always repeating bytes of 0xcd (I believe MSVC sets uninitialized memory to this value in debug builds to make it easier to recognize) but in release they vary.

> From your example it seems it uses the default encoding options which
> would only be PNG ICOs.

Yup!

Comment 3

[Brian R. Bondy [:bbondy]]

Do you recall the number of extra bytes?

Comment 4

[Brian R. Bondy [:bbondy]]

Ignore that last question I found the reason why.

So it seems this bug is because nsPNGEncoder.cpp has a value in mImageBufferSize which is not the actual image size as assumed.
It starts off as:
mImageBufferSize = 8192;
And then is doubled each time it needs more.

So we should be using mImageBufferUsed instead of mImageBufferSize.
I don't think this is a huge deal, at least it would not cause crashes for example, but we will be currently writing out som uninitialized memory at the end of the PNG flies.

Comment 5

[Brian R. Bondy [:bbondy]]

So the problem is here:

> mContainedEncoder->GetImageBufferSize(&imageBufferSize);
> mImageBufferSize = ICONFILEHEADERSIZE + ICODIRENTRYSIZE + 
>                   imageBufferSize;
> mImageBufferStart = static_cast<PRUint8*>(moz_malloc(mImageBufferSize));


Need a new function called GetImageBufferUsed which I think will solve the problem.

Comment 6

[Brian R. Bondy [:bbondy]]

I just verified also and all my jump list cached items are 8KB.  This was not seen before because all of our tests are reftests and PNG will ignore data on the end of the file that is extra.

Comment 7

[Brian R. Bondy [:bbondy]]

CC'ing imelven to see if Comment 4 is a security issue that should be addressed in the FF9 beta which is where this would be first introduced.

Comment 8

[Ian Melven :imelven]

(In reply to Brian R. Bondy [:bbondy] from comment #7)
> CC'ing imelven to see if Comment 4 is a security issue that should be
> addressed in the FF9 beta which is where this would be first introduced.

i think the security issue here is that since the memory is uninitialized some possibly sensitive data might leak into the ICO depending on what happened to be in that memory previously.

looking at https://wiki.mozilla.org/Security_Severity_Ratings this seems like somewhere between an sg:moderate and an sg:low depending on what data is leaked - so i would say we should go with sg:moderate to be on the more cautious side. i'll ask dveditz for his thoughts also.

Comment 9

[Daniel Veditz [:dveditz]]

Is it possible for arbitrary web content to cause us to use this encoder in a way that the attacker gets to read bits of the victim's memory? Maybe something like seed a canvas with a same-origin ICO image and then get the PNG data back as a data URI?

Is it only the ICO->PNG conversion that has the problem, or anything using the PNG encoder? Or anything using any of our image encoders? If it's easily accessible to content this is probably an [sg:high] bug: you can read arbitrary amounts of freed heap by using lots of images, and even better using larger images if it's not just the ICO converter.

Comment 10

[Brian R. Bondy [:bbondy]]

You can use canvas' to data URL to produce such an ICO ya. I will get a patch for this today and request a review to try to make it into a beta soon.

Comment 11

[Brian R. Bondy [:bbondy]]

Another option would be to disable the canvas support for generating ICO, but it would be simpler to just fix this.

Comment 12

[Ian Melven :imelven]

changing to [sg:high] due to the concerns above in comment 9 from dveditz and bbondy's assessment that this is possible using canvas toDataUrl.

Comment 13

[Brian R. Bondy [:bbondy]]

cc'ing clegnitto since we will probably request this be in beta and aurora to avoid a situation like this: http://www.mozilla.org/security/announce/2008/mfsa2008-07.html

I will finish the patch today and request a review from Joe today as well.

Comment 14

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1.

Still waiting for my build to finish but here is an untested patch.
I will request review once my build is done and I've tested it.

Comment 15

[Daniel Veditz [:dveditz]]

Comment on attachment 581376 [details] [diff] [review]
Patch v1.

From IRC discussion, getImageBufferSize() was added for this feature in the first place, we should just make it do what makes sense rather than add a second method. joedrew suggests using the new name for clarity.

Comment 16

[Brian R. Bondy [:bbondy]]

Attachment: Patch 2.

No longer need the getImageBuffertSize since getImageBufferUsed is all that is needed for encoders that contain other encoders (ICO that contains PNG).

This passes all /image tests.  verified that jump list icons are now no longer 8KB each, they are the size they are supposed to be.

Comment 17

[Joe Drew (not getting mail)]

Comment on attachment 581440 [details] [diff] [review]
Patch 2.

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

::: image/encoders/jpeg/nsJPEGEncoder.cpp
@@ +221,5 @@
>    return NS_ERROR_NOT_IMPLEMENTED;
>  }
>  
> +// Returns the number of bytes in the image buffer used.
> +NS_IMETHODIMP  nsJPEGEncoder::GetImageBufferUsed(PRUint32 *aOutputSize)

While you're in here, may as well remove that extra space.

Comment 18

[Brian R. Bondy [:bbondy]]

Comment on attachment 581440 [details] [diff] [review]
Patch 2.

This patch is important because using canvas.toDataURL you can encode ICO files with uninitialized memory from content. (so anyone that displays an HTML page) 

The ability to encode icons is new to FF9 and was added for Win7 jump lists.

In addition to canvas being able to access this, we are also writing out icons for sites you visit to your hard drive.  The data at the end of the ICO is ignored so this wasn't noticed until now because the icons displayed properly.

Comment 19

[Brian R. Bondy [:bbondy]]

The uninitialized memory could contain sensitive information.

Comment 20

[Brian R. Bondy [:bbondy]]

By the way, if approval is given I can land these today.

Comment 21

[Brian R. Bondy [:bbondy]]

Pushed to m-c:
http://hg.mozilla.org/mozilla-central/rev/9fae755f5103
https://tbpl.mozilla.org/?rev=9fae755f5103

Comment 22

[Alex Keybl [:akeybl]]

Comment on attachment 581440 [details] [diff] [review]
Patch 2.

[Triage Comment]
9.0beta6 (our last beta) is now in the hands of QA. Since this likely wouldn't cause us to chemspill FF9, we'll instead reconsider taking this as a ride-along if a 9.0.1 is required.

Approving for aurora and tracking for FF9, however.

Comment 23

[Brian R. Bondy [:bbondy]]

Pushed to aurora:
http://hg.mozilla.org/releases/mozilla-aurora/rev/80eba9145df9
https://tbpl.mozilla.org/?tree=Mozilla-Aurora&rev=80eba9145df9

Comment 24

[Brian R. Bondy [:bbondy]]

> While you're in here, may as well remove that extra space.

That was fixed in the 2 patches pushed out by the way.

Comment 25

[u279076]

I'm having some problems figuring out how to get the testcase to work. Can someone please provide some instruction? I'm trying to test this in Firefox 11.0RC build 2. Thanks

Comment 26

[Brian R. Bondy [:bbondy]]

An easy way to test this is to create a new profile, browse some sites and wait a couple minutes for the jump list items to populate.  Then check in your profile directory and make sure the favicons are of different sizes, and are fairly small.
Example directory:
C:\Users\Brian\AppData\Local\Mozilla\Firefox\Profiles\5v27b1ib.Aurora\jumpListCache

Comment 27

[Jason Smith [:jsmith]]

Tried the test case given in comment 26 on FF 9.0.1, 11, 12, 13. In FF 9.0.1, I see the jumpListCache appear with icon files with a size of 9 KB. In FF 11/12/13, I don't see a jumpListCache appear at all. Any ideas on why I'm not seeing a jumpListCache appear on later versions of firefox?

Comment 28

[Brian R. Bondy [:bbondy]]

>  I see the jumpListCache appear with icon files with a size of 9 KB.

By the way because Windows rounds its numbers actually go into the file properties to see the exact file size (not size on disk).

>  Any ideas on why I'm not seeing a jumpListCache appear on later versions of firefox?

I think you are probably not waiting long enough for them to generate or else looking in the wrong profile directory.

Comment 29

[Brian R. Bondy [:bbondy]]

I think you have to wait for 2 jump lists to build because the first one finds the icons that are missing and puts the request async for the icons.  But they won't appear until the next one.  You can adjust the frequency of how fast it regenerates the list by modifying: browser.taskbar.lists.refreshInSeconds

Comment 30

[Jason Smith [:jsmith]]

Gotcha. Got a successful verification on FF 11 then. With the rounding on Win 7 64-bit, the icon size differences are:

- FF 9.0.1: 9 KB
- FF 11: 2 KB (actual: 1.34 KB)

Comment 31

[Jason Smith [:jsmith]]

Still running into issues though getting the jump list cache to appear, even though it worked once. I've changed the refresh timer to 1 second. Waited a total of ten minutes. I'm looking at the right profile too (checked it).

Comment 32

[Brian R. Bondy [:bbondy]]

I'm not quite sure what would happen at 1 second jump list builds but I think that's too fast. Try like 30 seconds, also are you browsing sites?  I haven't gotten any reports of jump list icons not working.

Comment 33

[Jason Smith [:jsmith]]

Switched it to 30 seconds. Did more browsing (hit the google sites, bostonglobe, cnn). Reloaded many of the tabs I had open also. Still didn't get the jumpListCache to appear on FF Aurora. It's been 20 minutes so far.

Comment 34

[Brian R. Bondy [:bbondy]]

Alright.  Strange that there were no reports.
Feel free to post another bug since it isn't related to this bug.