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
This commit is contained in:
JordanP
parent
d4e75340ec
commit
114c84ae58
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
|
|
|||
Loading…
Reference in New Issue