Bug#345238: Shell command injection in delegate code (via file names)
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)
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)
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)
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)
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]
Processed: Re: Bug#345238: Shell command injection in delegate code (via file names)
Processing commands for [EMAIL PROTECTED]: > found 345238 4:5.4.4.5-1woody7 Bug#345238: [CVE-2005-4601] Shell command injection in delegate code (via file names) Bug marked as found in version 4:5.4.4.5-1woody7. > found 345238 6:6.0.6.2-2.5 Bug#345238: [CVE-2005-4601] Shell command injection in delegate code (via file names) Bug marked as found in version 6:6.0.6.2-2.5. > thanks Stopping processing here. Please contact me if you need assistance. Debian bug tracking system administrator (administrator, Debian Bugs database) -- 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)
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)
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/a
Bug#345238: Shell command injection in delegate code (via file names)
* 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)
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)
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]
