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: <c7a8d5f64e19f529a7595f26e150826f@paul-moore.com>
Date: Thu, 25 Sep 2025 16:41:18 -0400
From: Paul Moore <paul@...l-moore.com>
To: Ricardo Robaina <rrobaina@...hat.com>, audit@...r.kernel.org, linux-kernel@...r.kernel.org, netfilter-devel@...r.kernel.org, coreteam@...filter.org
Cc: eparis@...hat.com, pablo@...filter.org, kadlec@...filter.org, fw@...len.de, ej@...i.de, Ricardo Robaina <rrobaina@...hat.com>
Subject: Re: [PATCH v2] audit: include source and destination ports to  NETFILTER_PKT

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