The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla1.8beta2

Status

()

Core
ImageLib
P1
normal
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: dveditz, Assigned: Biesinger)

Tracking

({fixed-aviary1.0.5, fixed1.7.9})

Trunk
mozilla1.8beta2
x86
All
fixed-aviary1.0.5, fixed1.7.9
Points:
---
Bug Flags:
blocking1.7.9 +
blocking-aviary1.0.5 +
blocking-aviary1.0.8 -
blocking1.8b2 +
blocking-aviary1.5 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:dos] public on June 14. ready for branch landing)

Attachments

(5 attachments)

(Reporter)

Description

12 years ago
!!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.
(Reporter)

Comment 1

12 years ago
Created attachment 184486 [details]
windows .ico crashers (.zip)

Test images that crash Firefox on windows XP SP2
(Reporter)

Comment 2

12 years ago
Created attachment 184487 [details]
test images for SUSE linux 2.6.4-52 (.zip)
(Reporter)

Updated

12 years ago
Attachment #184486 - Attachment description: windows .ico crashers → windows .ico crashers (.zip)
(Reporter)

Updated

12 years ago
Attachment #184487 - Attachment description: test images for SUSE linux 2.6.4-52 → test images for SUSE linux 2.6.4-52 (.zip)
(Reporter)

Comment 3

12 years ago
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.
Whiteboard: [sg:fix] public on June 14
(Reporter)

Comment 4

12 years ago
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!
Depends on: 245631
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.
(Reporter)

Comment 6

12 years ago
Reassigning to biesi, there's more of his fingerprints on XBM than Pav's.
Assignee: pavlov → cbiesinger
OS: Windows XP → All
Priority: -- → P1
Target Milestone: --- → mozilla1.8beta2
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.
(Reporter)

Comment 8

12 years ago
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.
OS: All → Windows XP
Priority: P1 → --
Target Milestone: mozilla1.8beta2 → ---
Created attachment 184530 [details] [diff] [review]
use malloc/free
Attachment #184530 - Flags: superreview?(dveditz)
Attachment #184530 - Flags: review?(tor)
OS: Windows XP → All
Priority: -- → P1
Target Milestone: --- → mozilla1.8beta2
(Reporter)

Comment 10

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

sr=dveditz
Attachment #184530 - Flags: superreview?(dveditz) → superreview+
(Reporter)

Comment 11

12 years ago
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.
Flags: blocking1.8b2+
Flags: blocking-aviary1.1+
Flags: blocking-aviary1.0.5+
(Reporter)

Updated

12 years ago
Flags: blocking1.7.9+

Updated

12 years ago
Attachment #184530 - Flags: review?(tor) → review+
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[])
Attachment #184530 - Flags: approval1.8b2?

Comment 13

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

a=chofmann
Attachment #184530 - Flags: approval1.8b2? → approval1.8b2+
Attachment #184530 - Flags: approval1.7.9?
Attachment #184530 - Flags: approval-aviary1.0.5?
(Reporter)

Comment 14

12 years ago
Fix checked into trunk for the Deer Park Alpha
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Whiteboard: [sg:fix] public on June 14 → [sg:fix] public on June 14. Need branch landing
Comment on attachment 184530 [details] [diff] [review]
use malloc/free

a=caillon for the branches
Attachment #184530 - Flags: approval1.7.9?
Attachment #184530 - Flags: approval1.7.9+
Attachment #184530 - Flags: approval-aviary1.0.5?
Attachment #184530 - Flags: approval-aviary1.0.5+

Updated

12 years ago
Whiteboard: [sg:fix] public on June 14. Need branch landing → [sg:fix] public on June 14. ready for branch landing
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
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
Keywords: fixed-aviary1.0.5, fixed1.7.9
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

12 years ago
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

12 years ago
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

12 years ago
Created attachment 185653 [details] [diff] [review]
patch for nsImageGTK.cpp

use malloc/free in nsImageGTK.cpp
Attachment #185653 - Flags: superreview?(dveditz)
Attachment #185653 - Flags: review?(cbiesinger)

Comment 22

12 years ago
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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...
Attachment #185653 - Flags: review?(cbiesinger) → review+

Comment 24

12 years ago
(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.
(Reporter)

Comment 25

12 years ago
(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

12 years ago
should patch nsImageGTK.cpp to avoid OOM exceptions
BTW: this is published at http://www.niscc.gov.uk/niscc/docs/al-20050614-00488.html
Keywords: fixed-aviary1.0.5, fixed1.7.9

Comment 27

12 years ago
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.
(Reporter)

Comment 28

12 years ago
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 ?
Attachment #185653 - Flags: superreview?(dveditz)
Attachment #185653 - Flags: superreview+
Attachment #185653 - Flags: approval1.8b3+
Attachment #185653 - Flags: approval1.7.9+
Attachment #185653 - Flags: approval-aviary1.0.5+
(Reporter)

Comment 29

12 years ago
(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)
Assignee: cbiesinger → pavlov
Status: REOPENED → NEW
(Reporter)

Updated

12 years ago
Assignee: pavlov → cbiesinger

Comment 30

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

Comment 31

12 years ago
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
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.
Group: security
(Reporter)

Updated

12 years ago
Whiteboard: [sg:fix] public on June 14. ready for branch landing → [sg:dos] public on June 14. ready for branch landing

Comment 33

12 years ago
This vulnerability is fixed, please log new bugs for any remaining issues.
Flags: blocking-aviary1.0.6-
Keywords: fixed-aviary1.0.5, fixed1.7.9

Updated

12 years ago
Status: NEW → RESOLVED
Last Resolved: 12 years ago12 years ago
Resolution: --- → FIXED

Comment 34

12 years ago
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

12 years ago
*** Bug 238401 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.