Re: [libvirt] [PATCH v3 4/4] bridge driver: don't masquerade local subnet broadcast/multicast packets

Packets sent by guests on virbrN, *or* by dnsmasq on the same, to - 255.255.255.255/32 (netmask-independent local network broadcast address), or to - eg. 192.168.122.255/32 (ie. address- and netmask-dependent (= directed) local network broadcast address)

or to - 224.0.0.0/24 (local subnetwork multicast range) are never forwarded, hence it is not necessary to masquerade them. In fact we must not masquerade them: translating their source addresses or source ports (where applicable) may confuse receivers on virbrN. One example is the DHCP client in OVMF (= UEFI firmware for virtual machines): http://thread.gmane.org/gmane.comp.bios.tianocore.devel/1506/focus=2640 It expects DHCP replies to arrive from remote source port 67. Even though dnsmasq conforms to that, the destination address (255.255.255.255) and the source address (eg. 192.168.122.1) in the reply allow the UDP masquerading rule to match, which rewrites the source port to or above 1024. This prevents the DHCP client in OVMF from accepting the packet. Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=709418 Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; --- v2->v3: - also prevent masquerading of directed broadcast [Laine] src/network/bridge_driver_linux.c | 151 +++++++++++++++++++++++++++++++++++++- 1 file changed, 147 insertions(+), 4 deletions(-) diff --git a/src/network/bridge_driver_linux.c b/src/network/bridge_driver_linux.c index 80430a8..093cba1 100644 --- a/src/network/bridge_driver_linux.c +++ b/src/network/bridge_driver_linux.c @@ -127,11 +127,64 @@ out: return ret; } +/* Fill in preallocated virPfxSocketAddr objects with masquerading exceptions: + * + * 1. do not masquerade packets targeting 224.0.0.0/24 + * 2. do not masquerade packets targeting 255.255.255.255/32 + * 3. do not masquerade packets targeting the directed local broadcast + * address + * + * 224.0.0.0/24 is the local network multicast range. Packets are not + * forwarded outside. + * + * 255.255.255.255/32 is the broadcast address of any local network. Again, + * such packets are never forwarded, but strict DHCP clients don't accept + * DHCP replies with changed source ports. + * + * The directed local broadcast address looks like 192.168.122.255/32, and + * behaves like the generic broadcast address 255.255.255.255/32. + * + * Returns 0 on success and -1 on failure. + */ +static int networkFillMasqExceptions(const char *bridgeName, + const virPfxSocketAddr *bridge, + virPfxSocketAddr *localMulticast, + virPfxSocketAddr *genericBroadcast, + virPfxSocketAddr *directedBroadcast) +{ + int result; + + localMulticast->prefix = 24; + result = virSocketAddrParseIPv4(&localMulticast->addr, + "224.0.0.0"); + sa_assert(result != -1);

+ + genericBroadcast->prefix = 32; + result = virSocketAddrParseIPv4(&genericBroadcast->addr, + "255.255.255.255"); + sa_assert(result != -1); + + directedBroadcast->prefix = 32; + result = virSocketAddrBroadcastByPrefix(&bridge->addr, bridge->prefix, + &directedBroadcast->addr); + if (result == -1) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Couldn't form directed broadcast address for '%s'"), + bridgeName); + return -1; + } + return 0; +} + int networkAddMasqueradingFirewallRules(virNetworkObjPtr network, virNetworkIpDefPtr ipdef) { int prefix = virNetworkIpDefPrefix(ipdef); const char *forwardIf = virNetworkDefForwardIf(network->def, 0); + virPfxSocketAddr bridgePfxAddr, + localMulticast, + genericBroadcast, + directedBroadcast; if (prefix < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -140,6 +193,15 @@ int networkAddMasqueradingFirewallRules(virNetworkObjPtr network, goto masqerr1; } + bridgePfxAddr.addr = ipdef->address; + bridgePfxAddr.prefix = prefix; + if (networkFillMasqExceptions(network->def->bridge, + &bridgePfxAddr, + &localMulticast, + &genericBroadcast, + &directedBroadcast) == -1) + goto masqerr1; + /* allow forwarding packets from the bridge interface */ if (iptablesAddForwardAllowOut(&ipdef->address, prefix, @@ -167,11 +229,12 @@ int networkAddMasqueradingFirewallRules(virNetworkObjPtr network, /* * Enable masquerading. * - * We need to end up with 3 rules in the table in this order + * We need to end up with 6 rules in the table in this order * - * 1. protocol=tcp with sport mapping restriction - * 2. protocol=udp with sport mapping restriction - * 3. generic any protocol + * 1-3. masquerading exceptions + * 4. masquerade protocol=tcp with sport mapping restriction + * 5. masquerade protocol=udp with sport mapping restriction + * 6. generic, masquerade any protocol * * The sport mappings are required, because default IPtables * MASQUERADE maintain port numbers unchanged where possible. @@ -238,8 +301,65 @@ int networkAddMasqueradingFirewallRules(virNetworkObjPtr network, goto masqerr5; } + /* exempt generic broadcast address as destination */ + if (iptablesAddDontMasquerade(&bridgePfxAddr, + forwardIf, + &genericBroadcast) == -1) { + if (forwardIf) + virReportError(VIR_ERR_SYSTEM_ERROR, + _("failed to add iptables rule to prevent generic broadcast masquerading on %s"), + forwardIf); + else + virReportError(VIR_ERR_SYSTEM_ERROR, "%s", + _("failed to add iptables rule to prevent generic broadcast masquerading")); + goto masqerr6; + } + + /* exempt directed broadcast address as destination */ + if (iptablesAddDontMasquerade(&bridgePfxAddr, + forwardIf, + &directedBroadcast) == -1) { + if (forwardIf) + virReportError(VIR_ERR_SYSTEM_ERROR, + _("failed to add iptables rule to prevent directed broadcast masquerading on %s"), + forwardIf); + else + virReportError(VIR_ERR_SYSTEM_ERROR, "%s", + _("failed to add iptables rule to prevent directed broadcast masquerading")); + goto masqerr7; + } + + /* exempt local multicast range as destination */ + if (iptablesAddDontMasquerade(&bridgePfxAddr, + forwardIf, + &localMulticast) == -1) { + if (forwardIf) + virReportError(VIR_ERR_SYSTEM_ERROR, + _("failed to add iptables rule to prevent local multicast masquerading on %s"), + forwardIf); + else + virReportError(VIR_ERR_SYSTEM_ERROR, "%s", + _("failed to add iptables rule to prevent local multicast masquerading")); + goto masqerr8; + } + return 0; + masqerr8: + iptablesRemoveDontMasquerade(&bridgePfxAddr, + forwardIf, + &directedBroadcast); + masqerr7: + iptablesRemoveDontMasquerade(&bridgePfxAddr, + forwardIf, + &genericBroadcast); + masqerr6: + iptablesRemoveForwardMasquerade(&ipdef->address, + prefix, + forwardIf, + &network->def->forward.addr, + &network->def->forward.port, + "tcp"); masqerr5: iptablesRemoveForwardMasquerade(&ipdef->address, prefix, @@ -275,6 +395,29 @@ void networkRemoveMasqueradingFirewallRules(virNetworkObjPtr network, const char *forwardIf = virNetworkDefForwardIf(network->def, 0); if (prefix >= 0) { + virPfxSocketAddr bridgePfxAddr, + localMulticast, + genericBroadcast, + directedBroadcast; + + bridgePfxAddr.addr = ipdef->address; + bridgePfxAddr.prefix = prefix; + if (networkFillMasqExceptions(network->def->bridge, + &bridgePfxAddr, + &localMulticast, + &genericBroadcast, + &directedBroadcast) == 0) { + iptablesRemoveDontMasquerade(&bridgePfxAddr, + forwardIf, + &localMulticast); + iptablesRemoveDontMasquerade(&bridgePfxAddr, + forwardIf, + &directedBroadcast); + iptablesRemoveDontMasquerade(&bridgePfxAddr, + forwardIf, + &genericBroadcast); + } + iptablesRemoveForwardMasquerade(&ipdef->address, prefix, forwardIf,