From: Alan T. DeKok <aland@freeradius.org>
Date: Wed, 20 Jan 2021 14:07:01 +0000 (-0500)
Subject: aggregate tags across the entire packet
X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=94c8ad85ef158af4938f31130e0417e6ddcfdafa;p=thirdparty%2Ffreeradius-server.git

aggregate tags across the entire packet

not just one attribute

the previous tests were decoding lists of pairs, which worked.
updating the tests to decoding packets should the issue.

note that packet encode with tags is still failing.  That will be
fixed next.
---

diff --git a/src/protocols/radius/base.c b/src/protocols/radius/base.c
index a0ca7b1b10c..7f452d77e76 100644
--- a/src/protocols/radius/base.c
+++ b/src/protocols/radius/base.c
@@ -1076,13 +1076,13 @@ ssize_t	fr_radius_decode(TALLOC_CTX *ctx, uint8_t const *packet, size_t packet_l
 
 		attr += slen;
 		talloc_free_children(packet_ctx.tmp_ctx);
-		packet_ctx.tags = NULL;
 	}
 
 	/*
 	 *	We've parsed the whole packet, return that.
 	 */
 	talloc_free(packet_ctx.tmp_ctx);
+	talloc_free(packet_ctx.tags);
 	return packet_len;
 }
 
diff --git a/src/protocols/radius/decode.c b/src/protocols/radius/decode.c
index d85c4ec0368..b4724efc9f9 100644
--- a/src/protocols/radius/decode.c
+++ b/src/protocols/radius/decode.c
@@ -1034,10 +1034,6 @@ ssize_t fr_radius_decode_pair_value(TALLOC_CTX *ctx, fr_cursor_t *cursor, fr_dic
 		return -1;
 	}
 
-#ifdef __clang_analyzer__
-	if (!packet_ctx || !packet_ctx->tmp_ctx) return -1;
-#endif
-
 	FR_PROTO_HEX_DUMP(data, attr_len, "%s", __FUNCTION__ );
 
 	FR_PROTO_TRACE("Parent %s len %zu ... %zu", parent->name, attr_len, packet_len);
@@ -1087,12 +1083,27 @@ ssize_t fr_radius_decode_pair_value(TALLOC_CTX *ctx, fr_cursor_t *cursor, fr_dic
 			}
 
 			p = buffer;
-		}
+
+		} /* else the field is >=0x20, so it's not a tag */
 	}
 
 	if (tag) {
+		/*
+		 *	The EAP chbind code calls us with a NULL
+		 *	packet ctx.  Which means that we can't decode
+		 *	tags.  Which is OK, because the channel
+		 *	binding attributes used there don't have tags.
+		 */
+		if (!packet_ctx) goto raw;
+
 		if (!packet_ctx->tags) {
-			packet_ctx->tags = talloc_zero_array(packet_ctx->tmp_ctx, fr_radius_tag_ctx_t *, 32);
+			/*
+			 *	This should NOT be packet_ctx.tmp_ctx,
+			 *	as that is freed after decoding every
+			 *	packet.  We wish to aggregate the tags
+			 *	across multiple attributes.
+			 */
+			packet_ctx->tags = talloc_zero_array(NULL, fr_radius_tag_ctx_t *, 32);
 			if (!packet_ctx->tags) {
 				fr_pair_list_free(&vp);
 				fr_strerror_printf("%s: Internal sanity check %d", __FUNCTION__, __LINE__);
@@ -1421,6 +1432,8 @@ ssize_t fr_radius_decode_pair_value(TALLOC_CTX *ctx, fr_cursor_t *cursor, fr_dic
 	{
 		fr_dict_attr_t *raw;
 
+		if (!packet_ctx) goto raw;
+
 		/*
 		 *	Create an unknown attribute, and decode it as
 		 *	"octets".  Note that we have to account for
@@ -1436,6 +1449,7 @@ ssize_t fr_radius_decode_pair_value(TALLOC_CTX *ctx, fr_cursor_t *cursor, fr_dic
 		if (!child) goto raw;
 
 		raw->flags.is_raw = 1;
+
 		/*
 		 *	"long" extended.  Decode the value.
 		 */
@@ -1478,6 +1492,7 @@ ssize_t fr_radius_decode_pair_value(TALLOC_CTX *ctx, fr_cursor_t *cursor, fr_dic
 	if (!tag) {
 		vp = fr_pair_afrom_da(ctx, parent);
 	} else {
+		fr_assert(packet_ctx != NULL);
 		fr_assert(packet_ctx->tags != NULL);
 		fr_assert(packet_ctx->tags[tag] != NULL);
 		vp = fr_pair_afrom_da(packet_ctx->tags[tag]->parent, parent);
@@ -1593,6 +1608,7 @@ done:
 		return attr_len;
 	}
 
+	fr_assert(packet_ctx != NULL);
 	fr_assert(packet_ctx->tags != NULL);
 	fr_assert(packet_ctx->tags[tag] != NULL);
 	fr_cursor_append(&packet_ctx->tags[tag]->cursor, vp);
diff --git a/src/protocols/radius/packet.c b/src/protocols/radius/packet.c
index 94ab1d85a98..ab60c2c8d89 100644
--- a/src/protocols/radius/packet.c
+++ b/src/protocols/radius/packet.c
@@ -245,6 +245,7 @@ int fr_radius_packet_decode(fr_radius_packet_t *packet, fr_pair_list_t *list,
 	 */
 	fr_rand_seed(packet->data, RADIUS_HEADER_LENGTH);
 	talloc_free(packet_ctx.tmp_ctx);
+	talloc_free(packet_ctx.tags);
 
 	return 0;
 }
diff --git a/src/tests/unit/protocols/radius/rfc2868.txt b/src/tests/unit/protocols/radius/rfc2868.txt
index 0fc7775db81..62e4a1b11bb 100644
--- a/src/tests/unit/protocols/radius/rfc2868.txt
+++ b/src/tests/unit/protocols/radius/rfc2868.txt
@@ -72,6 +72,18 @@ match Tag-1 = { Tunnel-Type = PPTP, Tunnel-Medium-Type = IPv4 }, Tag-2 = { Tunne
 encode-pair -
 match 40 06 01 00 00 01 41 06 01 00 00 01 40 06 02 00 00 01
 
+#
+#  Tags are aggregated across the entire packet, too.
+#
+decode-proto 01 01 00 26 00000000000000000000000000000000 40 06 01 00 00 01 40 06 02 00 00 01 41 06 01 00 00 01
+match Packet-Type = Access-Request, Packet-Authentication-Vector = 0x00000000000000000000000000000000, Tag-1 = { Tunnel-Type = PPTP, Tunnel-Medium-Type = IPv4 }, Tag-2 = { Tunnel-Type = PPTP }
+
+#
+#  This currently fails.  :(
+#
+#encode-proto -
+#match 01 00 00 14 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
+
 # Invalid tag on decoding
 decode-pair 40 06 20 00 00 01
 match raw.Tunnel-Type = 0x20000001
@@ -96,4 +108,4 @@ decode-pair -
 match Tunnel-Client-Endpoint = "\001\002\003"
 
 count
-match 40
+match 42