Bug 278034 - tcpdump's ip6_print can read beyond buffer end
Summary: tcpdump's ip6_print can read beyond buffer end
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Some People
Assignee: Kristof Provost
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-03-29 18:54 UTC by Robert Morris
Modified: 2024-05-26 21:01 UTC (History)
2 users (show)

See Also:
linimon: mfc-stable14?
linimon: mfc-stable13?


Attachments
a packet trace file that causes tcpdump to read beyond the end of a buffer in pfsync_print() (376 bytes, application/vnd.tcpdump.pcap)
2024-03-29 18:54 UTC, Robert Morris
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Robert Morris 2024-03-29 18:54:36 UTC
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)
Comment 1 Gleb Smirnoff freebsd_committer freebsd_triage 2024-03-29 19:23:26 UTC
pfsync printing in  tcpdump is not present upstream.  It is our local 
modification.  Better to assign to pf team.
Comment 2 Kristof Provost freebsd_committer freebsd_triage 2024-04-01 07:50:38 UTC
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?
Comment 3 Kristof Provost freebsd_committer freebsd_triage 2024-04-01 08:45:03 UTC
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.
Comment 4 commit-hook freebsd_committer freebsd_triage 2024-04-04 08:08:17 UTC
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(-)
Comment 5 Mark Linimon freebsd_committer freebsd_triage 2024-04-05 09:47:23 UTC
Triage: assign to committer.
Comment 6 commit-hook freebsd_committer freebsd_triage 2024-04-18 13:37:12 UTC
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(-)
Comment 7 commit-hook freebsd_committer freebsd_triage 2024-04-18 13:38:15 UTC
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(-)