Re: Patches for security issues and other problems.
Daniel Kobras <[email protected]> Thu, 5 Jan 2006 18:37:44 +0100
| Newsgroups | gmane.comp.video.graphicsmagick.bugs |
|---|---|
| Message-ID | <[email protected]> |
On Wed, Jan 04, 2006 at 12:42:16AM +0100, Daniel Kobras wrote: > On Tue, Jan 03, 2006 at 04:27:16PM -0600, Bob Friesenhahn wrote: > > On Tue, 3 Jan 2006, Daniel Kobras wrote: > > >* CAN-2005-0397 (Format string vulnerability in magick/image.c) > > > Was originally reported against ImageMagick 6.x because the affected > > > function had been renamed, but also needs to be fixed in 5.x and > > > GraphicsMagick. Patch against 1.1.7 attached. > > > > The chunk of code being patched is indeed known to be broken. The > > proposed patch simply breaks it even more. It really was intended > > that image_info->filename contain a printf style specification since > > the code is testing to see if there is a %d specification in the > > filename which can be substituted. > > Sorry, you're right. I had just looked at the security patch that was > applied to the imagemagick packages without checking for correctness. > Well, at least it plugged the security hole. Looking at current IM, > they've implemented a more complex fix, but I'd say this version should > now choke on things like foo%d%n.jpg. Anyway, this should be easily > fixable. Investigating a bit further, similar code is used in several places. Therefore I introduced a new utility function to perform a single numeric expansion, hopefully in a secure way. The modified code passes all regression tests on my system, and in general seems to do its job. Patch against 1.1.7 attached. What do you think? Regards, Daniel.
format_string_fix_CAN-2005-0397
(text/plain, 8.5 KB)
Format string security fix, addressing CVE-2005-0397 and further related
problems. Introduces new function FormatStringNumeric() that allows a
single numeric format substitution on untrusted user input.
--- a/magick/blob.c Thu Jan 5 17:11:06 2006 +0100
+++ b/magick/blob.c Thu Jan 5 17:12:40 2006 +0100
@@ -2130,31 +2130,13 @@
static void FormMultiPartFilename(Image *image, const ImageInfo *image_info)
{
char
- filename[MaxTextExtent],
+ *filename,
*p;
/*
Form filename for multi-part images.
*/
- (void) strncpy(filename,image->filename,MaxTextExtent-1);
- for (p=strchr(filename,'%'); p != (char *) NULL; p=strchr(p+1,'%'))
- {
- char
- *q;
-
- q=p+1;
- if (*q == '0')
- (void) strtol(q,&q,10);
- if (*q == 'd')
- {
- char
- format[MaxTextExtent];
-
- (void) strncpy(format,p,MaxTextExtent-1);
- FormatString(p,format,GetImageIndexInList(image));
- break;
- }
- }
+ filename=FormatStringNumeric(image->filename,GetImageIndexInList(image));
if (!image_info->adjoin)
if ((image->previous != (Image *) NULL) ||
(image->next != (Image *) NULL))
@@ -2167,6 +2149,7 @@
MaxTextExtent-1);
}
(void) strncpy(image->filename,filename,MaxTextExtent-1);
+ MagickFreeMemory(filename);
}
MagickExport unsigned int OpenBlob(const ImageInfo *image_info,Image *image,
--- a/magick/command.c Thu Jan 5 17:11:06 2006 +0100
+++ b/magick/command.c Thu Jan 5 17:12:40 2006 +0100
@@ -437,16 +437,17 @@
if (first_scene != last_scene)
{
char
- filename[MaxTextExtent];
+ *filename;
/*
Form filename for multi-part images.
*/
- FormatString(filename,image_info->filename,scene);
+ filename=FormatStringNumeric(image_info->filename,scene);
if (LocaleCompare(filename,image_info->filename) == 0)
FormatString(filename,"%.1024s[%lu]",image_info->filename,
scene);
(void) strncpy(image_info->filename,filename,MaxTextExtent-1);
+ MagickFreeMemory(filename);
}
image_info->colorspace=quantize_info->colorspace;
image_info->dither=quantize_info->dither;
@@ -5337,15 +5338,16 @@
if (first_scene != last_scene)
{
char
- filename[MaxTextExtent];
+ *filename;
/*
Form filename for multi-part images.
*/
- FormatString(filename,image_info->filename,scene);
+ filename=FormatStringNumeric(image_info->filename,scene);
if (LocaleCompare(filename,image_info->filename) == 0)
FormatString(filename,"%.1024s.%lu",image_info->filename,scene);
(void) strncpy(image_info->filename,filename,MaxTextExtent-1);
+ MagickFreeMemory(filename);
}
(void) strcpy(image_info->magick,"MIFF");
image_info->colorspace=quantize_info->colorspace;
@@ -11916,15 +11918,16 @@
if (first_scene != last_scene)
{
char
- filename[MaxTextExtent];
+ *filename;
/*
Form filename for multi-part images.
*/
- FormatString(filename,image_info->filename,scene);
+ filename=FormatStringNumeric(image_info->filename,scene);
if (LocaleCompare(filename,image_info->filename) == 0)
FormatString(filename,"%.1024s.%lu",image_info->filename,scene);
(void) strncpy(image_info->filename,filename,MaxTextExtent-1);
+ MagickFreeMemory(filename);
}
(void) CloneString(&image_info->font,montage_info->font);
image_info->colorspace=quantize_info.colorspace;
--- a/magick/image.c Thu Jan 5 17:11:06 2006 +0100
+++ b/magick/image.c Thu Jan 5 17:12:40 2006 +0100
@@ -6244,16 +6244,19 @@
}
if (rectify)
{
+ char
+ *filename_fmt;
const MagickInfo
*magick_info;
/*
Rectify multi-image file support.
*/
- FormatString(filename,image_info->filename,0);
- if ((LocaleCompare(filename,image_info->filename) != 0) &&
- (strchr(filename,'%') == (char *) NULL))
+ filename_fmt=FormatStringNumeric(image_info->filename,0);
+ if ((LocaleCompare(filename_fmt,image_info->filename) != 0) &&
+ (strchr(filename_fmt,'%') == (char *) NULL))
image_info->adjoin=False;
+ MagickFreeMemory(filename_fmt);
magick_info=GetMagickInfo(magic,exception);
if (magick_info != (const MagickInfo *) NULL)
image_info->adjoin&=magick_info->adjoin;
--- a/magick/log.c Thu Jan 5 17:11:06 2006 +0100
+++ b/magick/log.c Thu Jan 5 17:12:40 2006 +0100
@@ -517,11 +517,11 @@
if (log_info->file == (FILE *) NULL)
{
char
- filename[MaxTextExtent];
-
- FormatString(filename,log_info->filename,
- log_info->generation);
+ *filename;
+
+ filename=FormatStringNumeric(log_info->filename,log_info->generation);
log_info->file=fopen(filename,"w");
+ MagickFreeMemory(filename);
if (log_info->file == (FILE *) NULL)
{
LiberateSemaphoreInfo(&log_semaphore);
@@ -569,11 +569,11 @@
if (log_info->file == (FILE *) NULL)
{
char
- filename[MaxTextExtent];
-
- FormatString(filename,log_info->filename,
- log_info->generation);
+ *filename;
+
+ filename=FormatStringNumeric(log_info->filename,log_info->generation);
log_info->file=fopen(filename,"w");
+ MagickFreeMemory(filename);
if (log_info->file == (FILE *) NULL)
{
LiberateSemaphoreInfo(&log_semaphore);
--- a/magick/utility.c Thu Jan 5 17:11:06 2006 +0100
+++ b/magick/utility.c Thu Jan 5 17:12:40 2006 +0100
@@ -1127,6 +1127,72 @@
# endif
#endif
va_end(operands);
+}
+
+/*
+%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
+% %
+% %
+% %
+% F o r m a t S t r i n g N u m e r i c %
+% %
+% %
+% %
+%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
+%
+% Method FormatStringNumeric formats output for a single numeric argument.
+% It takes into account that the format string given might be untrusted
+% user input, and returns a pointer to the formatted string.
+%
+% The format of the FormatStringNumeric method is:
+%
+% char * FormatStringNumeric(char *string,const char *format,int value)
+%
+% A description of each parameter follows.
+%
+% o format: A string describing the format to use to write the numeric
+% argument. Only the first numeric format identifier is replaced.
+%
+% o value: Numeric value to substitute into format string.
+%
+%
+*/
+MagickExport char *FormatStringNumeric(const char *format,int value)
+{
+ char
+ *p,
+ *string;
+
+ string = NULL;
+
+ (void) CloneString(&string, format);
+
+ for (p=strchr(format,'%'); p != (char *) NULL; p=strchr(p+1,'%'))
+ {
+ char
+ *q;
+
+ q=(char *) p+1;
+ if (*q == '0')
+ (void) strtol(q,&q,10);
+ if ((*q == '%') || (*q == 'd') || (*q == 'o') || (*q == 'x'))
+ {
+ char
+ c;
+
+ q++;
+ c=*q;
+ *q='\0';
+ (void) snprintf(string+(p-format),MaxTextExtent-(p-format),p,value);
+ *q=c;
+ (void) ConcatenateString(&string,q);
+ if (*(q-1) == '%')
+ p++;
+ else
+ break;
+ }
+ }
+ return string;
}
/*
--- a/magick/utility.h Thu Jan 5 17:11:06 2006 +0100
+++ b/magick/utility.h Thu Jan 5 17:12:40 2006 +0100
@@ -61,6 +61,7 @@
*AllocateString(const char *),
*Base64Encode(const unsigned char *,const size_t,size_t *),
*EscapeString(const char *,const char),
+ *FormatStringNumeric(const char *,int),
*GetPageGeometry(const char *),
**ListFiles(const char *,const char *,long *),
*SetClientName(const char *),