[docs] os.popen & os.system lack shell-related security warnings (issue 21557)

[pybugs at rebertia.com]

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#newcode2905
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.