From: Alan T. DeKok <aland@freeradius.org>
Date: Sun, 13 Apr 2025 15:17:03 +0000 (-0400)
Subject: clean up attr_filter and map_to_vp()
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=856cf46ab9fd87ebd341b1af4dc67111b0a2a748;p=thirdparty%2Ffreeradius-server.git

clean up attr_filter and map_to_vp()

only attr_filter passes comparison operators to map_to_vp()
we should arguably forbid comparison operators, as allowing them
could break other modules.

Also disallow comparison operators in map_to_request, as nothing
uses them.
---

diff --git a/src/lib/server/map.c b/src/lib/server/map.c
index faf3985b168..c09e1e9529e 100644
--- a/src/lib/server/map.c
+++ b/src/lib/server/map.c
@@ -1581,21 +1581,6 @@ int map_to_vp(TALLOC_CTX *ctx, fr_pair_list_t *out, request_t *request, map_t co
 
 	fr_assert(tmpl_is_attr(map->lhs));
 
-	/*
-	 *	Special case for !*, we don't need to parse RHS as this is a unary operator.
-	 */
-	if (map->op == T_OP_CMP_FALSE) return 0;
-
-	/*
-	 *	Hoist this early, too.
-	 */
-	if (map->op == T_OP_CMP_TRUE) {
-		MEM(n = fr_pair_afrom_da(ctx, tmpl_attr_tail_da(map->lhs)));
-		n->op = map->op;
-		fr_pair_append(out, n);
-		return 0;
-	}
-
 	/*
 	 *	If there's no RHS, then it MUST be an attribute, and
 	 *	it MUST be structural.  And it MAY have children.
@@ -1880,6 +1865,14 @@ int map_to_request(request_t *request, map_t const *map, radius_map_getvalue_t f
 	fr_assert(map->lhs != NULL);
 	fr_assert(map->rhs != NULL);
 
+	/*
+	 *	This function is called only when creating attributes.  It cannot be called for conditions.
+	 */
+	if (fr_comparison_op[map->op]) {
+		REDEBUG("Invalid operator in %s %s ...", map->lhs->name, fr_tokens[map->op]);
+		return -1;
+	}
+
 	tmp_ctx = talloc_pool(request, 1024);
 
 	/*
diff --git a/src/modules/rlm_attr_filter/rlm_attr_filter.c b/src/modules/rlm_attr_filter/rlm_attr_filter.c
index 7c49e15b463..5894af5856d 100644
--- a/src/modules/rlm_attr_filter/rlm_attr_filter.c
+++ b/src/modules/rlm_attr_filter/rlm_attr_filter.c
@@ -241,6 +241,34 @@ static unlang_action_t CC_HINT(nonnull) attr_filter_common(TALLOC_CTX *ctx, rlm_
 		fr_pair_list_init(&check_list);
 
 		while ((map = map_list_next(&pl->reply, map))) {
+			/*
+			 *	Special case for !*
+			 */
+			if (map->op == T_OP_CMP_FALSE) {
+				RWDEBUG("Unsupported operator '!*'");
+				continue;
+			}
+
+			/*
+			 *	Create an empty attribute.  This functionality is used by the attr_filter module.
+			 */
+			if (map->op == T_OP_CMP_TRUE) {
+				fr_pair_t *vp;
+
+				MEM(vp = fr_pair_afrom_da(ctx, tmpl_attr_tail_da(map->lhs)));
+				vp->op = T_OP_CMP_TRUE;
+				fr_pair_append(&check_list, vp);
+				continue;
+			}
+
+			/*
+			 *	@todo - this use of map_to_vp is completely wrong.  Nothing else uses
+			 *	comparison operators for map_to_vp().  This module doesn't handle nested
+			 *	pairs, and dumps all pairs in one list no matter their depth.  It requires
+			 *	that map_to_vp() appends the pair, rather than respecting the operator.
+			 *
+			 *	i.e. it really only works for RADIUS. :(
+			 */
 			if (map_to_vp(ctx, &tmp_list, request, map, NULL) < 0) {
 				RPWARN("Failed parsing map %s for check item, skipping it", map->lhs->name);
 				continue;
diff --git a/src/modules/rlm_cache/rlm_cache.c b/src/modules/rlm_cache/rlm_cache.c
index 335a06c0ea0..3cd11d41d96 100644
--- a/src/modules/rlm_cache/rlm_cache.c
+++ b/src/modules/rlm_cache/rlm_cache.c
@@ -1412,6 +1412,11 @@ static int cache_verify(map_t *map, void *uctx)
 		return -1;
 	}
 
+	if (!fr_assignment_op[map->op]) {
+		cf_log_err(map->ci, "Invalid operator '%s'", fr_tokens[map->op]);
+		return -1;
+	}
+
 	return 0;
 }