womax/nova
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

xenapi: Tidy up Popen calls to avoid command injection attacks

Browse Source 
* 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
This commit is contained in:
Euan Harris
2013-06-26 16:57:14 +01:00
parent 9d44f4db3a
commit 61ef64f48f
5 changed files with 85 additions and 127 deletions
Download Patch File Download Diff File Expand all files Collapse all files
plugins/xenserver/xenapi/etc/xapi.d/plugins/bittorrent
+4 -6
View File
@@ -169,12 +169,10 @@ def _seed(torrent_path, seed_cache_path, torrent_seed_duration,
torrent_listen_port_start, torrent_listen_port_end):
plugin_path = os.path.dirname(inspect.getabsfile(inspect.currentframe()))
seeder_path = os.path.join(plugin_path, SEEDER_PROCESS)
seed_cmd = "%s %s %s %d %d %d" % (
seeder_path, torrent_path, seed_cache_path, torrent_seed_duration,
torrent_listen_port_start, torrent_listen_port_end)
seed_proc = utils.make_subprocess(seed_cmd)
utils.finish_subprocess(seed_proc, seed_cmd)
seed_cmd = [seeder_path, torrent_path, seed_cache_path,
torrent_seed_duration, torrent_listen_port_start,
torrent_listen_port_end]
utils.run_command(seed_cmd)
def _seed_if_needed(seed_cache_path, tarball_path, torrent_path,
plugins/xenserver/xenapi/etc/xapi.d/plugins/migration
+3 -3
View File
@@ -33,15 +33,15 @@ def move_vhds_into_sr(session, instance_uuid, sr_path, uuid_stack):
def _rsync_vhds(instance_uuid, host, staging_path, user="root"):
ssh_cmd = '\"ssh -o StrictHostKeyChecking=no\"'
ssh_cmd = 'ssh -o StrictHostKeyChecking=no'
if not staging_path.endswith('/'):
staging_path += '/'
dest_path = '%s@%s:/images/instance%s/' % (user, host, instance_uuid)
rsync_cmd = "/usr/bin/rsync -av --progress -e %(ssh_cmd)s "\
"%(staging_path)s %(dest_path)s" % locals()
rsync_cmd = ["/usr/bin/rsync", "-av", "--progress", "-e", ssh_cmd,
staging_path, dest_path]
# NOTE(hillad): rsync's progress is carriage returned, requiring
# universal_newlines for real-time output.
plugins/xenserver/xenapi/etc/xapi.d/plugins/utils.py
+42 -32
View File
@@ -18,7 +18,6 @@ import cPickle as pickle
import errno
import logging
import os
import shlex
import shutil
import subprocess
import tempfile
@@ -56,37 +55,54 @@ def _rename(src, dst):
def make_subprocess(cmdline, stdout=False, stderr=False, stdin=False,
universal_newlines=False):
universal_newlines=False, close_fds=True):
"""Make a subprocess according to the given command-line string
"""
# NOTE(dprince): shlex python 2.4 doesn't like unicode so we
# explicitly convert to ascii
cmdline = cmdline.encode('ascii')
LOG.info("Running cmd '%s'" % cmdline)
LOG.info("Running cmd '%s'" % " ".join(cmdline))
kwargs = {}
kwargs['stdout'] = stdout and subprocess.PIPE or None
kwargs['stderr'] = stderr and subprocess.PIPE or None
kwargs['stdin'] = stdin and subprocess.PIPE or None
kwargs['universal_newlines'] = universal_newlines
args = shlex.split(cmdline)
LOG.info("Running args '%s'" % args)
proc = subprocess.Popen(args, **kwargs)
kwargs['close_fds'] = close_fds
proc = subprocess.Popen(cmdline, **kwargs)
return proc
def finish_subprocess(proc, cmdline, ok_exit_codes=None):
class SubprocessException(Exception):
def __init__(self, cmdline, ret, out, err):
Exception.__init__(self, "'%s' returned non-zero exit code: "
"retcode=%i, out='%s', stderr='%s'"
% (cmdline, ret, out, err))
self.cmdline = cmdline
self.ret = ret
self.out = out
self.err = err
def finish_subprocess(proc, cmdline, cmd_input=None, ok_exit_codes=None):
"""Ensure that the process returned a zero exit code indicating success
"""
if ok_exit_codes is None:
ok_exit_codes = [0]
out, err = proc.communicate(cmd_input)
out, err = proc.communicate()
ret = proc.returncode
if ret not in ok_exit_codes:
raise Exception("'%(cmdline)s' returned non-zero exit code: "
"retcode=%(ret)i, out='%(out)s', stderr='%(err)s'"
% locals())
return out, err
raise SubprocessException(' '.join(cmdline), ret, out, err)
return out
def run_command(cmd, cmd_input=None, ok_exit_codes=None):
"""Abstracts out the basics of issuing system commands. If the command
returns anything in stderr, an exception is raised with that information.
Otherwise, the output from stdout is returned.
cmd_input is passed to the process on standard input.
"""
proc = make_subprocess(cmd, stdout=True, stderr=True, stdin=True,
close_fds=True)
return finish_subprocess(proc, cmd, cmd_input=cmd_input,
ok_exit_codes=ok_exit_codes)
def make_staging_area(sr_path):
@@ -165,9 +181,8 @@ def _assert_vhd_not_hidden(path):
If this flag is incorrectly set, then when we move the VHD into the SR, it
will be deleted out from under us.
"""
query_cmd = "vhd-util query -n %(path)s -f" % locals()
query_proc = make_subprocess(query_cmd, stdout=True, stderr=True)
out, err = finish_subprocess(query_proc, query_cmd)
query_cmd = ["vhd-util", "query", "-n", path, "-f"]
out = run_command(query_cmd)
for line in out.splitlines():
if line.lower().startswith('hidden'):
@@ -190,10 +205,8 @@ def _validate_vhd(vdi_path):
Dom0's are out-of-sync. This would corrupt the SR if it were imported, so
generate an exception to bail.
"""
check_cmd = "vhd-util check -n %(vdi_path)s -p" % locals()
check_proc = make_subprocess(check_cmd, stdout=True, stderr=True)
out, err = finish_subprocess(
check_proc, check_cmd, ok_exit_codes=[0, 22])
check_cmd = ["vhd-util", "check", "-n", vdi_path, "-p"]
out = run_command(check_cmd, ok_exit_codes=[0, 22])
first_line = out.splitlines()[0].strip()
if 'invalid' in first_line:
@@ -230,10 +243,8 @@ def _validate_vdi_chain(vdi_path):
failures.
"""
def get_parent_path(path):
query_cmd = "vhd-util query -n %(path)s -p" % locals()
query_proc = make_subprocess(query_cmd, stdout=True, stderr=True)
out, err = finish_subprocess(
query_proc, query_cmd, ok_exit_codes=[0, 22])
query_cmd = ["vhd-util", "query", "-n", path, "-p"]
out = run_command(query_cmd, ok_exit_codes=[0, 22])
first_line = out.splitlines()[0].strip()
if first_line.endswith(".vhd"):
@@ -316,10 +327,9 @@ def import_vhds(sr_path, staging_path, uuid_stack):
for vhd_path in reversed(files_to_move):
if parent_path:
# Link to parent
modify_cmd = ("vhd-util modify -n %(vhd_path)s"
" -p %(parent_path)s" % locals())
modify_proc = make_subprocess(modify_cmd, stderr=True)
finish_subprocess(modify_proc, modify_cmd)
modify_cmd = ["vhd-util", "modify", "-n", vhd_path,
"-p", parent_path]
run_command(modify_cmd)
parent_path = vhd_path
@@ -353,7 +363,7 @@ def create_tarball(fileobj, path, callback=None):
:param path: path to create tarball from
:param callback: optional callback to call on each chunk written
"""
tar_cmd = "tar -zc --directory=%(path)s ." % locals()
tar_cmd = ["tar", "-zc", "--directory=%s" % path, "."]
tar_proc = make_subprocess(tar_cmd, stdout=True, stderr=True)
while True:
@@ -377,7 +387,7 @@ def extract_tarball(fileobj, path, callback=None):
:param path: path to extract tarball into
:param callback: optional callback to call on each chunk read
"""
tar_cmd = "tar -zx --directory=%(path)s" % locals()
tar_cmd = ["tar", "-zx", "--directory=%s" % path]
tar_proc = make_subprocess(tar_cmd, stderr=True, stdin=True)
while True:
plugins/xenserver/xenapi/etc/xapi.d/plugins/xenhost
+23 -62
View File
@@ -26,13 +26,11 @@ try:
except ImportError:
import simplejson as json
import logging
import os
import random
import re
import subprocess
import tempfile
import time
import utils
import pluginlib_nova as pluginlib
import XenAPI
import XenAPIPlugin
@@ -56,48 +54,13 @@ class TimeoutError(StandardError):
pass
def _run_command(cmd):
"""Abstracts out the basics of issuing system commands. If the command
returns anything in stderr, a PluginError is raised with that information.
Otherwise, the output from stdout is returned.
def _run_command(cmd, cmd_input=None):
"""Wrap utils.run_command to raise PluginError on failure
"""
pipe = subprocess.PIPE
proc = subprocess.Popen([cmd], shell=True, stdin=pipe, stdout=pipe,
stderr=pipe, close_fds=True)
proc.wait()
err = proc.stderr.read()
if err:
raise pluginlib.PluginError(err)
return proc.stdout.read()
# NOTE (salvatore-orlando):
# Instead of updating run_command a new method has been implemented,
# in order to avoid risking breaking existing functions calling _run_command
def _run_command_with_input(cmd, process_input):
"""Abstracts out the basics of issuing system commands. If the command
returns anything in stderr, a PluginError is raised with that information.
Otherwise, the output from stdout is returned.
process_input specificies a variable to use as the process' standard input.
"""
pipe = subprocess.PIPE
# cmd can be either a single string with command and arguments,
# or a sequence of string
if not hasattr(cmd, '__iter__'):
cmd = [cmd] # make it iterable
#Note(salvatore-orlando): the shell argument has been set to False
proc = subprocess.Popen(cmd, shell=False, stdin=pipe, stdout=pipe,
stderr=pipe, close_fds=True)
if process_input is not None:
(output, err) = proc.communicate(process_input)
else:
(output, err) = proc.communicate()
if err:
raise pluginlib.PluginError(err)
# This is tantamount to proc.stdout.read()
return output
try:
return utils.run_command(cmd, cmd_input=cmd_input)
except utils.SubprocessException, e:
raise pluginlib.PluginError(e.err)
def _resume_compute(session, compute_ref, compute_uuid):
@@ -111,7 +74,7 @@ def _resume_compute(session, compute_ref, compute_uuid):
# join has been successful, wait for xapi to become alive again
for c in xrange(0, DEFAULT_TRIES):
try:
_run_command("xe vm-start uuid=%s" % compute_uuid)
_run_command(["xe", "vm-start", "uuid=%s" % compute_uuid])
return
except pluginlib.PluginError, e:
logging.exception('Waited %d seconds for the slave to '
@@ -134,19 +97,17 @@ def set_host_enabled(self, arg_dict):
host_uuid = arg_dict['host_uuid']
if enabled == "true":
result = _run_command("xe host-enable uuid=%s" % host_uuid)
result = _run_command(["xe", "host-enable", "uuid=%s" % host_uuid])
elif enabled == "false":
result = _run_command("xe host-disable uuid=%s" % host_uuid)
result = _run_command(["xe", "host-disable", "uuid=%s" % host_uuid])
else:
raise pluginlib.PluginError(_("Illegal enabled status: %s") % enabled)
# Should be empty string
if result:
raise pluginlib.PluginError(result)
# Return the current enabled status
cmd = "xe host-param-list uuid=%s | grep enabled" % host_uuid
resp = _run_command(cmd)
# Response should be in the format: "enabled ( RO): true"
host_enabled = resp.strip().split()[-1]
cmd = ["xe", "host-param-get", "uuid=%s" % host_uuid, "param-name=enabled"]
host_enabled = _run_command(cmd)
if host_enabled == "true":
status = "enabled"
else:
@@ -231,7 +192,7 @@ def iptables_config(session, args):
'iptables-restore',
'ip6tables-save',
'ip6tables-restore'):
result = _run_command_with_input(cmd, process_input)
result = _run_command(cmd, process_input)
ret_str = json.dumps(dict(out=result,
err=''))
logging.debug("iptables_config:exit")
@@ -244,18 +205,18 @@ def iptables_config(session, args):
def _power_action(action, arg_dict):
# Host must be disabled first
host_uuid = arg_dict['host_uuid']
result = _run_command("xe host-disable uuid=%s" % host_uuid)
result = _run_command(["xe", "host-disable", "uuid=%s" % host_uuid])
if result:
raise pluginlib.PluginError(result)
# All running VMs must be shutdown
result = _run_command("xe vm-shutdown --multiple "
"resident-on=%s" % host_uuid)
result = _run_command(["xe", "vm-shutdown", "--multiple",
"resident-on=%s" % host_uuid])
if result:
raise pluginlib.PluginError(result)
cmds = {"reboot": "xe host-reboot uuid=%s",
"startup": "xe host-power-on uuid=%s",
"shutdown": "xe host-shutdown uuid=%s"}
result = _run_command(cmds[action] % host_uuid)
cmds = {"reboot": "host-reboot",
"startup": "host-power-on",
"shutdown": "host-shutdown",}
result = _run_command(["xe", cmds[action], "uuid=%s" % host_uuid])
# Should be empty string
if result:
raise pluginlib.PluginError(result)
@@ -316,7 +277,7 @@ def host_data(self, arg_dict):
information.
"""
host_uuid = arg_dict['host_uuid']
resp = _run_command("xe host-param-list uuid=%s" % host_uuid)
resp = _run_command(["xe", "host-param-list", "uuid=%s" % host_uuid])
parsed_data = parse_response(resp)
# We have the raw dict of values. Extract those that we need,
# and convert the data types as needed.
@@ -345,7 +306,7 @@ def parse_response(resp):
@jsonify
def host_uptime(self, arg_dict):
"""Returns the result of the uptime command on the xenhost."""
return {"uptime": _run_command('uptime')}
return {"uptime": _run_command(['uptime'])}
def cleanup(dct):
plugins/xenserver/xenapi/etc/xapi.d/plugins/xenstore.py
+13 -24
View File
@@ -27,9 +27,7 @@ try:
except ImportError:
import simplejson as json
import logging
import os
import subprocess
import utils
import XenAPIPlugin
@@ -69,14 +67,14 @@ def _record_exists(arg_dict):
is determined from the given path and dom_id in the arg_dict."""
cmd = ["xenstore-exists", "/local/domain/%(dom_id)s/%(path)s" % arg_dict]
try:
ret, result = _run_command(cmd)
_run_command(cmd)
return True
except XenstoreError, e:
if e.stderr == '':
# if stderr was empty, this just means the path did not exist
return False
# otherwise there was a real problem
raise
return True
@jsonify
@@ -89,7 +87,7 @@ def read_record(self, arg_dict):
"""
cmd = ["xenstore-read", "/local/domain/%(dom_id)s/%(path)s" % arg_dict]
try:
ret, result = _run_command(cmd)
result = _run_command(cmd)
return result.strip()
except XenstoreError, e:
if not arg_dict.get("ignore_missing_path", False):
@@ -99,7 +97,7 @@ def read_record(self, arg_dict):
# Just try again in case the agent write won the race against
# the record_exists check. If this fails again, it will likely raise
# an equally meaningful XenstoreError as the one we just caught
ret, result = _run_command(cmd)
result = _run_command(cmd)
return result.strip()
@@ -128,14 +126,14 @@ def list_records(self, arg_dict):
dirpath = "/local/domain/%(dom_id)s/%(path)s" % arg_dict
cmd = ["xenstore-ls", dirpath.rstrip("/")]
try:
ret, recs = _run_command(cmd)
recs = _run_command(cmd)
except XenstoreError, e:
if not _record_exists(arg_dict):
return {}
# Just try again in case the path was created in between
# the "ls" and the existence check. If this fails again, it will
# likely raise an equally meaningful XenstoreError
ret, recs = _run_command(cmd)
recs = _run_command(cmd)
base_path = arg_dict["path"]
paths = _paths_from_ls(recs)
ret = {}
@@ -160,13 +158,12 @@ def delete_record(self, arg_dict):
"""
cmd = ["xenstore-rm", "/local/domain/%(dom_id)s/%(path)s" % arg_dict]
try:
ret, result = _run_command(cmd)
return _run_command(cmd)
except XenstoreError, e:
if 'could not remove path' in e.stderr:
# Entry already gone. We're good to go.
return ''
raise
return result
def _paths_from_ls(recs):
@@ -174,7 +171,6 @@ def _paths_from_ls(recs):
useful. This method cleans that up into a dict with each path
as the key, and the associated string as the value.
"""
ret = {}
last_nm = ""
level = 0
path = []
@@ -203,19 +199,12 @@ def _paths_from_ls(recs):
def _run_command(cmd):
"""Abstracts out the basics of issuing system commands. If the command
returns anything in stderr, a PluginError is raised with that information.
Otherwise, a tuple of (return code, stdout data) is returned.
"""Wrap utils.run_command to raise XenstoreError on failure
"""
logging.info(' '.join(cmd))
pipe = subprocess.PIPE
proc = subprocess.Popen(cmd, stdin=pipe, stdout=pipe, stderr=pipe,
close_fds=True)
out, err = proc.communicate()
if proc.returncode is not os.EX_OK:
raise XenstoreError(cmd, proc.returncode, err, out)
return proc.returncode, out
try:
return utils.run_command(cmd)
except utils.SubprocessException, e:
raise XenstoreError(e.cmdline, e.ret, e.err, e.out)
if __name__ == "__main__":
XenAPIPlugin.dispatch(