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

[Oswald Buddenhagen]

moin,

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?

-- 
Hi! I'm a .signature virus! Copy me into your ~/.signature, please!
--
Confusion, chaos, panic - my work here is done.