From 97f75b2e7465f133a4b135c7dfd29d77c3605c4a Mon Sep 17 00:00:00 2001 From: Julia Kreger Date: Thu, 30 Apr 2026 15:45:54 -0700 Subject: [PATCH 1/2] security: move file url validation up into deploy_utils main path An issue was discovered where we were executing checksums prior to doing file path guard logic. We've moved the check into the same area of the code where we do all other url checks for consistency. This issue is tracked as CVE-2026-44919. Closes-Bug: 2150332 Change-Id: I09fa51801cf40da6f7be31014cca63ad1c253a5a Signed-off-by: Julia Kreger (cherry picked from commit a3f6d735ac3642ab95b49142c7305f072ae748d0) --- ironic/common/image_service.py | 3 +- ironic/drivers/modules/deploy_utils.py | 18 +++- .../unit/drivers/modules/test_deploy_utils.py | 83 +++++++++++-------- ...-file-url-validation-5fb819ef5ae74ddc.yaml | 15 ++++ 4 files changed, 80 insertions(+), 39 deletions(-) create mode 100644 releasenotes/notes/fix-file-url-validation-5fb819ef5ae74ddc.yaml 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. From 607c5f7342e29a871e12e1b1b85fc5fff39c411f Mon Sep 17 00:00:00 2001 From: Jay Faulkner Date: Tue, 19 May 2026 12:38:53 -0700 Subject: [PATCH 2/2] [stable-only][ci] Remove non-voting, obsolete jobs This is now the last supported branch, so some jobs -- like upgrade testing -- don't make sense anymore. Also removing all non-voting jobs as it's extremely unlikely at this point in the lifetime of this branch that they'll ever be fixed. Change-Id: Ia747511af0834dd5cbe370365d1714991ef35ada Signed-off-by: Jay Faulkner --- zuul.d/project.yaml | 65 --------------------------------------------- 1 file changed, 65 deletions(-) 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