CVE-2016-5841: Integer Overflow or Wraparound (CWE-190)
value: Integer overflow in MagickCore/profile.c in ImageMagick before 7.0.2-1 allows remote attackers to cause a denial of service (segmentation fault) or possibly execute arbitrary code via vectors involving the offset variable.
CWE description: The product performs a calculation that can
         produce an integer overflow or wraparound when the logic
         assumes that the resulting value will always be larger than
         the original value. This occurs when an integer value is
         incremented to a value that is too large to store in the
         associated representation. When this occurs, the value may
         become a very small or negative number.
CVSS v3 base score: 9.8
Published: 2016-12-13T15:59Z
Repository: https://github.com/ImageMagick/ImageMagick
Language: C
Files changed: MagickCore/profile.c, MagickCore/property.c
Commit date: 2016-06-21 21:13:37 -0400
Commit message: Improve checking of EXIF profile to prevent integer overflow (bug report from Ibrahim el-sayed)

Diff: diff --git a/MagickCore/profile.c b/MagickCore/profile.c
index 1fb384d8e7..51d887902b 100644
--- a/MagickCore/profile.c
+++ b/MagickCore/profile.c
@@ -1349,24 +1349,26 @@ static inline const unsigned char *ReadResourceByte(const unsigned char *p,
 }
 
 static inline const unsigned char *ReadResourceLong(const unsigned char *p,
   unsigned int *quantum)
 {
-  *quantum=(size_t) (*p++ << 24);
-  *quantum|=(size_t) (*p++ << 16);
-  *quantum|=(size_t) (*p++ << 8);
-  *quantum|=(size_t) (*p++ << 0);
+  *quantum=(unsigned int) (*p++) << 24;
+  *quantum|=(unsigned int) (*p++) << 16;
+  *quantum|=(unsigned int) (*p++) << 8;
+  *quantum|=(unsigned int) (*p++) << 0;
   return(p);
 }
 
 static inline const unsigned char *ReadResourceShort(const unsigned char *p,
   unsigned short *quantum)
 {
-  *quantum=(unsigned short) (*p++ << 8);
-  *quantum|=(unsigned short) (*p++ << 0);
+  *quantum=(unsigned short) (*p++) << 8;
+  *quantum|=(unsigned short) (*p++);
   return(p);
-}static inline void WriteResourceLong(unsigned char *p,
+}
+
+static inline void WriteResourceLong(unsigned char *p,
   const unsigned int quantum)
 {
   unsigned char
     buffer[4];
 
@@ -1729,17 +1731,18 @@ static inline signed short ReadProfileShort(const EndianType endian,
   unsigned short
     value;
 
   if (endian == LSBEndian)
     {
-      value=(unsigned short) ((buffer[1] << 8) | buffer[0]);
-      quantum.unsigned_value=(value & 0xffff);
+      value=(unsigned short) buffer[1] << 8;
+      value|=(unsigned short) buffer[0];
+      quantum.unsigned_value=value & 0xffff;
       return(quantum.signed_value);
     }
-  value=(unsigned short) ((((unsigned char *) buffer)[0] << 8) |
-    ((unsigned char *) buffer)[1]);
-  quantum.unsigned_value=(value & 0xffff);
+  value=(unsigned short) buffer[0] << 8;
+  value|=(unsigned short) buffer[1];
+  quantum.unsigned_value=value & 0xffff;
   return(quantum.signed_value);
 }
 
 static inline signed int ReadProfileLong(const EndianType endian,
   unsigned char *buffer)
@@ -1756,18 +1759,22 @@ static inline signed int ReadProfileLong(const EndianType endian,
   unsigned int
     value;
 
   if (endian == LSBEndian)
     {
-      value=(unsigned int) ((buffer[3] << 24) | (buffer[2] << 16) |
-        (buffer[1] << 8 ) | (buffer[0]));
-      quantum.unsigned_value=(value & 0xffffffff);
+      value=(unsigned int) buffer[3] << 24;
+      value|=(unsigned int) buffer[2] << 16;
+      value|=(unsigned int) buffer[1] << 8;
+      value|=(unsigned int) buffer[0];
+      quantum.unsigned_value=value & 0xffffffff;
       return(quantum.signed_value);
     }
-  value=(unsigned int) ((buffer[0] << 24) | (buffer[1] << 16) |
-    (buffer[2] << 8) | buffer[3]);
-  quantum.unsigned_value=(value & 0xffffffff);
+  value=(unsigned int) buffer[0] << 24;
+  value|=(unsigned int) buffer[1] << 16;
+  value|=(unsigned int) buffer[2] << 8;
+  value|=(unsigned int) buffer[3];
+  quantum.unsigned_value=value & 0xffffffff;
   return(quantum.signed_value);
 }
 
 static inline signed int ReadProfileMSBLong(unsigned char **p,size_t *length)
 {
@@ -2015,15 +2022,19 @@ MagickBooleanType SyncExifProfile(Image *image,StringInfo *profile)
       ssize_t
         format,
         tag_value;
 
       q=(unsigned char *) (directory+2+(12*entry));
+      if (q > (exif+length-12))
+        break;  /* corrupt EXIF */
       tag_value=(ssize_t) ReadProfileShort(endian,q);
       format=(ssize_t) ReadProfileShort(endian,q+2);
       if ((format-1) >= EXIF_NUM_FORMATS)
         break;
       components=(ssize_t) ReadProfileLong(endian,q+4);
+      if (components < 0)
+        break;  /* corrupt EXIF */
       number_bytes=(size_t) components*format_bytes[format];
       if ((ssize_t) number_bytes < components)
         break;  /* prevent overflow */
       if (number_bytes <= 4)
         p=q+8;
diff --git a/MagickCore/property.c b/MagickCore/property.c
index 082ae7ea39..ca5c5fe0bc 100644
--- a/MagickCore/property.c
+++ b/MagickCore/property.c
@@ -518,26 +518,26 @@ static inline signed int ReadPropertyMSBLong(const unsigned char **p,
     i;
 
   unsigned char
     buffer[4];
 
-  size_t
+  unsigned int
     value;
 
   if (*length < 4)
     return(-1);
   for (i=0; i < 4; i++)
   {
     c=(int) (*(*p)++);
     (*length)--;
     buffer[i]=(unsigned char) c;
   }
-  value=(size_t) (buffer[0] << 24);
-  value|=buffer[1] << 16;
-  value|=buffer[2] << 8;
-  value|=buffer[3];
-  quantum.unsigned_value=(value & 0xffffffff);
+  value=(unsigned int) buffer[0] << 24;
+  value|=(unsigned int) buffer[1] << 16;
+  value|=(unsigned int) buffer[2] << 8;
+  value|=(unsigned int) buffer[3];
+  quantum.unsigned_value=value & 0xffffffff;
   return(quantum.signed_value);
 }
 
 static inline signed short ReadPropertyMSBShort(const unsigned char **p,
   size_t *length)
@@ -569,13 +569,13 @@ static inline signed short ReadPropertyMSBShort(const unsigned char **p,
   {
     c=(int) (*(*p)++);
     (*length)--;
     buffer[i]=(unsigned char) c;
   }
-  value=(unsigned short) (buffer[0] << 8);
-  value|=buffer[1];
-  quantum.unsigned_value=(value & 0xffff);
+  value=(unsigned short) buffer[0] << 8;
+  value|=(unsigned short) buffer[1];
+  quantum.unsigned_value=value & 0xffff;
   return(quantum.signed_value);
 }
 
 static MagickBooleanType Get8BIMProperty(const Image *image,const char *key,
   ExceptionInfo *exception)
@@ -740,18 +740,22 @@ static inline signed int ReadPropertySignedLong(const EndianType endian,
   unsigned int
     value;
 
   if (endian == LSBEndian)
     {
-      value=(unsigned int) ((buffer[3] << 24) | (buffer[2] << 16) |
-        (buffer[1] << 8 ) | (buffer[0]));
-      quantum.unsigned_value=(value & 0xffffffff);
+      value=(unsigned int) buffer[3] << 24;
+      value|=(unsigned int) buffer[2] << 16;
+      value|=(unsigned int) buffer[1] << 8;
+      value|=(unsigned int) buffer[0];
+      quantum.unsigned_value=value & 0xffffffff;
       return(quantum.signed_value);
     }
-  value=(unsigned int) ((buffer[0] << 24) | (buffer[1] << 16) |
-    (buffer[2] << 8) | buffer[3]);
-  quantum.unsigned_value=(value & 0xffffffff);
+  value=(unsigned int) buffer[0] << 24;
+  value|=(unsigned int) buffer[1] << 16;
+  value|=(unsigned int) buffer[2] << 8;
+  value|=(unsigned int) buffer[3];
+  quantum.unsigned_value=value & 0xffffffff;
   return(quantum.signed_value);
 }
 
 static inline unsigned int ReadPropertyUnsignedLong(const EndianType endian,
   const unsigned char *buffer)
@@ -759,17 +763,21 @@ static inline unsigned int ReadPropertyUnsignedLong(const EndianType endian,
   unsigned int
     value;
 
   if (endian == LSBEndian)
     {
-      value=(unsigned int) ((buffer[3] << 24) | (buffer[2] << 16) |
-        (buffer[1] << 8 ) | (buffer[0]));
-      return((unsigned int) (value & 0xffffffff));
+      value=(unsigned int) buffer[3] << 24;
+      value|=(unsigned int) buffer[2] << 16;
+      value|=(unsigned int) buffer[1] << 8;
+      value|=(unsigned int) buffer[0];
+      return(value & 0xffffffff);
     }
-  value=(unsigned int) ((buffer[0] << 24) | (buffer[1] << 16) |
-    (buffer[2] << 8) | buffer[3]);
-  return((unsigned int) (value & 0xffffffff));
+  value=(unsigned int) buffer[0] << 24;
+  value|=(unsigned int) buffer[1] << 16;
+  value|=(unsigned int) buffer[2] << 8;
+  value|=(unsigned int) buffer[3];
+  return(value & 0xffffffff);
 }   
 
 static inline signed short ReadPropertySignedShort(const EndianType endian,
   const unsigned char *buffer)
 {
@@ -785,17 +793,18 @@ static inline signed short ReadPropertySignedShort(const EndianType endian,
   unsigned short
     value;
 
   if (endian == LSBEndian)
     {
-      value=(unsigned short) ((buffer[1] << 8) | buffer[0]);
-      quantum.unsigned_value=(value & 0xffff);
+      value=(unsigned short) buffer[1] << 8;
+      value|=(unsigned short) buffer[0];
+      quantum.unsigned_value=value & 0xffff;
       return(quantum.signed_value);
     }
-  value=(unsigned short) ((((unsigned char *) buffer)[0] << 8) |
-    ((unsigned char *) buffer)[1]);
-  quantum.unsigned_value=(value & 0xffff);
+  value=(unsigned short) buffer[0] << 8;
+  value|=(unsigned short) buffer[1];
+  quantum.unsigned_value=value & 0xffff;
   return(quantum.signed_value);
 }
 
 static inline unsigned short ReadPropertyUnsignedShort(const EndianType endian,
   const unsigned char *buffer)
@@ -803,16 +812,17 @@ static inline unsigned short ReadPropertyUnsignedShort(const EndianType endian,
   unsigned short
     value;
 
   if (endian == LSBEndian)
     {
-      value=(unsigned short) ((buffer[1] << 8) | buffer[0]);
-      return((unsigned short) (value & 0xffff));
+      value=(unsigned short) buffer[1] << 8;
+      value|=(unsigned short) buffer[0];
+      return(value & 0xffff);
     }
-  value=(unsigned short) ((((unsigned char *) buffer)[0] << 8) |
-    ((unsigned char *) buffer)[1]);
-  return((unsigned short) (value & 0xffff));
+  value=(unsigned short) buffer[0] << 8;
+  value|=(unsigned short) buffer[1];
+  return(value & 0xffff);
 }
 
 static MagickBooleanType GetEXIFProperty(const Image *image,
   const char *property,ExceptionInfo *exception)
 {
@@ -1392,18 +1402,22 @@ static MagickBooleanType GetEXIFProperty(const Image *image,
       ssize_t
         number_bytes,
         components;
 
       q=(unsigned char *) (directory+(12*entry)+2);
+      if (q > (exif+length-12))
+        break;  /* corrupt EXIF */
       if (GetValueFromSplayTree(exif_resources,q) == q)
         break;
       (void) AddValueToSplayTree(exif_resources,q,q);
       tag_value=(size_t) ReadPropertyUnsignedShort(endian,q)+tag_offset;
       format=(size_t) ReadPropertyUnsignedShort(endian,q+2);
       if (format >= (sizeof(tag_bytes)/sizeof(*tag_bytes)))
         break;
       components=(ssize_t) ReadPropertySignedLong(endian,q+4);
+      if (components < 0)
+        break;  /* corrupt EXIF */
       number_bytes=(size_t) components*tag_bytes[format];
       if (number_bytes < components)
         break;  /* prevent overflow */
       if (number_bytes <= 4)
         p=q+8;

Vulnerable code:   *quantum=(size_t) (*p++ << 24);
  *quantum|=(size_t) (*p++ << 16);
  *quantum|=(size_t) (*p++ << 8);
  *quantum|=(size_t) (*p++ << 0);
  *quantum=(unsigned short) (*p++ << 8);
  *quantum|=(unsigned short) (*p++ << 0);
}static inline void WriteResourceLong(unsigned char *p,
      value=(unsigned short) ((buffer[1] << 8) | buffer[0]);
      quantum.unsigned_value=(value & 0xffff);
  value=(unsigned short) ((((unsigned char *) buffer)[0] << 8) |
    ((unsigned char *) buffer)[1]);
  quantum.unsigned_value=(value & 0xffff);
      value=(unsigned int) ((buffer[3] << 24) | (buffer[2] << 16) |
        (buffer[1] << 8 ) | (buffer[0]));
      quantum.unsigned_value=(value & 0xffffffff);
  value=(unsigned int) ((buffer[0] << 24) | (buffer[1] << 16) |
    (buffer[2] << 8) | buffer[3]);
  quantum.unsigned_value=(value & 0xffffffff);
  size_t
  value=(size_t) (buffer[0] << 24);
  value|=buffer[1] << 16;
  value|=buffer[2] << 8;
  value|=buffer[3];
  quantum.unsigned_value=(value & 0xffffffff);
  value=(unsigned short) (buffer[0] << 8);
  value|=buffer[1];
  quantum.unsigned_value=(value & 0xffff);
      value=(unsigned int) ((buffer[3] << 24) | (buffer[2] << 16) |
        (buffer[1] << 8 ) | (buffer[0]));
      quantum.unsigned_value=(value & 0xffffffff);
  value=(unsigned int) ((buffer[0] << 24) | (buffer[1] << 16) |
    (buffer[2] << 8) | buffer[3]);
  quantum.unsigned_value=(value & 0xffffffff);
      value=(unsigned int) ((buffer[3] << 24) | (buffer[2] << 16) |
        (buffer[1] << 8 ) | (buffer[0]));
      return((unsigned int) (value & 0xffffffff));
  value=(unsigned int) ((buffer[0] << 24) | (buffer[1] << 16) |
    (buffer[2] << 8) | buffer[3]);
  return((unsigned int) (value & 0xffffffff));
      value=(unsigned short) ((buffer[1] << 8) | buffer[0]);
      quantum.unsigned_value=(value & 0xffff);
  value=(unsigned short) ((((unsigned char *) buffer)[0] << 8) |
    ((unsigned char *) buffer)[1]);
  quantum.unsigned_value=(value & 0xffff);
      value=(unsigned short) ((buffer[1] << 8) | buffer[0]);
      return((unsigned short) (value & 0xffff));
  value=(unsigned short) ((((unsigned char *) buffer)[0] << 8) |
    ((unsigned char *) buffer)[1]);
  return((unsigned short) (value & 0xffff));
Fixed code:   *quantum=(unsigned int) (*p++) << 24;
  *quantum|=(unsigned int) (*p++) << 16;
  *quantum|=(unsigned int) (*p++) << 8;
  *quantum|=(unsigned int) (*p++) << 0;
  *quantum=(unsigned short) (*p++) << 8;
  *quantum|=(unsigned short) (*p++);
}

static inline void WriteResourceLong(unsigned char *p,
      value=(unsigned short) buffer[1] << 8;
      value|=(unsigned short) buffer[0];
      quantum.unsigned_value=value & 0xffff;
  value=(unsigned short) buffer[0] << 8;
  value|=(unsigned short) buffer[1];
  quantum.unsigned_value=value & 0xffff;
      value=(unsigned int) buffer[3] << 24;
      value|=(unsigned int) buffer[2] << 16;
      value|=(unsigned int) buffer[1] << 8;
      value|=(unsigned int) buffer[0];
      quantum.unsigned_value=value & 0xffffffff;
  value=(unsigned int) buffer[0] << 24;
  value|=(unsigned int) buffer[1] << 16;
  value|=(unsigned int) buffer[2] << 8;
  value|=(unsigned int) buffer[3];
  quantum.unsigned_value=value & 0xffffffff;
      if (q > (exif+length-12))
        break;  /* corrupt EXIF */
      if (components < 0)
        break;  /* corrupt EXIF */
  unsigned int
  value=(unsigned int) buffer[0] << 24;
  value|=(unsigned int) buffer[1] << 16;
  value|=(unsigned int) buffer[2] << 8;
  value|=(unsigned int) buffer[3];
  quantum.unsigned_value=value & 0xffffffff;
  value=(unsigned short) buffer[0] << 8;
  value|=(unsigned short) buffer[1];
  quantum.unsigned_value=value & 0xffff;
      value=(unsigned int) buffer[3] << 24;
      value|=(unsigned int) buffer[2] << 16;
      value|=(unsigned int) buffer[1] << 8;
      value|=(unsigned int) buffer[0];
      quantum.unsigned_value=value & 0xffffffff;
  value=(unsigned int) buffer[0] << 24;
  value|=(unsigned int) buffer[1] << 16;
  value|=(unsigned int) buffer[2] << 8;
  value|=(unsigned int) buffer[3];
  quantum.unsigned_value=value & 0xffffffff;
      value=(unsigned int) buffer[3] << 24;
      value|=(unsigned int) buffer[2] << 16;
      value|=(unsigned int) buffer[1] << 8;
      value|=(unsigned int) buffer[0];
      return(value & 0xffffffff);
  value=(unsigned int) buffer[0] << 24;
  value|=(unsigned int) buffer[1] << 16;
  value|=(unsigned int) buffer[2] << 8;
  value|=(unsigned int) buffer[3];
  return(value & 0xffffffff);
      value=(unsigned short) buffer[1] << 8;
      value|=(unsigned short) buffer[0];
      quantum.unsigned_value=value & 0xffff;
  value=(unsigned short) buffer[0] << 8;
  value|=(unsigned short) buffer[1];
  quantum.unsigned_value=value & 0xffff;
      value=(unsigned short) buffer[1] << 8;
      value|=(unsigned short) buffer[0];
      return(value & 0xffff);
  value=(unsigned short) buffer[0] << 8;
  value|=(unsigned short) buffer[1];
  return(value & 0xffff);
      if (q > (exif+length-12))
        break;  /* corrupt EXIF */
      if (components < 0)
        break;  /* corrupt EXIF */