KProcess, KRun, KShell, KMacroExpander, their abuse, windows and security holes

[Jaroslaw Staniek]

Oswald Buddenhagen said the following, On 2008-03-16 22:40:

> first of all, a rant: i did a grep on KShell::quoteArg ... the number of
> hits is amazing. and 90+% indicate unnecessary use of a shell.
> the top three reasons *not* to go through a shell:
>   - it is not portable
>   - it is slow
>   - you risk security holes (if you get the argument quoting wrong) -
>     remember that delayed release some years ago?
> so if your code contains any of these:
>   - system()
>   - popen()
>   - KProcess::setShellCommand() or K3Process::setUseShell(true)
> you are likely to be doing something wrong. please review and fix any
> code that you maintain.
> on a related note, KMacroExpander::expandMacrosShellQuote() and
> KShell::splitArgs() are still under-used, leading to lots of duplicated
> (and usually not particularly robust) code. but see below.
> 
> 
> now to the broader topic ... starting subprocesses and supplying
> arguments to them. for now only unix ...
> 
> - the basic and usually only correct way to start a process is by using
>   KProcess (without resorting to setShellCommand). you can run processes
>   synchronously, asynchronously or completely detached, supply a working
>   dir, make redirections, chain processes and deal with their i/o.
> 
> - if you have the user supply file names, you might want to run them
>   through KShell::tildeExpand() prior to passing them to the process.
>   but KFileDialog already handles this internally, so usually there is
>   no need to bother with it (and expanding strings that are not meant to
>   be would be potentially harmful, anyway).
> 
> - then there is KShell::splitArgs() (used without AbortOnMeta). it
>   performs word-splitting according to somewhat simplified bash rules.
>   the result is usually fed into a KProcess.
>   meanwhile it is even somewhat popular.
>   but one has to wonder what it is actually used for - why would anyone
>   supply something that resembles a shell command, but is none?
> 
> - then exists the possibility to run a complete command through a shell
>   via KProcess::setShellCommand().
> 
>   - if you use KShell::quoteArg() or KShell::joinArgs() to construct a
>     command line, you should reconsider - see above.
> 
>   - a generally justified use of the shell are user-supplied command
>     lines. often such a command line is filtered through
>     KMacroExpander::expandMacrosShellQuote() prior to passing it to
>     KProcess to replace any expandos in the command.
> 
>   - a somewhat fancy way to execute a shell command is
>     KRun::runCommand()
> 
>   - a fairly efficient way to execute a shell command is using
>     KShell::splitArgs() with the AbortOnMeta flag - if it can be parsed
>     according to the simplified bash rules, it can be run directly,
>     otherwise it needs to be run through the shell.
>     KRun does that internally.
> 
> now comes into play this bloody braindead waste of time that is called
> windows. it pretty much screws us over with everything that actually
> requires using a shell. the gory details are explained in
> http://lists.kde.org/?l=kde-windows&m=119159232432366&w=2
> 
> as a consequence i suggest the following course of action:
> 
> - i would build the KShell::splitArgs() with AbortOnMeta magic from KRun
>   directly into KProcess::setShellCommand(). on unix, a flag to bypass
>   this automagic would exist. on windows, it would simply refuse to
>   run anything too complex - users can be expected to use batch files for
>   complex constructs anyway.
>   to KRun::runCommand() the same would apply.
> 
> - KShell::splitArgs() would disappear from the public api on windows. on
>   unix, its use should be limited. it would be mostly obsoleted by the
>   previous point anyway.
> 
> - any code that uses KShell::quoteArg() or KShell::joinArgs() is likely
>   to be either bogus or inherently platform-dependend. i think it
>   should be made unix-only again.
> 
> - KMacroExpander::expandMacrosShellQuote() would be safe only as far as
>   the improved KProcess::setShellCommand() goes.
> 
> anything i did not consider?

I can only say that the fact that bool 
KMacroExpanderBase::expandMacrosShellQuote( QString &str,
int &pos ) just fails on Windows makes KRun::processDesktopExec() failing too, 
what in turn breaks opening in my KMail :).

On windows it's true that 99.(9)% of use cases for symbol expanding is 
"<command> %f" call, what's indeed the pattern used so often in the windows 
registry.

-- 
regards / pozdrawiam, Jaroslaw Staniek
  Sponsored by OpenOffice Polska (http://www.openoffice.com.pl/en) to work on
  Kexi & KOffice (http://www.kexi.pl/en, http://www.koffice.org/kexi)
  KDE Libraries for MS Windows (http://windows.kde.org)