Created attachment 249560 [details] a packet trace file that causes tcpdump to read beyond the end of a buffer in pfsync_print() tcpdump's ip6_print() is passed the real length of the packet buffer in the length argument. It pulls len from the packet header: payload_len = GET_BE_U_2(ip6->ip6_plen); if (payload_len != 0) { len = payload_len + sizeof(struct ip6_hdr); if (length < len) ND_PRINT("truncated-ip6 - %u bytes missing!", len - length); If the header's claimed length is greater than the buffer size, tcpdump prints a warning but then continues. Later len (rather than length) is passed to ip_demux_print(): ip_demux_print(ndo, cp, len, 6, fragmented, GET_U_1(ip6->ip6_hlim), nh, bp); and is used by some of the functions it calls as the buffer length. For example, pfsync_print() uses this len as the limit for how far it looks into the buffer: while (plen > 0) { if (len < sizeof(*subh)) break; ...; len -= sizeof(*subh); Since this len was pulled from the packet, a broken packet can cause a read overrun. I've attached a demo packet that causes pfsync_print() to read past the end of the buffer. You may need an address sanitizer or valgrind to see the problem. # uname -a FreeBSD stock14 15.0-CURRENT FreeBSD 15.0-CURRENT #20 main-n268970-619e6f1f9288: Sat Mar 23 16:25:40 AST 2024 root@stock14:/usr/obj/usr/src/amd64.amd64/sys/GENERIC amd64 # tcpdump --version tcpdump version 4.99.4 libpcap version 1.10.4 OpenSSL 3.0.13 30 Jan 2024 # valgrind tcpdump -v -v -n -r - -K < tcpdump43a.dat ... ==9292== Invalid read of size 2 ==9292== at 0x22C90E: pfsync_print (src/contrib/tcpdump/print-pfsync.c:168) ==9292== by 0x1C40EE: ip6_print (src/contrib/tcpdump/print-ip6.c:487) ==9292== by 0x1B56BD: ethertype_print (src/contrib/tcpdump/print-ether.c:628) ==9292== by 0x1B5121: ether_common_print (src/contrib/tcpdump/print-ether.c:391) ==9292== by 0x1B5213: ether_print (src/contrib/tcpdump/print-ether.c:448) ==9292== by 0x1B5213: ether_if_print (src/contrib/tcpdump/print-ether.c:464) ==9292== by 0x18C30E: pretty_print_packet (src/contrib/tcpdump/print.c:417) ==9292== by 0x225D00: print_packet (src/contrib/tcpdump/tcpdump.c:3139) ==9292== by 0x48ACC9D: pcap_offline_read (in /lib/libpcap.so.8) ==9292== by 0x48AB248: pcap_loop (in /lib/libpcap.so.8) ==9292== by 0x2240AC: main (src/contrib/tcpdump/tcpdump.c:2581) ==9292== Address 0x5a3ce90 is 0 bytes after a block of size 1,024 alloc'd ==9292== at 0x484CDB4: malloc (vg_replace_malloc.c:446) ==9292== by 0x48AF550: pcap_check_header (in /lib/libpcap.so.8) ==9292== by 0x48AC9E2: pcap_fopen_offline_with_tstamp_precision (in /lib/libpcap.so.8) ==9292== by 0x48AC8DD: pcap_open_offline_with_tstamp_precision (in /lib/libpcap.so.8) ==9292== by 0x2235EF: main (src/contrib/tcpdump/tcpdump.c:2079)
pfsync printing in tcpdump is not present upstream. It is our local modification. Better to assign to pf team.
I'm not terribly familiar with the way tcpdump handles packet parsing. Is the issue here that ip6_print() passes the wrong length value, or that pfsync_print() misinterprets the passed length, and ought to be using accessor functions like GET_BE_U_2() and friends which do check the actual data length?
To answer my own question, I think this may suffice: diff --git a/contrib/tcpdump/print-pfsync.c b/contrib/tcpdump/print-pfsync.c index 5710e36ded6c..e22c11a2df2d 100644 --- a/contrib/tcpdump/print-pfsync.c +++ b/contrib/tcpdump/print-pfsync.c @@ -86,7 +86,7 @@ pfsync_ip_print(netdissect_options *ndo , const u_char *bp, u_int len) { struct pfsync_header *hdr = (struct pfsync_header *)bp; - if (len < PFSYNC_HDRLEN) + if (len < PFSYNC_HDRLEN || ! ND_TTEST_LEN(bp, len)) ND_PRINT("[|pfsync]"); else pfsync_print(ndo, hdr, bp + sizeof(struct pfsync_header), At the very least it makes valgrind happy without actually appearing to break tcpdump's support for pfsync.
A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=4848eb3af2a91b133c4b70cb9b71dd92ffec7f46 commit 4848eb3af2a91b133c4b70cb9b71dd92ffec7f46 Author: Kristof Provost <kp@FreeBSD.org> AuthorDate: 2024-04-01 09:42:14 +0000 Commit: Kristof Provost <kp@FreeBSD.org> CommitDate: 2024-04-04 08:07:05 +0000 tcpdump: cope with incorrect packet lengths It's possible for the capture buffer to be smaller than indicated by the header length. However, pfsync_print() only took the header length into account. As a result we could read outside of the buffer. Check that we have at least the expected amount of data before we start parsing. PR: 278034 MFC after: 2 weeks Sponsored by: Rubicon Communications, LLC ("Netgate") Differential Revision: https://reviews.freebsd.org/D44580 contrib/tcpdump/print-pfsync.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Triage: assign to committer.
A commit in branch stable/14 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=dc16f5fe14226da84ea4e77b04d31efa5c5f6853 commit dc16f5fe14226da84ea4e77b04d31efa5c5f6853 Author: Kristof Provost <kp@FreeBSD.org> AuthorDate: 2024-04-01 09:42:14 +0000 Commit: Kristof Provost <kp@FreeBSD.org> CommitDate: 2024-04-18 13:35:52 +0000 tcpdump: cope with incorrect packet lengths It's possible for the capture buffer to be smaller than indicated by the header length. However, pfsync_print() only took the header length into account. As a result we could read outside of the buffer. Check that we have at least the expected amount of data before we start parsing. PR: 278034 MFC after: 2 weeks Sponsored by: Rubicon Communications, LLC ("Netgate") Differential Revision: https://reviews.freebsd.org/D44580 (cherry picked from commit 4848eb3af2a91b133c4b70cb9b71dd92ffec7f46) contrib/tcpdump/print-pfsync.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
A commit in branch stable/13 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=bf0700716a2e04464311e0b585b947d7d3e825b9 commit bf0700716a2e04464311e0b585b947d7d3e825b9 Author: Kristof Provost <kp@FreeBSD.org> AuthorDate: 2024-04-01 09:42:14 +0000 Commit: Kristof Provost <kp@FreeBSD.org> CommitDate: 2024-04-18 07:41:28 +0000 tcpdump: cope with incorrect packet lengths It's possible for the capture buffer to be smaller than indicated by the header length. However, pfsync_print() only took the header length into account. As a result we could read outside of the buffer. Check that we have at least the expected amount of data before we start parsing. PR: 278034 MFC after: 2 weeks Sponsored by: Rubicon Communications, LLC ("Netgate") Differential Revision: https://reviews.freebsd.org/D44580 (cherry picked from commit 4848eb3af2a91b133c4b70cb9b71dd92ffec7f46) contrib/tcpdump/print-pfsync.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)