[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 1/3] network: validate DHCP ranges are completely within defined network




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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]