Closed Bug 408076 (CVE-2008-0420) Opened 17 years ago Closed 17 years ago

out of bounds read in BMP decoder can lead to information disclosure

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla1.9beta3

People

(Reporter: Gavin, Assigned: Gavin)

Details

(4 keywords, Whiteboard: [sg:moderate] fix in bug 408256. coordinate disclosure with other vendors, see comment 10)

Attachments

(4 files, 4 obsolete files)

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
Flags: blocking1.9?
Flags: blocking1.8.1.12?
Attached file provided code
This is the code that Michael provided in his initial report.
Attached image test image (obsolete) —
This is the example image Michael provided in his initial email.
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.
Keywords: testcase
Attachment #292780 - Attachment mime type: image/png → image/bmp
Attached image test image (obsolete) —
For some reason the test image I uploaded previously doesn't seem to work. Trying again.
Attachment #292780 - Attachment is obsolete: true
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.
Attachment #292782 - Attachment is obsolete: true
Attachment #292784 - Attachment mime type: application/zip → application/java-archive
Attached file modified testcase
This is a testcase based on the example code provided by Michael.
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.12?
Flags: blocking1.8.1.12+
Keywords: privacy
Whiteboard: [sg:moderate]
Yep - we should fix - any takers? :-)
Flags: blocking1.9? → blocking1.9+
Attached patch possible (incorrect) patch (obsolete) — Splinter Review
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.
Attached patch better possible patch (obsolete) — Splinter Review
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.
Attachment #292916 - Attachment is patch: true
Attachment #292916 - Attachment mime type: application/octet-stream → text/plain
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
Attached patch patchSplinter Review
You're right Alfred, that is a better fix.
Assignee: nobody → gavin.sharp
Attachment #292914 - Attachment is obsolete: true
Attachment #292916 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #292931 - Flags: superreview?(pavlov)
Attachment #292931 - Flags: review?(alfredkayser)
Priority: -- → P1
Target Milestone: --- → mozilla1.9 M11
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??
Priority: P1 → --
Target Milestone: mozilla1.9 M11 → ---
Comment on attachment 292931 [details] [diff] [review]
patch

Need to add check for biClrUsed value !=0.
Attachment #292931 - Flags: review?(alfredkayser) → review-
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.
Attachment #292931 - Flags: review- → review?(alfredkayser)
(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 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.
Whiteboard: [sg:moderate] → [sg:moderate] coordinate disclosure with other vendors, see comment 10
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
Attachment #292931 - Flags: review?(alfredkayser) → review+
Attachment #292931 - Flags: superreview?(pavlov) → superreview+
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
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Branch approval (for Firefox 2.0.0.12) will also be tracked in that bug.
Priority: -- → P1
Target Milestone: --- → mozilla1.9 M11
Flags: in-testsuite?
fix in bug 408256 checked into the branch
Keywords: fixed1.8.1.12
Whiteboard: [sg:moderate] coordinate disclosure with other vendors, see comment 10 → [sg:moderate] fix in bug 408256. coordinate disclosure with other vendors, see comment 10
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:1.8.1.12pre) Gecko/2008011803 BonEcho/2.0.0.12pre.
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.
Alias: CVE-2008-0420
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).
Group: security
Flags: blocking1.8.0.15?
Flags: blocking1.8.0.15? → blocking1.8.0.15+
Attachment #292931 - Flags: approval1.8.0.15?
Comment on attachment 292931 [details] [diff] [review]
patch

a=caillon for the 1.8.0.15 branch
Attachment #292931 - Flags: approval1.8.0.15? → approval1.8.0.15+
Tested and landed on the 1.8.0 branch (still had an old build lying around!)
Keywords: fixed1.8.0.15
mozilla/modules/libpr0n/decoders/bmp/nsBMPDecoder.cpp 	1.29.12.1 
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: