From 1c13ad05dd937b65330c389301c2f095b7bedcd3 Mon Sep 17 00:00:00 2001 From: Brian Haley Date: Thu, 27 Feb 2020 17:33:28 -0500 Subject: [PATCH] Remove extra header fields in proxied metadata requests If a user specifies a header in their request for metadata, it could override what the proxy would have inserted on their behalf. Make sure to remove any headers we don't want, and override something that might be present in the request. If the agent somehow gets a request with both headers it will silently drop it. Change-Id: Id6c103b7bcebe441c27c6049d349d84ba7fd15a6 Closes-bug: #1865036 (cherry picked from commit 5af046fd4e6387cdbf8bf65ea4c2039a4019b64b) --- neutron/agent/metadata/agent.py | 7 ++++++ neutron/agent/metadata/driver.py | 7 +++++- .../tests/unit/agent/metadata/test_agent.py | 25 +++++++++++++++++++ .../tests/unit/agent/metadata/test_driver.py | 1 + ...header-vulnerability-60c44eb7c76d560c.yaml | 8 ++++++ 5 files changed, 47 insertions(+), 1 deletion(-) create mode 100644 releasenotes/notes/metadata-proxy-header-vulnerability-60c44eb7c76d560c.yaml diff --git a/neutron/agent/metadata/agent.py b/neutron/agent/metadata/agent.py index a8c2d424ace..bfc12f52f93 100644 --- a/neutron/agent/metadata/agent.py +++ b/neutron/agent/metadata/agent.py @@ -159,6 +159,13 @@ class MetadataProxyHandler(object): network_id = req.headers.get('X-Neutron-Network-ID') router_id = req.headers.get('X-Neutron-Router-ID') + # Only one should be given, drop since it could be spoofed + if network_id and router_id: + LOG.debug("Both network and router IDs were specified in proxy " + "request, but only a single one of the two is allowed, " + "dropping") + return None, None + ports = self._get_ports(remote_address, network_id, router_id) LOG.debug("Gotten ports for remote_address %(remote_address)s, " "network_id %(network_id)s, router_id %(router_id)s are: " diff --git a/neutron/agent/metadata/driver.py b/neutron/agent/metadata/driver.py index 4887f44e60c..7d704e09fe5 100644 --- a/neutron/agent/metadata/driver.py +++ b/neutron/agent/metadata/driver.py @@ -63,7 +63,8 @@ defaults listen listener bind %(host)s:%(port)s server metadata %(unix_socket_path)s - http-request add-header X-Neutron-%(res_type)s-ID %(res_id)s + http-request del-header X-Neutron-%(res_type_del)s-ID + http-request set-header X-Neutron-%(res_type)s-ID %(res_id)s """ @@ -126,12 +127,16 @@ class HaproxyConfigurator(object): 'log_level': self.log_level, 'log_tag': self.log_tag } + # If using the network ID, delete any spurious router ID that might + # have been in the request, same for network ID when using router ID. if self.network_id: cfg_info['res_type'] = 'Network' cfg_info['res_id'] = self.network_id + cfg_info['res_type_del'] = 'Router' else: cfg_info['res_type'] = 'Router' cfg_info['res_id'] = self.router_id + cfg_info['res_type_del'] = 'Network' haproxy_cfg = _HAPROXY_CONFIG_TEMPLATE % cfg_info LOG.debug("haproxy_cfg = %s", haproxy_cfg) diff --git a/neutron/tests/unit/agent/metadata/test_agent.py b/neutron/tests/unit/agent/metadata/test_agent.py index f783dbd61e9..1f1d3796474 100644 --- a/neutron/tests/unit/agent/metadata/test_agent.py +++ b/neutron/tests/unit/agent/metadata/test_agent.py @@ -242,6 +242,9 @@ class _TestMetadataProxyHandlerCacheMixin(object): expected = [] + if networks and router_id: + return (instance_id, tenant_id) + if router_id: expected.append( mock.call( @@ -333,6 +336,28 @@ class _TestMetadataProxyHandlerCacheMixin(object): (None, None) ) + def test_get_instance_id_network_id_and_router_id_invalid(self): + network_id = 'the_nid' + router_id = 'the_rid' + headers = { + 'X-Neutron-Network-ID': network_id, + 'X-Neutron-Router-ID': router_id + } + + # The call should never do a port lookup, but mock it to verify + ports = [ + [{'device_id': 'device_id', + 'tenant_id': 'tenant_id', + 'network_id': network_id}] + ] + + self.assertEqual( + self._get_instance_and_tenant_id_helper(headers, ports, + networks=(network_id,), + router_id=router_id), + (None, None) + ) + def _proxy_request_test_helper(self, response_code=200, method='GET'): hdrs = {'X-Forwarded-For': '8.8.8.8'} body = 'body' diff --git a/neutron/tests/unit/agent/metadata/test_driver.py b/neutron/tests/unit/agent/metadata/test_driver.py index d1fd47d5845..0a8e7e6fa28 100644 --- a/neutron/tests/unit/agent/metadata/test_driver.py +++ b/neutron/tests/unit/agent/metadata/test_driver.py @@ -157,6 +157,7 @@ class TestMetadataDriverProcess(base.BaseTestCase): 'unix_socket_path': self.METADATA_SOCKET, 'res_type': 'Router', 'res_id': router_id, + 'res_type_del': 'Network', 'pidfile': self.PIDFILE, 'log_level': 'debug', 'log_tag': log_tag} diff --git a/releasenotes/notes/metadata-proxy-header-vulnerability-60c44eb7c76d560c.yaml b/releasenotes/notes/metadata-proxy-header-vulnerability-60c44eb7c76d560c.yaml new file mode 100644 index 00000000000..1af9d8a468c --- /dev/null +++ b/releasenotes/notes/metadata-proxy-header-vulnerability-60c44eb7c76d560c.yaml @@ -0,0 +1,8 @@ +--- +security: + - | + A change was made to the metadata proxy to not allow a user to override + header values, it will now always insert the correct information and + remove unnecessary fields before sending requests to the metadata agent. + For more information, see bug + `1865036 `_.