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.
Do you recall the number of extra bytes?
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.
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.
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.
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.
(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.
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.
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.
Another option would be to disable the canvas support for generating ICO, but it would be simpler to just fix this.
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.
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.
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.
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.
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 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.
The uninitialized memory could contain sensitive information.
By the way, if approval is given I can land these today.
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.
> While you're in here, may as well remove that extra space. That was fixed in the 2 patches pushed out by the way.
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
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?
> 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.
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)
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).
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.
Alright. Strange that there were no reports. Feel free to post another bug since it isn't related to this bug.