sashahart / vex

Run a command in the named virtualenv.
MIT License
372 stars 26 forks source link

use shell for subprocess.Popen on Windows #16

Closed schlamar closed 10 years ago

schlamar commented 10 years ago

fixes #14, see http://bugs.python.org/issue15451

new patch using your platform check convention :)

sashahart commented 10 years ago

See the Python docs on subprocess. e.g.

The only time you need to specify shell=True on Windows is when the command you wish to execute is built into the shell (e.g. dir or copy). 

and

Warning Passing shell=True can be a security hazard if combined with untrusted input. See the warning under Frequently Used Arguments for details.

It is actually hard to overstate the technical liability associated with using shell=True. In order to support this securely I would have to disallow the inclusion of user input in command line options (unless it was chosen from a whitelist) which is unacceptably inflexible. Otherwise there are just too many mechanisms for hostile parties to sneak malicious input into vex and then get it parsed in unintended ways by subprocess.Popen.

I would sooner attempt to import virtualenv and use it that way, and failing that, simply deprecate Windows support altogether, than ever use shell=True in vex.

schlamar commented 10 years ago

In order to support this securely

Your statement about security is pointless and totally out of context. In context of vex (or any other local command line application) every input is already run in a shell in the user environment so why should vex care about which input is further piped to the shell (any user/possible attacker could directly type in the shell without invoking vex).

The cited statements are only important if you run a process not in user environment which is using subprocess.Popen (server applications).

sashahart commented 10 years ago

Using shell=True is irresponsible. vex has no way of knowing whether it's being invoked in a context where it is being fed untrusted input.

In addition, this is a kludgy workaround rather than a clean fix for a well-understood problem. I'm not taking the PR or implementing shell=True and insulting me isn't going to change that.