Bug 1064098 - (CVE-2014-1947) CVE-2014-1947 ImageMagick: PSD writing layer name buffer overflow ("L%02ld")
CVE-2014-1947 ImageMagick: PSD writing layer name buffer overflow ("L%02ld")
Status: NEW
Product: Security Response
Classification: Other
Component: vulnerability (Show other bugs)
unspecified
All Linux
medium Severity medium
: ---
: ---
Assigned To: Red Hat Product Security
impact=moderate,public=20131114,repor...
: Security
Depends On: 1083082 1083083 1083084
Blocks: 1064101
  Show dependency treegraph
 
Reported: 2014-02-11 21:30 EST by Murray McAllister
Modified: 2015-07-29 13:53 EDT (History)
13 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed:
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description Murray McAllister 2014-02-11 21:30:11 EST
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
Comment 1 Tomas Hoger 2014-02-12 05:02:33 EST
(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
Comment 2 Tomas Hoger 2014-02-12 05:07:17 EST
There is this suspicious commit:
http://trac.imagemagick.org/changeset/14801

Even though no obvious link to RLE decoding mentioned in the Secunia advisory.
Comment 3 Vincent Danen 2014-02-12 12:44:13 EST
This was assigned CVE-2014-1947:

http://www.openwall.com/lists/oss-security/2014/02/12/13
Comment 4 Peter Hutterer 2014-02-12 23:08:27 EST
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.
Comment 5 Vincent Danen 2014-02-13 18:03:39 EST
(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.
Comment 6 Murray McAllister 2014-02-19 23:58:57 EST
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."
""
Comment 7 Murray McAllister 2014-02-20 00:24:26 EST
Created ImageMagick tracking bugs for this issue:

Affects: fedora-all [bug 1067278]
Comment 8 Murray McAllister 2014-02-20 00:28:21 EST
(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
Comment 9 Pavel Alexeev 2014-03-04 08:26:19 EST
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?
Comment 10 Tomas Hoger 2014-03-04 11:37:27 EST
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.
Comment 11 Pavel Alexeev 2014-03-07 03:58:46 EST
So, we can then just update it to last version and constatate what all three CVE fixed? Right?
Comment 13 Stefan Cornelius 2014-04-01 08:22:35 EDT
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.
Comment 15 Stefan Cornelius 2014-04-01 08:57:01 EDT
Created GraphicsMagick tracking bugs for this issue:

Affects: fedora-all [bug 1083082]
Affects: epel-5 [bug 1083083]
Affects: epel-6 [bug 1083084]
Comment 16 Stefan Cornelius 2014-04-01 08:59:41 EDT
(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.
Comment 17 Stefan Cornelius 2014-04-01 09:33:15 EDT
Statement:

The Red Hat Security Response Team has rated this issue as having moderate security impact. A future update may address this issue. For additional information, refer to the Issue Severity Classification: https://access.redhat.com/security/updates/classification/.
Comment 18 Tomas Hoger 2014-04-04 09:40:02 EDT
As CVE-2014-1947 and CVE-2014-2030 affect different versions, tracking of CVE-2014-2030 was split to a separate bug 1083477.
Comment 19 Fedora Update System 2014-04-15 11:38:00 EDT
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.
Comment 20 Rex Dieter 2014-08-20 10:41:18 EDT
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;
       };
Comment 21 Tomas Hoger 2014-08-25 05:50:40 EDT
(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.
Comment 22 Rex Dieter 2014-08-25 14:48:27 EDT
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;
       };
Comment 23 Tomas Hoger 2014-08-25 15:31:23 EDT
(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.
Comment 24 Rex Dieter 2014-08-28 08:51:53 EDT
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.
Comment 25 Fedora Update System 2014-08-28 11:34:29 EDT
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.
Comment 26 Rex Dieter 2014-08-28 11:52:20 EDT
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.
Comment 27 Bob Friesenhahn 2014-08-28 23:47:12 EDT
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.
Comment 28 Fedora Update System 2014-09-09 18:15:43 EDT
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.
Comment 29 Fedora Update System 2014-09-13 23:27:04 EDT
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.
Comment 30 Fedora Update System 2014-09-18 19:50:39 EDT
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.
Comment 31 Fedora Update System 2014-09-18 19:51:04 EDT
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.
Comment 32 Fedora Update System 2014-11-14 07:08:25 EST
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.

Note You need to log in before you can comment on or make changes to this bug.