buglwIP - A Lightweight TCP/IP stack - Bugs: bug #48328, Fatal RST/ACK loop caused by...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

bug #48328: Fatal RST/ACK loop caused by commit "fixed bug #48170"

Submitter:  Ambroz Bizjak <abizjak>
Submitted:  Tue 28 Jun 2016 10:25:59 AM UTC
   
 
Category:  TCP Severity:  3 - Normal
Item Group:  Faulty Behaviour Status:  Fixed
Privacy:  Public Assigned to:  goldsimon
Open/Closed:  Closed Planned Release:  None
lwIP version:  git head

Jump to the original submission

Sat 20 Aug 2016 11:55:00 AM UTC, comment #8: 

Hey,
Just a remark. I'm reading the TCP RFC (793) and I think the specs under Event Processing actually contradicts the more informal description on reset handling on page 37, which lwIP supposedly violates.

On page 69, acceptability of a received segment is defined, which does indeed classify out-of-sequence RSTs as acceptable. However, there is also this text:

"In the following it is assumed that the segment is the idealized segment that begins at RCV.NXT and does not exceed the window. ..."

And since the RST checking (page 70, 71) is specified after this, out-of-sequence (but in-window) RSTs should be dropped or buffered for later processing, rather than abort the connection immediately. On the other only non-acceptable (according to the exact definition on page 69) RSTs should be ACKed.

lwIP could be changed to be more compliant in that respect.

Ambroz Bizjak <abizjak>
Thu 30 Jun 2016 06:51:39 AM UTC, comment #7: 

I've pushed a better fix. Thanks for reporting.

Simon Goldschmidt <goldsimon>
Group administrator
Thu 30 Jun 2016 06:27:00 AM UTC, comment #6: 

Right, seems like I messed that up. However, I don't agree to the patch as it accepts "seqno == pcb->rcv_nxt" in stat SYN_SENT, too, which it didn't before.

Simon Goldschmidt <goldsimon>
Group administrator
Tue 28 Jun 2016 11:11:30 AM UTC, comment #5: 

I confirm the change in comment #3 (and comment #2 ignoring the second change that is irrelevant) fixes the issue for me.

Ambroz Bizjak <abizjak>
Tue 28 Jun 2016 11:07:50 AM UTC, comment #4: 

I don't agree with applying this patch, we should revert the wrong fix and apply the real fix in an atomic patch with both diffset combined.

Sylvain Rochet <gradator>
Group Member
Tue 28 Jun 2016 11:04:32 AM UTC, comment #3: 

Attached the patch.

(file #37611)

Axel Lin <axellin>
Group Member
Tue 28 Jun 2016 10:54:57 AM UTC, comment #2: 

That should do it, I referenced this commit in this mail which talk about this commit:

  http://lists.nongnu.org/archive/html/lwip-devel/2016-06/msg00053.html

Sylvain Rochet <gradator>
Group Member
Tue 28 Jun 2016 10:45:09 AM UTC, comment #1: 

Does below change work for you?
I found it on https://github.com/tabascoeye/lwip/commit/a59a9c102c924cca420dfbd5063fbcfb8e560bac

diff --git a/src/core/tcp_in.c b/src/core/tcp_in.c
index b26f8e6..1822206 100644
--- a/src/core/tcp_in.c
+++ b/src/core/tcp_in.c
@@ -703,8 +703,8 @@ tcp_process(struct tcp_pcb *pcb)
 
   /* Process incoming RST segments. */
   if (flags & TCP_RST) {
-    /* First, determine if the reset is acceptable. (in case of RST only if the sequence number matches) */
-    if (ackno == pcb->snd_nxt) {
+    /* First, determine if the reset is acceptable. (in case of RST only if the sequence number matches, special case for SYN_SENT state) */
+    if (((pcb->state == SYN_SENT) && (ackno == pcb->snd_nxt)) || (seqno == pcb->rcv_nxt)) {
       LWIP_DEBUGF(TCP_INPUT_DEBUG, ("tcp_process: Connection RESET\n"));
       LWIP_ASSERT("tcp_input: pcb->state != CLOSED", pcb->state != CLOSED);
       recv_flags |= TF_RESET;


Axel

Axel Lin <axellin>
Group Member
Tue 28 Jun 2016 10:25:59 AM UTC, original submission:  

The following commit causes a fatal DOS condition in my setup when a client closes the connection:
http://git.savannah.gnu.org/cgit/lwip.git/commit/?id=236bc194222b604f251cd86d4715cd05d9e40f94

I can trigger the issue by having a client connect to the device (TCP server) and the device send unlimited amounts of data, then close the client application shortly. The result is that the client machine and the device get caught in an infinite loop, the device sending ACK and the client machine always responding with RST.

I am absolutely positive this commit is the cause. However I am using a significantly modified lwIP version.

Attached is a Wireshark capture of the event, and below is a text summary of the packets. The device is 192.168.111.142.

      6 0.000663843    192.168.111.140       192.168.111.142       TCP      54     50210 → 80 [ACK] Seq=122206028 Ack=11397555 Win=64240 Len=0
      7 0.000853303    192.168.111.142       192.168.111.140       TCP      254    80 → 50210 [PSH, ACK] Seq=11397555 Ack=122206028 Win=3341 Len=200
      8 0.000872639    192.168.111.140       192.168.111.142       TCP      54     50210 → 80 [ACK] Seq=122206028 Ack=11397755 Win=64240 Len=0
      9 0.001238707    192.168.111.140       192.168.111.142       TCP      54     50210 → 80 [FIN, ACK] Seq=122206028 Ack=11397755 Win=64240 Len=0
     10 0.001277766    192.168.111.142       192.168.111.140       TCP      2974   80 → 50210 [PSH, ACK] Seq=11397755 Ack=122206028 Win=3341 Len=2920
     11 0.001295877    192.168.111.140       192.168.111.142       TCP      54     50210 → 80 [RST] Seq=122206028 Win=0 Len=0
     12 0.001378830    192.168.111.142       192.168.111.140       TCP      254    80 → 50210 [PSH, ACK] Seq=11400675 Ack=122206028 Win=3341 Len=200
     13 0.001385864    192.168.111.140       192.168.111.142       TCP      54     50210 → 80 [RST] Seq=122206028 Win=0 Len=0
     14 0.001387629    192.168.111.142       192.168.111.140       TCP      60     80 → 50210 [ACK] Seq=11400875 Ack=122206029 Win=3340 Len=0
     15 0.001390811    192.168.111.140       192.168.111.142       TCP      54     50210 → 80 [RST] Seq=122206029 Win=0 Len=0
     16 0.001548564    192.168.111.142       192.168.111.140       TCP      60     [TCP Dup ACK 14#1] 80 → 50210 [ACK] Seq=11400875 Ack=122206029 Win=3340 Len=0
     17 0.001562509    192.168.111.140       192.168.111.142       TCP      54     50210 → 80 [RST] Seq=122206029 Win=0 Len=0
     18 0.001667626    192.168.111.142       192.168.111.140       TCP      60     [TCP Dup ACK 14#2] 80 → 50210 [ACK] Seq=11400875 Ack=122206029 Win=3340 Len=0
     19 0.001680022    192.168.111.140       192.168.111.142       TCP      54     50210 → 80 [RST] Seq=122206029 Win=0 Len=0
     20 0.001771583    192.168.111.142       192.168.111.140       TCP      60     [TCP Dup ACK 14#3] 80 → 50210 [ACK] Seq=11400875 Ack=122206029 Win=3340 Len=0
     21 0.001776081    192.168.111.140       192.168.111.142       TCP      54     50210 → 80 [RST] Seq=122206029 Win=0 Len=0

Ambroz Bizjak <abizjak>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attached Files
file #37610:  error.pcapng added by abizjak (12KiB - application/x-pcapng)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by goldsimon (Updated the item)
  • -email is unavailable- added by gradator (Posted a comment)
  • -email is unavailable- added by axellin (Posted a comment)
  • -email is unavailable- added by abizjak (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 7 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2016-06-30 goldsimon StatusIn Progress Fixed
        Open/ClosedOpen Closed
    2016-06-30 goldsimon StatusNone In Progress
        Assigned toNone goldsimon
    2016-06-28 goldsimon Planned Release 2.0.0 Beta2
    2016-06-28 axellin Attached File- Added 0001-Fix-bug-48328-Fatal-RST-ACK-loop-caused-by-commit-fi.patch, #37611
    2016-06-28 abizjak Attached File- Added error.pcapng, #37610

    Back to the top

    Powered by Savane 3.13-758e.
    Corresponding source code