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:
Brian Haley 2017-11-15 19:24:22 -05:00
parent 6277b4a202
commit 08108c4199
2 changed files with 208 additions and 20 deletions

View File

@ -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)

View File

@ -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)