imagemagick CVE-2016-4562, CVE-2016-4563, CVE-2016-4564

[Brian May <bam@debian.org>]

CVE-2016-4562

The DrawDashPolygon function in MagickCore/draw.c in ImageMagick before
6.9.4-0 and 7.x before 7.0.1-2 mishandles calculations of certain
vertices integer data, which allows remote attackers to cause a denial
of service (buffer overflow and application crash) or possibly have
unspecified other impact via a crafted file.

CVE-2016-4563

The TraceStrokePolygon function in MagickCore/draw.c in ImageMagick
before 6.9.4-0 and 7.x before 7.0.1-2 mishandles the relationship
between the BezierQuantum value and certain strokes data, which allows
remote attackers to cause a denial of service (buffer overflow and
application crash) or possibly have unspecified other impact via a
crafted file.

CVE-2016-4564

The DrawImage function in MagickCore/draw.c in ImageMagick before
6.9.4-0 and 7.x before 7.0.1-2 makes an incorrect function call in
attempting to locate the next token, which allows remote attackers to
cause a denial of service (buffer overflow and application crash) or
possibly have unspecified other impact via a crafted file.

In security tracker, all of these link to the same commit:

https://github.com/ImageMagick/ImageMagick/commit/726812fa2fa7ce16bcf58f6e115f65427a1c0950
Prevent buffer overflow in magick/draw.c


This commit appears to have a number of changes that aren't related to
these CVEs, that I can tell:

* A number of "alpha=(Quantum) TransparentAlpha" changed to
  "alpha=(MagickRealType) TransparentAlpha"

  I believe thiese a defined as:
  typedef long double MagickRealType;                                              
  typedef double Quantum;

* "number_vertices" to "(ssize_t) number_vertices" in for loop - might
  be significant, but is the case already in wheezy version.

* "GetNextToken(q,&q,extent,keyword);" -->
  "GetNextToken(q,&q,MagickPathExtent,keyword);" wheezy doens't have
  this parameter. Not sure if that is good or bad.

* "weight=StringToUnsignedLong(token)" --> "weight=(ssize_t)
  StringToUnsignedLong(token)"

* Added newline character

* "weight=StringToUnsignedLong(option);" --> "weight=(ssize_t)
  StringToUnsignedLong(option);"


Significant changes to TraceStrokePolygon function:

@@ -6021,17 +6021,29 @@
       }
     if (q >= (ssize_t) (max_strokes-6*BezierQuantum-360))
       {
-         max_strokes+=6*BezierQuantum+360;
-         path_p=(PointInfo *) ResizeQuantumMemory(path_p,(size_t) max_strokes,
-           sizeof(*path_p));
-         path_q=(PointInfo *) ResizeQuantumMemory(path_q,(size_t) max_strokes,
-           sizeof(*path_q));
-         if ((path_p == (PointInfo *) NULL) || (path_q == (PointInfo *) NULL))
-           {
-             polygon_primitive=(PrimitiveInfo *)
-               RelinquishMagickMemory(polygon_primitive);
-             return((PrimitiveInfo *) NULL);
-           }
+        if (~max_strokes < (6*BezierQuantum+360))
+          {
+            path_p=(PointInfo *) RelinquishMagickMemory(path_p);
+            path_q=(PointInfo *) RelinquishMagickMemory(path_q);
+          }
+        else
+          {
+            max_strokes+=6*BezierQuantum+360;
+            path_p=(PointInfo *) ResizeQuantumMemory(path_p,max_strokes,
+              sizeof(*path_p));
+            path_q=(PointInfo *) ResizeQuantumMemory(path_q,max_strokes,
+              sizeof(*path_q));
+          }
+        if ((path_p == (PointInfo *) NULL) || (path_q == (PointInfo *) NULL))
+          {
+            if (path_p != (PointInfo *) NULL)
+              path_p=(PointInfo *) RelinquishMagickMemory(path_p);
+            if (path_q != (PointInfo *) NULL)
+              path_q=(PointInfo *) RelinquishMagickMemory(path_q);
+            polygon_primitive=(PrimitiveInfo *)
+              RelinquishMagickMemory(polygon_primitive);
+            return((PrimitiveInfo *) NULL);
+          }
       }
     dot_product=dx.q*dy.p-dx.p*dy.q;
     if (dot_product <= 0.0)


Am wondering if maybe only this last part is required - it merges
cleanly too. Although not really entirely sure how this one function can
fit all CVEs. Possibly this patch only fixes CVE-2016-4563?

Anyway, out of time now, just wanted to summarize the situation so I
don't forget...
-- 
Brian May <bam@debian.org>