2.65 KB, text/plain
421 bytes, application/java-archive
976 bytes, text/html
1.38 KB, patch
Stuart Parmenter: superreview+
Christopher Aillon (sabbatical, not receiving bugmail): approval1.8.0.next+
|Details | Diff | Splinter Review|
Created attachment 292779 [details] provided code This is the code that Michael provided in his initial report.
Created attachment 292780 [details] test image This is the example image Michael provided in his initial email.
Created attachment 292782 [details] test image For some reason the test image I uploaded previously doesn't seem to work. Trying again.
Comment on attachment 292782 [details] test image Strange, for some reason Bugzilla (or Firefox) is modifying the attachment when I upload it. It's 314 bytes locally.
Created attachment 292786 [details] modified testcase This is a testcase based on the example code provided by Michael.
Yep - we should fix - any takers? :-)
Created attachment 292914 [details] [diff] [review] possible (incorrect) patch I'm not really familiar with this code, but this patch seems to fix the bug. mNumColors is calculated based on either mBIH.bpp or mBIH.colors (which comes from the image), and then we never check whether the image data conforms to that range of colors. The only obvious way to do this was to do it in SetPixel as we're decoding the data - perhaps not optimal, but I don't see a better way. This uses color at index 0 if the passed-in color is outside the range of mNumColors - that's probably still not safe, because it looks like mColors' initialization depends entirely on the image data which could again be corrupt. With the given test image the 0 color appears to be black, which matches what I see when I open the image in IE (IE doesn't fail to load the image).
Safari and Opera (shipping versions on Windows) also seem to have this bug. I'll look into filing bugs on them as well.
Created attachment 292916 [details] [diff] [review] better possible patch This one just uses black (0,0,0) for invalid pixels, rather than relying on mColors. I suppose the question to answer is whether it's possible to produce a BMP file that doesn't initialize mColors (at http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/modules/libpr0n/decoders/bmp/nsBMPDecoder.cpp&rev=1.37&mark=303-327#303 ) - if it is, then there's still the possibility of reading uninitialized memory, even with this patch. Hoping someone who knows more can comment.
The patch itself looks good. There is no way to not have mColors uninitialized in the range 0 < mNumColors, as the parser always reads 3*mNumColors bytes from the image data. But there could be an easier way to safeguard this: 1. Allocate always a mColors table for 8-bit, so with 256 entries, even if mNumColors < 256 2. To always clear the whole table to 0 (using memset(mColors, 0, 256*sizeof(colorTable)) In this way the SetPixel doesn't need to check the boundary (as Idx is a PRUint8 and thus always between 0 and 255)
Hi, Imho Alfred Kayser's idea would be better (faster, and still safe). However, Gavin Sharp's patch is still correct, it fixes the issue. Regards, Michael 'Gynvael Coldwind' Skladnikiewicz
Created attachment 292931 [details] [diff] [review] patch You're right Alfred, that is a better fix.
The ICO decoder doesn't have this bug because it always uses the maximum value for mNumColors based on mBIH.bpp.
Actually this patch won't fix the reported problem completely. In the current code mNumColors is set to "1 << mBIH.bpp", assuming that the color table is large enough to cover all possible pixel values for resp. 1, 4 and 8 bpp (note that MSDN also says that the colormap could be used for bpp's 16 and 32...) But, as the report in comment #1 says, the actual number of colorvalues in the table in the file is specified by biClrUser (if not zero) MSDN: "biClrUser specifies the number of color indexes in the color table that are actually used by the bitmap. If this value is zero, the bitmap uses the maximum number of colors corresponding to the value of the biBitCount member for the compression mode specified by biCompression. If biClrUsed is nonzero and the biBitCount member is less than 16, the biClrUsed member specifies the actual number of colors the graphics engine or device driver accesses.". So, what actually needs to happen is that the mColors table is allocated according to bpp (as the pixel values coming from the image data are garanteed to be 1, 4 or 8 bit wide), but the decoder need to check for biClrUsed !=0 and in that case, only read that number colors from the file, and clear the table for the rest. Note, that this is probably not really a security issue, as the decoder will now try to always read 256 color entries for the table, even if biClrUsed is < 256, resulting in using some of the actual image data itself for the colortable, and then the image data is itself will be not complete. So, the image will look like garbage, but one will not be able address any uninitialized values in the colortable... (but keep the 'security-sensitive' flag if really needed...). So, now at least the colortable cleared and sane, but the reading of the image is not completely perfect. P.s., the example code set biClrUsed to 'i', which is probably '256' given the code above? This is strange??
Comment on attachment 292931 [details] [diff] [review] patch Need to add check for biClrUsed value !=0.
Tip: Instead of: mNumColors = 1 << mBIH.bpp; if (mBIH.colors && mBIH.colors < mNumColors) mNumColors = mBIH.colors; mColors = new PRUint32[mNumColors]; if (!mColors) return NS_ERROR_OUT_OF_MEMORY; memset(mColors, 0, 256 * sizeof(colorTable)); Do: mNumColors = 1 << mBIH.bpp; mColors = new PRUint32[mNumColors]; if (!mColors) return NS_ERROR_OUT_OF_MEMORY; memset(mColors, 0, mNumColors * sizeof(colorTable)); if (mBIH.colors && mBIH.colors < mNumColors) mNumColors = mBIH.colors; As noted before, the index is never more than allowed by the 'bpp', but mBIH.colors can specify a smaller table. In the proposed way we allocate and clear the table according to bpp, and only read values according to mBIH.colors (if non-zero).
Comment on attachment 292931 [details] [diff] [review] patch (In reply to comment #18) > In the proposed way we allocate and clear the table according to bpp, and only > read values according to mBIH.colors (if non-zero). My current patch already does that... it doesn't change the value of mNumColors, which is what we use to determine how many color entries to read. It merely changes how much memory we allocate to hold any potential color values, and zeroes it to ensure no UMR. I don't understand your other comment, it sounds like a spec compliance issue that was pre-existing and is unrelated to this bug.
(In reply to comment #16) > So, what actually needs to happen is that the mColors table is allocated > according to bpp (as the pixel values coming from the image data are garanteed > to be 1, 4 or 8 bit wide), but the decoder need to check for biClrUsed !=0 and > in that case, only read that number colors from the file, and clear the table > for the rest. That's already what happens... biClrUsed is mBIH.colors, and we use it to set mNumColors with and without my patch. I could tweak my patch to allocate only bpp*8 colorTable entries, rather than the maximum of 256, if that's what you're concerned about.
Michael Skladnikiewicz writes in mail to email@example.com.: A very similar bug has been found in another vendors browser. Would it be possible not to disclose any information about this bug until both Firefox devs and the other vendors devs fix the issues ? No doubt meaning what Gavin noted in comment 10 so I unhid that comment.
Comment on attachment 292931 [details] [diff] [review] patch I also wrote a patch for this yesterday. Initially I also tried checking against mNumColors all the time and also came around to using a fixed-size table. We could go with Alfred's suggestion of a variable sized table based on bpp, but the amount of memory saved is miniscule and simpler is better. r=dveditz
Just for the record, I am ok with the patch. It is the safest (in terms of protection and in terms of minimum impact) way, and won't harm that much.
Thanks Alfred. I landed this patch with a comment pointing to bug 408256 to avoid pointing out that this was a security fix to malicious people watching Bonsai. This is now fixed on the trunk: mozilla/modules/libpr0n/decoders/bmp/nsBMPDecoder.cpp 1.38
Branch approval (for Firefox 184.108.40.206) will also be tracked in that bug.
fix in bug 408256 checked into the branch
Gavin, do I need to host this testcase elsewhere since your uploaded image is zipped?
(In reply to comment #28) > Gavin, do I need to host this testcase elsewhere since your uploaded image is > zipped? No, the image is there only to be used in the modified testcase - it's linked to directly using a jar: URI. All you need to do to verify is load the "modified testcase" (attachment 292786 [details]) multiple times, and make sure you always see a solid black line and that the textbox contains all zeroes, rather than random colors/numbers.
And that is, indeed, what I see. Thanks. Verified in branch with Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:220.127.116.11pre) Gecko/2008011803 BonEcho/18.104.22.168pre.
Any word from Apple or Opera on a fix date? I guess we shouldn't publish an advisory for 22.214.171.124 until we hear back.
Guess it's 16 Feb, so You can publish it. I'll throw a bug description on bugtraq later today.
unhiding bug, Gynvael Coldwind // Vexillium have released their advisory (and mention both Opera and Safari, so no need to scrub comments).
Comment on attachment 292931 [details] [diff] [review] patch a=caillon for the 126.96.36.199 branch
Tested and landed on the 1.8.0 branch (still had an old build lying around!)