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

[Laszlo Ersek]

On 09/25/13 12:54, Laine Stump wrote:
> 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.

I think your comments have all been to the point -- I never had the
impression of nit-picking.

Thank you very much!
Laszlo