From 6f3ae396ed9a9525d4ca77fb8e9b2857fe79b568 Mon Sep 17 00:00:00 2001 From: Bertrand Lallau Date: Tue, 9 Sep 2014 14:56:59 +0200 Subject: [PATCH] Don't allow user to set firewall rule with port and no protocol Creating firewall rules specifying a destination port and/or a source port without a protocol, generates rules without src or dest port restriction. This was a real security issue for cloud users. This patch generates a 400 Bad request "Source/destination port requires a protocol" in case of creation/update of firewall rules specifying a destination port and/or a source port and without protocol. DocImpact Closes-Bug: #1365961 Change-Id: I4a3a1d9ae7ec4b2a864b3edc83d65ef7f80cbba5 --- neutron/db/firewall/firewall_db.py | 11 ++++ neutron/extensions/firewall.py | 4 ++ .../unit/db/firewall/test_db_firewall.py | 54 +++++++++++++++++++ 3 files changed, 69 insertions(+) diff --git a/neutron/db/firewall/firewall_db.py b/neutron/db/firewall/firewall_db.py index 275e1d4f92d..40ccf5d3d6f 100644 --- a/neutron/db/firewall/firewall_db.py +++ b/neutron/db/firewall/firewall_db.py @@ -355,6 +355,9 @@ class Firewall_db_mixin(firewall.FirewallPluginBase, base_db.CommonDbMixin): fwr = firewall_rule['firewall_rule'] self._validate_fwr_protocol_parameters(fwr) tenant_id = self._get_tenant_id_for_create(context, fwr) + if not fwr['protocol'] and (fwr['source_port'] or + fwr['destination_port']): + raise firewall.FirewallRuleWithPortWithoutProtocolInvalid() src_port_min, src_port_max = self._get_min_max_ports_from_range( fwr['source_port']) dst_port_min, dst_port_max = self._get_min_max_ports_from_range( @@ -396,6 +399,14 @@ class Firewall_db_mixin(firewall.FirewallPluginBase, base_db.CommonDbMixin): del fwr['destination_port'] with context.session.begin(subtransactions=True): fwr_db = self._get_firewall_rule(context, id) + protocol = fwr.get('protocol', fwr_db['protocol']) + if not protocol: + sport = fwr.get('source_port_range_min', + fwr_db['source_port_range_min']) + dport = fwr.get('destination_port_range_min', + fwr_db['destination_port_range_min']) + if sport or dport: + raise firewall.FirewallRuleWithPortWithoutProtocolInvalid() fwr_db.update(fwr) if fwr_db.firewall_policy_id: fwp_db = self._get_firewall_policy(context, diff --git a/neutron/extensions/firewall.py b/neutron/extensions/firewall.py index ff0fd39fb1f..fb66aaa4a4f 100644 --- a/neutron/extensions/firewall.py +++ b/neutron/extensions/firewall.py @@ -83,6 +83,10 @@ class FirewallRuleInvalidICMPParameter(qexception.InvalidInput): "is set to ICMP.") +class FirewallRuleWithPortWithoutProtocolInvalid(qexception.InvalidInput): + message = _("Source/destination port requires a protocol") + + class FirewallInvalidPortValue(qexception.InvalidInput): message = _("Invalid value for port %(port)s.") diff --git a/neutron/tests/unit/db/firewall/test_db_firewall.py b/neutron/tests/unit/db/firewall/test_db_firewall.py index 5e27f5fc46c..f070caf1a2d 100644 --- a/neutron/tests/unit/db/firewall/test_db_firewall.py +++ b/neutron/tests/unit/db/firewall/test_db_firewall.py @@ -614,6 +614,20 @@ class TestFirewallDBPlugin(FirewallPluginDbTestCase): for k, v in attrs.iteritems(): self.assertEqual(firewall_rule['firewall_rule'][k], v) + def test_create_firewall_rule_without_protocol_with_dport(self): + attrs = self._get_test_firewall_rule_attrs() + attrs['protocol'] = None + attrs['source_port'] = None + res = self._create_firewall_rule(self.fmt, **attrs) + self.assertEqual(400, res.status_int) + + def test_create_firewall_rule_without_protocol_with_sport(self): + attrs = self._get_test_firewall_rule_attrs() + attrs['protocol'] = None + attrs['destination_port'] = None + res = self._create_firewall_rule(self.fmt, **attrs) + self.assertEqual(400, res.status_int) + def test_show_firewall_rule_with_fw_policy_not_associated(self): attrs = self._get_test_firewall_rule_attrs() with self.firewall_rule() as fw_rule: @@ -709,6 +723,46 @@ class TestFirewallDBPlugin(FirewallPluginDbTestCase): for k, v in attrs.iteritems(): self.assertEqual(res['firewall_rule'][k], v) + def test_update_firewall_rule_with_port_and_no_proto(self): + with self.firewall_rule() as fwr: + data = {'firewall_rule': {'protocol': None, + 'destination_port': 80}} + req = self.new_update_request('firewall_rules', data, + fwr['firewall_rule']['id']) + res = req.get_response(self.ext_api) + self.assertEqual(400, res.status_int) + + def test_update_firewall_rule_without_ports_and_no_proto(self): + with self.firewall_rule() as fwr: + data = {'firewall_rule': {'protocol': None, + 'destination_port': None, + 'source_port': None}} + req = self.new_update_request('firewall_rules', data, + fwr['firewall_rule']['id']) + res = req.get_response(self.ext_api) + self.assertEqual(200, res.status_int) + + def test_update_firewall_rule_with_port(self): + with self.firewall_rule(source_port=None, + destination_port=None, + protocol=None) as fwr: + data = {'firewall_rule': {'destination_port': 80}} + req = self.new_update_request('firewall_rules', data, + fwr['firewall_rule']['id']) + res = req.get_response(self.ext_api) + self.assertEqual(400, res.status_int) + + def test_update_firewall_rule_with_port_and_protocol(self): + with self.firewall_rule(source_port=None, + destination_port=None, + protocol=None) as fwr: + data = {'firewall_rule': {'destination_port': 80, + 'protocol': 'tcp'}} + req = self.new_update_request('firewall_rules', data, + fwr['firewall_rule']['id']) + res = req.get_response(self.ext_api) + self.assertEqual(200, res.status_int) + def test_update_firewall_rule_with_policy_associated(self): name = "new_firewall_rule1" attrs = self._get_test_firewall_rule_attrs(name)