openstack/ironic
Code Issues Proposed changes

Merge "OSSA-2025-001: Disallow unsafe image file:// paths"

Browse Source
This commit is contained in:
Zuul 2025-05-08 17:20:51 +00:00 committed by Gerrit Code Review
commit 10590b36f5
8 changed files with 294 additions and 10 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

16
doc/source/install/standalone/enrollment.rst
View File

@ -32,13 +32,19 @@ 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 not be located in ``/dev``, ``/sys``, ``/proc``,
``/etc``, ``/boot``, ``/run`` or other system paths.
* File images must be located in a path listed in
:oslo_config:option:`conductor.file_url_allowed_paths`
.. 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

34
ironic/common/image_service.py
View File

@ -36,6 +36,11 @@ from ironic.common import utils
from ironic.conf import CONF
IMAGE_CHUNK_SIZE = 1024 * 1024 # 1mb
# NOTE(JayF): This is the check-of-last-resort; we also have an allowlist
# enabled by default. These represent paths that under no circumstances should
# we access for file:// URLs
BLOCKED_FILE_URL_PATHS = {'/dev', '/sys', '/proc', '/boot', '/etc', '/run'}
LOG = log.getLogger(__name__)
@ -813,14 +818,39 @@ 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)
for bad in BLOCKED_FILE_URL_PATHS:
if rpath == bad or rpath.startswith(bad + os.sep):
raise exception.ImageRefValidationFailed(
image_href=image_href,
reason=_("Security: The path %s is not permitted in file "
"URLs" % bad)
)
# 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):

15
ironic/conf/conductor.py
View File

@ -21,6 +21,7 @@ from oslo_config import types
from ironic.common import boot_modes
from ironic.common.i18n import _
from ironic.common import lessee_sources
from ironic.conf import types as ir_types
opts = [
@ -531,6 +532,20 @@ opts = [
'wish to disable this functionality, otherwise it is '
'automatically applied by the conductor should a '
'compressed artifact be detected.')),
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 "..".'
)),
]

55
ironic/conf/types.py Normal file
View File

@ -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)

61
ironic/tests/unit/common/test_image_service.py
View File

@ -615,20 +615,75 @@ 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_blocked_path(self, abspath_mock):
href = 'file:///dev/sda1'
href_path = '/dev/sda1'
href_dir = '/dev'
abspath_mock.side_effect = [href_dir, href_path]
# Explicitly allow the bad path
cfg.CONF.set_override('file_url_allowed_paths', [href_dir],
'conductor')
# Still raises an error because /dev is expressly forbidden
self.assertRaisesRegex(exception.ImageRefValidationFailed,
"is not permitted in file URLs",
self.service.validate_href, href)
abspath_mock.assert_has_calls(
[mock.call(href_dir), mock.call(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)

34
ironic/tests/unit/conf/test_conductor.py Normal file
View File

@ -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')

63
ironic/tests/unit/conf/test_types.py Normal file
View File

@ -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}")

26
releasenotes/notes/ossa-2025-001-disallow-unsafe-image-paths-670fdcfe3e4647d4.yaml Normal file
View File

@ -0,0 +1,26 @@
---
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. Ironic now
unconditionally forbids paths that provide access to system
configuration (/dev, /sys, /proc, /boot, /run, and /etc).
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).
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.