From: Guy Harris Date: Thu, 1 Aug 2024 09:35:48 +0000 (-0700) Subject: Don't increment packet buffer pointers past the end of the data. X-Git-Url: https://git.tcpdump.org/libpcap/commitdiff_plain/43b06fd35c337728266254d4df6181a63b36f07e?ds=sidebyside Don't increment packet buffer pointers past the end of the data. For the BPF, Airpcap, and NPF (WinPcap/Npcap) capture mechanisms, each entry in the buffer read from the capture mechanism is aligned on a word boundary, for some value of "word". This means that all entries *except for the last of them* has padding at the end. If we have bp as a pointer to the current entry and ep as a pointer to the end of the buffer, calculated by adding the number of bytes read to a (byte) pointer to the beginning of the buffer, and loop as long as bp < ep, incrementing bp by the length of the entry with padding added, this will put bp *past* the end of the buffer for the last entry, as the last entry lacks padding. Instead, compute the minimum of the rounded-up length of the entry and the space remaining in the buffer (ep - bp), which means that value will not include padding for the last entry in the buffer, and add that to bp after processing a packet. That ensures that bp will not go past ep. This addresses the issue mentioned in pull request #1339. (That issue is unlikely to be a problem, in practice, in any platform on which we run, but we should fix it anyway.) We also change the type of the count of bytes in the buffer to a u_int, which also means we change the stored count in a pcap_t and variables to which that's assigned, as well as some other variables, and rewrite some code to reflect that. --- diff --git a/pcap-airpcap.c b/pcap-airpcap.c index 7166ca0f..af4de349 100644 --- a/pcap-airpcap.c +++ b/pcap-airpcap.c @@ -577,7 +577,7 @@ static int airpcap_read(pcap_t *p, int cnt, pcap_handler callback, u_char *user) { struct pcap_airpcap *pa = p->priv; - int cc; + u_int cc; int n; register u_char *bp, *ep; UINT bytes_read; @@ -618,6 +618,13 @@ airpcap_read(pcap_t *p, int cnt, pcap_handler callback, u_char *user) p_AirpcapGetLastError(pa->adapter)); return (-1); } + + /* + * At this point, read_ret is guaranteed to be + * >= 0 and < p->bufsize; p->bufsize is a u_int, + * so its value is guaranteed to fit in cc, which + * is also a u_int. + */ cc = bytes_read; bp = p->buffer; } else @@ -634,6 +641,7 @@ airpcap_read(pcap_t *p, int cnt, pcap_handler callback, u_char *user) ep = bp + cc; for (;;) { register u_int caplen, hdrlen; + size_t packet_bytes; /* * Has "pcap_breakloop()" been called? @@ -651,7 +659,7 @@ airpcap_read(pcap_t *p, int cnt, pcap_handler callback, u_char *user) return (PCAP_ERROR_BREAK); } else { p->bp = bp; - p->cc = (int) (ep - bp); + p->cc = (u_int) (ep - bp); return (n); } } @@ -661,6 +669,22 @@ airpcap_read(pcap_t *p, int cnt, pcap_handler callback, u_char *user) caplen = bhp->Caplen; hdrlen = bhp->Hdrlen; datap = bp + hdrlen; + + /* + * Compute the number of bytes for this packet in + * the buffer. + * + * That's the sum of the header length and the packet + * data length plus, if this is not the last packet, + * the padding required to align the next packet on + * the appropriate boundary. + * + * That means that it should be the minimum of the + * number of bytes left in the buffer and the + * rounded-up sum of the header and packet data lengths. + */ + packet_bytes = min((u_int)(ep - bp), AIRPCAP_WORDALIGN(caplen + hdrlen)); + /* * Short-circuit evaluation: if using BPF filter * in the AirPcap adapter, no need to do it now - @@ -676,17 +700,17 @@ airpcap_read(pcap_t *p, int cnt, pcap_handler callback, u_char *user) pkthdr.caplen = caplen; pkthdr.len = bhp->Originallen; (*callback)(user, &pkthdr, datap); - bp += AIRPCAP_WORDALIGN(caplen + hdrlen); + bp += packet_bytes; if (++n >= cnt && !PACKET_COUNT_IS_UNLIMITED(cnt)) { p->bp = bp; - p->cc = (int)(ep - bp); + p->cc = (u_int)(ep - bp); return (n); } } else { /* * Skip this packet. */ - bp += AIRPCAP_WORDALIGN(caplen + hdrlen); + bp += packet_bytes; } } #undef bhp diff --git a/pcap-bpf.c b/pcap-bpf.c index c678dcf5..fbae364f 100644 --- a/pcap-bpf.c +++ b/pcap-bpf.c @@ -290,7 +290,7 @@ pcap_setnonblock_bpf(pcap_t *p, int nonblock) * buffer filled for a fresh BPF session. */ static int -pcap_next_zbuf_shm(pcap_t *p, ssize_t *cc) +pcap_next_zbuf_shm(pcap_t *p, u_int *cc) { struct pcap_bpf *pb = p->priv; struct bpf_zbuf_header *bzh; @@ -328,7 +328,7 @@ pcap_next_zbuf_shm(pcap_t *p, ssize_t *cc) * work. */ static int -pcap_next_zbuf(pcap_t *p, ssize_t *cc) +pcap_next_zbuf(pcap_t *p, u_int *cc) { struct pcap_bpf *pb = p->priv; struct bpf_zbuf bz; @@ -1178,16 +1178,13 @@ static int pcap_read_bpf(pcap_t *p, int cnt, pcap_handler callback, u_char *user) { struct pcap_bpf *pb = p->priv; - ssize_t cc; + u_int cc; int n = 0; register u_char *bp, *ep; u_char *datap; #ifdef PCAP_FDDIPAD register u_int pad; #endif -#ifdef HAVE_ZEROCOPY_BPF - int i; -#endif again: /* @@ -1214,18 +1211,22 @@ pcap_read_bpf(pcap_t *p, int cnt, pcap_handler callback, u_char *user) */ #ifdef HAVE_ZEROCOPY_BPF if (pb->zerocopy) { + int next_zbuf_ret; + if (p->buffer != NULL) pcap_ack_zbuf(p); - i = pcap_next_zbuf(p, &cc); - if (i == 0) + next_zbuf_ret = pcap_next_zbuf(p, &cc); + if (next_zbuf_ret == 0) goto again; - if (i < 0) - return (PCAP_ERROR); + if (next_zbuf_ret < 0) + return (next_zbuf_ret); } else #endif { - cc = read(p->fd, p->buffer, p->bufsize); - if (cc < 0) { + ssize_t read_ret; + + read_ret = read(p->fd, p->buffer, p->bufsize); + if (read_ret < 0) { /* Don't choke when we get ptraced */ switch (errno) { @@ -1302,6 +1303,14 @@ pcap_read_bpf(pcap_t *p, int cnt, pcap_handler callback, u_char *user) errno, "read"); return (PCAP_ERROR); } + + /* + * At this point, read_ret is guaranteed to be + * >= 0 and < p->bufsize; p->bufsize is a u_int, + * so its value is guaranteed to fit in cc, which + * is also a u_int. + */ + cc = (u_int)read_ret; } bp = p->buffer; } else @@ -1324,6 +1333,7 @@ pcap_read_bpf(pcap_t *p, int cnt, pcap_handler callback, u_char *user) #endif while (bp < ep) { register u_int caplen, hdrlen; + size_t packet_bytes; /* * Has "pcap_breakloop()" been called? @@ -1337,22 +1347,7 @@ pcap_read_bpf(pcap_t *p, int cnt, pcap_handler callback, u_char *user) */ if (p->break_loop) { p->bp = bp; - p->cc = (int)(ep - bp); - /* - * ep is set based on the return value of read(), - * but read() from a BPF device doesn't necessarily - * return a value that's a multiple of the alignment - * value for BPF_WORDALIGN(). However, whenever we - * increment bp, we round up the increment value by - * a value rounded up by BPF_WORDALIGN(), so we - * could increment bp past ep after processing the - * last packet in the buffer. - * - * We treat ep < bp as an indication that this - * happened, and just set p->cc to 0. - */ - if (p->cc < 0) - p->cc = 0; + p->cc = (u_int)(ep - bp); if (n == 0) { p->break_loop = 0; return (PCAP_ERROR_BREAK); @@ -1363,6 +1358,22 @@ pcap_read_bpf(pcap_t *p, int cnt, pcap_handler callback, u_char *user) caplen = bhp->bh_caplen; hdrlen = bhp->bh_hdrlen; datap = bp + hdrlen; + + /* + * Compute the number of bytes for this packet in + * the buffer. + * + * That's the sum of the header length and the packet + * data length plus, if this is not the last packet, + * the padding required to align the next packet on + * the appropriate boundary. + * + * That means that it should be the minimum of the + * number of bytes left in the buffer (ep - bp) and the + * rounded-up sum of the header and packet data lengths. + */ + packet_bytes = min((u_int)(ep - bp), BPF_WORDALIGN(caplen + hdrlen)); + /* * Short-circuit evaluation: if using BPF filter * in kernel, no need to do it now - we already know @@ -1430,22 +1441,17 @@ pcap_read_bpf(pcap_t *p, int cnt, pcap_handler callback, u_char *user) pkthdr.len = bhp->bh_datalen; #endif (*callback)(user, &pkthdr, datap); - bp += BPF_WORDALIGN(caplen + hdrlen); + bp += packet_bytes; if (++n >= cnt && !PACKET_COUNT_IS_UNLIMITED(cnt)) { p->bp = bp; - p->cc = (int)(ep - bp); - /* - * See comment above about p->cc < 0. - */ - if (p->cc < 0) - p->cc = 0; + p->cc = (u_int)(ep - bp); return (n); } } else { /* * Skip this packet. */ - bp += BPF_WORDALIGN(caplen + hdrlen); + bp += packet_bytes; } } #undef bhp diff --git a/pcap-int.h b/pcap-int.h index 0138ce32..3b854929 100644 --- a/pcap-int.h +++ b/pcap-int.h @@ -240,7 +240,7 @@ struct pcap { u_int bufsize; u_char *buffer; u_char *bp; - int cc; + u_int cc; sig_atomic_t break_loop; /* flag set to force break from packet-reading loop */ @@ -267,7 +267,7 @@ struct pcap { int snapshot; int linktype; /* Network linktype */ int linktype_ext; /* Extended information stored in the linktype field of a file */ - int offset; /* offset for proper alignment */ + u_int offset; /* offset for proper alignment */ int activated; /* true if the capture is really started */ int oldstyle; /* if we're opening with pcap_open_live() */ diff --git a/pcap-linux.c b/pcap-linux.c index a220f050..b07f90b5 100644 --- a/pcap-linux.c +++ b/pcap-linux.c @@ -187,7 +187,7 @@ struct pcap_linux { char *device; /* device name */ int filter_in_userland; /* must filter in userland */ - int blocks_to_filter_in_userland; + u_int blocks_to_filter_in_userland; int must_do_on_close; /* stuff we must do when we close */ int timeout; /* timeout for buffering */ int cooked; /* using SOCK_DGRAM rather than SOCK_RAW */ @@ -3448,7 +3448,7 @@ pcap_setnonblock_linux(pcap_t *handle, int nonblock) * Get the status field of the ring buffer frame at a specified offset. */ static inline u_int -pcap_get_ring_frame_status(pcap_t *handle, int offset) +pcap_get_ring_frame_status(pcap_t *handle, u_int offset) { struct pcap_linux *handlep = handle->priv; union thdr h; @@ -4307,7 +4307,7 @@ pcap_read_linux_mmap_v2(pcap_t *handle, int max_packets, pcap_handler callback, * the one we've just processed. */ packet_mmap_release(h.h2); - if (handlep->blocks_to_filter_in_userland > 0) { + if (handlep->blocks_to_filter_in_userland != 0) { handlep->blocks_to_filter_in_userland--; if (handlep->blocks_to_filter_in_userland == 0) { /* @@ -4438,7 +4438,7 @@ again: * just processed. */ packet_mmap_v3_release(h.h3); - if (handlep->blocks_to_filter_in_userland > 0) { + if (handlep->blocks_to_filter_in_userland != 0) { handlep->blocks_to_filter_in_userland--; if (handlep->blocks_to_filter_in_userland == 0) { /* @@ -4480,7 +4480,7 @@ pcap_setfilter_linux(pcap_t *handle, struct bpf_program *filter) struct sock_fprog fcode; int can_filter_in_kernel; int err = 0; - int n, offset; + u_int n, offset; if (!handle) return -1; @@ -4663,11 +4663,13 @@ pcap_setfilter_linux(pcap_t *handle, struct bpf_program *filter) * walk the ring backward and count the free blocks. */ offset = handle->offset; - if (--offset < 0) - offset = handle->cc - 1; + if (offset == 0) + offset = handle->cc; + offset--; for (n=0; n < handle->cc; ++n) { - if (--offset < 0) - offset = handle->cc - 1; + if (offset == 0) + offset = handle->cc; + offset--; if (pcap_get_ring_frame_status(handle, offset) != TP_STATUS_KERNEL) break; } diff --git a/pcap-netfilter-linux.c b/pcap-netfilter-linux.c index 79ac2cbd..cd3ca5f8 100644 --- a/pcap-netfilter-linux.c +++ b/pcap-netfilter-linux.c @@ -86,7 +86,7 @@ netfilter_read_linux(pcap_t *handle, int max_packets, pcap_handler callback, u_c struct pcap_netfilter *handlep = handle->priv; register u_char *bp, *ep; int count = 0; - ssize_t len; + u_int cc; /* * Has "pcap_breakloop()" been called? @@ -100,8 +100,8 @@ netfilter_read_linux(pcap_t *handle, int max_packets, pcap_handler callback, u_c handle->break_loop = 0; return PCAP_ERROR_BREAK; } - len = handle->cc; - if (len == 0) { + cc = handle->cc; + if (cc == 0) { /* * The buffer is empty; refill it. * @@ -111,22 +111,31 @@ netfilter_read_linux(pcap_t *handle, int max_packets, pcap_handler callback, u_c * to set handle->break_loop (we ignore it on other * platforms as well). */ + ssize_t read_ret; + do { - len = recv(handle->fd, handle->buffer, handle->bufsize, 0); + read_ret = recv(handle->fd, handle->buffer, handle->bufsize, 0); if (handle->break_loop) { handle->break_loop = 0; return PCAP_ERROR_BREAK; } - if (len == -1 && errno == ENOBUFS) + if (read_ret == -1 && errno == ENOBUFS) handlep->packets_nobufs++; - } while ((len == -1) && (errno == EINTR || errno == ENOBUFS)); + } while ((read_ret == -1) && (errno == EINTR || errno == ENOBUFS)); - if (len < 0) { + if (read_ret < 0) { pcapint_fmt_errmsg_for_errno(handle->errbuf, PCAP_ERRBUF_SIZE, errno, "Can't receive packet"); return PCAP_ERROR; } + /* + * At this point, read_ret is guaranteed to be + * >= 0 and < p->bufsize; p->bufsize is a u_int, + * so its value is guaranteed to fit in cc, which + * is also a u_int. + */ + cc = (u_int)read_ret; bp = (unsigned char *)handle->buffer; } else bp = handle->bp; @@ -137,7 +146,7 @@ netfilter_read_linux(pcap_t *handle, int max_packets, pcap_handler callback, u_c * This assumes that a single buffer of message will have * <= INT_MAX packets, so the message count doesn't overflow. */ - ep = bp + len; + ep = bp + cc; while (bp < ep) { const struct nlmsghdr *nlh = (const struct nlmsghdr *) bp; uint32_t msg_len; @@ -154,7 +163,7 @@ netfilter_read_linux(pcap_t *handle, int max_packets, pcap_handler callback, u_c */ if (handle->break_loop) { handle->bp = bp; - handle->cc = (int)(ep - bp); + handle->cc = (u_int)(ep - bp); if (count == 0) { handle->break_loop = 0; return PCAP_ERROR_BREAK; @@ -180,8 +189,8 @@ netfilter_read_linux(pcap_t *handle, int max_packets, pcap_handler callback, u_c break; } - if (nlh->nlmsg_len < sizeof(struct nlmsghdr) || (u_int)len < nlh->nlmsg_len) { - snprintf(handle->errbuf, PCAP_ERRBUF_SIZE, "Message truncated: (got: %zd) (nlmsg_len: %u)", len, nlh->nlmsg_len); + if (nlh->nlmsg_len < sizeof(struct nlmsghdr) || cc < nlh->nlmsg_len) { + snprintf(handle->errbuf, PCAP_ERRBUF_SIZE, "Message truncated: (got: %u) (nlmsg_len: %u)", cc, nlh->nlmsg_len); return -1; } @@ -289,9 +298,7 @@ netfilter_read_linux(pcap_t *handle, int max_packets, pcap_handler callback, u_c bp += msg_len; if (count >= max_packets && !PACKET_COUNT_IS_UNLIMITED(max_packets)) { handle->bp = bp; - handle->cc = (int)(ep - bp); - if (handle->cc < 0) - handle->cc = 0; + handle->cc = (u_int)(ep - bp); return count; } } diff --git a/pcap-npf.c b/pcap-npf.c index 76346f57..b8916712 100644 --- a/pcap-npf.c +++ b/pcap-npf.c @@ -573,7 +573,7 @@ static int pcap_read_npf(pcap_t *p, int cnt, pcap_handler callback, u_char *user) { PACKET Packet; - int cc; + u_int cc; int n; register u_char *bp, *ep; u_char *datap; @@ -683,6 +683,7 @@ pcap_read_npf(pcap_t *p, int cnt, pcap_handler callback, u_char *user) ep = bp + cc; for (;;) { register u_int caplen, hdrlen; + size_t packet_bytes; /* * Has "pcap_breakloop()" been called? @@ -700,7 +701,7 @@ pcap_read_npf(pcap_t *p, int cnt, pcap_handler callback, u_char *user) return (PCAP_ERROR_BREAK); } else { p->bp = bp; - p->cc = (int) (ep - bp); + p->cc = (u_int) (ep - bp); return (n); } } @@ -711,6 +712,21 @@ pcap_read_npf(pcap_t *p, int cnt, pcap_handler callback, u_char *user) hdrlen = bhp->bh_hdrlen; datap = bp + hdrlen; + /* + * Compute the number of bytes for this packet in + * the buffer. + * + * That's the sum of the header length and the packet + * data length plus, if this is not the last packet, + * the padding required to align the next packet on + * the appropriate boundary. + * + * That means that it should be the minimum of the + * number of bytes left in the buffer and the + * rounded-up sum of the header and packet data lengths. + */ + packet_bytes = min((u_int)(ep - bp), Packet_WORDALIGN(caplen + hdrlen)); + /* * Short-circuit evaluation: if using BPF filter * in kernel, no need to do it now - we already know @@ -731,7 +747,7 @@ pcap_read_npf(pcap_t *p, int cnt, pcap_handler callback, u_char *user) /* Discard all packets that are not '1 out of N' */ if (pw->samp_npkt != 0) { - bp += Packet_WORDALIGN(caplen + hdrlen); + bp += packet_bytes; continue; } break; @@ -746,7 +762,7 @@ pcap_read_npf(pcap_t *p, int cnt, pcap_handler callback, u_char *user) */ if (pkt_header->ts.tv_sec < pw->samp_time.tv_sec || (pkt_header->ts.tv_sec == pw->samp_time.tv_sec && pkt_header->ts.tv_usec < pw->samp_time.tv_usec)) { - bp += Packet_WORDALIGN(caplen + hdrlen); + bp += packet_bytes; continue; } @@ -768,17 +784,17 @@ pcap_read_npf(pcap_t *p, int cnt, pcap_handler callback, u_char *user) * XXX A bpf_hdr matches a pcap_pkthdr. */ (*callback)(user, (struct pcap_pkthdr*)bp, datap); - bp += Packet_WORDALIGN(caplen + hdrlen); + bp += packet_bytes; if (++n >= cnt && !PACKET_COUNT_IS_UNLIMITED(cnt)) { p->bp = bp; - p->cc = (int) (ep - bp); + p->cc = (u_int) (ep - bp); return (n); } } else { /* * Skip this packet. */ - bp += Packet_WORDALIGN(caplen + hdrlen); + bp += packet_bytes; } } #undef bhp diff --git a/pcap-rpcap.c b/pcap-rpcap.c index d3c0638f..bdf077a6 100644 --- a/pcap-rpcap.c +++ b/pcap-rpcap.c @@ -3658,7 +3658,7 @@ static int rpcap_discard(PCAP_SOCKET sock, SSL *ssl, uint32_t len, char *errbuf) static int rpcap_read_packet_msg(struct pcap_rpcap const *rp, pcap_t *p, size_t size) { u_char *bp; - int cc; + u_int cc; int bytes_read; bp = p->bp; @@ -3668,7 +3668,7 @@ static int rpcap_read_packet_msg(struct pcap_rpcap const *rp, pcap_t *p, size_t * Loop until we have the amount of data requested or we get * an error or interrupt. */ - while ((size_t)cc < size) + while (cc < size) { /* * We haven't read all of the packet header yet.