[libvirt] [PATCH v3 4/4] bridge driver: don't masquerade local subnet broadcast/multicast packets
Laine Stump
laine at laine.org
Tue Sep 24 09:31:07 UTC 2013
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 at 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.
More information about the libvir-list
mailing list