Comment 14 for bug 27952

Message-ID: <email address hidden>
Date: Tue, 10 Jan 2006 17:03:55 +0100
From: Daniel Kobras <email address hidden>
To: Matthias Clasen <email address hidden>, <email address hidden>
Subject: Re: Bug#345876: imagemagick: New format string vulnerability in SetImageInfo().

--fdj2RfSjLxBAspz7
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Mon, Jan 09, 2006 at 10:18:13AM -0500, Matthias Clasen wrote:
> Ah, got it now. Does this look more complete ?

Yes, but there are still a few more places to fix. The attached patch
for 6.2.4.5 should be fairly complete, apart from a few odd places in
coders/, but those fall more into the scope of CVE-2005-4601. Mind,
though, that I'm still testing the fix, so some polishing might be
needed still.

Regards,

Daniel.

--fdj2RfSjLxBAspz7
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline; filename="im_format_string.diff"

diff -r 8a3cbd342315 magick/animate.c
--- a/magick/animate.c Tue Jan 10 12:11:55 2006 +0100
+++ b/magick/animate.c Tue Jan 10 16:55:22 2006 +0100
@@ -604,7 +604,7 @@
               /*
                 Form filename for multi-part images.
               */
- (void) FormatMagickString(filename,MaxTextExtent,
+ (void) FormatMagickStringNumeric(filename,MaxTextExtent,
                 image_info->filename,scene);
               if (LocaleCompare(filename,image_info->filename) == 0)
                 (void) FormatMagickString(filename,MaxTextExtent,"%s[%lu]",
diff -r 8a3cbd342315 magick/blob.c
--- a/magick/blob.c Tue Jan 10 12:11:55 2006 +0100
+++ b/magick/blob.c Tue Jan 10 16:55:22 2006 +0100
@@ -2120,25 +2120,8 @@
       /*
         Form filename for multi-part images.
       */
- (void) CopyMagickString(filename,image->filename,MaxTextExtent);
- 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 == '%') || (*q == 'd') || (*q == 'o') || (*q == 'x'))
- {
- char
- format[MaxTextExtent];
-
- (void) CopyMagickString(format,p,MaxTextExtent);
- (void) FormatMagickString(p,MaxTextExtent,format,image->scene);
- break;
- }
- }
+ (void) FormatMagickStringNumeric(filename,MaxTextExtent,image->filename,
+ image->scene);
       if (image_info->adjoin == MagickFalse)
         if ((image->previous != (Image *) NULL) ||
             (GetNextImageInList(image) != (Image *) NULL))
diff -r 8a3cbd342315 magick/display.c
--- a/magick/display.c Tue Jan 10 12:11:55 2006 +0100
+++ b/magick/display.c Tue Jan 10 16:55:22 2006 +0100
@@ -1984,7 +1984,7 @@
               /*
                 Form filename for multi-part images.
               */
- (void) FormatMagickString(filename,MaxTextExtent,
+ (void) FormatMagickStringNumeric(filename,MaxTextExtent,
                 image_info->filename,scene);
               if (LocaleCompare(filename,image_info->filename) == 0)
                 (void) FormatMagickString(filename,MaxTextExtent,"%s.%lu",
diff -r 8a3cbd342315 magick/image.c
--- a/magick/image.c Tue Jan 10 12:11:55 2006 +0100
+++ b/magick/image.c Tue Jan 10 16:55:22 2006 +0100
@@ -2869,25 +2869,8 @@
       /*
         Rectify multi-image file support.
       */
- (void) CopyMagickString(filename,image_info->filename,MaxTextExtent);
- for (p=strchr(filename,'%'); 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
- format[MaxTextExtent];
-
- (void) CopyMagickString(format,p,MaxTextExtent);
- (void) FormatMagickString(p,MaxTextExtent,format,image_info->scene);
- break;
- }
- }
+ (void) FormatMagickStringNumeric(filename,MaxTextExtent,
+ image_info->filename,image_info->scene);
       if ((LocaleCompare(filename,image_info->filename) != 0) &&
           (strchr(filename,'%') == (char *) NULL))
         image_info->adjoin=MagickFalse;
diff -r 8a3cbd342315 magick/log.c
--- a/magick/log.c Tue Jan 10 12:11:55 2006 +0100
+++ b/magick/log.c Tue Jan 10 16:55:22 2006 +0100
@@ -914,8 +914,8 @@
           char
             filename[MaxTextExtent];

- (void) FormatMagickString(filename,MaxTextExtent,log_info->filename,
- log_info->generation % log_info->generations);
+ (void) FormatMagickStringNumeric(filename,MaxTextExtent,
+ log_info->filename,log_info->generation % log_info->generations);
           log_info->file=fopen(filename,"w");
           if (log_info->file == (FILE *) NULL)
             {
diff -r 8a3cbd342315 magick/montage.c
--- a/magick/montage.c Tue Jan 10 12:11:55 2006 +0100
+++ b/magick/montage.c Tue Jan 10 16:55:22 2006 +0100
@@ -530,7 +530,7 @@
               /*
                 Form filename for multi-part images.
               */
- (void) FormatMagickString(filename,MaxTextExtent,
+ (void) FormatMagickStringNumeric(filename,MaxTextExtent,
                 image_info->filename,scene);
               if (LocaleCompare(filename,image_info->filename) == 0)
                 (void) FormatMagickString(filename,MaxTextExtent,"%s.%lu",
diff -r 8a3cbd342315 magick/string.c
--- a/magick/string.c Tue Jan 10 12:11:55 2006 +0100
+++ b/magick/string.c Tue Jan 10 16:55:22 2006 +0100
@@ -953,6 +953,75 @@
       return((StringInfo *) NULL);
     }
   return(string_info);
+}
+

+/*
+%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
+% %
+% %
+% %
+% F o r m a t M a g i c k S t r i n g N u m e r i c %
+% %
+% %
+% %
+%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
+%
+% Method FormatMagickStringNumeric formats output for a single numeric
+% argument. It takes into account that the format string given might be
+% untrusted user input, and returns the length of the formatted string.
+%
+% The format of the FormatMagickStringNumeric method is:
+%
+% long FormatMagickStringNumeric(char *string,const size_t length,
+% const char *format,int value)
+%
+% A description of each parameter follows.
+%
+% o string: FormatMagickStringNumeric() returns the formatted string in this
+% character buffer.
+%
+% o length: The maximum length of the string.
+%
+% 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 long FormatMagickStringNumeric(char *string,const size_t length,const char *format,int value)
+{
+ char
+ *p;
+
+ (void) CopyMagickString(string, format, length);
+
+ 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),length-(p-format),p,value);
+ *q=c;
+ (void) ConcatenateMagickString(&string,q,length);
+ if (*(q-1) == '%')
+ p++;
+ else
+ break;
+ }
+ }
+ return (long)strlen(string);
 }

 /*
diff -r 8a3cbd342315 magick/string_.h
--- a/magick/string_.h Tue Jan 10 12:11:55 2006 +0100
+++ b/magick/string_.h Tue Jan 10 16:55:22 2006 +0100
@@ -60,6 +60,7 @@
     magick_attribute((format (printf,3,4))),
   FormatMagickStringList(char *,const size_t,const char *,va_list)
     magick_attribute((format (printf,3,0))),
+ FormatMagickStringNumeric(char *,const size_t,const char *,int),
   LocaleCompare(const char *,const char *),
   LocaleNCompare(const char *,const char *,const size_t);

--fdj2RfSjLxBAspz7--