Shell Injection in backup strategies

[Jason Hullinger]

Trove uses subprocess.Popen with shell=True in trove/trove/guestagent/strategies/backup/base.py line 61:

    def run(self):
        self.process = subprocess.Popen(self.command, shell=True,
                                        stdout=subprocess.PIPE,
                                        stderr=subprocess.PIPE,
                                        preexec_fn=os.setsid)
        self.pid = self.process.pid

This could be used, maliciously or not, to inject arbitrary commands into a command line string. An example of this could be triggered is in trove/trove/guestagent/strategies/backup/mysql_imply.py line 37. It is creating a MySQL string with single quote. If the password, either maliciously or just happens to contain another single quote, it will escape from the command and arbitrary data will be executed instead.

For more information on subprocess, shell=True and command injection see: https://docs.python.org/2/library/subprocess.html#frequently-used-arguments

[Grant Murphy (gmurphy)]

Thanks for your bug report. I agree. The process does not need to be created like this (with shell=True). It would probably be better to use the standard oslo wrappers for things like this. In relation to the example attack vector the mysql password would need to be set by an administrator IIUC so hopefully the severity of this is reasonably low.

[Tristan Cacqueray (tristan-cacqueray)]

The OSSA task is set to incomplete pending further security details. Maybe we should add OSSG-coresec to advice on this bug.

[Jason Hullinger (jason-hullinger)]

I would agree, the severity would be low here.

[Thierry Carrez (ttx)]

This doesn't look like a very practical vulnerability (since you need to be root to insert a malicious command in the sql connectionstring), so I'm leaning towards opening this bug, get it fixed in public in the future versions onf trove, and not issue a security advisory about it.

Adding OSSG to get their opinion on it

[Thierry Carrez (ttx)]

Independently reported by Travis McPeak on https://bugs.launchpad.net/ossa/+bug/1348426

[Bryan D. Payne (bdpayne)]

Thierry, I agree... this would be a nice hardening improvement, but no need for this to be private and/or issue an OSSA, in my opinion.

[Thierry Carrez (ttx)]

OK, unless someone complains in the next days, I'll open up this bug and close the OSSA task so that it's fixed quickly and publicly.

[Robert Clark (robert-clark)]

Happy for this to be opened up.

To repeat a comment I've made elsewhere; this should be hardened, the person that writes the config file for a service might not be intended to have root access to the production systems that run the service (think a remote developer/consultant etc) changes can be introduced in a number of ways that aren't obvious and don't require direct access ie. Changes to config management templates.

While I agree that this should be opened up, it should be noted that the modes of access are potentially much more complicated than "You need root access anyway" and this should be hardened at the earliest possible opportunity.

Is it worth considering an OSSN regarding this? "Validate config files where authors are not permitted root access to systems" - or some variation on that theme?

[Thierry Carrez (ttx)]

Running with shell=True is clearly a bug, and as you mention it enables permissions traversal, which has... interesting properties.

I think we could issue an OSSA (or OSSN) about it -- the problem is, that would make us declare that we consider escalation from config files write rights to executing arbitrary code as the service as being a vulnerability. I think there are plenty of ways throughout OpenStack to convert config file writing rights to service arbitrary code execution rights -- for example through the definition of entry points and plugins. We are a long way from having proper isolation here.

So I think it's a bit of a slippery slope. We may come one day to the point where we can consider those rights as being properly isolated, but as of today I think we should fix it and pass on the advisory.

[Robert Clark (robert-clark)]

So maybe what I suggest at the end of #8 is the most appropriate action?

[Thierry Carrez (ttx)]

@Robert: sounds good.

[Nikhil Manchanda (slicknik)]

So the reason this runs with shell = true is because it uses POSIX pipes to do redirection as part of the backup / restore command.

Is there a viable alternative which we can use that allows us to work with pipes doing the backup, and still harden the code wrt the security concern identified here?

[Travis McPeak (travis-mcpeak)]

Anything that you are using piped shell commands for can be written as separate statements. It might not read as smoothly, but it will be safer. Another approach is to move a lot of the operating system calls to their Python equivalents.

[Amrith Kumar (amrith)]

For Trove, this is related to 1349939, 1360954, and 1361622

[Nathan Kinder (nkinder)]

@slicknik
If you are referring to the usage of subprocess.PIPE for stderr/stdout, that will work fine with 'shell=False' as demonstrated by this sample program (create a /tmp/foo.txt with some content first):

-------------------------------------------------------------------------------------------
import os
import subprocess

command = '/usr/bin/cat /tmp/foo.txt'
backup_process = subprocess.Popen(['/usr/bin/cat', '/tmp/foo.txt'], shell=False,
                                  stdout=subprocess.PIPE,
                                  stderr=subprocess.PIPE,
                                  preexec_fn=os.setsid)

print '%s' % backup_process.stdout.read()
-------------------------------------------------------------------------------------------

Are you piping the output of one command to another in the command that is passed into subprocess.Popen() in BackupRunner? If so, you can also do that like this:

-------------------------------------------------------------------------------------------
import os
import subprocess

command = '/usr/bin/cat /tmp/foo.txt'
backup_process = subprocess.Popen(['/usr/bin/cat', '/tmp/foo.txt'], shell=False,
                                  stdout=subprocess.PIPE,
                                  stderr=subprocess.PIPE,
                                  preexec_fn=os.setsid)

grep_process = subprocess.Popen(['/usr/bin/grep', 'blah'], shell=False,
                                stdin=backup_process.stdout,
                                stdout=subprocess.PIPE)

print '%s' % grep_process.stdout.read()
-------------------------------------------------------------------------------------------

[Amrith Kumar (amrith)]

I was intending to fix the bug in a manner similar to what Nathan (nkinder) suggests in #15 above only that I'd rather take the guesswork out of the client side (trove, for example) code and instead encapsulate it into oslo.

To that end, I was thinking of extending execute() and providing execute_extended() which would take a series of command lines and construct the pipeline for you instead.

An important thing to bear in mind in this kind of pipelined execution is that things get a tad bit hairy when people start doing redirection in the command line (and there is pipelining). For example:

cmd1 2>&1 | cmd2

So, when you get something like this, subprocess.Popen() will puke on the 2>&1 unless you use shell=true.

But, this is a reasonable limitation to help eliminate the necessity to use shell=True

Note: I'm dubious about the use of the preexec_fn os.setsid that nkinder recommends as that is process wide and not thread specific.

[Nathan Kinder (nkinder)]

@amrith
I simply copied the 'preexec_fn' from the initial code snippet in this bug showing how Trove uses Popen today. I'm not recommending it's usage.

[Amrith Kumar (amrith)]

ouch, that's not good

[ANish (anish2good)]

The access require root privileges,the security hardening and assumption on the root to damage with the malicious script is still there.