[libvirt] [PATCH v2 2/2] bridge driver: don't masquerade local subnet broadcast/multicast packets
Laine Stump
laine at laine.org
Mon Sep 23 14:46:04 UTC 2013
On 09/23/2013 10:05 AM, Laszlo Ersek wrote:
> 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
> - 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 at redhat.com>
> ---
> src/network/bridge_driver_linux.c | 70 ++++++++++++++++++++++++++++++++++++---
> 1 file changed, 66 insertions(+), 4 deletions(-)
>
> diff --git a/src/network/bridge_driver_linux.c b/src/network/bridge_driver_linux.c
> index 80430a8..95add0e 100644
> --- a/src/network/bridge_driver_linux.c
> +++ b/src/network/bridge_driver_linux.c
> @@ -127,6 +127,9 @@ out:
> return ret;
> }
>
> +static const char networkLocalMulticast[] = "224.0.0.0/24";
> +static const char networkLocalBroadcast[] = "255.255.255.255/32";
1) After briefly looking at the references in the BZ, I agree with you
that it's safest to not yet bypass NAT on the entire multicast range.
Let's make sure we fully understand it, then submit a separate patch for
the larger range.
2) Along with 255.255.255.255/32, I think this patch can/should also add
a "networkDirectedLocalBroadcast" (which will obviously need to be a
local variable and recomputed each time). This can be computed by ORing
the ip address of the network with ~netmask, then appending a 32 prefix.
So for example, the directed broadcast for 192.168.122.1/24 would be
192.168.122.255/32.
Beyond that it looks good, so ACK with those changes (unless someone
else has anything to say).
> +
> int networkAddMasqueradingFirewallRules(virNetworkObjPtr network,
> virNetworkIpDefPtr ipdef)
> {
> @@ -167,11 +170,20 @@ 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 5 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. do not masquerade packets targeting 224.0.0.0/24
> + * 2. do not masquerade packets targeting 255.255.255.255/32
> + * 3. masquerade protocol=tcp with sport mapping restriction
> + * 4. masquerade protocol=udp with sport mapping restriction
> + * 5. generic, masquerade any protocol
> + *
> + * 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 sport mappings are required, because default IPtables
> * MASQUERADE maintain port numbers unchanged where possible.
> @@ -238,8 +250,50 @@ int networkAddMasqueradingFirewallRules(virNetworkObjPtr network,
> goto masqerr5;
> }
>
> + /* exempt local network broadcast address as destination */
> + if (iptablesAddForwardDontMasquerade(&ipdef->address,
> + prefix,
> + forwardIf,
> + networkLocalBroadcast) < 0) {
> + if (forwardIf)
> + virReportError(VIR_ERR_SYSTEM_ERROR,
> + _("failed to add iptables rule to prevent local broadcast masquerading on %s"),
> + forwardIf);
> + else
> + virReportError(VIR_ERR_SYSTEM_ERROR, "%s",
> + _("failed to add iptables rule to prevent local broadcast masquerading"));
> + goto masqerr6;
> + }
> +
> + /* exempt local multicast range as destination */
> + if (iptablesAddForwardDontMasquerade(&ipdef->address,
> + prefix,
> + forwardIf,
> + networkLocalMulticast) < 0) {
> + 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 masqerr7;
> + }
> +
> return 0;
>
> + masqerr7:
> + iptablesRemoveForwardDontMasquerade(&ipdef->address,
> + prefix,
> + forwardIf,
> + networkLocalBroadcast);
> + masqerr6:
> + iptablesRemoveForwardMasquerade(&ipdef->address,
> + prefix,
> + forwardIf,
> + &network->def->forward.addr,
> + &network->def->forward.port,
> + "tcp");
> masqerr5:
> iptablesRemoveForwardMasquerade(&ipdef->address,
> prefix,
> @@ -275,6 +329,14 @@ void networkRemoveMasqueradingFirewallRules(virNetworkObjPtr network,
> const char *forwardIf = virNetworkDefForwardIf(network->def, 0);
>
> if (prefix >= 0) {
> + iptablesRemoveForwardDontMasquerade(&ipdef->address,
> + prefix,
> + forwardIf,
> + networkLocalMulticast);
> + iptablesRemoveForwardDontMasquerade(&ipdef->address,
> + prefix,
> + forwardIf,
> + networkLocalBroadcast);
> iptablesRemoveForwardMasquerade(&ipdef->address,
> prefix,
> forwardIf,
More information about the libvir-list
mailing list