From c4685b85f61f28ea0d5cf4e41cb8feaa5193d9dd Mon Sep 17 00:00:00 2001

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

Date: Tue, 27 Jun 2017 21:49:20 -0400

Subject: [PATCH] FR-GV-301 - handle malformed WiMAX attributes

---

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

 src/tests/unit/wimax.txt |  12 ++++

 2 files changed, 142 insertions(+), 47 deletions(-)

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

index 7114e1650..ea4cbf06b 100644

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

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

@@ -3229,7 +3229,7 @@ static ssize_t data2vp_extended(TALLOC_CTX *ctx, RADIUS_PACKET *packet,

 	return end - data;

 }

-/** Convert a Vendor-Specific WIMAX to vps

+/** Convert a Vendor-Specific WIMAX to VPs

  *

  * @note Called ONLY for Vendor-Specific

  */

@@ -3241,25 +3241,54 @@ static ssize_t data2vp_wimax(TALLOC_CTX *ctx,

 			     VALUE_PAIR **pvp)

 {

 	ssize_t rcode;

-	size_t fraglen;

-	bool last_frag;

+	size_t wimax_len;

+	bool more;

 	uint8_t *head, *tail;

-	uint8_t const *frag, *end;

+	uint8_t const *attr, *end;

 	DICT_ATTR const *child;

-	if (attrlen < 8) return -1;

+	/*

+	 *	data = VID VID VID VID WiMAX-Attr WimAX-Len Continuation ...

+	 */

+

+	/*

+	 *	Not enough room for WiMAX Vendor + Wimax attr + length

+	 *	+ continuation, it's a bad attribute.

+	 */

+	if (attrlen < 8) {

+	raw:		

+		/*

+		 *	It's not a Vendor-Specific, it's unknown...

+		 */

+		child = dict_unknown_afrom_fields(ctx, PW_VENDOR_SPECIFIC, 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;

+	}

-	if (((size_t) (data[5] + 4)) != attrlen) return -1;

+	if (data[5] < 3) goto raw;		/* WiMAX-Length is too small */

 	child = dict_attrbyvalue(data[4], vendor);

-	if (!child) return -1;

+	if (!child) goto raw;

+	/*

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

+	 */

 	if ((data[6] & 0x80) == 0) {

+		if ((data[5] + 4) != attrlen) goto raw; /* WiMAX attribute doesn't fill Vendor-Specific */

+

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

 				data + 7, data[5] - 3, data[5] - 3,

 				pvp);

-		if (rcode < 0) return -1;

-		return 7 + rcode;

+

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

+		return attrlen;

 	}

 	/*

@@ -3268,61 +3297,115 @@ static ssize_t data2vp_wimax(TALLOC_CTX *ctx,

 	 *	MUST be all of the same VSA, WiMAX, and WiMAX-attr.

 	 *

 	 *	The first fragment doesn't have a RADIUS attribute

-	 *	header, so it needs to be treated a little special.

+	 *	header.

 	 */

-	fraglen = data[5] - 3;

-	frag = data + attrlen;

+	wimax_len = 0;

+	attr = data + 4;

 	end = data + packetlen;

-	last_frag = false;

-	while (frag < end) {

-		if (last_frag ||

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

-		    (frag[1] < 9) ||		       /* too short for wimax */

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

-		    (memcmp(frag + 2, data, 4) != 0) || /* not wimax */

-		    (frag[6] != data[4]) || /* not the same wimax attr */

-		    ((frag[7] + 6) != frag[1])) { /* doesn't fill the attr */

-			end = frag;

-			break;

-		}

+	while (attr < end) {

+		/*

+		 *	Not enough room for Attribute + length +

+		 *	continuation, it's bad.

+		 */

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

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

+		/*

+		 *	Must have non-zero data in the attribute.

+		 */

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

-		fraglen += frag[7] - 3;

-		frag += frag[1];

-	}

+		/*

+		 *	If the WiMAX attribute overflows the packet,

+		 *	it's bad.

+		 */

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

-	head = tail = malloc(fraglen);

-	if (!head) return -1;

+		/*

+		 *	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.

+		 */

+		wimax_len += attr[1] - 3;

+

+		/*

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

+		 *	no more.

+		 */

+		attr += attr[1];

+		if (!more) break;

+

+		/*

+		 *	data = VID VID VID VID WiMAX-Attr WimAX-Len Continuation ...

+		 *

+		 *	attr = Vendor-Specific VSA-Length VID VID VID VID WiMAX-Attr WimAX-Len Continuation ...

+		 *

+		 */

+

+		/*

+		 *	No room for Vendor-Specific + length +

+		 *	Vendor(4) + attr + length + continuation + data

+		 */

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

+

+		if (attr[0] != PW_VENDOR_SPECIFIC) goto raw;

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

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

+		if (memcmp(data, attr + 2, 4) != 0) goto raw; /* not WiMAX Vendor ID */

+

+		if (attr[1] != (attr[7] + 6)) goto raw; /* WiMAX attr doesn't exactly fill the VSA */

+

+		if (data[4] != attr[6]) goto raw; /* different WiMAX attribute */

+

+		/*

+		 *	Skip over the Vendor-Specific header, and

+		 *	continue with the WiMAX attributes.

+		 */

+		attr += 6;

+	}

 	/*

-	 *	And again, but faster and looser.

-	 *

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

-	 *	the fragments.

+	 *	No data in the WiMAX attribute, make a "raw" one.

 	 */

-	frag = data;

+	if (!wimax_len) goto raw;

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

-	tail += frag[4 + 1] - 3;

-	frag += attrlen;	/* should be frag[1] - 7 */

+	head = tail = malloc(wimax_len);

+	if (!head) return -1;

 	/*

-	 *	frag now points to RADIUS attributes

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

+	 *	contents.

 	 */

-	do {

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

-		tail += frag[2 + 4 + 1] - 3;

-		frag += frag[1];

-	} while (frag < end);

+	attr = data;

+	while (attr < end) {

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

+		tail += attr[4 + 1] - 3;

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

+		attr += 2;		 /* skip Vendor-Specific header */

+	}

-	VP_HEXDUMP("wimax fragments", head, fraglen);

+	VP_HEXDUMP("wimax fragments", head, wimax_len);

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

-			head, fraglen, fraglen, pvp);

+			head, wimax_len, wimax_len, pvp);

 	free(head);

-	if (rcode < 0) return rcode;

+	if (rcode < 0) goto raw;

 	return end - data;

 }

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

index 191b37e25..6e373d59a 100644

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

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

@@ -7,6 +7,12 @@ data 1a 0e 00 00 60 b5 01 08 00 01 05 31 2e 30

 decode -

 data WiMAX-Release = "1.0"

+decode 1a 08 00 00 60 b5 01 02

+data Attr-26 = 0x000060b50102

+

+decode 1a 0a 00 00 60 b5 01 04 00 01

+data Attr-26.24757.1 = 0x01

+

 encode WiMAX-Accounting-Capabilities = 1

 data 1a 0c 00 00 60 b5 01 06 00 02 03 01

@@ -143,3 +149,9 @@ data 1a ff 00 00 60 b5 01 f9 80 01 ff 45 45 45 45 45 45 45 45 45 45 45 45 45 45

 decode -

 data WiMAX-Release = "EEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEE", WiMAX-Idle-Mode-Notification-Cap = Supported

+

+#

+#  Continuation is set, but there's no continued data.

+decode 1a 0b 00 00 60 b5 31 05 80 00 00

+data Attr-26 = 0x000060b53105800000

+

-- 

2.13.2