Buffer overflows while processing DCM or PALM images

Bug #68144 reported by Christiane on 2006-10-25
254
Affects Status Importance Assigned to Milestone
graphicsmagick (Ubuntu)
High
Martin Pitt
imagemagick (Fedora)
Fix Released
Medium
imagemagick (Ubuntu)
High
Martin Pitt

Bug Description

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

CVE References

Revision history for this message
In , Lubomir (lubomir-redhat-bugs) wrote :

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.

Revision history for this message
In , Lubomir (lubomir-redhat-bugs) wrote :

Created attachment 138585
Relevan excerpt from a Debian patch

Christiane (cr-ct) on 2006-10-25
description: updated
Martin Pitt (pitti) on 2006-10-25
Changed in imagemagick:
assignee: nobody → pitti
importance: Undecided → High
Revision history for this message
Martin Pitt (pitti) wrote :

Confirmed that our imagemagick is affected. Thanks, Christiane!

Changed in imagemagick:
status: Unconfirmed → In Progress
Revision history for this message
Martin Pitt (pitti) wrote :

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

Changed in graphicsmagick:
importance: Undecided → High
status: Unconfirmed → Confirmed
Revision history for this message
Martin Pitt (pitti) wrote :

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

Changed in imagemagick:
status: In Progress → Fix Committed
Revision history for this message
Martin Pitt (pitti) wrote :

Fix prepared.

Changed in graphicsmagick:
status: Confirmed → Fix Committed
assignee: nobody → pitti
Revision history for this message
Martin Pitt (pitti) wrote :
Changed in imagemagick:
status: Fix Committed → Fix Released
Revision history for this message
In , Norm (norm-redhat-bugs) wrote :

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.

Revision history for this message
Martin Pitt (pitti) wrote :

 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

Changed in graphicsmagick:
status: Fix Committed → Fix Released
Changed in imagemagick:
status: Unknown → In Progress
Revision history for this message
In , Josh (josh-redhat-bugs) wrote :

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;

Revision history for this message
In , David (david-redhat-bugs) wrote :

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?

Revision history for this message
In , Norm (norm-redhat-bugs) wrote :

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;
              }
          }

Changed in imagemagick:
status: In Progress → Fix Committed
Revision history for this message
In , Lubomir (lubomir-redhat-bugs) wrote :

The correct patch got CVE-2007-0770

Revision history for this message
In , Red (red-redhat-bugs) wrote :

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

Changed in imagemagick:
status: Fix Committed → Fix Released
Changed in imagemagick (Fedora):
importance: Unknown → Medium
To post a comment you must log in.
This report contains Public Security information  Edit
Everyone can see this security related information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.