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

[Brian May <bam@debian.org>]

Just guessing a bit here:

Brian May <bam@debian.org> writes:

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

DrawDashPolygon had the following change:

- for (i=1; (i < number_vertices) && (length >= 0.0); i++)
+ for (i=1; (i < (ssize_t) number_vertices) && (length >= 0.0); i++)

Hoewever the wheezy version has:

for (i=1; i < (ssize_t) number_vertices; i++)

So *if* this is a security issue, I think the wheezy version is not
vulnerable.

Alternatively, DrawDashPolygon uses DrawStrokePolygon a lot, which in
turn uses TraceStrokePolygon, which gets on to the next CVE:


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

This looks pretty simple and obvious - we add extra checks to ensure we
resize the buffer to a sane size.


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

The upstream code changes:

- GetNextToken(q,&q,extent,keyword);
+ GetNextToken(q,&q,MagickPathExtent,keyword);

The wheezy version has:

GetMagickToken(q,&q,keyword);

As such we are missing the parameter that is changed. This parameter has
the documentation:

extent: maximum extent of the token.

Possibly the fact that the wheezy version doesn't have this parameter is
a security issue in itself, however fixing this would be rather
invasive.

I am going to continue looking at the differences between GetNextToken
and GetMagickToken later.


> * A number of "alpha=(Quantum) TransparentAlpha" changed to
>   "alpha=(MagickRealType) TransparentAlpha"
>
>   I believe thiese a defined as:
>   typedef long double MagickRealType;                                              
>   typedef double Quantum;

I am ignoring these, as while this might result in loss of precision I
can't see how these issues could be security issues.


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

Can't see these being security issues either.  StringToUnsignedLong
returns an unsigned long. weight is size_t. My C is rusty, however I
seem to recall copying an unsigned long to a size_t is the same
regardless of whether you have an explicit cast or not.


> 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?

Am inclined to:

1. Patch TraceStrokePolygon.
2. Mark CVE-2016-4563 as fixed in wheezy (but this does not mean it is
fixed in Jessie or above - probably need to check the Jessie version first).
3. Mark CVE-2016-4562 as not vulnerable.
4. Leave CVE-2016-4564 as vulnerable.
-- 
Brian May <bam@debian.org>