[libvirt] [PATCH v4 2/2] bridge driver: don't masquerade local subnet broadcast/multicast packets

[Laine Stump]

On 09/25/2013 06:45 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>
> ---
>  v2->v4:
>  - Rename iptables(Add|Remove)ForwardDontMasquerade to
>           iptables(Add|Remove)DontMasquerade [Laine].
>
>  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..066779a 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";
> +
>  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 (iptablesAddDontMasquerade(&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 (iptablesAddDontMasquerade(&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:
> +    iptablesRemoveDontMasquerade(&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) {
> +        iptablesRemoveDontMasquerade(&ipdef->address,
> +                                     prefix,
> +                                     forwardIf,
> +                                     networkLocalMulticast);
> +        iptablesRemoveDontMasquerade(&ipdef->address,
> +                                     prefix,
> +                                     forwardIf,
> +                                     networkLocalBroadcast);
>          iptablesRemoveForwardMasquerade(&ipdef->address,
>                                          prefix,
>                                          forwardIf,

ACK. I'll push both patches as soon as I've done a sanity build locally.

Thanks for putting up with the nit-picking and false direction.