Re: git: 7f944794868f - stable/12 - pf: Introduce ridentifier

On 1 Dec 2021, at 5:59, Özkan KIRIK wrote:
> Because tshark/wireshark don't know this header change yet.
>
I’ve looked at the Wireshark parser code, and .. well, it’s wrong. 
It arbitrarily adds 3 bytes to the header length, and that’s not the 
correct solution. I’m not going to implement kernel workarounds for 
application bugs.

> And even though tcpdump has been patched by this commit, it still
> cannot parse the packet properly also.
>
Try this patch:

	diff --git a/sys/net/if_pflog.h b/sys/net/if_pflog.h
	index c77d8da1440a..93a69a2bb3a5 100644
	--- a/sys/net/if_pflog.h
	+++ b/sys/net/if_pflog.h
	@@ -31,6 +31,8 @@
	 #ifndef _NET_IF_PFLOG_H_
	 #define        _NET_IF_PFLOG_H_

	+#include <net/bpf.h>
	+
	 #define        PFLOGIFS_MAX    16

	 #define        PFLOG_RULESET_NAME_SIZE 16
	@@ -51,11 +53,13 @@ struct pfloghdr {
	        u_int8_t        dir;
	        u_int8_t        pad[3];
	        u_int32_t       ridentifier;
	+       u_int8_t        reserve;        /* Appease broken software like 
Wireshark. */
	+       u_int8_t        pad2[3];
	 };

	-#define        PFLOG_HDRLEN            sizeof(struct pfloghdr)
	+#define        PFLOG_HDRLEN            BPF_WORDALIGN(offsetof(struct 
pfloghdr, pad2))
	 /* minus pad, also used as a signature */
	-#define        PFLOG_REAL_HDRLEN       offsetof(struct pfloghdr, pad)
	+#define        PFLOG_REAL_HDRLEN       offsetof(struct pfloghdr, pad2)

	 #ifdef _KERNEL
	 struct pf_rule;
	diff --git a/sys/netpfil/pf/if_pflog.c b/sys/netpfil/pf/if_pflog.c
	index 4853c1301d6f..5ccdf3a7dd45 100644
	--- a/sys/netpfil/pf/if_pflog.c
	+++ b/sys/netpfil/pf/if_pflog.c
	@@ -215,7 +215,8 @@ pflog_packet(struct pfi_kkif *kif, struct mbuf *m, 
sa_family_t af, u_int8_t dir,
	                return (0);

	        bzero(&hdr, sizeof(hdr));
	-       hdr.length = PFLOG_HDRLEN;
	+       hdr.length = PFLOG_REAL_HDRLEN;
	        hdr.af = af;
	        hdr.action = rm->action;
	        hdr.reason = reason;

It looks like I had assumed that the header was expected to be aligned 
to 4 bytes, but it’s actually expected to be aligned to sizeof(long). 
This should fix that.

I’ve gone one further and added a dummy field to arrange the updated 
struct so that Wireshark will work, essentially retaining its incorrect 
assumption. It really should be fixed as well though.

Kristof