[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/29/2015 12:30 PM, John Ferlan wrote:
>
> 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)...

Yep.

>
> Side note - seems there's no "FAIL" test for this condition -
> considering we passed the tests with the check this way...

Yeah, there is a test where the end address is outside the subnet:

> +    DO_TEST_RANGE("192.168.122.20", "10.0.0.1", "192.168.122.1", 24, -1, false);


But that one fails anyway, because the end address is lower than the
start address.

Thankfully I was waiting until after release to push . I'll change the
offending "start" to "end", and add this test:

   DO_TEST_RANGE("192.168.122.20", "192.168.123.1", "192.168.122.1", 24,
-1, false);

Thanks for your eagle eye!


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