Last Comment Bug 408076 - (CVE-2008-0420) out of bounds read in BMP decoder can lead to information disclosure
(CVE-2008-0420)
: out of bounds read in BMP decoder can lead to information disclosure
Status: RESOLVED FIXED
[sg:moderate] fix in bug 408256. coor...
: fixed1.8.0.15, privacy, testcase, verified1.8.1.12
Product: Core
Classification: Components
Component: ImageLib (show other bugs)
: Trunk
: All All
: P1 critical (vote)
: mozilla1.9beta3
Assigned To: :Gavin Sharp [email: gavin@gavinsharp.com]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2007-12-12 09:21 PST by :Gavin Sharp [email: gavin@gavinsharp.com]
Modified: 2008-03-12 09:27 PDT (History)
15 users (show)
mtschrep: blocking1.9+
dveditz: blocking1.8.1.12+
dveditz: wanted1.8.1.x+
caillon: blocking1.8.0.next+
jwalden+bmo: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
provided code (2.65 KB, text/plain)
2007-12-12 09:22 PST, :Gavin Sharp [email: gavin@gavinsharp.com]
no flags Details
test image (103 bytes, image/bmp)
2007-12-12 09:23 PST, :Gavin Sharp [email: gavin@gavinsharp.com]
no flags Details
test image (103 bytes, image/png)
2007-12-12 09:43 PST, :Gavin Sharp [email: gavin@gavinsharp.com]
no flags Details
zipped image (421 bytes, application/java-archive)
2007-12-12 09:48 PST, :Gavin Sharp [email: gavin@gavinsharp.com]
no flags Details
modified testcase (976 bytes, text/html)
2007-12-12 09:53 PST, :Gavin Sharp [email: gavin@gavinsharp.com]
no flags Details
possible (incorrect) patch (8.36 KB, patch)
2007-12-12 21:43 PST, :Gavin Sharp [email: gavin@gavinsharp.com]
no flags Details | Diff | Review
better possible patch (8.50 KB, patch)
2007-12-12 22:05 PST, :Gavin Sharp [email: gavin@gavinsharp.com]
no flags Details | Diff | Review
patch (1.38 KB, patch)
2007-12-13 01:56 PST, :Gavin Sharp [email: gavin@gavinsharp.com]
dveditz: review+
pavlov: superreview+
caillon: approval1.8.0.next+
Details | Diff | Review

Description :Gavin Sharp [email: gavin@gavinsharp.com] 2007-12-12 09:21:48 PST
Full credit goes to Michael Skladnikiewicz, who reported this to security@mozilla.org in an email with subject "Mozilla Firefox 2.0.0.11, 3.0b2pre and prior Remote Information Disclosure". His initial email is quoted below.
-------------------------------------------

Hi,

(credit: Gynvael Coldwind // Vexillium with help from udevd and porneL
(I didn't know about <canvas> ;D))

OK, Here is how it goes.

Firefox has a problem in handling 8bit BMP files.
The BMP format has a field in the BITMAPINFOHEADER named biClrUsed, the
field says how many colors does the palette contain. If this field is 0,
then 256 color pallet is used. When this field is not 0, the palette has
the given number of colors.

Now this is how it goes in Firefox:
1. Firefox allocates 256 * sizeof(RGB) for the palette
2. It copies the biClrUsed colors from the BMP file

Well, what is missing is:
1a. memset(pallete, 0, 256 * sizeof(RGB)
The palette still contains old data from the heap.

Now, we take a BMP file sized 256x1x8 with biClrUser = 0, and fill the
bitmap with gradient, from 0 to 255:
00 01 02 03 04 05 ... and so on

When displayed, the BMP file looks chaotic, and in fact it contains the
palette copied to the screen.

Here is where HTML 5.0 comes in and <canvas>. You can imagine the
rest... But I'll write it anyway. You can create a HTML/javascript that
copies the image from img to a canvas and then gets it data and, for
example using a form, posts it to some remote server.

This has been tested and it works. There is a PoC exploit in the bottom.

The harvested data contains mainly trash, but there are also parts of
other websites, parts of java scripts, even parts of favorites. Well, if
there are also cookies and passwords in heap, then they are also reachable.

I've attached also the scripts and the leak.bmp bitmap.

This will be posted on bugtraq as soon as a fixed version of FireFox is
released.

Please check also Thunderbird for this issue (I didn't check).

Best regards,
Looking forward to Your reply,

Michael "Gynvael Coldwind" Skladnikiewicz
Team Vexillium
Hispasec
Comment 1 :Gavin Sharp [email: gavin@gavinsharp.com] 2007-12-12 09:22:50 PST
Created attachment 292779 [details]
provided code

This is the code that Michael provided in his initial report.
Comment 2 :Gavin Sharp [email: gavin@gavinsharp.com] 2007-12-12 09:23:42 PST
Created attachment 292780 [details]
test image

This is the example image Michael provided in his initial email.
Comment 3 :Gavin Sharp [email: gavin@gavinsharp.com] 2007-12-12 09:24:20 PST
Follow-up email from Michael:
------------------------------------

Hello again,

Since I didn't receive any reply from You, I digged a little into the FF
code to provide You with additional information.

First of all, I was mistaking - it is not a missing memset function,
it's a boundary condition error.

In file mozilla\modules\libpr0n\decoders\bmp\nsBMPDecoder.cpp
Function ProcessData:

Line 260:
        if (mBIH.bpp <= 8) {
            mNumColors = 1 << mBIH.bpp;
            if (mBIH.colors && mBIH.colors < mNumColors)
                mNumColors = mBIH.colors;

            mColors = new colorTable[mNumColors];
            if (!mColors)
                return NS_ERROR_OUT_OF_MEMORY;
        }


Line 403:
                      case 8:
                        while (lpos > 0) {
                          SetPixel(d, *p, mColors);
                          --lpos;
                          ++p;
                        }
                        break;


There is a check missing - *p should be checked against mNumColors.

In a malformed BMP file the *p will be equal to or greater then
mNumColors, what causes one of two things:
1. Remote DoS - FF crashes when mColors[*p] reaches over the Heap's end
(this happens mostly on Windows XP, it hasn't happen on Vista nor Linux)
2. Remote Information Disclosure (using <canvas> and javascript,
information from the heap residing after the mColors can be copied -
I've written you a mail about this).

Looking forward to your reply.

G.C.
Comment 4 :Gavin Sharp [email: gavin@gavinsharp.com] 2007-12-12 09:43:34 PST
Created attachment 292782 [details]
test image

For some reason the test image I uploaded previously doesn't seem to work. Trying again.
Comment 5 :Gavin Sharp [email: gavin@gavinsharp.com] 2007-12-12 09:47:40 PST
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.
Comment 6 :Gavin Sharp [email: gavin@gavinsharp.com] 2007-12-12 09:48:25 PST
Created attachment 292784 [details]
zipped image
Comment 7 :Gavin Sharp [email: gavin@gavinsharp.com] 2007-12-12 09:53:46 PST
Created attachment 292786 [details]
modified testcase

This is a testcase based on the example code provided by Michael.
Comment 8 Mike Schroepfer 2007-12-12 20:46:18 PST
Yep - we should fix - any takers? :-)
Comment 9 :Gavin Sharp [email: gavin@gavinsharp.com] 2007-12-12 21:43:02 PST
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).
Comment 10 :Gavin Sharp [email: gavin@gavinsharp.com] 2007-12-12 21:43:57 PST
Safari and Opera (shipping versions on Windows) also seem to have this bug. I'll look into filing bugs on them as well.
Comment 11 :Gavin Sharp [email: gavin@gavinsharp.com] 2007-12-12 22:05:17 PST
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.
Comment 12 Alfred Kayser 2007-12-13 00:11:48 PST
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)
Comment 13 Gynvael Coldwind 2007-12-13 00:21:34 PST
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
Comment 14 :Gavin Sharp [email: gavin@gavinsharp.com] 2007-12-13 01:56:28 PST
Created attachment 292931 [details] [diff] [review]
patch

You're right Alfred, that is a better fix.
Comment 15 :Gavin Sharp [email: gavin@gavinsharp.com] 2007-12-13 01:58:59 PST
The ICO decoder doesn't have this bug because it always uses the maximum value for mNumColors based on mBIH.bpp.
Comment 16 Alfred Kayser 2007-12-13 03:04:41 PST
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 17 Alfred Kayser 2007-12-13 03:05:37 PST
Comment on attachment 292931 [details] [diff] [review]
patch

Need to add check for biClrUsed value !=0.
Comment 18 Alfred Kayser 2007-12-13 03:19:30 PST
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 19 :Gavin Sharp [email: gavin@gavinsharp.com] 2007-12-13 09:36:43 PST
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.
Comment 20 :Gavin Sharp [email: gavin@gavinsharp.com] 2007-12-13 09:48:53 PST
(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.
Comment 21 Daniel Veditz [:dveditz] 2007-12-13 10:41:22 PST
Michael Skladnikiewicz writes in mail to security@m.o.:

   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 22 Daniel Veditz [:dveditz] 2007-12-13 10:46:54 PST
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
Comment 24 Alfred Kayser 2007-12-13 13:05:31 PST
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.
Comment 25 :Gavin Sharp [email: gavin@gavinsharp.com] 2007-12-13 14:09:26 PST
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
Comment 26 :Gavin Sharp [email: gavin@gavinsharp.com] 2007-12-13 14:10:52 PST
Branch approval (for Firefox 2.0.0.12) will also be tracked in that bug.
Comment 27 Daniel Veditz [:dveditz] 2007-12-19 10:50:29 PST
fix in bug 408256 checked into the branch
Comment 28 Al Billings [:abillings] 2008-01-18 13:17:32 PST
Gavin, do I need to host this testcase elsewhere since your uploaded image is zipped?
Comment 29 :Gavin Sharp [email: gavin@gavinsharp.com] 2008-01-18 13:27:30 PST
(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.
Comment 30 Al Billings [:abillings] 2008-01-18 15:06:59 PST
And that is, indeed, what I see. Thanks.

Verified in branch with Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.12pre) Gecko/2008011803 BonEcho/2.0.0.12pre.
Comment 31 Daniel Veditz [:dveditz] 2008-02-07 15:43:30 PST
Any word from Apple or Opera on a fix date? I guess we shouldn't publish an advisory for 2.0.0.12 until we hear back.
Comment 33 Gynvael Coldwind 2008-02-16 04:06:25 PST
Guess it's 16 Feb, so You can publish it. I'll throw a bug description on bugtraq later today.
Comment 34 Daniel Veditz [:dveditz] 2008-02-19 10:23:20 PST
unhiding bug, Gynvael Coldwind // Vexillium have released their advisory (and mention both Opera and Safari, so no need to scrub comments).
Comment 35 Christopher Aillon (sabbatical, not receiving bugmail) 2008-03-11 20:17:04 PDT
Comment on attachment 292931 [details] [diff] [review]
patch

a=caillon for the 1.8.0.15 branch
Comment 36 :Gavin Sharp [email: gavin@gavinsharp.com] 2008-03-12 09:24:12 PDT
Tested and landed on the 1.8.0 branch (still had an old build lying around!)
Comment 37 :Gavin Sharp [email: gavin@gavinsharp.com] 2008-03-12 09:27:46 PDT
mozilla/modules/libpr0n/decoders/bmp/nsBMPDecoder.cpp 	1.29.12.1 

Note You need to log in before you can comment on or make changes to this bug.