Page MenuHomeFreeBSD

Fix handling of RST segments in SYN-RCVD state via the syn cache code path
ClosedPublic

Authored by tuexen on Oct 17 2018, 8:59 AM.

Details

Summary

The handling of RST segments in the SYN-RCVD state exists in the code paths. Both are not consistent and the one on the syn cache code does not conform to the relevant specifications (Page 69 of RFC 793 and Section 4.2 of RFC 5961).

This patch fixes this:

  • The sequence numbers checks are fixed as specified in page Page 69 RFC 793.
  • The sysctl variable net.inet.tcp.insecure_rst is now honored as specified in Section 4.2 of RFC 5961.
Test Plan

Get all tests from packetdrill tests passed.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

tuexen created this revision.Oct 17 2018, 8:59 AM
bz added a subscriber: bz.Oct 17 2018, 4:34 PM

Scrolling through it looks OK with cross-checking to the RFCs.
Mostly commenting on the fact that we should get this in before BETA as it does change the kernel KPI.
Hope someone will do the full technical review (want to add lstewart as well?)

In D17595#375585, @bz wrote:

Scrolling through it looks OK with cross-checking to the RFCs.
Mostly commenting on the fact that we should get this in before BETA as it does change the kernel KPI.

That is my hope. I could not change the signature of syncache_chkrst(), but it would be better to
include the change. I guess if I can convince people to review it tomorrow, I could try to get it in on Friday.

Hope someone will do the full technical review (want to add lstewart as well?)

Added Lawrence.

bz added a comment.Oct 18 2018, 3:54 PM

It would greatly help if the proposed commit message would also cite the RFC sections not just the RFCs; ideally actually comments in the code would do that as well at the right place so that future reference lookups will be easier.

tuexen edited the summary of this revision. (Show Details)Oct 18 2018, 4:11 PM
tuexen edited the summary of this revision. (Show Details)
bz accepted this revision as: bz.Oct 18 2018, 5:35 PM

For as much as my brain still functions this seems ok

Question to be ignored: does it also apply to non-default TCP stacks (which I have never looked at)?

rrs accepted this revision.Oct 18 2018, 6:04 PM
This revision is now accepted and ready to land.Oct 18 2018, 6:04 PM

syncache_chkrst() isn't a public function, so no worries on KBI.

glebius accepted this revision.Oct 18 2018, 6:18 PM
This revision was automatically updated to reflect the committed changes.
lstewart added inline comments.Oct 18 2018, 8:45 PM
sys/netinet/tcp_syncache.c
583 ↗(On Diff #49248)

Please reference Pg69 here and perhaps reproduce the table as well. A note well with respect to syncache_lookup() setting sc_irs to th_seq-1 might also be helpful as it took me a bit to reason about the +1 and SEQ_LEQ -> SEQ_LT changes.

1733 ↗(On Diff #49248)

I think this may not be strictly correct in the TFO case?

@tuexen Any thoughts on the TFO case?

@tuexen Any thoughts on the TFO case?

@lstewart Could you be more specific what you think is not handled correctly?

In D17595#375876, @bz wrote:

For as much as my brain still functions this seems ok
Question to be ignored: does it also apply to non-default TCP stacks (which I have never looked at)?

This fix also applies to non-default TCP stacks. The code fixed here is called in tcp_input.c:1168. The handling if RST-segments for sockets in the LISTEN state is handled in the generic code.

@lstewart Could you be more specific what you think is not handled correctly?

@lstewart Found the line you were referring to. Let me check...

tuexen marked an inline comment as done.Nov 1 2018, 11:36 AM
tuexen added inline comments.
sys/netinet/tcp_syncache.c
583 ↗(On Diff #49248)

Fixed in r339989.

tuexen marked an inline comment as done.Nov 1 2018, 9:01 PM
tuexen added inline comments.
sys/netinet/tcp_syncache.c
1733 ↗(On Diff #49248)

syncache_respond() is used for three different purposes:

  1. Transmit a SYN-ACK segment in response to a received SYN segment.
  2. Retransmit a SYN-ACK segment after a timeout.
  3. Send an ACK segment as a challenge ack in response to a received RST segment.

If the received SYN segment contains a TCP-FO cookie request, the SYN-ACK is transmitted and retransmitted correctly. If a RST segment is received and triggers the sending of a challenge ack, this is transmitted correctly.

If the received SYN segment contains an acceptable TCP-FO cookie, the SYN-ACK is transmitted and retransmitted via a different code path, since you don't process this using syn-cache entries. The same applies to the handling of received RST segments.