[PATCH nf-next] netfilter: conntrack: tcp: only close if RST matches exact sequence

[PATCH nf-next] netfilter: conntrack: tcp: only close if RST matches exact sequence

[Florian Westphal]

TCP resets cause instant transition from established to closed state
provided the reset is in-window.  Endpoints that implement RFC 5961
require resets to match the next expected sequence number.
RST segments that are in-window (but that do not match RCV.NXT) are
ignored, and a "challenge ACK" is sent back.

Main problem for conntrack is that its a middlebox, i.e.  whereas an end
host might have ACK'd SEQ (and would thus accept an RST with this
sequence number), conntrack might not have seen this ACK (yet).

Therefore we can't simply flag RSTs with non-exact match as invalid.

This updates RST processing as follows:

1. If the connection is in a state other than ESTABLISHED, nothing is
   changed, RST is subject to normal in-window check.

2. If the RSTs sequence number either matches exactly RCV.NXT,
   connection state moves to CLOSE.

3. The same applies if the RST sequence number aligns with a previous
   packet in the same direction.

In all other cases, the connection remains in ESTABLISHED state.
If the normal-in-window check passes, the timeout will be lowered
to that of CLOSE.

If the peer sends a challenge ack, connection timeout will be reset.

If the challenge ACK triggers another RST (RST was valid after all),
this 2nd RST will match expected sequence and conntrack state changes to
CLOSE.

If no challenge ACK is received, the connection will time out after
CLOSE seconds (10 seconds by default), just like without this patch.

Packetdrill test case:

0.000 socket(..., SOCK_STREAM, IPPROTO_TCP) = 3
0.000 setsockopt(3, SOL_SOCKET, SO_REUSEADDR, [1], 4) = 0
0.000 bind(3, ..., ...) = 0
0.000 listen(3, 1) = 0

0.100 < S 0:0(0) win 32792 <mss 1460,sackOK,nop,nop,nop,wscale 7>
0.100 > S. 0:0(0) ack 1 win 64240 <mss 1460,nop,nop,sackOK,nop,wscale 7>
0.200 < . 1:1(0) ack 1 win 257
0.200 accept(3, ..., ...) = 4

// Receive a segment.
0.210 < P. 1:1001(1000) ack 1 win 46
0.210 > . 1:1(0) ack 1001

// Application writes 1000 bytes.
0.250 write(4, ..., 1000) = 1000
0.250 > P. 1:1001(1000) ack 1001

// First reset, old sequence. Conntrack (correctly) considers this
// invalid due to failed window validation (regardless of this patch).
0.260 < R  2:2(0) ack 1001 win 260

// 2nd reset, but too far ahead sequence.  Same: correctly handled
// as invalid.
0.270 < R 99990001:99990001(0) ack 1001 win 260

// in-window, but not exact sequence.
// Current Linux kernels might reply with a challenge ack, and do not
// remove connection.
// Without this patch, conntrack state moves to CLOSE.
// With patch, timeout is lowered like CLOSE, but connection stays
// in ESTABLISHED state.
0.280 < R 1010:1010(0) ack 1001 win 260

// Expect challenge ACK
0.281 > . 1001:1001(0) ack 1001 win 501

// With or without this patch, RST will cause connection
// to move to CLOSE (sequence number matches)
// 0.282 < R 1001:1001(0) ack 1001 win 260

// ACK
0.300 < . 1001:1001(0) ack 1001 win 257

// more data could be exchanged here, connection
// is still established

// Client closes the connection.
0.610 < F. 1001:1001(0) ack 1001 win 260
0.650 > . 1001:1001(0) ack 1002

// Close the connection without reading outstanding data
0.700 close(4) = 0

// so one more reset.  Will be deemed acceptable with patch as well:
// connection is already closing.
0.701 > R. 1001:1001(0) ack 1002 win 501
// End packetdrill test case.

With patch, this generates following conntrack events:
   [NEW] 120 SYN_SENT src=10.0.2.1 dst=10.0.0.1 sport=5437 dport=80 [UNREPLIED]
[UPDATE] 60 SYN_RECV src=10.0.2.1 dst=10.0.0.1 sport=5437 dport=80
[UPDATE] 432000 ESTABLISHED src=10.0.2.1 dst=10.0.0.1 sport=5437 dport=80 [ASSURED]
[UPDATE] 120 FIN_WAIT src=10.0.2.1 dst=10.0.0.1 sport=5437 dport=80 [ASSURED]
[UPDATE] 60 CLOSE_WAIT src=10.0.2.1 dst=10.0.0.1 sport=5437 dport=80 [ASSURED]
[UPDATE] 10 CLOSE src=10.0.2.1 dst=10.0.0.1 sport=5437 dport=80 [ASSURED]

Without patch, first RST moves connection to close, whereas socket state
does not change until FIN is received.
   [NEW] 120 SYN_SENT src=10.0.2.1 dst=10.0.0.1 sport=5141 dport=80 [UNREPLIED]
[UPDATE] 60 SYN_RECV src=10.0.2.1 dst=10.0.0.1 sport=5141 dport=80
[UPDATE] 432000 ESTABLISHED src=10.0.2.1 dst=10.0.0.1 sport=5141 dport=80 [ASSURED]
[UPDATE] 10 CLOSE src=10.0.2.1 dst=10.0.0.1 sport=5141 dport=80 [ASSURED]

Cc: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 net/netfilter/nf_conntrack_proto_tcp.c | 50 ++++++++++++++++++++------
 1 file changed, 40 insertions(+), 10 deletions(-)

diff --git a/net/netfilter/nf_conntrack_proto_tcp.c b/net/netfilter/nf_conntrack_proto_tcp.c
index 01c748fa8913..a06875a466a4 100644
--- a/net/netfilter/nf_conntrack_proto_tcp.c
+++ b/net/netfilter/nf_conntrack_proto_tcp.c
@@ -828,6 +828,12 @@ static noinline bool tcp_new(struct nf_conn *ct, const struct sk_buff *skb,
 	return true;
 }
 
+static bool nf_conntrack_tcp_established(const struct nf_conn *ct)
+{
+	return ct->proto.tcp.state == TCP_CONNTRACK_ESTABLISHED &&
+	       test_bit(IPS_ASSURED_BIT, &ct->status);
+}
+
 /* Returns verdict for packet, or -1 for invalid. */
 int nf_conntrack_tcp_packet(struct nf_conn *ct,
 			    struct sk_buff *skb,
@@ -1030,16 +1036,38 @@ int nf_conntrack_tcp_packet(struct nf_conn *ct,
 			new_state = TCP_CONNTRACK_ESTABLISHED;
 		break;
 	case TCP_CONNTRACK_CLOSE:
-		if (index == TCP_RST_SET
-		    && (ct->proto.tcp.seen[!dir].flags & IP_CT_TCP_FLAG_MAXACK_SET)
-		    && before(ntohl(th->seq), ct->proto.tcp.seen[!dir].td_maxack)) {
-			/* Invalid RST  */
-			spin_unlock_bh(&ct->lock);
-			nf_ct_l4proto_log_invalid(skb, ct, "invalid rst");
-			return -NF_ACCEPT;
+		if (index != TCP_RST_SET)
+			break;
+
+		if (ct->proto.tcp.seen[!dir].flags & IP_CT_TCP_FLAG_MAXACK_SET) {
+			u32 seq = ntohl(th->seq);
+
+			if (before(seq, ct->proto.tcp.seen[!dir].td_maxack)) {
+				/* Invalid RST  */
+				spin_unlock_bh(&ct->lock);
+				nf_ct_l4proto_log_invalid(skb, ct, "invalid rst");
+				return -NF_ACCEPT;
+			}
+
+			if (!nf_conntrack_tcp_established(ct) ||
+			    seq == ct->proto.tcp.seen[!dir].td_maxack)
+				break;
+
+			/* Check if rst is part of train, such as
+			 *   foo:80 > bar:4379: P, 235946583:235946602(19) ack 42
+			 *   foo:80 > bar:4379: R, 235946602:235946602(0)  ack 42
+			 */
+			if (ct->proto.tcp.last_index == TCP_ACK_SET &&
+			    ct->proto.tcp.last_dir == dir &&
+			    seq == ct->proto.tcp.last_end)
+				break;
+
+			/* ... RST sequence number doesn't match exactly, keep
+			 * established state to allow a possible challenge ACK.
+			 */
+			new_state = old_state;
 		}
-		if (index == TCP_RST_SET
-		    && ((test_bit(IPS_SEEN_REPLY_BIT, &ct->status)
+		if (((test_bit(IPS_SEEN_REPLY_BIT, &ct->status)
 			 && ct->proto.tcp.last_index == TCP_SYN_SET)
 			|| (!test_bit(IPS_ASSURED_BIT, &ct->status)
 			    && ct->proto.tcp.last_index == TCP_ACK_SET))
@@ -1055,7 +1083,7 @@ int nf_conntrack_tcp_packet(struct nf_conn *ct,
 			 * segments we ignored. */
 			goto in_window;
 		}
-		/* Just fall through */
+		break;
 	default:
 		/* Keep compilers happy. */
 		break;
@@ -1090,6 +1118,8 @@ int nf_conntrack_tcp_packet(struct nf_conn *ct,
 	if (ct->proto.tcp.retrans >= tn->tcp_max_retrans &&
 	    timeouts[new_state] > timeouts[TCP_CONNTRACK_RETRANS])
 		timeout = timeouts[TCP_CONNTRACK_RETRANS];
+	else if (unlikely(index == TCP_RST_SET))
+		timeout = timeouts[TCP_CONNTRACK_CLOSE];
 	else if ((ct->proto.tcp.seen[0].flags | ct->proto.tcp.seen[1].flags) &
 		 IP_CT_TCP_FLAG_DATA_UNACKNOWLEDGED &&
 		 timeouts[new_state] > timeouts[TCP_CONNTRACK_UNACK])
-- 
2.19.2

Re: [PATCH nf-next] netfilter: conntrack: tcp: only close if RST matches exact sequence

[Pablo Neira Ayuso]

On Thu, Feb 21, 2019 at 05:09:31PM +0100, Florian Westphal wrote:
> TCP resets cause instant transition from established to closed state
> provided the reset is in-window.  Endpoints that implement RFC 5961
> require resets to match the next expected sequence number.
> RST segments that are in-window (but that do not match RCV.NXT) are
> ignored, and a "challenge ACK" is sent back.
> 
> Main problem for conntrack is that its a middlebox, i.e.  whereas an end
> host might have ACK'd SEQ (and would thus accept an RST with this
> sequence number), conntrack might not have seen this ACK (yet).
> 
> Therefore we can't simply flag RSTs with non-exact match as invalid.
> 
> This updates RST processing as follows:
> 
> 1. If the connection is in a state other than ESTABLISHED, nothing is
>    changed, RST is subject to normal in-window check.
> 
> 2. If the RSTs sequence number either matches exactly RCV.NXT,
>    connection state moves to CLOSE.
> 
> 3. The same applies if the RST sequence number aligns with a previous
>    packet in the same direction.
> 
> In all other cases, the connection remains in ESTABLISHED state.
> If the normal-in-window check passes, the timeout will be lowered
> to that of CLOSE.
> 
> If the peer sends a challenge ack, connection timeout will be reset.
> 
> If the challenge ACK triggers another RST (RST was valid after all),
> this 2nd RST will match expected sequence and conntrack state changes to
> CLOSE.
> 
> If no challenge ACK is received, the connection will time out after
> CLOSE seconds (10 seconds by default), just like without this patch.

Applied, thanks.

@Jozsef, if you could also have a look to confirm if you see any
issue, this looks fine to me and we, of course, can revert this in
this this tightening in RST tracking has any side issue. Thanks!

Re: [PATCH nf-next] netfilter: conntrack: tcp: only close if RST matches exact sequence

[Jozsef Kadlecsik]

Hi Pablo, Florian,

On Fri, 1 Mar 2019, Pablo Neira Ayuso wrote:

> On Thu, Feb 21, 2019 at 05:09:31PM +0100, Florian Westphal wrote:
> > TCP resets cause instant transition from established to closed state
> > provided the reset is in-window.  Endpoints that implement RFC 5961
> > require resets to match the next expected sequence number.
> > RST segments that are in-window (but that do not match RCV.NXT) are
> > ignored, and a "challenge ACK" is sent back.
> > 
> > Main problem for conntrack is that its a middlebox, i.e.  whereas an end
> > host might have ACK'd SEQ (and would thus accept an RST with this
> > sequence number), conntrack might not have seen this ACK (yet).
> > 
> > Therefore we can't simply flag RSTs with non-exact match as invalid.
> > 
> > This updates RST processing as follows:
> > 
> > 1. If the connection is in a state other than ESTABLISHED, nothing is
> >    changed, RST is subject to normal in-window check.
> > 
> > 2. If the RSTs sequence number either matches exactly RCV.NXT,
> >    connection state moves to CLOSE.
> > 
> > 3. The same applies if the RST sequence number aligns with a previous
> >    packet in the same direction.
> > 
> > In all other cases, the connection remains in ESTABLISHED state.
> > If the normal-in-window check passes, the timeout will be lowered
> > to that of CLOSE.
> > 
> > If the peer sends a challenge ack, connection timeout will be reset.
> > 
> > If the challenge ACK triggers another RST (RST was valid after all),
> > this 2nd RST will match expected sequence and conntrack state changes to
> > CLOSE.
> > 
> > If no challenge ACK is received, the connection will time out after
> > CLOSE seconds (10 seconds by default), just like without this patch.
> 
> Applied, thanks.
> 
> @Jozsef, if you could also have a look to confirm if you see any
> issue, this looks fine to me and we, of course, can revert this in
> this this tightening in RST tracking has any side issue. Thanks!

The only problem I see is this part:

> > If the peer sends a challenge ack, connection timeout will be reset.

RFC5961 does not discuss the issue of the timeout when the challenge ack 
is sent and get lost, never reaches the destination. Like in this 
case:

non RFC5961 client 	firewall	our firewall	RFC5961 server
in-window RST ->
			ct dropped	ct kept,
					CLOSED timeout
							<- challenge ack
					ct kept,
					ESTABLISHED timeout
			ack dropped

I think we should keep the CLOSED timeout when the challenge ack is 
detected, which probably needs a new flag (IP_CT_EXP_CHALLENGE_ACK is for 
SYN ACK challenges).

In my opinion it was a very bad design decision of RFC5961 that there's no 
TCP option/flag to signal to the peer that the sender speaks the RFC5961 
flavour.

Best regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.mta.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics, Hungarian Academy of Sciences
          H-1525 Budapest 114, POB. 49, Hungary

Re: [PATCH nf-next] netfilter: conntrack: tcp: only close if RST matches exact sequence

[Florian Westphal]

Jozsef Kadlecsik <kadlec@blackhole.kfki.hu> wrote:
> > @Jozsef, if you could also have a look to confirm if you see any
> > issue, this looks fine to me and we, of course, can revert this in
> > this this tightening in RST tracking has any side issue. Thanks!
> 
> The only problem I see is this part:
> 
> > > If the peer sends a challenge ack, connection timeout will be reset.
> 
> RFC5961 does not discuss the issue of the timeout when the challenge ack 
> is sent and get lost, never reaches the destination. Like in this 
> case:
> 
> non RFC5961 client 	firewall	our firewall	RFC5961 server
> in-window RST ->
> 			ct dropped	ct kept,
> 					CLOSED timeout
> 							<- challenge ack
> 					ct kept,
> 					ESTABLISHED timeout
> 			ack dropped


The ESTABLISHED timeout is 5 minutes because of missing ACKs
(outstanding data) in this case though (the RST has "wrong" sequence
number, so the conntrack is flagged accordingly until something
acks the data).

> I think we should keep the CLOSED timeout when the challenge ack is 
> detected, which probably needs a new flag (IP_CT_EXP_CHALLENGE_ACK is for 
> SYN ACK challenges).

I think its fine as is with the unacknowledged data timeout, but it
should be easy to keep the 10 seconds using the flag you suggest).

I think we can even reuse the EXP_CHALLENGE_ACK for this.

> In my opinion it was a very bad design decision of RFC5961 that there's no 
> TCP option/flag to signal to the peer that the sender speaks the RFC5961 
> flavour.

I think its fine, we would have the same problem if the RST was lost
before reaching us.

Re: [PATCH nf-next] netfilter: conntrack: tcp: only close if RST matches exact sequence

[Jozsef Kadlecsik]

On Sun, 3 Mar 2019, Florian Westphal wrote:

> Jozsef Kadlecsik <kadlec@blackhole.kfki.hu> wrote:
> > > @Jozsef, if you could also have a look to confirm if you see any
> > > issue, this looks fine to me and we, of course, can revert this in
> > > this this tightening in RST tracking has any side issue. Thanks!
> > 
> > The only problem I see is this part:
> > 
> > > > If the peer sends a challenge ack, connection timeout will be reset.
> > 
> > RFC5961 does not discuss the issue of the timeout when the challenge ack 
> > is sent and get lost, never reaches the destination. Like in this 
> > case:
> > 
> > non RFC5961 client 	firewall	our firewall	RFC5961 server
> > in-window RST ->
> > 			ct dropped	ct kept,
> > 					CLOSED timeout
> > 							<- challenge ack
> > 					ct kept,
> > 					ESTABLISHED timeout
> > 			ack dropped
> 
> 
> The ESTABLISHED timeout is 5 minutes because of missing ACKs 
> (outstanding data) in this case though (the RST has "wrong" sequence 
> number, so the conntrack is flagged accordingly until something acks the 
> data).

Sorry, but I don't see where is the outstanding data. Up to the RST and 
the challenge ACK there's no missing ACK. The RST hasn't got wrong seq 
because it's in the window. The RST flag is not counted. What do I miss?
 
> > I think we should keep the CLOSED timeout when the challenge ack is 
> > detected, which probably needs a new flag (IP_CT_EXP_CHALLENGE_ACK is for 
> > SYN ACK challenges).
> 
> I think its fine as is with the unacknowledged data timeout, but it
> should be easy to keep the 10 seconds using the flag you suggest).

If the conntrack timeout is not the ESTABLISHED one after the challenge 
ACK is seen, that's perfect.

Best regard,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.mta.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics, Hungarian Academy of Sciences
          H-1525 Budapest 114, POB. 49, Hungary

Re: [PATCH nf-next] netfilter: conntrack: tcp: only close if RST matches exact sequence

[Florian Westphal]

Jozsef Kadlecsik <kadlec@blackhole.kfki.hu> wrote:
> > The ESTABLISHED timeout is 5 minutes because of missing ACKs 
> > (outstanding data) in this case though (the RST has "wrong" sequence 
> > number, so the conntrack is flagged accordingly until something acks the 
> > data).
> 
> Sorry, but I don't see where is the outstanding data. Up to the RST and 
> the challenge ACK there's no missing ACK. The RST hasn't got wrong seq 
> because it's in the window. The RST flag is not counted. What do I miss?

There is no outstanding data, but conntrack thinks there is.

If we have this:
seq 100 ack x len 100
seq 200 rst x len 0

Then rst is accepted immediately and we move to CLOSE.

When we get this:
seq 100 ack x len 100
seq 210 rst x len 0

Then from conntrack point of view, we're 10 bytes "short" because td_end
is 200, and new sequence is 210.  So we hit this:
  if (after(end, sender->td_end)) {
    sender->td_end = end;
    sender->flags |= IP_CT_TCP_FLAG_DATA_UNACKNOWLEDGED;
							                     }
in tcp_in_window().

> If the conntrack timeout is not the ESTABLISHED one after the challenge 
> ACK is seen, that's perfect.

The challenge ACK will use last in-sequence number as seen by the
receiver, so 200 in this case.

If it would be 210, then that would mean that conntrack, for some
reason, did not see those 10 bytes, but the real endpoint did.

And in that case we'd move back to full ESTABLISHED timeout.

Re: [PATCH nf-next] netfilter: conntrack: tcp: only close if RST matches exact sequence

[Jozsef Kadlecsik]

On Sun, 3 Mar 2019, Florian Westphal wrote:

> Jozsef Kadlecsik <kadlec@blackhole.kfki.hu> wrote:
> > > The ESTABLISHED timeout is 5 minutes because of missing ACKs 
> > > (outstanding data) in this case though (the RST has "wrong" sequence 
> > > number, so the conntrack is flagged accordingly until something acks the 
> > > data).
> > 
> > Sorry, but I don't see where is the outstanding data. Up to the RST and 
> > the challenge ACK there's no missing ACK. The RST hasn't got wrong seq 
> > because it's in the window. The RST flag is not counted. What do I miss?
> 
> There is no outstanding data, but conntrack thinks there is.
> 
> If we have this:
> seq 100 ack x len 100
> seq 200 rst x len 0
> 
> Then rst is accepted immediately and we move to CLOSE.
> 
> When we get this:
> seq 100 ack x len 100
> seq 210 rst x len 0
> 
> Then from conntrack point of view, we're 10 bytes "short" because td_end
> is 200, and new sequence is 210.  So we hit this:
>   if (after(end, sender->td_end)) {
>     sender->td_end = end;
>     sender->flags |= IP_CT_TCP_FLAG_DATA_UNACKNOWLEDGED;
> 							                     }
> in tcp_in_window().

I see, thanks indeed! That explains perfectly.

I did not think about the packet before the RST one. :-)

Best regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.mta.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics, Hungarian Academy of Sciences
          H-1525 Budapest 114, POB. 49, Hungary