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

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

[Palla Raghunath]

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>
---
 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;
 
-- 
2.34.1

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

[Fernando Fernandez Mancera]

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!

>   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;
>   

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@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;
>