From a5e2a35f03c8e176dd005faa9aad81c29a16a704 Mon Sep 17 00:00:00 2001
From: stilez <stilez@users.noreply.github.com>
Date: Fri, 21 Feb 2014 02:19:56 +0000
Subject: [PATCH] Tighten is_subnet() functions

The is_subnet(), is_subnetv4() and is_subnetv6() functions have significant issues in their coding logic.

Issues:

1) Functions use is_numeric(), so they validate invalid bitcount parts such as '1.1.1.1/6.5' or '::8000/94.7' as valid subnet strings

2) Validation explodes to a list of 2 variables rather than testing the whole string, so it's possible to craft a malicious string or mis-enter text such as "::8000/24/;\nARBITRARYTEXT;", or any valid subnet followed by a third '/' character followed by arbitrary text, which always validates as permitted data. In v2.x this allows arbitrary or malformed text placement into core config files such as pf.conf, f/w tables, and probably exec() strings, from the GUI (reported previously)

3) is_subnet() and is_subnetv6() return false for the valid subnet /0 as a CIDR, although technically this is allowed and valid as a subnet (it may have unambiguous and useful meaning in a router or security context).

The existing code is easier to rewrite than fix. It's a lot shorter too. A single preg_match() does all the separating of valid subnet parts and identifies which if either type of subnet the string could be, by exactly matching EITHER (7-15 char string containing only 0-9.) OR (2-39 char string containing only 0-9a-f:) followed by '/' and (1-3 numeric digits). Depending which of the first two matches is non-empty, it then checks the "ip" part" and "numeric" part are each valid, and exits. Efficient and robust.

NOTE: - these functions returned a True/false value ( is_subnet() or !is_subnet() ). As any positive number evaluates in a Boolean sense to True, I have taken advantage of this to have "success" return 4 or 6 (not just "true") to say the type of subnet matched.  It's no extra effort or cost, and turns out to allow a lot of efficiency and code simplification elsewhere, since calling code that needs to validate a subnet *and* test if it's IPv4 or IPv6, a common situation, can now do this reliably with a single is_subnet() call:

  if (($sn = is_subnet($data)) {
      if ($sn == 4)
         IPv4 handler code...
      else
         IPv6 handler code...
  } else
      error handler code...

I've used it here to write is_subnetv4() and is_subnetv6() to be single line near-aliases of is_subnet(), that just check the return value is indeed 4 or 6.
---
 etc/inc/util.inc | 45 ++++++++++++---------------------------------
 1 file changed, 12 insertions(+), 33 deletions(-)

diff --git a/etc/inc/util.inc b/etc/inc/util.inc
index a81b155a77..9ce0fd03d0 100644
--- a/etc/inc/util.inc
+++ b/etc/inc/util.inc
@@ -593,50 +593,29 @@ function is_ipaddroralias($ipaddr) {
 
 }
 
-/* returns true if $subnet is a valid IPv4 or IPv6 subnet in CIDR format */
+/* returns true if $subnet is a valid IPv4 or IPv6 subnet in CIDR format
+   	false - if not a valid subnet
+   	true (numeric 4 or 6) - if valid, gives type of subnet */
 function is_subnet($subnet) {
-	if(is_subnetv4($subnet)) {
-		return true;
-	}
-	if(is_subnetv6($subnet)) {
-		return true;
+	if (is_string($subnet) && preg_match('/^(?:([0-9.]{7,15})|([0-9a-f:]{2,39}))\/(\d{1,3})$/i', $subnet, $parts)) {
+		if (is_ipaddrv4($parts[1]) && $parts[3] <= 32)
+			return 4;
+		if (is_ipaddrv6($parts[2]) && $parts[3] <= 128)
+			return 6;
 	}
 	return false;
 }
 
-/* returns true if $subnet is a valid IPv4 subnet in CIDR format */
+/* same as is_subnet() but accepts IPv4 only */
 function is_subnetv4($subnet) {
-	if (!is_string($subnet))
-		return false;
-
-	list($hp,$np) = explode('/', $subnet);
-
-	if (!is_ipaddrv4($hp))
-		return false;
-
-	if (!is_numeric($np) || ($np < 1) || ($np > 32))
-		return false;
-
-	return true;
+	return (is_subnet($subnet) == 4);
 }
 
-/* returns true if $subnet is a valid IPv6 subnet in CIDR format */
+/* same as is_subnet() but accepts IPv6 only */
 function is_subnetv6($subnet) {
-	if (!is_string($subnet))
-		return false;
-
-	list($hp,$np) = explode('/', $subnet);
-
-	if (!is_ipaddrv6($hp))
-		return false;
-
-	if (!is_numeric($np) || ($np < 1) || ($np > 128))
-		return false;
-
-	return true;
+	return (is_subnet($subnet) == 6);
 }
 
-
 /* returns true if $subnet is a valid subnet in CIDR format or an alias thereof */
 function is_subnetoralias($subnet) {
 	global $aliastable;