From 1f1ba4ac5299f65f532a85e701feb8f40b937168 Mon Sep 17 00:00:00 2001 From: Kevin Benton Date: Fri, 25 Mar 2016 02:45:11 -0700 Subject: [PATCH 1/2] Linux Bridge: Add mac spoofing filtering to ebtables The current mac-spoofing code in iptables has two issues. First, it occurs after the address discovery allow rules (e.g. DHCP), so MAC addresses can be spoofed on discovery protocols. Second, since it is based on iptables, it doesn't apply to protocols like STP. This means a VM could generate one of these types of packets with a spoofed MAC address to trick switches into learning that the spoofed MAC now belongs to the VM's port. The impact of this depends on the configuration of the environment (e.g. use of L2pop: see the bug report for details). This patch adds MAC spoofing filtering to the ARP protection code for Linux bridge based on ebtables. Only traffic sourced from the MAC address on the port or in the allowed address pair MACs will be allowed. This filtering will not be enabled if the port has port security disabled or if the device_owner starts with 'network:'. Change-Id: I39dc0e23fc118ede19ef2d986b29fc5a8e48ff78 Partial-Bug: #1558658 --- .../ml2/drivers/linuxbridge/agent/arp_protect.py | 59 ++++++++++++++++++++++ .../agent/linux/test_linuxbridge_arp_protect.py | 36 ++++++++++++- 2 files changed, 94 insertions(+), 1 deletion(-) diff --git a/neutron/plugins/ml2/drivers/linuxbridge/agent/arp_protect.py b/neutron/plugins/ml2/drivers/linuxbridge/agent/arp_protect.py index 54daff1..5fc9665 100644 --- a/neutron/plugins/ml2/drivers/linuxbridge/agent/arp_protect.py +++ b/neutron/plugins/ml2/drivers/linuxbridge/agent/arp_protect.py @@ -23,6 +23,7 @@ from neutron.common import utils LOG = logging.getLogger(__name__) SPOOF_CHAIN_PREFIX = 'neutronARP-' +MAC_CHAIN_PREFIX = 'neutronMAC-' def setup_arp_spoofing_protection(vif, port_details): @@ -39,6 +40,7 @@ def setup_arp_spoofing_protection(vif, port_details): LOG.debug("Skipping ARP spoofing rules for network owned port " "'%s'.", vif) return + _install_mac_spoofing_protection(vif, port_details, current_rules) # collect all of the addresses and cidrs that belong to the port addresses = {f['ip_address'] for f in port_details['fixed_ips']} if port_details.get('allowed_address_pairs'): @@ -72,6 +74,7 @@ def delete_arp_spoofing_protection(vifs, current_rules=None): for vif in vifs: if chain_exists(chain_name(vif), current_rules): ebtables(['-X', chain_name(vif)]) + _delete_mac_spoofing_protection(vifs, current_rules) def delete_unreferenced_arp_protection(current_vifs): @@ -126,6 +129,62 @@ def vif_jump_present(vif, current_rules): return False +@lockutils.synchronized('ebtables') +def _install_mac_spoofing_protection(vif, port_details, current_rules): + mac_addresses = {port_details['mac_address']} + if port_details.get('allowed_address_pairs'): + mac_addresses |= {p['mac_address'] + for p in port_details['allowed_address_pairs']} + mac_addresses = list(mac_addresses) + vif_chain = _mac_chain_name(vif) + # mac filter chain for each vif which has a default deny + if not chain_exists(vif_chain, current_rules): + ebtables(['-N', vif_chain, '-P', 'DROP']) + # check if jump rule already exists, if not, install it + if not _mac_vif_jump_present(vif, current_rules): + ebtables(['-A', 'FORWARD', '-i', vif, '-j', vif_chain]) + # we can't just feed all allowed macs at once because we can exceed + # the maximum argument size. limit to 500 per rule. + for chunk in (mac_addresses[i:i + 500] + for i in range(0, len(mac_addresses), 500)): + new_rule = ['-A', vif_chain, '-i', vif, + '--among-src', ','.join(chunk), '-j', 'RETURN'] + ebtables(new_rule) + _delete_vif_mac_rules(vif, current_rules) + + +def _mac_vif_jump_present(vif, current_rules): + searches = (('-i %s' % vif), ('-j %s' % _mac_chain_name(vif))) + for line in current_rules: + if all(s in line for s in searches): + return True + return False + + +def _mac_chain_name(vif): + return '%s%s' % (MAC_CHAIN_PREFIX, vif) + + +def _delete_vif_mac_rules(vif, current_rules): + chain = _mac_chain_name(vif) + for rule in current_rules: + if '-i %s' % vif in rule and '--among-src' in rule: + ebtables(['-D', chain] + rule.split()) + + +def _delete_mac_spoofing_protection(vifs, current_rules): + # delete the jump rule and then delete the whole chain + jumps = [vif for vif in vifs + if _mac_vif_jump_present(vif, current_rules)] + for vif in jumps: + ebtables(['-D', 'FORWARD', '-i', vif, '-j', + _mac_chain_name(vif)]) + for vif in vifs: + chain = _mac_chain_name(vif) + if chain_exists(chain, current_rules): + ebtables(['-X', chain]) + + # Used to scope ebtables commands in testing NAMESPACE = None diff --git a/neutron/tests/functional/agent/linux/test_linuxbridge_arp_protect.py b/neutron/tests/functional/agent/linux/test_linuxbridge_arp_protect.py index dd4052c..02cd0f8 100644 --- a/neutron/tests/functional/agent/linux/test_linuxbridge_arp_protect.py +++ b/neutron/tests/functional/agent/linux/test_linuxbridge_arp_protect.py @@ -14,6 +14,7 @@ # under the License. from neutron.common import constants +from neutron.common import utils from neutron.plugins.ml2.drivers.linuxbridge.agent import arp_protect from neutron.tests.common import machine_fixtures from neutron.tests.common import net_helpers @@ -34,10 +35,17 @@ class LinuxBridgeARPSpoofTestCase(functional_base.BaseSudoTestCase): bridge = lbfixture.bridge self.source, self.destination, self.observer = self.useFixture( machine_fixtures.PeerMachines(bridge, amount=3)).machines + self.addCleanup(self._ensure_rules_cleaned) + + def _ensure_rules_cleaned(self): + rules = [r for r in arp_protect.ebtables(['-L']).splitlines() + if r and 'Bridge' not in r] + self.assertEqual([], rules, 'Test leaked ebtables rules') def _add_arp_protection(self, machine, addresses, extra_port_dict=None): port_dict = {'fixed_ips': [{'ip_address': a} for a in addresses], - 'device_owner': 'nobody'} + 'device_owner': 'nobody', + 'mac_address': machine.port.link.address} if extra_port_dict: port_dict.update(extra_port_dict) name = net_helpers.VethFixture.get_peer_name(machine.port.name) @@ -55,12 +63,38 @@ class LinuxBridgeARPSpoofTestCase(functional_base.BaseSudoTestCase): arping(self.source.namespace, self.destination.ip) arping(self.destination.namespace, self.source.ip) + def test_arp_correct_protection_allowed_address_pairs(self): + smac = self.source.port.link.address + port = {'mac_address': '00:11:22:33:44:55', + 'allowed_address_pairs': [{'mac_address': smac, + 'ip_address': self.source.ip}]} + # make sure a large number of allowed address pairs works + for i in range(100000): + port['allowed_address_pairs'].append( + {'mac_address': utils.get_random_mac( + 'fa:16:3e:00:00:00'.split(':')), + 'ip_address': '10.10.10.10'}) + self._add_arp_protection(self.source, ['1.2.2.2'], port) + self._add_arp_protection(self.destination, [self.destination.ip]) + arping(self.source.namespace, self.destination.ip) + arping(self.destination.namespace, self.source.ip) + def test_arp_fails_incorrect_protection(self): self._add_arp_protection(self.source, ['1.1.1.1']) self._add_arp_protection(self.destination, ['2.2.2.2']) no_arping(self.source.namespace, self.destination.ip) no_arping(self.destination.namespace, self.source.ip) + def test_arp_fails_incorrect_mac_protection(self): + # a bad mac filter on the source will prevent any traffic from it + self._add_arp_protection(self.source, [self.source.ip], + {'mac_address': '00:11:22:33:44:55'}) + no_arping(self.source.namespace, self.destination.ip) + no_arping(self.destination.namespace, self.source.ip) + # correcting it should make it work + self._add_arp_protection(self.source, [self.source.ip]) + arping(self.source.namespace, self.destination.ip) + def test_arp_protection_removal(self): self._add_arp_protection(self.source, ['1.1.1.1']) self._add_arp_protection(self.destination, ['2.2.2.2']) -- 1.9.1 From 3bcaa5229f2d82c3e6f667fa5383c56ab435e039 Mon Sep 17 00:00:00 2001 From: Kevin Benton Date: Fri, 25 Mar 2016 04:47:28 -0700 Subject: [PATCH 2/2] 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. Closes-Bug: #1558658 Change-Id: I02984b21872e0f183db7404c10d8180dbd89075f --- .../drivers/openvswitch/agent/common/constants.py | 3 ++ .../openvswitch/agent/openflow/native/br_int.py | 43 +++++++++++++++++++--- .../openvswitch/agent/openflow/ovs_ofctl/br_int.py | 36 ++++++++++++++++-- .../drivers/openvswitch/agent/ovs_neutron_agent.py | 4 ++ neutron/tests/functional/agent/test_ovs_flows.py | 15 +++++++- .../agent/openflow/native/test_br_int.py | 8 +--- .../agent/openflow/ovs_ofctl/test_br_int.py | 4 +- 7 files changed, 96 insertions(+), 17 deletions(-) diff --git a/neutron/plugins/ml2/drivers/openvswitch/agent/common/constants.py b/neutron/plugins/ml2/drivers/openvswitch/agent/common/constants.py index 6503970..e458ba0 100644 --- a/neutron/plugins/ml2/drivers/openvswitch/agent/common/constants.py +++ b/neutron/plugins/ml2/drivers/openvswitch/agent/common/constants.py @@ -50,6 +50,9 @@ CANARY_TABLE = 23 # Table for ARP poison/spoofing prevention rules ARP_SPOOF_TABLE = 24 +# Table for MAC spoof filtering +MAC_SPOOF_TABLE = 25 + # Tables used for ovs firewall BASE_EGRESS_TABLE = 71 RULES_EGRESS_TABLE = 72 diff --git a/neutron/plugins/ml2/drivers/openvswitch/agent/openflow/native/br_int.py b/neutron/plugins/ml2/drivers/openvswitch/agent/openflow/native/br_int.py index e7bfb0d..089a4e4 100644 --- a/neutron/plugins/ml2/drivers/openvswitch/agent/openflow/native/br_int.py +++ b/neutron/plugins/ml2/drivers/openvswitch/agent/openflow/native/br_int.py @@ -19,6 +19,8 @@ ** OVS agent https://wiki.openstack.org/wiki/Ovs-flow-logic """ +import netaddr + from oslo_log import log as logging from ryu.lib.packet import ether_types from ryu.lib.packet import icmpv6 @@ -174,16 +176,47 @@ class OVSIntegrationBridge(ovs_bridge.OVSAgentBridge): match=match, dest_table_id=constants.ARP_SPOOF_TABLE) + def set_allowed_macs_for_port(self, port, mac_addresses=None, + allow_all=False): + if allow_all: + self.delete_flows(table_id=constants.LOCAL_SWITCHING, in_port=port) + self.delete_flows(table_id=constants.MAC_SPOOF_TABLE, in_port=port) + return + mac_addresses = mac_addresses or [] + for address in mac_addresses: + self.install_normal( + table_id=constants.MAC_SPOOF_TABLE, priority=2, + eth_src=address, in_port=port) + # normalize so we can see if macs are the same + mac_addresses = {netaddr.EUI(mac) for mac in mac_addresses} + flows = self.dump_flows(constants.MAC_SPOOF_TABLE) + for flow in flows: + if flow.table_id != constants.MAC_SPOOF_TABLE: + continue + matches = dict(flow.match.items()) + if matches.get('in_port') != port: + continue + if not matches.get('eth_src'): + continue + flow_mac = matches['eth_src'] + if netaddr.EUI(flow_mac) not in mac_addresses: + self.delete_flows(table_id=constants.MAC_SPOOF_TABLE, + in_port=port, eth_src=flow_mac) + self.install_goto(table_id=constants.LOCAL_SWITCHING, + priority=9, in_port=port, + dest_table_id=constants.MAC_SPOOF_TABLE) + def install_arp_spoofing_protection(self, port, ip_addresses): # allow ARP replies as long as they match addresses that actually # belong to the port. for ip in ip_addresses: masked_ip = self._cidr_to_ryu(ip) - self.install_normal(table_id=constants.ARP_SPOOF_TABLE, - priority=2, - eth_type=ether_types.ETH_TYPE_ARP, - arp_spa=masked_ip, - in_port=port) + self.install_goto(table_id=constants.ARP_SPOOF_TABLE, + priority=2, + eth_type=ether_types.ETH_TYPE_ARP, + arp_spa=masked_ip, + in_port=port, + dest_table_id=constants.MAC_SPOOF_TABLE) # Now that the rules are ready, direct ARP traffic from the port into # the anti-spoof table. diff --git a/neutron/plugins/ml2/drivers/openvswitch/agent/openflow/ovs_ofctl/br_int.py b/neutron/plugins/ml2/drivers/openvswitch/agent/openflow/ovs_ofctl/br_int.py index a7c55fc..bc7a4eb 100644 --- a/neutron/plugins/ml2/drivers/openvswitch/agent/openflow/ovs_ofctl/br_int.py +++ b/neutron/plugins/ml2/drivers/openvswitch/agent/openflow/ovs_ofctl/br_int.py @@ -18,6 +18,9 @@ * references ** OVS agent https://wiki.openstack.org/wiki/Ovs-flow-logic """ + +import netaddr + from neutron.common import constants as const from neutron.plugins.common import constants as p_const from neutron.plugins.ml2.drivers.openvswitch.agent.common import constants @@ -128,13 +131,40 @@ class OVSIntegrationBridge(ovs_bridge.OVSAgentBridge): icmp_type=const.ICMPV6_TYPE_NA, in_port=port, actions=("resubmit(,%s)" % constants.ARP_SPOOF_TABLE)) + def set_allowed_macs_for_port(self, port, mac_addresses=None, + allow_all=False): + if allow_all: + self.delete_flows(table_id=constants.LOCAL_SWITCHING, in_port=port) + self.delete_flows(table_id=constants.MAC_SPOOF_TABLE, in_port=port) + return + mac_addresses = mac_addresses or [] + for address in mac_addresses: + self.install_normal( + table_id=constants.MAC_SPOOF_TABLE, priority=2, + eth_src=address, in_port=port) + # normalize so we can see if macs are the same + mac_addresses = {netaddr.EUI(mac) for mac in mac_addresses} + flows = self.dump_flows_for(table=constants.MAC_SPOOF_TABLE, + in_port=port).splitlines() + for flow in flows: + if 'dl_src' not in flow: + continue + flow_mac = flow.split('dl_src=')[1].split(' ')[0].split(',')[0] + if netaddr.EUI(flow_mac) not in mac_addresses: + self.delete_flows(table_id=constants.MAC_SPOOF_TABLE, + in_port=port, eth_src=flow_mac) + self.add_flow(table=constants.LOCAL_SWITCHING, + priority=9, in_port=port, + actions=("resubmit(,%s)" % constants.MAC_SPOOF_TABLE)) + def install_arp_spoofing_protection(self, port, ip_addresses): # allow ARPs as long as they match addresses that actually # belong to the port. for ip in ip_addresses: - self.install_normal( - table_id=constants.ARP_SPOOF_TABLE, priority=2, - proto='arp', arp_spa=ip, in_port=port) + self.add_flow( + table=constants.ARP_SPOOF_TABLE, priority=2, + proto='arp', arp_spa=ip, in_port=port, + actions=("resubmit(,%s)" % constants.MAC_SPOOF_TABLE)) # Now that the rules are ready, direct ARP traffic from the port into # the anti-spoof table. diff --git a/neutron/plugins/ml2/drivers/openvswitch/agent/ovs_neutron_agent.py b/neutron/plugins/ml2/drivers/openvswitch/agent/ovs_neutron_agent.py index 7effec2..cf0b1fd 100644 --- a/neutron/plugins/ml2/drivers/openvswitch/agent/ovs_neutron_agent.py +++ b/neutron/plugins/ml2/drivers/openvswitch/agent/ovs_neutron_agent.py @@ -888,12 +888,14 @@ class OVSNeutronAgent(sg_rpc.SecurityGroupAgentRpcCallbackMixin, LOG.info(_LI("Skipping ARP spoofing rules for port '%s' because " "it has port security disabled"), vif.port_name) bridge.delete_arp_spoofing_protection(port=vif.ofport) + bridge.set_allowed_macs_for_port(port=vif.ofport, allow_all=True) return if port_details['device_owner'].startswith( n_const.DEVICE_OWNER_NETWORK_PREFIX): LOG.debug("Skipping ARP spoofing rules for network owned port " "'%s'.", vif.port_name) bridge.delete_arp_spoofing_protection(port=vif.ofport) + bridge.set_allowed_macs_for_port(port=vif.ofport, allow_all=True) return # clear any previous flows related to this port in our ARP table bridge.delete_arp_spoofing_allow_rules(port=vif.ofport) @@ -907,6 +909,7 @@ class OVSNeutronAgent(sg_rpc.SecurityGroupAgentRpcCallbackMixin, for p in port_details['allowed_address_pairs'] if p.get('mac_address')} + bridge.set_allowed_macs_for_port(vif.ofport, mac_addresses) ipv6_addresses = {ip for ip in addresses if netaddr.IPNetwork(ip).version == 6} # Allow neighbor advertisements for LLA address. @@ -1169,6 +1172,7 @@ class OVSNeutronAgent(sg_rpc.SecurityGroupAgentRpcCallbackMixin, ofports_deleted = set(previous.values()) - set(current.values()) for ofport in ofports_deleted: self.int_br.delete_arp_spoofing_protection(port=ofport) + self.int_br.set_allowed_macs_for_port(port=ofport, allow_all=True) # store map for next iteration self.vifname_to_ofport_map = current diff --git a/neutron/tests/functional/agent/test_ovs_flows.py b/neutron/tests/functional/agent/test_ovs_flows.py index d24cf2b..d2854dc 100644 --- a/neutron/tests/functional/agent/test_ovs_flows.py +++ b/neutron/tests/functional/agent/test_ovs_flows.py @@ -184,6 +184,17 @@ class ARPSpoofTestCase(OVSAgentTestBase): self.dst_p.addr.add('%s/24' % self.dst_addr) net_helpers.assert_ping(self.src_namespace, self.dst_addr, count=2) + 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) + net_helpers.assert_ping(self.src_namespace, self.dst_addr, count=2) + # 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') + net_helpers.assert_no_ping(self.src_namespace, self.dst_addr, count=2) + def test_arp_spoof_doesnt_block_ipv6(self): self.src_addr = '2000::1' self.dst_addr = '2000::2' @@ -282,7 +293,7 @@ class ARPSpoofTestCase(OVSAgentTestBase): net_helpers.assert_ping(self.src_namespace, self.dst_addr, count=2) def _setup_arp_spoof_for_port(self, port, addrs, psec=True, - device_owner='nobody'): + device_owner='nobody', mac=None): vif = next( vif for vif in self.br.get_vif_ports() if vif.port_name == port) ip_addr = addrs.pop() @@ -291,6 +302,8 @@ class ARPSpoofTestCase(OVSAgentTestBase): 'device_owner': device_owner, 'allowed_address_pairs': [ dict(ip_address=ip) for ip in addrs]} + if mac: + vif.vif_mac = mac ovsagt.OVSNeutronAgent.setup_arp_spoofing_protection( self.br_int, vif, details) diff --git a/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/openflow/native/test_br_int.py b/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/openflow/native/test_br_int.py index 32086ff..13b56fb 100644 --- a/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/openflow/native/test_br_int.py +++ b/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/openflow/native/test_br_int.py @@ -347,9 +347,7 @@ class OVSIntegrationBridgeTest(ovs_bridge_test_base.OVSBridgeTestBase): call._send_msg(ofpp.OFPFlowMod(dp, cookie=self.stamp, instructions=[ - ofpp.OFPInstructionActions(ofp.OFPIT_APPLY_ACTIONS, [ - ofpp.OFPActionOutput(ofp.OFPP_NORMAL, 0), - ]), + ofpp.OFPInstructionGotoTable(table_id=25), ], match=ofpp.OFPMatch( eth_type=self.ether_types.ETH_TYPE_ARP, @@ -361,9 +359,7 @@ class OVSIntegrationBridgeTest(ovs_bridge_test_base.OVSBridgeTestBase): call._send_msg(ofpp.OFPFlowMod(dp, cookie=self.stamp, instructions=[ - ofpp.OFPInstructionActions(ofp.OFPIT_APPLY_ACTIONS, [ - ofpp.OFPActionOutput(ofp.OFPP_NORMAL, 0), - ]), + ofpp.OFPInstructionGotoTable(table_id=25), ], match=ofpp.OFPMatch( eth_type=self.ether_types.ETH_TYPE_ARP, diff --git a/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/openflow/ovs_ofctl/test_br_int.py b/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/openflow/ovs_ofctl/test_br_int.py index 51892fd..5dc9a0c 100644 --- a/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/openflow/ovs_ofctl/test_br_int.py +++ b/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/openflow/ovs_ofctl/test_br_int.py @@ -215,10 +215,10 @@ class OVSIntegrationBridgeTest(ovs_bridge_test_base.OVSBridgeTestBase): ip_addresses = ['192.0.2.1', '192.0.2.2/32'] self.br.install_arp_spoofing_protection(port, ip_addresses) expected = [ - call.add_flow(proto='arp', actions='normal', + call.add_flow(proto='arp', actions='resubmit(,25)', arp_spa='192.0.2.1', priority=2, table=24, in_port=8888), - call.add_flow(proto='arp', actions='normal', + call.add_flow(proto='arp', actions='resubmit(,25)', arp_spa='192.0.2.2/32', priority=2, table=24, in_port=8888), call.add_flow(priority=10, table=0, in_port=8888, -- 1.9.1