From: Alan T. DeKok <aland@freeradius.org>
Date: Mon, 2 Sep 2024 12:22:36 +0000 (-0400)
Subject: don't let the user delete attributes we're looping over
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=e2a12dcbac4d7651f8102447696db50558d82e5a;p=thirdparty%2Ffreeradius-server.git

don't let the user delete attributes we're looping over

this isn't perfect, but it's good enough.

It can still crash if the user has one "foreach" loop nested
inside of another one, AND both loop over the same attributes,
AND a variable is deleted after the inner "foreach" loop is run.

The general answer then is "don't do that".

We could fix this by using the old way of copying all of the VPs,
looping over the copy, and then moving the copy back in place of
the original VPs.  But that process is expensive, and almost always
not needed.

For now, it's simpler to just say "don't modify the attributes
you're looping over"
---

diff --git a/src/lib/unlang/foreach.c b/src/lib/unlang/foreach.c
index 623a63bab6e..8f42d6e603e 100644
--- a/src/lib/unlang/foreach.c
+++ b/src/lib/unlang/foreach.c
@@ -52,6 +52,7 @@ typedef struct {
 	request_t		*request;			//!< The current request.
 	fr_dcursor_t		cursor;				//!< Used to track our place in the list
 	fr_pair_t		*key;				//!< local variable which contains the key
+	tmpl_t const		*vpt;				//!< pointer to the vpt
 
 	tmpl_dcursor_ctx_t	cc;				//!< tmpl cursor state
 
@@ -76,7 +77,22 @@ static xlat_action_t unlang_foreach_xlat(TALLOC_CTX *ctx, fr_dcursor_t *out,
 static int _free_unlang_frame_state_foreach(unlang_frame_state_foreach_t *state)
 {
 	if (state->key) {
+		fr_pair_t *vp;
+
+		tmpl_dcursor_clear(&state->cc);
+
+		/*
+		 *	Now that we're done, the leaf entries can be changed again.
+		 */
+		vp = tmpl_dcursor_init(NULL, NULL, &state->cc, &state->cursor, state->request, state->vpt);
+		fr_assert(vp != NULL);
+
+		do {
+			if (fr_type_is_leaf(vp->vp_type)) fr_pair_clear_immutable(vp);
+		
+		} while ((vp = fr_dcursor_next(&state->cursor)) != NULL);
 		tmpl_dcursor_clear(&state->cc);
+
 	} else {
 		request_data_get(state->request, FOREACH_REQUEST_DATA, state->depth);
 	}
@@ -138,6 +154,11 @@ static unlang_action_t unlang_foreach_next(rlm_rcode_t *p_result, request_t *req
 	vp = fr_dcursor_current(&state->cursor);
 	fr_assert(vp != NULL);
 
+	/*
+	 *	If we modified the value, copy it back to the original pair.  Note that the copy does NOT
+	 *	check the "immutable" flag.  That flag is for the people using unlang, not for the
+	 *	interpreter.
+	 */
 	if (!fr_type_is_structural(vp->vp_type) && (vp->vp_type == state->key->vp_type)) {
 		fr_value_box_clear_value(&vp->data);
 		(void) fr_value_box_copy(vp, &vp->data, &state->key->data);
@@ -213,6 +234,8 @@ static unlang_action_t unlang_foreach(rlm_rcode_t *p_result, request_t *request,
 	 *	We have a key variable, let's use that.
 	 */
 	if (gext->key) {
+		state->vpt = gext->vpt;
+
 		/*
 		 *	No matching attributes, we can't do anything.
 		 */
@@ -222,6 +245,19 @@ static unlang_action_t unlang_foreach(rlm_rcode_t *p_result, request_t *request,
 			return UNLANG_ACTION_CALCULATE_RESULT;
 		}
 
+		/*
+		 *	Before we loop over the variables, ensure that the user can't pull the rug out from
+		 *	under us.
+		 */
+		do {
+			if (fr_type_is_leaf(vp->vp_type)) fr_pair_set_immutable(vp);
+		
+		} while ((vp = fr_dcursor_next(&state->cursor)) != NULL);
+		tmpl_dcursor_clear(&state->cc);
+
+		vp = tmpl_dcursor_init(NULL, NULL, &state->cc, &state->cursor, request, gext->vpt);
+		fr_assert(vp != NULL);
+
 		/*
 		 *	Create the local variable and populate its value.
 		 */
diff --git a/src/lib/util/pair.h b/src/lib/util/pair.h
index 0a9e98d212b..776799edd49 100644
--- a/src/lib/util/pair.h
+++ b/src/lib/util/pair.h
@@ -686,9 +686,19 @@ bool		fr_pair_immutable(fr_pair_t const *vp) CC_HINT(nonnull);
 static inline CC_HINT(nonnull, always_inline)
 void fr_pair_set_immutable(fr_pair_t *vp)
 {
+	fr_assert(fr_type_is_leaf(vp->vp_type));
+
 	fr_value_box_set_immutable(&vp->data);
 }
 
+static inline CC_HINT(nonnull, always_inline)
+void fr_pair_clear_immutable(fr_pair_t *vp)
+{
+	fr_assert(fr_type_is_leaf(vp->vp_type));
+
+	fr_value_box_clear_immutable(&vp->data);
+}
+
 
 /* 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 d2901a73352..26e9c64673d 100644
--- a/src/lib/util/value.h
+++ b/src/lib/util/value.h
@@ -1075,6 +1075,12 @@ void fr_value_box_set_immutable(fr_value_box_t *box)
 	box->immutable = true;
 }
 
+static inline CC_HINT(nonnull, always_inline)
+void fr_value_box_clear_immutable(fr_value_box_t *box)
+{
+	box->immutable = false;
+}
+
 
 /** @name Assign and manipulate binary-unsafe C strings
  *
diff --git a/src/tests/keywords/foreach-delete b/src/tests/keywords/foreach-delete
new file mode 100644
index 00000000000..fec364436ed
--- /dev/null
+++ b/src/tests/keywords/foreach-delete
@@ -0,0 +1,32 @@
+#
+#  PRE: foreach-modify
+#
+
+&Tmp-Integer-0 := { 1, 3, 5, 11 }
+
+#
+#  Try to delete one of the variables we're looping over.  If the user could delete one,
+#  then the underlying tmpl_dcursor would crash, as it didn't know about the deletion.
+#
+foreach uint32 self (&Tmp-Integer-0) {
+	&request -= &Tmp-Integer-0[1]
+	&self += 19
+}
+
+if (&Tmp-Integer-0[0] != 20) {
+	test_fail
+}
+
+if (&Tmp-Integer-0[1] != 22) {
+	test_fail
+}
+
+if (&Tmp-Integer-0[2] != 24) {
+	test_fail
+}
+
+if (&Tmp-Integer-0[3] != 30) {
+	test_fail
+}
+
+success