Security bug in ShellExecuteEx()

[Francois Gouget]

Hi Micheal,

Michael Stefaniuc wrote:
> Hello,
> 
> On Fri, Apr 09, 2004 at 02:14:10AM +0200, Francois Gouget wrote:
> 
>>Index: dlls/shell32/shlexec.c
>>===================================================================
>>RCS file: /var/cvs/wine/dlls/shell32/shlexec.c,v
>>retrieving revision 1.40
>>diff -u -r1.40 shlexec.c
>>--- a/dlls/shell32/shlexec.c	7 Apr 2004 03:49:51 -0000	1.40
>>+++ b/dlls/shell32/shlexec.c	8 Apr 2004 23:12:53 -0000
>>@@ -1114,10 +1126,6 @@
>>         strcatW(wszApplicationName, wszCommandline);
>>     }
>> 
>>-    retval = execfunc(wszApplicationName, NULL, FALSE, &sei_tmp, sei);
>>-    if (retval > 32)
>>-        return TRUE;
>>-
>>     /* Else, try to find the executable */
> 
>           ^^^^^ You may want to remove this because it refers to the

Well, it's actually worst than that. Just before we have this code:

     if (wszCommandline[0]) {
         strcatW(wszApplicationName, wSpace);
         strcatW(wszApplicationName, wszCommandline);
     }
...
     /* Else, try to find the executable */

Now if you missed the beginning of ShellExecuteEx32() you would not 
suspect this but wszApplicationName is the same as sei_tmp.lpFile. So 
the above code, which was already there, would clobber sei_tmp.lpFile 
whenever wszCommandline (i.e. in fact sei_tmp.lpParameters) is not 
empty. This could probably cause ShellExecuteEx to fail in some 
mysterious circumstances.

Now, besides the non-obvious naming scheme, this code is further muddled 
by the fact that sometimes wszCommandline is used as a 1024 character 
temporary buffer, and by the fact that sei_tmp.lpParameters and 
wszCommandline are used interchangeably.

So I'll wait till the original patch is committed and then I'll submit a 
new patch which cleans up the above issues.


There's something else that seems wrong: the code that handles the 
lpIDList does:

     /* process the IDList */
     if (sei_tmp.fMask & SEE_MASK_IDLIST)
     {
         ...
         wszApplicationName[0] = '"';
         SHGetPathFromIDListW(sei_tmp.lpIDList, wszApplicationName+1);
         strcatW(wszApplicationName, wQuote);

     }

So we end up with a wszApplicationName (i.e. an sei_tmp.lpFile) which is 
quoted. Now as I have found out this does not work which is why I remove 
the quotes... a bit before.
So has this lpIDList code been tested? Does it work and if yes how come 
ShellExecute("\"notepad.exe\"") didn't?


-- 
Francois Gouget
fgouget at codeweavers.com