Bug 42865 (CVE-2003-0455)

<!-- SBZ_reproduce  -->
-

Created attachment 13080 [details]
putonftp template

Created attachment 13081 [details]
patchinfo

Created attachment 13085 [details]
the debian patch

This patch creates directories with random name in /tmp and never deletes them.
It could be a problem because imagemagick uses temporary files quite often.

Any idea how to solve it?

IMHO, a better approach would be: 
  - create a directory with name 'imagemagick-username' or reuse existing directory 
with this name 
  - set permissions to 700 
  - create a random file in this directory 
 
 

Good Morning, 
yes that is a good solution. 
 
Too be more paranoid. If mkdir() fails you have to 
check the owner and the permissions of the imagemagick-username 
directory to be sure that noone else created this direc. and plays 
games with your files in imagemagick-username/ like he would do in /tmp. 
 
Maybe you like the use of mkdtemp(3), AFAIK ssh use this function too. 

Created attachment 13098 [details]
my patch

Is this patch correct?

- is char *path really MaxTextExtent bytes long w/o any exceptions 
- return value of strncpy/-cat is not tested 
- I would be happier by using snprintf() instead of the strnc*() sequence 
- after assembling the code you should add a trailing \0 
- it is better to exit if the tmpdir already exists and suggest to use another 
TMPDIR variable, otherwise this code may run in an endless loop 
 

MaxTextExtent is used for all strings. 
 
strncpy/-cat is used on more places in ImageMagick without testing return value. 
 
For text formating there is this wraper function: 
 
MagickExport void FormatString(char *string,const char *format,...) 
{ 
  va_list 
    operands; 
 
  va_start(operands,format); 
#if !defined(HAVE_VSNPRINTF) 
  (void) vsprintf(string,format,operands); 
#else 
  (void) vsnprintf(string,MaxTextExtent,format,operands); 
#endif 
  va_end(operands); 
} 
 
It does not check the return value and does not add trailing 0. 
How severe is this bug? 
 
 

> strncpy/-cat is used on more places in ImageMagick without testing return 
value. 
 
That doesnt mean that it's wise. ;) 
 
FormatString() looks ok. *snprintf() always adds a \0. strnc*() does not. 
 
Not checking the return value is not that dangerous for this code as long as 
it does not become setuid. 

Created attachment 13115 [details]
my patch II

second try...
Is this OK?

-    (void) strcpy(path, mSafeTmpdir); 
	path is large enough, right? 
 
- MagickFatalError() 
	does it use va_xxx() functions resp. does it use 
	variable arguments? 
 
Unfortunately another bug was found in imagemagick. I'll make a bugreport 
ASAP. 

- value to mSafeTmpdir is assigned only this way: 
   mSafeTmpdir = strdup(path); 
 
- MagickFatalError expects exactly 3 arguments, it passes them to  
  ErrorHandler method. Default ErrorHandler writes them to stderr. 

Great, then it should be no problem for us. :) 

Thomas, I did not get any info about the second bug yet. Did you fill the bugreport? 

Hm, it was assigned to the wrong person. 
http://bugzilla.suse.de/show_bug.cgi?id=27947 
 
I'll reassign it. 

I submited fixed packages with putonftp and patchinfo. 

CVE-2003-0455

CVE-2003-0455: CVSS v2 Base Score: 4.6 (AV:L/AC:L/Au:N/C:P/I:P/A:P)