From: Alan T. DeKok <aland@freeradius.org>
Date: Sun, 20 Aug 2023 14:33:52 +0000 (-0400)
Subject: add immutable flag, and check it most places
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=79b81d33a45981e67892fedca3fc304d4063b273;p=thirdparty%2Ffreeradius-server.git

add immutable flag, and check it most places

the main purpose of immutable flags is to prevent users from
modifying values that the server wants to keep around.

As a result, the main checks for immutable values are in the
various editing routines:

    src/lib/util/calc.c	- cannot store results to immutable leaves
    src/lib/util/edit.c - cannot edit immutable values
			  or delete immutable leaves from lists
    src/lib/unlang/edit.c - cannot store to immutable leaves

Many of the internal value box / pair APIs will ignore the
immutable flag, and happily over-write values.  This behavior
is likely good enough for now.  The intention is to stop admins
from doing stupid things, and not to prevent the internal code
from doing what makes sense.

There is currently no flag in structural VPs which says "contains
an immutable child".  The edit code therefore has to check each
time by walking the list recursively.  That's fine for now.

There is as yet no code to set the immutable flag, or unit tests.
The goal is to have the decoders set the immutable flag as necessary,
which means that they don't need to save / restore attributes with
special meaning.
---

diff --git a/src/lib/unlang/edit.c b/src/lib/unlang/edit.c
index ab2f0def483..d833c347d00 100644
--- a/src/lib/unlang/edit.c
+++ b/src/lib/unlang/edit.c
@@ -1336,6 +1336,11 @@ static int check_lhs(request_t *request, unlang_frame_state_edit_t *state, edit_
 		// &control := ...
 	}
 
+	if (fr_pair_immutable(vp)) {
+		RWDEBUG("Cannot modify immutable value for %s", current->lhs.vpt->name);
+		return -1;
+	}
+
 	/*
 	 *	We found an existing attribute, with a modification operator.
 	 */
diff --git a/src/lib/util/calc.c b/src/lib/util/calc.c
index ef2a939a747..b413a178d4a 100644
--- a/src/lib/util/calc.c
+++ b/src/lib/util/calc.c
@@ -2119,6 +2119,11 @@ int fr_value_calc_assignment_op(TALLOC_CTX *ctx, fr_value_box_t *dst, fr_token_t
 	if (!fr_type_is_leaf(dst->type)) return invalid_type(dst->type);
 	if (!fr_type_is_leaf(src->type)) return invalid_type(src->type);
 
+	if (dst->immutable) {
+		fr_strerror_printf("Cannot modify immutable value");
+		return -1;
+	}
+
 	/*
 	 *	These operators are included here for testing and completeness.  But see comments in
 	 *	fr_edit_list_apply_pair_assignment() for what the caller should be doing.
@@ -2175,6 +2180,11 @@ int fr_value_calc_unary_op(TALLOC_CTX *ctx, fr_value_box_t *dst, fr_token_t op,
 
 	if (!fr_type_is_numeric(src->type)) return invalid_type(src->type);
 
+	if (dst->immutable) {
+		fr_strerror_printf("Cannot modify immutable value");
+		return -1;
+	}
+
 	if (op == T_OP_INCRM) {
 		/*
 		 *	Add 1 or subtract 1 means RHS is always 1.
diff --git a/src/lib/util/edit.c b/src/lib/util/edit.c
index 527b0ac8c7c..5959c03cc72 100644
--- a/src/lib/util/edit.c
+++ b/src/lib/util/edit.c
@@ -265,6 +265,11 @@ static int edit_record(fr_edit_list_t *el, fr_edit_op_t op, fr_pair_t *vp, fr_pa
 		case FR_EDIT_CLEAR:
 			if (!fr_type_is_structural(vp->vp_type)) return 0;
 
+			if (fr_pair_immutable(vp)) {
+				fr_strerror_printf("Cannot modify immutable value for %s", vp->da->name);
+				return -1;
+			}
+
 			fr_pair_list_free(&vp->vp_group);
 			return 0;
 
@@ -542,6 +547,11 @@ int fr_edit_list_insert_pair_after(fr_edit_list_t *el, fr_pair_list_t *list, fr_
  */
 int fr_edit_list_pair_delete(fr_edit_list_t *el, fr_pair_list_t *list, fr_pair_t *vp)
 {
+	if (fr_pair_immutable(vp)) {
+		fr_strerror_printf("Cannot modify immutable value for %s", vp->da->name);
+		return -1;
+	}
+
 	if (!el) {
 		fr_pair_delete(list, vp);
 		return 0;
@@ -565,6 +575,8 @@ int fr_edit_list_pair_delete_by_da(fr_edit_list_t *el, fr_pair_list_t *list, fr_
 	 *	Delete all VPs with a matching da.
 	 */
 	fr_pair_list_foreach(list, vp) {
+		if (fr_pair_immutable(vp)) continue;
+
 		if (vp->da != da) continue;
 
 		(void) fr_pair_remove(list, vp);
@@ -597,6 +609,11 @@ int fr_edit_list_replace_pair_value(fr_edit_list_t *el, fr_pair_t *vp, fr_value_
 {
 	if (!fr_type_is_leaf(vp->vp_type)) return -1;
 
+	if (vp->data.immutable) {
+		fr_strerror_printf("Cannot modify immutable value for %s", vp->da->name);
+		return -1;
+	}
+
 	if (el && (edit_record(el, FR_EDIT_VALUE, vp, NULL, NULL) < 0)) return -1;
 
 	if (!fr_type_is_fixed_size(vp->vp_type)) fr_value_box_clear(&vp->data);
@@ -615,6 +632,11 @@ int fr_edit_list_replace_pair(fr_edit_list_t *el, fr_pair_list_t *list, fr_pair_
 {
 	if (to_replace->da != vp->da) return -1;
 
+	if (fr_pair_immutable(to_replace)) {
+		fr_strerror_printf("Cannot modify immutable value for %s", vp->da->name);
+		return -1;
+	}
+
 	if (!el) {
 		if (fr_pair_insert_after(list, to_replace, vp) < 0) return -1;
 		fr_pair_delete(list, to_replace);
diff --git a/src/lib/util/pair.c b/src/lib/util/pair.c
index 89e70e12e6b..fa54dffa8df 100644
--- a/src/lib/util/pair.c
+++ b/src/lib/util/pair.c
@@ -1624,6 +1624,8 @@ int fr_pair_delete_by_da(fr_pair_list_t *list, fr_dict_attr_t const *da)
 
 	fr_pair_list_foreach(list, vp) {
 		if (da == vp->da) {
+			if (fr_pair_immutable(vp)) continue;
+
 			cnt++;
 			fr_pair_delete(list, vp);
 		}
@@ -2084,6 +2086,30 @@ bool fr_pair_validate_relaxed(fr_pair_t const *failed[2], fr_pair_list_t *filter
 	return true;
 }
 
+/**
+ *
+ * @param[in] vp	the pair to check
+ * @return
+ *	- true		the pair is immutable, or has an immutable child
+ *	- false		the pair is not immutable, or has no immutable children.
+ */
+bool fr_pair_immutable(fr_pair_t const *vp)
+{
+	if (fr_type_is_leaf(vp->vp_type)) return vp->data.immutable;
+
+	fr_pair_list_foreach(&vp->vp_group, child) {
+		if (fr_type_is_leaf(child->vp_type)) {
+			if (child->data.immutable) return true;
+
+			continue;
+		}
+
+		if (fr_pair_immutable(child)) return true;
+	}
+
+	return false;
+}
+
 /** Steal a list of pairs to a new context
  *
  */
diff --git a/src/lib/util/pair.h b/src/lib/util/pair.h
index 22ceec8014a..08c504145fd 100644
--- a/src/lib/util/pair.h
+++ b/src/lib/util/pair.h
@@ -628,6 +628,9 @@ bool		fr_pair_validate(fr_pair_t const *failed[2], fr_pair_list_t *filter,
 bool 		fr_pair_validate_relaxed(fr_pair_t const *failed[2], fr_pair_list_t *filter,
 					 fr_pair_list_t *list) CC_HINT(nonnull(2,3));
 
+bool		fr_pair_immutable(fr_pair_t const *vp) CC_HINT(nonnull);
+
+
 /* Lists */
 int		fr_pair_list_copy(TALLOC_CTX *ctx, fr_pair_list_t *to, fr_pair_list_t const *from);
 
diff --git a/src/lib/util/value.h b/src/lib/util/value.h
index da727fca499..02b8fa5459b 100644
--- a/src/lib/util/value.h
+++ b/src/lib/util/value.h
@@ -155,8 +155,11 @@ struct value_box_s {
 	/** Type and flags should appear together for packing efficiency
 	 */
 	fr_type_t		_CONST		type;			//!< Type of this value-box, at the start, see pair.h
+
 	unsigned int   				tainted : 1;		//!< i.e. did it come from an untrusted source
 	unsigned int   				secret : 1;		//!< Same as #fr_dict_attr_flags_t secret
+	unsigned int				immutable : 1;		//!< once set, the value cannot be changed
+
 	uint16_t		_CONST		safe;			//!< more detailed safety
 
 	fr_value_box_entry_t			entry;			//!< Doubly linked list entry.
@@ -466,6 +469,7 @@ void fr_value_box_init(fr_value_box_t *vb, fr_type_t type, fr_dict_attr_t const
 			.enumv = enumv,
 			.tainted = tainted,
 			.secret = enumv && enumv->flags.secret,
+			/* don't set the immutable flag.  The caller has to do it once he's finished editing the values */
 		}, sizeof(*vb));
 	fr_value_box_list_entry_init(vb);
 
@@ -980,6 +984,12 @@ void fr_value_box_set_secret(fr_value_box_t *box, bool secret)
 	box->secret = secret;
 }
 
+static inline CC_HINT(nonnull, always_inline)
+void fr_value_box_set_immutable(fr_value_box_t *box)
+{
+	box->immutable = true;
+}
+
 
 /** @name Assign and manipulate binary-unsafe C strings
  *