Date: Fri, 3 Dec 2021 09:05:02 -0500 From: mike tancsa <mike@sentex.net> To: Kristof Provost <kp@freebsd.org> Cc: freebsd-pf@freebsd.org Subject: Re: git: 7f944794868f - stable/12 - pf: Introduce ridentifier Message-ID: <1b5c4e46-7868-9fb9-4259-0c286b071310@sentex.net> In-Reply-To: <CAAcX-AHfWOcqEZBeFHGXo6VZ=ar6DyBEj1bvhjLHsX3UGCQVNw@mail.gmail.com> References: <202111261940.1AQJeGLZ022802@gitrepo.freebsd.org> <CAAcX-AFshuFUo7g9q_S3%2B5iS3Ko_Y%2BD0M5_nNEMPWOeMp66T3Q@mail.gmail.com> <52E4AB7A-6D27-4B11-ABCD-94BB12D389F4@FreeBSD.org> <CAAcX-AHfWOcqEZBeFHGXo6VZ=ar6DyBEj1bvhjLHsX3UGCQVNw@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
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 <kp@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
>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?1b5c4e46-7868-9fb9-4259-0c286b071310>