Last Comment Bug 295457 - NISCC vulnerability #891011 (Parsing of Various Image Formats by Web Browsers)
: NISCC vulnerability #891011 (Parsing of Various Image Formats by Web Browsers)
Status: RESOLVED FIXED
[sg:dos] public on June 14. ready for...
: fixed-aviary1.0.5, fixed1.7.9
Product: Core
Classification: Components
Component: ImageLib (show other bugs)
: Trunk
: x86 All
: P1 normal (vote)
: mozilla1.8beta2
Assigned To: Christian :Biesinger (don't email me, ping me on IRC)
:
: Milan Sreckovic [:milan]
Mentors:
: 238401 (view as bug list)
Depends on: 245631
Blocks:
  Show dependency treegraph
 
Reported: 2005-05-25 03:04 PDT by Daniel Veditz [:dveditz]
Modified: 2005-12-02 11:46 PST (History)
10 users (show)
dveditz: blocking1.7.9+
dveditz: blocking‑aviary1.0.5+
jaymoz: blocking‑aviary1.0.8-
dveditz: blocking1.8b2+
dveditz: blocking‑aviary1.5+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
windows .ico crashers (.zip) (963 bytes, application/octet-stream)
2005-05-25 03:06 PDT, Daniel Veditz [:dveditz]
no flags Details
test images for SUSE linux 2.6.4-52 (.zip) (1.49 KB, application/octet-stream)
2005-05-25 03:08 PDT, Daniel Veditz [:dveditz]
no flags Details
use malloc/free (7.01 KB, patch)
2005-05-25 13:54 PDT, Christian :Biesinger (don't email me, ping me on IRC)
tor: review+
dveditz: superreview+
caillon: approval‑aviary1.0.5+
caillon: approval1.7.9+
chofmann: approval1.8b2+
Details | Diff | Splinter Review
branch patch (7.01 KB, patch)
2005-06-03 07:38 PDT, Christian :Biesinger (don't email me, ping me on IRC)
no flags Details | Diff | Splinter Review
patch for nsImageGTK.cpp (4.43 KB, patch)
2005-06-08 02:20 PDT, Ginn Chen
cbiesinger: review+
dveditz: superreview+
dveditz: approval‑aviary1.0.5+
dveditz: approval1.7.9+
dveditz: approval1.8b3+
Details | Diff | Splinter Review

Description Daniel Veditz [:dveditz] 2005-05-25 03:04:30 PDT
!!THIS INFORMATION IS EMBARGOED UNTIL 1PM BST 14/6/2005!!

Please find attached a draft of the NISCC Vulnerability Advisory
Notice (VAN) for NISCC Vulnerability #891011; please note that the
information is shared under the terms of the Framework Agreement
which you have agreed to previously.

THIS IS NOT TO BE PASSED ON TO ANY THIRD PARTY WITHOUT PRIOR
AGREEMENT FROM THE NISCC VULNERABILITY TEAM.

NISCC will be disclosing this to the general public at 1PM BST
14/6/2005. You are all reminded that on the day of public release,
the NISCC Vulnerability page will receive a large number of hits,
therefore by passing NISCC a statement for inclusion on this page,
you will be targeting a high proportion of your customers; hence if
you wish to do so, statements will be accepted up to 4pm BST
13/6/2005 and then will be updated once daily. Please state whether
any statement is for public release via the NISCC website or intended
to be private information for NISCC only.

Many thanks for your co-operation with NISCC on the handling of this
vulnerability.

______________________________________________________________________Draft
NISCC Vulnerability Advisory 891011/NISCC/IMAGEFORMATS

Vulnerability Issues with the Parsing of Various Image Formats by Web Browsers

-------------------
Advisory Reference  891011/NISCC/IMAGEFORMATS
Release Date	    14 June 2005
Last Revision	    11 May 2005
Version Number	    0.2

Acknowledgement
---------------
These issues were identified using the Images Test Tool developed by Codenomicon
Ltd.
Comment 1 Daniel Veditz [:dveditz] 2005-05-25 03:06:52 PDT
Created attachment 184486 [details]
windows .ico crashers (.zip)

Test images that crash Firefox on windows XP SP2
Comment 2 Daniel Veditz [:dveditz] 2005-05-25 03:08:11 PDT
Created attachment 184487 [details]
test images for SUSE linux 2.6.4-52 (.zip)
Comment 3 Daniel Veditz [:dveditz] 2005-05-25 03:24:24 PDT
Note: I didn't include the full text of the advisory which was
non-browser-specific and mostly describes the tool they used (Sounds a bit like
"mangleme" for image formats). It will be public soon enough. The salient fact
for us are the test images. We may want to spin off sub-bugs.
Comment 4 Daniel Veditz [:dveditz] 2005-05-25 03:49:27 PDT
I couldn't reproduce the crash with 00000038.ico in 1.0.4, the other two crash
the 1.0 branch. On the trunk errors are correctly reported with all three images
due to the fixes from bug 245631 -- go Son Le!
Comment 5 Christopher Aillon (sabbatical, not receiving bugmail) 2005-05-25 08:54:16 PDT
Oh, nice, that's the bug I've been trying to get into the branches for a bit.  :-)

I'm still for taking that patch on branches.
Comment 6 Daniel Veditz [:dveditz] 2005-05-25 10:53:57 PDT
Reassigning to biesi, there's more of his fingerprints on XBM than Pav's.
Comment 7 Christian :Biesinger (don't email me, ping me on IRC) 2005-05-25 12:56:58 PDT
OK. so all of these XBM files crash with SIGABRT here, because operator new[]
threw an OOM exception (std::bad_alloc I believe).

If I change the two new[] operators in nsXBMDecoder.cpp to new (std::nothrow)
(and include <new>), then I don't crash.
Comment 8 Daniel Veditz [:dveditz] 2005-05-25 13:47:57 PDT
I got a similar crash on a personal windows build using VC71, because unlike the
release versions shipped using VC6 later versions of vc will also throw on new
as required by the standard. It can be turned off by compiler option, but it's
probably safer to go with least-common-denominator malloc for these simple byte
arrays rather than worry about which compilers support which features.
Comment 9 Christian :Biesinger (don't email me, ping me on IRC) 2005-05-25 13:54:52 PDT
Created attachment 184530 [details] [diff] [review]
use malloc/free
Comment 10 Daniel Veditz [:dveditz] 2005-05-25 16:11:21 PDT
Comment on attachment 184530 [details] [diff] [review]
use malloc/free

sr=dveditz
Comment 11 Daniel Veditz [:dveditz] 2005-05-25 16:27:57 PDT
Can we get this in the alpha? It'd be good if it were there, but if so check in
with a vague-ish comment like "use malloc to avoid OOM exceptions thrown by
libstdc++". Don't mention the NISCC announcement.
Comment 12 Christian :Biesinger (don't email me, ping me on IRC) 2005-05-26 14:36:41 PDT
Comment on attachment 184530 [details] [diff] [review]
use malloc/free

fixes crashes on large images on non-msvc6 builds. should be low risk (just
uses malloc/calloc instead of new[])
Comment 13 chris hofmann 2005-05-26 14:46:42 PDT
Comment on attachment 184530 [details] [diff] [review]
use malloc/free

a=chofmann
Comment 14 Daniel Veditz [:dveditz] 2005-05-26 15:46:17 PDT
Fix checked into trunk for the Deer Park Alpha
Comment 15 Christopher Aillon (sabbatical, not receiving bugmail) 2005-05-30 05:00:51 PDT
Comment on attachment 184530 [details] [diff] [review]
use malloc/free

a=caillon for the branches
Comment 16 Christian :Biesinger (don't email me, ping me on IRC) 2005-06-03 07:36:02 PDT
checked in to MOZILLA_1_7_BRANCH:
Checking in bmp/nsBMPDecoder.cpp;
/cvsroot/mozilla/modules/libpr0n/decoders/bmp/nsBMPDecoder.cpp,v  <-- 
nsBMPDecoder.cpp
new revision: 1.24.2.3; previous revision: 1.24.2.2
done
Checking in bmp/nsICODecoder.cpp;
/cvsroot/mozilla/modules/libpr0n/decoders/bmp/nsICODecoder.cpp,v  <-- 
nsICODecoder.cpp
new revision: 1.30.2.1; previous revision: 1.30
done
Checking in xbm/nsXBMDecoder.cpp;
/cvsroot/mozilla/modules/libpr0n/decoders/xbm/nsXBMDecoder.cpp,v  <-- 
nsXBMDecoder.cpp
new revision: 1.10.4.1; previous revision: 1.10
done
Comment 17 Christian :Biesinger (don't email me, ping me on IRC) 2005-06-03 07:37:28 PDT
fixed on AVIARY_1_0_1_20050124_BRANCH:
Checking in modules/libpr0n/decoders/bmp/nsBMPDecoder.cpp;
/cvsroot/mozilla/modules/libpr0n/decoders/bmp/nsBMPDecoder.cpp,v  <-- 
nsBMPDecoder.cpp
new revision: 1.24.2.1.2.1.2.1; previous revision: 1.24.2.1.2.1
done
Checking in modules/libpr0n/decoders/bmp/nsICODecoder.cpp;
/cvsroot/mozilla/modules/libpr0n/decoders/bmp/nsICODecoder.cpp,v  <-- 
nsICODecoder.cpp
new revision: 1.30.16.1; previous revision: 1.30
done
Checking in modules/libpr0n/decoders/xbm/nsXBMDecoder.cpp;
/cvsroot/mozilla/modules/libpr0n/decoders/xbm/nsXBMDecoder.cpp,v  <-- 
nsXBMDecoder.cpp
new revision: 1.10.18.1; previous revision: 1.10
done
Comment 18 Christian :Biesinger (don't email me, ping me on IRC) 2005-06-03 07:38:56 PDT
Created attachment 185265 [details] [diff] [review]
branch patch

this is the patch I checked in to the branches (the trunk patch didn't apply
quite cleanly, because bug 245631 didn't land on the branches)
Comment 19 Ginn Chen 2005-06-07 00:17:35 PDT
I still got crashes with both trunk and branch on Fedora Core 3 and Suse.

Like this,
The program 'Gecko' received an X Window System error.
This probably reflects a bug in the program.
The error was 'BadAlloc (insufficient resources for operation)'.
  (Details: serial 33675 error_code 11 request_code 53 minor_code 0)
  (Note to programmers: normally, X errors are reported asynchronously;
   that is, you will receive the error a while after causing it.
   To debug your program, run it with the --sync command line
   option to change this behavior. You can then get a meaningful
   backtrace from your debugger if you break on the gdk_x_error() function.)

The firefox I tested,
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2005-06-06-12-trunk-fs/
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2005-06-06-08-aviary1.0.1/

Did I miss something?
Comment 20 Ginn Chen 2005-06-07 03:54:23 PDT
The crash is in nsImageGTK.cpp
Line 176,
  mImageBits = (PRUint8*) new PRUint8[mSizeImage];

I changed it to malloc,
it didn't crash with gdb, but it still crashed without gdb.
I'm using debug build.
Comment 21 Ginn Chen 2005-06-08 02:20:50 PDT
Created attachment 185653 [details] [diff] [review]
patch for nsImageGTK.cpp

use malloc/free in nsImageGTK.cpp
Comment 22 Ginn Chen 2005-06-08 02:26:52 PDT
After my patch, mozilla won't crash on Solaris, but still crash on Linux.

It crashed at line 415 of nsImageGTK.cpp (line 411 before patch)

      gdk_draw_rgb_image_dithalign(mImagePixmap, sXbitGC,
                                   rect->x, rect->y,
                                   rect->width, rect->height,
                                   GDK_RGB_DITHER_MAX,
                                   mImageBits + mRowBytes*rect->y + 3*rect->x,
                                   mRowBytes,
                                   rect->x, rect->y);

I don't know how to deal with it.

My linux machine has 512M RAM, the malloc for hundreds of millions bytes just
returns a valid address.
Comment 23 Christian :Biesinger (don't email me, ping me on IRC) 2005-06-08 02:52:19 PDT
Comment on attachment 185653 [details] [diff] [review]
patch for nsImageGTK.cpp

>the malloc for hundreds of millions bytes just
>returns a valid address.

yeah, linux sucks like that...
Comment 24 Ginn Chen 2005-06-09 02:07:47 PDT
(In reply to comment #23)
> yeah, linux sucks like that...
> 

If I change xbm_width and xbm_height to 11383, I got crashes on Solaris too.
Sometimes 'BadAlloc', sometimes 'BadDrawable (invalid Pixmap or Window parameter)'
I think this problem is still there, if malloc returns not null.
Memory is run out, the system is fragile.

BTW: I wonder why this is a vulnerability.
Comment 25 Daniel Veditz [:dveditz] 2005-06-09 11:55:58 PDT
(In reply to comment #24)
> BTW: I wonder why this is a vulnerability.

Because NISCC has classed it as such. They only told us about the problems
(crashes) in our own program, perhaps another image-rendering program they
tested is actually exploitable.

Depending on how you look at it a potential DOS attack *is* a vulnerability.
Comment 26 Ginn Chen 2005-06-16 02:58:12 PDT
should patch nsImageGTK.cpp to avoid OOM exceptions
BTW: this is published at http://www.niscc.gov.uk/niscc/docs/al-20050614-00488.html
Comment 27 Jay Patel [:jay] 2005-06-16 16:02:33 PDT
Let's get the latest patch reviewed and see what other work needs to be done so
we can get this checked in soon, since this vulnerability went public recently.
Comment 28 Daniel Veditz [:dveditz] 2005-06-16 21:15:32 PDT
Comment on attachment 185653 [details] [diff] [review]
patch for nsImageGTK.cpp

sr=dveditz
a=dveditz for trunk and branches.

What about the equivalent spots in all the other platform gfx nsImageFoo ?
Comment 29 Daniel Veditz [:dveditz] 2005-06-16 22:14:33 PDT
(In reply to comment #28)
> What about the equivalent spots in all the other platform gfx nsImageFoo ?

That is, most of these spots:
http://lxr.mozilla.org/mozilla/search?string=new%20PRUint8

(I notice we missed one in nsICODecoder.cpp, too)
Comment 30 Ginn Chen 2005-06-16 23:11:38 PDT
What about
325       mColors = new colorTable[mNumColors];
326       if (!mColors)
327         return NS_ERROR_OUT_OF_MEMORY;
in nsICODecoder.cpp?
Comment 31 Ginn Chen 2005-06-16 23:56:06 PDT
MOZILLA_1_7_BRANCH:
Checking in gfx/src/gtk/nsImageGTK.cpp;
/cvsroot/mozilla/gfx/src/gtk/nsImageGTK.cpp,v  <--  nsImageGTK.cpp
new revision: 1.131.2.3; previous revision: 1.131.2.2
done

AVIARY_1_0_1_20050124_BRANCH:
Checking in nsImageGTK.cpp;
/cvsroot/mozilla/gfx/src/gtk/nsImageGTK.cpp,v  <--  nsImageGTK.cpp
new revision: 1.131.2.1.2.2.2.1; previous revision: 1.131.2.1.2.2
done

trunk:
Checking in nsImageGTK.cpp;
/cvsroot/mozilla/gfx/src/gtk/nsImageGTK.cpp,v  <--  nsImageGTK.cpp
new revision: 1.149; previous revision: 1.148
done
Comment 32 Christopher Aillon (sabbatical, not receiving bugmail) 2005-06-17 06:08:57 PDT
Now public: http://www.niscc.gov.uk/niscc/docs/al-20050614-00488.html

Opening bug.  Note that this is just a DoS, and not exploitable.
Comment 33 Jay Patel [:jay] 2005-06-17 15:13:35 PDT
This vulnerability is fixed, please log new bugs for any remaining issues.
Comment 34 Jay Patel [:jay] 2005-07-06 18:36:38 PDT
v.fixed on aviary with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.9)
Gecko/20050706 Firefox/1.0.5 using windows .ico images.  No crash.  

Since we saw more problems on Linux, we need to also verify this on that platform.
Comment 35 Adam Guthrie 2005-12-02 11:46:00 PST
*** Bug 238401 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.