ip-util: reject routes with characters outside address/prefix syntax

ip_route_sanity_check() was a format normalizer, not a validator: any
dot-free route (all IPv6, or arbitrary text) returned success
unexamined, and a dotted-netmask IPv4 route passed through unchanged.
Since route_adddel() substitutes the validated route into
route-add-cmd/route-del-cmd and executes it via `/bin/sh -c` as root,
a route string carrying shell metacharacters that survived this check
was a root command-injection vector.

Rewrite the check to fully parse the route as an IPv4 or IPv6 address
plus prefix, or the literal keyword "default" (the documented
all-traffic-through-VPN shortcut, checked separately by config.c after
this function runs), and reject anything left over. Numeric IPv4
prefixes are still normalized to a dotted netmask as before.

Adds REQ-MAIN-SEC-008 and tests/route-sanity-check.c, confirmed
against real config/test usage (including "route = default") so the
stricter validation doesn't regress documented syntax.

Signed-off-by: Nikos Mavrogiannopoulos <n.mavrogiannopoulos@gmail.com>

doc/requirements/internal/main.md

@@ -11,6 +11,8 @@ sources:
   - src/main-sec-mod-cmd.c
   - src/main-proc.c
   - src/ip-lease.c
+  - src/route-add.c
+  - src/ip-util.c
   - src/vpn.h
   - doc/design.md#the-main-process
   - doc/requirements/internal/ipc.md
@@ -364,6 +366,42 @@ be retained (so its score history is not lost prematurely).
 `last_reset + ban_reset_time`; confirm it is now removed.
 **Links:** REQ-MAIN-SEC-003
 
+### REQ-MAIN-SEC-008 — Route strings executed via `route-add-cmd`/`route-del-cmd` MUST be validated as well-formed IPv4/IPv6 CIDR, or the literal keyword `default`
+
+**Requirement:** `ip_route_sanity_check()` MUST reject (return < 0) any route
+string that is not (a) the exact literal string `default`, or (b) a
+syntactically valid IPv4 or IPv6 address plus prefix — the IPv4 address and
+prefix (or dotted netmask) parsed via `inet_pton`/`ipv4_mask_to_int` with
+the prefix numerically in `[0,32]`, and IPv6 addresses parsed via
+`inet_pton(AF_INET6, ...)` with the prefix in `[0,128]`. It MUST NOT return
+success (0) for a string it cannot fully parse as one of these two forms —
+in particular it MUST NOT accept an IPv6 or otherwise dot-free route
+unexamined, MUST NOT pass through a dotted-netmask route unexamined, and
+MUST NOT match `default` as a prefix or substring (e.g. `Default`,
+`default/24`, and `default; <cmd>` MUST all be rejected). This is a
+security boundary: `route()`/`route_adddel()` (`src/route-add.c`) execute
+the validated route, substituted into `route-add-cmd`/`route-del-cmd`, via
+`/bin/sh -c` as root (`src/route-add.c:50`); a route string containing
+shell metacharacters that survives validation is a root command-injection
+vector. The `default` exception exists because `config.c:2199-2200`
+recognizes that exact string, after this check runs, as the documented
+keyword (`doc/sample.config`) for routing all client traffic through the
+VPN — it is not an address and carries no shell metacharacters.
+**Strength:** MUST NOT
+**Status:** DERIVED
+**Source:** src/ip-util.c (`ip_route_sanity_check`); src/route-add.c:37-158
+(`call_script`, `route_adddel`); call sites: src/config.c:731,2195,2210;
+src/sup-config/file.c:254-266; src/sup-config/radius.c:80
+**Acceptance:** negative, local (unit) — `tests/route-sanity-check.c` feeds
+`ip_route_sanity_check()` routes containing shell metacharacters in both
+dot-free form (e.g. `::/0; touch /tmp/x`) and dotted-netmask form (e.g.
+`1.2.3.4/5.6.7.8; touch /tmp/x`), and confirms both are rejected. Positive
+cases (`10.0.0.0/24`, `10.0.0.0/255.255.255.0`, `2001:db8::/32`, `default`)
+MUST be accepted (numeric IPv4 prefixes normalized to a dotted netmask);
+`Default`, `default/24`, and `default; touch /tmp/x` MUST be rejected.
+**Links:** none (first requirement for `src/route-add.c`/`src/ip-util.c`
+route validation)
+
 ---
 
 ## TEARDOWN

src/ip-util.c

@@ -21,6 +21,8 @@
 #include "common/common.h"
 #include <string.h>
 #include <stdio.h>
+#include <stdlib.h>
+#include <errno.h>
 #include <talloc.h>
 #include <assert.h>
 #include <stddef.h>
@@ -79,62 +81,6 @@ unsigned int ipv6_prefix_to_mask(struct in6_addr *in6, unsigned int prefix)
 	return 1;
 }
 
-/* check whether a route is on the expected format, and if it cannot be
- * fixed, then returns a negative code.
- *
- * The expected format by clients for IPv4 is xxx.xxx.xxx.xxx/xxx.xxx.xxx.xxx, i.e.,
- * this function converts xxx.xxx.xxx.xxx/prefix to the above for IPv4.
- */
-int ip_route_sanity_check(void *pool, char **_route)
-{
-	char *p;
-	unsigned int prefix;
-	char *route = *_route, *n;
-	char *slash_ptr, *pstr;
-
-	/* this check is valid for IPv4 only */
-	p = strchr(route, '.');
-	if (p == NULL)
-		return 0;
-
-	p = strchr(p, '/');
-	if (p == NULL) {
-		oc_syslog(
-			LOG_ERR,
-			"route '%s' in wrong format, use xxx.xxx.xxx.xxx/xxx.xxx.xxx.xxx",
-			route);
-		return -1;
-	}
-	slash_ptr = p;
-	p++;
-
-	/* if we are in dotted notation exit */
-	if (strchr(p, '.') != 0)
-		return 0;
-
-	/* we are most likely in the xxx.xxx.xxx.xxx/prefix format */
-	prefix = atoi(p);
-
-	pstr = ipv4_prefix_to_strmask(pool, prefix);
-	if (pstr == NULL) {
-		oc_syslog(LOG_ERR, "cannot figure format of route '%s'", route);
-		return -1;
-	}
-
-	*slash_ptr = 0;
-
-	n = talloc_asprintf(pool, "%s/%s", route, pstr);
-	if (n == NULL) {
-		oc_syslog(LOG_ERR, "memory error");
-		return -1;
-	}
-	*_route = n;
-
-	talloc_free(pstr);
-	talloc_free(route);
-	return 0;
-}
-
 static int bit_count(uint32_t i)
 {
 	int c = 0;
@@ -172,6 +118,145 @@ static int ipv4_mask_to_int(const char *prefix)
 	return mask2prefix(in);
 }
 
+/* parses 'str' as a decimal, non-negative integer with no leading/
+ * trailing garbage, in [0, max]; returns 0 and sets *out on success */
+static int parse_uint_full(const char *str, unsigned int max, unsigned int *out)
+{
+	char *end;
+	unsigned long val;
+
+	if (str == NULL || *str == 0)
+		return -1;
+
+	errno = 0;
+	val = strtoul(str, &end, 10);
+	if (errno != 0 || *end != 0 || val > max)
+		return -1;
+
+	*out = (unsigned int)val;
+	return 0;
+}
+
+/* Checks that a route is a well-formed IPv4 or IPv6 address plus
+ * prefix (xxx.xxx.xxx.xxx/prefix, xxx.xxx.xxx.xxx/xxx.xxx.xxx.xxx, or
+ * an IPv6 address/prefix), and returns a negative code for anything
+ * else, including any route containing characters that do not belong
+ * to a valid address or prefix.
+ *
+ * This is a security boundary, not just a format check: the validated
+ * route is later substituted into route-add-cmd/route-del-cmd and
+ * executed via a shell as root (see route_adddel() in route-add.c), so
+ * a route string that is not fully consumed by address/prefix parsing
+ * must be rejected rather than passed through or partially normalized.
+ *
+ * For IPv4, normalizes a numeric /prefix to the equivalent dotted
+ * netmask, matching this function's historical output format.
+ *
+ * The literal string "default" is accepted unchanged: it is not an
+ * address at all, but the documented keyword (sample.config, "route =
+ * default") that config.c recognizes as a request to route all client
+ * traffic through the VPN (config.c:2199-2200). It carries no shell
+ * metacharacters, so accepting it here does not reopen the injection
+ * this function guards against.
+ */
+int ip_route_sanity_check(void *pool, char **_route)
+{
+	char *route = *_route, *n;
+	char *slash_ptr, *addr_part, *prefix_part;
+	struct in_addr in4;
+	struct in6_addr in6;
+	unsigned int prefix;
+	int is_ipv6;
+
+	if (strcmp(route, "default") == 0)
+		return 0;
+
+	is_ipv6 = (strchr(route, ':') != NULL);
+
+	slash_ptr = strchr(route, '/');
+	if (slash_ptr == NULL) {
+		oc_syslog(LOG_ERR,
+			  "route '%s' is missing a /prefix, use address/prefix",
+			  route);
+		return -1;
+	}
+
+	addr_part = talloc_strndup(pool, route, slash_ptr - route);
+	if (addr_part == NULL)
+		return -1;
+	prefix_part = slash_ptr + 1;
+
+	if (is_ipv6) {
+		if (inet_pton(AF_INET6, addr_part, &in6) != 1) {
+			oc_syslog(LOG_ERR,
+				  "route '%s' has an invalid IPv6 address",
+				  route);
+			talloc_free(addr_part);
+			return -1;
+		}
+
+		if (parse_uint_full(prefix_part, 128, &prefix) < 0) {
+			oc_syslog(LOG_ERR,
+				  "route '%s' has an invalid IPv6 prefix",
+				  route);
+			talloc_free(addr_part);
+			return -1;
+		}
+
+		talloc_free(addr_part);
+		return 0;
+	}
+
+	if (inet_pton(AF_INET, addr_part, &in4) != 1) {
+		oc_syslog(LOG_ERR, "route '%s' has an invalid IPv4 address",
+			  route);
+		talloc_free(addr_part);
+		return -1;
+	}
+
+	if (strchr(prefix_part, '.') != NULL) {
+		/* dotted-netmask form; must be a valid, contiguous mask */
+		if (ipv4_mask_to_int(prefix_part) < 0) {
+			oc_syslog(LOG_ERR,
+				  "route '%s' has an invalid IPv4 netmask",
+				  route);
+			talloc_free(addr_part);
+			return -1;
+		}
+		talloc_free(addr_part);
+		return 0;
+	}
+
+	if (parse_uint_full(prefix_part, 32, &prefix) < 0) {
+		oc_syslog(LOG_ERR, "route '%s' has an invalid IPv4 prefix",
+			  route);
+		talloc_free(addr_part);
+		return -1;
+	}
+
+	if (prefix == 0) {
+		n = talloc_asprintf(pool, "%s/0.0.0.0", addr_part);
+	} else {
+		char *pstr = ipv4_prefix_to_strmask(pool, prefix);
+
+		if (pstr == NULL) {
+			talloc_free(addr_part);
+			return -1;
+		}
+		n = talloc_asprintf(pool, "%s/%s", addr_part, pstr);
+		talloc_free(pstr);
+	}
+	talloc_free(addr_part);
+	if (n == NULL) {
+		oc_syslog(LOG_ERR, "memory error");
+		return -1;
+	}
+
+	talloc_free(route);
+	*_route = n;
+	return 0;
+}
+
 /* Converts a route from xxx.xxx.xxx.xxx/xxx.xxx.xxx.xxx format, to
  * xxx.xxx.xxx.xxx/prefix format.
  */

tests/meson.build

@@ -48,6 +48,7 @@ unit_tests = {
   'ipv6-prefix':    {'src': ['ipv6-prefix.c'],   'args': ['-DUNDER_TEST'], 'timeout': 30},
   'proxyproto-v1':  {'src': ['proxyproto-v1.c'], 'args': [], 'timeout': 30},
   'proxyproto-v2':  {'src': ['proxyproto-v2.c'], 'args': [], 'timeout': 30},
+  'route-sanity-check': {'src': ['route-sanity-check.c'], 'args': ['-DUNDER_TEST'], 'timeout': 30},
 }
 
 foreach name, cfg : unit_tests

tests/route-sanity-check.c

@@ -0,0 +1,111 @@
+/*
+ * Copyright (C) 2026 Nikos Mavrogiannopoulos
+ *
+ * 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 Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+/* REQ-MAIN-SEC-008: ip_route_sanity_check() is the only validation a
+ * route string undergoes before it can be substituted into
+ * route-add-cmd/route-del-cmd and executed via `/bin/sh -c` as root
+ * (src/route-add.c:50). It must reject anything that is not a
+ * well-formed IPv4/IPv6 CIDR, not merely normalize IPv4 prefix syntax.
+ */
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <talloc.h>
+
+#include "../src/ip-util.h"
+#include "../src/ip-util.c"
+
+static void expect_reject(const char *route)
+{
+	void *pool = talloc_new(NULL);
+	char *r = talloc_strdup(pool, route);
+	int ret;
+
+	ret = ip_route_sanity_check(pool, &r);
+	if (ret >= 0) {
+		fprintf(stderr,
+			"error: route '%s' should have been REJECTED but was accepted as '%s'\n",
+			route, r);
+		exit(1);
+	}
+	talloc_free(pool);
+}
+
+static void expect_accept(const char *route, const char *expected)
+{
+	void *pool = talloc_new(NULL);
+	char *r = talloc_strdup(pool, route);
+	int ret;
+
+	ret = ip_route_sanity_check(pool, &r);
+	if (ret < 0) {
+		fprintf(stderr,
+			"error: route '%s' should have been ACCEPTED but was rejected\n",
+			route);
+		exit(1);
+	}
+	if (strcmp(r, expected) != 0) {
+		fprintf(stderr,
+			"error: route '%s' normalized to '%s', expected '%s'\n",
+			route, r, expected);
+		exit(1);
+	}
+	talloc_free(pool);
+}
+
+int main(void)
+{
+	/* --- malicious routes: MUST be rejected regardless of family --- */
+
+	/* dot-free routes (IPv6, or garbage) used to bypass all validation */
+	expect_reject("::/0; touch /tmp/x");
+	expect_reject("2001:db8::/32; touch /tmp/x");
+	expect_reject("not-a-route-at-all");
+	expect_reject("$(touch /tmp/x)");
+	expect_reject("`touch /tmp/x`");
+
+	/* dotted-netmask routes used to pass through unexamined */
+	expect_reject("1.2.3.4/5.6.7.8; touch /tmp/x");
+	expect_reject("10.0.0.0/255.255.255.0; touch /tmp/x");
+	expect_reject("10.0.0.0/255.255.255.0 && touch /tmp/x");
+
+	/* out-of-range / malformed prefixes */
+	expect_reject("10.0.0.0/33");
+	expect_reject("2001:db8::/129");
+	expect_reject("10.0.0.0/abc");
+	expect_reject("999.999.999.999/24");
+
+	/* --- legitimate routes: MUST still be accepted --- */
+	expect_accept("10.0.0.0/24", "10.0.0.0/255.255.255.0");
+	expect_accept("10.0.0.0/255.255.255.0", "10.0.0.0/255.255.255.0");
+	expect_accept("2001:db8::/32", "2001:db8::/32");
+	expect_accept("fd00::/8", "fd00::/8");
+	expect_accept("0.0.0.0/0", "0.0.0.0/0.0.0.0");
+	expect_accept("::/0", "::/0");
+
+	/* the documented "route = default" keyword (sample.config,
+	 * config.c:2199-2200) is not an address and MUST be accepted
+	 * unchanged, exact-match only (not as a prefix/substring) */
+	expect_accept("default", "default");
+	expect_reject("Default");
+	expect_reject("default/24");
+	expect_reject("default; touch /tmp/x");
+
+	printf("All route sanity check tests passed.\n");
+	return 0;
+}