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

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

On Fri, Apr 24, 2026 at 04:33:56PM +0200, Fernando Fernandez Mancera wrote:
> On 4/24/26 1:38 PM, Pablo Neira Ayuso wrote:
> > On Tue, Apr 21, 2026 at 12:44:09PM +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.
> > 
> 
> Hi Pablo,
> 
> > AI reports xt_hashlimit could be a good candidate to check for
> > fragoff, I think it is, so I would suggest to expand it there to cover
> > this.
> > 
> 
> This seems like a good catch. I will work on this.
> 
> > It also mentions synproxy as another candidate but IPv6 synproxy does
> > not do ipv6_find_hdr() on purpose I think (it assumed nexthdr is TCP)
> > for SYN and ACK packets, so checking for fragoff there is not
> > possible. Given this is to deal with flood, I think think it is worth
> > the fragoff validation.
> > 
> 
> I don't think it makes sense for SYNPROXY. SYNPROXY requires conntrack to
> work both ipt_SYNPROXY and nft_synproxy and only allows LOCAL_IN and FORWARD
> hooks so it should be fine AFAIU. I don't understand how someone could skip
> defragmentation here.
> 
> Maybe something extra to add to ipt_SYNPROXY is a restriction for TCP
> protocol only. As the code currently assumes the transport layer is TCP
> which isn't enforced.
> 
> But that would be kind of a different fix. What do you think?

My suggestion is to update xt_hashlimit to handle fragoff.

Regarding synproxy, it is a right indeed that this depends on
nf_defrag so no changes are needed there.

So please send a v4 including xt_hashlimit.

Thanks Fernando.

> > > 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.
> > > ---
> > >   net/netfilter/xt_TPROXY.c | 11 +++++++++--
> > >   net/netfilter/xt_ecn.c    |  4 ++++
> > >   net/netfilter/xt_osf.c    |  3 +++
> > >   net/netfilter/xt_tcpmss.c |  4 ++++
> > >   4 files changed, 20 insertions(+), 2 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_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
> > > 
> > 
>