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
This commit is contained in:
Flavio Percoco
parent
1e88cc4d4b
commit
d529863a1e
|
|
@ -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):
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
Loading…
Reference in New Issue