linux-kernel - Re: [PATCH v2] audit: include source and destination ports to NETFILTER

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAABTaaC9tSJ2Say6RHiQ3Ffm-xo4g-Ld3r83GwUBYZ5STs-hCA@mail.gmail.com>
Date: Fri, 26 Sep 2025 15:28:55 -0300
From: Ricardo Robaina <rrobaina@...hat.com>
To: Paul Moore <paul@...l-moore.com>
Cc: audit@...r.kernel.org, linux-kernel@...r.kernel.org, 
	netfilter-devel@...r.kernel.org, coreteam@...filter.org, eparis@...hat.com, 
	pablo@...filter.org, kadlec@...filter.org, fw@...len.de, ej@...i.de
Subject: Re: [PATCH v2] audit: include source and destination ports to NETFILTER_PKT

Thanks for reviewing this patch, Paul! Sounds great, I'll work on the
v3 shortly.

On Thu, Sep 25, 2025 at 5:41 PM Paul Moore <paul@...l-moore.com> wrote:
>
> On Sep 25, 2025 Ricardo Robaina <rrobaina@...hat.com> wrote:
> >
> > NETFILTER_PKT records show both source and destination
> > addresses, in addition to the associated networking protocol.
> > However, it lacks the ports information, which is often
> > valuable for troubleshooting.
> >
> > This patch adds both source and destination port numbers,
> > 'sport' and 'dport' respectively, to TCP, UDP, UDP-Lite and
> > SCTP-related NETFILTER_PKT records.
> >
> >  type=NETFILTER_PKT ... saddr=127.0.0.1 daddr=127.0.0.1 proto=icmp
> >  type=NETFILTER_PKT ... saddr=::1 daddr=::1 proto=ipv6-icmp
> >  type=NETFILTER_PKT ... daddr=127.0.0.1 proto=udp sport=38173 dport=42424
> >  type=NETFILTER_PKT ... daddr=::1 proto=udp sport=56852 dport=42424
> >  type=NETFILTER_PKT ... daddr=127.0.0.1 proto=tcp sport=57022 dport=42424
> >  type=NETFILTER_PKT ... daddr=::1 proto=tcp sport=50810 dport=42424
> >  type=NETFILTER_PKT ... daddr=127.0.0.1 proto=sctp sport=54944 dport=42424
> >  type=NETFILTER_PKT ... daddr=::1 proto=sctp sport=57963 dport=42424
> >
> > Link: https://github.com/linux-audit/audit-kernel/issues/162
> > Signed-off-by: Ricardo Robaina <rrobaina@...hat.com>
> > ---
> >  net/netfilter/xt_AUDIT.c | 42 +++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 41 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/netfilter/xt_AUDIT.c b/net/netfilter/xt_AUDIT.c
> > index b6a015aee0ce..9fc8a5429fa9 100644
> > --- a/net/netfilter/xt_AUDIT.c
> > +++ b/net/netfilter/xt_AUDIT.c
> > @@ -19,6 +19,7 @@
> >  #include <linux/netfilter_bridge/ebtables.h>
> >  #include <net/ipv6.h>
> >  #include <net/ip.h>
> > +#include <linux/sctp.h>
> >
> >  MODULE_LICENSE("GPL");
> >  MODULE_AUTHOR("Thomas Graf <tgraf@...hat.com>");
> > @@ -32,6 +33,7 @@ static bool audit_ip4(struct audit_buffer *ab, struct sk_buff *skb)
> >  {
> >       struct iphdr _iph;
> >       const struct iphdr *ih;
> > +     __be16 dport, sport;
> >
> >       ih = skb_header_pointer(skb, skb_network_offset(skb), sizeof(_iph), &_iph);
> >       if (!ih)
> > @@ -40,6 +42,25 @@ static bool audit_ip4(struct audit_buffer *ab, struct sk_buff *skb)
> >       audit_log_format(ab, " saddr=%pI4 daddr=%pI4 proto=%hhu",
> >                        &ih->saddr, &ih->daddr, ih->protocol);
> >
> > +     switch (ih->protocol) {
> > +     case IPPROTO_TCP:
> > +             sport = tcp_hdr(skb)->source;
> > +             dport = tcp_hdr(skb)->dest;
> > +             break;
> > +     case IPPROTO_UDP:
> > +     case IPPROTO_UDPLITE:
> > +             sport = udp_hdr(skb)->source;
> > +             dport = udp_hdr(skb)->dest;
> > +             break;
> > +     case IPPROTO_SCTP:
> > +             sport = sctp_hdr(skb)->source;
> > +             dport = sctp_hdr(skb)->dest;
> > +     }
> > +
> > +     if (ih->protocol == IPPROTO_TCP || ih->protocol == IPPROTO_UDP ||
> > +         ih->protocol == IPPROTO_UDPLITE || ih->protocol == IPPROTO_SCTP)
> > +             audit_log_format(ab, " sport=%hu dport=%hu", ntohs(sport), ntohs(dport));
> >       return true;
> >  }
>
> Instead of having the switch statement and then doing an additional if
> statement, why not fold it all into the switch statement?  Yes, you
> would have multiple audit_log_format() calls, but they are trivial to
> cut-n-paste, and it saves the extra per-packet checking at runtime.
>
>   switch (ih->protocol) {
>   case IPPROTO_TCP:
>     audit_log_format(ab, " sport=...",
>                      tcp_hdr(skb)->source,
>                      tcp_hdr(skb)->dest);
>     break;
>     ...
>   }
>
> ... considering how expensive multiple audit_log_format() calls can be,
> it might even be worth considering consolidating the two calls into one:
>
>   switch (ih->protocol) {
>   case IPPROTO_TCP:
>     audit_log_format(ab, " saddr=...",
>                      ih->saddr,
>                      ...
>                      tcp_hdr(skb)->source,
>                      tcp_hdr(skb)->dest);
>     break;
>     ...
>   default:
>     audit_log_format(ab, " saddr=...",
>                      ih->saddr,
>                      ...);
>   }
>
> --
> paul-moore.com
>