From 114c84ae585c11c7e9492c96efa21570b6cd6b02 Mon Sep 17 00:00:00 2001 From: JordanP Date: Mon, 2 Feb 2015 13:36:52 +0000 Subject: [PATCH] Fix Scality SRB driver security concerns LP #1414531 raised 2 issues : 1)A potential arbitrary code execution if the Cinder Linux user has write access to /etc/cinder/cinder.conf 2)An overall concern/question about the usage of the command 'sudo sh -c' throughout the srb driver This patch fixes 1) with proper configuration validation and 2) with usage of cinder-rootwrap. Closes-Bug: 1414531 Change-Id: Idddb9633af3a45d65bbfa0146a14575e2984f6bd --- cinder/tests/test_srb.py | 42 ++++++++++++------- cinder/volume/drivers/srb.py | 78 ++++++++++++++++++++++-------------- 2 files changed, 76 insertions(+), 44 deletions(-) diff --git a/cinder/tests/test_srb.py b/cinder/tests/test_srb.py index 6fc2ee583..062121349 100644 --- a/cinder/tests/test_srb.py +++ b/cinder/tests/test_srb.py @@ -296,20 +296,20 @@ class SRBDriverTestCase(test.TestCase): def _fake_add_urls(self): def check(cmd_string): - return '> /sys/class/srb/add_urls' in cmd_string + return 'tee, /sys/class/srb/add_urls' in cmd_string def act(cmd): - self._urls.append(cmd[2].split()[1]) + self._urls.append(cmd[2]) return check, act def _fake_create(self): def check(cmd_string): - return '> /sys/class/srb/create' in cmd_string + return 'tee, /sys/class/srb/create' in cmd_string def act(cmd): - volname = cmd[2].split()[1] - volsize = cmd[2].split()[2] + volname = cmd[2].split()[0] + volsize = cmd[2].split()[1] self._volumes[volname] = { "name": volname, "size": self._convert_size(volsize), @@ -321,28 +321,28 @@ class SRBDriverTestCase(test.TestCase): def _fake_destroy(self): def check(cmd_string): - return '> /sys/class/srb/destroy' in cmd_string + return 'tee, /sys/class/srb/destroy' in cmd_string def act(cmd): - volname = cmd[2].split()[1] + volname = cmd[2] del self._volumes[volname] return check, act def _fake_extend(self): def check(cmd_string): - return '> /sys/class/srb/extend' in cmd_string + return 'tee, /sys/class/srb/extend' in cmd_string def act(cmd): - volname = cmd[2].split()[1] - volsize = cmd[2].split()[2] + volname = cmd[2].split()[0] + volsize = cmd[2].split()[1] self._volumes[volname]["size"] = self._convert_size(volsize) return check, act def _fake_attach(self): def check(cmd_string): - return '> /sys/class/srb/attach' in cmd_string + return 'tee, /sys/class/srb/attach' in cmd_string def act(_): pass @@ -351,7 +351,7 @@ class SRBDriverTestCase(test.TestCase): def _fake_detach(self): def check(cmd_string): - return '> /sys/class/srb/detach' in cmd_string + return 'tee, /sys/class/srb/detach' in cmd_string def act(_): pass @@ -681,6 +681,14 @@ class SRBDriverTestCase(test.TestCase): return check, act def _fake_execute(self, *cmd, **kwargs): + # Initial version of this driver used to perform commands this way : + # sh echo $cmd > /sys/class/srb + # As noted in LP #1414531 this is wrong, it should be + # tee /sys/class/srb < $cmd + # To avoid having to rewrite every unit tests, we insert the STDIN + # as part of the original command + if 'process_input' in kwargs: + cmd = cmd + (kwargs['process_input'],) cmd_string = ', '.join(cmd) ## # To test behavior, we need to stub part of the brick/local_dev/lvm @@ -724,7 +732,7 @@ class SRBDriverTestCase(test.TestCase): self.fail('Unexpected command: %s' % cmd_string) def _configure_driver(self): - srb.CONF.srb_base_urls = "http://localhost/volumes" + srb.CONF.srb_base_urls = "http://127.0.0.1/volumes" def setUp(self): super(SRBDriverTestCase, self).setUp() @@ -739,9 +747,15 @@ class SRBDriverTestCase(test.TestCase): def test_setup(self): """The url shall be added automatically""" self._driver.do_setup(None) - self.assertEqual('http://localhost/volumes', self._urls[0]) + self.assertEqual('http://127.0.0.1/volumes', + self._urls[0]) self._driver.check_for_setup_error() + @mock.patch.object(srb.CONF, 'srb_base_urls', "http://; evil") + def test_setup_malformated_url(self): + self.assertRaises(exception.VolumeBackendAPIException, + self._driver.do_setup, None) + def test_setup_no_config(self): """The driver shall not start without any url configured""" srb.CONF.srb_base_urls = None diff --git a/cinder/volume/drivers/srb.py b/cinder/volume/drivers/srb.py index df83ec3f1..45a326bb2 100644 --- a/cinder/volume/drivers/srb.py +++ b/cinder/volume/drivers/srb.py @@ -21,6 +21,7 @@ This driver provisions Linux SRB volumes leveraging RESTful storage platforms import contextlib import functools +import re import sys import time @@ -46,12 +47,17 @@ LOG = logging.getLogger(__name__) srb_opts = [ cfg.StrOpt('srb_base_urls', default=None, - help='Comma-separated list of REST servers to connect to'), + help='Comma-separated list of REST servers IP to connect to. ' + '(eg http://IP1/,http://IP2:81/path'), ] CONF = cfg.CONF CONF.register_opts(srb_opts) +ACCEPTED_REST_SERVER = re.compile(r'^http://' + '(\d{1,3}\.){3}\d{1,3}' + '(:\d+)?/[a-zA-Z0-9\-_\/]*$') + class retry: SLEEP_NONE = 'none' @@ -315,7 +321,7 @@ class SRBDriver(driver.VolumeDriver): CDMI). """ - VERSION = '1.0.0' + VERSION = '1.1.0' # Over-allocation ratio (multiplied with requested size) for thin # provisioning @@ -328,6 +334,7 @@ class SRBDriver(driver.VolumeDriver): self.urls_setup = False self.backend_name = None self.base_urls = None + self.root_helper = utils.get_root_helper() self._attached_devices = {} def _setup_urls(self): @@ -339,9 +346,10 @@ class SRBDriver(driver.VolumeDriver): message=_LE('Cound not setup urls on the Block Driver.'), info_message=_LI('Error creating Volume'), reraise=False): - cmd = 'echo ' + self.base_urls + ' > /sys/class/srb/add_urls' - putils.execute('sh', '-c', cmd, - root_helper='sudo', run_as_root=True) + cmd = self.base_urls + path = '/sys/class/srb/add_urls' + putils.execute('tee', path, process_input=cmd, + root_helper=self.root_helper, run_as_root=True) self.urls_setup = True def do_setup(self, context): @@ -349,11 +357,17 @@ class SRBDriver(driver.VolumeDriver): self.backend_name = self.configuration.safe_get('volume_backend_name') base_urls = self.configuration.safe_get('srb_base_urls') + sane_urls = [] if base_urls: - base_urls = ','.join(s.strip() for s in base_urls.split(',')) - - self.base_urls = base_urls - + for url in base_urls.split(','): + stripped_url = url.strip() + if ACCEPTED_REST_SERVER.match(stripped_url): + sane_urls.append(stripped_url) + else: + LOG.warning(_LW("%s is not an accepted REST server " + "IP address"), stripped_url) + + self.base_urls = ','.join(sane_urls) self._setup_urls() def check_for_setup_error(self): @@ -482,11 +496,11 @@ class SRBDriver(driver.VolumeDriver): reraise=exception.VolumeBackendAPIException(data=message)): size = self._size_int(volume['size']) * self.OVER_ALLOC_RATIO - cmd = 'echo ' + volume['name'] + ' ' - cmd += '%dG' % size - cmd += ' > /sys/class/srb/create' - putils.execute('/bin/sh', '-c', cmd, - root_helper='sudo', run_as_root=True) + cmd = volume['name'] + cmd += ' %dG' % size + path = '/sys/class/srb/create' + putils.execute('tee', path, process_input=cmd, + root_helper=self.root_helper, run_as_root=True) return self._set_device_path(volume) @@ -500,11 +514,11 @@ class SRBDriver(driver.VolumeDriver): reraise=exception.VolumeBackendAPIException(data=message)): size = self._size_int(new_size) * self.OVER_ALLOC_RATIO - cmd = 'echo ' + volume['name'] + ' ' - cmd += '%dG' % size - cmd += ' > /sys/class/srb/extend' - putils.execute('/bin/sh', '-c', cmd, - root_helper='sudo', run_as_root=True) + cmd = volume['name'] + cmd += ' %dG' % size + path = '/sys/class/srb/extend' + putils.execute('tee', path, process_input=cmd, + root_helper=self.root_helper, run_as_root=True) @staticmethod def _destroy_file(volume): @@ -515,9 +529,11 @@ class SRBDriver(driver.VolumeDriver): message=message, info_message=_LI('Error destroying Volume %s.') % volname, reraise=exception.VolumeBackendAPIException(data=message)): - cmd = 'echo ' + volume['name'] + ' > /sys/class/srb/destroy' - putils.execute('/bin/sh', '-c', cmd, - root_helper='sudo', run_as_root=True) + cmd = volume['name'] + path = '/sys/class/srb/destroy' + putils.execute('tee', path, process_input=cmd, + root_helper=utils.get_root_helper(), + run_as_root=True) # NOTE(joachim): Must only be called within a function decorated by: # @lockutils.synchronized('devices', 'cinder-srb-') @@ -572,10 +588,10 @@ class SRBDriver(driver.VolumeDriver): message=message, info_message=_LI('Error attaching Volume'), reraise=exception.VolumeBackendAPIException(data=message)): - cmd = 'echo ' + name + ' ' + devname - cmd += ' > /sys/class/srb/attach' - putils.execute('/bin/sh', '-c', cmd, - root_helper='sudo', run_as_root=True) + cmd = name + ' ' + devname + path = '/sys/class/srb/attach' + putils.execute('tee', path, process_input=cmd, + root_helper=self.root_helper, run_as_root=True) else: LOG.debug('Volume %s already attached', name) @@ -591,10 +607,11 @@ class SRBDriver(driver.VolumeDriver): def _do_detach(self, volume, vg): devname = self._device_name(volume) volname = self._get_volname(volume) - cmd = 'echo ' + devname + ' > /sys/class/srb/detach' + cmd = devname + path = '/sys/class/srb/detach' try: - putils.execute('/bin/sh', '-c', cmd, - root_helper='sudo', run_as_root=True) + putils.execute('tee', path, process_input=cmd, + root_helper=self.root_helper, run_as_root=True) except putils.ProcessExecutionError: with excutils.save_and_reraise_exception(reraise=True): try: @@ -664,7 +681,8 @@ class SRBDriver(driver.VolumeDriver): raise exception.VolumeIsBusy(volume_name=volume['name']) vg.destroy_vg() # NOTE(joachim) Force lvm vg flush through a vgs command - vgs = vg.get_all_volume_groups(root_helper='sudo', vg_name=vg.vg_name) + vgs = vg.get_all_volume_groups(root_helper=self.root_helper, + vg_name=vg.vg_name) if len(vgs) != 0: LOG.warning(_LW('Removed volume group %s still appears in vgs.'), vg.vg_name) -- 2.45.2