Ignore TCP SYN-ACK segments with non-matching SEG.ACK in SYN-SENT state
ClosedPublic

Authored by tuexen on Apr 18 2017, 8:34 PM.

Details

Summary

When a SYN-ACK is received in SYN-SENT state, RFC 793 requires the validation of SEG.ACK as the first step.
If the ACK is not acceptable, the segment a RST segment is sent and the segment is dropped. Currently, the segment was partially processed.

This patch moves the check for the SEG.ACK validation up to the front as required.

Test Plan

Get the tests from tcp-testsuite 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.
tuexen created this revision.Apr 18 2017, 8:34 PM
Herald added a subscriber: imp. · View Herald Transcript
hiren edited edge metadata.Apr 18 2017, 8:54 PM

For my understanding, what is the side-effect of this 'partial processing' before we eventually drop it? (I assume we do drop it but later in the path, right?)
Not against the patch but trying to understand what is driving this fix. :-)

I observed the bug by using a SYN-ACK without any options. The retransmitted SYN does not contain the SACK option anymore. So it wasn't retransmitted.

I observed the bug by using a SYN-ACK without any options. The retransmitted SYN does not contain the SACK option anymore. So it wasn't retransmitted.

Can you please explain the scenario in a little more detail?

The SUT sends a default SYN segment (including MSS option, SACK allowed, Timestamp and Window scaling). The tester responds with a SYN-ACK
without any options. In the tests I'm varying the SEG.ACK.

RFC 793 reads

If the state is SYN-SENT then

  first check the ACK bit

    If the ACK bit is set

      If SEG.ACK =< ISS, or SEG.ACK > SND.NXT, send a reset (unless
      the RST bit is set, if so drop the segment and return)

        <SEQ=SEG.ACK><CTL=RST>

      and discard the segment.  Return.

      If SND.UNA =< SEG.ACK =< SND.NXT then the ACK is acceptable.

When using SEG.ACK = SND.NXT+1 (see rcv-syn-ack-without-data-syn-sent-ack-outside-right-ipv4.pkt, TCP processes the incoming segment, disables the SACK extension and then drops it. Therefore, the retransmission of the SYN segment does not contain the SACK allowed option anymore. So the following is done before dropping the packet:

  • tp->t_rcvtime is updated
  • ECN handling performed (with updating CC)
  • TCP option processing
  • tp->rcv_wnd is updated

I think none of it should be done.

hiren accepted this revision as: hiren.Apr 20 2017, 8:26 PM

Thanks for the explanation. Change looks correct to me just that the code is a bit muddied with this single check moving elsewhere (for the right reasons) and a bit more code duplication in alt stack code. But I think those are out of the scope of this patch.

gnn accepted this revision.Apr 22 2017, 2:47 PM
This revision is now accepted and ready to land.Apr 22 2017, 2:47 PM
This revision was automatically updated to reflect the committed changes.