Reviewers: demian, http://bugs.python.org/review/21557/diff/13339/Doc/library/os.rst File Doc/library/os.rst (right): http://bugs.python.org/review/21557/diff/13339/Doc/library/os.rst#newcode290... Doc/library/os.rst:2905: .. warning:: On 2014/12/01 23:46:04, demian wrote:
This warning is a little confusing to me. If input sanitization is the issue (which is a perfectly valid concern), why not explain the issue and how to plug the hole rather than discouraging its use altogether?
As is, this reads a little strange to me given you're discouraging the use due to a specific case and then in the next paragraph explaining how to fix it. I'd rather just see an explanation of the possible security hole and how to account for it to make the usage here safe.
A. The patch doesn't discourage it altogether, it discourages it specifically "in cases where the command string is constructed from external input". B. os.popen() deserves to be discouraged. It *defaults to being insecure* and you must specifically remember to actively do the escaping yourself every time. This makes it usage pretty error-prone. History has shown that it's all too easy to overlook the need to do escaping. The subprocess module (when shell=False, which is by default) is not error-prone in this way, and is thus superior. subprocess is also more flexible and featureful. I know of no countervailing advantages whatsoever to using os.popen(). Users often do not need / do not make use of shell features in their commands. IMO, there's currently no good reason to ever use os.popen() instead of the subprocess module. Legacy code is the only reason to still be using it. Please review this at http://bugs.python.org/review/21557/ Affected files: Doc/library/os.rst diff -r 62a058c76869 Doc/library/os.rst --- a/Doc/library/os.rst Mon Dec 01 13:10:12 2014 +0200 +++ b/Doc/library/os.rst Mon Dec 01 12:15:45 2014 -0800 @@ -2899,8 +2899,23 @@ contains the signed integer return code from the child process. This is implemented using :class:`subprocess.Popen`; see that class's - documentation for more powerful ways to manage and communicate with - subprocesses. + documentation for safer and more powerful ways to manage and communicate + with subprocesses. + + .. warning:: + + The use of :func:`popen` is **strongly discouraged** in cases where the + command string is constructed from external input. Executing shell + commands that incorporate unsanitized input from an untrusted source + makes a program vulnerable to `shell injection + http://en.wikipedia.org/wiki/Shell_injection#Shell_injection`_, + a serious security flaw which can result in arbitrary command execution. + For greater safety, consider using :mod:`subprocess` with + ``shell=False`` instead. + + :func:`shlex.quote` can be used to properly escape whitespace and shell + metacharacters in strings that are going to be used to construct shell + commands. .. function:: spawnl(mode, path, ...) @@ -3046,10 +3061,25 @@ status of the command run; on systems using a non-native shell, consult your shell documentation. - The :mod:`subprocess` module provides more powerful facilities for spawning - new processes and retrieving their results; using that module is preferable - to using this function. See the :ref:`subprocess-replacements` section in - the :mod:`subprocess` documentation for some helpful recipes. + The :mod:`subprocess` module provides safer and more powerful facilities + for spawning new processes and retrieving their results; using that module + is preferable to using this function. See the :ref:`subprocess-replacements` + section in the :mod:`subprocess` documentation for some helpful recipes. + + .. warning:: + + The use of :func:`system` is **strongly discouraged** in cases where the + command string is constructed from external input. Executing shell + commands that incorporate unsanitized input from an untrusted source + makes a program vulnerable to `shell injection + http://en.wikipedia.org/wiki/Shell_injection#Shell_injection`_, + a serious security flaw which can result in arbitrary command execution. + For greater safety, consider using :mod:`subprocess` with + ``shell=False`` instead. + + :func:`shlex.quote` can be used to properly escape whitespace and shell + metacharacters in strings that are going to be used to construct shell + commands. Availability: Unix, Windows.