SEGV in 64-bit and 32-bit platforms 07c8accc

[JodieC]

Jodie's ID: 07c8accc
Source file that causes the SEGV:
https://www.dropbox.com/s/l6vmxamc8i8xy ... cc_00?dl=0

IM version: ImageMagick-6.8.9-8
32-bit Debian Wheezy (TKL)
64-bit Ubuntu 14.04

Command line: convert $filename png:/dev/null

Code: Select all

gdb run:
7a618fe in GetEXIFProperty (property=<optimized out>, image=0x80767e8) at magick/property.c:1412
1412	                    if ((isprint((int) p[i]) != 0) || (p[i] == '\0'))
gdb> context
_______________________________________________________________________________
     eax:B6902988 ebx:B7F7FF3C  ecx:B5901008  edx:00000000     eflags:00010246
     esi:B5901008 edi:00FFFFEB  esp:BFFF0550  ebp:070891BB     eip:B7A618FE
     cs:0073  ds:007B  es:007B  fs:0000  gs:0033  ss:007B    o d I t s Z a P c 
[007B:BFFF0550]---------------------------------------------------------[stack]
BFFF0580 : A8 A3 04 08  70 B9 04 08 - 70 B8 04 08  F0 BE 04 08 ....p...p.......
BFFF0570 : F0 BE 04 08  F4 0F B4 B7 - E8 BE 04 08  78 C7 04 08 ............x...
BFFF0560 : 00 00 52 40  00 00 00 00 - 00 00 F0 3F  B8 3C 06 08 ..R@.......?.<..
BFFF0550 : EC FF FF 00  01 00 00 00 - 74 91 08 08  D4 46 E7 B7 ........t....F..
[007B:B5901008]---------------------------------------------------------[ data]
B5901008 : 2E 00 00 00  00 00 00 00 - 00 00 00 00  00 00 00 00 ................
B5901018 : 00 00 00 00  00 00 00 00 - 00 00 00 00  00 00 00 00 ................
[0073:B7A618FE]---------------------------------------------------------[ code]
=> 0xb7a618fe <GetImageProperty+8734>:	movzbl 0x0(%ebp,%edx,1),%ecx
   0xb7a61903 <GetImageProperty+8739>:	mov    (%eax),%eax
   0xb7a61905 <GetImageProperty+8741>:	movzbl %cl,%edi
   0xb7a61908 <GetImageProperty+8744>:	testb  $0x40,0x1(%eax,%edi,2)
   0xb7a6190d <GetImageProperty+8749>:	jne    0xb7a61913 <GetImageProperty+8755>
   0xb7a6190f <GetImageProperty+8751>:	test   %cl,%cl
------------------------------------------------------------------------------
gdb> bt
#0  0xb7a618fe in GetEXIFProperty (property=<optimized out>, image=0x80767e8) at magick/property.c:1412
#1  GetImageProperty (image=image@entry=0x80767e8, property=property@entry=0xbfff37d0 "exif:*") at magick/property.c:2113
#2  0xb7a6d1d2 in SetImageProfileInternal (image=0x80854b8, name=0xbfff4830 "exif", profile=0x80854b8, recursive=MagickFalse) at magick/profile.c:1740
#3  0xb7a6e447 in SetImageProfile (image=0x80854b8, image@entry=0x80767e8, name=0x0, name@entry=0xbfff4830 "exif", profile=profile@entry=0x80854b8) at magick/profile.c:1747
#4  0xb7deb756 in ReadProfile (jpeg_info=0xbfff5b6c) at coders/jpeg.c:709
#5  0xb742c68b in ?? () from /usr/lib/i386-linux-gnu/libjpeg.so.8
#6  0xb7429d2e in ?? () from /usr/lib/i386-linux-gnu/libjpeg.so.8
#7  0xb7422927 in jpeg_consume_input () from /usr/lib/i386-linux-gnu/libjpeg.so.8
#8  0xb7422ba3 in jpeg_read_header () from /usr/lib/i386-linux-gnu/libjpeg.so.8
#9  0xb7dee6fe in ReadJPEGImage (image_info=0x8055798, exception=0x804a318) at coders/jpeg.c:1071
#10 0xb787f6de in ReadImage (image_info=image_info@entry=0x8051690, exception=exception@entry=0x804a318) at magick/constitute.c:492
#11 0xb78821f1 in ReadImages (image_info=image_info@entry=0x8051690, exception=exception@entry=0x804a318) at magick/constitute.c:853
#12 0xb75f7a4d in ConvertImageCommand (image_info=0x8051690, argc=0x3, argv=0x804b3e8, metadata=0x0, exception=0x804a318) at wand/convert.c:619
#13 0xb76d2546 in MagickCommandGenesis (image_info=0x804d588, command=0x80488a0 <ConvertImageCommand@plt>, argc=0x3, argv=0xbffff594, metadata=0x0, exception=0x804a318) at wand/mogrify.c:168
#14 0x08048a55 in ConvertMain (argv=0xbffff594, argc=0x3) at utilities/convert.c:81
#15 main (argc=0x3, argv=0xbffff594) at utilities/convert.c:92

[magick]

We can reproduce the problem you posted and have a patch in ImageMagick 6.8.9-10 Beta available by sometime tomorrow. Thanks.

Code: Select all

*** magick/property.c~  2014-10-29 12:04:10.922259683 -0400
--- magick/property.c   2014-10-29 11:51:55.617215440 -0400
***************
*** 1321,1326 ****
--- 1321,1328 ----
              The directory entry contains an offset.
            */
            offset=(ssize_t) ((int) ReadPropertyLong(endian,q+8));
+           if ((offset < 0) || (size_t) offset >= length)
+             continue;
            if ((ssize_t) (offset+number_bytes) < offset)
              continue;  /* prevent overflow */
            if ((size_t) (offset+number_bytes) > length)

[broucaries]

Another security bug (dos) i suppose...

[JodieC]

Another security bug (dos) i suppose...

Yep, with more to come. I used AFL(http://lcamtuf.coredump.cx/afl/) for this.

[magick]

Test against ImageMagick-6.9.0-1 Beta @ http://www.imagemagick.org/download/beta/. The Google security team has alerted us to a number of problems discovered by a fuzzer and we have attended to them. They intend to alert us to additional possible flaws soon.

[broucaries]

Could you send me (possibly encrypted) the patch ?

Could you also send me if not public when we should release ?

Bastien

[magick]

The patches are in ImageMagick 6.9.0-1 Beta, all in the coders folders (e.g. coders/pnm.c). They should be 100% compatible with the 6.8.9-6 release.

[broucaries]

Ok

I suppose these patch:
- http://trac.imagemagick.org/changeset/1 ... ders/dpx.c
- http://trac.imagemagick.org/changeset/1 ... ders/pcx.c
- http://trac.imagemagick.org/changeset/1 ... ders/pdb.c
- http://trac.imagemagick.org/changeset/1 ... ders/pnm.c
- http://trac.imagemagick.org/changeset/1 ... ers/pict.c
- http://trac.imagemagick.org/changeset/1 ... ders/psd.c
- http://trac.imagemagick.org/changeset/1 ... oders/ps.c
- http://trac.imagemagick.org/changeset/1 ... ders/sun.c
- http://trac.imagemagick.org/changeset/1 ... ders/xpm.c
- http://trac.imagemagick.org/changeset/1 ... ders/wpg.c

I have doubt about this one
- http://trac.imagemagick.org/changeset/1 ... ers/tiff.c
- http://trac.imagemagick.org/changeset/1 ... ers/viff.c
- http://trac.imagemagick.org/changeset/1 ... ders/png.c

What is this one :
http://trac.imagemagick.org/changeset/1 ... ers/jpeg.c

I need a description for each of this problem. I can write some of it but I need review

Bastien

[magick]

Regarding the JPEG patch. Its not a flaw patch. It instead supports JPEG images with a JPE file extension.

[broucaries]

Ok let try something

Ok

I suppose these patch:
- http://trac.imagemagick.org/changeset/1 ... ders/dpx.c

This one I have the beggining:
"A malformed imageme could trigger "
What are the consequence ? Do you have a CVE ?

- http://trac.imagemagick.org/changeset/1 ... ders/pcx.c

A heap buffer overrun triggered by special crafted pcx image. Could lead to a DOS.
Do you have a CVE ? Do you have a reproducer file ? BTW do you have a well know testsuite of malformed image ?

- http://trac.imagemagick.org/changeset/1 ... ders/pdb.c

A heap buffer overrun triggered by special crafted pdb image. Could lead to a DOS.
Do you have a CVE ? Do you have a reproducer file ?

- http://trac.imagemagick.org/changeset/1 ... ders/pnm.c

A off by one error ?
Do you have a CVE ? Do you have a reproducer file ?

- http://trac.imagemagick.org/changeset/1 ... ers/pict.c

A heap buffer overrun triggered by special crafted pict image. Could lead to a DOS.
Do you have a CVE ? Do you have a reproducer file ?

- http://trac.imagemagick.org/changeset/1 ... ders/psd.c

A heap buffer overrun triggered by special crafted psd image. Could lead to a DOS.
Do you have a CVE ? Do you have a reproducer file ?

- http://trac.imagemagick.org/changeset/1 ... oders/ps.c

A pointer dereference in ps handling code. Could lead to a DOS.
Do you have a CVE ? Do you have a reproducer file ?

- http://trac.imagemagick.org/changeset/1 ... ders/sun.c

An out of bound acess on malformed sun file ? Could you assert severity ? Do you have a CVE ? Do you have a reproducer file ?

- http://trac.imagemagick.org/changeset/1 ... ders/xpm.c

Need help here? An malformed xpl file could... ? Do you have a CVE ? Do you have a reproducer file ?

- http://trac.imagemagick.org/changeset/1 ... ders/wpg.c

A heap buffer overrun triggered by special crafted wpg image. Could lead to a DOS.
Do you have a CVE ? Do you have a reproducer file ?

I have doubt about this one
- http://trac.imagemagick.org/changeset/1 ... ers/tiff.c
- http://trac.imagemagick.org/changeset/1 ... ers/viff.c
- http://trac.imagemagick.org/changeset/1 ... ders/png.c

Need help for theses.

What is this one :
http://trac.imagemagick.org/changeset/1 ... ers/jpeg.c

Not a security problem

I need a description for each of this problem. I can write some of it but I need review

Bastien

[magick]

The image fuzzer presents a set of images with boundary conditions that cause faults in numerous open-source projects including Firefox, Google, and of course ImageMagick. They typically present as either heap corruption, noted by valgrind or asan, or a seg-fault. We identify the flaw in ImageMagick and patch it. We do not know the security implications. In most cases, ImageMagick completes the command and in rare cases returns a seg-fault.

Find the sample images that reveal the various flaws @ http://www.imagemagick.org/im_samples_2 ... d7.tar.bz2.

By default, ImageMagick will attempt to allocate as much memory as the image dimension demand. Set the area resource in policy.xml to a reasonable value to prevent large images from being allocated.

[broucaries]

Ok so my description are correct. Could you help me for tiff one and the png one ?

BTW the viff one is buggy and non portable. Cast to ssize_t could lead to negative number and thus implementation dependant access. Do you mean size_t here ? if it is negative it should be checked before passing to bound.

Bastien

[magick]

> Ok so my description are correct. Could you help me for tiff one and the png one ?

Glenn is the PNG maintainer (the author of libpng). Glenn can you report on your recent patches to coders/png.c?

The TIFF library TIFFGetField() uses var args. In most cases there are @ most 2 possible return arguments, and depending on the first argument might return a third argument. A user discovered this flaw and fixed it with the recent patch.

> BTW the viff one is buggy and non portable. Cast to ssize_t could lead to negative number and thus implementation dependent access. Do you mean
> size_t here ? if it is negative it should be checked before passing to bound.

ConstrainColormapIndex() ensures the colormap index is never negative. However, by convention, all offsets in ImageMagick are ssize_t-- since offsets could go negative-- although ConstrainColormapIndex() insures that can't happen.

[broucaries]

> Ok so my description are correct. Could you help me for tiff one and the png one ?

Glenn is the PNG maintainer (the author of libpng). Glenn can you report on your recent patches to coders/png.c?

The TIFF library TIFFGetField() uses var args. In most cases there are @ most 2 possible return arguments, and depending on the first argument might return a third argument. A user discovered this flaw and fixed it with the recent patch.

> BTW the viff one is buggy and non portable. Cast to ssize_t could lead to negative number and thus implementation dependent access. Do you mean
> size_t here ? if it is negative it should be checked before passing to bound.

ConstrainColormapIndex() ensures the colormap index is never negative. However, by convention, all offsets in ImageMagick are ssize_t-- since offsets could go negative-- although ConstrainColormapIndex() insures that can't happen.

You should therefore ensure that ConstrainColormapIndex() return casted to ssize_t is greater than 0 in ConstrainColormapIndex() . I do not see this check.

Bastien

[magick]

Agreed. Although unlikely image->colors could be greater than MAX_SSIZE_T, it might be. We'll add a check. Thanks.