FR-GV-304 - check for option overflowing the packet

[freeradius.git] / src / modules / proto_dhcp / dhcp.c

diff --git a/src/modules/proto_dhcp/dhcp.c b/src/modules/proto_dhcp/dhcp.c
index ba5903f..5fd922d 100644 (file)
--- a/src/modules/proto_dhcp/dhcp.c
+++ b/src/modules/proto_dhcp/dhcp.c
@@ -1,3 +1,4 @@
+
 /*
  * dhcp.c      Functions to send/receive dhcp packets.
  *
@@ -331,14 +332,14 @@ RADIUS_PACKET *fr_dhcp_recv(int sockfd)
        packet->id = ntohl(magic);
 
        code = dhcp_get_option((dhcp_packet_t *) packet->data,
-                              packet->data_len, 53);
+                              packet->data_len, PW_DHCP_MESSAGE_TYPE);
        if (!code) {
                fr_strerror_printf("No message-type option was found in the packet");
                rad_free(&packet);
                return NULL;
        }
 
-       if ((code[1] < 1) || (code[2] == 0) || (code[2] > DHCP_MAX_MESSAGE_TYPE)) {
+       if ((code[1] < 1) || (code[2] == 0) || (code[2] >= DHCP_MAX_MESSAGE_TYPE)) {
                fr_strerror_printf("Unknown value %d for message-type option", code[2]);
                rad_free(&packet);
                return NULL;
@@ -399,7 +400,7 @@ RADIUS_PACKET *fr_dhcp_recv(int sockfd)
                char src_ip_buf[256], dst_ip_buf[256];
 
                if ((packet->code >= PW_DHCP_DISCOVER) &&
-                   (packet->code <= (1024 + DHCP_MAX_MESSAGE_TYPE))) {
+                   (packet->code < (1024 + DHCP_MAX_MESSAGE_TYPE))) {
                        name = dhcp_message_types[packet->code - PW_DHCP_OFFSET];
                } else {
                        snprintf(type_buf, sizeof(type_buf), "%d",
@@ -454,7 +455,7 @@ int fr_dhcp_send(RADIUS_PACKET *packet)
                char dst_ip_buf[INET6_ADDRSTRLEN];
 
                if ((packet->code >= PW_DHCP_DISCOVER) &&
-                   (packet->code <= (1024 + DHCP_MAX_MESSAGE_TYPE))) {
+                   (packet->code < (1024 + DHCP_MAX_MESSAGE_TYPE))) {
                        name = dhcp_message_types[packet->code - PW_DHCP_OFFSET];
                } else {
                        snprintf(type_buf, sizeof(type_buf), "%d",
@@ -628,6 +629,24 @@ static int fr_dhcp_decode_suboption(TALLOC_CTX *ctx, VALUE_PAIR **tlv, uint8_t c
                uint32_t        attr;
 
                /*
+                *      Not enough room for the option header, it's a
+                *      bad packet.
+                */
+               if ((p + 2) > (data + len)) {
+                       fr_pair_list_free(&head);
+                       return -1;
+               }
+
+               /*
+                *      Not enough room for the option header + data,
+                *      it's a bad packet.
+                */
+               if ((p + 2 + p[1]) > (data + len)) {
+                       fr_pair_list_free(&head);
+                       return -1;
+               }
+
+               /*
                 *      The initial OID string looks like:
                 *      <iana>.0
                 *
@@ -773,25 +792,23 @@ static int fr_dhcp_attr2vp(TALLOC_CTX *ctx, VALUE_PAIR **vp_p, uint8_t const *da
                 *      multiple additional VPs
                 */
                fr_cursor_init(&cursor, vp_p);
-               for (;;) {
-                       q = memchr(p, '\0', q - p);
+               while (p < end) {
+                       q = memchr(p, '\0', end - p);
                        /* Malformed but recoverable */
                        if (!q) q = end;
 
                        fr_pair_value_bstrncpy(vp, (char const *)p, q - p);
                        p = q + 1;
 
+                       if (p >= end) break;
+
                        /* Need another VP for the next round */
-                       if (p < end) {
-                               vp = fr_pair_afrom_da(ctx, vp->da);
-                               if (!vp) {
-                                       fr_pair_list_free(vp_p);
-                                       return -1;
-                               }
-                               fr_cursor_insert(&cursor, vp);
-                               continue;
+                       vp = fr_pair_afrom_da(ctx, vp->da);
+                       if (!vp) {
+                               fr_pair_list_free(vp_p);
+                               return -1;
                        }
-                       break;
+                       fr_cursor_insert(&cursor, vp);
                }
        }
                break;
@@ -872,6 +889,16 @@ ssize_t fr_dhcp_decode_options(TALLOC_CTX *ctx, VALUE_PAIR **out, uint8_t const
                a_p = p + 2;
 
                /*
+                *      Ensure we've not been given a bad length value
+                */
+               if ((a_p + a_len) > q) {
+                       fr_strerror_printf("Length field value of option %u is incorrect.  "
+                                          "Got %u bytes, expected <= %zu bytes", p[0], p[1], q - a_p);
+                       fr_pair_list_free(out);
+                       return -1;
+               }
+
+               /*
                 *      Unknown attribute, create an octets type
                 *      attribute with the contents of the sub-option.
                 */
@@ -952,7 +979,6 @@ int fr_dhcp_decode(RADIUS_PACKET *packet)
         *      Decode the header.
         */
        for (i = 0; i < 14; i++) {
-               char *q;
 
                vp = fr_pair_make(packet, NULL, dhcp_header_names[i], NULL, T_OP_EQ);
                if (!vp) {
@@ -1005,14 +1031,18 @@ int fr_dhcp_decode(RADIUS_PACKET *packet)
                        break;
 
                case PW_TYPE_STRING:
-                       vp->vp_strvalue = q = talloc_array(vp, char, dhcp_header_sizes[i] + 1);
-                       vp->type = VT_DATA;
-                       memcpy(q, p, dhcp_header_sizes[i]);
-                       q[dhcp_header_sizes[i]] = '\0';
-                       vp->vp_length = strlen(vp->vp_strvalue);
-                       if (vp->vp_length == 0) {
-                               fr_pair_list_free(&vp);
+                       /*
+                        *      According to RFC 2131, these are null terminated strings.
+                        *      We don't trust everyone to abide by the RFC, though.
+                        */
+                       if (*p != '\0') {
+                               uint8_t *end;
+                               int len;
+                               end = memchr(p, '\0', dhcp_header_sizes[i]);
+                               len = end ? end - p : dhcp_header_sizes[i];
+                               fr_pair_value_bstrncpy(vp, p, len);
                        }
+                       if (vp->vp_length == 0) fr_pair_list_free(&vp);
                        break;
 
                case PW_TYPE_OCTETS:
@@ -1083,8 +1113,8 @@ int fr_dhcp_decode(RADIUS_PACKET *packet)
                        /*
                         *      Vendor is "MSFT 98"
                         */
-                       vp = fr_pair_find_by_num(head, 63, DHCP_MAGIC_VENDOR, TAG_ANY);
-                       if (vp && (strcmp(vp->vp_strvalue, "MSFT 98") == 0)) {
+                       vp = fr_pair_find_by_num(head, 60, DHCP_MAGIC_VENDOR, TAG_ANY);
+                       if (vp && (vp->vp_length >= 7) && (memcmp(vp->vp_octets, "MSFT 98", 7) == 0)) {
                                vp = fr_pair_find_by_num(head, 262, DHCP_MAGIC_VENDOR, TAG_ANY);
 
                                /*
@@ -1141,12 +1171,14 @@ int8_t fr_dhcp_attr_cmp(void const *a, void const *b)
        /*
         *      DHCP-Message-Type is first, for simplicity.
         */
-       if ((my_a->da->attr == 53) && (my_b->da->attr != 53)) return -1;
+       if ((my_a->da->attr == PW_DHCP_MESSAGE_TYPE) && (my_b->da->attr != PW_DHCP_MESSAGE_TYPE)) return -1;
+       if ((my_a->da->attr != PW_DHCP_MESSAGE_TYPE) && (my_b->da->attr == PW_DHCP_MESSAGE_TYPE)) return +1;
 
        /*
         *      Relay-Agent is last
         */
-       if ((my_a->da->attr == 82) && (my_b->da->attr != 82)) return 1;
+       if ((my_a->da->attr == PW_DHCP_OPTION_82) && (my_b->da->attr != PW_DHCP_OPTION_82)) return +1;
+       if ((my_a->da->attr != PW_DHCP_OPTION_82) && (my_b->da->attr == PW_DHCP_OPTION_82)) return -1;
 
        if (my_a->da->attr < my_b->da->attr) return -1;
        if (my_a->da->attr > my_b->da->attr) return 1;
@@ -1295,6 +1327,7 @@ static ssize_t fr_dhcp_vp2data_tlv(uint8_t *out, ssize_t outlen, vp_cursor_t *cu
                        return -1;
                }
 
+               debug_pair(vp);
                *opt_len += len;
                p += len;
        };
@@ -1322,7 +1355,7 @@ ssize_t fr_dhcp_encode_option(UNUSED TALLOC_CTX *ctx, uint8_t *out, size_t outle
        if (!vp) return -1;
 
        if (vp->da->vendor != DHCP_MAGIC_VENDOR) goto next; /* not a DHCP option */
-       if (vp->da->attr == 53) goto next; /* already done */
+       if (vp->da->attr == PW_DHCP_MESSAGE_TYPE) goto next; /* already done */
        if ((vp->da->attr > 255) && (DHCP_BASE_ATTR(vp->da->attr) != PW_DHCP_OPTION_82)) goto next;
 
        if (vp->da->flags.extended) {
@@ -1356,6 +1389,7 @@ ssize_t fr_dhcp_encode_option(UNUSED TALLOC_CTX *ctx, uint8_t *out, size_t outle
 
                } else {
                        len = fr_dhcp_vp2data(p, freespace, vp);
+                       if (len >= 0) debug_pair(vp);
                        fr_cursor_next(cursor);
                        previous = vp->da;
                }
@@ -1371,7 +1405,6 @@ ssize_t fr_dhcp_encode_option(UNUSED TALLOC_CTX *ctx, uint8_t *out, size_t outle
                p += len;
                *opt_len += len;
                freespace -= len;
-               debug_pair(vp);
 
        } while ((vp = fr_cursor_current(cursor)) && previous && (previous == vp->da) && vp->da->flags.array);
 
@@ -1413,7 +1446,7 @@ int fr_dhcp_encode(RADIUS_PACKET *packet)
 
 #ifndef NDEBUG
        if ((packet->code >= PW_DHCP_DISCOVER) &&
-           (packet->code <= (1024 + DHCP_MAX_MESSAGE_TYPE))) {
+           (packet->code < (1024 + DHCP_MAX_MESSAGE_TYPE))) {
                name = dhcp_message_types[packet->code - PW_DHCP_OFFSET];
        } else {
                name = "?Unknown?";
@@ -1671,7 +1704,6 @@ int fr_dhcp_encode(RADIUS_PACKET *packet)
        p[2] = packet->code - PW_DHCP_OFFSET;
        p += 3;
 
-
        /*
         *  Pre-sort attributes into contiguous blocks so that fr_dhcp_encode_option
         *  operates correctly. This changes the order of the list, but never mind...
@@ -1888,7 +1920,7 @@ int fr_dhcp_send_raw_packet(int sockfd, struct sockaddr_ll *p_ll, RADIUS_PACKET
                char dst_ip_buf[INET6_ADDRSTRLEN];
 
                if ((packet->code >= PW_DHCP_DISCOVER) &&
-                   (packet->code <= (1024 + DHCP_MAX_MESSAGE_TYPE))) {
+                   (packet->code < (1024 + DHCP_MAX_MESSAGE_TYPE))) {
                        name = dhcp_message_types[packet->code - PW_DHCP_OFFSET];
                } else {
                        snprintf(type_buf, sizeof(type_buf), "%d",
@@ -2047,7 +2079,7 @@ RADIUS_PACKET *fr_dhcp_recv_raw_packet(int sockfd, struct sockaddr_ll *p_ll, RAD
        packet->id = xid;
 
        code = dhcp_get_option((dhcp_packet_t *) packet->data,
-                              packet->data_len, 53);
+                              packet->data_len, PW_DHCP_MESSAGE_TYPE);
        if (!code) {
                fr_strerror_printf("No message-type option was found in the packet");
                rad_free(&packet);
@@ -2091,7 +2123,7 @@ RADIUS_PACKET *fr_dhcp_recv_raw_packet(int sockfd, struct sockaddr_ll *p_ll, RAD
                char src_ip_buf[256], dst_ip_buf[256];
 
                if ((packet->code >= PW_DHCP_DISCOVER) &&
-                   (packet->code <= (1024 + DHCP_MAX_MESSAGE_TYPE))) {
+                   (packet->code < (1024 + DHCP_MAX_MESSAGE_TYPE))) {
                        name = dhcp_message_types[packet->code - PW_DHCP_OFFSET];
                } else {
                        snprintf(type_buf, sizeof(type_buf), "%d", packet->code - PW_DHCP_OFFSET);