From 9ad9b8483913a38345133120b04c4677fab9cb68 Mon Sep 17 00:00:00 2001 From: Rodolfo Alonso Hernandez Date: Wed, 24 Apr 2019 07:09:21 +0000 Subject: [PATCH] Prevent "qbr" Linux Bridge from replying to ARP messages The Linux Bridge in between the VM TAP interface and OVS should [1][2]: - Reply only if the target IP address is local address configured on the incoming interface. - Always use the best local address. [1]http://kb.linuxvirtualserver.org/wiki/Using_arp_announce/arp_ignore_to_disable_ARP [2]http://linux-ip.net/html/ether-arp.html#ether-arp-flux Change-Id: I8721b680bbd9f59a67bd8e6855ffb291c208cdb8 Closes-Bug: #1825888 --- ...prevent-lb-reply-arp-6459133bfb056069.yaml | 8 +++++++ vif_plug_linux_bridge/linux_net.py | 17 +++++++++++++ .../tests/unit/test_linux_net.py | 8 +++++-- vif_plug_ovs/linux_net.py | 17 +++++++++++++ vif_plug_ovs/tests/unit/test_linux_net.py | 24 +++++++++++++++++-- 5 files changed, 70 insertions(+), 4 deletions(-) create mode 100644 releasenotes/notes/prevent-lb-reply-arp-6459133bfb056069.yaml diff --git a/releasenotes/notes/prevent-lb-reply-arp-6459133bfb056069.yaml b/releasenotes/notes/prevent-lb-reply-arp-6459133bfb056069.yaml new file mode 100644 index 00000000..95148b62 --- /dev/null +++ b/releasenotes/notes/prevent-lb-reply-arp-6459133bfb056069.yaml @@ -0,0 +1,8 @@ +--- +security: + - | + Prevent Linux Bridge from replying to ARP messages. It should reply only if + the target IP address is a local address configured on the incoming + interface and it should always use the best local address. See `The ARP + flux problem `_ for + more information. diff --git a/vif_plug_linux_bridge/linux_net.py b/vif_plug_linux_bridge/linux_net.py index a338a97d..539250bf 100644 --- a/vif_plug_linux_bridge/linux_net.py +++ b/vif_plug_linux_bridge/linux_net.py @@ -112,6 +112,22 @@ def _disable_ipv6(bridge): f.write('1') +# TODO(ralonsoh): extract into common module +def _arp_filtering(bridge): + """Prevent the bridge from replying to ARP messages with machine local IPs + + 1. Reply only if the target IP address is local address configured on the + incoming interface. + 2. Always use the best local address. + """ + arp_params = [('/proc/sys/net/ipv4/conf/%s/arp_ignore' % bridge, '1'), + ('/proc/sys/net/ipv4/conf/%s/arp_announce' % bridge, '2')] + for parameter, value in arp_params: + if os.path.exists(parameter): + with open(parameter, 'w') as f: + f.write(value) + + def _update_bridge_routes(interface, bridge): """Updates routing table for a given bridge and interface. :param interface: string interface name @@ -176,6 +192,7 @@ def _ensure_bridge_privileged(bridge, interface, net_attrs, gateway, LOG.debug('Starting Bridge %s', bridge) ip_lib.add(bridge, 'bridge') _disable_ipv6(bridge) + _arp_filtering(bridge) ip_lib.set(bridge, state='up') if interface and ip_lib.exists(interface): diff --git a/vif_plug_linux_bridge/tests/unit/test_linux_net.py b/vif_plug_linux_bridge/tests/unit/test_linux_net.py index 3fc8b90e..6daed382 100644 --- a/vif_plug_linux_bridge/tests/unit/test_linux_net.py +++ b/vif_plug_linux_bridge/tests/unit/test_linux_net.py @@ -92,12 +92,14 @@ class LinuxNetTest(testtools.TestCase): @mock.patch.object(ip_lib, "add") @mock.patch.object(ip_lib, "set") + @mock.patch.object(linux_net, "_arp_filtering") @mock.patch.object(linux_net, "_set_device_mtu") @mock.patch.object(linux_net, "_disable_ipv6") @mock.patch.object(linux_net, "_update_bridge_routes") @mock.patch.object(ip_lib, "exists") def test_ensure_bridge_priv_mtu_not_called(self, mock_dev_exists, - mock_routes, mock_disable_ipv6, mock_set_mtu, mock_ip_set, mock_add): + mock_routes, mock_disable_ipv6, mock_set_mtu, mock_arp_filtering, + mock_ip_set, mock_add): """This test validates that mtus are updated only if an interface is added to the bridge """ @@ -109,12 +111,14 @@ class LinuxNetTest(testtools.TestCase): @mock.patch.object(ip_lib, "add") @mock.patch.object(ip_lib, "set") + @mock.patch.object(linux_net, "_arp_filtering") @mock.patch.object(linux_net, "_set_device_mtu") @mock.patch.object(linux_net, "_disable_ipv6") @mock.patch.object(linux_net, "_update_bridge_routes") @mock.patch.object(ip_lib, "exists") def test_ensure_bridge_priv_mtu_order(self, mock_dev_exists, mock_routes, - mock_disable_ipv6, mock_set_mtu, mock_ip_set, mock_add): + mock_disable_ipv6, mock_set_mtu, mock_arp_filtering, mock_ip_set, + mock_add): """This test validates that when adding an interface to a bridge, the interface mtu is updated first followed by the bridge. This is required to work around diff --git a/vif_plug_ovs/linux_net.py b/vif_plug_ovs/linux_net.py index b36bcd22..e3e26729 100644 --- a/vif_plug_ovs/linux_net.py +++ b/vif_plug_ovs/linux_net.py @@ -109,11 +109,28 @@ def _disable_ipv6(bridge): f.write('1') +# TODO(ralonsoh): extract into common module +def _arp_filtering(bridge): + """Prevent the bridge from replying to ARP messages with machine local IPs + + 1. Reply only if the target IP address is local address configured on the + incoming interface. + 2. Always use the best local address. + """ + arp_params = [('/proc/sys/net/ipv4/conf/%s/arp_ignore' % bridge, '1'), + ('/proc/sys/net/ipv4/conf/%s/arp_announce' % bridge, '2')] + for parameter, value in arp_params: + if os.path.exists(parameter): + with open(parameter, 'w') as f: + f.write(value) + + @privsep.vif_plug.entrypoint def ensure_bridge(bridge): if not ip_lib.exists(bridge): ip_lib.add(bridge, 'bridge') _disable_ipv6(bridge) + _arp_filtering(bridge) # we bring up the bridge to allow it to switch packets set_interface_state(bridge, 'up') diff --git a/vif_plug_ovs/tests/unit/test_linux_net.py b/vif_plug_ovs/tests/unit/test_linux_net.py index 2f413f18..e2700c15 100644 --- a/vif_plug_ovs/tests/unit/test_linux_net.py +++ b/vif_plug_ovs/tests/unit/test_linux_net.py @@ -16,6 +16,7 @@ import os.path import testtools from os_vif.internal.ip.api import ip as ip_lib +from six.moves import builtins from vif_plug_ovs import exception from vif_plug_ovs import linux_net @@ -29,31 +30,37 @@ class LinuxNetTest(testtools.TestCase): privsep.vif_plug.set_client_mode(False) + @mock.patch.object(linux_net, "_arp_filtering") @mock.patch.object(linux_net, "set_interface_state") @mock.patch.object(linux_net, "_disable_ipv6") @mock.patch.object(ip_lib, "add") @mock.patch.object(ip_lib, "exists", return_value=False) def test_ensure_bridge(self, mock_dev_exists, mock_add, - mock_disable_ipv6, mock_set_state): + mock_disable_ipv6, mock_set_state, + mock_arp_filtering): linux_net.ensure_bridge("br0") mock_dev_exists.assert_called_once_with("br0") mock_add.assert_called_once_with("br0", "bridge") mock_disable_ipv6.assert_called_once_with("br0") mock_set_state.assert_called_once_with("br0", "up") + mock_arp_filtering.assert_called_once_with("br0") + @mock.patch.object(linux_net, "_arp_filtering") @mock.patch.object(linux_net, "set_interface_state") @mock.patch.object(linux_net, "_disable_ipv6") @mock.patch.object(ip_lib, "add") @mock.patch.object(ip_lib, "exists", return_value=True) def test_ensure_bridge_exists(self, mock_dev_exists, mock_add, - mock_disable_ipv6, mock_set_state): + mock_disable_ipv6, mock_set_state, + mock_arp_filtering): linux_net.ensure_bridge("br0") mock_dev_exists.assert_called_once_with("br0") mock_add.assert_not_called() mock_disable_ipv6.assert_called_once_with("br0") mock_set_state.assert_called_once_with("br0", "up") + mock_arp_filtering.assert_called_once_with("br0") @mock.patch('six.moves.builtins.open') @mock.patch("os.path.exists") @@ -72,6 +79,19 @@ class LinuxNetTest(testtools.TestCase): mock_exists.assert_called_once_with(exists_path) mock_open.assert_called_once_with(exists_path, 'w') + @mock.patch.object(os.path, 'exists', return_value=True) + @mock.patch.object(builtins, 'open') + def test__arp_filtering(self, mock_open, *args): + mock_open.side_effect = mock.mock_open(read_data=mock.Mock()) + linux_net._arp_filtering("br0") + + mock_open.assert_has_calls([ + mock.call('/proc/sys/net/ipv4/conf/br0/arp_ignore', 'w'), + mock.call('/proc/sys/net/ipv4/conf/br0/arp_announce', 'w')]) + mock_open.side_effect.return_value.write.assert_has_calls([ + mock.call('1'), + mock.call('2')]) + @mock.patch.object(ip_lib, "delete") @mock.patch.object(ip_lib, "exists", return_value=False) def test_delete_bridge_none(self, mock_dev_exists, mock_delete):