Rework the packet.c code for decoding and assembling DHCP messages.

This code suffered from many years of incremental improvements, fix it to:
- verify available buffer space in all cases and don't just trust the caller
- have an API where you always pass the full buffer length and absolute offset
- use consistent types for lengths and buffer offsets (int vs. size_t, ssize_t)
- don't just ignore errors, return and fail when something is wrong

OK rzalamena@

diff --git a/usr.sbin/dhcrelay/bpf.c b/usr.sbin/dhcrelay/bpf.c
index 275d307..b842cff 100644 (file)
--- a/usr.sbin/dhcrelay/bpf.c
+++ b/usr.sbin/dhcrelay/bpf.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: bpf.c,v 1.17 2017/02/13 22:49:38 krw Exp $ */
+/*     $OpenBSD: bpf.c,v 1.18 2017/04/05 14:40:56 reyk Exp $ */
 
 /* BPF socket interface code, originally contributed by Archie Cobbs. */
 
@@ -335,7 +335,10 @@ send_packet(struct interface_info *interface,
 {
        unsigned char buf[256];
        struct iovec iov[2];
-       int result, bufp = 0;
+       ssize_t bufp;
+       int result;
+
+       result = -1;
 
        if (interface->hw_address.htype == HTYPE_IPSEC_TUNNEL) {
                socklen_t slen = pc->pc_dst.ss_len;
@@ -345,9 +348,12 @@ send_packet(struct interface_info *interface,
        }
 
        /* Assemble the headers... */
-       assemble_hw_header(interface, buf, &bufp, pc);
-       assemble_udp_ip_header(interface, buf, &bufp, pc,
-           (unsigned char *)raw, len);
+       if ((bufp = assemble_hw_header(buf, sizeof(buf), 0, pc,
+           interface->hw_address.htype)) == -1)
+               goto done;
+       if ((bufp = assemble_udp_ip_header(buf, sizeof(buf), bufp, pc,
+           (unsigned char *)raw, len)) == -1)
+               goto done;
 
        /* Fire it off */
        iov[0].iov_base = (char *)buf;
@@ -366,7 +372,8 @@ ssize_t
 receive_packet(struct interface_info *interface, unsigned char *buf,
     size_t len, struct packet_ctx *pc)
 {
-       int length = 0, offset = 0;
+       int length = 0;
+       ssize_t offset = 0;
        struct bpf_hdr hdr;
 
        /*
@@ -430,32 +437,32 @@ receive_packet(struct interface_info *interface, unsigned char *buf,
                interface->rbuf_offset += hdr.bh_hdrlen;
 
                /* Decode the physical header... */
-               offset = decode_hw_header(interface,
-                   interface->rbuf, interface->rbuf_offset, pc);
+               offset = decode_hw_header(interface->rbuf,
+                   interface->rbuf_len, interface->rbuf_offset, pc,
+                   interface->hw_address.htype);
 
                /*
-                * If a physical layer checksum failed (dunno of any
-                * physical layer that supports this, but WTH), skip
-                * this packet.
+                * If decoding or a physical layer checksum failed
+                * (dunno of any physical layer that supports this, but WTH),
+                * skip this packet.
                 */
                if (offset < 0) {
                        interface->rbuf_offset += hdr.bh_caplen;
                        continue;
                }
-               interface->rbuf_offset += offset;
-               hdr.bh_caplen -= offset;
 
                /* Decode the IP and UDP headers... */
-               offset = decode_udp_ip_header(interface, interface->rbuf,
-                   interface->rbuf_offset, pc, hdr.bh_caplen);
+               offset = decode_udp_ip_header(interface->rbuf,
+                   interface->rbuf_len, offset, pc);
 
                /* If the IP or UDP checksum was bad, skip the packet... */
                if (offset < 0) {
                        interface->rbuf_offset += hdr.bh_caplen;
                        continue;
                }
-               interface->rbuf_offset += offset;
-               hdr.bh_caplen -= offset;
+
+               hdr.bh_caplen -= offset - interface->rbuf_offset;
+               interface->rbuf_offset = offset;
 
                /*
                 * If there's not enough room to stash the packet data,

diff --git a/usr.sbin/dhcrelay/dhcpd.h b/usr.sbin/dhcrelay/dhcpd.h
index 239f605..4a7dd50 100644 (file)
--- a/usr.sbin/dhcrelay/dhcpd.h
+++ b/usr.sbin/dhcrelay/dhcpd.h
@@ -1,4 +1,4 @@
-/*     $OpenBSD: dhcpd.h,v 1.22 2017/04/04 15:50:29 reyk Exp $ */
+/*     $OpenBSD: dhcpd.h,v 1.23 2017/04/05 14:40:56 reyk Exp $ */
 
 /*
  * Copyright (c) 2004 Henning Brauer <henning@openbsd.org>
@@ -168,14 +168,14 @@ void add_protocol(char *, int, void (*)(struct protocol *), void *);
 void remove_protocol(struct protocol *);
 
 /* packet.c */
-void assemble_hw_header(struct interface_info *, unsigned char *,
-    int *, struct packet_ctx *);
-void assemble_udp_ip_header(struct interface_info *, unsigned char *,
-    int *, struct packet_ctx *pc, unsigned char *, int);
-ssize_t decode_hw_header(struct interface_info *, unsigned char *,
-    int, struct packet_ctx *);
-ssize_t decode_udp_ip_header(struct interface_info *, unsigned char *,
-    int, struct packet_ctx *, int);
+ssize_t assemble_hw_header(unsigned char *, size_t, size_t,
+    struct packet_ctx *, unsigned int);
+ssize_t assemble_udp_ip_header(unsigned char *, size_t, size_t,
+    struct packet_ctx *pc, unsigned char *, size_t);
+ssize_t decode_hw_header(unsigned char *, size_t, size_t, struct packet_ctx *,
+    unsigned int);
+ssize_t decode_udp_ip_header(unsigned char *, size_t, size_t,
+    struct packet_ctx *);
 
 /* dhcrelay.c */
 extern int server_fd;

diff --git a/usr.sbin/dhcrelay/packet.c b/usr.sbin/dhcrelay/packet.c
index 506eaef..514fc0b 100644 (file)
--- a/usr.sbin/dhcrelay/packet.c
+++ b/usr.sbin/dhcrelay/packet.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: packet.c,v 1.13 2017/02/13 19:15:39 krw Exp $ */
+/*     $OpenBSD: packet.c,v 1.14 2017/04/05 14:40:56 reyk Exp $        */
 
 /* Packet assembly code, originally contributed by Archie Cobbs. */
 
@@ -60,13 +60,13 @@
 #include "log.h"
 
 
-u_int32_t      checksum(unsigned char *, unsigned, u_int32_t);
+u_int32_t      checksum(unsigned char *, u_int32_t, u_int32_t);
 u_int32_t      wrapsum(u_int32_t);
 
 u_int32_t
-checksum(unsigned char *buf, unsigned nbytes, u_int32_t sum)
+checksum(unsigned char *buf, u_int32_t nbytes, u_int32_t sum)
 {
-       int i;
+       u_int32_t i;
 
        /* Checksum all the pairs of bytes first... */
        for (i = 0; i < (nbytes & ~1U); i += 2) {
@@ -96,33 +96,47 @@ wrapsum(u_int32_t sum)
        return (htons(sum));
 }
 
-void
-assemble_hw_header(struct interface_info *interface, unsigned char *buf,
-    int *bufix, struct packet_ctx *pc)
+ssize_t
+assemble_hw_header(unsigned char *buf, size_t buflen,
+    size_t offset, struct packet_ctx *pc, unsigned int intfhtype)
 {
        struct ether_header eh;
 
-       /* Use the supplied address or let the kernel fill it. */
-       memcpy(eh.ether_shost, pc->pc_smac, ETHER_ADDR_LEN);
-       memcpy(eh.ether_dhost, pc->pc_dmac, ETHER_ADDR_LEN);
+       switch (intfhtype) {
+       case HTYPE_ETHER:
+               if (buflen < offset + ETHER_HDR_LEN)
+                       return (-1);
+
+               /* Use the supplied address or let the kernel fill it. */
+               memcpy(eh.ether_shost, pc->pc_smac, ETHER_ADDR_LEN);
+               memcpy(eh.ether_dhost, pc->pc_dmac, ETHER_ADDR_LEN);
 
-       eh.ether_type = htons(ETHERTYPE_IP);
+               eh.ether_type = htons(ETHERTYPE_IP);
+
+               memcpy(&buf[offset], &eh, ETHER_HDR_LEN);
+               offset += ETHER_HDR_LEN;
+               break;
+       default:
+               return (-1);
+       }
 
-       memcpy(&buf[*bufix], &eh, ETHER_HDR_LEN);
-       *bufix += ETHER_HDR_LEN;
+       return (offset);
 }
 
-void
-assemble_udp_ip_header(struct interface_info *interface, unsigned char *buf,
-    int *bufix, struct packet_ctx *pc, unsigned char *data, int len)
+ssize_t
+assemble_udp_ip_header(unsigned char *buf, size_t buflen, size_t offset,
+    struct packet_ctx *pc, unsigned char *data, size_t datalen)
 {
        struct ip ip;
        struct udphdr udp;
 
+       if (buflen < offset + sizeof(ip) + sizeof(udp))
+               return (-1);
+
        ip.ip_v = 4;
        ip.ip_hl = 5;
        ip.ip_tos = IPTOS_LOWDELAY;
-       ip.ip_len = htons(sizeof(ip) + sizeof(udp) + len);
+       ip.ip_len = htons(sizeof(ip) + sizeof(udp) + datalen);
        ip.ip_id = 0;
        ip.ip_off = 0;
        ip.ip_ttl = 16;
@@ -132,59 +146,75 @@ assemble_udp_ip_header(struct interface_info *interface, unsigned char *buf,
        ip.ip_dst.s_addr = ss2sin(&pc->pc_dst)->sin_addr.s_addr;
 
        ip.ip_sum = wrapsum(checksum((unsigned char *)&ip, sizeof(ip), 0));
-       memcpy(&buf[*bufix], &ip, sizeof(ip));
-       *bufix += sizeof(ip);
+       memcpy(&buf[offset], &ip, sizeof(ip));
+       offset += sizeof(ip);
 
        udp.uh_sport = ss2sin(&pc->pc_src)->sin_port;
        udp.uh_dport = ss2sin(&pc->pc_dst)->sin_port;
-       udp.uh_ulen = htons(sizeof(udp) + len);
+       udp.uh_ulen = htons(sizeof(udp) + datalen);
        memset(&udp.uh_sum, 0, sizeof(udp.uh_sum));
 
        udp.uh_sum = wrapsum(checksum((unsigned char *)&udp, sizeof(udp),
-           checksum(data, len, checksum((unsigned char *)&ip.ip_src,
+           checksum(data, datalen, checksum((unsigned char *)&ip.ip_src,
            2 * sizeof(ip.ip_src),
            IPPROTO_UDP + (u_int32_t)ntohs(udp.uh_ulen)))));
 
-       memcpy(&buf[*bufix], &udp, sizeof(udp));
-       *bufix += sizeof(udp);
+       memcpy(&buf[offset], &udp, sizeof(udp));
+       offset += sizeof(udp);
+
+       return (offset);
 }
 
 ssize_t
-decode_hw_header(struct interface_info *interface, unsigned char *buf,
-    int bufix, struct packet_ctx *pc)
+decode_hw_header(unsigned char *buf, size_t buflen,
+    size_t offset, struct packet_ctx *pc, unsigned int intfhtype)
 {
-       size_t offset = 0;
+       u_int32_t ip_len;
+       struct ip *ip;
 
-       if (interface->hw_address.htype == HTYPE_IPSEC_TUNNEL) {
-               u_int32_t ip_len;
-               struct ip *ip;
+       switch (intfhtype) {
+       case HTYPE_IPSEC_TUNNEL:
+               if (buflen < offset + ENC_HDRLEN + sizeof(*ip))
+                       return (-1);
+               offset += ENC_HDRLEN;
+               ip_len = (buf[offset] & 0xf) << 2;
+               if (buflen < offset + ip_len)
+                       return (-1);
 
-               bufix += ENC_HDRLEN;
-               ip_len = (buf[bufix] & 0xf) << 2;
-               ip = (struct ip *)(buf + bufix);
+               ip = (struct ip *)(buf + offset);
 
                /* Encapsulated IP */
                if (ip->ip_p != IPPROTO_IPIP)
                        return (-1);
 
                memset(pc->pc_dmac, 0xff, ETHER_ADDR_LEN);
-               offset = ENC_HDRLEN + ip_len;
-       } else {
-               memcpy(pc->pc_dmac, buf + bufix, ETHER_ADDR_LEN);
-               memcpy(pc->pc_smac, buf + bufix + ETHER_ADDR_LEN,
+               offset += ip_len;
+
+               pc->pc_htype = ARPHRD_ETHER;
+               pc->pc_hlen = ETHER_ADDR_LEN;
+               break;
+       case HTYPE_ETHER:
+               if (buflen < offset + ETHER_HDR_LEN)
+                       return (-1);
+
+               memcpy(pc->pc_dmac, buf + offset, ETHER_ADDR_LEN);
+               memcpy(pc->pc_smac, buf + offset + ETHER_ADDR_LEN,
                    ETHER_ADDR_LEN);
-               offset = sizeof(struct ether_header);
-       }
+               offset += ETHER_HDR_LEN;
 
-       pc->pc_htype = ARPHRD_ETHER;
-       pc->pc_hlen = ETHER_ADDR_LEN;
+               pc->pc_htype = ARPHRD_ETHER;
+               pc->pc_hlen = ETHER_ADDR_LEN;
+               break;
+       default:
+               return (-1);
+       }
 
        return (offset);
 }
 
 ssize_t
-decode_udp_ip_header(struct interface_info *interface, unsigned char *buf,
-    int bufix, struct packet_ctx *pc, int buflen)
+decode_udp_ip_header(unsigned char *buf, size_t buflen,
+    size_t offset, struct packet_ctx *pc)
 {
        struct ip *ip;
        struct udphdr *udp;
@@ -200,16 +230,17 @@ decode_udp_ip_header(struct interface_info *interface, unsigned char *buf,
        int len;
 
        /* Assure that an entire IP header is within the buffer. */
-       if (sizeof(*ip) > buflen)
+       if (buflen < offset + sizeof(*ip))
                return (-1);
-       ip_len = (buf[bufix] & 0xf) << 2;
-       if (ip_len > buflen)
+       ip_len = (buf[offset] & 0xf) << 2;
+       if (buflen < offset + ip_len)
                return (-1);
-       ip = (struct ip *)(buf + bufix);
+
+       ip = (struct ip *)(buf + offset);
        ip_packets_seen++;
 
        /* Check the IP header checksum - it should be zero. */
-       if (wrapsum(checksum(buf + bufix, ip_len, 0)) != 0) {
+       if (wrapsum(checksum(buf + offset, ip_len, 0)) != 0) {
                ip_packets_bad_checksum++;
                if (ip_packets_seen > 4 && ip_packets_bad_checksum != 0 &&
                    (ip_packets_seen / ip_packets_bad_checksum) < 2) {
@@ -231,25 +262,25 @@ decode_udp_ip_header(struct interface_info *interface, unsigned char *buf,
            sizeof(ss2sin(&pc->pc_dst)->sin_addr));
 
 #ifdef DEBUG
-       if (ntohs(ip->ip_len) != buflen)
-               log_debug("ip length %d disagrees with bytes received %d.",
-                   ntohs(ip->ip_len), buflen);
+       if (buflen != offset + ntohs(ip->ip_len))
+               log_debug("ip length %d disagrees with bytes received %zd.",
+                   ntohs(ip->ip_len), buflen - offset);
 #endif
 
        /* Assure that the entire IP packet is within the buffer. */
-       if (ntohs(ip->ip_len) > buflen)
+       if (buflen < offset + ntohs(ip->ip_len))
                return (-1);
 
        /* Assure that the UDP header is within the buffer. */
-       if (ip_len + sizeof(*udp) > buflen)
+       if (buflen < offset + ip_len + sizeof(*udp))
                return (-1);
-       udp = (struct udphdr *)(buf + bufix + ip_len);
+       udp = (struct udphdr *)(buf + offset + ip_len);
        udp_packets_seen++;
 
        /* Assure that the entire UDP packet is within the buffer. */
-       if (ip_len + ntohs(udp->uh_ulen) > buflen)
+       if (buflen < offset + ip_len + ntohs(udp->uh_ulen))
                return (-1);
-       data = buf + bufix + ip_len + sizeof(*udp);
+       data = buf + offset + ip_len + sizeof(*udp);
 
        /*
         * Compute UDP checksums, including the ``pseudo-header'', the
@@ -258,7 +289,7 @@ decode_udp_ip_header(struct interface_info *interface, unsigned char *buf,
         */
        udp_packets_length_checked++;
        len = ntohs(udp->uh_ulen) - sizeof(*udp);
-       if ((len < 0) || (len + data > buf + bufix + buflen)) {
+       if ((len < 0) || (len + data > buf + buflen)) {
                udp_packets_length_overflow++;
                if (udp_packets_length_checked > 4 &&
                    udp_packets_length_overflow != 0 &&
@@ -272,7 +303,7 @@ decode_udp_ip_header(struct interface_info *interface, unsigned char *buf,
                }
                return (-1);
        }
-       if (len + data != buf + bufix + buflen)
+       if (len + data != buf + buflen)
                log_debug("accepting packet with data after udp payload.");
 
        usum = udp->uh_sum;
@@ -298,5 +329,5 @@ decode_udp_ip_header(struct interface_info *interface, unsigned char *buf,
        ss2sin(&pc->pc_src)->sin_port = udp->uh_sport;
        ss2sin(&pc->pc_dst)->sin_port = udp->uh_dport;
 
-       return (ip_len + sizeof(*udp));
+       return (offset + ip_len + sizeof(*udp));
 }