[ntpsec commit] Rewrite decodenetnum()

[Daniel Franke]

Module:    ntpsec
Branch:    master
Commit:    f08eaec887218e5b99e7dd26ac624167e1c14e90
Changeset: http://git.ntpsec.org/ntpsec/commit/?id=f08eaec887218e5b99e7dd26ac624167e1c14e90

Author:    Daniel Fox Franke <dfoxfranke at gmail.com>
Date:      Fri Sep 25 17:39:02 2015 -0400

Rewrite decodenetnum()

Improve readability, avoid crashing on overlong input, and reject more
malformed input.

---

 libntp/decodenetnum.c | 154 +++++++++++++++++++++++++++++++++++---------------
 1 file changed, 109 insertions(+), 45 deletions(-)

diff --git a/libntp/decodenetnum.c b/libntp/decodenetnum.c
index c7c2563..aa2f3ac 100644
--- a/libntp/decodenetnum.c
+++ b/libntp/decodenetnum.c
@@ -1,13 +1,15 @@
 /*
- * decodenetnum - return a net number (this is crude, but careful)
+ * decodenetnum - convert text IP address and port to sockaddr_u
  */
 #include <config.h>
+#include <stddef.h>
+#include <stdbool.h>
+#include <string.h>
 #include <sys/types.h>
-#include <ctype.h>
 #include <sys/socket.h>
+#include <netdb.h>
 #include <netinet/in.h>
 
-#include "ntp.h"
 #include "ntp_stdlib.h"
 #include "ntp_assert.h"
 
@@ -15,6 +17,15 @@
  * decodenetnum		convert text IP address and port to sockaddr_u
  *
  * Returns false for failure, true for success.
+ *
+ * We accept:
+ * IPv4
+ * IPv6
+ * [IPv6]
+ * IPv4:port
+ * [IPv6]:port
+ *
+ * The IP must be numeric but the port can be symbolic.
  */
 bool
 decodenetnum(
@@ -23,57 +34,110 @@ decodenetnum(
 	)
 {
 	struct addrinfo hints, *ai = NULL;
-	int err;
-	u_short port;
-	const char *cp;
-	const char *port_str;
-	char *pp;
-	char *np;
-	char name[80];
+	const char *ip_start, *ip_end, *port_start, *temp;
+	const size_t numlen = strlen(num);
+	bool have_brackets;
+
+#ifdef INCLUDE_IPV6_SUPPORT
+	char ip[INET6_ADDRSTRLEN];
+#else
+	char ip[INET_ADDRSTRLEN];
+#endif
 
 	NTP_REQUIRE(num != NULL);
-	NTP_REQUIRE(strlen(num) < sizeof(name));
+	/* Quickly reject empty or impossibly long inputs. */
+	if(numlen == 0 ||
+	   numlen > (sizeof ip - 1) + (NI_MAXSERV - 1) + (3 /* "[]:" */)) {
+		return false;
+	}
 
-	port_str = NULL;
-	if ('[' != num[0]) {
-		/*
-		 * to distinguish IPv6 embedded colons from a port
-		 * specification on an IPv4 address, assume all 
-		 * legal IPv6 addresses have at least two colons.
-		 */
-		pp = strchr(num, ':');
-		if (NULL == pp)
-			cp = num;	/* no colons */
-		else if (NULL != strchr(pp + 1, ':'))
-			cp = num;	/* two or more colons */
-		else {			/* one colon */
-			strlcpy(name, num, sizeof(name));
-			cp = name;
-			pp = strchr(cp, ':');
-			*pp = '\0';
-			port_str = pp + 1;
+	/* Is this a bracketed IPv6 address? */
+	have_brackets = ('[' == num[0]);
+	if(have_brackets) {
+		/* If it's formatted like [IPv6]:port, the port part
+		   comes after the "]:". */
+		if((temp = strstr(num, "]:")) != NULL) {
+			ip_start = num + 1;
+			ip_end = temp;
+			port_start = temp + 2;
 		}
+		else if(num[numlen-1] == ']') {
+			/* It's just [IPv6]. */
+			ip_start = num + 1;
+			ip_end = ip_start + numlen - 1;
+			port_start = NULL;
+		}
+		else {
+			/* Anything else must be invalid. */
+			return false;
+		}
+	}
+	/* No brackets. Searching backward, see if there's at least one
+	 * colon... */
+	else if((temp = strrchr(num, ':')) != NULL) {
+		/* ...and then look for a second one, searching forward. */
+		if(strchr(num, ':') == temp) {
+			/* Searching from both directions gave us the same
+			   result, so there's only one colon. What's after
+			   it is the port. */
+			ip_start = num;
+			ip_end = temp;
+			port_start = temp + 1;
+		} else {
+			/* Two colons and no brackets, so it has to be a bare
+			   IPv6 address */
+			ip_start = num;
+			ip_end = ip_start + numlen;
+			port_start = NULL;
+		}
+	} else {
+		/* No colon, no brackets. */
+		ip_start = num;
+		ip_end = ip_start + numlen;
+		port_start = NULL;
+	}
+
+	/* Now we have ip_start pointing to the start of the IP,
+	   ip_end pointing past the end of the IP, and port_start
+	   either NULL or pointing to the start of the port. Check
+	   whether the IP is short enough to possibly be valid and
+	   if so copy it into ip. */
+	if(ip_end - ip_start + 1 > sizeof ip) {
+		return false;
 	} else {
-		cp = num + 1;
-		np = name; 
-		while (*cp && ']' != *cp)
-			*np++ = *cp++;
-		*np = 0;
-		if (']' == cp[0] && ':' == cp[1] && '\0' != cp[2])
-			port_str = &cp[2];
-		cp = name; 
+		memcpy(ip, ip_start, ip_end - ip_start);
+		ip[ip_end - ip_start] = '\0';
 	}
+
 	ZERO(hints);
-	hints.ai_flags = Z_AI_NUMERICHOST;
-	err = getaddrinfo(cp, "ntp", &hints, &ai);
-	if (err != 0)
+	hints.ai_socktype = SOCK_DGRAM;
+	hints.ai_flags = AI_NUMERICHOST;
+	hints.ai_protocol = IPPROTO_UDP;
+	/* One final validity check: only IPv6 addresses are allowed to
+	 * have brackets. */
+#ifdef INCLUDE_IPV6_SUPPORT
+	hints.ai_family = have_brackets ? AF_INET6 : AF_UNSPEC;
+#else
+	if(have_brackets) {
+		return false;
+	}
+	hints.ai_family = AF_INET;
+#endif
+
+	/* If we've gotten this far, then we still don't know that
+	   either the IP address or the port is well-formed, but at
+	   least they're unambiguously delimited from each other.
+	   Let getaddrinfo() perform all further validation. */
+	if(getaddrinfo(ip, port_start == NULL ? "ntp" : port_start,
+		       &hints, &ai) != 0) {
 		return false;
+	}
+
 	NTP_INSIST(ai->ai_addrlen <= sizeof(*netnum));
-	ZERO(*netnum);
-	memcpy(netnum, ai->ai_addr, ai->ai_addrlen);
+	if(netnum) {
+		ZERO(*netnum);
+		memcpy(netnum, ai->ai_addr, ai->ai_addrlen);
+	}
 	freeaddrinfo(ai);
-	if (NULL == port_str || 1 != sscanf(port_str, "%hu", &port))
-		port = NTP_PORT;
-	SET_PORT(netnum, port);
 	return true;
 }