A buffer overflow flaw affecting ImageMagick versions prior to 6.8.8-5 when handling PSD images was reported: http://secunia.com/advisories/56844/ Diffing ImageMagick-6.8.7/coders/psd.c and ImageMagick-6.8.8/coders/psd.c, it looks like the flaw may be FormatLocaleString() writing the amount of 6 long integers (approximately 48 bytes) into a buffer (layer_name) that is only 4 bytes: "" @@ -1224,7 +1224,7 @@ Allocate layered image. */ layer_info[i].image=CloneImage(image,layer_info[i].page.width, - layer_info[i].page.height == ~0U ? 1 : layer_info[i].page.height, + layer_info[i].page.height == ~0UL ? 1 : layer_info[i].page.height, MagickFalse,&image->exception); if (layer_info[i].image == (Image *) NULL) { @@ -2112,9 +2112,6 @@ StringInfo *bim_profile; - unsigned char - layer_name[4]; - /* Open image file. */ @@ -2372,12 +2369,15 @@ property=(const char *) GetImageProperty(next_image,"label"); if (property == (const char *) NULL) { + char + layer_name[MaxTextExtent]; + (void) WriteBlobMSBLong(image,16); (void) WriteBlobMSBLong(image,0); (void) WriteBlobMSBLong(image,0); - (void) FormatLocaleString((char *) layer_name,MaxTextExtent, - "L%06ld",(long) layer_count++); - WritePascalString( image, (char*)layer_name, 4 ); + (void) FormatLocaleString(layer_name,MaxTextExtent,"L%06ld",(long) + layer_count++); + WritePascalString(image,layer_name,4); } else { "" CVE request: http://www.openwall.com/lists/oss-security/2014/02/12/2
(In reply to Murray McAllister from comment #0) > Diffing ImageMagick-6.8.7/coders/psd.c and ImageMagick-6.8.8/coders/psd.c, > it looks like the flaw may be FormatLocaleString() writing the amount of 6 > long integers (approximately 48 bytes) into a buffer (layer_name) that is > only 4 bytes: It's writing less than those mentioned 48 bytes. It's writing a long integer in a way that at least 6 digits are always printed, prefixed with zeros if needed (e.g. L000001, L000123, or L1234567). Adding space for 'L' and terminating '\0', that implies there is at least 4 byte buffer overflow whenever that code is reached. Depending on actual memory layout, this may only clobber other local variables in WritePSDImage(). The maximum overflow is architecture dependent. It seems the maximum number of digits is written if layer_count is LONG_MIN, resulting in layer_name being L-2147483648 (32bit) or L-9223372036854775808 (64bit), yielding the maximum of 18 byte overflow. As the value printed is layer counter, it may not be possible to provide input image with that many layers. Practical overflow may not be much longer than the overflow that happens in the non-malicious case mentioned above. Older ImageMagick versions have this format string instead "L%02ld", so it does not always overflow, it only starts to overflow with 100 layers. Upstream commit: http://trac.imagemagick.org/changeset/13736
There is this suspicious commit: http://trac.imagemagick.org/changeset/14801 Even though no obvious link to RLE decoding mentioned in the Secunia advisory.
This was assigned CVE-2014-1947: http://www.openwall.com/lists/oss-security/2014/02/12/13
As noted in IRC, I believe what Murray found (second and third hunk) is a different issue. Tomas' commit identifies the patch mentioned in the Secunia advisory. The fix is in the DecodePSDPixels function and looks RLE-related. RLE is a simple compression using pairs of bytes, first the number of elements, then the value for an element. A pair of [3, a] would produce "aaa" on decompression. RLE can overflow at the pre-allocated target buffer if no size check for the uncompressed length of the current pair is performed. A simple example is allocating a buffer of size 4, then having a encoding pair of [3, a] and [256, b]. After writing 'aaa' the buffer still has room, so (current_index < buffer_length) is true and we decompress the next pair, writing 255 bytes beyond the buffer boundary. That's exactly what happens here. In this code: compact_pixels is the set of RLE pairs, number_pixels is the length of the buffer. i is the current pixel index, pixel points to the current characters. Buffer overflow can happen when (i >= number_pixels - 256). The patch fixed this by capping the length of the decompressed set to the remainder. The other size checks are ok, so an exploit would only be able use one packet to write past the buffer boundary, and the byte values aren't completely arbitrary but somewhat limited by the format. How to exploit that requires more creativity than I have. Max overflow is 256 bytes.
(In reply to Tomas Hoger from comment #2) > There is this suspicious commit: > http://trac.imagemagick.org/changeset/14801 > > Even though no obvious link to RLE decoding mentioned in the Secunia > advisory. This commit was assigned CVE-2014-1958 as per: http://seclists.org/oss-sec/2014/q1/343 This one is pretty messy. It looks like there may be further discussion about this but we probably want to file a second bug for this CVE once it gets detangled a bit.
quoting from MITRE in http://www.openwall.com/lists/oss-security/2014/02/19/13 "" The clarified meaning of CVE-2014-1947 is now the vulnerability in older ImageMagick versions (such as 6.5.4) that use the "L%02ld" string. The root cause here is that the code did not cover the case of more than 99 layers, which is apparently allowable but relatively uncommon. This has a resultant buffer overflow, e.g, L99\0 is safe but L100\0 is unsafe. When the overflow occurs, it can be described as "1 or more bytes too many." A new ID of CVE-2014-2030 is now assigned for the vulnerability in newer ImageMagick versions that use the "L%06ld" string. The root cause here is that the code did not recognize the relationship between the 8 (or more) characters in "L%06ld" and the actual buffer size. This has a resultant buffer overflow of "4 or more bytes too many." ""
Created ImageMagick tracking bugs for this issue: Affects: fedora-all [bug 1067278]
(In reply to Vincent Danen from comment #5) > (In reply to Tomas Hoger from comment #2) > > There is this suspicious commit: > > http://trac.imagemagick.org/changeset/14801 > > > > Even though no obvious link to RLE decoding mentioned in the Secunia > > advisory. > > This commit was assigned CVE-2014-1958 as per: > > http://seclists.org/oss-sec/2014/q1/343 > > This one is pretty messy. It looks like there may be further discussion > about this but we probably want to file a second bug for this CVE once it > gets detangled a bit. Filed https://bugzilla.redhat.com/show_bug.cgi?id=1067276
Citing upstream author from upstream bug report ( http://www.imagemagick.org/discourse-server/viewtopic.php?f=3&t=25128 ): "However, we're not sure about CVE-2014-2030. They say "new ID of CVE-2014-2030 is now assigned for the vulnerability in newer ImageMagick versions that use the "L%06ld" string. The root cause here is that the code did not recognize the relationship between the 8 (or more) characters in "L%06ld" and the actual buffer size." Since the layer_name buffer is now 4096 characters, whereas the maximum label size is 21 characters (i.e. L-9223372036854775808 @ 64-bit). So we're not sure why 2030 is filed as a vulnerability." Could someone answer it?
Both CVEs here are for ImageMagick versions before changeset 13736. CVE-2014-1947 is for versions that use format string "L%02ld". The layer_name there is layer_name[4] so overflow only occurs if there are more than 99 layers. Format was changed from "L%02ld" to "L%06ld" in r1448: http://trac.imagemagick.org/changeset/1448 CVE-2014-2030 is applicable to ImageMagick versions after the above r1448, where layer_name is still layer_name[4], but overflow occurs even with a single (unnamed) layer. Fixed in r13736 that made layer_name large enough: http://trac.imagemagick.org/changeset/13736 r13736 is fix for both CVEs, but anyone shipping pre-r1448 IM versions should use CVE-2014-1947 in released updates instead of CVE-2014-2030.
So, we can then just update it to last version and constatate what all three CVE fixed? Right?
CVE-2014-1947 also affects GraphicMagic. Although there are small differences, the root cause is identical and overall the code is similar enough to certain ImageMagick versions to reuse CVE-2014-1947. In GraphicsMagick, the buffer overflow is caught by the fortify source buffer overflow protection.
Created GraphicsMagick tracking bugs for this issue: Affects: fedora-all [bug 1083082] Affects: epel-5 [bug 1083083] Affects: epel-6 [bug 1083084]
(In reply to Pavel Alexeev (aka Pahan-Hubbitus) from comment #11) > So, we can then just update it to last version and constatate what all three > CVE fixed? Right? Yes, the latest version should fix all issues.
As CVE-2014-1947 and CVE-2014-2030 affect different versions, tracking of CVE-2014-2030 was split to a separate bug 1083477.
ImageMagick-6.8.6.3-4.fc20 has been pushed to the Fedora 20 stable repository. If problems still persist, please make note of it in this bug report.
Here's what I'm using for GraphicsMagick, adapting a similar fix (I think): http://pkgs.fedoraproject.org/cgit/GraphicsMagick.git/tree/GraphicsMagick-1.3.20-CVE-2014-1947.patch diff -up GraphicsMagick-1.3.20/coders/psd.c.CVE-2014-1947 GraphicsMagick-1.3.20/coders/psd.c --- GraphicsMagick-1.3.20/coders/psd.c.CVE-2014-1947 2014-08-16 15:33:23.000000000 -0500 +++ GraphicsMagick-1.3.20/coders/psd.c 2014-08-20 07:30:08.767862041 -0500 @@ -1719,8 +1719,7 @@ static unsigned int WritePSDImage(const i; unsigned char - *pixels, - layer_name[4]; + *pixels; unsigned int packet_size, @@ -1944,8 +1943,9 @@ static unsigned int WritePSDImage(const (void) WriteBlob(image, 3, &layer_name[1]); */ } else { - (void) sprintf((char *) layer_name, "L%02d", layer_count++ ); - WritePascalString( image, (char*)layer_name, 4 ); + char layer_name[4]; + (void) sprintf(layer_name, "L%02d", layer_count++ ); + WritePascalString( image, layer_name, 4 ); } tmp_image = tmp_image->next; };
(In reply to Rex Dieter from comment #20) > Here's what I'm using for GraphicsMagick What is the change supposed to do? It does not increase the size of layer_name, so more than 99 layers should still lead to overflow as described in comment 10.
Oh, indeed, I'd naively thought the main fix was adjusting to a char array (and avoiding the cast). Does this look better? --- GraphicsMagick-1.3.20/coders/psd.c.CVE-2014-1947 2014-08-16 15:33:23.000000000 -0500 +++ GraphicsMagick-1.3.20/coders/psd.c 2014-08-20 07:30:08.767862041 -0500 @@ -1719,8 +1719,7 @@ static unsigned int WritePSDImage(const i; unsigned char - *pixels, - layer_name[4]; + *pixels; unsigned int packet_size, @@ -1944,8 +1943,9 @@ static unsigned int WritePSDImage(const (void) WriteBlob(image, 3, &layer_name[1]); */ } else { - (void) sprintf((char *) layer_name, "L%02d", layer_count++ ); - WritePascalString( image, (char*)layer_name, 4 ); + char layer_name[MaxTextExtent]; + (void) sprintf(layer_name, "L%06ld", layer_count++ ); + WritePascalString( image, layer_name, 4 ); } tmp_image = tmp_image->next; };
(In reply to Rex Dieter from comment #22) > Oh, indeed, I'd naively thought the main fix was adjusting to a char array > (and avoiding the cast). Does this look better? I haven't looked at GM specifically. In IM, MaxTextExtent is large enough to hold layer name, regardless of whether L%02d or L%06ld format is used.
OK, not being intimately familiar with this code either, I'm going back to the original L%02d format variant. It would appear GM upstream may not be aware of this... I'll try to make some time soon to poke them about it too.
GraphicsMagick-1.3.20-1.fc20 has been pushed to the Fedora 20 stable repository. If problems still persist, please make note of it in this bug report.
This looks like a match, http://sourceforge.net/p/graphicsmagick/bugs/234/ added a comment there, and they responded to me already with some questions.
I just pushed GraphicsMagick changeset 14139:a083f9eeef1d with a patch which is mostly similar to Rex Dieter's patch. It seems doubtful that this issue is covered by any existing GraphicsMagick bug report. The PSD code in GraphicsMagick has many unresolved issues.
GraphicsMagick-1.3.20-3.fc20 has been pushed to the Fedora 20 stable repository. If problems still persist, please make note of it in this bug report.
GraphicsMagick-1.3.20-3.fc19 has been pushed to the Fedora 19 stable repository. If problems still persist, please make note of it in this bug report.
GraphicsMagick-1.3.20-3.el5 has been pushed to the Fedora EPEL 5 stable repository. If problems still persist, please make note of it in this bug report.
GraphicsMagick-1.3.20-3.el6 has been pushed to the Fedora EPEL 6 stable repository. If problems still persist, please make note of it in this bug report.
GraphicsMagick-1.3.20-3.fc21 has been pushed to the Fedora 21 stable repository. If problems still persist, please make note of it in this bug report.