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

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



On 09/23/2013 08:03 PM, 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
> - eg. 192.168.122.255/32 (ie. address- and netmask-dependent (= directed)
>   local network broadcast address)


As you pointed out in an email responding to my request for this - it's
not necessary as it is already covered by all of the rules that *do*
want masquerading being qualified with  "! -d 192.168.122.0/24" (for
example). So you can/should take it out. Sorry for creating the extra work.


> 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>
> ---
>  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);

You must have accidentally left this in. libvirt is a library, so it
must never assert. In a case where the called function is guaranteed to
never fail (due to the args passed in), you can enclose it in
ignore_value():

     ignore_value(cirSocketAddrParseIPv4(.......)


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


ACK once the ill-fated directed broadcast rule and sa_asserts are removed.


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