Buffer overflows while processing DCM or PALM images

[Christiane]

FRsirt rates this high risk. Remote compromises in web apps using magick might be possible.

This is what the Debian folks have patched in their graphicsmagick package:

   * coders/dcm.c: Fix buffer overflow, thanks to M Joonas Pihlaja.
   * coders/palm.c: Fix multiple heap overflows, again thanks to M Joonas
     Pihlaja.

See http://packages.debian.org/changelogs/pool/main/g/graphicsmagick/graphicsmagick_1.1.7-9/changelog#versionversion1.1.7-9 I guess, Ubuntu's graphicsmagic sources are affected, too. For the imagemagick sources, which are different from Debian's graphicsmagick, the patch is:

[correction: turns out I missed two additional heap checks; added them to the diff]

--------------------------------8<--------------------------------

diff -Naur imagemagick-6.2.4.5/coders/dcm.c imagemagick-6.2.4.5-patched/coders/dcm.c
--- imagemagick-6.2.4.5/coders/dcm.c 2005-09-01 04:28:09.000000000 +0200
+++ imagemagick-6.2.4.5-patched/coders/dcm.c 2006-10-25 11:21:24.000000000 +0200
@@ -2949,7 +2949,7 @@
             /*
               Photometric interpretation.
             */
- for (i=0; i < (long) length; i++)
+ for (i=0; i < (long) Min(length, MaxTextExtent-1); i++)
               photometric[i]=(char) data[i];
             photometric[i]='\0';
             break;
diff -Naur imagemagick-6.2.4.5/coders/palm.c imagemagick-6.2.4.5-patched/coders/palm.c
--- imagemagick-6.2.4.5/coders/palm.c 2005-05-08 03:07:43.000000000 +0200
+++ imagemagick-6.2.4.5-patched/coders/palm.c 2006-10-25 12:19:42.000000000 +0200
@@ -397,7 +397,7 @@
               image->compression=RLECompression;
               for (i=0; i < (long) bytes_per_row; )
               {
- count=ReadBlobByte(image);
+ count=Min(ReadBlobByte(image), bytes_per_row-i);
                 byte=ReadBlobByte(image);
                 (void) ResetMagickMemory(one_row+i,(int) byte,count);
                 i+=count;
@@ -430,6 +430,8 @@
       indexes=GetIndexes(image);
       if (bits_per_pixel == 16)
         {
+ if (image->columns > 2*bytes_per_row)
+ ThrowReaderException(CorruptImageError,CorruptImage,image);
           for (x=0; x < (long) image->columns; x++)
           {
             color16=(*ptr++ << 8);
@@ -446,6 +448,8 @@
           bit=8-bits_per_pixel;
           for (x=0; x < (long) image->columns; x++)
           {
+ if (ptr - one_row >= bytes_per_row)
+ ThrowReaderException(CorruptImageError,CorruptImage,image);
             index=(IndexPacket) (mask-(((*ptr) & (mask << bit)) >> bit));
             indexes[x]=index;
             *q++=image->colormap[index];

-------------------------------->8---------------------------------

I cannot verify if this builds cleanly, since configure dies on me here:

configure:3001: gcc-3.4 -c -g -O2 conftest.c >&5
conftest.c:2: error: syntax error before "me"
configure:3007: $? = 1
configure: failed program was:
| #ifndef __cplusplus
| choke me
| #endif
configure:3151: checking for style of include used by make
configure:3179: result: GNU
configure:3207: checking dependency style of gcc-3.4
configure:3297: result: gcc3
configure:3320: checking how to run the C preprocessor
configure:3438: result: g++-3.4
configure:3462: g++-3.4 conftest.c
conftest.c:14: error: `Syntax' does not name a type
configure:3468: $? = 1
configure: failed program was:
| /* confdefs.h. */
|
| #define PACKAGE_NAME "magick/magick.h"
| #define PACKAGE_TARNAME "magick-magick-h"
| #define PACKAGE_VERSION " "
| #define PACKAGE_STRING "magick/magick.h "
| #define PACKAGE_BUGREPORT "http://www.imagemagick.org"
| /* end confdefs.h. */
| #ifdef __STDC__
| # include <limits.h>
| #else
| # include <assert.h>
| #endif
| Syntax error
configure:3462: g++-3.4 conftest.c
conftest.c:14: error: `Syntax' does not name a type
[...]

[Lubomir (lubomir-redhat-bugs)]

Description of problem:

M. Joonas Pihlaja discovered security flaws in GraphicsMagick that also affect
ImageMagick -- one possible buffer overflow in coders/dcm.c:ReadDCMImage() and
three possible heap overflows in
coders/palm.c:ReadPALMImage(). Debian project includes a fix for GraphicsMagick
1.1.7 among other changes in their patch.
Version-Release number of selected component (if applicable):

How reproducible:

Potentially exploitable by maliciously crafted image.

Fix:

I attach the relevant part of the debian patch. It doesn't apply against
ImageMagick without modifications, because GraphicMagics project uses different
coding style. The patch needs to be reviewed and eventually needs to be rewritten.

[Lubomir (lubomir-redhat-bugs)]

Created attachment 138585
Relevan excerpt from a Debian patch

[Martin Pitt (pitti)]

Confirmed that our imagemagick is affected. Thanks, Christiane!

[Martin Pitt (pitti)]

Adding graphicsmagick task, in case someone is interested in fixing. (universe)

[Martin Pitt (pitti)]

Fixed packages prepared, awaiting re-opening of security queue.

[Martin Pitt (pitti)]

Fix prepared.

[Martin Pitt (pitti)]

http://www.ubuntu.com/usn/usn-372-1

[Norm (norm-redhat-bugs)]

Picked up and have ported the patch into ImageMagick and done a test build
locally. Will be looking into which all releases are impacted tomorrow.

[Martin Pitt (pitti)]

 graphicsmagick (1.1.7-8ubuntu0.1) edgy-security; urgency=low
 .
   * SECURITY UPDATE: Remote arbitrary code execution.
   * debian/control: Fix libwmf-dev build dependency to build at all.
   * debian/rules: Don't have a failed test suite fail the build.
   * coders/dcm.c, ReadDCMImage(): Fix buffer overflow in loop for photometric
     interpretation (statically sized photometric array).
   * coders/palm.c, ReadPALMImage(): Prevent buffer overflows when decoding RLE
     compression, too many columns, or too long rows.
   * References:
     CVE-2006-5456
     Closes: LP#68144

[Josh (josh-redhat-bugs)]

Suse has informed us that there is a bug in this patch, here is the corrected chunk:

@@ -399,6 +399,7 @@
               for (i=0; i < (long) bytes_per_row; )
               {
                 count=ReadBlobByte(image);
+ count=Min(count, bytes_per_row-i);
                 byte=ReadBlobByte(image);
                 (void) ResetMagickMemory(one_row+i,(int) byte,count);
                 i+=count;

[David (david-redhat-bugs)]

This bug ticket was used as a reference for a Fedora Core 5 fix for the
CVE-2006-5456 issue in ImageMagick. Because of comment #5, I verified
the patch used for FC-5 for this issue, at this URL:
http://cvs.fedora.redhat.com/viewcvs/rpms/ImageMagick/FC-5/ImageMagick-6.2.5-cve-2006-5456.patch?sortby=date&view=markup

It looks like the section mentioned in comment #5 is fine in FC-5's patch.

Am I missing something, Josh, or is it just in a proposed patch for an
RHEL package for which the issue in comment #5 is relevant?

[Norm (norm-redhat-bugs)]

I have to disagree with the updated chunk as it is possible to reach that
section of code with count uninitialized which would then result in an infinite
loop.

        if (compressionType == PALM_COMPRESSION_RLE)
          {
            image->compression=RLECompression;
            for (i=0; i < (long) bytes_per_row; )
            {
              count=Min(ReadBlobByte(image),bytes_per_row-i);
              byte=ReadBlobByte(image);
              (void) ResetMagickMemory(one_row+i,(int) byte,count);
              i+=count;
            }
        }

being the whole segment. If count == 0 at the beginning of that loop, then one
will never get out of it.

Additionally upstream maintains (in current svn) something closer to the
original chunk:
          if (compressionType == PALM_COMPRESSION_RLE)
            {
              /* TODO move out of loop! */
              image->compression=RLECompression;
              for (i=0; i < (long) bytes_per_row; )
              {
                count=(ssize_t) Min(ReadBlobByte(image),(long) bytes_per_row-i);
                byte=(unsigned long) ReadBlobByte(image);
                (void) ResetMagickMemory(one_row+i,(int) byte,(size_t) count);
                i+=count;
              }
          }

[Lubomir (lubomir-redhat-bugs)]

The correct patch got CVE-2007-0770

[Red (red-redhat-bugs)]

An advisory has been issued which should help the problem
described in this bug report. This report is therefore being
closed with a resolution of ERRATA. For more information
on the solution and/or where to find the updated files,
please follow the link below. You may reopen this bug report
if the solution does not work for you.

http://rhn.redhat.com/errata/RHSA-2007-0015.html