diff --git a/doc/source/install/standalone/enrollment.rst b/doc/source/install/standalone/enrollment.rst index 3050910702..77a8eb2092 100644 --- a/doc/source/install/standalone/enrollment.rst +++ b/doc/source/install/standalone/enrollment.rst @@ -32,13 +32,20 @@ There are however some limitations for different hardware interfaces: $ sha256sum image.qcow2 9f6c942ad81690a9926ff530629fb69a82db8b8ab267e2cbd59df417c1a28060 image.qcow2 -* :ref:`direct-deploy` started supporting ``file://`` images in the Victoria - release cycle, before that only HTTP(s) had been supported. +* If you're using :ref:`direct-deploy` with ``file://`` URLs, you have to + ensure the images meet all requirements: + + * File images must be accessible to every conductor + * File images must be located in a path listed in + :oslo.config:option:`conductor.file_url_allowed_paths` + * File images must not be located in ``/dev``, ``/sys``, ``/proc``, + ``/etc``, ``/boot``, ``/run`` or other system paths starting with the + Ironic 2025.2 release. .. warning:: - File images must be accessible to every conductor! Use a shared file - system if you have more than one conductor. The ironic CLI tool will not - transfer the file from a local machine to the conductor(s). + The Ironic CLI tool will not transfer the file from a local machine to the + conductor(s). Operators should use shared file systems or configuration + management to ensure consistent availability of images. .. note:: The Bare Metal service tracks content changes for non-Glance images by diff --git a/ironic/common/image_service.py b/ironic/common/image_service.py index df3a8786fc..03744b9f3f 100644 --- a/ironic/common/image_service.py +++ b/ironic/common/image_service.py @@ -273,14 +273,32 @@ class FileImageService(BaseImageService): :param image_href: Image reference. :raises: exception.ImageRefValidationFailed if source image file - doesn't exist. - :returns: Path to image file if it exists. + 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. """ image_path = urlparse.urlparse(image_href).path + + # Check if the path is in the blocklist + rpath = os.path.abspath(image_path) + + # Check if the path is in the allowlist + for allowed in CONF.conductor.file_url_allowed_paths: + if rpath == allowed or rpath.startswith(allowed + os.sep): + break + else: + raise exception.ImageRefValidationFailed( + image_href=image_href, + reason=_( + "Security: Path %s is not allowed for image source " + "file URLs" % image_path) + ) + + # Check if the file exists if not os.path.isfile(image_path): raise exception.ImageRefValidationFailed( image_href=image_href, reason=_("Specified image file not found.")) + return image_path def download(self, image_href, image_file): diff --git a/ironic/conf/conductor.py b/ironic/conf/conductor.py index 3240f468db..db4f67b3b1 100644 --- a/ironic/conf/conductor.py +++ b/ironic/conf/conductor.py @@ -19,6 +19,8 @@ from oslo_config import cfg from oslo_config import types from ironic.common.i18n import _ +from ironic.conf import types as ir_types + opts = [ cfg.IntOpt('workers_pool_size', @@ -449,6 +451,20 @@ opts = [ 'functionality by setting this option to True will ' 'create a more secure environment, however it may ' 'break users in an unexpected fashion.')), + cfg.ListOpt('file_url_allowed_paths', + default=['/var/lib/ironic', '/shared/html', '/templates', + '/opt/cache/files', '/vagrant'], + item_type=ir_types.ExplicitAbsolutePath(), + help=_( + 'List of paths that are allowed to be used as file:// ' + 'URLs. Files in /boot, /dev, /etc, /proc, /sys and other' + 'system paths are always disallowed for security reasons. ' + 'Any files in this path readable by ironic may be used as ' + 'an image source when deploying. Setting this value to ' + '"" (empty) disables file:// URL support. Paths listed ' + 'here are validated as absolute paths and will be rejected' + 'if they contain path traversal mechanisms, such as "..".' + )), ] diff --git a/ironic/conf/types.py b/ironic/conf/types.py new file mode 100644 index 0000000000..b9003d866a --- /dev/null +++ b/ironic/conf/types.py @@ -0,0 +1,55 @@ +# Licensed under the Apache License, Version 2.0 (the "License"); you may not +# use this file except in compliance with the License. You may obtain a copy +# of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +import os + +from oslo_config import types + + +class ExplicitAbsolutePath(types.ConfigType): + """Explicit Absolute path type. + + Absolute path values do not get transformed and are returned as + strings. They are validated to ensure they are absolute paths and are equal + to os.path.abspath(value) -- protecting from path traversal issues. + + Python path libraries generally define "absolute path" as anything + starting with a /, so tools like path.PurePath(str).is_absolute() is not + useful, because it will happily return that /tmp/../etc/resolv.conf is + absolute. This type is to be used in cases where we require the path to + be explicitly absolute. + + :param type_name: Type name to be used in the sample config file. + """ + + def __init__(self, type_name='explicit absolute path'): + super().__init__(type_name=type_name) + + def __call__(self, value): + value = str(value) + + # NOTE(JayF): This removes trailing / if provided, since + # os.path.abspath will not return a trailing slash. + if len(value) > 1: + value = value.rstrip('/') + absvalue = os.path.abspath(value) + if value != absvalue: + raise ValueError('Value must be an absolute path ' + 'containing no path traversal mechanisms. Config' + f'item was: {value}, but resolved to {absvalue}') + + return value + + def __repr__(self): + return 'explicit absolute path' + + def _formatter(self, value): + return self.quote_trailing_and_leading_space(value) diff --git a/ironic/tests/unit/common/test_image_service.py b/ironic/tests/unit/common/test_image_service.py index a8f325e8cf..700f70bcbe 100644 --- a/ironic/tests/unit/common/test_image_service.py +++ b/ironic/tests/unit/common/test_image_service.py @@ -482,20 +482,58 @@ class FileImageServiceTestCase(base.TestCase): def setUp(self): super(FileImageServiceTestCase, self).setUp() self.service = image_service.FileImageService() - self.href = 'file:///home/user/image.qcow2' - self.href_path = '/home/user/image.qcow2' + self.href = 'file:///var/lib/ironic/images/image.qcow2' + self.href_path = '/var/lib/ironic/images/image.qcow2' @mock.patch.object(os.path, 'isfile', return_value=True, autospec=True) def test_validate_href(self, path_exists_mock): self.service.validate_href(self.href) path_exists_mock.assert_called_once_with(self.href_path) - @mock.patch.object(os.path, 'isfile', return_value=False, autospec=True) + @mock.patch.object(os.path, 'isfile', return_value=False, + autospec=True) def test_validate_href_path_not_found_or_not_file(self, path_exists_mock): self.assertRaises(exception.ImageRefValidationFailed, self.service.validate_href, self.href) path_exists_mock.assert_called_once_with(self.href_path) + @mock.patch.object(os.path, 'abspath', autospec=True) + def test_validate_href_empty_allowlist(self, abspath_mock): + abspath_mock.return_value = self.href_path + cfg.CONF.set_override('file_url_allowed_paths', [], 'conductor') + self.assertRaisesRegex(exception.ImageRefValidationFailed, + "is not allowed for image source file URLs", + self.service.validate_href, self.href) + + @mock.patch.object(os.path, 'abspath', autospec=True) + def test_validate_href_not_in_allowlist(self, abspath_mock): + href = "file:///var/is/allowed/not/this/path/image.qcow2" + href_path = "/var/is/allowed/not/this/path/image.qcow2" + abspath_mock.side_effect = ['/var/lib/ironic', href_path] + cfg.CONF.set_override('file_url_allowed_paths', ['/var/lib/ironic'], + 'conductor') + self.assertRaisesRegex(exception.ImageRefValidationFailed, + "is not allowed for image source file URLs", + self.service.validate_href, href) + + @mock.patch.object(os.path, 'abspath', autospec=True) + @mock.patch.object(os.path, 'isfile', + return_value=True, autospec=True) + def test_validate_href_in_allowlist(self, + path_exists_mock, + abspath_mock): + href_dir = '/var/lib' # self.href_path is in /var/lib/ironic/images/ + # First call is ironic.conf.types.ExplicitAbsolutePath + # Second call is in validate_href() + abspath_mock.side_effect = [href_dir, self.href_path] + cfg.CONF.set_override('file_url_allowed_paths', [href_dir], + 'conductor') + result = self.service.validate_href(self.href) + self.assertEqual(self.href_path, result) + path_exists_mock.assert_called_once_with(self.href_path) + abspath_mock.assert_has_calls( + [mock.call(href_dir), mock.call(self.href_path)]) + @mock.patch.object(os.path, 'getmtime', return_value=1431087909.1641912, autospec=True) @mock.patch.object(os.path, 'getsize', return_value=42, autospec=True) diff --git a/ironic/tests/unit/conf/test_conductor.py b/ironic/tests/unit/conf/test_conductor.py new file mode 100644 index 0000000000..abc583c4be --- /dev/null +++ b/ironic/tests/unit/conf/test_conductor.py @@ -0,0 +1,34 @@ +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +from ironic.conf import CONF +from ironic.tests.base import TestCase + + +class ValidateConductorAllowedPaths(TestCase): + def test_abspath_validation_bad_path_raises(self): + """Verifies setting a relative path raises an error via oslo.config.""" + self.assertRaises( + ValueError, + CONF.set_override, + 'file_url_allowed_paths', + ['bad/path'], + 'conductor' + ) + + def test_abspath_validation_good_paths(self): + """Verifies setting an absolute path works via oslo.config.""" + CONF.set_override('file_url_allowed_paths', ['/var'], 'conductor') + + def test_abspath_validation_good_paths_trailing_slash(self): + """Verifies setting an absolute path works via oslo.config.""" + CONF.set_override('file_url_allowed_paths', ['/var/'], 'conductor') diff --git a/ironic/tests/unit/conf/test_types.py b/ironic/tests/unit/conf/test_types.py new file mode 100644 index 0000000000..889638c463 --- /dev/null +++ b/ironic/tests/unit/conf/test_types.py @@ -0,0 +1,63 @@ +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +from ironic.conf import types +from ironic.tests.base import TestCase + + +class ExplicitAbsolutePath(TestCase): + def test_explicit_absolute_path(self): + """Verifies the Opt subclass used to validate absolute paths.""" + good_paths = [ + '/etc/passwd', # Valid + '/usr/bin/python', # Valid + '/home/user/file.txt', # Valid - dot in filename allowed + '/var/lib/ironic/.secretdir', # Valid - hidden directory allowed + '/var/lib/ironic/oslo.config', # Valid - dots in filename allowed + '/tmp/', # Valid + '/', # Valid (root directory) + '/.hidden_root_file', # Valid + '/path/including/a/numb3r', # Valid + '/a/path/with/a/trailing/slash/' # Valid + ] + bad_paths = [ + 'relative/path', # Invalid - no leading slash + './file.txt', # Invalid - relative path + '../file.txt', # Invalid - relative path + 'file.txt', # Invalid - no leading slash + '', # Invalid - empty string + '/var/lib/ironic/../../../etc/passwd', # Invalid - path traversal + '/etc/../etc/passwd', # Invalid - path traversal + '/home/user/./config', # Invalid - contains current dir reference + '/home/user/../user/config', # Invalid - path traversal + '/../etc/passwd', # Invalid - path traversal at beginning + '/.', # Invalid - just current directory + '/..' # Invalid - just parent directory + ] + + eap = types.ExplicitAbsolutePath() + + def _trypath(tpath): + try: + eap(tpath) + except ValueError: + return False + else: + return True + + for path in good_paths: + self.assertTrue(_trypath(path), + msg=f"Improperly disallowed path: {path}") + + for path in bad_paths: + self.assertFalse(_trypath(path), + msg=f"Improperly allowed path: {path}") diff --git a/releasenotes/notes/ossa-2025-001-disallow-unsafe-image-paths-670fdcfe3e4647d4.yaml b/releasenotes/notes/ossa-2025-001-disallow-unsafe-image-paths-670fdcfe3e4647d4.yaml new file mode 100644 index 0000000000..2e14820ffa --- /dev/null +++ b/releasenotes/notes/ossa-2025-001-disallow-unsafe-image-paths-670fdcfe3e4647d4.yaml @@ -0,0 +1,29 @@ +--- +security: + - | + Fixes OSSA-2025-001, where Ironic did not properly filter file:// paths + when used as image sources. This would permit any file accessible by the + conductor to be used as an image to attempt deployment. + + Adds ``CONF.conductor.file_url_allowed_paths``, an allowlist configuration + defaulting to ``/var/lib/ironic``, ``/shared/html``, + ``/opt/cache/files``, ``/vagrant``, and ``/templates``, + permits operators to further restrict where the conductor will fetch + images for when provided a file:// URL. This default value was chosen + based on known usage by projects downstream of Ironic, including Metal3, + Bifrost, and OpenShift. These defaults may change to be more restrictive + at a later date. Operators using file:// URLs are encouraged to explicitly + set this value even if the current default is sufficient. Operators wishing + to fully disable the ability to deploy with a file:// URL should set this + configuration to "" (empty). + + Operators wishing to restore the original insecure behavior should set + ``CONF.conductor.file_url_allowed_paths`` to ``/``. Take note that in the + 2025.2 release and later, ``/dev``, ``/sys``, ``/proc``, ``/run``, and + ``/etc`` will be unconditionally blocked as a security measure. + + This issue only poses a significant security risk when Ironic's + automated cleaning process is disabled and the service is configured in + such a way that permits direct deployment by an untrusted API user, such as + standalone Ironic installations or environments granting ownership of nodes + to projects.