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! -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list