CVE-2016-5842: Out-of-bounds Read (CWE-125)
value: MagickCore/property.c in ImageMagick before 7.0.2-1 allows remote attackers to obtain sensitive memory information via vectors involving the q variable, which triggers an out-of-bounds read.
CWE description: The product reads data past the end, or before the beginning, of the intended buffer.
CVSS v3 base score: 7.5
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 */