From b65e25e3132c9b5b0942b138231440bc8d4dd873 Mon Sep 17 00:00:00 2001

Message-Id: <b65e25e3132c9b5b0942b138231440bc8d4dd873@dist-git>

From: Laine Stump <laine@laine.org>

Date: Mon, 10 Aug 2015 13:43:38 -0400

Subject: [PATCH] network: validate network NAT range

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.

(cherry picked from commit a6f9af8292b6462e509892b3a16acbcaaef61e4e)

Signed-off-by: Jiri Denemark <jdenemar@redhat.com>

---

 src/conf/network_conf.c  |   4 ++

 src/util/virsocketaddr.c | 184 ++++++++++++++++++++++++-----------------------

 tests/sockettest.c       |  46 +++++++++++-

 3 files changed, 144 insertions(+), 90 deletions(-)

diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c

index 0ebb373..b03c2fd 100644

--- a/src/conf/network_conf.c

+++ b/src/conf/network_conf.c

@@ -1729,6 +1729,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;

-    }

+    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;

-    /* 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_FAMILY(start) != VIR_SOCKET_ADDR_FAMILY(network)) {

+            virReportError(VIR_ERR_INTERNAL_ERROR,

+                           _("mismatch of address family in "

+                             "range %s - %s for network %s"),

+                           startStr, endStr, netStr);

+            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 within 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)) {

+            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;

-        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 ((virSocketAddrGetIPv4Addr(start, &t1) < 0) ||

-            (virSocketAddrGetIPv4Addr(end, &t2) < 0)) {

+        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.5.0