Bug 283799 - tcpdump (14.2+) endian-swaps uid when parsing pflog data
Summary: tcpdump (14.2+) endian-swaps uid when parsing pflog data
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: 14.2-RELEASE
Hardware: amd64 Any
: --- Affects Only Me
Assignee: freebsd-bugs (Nobody)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2025-01-02 17:13 UTC by eborisch+FreeBSD
Modified: 2025-01-14 02:55 UTC (History)
3 users (show)

See Also:


Attachments
Proposed patch (604 bytes, patch)
2025-01-02 23:29 UTC, eborisch+FreeBSD
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description eborisch+FreeBSD 2025-01-02 17:13:10 UTC
In this change:

https://cgit.freebsd.org/src/commit/contrib/tcpdump/print-pflog.c?h=releng/14.2&id=ec3da16d8bc19ad90f04cc227fc8f409813c44f4

Endian macros were added (apparently in error) here:

contrib/tcpdump/print-pflog.c line 122:
	if (GET_BE_U_4(hdr->uid) != UID_MAX)
		ND_PRINT(" [uid %u]", (unsigned)GET_BE_U_4(hdr->uid));

which causes the output uids to be treated to an incorrect byte-swap.
Comment 1 eborisch+FreeBSD 2025-01-02 17:14:50 UTC
Just to explicitly show before/after here:

-	if (hdr->uid != UID_MAX)
-		ND_PRINT(" [uid %u]", (unsigned)hdr->uid);
+	if (GET_BE_U_4(hdr->uid) != UID_MAX)
+		ND_PRINT(" [uid %u]", (unsigned)GET_BE_U_4(hdr->uid));
Comment 2 Joseph Mingrone freebsd_committer freebsd_triage 2025-01-02 22:10:29 UTC
[CC: kp@FreeSBD.org who has a better understanding of pf.]

Hello, and thank you for reporting.

hdr->uid is now defined as nd_uint32_t in contrib/tcpdump/pflog.h [0], so I /thought/ we needed GET_BE_U_4.  The equivalent upstream code (which has unfortunately diverged from ours) also uses GET_BE_U_4 when printing the uid [1].

I'm not a pf user, so I'm not clear on the expected output before and after this change.  This is what I see now:

% id -u
1001

% sudo service pf start
Enabling pf.

% sudo service pflog start
Starting pflog.

% cat /etc/pf.conf
if="em0"
block out log on $if inet proto icmp all

% ping google.com # (in another vt)

% sudo tcpdump -n -e -ttt -i pflog0
tcpdump: verbose output suppressed, use -v[v]... for full protocol decode
listening on pflog0, link-type PFLOG (OpenBSD pflog file), snapshot length 262144 bytes
 00:00:00.000000 rule 0/0(match) [uid 0]: block out on em0: 192.168.2.2 > 142.250.80.46: ICMP echo request, id 40305, seq 0, length 64

Could you share a recipe to demonstrate the problem?

--

[0] https://github.com/freebsd/freebsd-src/blob/main/contrib/tcpdump/pflog.h#L118-L151
[1] https://github.com/the-tcpdump-group/tcpdump/blob/master/print-pflog.c#L424-L432
Comment 3 Kristof Provost freebsd_committer freebsd_triage 2025-01-02 22:17:26 UTC
Given this https://cgit.freebsd.org/src/tree/sys/netpfil/pf/if_pflog.c#n256 I'd expect no endianness conversion to be required for uid (and rule_uid, rule_pid).

Arguably that's wrong, as we do use big endian for a lot of other fields, but it is what it is now and has been since 2007.
Comment 4 eborisch+FreeBSD 2025-01-02 23:28:49 UTC
Something like:

pass out log (user) on eth0

in pf.conf should generate cases. Traffic will need to be from someone other than root to be interesting.

I see things like:

# tcpdump -er /var/log/pflog

[...]
16:48:29.398601 rule 5/0(match) [uid 436469760]: pass out on vtnet0: machine.name.one.23101 > machine.name.two.ssh: Flags [S], seq 3725723442, win 65535, options [mss 1460, [|tcp]

where that uid should be 1050, and not 436469760.

>>> import socket
>>> socket.ntohl(436469760)
1050

Most of the other fields are from network code, which frequently lives in the bigendian world. UID and PID are local parameters, so the endian mis-match isn't that surprising, if not explicitly documented.
Comment 5 eborisch+FreeBSD 2025-01-02 23:29:56 UTC
Created attachment 256362 [details]
Proposed patch
Comment 6 Joseph Mingrone freebsd_committer freebsd_triage 2025-01-03 00:52:05 UTC
This patch, build, and run tests all look good here.  Kristof, are you good with the proposed change?
Comment 7 Kristof Provost freebsd_committer freebsd_triage 2025-01-03 09:32:22 UTC
(In reply to Joseph Mingrone from comment #6)
Yes, that looks good.
Comment 8 Joseph Mingrone freebsd_committer freebsd_triage 2025-01-03 16:49:16 UTC
Does this attribution work for you?

commit d282c04ff45f1b27c35f77385b7f0b2cdb82c3dd (HEAD -> pflogd-fix)
Author: Eric A. Borisch <eborisch+FreeBSD@gmail.com>
Date:   2025-01-02 20:21

    tcpdump/print-pflog: Print uid with host endianness

    PR:             283799
    Reported by:    eborisch+FreeBSD@gmail.com
    Reviewed by:    jrm, kp
    Fixes:          0a7e5f1f02aad2ff5fff1c60f44c6975fd07e1d9
Comment 9 commit-hook freebsd_committer freebsd_triage 2025-01-03 17:41:49 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=d72f87c0fd1418bdb814594ea8fc76a202f7d5c6

commit d72f87c0fd1418bdb814594ea8fc76a202f7d5c6
Author:     Eric A. Borisch <eborisch+FreeBSD@gmail.com>
AuthorDate: 2025-01-03 00:21:52 +0000
Commit:     Joseph Mingrone <jrm@FreeBSD.org>
CommitDate: 2025-01-03 17:37:45 +0000

    tcpdump/print-pflog: Print uid with host endianness

    PR:             283799
    Reported by:    eborisch@gmail.com
    Reviewed by:    jrm, kp
    Fixes:          0a7e5f1f02aad2ff5fff1c60f44c6975fd07e1d9

 contrib/tcpdump/print-pflog.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Comment 10 Joseph Mingrone freebsd_committer freebsd_triage 2025-01-03 17:44:29 UTC
Fixed and will MFC. (Oh darn.  I see the email was only corrected in the Reported by: filed.  Sorry about that.)
Comment 11 commit-hook freebsd_committer freebsd_triage 2025-01-14 02:55:04 UTC
A commit in branch stable/14 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=9110e31e1df788495d62c74b1a969c6deb303423

commit 9110e31e1df788495d62c74b1a969c6deb303423
Author:     Eric A. Borisch <eborisch@gmail.com>
AuthorDate: 2025-01-03 00:21:52 +0000
Commit:     Joseph Mingrone <jrm@FreeBSD.org>
CommitDate: 2025-01-14 02:49:14 +0000

    tcpdump/print-pflog: Print uid with host endianness

    PR:             283799
    Reported by:    Eric A. Borisch <eborisch@gmail.com>
    Reviewed by:    jrm, kp
    Fixes:          0a7e5f1f02aad2ff5fff1c60f44c6975fd07e1d9

    (cherry picked from commit d72f87c0fd1418bdb814594ea8fc76a202f7d5c6)

 contrib/tcpdump/print-pflog.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)