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