[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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 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,


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]