Also, thanks for all the awesome PF work that you have done! Its much
appreciated
---Mike
On 12/3/2021 1:05 AM, Özkan KIRIK wrote:
Thank you Kristof. The patch works properly!
On Wed, Dec 1, 2021 at 11:58 PM Kristof Provost <k...@freebsd.org> wrote:
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
