]> git.ipfire.org Git - people/ms/strongswan.git/blobdiff - src/libstrongswan/selectors/traffic_selector.c
traffic-selector: Avoid out-of-bound array access when calculating range
[people/ms/strongswan.git] / src / libstrongswan / selectors / traffic_selector.c
index 66eeefff591b55a19857f493d61926609edf7fde..2735a5cc16c3d4fae8d99ea014d3097662f759cc 100644 (file)
@@ -1,8 +1,8 @@
 /*
- * Copyright (C) 2007-2009 Tobias Brunner
+ * Copyright (C) 2007-2017 Tobias Brunner
  * Copyright (C) 2005-2007 Martin Willi
  * Copyright (C) 2005 Jan Hutter
- * Hochschule fuer Technik Rapperswil
+ * HSR Hochschule fuer Technik Rapperswil
  *
  * This program is free software; you can redistribute it and/or modify it
  * under the terms of the GNU General Public License as published by the
  * for more details.
  */
 
-#include <arpa/inet.h>
 #include <string.h>
-#include <netdb.h>
 #include <stdio.h>
 
 #include "traffic_selector.h"
 
-#include <utils/linked_list.h>
+#include <utils/debug.h>
+#include <utils/utils.h>
 #include <utils/identification.h>
-#include <debug.h>
+#include <collections/linked_list.h>
+
+#define IPV4_LEN       4
+#define IPV6_LEN       16
+#define TS_IP_LEN(this) ({ ((this)->type == TS_IPV4_ADDR_RANGE) ? IPV4_LEN : IPV6_LEN; })
 
 #define NON_SUBNET_ADDRESS_RANGE       255
 
@@ -53,7 +56,7 @@ struct private_traffic_selector_t {
        /**
         * IP protocol (UDP, TCP, ICMP, ...)
         */
-       u_int8_t protocol;
+       uint8_t protocol;
 
        /**
         * narrow this traffic selector to hosts external ip
@@ -64,55 +67,41 @@ struct private_traffic_selector_t {
        /**
         * subnet size in CIDR notation, 255 means a non-subnet address range
         */
-       u_int8_t netbits;
+       uint8_t netbits;
 
        /**
         * begin of address range, network order
         */
-       union {
-               /** dummy char for common address manipulation */
-               char from[0];
-               /** IPv4 address */
-               u_int32_t from4[1];
-               /** IPv6 address */
-               u_int32_t from6[4];
-       };
+       char from[IPV6_LEN];
 
        /**
         * end of address range, network order
         */
-       union {
-               /** dummy char for common address manipulation */
-               char to[0];
-               /** IPv4 address */
-               u_int32_t to4[1];
-               /** IPv6 address */
-               u_int32_t to6[4];
-       };
+       char to[IPV6_LEN];
 
        /**
         * begin of port range
         */
-       u_int16_t from_port;
+       uint16_t from_port;
 
        /**
         * end of port range
         */
-       u_int16_t to_port;
+       uint16_t to_port;
 };
 
 /**
  * calculate the "to"-address for the "from" address and a subnet size
  */
-static void calc_range(private_traffic_selector_t *this, u_int8_t netbits)
+static void calc_range(private_traffic_selector_t *this, uint8_t netbits)
 {
        size_t len;
        int bytes, bits;
-       u_int8_t mask;
+       uint8_t mask;
 
        this->netbits = netbits;
 
-       len   = (this->type == TS_IPV4_ADDR_RANGE) ? 4 : 16;
+       len   = TS_IP_LEN(this);
        bytes = (netbits + 7)/8;
        bits  = (bytes * 8) - netbits;
        mask  = bits ? (1 << bits) - 1 : 0;
@@ -120,18 +109,22 @@ static void calc_range(private_traffic_selector_t *this, u_int8_t netbits)
        memcpy(this->to, this->from, bytes);
        memset(this->from + bytes, 0x00, len - bytes);
        memset(this->to   + bytes, 0xff, len - bytes);
-       this->from[bytes-1] &= ~mask;
-       this->to[bytes-1]   |=  mask;
+
+       if (bytes)
+       {
+               this->from[bytes-1] &= ~mask;
+               this->to[bytes-1]   |=  mask;
+       }
 }
 
 /**
  * calculate the subnet size from the "to" and "from" addresses
  */
-static u_int8_t calc_netbits(private_traffic_selector_t *this)
+static uint8_t calc_netbits(private_traffic_selector_t *this)
 {
        int byte, bit;
-       u_int8_t netbits;
-       size_t size = (this->type == TS_IPV4_ADDR_RANGE) ? 4 : 16;
+       uint8_t netbits;
+       size_t size = TS_IP_LEN(this);
        bool prefix = TRUE;
 
        /* a perfect match results in a single address with a /32 or /128 netmask */
@@ -145,7 +138,7 @@ static u_int8_t calc_netbits(private_traffic_selector_t *this)
        {
                for (bit = 7; bit >= 0; bit--)
                {
-                       u_int8_t bitmask = 1 << bit;
+                       uint8_t bitmask = 1 << bit;
 
                        if (prefix)
                        {
@@ -174,7 +167,40 @@ static u_int8_t calc_netbits(private_traffic_selector_t *this)
 /**
  * internal generic constructor
  */
-static private_traffic_selector_t *traffic_selector_create(u_int8_t protocol, ts_type_t type, u_int16_t from_port, u_int16_t to_port);
+static private_traffic_selector_t *traffic_selector_create(uint8_t protocol,
+                                               ts_type_t type, uint16_t from_port, uint16_t to_port);
+
+/**
+ * Check if TS contains "opaque" ports
+ */
+static bool is_opaque(private_traffic_selector_t *this)
+{
+       return this->from_port == 0xffff && this->to_port == 0;
+}
+
+/**
+ * Check if TS contains "any" ports
+ */
+static bool is_any(private_traffic_selector_t *this)
+{
+       return this->from_port == 0 && this->to_port == 0xffff;
+}
+
+/**
+ * Print ICMP/ICMPv6 type and code
+ */
+static int print_icmp(printf_hook_data_t *data, uint16_t port)
+{
+       uint8_t type, code;
+
+       type = traffic_selector_icmp_type(port);
+       code = traffic_selector_icmp_code(port);
+       if (code)
+       {
+               return print_in_hook(data, "%d(%d)", type, code);
+       }
+       return print_in_hook(data, "%d", type);
+}
 
 /**
  * Described in header.
@@ -187,11 +213,10 @@ int traffic_selector_printf_hook(printf_hook_data_t *data,
        enumerator_t *enumerator;
        char from_str[INET6_ADDRSTRLEN] = "";
        char to_str[INET6_ADDRSTRLEN] = "";
-       char *serv_proto = NULL;
-       bool has_proto;
-       bool has_ports;
-       size_t written = 0;
-       u_int32_t from[4], to[4];
+       char *serv_proto = NULL, *sep = "";
+       bool has_proto, has_ports;
+       size_t written = 0, len;
+       char from[IPV6_LEN], to[IPV6_LEN];
 
        if (this == NULL)
        {
@@ -203,18 +228,18 @@ int traffic_selector_printf_hook(printf_hook_data_t *data,
                enumerator = list->create_enumerator(list);
                while (enumerator->enumerate(enumerator, (void**)&this))
                {
-                       /* call recursivly */
-                       written += print_in_hook(data, "%R ", this);
+                       written += print_in_hook(data, "%s%R", sep, this);
+                       sep = " ";
                }
                enumerator->destroy(enumerator);
                return written;
        }
 
-       memset(from, 0, sizeof(from));
-       memset(to, 0xFF, sizeof(to));
+       len = TS_IP_LEN(this);
+       memset(from, 0, len);
+       memset(to, 0xFF, len);
        if (this->dynamic &&
-               memeq(this->from, from, this->type == TS_IPV4_ADDR_RANGE ? 4 : 16) &&
-               memeq(this->to, to, this->type == TS_IPV4_ADDR_RANGE ? 4 : 16))
+               memeq(this->from, from, len) && memeq(this->to, to, len))
        {
                written += print_in_hook(data, "dynamic");
        }
@@ -222,21 +247,21 @@ int traffic_selector_printf_hook(printf_hook_data_t *data,
        {
                if (this->type == TS_IPV4_ADDR_RANGE)
                {
-                       inet_ntop(AF_INET, &this->from4, from_str, sizeof(from_str));
+                       inet_ntop(AF_INET, &this->from, from_str, sizeof(from_str));
                }
                else
                {
-                       inet_ntop(AF_INET6, &this->from6, from_str, sizeof(from_str));
+                       inet_ntop(AF_INET6, &this->from, from_str, sizeof(from_str));
                }
                if (this->netbits == NON_SUBNET_ADDRESS_RANGE)
                {
                        if (this->type == TS_IPV4_ADDR_RANGE)
                        {
-                               inet_ntop(AF_INET, &this->to4, to_str, sizeof(to_str));
+                               inet_ntop(AF_INET, &this->to, to_str, sizeof(to_str));
                        }
                        else
                        {
-                               inet_ntop(AF_INET6, &this->to6, to_str, sizeof(to_str));
+                               inet_ntop(AF_INET6, &this->to, to_str, sizeof(to_str));
                        }
                        written += print_in_hook(data, "%s..%s", from_str, to_str);
                }
@@ -248,7 +273,7 @@ int traffic_selector_printf_hook(printf_hook_data_t *data,
 
        /* check if we have protocol and/or port selectors */
        has_proto = this->protocol != 0;
-       has_ports = !(this->from_port == 0 && this->to_port == 0xFFFF);
+       has_ports = !is_any(this);
 
        if (!has_proto && !has_ports)
        {
@@ -272,31 +297,53 @@ int traffic_selector_printf_hook(printf_hook_data_t *data,
                        written += print_in_hook(data, "%d", this->protocol);
                }
        }
-
-       if (has_proto && has_ports)
+       else
        {
-               written += print_in_hook(data, "/");
+               written += print_in_hook(data, "0");
        }
 
        /* build port string */
        if (has_ports)
        {
+               written += print_in_hook(data, "/");
+
                if (this->from_port == this->to_port)
                {
-                       struct servent *serv = getservbyport(htons(this->from_port), serv_proto);
+                       struct servent *serv;
 
-                       if (serv)
+                       if (this->protocol == IPPROTO_ICMP ||
+                               this->protocol == IPPROTO_ICMPV6)
                        {
-                               written += print_in_hook(data, "%s", serv->s_name);
+                               written += print_icmp(data, this->from_port);
                        }
                        else
                        {
-                               written += print_in_hook(data, "%d", this->from_port);
+                               serv = getservbyport(htons(this->from_port), serv_proto);
+                               if (serv)
+                               {
+                                       written += print_in_hook(data, "%s", serv->s_name);
+                               }
+                               else
+                               {
+                                       written += print_in_hook(data, "%d", this->from_port);
+                               }
                        }
                }
+               else if (is_opaque(this))
+               {
+                       written += print_in_hook(data, "OPAQUE");
+               }
+               else if (this->protocol == IPPROTO_ICMP ||
+                                this->protocol == IPPROTO_ICMPV6)
+               {
+                       written += print_icmp(data, this->from_port);
+                       written += print_in_hook(data, "-");
+                       written += print_icmp(data, this->to_port);
+               }
                else
                {
-                       written += print_in_hook(data, "%d-%d", this->from_port, this->to_port);
+                       written += print_in_hook(data, "%d-%d",
+                                                                        this->from_port, this->to_port);
                }
        }
 
@@ -305,24 +352,44 @@ int traffic_selector_printf_hook(printf_hook_data_t *data,
        return written;
 }
 
-/**
- * Implements traffic_selector_t.get_subset
- */
-static traffic_selector_t *get_subset(private_traffic_selector_t *this, private_traffic_selector_t *other)
+METHOD(traffic_selector_t, get_subset, traffic_selector_t*,
+       private_traffic_selector_t *this, traffic_selector_t *other_public)
 {
+       private_traffic_selector_t *other, *subset;
+       uint16_t from_port, to_port;
+       u_char *from, *to;
+       uint8_t protocol;
+       size_t size;
+
+       other = (private_traffic_selector_t*)other_public;
+
        if (this->dynamic || other->dynamic)
        {       /* no set_address() applied, TS has no subset */
                return NULL;
        }
-       if (this->type == other->type && (this->protocol == other->protocol ||
-                                                               this->protocol == 0 || other->protocol == 0))
+
+       if (this->type != other->type)
        {
-               u_int16_t from_port, to_port;
-               u_char *from, *to;
-               u_int8_t protocol;
-               size_t size;
-               private_traffic_selector_t *new_ts;
+               return NULL;
+       }
 
+       if (this->protocol != other->protocol &&
+               this->protocol != 0 && other->protocol != 0)
+       {
+               return NULL;
+       }
+       /* select protocol, which is not zero */
+       protocol = max(this->protocol, other->protocol);
+
+       if ((is_opaque(this) && is_opaque(other)) ||
+               (is_opaque(this) && is_any(other)) ||
+               (is_opaque(other) && is_any(this)))
+       {
+               from_port = 0xffff;
+               to_port = 0;
+       }
+       else
+       {
                /* calculate the maximum port range allowed for both */
                from_port = max(this->from_port, other->from_port);
                to_port = min(this->to_port, other->to_port);
@@ -330,127 +397,66 @@ static traffic_selector_t *get_subset(private_traffic_selector_t *this, private_
                {
                        return NULL;
                }
-               /* select protocol, which is not zero */
-               protocol = max(this->protocol, other->protocol);
-
-               switch (this->type)
-               {
-                       case TS_IPV4_ADDR_RANGE:
-                               size = sizeof(this->from4);
-                               break;
-                       case TS_IPV6_ADDR_RANGE:
-                               size = sizeof(this->from6);
-                               break;
-                       default:
-                               return NULL;
-               }
-
-               /* get higher from-address */
-               if (memcmp(this->from, other->from, size) > 0)
-               {
-                       from = this->from;
-               }
-               else
-               {
-                       from = other->from;
-               }
-               /* get lower to-address */
-               if (memcmp(this->to, other->to, size) > 0)
-               {
-                       to = other->to;
-               }
-               else
-               {
-                       to = this->to;
-               }
-               /* if "from" > "to", we don't have a match */
-               if (memcmp(from, to, size) > 0)
-               {
-                       return NULL;
-               }
-
-               /* we have a match in protocol, port, and address: return it... */
-               new_ts = traffic_selector_create(protocol, this->type, from_port, to_port);
-               memcpy(new_ts->from, from, size);
-               memcpy(new_ts->to, to, size);
-               calc_netbits(new_ts);
-               return &new_ts->public;
        }
-       return NULL;
-}
-
-/**
- * Implements traffic_selector_t.equals
- */
-static bool equals(private_traffic_selector_t *this, private_traffic_selector_t *other)
-{
-       if (this->type != other->type)
+       size = TS_IP_LEN(this);
+       /* get higher from-address */
+       if (memcmp(this->from, other->from, size) > 0)
        {
-               return FALSE;
+               from = this->from;
        }
-       if (!(this->from_port == other->from_port &&
-                 this->to_port == other->to_port &&
-                 this->protocol == other->protocol))
+       else
        {
-               return FALSE;
+               from = other->from;
        }
-       switch (this->type)
+       /* get lower to-address */
+       if (memcmp(this->to, other->to, size) > 0)
        {
-               case TS_IPV4_ADDR_RANGE:
-                       if (memeq(this->from4, other->from4, sizeof(this->from4)) &&
-                               memeq(this->to4, other->to4, sizeof(this->to4)))
-                       {
-                               return TRUE;
-                       }
-                       break;
-               case TS_IPV6_ADDR_RANGE:
-                       if (memeq(this->from6, other->from6, sizeof(this->from6)) &&
-                               memeq(this->to6, other->to6, sizeof(this->to6)))
-                       {
-                               return TRUE;
-                       }
-                       break;
-               default:
-                       break;
+               to = other->to;
        }
-       return FALSE;
+       else
+       {
+               to = this->to;
+       }
+       /* if "from" > "to", we don't have a match */
+       if (memcmp(from, to, size) > 0)
+       {
+               return NULL;
+       }
+
+       /* we have a match in protocol, port, and address: return it... */
+       subset = traffic_selector_create(protocol, this->type, from_port, to_port);
+       memcpy(subset->from, from, size);
+       memcpy(subset->to, to, size);
+       calc_netbits(subset);
+
+       return &subset->public;
+}
+
+METHOD(traffic_selector_t, equals, bool,
+       private_traffic_selector_t *this, traffic_selector_t *other)
+{
+       return traffic_selector_cmp(&this->public, other, NULL) == 0;
 }
 
 METHOD(traffic_selector_t, get_from_address, chunk_t,
        private_traffic_selector_t *this)
 {
-       switch (this->type)
-       {
-               case TS_IPV4_ADDR_RANGE:
-                       return chunk_create(this->from, sizeof(this->from4));
-               case TS_IPV6_ADDR_RANGE:
-                       return chunk_create(this->from, sizeof(this->from6));
-               default:
-                       return chunk_empty;
-       }
+       return chunk_create(this->from, TS_IP_LEN(this));
 }
 
 METHOD(traffic_selector_t, get_to_address, chunk_t,
        private_traffic_selector_t *this)
 {
-       switch (this->type)
-       {
-               case TS_IPV4_ADDR_RANGE:
-                       return chunk_create(this->to, sizeof(this->to4));
-               case TS_IPV6_ADDR_RANGE:
-                       return chunk_create(this->to, sizeof(this->to6));
-               default:
-                       return chunk_empty;
-       }
+       return chunk_create(this->to, TS_IP_LEN(this));
 }
 
-METHOD(traffic_selector_t, get_from_port, u_int16_t,
+METHOD(traffic_selector_t, get_from_port, uint16_t,
        private_traffic_selector_t *this)
 {
        return this->from_port;
 }
 
-METHOD(traffic_selector_t, get_to_port, u_int16_t,
+METHOD(traffic_selector_t, get_to_port, uint16_t,
        private_traffic_selector_t *this)
 {
        return this->to_port;
@@ -462,7 +468,7 @@ METHOD(traffic_selector_t, get_type, ts_type_t,
        return this->type;
 }
 
-METHOD(traffic_selector_t, get_protocol, u_int8_t,
+METHOD(traffic_selector_t, get_protocol, uint8_t,
        private_traffic_selector_t *this)
 {
        return this->protocol;
@@ -489,7 +495,7 @@ METHOD(traffic_selector_t, is_host, bool,
        }
        else
        {
-               size_t length = (this->type == TS_IPV4_ADDR_RANGE) ? 4 : 16;
+               size_t length = TS_IP_LEN(this);
 
                if (this->dynamic)
                {
@@ -513,33 +519,27 @@ METHOD(traffic_selector_t, is_dynamic, bool,
 METHOD(traffic_selector_t, set_address, void,
        private_traffic_selector_t *this, host_t *host)
 {
-       if (this->dynamic)
-       {
-               this->type = host->get_family(host) == AF_INET ?
-                               TS_IPV4_ADDR_RANGE : TS_IPV6_ADDR_RANGE;
+       this->type = host->get_family(host) == AF_INET ? TS_IPV4_ADDR_RANGE
+                                                                                                  : TS_IPV6_ADDR_RANGE;
 
-               if (host->is_anyaddr(host))
-               {
-                       memset(this->from6, 0x00, sizeof(this->from6));
-                       memset(this->to6, 0xFF, sizeof(this->to6));
-                       this->netbits = 0;
-               }
-               else
-               {
-                       chunk_t from = host->get_address(host);
-                       memcpy(this->from, from.ptr, from.len);
-                       memcpy(this->to, from.ptr, from.len);
-                       this->netbits = from.len * 8;
-               }
-               this->dynamic = FALSE;
+       if (host->is_anyaddr(host))
+       {
+               memset(this->from, 0x00, sizeof(this->from));
+               memset(this->to, 0xFF, sizeof(this->to));
+               this->netbits = 0;
        }
+       else
+       {
+               chunk_t from = host->get_address(host);
+               memcpy(this->from, from.ptr, from.len);
+               memcpy(this->to, from.ptr, from.len);
+               this->netbits = from.len * 8;
+       }
+       this->dynamic = FALSE;
 }
 
-/**
- * Implements traffic_selector_t.is_contained_in.
- */
-static bool is_contained_in(private_traffic_selector_t *this,
-                                                       private_traffic_selector_t *other)
+METHOD(traffic_selector_t, is_contained_in, bool,
+       private_traffic_selector_t *this, traffic_selector_t *other)
 {
        private_traffic_selector_t *subset;
        bool contained_in = FALSE;
@@ -548,7 +548,7 @@ static bool is_contained_in(private_traffic_selector_t *this,
 
        if (subset)
        {
-               if (equals(subset, this))
+               if (equals(subset, &this->public))
                {
                        contained_in = TRUE;
                }
@@ -576,14 +576,14 @@ METHOD(traffic_selector_t, includes, bool,
 }
 
 METHOD(traffic_selector_t, to_subnet, bool,
-       private_traffic_selector_t *this, host_t **net, u_int8_t *mask)
+       private_traffic_selector_t *this, host_t **net, uint8_t *mask)
 {
        /* there is no way to do this cleanly, as the address range may
         * be anything else but a subnet. We use from_addr as subnet
         * and try to calculate a usable subnet mask.
         */
        int family, non_zero_bytes;
-       u_int16_t port = 0;
+       uint16_t port = 0;
        chunk_t net_chunk;
 
        *mask = (this->netbits == NON_SUBNET_ADDRESS_RANGE) ? calc_netbits(this)
@@ -593,11 +593,11 @@ METHOD(traffic_selector_t, to_subnet, bool,
        {
                case TS_IPV4_ADDR_RANGE:
                        family = AF_INET;
-                       net_chunk.len = sizeof(this->from4);
+                       net_chunk.len = IPV4_LEN;
                        break;
                case TS_IPV6_ADDR_RANGE:
                        family = AF_INET6;
-                       net_chunk.len = sizeof(this->from6);
+                       net_chunk.len = IPV6_LEN;
                        break;
                default:
                        /* unreachable */
@@ -628,26 +628,27 @@ METHOD(traffic_selector_t, clone_, traffic_selector_t*,
        private_traffic_selector_t *this)
 {
        private_traffic_selector_t *clone;
+       size_t len = TS_IP_LEN(this);
 
        clone = traffic_selector_create(this->protocol, this->type,
                                                                        this->from_port, this->to_port);
        clone->netbits = this->netbits;
        clone->dynamic = this->dynamic;
 
-       switch (clone->type)
-       {
-               case TS_IPV4_ADDR_RANGE:
-                       memcpy(clone->from4, this->from4, sizeof(this->from4));
-                       memcpy(clone->to4, this->to4, sizeof(this->to4));
-                       return &clone->public;
-               case TS_IPV6_ADDR_RANGE:
-                       memcpy(clone->from6, this->from6, sizeof(this->from6));
-                       memcpy(clone->to6, this->to6, sizeof(this->to6));
-                       return &clone->public;
-               default:
-                       /* unreachable */
-                       return &clone->public;
-       }
+       memcpy(clone->from, this->from, len);
+       memcpy(clone->to, this->to, len);
+       return &clone->public;
+}
+
+METHOD(traffic_selector_t, hash, u_int,
+       private_traffic_selector_t *this, u_int hash)
+{
+       return chunk_hash_inc(get_from_address(this),
+                       chunk_hash_inc(get_to_address(this),
+                        chunk_hash_inc(chunk_from_thing(this->from_port),
+                         chunk_hash_inc(chunk_from_thing(this->to_port),
+                          chunk_hash_inc(chunk_from_thing(this->protocol),
+                               hash)))));
 }
 
 METHOD(traffic_selector_t, destroy, void,
@@ -656,43 +657,86 @@ METHOD(traffic_selector_t, destroy, void,
        free(this);
 }
 
+/**
+ * Compare two integers
+ */
+static int compare_int(int a, int b)
+{
+       return a - b;
+}
+
+/*
+ * See header
+ */
+int traffic_selector_cmp(traffic_selector_t *a_pub, traffic_selector_t *b_pub,
+                                                void *opts)
+{
+       private_traffic_selector_t *a, *b;
+       size_t len;
+       int res;
+
+       a = (private_traffic_selector_t*)a_pub;
+       b = (private_traffic_selector_t*)b_pub;
+
+       /* IPv4 before IPv6 */
+       res = compare_int(a->type, b->type);
+       if (res)
+       {
+               return res;
+       }
+       len = TS_IP_LEN(a);
+       /* lower starting subnets first */
+       res = memcmp(a->from, b->from, len);
+       if (res)
+       {
+               return res;
+       }
+       /* larger subnets first */
+       res = memcmp(b->to, a->to, len);
+       if (res)
+       {
+               return res;
+       }
+       /* lower protocols first */
+       res = compare_int(a->protocol, b->protocol);
+       if (res)
+       {
+               return res;
+       }
+       /* lower starting ports first */
+       res = compare_int(a->from_port, b->from_port);
+       if (res)
+       {
+               return res;
+       }
+       /* larger port ranges first */
+       return compare_int(b->to_port, a->to_port);
+}
+
 /*
  * see header
  */
-traffic_selector_t *traffic_selector_create_from_bytes(u_int8_t protocol,
+traffic_selector_t *traffic_selector_create_from_bytes(uint8_t protocol,
                                                                                                ts_type_t type,
-                                                                                               chunk_t from, u_int16_t from_port,
-                                                                                               chunk_t to, u_int16_t to_port)
+                                                                                               chunk_t from, uint16_t from_port,
+                                                                                               chunk_t to, uint16_t to_port)
 {
        private_traffic_selector_t *this = traffic_selector_create(protocol, type,
                                                                                                                        from_port, to_port);
 
-       switch (type)
+       if (!this)
        {
-               case TS_IPV4_ADDR_RANGE:
-                       if (from.len != 4 || to.len != 4)
-                       {
-                               free(this);
-                               return NULL;
-                       }
-                       memcpy(this->from4, from.ptr, from.len);
-                       memcpy(this->to4, to.ptr, to.len);
-                       break;
-               case TS_IPV6_ADDR_RANGE:
-                       if (from.len != 16 || to.len != 16)
-                       {
-                               free(this);
-                               return NULL;
-                       }
-                       memcpy(this->from6, from.ptr, from.len);
-                       memcpy(this->to6, to.ptr, to.len);
-                       break;
-               default:
-                       free(this);
-                       return NULL;
+               return NULL;
+       }
+       if (from.len != to.len || from.len != TS_IP_LEN(this))
+       {
+               free(this);
+               return NULL;
        }
+       memcpy(this->from, from.ptr, from.len);
+       memcpy(this->to, to.ptr, to.len);
        calc_netbits(this);
-       return (&this->public);
+       return &this->public;
 }
 
 /*
@@ -701,21 +745,15 @@ traffic_selector_t *traffic_selector_create_from_bytes(u_int8_t protocol,
 traffic_selector_t *traffic_selector_create_from_rfc3779_format(ts_type_t type,
                                                                                                chunk_t from, chunk_t to)
 {
-       size_t len;
        private_traffic_selector_t *this = traffic_selector_create(0, type, 0, 65535);
+       size_t len;
 
-       switch (type)
+       if (!this)
        {
-               case TS_IPV4_ADDR_RANGE:
-                       len = 4;
-                       break;
-               case TS_IPV6_ADDR_RANGE:
-                       len = 16;
-                       break;
-               default:
-                       free(this);
-                       return NULL;
+               return NULL;
        }
+       len = TS_IP_LEN(this);
+
        memset(this->from, 0x00, len);
        memset(this->to  , 0xff, len);
 
@@ -725,51 +763,46 @@ traffic_selector_t *traffic_selector_create_from_rfc3779_format(ts_type_t type,
        }
        if (to.len > 1)
        {
-               u_int8_t mask = to.ptr[0] ? (1 << to.ptr[0]) - 1 : 0;
+               uint8_t mask = to.ptr[0] ? (1 << to.ptr[0]) - 1 : 0;
 
                memcpy(this->to, to.ptr+1, to.len-1);
                this->to[to.len-2] |= mask;
        }
-       this->netbits = chunk_equals(from, to) ? (from.len-1)*8 - from.ptr[0]
-                                                                                  : NON_SUBNET_ADDRESS_RANGE;
-       return (&this->public);
+       calc_netbits(this);
+       return &this->public;
 }
 
 /*
  * see header
  */
 traffic_selector_t *traffic_selector_create_from_subnet(host_t *net,
-                                                       u_int8_t netbits, u_int8_t protocol, u_int16_t port)
+                                                       uint8_t netbits, uint8_t protocol,
+                                                       uint16_t from_port, uint16_t to_port)
 {
        private_traffic_selector_t *this;
+       ts_type_t type;
        chunk_t from;
 
-       this = traffic_selector_create(protocol, 0, 0, 65535);
-
        switch (net->get_family(net))
        {
                case AF_INET:
-                       this->type = TS_IPV4_ADDR_RANGE;
+                       type = TS_IPV4_ADDR_RANGE;
                        break;
                case AF_INET6:
-                       this->type = TS_IPV6_ADDR_RANGE;
+                       type = TS_IPV6_ADDR_RANGE;
                        break;
                default:
                        net->destroy(net);
-                       free(this);
                        return NULL;
        }
+
+       this = traffic_selector_create(protocol, type, from_port, to_port);
+
        from = net->get_address(net);
        memcpy(this->from, from.ptr, from.len);
-       netbits = min(netbits, this->type == TS_IPV4_ADDR_RANGE ? 32 : 128);
+       netbits = min(netbits, TS_IP_LEN(this) * 8);
        calc_range(this, netbits);
-       if (port)
-       {
-               this->from_port = port;
-               this->to_port = port;
-       }
        net->destroy(net);
-
        return &this->public;
 }
 
@@ -777,55 +810,67 @@ traffic_selector_t *traffic_selector_create_from_subnet(host_t *net,
  * see header
  */
 traffic_selector_t *traffic_selector_create_from_string(
-                                                                               u_int8_t protocol, ts_type_t type,
-                                                                               char *from_addr, u_int16_t from_port,
-                                                                               char *to_addr, u_int16_t to_port)
+                                                                               uint8_t protocol, ts_type_t type,
+                                                                               char *from_addr, uint16_t from_port,
+                                                                               char *to_addr, uint16_t to_port)
 {
-       private_traffic_selector_t *this = traffic_selector_create(protocol, type,
-                                                                                                                       from_port, to_port);
+       private_traffic_selector_t *this;
+       int family;
 
        switch (type)
        {
                case TS_IPV4_ADDR_RANGE:
-                       if (inet_pton(AF_INET, from_addr, (struct in_addr*)this->from4) < 0)
-                       {
-                               free(this);
-                               return NULL;
-                       }
-                       if (inet_pton(AF_INET, to_addr, (struct in_addr*)this->to4) < 0)
-                       {
-                               free(this);
-                               return NULL;
-                       }
+                       family = AF_INET;
                        break;
                case TS_IPV6_ADDR_RANGE:
-                       if (inet_pton(AF_INET6, from_addr, (struct in6_addr*)this->from6) < 0)
-                       {
-                               free(this);
-                               return NULL;
-                       }
-                       if (inet_pton(AF_INET6, to_addr, (struct in6_addr*)this->to6) < 0)
-                       {
-                               free(this);
-                               return NULL;
-                       }
+                       family = AF_INET6;
                        break;
+               default:
+                       return NULL;
+       }
+
+       this = traffic_selector_create(protocol, type, from_port, to_port);
+
+       if (inet_pton(family, from_addr, this->from) != 1 ||
+               inet_pton(family, to_addr, this->to) != 1)
+       {
+               free(this);
+               return NULL;
        }
        calc_netbits(this);
-       return (&this->public);
+       return &this->public;
 }
 
 /*
  * see header
  */
-traffic_selector_t *traffic_selector_create_dynamic(u_int8_t protocol,
-                                                                       u_int16_t from_port, u_int16_t to_port)
+traffic_selector_t *traffic_selector_create_from_cidr(
+                                                                               char *string, uint8_t protocol,
+                                                                               uint16_t from_port, uint16_t to_port)
+{
+       host_t *net;
+       int bits;
+
+       net = host_create_from_subnet(string, &bits);
+       if (net)
+       {
+               return traffic_selector_create_from_subnet(net, bits, protocol,
+                                                                                                  from_port, to_port);
+       }
+       return NULL;
+}
+
+/*
+ * see header
+ */
+traffic_selector_t *traffic_selector_create_dynamic(uint8_t protocol,
+                                                                       uint16_t from_port, uint16_t to_port)
 {
        private_traffic_selector_t *this = traffic_selector_create(
                                                        protocol, TS_IPV4_ADDR_RANGE, from_port, to_port);
 
-       memset(this->from6, 0, sizeof(this->from6));
-       memset(this->to6, 0xFF, sizeof(this->to6));
+       memset(this->from, 0, sizeof(this->from));
+       memset(this->to, 0xFF, sizeof(this->to));
        this->netbits = 0;
        this->dynamic = TRUE;
 
@@ -835,15 +880,21 @@ traffic_selector_t *traffic_selector_create_dynamic(u_int8_t protocol,
 /*
  * see declaration
  */
-static private_traffic_selector_t *traffic_selector_create(u_int8_t protocol,
-                                               ts_type_t type, u_int16_t from_port, u_int16_t to_port)
+static private_traffic_selector_t *traffic_selector_create(uint8_t protocol,
+                                               ts_type_t type, uint16_t from_port, uint16_t to_port)
 {
        private_traffic_selector_t *this;
 
+       /* sanity check */
+       if (type != TS_IPV4_ADDR_RANGE && type != TS_IPV6_ADDR_RANGE)
+       {
+               return NULL;
+       }
+
        INIT(this,
                .public = {
-                       .get_subset = (traffic_selector_t*(*)(traffic_selector_t*,traffic_selector_t*))get_subset,
-                       .equals = (bool(*)(traffic_selector_t*,traffic_selector_t*))equals,
+                       .get_subset = _get_subset,
+                       .equals = _equals,
                        .get_from_address = _get_from_address,
                        .get_to_address = _get_to_address,
                        .get_from_port = _get_from_port,
@@ -852,11 +903,12 @@ static private_traffic_selector_t *traffic_selector_create(u_int8_t protocol,
                        .get_protocol = _get_protocol,
                        .is_host = _is_host,
                        .is_dynamic = _is_dynamic,
-                       .is_contained_in = (bool(*)(traffic_selector_t*,traffic_selector_t*))is_contained_in,
+                       .is_contained_in = _is_contained_in,
                        .includes = _includes,
                        .set_address = _set_address,
                        .to_subnet = _to_subnet,
                        .clone = _clone_,
+                       .hash = _hash,
                        .destroy = _destroy,
                },
                .from_port = from_port,
@@ -864,7 +916,10 @@ static private_traffic_selector_t *traffic_selector_create(u_int8_t protocol,
                .protocol = protocol,
                .type = type,
        );
-
+       if (protocol == IPPROTO_ICMP || protocol == IPPROTO_ICMPV6)
+       {
+               this->from_port = from_port < 256 ? from_port << 8 : from_port;
+               this->to_port = to_port < 256 ? to_port << 8 : to_port;
+       }
        return this;
 }
-