]> The Tcpdump Group git mirrors - libpcap/commitdiff
Make sure no read routine process more than INT_MAX packets.
authorGuy Harris <[email protected]>
Wed, 26 Jan 2022 23:00:26 +0000 (15:00 -0800)
committerGuy Harris <[email protected]>
Wed, 26 Jan 2022 23:00:26 +0000 (15:00 -0800)
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.

16 files changed:
dlpisubs.c
pcap-airpcap.c
pcap-bpf.c
pcap-dag.c
pcap-dos.c
pcap-dpdk.c
pcap-linux.c
pcap-netfilter-linux.c
pcap-nit.c
pcap-npf.c
pcap-pf.c
pcap-rdmasniff.c
pcap-rpcap.c
pcap-snf.c
pcap-snit.c
savefile.c

index 2ef09315bdf855e95ba53d172c16c1ad5cb05d5f..6815b0ec2cba3cc95f75170006e61f7effbf99bb 100644 (file)
@@ -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;
 
index d986ad9c39bcb37d273871c17b2527dfa34519a2..510e4c4e6a27c8be6e37cabd3b24e0c1e5355575 100644 (file)
@@ -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;
index 836e7d00c51569c61486ef094eb2e03c732f4ca8..c9967ee0851285ba3aac3465cb4ecd37fc9e02ac 100644 (file)
@@ -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)
index b207dd84cebab024db0a0d1c8403c391b228ad38..b3949d53e0c44b6aaeac096859cc6192be747720 100644 (file)
@@ -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;
index cdd60ea459e1a8b65773ad83086103f43a5d3672..e2b3fb7467225fe66e78e8b2fcbebb4d9cce9dee 100644 (file)
@@ -12,6 +12,7 @@
 #include <signal.h>
 #include <float.h>
 #include <fcntl.h>
+#include <limits.h> /* for INT_MAX */
 #include <io.h>
 
 #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);
index 765abe59ba6568932eda3a0031e6126a634f145f..94c330927f71d1bb9095803a17ad86ad34e1239b 100644 (file)
@@ -85,6 +85,7 @@ env DPDK_CFG="--log-level=debug -l0 -dlibrte_pmd_e1000.so -dlibrte_pmd_ixgbe.so
 #include <stdlib.h>
 #include <string.h>
 #include <unistd.h>
+#include <limits.h> /* for INT_MAX */
 #include <time.h>
 
 #include <sys/time.h>
@@ -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;
index e931f84f6a47b38ba30afb741145d14265470b33..bd8f2b2456fe06f0e29970e9303bbcc6aa0818f8 100644 (file)
@@ -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;
index d9550b02d399792b514eef51b3dbcdefa03da16d..33204a54e045bed86b69928f586cfb9c1ca66b10 100644 (file)
@@ -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;
index cfd95191d772b0c3df559a445e474cffd75bc38c..6f4f8dd87d0a8195b65475d14f561f4712f584ac 100644 (file)
@@ -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;
index 1117e02c48b4e9b1010e2425a601521a9907f790..0099a68fc1c45bf8c2109858a929f3a08f8cd8a6 100644 (file)
@@ -36,6 +36,7 @@
 #endif
 
 #include <errno.h>
+#include <limits.h> /* for INT_MAX */
 #define PCAP_DONT_INCLUDE_PCAP_BPF_H
 #include <Packet32.h>
 #include <pcap-int.h>
@@ -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
         */
index 4563a225ddc2500015d17715b0653c73398f2684..bd27933eff69e312717d908b9202e1ed7fdb2c20 100644 (file)
--- 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;
index 224821de4abdc8c36770cec4272690713465869c..0b871cb992c1cd60d69409399a5001b94d9a37c2 100644 (file)
@@ -38,6 +38,7 @@
 #include <infiniband/verbs.h>
 #include <stdlib.h>
 #include <string.h>
+#include <limits.h> /* for INT_MAX */
 #include <sys/time.h>
 
 #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;
index f5c126dbc15ce0ca51fd05b55d60c43ce22943ab..66c43559669b43953b4d12e5c8517364fe70ea2d 100644 (file)
@@ -42,6 +42,7 @@
 #include <stdlib.h>            /* for malloc(), free(), ... */
 #include <stdarg.h>            /* for functions with variable number of arguments */
 #include <errno.h>             /* for the errno variable */
+#include <limits.h>            /* 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))
        {
                /*
index a9162eba50105ffe865a2ee4116ae6ec38b0e2ee..b885e02657c1e9c1d8481788167a25e6e9527864 100644 (file)
@@ -9,6 +9,7 @@
 #include <stdlib.h>
 #include <string.h>
 #include <errno.h>
+#include <limits.h> /* for INT_MAX */
 
 #ifndef _WIN32
 #include <netinet/in.h>
@@ -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?
                 */
index ca6222cf01666aac7d185a281ff6dac6eaf146e0..ff0cb59a43dc2fca7b7beec7b9ca31977150d349 100644 (file)
@@ -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;
index acd915cf73057912fe7a6dffc541fb6e94648529..2b42b9b4558208dfb5d5cb62f9ca0b679c0783f2 100644 (file)
@@ -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;
                }
        }