From d529863a1e8d2307526bdb395b4aebe97f81603d Mon Sep 17 00:00:00 2001 From: Flavio Percoco Date: Thu, 9 Jul 2015 14:44:04 +0200 Subject: [PATCH] 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 --- glance/async/flows/base_import.py | 26 ++++ glance/tests/unit/async/flows/test_import.py | 135 ++++++++++++++----- 2 files changed, 130 insertions(+), 31 deletions(-) diff --git a/glance/async/flows/base_import.py b/glance/async/flows/base_import.py index 3416ce468b..26a09e58c6 100644 --- a/glance/async/flows/base_import.py +++ b/glance/async/flows/base_import.py @@ -13,13 +13,16 @@ # 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 encodeutils +from oslo_utils import excutils import six from stevedore import named from taskflow.patterns import linear_flow as lf @@ -147,6 +150,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, **kwargs): diff --git a/glance/tests/unit/async/flows/test_import.py b/glance/tests/unit/async/flows/test_import.py index 03d37c503d..7221e853d2 100644 --- a/glance/tests/unit/async/flows/test_import.py +++ b/glance/tests/unit/async/flows/test_import.py @@ -13,10 +13,12 @@ # License for the specific language governing permissions and limitations # under the License. +import json import mock import os import glance_store +from oslo_concurrency import processutils as putils from oslo_config import cfg from six.moves import cStringIO from six.moves import urllib @@ -107,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') @@ -190,6 +199,54 @@ class TestImportTask(test_utils.BaseTestCase): # the store as the flow failed before ImportToStore Task. self.assertFalse(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') @@ -214,20 +271,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)) + with mock.patch.object(putils, 'trycmd') as tmock: + tmock.return_value = (json.dumps({ + 'format': 'qcow2', + }), None) - # 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(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', @@ -310,15 +378,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_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_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)) def test_delete_from_fs(self): delete_fs = import_flow._DeleteFromFS(self.task.task_id,