From: Arran Cudbard-Bell <a.cudbardb@freeradius.org>
Date: Sun, 4 Sep 2022 03:28:16 +0000 (-0400)
Subject: Fix unsafe dereferences of fr_sbuff_current
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=397ba2a3abe67d68efdb2cc7936b9950f654a196;p=thirdparty%2Ffreeradius-server.git

Fix unsafe dereferences of fr_sbuff_current

Add significantly simpler error handling logic
---

diff --git a/src/bin/radsniff.c b/src/bin/radsniff.c
index 248afffbd11..201c9ef5e58 100644
--- a/src/bin/radsniff.c
+++ b/src/bin/radsniff.c
@@ -375,7 +375,7 @@ static void rs_packet_print_csv(uint64_t count, rs_status_t status, fr_pcap_t *h
 
 		memset(fr_sbuff_current(&sbuff), ',', conf->list_da_num);
 		fr_sbuff_advance(&sbuff, conf->list_da_num);
-		*fr_sbuff_current(&sbuff) = '\0';
+		fr_sbuff_terminate(&sbuff);
 	}
 
 	fprintf(stdout , "%s\n", buffer);
diff --git a/src/bin/unit_test_attribute.c b/src/bin/unit_test_attribute.c
index c210e52396c..c578fd2f23d 100644
--- a/src/bin/unit_test_attribute.c
+++ b/src/bin/unit_test_attribute.c
@@ -3549,7 +3549,7 @@ static int line_ranges_parse(TALLOC_CTX *ctx, fr_dlist_head_t *out, fr_sbuff_t *
 			goto error;
 		}
 
-		switch (*fr_sbuff_current(in)) {
+		fr_sbuff_switch(in, '\0') {
 		/*
 		 *	More ranges...
 		 */
@@ -3847,7 +3847,7 @@ int main(int argc, char *argv[])
 			while (fr_sbuff_extend(&in)) {
 				fr_sbuff_adv_until(&in, SIZE_MAX, &dir_sep, '\0');
 
-				switch (*fr_sbuff_current(&in)) {
+				fr_sbuff_switch(&in, '\0') {
 				case '/':
 					fr_sbuff_set(&dir_end, &in);
 					fr_sbuff_advance(&in, 1);
diff --git a/src/lib/ldap/filter.c b/src/lib/ldap/filter.c
index 96ff71fec10..8bf4eeb409a 100644
--- a/src/lib/ldap/filter.c
+++ b/src/lib/ldap/filter.c
@@ -60,7 +60,7 @@ static fr_slen_t ldap_filter_parse_logic(ldap_filter_t *node, fr_sbuff_t *sbuff,
 	fr_slen_t	ret = 0;
 	fr_slen_t	parsed = 0;
 
-	switch(*fr_sbuff_current(sbuff)) {
+	fr_sbuff_switch(sbuff, '\0') {
 	case '&':
 		node->logic_op = LDAP_FILTER_LOGIC_AND;
 		node->orig = talloc_typed_strdup(node, "&");
diff --git a/src/lib/server/users_file.c b/src/lib/server/users_file.c
index fc957c8dc89..86d2420a76b 100644
--- a/src/lib/server/users_file.c
+++ b/src/lib/server/users_file.c
@@ -183,7 +183,7 @@ static int users_include(TALLOC_CTX *ctx, fr_dict_t const *dict, fr_sbuff_t *sbu
 	 *	Otherwise the $INCLUDE name is an absolute path, use
 	 *	it as -is.
 	 */
-	c = *fr_sbuff_current(&name);
+	c = fr_sbuff_char(&name, '\0');
 	if (c != '/') {
 		p = strrchr(file, '/');
 
diff --git a/src/lib/unlang/xlat_expr.c b/src/lib/unlang/xlat_expr.c
index e8e99ddad29..9bef82b66e9 100644
--- a/src/lib/unlang/xlat_expr.c
+++ b/src/lib/unlang/xlat_expr.c
@@ -1824,7 +1824,7 @@ static const int precedence[T_TOKEN_LAST] = {
 
 #define fr_sbuff_skip_whitespace(_x) \
 	do { \
-		while (isspace((int) *fr_sbuff_current(_x))) fr_sbuff_advance(_x, 1); \
+		while (isspace((int) fr_sbuff_char(_x, '\0'))) fr_sbuff_advance(_x, 1); \
 	} while (0)
 
 static ssize_t tokenize_expression(xlat_exp_head_t *head, xlat_exp_t **out, fr_sbuff_t *in,
diff --git a/src/lib/unlang/xlat_tokenize.c b/src/lib/unlang/xlat_tokenize.c
index b50fb005da9..60baa525424 100644
--- a/src/lib/unlang/xlat_tokenize.c
+++ b/src/lib/unlang/xlat_tokenize.c
@@ -817,13 +817,13 @@ int xlat_tokenize_expansion(xlat_exp_head_t *head, fr_sbuff_t *in,
 	 *
 	 *	e.g. '%{myfirstxlat'
 	 */
-	if (!fr_sbuff_remaining(in)) {
+	if (!fr_sbuff_extend(in)) {
 		fr_strerror_const("Missing closing brace");
 		fr_sbuff_marker_release(&s_m);
 		return -1;
 	}
 
-	hint = *fr_sbuff_current(in);
+	hint = fr_sbuff_char(in, '\0');
 
 	XLAT_DEBUG("EXPANSION HINT TOKEN '%c'", hint);
 	if (len == 0) {
diff --git a/src/lib/util/base32.c b/src/lib/util/base32.c
index 8da1f32d820..8a59aa18dec 100644
--- a/src/lib/util/base32.c
+++ b/src/lib/util/base32.c
@@ -359,7 +359,7 @@ ssize_t fr_base32_decode_nstd(fr_sbuff_parse_error_t *err, fr_dbuff_t *out, fr_s
 	/*
 	 *	Find the first non-base32 char
 	 */
-	while (fr_sbuff_extend(&our_in) && fr_is_base32_nstd(*fr_sbuff_current(&our_in), alphabet)) {
+	while (fr_sbuff_extend(&our_in) && fr_is_base32_nstd(fr_sbuff_char(&our_in, '\0'), alphabet)) {
 		fr_sbuff_advance(&our_in, 1);
 	}
 
@@ -439,7 +439,7 @@ ssize_t fr_base32_decode_nstd(fr_sbuff_parse_error_t *err, fr_dbuff_t *out, fr_s
 			}
 			if (!fr_sbuff_next_if_char(&our_in, '=')) {
 				fr_strerror_printf("Found non-padding char '%c' at end of base32 string",
-						   *fr_sbuff_current(&our_in));
+						   fr_sbuff_char(&our_in, '\0'));
 
 				if (err) *err = FR_SBUFF_PARSE_ERROR_FORMAT;
 
@@ -450,7 +450,7 @@ ssize_t fr_base32_decode_nstd(fr_sbuff_parse_error_t *err, fr_dbuff_t *out, fr_s
 
 	if (no_trailing && fr_sbuff_extend(&our_in)) {
 		fr_strerror_printf("Found trailing garbage '%c' at end of base32 string",
-				   *fr_sbuff_current(&our_in));
+				   fr_sbuff_char(&our_in, '\0'));
 
 		if (err) *err = FR_SBUFF_PARSE_ERROR_TRAILING;
 
diff --git a/src/lib/util/base64.c b/src/lib/util/base64.c
index 51c1146a0ab..15f68958491 100644
--- a/src/lib/util/base64.c
+++ b/src/lib/util/base64.c
@@ -438,7 +438,7 @@ ssize_t	fr_base64_decode_nstd(fr_sbuff_parse_error_t *err, fr_dbuff_t *out, fr_s
 	/*
 	 *	Find the first non-base64 char
 	 */
-	while (fr_sbuff_extend(&our_in) && fr_is_base64_nstd(*fr_sbuff_current(&our_in), alphabet)) {
+	while (fr_sbuff_extend(&our_in) && fr_is_base64_nstd(fr_sbuff_char(&our_in, '\0'), alphabet)) {
 		fr_sbuff_advance(&our_in, 1);
 	}
 
@@ -487,7 +487,7 @@ ssize_t	fr_base64_decode_nstd(fr_sbuff_parse_error_t *err, fr_dbuff_t *out, fr_s
 			}
 			if (!fr_sbuff_next_if_char(&our_in, '=')) {
 				fr_strerror_printf("Found non-padding char '%c' at end of base64 string",
-						   *fr_sbuff_current(&our_in));
+						   fr_sbuff_char(&our_in, '\0'));
 				goto bad_format;
 			}
 		}
@@ -495,7 +495,7 @@ ssize_t	fr_base64_decode_nstd(fr_sbuff_parse_error_t *err, fr_dbuff_t *out, fr_s
 
 	if (no_trailing && fr_sbuff_extend(&our_in)) {
 		fr_strerror_printf("Found trailing garbage '%c' at end of base64 string",
-				   *fr_sbuff_current(&our_in));
+				   fr_sbuff_char(&our_in, '\0'));
 
 		if (err) *err = FR_SBUFF_PARSE_ERROR_TRAILING;
 
diff --git a/src/lib/util/dict_util.c b/src/lib/util/dict_util.c
index dde5faa40c0..f98559cc930 100644
--- a/src/lib/util/dict_util.c
+++ b/src/lib/util/dict_util.c
@@ -2970,7 +2970,7 @@ ssize_t	fr_dict_enum_by_name_substr(fr_dict_enum_value_t **out, fr_dict_attr_t c
 		int len = (p - name) + 1;
 		fr_dict_enum_value_t *enumv;
 
-		*p = *fr_sbuff_current(&our_in);
+		*p = fr_sbuff_char(&our_in, '\0');
 		if (!fr_dict_enum_allowed_chars[*p]) {
 			break;
 		}
diff --git a/src/lib/util/sbuff.c b/src/lib/util/sbuff.c
index 76e389069de..be38511cb1d 100644
--- a/src/lib/util/sbuff.c
+++ b/src/lib/util/sbuff.c
@@ -1068,7 +1068,7 @@ fr_slen_t fr_sbuff_out_bool(bool *out, fr_sbuff_t *in)
 	};
 
 	if (fr_sbuff_is_in_charset(&our_in, bool_prefix)) {
-		switch (tolower(*fr_sbuff_current(&our_in))) {
+		switch (tolower(fr_sbuff_char(&our_in, '\0'))) {
 		default:
 			break;
 
diff --git a/src/lib/util/sbuff.h b/src/lib/util/sbuff.h
index 79aa855899d..fc2db1a06d7 100644
--- a/src/lib/util/sbuff.h
+++ b/src/lib/util/sbuff.h
@@ -88,6 +88,8 @@ struct fr_sbuff_s {
 		char *p;					//!< Mutable position pointer.
 	};
 
+	char const *err;				//!< Where the last error occurred.
+
 	uint8_t			is_const:1;		//!< Can't be modified.
 	uint8_t			adv_parent:1;		//!< If true, advance the parent.
 
@@ -895,14 +897,35 @@ static inline fr_sbuff_t *fr_sbuff_init_talloc(TALLOC_CTX *ctx,
 	((size_t)(fr_sbuff_start(_sbuff_or_marker) > fr_sbuff_current(_sbuff_or_marker) ? \
 		0 : (fr_sbuff_current(_sbuff_or_marker) - fr_sbuff_start(_sbuff_or_marker))))
 
+
+/** Sets an error marker in the parent
+ *
+ * If an error already exists at this level it will be used instead of the provided error.
+ *
+ * @param[in] sbuff	who's parent we'll set the error marker in.
+ * @param[in] err	marker to set.
+ * @return <0 the negative offset of the error.
+ */
+static inline fr_slen_t _fr_sbuff_error(fr_sbuff_t *sbuff, char const *err)
+{
+	fr_sbuff_t *parent = sbuff->parent;
+
+	if (sbuff->err) err = sbuff->err;
+	if (parent) parent->err = err;
+
+	return -((err - fr_sbuff_start(sbuff)) + 1);
+}
+
 /** Return the current position as an error marker
+ *
+ * @param[in] _sbuff_or_marker		Error marker will be set from the current position of this sbuff.
  *
  * +1 is added to the position to disambiguate with 0 meaning "parsed no data".
  *
  * An error at offset 0 will be returned as -1.
  */
 #define fr_sbuff_error(_sbuff_or_marker) \
-	(-(fr_sbuff_used(_sbuff_or_marker) + 1))
+	_fr_sbuff_error(fr_sbuff_ptr(_sbuff_or_marker), fr_sbuff_current(_sbuff_or_marker));
 
 /** Like fr_sbuff_used, but adjusts for the value returned for the amount shifted
  *
@@ -1045,6 +1068,8 @@ do { \
 static inline void _fr_sbuff_set_recurse(fr_sbuff_t *sbuff, char const *p)
 {
 	sbuff->p_i = p;
+	sbuff->err = NULL;	/* Modifying the position of the sbuff clears the error */
+
 	if (sbuff->adv_parent && sbuff->parent) _fr_sbuff_set_recurse(sbuff->parent, p);
 }
 
@@ -1056,6 +1081,7 @@ static inline ssize_t _fr_sbuff_marker_set(fr_sbuff_marker_t *m, char const *p)
 	if (unlikely(p > sbuff->end)) return -(p - sbuff->end);
 	if (unlikely(p < sbuff->start)) return 0;
 
+	sbuff->err = NULL;	/* Modifying the position of any markers clears the error, unsure if this is correct? */
 	m->p_i = p;
 
 	return p - current;
diff --git a/src/lib/util/size.c b/src/lib/util/size.c
index 7e6a5b62a7e..f8fd3ee2f5f 100644
--- a/src/lib/util/size.c
+++ b/src/lib/util/size.c
@@ -68,7 +68,7 @@ fr_slen_t fr_size_from_str(size_t *out, fr_sbuff_t *in)
 	if (fr_sbuff_out(NULL, &size, &our_in) < 0) return fr_sbuff_error(&our_in);
 	if (!fr_sbuff_extend(&our_in)) goto done;
 
-	c = tolower(*fr_sbuff_current(&our_in));
+	c = tolower(fr_sbuff_char(&our_in, '\0'));
 
 	/*
 	 *	Special cases first...
diff --git a/src/lib/util/uri.c b/src/lib/util/uri.c
index 3cd3ea08d1d..7ed1df776e5 100644
--- a/src/lib/util/uri.c
+++ b/src/lib/util/uri.c
@@ -43,7 +43,6 @@ int fr_uri_escape(fr_value_box_list_t *uri, fr_uri_part_t const *uri_parts, void
 	fr_value_box_t		*uri_vb = NULL;
 	fr_uri_part_t const	*uri_part;
 	fr_sbuff_t		sbuff;
-	char const		*p;
 
 	uri_part = uri_parts;
 
@@ -92,12 +91,11 @@ int fr_uri_escape(fr_value_box_list_t *uri, fr_uri_part_t const *uri_parts, void
 
 		do {
 			fr_sbuff_adv_until(&sbuff, SIZE_MAX, uri_part->terminals, '\0');
-			p = fr_sbuff_current(&sbuff);
 
 			/*
 			 *	We've not found a terminal in the current box
 			 */
-			if (uri_part->part_adv[(uint8_t)*p] == 0) continue;
+			if (uri_part->part_adv[fr_sbuff_char(&sbuff, '\0')] == 0) continue;
 
 			/*
 			 *	This terminator has trailing characters to skip
@@ -107,7 +105,7 @@ int fr_uri_escape(fr_value_box_list_t *uri, fr_uri_part_t const *uri_parts, void
 			/*
 			 *	Move to the next part
 			 */
-			uri_part += uri_part->part_adv[(uint8_t)*p];
+			uri_part += uri_part->part_adv[fr_sbuff_char(&sbuff, '\0')];
 			if (!uri_part->terminals) break;
 		} while (fr_sbuff_advance(&sbuff, 1) > 0);
 	}