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