Move Linuxbridge ARP spoofing to nat table PREROUTING chain
It was found that adding ebtables rules to the filter table FORWARD chain could be vulnerable to a DoS attack. Moving to the nat table PREROUTING chain should mitigate this as it is consulted prior to allowing the frame in. In order to make this work with upgrades, had to make the code detect and remove any old rules that might still exist in the filter table. That can be removed after a cycle. Added some unit tests in addition to the existing functional tests. Change-Id: I87852b21db4404c58c83789cc267812030ac7d5f Closes-bug: #1732294
This commit is contained in:
parent
6277b4a202
commit
08108c4199
|
@ -70,26 +70,33 @@ def chain_name(vif):
|
|||
@lockutils.synchronized('ebtables')
|
||||
def delete_arp_spoofing_protection(vifs):
|
||||
current_rules = ebtables(['-L']).splitlines()
|
||||
_delete_arp_spoofing_protection(vifs, current_rules)
|
||||
_delete_arp_spoofing_protection(vifs, current_rules, table='nat',
|
||||
chain='PREROUTING')
|
||||
|
||||
# TODO(haleyb) this can go away in "R" cycle, it's here to cleanup
|
||||
# old chains in the filter table
|
||||
current_rules = ebtables(['-L'], table='filter').splitlines()
|
||||
_delete_arp_spoofing_protection(vifs, current_rules, table='filter',
|
||||
chain='FORWARD')
|
||||
|
||||
|
||||
def _delete_arp_spoofing_protection(vifs, current_rules):
|
||||
def _delete_arp_spoofing_protection(vifs, current_rules, table, chain):
|
||||
# delete the jump rule and then delete the whole chain
|
||||
jumps = [vif for vif in vifs if vif_jump_present(vif, current_rules)]
|
||||
for vif in jumps:
|
||||
ebtables(['-D', 'FORWARD', '-i', vif, '-j',
|
||||
chain_name(vif), '-p', 'ARP'])
|
||||
ebtables(['-D', chain, '-i', vif, '-j',
|
||||
chain_name(vif), '-p', 'ARP'], table=table)
|
||||
for vif in vifs:
|
||||
if chain_exists(chain_name(vif), current_rules):
|
||||
ebtables(['-X', chain_name(vif)])
|
||||
_delete_mac_spoofing_protection(vifs, current_rules)
|
||||
ebtables(['-X', chain_name(vif)], table=table)
|
||||
_delete_mac_spoofing_protection(vifs, current_rules, table=table,
|
||||
chain=chain)
|
||||
|
||||
|
||||
@lockutils.synchronized('ebtables')
|
||||
def delete_unreferenced_arp_protection(current_vifs):
|
||||
def _delete_unreferenced_arp_protection(current_vifs, table, chain):
|
||||
# deletes all jump rules and chains that aren't in current_vifs but match
|
||||
# the spoof prefix
|
||||
current_rules = ebtables(['-L']).splitlines()
|
||||
current_rules = ebtables(['-L'], table=table).splitlines()
|
||||
to_delete = []
|
||||
for line in current_rules:
|
||||
# we're looking to find and turn the following:
|
||||
|
@ -101,7 +108,19 @@ def delete_unreferenced_arp_protection(current_vifs):
|
|||
to_delete.append(devname)
|
||||
LOG.info("Clearing orphaned ARP spoofing entries for devices %s",
|
||||
to_delete)
|
||||
_delete_arp_spoofing_protection(to_delete, current_rules)
|
||||
_delete_arp_spoofing_protection(to_delete, current_rules, table=table,
|
||||
chain=chain)
|
||||
|
||||
|
||||
@lockutils.synchronized('ebtables')
|
||||
def delete_unreferenced_arp_protection(current_vifs):
|
||||
_delete_unreferenced_arp_protection(current_vifs,
|
||||
table='nat', chain='PREROUTING')
|
||||
|
||||
# TODO(haleyb) this can go away in "R" cycle, it's here to cleanup
|
||||
# old chains in the filter table
|
||||
_delete_unreferenced_arp_protection(current_vifs,
|
||||
table='filter', chain='FORWARD')
|
||||
|
||||
|
||||
@lockutils.synchronized('ebtables')
|
||||
|
@ -119,12 +138,12 @@ def _install_arp_spoofing_protection(vif, addresses, current_rules):
|
|||
# packets until the allows are installed, but that's better than leaked
|
||||
# spoofed packets and ARP can handle losses.
|
||||
ebtables(['-F', vif_chain])
|
||||
for addr in addresses:
|
||||
for addr in sorted(addresses):
|
||||
ebtables(['-A', vif_chain, '-p', 'ARP', '--arp-ip-src', addr,
|
||||
'-j', 'ACCEPT'])
|
||||
# check if jump rule already exists, if not, install it
|
||||
if not vif_jump_present(vif, current_rules):
|
||||
ebtables(['-A', 'FORWARD', '-i', vif, '-j',
|
||||
ebtables(['-A', 'PREROUTING', '-i', vif, '-j',
|
||||
vif_chain, '-p', 'ARP'])
|
||||
|
||||
|
||||
|
@ -155,13 +174,13 @@ def _install_mac_spoofing_protection(vif, port_details, 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])
|
||||
ebtables(['-A', 'PREROUTING', '-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']
|
||||
'--among-src', ','.join(sorted(chunk)), '-j', 'RETURN']
|
||||
ebtables(new_rule)
|
||||
_delete_vif_mac_rules(vif, current_rules)
|
||||
|
||||
|
@ -185,17 +204,17 @@ def _delete_vif_mac_rules(vif, current_rules):
|
|||
ebtables(['-D', chain] + rule.split())
|
||||
|
||||
|
||||
def _delete_mac_spoofing_protection(vifs, current_rules):
|
||||
def _delete_mac_spoofing_protection(vifs, current_rules, table, chain):
|
||||
# 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)])
|
||||
ebtables(['-D', chain, '-i', vif, '-j',
|
||||
_mac_chain_name(vif)], table=table)
|
||||
for vif in vifs:
|
||||
chain = _mac_chain_name(vif)
|
||||
if chain_exists(chain, current_rules):
|
||||
ebtables(['-X', chain])
|
||||
ebtables(['-X', chain], table=table)
|
||||
|
||||
|
||||
# Used to scope ebtables commands in testing
|
||||
|
@ -207,6 +226,7 @@ NAMESPACE = None
|
|||
retry=tenacity.retry_if_exception(lambda e: e.returncode == 255),
|
||||
reraise=True
|
||||
)
|
||||
def ebtables(comm):
|
||||
def ebtables(comm, table='nat'):
|
||||
execute = ip_lib.IPWrapper(NAMESPACE).netns.execute
|
||||
return execute(['ebtables', '--concurrent'] + comm, run_as_root=True)
|
||||
return execute(['ebtables', '-t', table, '--concurrent'] + comm,
|
||||
run_as_root=True)
|
||||
|
|
|
@ -0,0 +1,168 @@
|
|||
# Copyright (c) 2018 Red Hat, Inc.
|
||||
# All Rights Reserved.
|
||||
#
|
||||
# Licensed under the Apache License, Version 2.0 (the "License");
|
||||
# you may not use this file except in compliance with the License.
|
||||
# You may obtain a copy of the License at
|
||||
#
|
||||
# http://www.apache.org/licenses/LICENSE-2.0
|
||||
#
|
||||
# Unless required by applicable law or agreed to in writing, software
|
||||
# distributed under the License is distributed on an "AS IS" BASIS,
|
||||
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
|
||||
# See the License for the specific language governing permissions and
|
||||
# limitations under the License.
|
||||
|
||||
import mock
|
||||
from neutron_lib import constants
|
||||
|
||||
from neutron.agent.common import utils
|
||||
from neutron.plugins.ml2.drivers.linuxbridge.agent import arp_protect
|
||||
from neutron.tests import base
|
||||
|
||||
|
||||
VIF = 'vif_tap0'
|
||||
PORT_NO_SEC = {'port_security_enabled': False}
|
||||
PORT_TRUSTED = {'device_owner': constants.DEVICE_OWNER_ROUTER_GW}
|
||||
PORT = {'fixed_ips': [{'ip_address': '10.1.1.1'}],
|
||||
'device_owner': 'nobody',
|
||||
'mac_address': '00:11:22:33:44:55'}
|
||||
PORT_ADDR_PAIR = {'fixed_ips': [{'ip_address': '10.1.1.1'}],
|
||||
'device_owner': 'nobody',
|
||||
'mac_address': '00:11:22:33:44:55',
|
||||
'allowed_address_pairs': [
|
||||
{'mac_address': '00:11:22:33:44:66',
|
||||
'ip_address': '10.1.1.2'}]}
|
||||
|
||||
|
||||
class TestLinuxBridgeARPSpoofing(base.BaseTestCase):
|
||||
|
||||
def setUp(self):
|
||||
super(TestLinuxBridgeARPSpoofing, self).setUp()
|
||||
self.execute = mock.patch.object(utils, "execute").start()
|
||||
|
||||
@mock.patch.object(arp_protect, "delete_arp_spoofing_protection")
|
||||
def test_port_no_security(self, dasp):
|
||||
arp_protect.setup_arp_spoofing_protection(VIF, PORT_NO_SEC)
|
||||
dasp.assert_called_with([VIF])
|
||||
|
||||
@mock.patch.object(arp_protect, "delete_arp_spoofing_protection")
|
||||
def test_port_trusted(self, dasp):
|
||||
arp_protect.setup_arp_spoofing_protection(VIF, PORT_TRUSTED)
|
||||
dasp.assert_called_with([VIF])
|
||||
|
||||
def _test_port_add_arp_spoofing(self, vif, port):
|
||||
mac_addresses = {port['mac_address']}
|
||||
ip_addresses = {p['ip_address'] for p in port['fixed_ips']}
|
||||
if port.get('allowed_address_pairs'):
|
||||
mac_addresses |= {p['mac_address']
|
||||
for p in port['allowed_address_pairs']}
|
||||
ip_addresses |= {p['ip_address']
|
||||
for p in port['allowed_address_pairs']}
|
||||
spoof_chain = arp_protect.SPOOF_CHAIN_PREFIX + vif
|
||||
mac_chain = arp_protect.MAC_CHAIN_PREFIX + vif
|
||||
|
||||
expected = [
|
||||
mock.call(['ebtables', '-t', 'nat', '--concurrent', '-L'],
|
||||
check_exit_code=True, extra_ok_codes=None,
|
||||
log_fail_as_error=True, run_as_root=True),
|
||||
mock.ANY,
|
||||
mock.ANY,
|
||||
mock.call(['ebtables', '-t', 'nat', '--concurrent', '-N',
|
||||
'neutronMAC-%s' % vif, '-P', 'DROP'],
|
||||
check_exit_code=True, extra_ok_codes=None,
|
||||
log_fail_as_error=True, run_as_root=True),
|
||||
mock.ANY,
|
||||
mock.call(['ebtables', '-t', 'nat', '--concurrent', '-A',
|
||||
'PREROUTING', '-i', vif, '-j', mac_chain],
|
||||
check_exit_code=True, extra_ok_codes=None,
|
||||
log_fail_as_error=True, run_as_root=True),
|
||||
mock.call(['ebtables', '-t', 'nat', '--concurrent', '-A',
|
||||
mac_chain, '-i', vif,
|
||||
'--among-src', '%s' % ','.join(sorted(mac_addresses)),
|
||||
'-j', 'RETURN'],
|
||||
check_exit_code=True, extra_ok_codes=None,
|
||||
log_fail_as_error=True, run_as_root=True),
|
||||
mock.ANY,
|
||||
mock.ANY,
|
||||
mock.call(['ebtables', '-t', 'nat', '--concurrent', '-N',
|
||||
spoof_chain, '-P', 'DROP'],
|
||||
check_exit_code=True, extra_ok_codes=None,
|
||||
log_fail_as_error=True, run_as_root=True),
|
||||
mock.call(['ebtables', '-t', 'nat', '--concurrent', '-F',
|
||||
spoof_chain],
|
||||
check_exit_code=True, extra_ok_codes=None,
|
||||
log_fail_as_error=True, run_as_root=True),
|
||||
]
|
||||
for addr in sorted(ip_addresses):
|
||||
expected.extend([
|
||||
mock.call(['ebtables', '-t', 'nat', '--concurrent', '-A',
|
||||
spoof_chain, '-p', 'ARP',
|
||||
'--arp-ip-src', addr, '-j', 'ACCEPT'],
|
||||
check_exit_code=True, extra_ok_codes=None,
|
||||
log_fail_as_error=True, run_as_root=True),
|
||||
])
|
||||
expected.extend([
|
||||
mock.ANY,
|
||||
mock.call(['ebtables', '-t', 'nat', '--concurrent', '-A',
|
||||
'PREROUTING', '-i', vif, '-j',
|
||||
spoof_chain, '-p', 'ARP'],
|
||||
check_exit_code=True, extra_ok_codes=None,
|
||||
log_fail_as_error=True, run_as_root=True),
|
||||
])
|
||||
|
||||
arp_protect.setup_arp_spoofing_protection(vif, port)
|
||||
self.execute.assert_has_calls(expected)
|
||||
|
||||
def test_port_add_arp_spoofing(self):
|
||||
self._test_port_add_arp_spoofing(VIF, PORT)
|
||||
|
||||
def test_port_add_arp_spoofing_addr_pair(self):
|
||||
self._test_port_add_arp_spoofing(VIF, PORT_ADDR_PAIR)
|
||||
|
||||
@mock.patch.object(arp_protect, "chain_exists", return_value=True)
|
||||
@mock.patch.object(arp_protect, "vif_jump_present", return_value=True)
|
||||
def test_port_delete_arp_spoofing(self, ce, vjp):
|
||||
spoof_chain = arp_protect.SPOOF_CHAIN_PREFIX + VIF
|
||||
mac_chain = arp_protect.MAC_CHAIN_PREFIX + VIF
|
||||
expected = [
|
||||
mock.call(['ebtables', '-t', 'nat', '--concurrent', '-L'],
|
||||
check_exit_code=True, extra_ok_codes=None,
|
||||
log_fail_as_error=True, run_as_root=True),
|
||||
mock.ANY,
|
||||
mock.call(['ebtables', '-t', 'nat', '--concurrent', '-D',
|
||||
'PREROUTING', '-i', VIF, '-j', spoof_chain,
|
||||
'-p', 'ARP'],
|
||||
check_exit_code=True, extra_ok_codes=None,
|
||||
log_fail_as_error=True, run_as_root=True),
|
||||
mock.call(['ebtables', '-t', 'nat', '--concurrent', '-X',
|
||||
spoof_chain],
|
||||
check_exit_code=True, extra_ok_codes=None,
|
||||
log_fail_as_error=True, run_as_root=True),
|
||||
mock.ANY,
|
||||
mock.call(['ebtables', '-t', 'nat', '--concurrent', '-X',
|
||||
mac_chain],
|
||||
check_exit_code=True, extra_ok_codes=None,
|
||||
log_fail_as_error=True, run_as_root=True),
|
||||
mock.call(['ebtables', '-t', 'filter', '--concurrent', '-L'],
|
||||
check_exit_code=True, extra_ok_codes=None,
|
||||
log_fail_as_error=True, run_as_root=True),
|
||||
mock.ANY,
|
||||
mock.call(['ebtables', '-t', 'filter', '--concurrent', '-D',
|
||||
'FORWARD', '-i', VIF, '-j', spoof_chain,
|
||||
'-p', 'ARP'],
|
||||
check_exit_code=True, extra_ok_codes=None,
|
||||
log_fail_as_error=True, run_as_root=True),
|
||||
mock.call(['ebtables', '-t', 'filter', '--concurrent', '-X',
|
||||
spoof_chain],
|
||||
check_exit_code=True, extra_ok_codes=None,
|
||||
log_fail_as_error=True, run_as_root=True),
|
||||
mock.ANY,
|
||||
mock.call(['ebtables', '-t', 'filter', '--concurrent', '-X',
|
||||
mac_chain],
|
||||
check_exit_code=True, extra_ok_codes=None,
|
||||
log_fail_as_error=True, run_as_root=True),
|
||||
]
|
||||
|
||||
arp_protect.delete_arp_spoofing_protection([VIF])
|
||||
self.execute.assert_has_calls(expected)
|
Loading…
Reference in New Issue