To prevent remote code injection on Sheepdog store

Change-Id: Iae92eaf9eb023f36a1bab7c20ea41c985f2bf51b
Signed-off-by: Zhi Yan Liu <zhiyanl@cn.ibm.com>

glance/store/sheepdog.py

@@ -20,6 +20,7 @@ import hashlib
 from oslo.config import cfg
 
 from glance.common import exception
+from glance.common import utils
 from glance.openstack.common import excutils
 import glance.openstack.common.log as logging
 from glance.openstack.common import processutils
@@ -31,7 +32,7 @@ import glance.store.location
 
 LOG = logging.getLogger(__name__)
 
-DEFAULT_ADDR = 'localhost'
+DEFAULT_ADDR = '127.0.0.1'
 DEFAULT_PORT = 7000
 DEFAULT_CHUNKSIZE = 64  # in MiB
 
@@ -62,18 +63,14 @@ class SheepdogImage:
         self.chunk_size = chunk_size
 
     def _run_command(self, command, data, *params):
-        cmd = ("collie vdi %(command)s -a %(addr)s -p %(port)d %(name)s "
+        cmd = ["collie", "vdi"]
-               "%(params)s" %
+        cmd.extend(command)
-               {"command": command,
+        cmd.extend(["-a", self.addr, "-p", self.port, self.name])
-                "addr": self.addr,
+        cmd.extend(params)
-                "port": self.port,
-                "name": self.name,
-                "params": " ".join(map(str, params))})
 
         try:
-            return processutils.execute(
+            return processutils.execute(*cmd, process_input=data)[0]
-                cmd, process_input=data, shell=True)[0]
+        except (processutils.ProcessExecutionError, OSError) as exc:
-        except processutils.ProcessExecutionError as exc:
             LOG.error(exc)
             raise glance.store.BackendException(exc)
 
@@ -83,7 +80,7 @@ class SheepdogImage:
 
         Sheepdog Usage: collie vdi list -r -a address -p port image
         """
-        out = self._run_command("list -r", None)
+        out = self._run_command(["list", "-r"], None)
         return long(out.split(' ')[3])
 
     def read(self, offset, count):
@@ -93,7 +90,7 @@ class SheepdogImage:
 
         Sheepdog Usage: collie vdi read -a address -p port image offset len
         """
-        return self._run_command("read", None, str(offset), str(count))
+        return self._run_command(["read"], None, str(offset), str(count))
 
     def write(self, data, offset, count):
         """
@@ -102,7 +99,7 @@ class SheepdogImage:
 
         Sheepdog Usage: collie vdi write -a address -p port image offset len
         """
-        self._run_command("write", data, str(offset), str(count))
+        self._run_command(["write"], data, str(offset), str(count))
 
     def create(self, size):
         """
@@ -110,7 +107,7 @@ class SheepdogImage:
 
         Sheepdog Usage: collie vdi create -a address -p port image size
         """
-        self._run_command("create", None, str(size))
+        self._run_command(["create"], None, str(size))
 
     def delete(self):
         """
@@ -118,7 +115,7 @@ class SheepdogImage:
 
         Sheepdog Usage: collie vdi delete -a address -p port image
         """
-        self._run_command("delete", None)
+        self._run_command(["delete"], None)
 
     def exist(self):
         """
@@ -126,7 +123,7 @@ class SheepdogImage:
 
         Sheepdog Usage: collie vdi list -r -a address -p port image
         """
-        out = self._run_command("list -r", None)
+        out = self._run_command(["list", "-r"], None)
         if not out:
             return False
         else:
@@ -137,7 +134,7 @@ class StoreLocation(glance.store.location.StoreLocation):
     """
     Class describing a Sheepdog URI. This is of the form:
 
-        sheepdog://image
+        sheepdog://image-id
 
     """
 
@@ -148,10 +145,14 @@ class StoreLocation(glance.store.location.StoreLocation):
         return "sheepdog://%s" % self.image
 
     def parse_uri(self, uri):
-        if not uri.startswith('sheepdog://'):
+        valid_schema = 'sheepdog://'
-            raise exception.BadStoreUri(uri, "URI must start with %s://" %
+        if not uri.startswith(valid_schema):
-                                        'sheepdog')
+            raise exception.BadStoreUri(_("URI must start with %s://") %
-        self.image = uri[11:]
+                                        valid_schema)
+        self.image = uri[len(valid_schema):]
+        if not utils.is_uuid_like(self.image):
+            raise exception.BadStoreUri(_("URI must contains well-formated "
+                                          "image id"))
 
 
 class ImageIterator(object):
@@ -191,7 +192,7 @@ class Store(glance.store.base.Store):
 
         try:
             self.chunk_size = CONF.sheepdog_store_chunk_size * units.Mi
-            self.addr = CONF.sheepdog_store_address
+            self.addr = CONF.sheepdog_store_address.strip()
             self.port = CONF.sheepdog_store_port
         except cfg.ConfigFileValueError as e:
             reason = _("Error in store configuration: %s") % e
@@ -199,10 +200,18 @@ class Store(glance.store.base.Store):
             raise exception.BadStoreConfiguration(store_name='sheepdog',
                                                   reason=reason)
 
+        if ' ' in self.addr:
+            reason = (_("Invalid address configuration of sheepdog store: %s")
+                      % self.addr)
+            LOG.error(reason)
+            raise exception.BadStoreConfiguration(store_name='sheepdog',
+                                                  reason=reason)
+
         try:
-            processutils.execute("collie", shell=True)
+            cmd = ["collie", "vdi", "list", "-a", self.addr, "-p", self.port]
-        except processutils.ProcessExecutionError as exc:
+            processutils.execute(*cmd)
-            reason = _("Error in store configuration: %s") % exc
+        except Exception as e:
+            reason = _("Error in store configuration: %s") % e
             LOG.error(reason)
             raise exception.BadStoreConfiguration(store_name='sheepdog',
                                                   reason=reason)

glance/tests/unit/test_sheepdog_store.py

@@ -56,4 +56,5 @@ class TestStore(base.StoreClearingUnitTest):
                           'fake_image_id',
                           utils.LimitingReader(six.StringIO('xx'), 1),
                           2)
-        self.assertEqual(called_commands, ['list -r', 'create', 'delete'])
+        self.assertEqual([['list', '-r'], ['create'], ['delete']],
+                         called_commands)

glance/tests/unit/test_store_location.py

@@ -66,7 +66,7 @@ class TestStoreLocation(base.StoreClearingUnitTest):
             'rbd://imagename',
             'rbd://fsid/pool/image/snap',
             'rbd://%2F/%2F/%2F/%2F',
-            'sheepdog://imagename',
+            'sheepdog://244e75f1-9c69-4167-9db7-1aa7d1973f6c',
             'cinder://12345678-9012-3455-6789-012345678901',
             'vsphere://ip/folder/openstack_glance/2332298?dcPath=dc&dsName=ds',
         ]
@@ -382,15 +382,18 @@ class TestStoreLocation(base.StoreClearingUnitTest):
         """
         Test the specific StoreLocation for the Sheepdog store
         """
-        uri = 'sheepdog://imagename'
+        uri = 'sheepdog://244e75f1-9c69-4167-9db7-1aa7d1973f6c'
         loc = glance.store.sheepdog.StoreLocation({})
         loc.parse_uri(uri)
-        self.assertEqual('imagename', loc.image)
+        self.assertEqual('244e75f1-9c69-4167-9db7-1aa7d1973f6c', loc.image)
 
-        bad_uri = 'sheepdog:/image'
+        bad_uri = 'sheepdog:/244e75f1-9c69-4167-9db7-1aa7d1973f6c'
         self.assertRaises(exception.BadStoreUri, loc.parse_uri, bad_uri)
 
-        bad_uri = 'http://image'
+        bad_uri = 'http://244e75f1-9c69-4167-9db7-1aa7d1973f6c'
+        self.assertRaises(exception.BadStoreUri, loc.parse_uri, bad_uri)
+
+        bad_uri = 'image; name'
         self.assertRaises(exception.BadStoreUri, loc.parse_uri, bad_uri)
 
     def test_vmware_store_location(self):