Re: [PATCH nf] netfilter: ip6t_SYNPROXY: check TCP header before verifying checksum

[Fernando Fernandez Mancera]

On 10/9/26 4:28 PM, Fernando Fernandez Mancera wrote:
> On 9/26/26 10:45 PM, Palla Raghunath wrote:
> > synproxy_tg6() passes par->thoff to nf_ip6_checksum() before it has
> > checked that a TCP header is actually present at that offset.
> >
> > par->thoff comes from ipv6_find_hdr(), called by ip6_packet_match()
> > with target -1. ipv6_find_hdr() only checks that each extension header's
> > first two bytes are in the skb. It then adds that header's declared
> > length to the offset and stops at the first non-extension header, so it
> > can return an offset past the end of the packet. With
> > CHECKSUM_NONE, nf_ip6_checksum() calls skb_checksum(skb, 0, thoff, 0).
> > When thoff is larger than skb->len, the BUG_ON(len) in skb_checksum()
> > fires:
> >
> >    kernel BUG at net/core/skbuff.c:3606!
> >    RIP: 0010:skb_checksum+0x8b2/0x8c0
> >    Call Trace:
> >     nf_ip6_checksum+0x1bd/0x320 net/netfilter/utils.c:89
> >     synproxy_tg6+0x1a4/0x6e0 net/ipv6/netfilter/ip6t_SYNPROXY.c:21
> >     ip6t_do_table+0xd37/0x15b0 net/ipv6/netfilter/ip6_tables.c:366
> >     ...
> >
> > In the syzbot reproducer, a 60-byte packet carries an AH header, then two
> > hop-by-hop headers. The last hop-by-hop header has nexthdr TCP and
> > hdrlen 167, so thoff is 1400.
> >
> > Fetch the TCP header with skb_header_pointer() first, as
> > nf_reject_ip6_tcphdr_get() already does. This drops packets that do not
> > contain a full TCP header at thoff before the checksum is computed.
> >
> > Fixes: 4ad362282cb4 ("netfilter: add IPv6 SYNPROXY target")
> > Reported-by: syzbot+5a8667f002726fc59f88@xxxxxxxxxxxxxxxxxxxxxxxxx
> > Closes: https://syzkaller.appspot.com/bug?extid=5a8667f002726fc59f88
> > Signed-off-by: Palla Raghunath <raghunathpalla.0209@xxxxxxxxx>
> > ---
>
> Reviewed-by: Fernando Fernandez Mancera <fmancera@xxxxxxx>
>
> The nftables synproxy eval path is also affected but I think it would be 
> better to keep it in a separate patch in order to facilitate 
> backporting. The issue were introduced at different times and nftables 
> synproxy expression is "recent" compared to the iptables target.
>
> I am sending a patch for the nftables expression and will CC you in case 
> you can test it too.
>
> Thanks!

Actually, I thought twice about this. Could you please include the 
following diff in a v2? In addition, I think the comment is a bit 
redundant/not necessary. Could you just strip it?

Please use also the following fixes tag too.

Fixes: a311a8981727 ("netfilter: nft_synproxy: use the family-aware 
checksum helper")

Thanks,
Fernando.

diff --git a/net/netfilter/nft_synproxy.c b/net/netfilter/nft_synproxy.c
index 554a96a000f4..6b290a89886d 100644
--- a/net/netfilter/nft_synproxy.c
+++ b/net/netfilter/nft_synproxy.c
@@ -118,12 +118,6 @@ static void nft_synproxy_do_eval(const struct 
nft_synproxy *priv,
 		return;
 	}

-	if (nf_checksum(skb, nft_hook(pkt), thoff, IPPROTO_TCP,
-			nft_pf(pkt))) {
-		regs->verdict.code = NF_DROP;
-		return;
-	}
-
 	tcp = skb_header_pointer(skb, thoff,
 				 sizeof(struct tcphdr),
 				 &_tcph);
@@ -132,6 +126,12 @@ static void nft_synproxy_do_eval(const struct 
nft_synproxy *priv,
 		return;
 	}

+	if (nf_checksum(skb, nft_hook(pkt), thoff, IPPROTO_TCP,
+			nft_pf(pkt))) {
+		regs->verdict.code = NF_DROP;
+		return;
+	}
+
 	if (!synproxy_parse_options(skb, thoff, tcp, &opts)) {
 		regs->verdict.code = NF_DROP;
 		return;


> >   net/ipv6/netfilter/ip6t_SYNPROXY.c | 9 ++++++---
> >   1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/net/ipv6/netfilter/ip6t_SYNPROXY.c b/net/ipv6/netfilter/ 
> > ip6t_SYNPROXY.c
> > index d51d0c3e5fe9..04db1f82420b 100644
> > --- a/net/ipv6/netfilter/ip6t_SYNPROXY.c
> > +++ b/net/ipv6/netfilter/ip6t_SYNPROXY.c
> > @@ -18,13 +18,16 @@ synproxy_tg6(struct sk_buff *skb, const struct 
> > xt_action_param *par)
> >       struct synproxy_options opts = {};
> >       struct tcphdr *th, _th;
> > -    if (nf_ip6_checksum(skb, xt_hooknum(par), par->thoff, IPPROTO_TCP))
> > -        return NF_DROP;
> > -
> > +    /* par->thoff may point past the end of the packet; make sure a
> > +     * full TCP header is present before checksumming up to it.
> > +     */
> >       th = skb_header_pointer(skb, par->thoff, sizeof(_th), &_th);
> >       if (th == NULL)
> >           return NF_DROP;
> > +    if (nf_ip6_checksum(skb, xt_hooknum(par), par->thoff, IPPROTO_TCP))
> > +        return NF_DROP;
> > +
> >       if (!synproxy_parse_options(skb, par->thoff, th, &opts))
> >           return NF_DROP;