From 22c60a6bb555fc9852c456dbd832cc83f94e6274 Mon Sep 17 00:00:00 2001 From: reyk Date: Wed, 5 Apr 2017 14:40:56 +0000 Subject: [PATCH] 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@ --- usr.sbin/dhcrelay/bpf.c | 41 ++++++----- usr.sbin/dhcrelay/dhcpd.h | 18 ++--- usr.sbin/dhcrelay/packet.c | 147 ++++++++++++++++++++++--------------- 3 files changed, 122 insertions(+), 84 deletions(-) diff --git a/usr.sbin/dhcrelay/bpf.c b/usr.sbin/dhcrelay/bpf.c index 275d3074f07..b842cffc5fd 100644 --- 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 239f6053b04..4a7dd50fa61 100644 --- 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 @@ -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 506eaef1f4b..514fc0b2d8c 100644 --- 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)); } -- 2.20.1