Bug#345238: Shell command injection in delegate code (via file names)

2006-01-28 Thread Martin Schulze 
Daniel Kobras wrote:
 On Fri, Jan 27, 2006 at 10:59:34PM +0100, Martin Schulze wrote:
  Daniel Kobras wrote:
Gnah.  You are correct.  I'm extending the list of forbidden characters
by $().
   
   Upstream has reverted the blacklist and instead went for an improved
   version of the symlink fix I added to ImageMagick in unstable. The patch
   is more involved, but also more robust and doesn't impose limits on 
   allowed filenames. If you're interested I can extract the changes from
   upstream SVN.
  
  I've sen your patch and decided against it since it is quite intrusive.
  The blacklist approach should be sufficient for the updates in our stable
  releases.
 
 Yes, but then '(' and ')' are quite commonly found in filenames, so
 someone might trip over this change. The previous fix for CAN-2005-0397

I've decided that they're not dangerous on their own, but only the $
sign, so the patch doesn't touch () at all.

Regards,

Joey

-- 
Computers are not intelligent.  They only think they are.

Please always Cc to me when replying to me on the lists.


-- 
To UNSUBSCRIBE, email to [EMAIL PROTECTED]
with a subject of unsubscribe. Trouble? Contact [EMAIL PROTECTED]

Bug#345238: Shell command injection in delegate code (via file names)

2006-01-27 Thread Daniel Kobras 
found 345238 4:5.4.4.5-1woody7
found 345238 6:6.0.6.2-2.5
thanks

On Thu, Jan 05, 2006 at 01:49:11PM +0100, Daniel Kobras wrote:
 On Fri, Dec 30, 2005 at 02:19:27PM +0100, Florian Weimer wrote:
  With some user interaction, this is exploitable through Gnus and
  Thunderbird.  I think this warrants increasing the severity to
  grave.
 
 Here's the vanilla fix from upstream SVN, stripped off whitespace changes. 
 I wonder why they've banned ` but still allow $(...), though.

The security updates for woody and sarge (DSA-957) use a backport of
upstream's fix without further modifications, ie. this hole can still be
exploited through $(...) expansion. The following test case works on
woody and sarge with the latest imagemagick security updates installed:

% ls
test$(touch boo).fig
% display 'test$(touch boo).fig'
File test.fig does not exist
display: Delegate failed `fig2dev -L ps %i %o'.
% ls
boo  test$(touch boo).fig

Regards,

Daniel.



-- 
To UNSUBSCRIBE, email to [EMAIL PROTECTED]
with a subject of unsubscribe. Trouble? Contact [EMAIL PROTECTED]

Bug#345238: Shell command injection in delegate code (via file names)

2006-01-27 Thread Martin Schulze 
Daniel Kobras wrote:
 found 345238 4:5.4.4.5-1woody7
 found 345238 6:6.0.6.2-2.5
 thanks
 
 On Thu, Jan 05, 2006 at 01:49:11PM +0100, Daniel Kobras wrote:
  On Fri, Dec 30, 2005 at 02:19:27PM +0100, Florian Weimer wrote:
   With some user interaction, this is exploitable through Gnus and
   Thunderbird.  I think this warrants increasing the severity to
   grave.
  
  Here's the vanilla fix from upstream SVN, stripped off whitespace changes. 
  I wonder why they've banned ` but still allow $(...), though.
 
 The security updates for woody and sarge (DSA-957) use a backport of
 upstream's fix without further modifications, ie. this hole can still be
 exploited through $(...) expansion. The following test case works on
 woody and sarge with the latest imagemagick security updates installed:
 
 % ls
 test$(touch boo).fig
 % display 'test$(touch boo).fig'
 File test.fig does not exist
 display: Delegate failed `fig2dev -L ps %i %o'.
 % ls
 boo  test$(touch boo).fig

Gnah.  You are correct.  I'm extending the list of forbidden characters
by $().

Thanks,

Joey

-- 
The MS-DOS filesystem is nice for removable media.  -- H. Peter Anvin

Please always Cc to me when replying to me on the lists.


-- 
To UNSUBSCRIBE, email to [EMAIL PROTECTED]
with a subject of unsubscribe. Trouble? Contact [EMAIL PROTECTED]

Bug#345238: Shell command injection in delegate code (via file names)

2006-01-27 Thread Daniel Kobras 
On Fri, Jan 27, 2006 at 10:32:51PM +0100, Martin Schulze wrote:
 Daniel Kobras wrote:
  On Thu, Jan 05, 2006 at 01:49:11PM +0100, Daniel Kobras wrote:
   On Fri, Dec 30, 2005 at 02:19:27PM +0100, Florian Weimer wrote:
With some user interaction, this is exploitable through Gnus and
Thunderbird.  I think this warrants increasing the severity to
grave.
   
   Here's the vanilla fix from upstream SVN, stripped off whitespace 
   changes. 
   I wonder why they've banned ` but still allow $(...), though.
  
  The security updates for woody and sarge (DSA-957) use a backport of
  upstream's fix without further modifications, ie. this hole can still be
  exploited through $(...) expansion. The following test case works on
  woody and sarge with the latest imagemagick security updates installed:
  
  % ls
  test$(touch boo).fig
  % display 'test$(touch boo).fig'
  File test.fig does not exist
  display: Delegate failed `fig2dev -L ps %i %o'.
  % ls
  boo  test$(touch boo).fig
 
 Gnah.  You are correct.  I'm extending the list of forbidden characters
 by $().

Upstream has reverted the blacklist and instead went for an improved
version of the symlink fix I added to ImageMagick in unstable. The patch
is more involved, but also more robust and doesn't impose limits on 
allowed filenames. If you're interested I can extract the changes from
upstream SVN.

Regards,

Daniel.



-- 
To UNSUBSCRIBE, email to [EMAIL PROTECTED]
with a subject of unsubscribe. Trouble? Contact [EMAIL PROTECTED]

Bug#345238: Shell command injection in delegate code (via file names)

2006-01-27 Thread Martin Schulze 
Daniel Kobras wrote:
  Gnah.  You are correct.  I'm extending the list of forbidden characters
  by $().
 
 Upstream has reverted the blacklist and instead went for an improved
 version of the symlink fix I added to ImageMagick in unstable. The patch
 is more involved, but also more robust and doesn't impose limits on 
 allowed filenames. If you're interested I can extract the changes from
 upstream SVN.

I've sen your patch and decided against it since it is quite intrusive.
The blacklist approach should be sufficient for the updates in our stable
releases.

Regards,

Joey

-- 
The MS-DOS filesystem is nice for removable media.  -- H. Peter Anvin

Please always Cc to me when replying to me on the lists.


-- 
To UNSUBSCRIBE, email to [EMAIL PROTECTED]
with a subject of unsubscribe. Trouble? Contact [EMAIL PROTECTED]

Bug#345238: Shell command injection in delegate code (via file names)

2006-01-27 Thread Daniel Kobras 
On Fri, Jan 27, 2006 at 10:59:34PM +0100, Martin Schulze wrote:
 Daniel Kobras wrote:
   Gnah.  You are correct.  I'm extending the list of forbidden characters
   by $().
  
  Upstream has reverted the blacklist and instead went for an improved
  version of the symlink fix I added to ImageMagick in unstable. The patch
  is more involved, but also more robust and doesn't impose limits on 
  allowed filenames. If you're interested I can extract the changes from
  upstream SVN.
 
 I've sen your patch and decided against it since it is quite intrusive.
 The blacklist approach should be sufficient for the updates in our stable
 releases.

Yes, but then '(' and ')' are quite commonly found in filenames, so
someone might trip over this change. The previous fix for CAN-2005-0397
already partially broke support for movies and multi-layered images, so
I'm not that happy seeing even more functionality taken away. Hm, how
about we go with the quick fix for now, and I'll prepare a slightly more
complex but less user-visible patch for proposed-updates that you can
review later with your SRM hat on?

Regards,

Daniel.



-- 
To UNSUBSCRIBE, email to [EMAIL PROTECTED]
with a subject of unsubscribe. Trouble? Contact [EMAIL PROTECTED]

Bug#345238: Shell command injection in delegate code (via file names)

2006-01-17 Thread Daniel Kobras 
On Thu, Jan 05, 2006 at 02:04:39PM +0100, Florian Weimer wrote:
 A better fix would be to bypass the shell and invoke the delegate
 directly (using fork and execve).  If this is not feasible, the file
 name should be translated according to this pseudo-code:

I went for an even more simple fix: pass a temporary, securely named
symlink to external delegates, instead of the user-supplied filename. We
get rid of the problem this way without any restrictions on allowed
characters in filenames. There's still the problem of information
disclosure because the symlink in /tmp displays the full path to the
image file, but I think that's less severe than the original problem.
Furthermore, users can easily circumvent it setting MAGICK_TMPDIR to a
700 directory. Unfortunately, even though the hack should be good enough
for Debian, it is not suitable for upstream because of portability
issues.

 Please pass this message to upstream nevertheless (I couldn't find a
 security contact on their web pages).

Assuming you were referring to me, I'm currently too short on time to act
as an intermediary for problems in packages I'm not even the maintainer
of. Therefore, I'd be grateful if someone else stepped in and worked
with upstream to settle on a long-term solution. I'm not aware of a
specific security contact, but a message to one of their web forums
usually gets fast attention.

Regards,

Daniel.

diff -u imagemagick-6.2.4.5/magick/blob.c imagemagick-6.2.4.5/magick/blob.c
--- imagemagick-6.2.4.5/magick/blob.c
+++ imagemagick-6.2.4.5/magick/blob.c
@@ -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 -u imagemagick-6.2.4.5/debian/rules imagemagick-6.2.4.5/debian/rules
--- imagemagick-6.2.4.5/debian/rules
+++ imagemagick-6.2.4.5/debian/rules
@@ -24,7 +24,7 @@
--prefix=/usr \
--mandir=\$${prefix}/share/man \
--infodir=\$${prefix}/share/info \
-   --with-gs-font-dir=/usr/share/fonts/type1/gsfonts\
+   --with-gs-font-dir=/usr/share/fonts/type1/gsfonts \
--with-magick-plus-plus \
--enable-shared \
--enable-lzw \
diff -u imagemagick-6.2.4.5/debian/changelog 
imagemagick-6.2.4.5/debian/changelog
--- imagemagick-6.2.4.5/debian/changelog
+++ imagemagick-6.2.4.5/debian/changelog
@@ -1,3 +1,29 @@
+imagemagick (6:6.2.4.5-0.6) unstable; urgency=high
+
+  * Non-maintainer upload.
+  * magick/display.c: In DisplayImageCommand(), expand command line before
+allocating ressources based on argc. Patch and analysis thanks to
+Eero Häkkinen. Closes: #345595
+  * magick/{animate.c,blob.c,display.c,image.c,log.c,montage.c,string.c,
+string_.h}: Implement new utility function FormatMagickStringNumeric()
+to securely expand a user-supplied format string with a single numeric
+argument. Adjust code to use this function where appropriate.
+(CVE-2006-0082) Closes: #345876
+  * coders/pdf.c,coders/ps.c,magick/delegate.c,magick/delegate.h,
+magick/methods.h: Do not call external delegates with user-supplied
+filename, but with securely named symlinks only to prevent shell command
+injection (CVE-2005-4601). Closes: #345238
+  * debian/rules: Make sure to include trailing spaces in multi-line
+commands to keep recent make happy. Cures problems with ghostscript
+font path. Fix thanks to Jeff Lessem. Closes: #347486
+  * debian/imagemagick.mime: Rather than autodetect the type of an image,
+derive it from the mime type. As a side effect, this change allows to
+use arbitrary filenames with the 'see' command, even if they have
+special meaning to imagemagick internally. Also clean up some typos
+and superfluous entries once we're at it. Closes: #344997
+
+ -- Daniel Kobras [EMAIL PROTECTED]  Tue, 17 Jan 2006 18:33:58 +0100
+
 imagemagick (6:6.2.4.5-0.5) unstable; urgency=low
 
   * Another NMU to complete the installability fixes from 6:6.2.4.5-0.4.
diff -u imagemagick-6.2.4.5/debian/imagemagick.mime 
imagemagick-6.2.4.5/debian/imagemagick.mime
--- imagemagick-6.2.4.5/debian/imagemagick.mime
+++ imagemagick-6.2.4.5/debian/imagemagick.mime
@@ -1,45 +1,42 @@
-image/avs; display

Bug#345238: Shell command injection in delegate code (via file names)

2006-01-05 Thread Daniel Kobras 
tag 345238 + patch
thanks

On Fri, Dec 30, 2005 at 02:19:27PM +0100, Florian Weimer wrote:
 With some user interaction, this is exploitable through Gnus and
 Thunderbird.  I think this warrants increasing the severity to
 grave.

Here's the vanilla fix from upstream SVN, stripped off whitespace changes. 
I wonder why they've banned ` but still allow $(...), though.

Regards,

Daniel.

--- delegate.c.orig 2006-01-05 13:37:47.0 +0100
+++ delegate.c  2006-01-05 13:45:00.0 +0100
@@ -701,6 +701,8 @@
 MagickExport MagickBooleanType InvokeDelegate(ImageInfo *image_info,
   Image *image,const char *decode,const char *encode,ExceptionInfo *exception)
 {
+#define ProhibitedAlphabet  *?\'|`
+
   char
 *command,
 **commands;
@@ -753,11 +755,11 @@
 }
   image_info-temporary=MagickTrue;
 }
-  if (delegate_info-mode != 0)
-if (((decode != (const char *) NULL) 
+  if ((delegate_info-mode != 0) 
+  (((decode != (const char *) NULL) 
  (delegate_info-encode != (char *) NULL)) ||
 ((encode != (const char *) NULL) 
- (delegate_info-decode != (char *) NULL)))
+   (delegate_info-decode != (char *) NULL
   {
 char
   *magick;
@@ -771,6 +773,13 @@
 /*
   Delegate requires a particular image format.
 */
+  if ((strpbrk(image_info-filename,ProhibitedAlphabet) != (char *) NULL) 
||
+  (strpbrk(image-filename,ProhibitedAlphabet) != (char *) NULL))
+{
+  ThrowFileException(exception,FileOpenError,
+FilenameContainsProhibitedCharacters,image-filename);
+  return(MagickFalse);
+}
 if (AcquireUniqueFilename(image_info-unique) == MagickFalse)
   {
 ThrowFileException(exception,FileOpenError,
@@ -850,18 +859,25 @@
   for (i=0; commands[i] != (char *) NULL; i++)
   {
 status=MagickFalse;
+if ((strpbrk(image_info-filename,ProhibitedAlphabet) != (char *) NULL) ||
+(strpbrk(image-filename,ProhibitedAlphabet) != (char *) NULL))
+  {
+ThrowFileException(exception,FileOpenError,
+  FilenameContainsProhibitedCharacters,image-filename);
+break;
+  }
 if (AcquireUniqueFilename(image_info-unique) == MagickFalse)
   {
 ThrowFileException(exception,FileOpenError,
   UnableToCreateTemporaryFile,image_info-unique);
-return(MagickFalse);
+break;
   }
 if (AcquireUniqueFilename(image_info-zero) == MagickFalse)
   {
 (void) RelinquishUniqueFileResource(image_info-unique);
 ThrowFileException(exception,FileOpenError,
   UnableToCreateTemporaryFile,image_info-zero);
-return(MagickFalse);
+break;
   }
 command=TranslateText(image_info,image,commands[i]);
 if (command == (char *) NULL)

Bug#345238: Shell command injection in delegate code (via file names)

2006-01-05 Thread Florian Weimer 
* Daniel Kobras:

 tag 345238 + patch
 thanks

 On Fri, Dec 30, 2005 at 02:19:27PM +0100, Florian Weimer wrote:
 With some user interaction, this is exploitable through Gnus and
 Thunderbird.  I think this warrants increasing the severity to
 grave.

 Here's the vanilla fix from upstream SVN, stripped off whitespace changes. 
 I wonder why they've banned ` but still allow $(...), though.

 +#define ProhibitedAlphabet  *?\'|`

This choice of characters is indeed strange.  Perhaps some of them are
Windows-related.

 +  if ((strpbrk(image_info-filename,ProhibitedAlphabet) != (char *) 
 NULL) ||
 +  (strpbrk(image-filename,ProhibitedAlphabet) != (char *) NULL))
 +{
 +  ThrowFileException(exception,FileOpenError,
 +FilenameContainsProhibitedCharacters,image-filename);
 +  return(MagickFalse);
 +}

Wrong direction of test.  You should only pass on known-good
characters, not reject bad characters.

A better fix would be to bypass the shell and invoke the delegate
directly (using fork and execve).  If this is not feasible, the file
name should be translated according to this pseudo-code:

def translate(name):
result = '\''
for char in name:
if name == '\'':
result += '\\''
else:
result += char
result += '\''
return result

Using ' instead of  as the string terminator ensures that variable
expansion is disabled in the string.  If a single quote is contained
in the input string, it is replaced with '\'' (including the quotes),
which terminates the string processing, inserts a quoted '
character, and continues with string processing.  This way, all
characters (except ASCII NUL, naturally) can be safely passed through
the shell to the delegate.  The delegate, however, must have been
written to deal with arbitrary file names.

Unfortunately, is unlikely work on native Windows because command line
parsing is application-specific.

Please pass this message to upstream nevertheless (I couldn't find a
security contact on their web pages).


-- 
To UNSUBSCRIBE, email to [EMAIL PROTECTED]
with a subject of unsubscribe. Trouble? Contact [EMAIL PROTECTED]

Bug#345238: Shell command injection in delegate code (via file names)

2006-01-02 Thread Florian Weimer 
retitle 345238 [CVE-2005-4601] Shell command injection in delegate code (via 
file names)
thanks 

This issue has been assigned CVE-2005-4601.  Please mention this
identifier in the changelog when fixing this bug.


-- 
To UNSUBSCRIBE, email to [EMAIL PROTECTED]
with a subject of unsubscribe. Trouble? Contact [EMAIL PROTECTED]

Bug#345238: Shell command injection in delegate code (via file names)

2005-12-30 Thread Florian Weimer 
severity 345238 grave
thanks

With some user interaction, this is exploitable through Gnus and
Thunderbird.  I think this warrants increasing the severity to
grave.


-- 
To UNSUBSCRIBE, email to [EMAIL PROTECTED]
with a subject of unsubscribe. Trouble? Contact [EMAIL PROTECTED]

Bug#345238: Shell command injection in delegate code (via file names)

2005-12-29 Thread Florian Weimer 
Package: imagemagick
Version: 6.2.4.5-0.3
Tags: security

The delegate code in Imagemagick is vulnerable to shell command
injection, using specially crafted file names:

$ cp /usr/lib/openoffice/share/template/en-US/wizard/bitmap/germany.wmf \
  ' ; echo Hi! 2; : '.gif
$ display ' ; echo Hi! 2; : '.gif

It should work with other file formats besides WMF (those for which
delegates are defined).

I'm leaving the severity at normal, because it doesn't seem to be
*that* important.  Perhaps this is exploitable through MIME-enabled
MUAs, which would warrant a higher severity.


-- 
To UNSUBSCRIBE, email to [EMAIL PROTECTED]
with a subject of unsubscribe. Trouble? Contact [EMAIL PROTECTED]