Use the expected sequence number for challenge ACK messages

[mj-servus]

That avoids avoid endless ACK/RST retransmissions.

Description

See #1235

Test Steps

Checklist:

• I have tested my changes. No regression in existing tests.
• I have modified and/or added unit-tests to cover the code changes in this Pull Request.

Related Issue

#1235

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[AhmedIsmail02]

@mj-servus

Thanks for your contribution.

I'm trying to understand the changes, from what I've seen in the wire shark capture on issue!1235 and AFAIK, the target device should have dropped the connection after the second RST packet was received as this packet contains a sequence number equals to the challenge ACK packet acknowledgement number. However the target didn't reset the connection, that's why i believe @tony-josi-aws has suggested here that the second RST packet should be handled separately as in we need to compare the second RST packet's sequence number with the challenge ACK packets's acknowledgement number. However, I can't see this being part of the changes, am I missing something?

[tony-josi-aws]

@mj-servus

Thanks for your contribution to FreeRTOS+TCP. I just tested your implementation locally and verified that it works by not leading to continuously sending ACK/RST.

Attaching PCAP and screen capture of the tests for anyone interested:

the second RST (packet 11) from the external device (in this case, a server running on Windows OS) has triggered a closing of the socket on the FreeRTOS+TCP device:

PCAP file: wshark.zip

A further improvement to be fully complaint with RFC 5961 [3.2. Mitigation] is to set the sequence number of the challenges ACK as per the TCP sockets SND.NXT (pxSocket->u.xTCP.xTCPWindow.ulOurSequenceNumber), similar to how acknowledge is set via pxProtocolHeaders->xTCPHeader.ulSequenceNumber = FreeRTOS_htonl( ulCurrentSequenceNumber );, as they are swapped inside the prvTCPSendSpecialPacketHelper.

In my test case, the acknowledgement number of the first RST was what was expected by the FreeRTOS+TCP device, so swapping it to the sequence number was as per RFC:

 TCP MUST
      send an acknowledgment (challenge ACK):

      <SEQ=SND.NXT><ACK=RCV.NXT><CTL=ACK>

But I believe in the case of a malicious RST this might not be the case, hence it might be more reasonable to use pxSocket->u.xTCP.xTCPWindow.ulOurSequenceNumber to be set as SEG.SEQ for the challenge ACK than swapping the first RST's ACK number as it is currently done.

So, in my opinion, it makes sense to also take pxSocket->u.xTCP.xTCPWindow.ulOurSequenceNumber as an argument to prvTCPSendChallengeAck.

Feel free to share your opinion, and if you wish to add those changes to the PR please do so or let us know.

@AhmedIsmail02

Thanks for taking a look at this PR.
With respect to my response to issue #1235 , the sequence number and the acknowledge number of the initial RST packet are swapped and are used in the challenge ACK but the problem was that the SEG.SEQ and SEG.ACK of the resultant challenge ACK were not as per RFC and hence the second RST from the external device was not meeting the conditions for a socket close.

In this PR, @mj-servus corrects that by updating the sequence number of the first RST which is used as the base packet to form the challenge ACK. Updating the sequence number causes the acknowledgement number of the challenge ACK to be updated with that sequence number because of the swapping mentioned in the previous paragraph.

[mj-servus]

Thank you very much for the suggestions. I adopted the code accordingly.

[AhmedIsmail02]

@tony-josi-aws Thanks for your detailed explanation.
Changes LGTM.