From f0a19964c21786eb984f295ff98b0ca46eb6d290 Mon Sep 17 00:00:00 2001 From: Dmitry Tantsur Date: Mon, 4 May 2026 17:14:07 +0200 Subject: [PATCH] CVE-2026-44918: Prevent rehoming resources to nodes with different owner Ironic has a few similar patterns where only the source node is verified during a create or update operation: This is a focused backport to address object re-homing outside the scope of parent/child node modeling. i.e., only item 3 from the original commit to fix the CVE, because items 1 and 2 from that fix did not apply to this branch. 3. Ports, port groups, volume targets and volume connectors can be patched to be owned by a node from a different project. Related-Bug: 2150450 Change-Id: Iaa512f380926a9fc032eefea5bcdcbc5040a4739 Signed-off-by: Julia Kreger Signed-off-by: Jay Faulkner (cherry picked from commit b58d4adc43551323b7ae8d5252636d796e32b949) (cherry picked from commit d99b35dcda7df5c09621e1c1a2c4def112d1d7c6) (cherry picked from commit 54e4cf7a400dc32fb471829ac6d76a93749d0421) (cherry picked from commit 72d759ab30b2895c2f1012eb7e85e450dba842f8) --- ironic/api/controllers/v1/port.py | 18 +---- ironic/api/controllers/v1/portgroup.py | 21 ++--- ironic/api/controllers/v1/utils.py | 76 +++++++++++++------ ironic/api/controllers/v1/volume_connector.py | 21 ++--- ironic/api/controllers/v1/volume_target.py | 31 +++----- .../unit/api/controllers/v1/test_node.py | 6 ++ .../unit/api/controllers/v1/test_utils.py | 18 ----- .../2150450-owners-b4019d5fa63a8bc1.yaml | 14 ++++ tox.ini | 2 +- 9 files changed, 101 insertions(+), 106 deletions(-) create mode 100644 releasenotes/notes/2150450-owners-b4019d5fa63a8bc1.yaml diff --git a/ironic/api/controllers/v1/port.py b/ironic/api/controllers/v1/port.py index d70c13f27e..6c25622d73 100644 --- a/ironic/api/controllers/v1/port.py +++ b/ironic/api/controllers/v1/port.py @@ -634,12 +634,9 @@ def patch(self, port_uuid, patch): 'baremetal:port:update', port_uuid) port_dict = rpc_port.as_dict() - # NOTE(lucasagomes): - # 1) Remove node_id because it's an internal value and + # NOTE(lucasagomes): Remove node_id because it's an internal value and # not present in the API object - # 2) Add node_uuid port_dict.pop('node_id', None) - port_dict['node_uuid'] = rpc_node.uuid # NOTE(vsaienko): # 1) Remove portgroup_id because it's an internal value and # not present in the API object @@ -650,7 +647,10 @@ def patch(self, port_uuid, patch): context, port_dict.pop('portgroup_id')) port_dict['portgroup_uuid'] = portgroup and portgroup.uuid or None + rpc_node = api_utils.authorize_node_link_patch( + rpc_node, patch, 'node_uuid') port_dict = api_utils.apply_jsonpatch(port_dict, patch) + port_dict['node_uuid'] = rpc_node.uuid try: if api_utils.is_path_updated(patch, '/portgroup_uuid'): @@ -665,16 +665,6 @@ def patch(self, port_uuid, patch): e.code = http_client.BAD_REQUEST # BadRequest raise - try: - if port_dict['node_uuid'] != rpc_node.uuid: - rpc_node = objects.Node.get( - api.request.context, port_dict['node_uuid']) - except exception.NodeNotFound as e: - # Change error code because 404 (NotFound) is inappropriate - # response for a PATCH request to change a Port - e.code = http_client.BAD_REQUEST # BadRequest - raise - api_utils.patched_validate_with_schema( port_dict, PORT_PATCH_SCHEMA, PORT_PATCH_VALIDATOR) diff --git a/ironic/api/controllers/v1/portgroup.py b/ironic/api/controllers/v1/portgroup.py index 6c68e07ba3..afc0df20ef 100644 --- a/ironic/api/controllers/v1/portgroup.py +++ b/ironic/api/controllers/v1/portgroup.py @@ -470,29 +470,20 @@ def patch(self, portgroup_ident, patch): portgroup_dict = rpc_portgroup.as_dict() - # NOTE: - # 1) Remove node_id because it's an internal value and + # NOTE: Remove node_id because it's an internal value and # not present in the API object - # 2) Add node_uuid portgroup_dict.pop('node_id') - portgroup_dict['node_uuid'] = rpc_node.uuid + + rpc_node = api_utils.authorize_node_link_patch( + rpc_node, patch, 'node_uuid') + portgroup_dict = api_utils.apply_jsonpatch(portgroup_dict, patch) + portgroup_dict['node_uuid'] = rpc_node.uuid if 'mode' not in portgroup_dict: msg = _("'mode' is a mandatory attribute and can not be removed") raise exception.ClientSideError(msg) - try: - if portgroup_dict['node_uuid'] != rpc_node.uuid: - rpc_node = objects.Node.get(api.request.context, - portgroup_dict['node_uuid']) - - except exception.NodeNotFound as e: - # Change error code because 404 (NotFound) is inappropriate - # response for a POST request to patch a Portgroup - e.code = http_client.BAD_REQUEST # BadRequest - raise - api_utils.patched_validate_with_schema( portgroup_dict, PORTGROUP_PATCH_SCHEMA, PORTGROUP_PATCH_VALIDATOR) diff --git a/ironic/api/controllers/v1/utils.py b/ironic/api/controllers/v1/utils.py index 0494077cc4..ed13b56f93 100644 --- a/ironic/api/controllers/v1/utils.py +++ b/ironic/api/controllers/v1/utils.py @@ -25,6 +25,7 @@ from jsonschema import exceptions as json_schema_exc import os_traits from oslo_config import cfg +from oslo_log import log from oslo_policy import policy as oslo_policy from oslo_utils import uuidutils from pecan import rest @@ -46,6 +47,8 @@ CONF = cfg.CONF +LOG = log.getLogger(__name__) + _JSONPATCH_EXCEPTIONS = (jsonpatch.JsonPatchConflict, jsonpatch.JsonPatchException, @@ -294,28 +297,6 @@ def replace_node_uuid_with_id(to_dict): return node -def replace_node_id_with_uuid(to_dict): - """Replace ``node_id`` dict value with ``node_uuid`` - - ``node_uuid`` is found by fetching the node by id lookup. - - :param to_dict: Dict to set ``node_uuid`` value on - :returns: The node object from the lookup - :raises: NodeNotFound with status_code set to 400 BAD_REQUEST - when node is not found. - """ - try: - node = objects.Node.get_by_id(api.request.context, - to_dict.pop('node_id')) - to_dict['node_uuid'] = node.uuid - except exception.NodeNotFound as e: - # Change error code because 404 (NotFound) is inappropriate - # response for requests acting on non-nodes - e.code = http_client.BAD_REQUEST # BadRequest - raise - return node - - def patch_update_changed_fields(from_dict, rpc_object, fields, schema, id_map=None): """Update rpc object based on changed fields in a dict. @@ -1475,6 +1456,57 @@ def check_policy_true(policy_name): return policy.check_policy(policy_name, cdict, api.request.context) +def authorize_node_link(orig_node, new_uuid, field_name): + """Authorize creating or changing a link to the node on a resource. + + :param orig_node: Node that currently owns the resource. + :param new_uuid: UUID of the new node. + :param field_name: Human-readable field name for logging and error message. + :raises: Invalid on access error + :returns: The new Node object + """ + msg = _("Unable to apply the requested %s '%s'. " + "Requested value was invalid.") + + try: + new_node = check_node_policy_and_retrieve( + 'baremetal:node:get', new_uuid) + except Exception as exc: + LOG.debug("Rejecting %s %s: %s", field_name, new_uuid, exc) + # Important: do not disclose if the node exists + raise exception.Invalid(msg % (field_name, new_uuid)) + + if isinstance(orig_node, objects.Node): + orig_node = orig_node.as_dict() # adjust for different callers + + if orig_node.get('owner') != new_node.owner: + LOG.warning("Project mismatch on setting or changing %(field)s: " + "current owner '%(orig)s', new %(field)s owner '%(new)s'", + {'orig': orig_node.get('owner'), 'new': new_node.owner, + 'field': field_name}) + # Important: same error message as when the node does not exist + raise exception.Invalid(msg % (field_name, new_uuid)) + + return new_node + + +def authorize_node_link_patch(orig_node, patch, field_name): + """Authorize changing a link to the node on a resource. + + :param orig_node: Node that currently owns the resource. + :param patch: JSON patch being applied. + :param field_name: Field names that contains the link. + :raises: Invalid on access error + :returns: The new Node object or the old one if not changed + """ + new_node_id = get_patch_values(patch, f'/{field_name}') + if new_node_id: + return authorize_node_link( + orig_node, new_node_id[0], field_name) + + return orig_node + + def check_owner_policy(object_type, policy_name, owner, lessee=None, conceal_node=False): """Check if the policy authorizes this request on an object. diff --git a/ironic/api/controllers/v1/volume_connector.py b/ironic/api/controllers/v1/volume_connector.py index dd003e8671..0393ce41cf 100644 --- a/ironic/api/controllers/v1/volume_connector.py +++ b/ironic/api/controllers/v1/volume_connector.py @@ -332,23 +332,16 @@ def patch(self, connector_uuid, patch): raise exception.InvalidUUID(message=message) connector_dict = rpc_connector.as_dict() - # NOTE(smoriya): - # 1) Remove node_id because it's an internal value and + # NOTE(smoriya): Remove node_id because it's an internal value and # not present in the API object - # 2) Add node_uuid - rpc_node = api_utils.replace_node_id_with_uuid(connector_dict) + connector_dict.pop('node_id', None) + # NOTE(dtantsur): Patch won't apply if the field does not exist. + connector_dict['node_uuid'] = None + rpc_node = api_utils.authorize_node_link_patch( + rpc_node, patch, 'node_uuid') connector_dict = api_utils.apply_jsonpatch(connector_dict, patch) - - try: - if connector_dict['node_uuid'] != rpc_node.uuid: - rpc_node = objects.Node.get( - api.request.context, connector_dict['node_uuid']) - except exception.NodeNotFound as e: - # Change error code because 404 (NotFound) is inappropriate - # response for a PATCH request to change a Port - e.code = http_client.BAD_REQUEST # BadRequest - raise + connector_dict['node_uuid'] = rpc_node.uuid api_utils.patched_validate_with_schema( connector_dict, CONNECTOR_SCHEMA, CONNECTOR_VALIDATOR) diff --git a/ironic/api/controllers/v1/volume_target.py b/ironic/api/controllers/v1/volume_target.py index 2fe1afb032..f313cfefb0 100644 --- a/ironic/api/controllers/v1/volume_target.py +++ b/ironic/api/controllers/v1/volume_target.py @@ -345,9 +345,8 @@ def patch(self, target_uuid, patch): """ context = api.request.context - api_utils.check_volume_policy_and_retrieve('baremetal:volume:update', - target_uuid, - target=True) + rpc_target, rpc_node = api_utils.check_volume_policy_and_retrieve( + 'baremetal:volume:update', target_uuid, target=True) if self.parent_node_ident: raise exception.OperationNotPermitted() @@ -361,29 +360,17 @@ def patch(self, target_uuid, patch): "%(uuid)s.") % {'uuid': str(value)} raise exception.InvalidUUID(message=message) - rpc_target = objects.VolumeTarget.get_by_uuid(context, target_uuid) target_dict = rpc_target.as_dict() - # NOTE(smoriya): - # 1) Remove node_id because it's an internal value and + # NOTE(smoriya): Remove node_id because it's an internal value and # not present in the API object - # 2) Add node_uuid - rpc_node = api_utils.replace_node_id_with_uuid(target_dict) + target_dict.pop('node_id', None) + # NOTE(dtantsur): Patch won't apply if the field does not exist. + target_dict['node_uuid'] = None + rpc_node = api_utils.authorize_node_link_patch( + rpc_node, patch, 'node_uuid') target_dict = api_utils.apply_jsonpatch(target_dict, patch) - - try: - if target_dict['node_uuid'] != rpc_node.uuid: - - # TODO(TheJulia): I guess the intention is to - # permit the mapping to be changed - # should we even allow this at all? - rpc_node = objects.Node.get( - api.request.context, target_dict['node_uuid']) - except exception.NodeNotFound as e: - # Change error code because 404 (NotFound) is inappropriate - # response for a PATCH request to change a volume target - e.code = http_client.BAD_REQUEST # BadRequest - raise + target_dict['node_uuid'] = rpc_node.uuid api_utils.patched_validate_with_schema( target_dict, TARGET_SCHEMA, TARGET_VALIDATOR) diff --git a/ironic/tests/unit/api/controllers/v1/test_node.py b/ironic/tests/unit/api/controllers/v1/test_node.py index 3c0049a737..ee74bec461 100644 --- a/ironic/tests/unit/api/controllers/v1/test_node.py +++ b/ironic/tests/unit/api/controllers/v1/test_node.py @@ -2698,6 +2698,12 @@ def test_update_ok(self, mock_notify): timeutils.parse_isotime(response.json['updated_at'])) self.mock_update_node.assert_called_once_with( mock.ANY, mock.ANY, mock.ANY, 'test-topic', None) + # NOTE(TheJulia) As we save a hydrated database object back, we need + # to double check what gets saved back out to the database to validate + # handling is proper. + updated_node = self.mock_update_node.call_args.args[2] + self.assertEqual(updated_node.instance_uuid, + 'aaaaaaaa-1111-bbbb-2222-cccccccccccc') mock_notify.assert_has_calls([mock.call(mock.ANY, mock.ANY, 'update', obj_fields.NotificationLevel.INFO, obj_fields.NotificationStatus.START, diff --git a/ironic/tests/unit/api/controllers/v1/test_utils.py b/ironic/tests/unit/api/controllers/v1/test_utils.py index d12e4328a5..d160fbe93c 100644 --- a/ironic/tests/unit/api/controllers/v1/test_utils.py +++ b/ironic/tests/unit/api/controllers/v1/test_utils.py @@ -931,24 +931,6 @@ def test_replace_node_uuid_with_id_not_found(self, mock_gbu, mock_pr): utils.replace_node_uuid_with_id, to_dict) self.assertEqual(400, e.code) - @mock.patch.object(objects.Node, 'get_by_id', autospec=True) - def test_replace_node_id_with_uuid(self, mock_gbi, mock_pr): - node = obj_utils.get_test_node(self.context, uuid=self.valid_uuid) - mock_gbi.return_value = node - to_dict = {'node_id': 1} - - self.assertEqual(node, utils.replace_node_id_with_uuid(to_dict)) - self.assertEqual({'node_uuid': self.valid_uuid}, to_dict) - - @mock.patch.object(objects.Node, 'get_by_id', autospec=True) - def test_replace_node_id_with_uuid_not_found(self, mock_gbi, mock_pr): - to_dict = {'node_id': 1} - mock_gbi.side_effect = exception.NodeNotFound(node=1) - - e = self.assertRaises(exception.NodeNotFound, - utils.replace_node_id_with_uuid, to_dict) - self.assertEqual(400, e.code) - class TestVendorPassthru(base.TestCase): diff --git a/releasenotes/notes/2150450-owners-b4019d5fa63a8bc1.yaml b/releasenotes/notes/2150450-owners-b4019d5fa63a8bc1.yaml new file mode 100644 index 0000000000..abb8cbc2ee --- /dev/null +++ b/releasenotes/notes/2150450-owners-b4019d5fa63a8bc1.yaml @@ -0,0 +1,14 @@ +--- +security: + - | + Prevents changing the ``node_uuid`` of ports, port groups, volume targets, + and volume connectors to point to a node with a different owner from the + initial node. In some cases this is not normally permitted due to the + database model, but additional access checking was added across these + similar resources for consistency in the event the Ironic project fixes + `12150252 `_. +issues: + - | + System operators should note that changing the owner field on a node does + not affect its child or parent nodes, potentially resulting in these nodes + being in different projects. \ No newline at end of file diff --git a/tox.ini b/tox.ini index 29236854de..91038fa71c 100644 --- a/tox.ini +++ b/tox.ini @@ -137,7 +137,7 @@ filename = *.py,app.wsgi exclude=.*,dist,doc,*lib/python*,*egg,build import-order-style = pep8 application-import-names = ironic -max-complexity=19 +max-complexity=21 # [H106] Don't put vim configuration in source files. # [H203] Use assertIs(Not)None to check for None. # [H204] Use assert(Not)Equal to check for equality.