netdev - Re: [PATCH net-next] netfilter: conntrack: avoid sending RST to reply out-of-window skb

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240307093310.GI4420@breakpoint.cc>
Date: Thu, 7 Mar 2024 10:33:10 +0100
From: Florian Westphal <fw@...len.de>
To: Jason Xing <kerneljasonxing@...il.com>
Cc: edumazet@...gle.com, pablo@...filter.org, kadlec@...filter.org,
	fw@...len.de, kuba@...nel.org, pabeni@...hat.com,
	davem@...emloft.net, netfilter-devel@...r.kernel.org,
	coreteam@...filter.org, netdev@...r.kernel.org,
	Jason Xing <kernelxing@...cent.com>
Subject: Re: [PATCH net-next] netfilter: conntrack: avoid sending RST to
 reply out-of-window skb

Jason Xing <kerneljasonxing@...il.com> wrote:
> client_ip:client_port <--> server_ip:b_port
> 
> Then, some strange skbs from client or gateway, say, out-of-window
> skbs are sent to the server_ip:a_port (not b_port) due to DNAT
> clearing skb->_nfct value in nf_conntrack_in() function. Why?
> Because the tcp_in_window() considers the incoming skb as an
> invalid skb by returning NFCT_TCP_INVALID.

So far everything is as intended.

> I think, even we have set DNAT policy, it would be better if the
> whole process/behaviour adheres to the original TCP behaviour.
> 
> Signed-off-by: Jason Xing <kernelxing@...cent.com>
> ---
>  net/netfilter/nf_conntrack_proto_tcp.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/net/netfilter/nf_conntrack_proto_tcp.c b/net/netfilter/nf_conntrack_proto_tcp.c
> index ae493599a3ef..3f3e620f3969 100644
> --- a/net/netfilter/nf_conntrack_proto_tcp.c
> +++ b/net/netfilter/nf_conntrack_proto_tcp.c
> @@ -1253,13 +1253,11 @@ int nf_conntrack_tcp_packet(struct nf_conn *ct,
>  	res = tcp_in_window(ct, dir, index,
>  			    skb, dataoff, th, state);
>  	switch (res) {
> -	case NFCT_TCP_IGNORE:
> -		spin_unlock_bh(&ct->lock);
> -		return NF_ACCEPT;
>  	case NFCT_TCP_INVALID:
>  		nf_tcp_handle_invalid(ct, dir, index, skb, state);
> +	case NFCT_TCP_IGNORE:
>  		spin_unlock_bh(&ct->lock);
> -		return -NF_ACCEPT;
> +		return NF_ACCEPT;

This looks wrong.  -NF_ACCEPT means 'pass packet, but its not part
of the connection' (packet will match --ctstate INVALID check).

This change disables most of the tcp_in_window() test, this will
pretend everything is fine even though tcp_in_window says otherwise.

You could:
 - drop invalid tcp packets in input hook
 - set nf_conntrack_tcp_be_liberal=1

both will avoid this 'rst' issue.