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!