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

diff --git a/cinder/tests/test_srb.py b/cinder/tests/test_srb.py
index 6fc2ee5833b7bd4876cfedb8717b753645680241..06212134970d4042c2a97ed011f72c1b35200cf7 100644 (file)
--- 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 df83ec3f1a1aeca501a0784b62fd2e152cf88dc6..45a326bb217ad226ff44fa7682a2bb7e8b651611 100644 (file)
--- 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)