From eb99e45829a1b4c93db5692bdbf636a86faa56c4 Mon Sep 17 00:00:00 2001
From: Flavio Percoco <flaper87@gmail.com>
Date: Thu, 9 Jul 2015 14:44:04 +0200
Subject: Don't import files with backed files

There's a security issue where it'd be possible to import images with
backed files using the task engine and then use/convert those to access
system files or any other file in the system. An example of an attack
would be to import an image with a backing file pointing to
`/etc/passwd`, then convert it to raw and download the generated image.

This patch forbids importing files with baking files entirely. It does
that in the `_ImportToFS` task, which is the one that imports the image
locally to then execute other tasks on it. It's not necessary for the
`_ImportToStore` task because other tasks won't be executed when the
image is imported in the final store.

Change-Id: I35f43c3b3f326942fb53b7dadb94700ac4513494
Closes-bug: #1471912
(cherry picked from commit d529863a1e8d2307526bdb395b4aebe97f81603d)

diff --git a/glance/async/flows/base_import.py b/glance/async/flows/base_import.py
index 7656bde..d216aa8 100644
--- a/glance/async/flows/base_import.py
+++ b/glance/async/flows/base_import.py
@@ -13,12 +13,15 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
+import json
 import logging
 import os
 
 import glance_store as store_api
 from glance_store import backend
+from oslo_concurrency import processutils as putils
 from oslo_config import cfg
+from oslo_utils import excutils
 import six
 from stevedore import named
 from taskflow.patterns import linear_flow as lf
@@ -146,6 +149,29 @@ class _ImportToFS(task.Task):
         data = script_utils.get_image_data_iter(self.uri)
 
         path = self.store.add(image_id, data, 0, context=None)[0]
+
+        try:
+            # NOTE(flaper87): Consider moving this code to a common
+            # place that other tasks can consume as well.
+            stdout, stderr = putils.trycmd('qemu-img', 'info',
+                                           '--output=json', path,
+                                           log_errors=putils.LOG_ALL_ERRORS)
+        except OSError as exc:
+            with excutils.save_and_reraise_exception():
+                msg = (_LE('Failed to execute security checks on the image '
+                           '%(task_id)s: %(exc)s') %
+                       {'task_id': self.task_id, 'exc': exc.message})
+                LOG.error(msg)
+
+        metadata = json.loads(stdout)
+
+        backing_file = metadata.get('backing-filename')
+        if backing_file is not None:
+            msg = _("File %(path)s has invalid backing file "
+                    "%(bfile)s, aborting.") % {'path': path,
+                                               'bfile': backing_file}
+            raise RuntimeError(msg)
+
         return path
 
     def revert(self, image_id, result=None, **kwargs):
diff --git a/glance/tests/unit/async/flows/test_import.py b/glance/tests/unit/async/flows/test_import.py
index 70f790c..4cf3d13 100644
--- a/glance/tests/unit/async/flows/test_import.py
+++ b/glance/tests/unit/async/flows/test_import.py
@@ -13,14 +13,17 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
+import json
 import mock
 import os
 import urllib2
 
 import glance_store
+from oslo_concurrency import processutils as putils
 from oslo_config import cfg
 from six.moves import cStringIO
 from taskflow import task
+from taskflow.types import failure
 
 import glance.async.flows.base_import as import_flow
 from glance.async import taskflow_executor
@@ -106,16 +109,23 @@ class TestImportTask(test_utils.BaseTestCase):
 
         with mock.patch.object(script_utils, 'get_image_data_iter') as dmock:
             dmock.return_value = cStringIO("TEST_IMAGE")
-            executor.begin_processing(self.task.task_id)
-            image_path = os.path.join(self.test_dir, self.image.image_id)
-            tmp_image_path = os.path.join(self.work_dir,
-                                          "%s.tasks_import" % image_path)
-            self.assertFalse(os.path.exists(tmp_image_path))
-            self.assertTrue(os.path.exists(image_path))
-            self.assertEqual(1, len(list(self.image.locations)))
-            self.assertEqual("file://%s/%s" % (self.test_dir,
-                                               self.image.image_id),
-                             self.image.locations[0]['url'])
+
+            with mock.patch.object(putils, 'trycmd') as tmock:
+                tmock.return_value = (json.dumps({
+                    'format': 'qcow2',
+                }), None)
+
+                executor.begin_processing(self.task.task_id)
+                image_path = os.path.join(self.test_dir, self.image.image_id)
+                tmp_image_path = os.path.join(self.work_dir,
+                                              "%s.tasks_import" % image_path)
+
+                self.assertFalse(os.path.exists(tmp_image_path))
+                self.assertTrue(os.path.exists(image_path))
+                self.assertEqual(1, len(list(self.image.locations)))
+                self.assertEqual("file://%s/%s" % (self.test_dir,
+                                                   self.image.image_id),
+                                 self.image.locations[0]['url'])
 
     def test_import_flow_missing_work_dir(self):
         self.config(engine_mode='serial', group='taskflow_executor')
@@ -151,6 +161,54 @@ class TestImportTask(test_utils.BaseTestCase):
                 self.assertFalse(os.path.exists(tmp_image_path))
                 self.assertTrue(os.path.exists(image_path))
 
+    def test_import_flow_backed_file_import_to_fs(self):
+        self.config(engine_mode='serial', group='taskflow_executor')
+
+        img_factory = mock.MagicMock()
+
+        executor = taskflow_executor.TaskExecutor(
+            self.context,
+            self.task_repo,
+            self.img_repo,
+            img_factory)
+
+        self.task_repo.get.return_value = self.task
+
+        def create_image(*args, **kwargs):
+            kwargs['image_id'] = UUID1
+            return self.img_factory.new_image(*args, **kwargs)
+
+        self.img_repo.get.return_value = self.image
+        img_factory.new_image.side_effect = create_image
+
+        with mock.patch.object(script_utils, 'get_image_data_iter') as dmock:
+            dmock.return_value = cStringIO("TEST_IMAGE")
+
+            with mock.patch.object(putils, 'trycmd') as tmock:
+                tmock.return_value = (json.dumps({
+                    'backing-filename': '/etc/password'
+                }), None)
+
+                with mock.patch.object(import_flow._ImportToFS,
+                                       'revert') as rmock:
+                    self.assertRaises(RuntimeError,
+                                      executor.begin_processing,
+                                      self.task.task_id)
+                    self.assertTrue(rmock.called)
+                    self.assertIsInstance(rmock.call_args[1]['result'],
+                                          failure.Failure)
+
+                    image_path = os.path.join(self.test_dir,
+                                              self.image.image_id)
+
+                    fname = "%s.tasks_import" % image_path
+                    tmp_image_path = os.path.join(self.work_dir, fname)
+
+                    self.assertFalse(os.path.exists(tmp_image_path))
+                    # Note(sabari): The image should not have been uploaded to
+                    # the store as the flow failed before ImportToStore Task.
+                    self.assertFalse(os.path.exists(image_path))
+
     def test_import_flow_revert(self):
         self.config(engine_mode='serial',
                     group='taskflow_executor')
@@ -175,20 +233,31 @@ class TestImportTask(test_utils.BaseTestCase):
         with mock.patch.object(script_utils, 'get_image_data_iter') as dmock:
             dmock.return_value = cStringIO("TEST_IMAGE")
 
-            with mock.patch.object(import_flow, "_get_import_flows") as imock:
-                imock.return_value = (x for x in [_ErrorTask()])
-                self.assertRaises(RuntimeError,
-                                  executor.begin_processing, self.task.task_id)
-                image_path = os.path.join(self.test_dir, self.image.image_id)
-                tmp_image_path = os.path.join(self.work_dir,
-                                              "%s.tasks_import" % image_path)
-                self.assertFalse(os.path.exists(tmp_image_path))
-
-                # NOTE(flaper87): Eventually, we want this to be assertTrue.
-                # The current issue is there's no way to tell taskflow to
-                # continue on failures. That is, revert the subflow but keep
-                # executing the parent flow. Under discussion/development.
-                self.assertFalse(os.path.exists(image_path))
+            with mock.patch.object(putils, 'trycmd') as tmock:
+                tmock.return_value = (json.dumps({
+                    'format': 'qcow2',
+                }), None)
+
+                with mock.patch.object(import_flow,
+                                       "_get_import_flows") as imock:
+                    imock.return_value = (x for x in [_ErrorTask()])
+                    self.assertRaises(RuntimeError,
+                                      executor.begin_processing,
+                                      self.task.task_id)
+
+                    image_path = os.path.join(self.test_dir,
+                                              self.image.image_id)
+                    tmp_image_path = os.path.join(self.work_dir,
+                                                  ("%s.tasks_import" %
+                                                   image_path))
+                    self.assertFalse(os.path.exists(tmp_image_path))
+
+                    # NOTE(flaper87): Eventually, we want this to be assertTrue
+                    # The current issue is there's no way to tell taskflow to
+                    # continue on failures. That is, revert the subflow but
+                    # keep executing the parent flow. Under
+                    # discussion/development.
+                    self.assertFalse(os.path.exists(image_path))
 
     def test_import_flow_no_import_flows(self):
         self.config(engine_mode='serial',
@@ -271,15 +340,20 @@ class TestImportTask(test_utils.BaseTestCase):
         with mock.patch.object(script_utils, 'get_image_data_iter') as dmock:
             dmock.return_value = "test"
 
-            image_id = UUID1
-            path = import_fs.execute(image_id)
-            reader, size = glance_store.get_from_backend(path)
-            self.assertEqual(4, size)
-            self.assertEqual(dmock.return_value, "".join(reader))
+            with mock.patch.object(putils, 'trycmd') as tmock:
+                tmock.return_value = (json.dumps({
+                    'format': 'qcow2',
+                }), None)
+
+                image_id = UUID1
+                path = import_fs.execute(image_id)
+                reader, size = glance_store.get_from_backend(path)
+                self.assertEqual(4, size)
+                self.assertEqual(dmock.return_value, "".join(reader))
 
-            image_path = os.path.join(self.work_dir, image_id)
-            tmp_image_path = os.path.join(self.work_dir, image_path)
-            self.assertTrue(os.path.exists(tmp_image_path))
+                image_path = os.path.join(self.work_dir, image_id)
+                tmp_image_path = os.path.join(self.work_dir, image_path)
+                self.assertTrue(os.path.exists(tmp_image_path))
 
     def test_delete_from_fs(self):
         delete_fs = import_flow._DeleteFromFS(self.task.task_id,
-- 
cgit v0.10.2