From 98c25db51d5fe00883b8f47af8be8a9ac2fddc9f Mon Sep 17 00:00:00 2001 From: Jay Faulkner Date: Mon, 21 Apr 2025 15:57:37 -0700 Subject: [PATCH] OSSA-2025-001: Disallow unsafe image file:// paths Before this change, Ironic did not filter file:// paths when used as an image source except to ensure they were a file (and not, e.g. a character device). This is problematic from a security perspective because you could end up with config files from well-known paths being written to disk on a node. The allowlist default list is huge, but it includes all known usages of file:// URLs across Bifrost, Ironic, Metal3, and OpenShift in both CI and default configuration. For the backportable version of this patch for stable branches, we have omitted the unconditional block of system paths in order to permit operators using those branches to fully disable the new security functionality. Generated-by: Jetbrains Junie Closes-bug: 2107847 Change-Id: I2fa995439ee500f9dd82ec8ccfa1a25ee8e1179c --- doc/source/install/standalone/enrollment.rst | 17 +++-- ironic/common/image_service.py | 22 ++++++- ironic/conf/conductor.py | 16 +++++ ironic/conf/types.py | 55 ++++++++++++++++ .../tests/unit/common/test_image_service.py | 44 ++++++++++++- ironic/tests/unit/conf/test_conductor.py | 34 ++++++++++ ironic/tests/unit/conf/test_types.py | 63 +++++++++++++++++++ ...w-unsafe-image-paths-670fdcfe3e4647d4.yaml | 29 +++++++++ 8 files changed, 270 insertions(+), 10 deletions(-) create mode 100644 ironic/conf/types.py create mode 100644 ironic/tests/unit/conf/test_conductor.py create mode 100644 ironic/tests/unit/conf/test_types.py create mode 100644 releasenotes/notes/ossa-2025-001-disallow-unsafe-image-paths-670fdcfe3e4647d4.yaml 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.