iprange: rework ip range parsing functions

The previous parsing functions had a few problems:

  * They did accept negative numbers in addresses (e.g. 192.0.2.-5).

  * They relied on C undefined behaviour for detecting /0 prefix
    length: "mask = htonl(~((1 << (32 - m)) - 1)" was wrong for m = 0,
    because that resulted in a left shift of 32 bits, on a 32 bit wide
    value (the right operand of a bitwise shift operator must be
    strictly smaller than the width of the promoted left operand).

  * They misinterpreted /32 prefixes as disable requests. In fact, due
    to the undefined behaviour described above, /0 and /32 prefix
    lengths were represented in the same way by parse1(), that is, with
    an iprange_t structure where ->begin == ->end. Therefore
    load_ranges() had no way to distinguish between them and did
    disable the module in both cases.

This patch fixes these issues and brings the following improvements:

  * It uses getaddrinfo() to parse IP addresses, so it accept (almost)
    all IPv4 representations and is more easily extensible to IPv6 in
    the future.

  * It warns when the IP address used in CIDR notation is not the first
    address in the range (e.g. the first address of 192.0.2.1/24 is
    192.0.2.0, not 192.0.2.1).

  * It doesn't _exit() on parsing failures, thus making the functions
    usable in an EV_CONFIG_RELOAD handler.

While there, the unfinished tunnel_ranges code, which was already
commented, has been removed.

Signed-off-by: Guillaume Nault <g.nault@alphalink.fr>

3 files changed, 171 insertions, 67 deletions

diff --git a/accel-pppd/iprange.c b/accel-pppd/iprange.c
index 6ea2c2f9..608574e3 100644
--- a/accel-pppd/iprange.c
+++ b/accel-pppd/iprange.c
@@ -1,12 +1,16 @@
+#include <stdbool.h>
 #include <stdlib.h>
 #include <stdio.h>
 #include <stdint.h>
 #include <unistd.h>
 #include <string.h>
+#include <arpa/inet.h>
+#include <netinet/in.h>
 
 #include "triton.h"
 #include "list.h"
 #include "log.h"
+#include "utils.h"
 
 #include "iprange.h"
 
@@ -22,64 +26,138 @@ struct iprange_t
 static int conf_disable = 0;
 
 static LIST_HEAD(client_ranges);
-//static LIST_HEAD(tunnel_ranges);
 
-//parses ranges like x.x.x.x/mask
-static struct iprange_t *parse1(const char *str)
+/* Maximum IPv4 address length with CIDR notation but no extra 0,
+ * e.g. "0xff.0xff.0xff.0xff/32".
+ */
+#define CIDR_MAXLEN 22
+
+
+static void free_ranges(struct list_head *head)
 {
-	int n,f1,f2,f3,f4,m;
-	struct iprange_t *r;
-	int mask;
-
-	n = sscanf(str, "%u.%u.%u.%u/%u",&f1, &f2, &f3, &f4, &m);
-	if (n != 5)
-		return NULL;
-	if (f1 > 255)
-		return NULL;
-	if (f2 > 255)
-		return NULL;
-	if (f3 > 255)
-		return NULL;
-	if (f4 > 255)
-		return NULL;
-	if (m > 32)
-		return NULL;
-
-	r = _malloc(sizeof(*r));
-	r->begin = (f4 << 24) | (f3 << 16) | (f2 << 8) | f1;
-
-	mask = htonl(~((1 << (32 - m)) - 1));
-	r->end = ntohl(r->begin | ~mask);
-	r->begin = ntohl(r->begin);
-
-	return r;
+	struct iprange_t *range;
+
+	while (!list_empty(head)) {
+		range = list_first_entry(head, typeof(*range), entry);
+		list_del(&range->entry);
+		_free(range);
+	}
 }
 
-//parses ranges like x.x.x.x-y
-static struct iprange_t *parse2(const char *str)
+static int parse_iprange(const char *str, struct iprange_t **range)
 {
-	int n,f1,f2,f3,f4,m;
-	struct iprange_t *r;
+	char ipstr[CIDR_MAXLEN + 1] = { 0 };
+	struct iprange_t *_range;
+	struct in_addr addr;
+	const char *errmsg;
+	char *suffix_str;
+	uint32_t ipmin;
+	uint32_t ipmax;
+	bool is_cidr;
+
+	/* Extra spaces and comments must have already been removed */
+	if (strpbrk(str, " \t#")) {
+		log_error("iprange: impossible to parse range \"%s\":"
+			  " invalid space or comment character found\n",
+			  str);
+		return -1;
+	}
+
+	if (!strcmp(str, "disable"))
+		goto disable;
+
+	strncpy(ipstr, str, CIDR_MAXLEN + 1);
+	if (ipstr[CIDR_MAXLEN] != '\0') {
+		log_error("iprange: impossible to parse range \"%s\":"
+			  " line too long\n",
+			  str);
+		return -1;
+	}
+
+	suffix_str = strpbrk(ipstr, "-/");
+	if (!suffix_str) {
+		log_error("iprange: impossible to parse range \"%s\":"
+			  " unrecognised range format\n",
+			  str);
+		return -1;
+	}
+
+	is_cidr = *suffix_str == '/';
+	*suffix_str = '\0';
+	++suffix_str;
+
+	if (u_parse_ip4addr(ipstr, &addr, &errmsg)) {
+		log_error("iprange: impossible to parse range \"%s\":"
+			  " invalid IPv4 address \"%s\"\n",
+			  str, ipstr);
+		return -1;
+	}
+	ipmin = ntohl(addr.s_addr);
+
+
+	/* If is_cidr is set, range is given with CIDR notation,
+	 * e.g. "192.0.2.0/24".
+	 * If unset, range is an IP address where the last octet is replaced by
+	 * an octet range, e.g. "192.0.2.0-255".
+	 */
+	if (is_cidr) {
+		long int prefix_len;
+		uint32_t mask;
+
+		if (u_readlong(&prefix_len, suffix_str, 0, 32)) {
+			log_error("iprange: impossible to parse range \"%s\":"
+				  " invalid CIDR prefix length \"/%s\"\n",
+				  str, suffix_str);
+			return -1;
+		}
+
+		/* Interpret /0 as disable request */
+		if (prefix_len == 0)
+			goto disable;
+
+		mask = UINT32_MAX << (32 - prefix_len);
+
+		if (ipmin != (ipmin & mask)) {
+			char buf[INET_ADDRSTRLEN] = { 0 };
 
-	n = sscanf(str, "%u.%u.%u.%u-%u",&f1, &f2, &f3, &f4, &m);
-	if (n != 5)
-		return NULL;
-	if (f1 > 255)
-		return NULL;
-	if (f2 > 255)
-		return NULL;
-	if (f3 > 255)
-		return NULL;
-	if (f4 > 255)
-		return NULL;
-	if (m < f4 || m > 255)
-		return NULL;
-
-	r = _malloc(sizeof(*r));
-	r->begin = ntohl((f4 << 24) | (f3 << 16) | (f2 << 8) | f1);
-	r->end = ntohl((m << 24) | (f3 << 16) | (f2 << 8) | f1);
-
-	return r;
+			ipmin &= mask;
+			addr.s_addr = htonl(ipmin);
+			log_warn("iprange: first IP of range %s will be %s\n",
+				 str, inet_ntop(AF_INET, &addr, buf,
+						sizeof(buf)));
+		}
+
+		ipmax = ipmin | ~mask;
+	} else {
+		long int max;
+
+		if (u_readlong(&max, suffix_str, ipmin & 0xff, 255)) {
+			log_error("iprange: impossible to parse range \"%s\":"
+				  " invalid upper bound \"-%s\"\n",
+				  str, suffix_str);
+			return -1;
+		}
+
+		ipmax = (ipmin & 0xffffff00) | max;
+	}
+
+	_range = _malloc(sizeof(*_range));
+	if (!_range) {
+		log_error("iprange: impossible to allocate range \"%s\":"
+			  " memory allocation failed\n", str);
+		return -1;
+	}
+
+	_range->begin = ipmin;
+	_range->end = ipmax;
+	*range = _range;
+
+	return 0;
+
+disable:
+	*range = NULL;
+
+	return 0;
 }
 
 static void load_ranges(struct list_head *list, const char *conf_sect)
@@ -94,24 +172,22 @@ static void load_ranges(struct list_head *list, const char *conf_sect)
 	}
 
 	list_for_each_entry(opt, &s->items, entry) {
-		if (!strcmp(opt->name, "disable"))
-			goto disable;
-		r = parse1(opt->name);
-		if (!r)
-			r = parse2(opt->name);
+		/* Ignore parsing errors, parse_iprange() already logs suitable
+		 * error message.
+		 */
+		if (parse_iprange(opt->name, &r) < 0)
+			continue;
+
 		if (!r) {
-			log_emerg("iprange: cann't parse '%s' in '%s'\n", opt->name, conf_sect);
-			_exit(EXIT_FAILURE);
+			log_warn("iprange: iprange module disabled, improper IP address assignment may cause kernel soft lockup!\n");
+			free_ranges(list);
+			conf_disable = 1;
+
+			return;
 		}
-		if (r->begin == r->end)
-			goto disable;
+
 		list_add_tail(&r->entry, list);
 	}
-
-	return;
-disable:
-	conf_disable = 1;
-	log_emerg("iprange: iprange module disabled so improper ip address assigning may cause kernel soft lockup!\n");
 }
 
 static int check_range(struct list_head *list, in_addr_t ipaddr)
@@ -145,7 +221,6 @@ int __export iprange_tunnel_check(in_addr_t ipaddr)
 static void iprange_init(void)
 {
 	load_ranges(&client_ranges, "client-ip-range");
-	//load_ranges(&tunnel_ranges, "tunnel-ip-range");
 }
 
 DEFINE_INIT(10, iprange_init);
diff --git a/accel-pppd/utils.c b/accel-pppd/utils.c
index 81b4c993..3b87ee16 100644
--- a/accel-pppd/utils.c
+++ b/accel-pppd/utils.c
@@ -1,7 +1,9 @@
 #include <errno.h>
+#include <netdb.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <unistd.h>
+#include <netinet/in.h>
 
 #include "triton.h"
 #include "utils.h"
@@ -35,6 +37,31 @@ int __export u_readlong(long int *dst, const char *src,
         }
 }
 
+int __export u_parse_ip4addr(const char *src, struct in_addr *addr,
+			     const char **err_msg)
+{
+	struct addrinfo hint = {
+		.ai_flags = AI_NUMERICHOST,
+		.ai_family = AF_INET,
+		.ai_socktype = 0,
+		.ai_protocol = 0,
+	};
+	struct addrinfo *ainfo;
+	int err;
+
+	err = getaddrinfo(src, NULL, &hint, &ainfo);
+	if (err) {
+		*err_msg = gai_strerror(err);
+		return err;
+	}
+
+	*addr = ((struct sockaddr_in *)ainfo->ai_addr)->sin_addr;
+
+	freeaddrinfo(ainfo);
+
+	return 0;
+}
+
 int __export u_randbuf(void *buf, size_t buf_len, int *err)
 {
 	uint8_t *u8buf = buf;
diff --git a/accel-pppd/utils.h b/accel-pppd/utils.h
index be62f6a3..87582648 100644
--- a/accel-pppd/utils.h
+++ b/accel-pppd/utils.h
@@ -5,6 +5,8 @@
 
 void u_inet_ntoa(in_addr_t, char *str);
 int u_readlong(long int *dst, const char *src, long int min, long int max);
+int u_parse_ip4addr(const char *src, struct in_addr *addr,
+		    const char **err_msg);
 int u_randbuf(void *buf, size_t buf_len, int *err);
 
 #endif