From f0415ac20eaf5ab4abb9bd4839bf6d04ceee85d0 Mon Sep 17 00:00:00 2001 From: armando-migliaccio Date: Fri, 28 Aug 2015 13:53:04 -0700 Subject: [PATCH] Revert "Add support for unaddressed port" This implementation may expose a vulnerability where a malicious user can sieze the opportunity of a time window where a port may land unaddressed on a shared network, thus allowing him/her to suck up all the tenant traffic he/she wants....oh the shivers. This reverts commit d4c52b7f5a36a103a92bf9dcda7f371959112292. Change-Id: I7ebdaa8d3defa80eab90e460fde541a5bdd8864c --- neutron/agent/linux/iptables_firewall.py | 48 +++++++------------ .../openvswitch/agent/ovs_neutron_agent.py | 4 -- .../agent/linux/test_iptables_firewall.py | 27 +++++++++++ .../agent/test_ovs_neutron_agent.py | 15 ++---- 4 files changed, 47 insertions(+), 47 deletions(-) diff --git a/neutron/agent/linux/iptables_firewall.py b/neutron/agent/linux/iptables_firewall.py index a695733e8..339b9370f 100644 --- a/neutron/agent/linux/iptables_firewall.py +++ b/neutron/agent/linux/iptables_firewall.py @@ -50,13 +50,6 @@ MAX_CONNTRACK_ZONES = 65535 comment_rule = iptables_manager.comment_rule -def port_needs_l3_security(port): - if port['fixed_ips'] or port.get('allowed_address_pairs'): - return True - else: - return False - - class IptablesFirewallDriver(firewall.FirewallDriver): """Driver which enforces security groups through iptables rules.""" IPTABLES_DIRECTION = {firewall.INGRESS_DIRECTION: 'physdev-out', @@ -381,20 +374,17 @@ class IptablesFirewallDriver(firewall.FirewallDriver): mac_ipv6_pairs.append((mac, ip_address)) def _spoofing_rule(self, port, ipv4_rules, ipv6_rules): - if port_needs_l3_security(port): - # Allow dhcp client packets - ipv4_rules += [comment_rule('-p udp -m udp --sport 68 --dport 67 ' - '-j RETURN', comment=ic.DHCP_CLIENT)] - # Drop Router Advts from the port. - ipv6_rules += [comment_rule('-p icmpv6 --icmpv6-type %s ' - '-j DROP' % constants.ICMPV6_TYPE_RA, - comment=ic.IPV6_RA_DROP)] - ipv6_rules += [comment_rule('-p icmpv6 -j RETURN', - comment=ic.IPV6_ICMP_ALLOW)] - ipv6_rules += [comment_rule('-p udp -m udp --sport 546 --dport ' - '547 -j RETURN', - comment=ic.DHCP_CLIENT)] - + # Allow dhcp client packets + ipv4_rules += [comment_rule('-p udp -m udp --sport 68 --dport 67 ' + '-j RETURN', comment=ic.DHCP_CLIENT)] + # Drop Router Advts from the port. + ipv6_rules += [comment_rule('-p icmpv6 --icmpv6-type %s ' + '-j DROP' % constants.ICMPV6_TYPE_RA, + comment=ic.IPV6_RA_DROP)] + ipv6_rules += [comment_rule('-p icmpv6 -j RETURN', + comment=ic.IPV6_ICMP_ALLOW)] + ipv6_rules += [comment_rule('-p udp -m udp --sport 546 --dport 547 ' + '-j RETURN', comment=ic.DHCP_CLIENT)] mac_ipv4_pairs = [] mac_ipv6_pairs = [] @@ -500,14 +490,11 @@ class IptablesFirewallDriver(firewall.FirewallDriver): ipv6_iptables_rules) elif direction == firewall.INGRESS_DIRECTION: ipv6_iptables_rules += self._accept_inbound_icmpv6() - - if port_needs_l3_security(port): - # include IPv4 and IPv6 iptable rules from security group - ipv4_iptables_rules += self._convert_sgr_to_iptables_rules( - ipv4_sg_rules) - ipv6_iptables_rules += self._convert_sgr_to_iptables_rules( - ipv6_sg_rules) - + # include IPv4 and IPv6 iptable rules from security group + ipv4_iptables_rules += self._convert_sgr_to_iptables_rules( + ipv4_sg_rules) + ipv6_iptables_rules += self._convert_sgr_to_iptables_rules( + ipv6_sg_rules) # finally add the rules to the port chain for a given direction self._add_rules_to_chain_v4v6(self._port_chain_name(port, direction), ipv4_iptables_rules, @@ -518,8 +505,7 @@ class IptablesFirewallDriver(firewall.FirewallDriver): self._spoofing_rule(port, ipv4_iptables_rules, ipv6_iptables_rules) - if port_needs_l3_security(port): - self._drop_dhcp_rule(ipv4_iptables_rules, ipv6_iptables_rules) + self._drop_dhcp_rule(ipv4_iptables_rules, ipv6_iptables_rules) def _update_ipset_members(self, security_group_ids): for ip_version, sg_ids in security_group_ids.items(): 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 0c590e38c..e76bc77b3 100644 --- a/neutron/plugins/ml2/drivers/openvswitch/agent/ovs_neutron_agent.py +++ b/neutron/plugins/ml2/drivers/openvswitch/agent/ovs_neutron_agent.py @@ -33,7 +33,6 @@ from neutron.agent.common import polling from neutron.agent.common import utils from neutron.agent.l2.extensions import manager as ext_manager from neutron.agent.linux import ip_lib -from neutron.agent.linux.iptables_firewall import port_needs_l3_security from neutron.agent import rpc as agent_rpc from neutron.agent import securitygroups_rpc as sg_rpc from neutron.api.rpc.handlers import dvr_rpc @@ -829,9 +828,6 @@ class OVSNeutronAgent(sg_rpc.SecurityGroupAgentRpcCallbackMixin, addresses |= {p['ip_address'] for p in port_details['allowed_address_pairs']} - if not port_needs_l3_security(port_details): - return - addresses = {ip for ip in addresses if netaddr.IPNetwork(ip).version == 4} if any(netaddr.IPNetwork(ip).prefixlen == 0 for ip in addresses): diff --git a/neutron/tests/unit/agent/linux/test_iptables_firewall.py b/neutron/tests/unit/agent/linux/test_iptables_firewall.py index bf183bba5..61494d851 100644 --- a/neutron/tests/unit/agent/linux/test_iptables_firewall.py +++ b/neutron/tests/unit/agent/linux/test_iptables_firewall.py @@ -1475,6 +1475,15 @@ class IptablesFirewallTestCase(BaseIptablesFirewallTestCase): '--physdev-is-bridged ' '-j $ifake_dev', comment=ic.SG_TO_VM_SG), + mock.call.add_rule( + 'ifake_dev', + '-m state --state INVALID -j DROP', comment=None), + mock.call.add_rule( + 'ifake_dev', + '-m state --state RELATED,ESTABLISHED -j RETURN', + comment=None), + mock.call.add_rule('ifake_dev', '-j $sg-fallback', + comment=None), mock.call.add_chain('ofake_dev'), mock.call.add_rule('FORWARD', '-m physdev --physdev-in tapfake_dev ' @@ -1496,8 +1505,26 @@ class IptablesFirewallTestCase(BaseIptablesFirewallTestCase): mock.call.add_rule( 'sfake_dev', '-j DROP', comment=ic.PAIR_DROP), + mock.call.add_rule( + 'ofake_dev', + '-p udp -m udp --sport 68 --dport 67 -j RETURN', + comment=None), mock.call.add_rule('ofake_dev', '-j $sfake_dev', comment=None), + mock.call.add_rule( + 'ofake_dev', + '-p udp -m udp --sport 67 --dport 68 -j DROP', + comment=None), + mock.call.add_rule( + 'ofake_dev', + '-m state --state INVALID -j DROP', + comment=None), + mock.call.add_rule( + 'ofake_dev', + '-m state --state RELATED,ESTABLISHED -j RETURN', + comment=None), + mock.call.add_rule('ofake_dev', '-j $sg-fallback', + comment=None), mock.call.add_rule('sg-chain', '-j ACCEPT')] self.v4filter_inst.assert_has_calls(calls) diff --git a/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/test_ovs_neutron_agent.py b/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/test_ovs_neutron_agent.py index c21381487..72b5e2992 100644 --- a/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/test_ovs_neutron_agent.py +++ b/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/test_ovs_neutron_agent.py @@ -1291,7 +1291,7 @@ class TestOvsNeutronAgent(object): self.assertTrue(int_br.delete_arp_spoofing_protection.called) self.assertFalse(int_br.install_arp_spoofing_protection.called) - def test_arp_spoofing_basic_rule_setup_without_ip(self): + def test_arp_spoofing_basic_rule_setup(self): vif = FakeVif() fake_details = {'fixed_ips': []} self.agent.prevent_arp_spoofing = True @@ -1300,18 +1300,9 @@ class TestOvsNeutronAgent(object): self.assertEqual( [mock.call(port=vif.ofport)], int_br.delete_arp_spoofing_protection.mock_calls) - self.assertFalse(int_br.install_arp_spoofing_protection.called) - - def test_arp_spoofing_basic_rule_setup_fixed_ip(self): - vif = FakeVif() - fake_details = {'fixed_ips': [{'ip_address': '192.168.44.100'}]} - self.agent.prevent_arp_spoofing = True - int_br = mock.create_autospec(self.agent.int_br) - self.agent.setup_arp_spoofing_protection(int_br, vif, fake_details) self.assertEqual( - [mock.call(port=vif.ofport)], - int_br.delete_arp_spoofing_protection.mock_calls) - self.assertTrue(int_br.install_arp_spoofing_protection.called) + [mock.call(ip_addresses=set(), port=vif.ofport)], + int_br.install_arp_spoofing_protection.mock_calls) def test_arp_spoofing_fixed_and_allowed_addresses(self): vif = FakeVif() -- 2.45.2