dnsmasq documentation says that the *IPv4* prefix/network
address/broadcast address sent to dhcp clients will be automatically
determined by dnsmasq by looking at the interface it's listening on,
so the original libvirt code did not add a netmask to the dnsmasq
commandline (or later, the dnsmasq conf file).
For *IPv6* however, dnsmasq apparently cannot automatically determine
the prefix (functionally the same as a netmask), and it must be
explicitly provided in the conf file (as a part of the dhcp-range
option). So many years after IPv4 DHCP support had been added, when
IPv6 dhcp support was added the prefix was included at the end of the
dhcp-range setting, but only for IPv6.
Recently a user reported (privately, because they suspected a possible
security implication, which turned out to be unfounded) a bug on a
host where one of the interfaces was a superset of the libvirt network
where dhcp is needed (e.g., the host's ethernet is 10.0.0.20/8, and
the libvirt network is 10.10.0.1/24). For some reason dnsmasq was
supplying the netmask for the /8 network to clients requesting an
address on the /24 interface.
This seems like a bug in dnsmasq, but even if/when it gets fixed
there, it looks like there is no harm in just always adding the
netmask to all IPv4 dhcp-range options similar to how prefix is added
to all IPv6 dhcp-range options.
Signed-off-by: Laine Stump <laine@laine.org>
---
src/network/bridge_driver.c | 27 +++++++++++++++----
.../dhcp6-nat-network.conf | 2 +-
.../networkxml2confdata/isolated-network.conf | 2 +-
.../nat-network-dns-srv-record-minimal.conf | 2 +-
.../nat-network-dns-srv-record.conf | 2 +-
.../nat-network-dns-txt-record.conf | 2 +-
.../networkxml2confdata/nat-network-mtu.conf | 2 +-
.../nat-network-name-with-quotes.conf | 2 +-
tests/networkxml2confdata/nat-network.conf | 2 +-
.../networkxml2confdata/netboot-network.conf | 2 +-
.../netboot-proxy-network.conf | 2 +-
.../networkxml2confdata/ptr-domains-auto.conf | 2 +-
12 files changed, 33 insertions(+), 16 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 6d80818e40..9fa902896b 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -1320,11 +1320,28 @@ networkDnsmasqConfContents(virNetworkObjPtr obj,
!(eaddr = virSocketAddrFormat(&ipdef->ranges[r].end)))
goto cleanup;
- virBufferAsprintf(&configbuf, "dhcp-range=%s,%s",
- saddr, eaddr);
- if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET6))
- virBufferAsprintf(&configbuf, ",%d", prefix);
- virBufferAddLit(&configbuf, "\n");
+ if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET6)) {
+ virBufferAsprintf(&configbuf, "dhcp-range=%s,%s,%d\n",
+ saddr, eaddr, prefix);
+ } else {
+ /* IPv4 - dnsmasq requires a netmask rather than prefix */
+ virSocketAddr netmask;
+ char *netmaskStr;
+
+ if (virSocketAddrPrefixToNetmask(prefix, &netmask, AF_INET) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Failed to translate bridge '%s' "
+ "prefix %d to netmask"),
+ def->bridge, prefix);
+ goto cleanup;
+ }
+
+ if (!(netmaskStr = virSocketAddrFormat(&netmask)))
+ goto cleanup;
+ virBufferAsprintf(&configbuf, "dhcp-range=%s,%s,%s\n",
+ saddr, eaddr, netmaskStr);
+ VIR_FREE(netmaskStr);
+ }
VIR_FREE(saddr);
VIR_FREE(eaddr);
diff --git a/tests/networkxml2confdata/dhcp6-nat-network.conf b/tests/networkxml2confdata/dhcp6-nat-network.conf
index d1058df3b6..536974e508 100644
--- a/tests/networkxml2confdata/dhcp6-nat-network.conf
+++ b/tests/networkxml2confdata/dhcp6-nat-network.conf
@@ -8,7 +8,7 @@ strict-order
except-interface=lo
bind-dynamic
interface=virbr0
-dhcp-range=192.168.122.2,192.168.122.254
+dhcp-range=192.168.122.2,192.168.122.254,255.255.255.0
dhcp-no-override
dhcp-authoritative
dhcp-range=2001:db8:ac10:fd01::1:10,2001:db8:ac10:fd01::1:ff,64
diff --git a/tests/networkxml2confdata/isolated-network.conf b/tests/networkxml2confdata/isolated-network.conf
index ce4a59f6c1..693a83d9a0 100644
--- a/tests/networkxml2confdata/isolated-network.conf
+++ b/tests/networkxml2confdata/isolated-network.conf
@@ -10,7 +10,7 @@ bind-interfaces
listen-address=192.168.152.1
dhcp-option=3
no-resolv
-dhcp-range=192.168.152.2,192.168.152.254
+dhcp-range=192.168.152.2,192.168.152.254,255.255.255.0
dhcp-no-override
dhcp-authoritative
dhcp-lease-max=253
diff --git a/tests/networkxml2confdata/nat-network-dns-srv-record-minimal.conf b/tests/networkxml2confdata/nat-network-dns-srv-record-minimal.conf
index f35ea1d5d4..0b2ca6f5aa 100644
--- a/tests/networkxml2confdata/nat-network-dns-srv-record-minimal.conf
+++ b/tests/networkxml2confdata/nat-network-dns-srv-record-minimal.conf
@@ -13,7 +13,7 @@ listen-address=fc00:db8:ac10:fe01::1
listen-address=fc00:db8:ac10:fd01::1
listen-address=10.24.10.1
srv-host=_name._tcp
-dhcp-range=192.168.122.2,192.168.122.254
+dhcp-range=192.168.122.2,192.168.122.254,255.255.255.0
dhcp-no-override
dhcp-authoritative
dhcp-lease-max=253
diff --git a/tests/networkxml2confdata/nat-network-dns-srv-record.conf b/tests/networkxml2confdata/nat-network-dns-srv-record.conf
index af1ed70758..a18c09aaa7 100644
--- a/tests/networkxml2confdata/nat-network-dns-srv-record.conf
+++ b/tests/networkxml2confdata/nat-network-dns-srv-record.conf
@@ -15,7 +15,7 @@ srv-host=_name4._tcp.test4.com,test4.example.com,4444
srv-host=_name5._udp,test5.example.com,1,55,555
srv-host=_name6._tcp.test6.com,test6.example.com,6666,0,666
srv-host=_name7._tcp.test7.com,test7.example.com,1,0,777
-dhcp-range=192.168.122.2,192.168.122.254
+dhcp-range=192.168.122.2,192.168.122.254,255.255.255.0
dhcp-no-override
dhcp-authoritative
dhcp-lease-max=253
diff --git a/tests/networkxml2confdata/nat-network-dns-txt-record.conf b/tests/networkxml2confdata/nat-network-dns-txt-record.conf
index 7f560fbb5c..735c261c01 100644
--- a/tests/networkxml2confdata/nat-network-dns-txt-record.conf
+++ b/tests/networkxml2confdata/nat-network-dns-txt-record.conf
@@ -9,7 +9,7 @@ except-interface=lo
bind-dynamic
interface=virbr0
txt-record=example,example value
-dhcp-range=192.168.122.2,192.168.122.254
+dhcp-range=192.168.122.2,192.168.122.254,255.255.255.0
dhcp-no-override
dhcp-authoritative
dhcp-lease-max=253
diff --git a/tests/networkxml2confdata/nat-network-mtu.conf b/tests/networkxml2confdata/nat-network-mtu.conf
index 91b574b964..1dd4754f2a 100644
--- a/tests/networkxml2confdata/nat-network-mtu.conf
+++ b/tests/networkxml2confdata/nat-network-mtu.conf
@@ -8,7 +8,7 @@ strict-order
except-interface=lo
bind-dynamic
interface=virbr0
-dhcp-range=192.168.122.2,192.168.122.254
+dhcp-range=192.168.122.2,192.168.122.254,255.255.255.0
dhcp-no-override
dhcp-authoritative
dhcp-lease-max=253
diff --git a/tests/networkxml2confdata/nat-network-name-with-quotes.conf b/tests/networkxml2confdata/nat-network-name-with-quotes.conf
index 36e11d17b9..1b06de3066 100644
--- a/tests/networkxml2confdata/nat-network-name-with-quotes.conf
+++ b/tests/networkxml2confdata/nat-network-name-with-quotes.conf
@@ -13,7 +13,7 @@ listen-address=fc00:db8:ac10:fe01::1
listen-address=fc00:db8:ac10:fd01::1
listen-address=10.24.10.1
srv-host=_name._tcp
-dhcp-range=192.168.122.2,192.168.122.254
+dhcp-range=192.168.122.2,192.168.122.254,255.255.255.0
dhcp-no-override
dhcp-authoritative
dhcp-lease-max=253
diff --git a/tests/networkxml2confdata/nat-network.conf b/tests/networkxml2confdata/nat-network.conf
index a3c8b102d3..873a360acc 100644
--- a/tests/networkxml2confdata/nat-network.conf
+++ b/tests/networkxml2confdata/nat-network.conf
@@ -8,7 +8,7 @@ strict-order
except-interface=lo
bind-dynamic
interface=virbr0
-dhcp-range=192.168.122.2,192.168.122.254
+dhcp-range=192.168.122.2,192.168.122.254,255.255.255.0
dhcp-no-override
dhcp-authoritative
dhcp-lease-max=253
diff --git a/tests/networkxml2confdata/netboot-network.conf b/tests/networkxml2confdata/netboot-network.conf
index b554a5456c..99272b9d68 100644
--- a/tests/networkxml2confdata/netboot-network.conf
+++ b/tests/networkxml2confdata/netboot-network.conf
@@ -10,7 +10,7 @@ expand-hosts
except-interface=lo
bind-interfaces
listen-address=192.168.122.1
-dhcp-range=192.168.122.2,192.168.122.254
+dhcp-range=192.168.122.2,192.168.122.254,255.255.255.0
dhcp-no-override
dhcp-authoritative
enable-tftp
diff --git a/tests/networkxml2confdata/netboot-proxy-network.conf b/tests/networkxml2confdata/netboot-proxy-network.conf
index afb4033f7e..fb0a20cff4 100644
--- a/tests/networkxml2confdata/netboot-proxy-network.conf
+++ b/tests/networkxml2confdata/netboot-proxy-network.conf
@@ -10,7 +10,7 @@ expand-hosts
except-interface=lo
bind-interfaces
listen-address=192.168.122.1
-dhcp-range=192.168.122.2,192.168.122.254
+dhcp-range=192.168.122.2,192.168.122.254,255.255.255.0
dhcp-no-override
dhcp-authoritative
dhcp-boot=pxeboot.img,,10.20.30.40
diff --git a/tests/networkxml2confdata/ptr-domains-auto.conf b/tests/networkxml2confdata/ptr-domains-auto.conf
index 7f1a393dd5..86701c4ddf 100644
--- a/tests/networkxml2confdata/ptr-domains-auto.conf
+++ b/tests/networkxml2confdata/ptr-domains-auto.conf
@@ -10,7 +10,7 @@ local=/1.0.e.f.0.1.c.a.8.b.d.0.1.0.0.2.ip6.arpa/
except-interface=lo
bind-dynamic
interface=virbr0
-dhcp-range=192.168.122.2,192.168.122.254
+dhcp-range=192.168.122.2,192.168.122.254,255.255.255.0
dhcp-no-override
dhcp-authoritative
dhcp-lease-max=253
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 2/18/19 6:21 PM, Laine Stump wrote:
> dnsmasq documentation says that the *IPv4* prefix/network
> address/broadcast address sent to dhcp clients will be automatically
> determined by dnsmasq by looking at the interface it's listening on,
> so the original libvirt code did not add a netmask to the dnsmasq
> commandline (or later, the dnsmasq conf file).
>
> For *IPv6* however, dnsmasq apparently cannot automatically determine
> the prefix (functionally the same as a netmask), and it must be
> explicitly provided in the conf file (as a part of the dhcp-range
> option). So many years after IPv4 DHCP support had been added, when
> IPv6 dhcp support was added the prefix was included at the end of the
> dhcp-range setting, but only for IPv6.
>
> Recently a user reported (privately, because they suspected a possible
> security implication, which turned out to be unfounded) a bug on a
> host where one of the interfaces was a superset of the libvirt network
> where dhcp is needed (e.g., the host's ethernet is 10.0.0.20/8, and
> the libvirt network is 10.10.0.1/24). For some reason dnsmasq was
> supplying the netmask for the /8 network to clients requesting an
> address on the /24 interface.
>
> This seems like a bug in dnsmasq, but even if/when it gets fixed
> there, it looks like there is no harm in just always adding the
> netmask to all IPv4 dhcp-range options similar to how prefix is added
> to all IPv6 dhcp-range options.
>
> Signed-off-by: Laine Stump <laine@laine.org>
> ---
> src/network/bridge_driver.c | 27 +++++++++++++++----
> .../dhcp6-nat-network.conf | 2 +-
> .../networkxml2confdata/isolated-network.conf | 2 +-
> .../nat-network-dns-srv-record-minimal.conf | 2 +-
> .../nat-network-dns-srv-record.conf | 2 +-
> .../nat-network-dns-txt-record.conf | 2 +-
> .../networkxml2confdata/nat-network-mtu.conf | 2 +-
> .../nat-network-name-with-quotes.conf | 2 +-
> tests/networkxml2confdata/nat-network.conf | 2 +-
> .../networkxml2confdata/netboot-network.conf | 2 +-
> .../netboot-proxy-network.conf | 2 +-
> .../networkxml2confdata/ptr-domains-auto.conf | 2 +-
> 12 files changed, 33 insertions(+), 16 deletions(-)
>
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index 6d80818e40..9fa902896b 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -1320,11 +1320,28 @@ networkDnsmasqConfContents(virNetworkObjPtr obj,
> !(eaddr = virSocketAddrFormat(&ipdef->ranges[r].end)))
> goto cleanup;
>
> - virBufferAsprintf(&configbuf, "dhcp-range=%s,%s",
> - saddr, eaddr);
> - if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET6))
> - virBufferAsprintf(&configbuf, ",%d", prefix);
> - virBufferAddLit(&configbuf, "\n");
> + if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET6)) {
> + virBufferAsprintf(&configbuf, "dhcp-range=%s,%s,%d\n",
> + saddr, eaddr, prefix);
> + } else {
> + /* IPv4 - dnsmasq requires a netmask rather than prefix */
> + virSocketAddr netmask;
Does it matter if this isn't initialized? e.g. "= { 0 };"
> + char *netmaskStr;
VIR_AUTOFREE(char *) netmaskStr = NULL;
> +
> + if (virSocketAddrPrefixToNetmask(prefix, &netmask, AF_INET) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Failed to translate bridge '%s' "
> + "prefix %d to netmask"),
> + def->bridge, prefix);
> + goto cleanup;
> + }
> +
> + if (!(netmaskStr = virSocketAddrFormat(&netmask)))
> + goto cleanup;
> + virBufferAsprintf(&configbuf, "dhcp-range=%s,%s,%s\n",
> + saddr, eaddr, netmaskStr);
> + VIR_FREE(netmaskStr);
w/ VIR_AUTOFREE, this isn't necessary
I know bridge_driver doesn't use it yet, but doesn't harm to start.
Reviewed-by: John Ferlan <jferlan@redhat.com>
John
[...]
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 2/20/19 4:10 PM, John Ferlan wrote:
>
> On 2/18/19 6:21 PM, Laine Stump wrote:
>> dnsmasq documentation says that the *IPv4* prefix/network
>> address/broadcast address sent to dhcp clients will be automatically
>> determined by dnsmasq by looking at the interface it's listening on,
>> so the original libvirt code did not add a netmask to the dnsmasq
>> commandline (or later, the dnsmasq conf file).
>>
>> For *IPv6* however, dnsmasq apparently cannot automatically determine
>> the prefix (functionally the same as a netmask), and it must be
>> explicitly provided in the conf file (as a part of the dhcp-range
>> option). So many years after IPv4 DHCP support had been added, when
>> IPv6 dhcp support was added the prefix was included at the end of the
>> dhcp-range setting, but only for IPv6.
>>
>> Recently a user reported (privately, because they suspected a possible
>> security implication, which turned out to be unfounded) a bug on a
>> host where one of the interfaces was a superset of the libvirt network
>> where dhcp is needed (e.g., the host's ethernet is 10.0.0.20/8, and
>> the libvirt network is 10.10.0.1/24). For some reason dnsmasq was
>> supplying the netmask for the /8 network to clients requesting an
>> address on the /24 interface.
>>
>> This seems like a bug in dnsmasq, but even if/when it gets fixed
>> there, it looks like there is no harm in just always adding the
>> netmask to all IPv4 dhcp-range options similar to how prefix is added
>> to all IPv6 dhcp-range options.
>>
>> Signed-off-by: Laine Stump <laine@laine.org>
>> ---
>> src/network/bridge_driver.c | 27 +++++++++++++++----
>> .../dhcp6-nat-network.conf | 2 +-
>> .../networkxml2confdata/isolated-network.conf | 2 +-
>> .../nat-network-dns-srv-record-minimal.conf | 2 +-
>> .../nat-network-dns-srv-record.conf | 2 +-
>> .../nat-network-dns-txt-record.conf | 2 +-
>> .../networkxml2confdata/nat-network-mtu.conf | 2 +-
>> .../nat-network-name-with-quotes.conf | 2 +-
>> tests/networkxml2confdata/nat-network.conf | 2 +-
>> .../networkxml2confdata/netboot-network.conf | 2 +-
>> .../netboot-proxy-network.conf | 2 +-
>> .../networkxml2confdata/ptr-domains-auto.conf | 2 +-
>> 12 files changed, 33 insertions(+), 16 deletions(-)
>>
>> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
>> index 6d80818e40..9fa902896b 100644
>> --- a/src/network/bridge_driver.c
>> +++ b/src/network/bridge_driver.c
>> @@ -1320,11 +1320,28 @@ networkDnsmasqConfContents(virNetworkObjPtr obj,
>> !(eaddr = virSocketAddrFormat(&ipdef->ranges[r].end)))
>> goto cleanup;
>>
>> - virBufferAsprintf(&configbuf, "dhcp-range=%s,%s",
>> - saddr, eaddr);
>> - if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET6))
>> - virBufferAsprintf(&configbuf, ",%d", prefix);
>> - virBufferAddLit(&configbuf, "\n");
>> + if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET6)) {
>> + virBufferAsprintf(&configbuf, "dhcp-range=%s,%s,%d\n",
>> + saddr, eaddr, prefix);
>> + } else {
>> + /* IPv4 - dnsmasq requires a netmask rather than prefix */
>> + virSocketAddr netmask;
> Does it matter if this isn't initialized? e.g. "= { 0 };"
None of the other instances of virSocketAddr are initialized to 0, and
the intent is that the utility functions should fill in all fields that
are relevant. I'd rather not be the one responsible for starting a cargo
cult of explicitly initializing them when it's not necessary :-)
>
>> + char *netmaskStr;
> VIR_AUTOFREE(char *) netmaskStr = NULL;
>
>> +
>> + if (virSocketAddrPrefixToNetmask(prefix, &netmask, AF_INET) < 0) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>> + _("Failed to translate bridge '%s' "
>> + "prefix %d to netmask"),
>> + def->bridge, prefix);
>> + goto cleanup;
>> + }
>> +
>> + if (!(netmaskStr = virSocketAddrFormat(&netmask)))
>> + goto cleanup;
>> + virBufferAsprintf(&configbuf, "dhcp-range=%s,%s,%s\n",
>> + saddr, eaddr, netmaskStr);
>> + VIR_FREE(netmaskStr);
> w/ VIR_AUTOFREE, this isn't necessary
>
> I know bridge_driver doesn't use it yet, but doesn't harm to start.
Yeah, I'll do that before I push.
>
> Reviewed-by: John Ferlan <jferlan@redhat.com>
>
> John
>
> [...]
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2026 Red Hat, Inc.