--- /dev/null
+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
+
projects / gentoo.git / commitdiff