[libvirt] [PATCH 0/3] Better error reporting of bad IP address ranges

This resolves: https://bugzilla.redhat.com/show_bug.cgi?id=985653 Laine Stump (3): network: validate DHCP ranges are completely within defined network network: cleanup range loop in networkDnsmasqConfContents util: report all address range errors in virSocketAddrGetRange() src/conf/network_conf.c | 18 +-- src/network/bridge_driver.c | 24 +-- src/util/virsocketaddr.c | 180 ++++++++++++++++++--- src/util/virsocketaddr.h | 6 +- tests/networkxml2xmlupdatein/dhcp-range-10.xml | 1 + tests/networkxml2xmlupdatein/dhcp-range.xml | 2 +- .../dhcp6host-routed-network-another-range.xml | 2 +- .../dhcp6host-routed-network-range.xml | 2 +- tests/networkxml2xmlupdatetest.c | 5 + tests/sockettest.c | 55 ++++--- 10 files changed, 228 insertions(+), 67 deletions(-) create mode 100644 tests/networkxml2xmlupdatein/dhcp-range-10.xml -- 2.1.0

virSocketAddrGetRange() has been updated to take the network address and prefix, and now checks that both the start and end of the range are within that network, thus validating that the entire range of addresses is in the network. For IPv4, it also checks that ranges to not start with the "network address" of the subnet, nor end with the broadcast address of the subnet (this check doesn't apply to IPv6, since IPv6 doesn't have a broadcast or network address) Negative tests have been added to the network update and socket tests to verify that bad ranges properly generate an error. This resolves: https://bugzilla.redhat.com/show_bug.cgi?id=985653 --- src/conf/network_conf.c | 14 ++--- src/network/bridge_driver.c | 4 +- src/util/virsocketaddr.c | 61 ++++++++++++++++++---- src/util/virsocketaddr.h | 6 ++- tests/networkxml2xmlupdatein/dhcp-range-10.xml | 1 + tests/networkxml2xmlupdatein/dhcp-range.xml | 2 +- .../dhcp6host-routed-network-another-range.xml | 2 +- .../dhcp6host-routed-network-range.xml | 2 +- tests/networkxml2xmlupdatetest.c | 5 ++ tests/sockettest.c | 55 ++++++++++++------- 10 files changed, 112 insertions(+), 40 deletions(-) create mode 100644 tests/networkxml2xmlupdatein/dhcp-range-10.xml diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index bc63a3d..f9f3d3d 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -832,8 +832,9 @@ int virNetworkIpDefNetmask(const virNetworkIpDef *def, static int virSocketAddrRangeParseXML(const char *networkName, - xmlNodePtr node, - virSocketAddrRangePtr range) + virNetworkIpDefPtr ipdef, + xmlNodePtr node, + virSocketAddrRangePtr range) { @@ -859,7 +860,8 @@ virSocketAddrRangeParseXML(const char *networkName, goto cleanup; /* do a sanity check of the range */ - if (virSocketAddrGetRange(&range->start, &range->end) < 0) { + if (virSocketAddrGetRange(&range->start, &range->end, &ipdef->address, + virNetworkIpDefPrefix(ipdef)) < 0) { virReportError(VIR_ERR_XML_ERROR, _("Invalid dhcp range '%s' to '%s' in network '%s'"), start, end, networkName); @@ -1009,8 +1011,8 @@ virNetworkDHCPDefParseXML(const char *networkName, if (VIR_REALLOC_N(def->ranges, def->nranges + 1) < 0) return -1; - if (virSocketAddrRangeParseXML(networkName, cur, - &def->ranges[def->nranges]) < 0) { + if (virSocketAddrRangeParseXML(networkName, def, cur, + &def->ranges[def->nranges]) < 0) { return -1; } def->nranges++; @@ -3608,7 +3610,7 @@ virNetworkDefUpdateIPDHCPRange(virNetworkDefPtr def, goto cleanup; } - if (virSocketAddrRangeParseXML(def->name, ctxt->node, &range) < 0) + if (virSocketAddrRangeParseXML(def->name, ipdef, ctxt->node, &range) < 0) goto cleanup; /* check if an entry with same name/address/ip already exists */ diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 4b53475..e08a316 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -1193,7 +1193,9 @@ networkDnsmasqConfContents(virNetworkObjPtr network, VIR_FREE(saddr); VIR_FREE(eaddr); nbleases += virSocketAddrGetRange(&ipdef->ranges[r].start, - &ipdef->ranges[r].end); + &ipdef->ranges[r].end, + &ipdef->address, + virNetworkIpDefPrefix(ipdef)); } /* diff --git a/src/util/virsocketaddr.c b/src/util/virsocketaddr.c index 67ed330..0edf345 100644 --- a/src/util/virsocketaddr.c +++ b/src/util/virsocketaddr.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2009-2014 Red Hat, Inc. + * Copyright (C) 2009-2015 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -605,31 +605,69 @@ int virSocketAddrCheckNetmask(virSocketAddrPtr addr1, virSocketAddrPtr addr2, * virSocketGetRange: * @start: start of an IP range * @end: end of an IP range + * @network: IP address of network that should completely contain this range + * @prefix: prefix of the network * - * Check the order of the 2 addresses and compute the range, this - * will return 1 for identical addresses. Errors can come from incompatible - * addresses type, excessive range (>= 2^^16) where the two addresses are - * unrelated or inverted start and end. + * Check the order of the 2 addresses and compute the range, this will + * return 1 for identical addresses. Errors can come from incompatible + * addresses type, excessive range (>= 2^^16) where the two addresses + * are unrelated, inverted start and end, or a range that is not + * within network/prefix. * * Returns the size of the range or -1 in case of failure */ -int virSocketAddrGetRange(virSocketAddrPtr start, virSocketAddrPtr end) +int +virSocketAddrGetRange(virSocketAddrPtr start, virSocketAddrPtr end, + virSocketAddrPtr network, int prefix) { int ret = 0; size_t i; + virSocketAddr netmask; + + if (start == NULL || end == NULL || network == NULL) + return -1; + + if (VIR_SOCKET_ADDR_FAMILY(start) != VIR_SOCKET_ADDR_FAMILY(end) || + VIR_SOCKET_ADDR_FAMILY(start) != VIR_SOCKET_ADDR_FAMILY(network)) + return -1; - if ((start == NULL) || (end == NULL)) + if (prefix < 0 || + virSocketAddrPrefixToNetmask(prefix, &netmask, VIR_SOCKET_ADDR_FAMILY(network)) < 0) return -1; - if (start->data.stor.ss_family != end->data.stor.ss_family) + + /* both start and end of range need to be in the same network as + * "network" + */ + if (virSocketAddrCheckNetmask(start, network, &netmask) <= 0 || + virSocketAddrCheckNetmask(start, network, &netmask) <= 0) return -1; - if (start->data.stor.ss_family == AF_INET) { + if (VIR_SOCKET_ADDR_IS_FAMILY(start, AF_INET)) { virSocketAddrIPv4 t1, t2; + virSocketAddr netaddr, broadcast; + + if (virSocketAddrBroadcast(network, &netmask, &broadcast) < 0 || + virSocketAddrMask(network, &netmask, &netaddr) < 0) + return -1; + + /* Don't allow the start of the range to be the network + * address (usually "...0") or the end of the range to be the + * broadcast address (usually "...255"). (the opposite also + * isn't allowed, but checking for that is implicit in all the + * other combined checks) (IPv6 doesn't have broadcast and + * network addresses, so this check is only done for IPv4) + */ + if (virSocketAddrEqual(start, &netaddr) || + virSocketAddrEqual(end, &broadcast)) + return -1; if ((virSocketAddrGetIPv4Addr(start, &t1) < 0) || (virSocketAddrGetIPv4Addr(end, &t2) < 0)) return -1; + /* legacy check that everything except the last two bytes are + * the same + */ for (i = 0; i < 2; i++) { if (t1[i] != t2[i]) return -1; @@ -638,13 +676,16 @@ int virSocketAddrGetRange(virSocketAddrPtr start, virSocketAddrPtr end) if (ret < 0) return -1; ret++; - } else if (start->data.stor.ss_family == AF_INET6) { + } else if (VIR_SOCKET_ADDR_IS_FAMILY(start, AF_INET6)) { virSocketAddrIPv6 t1, t2; if ((virSocketAddrGetIPv6Addr(start, &t1) < 0) || (virSocketAddrGetIPv6Addr(end, &t2) < 0)) return -1; + /* legacy check that everything except the last two bytes are + * the same + */ for (i = 0; i < 7; i++) { if (t1[i] != t2[i]) return -1; diff --git a/src/util/virsocketaddr.h b/src/util/virsocketaddr.h index 99ab46f..9e2680a 100644 --- a/src/util/virsocketaddr.h +++ b/src/util/virsocketaddr.h @@ -1,5 +1,5 @@ /* - * Copyright (C) 2009-2013 Red Hat, Inc. + * Copyright (C) 2009-2013, 2015 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -96,7 +96,9 @@ int virSocketAddrSetPort(virSocketAddrPtr addr, int port); int virSocketAddrGetPort(virSocketAddrPtr addr); int virSocketAddrGetRange(virSocketAddrPtr start, - virSocketAddrPtr end); + virSocketAddrPtr end, + virSocketAddrPtr network, + int prefix); int virSocketAddrIsNetmask(virSocketAddrPtr netmask); diff --git a/tests/networkxml2xmlupdatein/dhcp-range-10.xml b/tests/networkxml2xmlupdatein/dhcp-range-10.xml new file mode 100644 index 0000000..ed775c8 --- /dev/null +++ b/tests/networkxml2xmlupdatein/dhcp-range-10.xml @@ -0,0 +1 @@ +<range start='10.0.0.10' end='10.0.0.100'/> diff --git a/tests/networkxml2xmlupdatein/dhcp-range.xml b/tests/networkxml2xmlupdatein/dhcp-range.xml index ed775c8..d5e283c 100644 --- a/tests/networkxml2xmlupdatein/dhcp-range.xml +++ b/tests/networkxml2xmlupdatein/dhcp-range.xml @@ -1 +1 @@ -<range start='10.0.0.10' end='10.0.0.100'/> +<range start='192.168.122.10' end='192.168.122.100'/> diff --git a/tests/networkxml2xmlupdateout/dhcp6host-routed-network-another-range.xml b/tests/networkxml2xmlupdateout/dhcp6host-routed-network-another-range.xml index ee6eb7a..4a1360d 100644 --- a/tests/networkxml2xmlupdateout/dhcp6host-routed-network-another-range.xml +++ b/tests/networkxml2xmlupdateout/dhcp6host-routed-network-another-range.xml @@ -8,7 +8,7 @@ <mac address='12:34:56:78:9a:bc'/> <ip address='192.168.122.1' netmask='255.255.255.0'> <dhcp> - <range start='10.0.0.10' end='10.0.0.100'/> + <range start='192.168.122.10' end='192.168.122.100'/> <host mac='00:16:3e:77:e2:ed' name='a.example.com' ip='192.168.122.10'/> <host mac='00:16:3e:3e:a9:1a' name='b.example.com' ip='192.168.122.11'/> </dhcp> diff --git a/tests/networkxml2xmlupdateout/dhcp6host-routed-network-range.xml b/tests/networkxml2xmlupdateout/dhcp6host-routed-network-range.xml index ee6eb7a..4a1360d 100644 --- a/tests/networkxml2xmlupdateout/dhcp6host-routed-network-range.xml +++ b/tests/networkxml2xmlupdateout/dhcp6host-routed-network-range.xml @@ -8,7 +8,7 @@ <mac address='12:34:56:78:9a:bc'/> <ip address='192.168.122.1' netmask='255.255.255.0'> <dhcp> - <range start='10.0.0.10' end='10.0.0.100'/> + <range start='192.168.122.10' end='192.168.122.100'/> <host mac='00:16:3e:77:e2:ed' name='a.example.com' ip='192.168.122.10'/> <host mac='00:16:3e:3e:a9:1a' name='b.example.com' ip='192.168.122.11'/> </dhcp> diff --git a/tests/networkxml2xmlupdatetest.c b/tests/networkxml2xmlupdatetest.c index af697bb..0241378 100644 --- a/tests/networkxml2xmlupdatetest.c +++ b/tests/networkxml2xmlupdatetest.c @@ -202,6 +202,11 @@ mymain(void) "dhcp6host-routed-network-range", VIR_NETWORK_UPDATE_COMMAND_ADD_FIRST, 0); + DO_TEST_INDEX_FAIL("add-dhcp-range-outside-net", + "dhcp-range-10", + "dhcp6host-routed-network", + VIR_NETWORK_UPDATE_COMMAND_ADD_FIRST, + 0); DO_TEST_INDEX("append-dhcp-range", "dhcp-range", "dhcp6host-routed-network", diff --git a/tests/sockettest.c b/tests/sockettest.c index 0d348d9..84170d5 100644 --- a/tests/sockettest.c +++ b/tests/sockettest.c @@ -1,7 +1,7 @@ /* * sockettest.c: Testing for src/util/network.c APIs * - * Copyright (C) 2010-2011, 2014 Red Hat, Inc. + * Copyright (C) 2010-2011, 2014, 2015 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -86,17 +86,22 @@ static int testFormatHelper(const void *opaque) } -static int testRange(const char *saddrstr, const char *eaddrstr, int size, bool pass) +static int +testRange(const char *saddrstr, const char *eaddrstr, + const char *netstr, int prefix, int size, bool pass) { virSocketAddr saddr; virSocketAddr eaddr; + virSocketAddr netaddr; if (virSocketAddrParse(&saddr, saddrstr, AF_UNSPEC) < 0) return -1; if (virSocketAddrParse(&eaddr, eaddrstr, AF_UNSPEC) < 0) return -1; + if (virSocketAddrParse(&netaddr, netstr, AF_UNSPEC) < 0) + return -1; - int gotsize = virSocketAddrGetRange(&saddr, &eaddr); + int gotsize = virSocketAddrGetRange(&saddr, &eaddr, &netaddr, prefix); VIR_DEBUG("Size want %d vs got %d", size, gotsize); if (gotsize < 0 || gotsize != size) { return pass ? -1 : 0; @@ -105,16 +110,23 @@ static int testRange(const char *saddrstr, const char *eaddrstr, int size, bool } } + struct testRangeData { const char *saddr; const char *eaddr; + const char *netaddr; + int prefix; int size; bool pass; }; + + static int testRangeHelper(const void *opaque) { const struct testRangeData *data = opaque; - return testRange(data->saddr, data->eaddr, data->size, data->pass); + return testRange(data->saddr, data->eaddr, + data->netaddr, data->prefix, + data->size, data->pass); } @@ -287,10 +299,12 @@ mymain(void) ret = -1; \ } while (0) -#define DO_TEST_RANGE(saddr, eaddr, size, pass) \ +#define DO_TEST_RANGE(saddr, eaddr, netaddr, prefix, size, pass) \ do { \ - struct testRangeData data = { saddr, eaddr, size, pass }; \ - if (virtTestRun("Test range " saddr " -> " eaddr " size " #size, \ + struct testRangeData data \ + = { saddr, eaddr, netaddr, prefix, size, pass }; \ + if (virtTestRun("Test range " saddr " -> " eaddr "(" netaddr \ + "/" #prefix") size " #size, \ testRangeHelper, &data) < 0) \ ret = -1; \ } while (0) @@ -357,17 +371,22 @@ mymain(void) DO_TEST_PARSE_AND_FORMAT("::1", AF_UNIX, false); DO_TEST_PARSE_AND_FORMAT("::ffff", AF_UNSPEC, true); - DO_TEST_RANGE("192.168.122.1", "192.168.122.1", 1, true); - DO_TEST_RANGE("192.168.122.1", "192.168.122.20", 20, true); - DO_TEST_RANGE("192.168.122.0", "192.168.122.255", 256, true); - DO_TEST_RANGE("192.168.122.20", "192.168.122.1", -1, false); - DO_TEST_RANGE("10.0.0.1", "192.168.122.20", -1, false); - DO_TEST_RANGE("192.168.122.20", "10.0.0.1", -1, false); - - DO_TEST_RANGE("2000::1", "2000::1", 1, true); - DO_TEST_RANGE("2000::1", "2000::2", 2, true); - DO_TEST_RANGE("2000::2", "2000::1", -1, false); - DO_TEST_RANGE("2000::1", "9001::1", -1, false); + DO_TEST_RANGE("192.168.122.1", "192.168.122.1", "192.168.122.1", 24, 1, true); + DO_TEST_RANGE("192.168.122.1", "192.168.122.20", "192.168.122.22", 24, 20, true); + DO_TEST_RANGE("192.168.122.0", "192.168.122.254", "192.168.122.1", 24, -1, false); + DO_TEST_RANGE("192.168.122.1", "192.168.122.255", "192.168.122.1", 24, -1, false); + DO_TEST_RANGE("192.168.122.0", "192.168.122.255", "192.168.122.1", 16, 256, true); + DO_TEST_RANGE("192.168.122.20", "192.168.122.1", "192.168.122.1", 24, -1, false); + DO_TEST_RANGE("10.0.0.1", "192.168.122.20", "192.168.122.1", 24, -1, false); + DO_TEST_RANGE("192.168.122.20", "10.0.0.1", "192.168.122.1", 24, -1, false); + DO_TEST_RANGE("172.16.0.50", "172.16.0.254", "1.2.3.4", 8, -1, false); + DO_TEST_RANGE("192.168.122.1", "192.168.123.20", "192.168.122.22", 24, -1, false); + DO_TEST_RANGE("192.168.122.1", "192.168.123.20", "192.168.122.22", 23, 276, true); + + DO_TEST_RANGE("2000::1", "2000::1", "2000::1", 64, 1, true); + DO_TEST_RANGE("2000::1", "2000::2", "2000::1", 64, 2, true); + DO_TEST_RANGE("2000::2", "2000::1", "2000::1", 64, -1, false); + DO_TEST_RANGE("2000::1", "9001::1", "2000::1", 64, -1, false); DO_TEST_NETMASK("192.168.122.1", "192.168.122.2", "255.255.255.0", true); -- 2.1.0

On 05/26/2015 03:48 PM, Laine Stump wrote:
virSocketAddrGetRange() has been updated to take the network address and prefix, and now checks that both the start and end of the range are within that network, thus validating that the entire range of addresses is in the network. For IPv4, it also checks that ranges to not start with the "network address" of the subnet, nor end with the broadcast address of the subnet (this check doesn't apply to IPv6, since IPv6 doesn't have a broadcast or network address)
Negative tests have been added to the network update and socket tests to verify that bad ranges properly generate an error.
This resolves: https://bugzilla.redhat.com/show_bug.cgi?id=985653 --- src/conf/network_conf.c | 14 ++--- src/network/bridge_driver.c | 4 +- src/util/virsocketaddr.c | 61 ++++++++++++++++++---- src/util/virsocketaddr.h | 6 ++- tests/networkxml2xmlupdatein/dhcp-range-10.xml | 1 + tests/networkxml2xmlupdatein/dhcp-range.xml | 2 +- .../dhcp6host-routed-network-another-range.xml | 2 +- .../dhcp6host-routed-network-range.xml | 2 +- tests/networkxml2xmlupdatetest.c | 5 ++ tests/sockettest.c | 55 ++++++++++++------- 10 files changed, 112 insertions(+), 40 deletions(-) create mode 100644 tests/networkxml2xmlupdatein/dhcp-range-10.xml
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index bc63a3d..f9f3d3d 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -832,8 +832,9 @@ int virNetworkIpDefNetmask(const virNetworkIpDef *def,
static int virSocketAddrRangeParseXML(const char *networkName, - xmlNodePtr node, - virSocketAddrRangePtr range) + virNetworkIpDefPtr ipdef, + xmlNodePtr node, + virSocketAddrRangePtr range) {
@@ -859,7 +860,8 @@ virSocketAddrRangeParseXML(const char *networkName, goto cleanup;
/* do a sanity check of the range */ - if (virSocketAddrGetRange(&range->start, &range->end) < 0) { + if (virSocketAddrGetRange(&range->start, &range->end, &ipdef->address, + virNetworkIpDefPrefix(ipdef)) < 0) { virReportError(VIR_ERR_XML_ERROR, _("Invalid dhcp range '%s' to '%s' in network '%s'"), start, end, networkName); @@ -1009,8 +1011,8 @@ virNetworkDHCPDefParseXML(const char *networkName,
if (VIR_REALLOC_N(def->ranges, def->nranges + 1) < 0) return -1; - if (virSocketAddrRangeParseXML(networkName, cur, - &def->ranges[def->nranges]) < 0) { + if (virSocketAddrRangeParseXML(networkName, def, cur, + &def->ranges[def->nranges]) < 0) { return -1; } def->nranges++; @@ -3608,7 +3610,7 @@ virNetworkDefUpdateIPDHCPRange(virNetworkDefPtr def, goto cleanup; }
- if (virSocketAddrRangeParseXML(def->name, ctxt->node, &range) < 0) + if (virSocketAddrRangeParseXML(def->name, ipdef, ctxt->node, &range) < 0) goto cleanup;
/* check if an entry with same name/address/ip already exists */ diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 4b53475..e08a316 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -1193,7 +1193,9 @@ networkDnsmasqConfContents(virNetworkObjPtr network, VIR_FREE(saddr); VIR_FREE(eaddr); nbleases += virSocketAddrGetRange(&ipdef->ranges[r].start, - &ipdef->ranges[r].end); + &ipdef->ranges[r].end, + &ipdef->address, + virNetworkIpDefPrefix(ipdef)); }
/* diff --git a/src/util/virsocketaddr.c b/src/util/virsocketaddr.c index 67ed330..0edf345 100644 --- a/src/util/virsocketaddr.c +++ b/src/util/virsocketaddr.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2009-2014 Red Hat, Inc. + * Copyright (C) 2009-2015 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -605,31 +605,69 @@ int virSocketAddrCheckNetmask(virSocketAddrPtr addr1, virSocketAddrPtr addr2, * virSocketGetRange: * @start: start of an IP range * @end: end of an IP range + * @network: IP address of network that should completely contain this range + * @prefix: prefix of the network * - * Check the order of the 2 addresses and compute the range, this - * will return 1 for identical addresses. Errors can come from incompatible - * addresses type, excessive range (>= 2^^16) where the two addresses are - * unrelated or inverted start and end. + * Check the order of the 2 addresses and compute the range, this will + * return 1 for identical addresses. Errors can come from incompatible + * addresses type, excessive range (>= 2^^16) where the two addresses + * are unrelated, inverted start and end, or a range that is not + * within network/prefix. * * Returns the size of the range or -1 in case of failure */ -int virSocketAddrGetRange(virSocketAddrPtr start, virSocketAddrPtr end) +int +virSocketAddrGetRange(virSocketAddrPtr start, virSocketAddrPtr end, + virSocketAddrPtr network, int prefix) { int ret = 0; size_t i; + virSocketAddr netmask; + + if (start == NULL || end == NULL || network == NULL) + return -1; + + if (VIR_SOCKET_ADDR_FAMILY(start) != VIR_SOCKET_ADDR_FAMILY(end) || + VIR_SOCKET_ADDR_FAMILY(start) != VIR_SOCKET_ADDR_FAMILY(network)) + return -1;
- if ((start == NULL) || (end == NULL)) + if (prefix < 0 || + virSocketAddrPrefixToNetmask(prefix, &netmask, VIR_SOCKET_ADDR_FAMILY(network)) < 0) return -1; - if (start->data.stor.ss_family != end->data.stor.ss_family) + + /* both start and end of range need to be in the same network as + * "network" + */ + if (virSocketAddrCheckNetmask(start, network, &netmask) <= 0 || + virSocketAddrCheckNetmask(start, network, &netmask) <= 0)
Should I assume this is a cut-n-paste (eg, the second call should use end not start)... Side note - seems there's no "FAIL" test for this condition - considering we passed the tests with the check this way... John
return -1;
- if (start->data.stor.ss_family == AF_INET) { + if (VIR_SOCKET_ADDR_IS_FAMILY(start, AF_INET)) { virSocketAddrIPv4 t1, t2; + virSocketAddr netaddr, broadcast; + + if (virSocketAddrBroadcast(network, &netmask, &broadcast) < 0 || + virSocketAddrMask(network, &netmask, &netaddr) < 0) + return -1; + + /* Don't allow the start of the range to be the network + * address (usually "...0") or the end of the range to be the + * broadcast address (usually "...255"). (the opposite also + * isn't allowed, but checking for that is implicit in all the + * other combined checks) (IPv6 doesn't have broadcast and + * network addresses, so this check is only done for IPv4) + */ + if (virSocketAddrEqual(start, &netaddr) || + virSocketAddrEqual(end, &broadcast)) + return -1;
if ((virSocketAddrGetIPv4Addr(start, &t1) < 0) || (virSocketAddrGetIPv4Addr(end, &t2) < 0)) return -1;
+ /* legacy check that everything except the last two bytes are + * the same + */ for (i = 0; i < 2; i++) { if (t1[i] != t2[i]) return -1; @@ -638,13 +676,16 @@ int virSocketAddrGetRange(virSocketAddrPtr start, virSocketAddrPtr end) if (ret < 0) return -1; ret++; - } else if (start->data.stor.ss_family == AF_INET6) { + } else if (VIR_SOCKET_ADDR_IS_FAMILY(start, AF_INET6)) { virSocketAddrIPv6 t1, t2;
if ((virSocketAddrGetIPv6Addr(start, &t1) < 0) || (virSocketAddrGetIPv6Addr(end, &t2) < 0)) return -1;
+ /* legacy check that everything except the last two bytes are + * the same + */ for (i = 0; i < 7; i++) { if (t1[i] != t2[i]) return -1; diff --git a/src/util/virsocketaddr.h b/src/util/virsocketaddr.h index 99ab46f..9e2680a 100644 --- a/src/util/virsocketaddr.h +++ b/src/util/virsocketaddr.h @@ -1,5 +1,5 @@ /* - * Copyright (C) 2009-2013 Red Hat, Inc. + * Copyright (C) 2009-2013, 2015 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -96,7 +96,9 @@ int virSocketAddrSetPort(virSocketAddrPtr addr, int port); int virSocketAddrGetPort(virSocketAddrPtr addr);
int virSocketAddrGetRange(virSocketAddrPtr start, - virSocketAddrPtr end); + virSocketAddrPtr end, + virSocketAddrPtr network, + int prefix);
int virSocketAddrIsNetmask(virSocketAddrPtr netmask);
diff --git a/tests/networkxml2xmlupdatein/dhcp-range-10.xml b/tests/networkxml2xmlupdatein/dhcp-range-10.xml new file mode 100644 index 0000000..ed775c8 --- /dev/null +++ b/tests/networkxml2xmlupdatein/dhcp-range-10.xml @@ -0,0 +1 @@ +<range start='10.0.0.10' end='10.0.0.100'/> diff --git a/tests/networkxml2xmlupdatein/dhcp-range.xml b/tests/networkxml2xmlupdatein/dhcp-range.xml index ed775c8..d5e283c 100644 --- a/tests/networkxml2xmlupdatein/dhcp-range.xml +++ b/tests/networkxml2xmlupdatein/dhcp-range.xml @@ -1 +1 @@ -<range start='10.0.0.10' end='10.0.0.100'/> +<range start='192.168.122.10' end='192.168.122.100'/> diff --git a/tests/networkxml2xmlupdateout/dhcp6host-routed-network-another-range.xml b/tests/networkxml2xmlupdateout/dhcp6host-routed-network-another-range.xml index ee6eb7a..4a1360d 100644 --- a/tests/networkxml2xmlupdateout/dhcp6host-routed-network-another-range.xml +++ b/tests/networkxml2xmlupdateout/dhcp6host-routed-network-another-range.xml @@ -8,7 +8,7 @@ <mac address='12:34:56:78:9a:bc'/> <ip address='192.168.122.1' netmask='255.255.255.0'> <dhcp> - <range start='10.0.0.10' end='10.0.0.100'/> + <range start='192.168.122.10' end='192.168.122.100'/> <host mac='00:16:3e:77:e2:ed' name='a.example.com' ip='192.168.122.10'/> <host mac='00:16:3e:3e:a9:1a' name='b.example.com' ip='192.168.122.11'/> </dhcp> diff --git a/tests/networkxml2xmlupdateout/dhcp6host-routed-network-range.xml b/tests/networkxml2xmlupdateout/dhcp6host-routed-network-range.xml index ee6eb7a..4a1360d 100644 --- a/tests/networkxml2xmlupdateout/dhcp6host-routed-network-range.xml +++ b/tests/networkxml2xmlupdateout/dhcp6host-routed-network-range.xml @@ -8,7 +8,7 @@ <mac address='12:34:56:78:9a:bc'/> <ip address='192.168.122.1' netmask='255.255.255.0'> <dhcp> - <range start='10.0.0.10' end='10.0.0.100'/> + <range start='192.168.122.10' end='192.168.122.100'/> <host mac='00:16:3e:77:e2:ed' name='a.example.com' ip='192.168.122.10'/> <host mac='00:16:3e:3e:a9:1a' name='b.example.com' ip='192.168.122.11'/> </dhcp> diff --git a/tests/networkxml2xmlupdatetest.c b/tests/networkxml2xmlupdatetest.c index af697bb..0241378 100644 --- a/tests/networkxml2xmlupdatetest.c +++ b/tests/networkxml2xmlupdatetest.c @@ -202,6 +202,11 @@ mymain(void) "dhcp6host-routed-network-range", VIR_NETWORK_UPDATE_COMMAND_ADD_FIRST, 0); + DO_TEST_INDEX_FAIL("add-dhcp-range-outside-net", + "dhcp-range-10", + "dhcp6host-routed-network", + VIR_NETWORK_UPDATE_COMMAND_ADD_FIRST, + 0); DO_TEST_INDEX("append-dhcp-range", "dhcp-range", "dhcp6host-routed-network", diff --git a/tests/sockettest.c b/tests/sockettest.c index 0d348d9..84170d5 100644 --- a/tests/sockettest.c +++ b/tests/sockettest.c @@ -1,7 +1,7 @@ /* * sockettest.c: Testing for src/util/network.c APIs * - * Copyright (C) 2010-2011, 2014 Red Hat, Inc. + * Copyright (C) 2010-2011, 2014, 2015 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -86,17 +86,22 @@ static int testFormatHelper(const void *opaque) }
-static int testRange(const char *saddrstr, const char *eaddrstr, int size, bool pass) +static int +testRange(const char *saddrstr, const char *eaddrstr, + const char *netstr, int prefix, int size, bool pass) { virSocketAddr saddr; virSocketAddr eaddr; + virSocketAddr netaddr;
if (virSocketAddrParse(&saddr, saddrstr, AF_UNSPEC) < 0) return -1; if (virSocketAddrParse(&eaddr, eaddrstr, AF_UNSPEC) < 0) return -1; + if (virSocketAddrParse(&netaddr, netstr, AF_UNSPEC) < 0) + return -1;
- int gotsize = virSocketAddrGetRange(&saddr, &eaddr); + int gotsize = virSocketAddrGetRange(&saddr, &eaddr, &netaddr, prefix); VIR_DEBUG("Size want %d vs got %d", size, gotsize); if (gotsize < 0 || gotsize != size) { return pass ? -1 : 0; @@ -105,16 +110,23 @@ static int testRange(const char *saddrstr, const char *eaddrstr, int size, bool } }
+ struct testRangeData { const char *saddr; const char *eaddr; + const char *netaddr; + int prefix; int size; bool pass; }; + + static int testRangeHelper(const void *opaque) { const struct testRangeData *data = opaque; - return testRange(data->saddr, data->eaddr, data->size, data->pass); + return testRange(data->saddr, data->eaddr, + data->netaddr, data->prefix, + data->size, data->pass); }
@@ -287,10 +299,12 @@ mymain(void) ret = -1; \ } while (0)
-#define DO_TEST_RANGE(saddr, eaddr, size, pass) \ +#define DO_TEST_RANGE(saddr, eaddr, netaddr, prefix, size, pass) \ do { \ - struct testRangeData data = { saddr, eaddr, size, pass }; \ - if (virtTestRun("Test range " saddr " -> " eaddr " size " #size, \ + struct testRangeData data \ + = { saddr, eaddr, netaddr, prefix, size, pass }; \ + if (virtTestRun("Test range " saddr " -> " eaddr "(" netaddr \ + "/" #prefix") size " #size, \ testRangeHelper, &data) < 0) \ ret = -1; \ } while (0) @@ -357,17 +371,22 @@ mymain(void) DO_TEST_PARSE_AND_FORMAT("::1", AF_UNIX, false); DO_TEST_PARSE_AND_FORMAT("::ffff", AF_UNSPEC, true);
- DO_TEST_RANGE("192.168.122.1", "192.168.122.1", 1, true); - DO_TEST_RANGE("192.168.122.1", "192.168.122.20", 20, true); - DO_TEST_RANGE("192.168.122.0", "192.168.122.255", 256, true); - DO_TEST_RANGE("192.168.122.20", "192.168.122.1", -1, false); - DO_TEST_RANGE("10.0.0.1", "192.168.122.20", -1, false); - DO_TEST_RANGE("192.168.122.20", "10.0.0.1", -1, false); - - DO_TEST_RANGE("2000::1", "2000::1", 1, true); - DO_TEST_RANGE("2000::1", "2000::2", 2, true); - DO_TEST_RANGE("2000::2", "2000::1", -1, false); - DO_TEST_RANGE("2000::1", "9001::1", -1, false); + DO_TEST_RANGE("192.168.122.1", "192.168.122.1", "192.168.122.1", 24, 1, true); + DO_TEST_RANGE("192.168.122.1", "192.168.122.20", "192.168.122.22", 24, 20, true); + DO_TEST_RANGE("192.168.122.0", "192.168.122.254", "192.168.122.1", 24, -1, false); + DO_TEST_RANGE("192.168.122.1", "192.168.122.255", "192.168.122.1", 24, -1, false); + DO_TEST_RANGE("192.168.122.0", "192.168.122.255", "192.168.122.1", 16, 256, true); + DO_TEST_RANGE("192.168.122.20", "192.168.122.1", "192.168.122.1", 24, -1, false); + DO_TEST_RANGE("10.0.0.1", "192.168.122.20", "192.168.122.1", 24, -1, false); + DO_TEST_RANGE("192.168.122.20", "10.0.0.1", "192.168.122.1", 24, -1, false); + DO_TEST_RANGE("172.16.0.50", "172.16.0.254", "1.2.3.4", 8, -1, false); + DO_TEST_RANGE("192.168.122.1", "192.168.123.20", "192.168.122.22", 24, -1, false); + DO_TEST_RANGE("192.168.122.1", "192.168.123.20", "192.168.122.22", 23, 276, true); + + DO_TEST_RANGE("2000::1", "2000::1", "2000::1", 64, 1, true); + DO_TEST_RANGE("2000::1", "2000::2", "2000::1", 64, 2, true); + DO_TEST_RANGE("2000::2", "2000::1", "2000::1", 64, -1, false); + DO_TEST_RANGE("2000::1", "9001::1", "2000::1", 64, -1, false);
DO_TEST_NETMASK("192.168.122.1", "192.168.122.2", "255.255.255.0", true);

On 05/29/2015 12:30 PM, John Ferlan wrote:
On 05/26/2015 03:48 PM, Laine Stump wrote:
virSocketAddrGetRange() has been updated to take the network address and prefix, and now checks that both the start and end of the range are within that network, thus validating that the entire range of addresses is in the network. For IPv4, it also checks that ranges to not start with the "network address" of the subnet, nor end with the broadcast address of the subnet (this check doesn't apply to IPv6, since IPv6 doesn't have a broadcast or network address)
Negative tests have been added to the network update and socket tests to verify that bad ranges properly generate an error.
This resolves: https://bugzilla.redhat.com/show_bug.cgi?id=985653 --- src/conf/network_conf.c | 14 ++--- src/network/bridge_driver.c | 4 +- src/util/virsocketaddr.c | 61 ++++++++++++++++++---- src/util/virsocketaddr.h | 6 ++- tests/networkxml2xmlupdatein/dhcp-range-10.xml | 1 + tests/networkxml2xmlupdatein/dhcp-range.xml | 2 +- .../dhcp6host-routed-network-another-range.xml | 2 +- .../dhcp6host-routed-network-range.xml | 2 +- tests/networkxml2xmlupdatetest.c | 5 ++ tests/sockettest.c | 55 ++++++++++++------- 10 files changed, 112 insertions(+), 40 deletions(-) create mode 100644 tests/networkxml2xmlupdatein/dhcp-range-10.xml
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index bc63a3d..f9f3d3d 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -832,8 +832,9 @@ int virNetworkIpDefNetmask(const virNetworkIpDef *def,
static int virSocketAddrRangeParseXML(const char *networkName, - xmlNodePtr node, - virSocketAddrRangePtr range) + virNetworkIpDefPtr ipdef, + xmlNodePtr node, + virSocketAddrRangePtr range) {
@@ -859,7 +860,8 @@ virSocketAddrRangeParseXML(const char *networkName, goto cleanup;
/* do a sanity check of the range */ - if (virSocketAddrGetRange(&range->start, &range->end) < 0) { + if (virSocketAddrGetRange(&range->start, &range->end, &ipdef->address, + virNetworkIpDefPrefix(ipdef)) < 0) { virReportError(VIR_ERR_XML_ERROR, _("Invalid dhcp range '%s' to '%s' in network '%s'"), start, end, networkName); @@ -1009,8 +1011,8 @@ virNetworkDHCPDefParseXML(const char *networkName,
if (VIR_REALLOC_N(def->ranges, def->nranges + 1) < 0) return -1; - if (virSocketAddrRangeParseXML(networkName, cur, - &def->ranges[def->nranges]) < 0) { + if (virSocketAddrRangeParseXML(networkName, def, cur, + &def->ranges[def->nranges]) < 0) { return -1; } def->nranges++; @@ -3608,7 +3610,7 @@ virNetworkDefUpdateIPDHCPRange(virNetworkDefPtr def, goto cleanup; }
- if (virSocketAddrRangeParseXML(def->name, ctxt->node, &range) < 0) + if (virSocketAddrRangeParseXML(def->name, ipdef, ctxt->node, &range) < 0) goto cleanup;
/* check if an entry with same name/address/ip already exists */ diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 4b53475..e08a316 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -1193,7 +1193,9 @@ networkDnsmasqConfContents(virNetworkObjPtr network, VIR_FREE(saddr); VIR_FREE(eaddr); nbleases += virSocketAddrGetRange(&ipdef->ranges[r].start, - &ipdef->ranges[r].end); + &ipdef->ranges[r].end, + &ipdef->address, + virNetworkIpDefPrefix(ipdef)); }
/* diff --git a/src/util/virsocketaddr.c b/src/util/virsocketaddr.c index 67ed330..0edf345 100644 --- a/src/util/virsocketaddr.c +++ b/src/util/virsocketaddr.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2009-2014 Red Hat, Inc. + * Copyright (C) 2009-2015 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -605,31 +605,69 @@ int virSocketAddrCheckNetmask(virSocketAddrPtr addr1, virSocketAddrPtr addr2, * virSocketGetRange: * @start: start of an IP range * @end: end of an IP range + * @network: IP address of network that should completely contain this range + * @prefix: prefix of the network * - * Check the order of the 2 addresses and compute the range, this - * will return 1 for identical addresses. Errors can come from incompatible - * addresses type, excessive range (>= 2^^16) where the two addresses are - * unrelated or inverted start and end. + * Check the order of the 2 addresses and compute the range, this will + * return 1 for identical addresses. Errors can come from incompatible + * addresses type, excessive range (>= 2^^16) where the two addresses + * are unrelated, inverted start and end, or a range that is not + * within network/prefix. * * Returns the size of the range or -1 in case of failure */ -int virSocketAddrGetRange(virSocketAddrPtr start, virSocketAddrPtr end) +int +virSocketAddrGetRange(virSocketAddrPtr start, virSocketAddrPtr end, + virSocketAddrPtr network, int prefix) { int ret = 0; size_t i; + virSocketAddr netmask; + + if (start == NULL || end == NULL || network == NULL) + return -1; + + if (VIR_SOCKET_ADDR_FAMILY(start) != VIR_SOCKET_ADDR_FAMILY(end) || + VIR_SOCKET_ADDR_FAMILY(start) != VIR_SOCKET_ADDR_FAMILY(network)) + return -1;
- if ((start == NULL) || (end == NULL)) + if (prefix < 0 || + virSocketAddrPrefixToNetmask(prefix, &netmask, VIR_SOCKET_ADDR_FAMILY(network)) < 0) return -1; - if (start->data.stor.ss_family != end->data.stor.ss_family) + + /* both start and end of range need to be in the same network as + * "network" + */ + if (virSocketAddrCheckNetmask(start, network, &netmask) <= 0 || + virSocketAddrCheckNetmask(start, network, &netmask) <= 0) Should I assume this is a cut-n-paste (eg, the second call should use end not start)...
Yep.
Side note - seems there's no "FAIL" test for this condition - considering we passed the tests with the check this way...
Yeah, there is a test where the end address is outside the subnet:
+ DO_TEST_RANGE("192.168.122.20", "10.0.0.1", "192.168.122.1", 24, -1, false);
But that one fails anyway, because the end address is lower than the start address. Thankfully I was waiting until after release to push . I'll change the offending "start" to "end", and add this test: DO_TEST_RANGE("192.168.122.20", "192.168.123.1", "192.168.122.1", 24, -1, false); Thanks for your eagle eye!

This loop had automatic variable definitions mixed with code. This patch moves the definitions to the top of the function and puts cleanup for them at the bottom. No functional change. --- src/network/bridge_driver.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index e08a316..68c35e9 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -926,6 +926,7 @@ networkDnsmasqConfContents(virNetworkObjPtr network, virNetworkDNSDefPtr dns = &network->def->dns; virNetworkIpDefPtr tmpipdef, ipdef, ipv4def, ipv6def; bool ipv6SLAAC; + char *saddr = NULL, *eaddr = NULL; *configstr = NULL; @@ -1180,14 +1181,10 @@ networkDnsmasqConfContents(virNetworkObjPtr network, while (ipdef) { for (r = 0; r < ipdef->nranges; r++) { - char *saddr = virSocketAddrFormat(&ipdef->ranges[r].start); - if (!saddr) + if (!(saddr = virSocketAddrFormat(&ipdef->ranges[r].start)) || + !(eaddr = virSocketAddrFormat(&ipdef->ranges[r].end))) goto cleanup; - char *eaddr = virSocketAddrFormat(&ipdef->ranges[r].end); - if (!eaddr) { - VIR_FREE(saddr); - goto cleanup; - } + virBufferAsprintf(&configbuf, "dhcp-range=%s,%s\n", saddr, eaddr); VIR_FREE(saddr); @@ -1289,6 +1286,8 @@ networkDnsmasqConfContents(virNetworkObjPtr network, ret = 0; cleanup: + VIR_FREE(saddr); + VIR_FREE(eaddr); virBufferFreeAndReset(&configbuf); return ret; } -- 2.1.0

There are now many more reasons that virSocketAddrGetRange() could fail, so it is much more informative to report the error there instead of in the caller. (one of the two callers was previously assuming success, which is almost surely safe based on the parsing that has already happened to the config by that time, but it still is nicer to account for an error "just in case") --- src/conf/network_conf.c | 6 +- src/network/bridge_driver.c | 7 ++- src/util/virsocketaddr.c | 141 ++++++++++++++++++++++++++++++++++++-------- 3 files changed, 122 insertions(+), 32 deletions(-) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index f9f3d3d..31d4463 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -861,12 +861,8 @@ virSocketAddrRangeParseXML(const char *networkName, /* do a sanity check of the range */ if (virSocketAddrGetRange(&range->start, &range->end, &ipdef->address, - virNetworkIpDefPrefix(ipdef)) < 0) { - virReportError(VIR_ERR_XML_ERROR, - _("Invalid dhcp range '%s' to '%s' in network '%s'"), - start, end, networkName); + virNetworkIpDefPrefix(ipdef)) < 0) goto cleanup; - } ret = 0; diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 68c35e9..a75d98d 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -1181,6 +1181,8 @@ networkDnsmasqConfContents(virNetworkObjPtr network, while (ipdef) { for (r = 0; r < ipdef->nranges; r++) { + int thisRange; + if (!(saddr = virSocketAddrFormat(&ipdef->ranges[r].start)) || !(eaddr = virSocketAddrFormat(&ipdef->ranges[r].end))) goto cleanup; @@ -1189,10 +1191,13 @@ networkDnsmasqConfContents(virNetworkObjPtr network, saddr, eaddr); VIR_FREE(saddr); VIR_FREE(eaddr); - nbleases += virSocketAddrGetRange(&ipdef->ranges[r].start, + thisRange = virSocketAddrGetRange(&ipdef->ranges[r].start, &ipdef->ranges[r].end, &ipdef->address, virNetworkIpDefPrefix(ipdef)); + if (thisRange < 0) + goto cleanup; + nbleases += thisRange; } /* diff --git a/src/util/virsocketaddr.c b/src/util/virsocketaddr.c index 0edf345..35d0d80 100644 --- a/src/util/virsocketaddr.c +++ b/src/util/virsocketaddr.c @@ -26,6 +26,7 @@ #include "virsocketaddr.h" #include "virerror.h" #include "virstring.h" +#include "viralloc.h" #include <netdb.h> @@ -623,32 +624,63 @@ virSocketAddrGetRange(virSocketAddrPtr start, virSocketAddrPtr end, int ret = 0; size_t i; virSocketAddr netmask; + char *startStr = NULL, *endStr = NULL, *netStr = NULL; - if (start == NULL || end == NULL || network == NULL) - return -1; + if (start == NULL || end == NULL || network == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("NULL argument - %p %p %p"), start, end, network); + goto error; + } + + startStr = virSocketAddrFormat(start); + endStr = virSocketAddrFormat(end); + netStr = virSocketAddrFormat(network); + if (!(startStr && endStr && netStr)) + goto error; /*error already reported */ if (VIR_SOCKET_ADDR_FAMILY(start) != VIR_SOCKET_ADDR_FAMILY(end) || - VIR_SOCKET_ADDR_FAMILY(start) != VIR_SOCKET_ADDR_FAMILY(network)) - return -1; + VIR_SOCKET_ADDR_FAMILY(start) != VIR_SOCKET_ADDR_FAMILY(network)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("mismatch of address family in " + "range %s - %s for network %s"), + startStr, endStr, netStr); + goto error; + } if (prefix < 0 || - virSocketAddrPrefixToNetmask(prefix, &netmask, VIR_SOCKET_ADDR_FAMILY(network)) < 0) - return -1; + virSocketAddrPrefixToNetmask(prefix, &netmask, + VIR_SOCKET_ADDR_FAMILY(network)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("bad prefix %d for network %s when " + " checking range %s - %s"), + prefix, netStr, startStr, endStr); + goto error; + } /* both start and end of range need to be in the same network as * "network" */ if (virSocketAddrCheckNetmask(start, network, &netmask) <= 0 || - virSocketAddrCheckNetmask(start, network, &netmask) <= 0) - return -1; + virSocketAddrCheckNetmask(start, network, &netmask) <= 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("range %s - %s is not entirely within " + "network %s/%d"), + startStr, endStr, netStr, prefix); + goto error; + } if (VIR_SOCKET_ADDR_IS_FAMILY(start, AF_INET)) { virSocketAddrIPv4 t1, t2; virSocketAddr netaddr, broadcast; if (virSocketAddrBroadcast(network, &netmask, &broadcast) < 0 || - virSocketAddrMask(network, &netmask, &netaddr) < 0) - return -1; + virSocketAddrMask(network, &netmask, &netaddr) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("failed to construct broadcast or network " + "address for network %s/%d"), + netStr, prefix); + goto error; + } /* Don't allow the start of the range to be the network * address (usually "...0") or the end of the range to be the @@ -657,47 +689,104 @@ virSocketAddrGetRange(virSocketAddrPtr start, virSocketAddrPtr end, * other combined checks) (IPv6 doesn't have broadcast and * network addresses, so this check is only done for IPv4) */ - if (virSocketAddrEqual(start, &netaddr) || - virSocketAddrEqual(end, &broadcast)) - return -1; + if (virSocketAddrEqual(start, &netaddr)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("start of range %s - %s in network %s/%d " + "is the network address"), + startStr, endStr, netStr, prefix); + goto error; + } + + if (virSocketAddrEqual(end, &broadcast)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("end of range %s - %s in network %s/%d " + "is the broadcast address"), + startStr, endStr, netStr, prefix); + goto error; + } if ((virSocketAddrGetIPv4Addr(start, &t1) < 0) || - (virSocketAddrGetIPv4Addr(end, &t2) < 0)) - return -1; + (virSocketAddrGetIPv4Addr(end, &t2) < 0)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("failed to get IPv4 address " + "for start or end of range %s - %s " + "inf for network %s/%d"), + startStr, endStr, netStr, prefix); + goto error; + } /* legacy check that everything except the last two bytes are * the same */ for (i = 0; i < 2; i++) { - if (t1[i] != t2[i]) - return -1; + if (t1[i] != t2[i]) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("range %s - %s is too large (> 65535) " + "in network %s/%d"), + startStr, endStr, netStr, prefix); + goto error; + } } ret = (t2[2] - t1[2]) * 256 + (t2[3] - t1[3]); - if (ret < 0) - return -1; + if (ret < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("range %s - %s in areversed " + "in network %s/%d"), + startStr, endStr, netStr, prefix); + goto error; + } ret++; } else if (VIR_SOCKET_ADDR_IS_FAMILY(start, AF_INET6)) { virSocketAddrIPv6 t1, t2; if ((virSocketAddrGetIPv6Addr(start, &t1) < 0) || - (virSocketAddrGetIPv6Addr(end, &t2) < 0)) - return -1; + (virSocketAddrGetIPv6Addr(end, &t2) < 0)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("failed to get IPv6 address " + "for start or end of range %s - %s " + "inf for network %s/%d"), + startStr, endStr, netStr, prefix); + goto error; + } /* legacy check that everything except the last two bytes are * the same */ for (i = 0; i < 7; i++) { - if (t1[i] != t2[i]) - return -1; + if (t1[i] != t2[i]) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("range %s - %s is too large (> 65535) " + "in network %s/%d"), + startStr, endStr, netStr, prefix); + goto error; + } } ret = t2[7] - t1[7]; - if (ret < 0) - return -1; + if (ret < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("range %s - %s start larger than end " + "in network %s/%d"), + startStr, endStr, netStr, prefix); + goto error; + } ret++; } else { - return -1; + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unsupported address family " + "for range %s - %s " + "in network %s/%d, must be ipv4 or ipv6"), + startStr, endStr, netStr, prefix); + goto error; } + cleanup: + VIR_FREE(startStr); + VIR_FREE(endStr); + VIR_FREE(netStr); return ret; + + error: + ret = -1; + goto cleanup; } -- 2.1.0

On 26.05.2015 21:48, Laine Stump wrote:
This resolves: https://bugzilla.redhat.com/show_bug.cgi?id=985653
Laine Stump (3): network: validate DHCP ranges are completely within defined network network: cleanup range loop in networkDnsmasqConfContents util: report all address range errors in virSocketAddrGetRange()
src/conf/network_conf.c | 18 +-- src/network/bridge_driver.c | 24 +-- src/util/virsocketaddr.c | 180 ++++++++++++++++++--- src/util/virsocketaddr.h | 6 +- tests/networkxml2xmlupdatein/dhcp-range-10.xml | 1 + tests/networkxml2xmlupdatein/dhcp-range.xml | 2 +- .../dhcp6host-routed-network-another-range.xml | 2 +- .../dhcp6host-routed-network-range.xml | 2 +- tests/networkxml2xmlupdatetest.c | 5 + tests/sockettest.c | 55 ++++--- 10 files changed, 228 insertions(+), 67 deletions(-) create mode 100644 tests/networkxml2xmlupdatein/dhcp-range-10.xml
ACK and safe for the freeze. Michal
participants (3)
-
John Ferlan
-
Laine Stump
-
Michal Privoznik