Closed
Bug 775794
(CVE-2012-3966)
Opened 12 years ago
Closed 12 years ago
nsICODecoder transparency bitmask memory corruption
Categories
(Core :: Graphics: ImageLib, defect)
Tracking
()
VERIFIED
FIXED
mozilla17
People
(Reporter: hoguin, Assigned: bbondy)
References
Details
(4 keywords, Whiteboard: [advisory-tracking+])
Attachments
(2 files)
1.57 KB,
image/x-icon
|
Details | |
889 bytes,
patch
|
jgilbert
:
review+
akeybl
:
approval-mozilla-aurora+
akeybl
:
approval-mozilla-beta+
akeybl
:
approval-mozilla-esr10+
|
Details | Diff | Splinter Review |
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]
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee | ||
Comment 1•12 years ago
|
||
Thanks for the detailed and clear bug report, also a great find.
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → netzen
Assignee | ||
Comment 2•12 years ago
|
||
Ran through reftests and they all pass, and it loads the reference image now without crashing.
Attachment #644144 -
Flags: review?(jgilbert)
Comment 4•12 years ago
|
||
Clearly causing memory corruption (random crashes in unrelated areas).
bp-60b5d0f4-5149-41a1-a656-46e9b2120720
bp-40ed92c6-5c44-4974-8e22-9b9742120720
status-firefox-esr10:
--- → affected
status-firefox15:
--- → affected
status-firefox16:
--- → affected
status-firefox17:
--- → affected
Updated•12 years ago
|
Attachment #644144 -
Flags: review?(jgilbert) → review+
Assignee | ||
Comment 5•12 years ago
|
||
Target Milestone: --- → mozilla17
Assignee | ||
Comment 6•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 7•12 years ago
|
||
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
Attachment #644144 -
Flags: approval-mozilla-esr10?
Attachment #644144 -
Flags: approval-mozilla-beta?
Attachment #644144 -
Flags: approval-mozilla-aurora?
Comment 8•12 years ago
|
||
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.
Keywords: sec-critical
Updated•12 years ago
|
tracking-firefox-esr10:
--- → 15+
Updated•12 years ago
|
Attachment #644144 -
Flags: approval-mozilla-esr10?
Attachment #644144 -
Flags: approval-mozilla-esr10+
Attachment #644144 -
Flags: approval-mozilla-beta?
Attachment #644144 -
Flags: approval-mozilla-beta+
Attachment #644144 -
Flags: approval-mozilla-aurora?
Attachment #644144 -
Flags: approval-mozilla-aurora+
Assignee | ||
Comment 9•12 years ago
|
||
Assignee | ||
Comment 10•12 years ago
|
||
Updated•12 years ago
|
Whiteboard: [advisory-tracking+]
Comment 11•12 years ago
|
||
Can this bug be verified by adding a testcase to in-testsuite?
Whiteboard: [advisory-tracking+] → [advisory-tracking+][qa?]
Assignee | ||
Comment 12•12 years ago
|
||
I don't think so
Comment 13•12 years ago
|
||
(In reply to Brian R. Bondy [:bbondy] from comment #12)
> I don't think so
Okay, thanks. Will verify with the attached testcase manually.
Whiteboard: [advisory-tracking+][qa?] → [advisory-tracking+][qa+]
Updated•12 years ago
|
Alias: CVE-2012-3966
Comment 14•12 years ago
|
||
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
Status: RESOLVED → VERIFIED
Keywords: verifyme
QA Contact: anthony.s.hughes
Whiteboard: [advisory-tracking+][qa+] → [advisory-tracking+]
Updated•12 years ago
|
Group: core-security
Updated•11 years ago
|
Flags: sec-bounty+
Updated•6 months ago
|
Keywords: reporter-external
You need to log in
before you can comment on or make changes to this bug.
Description
•