TCP Stack: Challenge ACKs and Timestamps

[Jonathan T. Looney]

Hi Sam,

Thanks for bringing this to our attention! Comments in-line.

On 1/5/16, 2:57 AM, "hiren panchasara"
<owner-freebsd-transport at freebsd.org on behalf of
hiren at strugglingcoder.info> wrote:

>bcc: current@
>Moving the discussion to transport at .
>
>On 12/28/15 at 12:25P, Sam Kumar wrote:
>> Hello,
>> I am working with the code for the TCP Stack. I noticed that support for
>> Challenge Acks have been added since the latest release (10.2.0), and I
>> think I may have found a bug in how they relate to TCP timestamps. All
>>line
>> numbers below are those in commit
>>e66e064c45687b5d294565dbd829b419848f7992.
>> 
>> Looking at tcp_input.c, at lines 1594 to 1604, I see code that expects a
>> timestamp to be in every segment during the session, if they were
>> negotiated when the connection was being established.
>> (
>> 
>>https://github.com/freebsd/freebsd/blob/master/sys/netinet/tcp_input.c#L1
>>595
>> )

The code *expects* this, but doesn't *enforce* it. If a timestamp is
missing, it logs a debug-level message, but still accepts the packet. I
think this is reasonable behavior based on the text of RFC 1323, which (I
believe) is what was current when this code was written. This is still
allowed under RFC 7323.


>> 
>> Looking at tcp_input.c, at lines 2161 and 2188, I see that Challenge
>>ACKs
>> are sent via calls to tcp_respond().
>> (
>> 
>>https://github.com/freebsd/freebsd/blob/master/sys/netinet/tcp_input.c#L2
>>161
>> and
>> 
>>https://github.com/freebsd/freebsd/blob/master/sys/netinet/tcp_input.c#L2
>>188
>> )
>> 
>> Looking at tcp_subr.c, at line 978, I see that the segment sent by
>> tcp_respond() never contains TCP options.
>> 
>>(https://github.com/freebsd/freebsd/blob/master/sys/netinet/tcp_subr.c#L9
>>78)
>> 
>> Therefore, it seems to me that Challenge ACKs will never contain any TCP
>> options. This violates the condition that once timestamps are
>>negotiated,
>> they must be present in every segment.

I believe your reading of the code is correct. And, I believe RFC 1323
would have allowed this. However, RFC 7323 (just released in September
2014) changes this:

   Once TSopt has been successfully negotiated, that is both <SYN> and
   <SYN,ACK> contain TSopt, the TSopt MUST be sent in every non-<RST>
   segment for the duration of the connection, and SHOULD be sent in an
   <RST> segment (see Section 5.2 for details).  The TCP SHOULD remember
   this state by setting a flag, referred to as Snd.TS.OK, to one.  If a
   non-<RST> segment is received without a TSopt, a TCP SHOULD silently
   drop the segment.  A TCP MUST NOT abort a TCP connection because any
   segment lacks an expected TSopt.


Based on this new requirement, it appears that our implementation does not
comply.

We have two options:
A) Switch from using tcp_respond() to using tcp_output() with TF_ACKNOW.
B) Fix tcp_respond() so it will add appropriate options. (In addition to
the timestamp option, it might be important to add the signature option.)

I think option B is probably easy enough to implement without adding all
the overhead of tcp_output(). Unless someone objects, I can work on
implementing this approach.

Please feel free to open a PR to track this problem.

Jonathan

>> 
>> Please let me know if I am mistaken, or if this is actually a bug.
>> 
>> Sam Kumar
>> _______________________________________________
>> freebsd-current at freebsd.org mailing list
>> https://lists.freebsd.org/mailman/listinfo/freebsd-current
>> To unsubscribe, send any mail to
>>"freebsd-current-unsubscribe at freebsd.org"