408076 - (CVE-2008-0420) out of bounds read in BMP decoder can lead to information disclosure

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect, P1)

Description

[:Gavin Sharp [email: [e-mail]]]

Full credit goes to Michael Skladnikiewicz, who reported this to [e-mail] 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: [e-mail]]]

Attachment: provided code

This is the code that Michael provided in his initial report.

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Attachment: test image

This is the example image Michael provided in his initial email.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

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: [e-mail]]]

Attachment: test image

For some reason the test image I uploaded previously doesn't seem to work. Trying again.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

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: [e-mail]]]

Attachment: zipped image

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Attachment: modified testcase

This is a testcase based on the example code provided by Michael.

Comment 8

[Mike Schroepfer]

Yep - we should fix - any takers? :-)

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Attachment: 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: [e-mail]]]

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: [e-mail]]]

Attachment: 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]

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]

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: [e-mail]]]

Attachment: patch

You're right Alfred, that is a better fix.

Comment 15

[:Gavin Sharp [email: [e-mail]]]

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]

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]

Comment on attachment 292931 [details] [diff] [review]
patch

Need to add check for biClrUsed value !=0.

Comment 18

[Alfred Kayser]

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: [e-mail]]]

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: [e-mail]]]

(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]]

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]]

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]

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: [e-mail]]]

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: [e-mail]]]

Branch approval (for Firefox 2.0.0.12) will also be tracked in that bug.

Comment 27

[Daniel Veditz [:dveditz]]

fix in bug 408256 checked into the branch

Comment 28

[Al Billings [:abillings - ex-MoCo]]

Gavin, do I need to host this testcase elsewhere since your uploaded image is zipped?

Comment 29

[:Gavin Sharp [email: [e-mail]]]

(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 - ex-MoCo]]

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]]

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]

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]]

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)]

Comment on attachment 292931 [details] [diff] [review]
patch

a=caillon for the 1.8.0.15 branch

Comment 36

[:Gavin Sharp [email: [e-mail]]]

Tested and landed on the 1.8.0 branch (still had an old build lying around!)

Comment 37

[:Gavin Sharp [email: [e-mail]]]

mozilla/modules/libpr0n/decoders/bmp/nsBMPDecoder.cpp 	1.29.12.1