775794 - (CVE-2012-3966) nsICODecoder transparency bitmask memory corruption

Status: VERIFIED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Frédéric Hoguin]

Attachment: Sample file triggering the bug

This bug is present in the nsICODecoder class, when processing the transparency bitmask.

It allows a remote attacker to write random memory, hence the critical severity.

User action is needed in the following form: the user needs to open a web page containing a specially crafted image.

First, a little summary of how Firefox processes a BMP embedded inside an ICO image:

-It reads the ICONDIRENTRY
-It reads the image header
-It divides the image height by 2
-It passes the image header and the image data to a BMP decoder
-It applies the transparency bitmask to the rendered image.

Before passing the BMP image header and data to the BMP decoder, the nsICODecoder checks that the properties in the ICONDIRENTRY structure are consistent with the properties in the BMP header, and fixes them if necessary.

Since the maximum height of an icon is 256, it checks that the BMP header height is no more than 256 (the image is considered invalid if it exceeds 256).
Also, if the height in the ICONDIRENTRY structure and the height in the BMP header don't match, then the height in the BMP header is trusted over the height in the ICONDIRENTRY structure.
Here is the code responsible for doing this, in the file nsICODecoder.cpp:

166 // A BMP inside of an ICO has *2 height because of the AND mask
167 // that follows the actual bitmap.  The BMP shouldn't know about
168 // this difference though.
169 bool
170 nsICODecoder::FixBitmapHeight(PRInt8 *bih) 
171 {
172   // Get the height from the BMP file information header
173   PRInt32 height;
174   memcpy(&height, bih + 8, sizeof(height));
175   height = LITTLE_TO_NATIVE32(height);
176 
177   // The bitmap height is by definition * 2 what it should be to account for
178   // the 'AND mask'. It is * 2 even if the `AND mask` is not present.
179   height /= 2;
180 
181   if (height > 256) {
182     return false;
183   }
184 
185   // We should always trust the height from the bitmap itself instead of 
186   // the ICO height.  So fix the ICO height.
187   if (height == 256) {
188     mDirEntry.mHeight = 0;
189   } else {
190     mDirEntry.mHeight = (PRInt8)height;
191   }
192 
193   // Fix the BMP height in the BIH so that the BMP decoder can work properly
194   height = NATIVE32_TO_LITTLE(height);
195   memcpy(bih + 8, &height, sizeof(height));
196   return true;
197 }

The mDirEntry.mHeight variable is then used to calculate the pixel offset when applying the transparency bitmask. Note: GetRealHeight() returns mDirEntry.mHeight:

542           mCurLine = GetRealHeight();
[...]
575             PRUint32* decoded = imageData + mCurLine * GetRealWidth();
576             PRUint32* decoded_end = decoded + GetRealWidth();
577             PRUint8* p = mRow, *p_end = mRow + rowSize; 
578             while (p < p_end) {
579               PRUint8 idx = *p++;
580               for (PRUint8 bit = 0x80; bit && decoded<decoded_end; bit >>= 1) {
581                 // Clear pixel completely for transparency.
582                 if (idx & bit) {
583                   *decoded = 0;
584                 }
585                 decoded++;
586               }
587             }



The vulnerability is in the nsICODecoder::FixBitmapHeight function.
The "height" variable is declared as a PRInt32, which is a signed 32 bits integer.
Consequently, if the "height" field of the BMP header is negative, it will pass the check at line 181:

181   if (height > 256) {
182     return false;
183   }

It will then be truncated to fit into a 8 bits integer, namely mDirEntry.mHeight:

187   if (height == 256) {
188     mDirEntry.mHeight = 0;
189   } else {
190     mDirEntry.mHeight = (PRInt8)height;
191   }

mDirEntry.mHeight is an unsigned 8 bits integer, so when it is assigned to mCurLine, mCurLine can be higher than the real height of the image, causing the processing of the transparency bitmask to write past the imageData array, resulting in memory corruption.
This is possible because negative heights in BMP headers are acceptable, so the BMP decoder will process the image normally.


To illustrate what happens with an example, we'll take a 32x8 pixels image. We have to double the initial height to account for the transparency bitmask, and make it negative, so the "height" value in the BMP header will be -16, which is 0xfffffff0.
When we enter the nsICODecoder::FixBitmapHeight function, here is what happens:
-First, the "height" is divided by 2. Its new value is -8, or 0xfffffff8.
-The check at line 181 is passed, since -8 is not superior to 256.
-mDirEntry.mHeight is assigned the value of 0xf8 (which is 0xfffffff8 truncated to 8 bits).
-The BMP decoder processes the BMP data, allocating an array large enough to contain our 32x8 pixels image.
-The mCurLine variable is assigned the value of mDirEntry.mHeight. Since mDirEntry.mHeight is an unsigned byte, mCurLine is equal to 0x000000f8, which is 248 in decimal.
-nsICODecoder uses mCurLine to calculate an offset to the image data array. Since 248 is larger than the image real height of 8, data is written past the end of the imageData array, causing memory corruption (at line 583).

The attached sample ICO file triggers the vulnerability.

Here is a WinDbg log when exploiting the vulnerability in Firefox on Windows XP:

(540.564): Access violation - code c0000005 (first chance)
First chance exceptions are reported before any exception handling.
This exception may be expected and handled.
eax=071a7c80 ebx=01f03b64 ecx=01f03b63 edx=000000ff esi=071a7bc0 edi=05f86530
eip=0113834a esp=0012cc68 ebp=0012cca8 iopl=0         nv up ei ng nz na po nc
cs=001b  ss=0023  ds=0023  es=0023  fs=003b  gs=0000             efl=00010282
e:\builds\moz2_slave\rel-m-rel-w32-bld\build\image\decoders\nsicodecoder.cpp(583)
xul!mozilla::imagelib::nsICODecoder::WriteInternal+0x60a:
0113834a c70600000000    mov     dword ptr [esi],0    ds:0023:071a7bc0=????????
0:000> k
ChildEBP RetAddr  
0012cca8 010de143 xul!mozilla::imagelib::nsICODecoder::WriteInternal+0x60a [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\image\decoders\nsicodecoder.cpp @ 583]
0012ccc8 0113410e xul!mozilla::imagelib::RasterImage::WriteToDecoder+0x3e [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\image\src\rasterimage.cpp @ 2370]
0012cce4 010b9526 xul!mozilla::imagelib::RasterImage::DecodeSomeData+0x35 [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\image\src\rasterimage.cpp @ 2702]
0012cd3c 010f6ba2 xul!mozilla::imagelib::imgDecodeWorker::Run+0xfe [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\image\src\rasterimage.cpp @ 2819]
0012cd50 010f6be3 xul!mozilla::imagelib::RasterImage::AddSourceData+0xcf [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\image\src\rasterimage.cpp @ 1496]
0012cda4 0104cfb6 xul!mozilla::imagelib::RasterImage::WriteToRasterImage+0x15 [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\image\src\rasterimage.cpp @ 2917]
0012cda4 0104cfb6 xul!imgRequest::OnDataAvailable+0x466 [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\image\src\imgrequest.cpp @ 1166]
0012d004 010f1121 xul!imgRequest::OnDataAvailable+0x466 [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\image\src\imgrequest.cpp @ 1166]
0012d024 010a7f3a xul!ProxyListener::OnDataAvailable+0x26 [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\image\src\imgloader.cpp @ 2091]
0012d050 010981c3 xul!nsBaseChannel::OnDataAvailable+0x49 [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\netwerk\base\src\nsbasechannel.cpp @ 774]
0012d094 0109807a xul!nsInputStreamPump::OnStateTransfer+0xd3 [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\netwerk\base\src\nsinputstreampump.cpp @ 512]
0012d0a8 01097df3 xul!nsInputStreamPump::OnInputStreamReady+0x6d [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\netwerk\base\src\nsinputstreampump.cpp @ 409]
0012d0b8 00fbee5f xul!nsInputStreamReadyEvent::Run+0x1d [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\xpcom\io\nsstreamutils.cpp @ 115]
0012d110 00fe3e9e xul!nsThread::ProcessNextEvent+0x19f [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\xpcom\threads\nsthread.cpp @ 666]
0012d144 011bc8e2 xul!mozilla::ipc::MessagePump::Run+0x6e [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\ipc\glue\messagepump.cpp @ 110]
0012d17c 011bc8b3 xul!MessageLoop::RunHandler+0x21 [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\ipc\chromium\src\base\message_loop.cc @ 202]
0012d198 011991b8 xul!MessageLoop::Run+0x15 [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\ipc\chromium\src\base\message_loop.cc @ 176]
0012d1a4 011bc9b0 xul!nsBaseAppShell::Run+0x34 [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\widget\src\xpwidgets\nsbaseappshell.cpp @ 191]
0012f0f8 011bc9f2 xul!nsAppShell::Run+0x4d [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\widget\src\windows\nsappshell.cpp @ 258]
0012f4a4 00401796 xul!nsAppStartup::Run+0x1e [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\toolkit\components\startup\nsappstartup.cpp @ 221]
0012ff7c 00401ab0 firefox!wmain+0x796 [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\toolkit\xre\nswindowswmain.cpp @ 107]
0012ffc0 7c817077 firefox!__tmainCRTStartup+0x10f [f:\sp\vctools\crt_bld\self_x86\crt\src\crtexe.c @ 594]

Comment 1

[Brian R. Bondy [:bbondy]]

Thanks for the detailed and clear bug report, also a great find.

Comment 2

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1.

Ran through reftests and they all pass, and it loads the reference image now without crashing.

Comment 4

[Daniel Veditz [:dveditz]]

Clearly causing memory corruption (random crashes in unrelated areas).
bp-60b5d0f4-5149-41a1-a656-46e9b2120720
bp-40ed92c6-5c44-4974-8e22-9b9742120720

Comment 5

[Brian R. Bondy [:bbondy]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/5bd9db1381a6

Comment 6

[Brian R. Bondy [:bbondy]]

https://hg.mozilla.org/mozilla-central/rev/5bd9db1381a6

Comment 7

[Brian R. Bondy [:bbondy]]

Comment on attachment 644144 [details] [diff] [review]
Patch v1.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: Has not been triaged for sec rating yet, but it is a security concern and a user can crash the browser at least via loading a web page.
User impact if declined: Crashes at least
Fix Landed on Version: m-c mozilla17
Risk to taking this patch (and alternatives if risky): Low 
String or UUID changes made by this patch: None

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 695421
User impact if declined: See above
Testing completed (on m-c, etc.): Testing completed locally, already pushed to m-c.
Risk to taking this patch (and alternatives if risky): Low
String or UUID changes made by this patch: None

Comment 8

[Daniel Veditz [:dveditz]]

With proper "heap feng shui" this more controlled nulling of bytes could more of a worry than the block of nulls in the other BMP bug.

Comment 9

[Brian R. Bondy [:bbondy]]

https://hg.mozilla.org/releases/mozilla-beta/rev/94b439f3d2d0
http://hg.mozilla.org/releases/mozilla-aurora/rev/e328a4f6d4dc

Comment 10

[Brian R. Bondy [:bbondy]]

http://hg.mozilla.org/releases/mozilla-esr10/rev/bb1d806b2efa

Comment 11

[u279076]

Can this bug be verified by adding a testcase to in-testsuite?

Comment 12

[Brian R. Bondy [:bbondy]]

I don't think so

Comment 13

[u279076]

(In reply to Brian R. Bondy [:bbondy] from comment #12)
> I don't think so

Okay, thanks. Will verify with the attached testcase manually.

Comment 14

[u279076]

Confirmed reproducible with 2012-07-19 Firefox 17.0a1 Debug.

Verified fixed with:
 * 2012-08-24 Firefox 17.0a1 Debug
 * 2012-08-24 Firefox 16.0a2 Debug
 * 2012-08-24 Firefox 15.0 Debug
 * 2012-08-24 Firefox 10.0.7esr Debug