{"sha":"06a18202ab52c64803f044b8f848ed1c160905d2","node_id":"C_kwDOACDildoAKDA2YTE4MjAyYWI1MmM2NDgwM2YwNDRiOGY4NDhlZDFjMTYwOTA1ZDI","commit":{"author":{"name":"Guillaume Espanel","email":"guillaume.espanel.ext@ovhcloud.com","date":"2023-01-25T10:53:09Z"},"committer":{"name":"Abhishek Kekane","email":"akekane@redhat.com","date":"2023-02-07T16:49:15Z"},"message":"Limit CaptureRegion sizes in format_inspector for VMDK and VHDX\n\nVMDK:\nWhen parsing a VMDK file to calculate its size, the format_inspector\ndetermines the location of the Descriptor section by reading two\nuint64 from the headers of the file and uses them to create the\ndescriptor CaptureRegion.\n\nIt would be possible to craft a VMDK file that commands the\nformat_inspector to create a very big CaptureRegion, thus exhausting\nresources on the glance-api process.\n\nThis patch binds the beginning of the descriptor to 0x200 and limits\nthe size of the CaptureRegion to 1MB, similar to how the VMDK\ndescriptor is parsed by qemu.\n\nVHDX:\nIt is a bit more involved, but similar: when looking for the\nVIRTUAL_DISK_SIZE metadata, the format_inspector was creating an\nunbounded CaptureRegion.\n\nIn the same way as it seems to be done in Qemu, we now limit the upper\nbound of this CaptureRegion.\n\nCloses-Bug: #2006490\nChange-Id: I3ec5a33df20e1cfb6673f4ff1c7c91aacd065532\n(cherry picked from commit d4d33ee30f303f783c0640cd72acb31b313e1164)","tree":{"sha":"3ffd1503a2c52726f020a7330ce91dea92472bc5","url":"https://api.github.com/repos/openstack/glance/git/trees/3ffd1503a2c52726f020a7330ce91dea92472bc5"},"url":"https://api.github.com/repos/openstack/glance/git/commits/06a18202ab52c64803f044b8f848ed1c160905d2","comment_count":0,"verification":{"verified":false,"reason":"unsigned","signature":null,"payload":null,"verified_at":null}},"url":"https://api.github.com/repos/openstack/glance/commits/06a18202ab52c64803f044b8f848ed1c160905d2","html_url":"https://github.com/openstack/glance/commit/06a18202ab52c64803f044b8f848ed1c160905d2","comments_url":"https://api.github.com/repos/openstack/glance/commits/06a18202ab52c64803f044b8f848ed1c160905d2/comments","author":{"login":"quatre","id":262623,"node_id":"MDQ6VXNlcjI2MjYyMw==","avatar_url":"https://avatars.githubusercontent.com/u/262623?v=4","gravatar_id":"","url":"https://api.github.com/users/quatre","html_url":"https://github.com/quatre","followers_url":"https://api.github.com/users/quatre/followers","following_url":"https://api.github.com/users/quatre/following{/other_user}","gists_url":"https://api.github.com/users/quatre/gists{/gist_id}","starred_url":"https://api.github.com/users/quatre/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/quatre/subscriptions","organizations_url":"https://api.github.com/users/quatre/orgs","repos_url":"https://api.github.com/users/quatre/repos","events_url":"https://api.github.com/users/quatre/events{/privacy}","received_events_url":"https://api.github.com/users/quatre/received_events","type":"User","user_view_type":"public","site_admin":false},"committer":{"login":"konan-abhi","id":46881737,"node_id":"MDQ6VXNlcjQ2ODgxNzM3","avatar_url":"https://avatars.githubusercontent.com/u/46881737?v=4","gravatar_id":"","url":"https://api.github.com/users/konan-abhi","html_url":"https://github.com/konan-abhi","followers_url":"https://api.github.com/users/konan-abhi/followers","following_url":"https://api.github.com/users/konan-abhi/following{/other_user}","gists_url":"https://api.github.com/users/konan-abhi/gists{/gist_id}","starred_url":"https://api.github.com/users/konan-abhi/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/konan-abhi/subscriptions","organizations_url":"https://api.github.com/users/konan-abhi/orgs","repos_url":"https://api.github.com/users/konan-abhi/repos","events_url":"https://api.github.com/users/konan-abhi/events{/privacy}","received_events_url":"https://api.github.com/users/konan-abhi/received_events","type":"User","user_view_type":"public","site_admin":false},"parents":[{"sha":"b1e5292248fefb7cd1ff4292c9b94d336f4cf73b","url":"https://api.github.com/repos/openstack/glance/commits/b1e5292248fefb7cd1ff4292c9b94d336f4cf73b","html_url":"https://github.com/openstack/glance/commit/b1e5292248fefb7cd1ff4292c9b94d336f4cf73b"}],"stats":{"total":142,"additions":139,"deletions":3},"files":[{"sha":"550cceadbb2146280c4010820e2e660c5077df2c","filename":"glance/common/format_inspector.py","status":"modified","additions":19,"deletions":3,"changes":22,"blob_url":"https://github.com/openstack/glance/blob/06a18202ab52c64803f044b8f848ed1c160905d2/glance%2Fcommon%2Fformat_inspector.py","raw_url":"https://github.com/openstack/glance/raw/06a18202ab52c64803f044b8f848ed1c160905d2/glance%2Fcommon%2Fformat_inspector.py","contents_url":"https://api.github.com/repos/openstack/glance/contents/glance%2Fcommon%2Fformat_inspector.py?ref=06a18202ab52c64803f044b8f848ed1c160905d2","patch":"@@ -345,6 +345,7 @@ class VHDXInspector(FileInspector):\n     \"\"\"\n     METAREGION = '8B7CA206-4790-4B9A-B8FE-575F050F886E'\n     VIRTUAL_DISK_SIZE = '2FA54224-CD1B-4876-B211-5DBED83BF4B8'\n+    VHDX_METADATA_TABLE_MAX_SIZE = 32 * 2048  # From qemu\n \n     def __init__(self, *a, **k):\n         super(VHDXInspector, self).__init__(*a, **k)\n@@ -459,6 +460,8 @@ def _find_meta_entry(self, desired_guid):\n                 item_offset, item_length, _reserved = struct.unpack(\n                     '<III',\n                     meta_buffer[entry_offset + 16:entry_offset + 28])\n+                item_length = min(item_length,\n+                                  self.VHDX_METADATA_TABLE_MAX_SIZE)\n                 self.region('metadata').length = len(meta_buffer)\n                 self._log.debug('Found entry at offset %x', item_offset)\n                 # Metadata item offset is from the beginning of the metadata\n@@ -516,6 +519,12 @@ class VMDKInspector(FileInspector):\n     variable number of 512 byte sectors, but is just text defining the\n     layout of the disk.\n     \"\"\"\n+\n+    # The beginning and max size of the descriptor is also hardcoded in Qemu\n+    # at 0x200 and 1MB - 1\n+    DESC_OFFSET = 0x200\n+    DESC_MAX_SIZE = (1 << 20) - 1\n+\n     def __init__(self, *a, **k):\n         super(VMDKInspector, self).__init__(*a, **k)\n         self.new_region('header', CaptureRegion(0, 512))\n@@ -532,15 +541,22 @@ def post_process(self):\n \n         if sig != b'KDMV':\n             raise ImageFormatError('Signature KDMV not found: %r' % sig)\n-            return\n \n         if ver not in (1, 2, 3):\n             raise ImageFormatError('Unsupported format version %i' % ver)\n-            return\n+\n+        # Since we parse both desc_sec and desc_num (the location of the\n+        # VMDK's descriptor, expressed in 512 bytes sectors) we enforce a\n+        # check on the bounds to create a reasonable CaptureRegion. This\n+        # is similar to how it's done in qemu.\n+        desc_offset = desc_sec * 512\n+        desc_size = min(desc_num * 512, self.DESC_MAX_SIZE)\n+        if desc_offset != self.DESC_OFFSET:\n+            raise ImageFormatError(\"Wrong descriptor location\")\n \n         if not self.has_region('descriptor'):\n             self.new_region('descriptor', CaptureRegion(\n-                desc_sec * 512, desc_num * 512))\n+                desc_offset, desc_size))\n \n     @property\n     def format_match(self):"},{"sha":"db6a9830bd4c19de9e3a76ca51e13eb1b4e78f4a","filename":"glance/tests/unit/common/test_format_inspector.py","status":"modified","additions":120,"deletions":0,"changes":120,"blob_url":"https://github.com/openstack/glance/blob/06a18202ab52c64803f044b8f848ed1c160905d2/glance%2Ftests%2Funit%2Fcommon%2Ftest_format_inspector.py","raw_url":"https://github.com/openstack/glance/raw/06a18202ab52c64803f044b8f848ed1c160905d2/glance%2Ftests%2Funit%2Fcommon%2Ftest_format_inspector.py","contents_url":"https://api.github.com/repos/openstack/glance/contents/glance%2Ftests%2Funit%2Fcommon%2Ftest_format_inspector.py?ref=06a18202ab52c64803f044b8f848ed1c160905d2","patch":"@@ -16,6 +16,7 @@\n import io\n import os\n import re\n+import struct\n import subprocess\n import tempfile\n from unittest import mock\n@@ -63,6 +64,28 @@ def _create_img(self, fmt, size):\n             shell=True)\n         return fn\n \n+    def _create_allocated_vmdk(self, size_mb):\n+        # We need a \"big\" VMDK file to exercise some parts of the code of the\n+        # format_inspector. A way to create one is to first create an empty\n+        # file, and then to convert it with the -S 0 option.\n+        fn = tempfile.mktemp(prefix='glance-unittest-formatinspector-',\n+                             suffix='.vmdk')\n+        self._created_files.append(fn)\n+        zeroes = tempfile.mktemp(prefix='glance-unittest-formatinspector-',\n+                                 suffix='.zero')\n+        self._created_files.append(zeroes)\n+\n+        # Create an empty file\n+        subprocess.check_output(\n+            'dd if=/dev/zero of=%s bs=1M count=%i' % (zeroes, size_mb),\n+            shell=True)\n+\n+        # Convert it to VMDK\n+        subprocess.check_output(\n+            'qemu-img convert -f raw -O vmdk -S 0 %s %s' % (zeroes, fn),\n+            shell=True)\n+        return fn\n+\n     def _test_format_at_block_size(self, format_name, img, block_size):\n         fmt = format_inspector.get_inspector(format_name)()\n         self.assertIsNotNone(fmt,\n@@ -119,6 +142,64 @@ def test_vhdx(self):\n     def test_vmdk(self):\n         self._test_format('vmdk')\n \n+    def test_vmdk_bad_descriptor_offset(self):\n+        format_name = 'vmdk'\n+        image_size = 10 * units.Mi\n+        descriptorOffsetAddr = 0x1c\n+        BAD_ADDRESS = 0x400\n+        img = self._create_img(format_name, image_size)\n+\n+        # Corrupt the header\n+        fd = open(img, 'r+b')\n+        fd.seek(descriptorOffsetAddr)\n+        fd.write(struct.pack('<Q', BAD_ADDRESS // 512))\n+        fd.close()\n+\n+        # Read the format in various sizes, some of which will read whole\n+        # sections in a single read, others will be completely unaligned, etc.\n+        for block_size in (64 * units.Ki, 512, 17, 1 * units.Mi):\n+            fmt = self._test_format_at_block_size(format_name, img, block_size)\n+            self.assertTrue(fmt.format_match,\n+                            'Failed to match %s at size %i block %i' % (\n+                                format_name, image_size, block_size))\n+            self.assertEqual(0, fmt.virtual_size,\n+                             ('Calculated a virtual size for a corrupt %s at '\n+                              'size %i block %i') % (format_name, image_size,\n+                                                     block_size))\n+\n+    def test_vmdk_bad_descriptor_mem_limit(self):\n+        format_name = 'vmdk'\n+        image_size = 5 * units.Mi\n+        virtual_size = 5 * units.Mi\n+        descriptorOffsetAddr = 0x1c\n+        descriptorSizeAddr = descriptorOffsetAddr + 8\n+        twoMBInSectors = (2 << 20) // 512\n+        # We need a big VMDK because otherwise we will not have enough data to\n+        # fill-up the CaptureRegion.\n+        img = self._create_allocated_vmdk(image_size // units.Mi)\n+\n+        # Corrupt the end of descriptor address so it \"ends\" at 2MB\n+        fd = open(img, 'r+b')\n+        fd.seek(descriptorSizeAddr)\n+        fd.write(struct.pack('<Q', twoMBInSectors))\n+        fd.close()\n+\n+        # Read the format in various sizes, some of which will read whole\n+        # sections in a single read, others will be completely unaligned, etc.\n+        for block_size in (64 * units.Ki, 512, 17, 1 * units.Mi):\n+            fmt = self._test_format_at_block_size(format_name, img, block_size)\n+            self.assertTrue(fmt.format_match,\n+                            'Failed to match %s at size %i block %i' % (\n+                                format_name, image_size, block_size))\n+            self.assertEqual(virtual_size, fmt.virtual_size,\n+                             ('Failed to calculate size for %s at size %i '\n+                              'block %i') % (format_name, image_size,\n+                                             block_size))\n+            memory = sum(fmt.context_info.values())\n+            self.assertLess(memory, 1.5 * units.Mi,\n+                            'Format used more than 1.5MiB of memory: %s' % (\n+                                fmt.context_info))\n+\n     def test_vdi(self):\n         self._test_format('vdi')\n \n@@ -275,3 +356,42 @@ def test_get_inspector(self):\n         self.assertEqual(format_inspector.QcowInspector,\n                          format_inspector.get_inspector('qcow2'))\n         self.assertIsNone(format_inspector.get_inspector('foo'))\n+\n+\n+class TestFormatInspectorsTargeted(test_utils.BaseTestCase):\n+    def _make_vhd_meta(self, guid_raw, item_length):\n+        # Meta region header, padded to 32 bytes\n+        data = struct.pack('<8sHH', b'metadata', 0, 1)\n+        data += b'0' * 20\n+\n+        # Metadata table entry, 16-byte GUID, 12-byte information,\n+        # padded to 32-bytes\n+        data += guid_raw\n+        data += struct.pack('<III', 256, item_length, 0)\n+        data += b'0' * 6\n+\n+        return data\n+\n+    def test_vhd_table_over_limit(self):\n+        ins = format_inspector.VHDXInspector()\n+        meta = format_inspector.CaptureRegion(0, 0)\n+        desired = b'012345678ABCDEF0'\n+        # This is a poorly-crafted image that specifies a larger table size\n+        # than is allowed\n+        meta.data = self._make_vhd_meta(desired, 33 * 2048)\n+        ins.new_region('metadata', meta)\n+        new_region = ins._find_meta_entry(ins._guid(desired))\n+        # Make sure we clamp to our limit of 32 * 2048\n+        self.assertEqual(\n+            format_inspector.VHDXInspector.VHDX_METADATA_TABLE_MAX_SIZE,\n+            new_region.length)\n+\n+    def test_vhd_table_under_limit(self):\n+        ins = format_inspector.VHDXInspector()\n+        meta = format_inspector.CaptureRegion(0, 0)\n+        desired = b'012345678ABCDEF0'\n+        meta.data = self._make_vhd_meta(desired, 16 * 2048)\n+        ins.new_region('metadata', meta)\n+        new_region = ins._find_meta_entry(ins._guid(desired))\n+        # Table size was under the limit, make sure we get it back\n+        self.assertEqual(16 * 2048, new_region.length)"}]}