From 87390bfe6d266ab81312e072759203ccbb261906 Mon Sep 17 00:00:00 2001

From: "Alan T. DeKok" <aland@freeradius.org>

Date: Wed, 28 Jun 2017 12:13:03 -0400

Subject: [PATCH] make data2vp_extended() be more like data2vp_wimax()

There is no exploit, but making the code simpler is good.

---

 src/lib/radius.c            | 170 ++++++++++++++++++++++++++++++--------------

 src/tests/unit/extended.txt |   2 +-

 2 files changed, 116 insertions(+), 56 deletions(-)

diff --git a/src/lib/radius.c b/src/lib/radius.c

index ea4cbf06b..b9f0f59c9 100644

--- a/src/lib/radius.c

+++ b/src/lib/radius.c

@@ -3161,70 +3161,143 @@ static ssize_t data2vp_extended(TALLOC_CTX *ctx, RADIUS_PACKET *packet,

 				VALUE_PAIR **pvp)

 {

 	ssize_t rcode;

-	size_t fraglen;

+	size_t ext_len;

+	bool more;

 	uint8_t *head, *tail;

-	uint8_t const *frag, *end;

-	uint8_t const *attr;

-	int fragments;

-	bool last_frag;

+	uint8_t const *attr, *end;

+	DICT_ATTR const *child;

+

+	/*

+	 *	data = Ext-Attr Flag ...

+	 */

+

+	/*

+	 *	Not enough room for Ext-Attr + Flag + data, it's a bad

+	 *	attribute.

+	 */

+	if (attrlen < 3) {

+	raw:

+		/*

+		 *	It's not an Extended attribute, it's unknown...

+		 */

+		child = dict_unknown_afrom_fields(ctx, (da->vendor/ FR_MAX_VENDOR) & 0xff, 0);

+		if (!child) {

+			fr_strerror_printf("Internal sanity check %d", __LINE__);

+			return -1;

+		}

+

+		rcode = data2vp(ctx, packet, original, secret, child,

+				data, attrlen, attrlen, pvp);

+		if (rcode < 0) return rcode;

+		return attrlen;

+	}

+

+	/*

+	 *	No continued data, just decode the attribute in place.

+	 */

+	if ((data[1] & 0x80) == 0) {

+		rcode = data2vp(ctx, packet, original, secret, da,

+				data + 2, attrlen - 2, attrlen - 2,

+				pvp);

-	if (attrlen < 3) return -1;

+		if ((rcode < 0) || (((size_t) rcode + 2) != attrlen)) goto raw; /* didn't decode all of the data */

+		return attrlen;

+	}

+

+	/*

+	 *	It's continued, but there are no subsequent fragments,

+	 *	it's bad.

+	 */

+	if (attrlen >= packetlen) goto raw;

 	/*

 	 *	Calculate the length of all of the fragments.  For

 	 *	now, they MUST be contiguous in the packet, and they

-	 *	MUST be all of the same TYPE and EXTENDED-TYPE

+	 *	MUST be all of the same Type and Ext-Type

+	 *

+	 *	We skip the first fragment, which doesn't have a

+	 *	RADIUS attribute header.

 	 */

-	attr = data - 2;

-	fraglen = attrlen - 2;

-	frag = data + attrlen;

+	ext_len = attrlen - 2;

+	attr = data + attrlen;

 	end = data + packetlen;

-	fragments = 1;

-	last_frag = false;

-

-	while (frag < end) {

-		if (last_frag ||

-		    (frag[0] != attr[0]) ||

-		    (frag[1] < 4) ||		       /* too short for long-extended */

-		    (frag[2] != attr[2]) ||

-		    ((frag + frag[1]) > end)) {		/* overflow */

-			end = frag;

-			break;

-		}

-		last_frag = ((frag[3] & 0x80) == 0);

+	while (attr < end) {

+		/*

+		 *	Not enough room for Attr + length + Ext-Attr

+		 *	continuation, it's bad.

+		 */

+		if ((end - attr) < 4) goto raw;

+

+		if (attr[1] < 4) goto raw;

+

+		/*

+		 *	If the attribute overflows the packet, it's

+		 *	bad.

+		 */

+		if ((attr + attr[1]) > end) goto raw;

+

+		if (attr[0] != ((da->vendor / FR_MAX_VENDOR) & 0xff)) goto raw; /* not the same Extended-Attribute-X */

+

+		if (attr[2] != data[0]) goto raw; /* Not the same Ext-Attr */

+

+		/*

+		 *	Check the continuation flag.

+		 */

+		more = ((attr[2] & 0x80) != 0);

+

+		/*

+		 *	Or, there's no more data, in which case we

+		 *	shorten "end" to finish at this attribute.

+		 */

+		if (!more) end = attr + attr[1];

+

+		/*

+		 *	There's more data, but we're at the end of the

+		 *	packet.  The attribute is malformed!

+		 */

+		if (more && ((attr + attr[1]) == end)) goto raw;

+

+		/*

+		 *	Add in the length of the data we need to

+		 *	concatenate together.

+		 */

+		ext_len += attr[1] - 4;

-		fraglen += frag[1] - 4;

-		frag += frag[1];

-		fragments++;

+		/*

+		 *	Go to the next attribute, and stop if there's

+		 *	no more.

+		 */

+		attr += attr[1];

+		if (!more) break;

 	}

-	head = tail = malloc(fraglen);

-	if (!head) return -1;

+	if (!ext_len) goto raw;

-	VP_TRACE("Fragments %d, total length %d\n", fragments, (int) fraglen);

+	head = tail = malloc(ext_len);

+	if (!head) goto raw;

 	/*

-	 *	And again, but faster and looser.

-	 *

-	 *	We copy the first fragment, followed by the rest of

-	 *	the fragments.

+	 *	Copy the data over, this time trusting the attribute

+	 *	contents.

 	 */

-	frag = attr;

+	attr = data;

+	memcpy(tail, attr + 2, attrlen - 2);

+	tail += attrlen - 2;

+	attr += attrlen;

-	while (fragments >  0) {

-		memcpy(tail, frag + 4, frag[1] - 4);

-		tail += frag[1] - 4;

-		frag += frag[1];

-		fragments--;

+	while (attr < end) {

+		memcpy(tail, attr + 4, attr[1] - 4);

+		tail += attr[1] - 4;

+		attr += attr[1]; /* skip VID+WiMax header */

 	}

-	VP_HEXDUMP("long-extended fragments", head, fraglen);

+	VP_HEXDUMP("long-extended fragments", head, ext_len);

 	rcode = data2vp(ctx, packet, original, secret, da,

-			head, fraglen, fraglen, pvp);

+			head, ext_len, ext_len, pvp);

 	free(head);

-	if (rcode < 0) return rcode;

+	if (rcode < 0) goto raw;

 	return end - data;

 }

@@ -3827,19 +3900,6 @@ ssize_t data2vp(TALLOC_CTX *ctx,

 		}

 		/*

-		 *	If there no more fragments, then the contents

-		 *	have to be a well-known data type.

-		 *

-		 */

-		if ((data[1] & 0x80) == 0) {

-			rcode = data2vp(ctx, packet, original, secret, child,

-					data + 2, attrlen - 2, attrlen - 2,

-					pvp);

-			if (rcode < 0) goto raw;

-			return 2 + rcode;

-		}

-

-		/*

 		 *	This requires a whole lot more work.

 		 */

 		return data2vp_extended(ctx, packet, original, secret, child,

diff --git a/src/tests/unit/extended.txt b/src/tests/unit/extended.txt

index 8b0e3a2c4..9810b194c 100644

--- a/src/tests/unit/extended.txt

+++ b/src/tests/unit/extended.txt

@@ -80,7 +80,7 @@ data Attr-245.26.1.6 = 0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa

 # again, but the second one attr is not an extended attr

 decode f5 ff 1a 80 00 00 00 01 06 aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa ab bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb 01 05 62 6f 62

-data Attr-245.26.1.6 = 0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaabbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb, User-Name = "bob"

+data Attr-245 = 0x1a800000000106aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaabbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb, User-Name = "bob"

 # No data means that the attribute is an "invalid attribute"

 decode f5 04 01 00

-- 

2.13.2