From: Alan T. DeKok <aland@freeradius.org>
Date: Wed, 24 Mar 2021 18:19:56 +0000 (-0400)
Subject: cleanups and comments for future work
X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=7086275c2b7e34a352ed28d4e40e52ab3330e282;p=thirdparty%2Ffreeradius-server.git

cleanups and comments for future work
---

diff --git a/src/lib/util/dict.h b/src/lib/util/dict.h
index 4b4f0dd42c0..dcbe2788578 100644
--- a/src/lib/util/dict.h
+++ b/src/lib/util/dict.h
@@ -87,23 +87,39 @@ typedef struct {
 
 	unsigned int		virtual : 1;			//!< for dynamic expansion
 
+	/*
+	 *	@todo - if we want to clean these fields up, make
+	 *	"subtype" and "type_size" both 4-bit bitfields.  That
+	 *	gives us an extra 8 bits for adding new flags, and we
+	 *	can likely get rid of "extra", in order to save one
+	 *	more bit.
+	 */
 	unsigned int		extra : 1;			//!< really "subtype is used by dict, not by protocol"
 
+	/*
+	 *	main: extra is set, then this field is is key, bit, or a uint16 length field.
+	 *	radius: is one of 9 options for flags
+	 *	dhcp v4/v6: DNS label, or partial DNS label
+	 */
 	uint8_t			subtype;			//!< protocol-specific values, OR key fields
 
+	/*
+	 *	Length in bytes for most attributes.
+	 *	Length in bits for da_is_bit_field(da)
+	 */
 	uint8_t			length;				//!< length of the attribute
 
 	/*
 	 *	TLVs: 1, 2, or 4.
 	 *	date / time types: fr_time_res_t, which has 4 possible values.
-	 *	bit fields: offset in the byte where this bit field ends.
+	 *	bit fields: offset in the byte where this bit field ends, which is only
+	 *  	used as a caching mechanism during parsing of the dictionaries.
 	 */
 	uint8_t			type_size;			//!< For TLV2 and root attributes.
 } fr_dict_attr_flags_t;
 
 #define flag_time_res type_size
 #define flag_byte_offset type_size
-#define flag_struct_offset type_size
 
 /** subtype values for the dictionary when extra=1
  *
diff --git a/src/lib/util/dict_tokenize.c b/src/lib/util/dict_tokenize.c
index dda9b950933..713d44d11c8 100644
--- a/src/lib/util/dict_tokenize.c
+++ b/src/lib/util/dict_tokenize.c
@@ -203,7 +203,10 @@ static int dict_process_type_field(dict_tokenize_ctx_t *ctx, char const *name, f
 			return -1;
 		}
 
-		if ((length == 0) || (length > 253)) {
+		/*
+		 *	"length" has to fit into a uint8_t field.
+		 */
+		if ((length == 0) || (length > 255)) {
 			fr_strerror_printf("Invalid length for '%s[...]'", name);
 			return -1;
 		}
@@ -245,7 +248,11 @@ static int dict_process_type_field(dict_tokenize_ctx_t *ctx, char const *name, f
 			}
 
 			/*
-			 *	We track where on a byte boundary this bit field ends.
+			 *	Cache where on a byte boundary this
+			 *	bit field ends.  We could have the
+			 *	validation function loop through all
+			 *	previous siblings, but that's
+			 *	annoying.
 			 */
 			flags->flag_byte_offset = length;
 
@@ -948,8 +955,7 @@ static int dict_read_process_member(dict_tokenize_ctx_t *ctx, char **argv, int a
 			 *	located.
 			 */
 			if (flags.extra && (flags.subtype == FLAG_BIT_FIELD)) {
-				flags.flag_byte_offset += previous->flags.flag_byte_offset;
-				flags.flag_byte_offset &= 0x07;
+				flags.flag_byte_offset = (flags.length + previous->flags.flag_byte_offset) & 0x07;
 
 			} else {
 				if (previous->flags.flag_byte_offset != 0) {
diff --git a/src/protocols/radius/base.c b/src/protocols/radius/base.c
index 9765f1774f2..9006d679b02 100644
--- a/src/protocols/radius/base.c
+++ b/src/protocols/radius/base.c
@@ -1138,6 +1138,11 @@ static bool attr_valid(UNUSED fr_dict_t *dict, fr_dict_attr_t const *parent,
 		return false;
 	}
 
+	if (flags->length > 253) {
+		fr_strerror_printf("Attributes cannot be more than 253 octets in length");
+		return false;
+	}
+
 	if (flags->subtype > FLAG_ENCRYPT_ASCEND_SECRET) {
 		fr_strerror_printf("Invalid flag value %u", flags->subtype);
 		return false;