tcp: Handle <RST,ACK> in SYN-RCVD per RFC9293

[rscheff]

Summary

RFC793 (original TCP specification) declared that during the
SYN-RCVD state, only pure <RST> segments are to be processed.

However, RFC5961 and the current TCP specification RFC9293
improved this by stating that also <RST,ACK> should first
be checked for acceptability, and if acceptable, may either
trigger a challenge ACK, reset the connection, or get
silently ignored.

This was found with a hyperscaler loadbalancer, which would
validate the reachability of TCP services by frequently performing
a TCP 3WHS, immediately followed by a <RST,ACK>. When the <RST,ACK>
overtakes the final <ACK> of the 3WHS, that RST got ignoried, leading
to exhaustion of TCP tables, as the processing was different between
SYN-RCVD and ESTABLISHED state.

MFC to 13 and 12 after 1 week...

Test Plan

Verify that

<SYN
>SYN,ACK
<RST,ACK

with the exact Sequence number resets the session,

<SYN
>SYN,ACK
<RST,ACK

with a lower (not acceptable) sequence number is silently ignored, and

<SYN
>SYN,ACK
<RST,ACK

with a sequence number+1 elicits a challenge ACK

[tuexen]

Give me a day or two to add corresponding packetdrill test to the TCP testsuite...

[rscheff]

No problem; I copied the ipv4 tests from rcv-rst-syn-rcvd and added the ACK bit into them; they don't seem to check if RST,FIN, RST,SYN are silently ignored though; also, in the simultaneous open case the ECONNREFUSED is apparently not reached. So something in tcp_input is still amiss.

[cc]

Would you please elaborate the corresponding specification in RFC5961 and RFC9293?

[rscheff]

it's in the changed comment (RFC9293, Section 3.10.7.4)

In short, RFC793 declared that only pure RSTs (no RST/ACK) would be acceptable in SYN-RCVD; since RFC5961 / RFC9293, the acceptance test (Seq / Ack of the RST or RST/ACK) has to be checked for acceptability, and if the RST is exactly at the left edge of the window, the session should be closed; if it's in-window, a challange ACK is to be sent, and in all other cases the RST is silently ignored.

This patch does NOT yet address the simultaneous SYN case - where the session seems to linger and no error is returned to the socket, when the session should get closed...

[rscheff]

• fix SYN-RCVD path for RST

[cc]

Also please add an additional test for out of window seq number test:
your existing "with a lower (not acceptable) sequence number is silently ignored", and + "with a higher than window (not acceptable) sequence number is silently ignored"

sys/netinet/tcp_input.c

2178–2180

Is this a DUP check? From the comment, it looks to be the same handling for simultaneous opening.

This makes me wonder what happens in your test plan before this change? Or can you also add simultaneous opening test if not?

sys/netinet/tcp_syncache.c

667–673

For non-simultaneous case (pure passive open), the comment should match RFC9293, Section 3.10.7.4.

[rscheff]

• remove RST special case handling for SYN-RCVD

[rscheff]

In D40982#934677, @cc wrote:

Also please add an additional test for out of window seq number test:
your existing "with a lower (not acceptable) sequence number is silently ignored", and + "with a higher than window (not acceptable) sequence number is silently ignored"

Don't quite understand. The logic is doing exactly that, with the SEQ_LT and SEQ_GT checks; if the RST (with or without ACK) is ourside the acceptable window, the incoming segment is droped; if the RST matches exactly the next expected sequence number, the connection is reset and socket error code updated, and in the remaining cases, a challenge ACK is sent...

sys/netinet/tcp_input.c

2178–2180

The problem here (and I was initially confused too) is, that originally,this code never executes with RST/ACK - as there is the special handling of RST in SYN-RCVD state, and an RST/ACK was effectively illegal (silently ignored), or with RACK, the ACK is processed before the RST (@rrs is looking into the ordering in the RACK stack).

In the non simultaneous SYN, the base stack uses the syncache (and that RST or RST/ACK handling is done there); because of the special handling of SYN-RCVD and SYN-SENT states above, the RST handling here is really only during later states.

This particular switch case could be removed, however - since all the special RST handling for the two SYN states is done further up.

sys/netinet/tcp_syncache.c

667–673

If the code is updated to conform fully to RFC9293 certainly. Since this is currently a point update for the SYN-RCVD state for RST/ACK (in addition to pure RST), I don't think this comment here needs updating yet.

[cc]

Also please add an additional test for out of window seq number test:
your existing "with a lower (not acceptable) sequence number is silently ignored", and + "with a higher than window (not acceptable) sequence number is silently ignored"

I meant the unit test shall cover both cases of the "out of window seq number test": lower and higher.

[tuexen]

In D40982#934698, @cc wrote:

Also please add an additional test for out of window seq number test:
your existing "with a lower (not acceptable) sequence number is silently ignored", and + "with a higher than window (not acceptable) sequence number is silently ignored"

I meant the unit test shall cover both cases of the "out of window seq number test": lower and higher.

That is on my plate...

[rscheff]

• update patch to conform and use insecure_rst as expected

[rscheff]

Thanks @tuexen for preparing the full suite of tests per RFC9293:

https://github.com/freebsd-net/tcp-testsuite/tree/master/state-event-engine/rcv-rst-ack-syn-rcvd

and giving very helpful insights in improving this patch!