Comment 10 for bug 27767

Message-ID: <email address hidden>
Date: Thu, 05 Jan 2006 14:04:39 +0100
From: Florian Weimer <email address hidden>
To: Daniel Kobras <email address hidden>
Cc: <email address hidden>
Subject: Re: 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).