Page 1 of 1

ImageMagick Invalid Validation and Denial of Service

Posted: 2012-02-03T19:08:45-07:00
by magick
Concerning ImageMagick 6.7.5-7 and earlier:
  • [CVE-2012-0247] When parsing a maliciously crafted image with incorrect offset and count in the ResolutionUnit tag in EXIF IFD0, ImageMagick copies two bytes into an invalid address.
  • [CVE-2012-0248] When parsing a maliciously crafted image with an IFD whose all IOP tags' value offsets point to the beginning of the IFD itself. As a result, ImageMagick parses the IFD structure indefinitely, causing a denial of service.
Thanks goes to the Mr Joonas Kuorilehto & Mr Aleksis Kauppinen from Codenomicon CROSS project for discovering the vulnerabilities and providing a test case file. Also to the Finnish Communications Regulatory Authority (CERT-FI) for alerting us to these vulnerabilities/

Thanks goes to security @ redhat.com for alerting us that SyncImageProfiles() is vulnerable to CVE-2012-0248.

Here are patches that repairs these vulnerabilities:

Code: Select all

--- ImageMagick-6.7.4-10/magick/property.c      2011-12-23 15:52:11.000000000 -0500
+++ ImageMagick-6.7.5-8/magick/property.c       2012-02-29 09:55:22.305068822 -0500
@@ -443,6 +443,13 @@
   return(y);
 }
 
+static inline ssize_t MagickMin(const ssize_t x,const ssize_t y)
+{
+  if (x < y)
+    return(x);
+  return(y);
+}
+
 static inline int ReadPropertyByte(const unsigned char **p,size_t *length)
 {
   int
@@ -577,7 +584,7 @@
       continue;
     if (ReadPropertyByte(&info,&length) != (unsigned char) 'M')
       continue;
-    id=(ssize_t) ReadPropertyMSBShort(&info,&length);
+    id=(ssize_t) ((int) ReadPropertyMSBShort(&info,&length));
     if (id < (ssize_t) start)
       continue;
     if (id > (ssize_t) stop)
@@ -608,7 +615,7 @@
             No name match, scroll forward and try next.
           */
           info+=count;
-          length-=count;
+          length-=MagickMin(count,(ssize_t) length);
           continue;
         }
     if ((*name == '#') && (sub_number != 1))
@@ -618,7 +625,7 @@
         */
         sub_number--;
         info+=count;
-        length-=count;
+        length-=MagickMin(count,(ssize_t) length);
         continue;
       }
     /*
@@ -633,7 +640,7 @@
         (void) CopyMagickMemory(attribute,(char *) info,(size_t) count);
         attribute[count]='\0';
         info+=count;
-        length-=count;
+        length-=MagickMin(count,(ssize_t) length);
         if ((id <= 1999) || (id >= 2999))
           (void) SetImageProperty((Image *) image,key,(const char *)
             attribute);
@@ -773,7 +780,9 @@
       *directory;
 
     size_t
-      entry,
+      entry;
+
+    ssize_t
       offset;
   } DirectoryInfo;
 
@@ -1085,14 +1094,17 @@
     entry,
     length,
     number_entries,
-    tag_offset,
     tag;
 
+  SplayTreeInfo
+    *exif_resources;
+
   ssize_t
     all,
     id,
     level,
     offset,
+    tag_offset,
     tag_value;
 
   static int
@@ -1208,7 +1220,7 @@
   }
   if (length < 16)
     return(MagickFalse);
-  id=(ssize_t) ReadPropertyShort(LSBEndian,exif);
+  id=(ssize_t) ((int) ReadPropertyShort(LSBEndian,exif));
   endian=LSBEndian;
   if (id == 0x4949)
     endian=LSBEndian;
@@ -1223,7 +1235,7 @@
     This the offset to the first IFD.
   */
   offset=(ssize_t) ((int) ReadPropertyLong(endian,exif+4));
-  if ((size_t) offset >= length)
+  if ((offset < 0) || (size_t) offset >= length)
     return(MagickFalse);
   /*
     Set the pointer to the first IFD and follow it were it leads.
@@ -1233,6 +1245,8 @@
   level=0;
   entry=0;
   tag_offset=0;
+  exif_resources=NewSplayTree((int (*)(const void *,const void *)) NULL,
+    (void *(*)(void *)) NULL,(void *(*)(void *)) NULL);
   do
   {
     /*
@@ -1248,7 +1262,7 @@
     /*
       Determine how many entries there are in the current IFD.
     */
-    number_entries=ReadPropertyShort(endian,directory);
+    number_entries=(size_t) ((int) ReadPropertyShort(endian,directory));
     for ( ; entry < number_entries; entry++)
     {
       register unsigned char
@@ -1262,9 +1276,12 @@
       ssize_t
         components;
 
-      q=(unsigned char *) (directory+2+(12*entry));
-      tag_value=(ssize_t) (ReadPropertyShort(endian,q)+tag_offset);
-      format=(size_t) ReadPropertyShort(endian,q+2);
+      q=(unsigned char *) (directory+(12*entry)+2);
+      if (GetValueFromSplayTree(exif_resources,q) == q)
+        break;
+      (void) AddValueToSplayTree(exif_resources,q,q);
+      tag_value=(ssize_t) ((int) ReadPropertyShort(endian,q)+tag_offset);
+      format=(size_t) ((int) ReadPropertyShort(endian,q+2));
       if (format >= (sizeof(tag_bytes)/sizeof(*tag_bytes)))
         break;
       components=(ssize_t) ((int) ReadPropertyLong(endian,q+4));
@@ -1280,6 +1297,8 @@
             The directory entry contains an offset.
           */
           offset=(ssize_t) ((int) ReadPropertyLong(endian,q+8));
+          if ((offset+number_bytes) < offset)
+            continue;  /* prevent overflow */
           if ((size_t) (offset+number_bytes) > length)
             continue;
           p=(unsigned char *) (exif+offset);
@@ -1316,7 +1335,7 @@
             case EXIF_FMT_ULONG:
             {
               EXIFMultipleValues(4,"%.20g",(double)
-                ReadPropertyLong(endian,p1));
+                ((int) ReadPropertyLong(endian,p1)));
               break;
             }
             case EXIF_FMT_SLONG:
@@ -1328,8 +1347,8 @@
             case EXIF_FMT_URATIONAL:
             {
               EXIFMultipleFractions(8,"%.20g/%.20g",(double)
-                ReadPropertyLong(endian,p1),(double)
-                ReadPropertyLong(endian,p1+4));
+                ((int) ReadPropertyLong(endian,p1)),(double)
+                ((int) ReadPropertyLong(endian,p1+4)));
               break;
             }
             case EXIF_FMT_SRATIONAL:
@@ -1430,19 +1449,19 @@
             }
         }
         if ((tag_value == TAG_EXIF_OFFSET) ||
-            (tag_value == TAG_INTEROP_OFFSET) ||
-            (tag_value == TAG_GPS_OFFSET))
+            (tag_value == TAG_INTEROP_OFFSET) || (tag_value == TAG_GPS_OFFSET))
           {
-            size_t
+            ssize_t
               offset;
 
-            offset=(size_t) ReadPropertyLong(endian,p);
-            if ((offset < length) && (level < (MaxDirectoryStack-2)))
+            offset=(ssize_t) ((int) ReadPropertyLong(endian,p));
+            if (((size_t) offset < length) && (level < (MaxDirectoryStack-2)))
               {
-                size_t
+                ssize_t
                   tag_offset1;
 
-                tag_offset1=(tag_value == TAG_GPS_OFFSET) ? 0x10000UL : 0UL;
+                tag_offset1=(ssize_t) ((tag_value == TAG_GPS_OFFSET) ? 0x10000 :
+                  0);
                 directory_stack[level].directory=directory;
                 entry++;
                 directory_stack[level].entry=entry;
@@ -1454,9 +1473,9 @@
                 level++;
                 if ((directory+2+(12*number_entries)) > (exif+length))
                   break;
-                offset=(size_t) ReadPropertyLong(endian,directory+2+(12*
-                  number_entries));
-                if ((offset != 0) && (offset < length) &&
+                offset=(ssize_t) ((int) ReadPropertyLong(endian,directory+2+(12*
+                  number_entries)));
+                if ((offset != 0) && ((size_t) offset < length) &&
                     (level < (MaxDirectoryStack-2)))
                   {
                     directory_stack[level].directory=exif+offset;
@@ -1469,6 +1488,7 @@
           }
     }
   } while (level > 0);
+  exif_resources=DestroySplayTree(exif_resources);
   return(status);
 }
 
@@ -1609,7 +1629,7 @@
   in_subpath=MagickFalse;
   while (length > 0)
   {
-    selector=(ssize_t) ReadPropertyMSBShort(&blob,&length);
+    selector=(ssize_t) ((int) ReadPropertyMSBShort(&blob,&length));
     switch (selector)
     {
       case 0:
@@ -1618,15 +1638,15 @@
         if (knot_count != 0)
           {
             blob+=24;
-            length-=24;
+            length-=MagickMin(24,(ssize_t) length);
             break;
           }
         /*
           Expected subpath length record.
         */
-        knot_count=(ssize_t) ReadPropertyMSBShort(&blob,&length);
+        knot_count=(ssize_t) ((int) ReadPropertyMSBShort(&blob,&length));
         blob+=22;
-        length-=22;
+        length-=MagickMin(22,(ssize_t) length);
         break;
       }
       case 1:
@@ -1640,7 +1660,7 @@
               Unexpected subpath knot
             */
             blob+=24;
-            length-=24;
+            length-=MagickMin(24,(ssize_t) length);
             break;
           }
         /*
@@ -1652,8 +1672,8 @@
             xx,
             yy;
 
-          yy=ReadPropertyMSBLong(&blob,&length);
-          xx=ReadPropertyMSBLong(&blob,&length);
+          yy=(size_t) ((int) ReadPropertyMSBLong(&blob,&length));
+          xx=(size_t) ((int) ReadPropertyMSBLong(&blob,&length));
           x=(ssize_t) xx;
           if (xx > 2147483647)
             x=(ssize_t) xx-4294967295U-1;
@@ -1741,7 +1761,7 @@
       default:
       {
         blob+=24;
-        length-=24;
+        length-=MagickMin(24,(ssize_t) length);
         break;
       }
     }
@@ -1806,7 +1826,7 @@
   in_subpath=MagickFalse;
   while (length != 0)
   {
-    selector=(ssize_t) ReadPropertyMSBShort(&blob,&length);
+    selector=(ssize_t) ((int) ReadPropertyMSBShort(&blob,&length));
     switch (selector)
     {
       case 0:
@@ -1815,15 +1835,15 @@
         if (knot_count != 0)
           {
             blob+=24;
-            length-=24;
+            length-=MagickMin(24,(ssize_t) length);
             break;
           }
         /*
           Expected subpath length record.
         */
-        knot_count=(ssize_t) ReadPropertyMSBShort(&blob,&length);
+        knot_count=(ssize_t) ((int) ReadPropertyMSBShort(&blob,&length));
         blob+=22;
-        length-=22;
+        length-=MagickMin(22,(ssize_t) length);
         break;
       }
       case 1:
@@ -1837,7 +1857,7 @@
               Unexpected subpath knot.
             */
             blob+=24;
-            length-=24;
+            length-=MagickMin(24,(ssize_t) length);
             break;
           }
         /*
@@ -1912,7 +1932,7 @@
       default:
       {
         blob+=24;
-        length-=24;
+        length-=MagickMin(24,(ssize_t) length);
         break;
       }
     }
--- ImageMagick-6.7.4-10/magick/profile.c       2011-12-23 12:03:02.000000000 -0500
+++ ImageMagick-6.7.5-8/magick/profile.c        2012-02-29 10:12:27.092282962 -0500
@@ -6620,17 +6620,18 @@
   EndianType
     endian;
 
-  int
-    offset;
-
   size_t
     entry,
     length,
     number_entries;
 
+  SplayTreeInfo
+    *exif_resources;
+
   ssize_t
     id,
-    level;
+    level,
+    offset;
 
   static int
     format_bytes[] = {0, 1, 1, 2, 4, 8, 1, 1, 2, 4, 8, 4, 8};
@@ -6682,12 +6683,14 @@
   /*
     This the offset to the first IFD.
   */
-  offset=(int) ReadProfileLong(endian,exif+4);
-  if ((size_t) offset >= length)
+  offset=(ssize_t) ((int) ReadProfileLong(endian,exif+4));
+  if ((offset < 0) || ((size_t) offset >= length))
     return(MagickFalse);
   directory=exif+offset;
   level=0;
   entry=0;
+  exif_resources=NewSplayTree((int (*)(const void *,const void *)) NULL,
+    (void *(*)(void *)) NULL,(void *(*)(void *)) NULL);
   do
   {
     if (level > 0)
@@ -6717,6 +6720,9 @@
         tag_value;
 
       q=(unsigned char *) (directory+2+(12*entry));
+      if (GetValueFromSplayTree(exif_resources,q) == q)
+        break;
+      (void) AddValueToSplayTree(exif_resources,q,q);
       tag_value=(ssize_t) ReadProfileShort(endian,q);
       format=(ssize_t) ReadProfileShort(endian,q+2);
       if ((format-1) >= EXIF_NUM_FORMATS)
@@ -6727,13 +6733,15 @@
         p=q+8;
       else
         {
-          int
+          ssize_t
             offset;
 
           /*
             The directory entry contains an offset.
           */
-          offset=(int) ReadProfileLong(endian,q+8);
+          offset=(ssize_t) ((int) ReadProfileLong(endian,q+8));
+          if ((offset+number_bytes) < offset)
+            continue;  /* prevent overflow */
           if ((size_t) (offset+number_bytes) > length)
             continue;
           p=(unsigned char *) (exif+offset);
@@ -6742,28 +6750,25 @@
       {
         case 0x011a:
         {
-          (void) WriteProfileLong(endian,(size_t)
-            (image->x_resolution+0.5),p);
+          (void) WriteProfileLong(endian,(size_t) (image->x_resolution+0.5),p);
           (void) WriteProfileLong(endian,1UL,p+4);
           break;
         }
         case 0x011b:
         {
-          (void) WriteProfileLong(endian,(size_t)
-            (image->y_resolution+0.5),p);
+          (void) WriteProfileLong(endian,(size_t) (image->y_resolution+0.5),p);
           (void) WriteProfileLong(endian,1UL,p+4);
           break;
         }
         case 0x0112:
         {
-          (void) WriteProfileShort(endian,(unsigned short)
-            image->orientation,p);
+          (void) WriteProfileShort(endian,(unsigned short) image->orientation,
+            p);
           break;
         }
         case 0x0128:
         {
-          (void) WriteProfileShort(endian,(unsigned short)
-            (image->units+1),p);
+          (void) WriteProfileShort(endian,(unsigned short) (image->units+1),p);
           break;
         }
         default:
@@ -6771,11 +6776,11 @@
       }
       if ((tag_value == TAG_EXIF_OFFSET) || (tag_value == TAG_INTEROP_OFFSET))
         {
-          size_t
+          ssize_t
             offset;
 
-          offset=(size_t) ReadProfileLong(endian,p);
-          if ((offset < length) && (level < (MaxDirectoryStack-2)))
+          offset=(ssize_t) ((int) ReadProfileLong(endian,p));
+          if (((size_t) offset < length) && (level < (MaxDirectoryStack-2)))
             {
               directory_stack[level].directory=directory;
               entry++;
@@ -6786,9 +6791,9 @@
               level++;
               if ((directory+2+(12*number_entries)) > (exif+length))
                 break;
-              offset=(size_t) ReadProfileLong(endian,directory+2+(12*
-                number_entries));
-              if ((offset != 0) && (offset < length) &&
+              offset=(ssize_t) ((int) ReadProfileLong(endian,directory+2+(12*
+                number_entries)));
+              if ((offset != 0) && ((size_t) offset < length) &&
                   (level < (MaxDirectoryStack-2)))
                 {
                   directory_stack[level].directory=exif+offset;
@@ -6800,5 +6805,6 @@
         }
     }
   } while (level > 0);
+  exif_resources=DestroySplayTree(exif_resources);
   return(MagickTrue);
 }