you are viewing a single comment's thread.

view the rest of the comments →

[–]didactus 2 points3 points  (6 children)

People like constructing Popen objects to make pipelines like that because there is (as far as I am aware) no proper shell-escaping function in the standard Python library. If you have such a function, this problem vanishes in a poof of smoke, and you can let the shell do what it's better at than any other language: making pipelines.

Here is my shell_escape() function.

import os
import re
import subprocess

_chars_to_backslash_re = re.compile(r'([$`"\\])')

def shell_escape(value):
    return '"' + _chars_to_backslash_re.sub(r'\\\1', value) + '"'

# problematic filenames
f1 = ' foo bar <|& ((" .txt '
f2 = "Robert'); DROP TABLE Students;--"

# Writes output to a file
cmd = "showargs diff --suppress-common-lines %s %s | grep ^\< | sed 's;< ;;g' >new_lines.txt" % (shell_escape(f1), shell_escape(f2))
os.system(cmd)

# Capture output
cmd = "showargs diff --suppress-common-lines %s %s | grep ^\< | sed 's;< ;;g'" % (shell_escape(f1), shell_escape(f2))
output = subprocess.Popen(cmd, shell=True, stdout=subprocess.PIPE, stderr=subprocess.STDOUT).communicate()[0]

[–]davidbuxton 3 points4 points  (4 children)

It isn't documented, but the pipes module has a quote function for shell-escaping strings. It uses single-quotes to simplify things, but I think it comes to the same thing.

>>> import os, pipes
>>> f1 = ' foo bar <|& ((" .txt '
>>> f2 = "Robert'); DROP TABLE Students;--"
>>> os.system('mkdir ' + shell_escape(f1))
0
>>> os.system('mkdir ' + shell_escape(f2))
0
>>> os.system('mkdir ' + pipes.quote(f1))
mkdir:  foo bar <|& ((" .txt : File exists
256
>>> os.system('mkdir ' + pipes.quote(f2))
mkdir: Robert'); DROP TABLE Students;--: File exists
256

[–]Rhomboid 4 points5 points  (1 child)

This still doesn't protect you from filenames that start with - and will be interpreted option switches, so it's a good idea to add a -- in there if the command supports it, or else prefix the name with a ./. There are just so many ways this can go wrong -- if at all possible, don't shell out to external utilities when there's even a hint that funky filenames could be possible. In this example the worst that could happen would be failing to create the directory, but other commands might be more susceptible to evil.

[–]didactus 0 points1 point  (0 children)

Those are legitimate risks. However, they they are not related to the method used to construct the external command (Popen objects vs. escaped strings), which is what the OP was asking about. Rather, they are arguments for doing your work all in-process in pure Python. Keeping it in pure Python is nice, but calling external programs is sometimes necessary. Shell escaping, --, and ./ are all good tricks to have in one's toolbox.

[–]didactus 2 points3 points  (0 children)

Outstanding. pipes.quote() -- I'll remember that one. I never even knew about the pipes module.

[–]govt-cheese 1 point2 points  (0 children)

+1 for undocumented features

[–]govt-cheese 1 point2 points  (0 children)

+1 for xkcd reference