This patch modifies virSocketAddrGetRange() to function properly when the containing network/prefix of the address range isn't known, for example in the case of the NAT range of a virtual network (since it is a range of addresses on the *host*, not within the network itself). We then take advantage of this new functionality to validate the NAT range of a virtual network. Extra test cases are also added to verify that virSocketAddrGetRange() works properly in both positive and negative cases when the network pointer is NULL. This is the *real* fix for: https://bugzilla.redhat.com/show_bug.cgi?id=985653 Commits 1e334a and 48e8b9 had earlier been pushed as fixes for that bug, but I had neglected to read the report carefully, so instead of fixing validation for the NAT range, I had fixed validation for the DHCP range. sigh. --- The changes to virSocketAddrGetRange() *look* like they are extensive, but really they almost completely consist of: 1) reordering and reindenting some of the checks so that they are only executed when we have a valid network address 2) modifying the error messages that could occur when there isn't a valid network so that they don't attempt to use the network address or prefix. src/conf/network_conf.c | 4 ++ src/util/virsocketaddr.c | 168 +++++++++++++++++++++++++---------------------- tests/sockettest.c | 46 ++++++++++++- 3 files changed, 136 insertions(+), 82 deletions(-) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index f77af15..b8d918c 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -1731,6 +1731,10 @@ virNetworkForwardNatDefParseXML(const char *networkName, goto cleanup; } + /* verify that start <= end */ + if (virSocketAddrGetRange(&def->addr.start, &def->addr.end, NULL, 0) < 0) + goto cleanup; + /* ports for SNAT and MASQUERADE */ nNatPorts = virXPathNodeSet("./port", ctxt, &natPortNodes); if (nNatPorts < 0) { diff --git a/src/util/virsocketaddr.c b/src/util/virsocketaddr.c index 81539b3..900aa5b 100644 --- a/src/util/virsocketaddr.c +++ b/src/util/virsocketaddr.c @@ -628,126 +628,136 @@ virSocketAddrGetRange(virSocketAddrPtr start, virSocketAddrPtr end, virSocketAddr netmask; char *startStr = NULL, *endStr = NULL, *netStr = NULL; - if (start == NULL || end == NULL || network == NULL) { + if (start == NULL || end == NULL) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("NULL argument - %p %p %p"), start, end, network); + _("NULL argument - %p %p"), start, end); goto error; } startStr = virSocketAddrFormat(start); endStr = virSocketAddrFormat(end); - netStr = virSocketAddrFormat(network); - if (!(startStr && endStr && netStr)) + if (!startStr || !endStr) 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)) { + if (VIR_SOCKET_ADDR_FAMILY(start) != VIR_SOCKET_ADDR_FAMILY(end)) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("mismatch of address family in " - "range %s - %s for network %s"), - startStr, endStr, netStr); + _("mismatch of address family in range %s - %s"), + startStr, endStr); goto error; } - if (prefix < 0 || - 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(end, 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 (network) { + /* some checks can only be done if we have details of the + * network the range should be within + */ + if (!(netStr = virSocketAddrFormat(network))) + goto error; - if (virSocketAddrBroadcast(network, &netmask, &broadcast) < 0 || - virSocketAddrMask(network, &netmask, &netaddr) < 0) { + if (VIR_SOCKET_ADDR_FAMILY(start) != VIR_SOCKET_ADDR_FAMILY(network)) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("failed to construct broadcast or network " - "address for network %s/%d"), - netStr, prefix); + _("mismatch of address family in " + "range %s - %s for network %s"), + startStr, endStr, netStr); 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 - * 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)) { + if (prefix < 0 || + virSocketAddrPrefixToNetmask(prefix, &netmask, + VIR_SOCKET_ADDR_FAMILY(network)) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("start of range %s - %s in network %s/%d " - "is the network address"), - startStr, endStr, netStr, prefix); + _("bad prefix %d for network %s when " + " checking range %s - %s"), + prefix, netStr, startStr, endStr); goto error; } - if (virSocketAddrEqual(end, &broadcast)) { + /* both start and end of range need to be within network */ + if (virSocketAddrCheckNetmask(start, network, &netmask) <= 0 || + virSocketAddrCheckNetmask(end, network, &netmask) <= 0) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("end of range %s - %s in network %s/%d " - "is the broadcast address"), + _("range %s - %s is not entirely within " + "network %s/%d"), startStr, endStr, netStr, prefix); goto error; } - if ((virSocketAddrGetIPv4Addr(start, &t1) < 0) || - (virSocketAddrGetIPv4Addr(end, &t2) < 0)) { + if (VIR_SOCKET_ADDR_IS_FAMILY(start, AF_INET)) { + virSocketAddr netaddr, broadcast; + + if (virSocketAddrBroadcast(network, &netmask, &broadcast) < 0 || + 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 + * 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)) { + 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 (VIR_SOCKET_ADDR_IS_FAMILY(start, AF_INET)) { + virSocketAddrIPv4 t1, t2; + + if (virSocketAddrGetIPv4Addr(start, &t1) < 0 || + virSocketAddrGetIPv4Addr(end, &t2) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("failed to get IPv4 address " - "for start or end of range %s - %s " - "in network %s/%d"), - startStr, endStr, netStr, prefix); + "for start or end of range %s - %s"), + startStr, endStr); goto error; } - /* legacy check that everything except the last two bytes are - * the same + /* legacy check that everything except the last two bytes + * are the same */ for (i = 0; i < 2; i++) { 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); + _("range %s - %s is too large (> 65535)"), + startStr, endStr); goto error; } } ret = (t2[2] - t1[2]) * 256 + (t2[3] - t1[3]); if (ret < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("range %s - %s is reversed " - "in network %s/%d"), - startStr, endStr, netStr, prefix); + _("range %s - %s is reversed "), + startStr, endStr); 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)) { + if (virSocketAddrGetIPv6Addr(start, &t1) < 0 || + virSocketAddrGetIPv6Addr(end, &t2) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("failed to get IPv6 address " - "for start or end of range %s - %s " - "in network %s/%d"), - startStr, endStr, netStr, prefix); + "for start or end of range %s - %s"), + startStr, endStr); goto error; } @@ -757,29 +767,27 @@ virSocketAddrGetRange(virSocketAddrPtr start, virSocketAddrPtr end, for (i = 0; i < 7; i++) { 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); + _("range %s - %s is too large (> 65535)"), + startStr, endStr); goto error; } } ret = t2[7] - t1[7]; if (ret < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("range %s - %s start larger than end " - "in network %s/%d"), - startStr, endStr, netStr, prefix); + _("range %s - %s start larger than end"), + startStr, endStr); goto error; } ret++; } else { 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); + "for range %s - %s, must be ipv4 or ipv6"), + startStr, endStr); goto error; } + cleanup: VIR_FREE(startStr); VIR_FREE(endStr); diff --git a/tests/sockettest.c b/tests/sockettest.c index 292edb6..8f46218 100644 --- a/tests/sockettest.c +++ b/tests/sockettest.c @@ -98,10 +98,11 @@ testRange(const char *saddrstr, const char *eaddrstr, return -1; if (virSocketAddrParse(&eaddr, eaddrstr, AF_UNSPEC) < 0) return -1; - if (virSocketAddrParse(&netaddr, netstr, AF_UNSPEC) < 0) + if (netstr && virSocketAddrParse(&netaddr, netstr, AF_UNSPEC) < 0) return -1; - int gotsize = virSocketAddrGetRange(&saddr, &eaddr, &netaddr, prefix); + int gotsize = virSocketAddrGetRange(&saddr, &eaddr, + netstr ? &netaddr : NULL, prefix); VIR_DEBUG("Size want %d vs got %d", size, gotsize); if (pass) { /* fail if virSocketAddrGetRange returns failure, or unexpected size */ @@ -311,6 +312,15 @@ mymain(void) ret = -1; \ } while (0) +#define DO_TEST_RANGE_SIMPLE(saddr, eaddr, size, pass) \ + do { \ + struct testRangeData data \ + = { saddr, eaddr, NULL, 0, size, pass }; \ + if (virtTestRun("Test range " saddr " -> " eaddr "size " #size, \ + testRangeHelper, &data) < 0) \ + ret = -1; \ + } while (0) + #define DO_TEST_NETMASK(addr1, addr2, netmask, pass) \ do { \ struct testNetmaskData data = { addr1, addr2, netmask, pass }; \ @@ -373,23 +383,55 @@ mymain(void) DO_TEST_PARSE_AND_FORMAT("::1", AF_UNIX, false); DO_TEST_PARSE_AND_FORMAT("::ffff", AF_UNSPEC, true); + /* tests that specify a network that should contain the range */ 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); + /* start of range is "network address" */ DO_TEST_RANGE("192.168.122.0", "192.168.122.254", "192.168.122.1", 24, -1, false); + /* end of range is "broadcast address" */ 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); + /* range is reversed */ DO_TEST_RANGE("192.168.122.20", "192.168.122.1", "192.168.122.1", 24, -1, false); + /* start address outside network */ DO_TEST_RANGE("10.0.0.1", "192.168.122.20", "192.168.122.1", 24, -1, false); + /* end address outside network and range reversed */ DO_TEST_RANGE("192.168.122.20", "10.0.0.1", "192.168.122.1", 24, -1, false); + /* entire range outside network */ DO_TEST_RANGE("172.16.0.50", "172.16.0.254", "1.2.3.4", 8, -1, false); + /* end address outside network */ 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); + /* range reversed */ DO_TEST_RANGE("2000::2", "2000::1", "2000::1", 64, -1, false); + /* range too large (> 65536) */ DO_TEST_RANGE("2000::1", "9001::1", "2000::1", 64, -1, false); + /* tests that *don't* specify a containing network + * (so fewer things can be checked) + */ + DO_TEST_RANGE_SIMPLE("192.168.122.1", "192.168.122.1", 1, true); + DO_TEST_RANGE_SIMPLE("192.168.122.1", "192.168.122.20", 20, true); + DO_TEST_RANGE_SIMPLE("192.168.122.0", "192.168.122.255", 256, true); + /* range is reversed */ + DO_TEST_RANGE_SIMPLE("192.168.122.20", "192.168.122.1", -1, false); + /* range too large (> 65536) */ + DO_TEST_RANGE_SIMPLE("10.0.0.1", "192.168.122.20", -1, false); + /* range reversed */ + DO_TEST_RANGE_SIMPLE("192.168.122.20", "10.0.0.1", -1, false); + DO_TEST_RANGE_SIMPLE("172.16.0.50", "172.16.0.254", 205, true); + DO_TEST_RANGE_SIMPLE("192.168.122.1", "192.168.123.20", 276, true); + + DO_TEST_RANGE_SIMPLE("2000::1", "2000::1", 1, true); + DO_TEST_RANGE_SIMPLE("2000::1", "2000::2", 2, true); + /* range reversed */ + DO_TEST_RANGE_SIMPLE("2000::2", "2000::1", -1, false); + /* range too large (> 65536) */ + DO_TEST_RANGE_SIMPLE("2000::1", "9001::1", -1, false); + DO_TEST_NETMASK("192.168.122.1", "192.168.122.2", "255.255.255.0", true); DO_TEST_NETMASK("192.168.122.1", "192.168.122.4", -- 2.1.0