FR-GV-301 - handle malformed WiMAX attributes

diff --git a/src/lib/radius.c b/src/lib/radius.c
index 23e5763..34acf8a 100644 (file)
--- a/src/lib/radius.c
+++ b/src/lib/radius.c
@@ -3221,7 +3221,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
  */
@@ -3233,25 +3233,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;
        }
 
        /*
@@ -3260,61 +3289,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 191b37e..6e373d5 100644 (file)
--- 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
+