network: validate DHCP ranges are completely within defined network

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

diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index bc63a3ddc299c7230190e2bf2ea0f7cc237a167f..f9f3d3d17e33ba7419ffd9871c71b271503dbc4c 100644 (file)
--- 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 f438c0b49b309e0517bceb051feabb5bc712d2cd..5caa6b944f872c48b994909a8347770f64788ff5 100644 (file)
--- 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 67ed330da8bf532c86ab4ffdc1145c3d434eab56..bab8608cf87d19571e84da3d075a0855863f3c92 100644 (file)
--- 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(end, 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 99ab46f03832ab8379fc88ba099a7ada973904e1..9e2680a4d2f6f1eb9000e238ad8ef74fe1db6b8f 100644 (file)
--- 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 (file)
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 ed775c8a42ce813c1b5a83d839538560d1335cdc..d5e283c45b0be835665deab90cc9b8e7c971d262 100644 (file)
--- 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 ee6eb7a27cdd01b5798e310614690632b97389b7..4a1360d76997b1a962cc83325fe43b75cd4d3e21 100644 (file)
--- 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 ee6eb7a27cdd01b5798e310614690632b97389b7..4a1360d76997b1a962cc83325fe43b75cd4d3e21 100644 (file)
--- 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 af697bb56858bfe17d9ef42f79c6fd1b23aa7a80..024137858eace5e0facf36483fc0068d2eafe223 100644 (file)
--- 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 086c0eacaf624194bab4d806c15354d2ae9fd6c0..292edb610071a372a9c8264dff25c273395e74ec 100644 (file)
--- 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 (pass) {
         /* fail if virSocketAddrGetRange returns failure, or unexpected size */
@@ -107,16 +112,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);
 }
 
 
@@ -289,10 +301,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)
@@ -359,17 +373,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);