Re: [PATCH 3/3 nf v4] netfilter: xtables: fix L4 header parsing for non-first fragments

[Pablo Neira Ayuso <pablo@netfilter.org>]

Hi Fernando,

On Mon, Apr 27, 2026 at 01:27:20PM +0200, Fernando Fernandez Mancera wrote:
> Multiple targets and matches relies on L4 header to operate. For
> fragmented packets, every fragment carries the transport protocol
> identifier, but only the first fragment contains the L4 header.
> 
> As the 'raw' table can be configured to run at priority -450 (before
> defragmentation at -400), the target/match can be reached before
> reassembly. In this case, non-first fragments have their payload
> incorrectly parsed as a TCP/UDP header. This would be of course a
> misconfiguration scenario. In most of the cases this just lead to a
> unreliable behavior for fragmented traffic.
> 
> Add a fragment check to ensure target/match only evaluates unfragmented
> packets or the first fragment in the stream.
> 
> Fixes: 902d6a4c2a4f ("netfilter: nf_defrag: Skip defrag if NOTRACK is set")
> Signed-off-by: Fernando Fernandez Mancera <fmancera@suse.de>
> ---
> v2: handled ecn, socket and tcpmss matches
> v3: extracted socket to its own patch with a generic solution for
> nft/xt, added a comment specifying that par->fragoff is fine for
> ecn/tcpmss ipv6 as they enforce -p tcp. Keep on mind that osf only
> supports ipv4.
> v4: handled xt_hashlimit too

Please, send a v5 including nft_payload_fast_eval():

diff --git a/net/netfilter/nf_tables_core.c b/net/netfilter/nf_tables_core.c
index 5ddd5b6e135f..8ab186f86dd4 100644
--- a/net/netfilter/nf_tables_core.c
+++ b/net/netfilter/nf_tables_core.c
@@ -153,7 +153,7 @@ static bool nft_payload_fast_eval(const struct nft_expr *expr,
        if (priv->base == NFT_PAYLOAD_NETWORK_HEADER)
                ptr = skb_network_header(skb) + pkt->nhoff;
        else {
-               if (!(pkt->flags & NFT_PKTINFO_L4PROTO))
+               if (!(pkt->flags & NFT_PKTINFO_L4PROTO) || pkt->fragoff)
                        return false;
                ptr = skb->data + nft_thoff(pkt);
        }


Thanks.


> ---
>  net/netfilter/xt_TPROXY.c    | 11 +++++++++--
>  net/netfilter/xt_ecn.c       |  4 ++++
>  net/netfilter/xt_hashlimit.c |  4 +++-
>  net/netfilter/xt_osf.c       |  3 +++
>  net/netfilter/xt_tcpmss.c    |  4 ++++
>  5 files changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/net/netfilter/xt_TPROXY.c b/net/netfilter/xt_TPROXY.c
> index e4bea1d346cf..5f60e7298a1e 100644
> --- a/net/netfilter/xt_TPROXY.c
> +++ b/net/netfilter/xt_TPROXY.c
> @@ -86,6 +86,9 @@ tproxy_tg4_v0(struct sk_buff *skb, const struct xt_action_param *par)
>  {
>  	const struct xt_tproxy_target_info *tgi = par->targinfo;
>  
> +	if (par->fragoff)
> +		return NF_DROP;
> +
>  	return tproxy_tg4(xt_net(par), skb, tgi->laddr, tgi->lport,
>  			  tgi->mark_mask, tgi->mark_value);
>  }
> @@ -95,6 +98,9 @@ tproxy_tg4_v1(struct sk_buff *skb, const struct xt_action_param *par)
>  {
>  	const struct xt_tproxy_target_info_v1 *tgi = par->targinfo;
>  
> +	if (par->fragoff)
> +		return NF_DROP;
> +
>  	return tproxy_tg4(xt_net(par), skb, tgi->laddr.ip, tgi->lport,
>  			  tgi->mark_mask, tgi->mark_value);
>  }
> @@ -106,6 +112,7 @@ tproxy_tg6_v1(struct sk_buff *skb, const struct xt_action_param *par)
>  {
>  	const struct ipv6hdr *iph = ipv6_hdr(skb);
>  	const struct xt_tproxy_target_info_v1 *tgi = par->targinfo;
> +	unsigned short fragoff = 0;
>  	struct udphdr _hdr, *hp;
>  	struct sock *sk;
>  	const struct in6_addr *laddr;
> @@ -113,8 +120,8 @@ tproxy_tg6_v1(struct sk_buff *skb, const struct xt_action_param *par)
>  	int thoff = 0;
>  	int tproto;
>  
> -	tproto = ipv6_find_hdr(skb, &thoff, -1, NULL, NULL);
> -	if (tproto < 0)
> +	tproto = ipv6_find_hdr(skb, &thoff, -1, &fragoff, NULL);
> +	if (tproto < 0 || fragoff)
>  		return NF_DROP;
>  
>  	hp = skb_header_pointer(skb, thoff, sizeof(_hdr), &_hdr);
> diff --git a/net/netfilter/xt_ecn.c b/net/netfilter/xt_ecn.c
> index b96e8203ac54..a8503f5d26bf 100644
> --- a/net/netfilter/xt_ecn.c
> +++ b/net/netfilter/xt_ecn.c
> @@ -30,6 +30,10 @@ static bool match_tcp(const struct sk_buff *skb, struct xt_action_param *par)
>  	struct tcphdr _tcph;
>  	const struct tcphdr *th;
>  
> +	/* this is fine for IPv6 as ecn_mt_check6() enforces -p tcp */
> +	if (par->fragoff)
> +		return false;
> +
>  	/* In practice, TCP match does this, so can't fail.  But let's
>  	 * be good citizens.
>  	 */
> diff --git a/net/netfilter/xt_hashlimit.c b/net/netfilter/xt_hashlimit.c
> index 3bd127bfc114..2704b4b60d1e 100644
> --- a/net/netfilter/xt_hashlimit.c
> +++ b/net/netfilter/xt_hashlimit.c
> @@ -658,6 +658,8 @@ hashlimit_init_dst(const struct xt_hashlimit_htable *hinfo,
>  		if (!(hinfo->cfg.mode &
>  		      (XT_HASHLIMIT_HASH_DPT | XT_HASHLIMIT_HASH_SPT)))
>  			return 0;
> +		if (ntohs(ip_hdr(skb)->frag_off) & IP_OFFSET)
> +			return -1;
>  		nexthdr = ip_hdr(skb)->protocol;
>  		break;
>  #if IS_ENABLED(CONFIG_IP6_NF_IPTABLES)
> @@ -681,7 +683,7 @@ hashlimit_init_dst(const struct xt_hashlimit_htable *hinfo,
>  			return 0;
>  		nexthdr = ipv6_hdr(skb)->nexthdr;
>  		protoff = ipv6_skip_exthdr(skb, sizeof(struct ipv6hdr), &nexthdr, &frag_off);
> -		if ((int)protoff < 0)
> +		if ((int)protoff < 0 || ntohs(frag_off) & IP6_OFFSET)
>  			return -1;
>  		break;
>  	}
> diff --git a/net/netfilter/xt_osf.c b/net/netfilter/xt_osf.c
> index dc9485854002..e8807caede68 100644
> --- a/net/netfilter/xt_osf.c
> +++ b/net/netfilter/xt_osf.c
> @@ -27,6 +27,9 @@
>  static bool
>  xt_osf_match_packet(const struct sk_buff *skb, struct xt_action_param *p)
>  {
> +	if (p->fragoff)
> +		return false;
> +
>  	return nf_osf_match(skb, xt_family(p), xt_hooknum(p), xt_in(p),
>  			    xt_out(p), p->matchinfo, xt_net(p), nf_osf_fingers);
>  }
> diff --git a/net/netfilter/xt_tcpmss.c b/net/netfilter/xt_tcpmss.c
> index 0d32d4841cb3..b9da8269161d 100644
> --- a/net/netfilter/xt_tcpmss.c
> +++ b/net/netfilter/xt_tcpmss.c
> @@ -32,6 +32,10 @@ tcpmss_mt(const struct sk_buff *skb, struct xt_action_param *par)
>  	u8 _opt[15 * 4 - sizeof(_tcph)];
>  	unsigned int i, optlen;
>  
> +	/* this is fine for IPv6 as xt_tcpmss enforces -p tcp */
> +	if (par->fragoff)
> +		return false;
> +
>  	/* If we don't have the whole header, drop packet. */
>  	th = skb_header_pointer(skb, par->thoff, sizeof(_tcph), &_tcph);
>  	if (th == NULL)
> -- 
> 2.53.0
> 
>