FR-GV-304 - check for option overflowing the packet
[freeradius.git] / src / modules / proto_dhcp / dhcp.c
index 437e844..5fd922d 100644 (file)
@@ -1,3 +1,4 @@
+
 /*
  * dhcp.c      Functions to send/receive dhcp packets.
  *
@@ -142,8 +143,16 @@ static char const *dhcp_message_types[] = {
        "DHCP-Release",
        "DHCP-Inform",
        "DHCP-Force-Renew",
+       "DHCP-Lease-Query",
+       "DHCP-Lease-Unassigned",
+       "DHCP-Lease-Unknown",
+       "DHCP-Lease-Active",
+       "DHCP-Bulk-Lease-Query",
+       "DHCP-Lease-Query-Done"
 };
 
+#define DHCP_MAX_MESSAGE_TYPE (sizeof(dhcp_message_types) / sizeof(dhcp_message_types[0]))
+
 static int dhcp_header_sizes[] = {
        1, 1, 1, 1,
        4, 2, 2, 4,
@@ -294,14 +303,14 @@ RADIUS_PACKET *fr_dhcp_recv(int sockfd)
                return NULL;
        }
 
-       if (packet->data[1] != 1) {
+       if (packet->data[1] > 1) {
                fr_strerror_printf("DHCP can only receive ethernet requests, not type %02x",
                      packet->data[1]);
                rad_free(&packet);
                return NULL;
        }
 
-       if (packet->data[2] != 6) {
+       if ((packet->data[2] != 0) && (packet->data[2] != 6)) {
                fr_strerror_printf("Ethernet HW length is wrong length %d",
                        packet->data[2]);
                rad_free(&packet);
@@ -323,15 +332,15 @@ 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] > 8)) {
-               fr_strerror_printf("Unknown value for message-type option");
+       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;
        }
@@ -391,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 <= PW_DHCP_INFORM)) {
+                   (packet->code < (1024 + DHCP_MAX_MESSAGE_TYPE))) {
                        name = dhcp_message_types[packet->code - PW_DHCP_OFFSET];
                } else {
                        snprintf(type_buf, sizeof(type_buf), "%d",
@@ -446,7 +455,7 @@ int fr_dhcp_send(RADIUS_PACKET *packet)
                char dst_ip_buf[INET6_ADDRSTRLEN];
 
                if ((packet->code >= PW_DHCP_DISCOVER) &&
-                   (packet->code <= PW_DHCP_INFORM)) {
+                   (packet->code < (1024 + DHCP_MAX_MESSAGE_TYPE))) {
                        name = dhcp_message_types[packet->code - PW_DHCP_OFFSET];
                } else {
                        snprintf(type_buf, sizeof(type_buf), "%d",
@@ -620,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
                 *
@@ -645,7 +672,7 @@ static int fr_dhcp_decode_suboption(TALLOC_CTX *ctx, VALUE_PAIR **tlv, uint8_t c
                if (!da) {
                        da = dict_unknown_afrom_fields(ctx, attr, (*tlv)->da->vendor);
                        if (!da) {
-                               pairfree(&head);
+                               fr_pair_list_free(&head);
                                return -1;
                        }
                }
@@ -654,17 +681,17 @@ static int fr_dhcp_decode_suboption(TALLOC_CTX *ctx, VALUE_PAIR **tlv, uint8_t c
                a_p = p + 2;
                num_entries = fr_dhcp_array_members(&a_len, da);
                for (i = 0; i < num_entries; i++) {
-                       vp = pairalloc(ctx, da);
+                       vp = fr_pair_afrom_da(ctx, da);
                        if (!vp) {
-                               pairfree(&head);
+                               fr_pair_list_free(&head);
                                return -1;
                        }
                        vp->op = T_OP_EQ;
-                       pairsteal(ctx, vp); /* for unknown attributes hack */
+                       fr_pair_steal(ctx, vp); /* for unknown attributes hack */
 
                        if (fr_dhcp_attr2vp(ctx, &vp, a_p, a_len) < 0) {
                                dict_attr_free(&da);
-                               pairfree(&head);
+                               fr_pair_list_free(&head);
                                goto malformed;
                        }
                        fr_cursor_merge(&cursor, vp);
@@ -701,8 +728,8 @@ static int fr_dhcp_decode_suboption(TALLOC_CTX *ctx, VALUE_PAIR **tlv, uint8_t c
        return 0;
 
 malformed:
-       pair2unknown(*tlv);
-       pairmemcpy(*tlv, data, len);
+       fr_pair_to_unknown(*tlv);
+       fr_pair_value_memcpy(*tlv, data, len);
 
        return 0;
 }
@@ -756,7 +783,7 @@ static int fr_dhcp_attr2vp(TALLOC_CTX *ctx, VALUE_PAIR **vp_p, uint8_t const *da
                q = end = data + len;
 
                if (!vp->da->flags.array) {
-                       pairbstrncpy(vp, (char const *)p, q - p);
+                       fr_pair_value_bstrncpy(vp, (char const *)p, q - p);
                        break;
                }
 
@@ -765,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;
 
-                       pairbstrncpy(vp, (char const *)p, q - p);
+                       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 = pairalloc(ctx, vp->da);
-                               if (!vp) {
-                                       pairfree(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;
@@ -798,11 +823,11 @@ static int fr_dhcp_attr2vp(TALLOC_CTX *ctx, VALUE_PAIR **vp_p, uint8_t const *da
         *      vp's original DICT_ATTR with an unknown one.
         */
        raw:
-               if (pair2unknown(vp) < 0) return -1;
+               if (fr_pair_to_unknown(vp) < 0) return -1;
 
        case PW_TYPE_OCTETS:
                if (len > 255) return -1;
-               pairmemcpy(vp, data, len);
+               fr_pair_value_memcpy(vp, data, len);
                break;
 
        /*
@@ -864,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.
                 */
@@ -871,15 +906,15 @@ ssize_t fr_dhcp_decode_options(TALLOC_CTX *ctx, VALUE_PAIR **out, uint8_t const
                if (!da) {
                        da = dict_unknown_afrom_fields(ctx, p[0], DHCP_MAGIC_VENDOR);
                        if (!da) {
-                               pairfree(out);
+                               fr_pair_list_free(out);
                                return -1;
                        }
-                       vp = pairalloc(ctx, da);
+                       vp = fr_pair_afrom_da(ctx, da);
                        if (!vp) {
-                               pairfree(out);
+                               fr_pair_list_free(out);
                                return -1;
                        }
-                       pairmemcpy(vp, a_p, a_len);
+                       fr_pair_value_memcpy(vp, a_p, a_len);
                        fr_cursor_insert(&cursor, vp);
 
                        goto next;
@@ -891,16 +926,16 @@ ssize_t fr_dhcp_decode_options(TALLOC_CTX *ctx, VALUE_PAIR **out, uint8_t const
                 */
                num_entries = fr_dhcp_array_members(&a_len, da);
                for (i = 0; i < num_entries; i++) {
-                       vp = pairalloc(ctx, da);
+                       vp = fr_pair_afrom_da(ctx, da);
                        if (!vp) {
-                               pairfree(out);
+                               fr_pair_list_free(out);
                                return -1;
                        }
                        vp->op = T_OP_EQ;
 
                        if (fr_dhcp_attr2vp(ctx, &vp, a_p, a_len) < 0) {
-                               pairfree(&vp);
-                               pairfree(out);
+                               fr_pair_list_free(&vp);
+                               fr_pair_list_free(out);
                                return -1;
                        }
                        fr_cursor_merge(&cursor, vp);
@@ -934,7 +969,7 @@ int fr_dhcp_decode(RADIUS_PACKET *packet)
                fprintf(fr_log_fp, "\n");
        }
 
-       if (packet->data[1] != 1) {
+       if (packet->data[1] > 1) {
                fr_strerror_printf("Packet is not Ethernet: %u",
                      packet->data[1]);
                return -1;
@@ -944,27 +979,33 @@ int fr_dhcp_decode(RADIUS_PACKET *packet)
         *      Decode the header.
         */
        for (i = 0; i < 14; i++) {
-               char *q;
 
-               vp = pairmake(packet, NULL, dhcp_header_names[i], NULL, T_OP_EQ);
+               vp = fr_pair_make(packet, NULL, dhcp_header_names[i], NULL, T_OP_EQ);
                if (!vp) {
                        char buffer[256];
                        strlcpy(buffer, fr_strerror(), sizeof(buffer));
                        fr_strerror_printf("Cannot decode packet due to internal error: %s", buffer);
-                       pairfree(&head);
+                       fr_pair_list_free(&head);
                        return -1;
                }
 
                /*
-                *      If chaddr does != 6 bytes it's probably not ethernet, and we should store
+                *      If chaddr != 6 bytes it's probably not ethernet, and we should store
                 *      it as an opaque type (octets).
                 */
-               if ((i == 11) && (packet->data[1] == 1) && (packet->data[2] != sizeof(vp->vp_ether))) {
-                       DICT_ATTR const *da = dict_unknown_afrom_fields(packet, vp->da->attr, vp->da->vendor);
-                       if (!da) {
-                               return -1;
+               if (i == 11) {
+                       /*
+                        *      Skip chaddr if it doesn't exist.
+                        */
+                       if ((packet->data[1] == 0) || (packet->data[2] == 0)) continue;
+
+                       if ((packet->data[1] == 1) && (packet->data[2] != sizeof(vp->vp_ether))) {
+                               DICT_ATTR const *da = dict_unknown_afrom_fields(packet, vp->da->attr, vp->da->vendor);
+                               if (!da) {
+                                       return -1;
+                               }
+                               vp->da = da;
                        }
-                       vp->da = da;
                }
 
                switch (vp->da->type) {
@@ -990,18 +1031,24 @@ 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) {
-                               pairfree(&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:
-                       pairmemcpy(vp, p, packet->data[2]);
+                       if (packet->data[2] == 0) break;
+
+                       fr_pair_value_memcpy(vp, p, packet->data[2]);
                        break;
 
                case PW_TYPE_ETHERNET:
@@ -1011,7 +1058,7 @@ int fr_dhcp_decode(RADIUS_PACKET *packet)
 
                default:
                        fr_strerror_printf("BAD TYPE %d", vp->da->type);
-                       pairfree(&vp);
+                       fr_pair_list_free(&vp);
                        break;
                }
                p += dhcp_header_sizes[i];
@@ -1039,7 +1086,7 @@ int fr_dhcp_decode(RADIUS_PACKET *packet)
                }
 
                if (options) {
-                       for (vp = fr_cursor_init(&options_cursor, options);
+                       for (vp = fr_cursor_init(&options_cursor, &options);
                             vp;
                             vp = fr_cursor_next(&options_cursor)) {
                                debug_pair(vp);
@@ -1061,19 +1108,19 @@ int fr_dhcp_decode(RADIUS_PACKET *packet)
                /*
                 *      DHCP Opcode is request
                 */
-               vp = pairfind(head, 256, DHCP_MAGIC_VENDOR, TAG_ANY);
+               vp = fr_pair_find_by_num(head, 256, DHCP_MAGIC_VENDOR, TAG_ANY);
                if (vp && vp->vp_integer == 3) {
                        /*
                         *      Vendor is "MSFT 98"
                         */
-                       vp = pairfind(head, 63, DHCP_MAGIC_VENDOR, TAG_ANY);
-                       if (vp && (strcmp(vp->vp_strvalue, "MSFT 98") == 0)) {
-                               vp = pairfind(head, 262, DHCP_MAGIC_VENDOR, TAG_ANY);
+                       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);
 
                                /*
                                 *      Reply should be broadcast.
                                 */
-                               if (vp) vp->vp_integer |= 0x8000;
+                               if (vp) vp->vp_short |= 0x8000;
                                packet->data[10] |= 0x80;
                        }
                }
@@ -1089,8 +1136,8 @@ int fr_dhcp_decode(RADIUS_PACKET *packet)
         *      Client can request a LARGER size, but not a smaller
         *      one.  They also cannot request a size larger than MTU.
         */
-       maxms = pairfind(packet->vps, 57, DHCP_MAGIC_VENDOR, TAG_ANY);
-       mtu = pairfind(packet->vps, 26, DHCP_MAGIC_VENDOR, TAG_ANY);
+       maxms = fr_pair_find_by_num(packet->vps, 57, DHCP_MAGIC_VENDOR, TAG_ANY);
+       mtu = fr_pair_find_by_num(packet->vps, 26, DHCP_MAGIC_VENDOR, TAG_ANY);
 
        if (mtu && (mtu->vp_integer < DEFAULT_PACKET_SIZE)) {
                fr_strerror_printf("DHCP Fatal: Client says MTU is smaller than minimum permitted by the specification");
@@ -1124,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;
@@ -1146,7 +1195,7 @@ int8_t fr_dhcp_attr_cmp(void const *a, void const *b)
  * @param vp option to encode.
  * @return the length of data writen, -1 if out of buffer, -2 if unsupported type.
  */
-static ssize_t fr_dhcp_vp2attr(uint8_t *out, size_t outlen, VALUE_PAIR *vp)
+static ssize_t fr_dhcp_vp2data(uint8_t *out, size_t outlen, VALUE_PAIR *vp)
 {
        uint32_t lvalue;
        uint8_t *p = out;
@@ -1182,10 +1231,6 @@ static ssize_t fr_dhcp_vp2attr(uint8_t *out, size_t outlen, VALUE_PAIR *vp)
                memcpy(p, vp->vp_strvalue, vp->vp_length);
                break;
 
-       case PW_TYPE_TLV:       /* FIXME: split it on 255? */
-               memcpy(p, vp->vp_tlv, vp->vp_length);
-               break;
-
        case PW_TYPE_OCTETS:
                memcpy(p, vp->vp_octets, vp->vp_length);
                break;
@@ -1200,89 +1245,94 @@ static ssize_t fr_dhcp_vp2attr(uint8_t *out, size_t outlen, VALUE_PAIR *vp)
 
 /** Create a new TLV attribute from multiple sub options
  *
- * @param[in,out] ctx to allocate new attribute in.
+ * @param[in,out] out buffer to write the data
+ * @param[out] outlen length of the output buffer
  * @param[in,out] cursor should be set to the start of the list of TLV attributes.
  *   Will be advanced to the first non-TLV attribute.
- * @return attribute holding the concatenation of the values of the sub options.
+ * @return length of data encoded, or -1 on error
  */
-static VALUE_PAIR *fr_dhcp_vp2suboption(TALLOC_CTX *ctx, vp_cursor_t *cursor)
+static ssize_t fr_dhcp_vp2data_tlv(uint8_t *out, ssize_t outlen, vp_cursor_t *cursor)
 {
-       ssize_t length;
+       ssize_t len;
        unsigned int parent;    /* Parent attribute of suboption */
        uint8_t attr = 0;
-       uint8_t *p, *opt_len = NULL;
-       vp_cursor_t to_pack;
-       VALUE_PAIR *vp, *tlv;
+       uint8_t *p, *opt_len;
+       vp_cursor_t tlv_cursor;
+       VALUE_PAIR *vp;
 
 #define SUBOPTION_PARENT(_x) (_x & 0xffff00ff)
 #define SUBOPTION_ATTR(_x) ((_x & 0xff00) >> 8)
 
        vp = fr_cursor_current(cursor);
-       if (!vp) return NULL;
+       if (!vp) return -1;
 
        parent = SUBOPTION_PARENT(vp->da->attr);
-       tlv = paircreate(ctx, parent, DHCP_MAGIC_VENDOR);
-       if (!tlv) return NULL;
 
-       fr_cursor_copy(&to_pack, cursor);
+       /*
+        *      Remember where we started off.
+        */
+       fr_cursor_copy(&tlv_cursor, cursor);
 
        /*
-        *  Loop over TLVs to determine how much memory we need to allocate
+        *      Loop over TLVs to determine how much memory we need to allocate
         *
-        *  We advanced the cursor we were passed, so if we fail encoding,
-        *  the cursor is at the right position for the next potentially
-        *  encodable attr.
+        *      We advanced the tlv_cursor we were passed, so if we
+        *      fail encoding, the tlv_cursor is at the right position
+        *      for the next potentially encodable attr.
         */
-       for (vp = fr_cursor_current(cursor);
-            vp && vp->da->flags.is_tlv && !vp->da->flags.extended && (SUBOPTION_PARENT(vp->da->attr) == parent);
-            vp = fr_cursor_next(cursor)) {
+       len = 0;
+       for (vp = fr_cursor_current(&tlv_cursor);
+            vp && vp->da->flags.is_tlv && (SUBOPTION_PARENT(vp->da->attr) == parent);
+            vp = fr_cursor_next(&tlv_cursor)) {
+               if (SUBOPTION_ATTR(vp->da->attr) == 0) {
+                       fr_strerror_printf("Invalid attribute number 0");
+                       return -1;
+               }
+
                /*
-                *  If it's not an array type or is an array type, but is not the same
-                *  as the previous attribute, we add 2 for the additional sub-option
-                *  header bytes.
+                *      If it's not an array type or is an array type,
+                *      but is not the same as the previous attribute,
+                *      we add 2 for the additional sub-option header
+                *      bytes.
                 */
                if (!vp->da->flags.array || (SUBOPTION_ATTR(vp->da->attr) != attr)) {
                        attr = SUBOPTION_ATTR(vp->da->attr);
-                       tlv->vp_length += 2;
+                       len += 2;
                }
-               tlv->vp_length += vp->vp_length;
+               len += vp->vp_length;
        }
 
-       tlv->vp_tlv = talloc_zero_array(tlv, uint8_t, tlv->vp_length);
-       if (!tlv->vp_tlv) {
-               talloc_free(tlv);
-               return NULL;
+       if (len > outlen) {
+               fr_strerror_printf("Insufficient room for suboption");
+               return -1;
        }
-       p = tlv->vp_tlv;
 
        attr = 0;
-       for (vp = fr_cursor_current(&to_pack);
-            vp && vp->da->flags.is_tlv && !vp->da->flags.extended && (SUBOPTION_PARENT(vp->da->attr) == parent);
-            vp = fr_cursor_next(&to_pack)) {
-               if (SUBOPTION_ATTR(vp->da->attr) == 0) {
-                       fr_strerror_printf("Invalid attribute number 0");
-                       return NULL;
-               }
-
+       opt_len = NULL;
+       p = out;
+       
+       for (vp = fr_cursor_current(cursor);
+            vp && vp->da->flags.is_tlv && (SUBOPTION_PARENT(vp->da->attr) == parent);
+            vp = fr_cursor_next(cursor)) {
                /* Don't write out the header, were packing array options */
-               if (!vp->da->flags.array || (attr != SUBOPTION_ATTR(vp->da->attr))) {
+               if (!opt_len || !vp->da->flags.array || (attr != SUBOPTION_ATTR(vp->da->attr))) {
                        attr = SUBOPTION_ATTR(vp->da->attr);
                        *p++ = attr;
                        opt_len = p++;
+                       *opt_len = 0;
                }
 
-               length = fr_dhcp_vp2attr(p, (tlv->vp_tlv + tlv->vp_length) - p, vp);
-               if ((length < 0) || (length > 255)) {
-                       talloc_free(tlv);
-                       return NULL;
+               len = fr_dhcp_vp2data(p, out + outlen - p, vp);
+               if ((len < 0) || (len > 255)) {
+                       return -1;
                }
 
-               fr_assert(opt_len);
-               *opt_len += length;
-               p += length;
+               debug_pair(vp);
+               *opt_len += len;
+               p += len;
        };
 
-       return tlv;
+       return p - out;
 }
 
 /** Encode a DHCP option and any sub-options.
@@ -1293,7 +1343,7 @@ static VALUE_PAIR *fr_dhcp_vp2suboption(TALLOC_CTX *ctx, vp_cursor_t *cursor)
  * @param cursor with current VP set to the option to be encoded. Will be advanced to the next option to encode.
  * @return > 0 length of data written, < 0 error, 0 not valid option (skipping).
  */
-ssize_t fr_dhcp_encode_option(TALLOC_CTX *ctx, uint8_t *out, size_t outlen, vp_cursor_t *cursor)
+ssize_t fr_dhcp_encode_option(UNUSED TALLOC_CTX *ctx, uint8_t *out, size_t outlen, vp_cursor_t *cursor)
 {
        VALUE_PAIR *vp;
        DICT_ATTR const *previous;
@@ -1305,7 +1355,7 @@ ssize_t fr_dhcp_encode_option(TALLOC_CTX *ctx, uint8_t *out, size_t outlen, vp_c
        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) {
@@ -1329,49 +1379,34 @@ ssize_t fr_dhcp_encode_option(TALLOC_CTX *ctx, uint8_t *out, size_t outlen, vp_c
 
        /* DHCP options with the same number get coalesced into a single option */
        do {
-               VALUE_PAIR *tlv = NULL;
-
-               /* Sub option */
-               if (vp->da->flags.is_tlv) {
-                       /*
-                        *  Coalesce TLVs into one sub-option.
-                        *  Cursor will be advanced to next non-TLV attribute.
-                        */
-                       tlv = vp = fr_dhcp_vp2suboption(ctx, cursor);
-
-                       /*
-                        *  Skip if there's an issue coalescing the sub-options.
-                        *  Cursor will still have been advanced to next non-TLV attribute.
-                        */
-                       if (!tlv) return 0;
                /*
-                *  If not calling fr_dhcp_vp2suboption() advance the cursor, so fr_cursor_current()
-                *  returns the next attribute.
+                *      Sub-option encoder will encode the data and
+                *      advance the cursor.
                 */
+               if (vp->da->flags.is_tlv) {
+                       len = fr_dhcp_vp2data_tlv(p, freespace, cursor);
+                       previous = NULL;
+
                } else {
+                       len = fr_dhcp_vp2data(p, freespace, vp);
+                       if (len >= 0) debug_pair(vp);
                        fr_cursor_next(cursor);
+                       previous = vp->da;
                }
 
-               if ((*opt_len + vp->vp_length) > 255) {
+               if (len < 0) return len;
+
+               if ((*opt_len + len) > 255) {
                        fr_strerror_printf("Skipping \"%s\": Option splitting not supported "
                                           "(option > 255 bytes)", vp->da->name);
-                       talloc_free(tlv);
                        return 0;
                }
 
-               len = fr_dhcp_vp2attr(p, freespace, vp);
-               talloc_free(tlv);
-               if (len < 0) {
-                       /* Failed encoding option */
-                       return len;
-               }
-
                p += len;
                *opt_len += len;
                freespace -= len;
 
-               previous = vp->da;
-       } while ((vp = fr_cursor_current(cursor)) && (previous == vp->da) && vp->da->flags.array);
+       } while ((vp = fr_cursor_current(cursor)) && previous && (previous == vp->da) && vp->da->flags.array);
 
        return p - out;
 }
@@ -1383,6 +1418,7 @@ int fr_dhcp_encode(RADIUS_PACKET *packet)
        vp_cursor_t cursor;
        VALUE_PAIR *vp;
        uint32_t lvalue;
+       uint16_t        svalue;
        size_t dhcp_size;
        ssize_t len;
 #ifndef NDEBUG
@@ -1402,7 +1438,7 @@ int fr_dhcp_encode(RADIUS_PACKET *packet)
        if (packet->code == 0) packet->code = PW_DHCP_NAK;
 
        /* store xid */
-       if ((vp = pairfind(packet->vps, 260, DHCP_MAGIC_VENDOR, TAG_ANY))) {
+       if ((vp = fr_pair_find_by_num(packet->vps, 260, DHCP_MAGIC_VENDOR, TAG_ANY))) {
                packet->id = vp->vp_integer;
        } else {
                packet->id = fr_rand();
@@ -1410,7 +1446,7 @@ int fr_dhcp_encode(RADIUS_PACKET *packet)
 
 #ifndef NDEBUG
        if ((packet->code >= PW_DHCP_DISCOVER) &&
-           (packet->code <= PW_DHCP_INFORM)) {
+           (packet->code < (1024 + DHCP_MAX_MESSAGE_TYPE))) {
                name = dhcp_message_types[packet->code - PW_DHCP_OFFSET];
        } else {
                name = "?Unknown?";
@@ -1450,7 +1486,7 @@ int fr_dhcp_encode(RADIUS_PACKET *packet)
         */
 
        /* DHCP-DHCP-Maximum-Msg-Size */
-       vp = pairfind(packet->vps, 57, DHCP_MAGIC_VENDOR, TAG_ANY);
+       vp = fr_pair_find_by_num(packet->vps, 57, DHCP_MAGIC_VENDOR, TAG_ANY);
        if (vp && (vp->vp_integer > mms)) {
                mms = vp->vp_integer;
 
@@ -1458,7 +1494,7 @@ int fr_dhcp_encode(RADIUS_PACKET *packet)
        }
 #endif
 
-       vp = pairfind(packet->vps, 256, DHCP_MAGIC_VENDOR, TAG_ANY);
+       vp = fr_pair_find_by_num(packet->vps, 256, DHCP_MAGIC_VENDOR, TAG_ANY);
        if (vp) {
                *p++ = vp->vp_integer & 0xff;
        } else {
@@ -1466,22 +1502,22 @@ int fr_dhcp_encode(RADIUS_PACKET *packet)
        }
 
        /* DHCP-Hardware-Type */
-       if ((vp = pairfind(packet->vps, 257, DHCP_MAGIC_VENDOR, TAG_ANY))) {
-               *p++ = vp->vp_integer & 0xFF;
+       if ((vp = fr_pair_find_by_num(packet->vps, 257, DHCP_MAGIC_VENDOR, TAG_ANY))) {
+               *p++ = vp->vp_byte;
        } else {
                *p++ = 1;               /* hardware type = ethernet */
        }
 
        /* DHCP-Hardware-Address-Length */
-       if ((vp = pairfind(packet->vps, 258, DHCP_MAGIC_VENDOR, TAG_ANY))) {
-               *p++ = vp->vp_integer & 0xFF;
+       if ((vp = fr_pair_find_by_num(packet->vps, 258, DHCP_MAGIC_VENDOR, TAG_ANY))) {
+               *p++ = vp->vp_byte;
        } else {
                *p++ = 6;               /* 6 bytes of ethernet */
        }
 
        /* DHCP-Hop-Count */
-       if ((vp = pairfind(packet->vps, 259, DHCP_MAGIC_VENDOR, TAG_ANY))) {
-               *p = vp->vp_integer & 0xff;
+       if ((vp = fr_pair_find_by_num(packet->vps, 259, DHCP_MAGIC_VENDOR, TAG_ANY))) {
+               *p = vp->vp_byte;
        }
        p++;
 
@@ -1491,27 +1527,27 @@ int fr_dhcp_encode(RADIUS_PACKET *packet)
        p += 4;
 
        /* DHCP-Number-of-Seconds */
-       if ((vp = pairfind(packet->vps, 261, DHCP_MAGIC_VENDOR, TAG_ANY))) {
-               lvalue = htonl(vp->vp_integer);
-               memcpy(p, &lvalue, 2);
+       if ((vp = fr_pair_find_by_num(packet->vps, 261, DHCP_MAGIC_VENDOR, TAG_ANY))) {
+               svalue = htons(vp->vp_short);
+               memcpy(p, &svalue, 2);
        }
        p += 2;
 
        /* DHCP-Flags */
-       if ((vp = pairfind(packet->vps, 262, DHCP_MAGIC_VENDOR, TAG_ANY))) {
-               lvalue = htons(vp->vp_integer);
-               memcpy(p, &lvalue, 2);
+       if ((vp = fr_pair_find_by_num(packet->vps, 262, DHCP_MAGIC_VENDOR, TAG_ANY))) {
+               svalue = htons(vp->vp_short);
+               memcpy(p, &svalue, 2);
        }
        p += 2;
 
        /* DHCP-Client-IP-Address */
-       if ((vp = pairfind(packet->vps, 263, DHCP_MAGIC_VENDOR, TAG_ANY))) {
+       if ((vp = fr_pair_find_by_num(packet->vps, 263, DHCP_MAGIC_VENDOR, TAG_ANY))) {
                memcpy(p, &vp->vp_ipaddr, 4);
        }
        p += 4;
 
        /* DHCP-Your-IP-address */
-       if ((vp = pairfind(packet->vps, 264, DHCP_MAGIC_VENDOR, TAG_ANY))) {
+       if ((vp = fr_pair_find_by_num(packet->vps, 264, DHCP_MAGIC_VENDOR, TAG_ANY))) {
                lvalue = vp->vp_ipaddr;
        } else {
                lvalue = htonl(INADDR_ANY);
@@ -1520,7 +1556,7 @@ int fr_dhcp_encode(RADIUS_PACKET *packet)
        p += 4;
 
        /* DHCP-Server-IP-Address */
-       vp = pairfind(packet->vps, 265, DHCP_MAGIC_VENDOR, TAG_ANY);
+       vp = fr_pair_find_by_num(packet->vps, 265, DHCP_MAGIC_VENDOR, TAG_ANY);
        if (vp) {
                lvalue = vp->vp_ipaddr;
        } else {
@@ -1532,7 +1568,7 @@ int fr_dhcp_encode(RADIUS_PACKET *packet)
        /*
         *      DHCP-Gateway-IP-Address
         */
-       if ((vp = pairfind(packet->vps, 266, DHCP_MAGIC_VENDOR, TAG_ANY))) {
+       if ((vp = fr_pair_find_by_num(packet->vps, 266, DHCP_MAGIC_VENDOR, TAG_ANY))) {
                lvalue = vp->vp_ipaddr;
        } else {
                lvalue = htonl(INADDR_ANY);
@@ -1541,15 +1577,22 @@ int fr_dhcp_encode(RADIUS_PACKET *packet)
        p += 4;
 
        /* DHCP-Client-Hardware-Address */
-       if ((vp = pairfind(packet->vps, 267, DHCP_MAGIC_VENDOR, TAG_ANY))) {
+       if ((vp = fr_pair_find_by_num(packet->vps, 267, DHCP_MAGIC_VENDOR, TAG_ANY))) {
                if (vp->vp_length == sizeof(vp->vp_ether)) {
+                       /*
+                        *      Ensure that we mark the packet as being Ethernet.
+                        *      This is mainly for DHCP-Lease-Query responses.
+                        */
+                       packet->data[1] = 1;
+                       packet->data[2] = 6;
+
                        memcpy(p, vp->vp_ether, vp->vp_length);
                } /* else ignore it */
        }
        p += DHCP_CHADDR_LEN;
 
        /* DHCP-Server-Host-Name */
-       if ((vp = pairfind(packet->vps, 268, DHCP_MAGIC_VENDOR, TAG_ANY))) {
+       if ((vp = fr_pair_find_by_num(packet->vps, 268, DHCP_MAGIC_VENDOR, TAG_ANY))) {
                if (vp->vp_length > DHCP_SNAME_LEN) {
                        memcpy(p, vp->vp_strvalue, DHCP_SNAME_LEN);
                } else {
@@ -1569,7 +1612,7 @@ int fr_dhcp_encode(RADIUS_PACKET *packet)
         */
 
        /* DHCP-Boot-Filename */
-       vp = pairfind(packet->vps, 269, DHCP_MAGIC_VENDOR, TAG_ANY);
+       vp = fr_pair_find_by_num(packet->vps, 269, DHCP_MAGIC_VENDOR, TAG_ANY);
        if (vp) {
                if (vp->vp_length > DHCP_FILE_LEN) {
                        memcpy(p, vp->vp_strvalue, DHCP_FILE_LEN);
@@ -1595,7 +1638,7 @@ int fr_dhcp_encode(RADIUS_PACKET *packet)
                for (i = 0; i < 14; i++) {
                        char *q;
 
-                       vp = pairmake(packet, NULL,
+                       vp = fr_pair_make(packet, NULL,
                                      dhcp_header_names[i], NULL, T_OP_EQ);
                        if (!vp) {
                                char buffer[256];
@@ -1631,7 +1674,7 @@ int fr_dhcp_encode(RADIUS_PACKET *packet)
                                break;
 
                        case PW_TYPE_OCTETS: /* only for Client HW Address */
-                               pairmemcpy(vp, p, packet->data[2]);
+                               fr_pair_value_memcpy(vp, p, packet->data[2]);
                                break;
 
                        case PW_TYPE_ETHERNET: /* only for Client HW Address */
@@ -1640,14 +1683,14 @@ int fr_dhcp_encode(RADIUS_PACKET *packet)
 
                        default:
                                fr_strerror_printf("Internal sanity check failed %d %d", vp->da->type, __LINE__);
-                               pairfree(&vp);
+                               fr_pair_list_free(&vp);
                                break;
                        }
 
                        p += dhcp_header_sizes[i];
 
                        debug_pair(vp);
-                       pairfree(&vp);
+                       fr_pair_list_free(&vp);
                }
 
                /*
@@ -1661,12 +1704,11 @@ 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...
         */
-       pairsort(&packet->vps, fr_dhcp_attr_cmp);
+       fr_pair_list_sort(&packet->vps, fr_dhcp_attr_cmp);
        fr_cursor_init(&cursor, &packet->vps);
 
        /*
@@ -1676,7 +1718,6 @@ int fr_dhcp_encode(RADIUS_PACKET *packet)
        while ((vp = fr_cursor_current(&cursor))) {
                len = fr_dhcp_encode_option(packet, p, packet->data_len - (p - packet->data), &cursor);
                if (len < 0) break;
-               if (len > 0) debug_pair(vp);
                p += len;
        };
 
@@ -1822,7 +1863,7 @@ int fr_dhcp_send_raw_packet(int sockfd, struct sockaddr_ll *p_ll, RADIUS_PACKET
 
        /* set ethernet source address to our MAC address (DHCP-Client-Hardware-Address). */
        u_char dhmac[ETH_ADDR_LEN] = { 0 };
-       if ((vp = pairfind(packet->vps, 267, DHCP_MAGIC_VENDOR, TAG_ANY))) {
+       if ((vp = fr_pair_find_by_num(packet->vps, 267, DHCP_MAGIC_VENDOR, TAG_ANY))) {
                if (vp->length == sizeof(vp->vp_ether)) {
                        memcpy(dhmac, vp->vp_ether, vp->length);
                }
@@ -1879,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 <= PW_DHCP_INFORM)) {
+                   (packet->code < (1024 + DHCP_MAX_MESSAGE_TYPE))) {
                        name = dhcp_message_types[packet->code - PW_DHCP_OFFSET];
                } else {
                        snprintf(type_buf, sizeof(type_buf), "%d",
@@ -1968,7 +2009,7 @@ RADIUS_PACKET *fr_dhcp_recv_raw_packet(int sockfd, struct sockaddr_ll *p_ll, RAD
         * Check if it matches the source HW address used (DHCP-Client-Hardware-Address = 267)
         */
        if ( (memcmp(&eth_bcast, &eth_hdr->ether_dst, ETH_ADDR_LEN) != 0) &&
-                       (vp = pairfind(request->vps, 267, DHCP_MAGIC_VENDOR, TAG_ANY)) &&
+                       (vp = fr_pair_find_by_num(request->vps, 267, DHCP_MAGIC_VENDOR, TAG_ANY)) &&
                        (vp->length == sizeof(vp->vp_ether)) &&
                        (memcmp(vp->vp_ether, &eth_hdr->ether_dst, ETH_ADDR_LEN) != 0) ) {
                /* No match. */
@@ -2038,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);
@@ -2082,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 <= PW_DHCP_INFORM)) {
+                   (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);