diff --git a/ironic/common/image_service.py b/ironic/common/image_service.py index c68be564f7..209f6363da 100644 --- a/ironic/common/image_service.py +++ b/ironic/common/image_service.py @@ -909,8 +909,9 @@ def validate_href(self, image_href): doesn't exist, is in a blocked path, or is not in an allowed path. :returns: Path to image file if it exists and is allowed. """ + # TODO(TheJulia): Validate there are *THREE* slashes in the file path + # URL, otherwise urlparse doesn't split it properly. image_path = urlparse.urlparse(image_href).path - # Check if the path is in the blocklist rpath = os.path.abspath(image_path) diff --git a/ironic/drivers/modules/deploy_utils.py b/ironic/drivers/modules/deploy_utils.py index c1d3de09e9..e0b2a61b9a 100644 --- a/ironic/drivers/modules/deploy_utils.py +++ b/ironic/drivers/modules/deploy_utils.py @@ -1254,6 +1254,11 @@ def _validate_image_url(node, url, secret=False, inspect_image=None, def _cache_and_convert_image(task, instance_info, image_info=None): """Cache an image locally and convert it to RAW if needed. + Caches the supplied image as defined in the request, and converts it + to raw if required. This method should only be called once initial + first pass validation has been performed and can be called on multiple + code paths where the file contents must be downloaded. + :param task: The Taskmanager object related to this action. :param instance_info: The instance_info field being used in association with this method call. @@ -1515,8 +1520,8 @@ def build_instance_info_for_deploy(task): di_info['image_source'] = image_source node.driver_internal_info = di_info if not is_glance_image: - if (image_source.startswith('file://') - or image_download_source == 'local'): + is_file_url = image_source.startswith('file://') + if (is_file_url or image_download_source == 'local'): # In this case, we're explicitly downloading (or copying a file) # hosted locally so IPA can download it directly from Ironic. @@ -1524,7 +1529,14 @@ def build_instance_info_for_deploy(task): # based deploy source since we don't want to, nor should we be in # in the business of copying large numbers of files as it is a # huge performance impact. - + if is_file_url: + # In this case, we need to validate the URL first before + # moving on to _cache_and_convert_image, because it's whole + # existence is to download, checksum, convert, etc. + image_service.FileImageService().validate_href( + image_href=image_source) + # Either the file is local, or the file needs to be downloaded. + # _cache_and_convert_image handles both cases _cache_and_convert_image(task, instance_info) else: # This is the "all other cases" logic for aspects like the user diff --git a/ironic/tests/unit/drivers/modules/test_deploy_utils.py b/ironic/tests/unit/drivers/modules/test_deploy_utils.py index 69c0cceb26..fb8d64395f 100644 --- a/ironic/tests/unit/drivers/modules/test_deploy_utils.py +++ b/ironic/tests/unit/drivers/modules/test_deploy_utils.py @@ -2664,6 +2664,54 @@ def test_build_instance_info_for_deploy_source_redirect_not_path( self.assertEqual('https://image-url/file', task.node.instance_info['image_source']) + @mock.patch.object(os.path, 'isfile', autospec=True) + @mock.patch.object(utils, '_cache_and_convert_image', autospec=True) + def test_build_instance_info_for_deploy_file_url_valid( + self, mock_cache_image, mock_isfile): + i_info = self.node.instance_info + driver_internal_info = self.node.driver_internal_info + i_info['image_source'] = 'file:///var/lib/ironic/files/foo/bar' + driver_internal_info['is_whole_disk_image'] = True + self.node.instance_info = i_info + self.node.driver_internal_info = driver_internal_info + self.node.save() + mock_isfile.return_value = True + with task_manager.acquire( + self.context, self.node.uuid, shared=False) as task: + + utils.build_instance_info_for_deploy(task) + mock_cache_image.assert_called_once_with( + mock.ANY, + {'configdrive': 'TG9yZW0gaXBzdW0gZG9sb3Igc2l0IGFtZXQ=', + 'image_url': None, + 'foo': 'bar', + 'image_source': 'file:///var/lib/ironic/files/foo/bar', + 'image_type': 'whole-disk'}) + + @mock.patch.object(utils, 'cache_instance_image', autospec=True) + def test_build_instance_info_for_deploy_file_url_invalid( + self, mock_cache_image): + mock_cache_image.return_value = ('fake', '/tmp/foo', 'qcow2') + i_info = self.node.instance_info + driver_internal_info = self.node.driver_internal_info + url = 'file:///dev/zero' + i_info['image_source'] = url + self.node.instance_info = i_info + driver_internal_info['is_whole_disk_image'] = True + self.node.driver_internal_info = driver_internal_info + self.node.save() + + with task_manager.acquire( + self.context, self.node.uuid, shared=False) as task: + self.assertRaisesRegex( + exception.ImageRefValidationFailed, + 'Validation of image href file:///dev/zero failed, reason: ' + 'Security: Path /dev/zero is not allowed for image source ' + 'file URLs', + utils.build_instance_info_for_deploy, task) + + mock_cache_image.assert_not_called() + class TestBuildInstanceInfoForHttpProvisioning(db_base.DbTestCase): def setUp(self): @@ -2804,41 +2852,6 @@ def test_build_instance_info_already_raw_keeps_md5(self): self.assertEqual(instance_info['image_disk_format'], 'raw') self.checksum_mock.assert_not_called() - @mock.patch.object(image_service.HttpImageService, 'validate_href', - autospec=True) - def test_build_instance_info_file_image(self, validate_href_mock): - i_info = self.node.instance_info - driver_internal_info = self.node.driver_internal_info - i_info['image_source'] = 'file://image-ref' - i_info['image_checksum'] = 'aa' - i_info['root_gb'] = 10 - driver_internal_info['is_whole_disk_image'] = True - self.node.instance_info = i_info - self.node.driver_internal_info = driver_internal_info - self.node.save() - - expected_url = ( - 'http://172.172.24.10:8080/agent_images/%s' % self.node.uuid) - - with task_manager.acquire( - self.context, self.node.uuid, shared=False) as task: - - info = utils.build_instance_info_for_deploy(task) - - self.assertEqual(expected_url, info['image_url']) - self.assertEqual('sha256', info['image_os_hash_algo']) - self.assertEqual('fake-checksum', info['image_os_hash_value']) - self.assertEqual('raw', info['image_disk_format']) - self.cache_image_mock.assert_called_once_with( - task.context, task.node, force_raw=True, - expected_format=None, - expected_checksum='aa', - expected_checksum_algo=None) - self.checksum_mock.assert_called_once_with( - self.fake_path, algorithm='sha256') - validate_href_mock.assert_called_once_with( - mock.ANY, expected_url, False) - @mock.patch.object(image_service.HttpImageService, 'validate_href', autospec=True) def test_build_instance_info_local_image(self, validate_href_mock): diff --git a/releasenotes/notes/fix-file-url-validation-5fb819ef5ae74ddc.yaml b/releasenotes/notes/fix-file-url-validation-5fb819ef5ae74ddc.yaml new file mode 100644 index 0000000000..0b7b49a9d5 --- /dev/null +++ b/releasenotes/notes/fix-file-url-validation-5fb819ef5ae74ddc.yaml @@ -0,0 +1,15 @@ +--- +fixes: + - | + Fixes an issue in the url handling logic of instance deploy logic where + file paths validity was not checked upfront, and the conductor service + would directly begin calculating checksums. This highlighted a DoS issue + where an attacker could exhaust conductor threads by attempting to request, + "file:///dev/zero", which would never return the thread. This is because + file URLs don't require downloading first, and validity checking logic + was all in the file access logic, not checksum logic. + + Ironic now explicitly invokes the URL/validity checking logic prior to + calling the checksum logic which effectively prevents this issue. + + This issue is tracked as CVE-2026-44919. diff --git a/zuul.d/project.yaml b/zuul.d/project.yaml index ad3a87720e..07234671e6 100644 --- a/zuul.d/project.yaml +++ b/zuul.d/project.yaml @@ -23,87 +23,22 @@ - ironic-tempest-uefi-redfish-https - ironic-tempest-ovn-uefi-ipmi-pxe - ironic-tempest-ovn-uefi-ipxe-ipv6 - # NOTE(TheJulia) Marking multinode non-voting on 20210311 - # Due to a high failure rate on limestone where the compute1 - # machine never appears to be able to communicate across the - # vxlan tunnel, possible mtu issue, but non-voting until we - # understand it in mind for the upcoming release. - - ironic-tempest-ipa-wholedisk-direct-tinyipa-multinode: - voting: false - - ironic-tempest-ipa-wholedisk-direct-tinyipa-multinode-shard: - voting: false - ironic-tempest-bios-ipmi-direct-tinyipa - ironic-tempest-bfv - ironic-tempest-ipa-partition-uefi-pxe-grub2 - ironic-tempest-uefi-redfish-vmedia-4k - # NOTE(rpittau): Currently broken because of an issue with parted - - metalsmith-integration-glance-centos9-uefi: - voting: false - ##################################################################### - # Grenade should be removed in advance of the unmaintained branches # - # as it doesn't know how to upgrade from an unmaintained branch. # - ##################################################################### - - ironic-grenade - - ironic-grenade-skip-level: - voting: false - ############################################################### - # CI Jobs Below this line may be *removed* on Stable Branches # - ############################################################### - # NOTE(TheJulia): At present, metal3 doesn't leverage - # stable branches, and as far as we are aware these jobs - # can be removed once this branch is made stable. - # - metal3-integration - # Non-voting jobs - - ironic-inspector-tempest: - voting: false - - ironic-inspector-tempest-uefi-redfish-vmedia: - voting: false - - ironic-standalone-aarch64: - voting: false - - ironic-tempest-ipa-wholedisk-bios-ipmi-direct-dib: - voting: false - - ironic-standalone-anaconda: - voting: false - - bifrost-integration-tinyipa-ubuntu-noble: - voting: false - - bifrost-integration-redfish-vmedia-uefi-centos-9: - voting: false gate: jobs: - ironic-tox-unit-mysql-migrations - ironic-tox-unit-with-driver-libs - ironic-tempest-functional-python3 - - ironic-grenade - # NOTE(JayF): Disabling standalone jobs on 20240228 from voting, there's a - # dnsmasq bug only exposed on these jobs. - #- ironic-standalone-redfish - ironic-tempest-bios-redfish-pxe - ironic-tempest-uefi-redfish-vmedia - ironic-tempest-ramdisk-bios-snmp-pxe - ironic-tempest-uefi-redfish-https - ironic-tempest-ovn-uefi-ipmi-pxe - ironic-tempest-ovn-uefi-ipxe-ipv6 - # NOTE(TheJulia): Disabled multinode on 20210311 due to Limestone - # seeming to be - # - ironic-tempest-ipa-wholedisk-direct-tinyipa-multinode - ironic-tempest-bios-ipmi-direct-tinyipa - ironic-tempest-bfv - ironic-tempest-ipa-partition-uefi-pxe-grub2 - ironic-tempest-uefi-redfish-vmedia-4k - # NOTE(rpittau): Currently broken because of an issue with parted - #- metalsmith-integration-glance-centos9-uefi - # NOTE(TheJulia): At present, metal3 doesn't leverage - # stable branches, and as far as we are aware these jobs - # can be removed once this branch is made stable. - # - metal3-integration - experimental: - jobs: - # NOTE(dtantsur): this job is rarely used, no need to run it always. - - bifrost-benchmark-ironic: - voting: false - # TODO(dtantsur): these jobs are useful but currently hopelessly - # broken. Fix them and bring back to the gate. - - ironic-grenade-multinode-multitenant: - voting: false - - ironic-inspector-tempest-discovery-fast-track: - voting: false