From d48147b0fc192c49646951294479573e4ed9ae78 Mon Sep 17 00:00:00 2001 From: Kevin Benton Date: Mon, 28 Mar 2016 22:21:53 -0700 Subject: [PATCH] OVS: Add mac spoofing filtering to flows The mac-spoofing filtering done by iptables was not adequate. See the bug report and change I39dc0e23fc118ede19ef2d986b29fc5a8e48ff78 for more information. This patch adds flows to the OVS agent to block any traffic from the VM that isn't in the allowed address pairs macs or the mac address field of the port. Conflicts: This had to be heavily modified to work with Kilo due to the lack of a pluggable openflow interface that was present in master, mitaka, and liberty. Closes-Bug: #1558658 Change-Id: I02984b21872e0f183db7404c10d8180dbd89075f (cherry-picked from 997d7b03fb7f5528f0a3ce70867b9dcd9321509e) --- .../openvswitch/agent/ovs_neutron_agent.py | 46 +++++++++++++++++-- .../plugins/openvswitch/common/constants.py | 3 ++ .../tests/functional/agent/test_ovs_flows.py | 32 ++++++++++++- .../agent/test_ovs_neutron_agent.py | 11 +++-- 4 files changed, 84 insertions(+), 8 deletions(-) diff --git a/neutron/plugins/openvswitch/agent/ovs_neutron_agent.py b/neutron/plugins/openvswitch/agent/ovs_neutron_agent.py index 67aa40bbf9c..477678ee88a 100644 --- a/neutron/plugins/openvswitch/agent/ovs_neutron_agent.py +++ b/neutron/plugins/openvswitch/agent/ovs_neutron_agent.py @@ -734,20 +734,22 @@ class OVSNeutronAgent(sg_rpc.SecurityGroupAgentRpcCallbackMixin, if port.ofport != -1: self.int_br.delete_flows(in_port=port.ofport) - @staticmethod - def setup_arp_spoofing_protection(bridge, vif, port_details): + @classmethod + def setup_arp_spoofing_protection(cls, bridge, vif, port_details): # clear any previous flows related to this port in our ARP table bridge.delete_flows(table=constants.ARP_SPOOF_TABLE, in_port=vif.ofport) if not port_details.get('port_security_enabled', True): bridge.delete_flows(table=constants.LOCAL_SWITCHING, in_port=vif.ofport, proto='arp') + cls.set_allowed_macs_for_port(bridge, vif.ofport, allow_all=True) LOG.info(_LI("Skipping ARP spoofing rules for port '%s' because " "it has port security disabled"), vif.port_name) return if port_details['device_owner'].startswith('network:'): bridge.delete_flows(table=constants.LOCAL_SWITCHING, in_port=vif.ofport, proto='arp') + cls.set_allowed_macs_for_port(bridge, vif.ofport, allow_all=True) LOG.debug("Skipping ARP spoofing rules for network owned port " "'%s'.", vif.port_name) return @@ -756,9 +758,14 @@ class OVSNeutronAgent(sg_rpc.SecurityGroupAgentRpcCallbackMixin, # collect all of the addresses and cidrs that belong to the port addresses = [f['ip_address'] for f in port_details['fixed_ips']] + mac_addresses = {vif.vif_mac} if port_details.get('allowed_address_pairs'): addresses += [p['ip_address'] for p in port_details['allowed_address_pairs']] + mac_addresses |= {p['mac_address'] + for p in port_details['allowed_address_pairs']} + + cls.set_allowed_macs_for_port(bridge, vif.ofport, mac_addresses) if any(netaddr.IPNetwork(ip).prefixlen == 0 for ip in addresses): # don't try to install protection because a /0 prefix allows any # address anyway and the ARP_SPA can only match on /1 or more. @@ -771,9 +778,13 @@ class OVSNeutronAgent(sg_rpc.SecurityGroupAgentRpcCallbackMixin, for ip in addresses: if netaddr.IPNetwork(ip).version != 4: continue + if cfg.CONF.AGENT.enable_mac_spoofing_protection: + action = "resubmit(,%s)" % constants.MAC_SPOOF_TABLE + else: + action = "NORMAL" bridge.add_flow(table=constants.ARP_SPOOF_TABLE, priority=2, proto='arp', arp_spa=ip, in_port=vif.ofport, - actions="NORMAL") + actions=action) # drop any ARPs in this table that aren't explicitly allowed bridge.add_flow(table=constants.ARP_SPOOF_TABLE, priority=1, @@ -787,6 +798,34 @@ class OVSNeutronAgent(sg_rpc.SecurityGroupAgentRpcCallbackMixin, priority=10, proto='arp', in_port=vif.ofport, actions=("resubmit(,%s)" % constants.ARP_SPOOF_TABLE)) + @staticmethod + def set_allowed_macs_for_port(br, port, mac_addresses=None, + allow_all=False): + if not cfg.CONF.AGENT.enable_mac_spoofing_protection: + return + if allow_all: + br.delete_flows(table=constants.LOCAL_SWITCHING, in_port=port) + br.delete_flows(table=constants.MAC_SPOOF_TABLE, in_port=port) + return + mac_addresses = mac_addresses or [] + for address in mac_addresses: + br.add_flow(table=constants.MAC_SPOOF_TABLE, priority=2, + eth_src=address, in_port=port, + actions="NORMAL") + # normalize so we can see if macs are the same + mac_addresses = {netaddr.EUI(mac) for mac in mac_addresses} + flows = br.dump_flows_for_table(constants.MAC_SPOOF_TABLE).splitlines() + for flow in flows: + if 'dl_src' not in flow or 'in_port=%s' % port not in flow: + continue + flow_mac = flow.split('dl_src=')[1].split(' ')[0].split(',')[0] + if netaddr.EUI(flow_mac) not in mac_addresses: + br.delete_flows(table=constants.MAC_SPOOF_TABLE, + in_port=port, eth_src=flow_mac) + br.add_flow(table=constants.LOCAL_SWITCHING, + priority=9, in_port=port, + actions=("resubmit(,%s)" % constants.MAC_SPOOF_TABLE)) + def port_unbound(self, vif_id, net_uuid=None): '''Unbind port. @@ -1752,6 +1791,7 @@ def create_agent_config_map(config): def main(): cfg.CONF.register_opts(ip_lib.OPTS) config.register_root_helper(cfg.CONF) + config.register_spoofing_opts(cfg.CONF) common_config.init(sys.argv[1:]) common_config.setup_logging() q_utils.log_opt_values(LOG) diff --git a/neutron/plugins/openvswitch/common/constants.py b/neutron/plugins/openvswitch/common/constants.py index 40fa8f0f07f..425cf60885c 100644 --- a/neutron/plugins/openvswitch/common/constants.py +++ b/neutron/plugins/openvswitch/common/constants.py @@ -62,6 +62,9 @@ CANARY_TABLE = 23 # Table for ARP poison/spoofing prevention rules ARP_SPOOF_TABLE = 24 +# Table for MAC spoof filtering +MAC_SPOOF_TABLE = 25 + # type for ARP reply in ARP header ARP_REPLY = '0x2' diff --git a/neutron/tests/functional/agent/test_ovs_flows.py b/neutron/tests/functional/agent/test_ovs_flows.py index cccdac90f39..ebaefa07461 100644 --- a/neutron/tests/functional/agent/test_ovs_flows.py +++ b/neutron/tests/functional/agent/test_ovs_flows.py @@ -13,6 +13,8 @@ # License for the specific language governing permissions and limitations # under the License. +from oslo_config import cfg + from neutron.agent.linux import ip_lib from neutron.cmd.sanity import checks from neutron.plugins.openvswitch.agent import ovs_neutron_agent as ovsagt @@ -52,6 +54,26 @@ class ARPSpoofTestCase(test_ovs_lib.OVSBridgeTestBase, self.dst_p.addr.add('%s/24' % self.dst_addr) self.pinger.assert_ping(self.dst_addr) + def test_mac_spoof_blocks_wrong_mac(self): + self._setup_arp_spoof_for_port(self.src_p.name, [self.src_addr]) + self._setup_arp_spoof_for_port(self.dst_p.name, [self.dst_addr]) + self.src_p.addr.add('%s/24' % self.src_addr) + self.dst_p.addr.add('%s/24' % self.dst_addr) + self.pinger.assert_ping(self.dst_addr) + # changing the allowed mac should stop the port from working + self._setup_arp_spoof_for_port(self.src_p.name, [self.src_addr], + mac='00:11:22:33:44:55') + self.pinger.assert_no_ping(self.dst_addr) + + def test_mac_spoof_disabled_allows_wrong_mac(self): + cfg.CONF.set_override('enable_mac_spoofing_protection', False, 'AGENT') + self._setup_arp_spoof_for_port(self.src_p.name, [self.src_addr], + mac='00:11:22:33:44:55') + self._setup_arp_spoof_for_port(self.dst_p.name, [self.dst_addr]) + self.src_p.addr.add('%s/24' % self.src_addr) + self.dst_p.addr.add('%s/24' % self.dst_addr) + self.pinger.assert_ping(self.dst_addr) + def test_arp_spoof_doesnt_block_ipv6(self): self.src_addr = '2000::1' self.dst_addr = '2000::2' @@ -122,18 +144,24 @@ class ARPSpoofTestCase(test_ovs_lib.OVSBridgeTestBase, self.pinger.assert_ping(self.dst_addr) def _setup_arp_spoof_for_port(self, port, addrs, psec=True, - device_owner='nobody'): + device_owner='nobody', mac=None): of_port_map = self.br.get_vif_port_to_ofport_map() class VifPort(object): ofport = of_port_map[port] port_name = port + # ugly hack to deal with the fact that we can't retrieve macs + # from OVSDB in kilo + vif_mac = mac or ( + self.src_p.link.address if port == self.src_p.name + else self.dst_p.link.address) ip_addr = addrs.pop() details = {'port_security_enabled': psec, 'fixed_ips': [{'ip_address': ip_addr}], 'device_owner': device_owner, 'allowed_address_pairs': [ - dict(ip_address=ip) for ip in addrs]} + dict(ip_address=ip, mac_address=VifPort.vif_mac) + for ip in addrs]} ovsagt.OVSNeutronAgent.setup_arp_spoofing_protection( self.br, VifPort(), details) diff --git a/neutron/tests/unit/plugins/openvswitch/agent/test_ovs_neutron_agent.py b/neutron/tests/unit/plugins/openvswitch/agent/test_ovs_neutron_agent.py index 53a1b415e72..75432f8eb56 100644 --- a/neutron/tests/unit/plugins/openvswitch/agent/test_ovs_neutron_agent.py +++ b/neutron/tests/unit/plugins/openvswitch/agent/test_ovs_neutron_agent.py @@ -46,6 +46,7 @@ FAKE_IP2 = '10.0.0.2' class FakeVif(object): ofport = 99 port_name = 'name' + vif_mac = '00:22:44:66:88:AA' class CreateAgentConfigMap(base.BaseTestCase): @@ -1165,6 +1166,7 @@ class TestOvsNeutronAgent(base.BaseTestCase): fake_details = {'fixed_ips': [], 'device_owner': 'nobody'} self.agent.prevent_arp_spoofing = True int_br = mock.Mock() + int_br.dump_flows_for_table.return_value = '' self.agent.setup_arp_spoofing_protection(int_br, vif, fake_details) int_br.delete_flows.assert_has_calls( [mock.call(table=mock.ANY, in_port=vif.ofport)]) @@ -1183,18 +1185,21 @@ class TestOvsNeutronAgent(base.BaseTestCase): 'device_owner': 'nobody', 'fixed_ips': [{'ip_address': '192.168.44.100'}, {'ip_address': '192.168.44.101'}], - 'allowed_address_pairs': [{'ip_address': '192.168.44.102/32'}, - {'ip_address': '192.168.44.103/32'}] + 'allowed_address_pairs': [{'ip_address': '192.168.44.102/32', + 'mac_address': FAKE_MAC}, + {'ip_address': '192.168.44.103/32', + 'mac_address': FAKE_MAC}] } self.agent.prevent_arp_spoofing = True int_br = mock.Mock() + int_br.dump_flows_for_table.return_value = '' self.agent.setup_arp_spoofing_protection(int_br, vif, fake_details) # make sure all addresses are allowed for addr in ('192.168.44.100', '192.168.44.101', '192.168.44.102/32', '192.168.44.103/32'): int_br.add_flow.assert_any_call( table=constants.ARP_SPOOF_TABLE, in_port=vif.ofport, - proto='arp', actions='NORMAL', arp_spa=addr, priority=mock.ANY) + proto='arp', actions=mock.ANY, arp_spa=addr, priority=mock.ANY) def test__get_ofport_moves(self): previous = {'port1': 1, 'port2': 2}