775793 - nsBMPDecoder alpha channel processing 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 nsBMPDecoder class, in the code responsible for processing alpha channel data.

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.


The vulnerable code lies in nsBMPDecoder.cpp, at line 519:

510                           if (mUseAlphaData) {
511                             if (!mHaveAlphaData && p[3]) {
512                               // Non-zero alpha byte detected! Clear previous
513                               // pixels that we have already processed.
514                               // This works because we know that if we 
515                               // are reaching here then the alpha data in byte 
516                               // 4 has been right all along.  And we know it
517                               // has been set to 0 the whole time, so that 
518                               // means that everything is transparent so far.
519                               memset(mImageData + (mCurLine - 1) * GetWidth(), 0, 
520                                      (GetHeight() - mCurLine + 1) * 
521                                      GetWidth() * sizeof(PRUint32));
522                               mHaveAlphaData = true;
523                             }

This code is responsible for clearing pixels in the mImageData array, when it is processing a BMP image with an alpha channel, and encounters for the first time a non-zero alpha byte.
The third parameter of memset is calculated using GetHeight(), which returns the raw "height" header value, even when it is negative (which is allowed for BMP images, it means pixels are stored top-to-bottom instead of bottom-to-top). The result of this calculation is then used as an unsigned 32 bits integer by memset.
If GetHeight() returns a negative value, the calculation will result in a very large unsigned number, and memset() will run past the mImageData array, resulting in a memory corruption, which could in turn lead to arbitrary code execution.

The code snippet above will be executed only if mUseAlphaData is true. The only way for this to happen is to have a BMP image embedded in an icon file (.ico). The nsICODecoder will instantiate a nsBMPDecoder, and call its method SetUseAlphaData with the parameter "true", which will set mUseAlphaData to true:

nsICODecoder.cpp:
434     nsBMPDecoder *bmpDecoder = new nsBMPDecoder(mImage, mObserver); 
435     mContainedDecoder = bmpDecoder;
436     bmpDecoder->SetUseAlphaData(true);

Another condition is that the BMP pixel array must be in 32 bits per pixel format (Red, Green, Blue, Alpha). This is done simply by setting the "bpp" field to 32 in the image header.

The last condition is that at least one alpha byte must be non-zero.

The attached file "bmp_with_alpha.ico" triggers the vulnerability.

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

(53c.734): Access violation - code c0000005 (first chance)
First chance exceptions are reported before any exception handling.
This exception may be expected and handled.
eax=062f0380 ebx=05c3a0d0 ecx=01ffffd8 edx=00000000 esi=05d1fdd8 edi=062f1000
eip=781472d7 esp=0012cbe8 ebp=0012cbec iopl=0         nv up ei pl nz na pe nc
cs=001b  ss=0023  ds=0023  es=0023  fs=003b  gs=0000             efl=00010206
MSVCR80!strnicmp+0x7c:
781472d7 660f7f07        movdqa  xmmword ptr [edi],xmm0 ds:0023:062f1000=????????????????????????????????
0:000> k
ChildEBP RetAddr  
WARNING: Stack unwind information not available. Following frames may be wrong.
0012cbec 78147344 MSVCR80!strnicmp+0x7c
0012cc0c 012ecf98 MSVCR80!strnicmp+0xe9
0012cc44 0119f049 xul!mozilla::imagelib::nsBMPDecoder::WriteInternal+0x1a9368
0012cca8 010de143 xul!mozilla::imagelib::nsICODecoder::FixBitmapWidth+0x13 [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\image\decoders\nsicodecoder.cpp @ 208]
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]
0012d104 100026f5 xul!nsThread::ProcessNextEvent+0x19f [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\xpcom\threads\nsthread.cpp @ 666]
0012d144 011bc8e2 nspr4!PR_Unlock+0x25
00000000 00000000 xul!MessageLoop::RunHandler+0x21 [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\ipc\chromium\src\base\message_loop.cc @ 202]

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

In addition there's an invalidation that's incorrect, but clearly this part is more serious.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Regression from https://hg.mozilla.org/mozilla-central/rev/72bace03b6ce which first shipped in mozilla9.

Comment 3

[Brian R. Bondy [:bbondy]]

Nice find! Thanks for the detailed and clear bug report.

Comment 4

[Brian R. Bondy [:bbondy]]

(In reply to David Baron [:dbaron] from comment #1)
> In addition there's an invalidation that's incorrect, but clearly this part
> is more serious.

Could you post a new bug for that and CC me? Or if you provide more info then I can post.

Comment 5

[Kelsey Gilbert [:jgilbert]]

Attachment: Clarify usage of height and GetHeight in the BMP decoder

This should do it. I'll check for repro presently.

Comment 6

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 644132 [details] [diff] [review]
Clarify usage of height and GetHeight in the BMP decoder

Review of attachment 644132 [details] [diff] [review]:
-----------------------------------------------------------------

Oops.

::: image/decoders/nsBMPDecoder.h
@@ +40,3 @@
>      PRInt32 GetHeight() const;
> +    // Is the height inverted in the BIH header
> +    bool IsHeightInverted() const;

Ack, thought I deleted this.

Comment 7

[Brian R. Bondy [:bbondy]]

By the way I already had a patch for this, could you be sure to take a bug before you work on it or else don't work on a bug when someone else takes it? That way we don't waste time duplicating efforts.

Comment 8

[Kelsey Gilbert [:jgilbert]]

(In reply to Brian R. Bondy [:bbondy] from comment #7)
> By the way I already had a patch for this, could you be sure to take a bug
> before you work on it or else don't work on a bug when someone else takes
> it? That way we don't waste time duplicating efforts.

Oops, sorry. Will do in the future.

Comment 9

[Brian R. Bondy [:bbondy]]

Comment on attachment 644132 [details] [diff] [review]
Clarify usage of height and GetHeight in the BMP decoder

Review of attachment 644132 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, please push to try first but it should all pass. Also we should request this for aurora, beta and esr10 once it lands on m-c.

Comment 10

[Brian R. Bondy [:bbondy]]

Just remove your self found nit btw. 
Also for dbaron please ignore my Comment 4, I realized what you meant after I posted that.

Comment 12

[Daniel Veditz [:dveditz]]

I'm afraid I don't have the imagination to see how to parlay writing a large number of zeros into the kind of control you need for an exploit. Maybe I should suspend judgement until we see the "Exploiting jemalloc" talk at this year's BlackHat.

Comment 13

[Kelsey Gilbert [:jgilbert]]

Crash does not occur after patch. Try is clean.

Comment 14

[Kelsey Gilbert [:jgilbert]]

Attachment: fixed patch

Carrying forward r+.

Comment 15

[Kelsey Gilbert [:jgilbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/673c2c03067a

Comment 16

[Kelsey Gilbert [:jgilbert]]

Can we get a sg rating for this? It's pretty trivial to trigger, and I believe you can basically choose how many zeros you want.

Comment 17

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 645036 [details] [diff] [review]
fixed patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 682688
User impact if declined: Crashy browser for an uncommon set of valid images, and security concerns.
Testing completed (on m-c, etc.): m-c, tested/fixed locally
Risk to taking this patch (and alternatives if risky): Low.
String or UUID changes made by this patch: None.

Comment 18

[Daniel Veditz [:dveditz]]

I'm less worried about a consecutive string of nulls than the slightly different case in bug 775794. But we should fix both anyway.

Comment 19

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/673c2c03067a

Comment 20

[Alex Keybl [:akeybl]]

Comment on attachment 645036 [details] [diff] [review]
fixed patch

[Triage Comment]
Please also prepare a patch for the ESR branch.

Comment 21

[Kelsey Gilbert [:jgilbert]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/76745d7889ef

Comment 22

[Kelsey Gilbert [:jgilbert]]

https://hg.mozilla.org/releases/mozilla-beta/rev/fe4c021dd904

Comment 23

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 645036 [details] [diff] [review]
fixed patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: Request from akeybl.
User impact if declined: Crashes for valid (if uncommon) images, DOS through crashing. Sec-moderate vulnerability.
Fix Landed on Version: m-c, aurora, beta
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.

Comment 24

[Kelsey Gilbert [:jgilbert]]

Should I prepare a FF 14 patch, or is that a WONTFIX?

Comment 25

[Alex Keybl [:akeybl]]

(In reply to Jeff Gilbert [:jgilbert] from comment #24)
> Should I prepare a FF 14 patch, or is that a WONTFIX?

Thanks for checking in - this is wontfix for 14 at this point.

Comment 26

[Kelsey Gilbert [:jgilbert]]

Attachment: esr10 backport

This is the patch backported to esr10.

Comment 27

[Kelsey Gilbert [:jgilbert]]

https://hg.mozilla.org/releases/mozilla-esr10/rev/6fdfc850e249

Comment 28

[u279076]

Does this need a testcase in-testsuite?

Comment 29

[Brian R. Bondy [:bbondy]]

I don't think so

Comment 30

[u279076]

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

Okay, thanks, we will verify this manually with the attached testcase.

Comment 31

[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