commit f088ce0d197abd41f5790f542b213c106621cb16 Author: Dan Smith Date: Thu Nov 10 11:25:49 2022 -0800 Enforce image safety during image_conversion This does two things: 1. It makes us check that the QCOW backing_file is unset on those types of images. Nova and Cinder do this already to prevent an arbitrary (and trivial to accomplish) host file exposure exploit. 2. It makes us restrict VMDK files to only allowed subtypes. These files can name arbitrary files on disk as extents, providing the same sort of attack. Default that list to just the types we believe are actually useful for openstack, and which are monolithic. The configuration option to specify allowed subtypes is added in glance's config and not in the import options so that we can extend this check later to image ingest. The format_inspector can tell us what the type and subtype is, and we could reject those images early and even in the case where image_conversion is not enabled. This is primarily done for the interoperable import image_conversion task, but also for the legacy convert (which does its image inspection in base_import). The latter is likely irrelevant at this point, but it is included for completeness. The legacy convert also already had the qcow backing file check, but no test for it, so this adds that as well. Change-Id: Idf561f6306cebf756c787d8eefdc452ce44bd5e0 diff --git a/glance/async_/flows/base_import.py b/glance/async_/flows/base_import.py index e6bb526b4..564016ba2 100644 --- a/glance/async_/flows/base_import.py +++ b/glance/async_/flows/base_import.py @@ -181,6 +181,23 @@ class _ImportToFS(task.Task): 'bfile': backing_file} raise RuntimeError(msg) + if metadata.get('format') == 'vmdk': + create_type = metadata.get( + 'format-specific', {}).get( + 'data', {}).get('create-type') + allowed = CONF.image_format.vmdk_allowed_types + if not create_type: + raise RuntimeError(_('Unable to determine VMDK create-type')) + if not len(allowed): + LOG.warning(_('Refusing to process VMDK file as ' + 'vmdk_allowed_types is empty')) + raise RuntimeError(_('Invalid VMDK create-type specified')) + if create_type not in allowed: + LOG.warning(_('Refusing to process VMDK file with create-type ' + 'of %r which is not in allowed set of: %s'), + create_type, ','.join(allowed)) + raise RuntimeError(_('Invalid VMDK create-type specified')) + return path def revert(self, image_id, result, **kwargs): diff --git a/glance/async_/flows/plugins/image_conversion.py b/glance/async_/flows/plugins/image_conversion.py index 32c7b7fe0..03b7d4fab 100644 --- a/glance/async_/flows/plugins/image_conversion.py +++ b/glance/async_/flows/plugins/image_conversion.py @@ -116,6 +116,28 @@ class _ConvertImage(task.Task): virtual_size = metadata.get('virtual-size', 0) action.set_image_attribute(virtual_size=virtual_size) + if 'backing-filename' in metadata: + LOG.warning('Refusing to process QCOW image with a backing file') + raise RuntimeError( + 'QCOW images with backing files are not allowed') + + if metadata.get('format') == 'vmdk': + create_type = metadata.get( + 'format-specific', {}).get( + 'data', {}).get('create-type') + allowed = CONF.image_format.vmdk_allowed_types + if not create_type: + raise RuntimeError(_('Unable to determine VMDK create-type')) + if not len(allowed): + LOG.warning(_('Refusing to process VMDK file as ' + 'vmdk_allowed_types is empty')) + raise RuntimeError(_('Invalid VMDK create-type specified')) + if create_type not in allowed: + LOG.warning(_('Refusing to process VMDK file with create-type ' + 'of %r which is not in allowed set of: %s'), + create_type, ','.join(allowed)) + raise RuntimeError(_('Invalid VMDK create-type specified')) + if source_format == target_format: LOG.debug("Source is already in target format, " "not doing conversion for %s", self.image_id) diff --git a/glance/common/config.py b/glance/common/config.py index dd7e1b6e9..7891daccf 100644 --- a/glance/common/config.py +++ b/glance/common/config.py @@ -99,6 +99,18 @@ image_format_opts = [ "image attribute"), deprecated_opts=[cfg.DeprecatedOpt('disk_formats', group='DEFAULT')]), + cfg.ListOpt('vmdk_allowed_types', + default=['streamOptimized', 'monolithicSparse'], + help=_("A list of strings describing allowed VMDK " + "'create-type' subformats that will be allowed. " + "This is recommended to only include " + "single-file-with-sparse-header variants to avoid " + "potential host file exposure due to processing named " + "extents. If this list is empty, then no VDMK image " + "types allowed. Note that this is currently only " + "checked during image conversion (if enabled), and " + "limits the types of VMDK images we will convert " + "from.")), ] task_opts = [ cfg.IntOpt('task_time_to_live', diff --git a/glance/tests/unit/async_/flows/plugins/test_image_conversion.py b/glance/tests/unit/async_/flows/plugins/test_image_conversion.py index 77d68acf8..0cffc404c 100644 --- a/glance/tests/unit/async_/flows/plugins/test_image_conversion.py +++ b/glance/tests/unit/async_/flows/plugins/test_image_conversion.py @@ -172,6 +172,52 @@ class TestConvertImageTask(test_utils.BaseTestCase): # Make sure we did not update the image self.img_repo.save.assert_not_called() + def test_image_convert_invalid_qcow(self): + data = {'format': 'qcow2', + 'backing-filename': '/etc/hosts'} + + convert = self._setup_image_convert_info_fail() + with mock.patch.object(processutils, 'execute') as exc_mock: + exc_mock.return_value = json.dumps(data), '' + e = self.assertRaises(RuntimeError, + convert.execute, 'file:///test/path.qcow') + self.assertEqual('QCOW images with backing files are not allowed', + str(e)) + + def _test_image_convert_invalid_vmdk(self): + data = {'format': 'vmdk', + 'format-specific': { + 'data': { + 'create-type': 'monolithicFlat', + }}} + + convert = self._setup_image_convert_info_fail() + with mock.patch.object(processutils, 'execute') as exc_mock: + exc_mock.return_value = json.dumps(data), '' + convert.execute('file:///test/path.vmdk') + + def test_image_convert_invalid_vmdk(self): + e = self.assertRaises(RuntimeError, + self._test_image_convert_invalid_vmdk) + self.assertEqual('Invalid VMDK create-type specified', str(e)) + + def test_image_convert_valid_vmdk_no_types(self): + with mock.patch.object(CONF.image_format, 'vmdk_allowed_types', + new=[]): + # We make it past the VMDK check and fail because our file + # does not exist + e = self.assertRaises(RuntimeError, + self._test_image_convert_invalid_vmdk) + self.assertEqual('Invalid VMDK create-type specified', str(e)) + + def test_image_convert_valid_vmdk(self): + with mock.patch.object(CONF.image_format, 'vmdk_allowed_types', + new=['monolithicSparse', 'monolithicFlat']): + # We make it past the VMDK check and fail because our file + # does not exist + self.assertRaises(FileNotFoundError, + self._test_image_convert_invalid_vmdk) + def test_image_convert_fails(self): convert = self._setup_image_convert_info_fail() with mock.patch.object(processutils, 'execute') as exc_mock: diff --git a/glance/tests/unit/async_/flows/test_convert.py b/glance/tests/unit/async_/flows/test_convert.py index a00cf718f..1dcd01f59 100644 --- a/glance/tests/unit/async_/flows/test_convert.py +++ b/glance/tests/unit/async_/flows/test_convert.py @@ -120,7 +120,7 @@ class TestImportTask(test_utils.BaseTestCase): rmtree_mock.return_value = None image_convert.revert(image, 'file:///tmp/test') - def test_import_flow_with_convert_and_introspect(self): + def _test_import_flow_with_convert_and_introspect(self, fake_img_info): self.config(engine_mode='serial', group='taskflow_executor') @@ -154,21 +154,9 @@ class TestImportTask(test_utils.BaseTestCase): # what they were supposed to do. assert os.path.exists(args[3].split("file://")[-1]) - return (json.dumps({ - "virtual-size": 10737418240, - "filename": "/tmp/image.qcow2", - "cluster-size": 65536, - "format": "qcow2", - "actual-size": 373030912, - "format-specific": { - "type": "qcow2", - "data": { - "compat": "0.10" - } - }, - "dirty-flag": False - }), None) + return json.dumps(fake_img_info), None + image_path = os.path.join(self.work_dir, self.image.image_id) open("%s.converted" % image_path, 'a').close() return ("", None) @@ -186,7 +174,7 @@ class TestImportTask(test_utils.BaseTestCase): # NOTE(flaper87): Workdir should be empty after all # the tasks have been executed. self.assertEqual([], os.listdir(self.work_dir)) - self.assertEqual('qcow2', image.disk_format) + self.assertEqual(fake_img_info['format'], image.disk_format) self.assertEqual(10737418240, image.virtual_size) # NOTE(hemanthm): Asserting that the source format is passed @@ -200,3 +188,80 @@ class TestImportTask(test_utils.BaseTestCase): # here, hence we fetch the 2nd set of args from the args list. convert_call_args, _ = exc_mock.call_args_list[1] self.assertIn('-f', convert_call_args) + + def test_import_flow_with_convert_and_inspect(self): + img_info = { + "virtual-size": 10737418240, + "filename": "/tmp/image.qcow2", + "cluster-size": 65536, + "format": "qcow2", + "actual-size": 373030912, + "format-specific": { + "type": "qcow2", + "data": { + "compat": "0.10" + } + }, + "dirty-flag": False + } + + self._test_import_flow_with_convert_and_introspect(img_info) + + def test_import_flow_with_convert_and_inspect_qcow_backing(self): + img_info = { + "virtual-size": 10737418240, + "filename": "/tmp/image.qcow2", + "cluster-size": 65536, + "format": "qcow2", + "actual-size": 373030912, + "backing-filename": "/path/to/file", + "format-specific": { + "type": "qcow2", + "data": { + "compat": "0.10" + } + }, + "dirty-flag": False + } + + self.assertRaises(RuntimeError, + self._test_import_flow_with_convert_and_introspect, + img_info) + + def test_import_flow_with_convert_and_inspect_vmdk_disallowed(self): + img_info = { + "virtual-size": 10737418240, + "filename": "/tmp/image.vmdk", + "cluster-size": 65536, + "format": "vmdk", + "actual-size": 373030912, + "format-specific": { + "type": "vmdk", + "data": { + "create-type": "monolithicFlat", + } + }, + "dirty-flag": False + } + + self.assertRaises(RuntimeError, + self._test_import_flow_with_convert_and_introspect, + img_info) + + def test_import_flow_with_convert_and_inspect_vmdk_allowed(self): + img_info = { + "virtual-size": 10737418240, + "filename": "/tmp/image.vmdk", + "cluster-size": 65536, + "format": "vmdk", + "actual-size": 373030912, + "format-specific": { + "type": "vmdk", + "data": { + "create-type": "streamOptimized", + } + }, + "dirty-flag": False + } + + self._test_import_flow_with_convert_and_introspect(img_info)