File 931850e5_port.patch of Package freeradius-server.3979

Overview Repositories Revisions Requests Users Attributes Meta

File 931850e5_port.patch of Package freeradius-server.3979

TODO:

commit 931850e5d2f65193520c2d9c9878148c0cdc16a6
Author: Alan T. DeKok <aland@freeradius.org>
Date:   Tue Jun 27 21:49:20 2017 -0400

FR-GV-301 - handle malformed WiMAX attributes

Index: freeradius-server-3.0.3/src/lib/radius.c
===================================================================
--- freeradius-server-3.0.3.orig/src/lib/radius.c
+++ freeradius-server-3.0.3/src/lib/radius.c
@@ -3219,7 +3219,7 @@ static ssize_t data2vp_extended(RADIUS_P
 }
 
 /**
- * @brief Convert a Vendor-Specific WIMAX to vps
+ * @brief Convert a Vendor-Specific WIMAX to VPs
  *
  *	Called ONLY for Vendor-Specific
  */
@@ -3231,25 +3231,52 @@ static ssize_t data2vp_wimax(RADIUS_PACK
 			     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;
 
-	if (((size_t) (data[5] + 4)) != attrlen) 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_attrunknown(PW_VENDOR_SPECIFIC, 0, 1);
+               if (!child) {
+                       fr_strerror_printf("Internal sanity check %d", __LINE__);
+                       return -1;
+               }
+
+               rcode = data2vp(packet, original, secret, child,
+                               data, attrlen, attrlen, pvp);
+               if (rcode < 0) return rcode;
+               return attrlen;
+       }
+
+	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;
 
 	if ((data[6] & 0x80) == 0) {
+		if ((data[5] + 4) != attrlen) goto raw; /* WiMAX attribute doesn't fill Vendor-Specific */
+
 		rcode = data2vp(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;
 	}
 
 	/*
@@ -3258,61 +3285,113 @@ static ssize_t data2vp_wimax(RADIUS_PACK
 	 *	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;
-		}
-
-		last_frag = ((frag[8] & 0x80) == 0);
-
-		fraglen += frag[7] - 3;
-		frag += frag[1];
-	}
-
-	head = tail = malloc(fraglen);
-	if (!head) return -1;
+	while (attr < end) {
+		/*
+		*      Not enough room for Attribute + length +
+		*      continuation, it's bad.
+		*/
+		if ((end - attr) < 3) goto raw;
+
+		// Must have non-zero data in the attribute.
+		if (attr[1] <= 3) goto raw;
+
+		/*
+		* If the WiMAX attribute overflows the packet,
+		* it's bad.
+		*/
+		if ((attr + attr[1]) > end) goto raw;
+
+		/*
+                *      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(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;
 }
Index: freeradius-server-3.0.3/src/tests/unit/wimax.txt
===================================================================
--- freeradius-server-3.0.3.orig/src/tests/unit/wimax.txt
+++ freeradius-server-3.0.3/src/tests/unit/wimax.txt
@@ -7,6 +7,12 @@ data 1a 0e 00 00 60 b5 01 08 00 01 05 31
 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

Places

File 931850e5_port.patch of Package freeradius-server.3979

Places