Re: 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?