[libvirt] [PATCH v2 3/4] util: create private chains for virtual network firewall rules

Historically firewall rules for virtual networks were added straight into the base chains. This works but has a number of bugs and design limitations: - It is inflexible for admins wanting to add extra rules ahead of libvirt's rules, via hook scripts. - It is not clear to the admin that the rules were created by libvirt - Each rule must be deleted by libvirt individually since they are all directly in the builtin chains - The ordering of rules in the forward chain is incorrect when multiple networks are created, allowing traffic to mistakenly flow between networks in one direction. To address all of these problems, libvirt needs to move to creating rules in its own private chains. In the top level builtin chains, libvirt will add links to its own private top level chains. Addressing the traffic ordering bug requires some extra steps. With everything going into the FORWARD chain there was interleaving of rules for outbound traffic and inbound traffic for each network: -A FORWARD -d 192.168.3.0/24 -o virbr1 -m conntrack --ctstate RELATED,ESTABLISHED -j ACCEPT -A FORWARD -s 192.168.3.0/24 -i virbr1 -j ACCEPT -A FORWARD -i virbr1 -o virbr1 -j ACCEPT -A FORWARD -o virbr1 -j REJECT --reject-with icmp-port-unreachable -A FORWARD -i virbr1 -j REJECT --reject-with icmp-port-unreachable -A FORWARD -d 192.168.2.0/24 -o virbr0 -m conntrack --ctstate RELATED,ESTABLISHED -j ACCEPT -A FORWARD -s 192.168.2.0/24 -i virbr0 -j ACCEPT -A FORWARD -i virbr0 -o virbr0 -j ACCEPT -A FORWARD -o virbr0 -j REJECT --reject-with icmp-port-unreachable -A FORWARD -i virbr0 -j REJECT --reject-with icmp-port-unreachable The rule allowing outbound traffic from virbr1 would mistakenly allow packets from virbr1 to virbr0, before the rule denying input to virbr0 gets a chance to run. What we really need todo is group the forwarding rules into three distinct sets: * Cross rules - LIBVIRT_FWX -A FORWARD -i virbr1 -o virbr1 -j ACCEPT -A FORWARD -i virbr0 -o virbr0 -j ACCEPT * Incoming rules - LIBVIRT_FWI -A FORWARD -d 192.168.3.0/24 -o virbr1 -m conntrack --ctstate RELATED,ESTABLISHED -j ACCEPT -A FORWARD -o virbr1 -j REJECT --reject-with icmp-port-unreachable -A FORWARD -d 192.168.2.0/24 -o virbr0 -m conntrack --ctstate RELATED,ESTABLISHED -j ACCEPT -A FORWARD -o virbr0 -j REJECT --reject-with icmp-port-unreachable * Outgoing rules - LIBVIRT_FWO -A FORWARD -s 192.168.3.0/24 -i virbr1 -j ACCEPT -A FORWARD -i virbr1 -j REJECT --reject-with icmp-port-unreachable -A FORWARD -s 192.168.2.0/24 -i virbr0 -j ACCEPT -A FORWARD -i virbr0 -j REJECT --reject-with icmp-port-unreachable There is thus no risk of outgoing rules for one network mistakenly allowing incoming traffic for another network, as all incoming rules are evalated first. With this in mind, we'll thus need three distinct chains linked from the FORWARD chain, so we end up with: INPUT --> LIBVIRT_INP (filter) OUTPUT --> LIBVIRT_OUT (filter) FORWARD +-> LIBVIRT_FWX (filter) +-> LIBVIRT_FWO \-> LIBVIRT_FWI POSTROUTING --> LIBVIRT_PRT (nat & mangle) Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com&gt; --- src/libvirt_private.syms | 1 + src/network/bridge_driver_linux.c | 3 + src/util/viriptables.c | 131 ++++++++++++++++++++++++++++++ src/util/viriptables.h | 2 + 4 files changed, 137 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index fd63c9ca61..31aa14b74d 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2071,6 +2071,7 @@ iptablesRemoveOutputFixUdpChecksum; iptablesRemoveTcpInput; iptablesRemoveUdpInput; iptablesRemoveUdpOutput; +iptablesSetupPrivateChains; # util/viriscsi.h diff --git a/src/network/bridge_driver_linux.c b/src/network/bridge_driver_linux.c index 5650e1e061..435de5e17d 100644 --- a/src/network/bridge_driver_linux.c +++ b/src/network/bridge_driver_linux.c @@ -38,6 +38,9 @@ VIR_LOG_INIT("network.bridge_driver_linux"); int networkPreReloadFirewallRules(bool startup ATTRIBUTE_UNUSED) { + int ret = iptablesSetupPrivateChains(); + if (ret < 0) + return -1; return 0; } diff --git a/src/util/viriptables.c b/src/util/viriptables.c index f379844d28..fbcf394fac 100644 --- a/src/util/viriptables.c +++ b/src/util/viriptables.c @@ -40,6 +40,7 @@ #include "virthread.h" #include "virstring.h" #include "virutil.h" +#include "virhash.h" VIR_LOG_INIT("util.iptables"); @@ -51,6 +52,136 @@ enum { }; +typedef struct { + const char *parent; + const char *child; +} iptablesGlobalChain; + +typedef struct { + virFirewallLayer layer; + const char *table; + iptablesGlobalChain *chains; + size_t nchains; + bool *changed; +} iptablesGlobalChainData; + + +static int +iptablesPrivateChainCreate(virFirewallPtr fw, + virFirewallLayer layer, + const char *const *lines, + void *opaque) +{ + iptablesGlobalChainData *data = opaque; + virHashTablePtr chains = NULL; + virHashTablePtr links = NULL; + const char *const *tmp; + int ret = -1; + size_t i; + + if (!(chains = virHashCreate(50, NULL))) + goto cleanup; + if (!(links = virHashCreate(50, NULL))) + goto cleanup; + + tmp = lines; + while (tmp && *tmp) { + if (STRPREFIX(*tmp, "-N ")) { /* eg "-N LIBVIRT_INP" */ + if (virHashUpdateEntry(chains, *tmp + 3, (void *)0x1) < 0) + goto cleanup; + } else if (STRPREFIX(*tmp, "-A ")) { /* eg "-A INPUT -j LIBVIRT_INP" */ + char *sep = strchr(*tmp + 3, ' '); + if (sep) { + *sep = '\0'; + if (STRPREFIX(sep + 1, "-j ")) { + if (virHashUpdateEntry(links, sep + 4, + (char *)*tmp + 3) < 0) + goto cleanup; + } + } + } + tmp++; + } + + for (i = 0; i < data->nchains; i++) { + const char *from; + if (!virHashLookup(chains, data->chains[i].child)) { + virFirewallAddRule(fw, layer, + "--table", data->table, + "--new-chain", data->chains[i].child, NULL); + *data->changed = true; + } + + from = virHashLookup(links, data->chains[i].child); + if (!from || STRNEQ(from, data->chains[i].parent)) + virFirewallAddRule(fw, layer, + "--table", data->table, + "--insert", data->chains[i].parent, + "--jump", data->chains[i].child, NULL); + } + + ret = 0; + cleanup: + virHashFree(chains); + virHashFree(links); + return ret; +} + + +int +iptablesSetupPrivateChains(void) +{ + virFirewallPtr fw = NULL; + int ret = -1; + iptablesGlobalChain filter_chains[] = { + {"INPUT", "LIBVIRT_INP"}, + {"OUTPUT", "LIBVIRT_OUT"}, + {"FORWARD", "LIBVIRT_FWO"}, + {"FORWARD", "LIBVIRT_FWI"}, + {"FORWARD", "LIBVIRT_FWX"}, + }; + iptablesGlobalChain natmangle_chains[] = { + {"POSTROUTING", "LIBVIRT_PRT"}, + }; + bool changed = false; + iptablesGlobalChainData data[] = { + { VIR_FIREWALL_LAYER_IPV4, "filter", + filter_chains, ARRAY_CARDINALITY(filter_chains), &changed }, + { VIR_FIREWALL_LAYER_IPV4, "nat", + natmangle_chains, ARRAY_CARDINALITY(natmangle_chains), &changed }, + { VIR_FIREWALL_LAYER_IPV4, "mangle", + natmangle_chains, ARRAY_CARDINALITY(natmangle_chains), &changed }, + { VIR_FIREWALL_LAYER_IPV6, "filter", + filter_chains, ARRAY_CARDINALITY(filter_chains), &changed }, + { VIR_FIREWALL_LAYER_IPV6, "nat", + natmangle_chains, ARRAY_CARDINALITY(natmangle_chains), &changed }, + { VIR_FIREWALL_LAYER_IPV6, "mangle", + natmangle_chains, ARRAY_CARDINALITY(natmangle_chains), &changed }, + }; + size_t i; + + fw = virFirewallNew(); + + virFirewallStartTransaction(fw, 0); + + for (i = 0; i < ARRAY_CARDINALITY(data); i++) + virFirewallAddRuleFull(fw, data[i].layer, + false, iptablesPrivateChainCreate, + &(data[i]), "--table", data[i].table, + "--list-rules", NULL); + + if (virFirewallApply(fw) < 0) + goto cleanup; + + ret = changed ? 1 : 0; + + cleanup: + + virFirewallFree(fw); + return ret; +} + + static void iptablesInput(virFirewallPtr fw, virFirewallLayer layer, diff --git a/src/util/viriptables.h b/src/util/viriptables.h index 9ea25fc096..1db97937a1 100644 --- a/src/util/viriptables.h +++ b/src/util/viriptables.h @@ -27,6 +27,8 @@ # include "virsocketaddr.h" # include "virfirewall.h" +int iptablesSetupPrivateChains (void); + void iptablesAddTcpInput (virFirewallPtr fw, virFirewallLayer layer, const char *iface, -- 2.19.2