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

[Fernando Fernandez Mancera <fmancera@suse.de>]

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@syzkaller.appspotmail.com
>> Closes: https://syzkaller.appspot.com/bug?extid=5a8667f002726fc59f88
>> Signed-off-by: Palla Raghunath <raghunathpalla.0209@gmail.com>
>> ---
> 
> Reviewed-by: Fernando Fernandez Mancera <fmancera@suse.de>
> 
> 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;
>