From c4685b85f61f28ea0d5cf4e41cb8feaa5193d9dd Mon Sep 17 00:00:00 2001 From: "Alan T. DeKok" 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