CVE-2009-1440: aMule Insufficient Character Escaping

This is a simple bug reported by Sam Hocevar on 21 April 2009 and affects aMule 2.2.4 release. aMule is a popular open source peer to peer file sharing software that works with various different P2P networks. Here is the buggy code as seen at src/DownloadListCtrl.cpp.

void CDownloadListCtrl::PreviewFile(CPartFile* file)
{
        wxString command;
        // If no player set in preferences, use mplayer.
        // And please, do a warning also :P
        if (thePrefs::GetVideoPlayer().IsEmpty()) {
   ...
                command = wxT("xterm -T \"aMule Preview\" -iconic -e mplayer ") QUOTE wxT("$file") QUOTE;
        } else {
                command = thePrefs::GetVideoPlayer();
        }

        // Check if we are (pre)viewing a completed file or not
        if (file->GetStatus() != PS_COMPLETE) {
   ...   
        } else {
                // This is a complete file
                // FIXME: This is probably not going to work if the filenames are mangled ...
                wxString rawFileName = file->GetFullName().GetRaw();
                if (!command.Replace(wxT("$file"), rawFileName)) {
                        // No magic string, so we just append the filename to the player command
                        // Need to use quotes in case filename contains spaces
                        command << wxT(" ") << QUOTE << rawFileName << QUOTE;
                }
        }
    ...
        // We can't use wxShell here, it blocks the app
        CTerminationProcess *p = new CTerminationProcess(command);
        int ret = wxExecute(command, wxEXEC_ASYNC, p);
    ...
}
&#91;/sourcecode&#93;

In the first if statement, if there is no video player set in the preferences, aMule will attempt to invoke mplayer in a xterm terminal simply like this:

xterm -T "aMule Preview" -iconic -e mplayer 'our_file'

Otherwise, it will use the video player defined in the preferences. If the file that is pre-viewed is complete, it will move into the second else clause. There it appends the raw file name to the command to be executed. At last, it executes the command. The vulnerability is that a user can create a file which has single quote charater ' to its filename in order to escape and execute arbitrary commands. To fix this, the following patch was applied.

&#91;sourcecode language="cpp"&#93;
                wxString rawFileName = file->GetFullName().GetRaw();
+
+#ifndef __WXMSW__
+               rawFileName.Replace(QUOTE, wxT("'\"'\"'"));
+#endif
+
                if (!command.Replace(wxT("$file"), rawFileName)) {

Using this, the single quote character is filtered from the raw filename.