From: Alan T. DeKok <aland@freeradius.org>
Date: Mon, 21 Mar 2022 15:26:30 +0000 (-0400)
Subject: allow "length=uint8" for structs
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=de529606e50a7f23fec791742a22baee0e075dab;p=thirdparty%2Ffreeradius-server.git

allow "length=uint8" for structs

and update encode / decode of struct

and ensure that the encoders don't add too many lengths
---

diff --git a/src/lib/util/dict_validate.c b/src/lib/util/dict_validate.c
index 03bd829b146..eef34c956c3 100644
--- a/src/lib/util/dict_validate.c
+++ b/src/lib/util/dict_validate.c
@@ -196,7 +196,7 @@ bool dict_attr_flags_valid(fr_dict_t *dict, fr_dict_attr_t const *parent,
 		case FR_TYPE_UINT32:
 		case FR_TYPE_UINT64:
 			if ((flags->subtype != FLAG_KEY_FIELD) && (flags->subtype != FLAG_BIT_FIELD)) {
-				fr_strerror_const("Invalid type for extra flag.");
+				fr_strerror_const("Invalid type (not 'key' field or 'bit' field) for extra flag.");
 				return false;
 			}
 
@@ -230,7 +230,7 @@ bool dict_attr_flags_valid(fr_dict_t *dict, fr_dict_attr_t const *parent,
 				if ((flags->subtype != FLAG_LENGTH_UINT8) && (flags->subtype != FLAG_LENGTH_UINT16)) goto invalid_extra;
 			} else if (flags->subtype) {
 			invalid_extra:
-				fr_strerror_const("Invalid type for extra flag.");
+				fr_strerror_const("Invalid type (not 'length=...') for extra flag.");
 				return false;
 			}
 
@@ -239,8 +239,8 @@ bool dict_attr_flags_valid(fr_dict_t *dict, fr_dict_attr_t const *parent,
 			break;
 
 		case FR_TYPE_STRUCT:
-			if (flags->subtype != FLAG_LENGTH_UINT16) {
-				fr_strerror_const("Invalid type for extra flag.");
+			if ((flags->subtype != FLAG_LENGTH_UINT8) && (flags->subtype != FLAG_LENGTH_UINT16)) {
+				fr_strerror_const("Invalid type (not 'length=...') for extra flag.");
 				return false;
 			}
 
diff --git a/src/lib/util/struct.c b/src/lib/util/struct.c
index 372351a6151..afe5eb2c896 100644
--- a/src/lib/util/struct.c
+++ b/src/lib/util/struct.c
@@ -110,26 +110,37 @@ ssize_t fr_struct_from_network(TALLOC_CTX *ctx, fr_pair_list_t *out,
 	 *	Decode structs with length prefixes.
 	 */
 	if (da_is_length_field(parent)) {
-		size_t struct_len;
+		size_t struct_len, need;
 
-		if ((end - p) < 2) {
+		if (parent->flags.subtype == FLAG_LENGTH_UINT8) {
+			need = 1;
+		} else {
+			need = 2;
+		}
+
+		if ((size_t) (end - p) < need) {
 			FR_PROTO_TRACE("Insufficient room for length header");
 			goto unknown;
 		}
 
-		struct_len = (p[0] << 8) | p[1];
-		if ((p + struct_len + 2) > end) {
+		struct_len = p[0];
+		if (need > 1) {
+			struct_len <<= 8;
+			struct_len |= p[1];
+		}
+
+		if ((p + struct_len + need) > end) {
 			FR_PROTO_TRACE("Length header is larger than remaining data");
 			goto unknown;
 		}
 
 		/*
-		 *	Skip the "length" field, and tell the decoder
-		 *	to stop at the end of the length field.
+		 *	Skip the length field, and tell the decoder to
+		 *	stop at the end of the data we're supposed to decode.
 		 */
-		p += 2;
+		p += need;
 		end = p + struct_len;
-		data_len = struct_len + 2;
+		data_len = struct_len + need;
 	}
 
 	while (p < end) {
@@ -532,7 +543,11 @@ ssize_t fr_struct_to_network(fr_dbuff_t *dbuff,
 	 *	Some structs are prefixed by a 16-bit length.
 	 */
 	if (da_is_length_field(parent)) {
-		FR_DBUFF_ADVANCE_RETURN(&work_dbuff, 2);
+		if (parent->flags.subtype == FLAG_LENGTH_UINT8) {
+			FR_DBUFF_ADVANCE_RETURN(&work_dbuff, 1);
+		} else {
+			FR_DBUFF_ADVANCE_RETURN(&work_dbuff, 2);
+		}
 		do_length = true;
 	}
 
@@ -778,12 +793,30 @@ done:
 	}
 
 	if (do_length) {
-		uint32_t len = fr_dbuff_used(&work_dbuff) - 2;
-		if (len > 65535) {
+		size_t need, max, len;
+
+		/*
+		 *	@todo - maybe just limit the size of the work_dbuff.
+		 */
+		if (parent->flags.subtype == FLAG_LENGTH_UINT8) {
+			need = 1;
+			max = 256;
+		} else {
+			need = 2;
+			max = 65536;
+		}
+
+		len = fr_dbuff_used(&work_dbuff) - need;
+		if (len > max) {
 			fr_strerror_const("Structure size is too large for 16-bit length field.");
 			return -1;
 		}
-		fr_dbuff_in(&hdr_dbuff, (uint16_t)len);
+
+		if (need == 1) {
+			fr_dbuff_in(&hdr_dbuff, (uint8_t)len);
+		} else {
+			fr_dbuff_in(&hdr_dbuff, (uint16_t)len);
+		}
 	}
 
 	/*
diff --git a/src/protocols/dhcpv4/encode.c b/src/protocols/dhcpv4/encode.c
index 2712dea6a6b..3bb6f2bc2aa 100644
--- a/src/protocols/dhcpv4/encode.c
+++ b/src/protocols/dhcpv4/encode.c
@@ -238,8 +238,10 @@ static ssize_t encode_array(fr_dbuff_t *dbuff,
 		/*
 		 *	If the data is variable length i.e. strings or octets
 		 *	we need to include an 8-bit length field before each element.
+		 *
+		 *	The struct encoder adds it's own length field.
 		 */
-		if (da_is_length_field(da)) {
+		if (da_is_length_field(da) && (da->type != FR_TYPE_STRUCT)) {
 			len_field = true;
 			FR_DBUFF_ADVANCE_RETURN(&element_dbuff, sizeof(uint8_t));	/* Make room for the length field */
 		}
diff --git a/src/protocols/dhcpv6/encode.c b/src/protocols/dhcpv6/encode.c
index 76fa0f27d59..f7acd815c35 100644
--- a/src/protocols/dhcpv6/encode.c
+++ b/src/protocols/dhcpv6/encode.c
@@ -435,8 +435,10 @@ static inline ssize_t encode_array(fr_dbuff_t *dbuff,
 		 *   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-...-+-+-+-+-+-+-+
 		 *   |       text-len                |        String                 |
 		 *   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-...-+-+-+-+-+-+-+
+		 *
+		 *	The struct encoder already adds the length field.
 		 */
-		if (da_is_length_field(da)) {
+		if (da_is_length_field(da) && (da->type != FR_TYPE_STRUCT)) {
 			len_field = true;
 			FR_DBUFF_ADVANCE_RETURN(&element_dbuff, sizeof(uint16_t));	/* Make room for the length field */
 		}