You can not select more than 25 topics
Topics must start with a letter or number, can include dashes ('-') and can be up to 35 characters long.
261 lines
12 KiB
261 lines
12 KiB
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
|
|
|