[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

RFC: CVE-2018-6799/graphicsmagick



Hi all,

I have been working on CVE-2018-6799 for graphicsmagick in wheezy and
the fix seems to be somewhat problematic. I have cc'd the Security Team
as I believe that this same problem will need to be addressed for the
updates to jessie and stretch.

The fix from upstream consists of two parts:

http://hg.graphicsmagick.org/hg/GraphicsMagick/rev/b41e2efce6d3
http://hg.graphicsmagick.org/hg/GraphicsMagick/rev/d30ed06e9b87

Backporting both commits to wheezy was fairly easy; I have attached the
patches (as CVE-2018-6799-1-of-3.patch and CVE-2018-6799-2-of-3.patch)
for comparison.

The problem that I encounterd was that upstream's fix changes the
signature of InterpolateViewColor() so that instead of returning void it
now returns MagickPassFail (defined to be unsigned int). This would
appear to break ABI compatibility, which I am not sure is a good idea in
a security update. In any event, I asked for advice in #debian-devel and
Olly Betts suggested renaming InterpolateViewColor() and then creating a
small wrapper with the original signature to preserve ABI compatibility.
This is what I have done in the third patch I have attached
(CVE-2018-6799-3-of-3.patch).

I would appreciate it if I could some feedback. Specifically:

- Is it correct to be concerned about the ABI in this case?  (I know
  that changing type/order of parameters and struct members would break
  ABI compatibility, but I am not as certain about return type).
- Assuming that something needs to be done to preserve ABI compatiblity,
  is Olly's recommended approach appropriate/viable? (It seems so to me)
- Assuming the approach is viable, is my implementation correct?

Although there are only a small number of reverse-dependencies of
libgraphicsmagick3 and libgraphicsmagick++3, I would prefer to proceed
cautiously.

Regards,

-Roberto

-- 
Roberto C. Sánchez
# HG changeset patch
# User Bob Friesenhahn <bfriesen@GraphicsMagick.org>
# Date 1515365215 21600
# Node ID b41e2efce6d3a7390b98f7b60e84fc0a9acf4347
# Parent  021fbbedbd45087dddeac8728f0d41b7f5fc4c89
SetNexus(): Fix heap overwrite in AcquireCacheNexus() due to SetNexus() not using an allocated staging area for the pixels like it should.

bug: https://sourceforge.net/p/graphicsmagick/bugs/531/ https://sourceforge.net/p/graphicsmagick/bugs/532/
origin: http://hg.graphicsmagick.org/hg/GraphicsMagick/rev/b41e2efce6d3

(cherry picked from commit b41e2efce6d3)
[rcs: Backported to wheezy]

--- graphicsmagick.git.orig/magick/pixel_cache.c
+++ graphicsmagick.git/magick/pixel_cache.c
@@ -1,5 +1,5 @@
 /*
-% Copyright (C) 2003 - 2010 GraphicsMagick Group
+% Copyright (C) 2003 - 2018 GraphicsMagick Group
 % Copyright (C) 2002 ImageMagick Studio
 %
 % This program is covered by multiple licenses, which are described in
@@ -34,6 +34,7 @@
 #include "magick/studio.h"
 #include "magick/blob.h"
 #include "magick/constitute.h"
+#include "magick/enum_strings.h"
 #include "magick/list.h"
 #include "magick/log.h"
 #include "magick/magick.h"
@@ -741,7 +742,7 @@
 %    o x,y,columns,rows:  These values define the perimeter of a region of
 %      pixels.
 %
-%    o nexus: specifies which cache nexus to acquire.
+%    o nexus_info: specifies which cache nexus to acquire.
 %
 %    o exception: Return any errors or warnings in this structure.
 %
@@ -818,6 +819,7 @@
   region.y=y;
   region.width=columns;
   region.height=rows;
+  /* Define the region of the cache for the cache nexus */
   pixels=SetNexus(image,&region,nexus_info,exception);
   if (pixels == (PixelPacket *) NULL)
     return((const PixelPacket *) NULL);
@@ -1138,7 +1140,8 @@
         offset;
       
       offset=y*(magick_off_t) cache_info->columns+x;
-      if ((cache_info->indexes_valid) && (PseudoClass == image->storage_class))
+      if ((cache_info->indexes_valid) &&
+          (PseudoClass == cache_info->storage_class))
         *pixel=image->colormap[cache_info->indexes[offset]];
       else
         *pixel=cache_info->pixels[offset];
@@ -2665,7 +2668,7 @@
 %
 %
 */
-MagickExport void
+MagickExport MagickPassFail
 InterpolateViewColor(const ViewInfo *view,
                      PixelPacket *color,
                      const double x_offset,
@@ -2680,6 +2683,7 @@
     {
       (void) AcquireOneCacheViewPixel(view,color,(long) x_offset,
                                       (long) y_offset,exception);
+      return MagickFail;
     }
   else
     {
@@ -2706,6 +2710,8 @@
         (one_minus_beta*(one_minus_alpha*p[0].opacity+alpha*p[1].opacity)+
          beta*(one_minus_alpha*p[2].opacity+alpha*p[3].opacity)+0.5);
     }
+
+  return MagickPass;
 }
 MagickExport PixelPacket InterpolateColor(const Image *image,
   const double x_offset,const double y_offset,ExceptionInfo *exception)
@@ -3063,9 +3069,13 @@
           if (cache_info->indexes_valid)
             cache_info->indexes=(IndexPacket *) (pixels+number_pixels);
           FormatSize(cache_info->length,format);
-          (void) LogMagickEvent(CacheEvent,GetMagickModule(),
-                                "open %.1024s (%.1024s)",cache_info->filename,
-                                format);
+          if (image->logging)
+            (void) LogMagickEvent(CacheEvent,GetMagickModule(),
+                                  "open %.1024s (%.1024s) storage_class=%s,"
+                                  " colorspace=%s",cache_info->filename,
+                                  format,
+                                  ClassTypeToString(cache_info->storage_class),
+                                  ColorspaceTypeToString(cache_info->colorspace));
           return(MagickPass);
         }
     }
@@ -3167,12 +3177,16 @@
   /*   (void) signal(SIGBUS,CacheSignalHandler); */
 #endif
   FormatSize(cache_info->length,format);
-  (void) LogMagickEvent(CacheEvent,GetMagickModule(),
-                        "open %.1024s (%.1024s[%d], %.1024s, %.1024s)",
-                        cache_info->filename,cache_info->cache_filename,
-                        cache_info->file,
-                        cache_info->type == MapCache ? "memory-mapped" : "disk",
-                        format);
+  if (image->logging)
+    (void) LogMagickEvent(CacheEvent,GetMagickModule(),
+                          "open %.1024s (%.1024s[%d], %.1024s, %.1024s)"
+                          " storage_class=%s, colorspace=%s",
+                          cache_info->filename,cache_info->cache_filename,
+                          cache_info->file,
+                          cache_info->type == MapCache ? "memory-mapped" : "disk",
+                          format,
+                          ClassTypeToString(cache_info->storage_class),
+                          ColorspaceTypeToString(cache_info->colorspace));
   return(MagickPass);
 }
 
@@ -4044,7 +4058,7 @@
 %
 */
 static PixelPacket *
-SetNexus(const Image *image,const RectangleInfo *region,
+SetNexus(const Image *image,const RectangleInfo * restrict region,
          NexusInfo *nexus_info,ExceptionInfo *exception)
 {
   const CacheInfo
@@ -4061,37 +4075,53 @@
   assert(image != (const Image *) NULL);
   cache_info=(const CacheInfo *) image->cache;
   assert(cache_info->signature == MagickSignature);
-  nexus_info->region=*region;
-  if ((cache_info->type != PingCache) && (cache_info->type != DiskCache) &&
-      (image->clip_mask == (const Image *) NULL))
-    {
-      magick_off_t
-	offset;
+#if 0
+  fprintf(stderr,"SetNexus(): cache_info: %lux%lu,"
+          " region: %lux%lu%+ld%+ld, cache_info->type: %u\n",
+          cache_info->columns, cache_info->rows,
+          region->width, region->height, region->x,region->y,
+          cache_info->type);
+#endif
+  if ((cache_info->type != PingCache) &&
+      (cache_info->type != DiskCache) &&
+      (image->clip_mask == (const Image *) NULL) &&
+      (region->x >=0) &&
+      (region->y >= 0))
+    {
+      if ((/* All/part of one row */
+           (region->height == 1) &&
+           ((region->x+region->width) <= cache_info->columns)
+           )
+          ||
+          (/* One or more full rows */
+           (region->x == 0) &&
+           (region->width == cache_info->columns) &&
+           (region->y+region->height <= cache_info->rows)
+           )
+          )
+        {
+          /*
+            Pixels are accessed directly from memory.
+          */
+          size_t
+            offset;
 
-      offset=nexus_info->region.y*(magick_off_t) cache_info->columns+nexus_info->region.x;
-      length=(nexus_info->region.height-1)*cache_info->columns+nexus_info->region.width-1;
-      number_pixels=(magick_uint64_t) cache_info->columns*cache_info->rows;
-      if ((offset >= 0) && (((magick_uint64_t) offset+length) < number_pixels))
-        if ((((nexus_info->region.x+nexus_info->region.width) <= cache_info->columns) &&
-             (nexus_info->region.height == 1)) ||
-            ((nexus_info->region.x == 0) &&
-             ((nexus_info->region.width % cache_info->columns) == 0)))
-          {
-            /*
-              Pixels are accessed directly from memory.
-            */
-            nexus_info->pixels=cache_info->pixels+offset;
-            nexus_info->indexes=(IndexPacket *) NULL;
-            if (cache_info->indexes_valid)
-              nexus_info->indexes=cache_info->indexes+offset;
-            return(nexus_info->pixels);
-          }
+          offset=((size_t) region->y)*cache_info->columns+((size_t) region->x);
+
+          nexus_info->pixels=cache_info->pixels+offset;
+          nexus_info->indexes=(IndexPacket *) NULL;
+          if (cache_info->indexes_valid)
+            nexus_info->indexes=cache_info->indexes+offset;
+          nexus_info->region=*region;
+          /* fprintf(stderr,"Pixels in core\n"); */
+          return(nexus_info->pixels);
+        }
     }
   /*
     Pixels are stored in a staging area until they are synced to the cache.
   */
-  number_pixels=(magick_uint64_t)
-    Max(nexus_info->region.width*nexus_info->region.height,cache_info->columns);
+  number_pixels=
+    (magick_uint64_t) Max(region->width*region->height,cache_info->columns);
   packet_size=sizeof(PixelPacket);
   if (cache_info->indexes_valid)
     packet_size+=sizeof(IndexPacket);
@@ -4121,13 +4151,18 @@
 			    "region height=%lu, cache columns=%lu)!",
 			    (unsigned long) length,
 			    number_pixels,
-			    nexus_info->region.width,
-			    nexus_info->region.height,
+                            region->width,
+                            region->height,
 			    cache_info->columns);
       ThrowException(exception,ResourceLimitError,MemoryAllocationFailed,
 		     image->filename);
     }
+  else
+    {
+      nexus_info->region=*region;
+    }
 
+  /* fprintf(stderr,"Pixels in staging\n"); */
   return(nexus_info->pixels);
 }
 
# HG changeset patch
# User Bob Friesenhahn <bfriesen@GraphicsMagick.org>
# Date 1515365250 21600
# Node ID d30ed06e9b87edbf4c368ecee1e870434e65c293
# Parent  b41e2efce6d3a7390b98f7b60e84fc0a9acf4347
InterpolateViewColor(): Now returns MagickPassFail rather than void.

bug: https://sourceforge.net/p/graphicsmagick/bugs/531/ https://sourceforge.net/p/graphicsmagick/bugs/532/
origin: http://hg.graphicsmagick.org/hg/GraphicsMagick/rev/d30ed06e9b87

(cherry picked from commit d30ed06e9b87)
[rcs: Backported to wheezy]

--- graphicsmagick.git.orig/magick/composite.c
+++ graphicsmagick.git/magick/composite.c
@@ -2070,18 +2070,23 @@
                 if (update_image->matte)
                   y_displace=(vertical_scale*(p->opacity-
                                               (((double) MaxRGB+1.0)/2)))/(((double) MaxRGB+1.0)/2);
-		InterpolateViewColor(AccessDefaultCacheView(canvas_image),r,
-				     x_offset+x+x_displace,y_offset+y+y_displace,
-				     &canvas_image->exception);
+                if (InterpolateViewColor(AccessDefaultCacheView(canvas_image),r,
+                                         x_offset+x+x_displace,y_offset+y+y_displace,
+                                         &canvas_image->exception) == MagickFail)
+                  {
+                    status=MagickFail;
+                    break;
+                  }
                 p++;
                 q++;
                 r++;
               }
-            if (!SyncImagePixels(change_image))
-              {
-                status=MagickFail;
-                break;
-              }
+            if (status != MagickFail)
+              if (!SyncImagePixels(change_image))
+                {
+                  status=MagickFail;
+                  break;
+                }
           }
         break;
       }
--- graphicsmagick.git.orig/magick/fx.c
+++ graphicsmagick.git/magick/fx.c
@@ -688,15 +688,20 @@
                     factor=1.0;
                     if (distance > 0.0)
                       factor=pow(sin(MagickPI*sqrt(distance)/radius/2),-amount);
-                    InterpolateViewColor(image_view,q,
-                                         factor*x_distance/x_scale+x_center,
-                                         factor*y_distance/y_scale+y_center,
-                                         exception);
+                    if (InterpolateViewColor(image_view,q,
+                                             factor*x_distance/x_scale+x_center,
+                                             factor*y_distance/y_scale+y_center,
+                                             exception) == MagickFail)
+                      {
+                        thread_status=MagickFail;
+                        break;
+                      }
                   }
                 q++;
               }
-            if (!SyncImagePixelsEx(implode_image,exception))
-              thread_status=MagickFail;
+            if (thread_status != MagickFail)
+              if (!SyncImagePixelsEx(implode_image,exception))
+                thread_status=MagickFail;
           }
 #if defined(HAVE_OPENMP)
 #  pragma omp critical (GM_ImplodeImage)
@@ -1585,15 +1590,20 @@
                     factor=1.0-sqrt(distance)/radius;
                     sine=sin(degrees*factor*factor);
                     cosine=cos(degrees*factor*factor);
-                    InterpolateViewColor(image_view,q,
-                                         (cosine*x_distance-sine*y_distance)/x_scale+x_center,
-                                         (sine*x_distance+cosine*y_distance)/y_scale+y_center,
-                                         exception);
+                    if (InterpolateViewColor(image_view,q,
+                                             (cosine*x_distance-sine*y_distance)/x_scale+x_center,
+                                             (sine*x_distance+cosine*y_distance)/y_scale+y_center,
+                                             exception) == MagickFail)
+                      {
+                        thread_status=MagickFail;
+                        break;
+                      }
                   }
                 q++;
               }
-            if (!SyncImagePixelsEx(swirl_image,exception))
-              thread_status=MagickFail;
+            if (thread_status != MagickFail)
+              if (!SyncImagePixelsEx(swirl_image,exception))
+                thread_status=MagickFail;
           }
 #if defined(HAVE_OPENMP)
 #  pragma omp critical (GM_SwirlImage)
@@ -1756,12 +1766,17 @@
           {
             for (x=0; x < (long) wave_image->columns; x++)
               {
-                InterpolateViewColor(image_view,&q[x],(double) x,
-                                     (double) y-sine_map[x],
-                                     exception);
+                if (InterpolateViewColor(image_view,&q[x],(double) x,
+                                         (double) y-sine_map[x],
+                                         exception) == MagickFail)
+                  {
+                    thread_status=MagickFail;
+                    break;
+                  }
               }
-            if (!SyncImagePixelsEx(wave_image,exception))
-              thread_status=MagickFail;
+            if (thread_status != MagickFail)
+              if (!SyncImagePixelsEx(wave_image,exception))
+                thread_status=MagickFail;
           }
 #if defined(HAVE_OPENMP)
 #  pragma omp critical (GM_WaveImage)
--- graphicsmagick.git.orig/magick/pixel_cache.h
+++ graphicsmagick.git/magick/pixel_cache.h
@@ -338,7 +338,7 @@
       const double y_offset,ExceptionInfo *exception)
       MAGICK_FUNC_DEPRECATED;
 
-  extern MagickExport void
+  extern MagickExport MagickPassFail
     InterpolateViewColor(const ViewInfo *view,PixelPacket *color,
        const double x_offset,const double y_offset,
        ExceptionInfo *exception);
--- graphicsmagick.git.orig/magick/render.c
+++ graphicsmagick.git/magick/render.c
@@ -1179,10 +1179,15 @@
 		obtaining a pixel value from a close pixel.
 	      */
 #if 1
-	      InterpolateViewColor(AccessDefaultCacheView(composite),&pixel,
-				   point.x,
-				   point.y,
-				   &image->exception);
+              if (InterpolateViewColor(AccessDefaultCacheView(composite),
+                                       &pixel,
+                                       point.x,
+                                       point.y,
+                                       &image->exception) == MagickFail)
+                {
+                  thread_status=MagickFail;
+                  break;
+                }
 #else
               (void) AcquireOnePixelByReference(composite,&pixel,(long) point.x,
                                                 (long) point.y,&image->exception);
@@ -1192,8 +1197,9 @@
               AlphaCompositePixel(q,&pixel,pixel.opacity,q,q->opacity);
               q++;
             }
-          if (!SyncImagePixelsEx(image,&image->exception))
-            thread_status=MagickFail;
+          if (thread_status != MagickFail)
+            if (!SyncImagePixelsEx(image,&image->exception))
+              thread_status=MagickFail;
         }
 #if defined(HAVE_OPENMP)
 #  pragma omp critical (GM_DrawAffineImage)
bug: https://sourceforge.net/p/graphicsmagick/bugs/531/ https://sourceforge.net/p/graphicsmagick/bugs/532/

NOTE: This change was introduced in version 1.3.16-1.1+deb7u18 with the two
upstream patches for CVE-2018-6799 in order to maintain ABI compatibility
(i.e., InterpolateViewColor() still has a void return type and the function
which returns MagickPassFail has been renamed). Thanks to Olly Betts for
 suggesting this approach.

--- graphicsmagick.git.orig/magick/composite.c
+++ graphicsmagick.git/magick/composite.c
@@ -2070,7 +2070,7 @@
                 if (update_image->matte)
                   y_displace=(vertical_scale*(p->opacity-
                                               (((double) MaxRGB+1.0)/2)))/(((double) MaxRGB+1.0)/2);
-                if (InterpolateViewColor(AccessDefaultCacheView(canvas_image),r,
+                if (InterpolateViewColorPassFail(AccessDefaultCacheView(canvas_image),r,
                                          x_offset+x+x_displace,y_offset+y+y_displace,
                                          &canvas_image->exception) == MagickFail)
                   {
--- graphicsmagick.git.orig/magick/fx.c
+++ graphicsmagick.git/magick/fx.c
@@ -688,7 +688,7 @@
                     factor=1.0;
                     if (distance > 0.0)
                       factor=pow(sin(MagickPI*sqrt(distance)/radius/2),-amount);
-                    if (InterpolateViewColor(image_view,q,
+                    if (InterpolateViewColorPassFail(image_view,q,
                                              factor*x_distance/x_scale+x_center,
                                              factor*y_distance/y_scale+y_center,
                                              exception) == MagickFail)
@@ -1590,7 +1590,7 @@
                     factor=1.0-sqrt(distance)/radius;
                     sine=sin(degrees*factor*factor);
                     cosine=cos(degrees*factor*factor);
-                    if (InterpolateViewColor(image_view,q,
+                    if (InterpolateViewColorPassFail(image_view,q,
                                              (cosine*x_distance-sine*y_distance)/x_scale+x_center,
                                              (sine*x_distance+cosine*y_distance)/y_scale+y_center,
                                              exception) == MagickFail)
@@ -1766,7 +1766,7 @@
           {
             for (x=0; x < (long) wave_image->columns; x++)
               {
-                if (InterpolateViewColor(image_view,&q[x],(double) x,
+                if (InterpolateViewColorPassFail(image_view,&q[x],(double) x,
                                          (double) y-sine_map[x],
                                          exception) == MagickFail)
                   {
--- graphicsmagick.git.orig/magick/pixel_cache.c
+++ graphicsmagick.git/magick/pixel_cache.c
@@ -2668,11 +2668,21 @@
 %
 %
 */
-MagickExport MagickPassFail
+MagickExport void
 InterpolateViewColor(const ViewInfo *view,
                      PixelPacket *color,
                      const double x_offset,
                      const double y_offset,
+                     ExceptionInfo *exception)
+{
+        InterpolateViewColorPassFail(view,color,x_offset,y_offset,exception);
+}
+
+MagickExport MagickPassFail
+InterpolateViewColorPassFail(const ViewInfo *view,
+                     PixelPacket *color,
+                     const double x_offset,
+                     const double y_offset,
                      ExceptionInfo *exception)
 {
   register const PixelPacket
--- graphicsmagick.git.orig/magick/pixel_cache.h
+++ graphicsmagick.git/magick/pixel_cache.h
@@ -338,11 +338,16 @@
       const double y_offset,ExceptionInfo *exception)
       MAGICK_FUNC_DEPRECATED;
 
-  extern MagickExport MagickPassFail
+  extern MagickExport void
     InterpolateViewColor(const ViewInfo *view,PixelPacket *color,
        const double x_offset,const double y_offset,
        ExceptionInfo *exception);
 
+  extern MagickExport MagickPassFail
+    InterpolateViewColorPassFail(const ViewInfo *view,PixelPacket *color,
+       const double x_offset,const double y_offset,
+       ExceptionInfo *exception);
+
   /*
     Modify cache ensures that there is only one reference to the
     pixel cache so that it may be safely modified.
--- graphicsmagick.git.orig/magick/render.c
+++ graphicsmagick.git/magick/render.c
@@ -1179,7 +1179,7 @@
 		obtaining a pixel value from a close pixel.
 	      */
 #if 1
-              if (InterpolateViewColor(AccessDefaultCacheView(composite),
+              if (InterpolateViewColorPassFail(AccessDefaultCacheView(composite),
                                        &pixel,
                                        point.x,
                                        point.y,

Reply to: