| 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 |
| |
