From: Alan T. DeKok <aland@freeradius.org>
Date: Tue, 19 Apr 2022 21:58:56 +0000 (-0400)
Subject: add verify_tlvs() function
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=603815b3a37489341591c7997cbc4ae194d38ecb;p=thirdparty%2Ffreeradius-server.git

add verify_tlvs() function

so we don't go crazy creating structures, only to throw them
away when we discover that something bad is going on
---

diff --git a/src/lib/util/decode.c b/src/lib/util/decode.c
index dc9d505f136..14b175b69cb 100644
--- a/src/lib/util/decode.c
+++ b/src/lib/util/decode.c
@@ -123,6 +123,7 @@ ssize_t fr_pair_raw_from_network(TALLOC_CTX *ctx, fr_pair_list_t *out, fr_dict_a
  * @param[in] data_len		of data to parse.
  * @param[in] decode_ctx	passed to decode_tlv
  * @param[in] decode_tlv	function to decode one attribute / option / tlv
+ * @param[in] verify_tlvs	simple function to see if the TLVs are even vaguely well-formed
  * @param[in] nested		whether or not we create nested VPs.
  *	<0 on error - decode error, or OOM
  *	data_len on success
@@ -138,7 +139,9 @@ ssize_t fr_pair_raw_from_network(TALLOC_CTX *ctx, fr_pair_list_t *out, fr_dict_a
 ssize_t fr_pair_tlvs_from_network(TALLOC_CTX *ctx, fr_pair_list_t *out,
 				  fr_dict_attr_t const *parent,
 				  uint8_t const *data, size_t const data_len,
-				  void *decode_ctx, fr_pair_decode_value_t decode_tlv, bool nested)
+				  void *decode_ctx, fr_pair_decode_value_t decode_tlv,
+				  fr_pair_tlvs_verify_t verify_tlvs,
+				  bool nested)
 {
 	uint8_t const *p, *end;
 	fr_pair_list_t tlvs, *list;
@@ -150,6 +153,12 @@ ssize_t fr_pair_tlvs_from_network(TALLOC_CTX *ctx, fr_pair_list_t *out,
 	if (!fr_cond_assert_msg((parent->type == FR_TYPE_TLV || (parent->type == FR_TYPE_VENDOR)),
 				"%s: Internal sanity check failed, attribute \"%s\" is not of type 'tlv'",
 				__FUNCTION__, parent->name)) return PAIR_DECODE_FATAL_ERROR;
+
+	/*
+	 *	Do a quick sanity check to see if the TLVs are at all OK.
+	 */
+	if (verify_tlvs && !verify_tlvs(data, data_len)) return fr_pair_raw_from_network(ctx, out, parent, data, data_len);
+
 	p = data;
 	end = data + data_len;
 
@@ -168,7 +177,8 @@ ssize_t fr_pair_tlvs_from_network(TALLOC_CTX *ctx, fr_pair_list_t *out,
 		ssize_t slen;
 
 		slen = decode_tlv(child_ctx, list, parent, p, (end - p), decode_ctx);
-		if (slen <= 0) {
+		if (slen < 0) {
+			FR_PROTO_TRACE("    tlv decode failed at offset %zu - converting to raw", (size_t) (p - data));
 			fr_pair_list_free(list);
 			talloc_free(vp);
 			return fr_pair_raw_from_network(ctx, out, parent, data, data_len);
diff --git a/src/lib/util/decode.h b/src/lib/util/decode.h
index 166963545f5..f296fc2c250 100644
--- a/src/lib/util/decode.h
+++ b/src/lib/util/decode.h
@@ -45,6 +45,8 @@ typedef ssize_t (*fr_pair_decode_value_t)(TALLOC_CTX *ctx, fr_pair_list_t *out,
 					   fr_dict_attr_t const *parent,
 					   uint8_t const *data, size_t const data_len, void *decode_ctx);
 
+typedef bool (*fr_pair_tlvs_verify_t)(uint8_t const *data, size_t const data_len);
+
 #define PROTO_DECODE_FUNC(_name) static ssize_t decode_ ## _name(TALLOC_CTX *ctx, fr_pair_list_t *out, \
 					   fr_dict_attr_t const *parent, \
 					   uint8_t const *data, size_t const data_len, void *decode_ctx)
@@ -58,7 +60,8 @@ ssize_t fr_pair_raw_from_network(TALLOC_CTX *ctx, fr_pair_list_t *out, fr_dict_a
 ssize_t fr_pair_tlvs_from_network(TALLOC_CTX *ctx, fr_pair_list_t *out,
 				  fr_dict_attr_t const *parent,
 				  uint8_t const *data, size_t const data_len,
-				  void *decode_ctx, fr_pair_decode_value_t decode_tlv, bool nested) CC_HINT(nonnull(1,2,3,4,7));
+				  void *decode_ctx, fr_pair_decode_value_t decode_tlv, fr_pair_tlvs_verify_t verify_tlvs,
+				  bool nested) CC_HINT(nonnull(1,2,3,4,7));
 
 ssize_t fr_pair_dns_labels_from_network(TALLOC_CTX *ctx, fr_pair_list_t *out,
 					fr_dict_attr_t const *parent, uint8_t const *start,
diff --git a/src/protocols/dhcpv4/decode.c b/src/protocols/dhcpv4/decode.c
index 242bf68ebba..23c384f132f 100644
--- a/src/protocols/dhcpv4/decode.c
+++ b/src/protocols/dhcpv4/decode.c
@@ -36,11 +36,27 @@ static ssize_t decode_option(TALLOC_CTX *ctx, fr_pair_list_t *out,
 			      fr_dict_attr_t const *parent,
 			      uint8_t const *data, size_t const data_len, void *decode_ctx);
 
+static bool verify_tlvs(uint8_t const *data, size_t data_len)
+{
+	uint8_t const *p = data;
+	uint8_t const *end = data + data_len;
+
+	while (p < end) {
+		if ((end - p) < 2) return false;
+
+		if ((p + p[1]) > end) return false;
+
+		p += 2 + p[1];
+	}
+
+	return true;
+}
+
 static ssize_t decode_tlv_trampoline(TALLOC_CTX *ctx, fr_pair_list_t *out,
 				     fr_dict_attr_t const *parent,
 				     uint8_t const *data, size_t const data_len, void *decode_ctx)
 {
-	return fr_pair_tlvs_from_network(ctx, out, parent, data, data_len, decode_ctx, decode_option, false);
+	return fr_pair_tlvs_from_network(ctx, out, parent, data, data_len, decode_ctx, decode_option, verify_tlvs, false);
 }
 
 static ssize_t decode_value(TALLOC_CTX *ctx, fr_pair_list_t *out, fr_dict_attr_t const *parent,
@@ -388,7 +404,7 @@ next:
 	}
 	p++;
 
-	len = fr_pair_tlvs_from_network(ctx, out, vendor, p, option_len, decode_ctx, decode_option, false);
+	len = fr_pair_tlvs_from_network(ctx, out, vendor, p, option_len, decode_ctx, decode_option, verify_tlvs, false);
 	if (len <= 0) return len;
 
 	p += len;
@@ -453,10 +469,10 @@ static ssize_t decode_option(TALLOC_CTX *ctx, fr_pair_list_t *out,
 	if (!da) {
 		da = fr_dict_unknown_attr_afrom_num(packet_ctx->tmp_ctx, parent, option);
 		if (!da) return PAIR_DECODE_OOM;
-	}
-	FR_PROTO_TRACE("decode context changed %s -> %s",da->parent->name, da->name);
 
-	if ((da->type == FR_TYPE_STRING) && da_is_dns_label(da)) {
+		slen = fr_pair_raw_from_network(ctx, out, da, data + 2, len);
+
+	} else if ((da->type == FR_TYPE_STRING) && da_is_dns_label(da)) {
 		slen = fr_pair_dns_labels_from_network(ctx, out, da, data + 2, data + 2, len, NULL, true);
 
 	} else if (da->flags.array) {
@@ -466,7 +482,7 @@ static ssize_t decode_option(TALLOC_CTX *ctx, fr_pair_list_t *out,
 		slen = decode_vsa(ctx, out, da, data + 2, len, decode_ctx);
 
 	} else if (da->type == FR_TYPE_TLV) {
-		slen = fr_pair_tlvs_from_network(ctx, out, da, data + 2, len, decode_ctx, decode_option, false);
+		slen = fr_pair_tlvs_from_network(ctx, out, da, data + 2, len, decode_ctx, decode_option, verify_tlvs, false);
 
 	} else {
 		slen = decode_value(ctx, out, da, data + 2, len, decode_ctx);
@@ -572,7 +588,8 @@ ssize_t fr_dhcpv4_decode_option(TALLOC_CTX *ctx, fr_pair_list_t *out,
 			slen = decode_vsa(ctx, out, da, packet_ctx->buffer, q - packet_ctx->buffer, packet_ctx);
 
 		} else if (da->type == FR_TYPE_TLV) {
-			slen = fr_pair_tlvs_from_network(ctx, out, da, packet_ctx->buffer, q - packet_ctx->buffer, packet_ctx, decode_option, false);
+			slen = fr_pair_tlvs_from_network(ctx, out, da, packet_ctx->buffer, q - packet_ctx->buffer,
+							 packet_ctx, decode_option, verify_tlvs, false);
 
 		} else if (da->flags.array) {
 			slen = fr_pair_array_from_network(ctx, out, da, packet_ctx->buffer, q - packet_ctx->buffer, packet_ctx, decode_value);
diff --git a/src/protocols/dhcpv6/decode.c b/src/protocols/dhcpv6/decode.c
index c30b7b1ec55..1adfa10bead 100644
--- a/src/protocols/dhcpv6/decode.c
+++ b/src/protocols/dhcpv6/decode.c
@@ -44,7 +44,7 @@ static ssize_t decode_tlv_trampoline(TALLOC_CTX *ctx, fr_pair_list_t *out,
 				     fr_dict_attr_t const *parent,
 				     uint8_t const *data, size_t const data_len, void *decode_ctx)
 {
-	return fr_pair_tlvs_from_network(ctx, out, parent, data, data_len, decode_ctx, decode_option, true);
+	return fr_pair_tlvs_from_network(ctx, out, parent, data, data_len, decode_ctx, decode_option, NULL, true);
 }
 
 static ssize_t decode_value(TALLOC_CTX *ctx, fr_pair_list_t *out,
@@ -194,7 +194,7 @@ static ssize_t decode_value(TALLOC_CTX *ctx, fr_pair_list_t *out,
 		 *	header, as we're just decoding the values
 		 *	here.
 		 */
-		slen = fr_pair_tlvs_from_network(vp, &vp->vp_group, fr_dict_root(dict_dhcpv6), data, data_len, decode_ctx, decode_option, false);
+		slen = fr_pair_tlvs_from_network(vp, &vp->vp_group, fr_dict_root(dict_dhcpv6), data, data_len, decode_ctx, decode_option, NULL, false);
 		if (slen < 0) {
 			talloc_free(vp);
 			return slen;
@@ -291,7 +291,7 @@ static ssize_t decode_vsa(TALLOC_CTX *ctx, fr_pair_list_t *out,
 
 	FR_PROTO_TRACE("decode context %s -> %s", parent->name, da->name);
 
-	return fr_pair_tlvs_from_network(ctx, out, da, data + 4, data_len - 4, decode_ctx, decode_option, false);
+	return fr_pair_tlvs_from_network(ctx, out, da, data + 4, data_len - 4, decode_ctx, decode_option, NULL, false);
 }
 
 static ssize_t decode_option(TALLOC_CTX *ctx, fr_pair_list_t *out,
@@ -364,7 +364,7 @@ static ssize_t decode_option(TALLOC_CTX *ctx, fr_pair_list_t *out,
 		slen = decode_vsa(ctx, out, da, data + 4, len, decode_ctx);
 
 	} else if (da->type == FR_TYPE_TLV) {
-		slen = fr_pair_tlvs_from_network(ctx, out, da, data + 4, len, decode_ctx, decode_option, false);
+		slen = fr_pair_tlvs_from_network(ctx, out, da, data + 4, len, decode_ctx, decode_option, NULL, false);
 
 	} else {
 		slen = decode_value(ctx, out, da, data + 4, len, decode_ctx);
diff --git a/src/protocols/dns/decode.c b/src/protocols/dns/decode.c
index 4c12f2cae0e..f4ed6f3a0ee 100644
--- a/src/protocols/dns/decode.c
+++ b/src/protocols/dns/decode.c
@@ -45,7 +45,7 @@ static ssize_t decode_tlv_trampoline(TALLOC_CTX *ctx, fr_pair_list_t *out,
 				     fr_dict_attr_t const *parent,
 				     uint8_t const *data, size_t const data_len, void *decode_ctx)
 {
-	return fr_pair_tlvs_from_network(ctx, out, parent, data, data_len, decode_ctx, decode_option, true);
+	return fr_pair_tlvs_from_network(ctx, out, parent, data, data_len, decode_ctx, decode_option, NULL, true);
 }
 
 
diff --git a/src/protocols/radius/decode.c b/src/protocols/radius/decode.c
index 95b13de0cbd..50797a3f0f2 100644
--- a/src/protocols/radius/decode.c
+++ b/src/protocols/radius/decode.c
@@ -478,7 +478,7 @@ static ssize_t decode_rfc(TALLOC_CTX *ctx, fr_pair_list_t *out,
 		slen = fr_pair_array_from_network(ctx, out, da, data + 2, len - 2, decode_ctx, decode_value);
 
 	} else if (da->type == FR_TYPE_TLV) {
-		slen = fr_pair_tlvs_from_network(ctx, out, da, data + 2, len - 2, decode_ctx, decode_rfc, false);
+		slen = fr_pair_tlvs_from_network(ctx, out, da, data + 2, len - 2, decode_ctx, decode_rfc, NULL, false);
 
 	} else {
 		slen = decode_value(ctx, out, da, data + 2, len - 2, decode_ctx);
@@ -623,7 +623,7 @@ static ssize_t decode_digest_attributes(TALLOC_CTX *ctx, fr_pair_list_t *out,
 	if (!vp) return PAIR_DECODE_OOM;
 
 redo:
-	slen = fr_pair_tlvs_from_network(vp, &vp->vp_group, parent, p + 2, p[1] - 2, packet_ctx, decode_rfc, false);
+	slen = fr_pair_tlvs_from_network(vp, &vp->vp_group, parent, p + 2, p[1] - 2, packet_ctx, decode_rfc, NULL, false);
 	if (slen <= 0) {
 		talloc_free(vp);
 		return slen;