328258 - Firefox "ARGB32_image_ARGB32 ()" .gif File Processing DoS

Status: VERIFIED FIXED

(Core :: Graphics, defect, P1)

Description

[Tom Ferris]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20060221 Firefox/1.6a1
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20060221 Firefox/1.6a1

A file processing vulnerability exists within Firefox and Deer Park Alpha 2, which allows for an attacker to cause the browser to crash, and or execute arbitrary code on a targeted host.

The vulnerability is caused due to a boundary error within the “ARGB32_image_ARGB32 ()” function when processing a malformed .gif  file.  Below is output from gdb on OS X 10.4.5 PPC with Deerpark2 (build 2006022104):

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_INVALID_ADDRESS at address: 0x0efcd6f8
0x9043c6f0 in ARGB32_image_ARGB32 ()

Versions Tested:
Firefox 1.5.0.1 on OS X 10.4.5 PPC
Deer Park Alpha 2 build: 2006022104

Talkback Incident #:
TB15506379Q

Please let me know if you need any other information.

Thanks,

-- 

Tom Ferris
Researcher
www.security-protocols.com
Key fingerprint = 0DFA 6275 BA05 0380 DD91  34AD C909 A338 D1AF 5D78



Reproducible: Always

Steps to Reproduce:

Comment 1

[Tom Ferris]

Attachment: .gif file testcase

.gif PoC

Comment 2

[Tom Ferris]

Also forgot to mention that win32 was not affected by this.  I have not yet tested linux.

Comment 3

[Daniel Veditz [:dveditz]]

Looks like it's crashing down in mac libraries. Josh, could you test this image on some other mac programs, particularly Safari and Camino?

Talkback information is a little suspect here: nsRenderingContextImpl.cpp only has 497 lines, couldn't be crashing on ling 848 

CoreGraphics.256.33.0 + 0xca6f0 (0x9043c6f0)
CoreGraphics.256.33.0 + 0x718f0 (0x903e38f0)
libRIP.A.dylib.256.30.0 + 0x3388 (0x94662388)
libRIP.A.dylib.256.30.0 + 0x5d14 (0x94664d14)
libRIP.A.dylib.256.30.0 + 0x5788 (0x94664788)
libRIP.A.dylib.256.30.0 + 0x418c (0x9466318c)
CoreGraphics.256.33.0 + 0x5f0ac (0x903d10ac)
CoreGraphics.256.33.0 + 0x5f014 (0x903d1014)
nsImageMac::Draw()   nsRenderingContextImpl::DrawImage()  [/builds/tinderbox/Fx-Trunk/Darwin_7.9.0_Depend/mozilla/gfx/src/shared/nsRenderingContextImpl.cpp, line 848]
nsImageFrame::PaintImage()  [/builds/tinderbox/Fx-Trunk/Darwin_7.9.0_Depend/mozilla/layout/generic/nsImageFrame.cpp, line 545]
nsDisplayImage::Paint()  [/builds/tinderbox/Fx-Trunk/Darwin_7.9.0_Depend/mozilla/layout/generic/nsImageFrame.cpp, line 177]
nsDisplayList::Paint()   nsDisplayClip::Paint()   nsDisplayList::Paint()   nsLayoutUtils::PaintFrame()  [/builds/tinderbox/Fx-Trunk/Darwin_7.9.0_Depend/mozilla/layout/base/nsLayoutUtils.cpp, line 712]
PresShell::Paint()   nsViewManager::RenderViews()  [/builds/tinderbox/Fx-Trunk/Darwin_7.9.0_Depend/mozilla/view/src/nsViewManager.cpp, line 861]
nsViewManager::Refresh()  [/builds/tinderbox/Fx-Trunk/Darwin_7.9.0_Depend/mozilla/view/src/nsViewManager.cpp, line 830]
nsViewManager::DispatchEvent()  [/builds/tinderbox/Fx-Trunk/Darwin_7.9.0_Depend/mozilla/view/src/nsViewManager.cpp, line 842]
HandleEvent()  [/builds/tinderbox/Fx-Trunk/Darwin_7.9.0_Depend/mozilla/view/src/nsView.cpp, line 176]
nsWindow::DispatchEvent()  [/builds/tinderbox/Fx-Trunk/Darwin_7.9.0_Depend/mozilla/widget/src/mac/nsWindow.cpp, line 1809]
nsWindow::DispatchWindowEvent()  [/builds/tinderbox/Fx-Trunk/Darwin_7.9.0_Depend/mozilla/widget/src/mac/nsWindow.cpp, line 1832]
nsWindow::UpdateWidget()  [/builds/tinderbox/Fx-Trunk/Darwin_7.9.0_Depend/mozilla/widget/src/mac/nsWindow.cpp, line 1635]
nsWindow::UpdateWidget()  [/builds/tinderbox/Fx-Trunk/Darwin_7.9.0_Depend/mozilla/widget/src/mac/nsWindow.cpp, line 830]
nsWindow::UpdateWidget()  [/builds/tinderbox/Fx-Trunk/Darwin_7.9.0_Depend/mozilla/widget/src/mac/nsWindow.cpp, line 830]
nsWindow::UpdateWidget()  [/builds/tinderbox/Fx-Trunk/Darwin_7.9.0_Depend/mozilla/widget/src/mac/nsWindow.cpp, line 830]
nsWindow::UpdateWidget()  [/builds/tinderbox/Fx-Trunk/Darwin_7.9.0_Depend/mozilla/widget/src/mac/nsWindow.cpp, line 830]
nsWindow::UpdateWidget()  [/builds/tinderbox/Fx-Trunk/Darwin_7.9.0_Depend/mozilla/widget/src/mac/nsWindow.cpp, line 830]
nsWindow::PaintUpdateRectProc()  [/builds/tinderbox/Fx-Trunk/Darwin_7.9.0_Depend/mozilla/widget/src/mac/nsWindow.cpp, line 1285]
nsWindow::HandleUpdateEvent()  [/builds/tinderbox/Fx-Trunk/Darwin_7.9.0_Depend/mozilla/widget/src/mac/nsWindow.cpp, line 70]
nsWindow::Update()  [/builds/tinderbox/Fx-Trunk/Darwin_7.9.0_Depend/mozilla/widget/src/mac/nsWindow.cpp, line 1241]
nsMacWindow::Update()  [/builds/tinderbox/Fx-Trunk/Darwin_7.9.0_Depend/mozilla/widget/src/mac/nsMacWindow.cpp, line 2173]
nsMacWindow::WindowEventHandler()  [/builds/tinderbox/Fx-Trunk/Darwin_7.9.0_Depend/mozilla/widget/src/mac/nsMacWindow.cpp, line 705]
DispatchEventToHandlers()
SendEventToEventTargetInternal()
SendEventToEventTargetWithOptions()
HandleWindowEvent()
ToolboxEventDispatcherHandler()
DispatchEventToHandlers()
SendEventToEventTargetInternal()
SendEventToEventTarget()
ToolboxEventDispatcher()
TryEventDispatcher()
GetOrPeekEvent()
GetNextEventMatchingMask()
WNEInternal()
WaitNextEvent()
nsMacMessagePump::GetEvent()   nsMacMessagePump::DoMessagePump()   nsAppShell::Run()  [/builds/tinderbox/Fx-Trunk/Darwin_7.9.0_Depend/mozilla/widget/src/mac/nsAppShell.cpp, line 108]
nsAppStartup::Run()   XRE_main()  [/builds/tinderbox/Fx-Trunk/Darwin_7.9.0_Depend/mozilla/toolkit/xre/nsAppRunner.cpp, line 2360]

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

> Reason: KERN_INVALID_ADDRESS at address: 0x0efcd6f8
> 0x9043c6f0 in ARGB32_image_ARGB32 ()

what does "bt" show when you crash, in gdb?

Comment 5

[Tom Ferris]

(In reply to comment #4)
> > Reason: KERN_INVALID_ADDRESS at address: 0x0efcd6f8
> > 0x9043c6f0 in ARGB32_image_ARGB32 ()
> 
> what does "bt" show when you crash, in gdb?
> 

Here is the backtrace using Deer Park 2:

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_INVALID_ADDRESS at address: 0x0efcd6f8
0x9043c6f0 in ARGB32_image_ARGB32 ()
(gdb) bt
#0  0x9043c6f0 in ARGB32_image_ARGB32 ()
#1  0x903e38f0 in ARGB32_image_mark ()
#2  0x903d5278 in ARGB32_image ()
#3  0x94662388 in ripd_Mark ()
#4  0x94664d14 in ripl_BltImage ()
#5  0x94664788 in ripc_RenderImage ()
#6  0x9466318c in ripc_DrawImage ()
#7  0x903d10ac in CGContextDelegateDrawImage ()
#8  0x903d1014 in CGContextDrawImage ()
#9  0x001860cc in nsImageMac::Draw ()
#10 0x004f626c in nsRenderingContextImpl::DrawImage ()
#11 0x00265474 in nsImageFrame::PaintImage ()
#12 0x0026524c in nsDisplayImage::Paint ()
#13 0x00512ad0 in nsDisplayList::Paint ()
#14 0x00514510 in nsDisplayClip::Paint ()
#15 0x00512ad0 in nsDisplayList::Paint ()
#16 0x0051598c in nsLayoutUtils::PaintFrame ()
#17 0x001b6380 in PresShell::Paint ()
#18 0x0027145c in nsViewManager::RenderViews ()
#19 0x00270ec0 in nsViewManager::Refresh ()
#20 0x00272da8 in nsViewManager::DispatchEvent ()
#21 0x005343bc in ViewWrapper::GetInterface ()
#22 0x006e3a6c in nsWindow::DispatchEvent ()
#23 0x006e3b34 in nsWindow::DispatchWindowEvent ()
#24 0x006e3694 in nsWindow::UpdateWidget ()
#25 0x006e3764 in nsWindow::UpdateWidget ()
#26 0x006e3764 in nsWindow::UpdateWidget ()
#27 0x006e3764 in nsWindow::UpdateWidget ()
#28 0x006e3764 in nsWindow::UpdateWidget ()
#29 0x006e3764 in nsWindow::UpdateWidget ()
#30 0x006e2f08 in nsWindow::PaintUpdateRectProc ()
#31 0x006e32c4 in nsWindow::HandleUpdateEvent ()
#32 0x006e2d64 in nsWindow::Update ()
#33 0x0037007c in nsMacWindow::Update ()
#34 0x0036dc44 in nsMacWindow::WindowEventHandler ()
#35 0x9317bff4 in DispatchEventToHandlers ()
#36 0x9317b74c in SendEventToEventTargetInternal ()
#37 0x9317b5c8 in SendEventToEventTargetWithOptions ()
#38 0x9324f160 in HandleWindowEvent ()
#39 0x93182890 in ToolboxEventDispatcherHandler ()
#40 0x9317c244 in DispatchEventToHandlers ()
#41 0x9317b74c in SendEventToEventTargetInternal ()
#42 0x931824ec in SendEventToEventTarget ()
#43 0x931c3564 in ToolboxEventDispatcher ()
#44 0x932625b8 in TryEventDispatcher ()
#45 0x9326220c in GetOrPeekEvent ()
#46 0x93262064 in GetNextEventMatchingMask ()
#47 0x93261df0 in WNEInternal ()
#48 0x93261d50 in WaitNextEvent ()
#49 0x006d9700 in nsMacMessagePump::GetEvent ()
#50 0x006d965c in nsMacMessagePump::DoMessagePump ()
#51 0x00369238 in nsAppShell::Run ()
#52 0x00405b80 in nsAppStartup::Run ()
#53 0x00014514 in XRE_main ()
#54 0x0000f698 in start ()
#55 0x0000f518 in start ()

Also, I have tried this using Safari and Preview (native OS X image viewer) and I do not get the crash.  I will test Camino in a few.

Comment 6

[Josh Aas]

I can't reproduce this crash with debug or opt builds of Firefox 1.5.0.1 on Mac OS X 10.4.5. I assume I'm supposed to repro this just by clicking on the test case and loading the image.

Comment 7

[Tom Ferris]

Attachment: testcase-2

Comment 8

[Tom Ferris]

I have attached another testcase, give that one a try.

Comment 9

[Josh Aas]

Attachment: gdb bt output

I can reproduce by loading the HTML file packaged with the GIF image, not by just loading the image. Here is the output from gdb bt, probably has better line numbers than talkback came up with.

Comment 10

[Josh Aas]

Repro steps:

1. Download the zip file that is the second attachment to this bug.
2. Open the HTML file contained inside in Firefox 1.5.0.1.

The HTML file contains only the following text:

<HEAD>
<META HTTP-EQUIV="Refresh" content="0;URL=bug-328258.html">
<IMG src="bug-328258.gif">

Comment 11

[Josh Aas]

When I try to open the HTML file in Safari, it refreshes constantly (from disk). This is probably the desired behavior given the meta tag in the HTML file. When I removed the meta line, we still crashed in the same place.

Comment 12

[Josh Aas]

Javier: iirc, you wrote our image rendering code that uses Quartz. Any ideas?

Comment 13

[Tom Ferris]

(In reply to comment #11)
> When I try to open the HTML file in Safari, it refreshes constantly (from
> disk). This is probably the desired behavior given the meta tag in the HTML
> file. When I removed the meta line, we still crashed in the same place.
> 

What version of Safari did you get the crash?  I just removed the META refresh tag, and I did not get the crash on Safari 2.0.3 (417.8)?

Comment 14

[Josh Aas]

This does not crash Camino 1.0. Camino draws a black bar on the page for the image.

Comment 15

[Josh Aas]

Tom: I didn't mean that I got Safari to crash. I meant that Safari did *not* crash. But I could see the refresh behavior in Safari since it didn't crash, and removing the refresh behavior from the file does not fix the crash in Firefox. Sorry for the unclear comment.

Comment 16

[Mark Mentovai]

Can someone tell us how this gif is malformed?  It'd be easier to attack if that was already known, instead of having to dissect it.

Comment 17

[Tom Ferris]

Attachment: camino crash screenshot

Comment 18

[jhp (no longer active)]

Strange: FF 1.5.0.1 (opt) crashes on the first load, but my debug build from the 1.8 branch reloads the image at least 15-20 times before it crashes.  But at the same spot.

Comment 19

[jhp (no longer active)]

Safari crashes for me too, but only if I load the gif and then start scrolling towards the bottom.

Comment 20

[jhp (no longer active)]

Hmm, if I set the malloc defines in the environment, then I crash here when loading the gif itself (whether resized or not):

Exception:  EXC_BAD_ACCESS (0x0001)
Codes:      KERN_PROTECTION_FAILURE (0x0002) at 0x017e0e54

Thread 0 Crashed:
0   libimglib2.dylib               	0x0ad9ec34 do_lzw(gif_struct*, unsigned char const*) + 868 (GIF2.cpp:347)
1   libimglib2.dylib               	0x0ad9f104 gif_write(gif_struct*, unsigned char const*, unsigned) + 356 (GIF2.cpp:460)
2   libimglib2.dylib               	0x0ada1208 nsGIFDecoder2::ProcessData(unsigned char*, unsigned, unsigned*) + 80 (nsGIFDecoder2.cpp:230)


You can enable the malloc env vars like so:
export MallocStackLogging=1
export MallocStackLoggingNoCompact=1
export MallocScribble=1
export MallocGuardEdges=1

CCing tor, since he might have some input.

Comment 21

[jhp (no longer active)]

Opened do_lzw() bug as bug 328509.  Not sure if that is the cause of this bug, though.

Comment 22

[jhp (no longer active)]

Workaround from bug 328509 didn't fix this crash.

Since I'm seeing similar crashes on both Firefox and Safari, I opened a bug report with Apple (#4458135).  We'll see if they have anything to say.

Comment 23

[Daniel Veditz [:dveditz]]

Why is this still unconfirmed? Multiple confirmations of this crash in the bug

Comment 24

[Josh Aas]

I have been working with Apple to get this fixed over the past week...

Comment 25

[Mike Connor [:mconnor]]

Josh, what's the status here?  It's been five months since last update.

Comment 26

[Mats Palmgren (inactive)]

Attachment: more GDB data

Comment 27

[Mats Palmgren (inactive)]

Attachment: wallpaper

This crash is weird. AFAICT it's a Quartz crash when scaling down a large
image to a small destination rect (height: 57199 -> 5).
This wallpaper fixes the crash for the testcase. The values I chose for
kHugeScale and kReasonableScale is just a guess...

Comment 28

[Daniel Veditz [:dveditz]]

Josh: can you ping Apple again. It's been a year, if they're not going to fix it we're going to have to warn other developers.

We should also land Mats's fix if it wallpapers over the crash.

Comment 29

[Simon Fraser [no longer active]]

what's the radar bug for this?

Comment 30

[Josh Aas]

This still happens on Mac OS X 10.4.9, though I thought it might not since 10.4.9 release notes said that it fixed crashes in image decoding or something like that.

Daniel - I pointed this issue out to Window a couple months ago since Apple has done nothing about it for so long. She said she would talk to Apple, I haven't heard anything back in a while. CC'd her for a status update.

I don't have a radar # because I talked directly with someone at Apple who said they already had that bug in their system. I'll ask for the # from them.

Comment 31

[Josh Aas]

I filed Apple bug #5087549 on this issue to make sure they have it in their system.

Comment 32

[Stuart Parmenter]

clearing blocking-1.9? since this mac code isn't used on the trunk

Comment 33

[Mats Palmgren (inactive)]

Bug 376713 is the same issue for trunk.

Comment 34

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Do we know yet what the actual underlying issue is?  Is "AFAICT it's a Quartz crash when scaling down a large image to a small destination rect (height: 57199 -> 5)." correct?  Trying to figure out how to work around this in cairo.

Comment 35

[chris hofmann]

did the attempt at fixing in cairo pan out?

Comment 36

[Damon Sicore (:damons)]

Targeting M9 as this bug will block beta.

Comment 37

[John Daggett (:jtd)]

Original testcase still crashes:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a9pre) Gecko/2007092604 Minefield/3.0a9pre

Mac OS X 10.4.10 (8R2218) [Intel]

Comment 38

[John Daggett (:jtd)]

Attachment: simple test program that duplicates the crash

To duplicate crash with test program:

1. In PB, create a new project, Command Line, Standard Tool
2. Add in ApplicationServices framework
3. Copy in the attached main.c
4. Enable guard malloc:
   a. Debug > Enable Guard Malloc
   b. In project window, right-click on executable, Get Info
   c. In the Arguments tab, add the following env variables:
      MALLOC_PERMIT_INSANE_REQUESTS=1
      MALLOC_PROTECT_BEFORE=1
5. Debug > Debug Executable

Result: crash will occur in Quartz routine argb32_image_argb32 with a bad access error

Note: running normally works fine, you need to have guard malloc enabled

Comment 39

[John Daggett (:jtd)]

Interesting tidbit, without flipped CTM the code does not crash.  Not exactly a solution but possibly a workaround.

I'm going to debug this some more on Monday and file a new bug report with Apple.

Comment 40

[John Daggett (:jtd)]

Attachment: simple test app with project and run script

I played around with this some more today and added a segfault handler to make it easy to test with a variety of sizes.  It appears that CGImage objects are limited to bitmaps with heights of 32768 pixels or less.

To duplicate crash with test program:

1. Download the attached DMG file and copy the project folder
2. Open the project in PB
3. Build a debug version
4. Open Terminal and change the working directory to the project folder
5. Enter the command:

   ./runtest.sh

This will iterate over tests with different image sizes.  It's set up to automatically use guard malloc, so the crash will occur more easily.  To test a give set of sizes use:

   ./runtest.sh <width> <start height> <end height> <height stepsize>




1. In PB, create a new project, Command Line, Standard Tool
2. Add in ApplicationServices framework
3. Copy in the attached main.c
4. Enable guard malloc:
   a. Debug > Enable Guard Malloc
   b. In project window, right-click on executable, Get Info
   c. In the Arguments tab, add the following env variables:
      MALLOC_PERMIT_INSANE_REQUESTS=1
      MALLOC_PROTECT_BEFORE=1
5. Debug > Debug Executable

Result: crash will occur in Quartz routine argb32_image_argb32 with a bad
access error

Comment 41

[John Daggett (:jtd)]

Argh, ignore my copy/paste duplicate of the previous instructions at the end of the last comment...

Comment 42

[John Daggett (:jtd)]

As to the workaround/fix, I think we clearly need to put in code to limit the size of surfaces in cairo quartz surfaces.  We may also want to also put in explicit limits in our image lib code.  Will investigate more tomorrow.

Comment 43

[John Daggett (:jtd)]

With test app, logged a new Apple bug:

5514949 Calling CGDrawImage with large bitmap image crashes

Both Firefox and Safari crash while rendering GIF files with very large height values.  This boils down to a bug within the CGDrawImage code, it appears that bitmap images with heights larger than 32768 cause a bad pointer calculation to reference memory outside the src image buffer.

Reproducible: always

Steps:

1. Download the attached DMG file and copy the project folder
2. Open the project in PB
3. Build a debug version
4. Open Terminal and change the working directory to the project folder
5. Enter the command:

   ./runtest.sh

Result: looking carefully at the output shows that a segfault occurred for images above 32768 pixels in height but not below that.

The script uses guard malloc to make it clear when the code is referencing outside the buffer areas.

The gdb backtrace shows this stack:

#0  0x903d2574 in argb32_image_argb32 ()
#1  0x9034e92e in argb32_image_mark ()
#2  0x9033c320 in argb32_image ()
#3  0x943173a7 in ripl_Mark ()
#4  0x94310bb7 in ripl_BltImage ()
#5  0x943105f5 in ripc_RenderImage ()
#6  0x9430e48d in ripc_DrawImage ()
#7  0x90336de1 in CGContextDrawImage ()
#8  0x0000dc8e in createAndCopyImage (width=537, height=32769, stride=2148) at /Users/jd/dev/cgimagecrash/main.c:65
#9  0x0000debd in main (argc=4, argv=0xbffffa64) at /Users/jd/dev/cgimagecrash/main.c:131

It looks like the code in argb32_image_argb32 is using Fixed coordinates when referencing the src image buffer, the sar instruction below is a *signed* shift, so shifting 0x80008000 pulls in the high-order bit and results in 0xFFFF8000 instead of 0x00008000.

argb32_image_argb32
with height = 32769
0x903d24e1 <+142>:	mov    -64(%ebp),%eax   ; eax = 80008000
0x903d24e4 <+145>:	sar    $0x10,%eax       ; eax = FFFF8000
0x903d24e7 <+148>:	imul   -84(%ebp),%eax   ; eax = FBCE0000 (-84(sp) ==> 0x864)
0x903d24eb <+152>:	mov    %eax,-28(%ebp)

The offset calculated above is added to the src base, producing an address that is *below* the src base and hence the reference outside the src image buffer:

0x903d256e  <+0283>  mov    -44(%ebp),%edi  ; edi = FBCE0000
0x903d2571  <+0286>  add    -88(%ebp),%edi  ; edi = ABE27000 (-88(sp) ==> src base)
0x903d2574  <+0289>  mov    (%edi),%eax     ; *crash*

Comment 44

[Daniel Veditz [:dveditz]]

(In reply to comment #42)
> As to the workaround/fix, I think we clearly need to put in code to limit the
> size of surfaces in cairo quartz surfaces.  We may also want to also put in
> explicit limits in our image lib code.  Will investigate more tomorrow.

We do have a limit:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/gfx/src/shared/gfxImageFrame.cpp&rev=1.41&mark=87-89#60

I believe we originally tried 32K but that annoyed some fans of high-res NASA imagery that often have images 40K pixels wide, so we raised it.

There's a separate size check in Thebes, and it appears to be per platform. If all quartz images go through here we could lower the limit for mac only if necessary:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/gfx/thebes/src/gfxQuartzSurface.cpp&rev=1.11&mark=45#42

CheckSurfaceSize() takes an optional max size. If not specified it simply checks for overflow, but you could pass 32K and limit quartz images specifically. Currently only takes one limit checked against both dimensions though, which means we'd unnecessarily block wide but short images that would otherwise be OK.

Comment 45

[John Daggett (:jtd)]

I think for this we also need to integrate in the fix for bug 397293, which is related to error handling within the surface creation routines.

Comment 46

[John Daggett (:jtd)]

Attachment: beginnings of a patch, v.0.1

(In reply to comment #44)
> I believe we originally tried 32K but that annoyed some fans of high-res NASA
> imagery that often have images 40K pixels wide, so we raised it.
> 
> There's a separate size check in Thebes, and it appears to be per platform. If
> all quartz images go through here we could lower the limit for mac only if
> necessary:
>
> CheckSurfaceSize() takes an optional max size. If not specified it simply
> checks for overflow, but you could pass 32K and limit quartz images
> specifically. Currently only takes one limit checked against both dimensions
> though, which means we'd unnecessarily block wide but short images that would
> otherwise be OK.

I'm thinking along the lines of (1) put a limit in cairo code and (2) a hard-wired limit within CheckSurfaceSize() for the Mac.  I need to test this some more, I'm failing now in GIF decoder code, looks like something is ignoring allocation failure.

Comment 47

[John Daggett (:jtd)]

Attachment: patch for GIF case, v.0.2

Added in an additional check in gfxImageFrame and added in error handling code in GIF decoder.  Still need to run through the same checks for other image types.

Comment 48

[John Daggett (:jtd)]

Attachment: malformed GIF file that causes GifWrite to not call EndGIF

This is just the original testcase GIF, truncated to 128 bytes.  This is just enough to cause GifWrite to read in the image size, fail to create a large buffer, set gif_error state but then fall out of the read loop and return NS_OK without calling EndGIF.  Seems benign, even with an optimized build, but draws differently onscreen.  Probably not such a big deal...

Comment 49

[John Daggett (:jtd)]

One other thing, shouldn't the NS_ERROR's and NS_ASSERTION's be NS_WARNING's in gfxImageFrame::Init, at least the ones other than (width, height) < 0?  These sizes are dictated by content, not by other code, unless we are expecting all the image decoders to explicitly validate the incoming image size.

http://mxr.mozilla.org/seamonkey/source/gfx/src/shared/gfxImageFrame.cpp#60 

Comment 50

[John Daggett (:jtd)]

Additional info requested by Apple re bug 5514949:

I don't have the privs to see this bug anymore so I can't download the RunTestResult.rtf as requested.  Here are the results from running runtest.sh without any parameters.  Note that a SIGSEGV occurs when the height of the bitmap is set to 32769 pixels or above.  Here's the calculation that's going wrong:

srcPtr = ( srcBase + ( height - 1 ) * stride )
       = ( 0xb0147000 + 0xfbce0000 ) ==> 0xabe27000

The 0xabe27000 address is outside the bounds of the src buffer, so with guard pages enabled accessing it causes an access fault.

The correct calculation would look like:

       = ( 0xb0147000 + 0x04320000 ) ==> 0xb4467000

johnmb17lan1:~/dev/cgimagecrash jd$ ./runtest.sh
Allocations will be placed on word (4 byte) boundaries.
 - Small buffer overruns may not be noticed.
 - Applications using AltiVec instructions may fail.
GuardMalloc-11
width: 537 start height: 32765 stop height: 32775 step: 1 stride: 2148

h = 32765
Allocations will be placed on word (4 byte) boundaries.
 - Small buffer overruns may not be noticed.
 - Applications using AltiVec instructions may fail.
GuardMalloc-11
src: base 0xb0147000, width 537, height 32765, stride 2148
dest: base 0xb4479000, width 537, height 32765, stride 2148
Allocations will be placed on word (4 byte) boundaries.
 - Small buffer overruns may not be noticed.
 - Applications using AltiVec instructions may fail.
GuardMalloc-11

h = 32766
Allocations will be placed on word (4 byte) boundaries.
 - Small buffer overruns may not be noticed.
 - Applications using AltiVec instructions may fail.
GuardMalloc-11
src: base 0xb0147000, width 537, height 32766, stride 2148
dest: base 0xb447a000, width 537, height 32766, stride 2148
Allocations will be placed on word (4 byte) boundaries.
 - Small buffer overruns may not be noticed.
 - Applications using AltiVec instructions may fail.
GuardMalloc-11

h = 32767
Allocations will be placed on word (4 byte) boundaries.
 - Small buffer overruns may not be noticed.
 - Applications using AltiVec instructions may fail.
GuardMalloc-11
src: base 0xb0147000, width 537, height 32767, stride 2148
dest: base 0xb447a000, width 537, height 32767, stride 2148
Allocations will be placed on word (4 byte) boundaries.
 - Small buffer overruns may not be noticed.
 - Applications using AltiVec instructions may fail.
GuardMalloc-11

h = 32768
Allocations will be placed on word (4 byte) boundaries.
 - Small buffer overruns may not be noticed.
 - Applications using AltiVec instructions may fail.
GuardMalloc-11
src: base 0xb0147000, width 537, height 32768, stride 2148
dest: base 0xb447b000, width 537, height 32768, stride 2148
Allocations will be placed on word (4 byte) boundaries.
 - Small buffer overruns may not be noticed.
 - Applications using AltiVec instructions may fail.
GuardMalloc-11

h = 32769
Allocations will be placed on word (4 byte) boundaries.
 - Small buffer overruns may not be noticed.
 - Applications using AltiVec instructions may fail.
GuardMalloc-11
src: base 0xb0147000, width 537, height 32769, stride 2148
dest: base 0xb447c000, width 537, height 32769, stride 2148
SEGV addr: 0xabe27000 %edi: 0xabe27000 -44(%ebp): 0xfbce0000 -88(%ebp): b0147000
Allocations will be placed on word (4 byte) boundaries.
 - Small buffer overruns may not be noticed.
 - Applications using AltiVec instructions may fail.
GuardMalloc-11

h = 32770
Allocations will be placed on word (4 byte) boundaries.
 - Small buffer overruns may not be noticed.
 - Applications using AltiVec instructions may fail.
GuardMalloc-11
src: base 0xb0147000, width 537, height 32770, stride 2148
dest: base 0xb447c000, width 537, height 32770, stride 2148
SEGV addr: 0xabe27864 %edi: 0xabe27864 -44(%ebp): 0xfbce0864 -88(%ebp): b0147000
Allocations will be placed on word (4 byte) boundaries.
 - Small buffer overruns may not be noticed.
 - Applications using AltiVec instructions may fail.
GuardMalloc-11

h = 32771
Allocations will be placed on word (4 byte) boundaries.
 - Small buffer overruns may not be noticed.
 - Applications using AltiVec instructions may fail.
GuardMalloc-11
src: base 0xb0147000, width 537, height 32771, stride 2148
dest: base 0xb447d000, width 537, height 32771, stride 2148
SEGV addr: 0xabe280c8 %edi: 0xabe280c8 -44(%ebp): 0xfbce10c8 -88(%ebp): b0147000
Allocations will be placed on word (4 byte) boundaries.
 - Small buffer overruns may not be noticed.
 - Applications using AltiVec instructions may fail.
GuardMalloc-11

h = 32772
Allocations will be placed on word (4 byte) boundaries.
 - Small buffer overruns may not be noticed.
 - Applications using AltiVec instructions may fail.
GuardMalloc-11
src: base 0xb0147000, width 537, height 32772, stride 2148
dest: base 0xb447d000, width 537, height 32772, stride 2148
SEGV addr: 0xabe2892c %edi: 0xabe2892c -44(%ebp): 0xfbce192c -88(%ebp): b0147000
Allocations will be placed on word (4 byte) boundaries.
 - Small buffer overruns may not be noticed.
 - Applications using AltiVec instructions may fail.
GuardMalloc-11

h = 32773
Allocations will be placed on word (4 byte) boundaries.
 - Small buffer overruns may not be noticed.
 - Applications using AltiVec instructions may fail.
GuardMalloc-11
src: base 0xb0147000, width 537, height 32773, stride 2148
dest: base 0xb447e000, width 537, height 32773, stride 2148
SEGV addr: 0xabe29190 %edi: 0xabe29190 -44(%ebp): 0xfbce2190 -88(%ebp): b0147000
Allocations will be placed on word (4 byte) boundaries.
 - Small buffer overruns may not be noticed.
 - Applications using AltiVec instructions may fail.
GuardMalloc-11

h = 32774
Allocations will be placed on word (4 byte) boundaries.
 - Small buffer overruns may not be noticed.
 - Applications using AltiVec instructions may fail.
GuardMalloc-11
src: base 0xb0147000, width 537, height 32774, stride 2148
dest: base 0xb447e000, width 537, height 32774, stride 2148
SEGV addr: 0xabe299f4 %edi: 0xabe299f4 -44(%ebp): 0xfbce29f4 -88(%ebp): b0147000
Allocations will be placed on word (4 byte) boundaries.
 - Small buffer overruns may not be noticed.
 - Applications using AltiVec instructions may fail.
GuardMalloc-11

h = 32775
Allocations will be placed on word (4 byte) boundaries.
 - Small buffer overruns may not be noticed.
 - Applications using AltiVec instructions may fail.
GuardMalloc-11
src: base 0xb0147000, width 537, height 32775, stride 2148
dest: base 0xb447f000, width 537, height 32775, stride 2148
SEGV addr: 0xabe2a258 %edi: 0xabe2a258 -44(%ebp): 0xfbce3258 -88(%ebp): b0147000
Allocations will be placed on word (4 byte) boundaries.
 - Small buffer overruns may not be noticed.
 - Applications using AltiVec instructions may fail.
GuardMalloc-11
johnmb17lan1:~/dev/cgimagecrash jd$

Comment 51

[John Daggett (:jtd)]

Attachment: malformed BMP file with height 57137px

Testing strategy:

For each image type (gif, bmp, jpg, icon, xbm, ico) create a sample image with a malformed height field.  We should be able to read these without crashing or any assertions firing (warnings fine).

gfxImageFrame::Init checks the height and width.  I'm thinking that's where tight limits should be enforced.  Basic sanity checking should occur within the decoder (i.e. width, height > 0, depth is something sane).

http://mxr.mozilla.org/seamonkey/source/gfx/src/shared/gfxImageFrame.cpp#60

Comment 52

[John Daggett (:jtd)]

Attachment: malformed BMP file with height 57137px

Comment 53

[John Daggett (:jtd)]

Attachment: malformed XBM image with 57137px height

Comment 54

[John Daggett (:jtd)]

Attachment: malformed PNG image with 50000px height

Comment 55

[John Daggett (:jtd)]

Attachment: malformed JPG image with 57137px height

Comment 56

[John Daggett (:jtd)]

Attachment: malformed PNG image with 57199px height

Comment 57

[John Daggett (:jtd)]

Attachment: testcase with lots of very naughty bits

This page includes references to all the malformed images attached to this bug to test large images for GIF, BMP, XBM, PNG, and JPG.  Patch should avoid crashing failure and assertions for all images on this page.

Right now I'm seeing for BMP and XBM the following assertions:

###!!! ASSERTION: writer returned an error with non-zero writeCount: '(NS_FAILED(rv) ? (*writeCount == 0) : PR_TRUE)', file /builds/trunk/mozilla/xpcom/io/nsInputStreamTee.cpp, line 105
###!!! ASSERTION: OnDataAvailable implementation consumed no data: 'Error', file /builds/trunk/mozilla/netwerk/base/src/nsInputStreamPump.cpp, line 529

Occasionally I'm seeing this also, not quite sure what the image type that causes this is yet:

###!!! ASSERTION: cannot call on a dirty frame not currently being reflowed: '!NS_SUBTREE_DIRTY(this) || (GetStateBits() & NS_FRAME_IN_REFLOW)', file /builds/trunk/mozilla/layout/generic/nsFrame.cpp, line 556

Comment 58

[Damon Sicore (:damons)]

After discussing this with Vlad and Stuart, and since this is still a private security issue, we don't think this should block the beta release.  Moving to the next milestone.

Comment 59

[John Daggett (:jtd)]

Attachment: patch for all relevant cases, v1.0

Patch for trunk code, prevents crashes for all image types and adjust error handling code so that assertions don't occur handling large images that cause size-related error code to fire.

Comment 60

[Mark Mentovai]

Comment on attachment 284274 [details] [diff] [review]
patch for all relevant cases, v1.0

+  // CoreGraphics is limited to images < 32K in *height*, so clamp all surfaces on the Mac to that height
+  if (aHeight > SHRT_MAX) {

< in the comment and > in the code - these aren't equivalent unless one is <= or the other is >=.  Fix the proper one to make it clear that you've considered the off-by-one case.

Comment 61

[John Daggett (:jtd)]

(In reply to comment #60)
> (From update of attachment 284274 [details] [diff] [review])
> +  // CoreGraphics is limited to images < 32K in *height*, so clamp all
> surfaces on the Mac to that height
> +  if (aHeight > SHRT_MAX) {
> 
> < in the comment and > in the code - these aren't equivalent unless one is <=
> or the other is >=.  Fix the proper one to make it clear that you've considered
> the off-by-one case.

Comment 50 shows the calculation that is going astray, the failure actually occurs when (height - 1) > SHRT_MAX.  For simplicity sake, I'm going to the condition as is, hopefully we can remove it in the future.  The comment is a general one, it's not intended to explain the precise conditions for this bug.  CVS blame logs will trace back to this bug for fuller explanation.

Comment 62

[Stuart Parmenter]

Comment on attachment 284274 [details] [diff] [review]
patch for all relevant cases, v1.0

your tabs are wrong for the cairo files

vlad should take a look at the mac changes

+    // if error has already occurred, just return
+    if (decoder->mErrorResult != NS_OK) {
+        return NS_OK;
+    }

you shouldn't need this -- if you return an error from ProcessData and that returns it from ReadSegCb then you won't be called with more data -- the image load will be canceled.
+    if (NS_SUCCEEDED(rv) && mErrorResult != NS_OK) {

you should just use && NS_FAILED(mErrorResult) here

Comment 63

[John Daggett (:jtd)]

Attachment: patch for all relevant cases, v1.1

Rework error handling in GIF, BMP, XBM code to immediately abort image reading.  Corrected tabbing in cairo file.

Comment 64

[Stuart Parmenter]

Comment on attachment 285066 [details] [diff] [review]
patch for all relevant cases, v1.1

tabbing is still wrong in the cairo file.  they use some weird tab/space mix. (tabs for 8 spaces, spaces for 4)  -- the rest looks fine

Comment 65

[Colin Barrett [:cbarrett]]

(In reply to comment #64)
> (From update of attachment 285066 [details] [diff] [review])
> tabbing is still wrong in the cairo file.  they use some weird tab/space mix.
> (tabs for 8 spaces, spaces for 4)  -- the rest looks fine

IIRC that is how emacs works by default. (*sobs*)

Comment 66

[John Daggett (:jtd)]

(In reply to comment #64)
> (From update of attachment 285066 [details] [diff] [review])
> tabbing is still wrong in the cairo file.  they use some weird tab/space mix.
> (tabs for 8 spaces, spaces for 4)  -- the rest looks fine

Unless I'm doing something funky here I see that weird mix throughout cairo files.  If we're trying to keep these files in sync with the cairo repository, I'm not sure it's a good idea to fix up the splattermix of tabs and spaces.

Comment 67

[John Daggett (:jtd)]

Comment on attachment 285066 [details] [diff] [review]
patch for all relevant cases, v1.1

request vlad review of cairo changes

Comment 68

[John Daggett (:jtd)]

Attachment: patch for all relevant cases, v1.2

Now with tab-space technology...

Comment 69

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 285192 [details] [diff] [review]
 patch for all relevant cases, v1.2

You're going to hate me, but... can't review this type of diff.  Can you make a unified one?  -u8 ideally, add a "diff -u8pN" line to ~/.cvsrc

Comment 70

[John Daggett (:jtd)]

Attachment: patch for all relevant cases, v1.3

Argh, sorry about that!

Comment 71

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 285431 [details] [diff] [review]
patch for all relevant cases, v1.3

Looks great

Comment 72

[Stuart Parmenter]

Comment on attachment 285431 [details] [diff] [review]
patch for all relevant cases, v1.3

this should go in for beta

Comment 73

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 285431 [details] [diff] [review]
patch for all relevant cases, v1.3

a=endgame drivers

Comment 74

[Mike Beltzner [:beltzner, not reading bugmail]]

(that is to say, approved for M9 landing)

Comment 75

[Damon Sicore (:damons)]

John, please land this ASAP.
            

Comment 76

[John Daggett (:jtd)]

Checked in.  Waiting for a nightly to test with before marking fixed/resolved.  Note: first official checkin, no great destruction, yay!

Comment 77

[John Daggett (:jtd)]

Confirmed in latest nightly:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a9pre) Gecko/2007102404 Minefield/3.0a9pre

Comment 78

[Damon Sicore (:damons)]

Congratulations on your first commit!   :)

Comment 79

[georgi - hopefully not receiving bugspam]

is set DYLD_INSERT_LIBRARIES to /usr/lib/libgmalloc.dylib
described at:
http://developer.apple.com/documentation/Darwin/Reference/Manpages/man3/libgmalloc.3.html

expected to work with optimized trunk?

trunk doesn't seem to start with debugging heap.

Comment 80

[Bob Clary [:bc] (inactive)]

I couldn't crash with opt or debug 1.9.0 builds on macppc or windows with the testcases in this bug.

In fc6 linux I crash on the images with

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)'.

which appears to be bug 210931.

verifying.

Comment 81

[Daniel Veditz [:dveditz]]

Can we adapt this fix for the 1.8 branch?

Comment 82

[Daniel Veditz [:dveditz]]

Can we get a branch patch for this, please? The cairo/thebes bits at the very least would be different.

Comment 83

[Daniel Veditz [:dveditz]]

Guess we're not getting this one for 1.8.1.12, and it's not quite as reproducible as on trunk.

Comment 84

[John Daggett (:jtd)]

Received a mail from Apple stating the underlying bug should be fixed in the next 10.5 release, no mention of a 10.4 fix:

Hi John,

This is a courtesy email regarding Bug ID# 5514949.  We believe this issue has been addressed in Mac OS X Client 10.5.2, build 9C31.  Please verify with this release, and update this report with your results.

Mac OS X Client 10.5.2, build 9C31:
http://www.apple.com/support/downloads/macosx1052comboupdate.html

Bug reports requiring your update will appear under ‘My Originated Problems’.  Please review this bug report and provide the requested information via the Apple Bug Reporter. Once your report has been updated, Engineering will be alerted of the new information.

<http://bugreport.apple.com>

Thank you for your assistance in helping us discover and isolate bugs within our products. 

Best Regards,

Stoney Gamble
Apple Developer Connection 
Worldwide Developer Relations

Comment 85

[Samuel Sidler (old account; do not CC)]

10.5.2 is the current release. Can you confirm that it's been fixed?

Comment 86

[John Daggett (:jtd)]

(In reply to comment #85)
> 10.5.2 is the current release. Can you confirm that it's been fixed?

Will confirm today (Japan time...)

Comment 87

[John Daggett (:jtd)]

Confirmed fixed in Mac OS X Client 10.5.2, build 9C31.

Comment 88

[Al Billings [:abillings - ex-MoCo]]

Using a 10.5.2 OS X box, I've tried the attached test cases and image files in Firefox 2.0.0.12 (Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.12) Gecko/2008020121 Firefox/2.0.0.12). I cannot reproduce any crash at all in 2.0.0.12. 

Is this a 10.4 specific issue? If I can't reproduce the crash, it is hard to validate the fix. :-)

Comment 89

[John Daggett (:jtd)]

(In reply to comment #88)

As noted in comment #87 the underlying Apple bug was fixed in a 10.5.2 update, so you need to dig out a 10.5 build from before that.  Or with 10.4.

Comment 90

[Al Billings [:abillings - ex-MoCo]]

Ah, sorry. The eyes glazed over and I missed the note. We've got Tiger around so I'll verify on that when I get access to that machine tomorrow.

Comment 91

[Al Billings [:abillings - ex-MoCo]]

Verified in 10.4.11 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.13) Gecko/2008031115 Firefox/2.0.0.13.

Comment 92

[Jeff Walden [:Waldo]]

Can we dump a crashtest for this in the tree now, please?