From: Guy Harris Date: Wed, 26 Jan 2022 23:00:26 +0000 (-0800) Subject: Make sure no read routine process more than INT_MAX packets. X-Git-Url: https://git.tcpdump.org/libpcap/commitdiff_plain/c60ebf10efd105d149f7c2d3eb15dec38af45001 Make sure no read routine process more than INT_MAX packets. Some read routines don't read a single bufferful of packets and process just those packets; if packets continue to be made available, they could conceivably process an arbitrary number of packets. That would mean that the packet count overflows; either that makes it look like a negative number, making it look as if an error occurred, or makes it look like a too-small positive number. This can't be fixed by making the count 64-bit, as it ultimately gets returned by pcap_dispatch(), which is defined to return an int. Instead, if the maximum packet count argument to those routines is a value that means "no maximum", we set the maximum to INT_MAX. Those routines are *not* defined to loop forever, so this isn't an issue. This should fix issue #1087. --- diff --git a/dlpisubs.c b/dlpisubs.c index 2ef09315..6815b0ec 100644 --- a/dlpisubs.c +++ b/dlpisubs.c @@ -146,7 +146,12 @@ pcap_process_pkts(pcap_t *p, pcap_handler callback, u_char *user, #endif #endif - /* Loop through packets */ + /* + * Loop through packets. + * + * This assumes that a single buffer of packets will have + * <= INT_MAX packets, so the packet count doesn't overflow. + */ ep = bufp + len; n = 0; diff --git a/pcap-airpcap.c b/pcap-airpcap.c index d986ad9c..510e4c4e 100644 --- a/pcap-airpcap.c +++ b/pcap-airpcap.c @@ -627,6 +627,9 @@ airpcap_read(pcap_t *p, int cnt, pcap_handler callback, u_char *user) /* * Loop through each packet. + * + * This assumes that a single buffer of packets will have + * <= INT_MAX packets, so the packet count doesn't overflow. */ #define bhp ((AirpcapBpfHeader *)bp) n = 0; diff --git a/pcap-bpf.c b/pcap-bpf.c index 836e7d00..c9967ee0 100644 --- a/pcap-bpf.c +++ b/pcap-bpf.c @@ -1177,6 +1177,9 @@ pcap_read_bpf(pcap_t *p, int cnt, pcap_handler callback, u_char *user) /* * Loop through each packet. + * + * This assumes that a single buffer of packets will have + * <= INT_MAX packets, so the packet count doesn't overflow. */ #ifdef BIOCSTSTAMP #define bhp ((struct bpf_xhdr *)bp) diff --git a/pcap-dag.c b/pcap-dag.c index b207dd84..b3949d53 100644 --- a/pcap-dag.c +++ b/pcap-dag.c @@ -391,7 +391,12 @@ dag_read(pcap_t *p, int cnt, pcap_handler callback, u_char *user) } - /* Process the packets. */ + /* + * Process the packets. + * + * This assumes that a single buffer of packets will have + * <= INT_MAX packets, so the packet count doesn't overflow. + */ while (pd->dag_mem_top - pd->dag_mem_bottom >= dag_record_size) { unsigned short packet_len = 0; diff --git a/pcap-dos.c b/pcap-dos.c index cdd60ea4..e2b3fb74 100644 --- a/pcap-dos.c +++ b/pcap-dos.c @@ -12,6 +12,7 @@ #include #include #include +#include /* for INT_MAX */ #include #if defined(USE_32BIT_DRIVERS) @@ -355,7 +356,22 @@ pcap_read_dos (pcap_t *p, int cnt, pcap_handler callback, u_char *data) { int rc, num = 0; - while (num <= cnt || PACKET_COUNT_IS_UNLIMITED(cnt)) + /* + * This can conceivably process more than INT_MAX packets, + * which would overflow the packet count, causing it either + * to look like a negative number, and thus cause us to + * return a value that looks like an error, or overflow + * back into positive territory, and thus cause us to + * return a too-low count. + * + * Therefore, if the packet count is unlimited, we clip + * it at INT_MAX; this routine is not expected to + * process packets indefinitely, so that's not an issue. + */ + if (PACKET_COUNT_IS_UNLIMITED(cnt)) + cnt = INT_MAX; + + while (num <= cnt) { if (p->fd <= 0) return (-1); diff --git a/pcap-dpdk.c b/pcap-dpdk.c index 765abe59..94c33092 100644 --- a/pcap-dpdk.c +++ b/pcap-dpdk.c @@ -85,6 +85,7 @@ env DPDK_CFG="--log-level=debug -l0 -dlibrte_pmd_e1000.so -dlibrte_pmd_ixgbe.so #include #include #include +#include /* for INT_MAX */ #include #include @@ -331,13 +332,28 @@ static int pcap_dpdk_dispatch(pcap_t *p, int max_cnt, pcap_handler cb, u_char *c u_char *large_buffer=NULL; int timeout_ms = p->opt.timeout; - if ( !PACKET_COUNT_IS_UNLIMITED(max_cnt) && max_cnt < MAX_PKT_BURST){ + /* + * This can conceivably process more than INT_MAX packets, + * which would overflow the packet count, causing it either + * to look like a negative number, and thus cause us to + * return a value that looks like an error, or overflow + * back into positive territory, and thus cause us to + * return a too-low count. + * + * Therefore, if the packet count is unlimited, we clip + * it at INT_MAX; this routine is not expected to + * process packets indefinitely, so that's not an issue. + */ + if (PACKET_COUNT_IS_UNLIMITED(max_cnt)) + max_cnt = INT_MAX; + + if (max_cnt < MAX_PKT_BURST){ burst_cnt = max_cnt; }else{ burst_cnt = MAX_PKT_BURST; } - while( PACKET_COUNT_IS_UNLIMITED(max_cnt) || pkt_cnt < max_cnt){ + while( pkt_cnt < max_cnt){ if (p->break_loop){ p->break_loop = 0; return PCAP_ERROR_BREAK; diff --git a/pcap-linux.c b/pcap-linux.c index e931f84f..bd8f2b24 100644 --- a/pcap-linux.c +++ b/pcap-linux.c @@ -4064,9 +4064,22 @@ pcap_read_linux_mmap_v2(pcap_t *handle, int max_packets, pcap_handler callback, } } - /* non-positive values of max_packets are used to require all - * packets currently available in the ring */ - while ((pkts < max_packets) || PACKET_COUNT_IS_UNLIMITED(max_packets)) { + /* + * This can conceivably process more than INT_MAX packets, + * which would overflow the packet count, causing it either + * to look like a negative number, and thus cause us to + * return a value that looks like an error, or overflow + * back into positive territory, and thus cause us to + * return a too-low count. + * + * Therefore, if the packet count is unlimited, we clip + * it at INT_MAX; this routine is not expected to + * process packets indefinitely, so that's not an issue. + */ + if (PACKET_COUNT_IS_UNLIMITED(max_packets)) + max_packets = INT_MAX; + + while (pkts < max_packets) { /* * Get the current ring buffer frame, and break if * it's still owned by the kernel. @@ -4159,9 +4172,22 @@ again: return pkts; } - /* non-positive values of max_packets are used to require all - * packets currently available in the ring */ - while ((pkts < max_packets) || PACKET_COUNT_IS_UNLIMITED(max_packets)) { + /* + * This can conceivably process more than INT_MAX packets, + * which would overflow the packet count, causing it either + * to look like a negative number, and thus cause us to + * return a value that looks like an error, or overflow + * back into positive territory, and thus cause us to + * return a too-low count. + * + * Therefore, if the packet count is unlimited, we clip + * it at INT_MAX; this routine is not expected to + * process packets indefinitely, so that's not an issue. + */ + if (PACKET_COUNT_IS_UNLIMITED(max_packets)) + max_packets = INT_MAX; + + while (pkts < max_packets) { int packets_to_read; if (handlep->current_packet == NULL) { @@ -4174,12 +4200,12 @@ again: } packets_to_read = handlep->packets_left; - if (!PACKET_COUNT_IS_UNLIMITED(max_packets) && - packets_to_read > (max_packets - pkts)) { + if (packets_to_read > (max_packets - pkts)) { /* - * We've been given a maximum number of packets - * to process, and there are more packets in - * this buffer than that. Only process enough + * There are more packets in the buffer than + * the number of packets we have left to + * process to get up to the maximum number + * of packets to process. Only process enough * of them to get us up to that maximum. */ packets_to_read = max_packets - pkts; diff --git a/pcap-netfilter-linux.c b/pcap-netfilter-linux.c index d9550b02..33204a54 100644 --- a/pcap-netfilter-linux.c +++ b/pcap-netfilter-linux.c @@ -136,6 +136,13 @@ netfilter_read_linux(pcap_t *handle, int max_packets, pcap_handler callback, u_c bp = (unsigned char *)handle->buffer; } else bp = handle->bp; + + /* + * Loop through each message. + * + * This assumes that a single buffer of message will have + * <= INT_MAX packets, so the message count doesn't overflow. + */ ep = bp + len; while (bp < ep) { const struct nlmsghdr *nlh = (const struct nlmsghdr *) bp; diff --git a/pcap-nit.c b/pcap-nit.c index cfd95191..6f4f8dd8 100644 --- a/pcap-nit.c +++ b/pcap-nit.c @@ -125,6 +125,9 @@ pcap_read_nit(pcap_t *p, int cnt, pcap_handler callback, u_char *user) * Loop through each packet. The increment expression * rounds up to the next int boundary past the end of * the previous packet. + * + * This assumes that a single buffer of packets will have + * <= INT_MAX packets, so the packet count doesn't overflow. */ n = 0; ep = bp + cc; diff --git a/pcap-npf.c b/pcap-npf.c index 1117e02c..0099a68f 100644 --- a/pcap-npf.c +++ b/pcap-npf.c @@ -36,6 +36,7 @@ #endif #include +#include /* for INT_MAX */ #define PCAP_DONT_INCLUDE_PCAP_BPF_H #include #include @@ -633,6 +634,9 @@ pcap_read_npf(pcap_t *p, int cnt, pcap_handler callback, u_char *user) /* * Loop through each packet. + * + * This assumes that a single buffer of packets will have + * <= INT_MAX packets, so the packet count doesn't overflow. */ #define bhp ((struct bpf_hdr *)bp) n = 0; @@ -792,6 +796,21 @@ pcap_read_win32_dag(pcap_t *p, int cnt, pcap_handler callback, u_char *user) endofbuf = (char*)header + cc; + /* + * This can conceivably process more than INT_MAX packets, + * which would overflow the packet count, causing it either + * to look like a negative number, and thus cause us to + * return a value that looks like an error, or overflow + * back into positive territory, and thus cause us to + * return a too-low count. + * + * Therefore, if the packet count is unlimited, we clip + * it at INT_MAX; this routine is not expected to + * process packets indefinitely, so that's not an issue. + */ + if (PACKET_COUNT_IS_UNLIMITED(cnt)) + cnt = INT_MAX; + /* * Cycle through the packets */ diff --git a/pcap-pf.c b/pcap-pf.c index 4563a225..bd27933e 100644 --- a/pcap-pf.c +++ b/pcap-pf.c @@ -133,6 +133,9 @@ pcap_read_pf(pcap_t *pc, int cnt, pcap_handler callback, u_char *user) bp = pc->bp; /* * Loop through each packet. + * + * This assumes that a single buffer of packets will have + * <= INT_MAX packets, so the packet count doesn't overflow. */ n = 0; pad = pc->fddipad; diff --git a/pcap-rdmasniff.c b/pcap-rdmasniff.c index 224821de..0b871cb9 100644 --- a/pcap-rdmasniff.c +++ b/pcap-rdmasniff.c @@ -38,6 +38,7 @@ #include #include #include +#include /* for INT_MAX */ #include #if !defined(IBV_FLOW_ATTR_SNIFFER) @@ -136,7 +137,22 @@ rdmasniff_read(pcap_t *handle, int max_packets, pcap_handler callback, u_char *u priv->cq_event = 1; } - while (count < max_packets || PACKET_COUNT_IS_UNLIMITED(max_packets)) { + /* + * This can conceivably process more than INT_MAX packets, + * which would overflow the packet count, causing it either + * to look like a negative number, and thus cause us to + * return a value that looks like an error, or overflow + * back into positive territory, and thus cause us to + * return a too-low count. + * + * Therefore, if the packet count is unlimited, we clip + * it at INT_MAX; this routine is not expected to + * process packets indefinitely, so that's not an issue. + */ + if (PACKET_COUNT_IS_UNLIMITED(max_packets)) + max_packets = INT_MAX; + + while (count < max_packets) { if (ibv_poll_cq(priv->cq, 1, &wc) != 1) { priv->cq_event = 0; break; diff --git a/pcap-rpcap.c b/pcap-rpcap.c index f5c126db..66c43559 100644 --- a/pcap-rpcap.c +++ b/pcap-rpcap.c @@ -42,6 +42,7 @@ #include /* for malloc(), free(), ... */ #include /* for functions with variable number of arguments */ #include /* for the errno variable */ +#include /* for INT_MAX */ #include "sockutils.h" #include "pcap-int.h" #include "rpcap-protocol.h" @@ -645,6 +646,21 @@ static int pcap_read_rpcap(pcap_t *p, int cnt, pcap_handler callback, u_char *us } } + /* + * This can conceivably process more than INT_MAX packets, + * which would overflow the packet count, causing it either + * to look like a negative number, and thus cause us to + * return a value that looks like an error, or overflow + * back into positive territory, and thus cause us to + * return a too-low count. + * + * Therefore, if the packet count is unlimited, we clip + * it at INT_MAX; this routine is not expected to + * process packets indefinitely, so that's not an issue. + */ + if (PACKET_COUNT_IS_UNLIMITED(cnt)) + cnt = INT_MAX; + while (n < cnt || PACKET_COUNT_IS_UNLIMITED(cnt)) { /* diff --git a/pcap-snf.c b/pcap-snf.c index a9162eba..b885e026 100644 --- a/pcap-snf.c +++ b/pcap-snf.c @@ -9,6 +9,7 @@ #include #include #include +#include /* for INT_MAX */ #ifndef _WIN32 #include @@ -139,9 +140,24 @@ snf_read(pcap_t *p, int cnt, pcap_handler callback, u_char *user) if (!p) return -1; + /* + * This can conceivably process more than INT_MAX packets, + * which would overflow the packet count, causing it either + * to look like a negative number, and thus cause us to + * return a value that looks like an error, or overflow + * back into positive territory, and thus cause us to + * return a too-low count. + * + * Therefore, if the packet count is unlimited, we clip + * it at INT_MAX; this routine is not expected to + * process packets indefinitely, so that's not an issue. + */ + if (PACKET_COUNT_IS_UNLIMITED(cnt)) + cnt = INT_MAX; + n = 0; timeout = ps->snf_timeout; - while (n < cnt || PACKET_COUNT_IS_UNLIMITED(cnt)) { + while (n < cnt) { /* * Has "pcap_breakloop()" been called? */ diff --git a/pcap-snit.c b/pcap-snit.c index ca6222cf..ff0cb59a 100644 --- a/pcap-snit.c +++ b/pcap-snit.c @@ -139,6 +139,9 @@ pcap_read_snit(pcap_t *p, int cnt, pcap_handler callback, u_char *user) /* * loop through each snapshot in the chunk + * + * This assumes that a single buffer of packets will have + * <= INT_MAX packets, so the packet count doesn't overflow. */ n = 0; ep = bp + cc; diff --git a/savefile.c b/savefile.c index acd915cf..2b42b9b4 100644 --- a/savefile.c +++ b/savefile.c @@ -621,6 +621,21 @@ pcap_offline_read(pcap_t *p, int cnt, pcap_handler callback, u_char *user) int n = 0; u_char *data; + /* + * This can conceivably process more than INT_MAX packets, + * which would overflow the packet count, causing it either + * to look like a negative number, and thus cause us to + * return a value that looks like an error, or overflow + * back into positive territory, and thus cause us to + * return a too-low count. + * + * Therefore, if the packet count is unlimited, we clip + * it at INT_MAX; this routine is not expected to + * process packets indefinitely, so that's not an issue. + */ + if (PACKET_COUNT_IS_UNLIMITED(cnt)) + cnt = INT_MAX; + for (;;) { struct pcap_pkthdr h; int status; @@ -664,7 +679,7 @@ pcap_offline_read(pcap_t *p, int cnt, pcap_handler callback, u_char *user) pcap_filter(fcode, data, h.len, h.caplen)) { (*callback)(user, &h, data); n++; /* count the packet */ - if (!PACKET_COUNT_IS_UNLIMITED(cnt) && n >= cnt) + if (n >= cnt) break; } }