295457 - NISCC vulnerability #891011 (Parsing of Various Image Formats by Web Browsers)

Status: RESOLVED FIXED

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

Description

[Daniel Veditz [:dveditz]]

!!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]]

Attachment: windows .ico crashers (.zip)

Test images that crash Firefox on windows XP SP2

Comment 2

[Daniel Veditz [:dveditz]]

Attachment: test images for SUSE linux 2.6.4-52 (.zip)

Comment 3

[Daniel Veditz [:dveditz]]

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]]

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)]

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]]

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)]

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]]

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)]

Attachment: use malloc/free

Comment 10

[Daniel Veditz [:dveditz]]

Comment on attachment 184530 [details] [diff] [review]
use malloc/free

sr=dveditz

Comment 11

[Daniel Veditz [:dveditz]]

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)]

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]

Comment on attachment 184530 [details] [diff] [review]
use malloc/free

a=chofmann

Comment 14

[Daniel Veditz [:dveditz]]

Fix checked into trunk for the Deer Park Alpha

Comment 15

[Christopher Aillon (sabbatical, not receiving bugmail)]

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)]

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)]

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)]

Attachment: 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]

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]

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]

Attachment: patch for nsImageGTK.cpp

use malloc/free in nsImageGTK.cpp

Comment 22

[Ginn Chen]

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)]

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]

(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]]

(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]

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]]

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]]

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]]

(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]

What about
325       mColors = new colorTable[mNumColors];
326       if (!mColors)
327         return NS_ERROR_OUT_OF_MEMORY;
in nsICODecoder.cpp?

Comment 31

[Ginn Chen]

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)]

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]]

This vulnerability is fixed, please log new bugs for any remaining issues.

Comment 34

[Jay Patel [:jay]]

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]

*** Bug 238401 has been marked as a duplicate of this bug. ***