[PATCH nf 1/2] netfilter: conntrack: move synack init code to helper

[PATCH nf 1/2] netfilter: conntrack: move synack init code to helper

[Florian Westphal]

It seems more readable to use a common helper in the followup fix rather
than copypaste or goto.

No functional change intended.  The function is only called for syn-ack
or syn in repy direction in case of simultaneous open.

Cc: Jozsef Kadlecsik <kadlec@netfilter.org>
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 net/netfilter/nf_conntrack_proto_tcp.c | 47 ++++++++++++++++----------
 1 file changed, 29 insertions(+), 18 deletions(-)

diff --git a/net/netfilter/nf_conntrack_proto_tcp.c b/net/netfilter/nf_conntrack_proto_tcp.c
index af5115e127cf..88c89e97d8a2 100644
--- a/net/netfilter/nf_conntrack_proto_tcp.c
+++ b/net/netfilter/nf_conntrack_proto_tcp.c
@@ -446,6 +446,32 @@ static void tcp_sack(const struct sk_buff *skb, unsigned int dataoff,
 	}
 }
 
+static void tcp_init_sender(struct ip_ct_tcp_state *sender,
+			    struct ip_ct_tcp_state *receiver,
+			    const struct sk_buff *skb,
+			    unsigned int dataoff,
+			    const struct tcphdr *tcph,
+			    u32 end, u32 win)
+{
+	/* SYN-ACK in reply to a SYN
+	 * or SYN from reply direction in simultaneous open.
+	 */
+	sender->td_end =
+	sender->td_maxend = end;
+	sender->td_maxwin = (win == 0 ? 1 : win);
+
+	tcp_options(skb, dataoff, tcph, sender);
+	/* RFC 1323:
+	 * Both sides must send the Window Scale option
+	 * to enable window scaling in either direction.
+	 */
+	if (!(sender->flags & IP_CT_TCP_FLAG_WINDOW_SCALE &&
+	      receiver->flags & IP_CT_TCP_FLAG_WINDOW_SCALE)) {
+		sender->td_scale = 0;
+		receiver->td_scale = 0;
+	}
+}
+
 static bool tcp_in_window(struct nf_conn *ct,
 			  enum ip_conntrack_dir dir,
 			  unsigned int index,
@@ -499,24 +525,9 @@ static bool tcp_in_window(struct nf_conn *ct,
 		 * Initialize sender data.
 		 */
 		if (tcph->syn) {
-			/*
-			 * SYN-ACK in reply to a SYN
-			 * or SYN from reply direction in simultaneous open.
-			 */
-			sender->td_end =
-			sender->td_maxend = end;
-			sender->td_maxwin = (win == 0 ? 1 : win);
-
-			tcp_options(skb, dataoff, tcph, sender);
-			/*
-			 * RFC 1323:
-			 * Both sides must send the Window Scale option
-			 * to enable window scaling in either direction.
-			 */
-			if (!(sender->flags & IP_CT_TCP_FLAG_WINDOW_SCALE
-			      && receiver->flags & IP_CT_TCP_FLAG_WINDOW_SCALE))
-				sender->td_scale =
-				receiver->td_scale = 0;
+			tcp_init_sender(sender, receiver,
+					skb, dataoff, tcph,
+					end, win);
 			if (!tcph->ack)
 				/* Simultaneous open */
 				return true;
-- 
2.34.1

[PATCH nf 2/2] netfilter: conntrack: re-init state for retransmitted syn-ack

[Florian Westphal]

TCP conntrack assumes that a syn-ack retransmit is identical to the
previous syn-ack.  This isn't correct and causes stuck 3whs in some more
esoteric scenarios.  tcpdump to illustrate the problem:

 client > server: Flags [S] seq 1365731894, win 29200, [mss 1460,sackOK,TS val 2083035583 ecr 0,wscale 7]
 server > client: Flags [S.] seq 145824453, ack 643160523, win 65535, [mss 8952,wscale 5,TS val 3215367629 ecr 2082921663]

Note the invalid/outdated synack ack number.
Conntrack marks this syn-ack as out-of-window/invalid, but it did
initialize the reply direction parameters based on this packets content.

 client > server: Flags [S] seq 1365731894, win 29200, [mss 1460,sackOK,TS val 2083036623 ecr 0,wscale 7]

... retransmit...

 server > client: Flags [S.], seq 145824453, ack 643160523, win 65535, [mss 8952,wscale 5,TS val 3215368644 ecr 2082921663]

and another bogus synack. This repeats, then client re-uses for a new
attempt:

client > server: Flags [S], seq 2375731741, win 29200, [mss 1460,sackOK,TS val 2083100223 ecr 0,wscale 7]
server > client: Flags [S.], seq 145824453, ack 643160523, win 65535, [mss 8952,wscale 5,TS val 3215430754 ecr 2082921663]

... but still gets a invalid syn-ack.

This repeats until:

 server > client: Flags [S.], seq 145824453, ack 643160523, win 65535, [mss 8952,wscale 5,TS val 3215437785 ecr 2082921663]
 server > client: Flags [R.], seq 145824454, ack 643160523, win 65535, [mss 8952,wscale 5,TS val 3215443451 ecr 2082921663]
 client > server: Flags [S], seq 2375731741, win 29200, [mss 1460,sackOK,TS val 2083115583 ecr 0,wscale 7]
 server > client: Flags [S.], seq 162602410, ack 2375731742, win 65535, [mss 8952,wscale 5,TS val 3215445754 ecr 2083115583]

This syn-ack has the correct ack number, but conntrack flags it as
invalid: The internal state was created from the first syn-ack seen
so the sequence number of the syn-ack is treated as being outside of
the announced window.

Don't assume that retransmitted syn-ack is identical to previous one.
Treat it like the first syn-ack and reinit state.

Cc: Jozsef Kadlecsik <kadlec@netfilter.org>
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 net/netfilter/nf_conntrack_proto_tcp.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/net/netfilter/nf_conntrack_proto_tcp.c b/net/netfilter/nf_conntrack_proto_tcp.c
index 88c89e97d8a2..d1582b888c0d 100644
--- a/net/netfilter/nf_conntrack_proto_tcp.c
+++ b/net/netfilter/nf_conntrack_proto_tcp.c
@@ -571,6 +571,18 @@ static bool tcp_in_window(struct nf_conn *ct,
 		sender->td_maxwin = (win == 0 ? 1 : win);
 
 		tcp_options(skb, dataoff, tcph, sender);
+	} else if (tcph->syn && dir == IP_CT_DIR_REPLY &&
+		   state->state == TCP_CONNTRACK_SYN_SENT) {
+		/* Retransmitted syn-ack, or syn (simultaneous open).
+		 *
+		 * Re-init state for this direction, just like for the first
+		 * syn(-ack) reply, it might differ in seq, ack or tcp options.
+		 */
+		tcp_init_sender(sender, receiver,
+				skb, dataoff, tcph,
+				end, win);
+		if (!tcph->ack)
+			return true;
 	}
 
 	if (!(tcph->ack)) {
-- 
2.34.1

Re: [PATCH nf 1/2] netfilter: conntrack: move synack init code to helper

[Jozsef Kadlecsik]

On Sat, 29 Jan 2022, Florian Westphal wrote:

> It seems more readable to use a common helper in the followup fix rather
> than copypaste or goto.
> 
> No functional change intended.  The function is only called for syn-ack
> or syn in repy direction in case of simultaneous open.
> 
> Cc: Jozsef Kadlecsik <kadlec@netfilter.org>
> Signed-off-by: Florian Westphal <fw@strlen.de>

Acked-by: Jozsef Kadlecsik <kadlec@netfilter.org>

> ---
>  net/netfilter/nf_conntrack_proto_tcp.c | 47 ++++++++++++++++----------
>  1 file changed, 29 insertions(+), 18 deletions(-)
> 
> diff --git a/net/netfilter/nf_conntrack_proto_tcp.c b/net/netfilter/nf_conntrack_proto_tcp.c
> index af5115e127cf..88c89e97d8a2 100644
> --- a/net/netfilter/nf_conntrack_proto_tcp.c
> +++ b/net/netfilter/nf_conntrack_proto_tcp.c
> @@ -446,6 +446,32 @@ static void tcp_sack(const struct sk_buff *skb, unsigned int dataoff,
>  	}
>  }
>  
> +static void tcp_init_sender(struct ip_ct_tcp_state *sender,
> +			    struct ip_ct_tcp_state *receiver,
> +			    const struct sk_buff *skb,
> +			    unsigned int dataoff,
> +			    const struct tcphdr *tcph,
> +			    u32 end, u32 win)
> +{
> +	/* SYN-ACK in reply to a SYN
> +	 * or SYN from reply direction in simultaneous open.
> +	 */
> +	sender->td_end =
> +	sender->td_maxend = end;
> +	sender->td_maxwin = (win == 0 ? 1 : win);
> +
> +	tcp_options(skb, dataoff, tcph, sender);
> +	/* RFC 1323:
> +	 * Both sides must send the Window Scale option
> +	 * to enable window scaling in either direction.
> +	 */
> +	if (!(sender->flags & IP_CT_TCP_FLAG_WINDOW_SCALE &&
> +	      receiver->flags & IP_CT_TCP_FLAG_WINDOW_SCALE)) {
> +		sender->td_scale = 0;
> +		receiver->td_scale = 0;
> +	}
> +}
> +
>  static bool tcp_in_window(struct nf_conn *ct,
>  			  enum ip_conntrack_dir dir,
>  			  unsigned int index,
> @@ -499,24 +525,9 @@ static bool tcp_in_window(struct nf_conn *ct,
>  		 * Initialize sender data.
>  		 */
>  		if (tcph->syn) {
> -			/*
> -			 * SYN-ACK in reply to a SYN
> -			 * or SYN from reply direction in simultaneous open.
> -			 */
> -			sender->td_end =
> -			sender->td_maxend = end;
> -			sender->td_maxwin = (win == 0 ? 1 : win);
> -
> -			tcp_options(skb, dataoff, tcph, sender);
> -			/*
> -			 * RFC 1323:
> -			 * Both sides must send the Window Scale option
> -			 * to enable window scaling in either direction.
> -			 */
> -			if (!(sender->flags & IP_CT_TCP_FLAG_WINDOW_SCALE
> -			      && receiver->flags & IP_CT_TCP_FLAG_WINDOW_SCALE))
> -				sender->td_scale =
> -				receiver->td_scale = 0;
> +			tcp_init_sender(sender, receiver,
> +					skb, dataoff, tcph,
> +					end, win);
>  			if (!tcph->ack)
>  				/* Simultaneous open */
>  				return true;
> -- 
> 2.34.1
> 
> 

-
E-mail  : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.hu
PGP key : https://wigner.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics
          H-1525 Budapest 114, POB. 49, Hungary

Re: [PATCH nf 2/2] netfilter: conntrack: re-init state for retransmitted syn-ack

[Jozsef Kadlecsik]

Hi Florian,

On Sat, 29 Jan 2022, Florian Westphal wrote:

> TCP conntrack assumes that a syn-ack retransmit is identical to the
> previous syn-ack.  This isn't correct and causes stuck 3whs in some more
> esoteric scenarios.  tcpdump to illustrate the problem:
> 
>  client > server: Flags [S] seq 1365731894, win 29200, [mss 1460,sackOK,TS val 2083035583 ecr 0,wscale 7]
>  server > client: Flags [S.] seq 145824453, ack 643160523, win 65535, [mss 8952,wscale 5,TS val 3215367629 ecr 2082921663]
> 
> Note the invalid/outdated synack ack number.
> Conntrack marks this syn-ack as out-of-window/invalid, but it did
> initialize the reply direction parameters based on this packets content.
> 
>  client > server: Flags [S] seq 1365731894, win 29200, [mss 1460,sackOK,TS val 2083036623 ecr 0,wscale 7]
> 
> ... retransmit...
> 
>  server > client: Flags [S.], seq 145824453, ack 643160523, win 65535, [mss 8952,wscale 5,TS val 3215368644 ecr 2082921663]
> 
> and another bogus synack. This repeats, then client re-uses for a new
> attempt:
> 
> client > server: Flags [S], seq 2375731741, win 29200, [mss 1460,sackOK,TS val 2083100223 ecr 0,wscale 7]
> server > client: Flags [S.], seq 145824453, ack 643160523, win 65535, [mss 8952,wscale 5,TS val 3215430754 ecr 2082921663]
> 
> ... but still gets a invalid syn-ack.
> 
> This repeats until:
> 
>  server > client: Flags [S.], seq 145824453, ack 643160523, win 65535, [mss 8952,wscale 5,TS val 3215437785 ecr 2082921663]
>  server > client: Flags [R.], seq 145824454, ack 643160523, win 65535, [mss 8952,wscale 5,TS val 3215443451 ecr 2082921663]
>  client > server: Flags [S], seq 2375731741, win 29200, [mss 1460,sackOK,TS val 2083115583 ecr 0,wscale 7]
>  server > client: Flags [S.], seq 162602410, ack 2375731742, win 65535, [mss 8952,wscale 5,TS val 3215445754 ecr 2083115583]
> 
> This syn-ack has the correct ack number, but conntrack flags it as
> invalid: The internal state was created from the first syn-ack seen
> so the sequence number of the syn-ack is treated as being outside of
> the announced window.

I can only assume that the client is/are behind like a carrier-grade NAT
and the bogus SYN-ACK sent by the server is replying a connection attempt 
from another client. Yes, the best thing to do is to reinit the state.

> Don't assume that retransmitted syn-ack is identical to previous one.
> Treat it like the first syn-ack and reinit state.
> 
> Cc: Jozsef Kadlecsik <kadlec@netfilter.org>
> Signed-off-by: Florian Westphal <fw@strlen.de>

Acked-by: Jozsef Kadlecsik <kadlec@netfilter.org>

Best regards,
Jozsef

> ---
>  net/netfilter/nf_conntrack_proto_tcp.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/net/netfilter/nf_conntrack_proto_tcp.c b/net/netfilter/nf_conntrack_proto_tcp.c
> index 88c89e97d8a2..d1582b888c0d 100644
> --- a/net/netfilter/nf_conntrack_proto_tcp.c
> +++ b/net/netfilter/nf_conntrack_proto_tcp.c
> @@ -571,6 +571,18 @@ static bool tcp_in_window(struct nf_conn *ct,
>  		sender->td_maxwin = (win == 0 ? 1 : win);
>  
>  		tcp_options(skb, dataoff, tcph, sender);
> +	} else if (tcph->syn && dir == IP_CT_DIR_REPLY &&
> +		   state->state == TCP_CONNTRACK_SYN_SENT) {
> +		/* Retransmitted syn-ack, or syn (simultaneous open).
> +		 *
> +		 * Re-init state for this direction, just like for the first
> +		 * syn(-ack) reply, it might differ in seq, ack or tcp options.
> +		 */
> +		tcp_init_sender(sender, receiver,
> +				skb, dataoff, tcph,
> +				end, win);
> +		if (!tcph->ack)
> +			return true;
>  	}
>  
>  	if (!(tcph->ack)) {
> -- 
> 2.34.1
> 
> 

-
E-mail  : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.hu
PGP key : https://wigner.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics
          H-1525 Budapest 114, POB. 49, Hungary

Re: [PATCH nf 2/2] netfilter: conntrack: re-init state for retransmitted syn-ack

[Florian Westphal]

Jozsef Kadlecsik <kadlec@netfilter.org> wrote:
> I can only assume that the client is/are behind like a carrier-grade NAT
> and the bogus SYN-ACK sent by the server is replying a connection attempt 
> from another client. Yes, the best thing to do is to reinit the state.

Yes, thats my guess as well, some sort of CGN or stateless nat hding
multiple clients.

Thanks for the quick review!

Re: [PATCH nf 1/2] netfilter: conntrack: move synack init code to helper

[Pablo Neira Ayuso]

On Sat, Jan 29, 2022 at 09:18:54PM +0100, Jozsef Kadlecsik wrote:
> On Sat, 29 Jan 2022, Florian Westphal wrote:
> 
> > It seems more readable to use a common helper in the followup fix rather
> > than copypaste or goto.
> > 
> > No functional change intended.  The function is only called for syn-ack
> > or syn in repy direction in case of simultaneous open.
> > 
> > Cc: Jozsef Kadlecsik <kadlec@netfilter.org>
> > Signed-off-by: Florian Westphal <fw@strlen.de>
> 
> Acked-by: Jozsef Kadlecsik <kadlec@netfilter.org>

Applied, thanks

Re: [PATCH nf 2/2] netfilter: conntrack: re-init state for retransmitted syn-ack

[Pablo Neira Ayuso]

On Sat, Jan 29, 2022 at 09:25:05PM +0100, Jozsef Kadlecsik wrote:
> Hi Florian,
> 
> On Sat, 29 Jan 2022, Florian Westphal wrote:
> 
> > TCP conntrack assumes that a syn-ack retransmit is identical to the
> > previous syn-ack.  This isn't correct and causes stuck 3whs in some more
> > esoteric scenarios.  tcpdump to illustrate the problem:
> > 
> >  client > server: Flags [S] seq 1365731894, win 29200, [mss 1460,sackOK,TS val 2083035583 ecr 0,wscale 7]
> >  server > client: Flags [S.] seq 145824453, ack 643160523, win 65535, [mss 8952,wscale 5,TS val 3215367629 ecr 2082921663]
> > 
> > Note the invalid/outdated synack ack number.
> > Conntrack marks this syn-ack as out-of-window/invalid, but it did
> > initialize the reply direction parameters based on this packets content.
> > 
> >  client > server: Flags [S] seq 1365731894, win 29200, [mss 1460,sackOK,TS val 2083036623 ecr 0,wscale 7]
> > 
> > ... retransmit...
> > 
> >  server > client: Flags [S.], seq 145824453, ack 643160523, win 65535, [mss 8952,wscale 5,TS val 3215368644 ecr 2082921663]
> > 
> > and another bogus synack. This repeats, then client re-uses for a new
> > attempt:
> > 
> > client > server: Flags [S], seq 2375731741, win 29200, [mss 1460,sackOK,TS val 2083100223 ecr 0,wscale 7]
> > server > client: Flags [S.], seq 145824453, ack 643160523, win 65535, [mss 8952,wscale 5,TS val 3215430754 ecr 2082921663]
> > 
> > ... but still gets a invalid syn-ack.
> > 
> > This repeats until:
> > 
> >  server > client: Flags [S.], seq 145824453, ack 643160523, win 65535, [mss 8952,wscale 5,TS val 3215437785 ecr 2082921663]
> >  server > client: Flags [R.], seq 145824454, ack 643160523, win 65535, [mss 8952,wscale 5,TS val 3215443451 ecr 2082921663]
> >  client > server: Flags [S], seq 2375731741, win 29200, [mss 1460,sackOK,TS val 2083115583 ecr 0,wscale 7]
> >  server > client: Flags [S.], seq 162602410, ack 2375731742, win 65535, [mss 8952,wscale 5,TS val 3215445754 ecr 2083115583]
> > 
> > This syn-ack has the correct ack number, but conntrack flags it as
> > invalid: The internal state was created from the first syn-ack seen
> > so the sequence number of the syn-ack is treated as being outside of
> > the announced window.
> 
> I can only assume that the client is/are behind like a carrier-grade NAT
> and the bogus SYN-ACK sent by the server is replying a connection attempt 
> from another client. Yes, the best thing to do is to reinit the state.
> 
> > Don't assume that retransmitted syn-ack is identical to previous one.
> > Treat it like the first syn-ack and reinit state.
> > 
> > Cc: Jozsef Kadlecsik <kadlec@netfilter.org>
> > Signed-off-by: Florian Westphal <fw@strlen.de>
> 
> Acked-by: Jozsef Kadlecsik <kadlec@netfilter.org>

Applied, thanks