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 <juliaashleykreger@gmail.com>

ironic/common/image_service.py

@@ -956,8 +956,9 @@ class FileImageService(BaseImageService):
             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)
         for bad in BLOCKED_FILE_URL_PATHS:

ironic/drivers/modules/deploy_utils.py

@@ -1282,6 +1282,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.
@@ -1603,8 +1608,8 @@ def build_instance_info_for_deploy(task):
         check_image_size(node, image_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.
 
@@ -1612,7 +1617,13 @@ 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

ironic/tests/unit/drivers/modules/test_deploy_utils.py

@@ -2945,6 +2945,53 @@ class TestBuildInstanceInfoForDeploy(db_base.DbTestCase):
             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: The path /dev is not permitted in file URLs',
+                utils.build_instance_info_for_deploy, task)
+
+            mock_cache_image.assert_not_called()
+
 
 class TestBuildInstanceInfoForHttpProvisioning(db_base.DbTestCase):
     def setUp(self):
@@ -3086,41 +3133,6 @@ class TestBuildInstanceInfoForHttpProvisioning(db_base.DbTestCase):
         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):

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.