Bug 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

RESOLVED FIXED in Firefox 10

Status

()

Core
ImageLib
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: TimAbraldes, Assigned: bbondy)

Tracking

Trunk
mozilla10
x86_64
Windows 7
Points:
---

Firefox Tracking Flags

(firefox8 unaffected, firefox9+ wontfix, firefox10+ fixed, firefox11+ verified, status1.9.2 unaffected)

Details

(Whiteboard: [sg:high][qa+])

Attachments

(2 attachments, 1 obsolete attachment)

Created attachment 581113 [details]
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
(Assignee)

Comment 1

6 years ago
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.
(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!
Summary: When using imgITools.encodeImage to encode image/vnd.microsoft.icon, output is always the same size and contains uninitialized bytes → When using imgITools.encodeImage to encode image/vnd.microsoft.icon as png, output is always the same size and contains uninitialized bytes
(Assignee)

Comment 3

6 years ago
Do you recall the number of extra bytes?
(Assignee)

Comment 4

6 years ago
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.
(Assignee)

Updated

6 years ago
Assignee: nobody → netzen
(Assignee)

Comment 5

6 years ago
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.
(Assignee)

Comment 6

6 years ago
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.
(Assignee)

Comment 7

6 years ago
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

6 years ago
(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.
Whiteboard: [sg:moderate]
(Assignee)

Updated

6 years ago
Group: mozilla-confidential

Updated

6 years ago
Group: mozilla-confidential → core-security
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.
(Assignee)

Comment 10

6 years ago
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.
(Assignee)

Comment 11

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

Comment 12

6 years ago
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.

Updated

6 years ago
Whiteboard: [sg:moderate] → [sg:high]

Updated

6 years ago
status-firefox10: --- → affected
status-firefox11: --- → affected
status-firefox8: --- → unaffected
status-firefox9: --- → affected
(Assignee)

Comment 13

6 years ago
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.
(Assignee)

Comment 14

6 years ago
Created attachment 581376 [details] [diff] [review]
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 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.
Attachment #581376 - Flags: feedback-
tracking-firefox10: --- → +
tracking-firefox11: --- → +
tracking-firefox9: --- → ?
(Assignee)

Comment 16

6 years ago
Created attachment 581440 [details] [diff] [review]
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.
Attachment #581376 - Attachment is obsolete: true
Attachment #581440 - Flags: review?(joe)
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.
Attachment #581440 - Flags: review?(joe) → review+
(Assignee)

Comment 18

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

Comment 19

6 years ago
The uninitialized memory could contain sensitive information.
(Assignee)

Comment 20

6 years ago
By the way, if approval is given I can land these today.
(Assignee)

Comment 21

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

Updated

6 years ago
tracking-firefox9: ? → +
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.
Attachment #581440 - Flags: approval-mozilla-beta?
Attachment #581440 - Flags: approval-mozilla-beta-
Attachment #581440 - Flags: approval-mozilla-aurora?
Attachment #581440 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 23

6 years ago
Pushed to aurora:
http://hg.mozilla.org/releases/mozilla-aurora/rev/80eba9145df9
https://tbpl.mozilla.org/?tree=Mozilla-Aurora&rev=80eba9145df9
(Assignee)

Updated

6 years ago
Target Milestone: --- → mozilla10
(Assignee)

Comment 24

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

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

Updated

6 years ago
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Updated

6 years ago
status-firefox10: affected → fixed
status-firefox11: affected → fixed
Whiteboard: [sg:high] → [sg:high][qa+]
status1.9.2: --- → unaffected
status-firefox9: affected → wontfix
Alias: CVE-2012-0447
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
(Assignee)

Comment 26

6 years ago
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
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?
(Assignee)

Comment 28

6 years ago
>  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.
(Assignee)

Comment 29

6 years ago
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
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)
status-firefox11: fixed → verified
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).
(Assignee)

Comment 32

6 years ago
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.
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.
(Assignee)

Comment 34

6 years ago
Alright.  Strange that there were no reports.
Feel free to post another bug since it isn't related to this bug.
Group: core-security
You need to log in before you can comment on or make changes to this bug.