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 <bfrie...@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,®ion,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 <bfrie...@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,
