homepage

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.

This was just discussed in issue 6760.

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.

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.

Good idea :)

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?

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.

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.

My apologies for the extra email...

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).

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.

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?

"the above Note" mentioned in those last two lines demonstrates shlex.split() and correct tokenization.

Committed (as a warning) in r86419. Thanks, Chris.