This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

Title: subprocess.Popen documentation should contain a good warning about the security implications when using shell=True
Type: behavior Stage: resolved
Components: Documentation Versions: Python 3.1, Python 3.2, Python 2.7
Status: closed Resolution: accepted
Dependencies: Superseder:
Assigned To: docs@python Nosy List: benjamin.peterson, brian.curtin, christoph.neuroth, cvrebert, docs@python, eric.araujo, eric.smith, georg.brandl, ncoghlan, pitrou, r.david.murray
Priority: normal Keywords: patch

Created on 2010-02-17 10:15 by christoph.neuroth, last changed 2022-04-11 14:56 by admin. This issue is now closed.

File name Uploaded Description Edit
subprocess.rst.diff cvrebert, 2010-07-23 07:47 first draft docs patch
subprocess.rst.diff cvrebert, 2010-07-26 05:45 give cat an argument
subprocess.rst.diff cvrebert, 2010-08-29 04:51 last part snipped per R. David
Messages (14)
msg99465 - (view) Author: Christoph Neuroth (christoph.neuroth) Date: 2010-02-17 10:15
Currently, the documentation of subprocess only says "Calling the program through the shell is usually not required.". IMHO there should be a real warning (like, in its own box with a couple of big exclamation marks ;)) about the security implications of using this and detailed instructions of how to avoid it. People tend to use this functionality just because they "know how to use the shell" and its just so convenient - and by doing so they create huge security holes in their applications.
msg99467 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2010-02-17 11:55
This was just discussed in issue 6760.
msg99504 - (view) Author: Christoph Neuroth (christoph.neuroth) Date: 2010-02-18 14:05
You're right, that has been improved in regard to how you can do it instead. However, I still think it lacks to mention the security risk involved - compare this to e.g. os.tempnam(), which has a warning in a red box.
msg99505 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2010-02-18 14:17
If you want to generate some more discussion, I suggest you close this issue and reopen the other one, since that has more people on the nosy list.
msg99506 - (view) Author: Christoph Neuroth (christoph.neuroth) Date: 2010-02-18 14:24
Good idea :)
msg111135 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2010-07-22 00:26
Since the other bug was about a different topic and was closed as fixed, I'd rather have this issue open.  What I've done instead is merge the nosy list from the other bug...anyone who is not interested can of course opt out of this one.

Christoph, would you care to suggest a patch?
msg111140 - (view) Author: Chris Rebert (cvrebert) * Date: 2010-07-22 00:41
I'm busy with finding an apartment and taking exams for the next week-or-so, but after that I'll try and suggest a patch. If anyone wants to have a crack at it between now and then, don't let me stop you.
msg111250 - (view) Author: Chris Rebert (cvrebert) * Date: 2010-07-23 07:47
I found some extra time. Here's an initial suggested patch against py3k head.

Disclaimer: I have no special expertise in computer security beyond having read "Secure Coding: Principles and Practices" a while back.
msg111598 - (view) Author: Chris Rebert (cvrebert) * Date: 2010-07-26 05:45
My apologies for the extra email...
msg113578 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2010-08-11 02:14
Chris, thank you for the patch, sorry I didn't acknowledge it earlier.  I think the core of the patch is good, though I will probably drop the text starting from "To safely use..." when I get time to apply it.  I will also add a note along the lines of "this applies also to Windows, although the details of the exploits differ".

I would like input from others as to whether this should indeed be a red warning box, or is adequate as a note.  We try to minimize the number of red boxes, but this may well be an appropriate case for one (I'm leaning that way myself).
msg115175 - (view) Author: Chris Rebert (cvrebert) * Date: 2010-08-29 04:51
Adjusted patch per R. David's comment.

I obviously think it should be a full red warning box (that's how it is in my patch), but my opinion clearly isn't an outside one.

Also, Ping/Bump on finally getting this applied.
msg120980 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2010-11-11 22:58
Looks good to me, except the last two lines which I would reword or just remove.

I wonder how many people use shell=True merely for the convenience of passing a string instead of a list.  What do you think about adding a mention of str.split and shlex.split?
msg120981 - (view) Author: Chris Rebert (cvrebert) * Date: 2010-11-11 23:03
"the above Note" mentioned in those last two lines demonstrates shlex.split() and correct tokenization.
msg120996 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2010-11-12 00:36
Committed (as a warning) in r86419.  Thanks, Chris.
Date User Action Args
2022-04-11 14:56:57adminsetgithub: 52198
2010-11-12 00:36:42r.david.murraysetstatus: open -> closed

messages: + msg120996
stage: patch review -> resolved
2010-11-11 23:03:16cvrebertsetmessages: + msg120981
2010-11-11 22:58:35eric.araujosetversions: - Python 2.6
nosy: + eric.araujo

messages: + msg120980

type: security -> behavior
2010-08-29 04:51:55cvrebertsetfiles: + subprocess.rst.diff

messages: + msg115175
2010-08-11 02:14:21r.david.murraysetnosy: + docs@python
messages: + msg113578

assignee: georg.brandl -> docs@python
stage: needs patch -> patch review
2010-07-26 05:46:14cvrebertsetfiles: - subprocess.rst.diff
2010-07-26 05:45:09cvrebertsetfiles: + subprocess.rst.diff

messages: + msg111598
2010-07-26 05:33:09cvrebertsetfiles: + subprocess.rst.diff
2010-07-23 16:16:42terry.reedysetnosy: - terry.reedy
2010-07-23 07:47:46cvrebertsetfiles: + subprocess.rst.diff
keywords: + patch
messages: + msg111250
2010-07-22 00:41:05cvrebertsetmessages: + msg111140
2010-07-22 00:26:06r.david.murraysetstatus: closed -> open

superseder: patch to subprocess docs to better explain Popen's 'args' argument ->
versions: + Python 2.7, Python 3.2
nosy: + terry.reedy, ncoghlan, pitrou, benjamin.peterson, r.david.murray, brian.curtin, cvrebert

messages: + msg111135
resolution: duplicate -> accepted
stage: resolved -> needs patch
2010-02-18 14:24:09christoph.neurothsetstatus: open -> closed

messages: + msg99506
2010-02-18 14:17:44eric.smithsetmessages: + msg99505
2010-02-18 14:05:43christoph.neurothsetstatus: closed -> open

messages: + msg99504
2010-02-17 11:55:23eric.smithsetstatus: open -> closed

superseder: patch to subprocess docs to better explain Popen's 'args' argument

nosy: + eric.smith
messages: + msg99467
resolution: duplicate
stage: resolved
2010-02-17 10:15:57christoph.neurothcreate