Merge "Reject unsafe qcow and vmdk files" into stable/2023.1

glance/async_/flows/plugins/image_conversion.py

@@ -25,6 +25,7 @@ from taskflow.patterns import linear_flow as lf
 from taskflow import task
 
 from glance.async_ import utils
+from glance.common import format_inspector
 from glance.i18n import _, _LI
 
 LOG = logging.getLogger(__name__)
@@ -87,8 +88,40 @@ class _ConvertImage(task.Task):
                                              'target': target_format}
         self.dest_path = dest_path
 
+        source_format = action.image_disk_format
+        inspector_cls = format_inspector.get_inspector(source_format)
+        if not inspector_cls:
+            # We cannot convert from disk_format types that qemu-img doesn't
+            # support (like iso, ploop, etc). The ones it supports overlaps
+            # with the ones we have inspectors for, so reject conversion for
+            # any format we don't have an inspector for.
+            raise RuntimeError(
+                'Unable to convert from format %s' % source_format)
+
+        # Use our own cautious inspector module (if we have one for this
+        # format) to make sure a file is the format the submitter claimed
+        # it is and that it passes some basic safety checks _before_ we run
+        # qemu-img on it.
+        # See https://bugs.launchpad.net/nova/+bug/2059809 for details.
+        try:
+            inspector = inspector_cls.from_file(src_path)
+            if not inspector.safety_check():
+                LOG.error('Image failed %s safety check; aborting conversion',
+                          source_format)
+                raise RuntimeError('Image has disallowed configuration')
+        except RuntimeError:
+            raise
+        except format_inspector.ImageFormatError as e:
+            LOG.error('Image claimed to be %s format failed format '
+                      'inspection: %s', source_format, e)
+            raise RuntimeError('Image format detection failed')
+        except Exception as e:
+            LOG.exception('Unknown error inspecting image format: %s', e)
+            raise RuntimeError('Unable to inspect image')
+
         try:
             stdout, stderr = putils.trycmd("qemu-img", "info",
+                                           "-f", source_format,
                                            "--output=json",
                                            src_path,
                                            prlimit=utils.QEMU_IMG_PROC_LIMITS,
@@ -105,13 +138,10 @@ class _ConvertImage(task.Task):
             raise RuntimeError(stderr)
 
         metadata = json.loads(stdout)
-        try:
-            source_format = metadata['format']
-        except KeyError:
-            msg = ("Failed to do introspection as part of image "
-                   "conversion for %(iid)s: Source format not reported")
-            LOG.error(msg, {'iid': self.image_id})
-            raise RuntimeError(msg)
+        if metadata.get('format') != source_format:
+            LOG.error('Image claiming to be %s reported as %s by qemu-img',
+                      source_format, metadata.get('format', 'unknown'))
+            raise RuntimeError('Image metadata disagrees about format')
 
         virtual_size = metadata.get('virtual-size', 0)
         action.set_image_attribute(virtual_size=virtual_size)

glance/tests/unit/async_/flows/plugins/test_image_conversion.py

@@ -13,6 +13,7 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
+import fixtures
 import json
 import os
 from unittest import mock
@@ -24,6 +25,7 @@ from oslo_config import cfg
 import glance.async_.flows.api_image_import as import_flow
 import glance.async_.flows.plugins.image_conversion as image_conversion
 from glance.async_ import utils as async_utils
+from glance.common import format_inspector
 from glance.common import utils
 from glance import domain
 from glance import gateway
@@ -90,6 +92,11 @@ class TestConvertImageTask(test_utils.BaseTestCase):
                                                        self.image_id,
                                                        self.task.task_id)
 
+        self.inspector_mock = mock.MagicMock()
+        self.useFixture(fixtures.MockPatch('glance.common.format_inspector.'
+                                           'get_inspector',
+                                           self.inspector_mock))
+
     @mock.patch.object(os, 'stat')
     @mock.patch.object(os, 'remove')
     def test_image_convert_success(self, mock_os_remove, mock_os_stat):
@@ -104,7 +111,7 @@ class TestConvertImageTask(test_utils.BaseTestCase):
         image = mock.MagicMock(image_id=self.image_id, virtual_size=None,
                                extra_properties={
                                    'os_glance_import_task': self.task.task_id},
-                               disk_format='qcow2')
+                               disk_format='raw')
         self.img_repo.get.return_value = image
 
         with mock.patch.object(processutils, 'execute') as exc_mock:
@@ -126,7 +133,7 @@ class TestConvertImageTask(test_utils.BaseTestCase):
         self.assertEqual(456, image.virtual_size)
         self.assertEqual(123, image.size)
 
-    def _setup_image_convert_info_fail(self):
+    def _setup_image_convert_info_fail(self, disk_format='qcow2'):
         image_convert = image_conversion._ConvertImage(self.context,
                                                        self.task.task_id,
                                                        self.task_type,
@@ -136,7 +143,7 @@ class TestConvertImageTask(test_utils.BaseTestCase):
         image = mock.MagicMock(image_id=self.image_id, virtual_size=None,
                                extra_properties={
                                    'os_glance_import_task': self.task.task_id},
-                               disk_format='qcow2')
+                               disk_format=disk_format)
         self.img_repo.get.return_value = image
         return image_convert
 
@@ -148,6 +155,7 @@ class TestConvertImageTask(test_utils.BaseTestCase):
                               convert.execute, 'file:///test/path.raw')
             exc_mock.assert_called_once_with(
                 'qemu-img', 'info',
+                '-f', 'qcow2',
                 '--output=json',
                 '/test/path.raw',
                 prlimit=async_utils.QEMU_IMG_PROC_LIMITS,
@@ -164,6 +172,7 @@ class TestConvertImageTask(test_utils.BaseTestCase):
                               convert.execute, 'file:///test/path.raw')
             exc_mock.assert_called_once_with(
                 'qemu-img', 'info',
+                '-f', 'qcow2',
                 '--output=json',
                 '/test/path.raw',
                 prlimit=async_utils.QEMU_IMG_PROC_LIMITS,
@@ -200,6 +209,36 @@ class TestConvertImageTask(test_utils.BaseTestCase):
             self.assertEqual('QCOW images with data-file set are not allowed',
                              str(e))
 
+    def test_image_convert_no_inspector_match(self):
+        convert = self._setup_image_convert_info_fail()
+        self.inspector_mock.return_value = None
+        self.assertRaisesRegex(RuntimeError,
+                               'Unable to convert from format',
+                               convert.execute, 'file:///test/path.hpfs')
+
+    def test_image_convert_fails_inspection_safety_check(self):
+        convert = self._setup_image_convert_info_fail()
+        inspector = self.inspector_mock.return_value.from_file.return_value
+        inspector.safety_check.return_value = False
+        self.assertRaisesRegex(RuntimeError,
+                               'Image has disallowed configuration',
+                               convert.execute, 'file:///test/path.qcow')
+
+    def test_image_convert_fails_inspection_format_check(self):
+        convert = self._setup_image_convert_info_fail()
+        self.inspector_mock.return_value.from_file.side_effect = (
+            format_inspector.ImageFormatError())
+        self.assertRaisesRegex(RuntimeError,
+                               'Image format detection failed',
+                               convert.execute, 'file:///test/path.qcow')
+
+    def test_image_convert_fails_inspection_error(self):
+        convert = self._setup_image_convert_info_fail()
+        self.inspector_mock.return_value.from_file.side_effect = ValueError
+        self.assertRaisesRegex(RuntimeError,
+                               'Unable to inspect image',
+                               convert.execute, 'file:///test/path.qcow')
+
     def _test_image_convert_invalid_vmdk(self):
         data = {'format': 'vmdk',
                 'format-specific': {
@@ -207,7 +246,7 @@ class TestConvertImageTask(test_utils.BaseTestCase):
                         'create-type': 'monolithicFlat',
                     }}}
 
-        convert = self._setup_image_convert_info_fail()
+        convert = self._setup_image_convert_info_fail(disk_format='vmdk')
         with mock.patch.object(processutils, 'execute') as exc_mock:
             exc_mock.return_value = json.dumps(data), ''
             convert.execute('file:///test/path.vmdk')
@@ -236,7 +275,7 @@ class TestConvertImageTask(test_utils.BaseTestCase):
                               self._test_image_convert_invalid_vmdk)
 
     def test_image_convert_fails(self):
-        convert = self._setup_image_convert_info_fail()
+        convert = self._setup_image_convert_info_fail(disk_format='raw')
         with mock.patch.object(processutils, 'execute') as exc_mock:
             exc_mock.side_effect = [('{"format":"raw"}', ''),
                                     OSError('convert_fail')]
@@ -244,6 +283,7 @@ class TestConvertImageTask(test_utils.BaseTestCase):
                               convert.execute, 'file:///test/path.raw')
             exc_mock.assert_has_calls(
                 [mock.call('qemu-img', 'info',
+                           '-f', 'raw',
                            '--output=json',
                            '/test/path.raw',
                            prlimit=async_utils.QEMU_IMG_PROC_LIMITS,
@@ -256,7 +296,7 @@ class TestConvertImageTask(test_utils.BaseTestCase):
         self.img_repo.save.assert_not_called()
 
     def test_image_convert_reports_fail(self):
-        convert = self._setup_image_convert_info_fail()
+        convert = self._setup_image_convert_info_fail(disk_format='raw')
         with mock.patch.object(processutils, 'execute') as exc_mock:
             exc_mock.side_effect = [('{"format":"raw"}', ''),
                                     ('', 'some error')]
@@ -264,6 +304,7 @@ class TestConvertImageTask(test_utils.BaseTestCase):
                               convert.execute, 'file:///test/path.raw')
             exc_mock.assert_has_calls(
                 [mock.call('qemu-img', 'info',
+                           '-f', 'raw',
                            '--output=json',
                            '/test/path.raw',
                            prlimit=async_utils.QEMU_IMG_PROC_LIMITS,
@@ -281,9 +322,10 @@ class TestConvertImageTask(test_utils.BaseTestCase):
             exc_mock.return_value = ('{}', '')
             exc = self.assertRaises(RuntimeError,
                                     convert.execute, 'file:///test/path.raw')
-            self.assertIn('Source format not reported', str(exc))
+            self.assertIn('Image metadata disagrees about format', str(exc))
             exc_mock.assert_called_once_with(
                 'qemu-img', 'info',
+                '-f', 'qcow2',
                 '--output=json',
                 '/test/path.raw',
                 prlimit=async_utils.QEMU_IMG_PROC_LIMITS,
@@ -301,6 +343,7 @@ class TestConvertImageTask(test_utils.BaseTestCase):
             # Make sure we only called qemu-img for inspection, not conversion
             exc_mock.assert_called_once_with(
                 'qemu-img', 'info',
+                '-f', 'qcow2',
                 '--output=json',
                 '/test/path.qcow',
                 prlimit=async_utils.QEMU_IMG_PROC_LIMITS,