Just guessing a bit here: Brian May <[email protected]> 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 <[email protected]>
