From 8138e2fe38ad2cde5963685df47b1e4286776352 Mon Sep 17 00:00:00 2001 From: Kevin Benton Date: Tue, 25 Aug 2015 22:03:27 -0700 Subject: [PATCH] Stop device_owner from being set to 'network:*' This patch adjusts the FieldCheck class in the policy engine to allow a regex rule. It then leverages that to prevent users from setting the device_owner field to anything that starts with 'network:' on networks which they do not own. This policy adjustment is necessary because any ports with a device_owner that starts with 'network:' will not have any security group rules applied because it is assumed they are trusted network devices (e.g. router ports, DHCP ports, etc). These security rules include the anti-spoofing protection for DHCP, IPv6 ICMP messages, and IP headers. Without this policy adjustment, tenants can abuse this trust when connected to a shared network with other tenants by setting their VM port's device_owner field to 'network:' and hijack other tenants' traffic via DHCP spoofing or MAC/IP spoofing. Closes-Bug: #1489111 Change-Id: Ia64cf16142e0e4be44b5b0ed72c8e00792d770f9 (cherry picked from commit 959a2f28cbbfc309381ea9ffb55090da6fb9c78f) --- etc/policy.json | 3 +++ neutron/api/v2/attributes.py | 2 +- neutron/policy.py | 3 +++ neutron/tests/etc/policy.json | 3 +++ neutron/tests/unit/test_policy.py | 16 ++++++++++++++++ 5 files changed, 26 insertions(+), 1 deletion(-) diff --git a/etc/policy.json b/etc/policy.json index 8a5de9b..0f04eb2 100644 --- a/etc/policy.json +++ b/etc/policy.json @@ -46,7 +46,9 @@ "update_network:router:external": "rule:admin_only", "delete_network": "rule:admin_or_owner", + "network_device": "field:port:device_owner=~^network:", "create_port": "", + "create_port:device_owner": "not rule:network_device or rule:admin_or_network_owner or rule:context_is_advsvc", "create_port:mac_address": "rule:admin_or_network_owner or rule:context_is_advsvc", "create_port:fixed_ips": "rule:admin_or_network_owner or rule:context_is_advsvc", "create_port:port_security_enabled": "rule:admin_or_network_owner or rule:context_is_advsvc", @@ -61,6 +63,7 @@ "get_port:binding:host_id": "rule:admin_only", "get_port:binding:profile": "rule:admin_only", "update_port": "rule:admin_or_owner or rule:context_is_advsvc", + "update_port:device_owner": "not rule:network_device or rule:admin_or_network_owner or rule:context_is_advsvc", "update_port:mac_address": "rule:admin_only or rule:context_is_advsvc", "update_port:fixed_ips": "rule:admin_or_network_owner or rule:context_is_advsvc", "update_port:port_security_enabled": "rule:admin_or_network_owner or rule:context_is_advsvc", diff --git a/neutron/api/v2/attributes.py b/neutron/api/v2/attributes.py index b9c179a..9ceee78 100644 --- a/neutron/api/v2/attributes.py +++ b/neutron/api/v2/attributes.py @@ -766,7 +766,7 @@ RESOURCE_ATTRIBUTE_MAP = { 'is_visible': True}, 'device_owner': {'allow_post': True, 'allow_put': True, 'validate': {'type:string': DEVICE_OWNER_MAX_LEN}, - 'default': '', + 'default': '', 'enforce_policy': True, 'is_visible': True}, 'tenant_id': {'allow_post': True, 'allow_put': False, 'validate': {'type:string': TENANT_ID_MAX_LEN}, diff --git a/neutron/policy.py b/neutron/policy.py index 9e586dd..961ae21 100644 --- a/neutron/policy.py +++ b/neutron/policy.py @@ -335,6 +335,7 @@ class FieldCheck(policy.Check): self.field = field self.value = conv_func(value) + self.regex = re.compile(value[1:]) if value.startswith('~') else None def __call__(self, target_dict, cred_dict, enforcer): target_value = target_dict.get(self.field) @@ -344,6 +345,8 @@ class FieldCheck(policy.Check): "%(target_dict)s", {'field': self.field, 'target_dict': target_dict}) return False + if self.regex: + return bool(self.regex.match(target_value)) return target_value == self.value diff --git a/neutron/tests/etc/policy.json b/neutron/tests/etc/policy.json index 8a5de9b..0f04eb2 100644 --- a/neutron/tests/etc/policy.json +++ b/neutron/tests/etc/policy.json @@ -46,7 +46,9 @@ "update_network:router:external": "rule:admin_only", "delete_network": "rule:admin_or_owner", + "network_device": "field:port:device_owner=~^network:", "create_port": "", + "create_port:device_owner": "not rule:network_device or rule:admin_or_network_owner or rule:context_is_advsvc", "create_port:mac_address": "rule:admin_or_network_owner or rule:context_is_advsvc", "create_port:fixed_ips": "rule:admin_or_network_owner or rule:context_is_advsvc", "create_port:port_security_enabled": "rule:admin_or_network_owner or rule:context_is_advsvc", @@ -61,6 +63,7 @@ "get_port:binding:host_id": "rule:admin_only", "get_port:binding:profile": "rule:admin_only", "update_port": "rule:admin_or_owner or rule:context_is_advsvc", + "update_port:device_owner": "not rule:network_device or rule:admin_or_network_owner or rule:context_is_advsvc", "update_port:mac_address": "rule:admin_only or rule:context_is_advsvc", "update_port:fixed_ips": "rule:admin_or_network_owner or rule:context_is_advsvc", "update_port:port_security_enabled": "rule:admin_or_network_owner or rule:context_is_advsvc", diff --git a/neutron/tests/unit/test_policy.py b/neutron/tests/unit/test_policy.py index 3888ce3..4be404f 100644 --- a/neutron/tests/unit/test_policy.py +++ b/neutron/tests/unit/test_policy.py @@ -232,6 +232,7 @@ class NeutronPolicyTestCase(base.BaseTestCase): "regular_user": "role:user", "shared": "field:networks:shared=True", "external": "field:networks:router:external=True", + "network_device": "field:port:device_owner=~^network:", "default": '@', "create_network": "rule:admin_or_owner", @@ -243,6 +244,7 @@ class NeutronPolicyTestCase(base.BaseTestCase): "create_subnet": "rule:admin_or_network_owner", "create_port:mac": "rule:admin_or_network_owner or " "rule:context_is_advsvc", + "create_port:device_owner": "not rule:network_device", "update_port": "rule:admin_or_owner or rule:context_is_advsvc", "get_port": "rule:admin_or_owner or rule:context_is_advsvc", "delete_port": "rule:admin_or_owner or rule:context_is_advsvc", @@ -312,6 +314,20 @@ class NeutronPolicyTestCase(base.BaseTestCase): self._test_nonadmin_action_on_attr('create', 'shared', True, common_policy.PolicyNotAuthorized) + def test_create_port_device_owner_regex(self): + blocked_values = ('network:', 'network:abdef', 'network:dhcp', + 'network:router_interface') + for val in blocked_values: + self._test_advsvc_action_on_attr( + 'create', 'port', 'device_owner', val, + common_policy.PolicyNotAuthorized + ) + ok_values = ('network', 'networks', 'my_network:test', 'my_network:') + for val in ok_values: + self._test_advsvc_action_on_attr( + 'create', 'port', 'device_owner', val + ) + def test_advsvc_get_network_works(self): self._test_advsvc_action_on_attr('get', 'network', 'shared', False) -- 1.9.1