Re: Security bug in ShellExecuteEx()

[Michael Stefaniuc]

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
    	just removed if.
>  wcmd[0] = '\0';
>  retval = SHELL_FindExecutable(sei_tmp.lpDirectory, lpFile, sei_tmp.lpVerb, wcmd, 1024, lpstrProtocol, &env, sei_tmp.lpIDList, sei_tmp.lpParameters);

bye
    michael

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?

Security bug in ShellExecuteEx()

[Martin Fuchs]

On 09.04.2004 17:15:49 Francois Gouget wrote:
> 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.

It wouldn't call it "clobber" - it's intended to change sei_tmp.lpFile 
I programmed it this way to pass modified versions of the
SHELLEXECUTEINFO structure to the worker functions.
This is used to expand environment strings and to translate
shell PIDLs to file system paths.

However all this shellexecute code is not trivial. There are really much
too many special cases to consider. If you ask me, it should be written
from scratch to clean up this mess - but who claims to be able to
do this?  ;-)
> ...
> 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?

Well, I have tested it. But not directly in Wine. I am doing my shell32
tests with ReactOS Explorer using the MSVC debugger on Win XP. So may
be there are really quotation problems with this code in Wine.

Regards,

Martin