XenApi migration driver should not use shell=True with Popen

[Erica Windisch]

The XenApi drivers split a string to create an array for subprocess.Popen, rather than passing an array directly. This invites the potential for command injection / manipulation.

There is no clearly valid reason to use string splitting here when arguments can be passed, as elsewhere, directly into Popen.

The behavior here is present in current Trunk, Folsom, and Essex. Per Trunk and Folsom, _rsync_vhds calls plugins.utils.subprocess to perform the splitting. In Essex, this behaviorism was present directly in migration/transfer_vhd.py, rather than in utils.py. Earlier releases have not been evaluated.

I am not certain if this is directly exploitable. The user field is inserted into the generated strings used for command-line execution, and it does seem that Keystone allows usernames to contain arbitrary tokens/characters such as spaces. It is not clear to me if the user field directly matches that in Keystone, if the user field is otherwise validated in the API, etc. Other fields inserted into the command string seem to be internally generated.

[Russell Bryant (russellb)]

Are you still investigating this to determine if it is exploitable? The first step in handling this is to determine that. That will determine the process we use for fixing the bug (whether it needs to stay private or not).

[Erica Windisch (ewindisch)]

I've looked further and realized that the user variable is clearly not a vector.

Someone could clearly exploit a Xen host through this method given access to the database or messaging bus. Through said such mechanism, they could also trigger a host to send them the contents of a guest's disk over the network. Of course, such things may be true in many places throughout Nova today.

There doesn't appear to be any variables used in this function that is sourced directly from user input in an otherwise normally operating and secure deployment.

[Russell Bryant (russellb)]

Thanks for confirming that there is no exploit path here. I'm going to open this bug up. I tagged it with "security" so that it's noted as a security hardening bug.

[Michael Still (mikal)]

Is this bug still valid? I can't find any calls to subprocess or terrible uses of split() in virt/xenapi for grizzly. There are plenty of calls to utils.execute(), but they seem sane to me.

[Michael Still (mikal)]

I had RussellB double check this as well, and we think this is invalid now. Please reopen if you disagree.

[Erica Windisch (ewindisch)]

Most of these scripts do appear to be fixed in one way or another. However, there is still a problem in nova/plugins/xenserver/xenapi/etc/xapi.d/plugins/xenhost. It defines _run_command() which uses shell=True.

[Euan Harris (euanh)]

Changing the summary - xenhost can't use execvp because it needs to process and return the output of the xe processes that it spawns.

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/34580

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/34580
Committed: http://github.com/openstack/nova/commit/61ef64f48f6815e6e6bb771d2fb33ddcf2f30f40
Submitter: Jenkins
Branch: master

commit 61ef64f48f6815e6e6bb771d2fb33ddcf2f30f40
Author: Euan Harris <email address hidden>
Date: Wed Jun 26 16:57:14 2013 +0100

    xenapi: Tidy up Popen calls to avoid command injection attacks

     * Rewrite xenhost._run_command and xenstore._run_command to use
       utils.make_subprocess and utils.finish_process
     * Change exception raised by utils.finish_process to retain information
       needed by calls in xenhost and xenstore

    Change-Id: Idcdb50bededf0acde92f1774d6752043ba8f97ce
    Fixes: bug #1074087

[Matthew Thode (prometheanfire)]

Is this fix going to be backported to folsom or grizzly? (does it need to be backported)?

[Euan Harris (euanh)]

The analysis in comment #2 suggests that, since the calls to Popen and split don't take any user input, this bug doesn't allow an attack from the outside. Therefore it isn't necessary to backport it.