[lxc-devel] [lxd/master] Device: Fixes ebtables filter leak with Bridged NIC on umanaged bridge

[tomponline on Github]

A non-text attachment was scrubbed...
Name: not available
Type: text/x-mailbox
Size: 661 bytes
Desc: not available
URL: <http://lists.linuxcontainers.org/pipermail/lxc-devel/attachments/20200207/6f4fe6a4/attachment-0001.bin>
-------------- next part --------------
From 586544246055075ac066ee348712db0222af2b5a Mon Sep 17 00:00:00 2001
From: Thomas Parrott <thomas.parrott at canonical.com>
Date: Fri, 7 Feb 2020 09:05:15 +0000
Subject: [PATCH 1/2] test/suites/container/devices/nic/bridged: Adds test to
 detect leaked filters

When deleting/stopping an instance using security filtering on an unmanaged network, the filters should be removed.

Signed-off-by: Thomas Parrott <thomas.parrott at canonical.com>
---
 test/suites/container_devices_nic_bridged_filtering.sh | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/test/suites/container_devices_nic_bridged_filtering.sh b/test/suites/container_devices_nic_bridged_filtering.sh
index fe19c1259d..7b2728f9b5 100644
--- a/test/suites/container_devices_nic_bridged_filtering.sh
+++ b/test/suites/container_devices_nic_bridged_filtering.sh
@@ -376,7 +376,12 @@ test_container_devices_nic_bridged_filtering() {
     echo "Shouldn't be able to unset IPv6 address with ipv4_filtering enabled and DHCPv6 disabled"
   fi
 
+  # Delete container and check filters are cleaned up.
   lxc delete -f "${ctPrefix}A"
+  if ebtables --concurrent -L --Lmac2 --Lx | grep -e "${ctAHost}" ; then
+      echo "ebtables filter still applied after delete"
+      false
+  fi
 
   # Test MAC filtering on unmanaged bridge.
   ip link add "${brName}2" type bridge

From 20df689d34cf948f1e97942fa78a907c21d2b683 Mon Sep 17 00:00:00 2001
From: Thomas Parrott <thomas.parrott at canonical.com>
Date: Fri, 7 Feb 2020 09:25:01 +0000
Subject: [PATCH 2/2] lxd/device/nic/bridged: Fixes bug that leaks ebtables
 filters

When using static IPs with an unmanaged bridge parent.

Because there is no DHCP allocation in dnsmasq, previously the attempt to remove the rules based on the device's static IPs was not attempted.

Signed-off-by: Thomas Parrott <thomas.parrott at canonical.com>
---
 lxd/device/nic_bridged.go | 54 +++++++++++++++++++++++++++------------
 1 file changed, 38 insertions(+), 16 deletions(-)

diff --git a/lxd/device/nic_bridged.go b/lxd/device/nic_bridged.go
index ad089ae747..2c60fb7b8f 100644
--- a/lxd/device/nic_bridged.go
+++ b/lxd/device/nic_bridged.go
@@ -360,10 +360,7 @@ func (d *nicBridged) postStop() error {
 	}
 
 	networkRemoveVethRoutes(d.config)
-	err := d.removeFilters(d.config)
-	if err != nil {
-		logger.Errorf("Failed to remove nic filters: %v", err)
-	}
+	d.removeFilters(d.config)
 
 	return nil
 }
@@ -467,31 +464,56 @@ func (d *nicBridged) setupHostFilters(oldConfig deviceConfig.Device) error {
 }
 
 // removeFilters removes any network level filters defined for the instance.
-func (d *nicBridged) removeFilters(m deviceConfig.Device) error {
+func (d *nicBridged) removeFilters(m deviceConfig.Device) {
 	if m["hwaddr"] == "" {
-		return fmt.Errorf("Failed to remove network filters for %s: hwaddr not defined", m["name"])
+		logger.Errorf("Failed to remove network filters for %s: hwaddr not defined", m["name"])
+		return
 	}
 
 	if m["host_name"] == "" {
-		return fmt.Errorf("Failed to remove network filters for %s: host_name not defined", m["name"])
+		logger.Errorf("Failed to remove network filters for %s: host_name not defined", m["name"])
+		return
 	}
 
 	// Remove any IPv6 filters used for this instance.
 	err := d.state.Firewall.InstanceClear(firewallConsts.FamilyIPv6, firewallConsts.TableFilter, fmt.Sprintf("%s - ipv6_filtering", d.inst.Name()))
 	if err != nil {
-		return fmt.Errorf("Failed to clear ip6tables ipv6_filter rules for %s: %v", m["name"], err)
+		logger.Errorf("Failed to clear ip6tables ipv6_filter rules for %s: %v", m["name"], err)
 	}
 
-	// Read current static IP allocation configured from dnsmasq host config (if exists).
-	var IPv4, IPv6 dhcpAllocation
-	if shared.PathExists(shared.VarPath("networks", m["parent"], "dnsmasq.hosts") + "/" + d.inst.Name()) {
-		IPv4, IPv6, err = d.getDHCPStaticIPs(m["parent"], d.inst.Name())
-		if err != nil {
-			return fmt.Errorf("Failed to retrieve static IPs for filter removal from %s: %v", m["name"], err)
+	var IPv4, IPv6 net.IP
+
+	if m["ipv4.address"] != "" {
+		IPv4 = net.ParseIP(m["ipv4.address"])
+	}
+
+	if m["ipv6.address"] != "" {
+		IPv6 = net.ParseIP(m["ipv6.address"])
+	}
+
+	// Remove filters for static MAC and IPs (if specified above).
+	// This covers the case when filtering is used with an unmanaged bridge.
+	err = d.state.Firewall.InstanceNicBridgedRemoveFilters(m, IPv4, IPv6)
+	if err != nil {
+		logger.Errorf("Failed to remove nic static filters: %v", err)
+	}
+
+	// Read current static DHCP IP allocation configured from dnsmasq host config (if exists).
+	// This covers the case when IPs are not defined in config, but have been assigned in managed DHCP.
+	IPv4Alloc, IPv6Alloc, err := d.getDHCPStaticIPs(m["parent"], d.inst.Name())
+	if err != nil {
+		if os.IsNotExist(err) {
+			return
 		}
+
+		logger.Errorf("Failed to retrieve static IPs for filter removal from %s: %v", m["name"], err)
+		return
 	}
 
-	return d.state.Firewall.InstanceNicBridgedRemoveFilters(m, IPv4.IP, IPv6.IP)
+	err = d.state.Firewall.InstanceNicBridgedRemoveFilters(m, IPv4Alloc.IP, IPv6Alloc.IP)
+	if err != nil {
+		logger.Errorf("Failed to remove nic DHCP assigned filters: %v", err)
+	}
 }
 
 // getDHCPStaticIPs retrieves the dnsmasq statically allocated IPs for a instance.
@@ -499,7 +521,7 @@ func (d *nicBridged) removeFilters(m deviceConfig.Device) error {
 func (d *nicBridged) getDHCPStaticIPs(network string, instanceName string) (dhcpAllocation, dhcpAllocation, error) {
 	var IPv4, IPv6 dhcpAllocation
 
-	file, err := os.Open(shared.VarPath("networks", network, "dnsmasq.hosts") + "/" + instanceName)
+	file, err := os.Open(shared.VarPath("networks", network, "dnsmasq.hosts", instanceName))
 	if err != nil {
 		return IPv4, IPv6, err
 	}