6

I have an python cgi script that handles login, this is because my website is three (school) websites combined and before my website can be used the data needs to be extracted from those websites. This extraction takes 2 minutes so I want to make a fancy (semi-fake) loading screen.

My register code ends with:

import subprocess
token = "".join(random.choice(
                    string.ascii_lowercase + string.digits + string.ascii_uppercase)
                        for _ in range(5)) #generate 5 random characters
#run initScript
subprocess.Popen("python {}/python/initUser.py {} {}".format(
    os.getcwd(), uid,token), shell=True, stdin=None, stdout=None, stderr=None,
    close_fds=True)

print "Content-type: text/html"
print "Location: registerLoading.php?token={}".format(token)
print
sys.exit(0)

With the subprocess line stolen from: Run Process and Don't Wait

But the subprocess line is still blocking and I can't figure out why.

I'm developing on ubuntu 16.04, and it's going to run on an raspbarry pi 3 (that explains the loading time)

martineau
  • 119,623
  • 25
  • 170
  • 301
Quinten
  • 422
  • 4
  • 12
  • `subprocess.Popen` is hanging? What if you run python by itself from the command line with those parameters? – rogerdpack Mar 15 '18 at 22:13
  • Interesting question. Take a look at this: http://eyalarubas.com/python-subproc-nonblock.html – dudeman Mar 15 '18 at 22:19
  • @rogerdpack if I run `python .../python/initUser.py ... ` it hangs for 2 minutes if you mean that. And if this script is run as a normal script (non-cgi executed) it also hangs – Quinten Mar 15 '18 at 22:23
  • 1
    This is a *really* dangerous way to use `Popen` -- and unnecessarily so. Take out the `shell=True`, and make it `['python', './python/initUser.py', str(uid), str(token)]` and you're much safer. – Charles Duffy Mar 15 '18 at 22:24
  • 2
    As it is, if someone requests a uid that contains the string `$(rm -rf ~)`, you just deleted your home directory. – Charles Duffy Mar 15 '18 at 22:24
  • @CharlesDuffy correct, except the uid is the primary value extracted from my database, the user never sees, knows or defines his/her uid. But you're right so I'l use your suggestion and research how to use shell=False (it was just cause of laziness) – Quinten Mar 15 '18 at 22:33
  • The other problem with `shell=True` is that it makes it harder to debug cases like this one, because there's more stuff happening. – abarnert Mar 15 '18 at 23:09

1 Answers1

10

close_fds has no effect on stdout. You want devnull file handles (subprocess.DEVNULL in Python 3.3+), so that that the stdout of this script is closed with the call to exit:

subprocess.Popen(
   ["python", "python/initUser.py", uid, token],
   stdin=None, stdout=open(os.devnull, 'wb'), stderr=open(os.devnull, 'wb'))

Note that I also replaced the shell command with a list form. This makes the code safe against command injection - previously, every user could run arbitrary shell commands on your webserver.

In addition, you may also want to beef up the security of the token. 5 characters can be brute-forced, but much more importantly, random.choice is not cryptographically secure. Use random.SystemRandom().choice instead, or the much more modern secrets.token_urlsafe in Python 3.6+.

phihag
  • 278,196
  • 72
  • 453
  • 469
  • Is it correct that this works because passing `stderr=None,stdout=None` still creates a file handler which links it with it's parent process and that that makes it hold? – Quinten Mar 15 '18 at 22:36
  • 1
    @QuintenColtof Precisely, the default (`None`) is to use the current process' stdout and stderr. – phihag Mar 15 '18 at 22:38
  • Okey thanks for the quick response and helpful tips on security :D – Quinten Mar 15 '18 at 22:44
  • You could use [`subprocess.DEVNULL`](https://docs.python.org/3/library/subprocess.html#subprocess.DEVNULL), if you are using Python 3.3 or newer. – Martijn Pieters Mar 15 '18 at 23:06
  • @MartijnPieters Good point. The reason I did not is that the OP evidently uses Python 2.7. Updated the answer. – phihag Mar 15 '18 at 23:10
  • Yup, I know they are using 2.7, but your answer was already pointing to new 3.6+ features. :-) – Martijn Pieters Mar 15 '18 at 23:18