buglwIP - A Lightweight TCP/IP stack - Bugs: bug #28288, Data after FIN in oos queue

 
 

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

bug #28288: Data after FIN in oos queue

Submitter:  Oleg Tyshev <olegreen>
Submitted:  Mon 14 Dec 2009 10:59:34 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:  CVS Head

Jump to the original submission

Tue 27 Sep 2011 07:17:52 PM UTC, comment #16: 

I've added the check (for state < CLOSE_WAIT) and the assert (for state >= ESTABLISHE), so that tcp_receive is only called for processing ACK and data in ESTABLISHED/FIN_WAIT_1/2 and called for processing ACKs in CLOSE_WAIT, CLOSING, LAST_ACK and TIME_WAIT.

Closing as fixed (as my unit test show), please reopen if you think differently.

Simon Goldschmidt <goldsimon>
Group administrator
Tue 27 Sep 2011 01:36:19 PM UTC, comment #15: 

So that would mean ((state >= ESTABLISHED) && (state < CLOSE_WAIT))? That would be OK to add, but it would not change anything since tcp_receive() is no called for states < ESTABLISHED. Adding an assert (plus a comment) would be the best thing to do to make the code clearer, I guess.

Simon Goldschmidt <goldsimon>
Group administrator
Tue 27 Sep 2011 12:59:50 PM UTC, comment #14: 

Looks like an improvement, although I'm concerned about accepting data in other invalid states that are < CLOSE_WAIT.  Really we should only accept data in ESTABLISHED or one of the two FIN_WAIT states.

Kieran Mansley <kieranm>
Group Member
Mon 26 Sep 2011 08:14:14 PM UTC, comment #13: 

After writing unit tests for this, it turns out that the only thing lwIP missed is to reject data or FIN after the first FIN has been received (the ooseq code correctly dismisses all data after an ooseq FIN).

So to prevent data-after-FIN processing, I've simply added a check to tcp_receive()

from:
  /* If the incoming segment contains data, we must process it
     further. */
  if (tcplen > 0) {

to:
  /* If the incoming segment contains data, we must process it
     further unless the pcb already received a FIN.
     (RFC 793, chapeter 3.9, SEGMENT ARRIVES in states CLOSE-WAIT, CLOSING,
     LAST-ACK and TIME-WAIT: "Ignore the segment text.") */
  if ((tcplen > 0) && (pcb->state < CLOSE_WAIT))  {

Anyone against this? If not, I'll commit it in a day or two... (I like to discuss TCP changes before doing such a change)

Simon Goldschmidt <goldsimon>
Group administrator
Tue 04 May 2010 07:38:03 PM UTC, comment #12: 

I think this can be delayed, too, since it doesn't involve the API at all, so won't result in API changes.

Simon Goldschmidt <goldsimon>
Group administrator
Tue 16 Feb 2010 06:15:31 PM UTC, comment #11: 

Even case 1 is possibly solved best by keeping the seqno of a FIN (once received, no matter if inseq or ooseq) in the pcb. We can then check any incoming segment:
- if we already received a FIN and
- inseg has a FIN and
- these FINs have different seqnos
-> RST and abort the connection.

When trying to make up code for handling that, I stumbled accross the fact that I didn't know / find other example code how to abort a connection in that case. It seems we currently only have code that either closes or processes received RSTs - I didn't find code where we abort/RST the connection ourself due to illegal data?

Simon Goldschmidt <goldsimon>
Group administrator
Tue 12 Jan 2010 03:58:23 PM UTC, comment #10: 

For 3 (received FIN is inseq, another inseq FIN has been received before) this is just a generalisation of receiving any data after an earlier FIN.  We could leave rcv_nxt for the pcb at the sequence following the FIN to store that value.  I would expect the RFC behaviour here to be to drop the data after the FIN and reset the connection to indicate that some data have been dropped, but I haven't checked.

Kieran Mansley <kieranm>
Group Member
Mon 11 Jan 2010 05:49:21 PM UTC, comment #9: 

Possible states for duplicate FINs are
1. received FIN is ooseq, ooseq already contains FIN (with another seqno) or
2. received FIN is inseq, ooseq already contains FIN (with another seqno) or
3. received FIN is inseq, another inseq FIN has been received before

1. is not covered yet: I'd cover it by walking the whole ooseq queue to check we only have one FIN after inserting a segment that contained FIN (in tcp_receive())

2. is covered in tcp_receive() by binning the whole ooseq queue if (TCPH_FLAGS(inseg.tcphdr) & TCP_FIN), although we don't check for double FIN here, we only dump all data after FIN (including a possible 2nd FIN).

3. is difficult to cover: for that, we would need to remember the seqno of the first FIN. In this case, we rely on the remote side as all data is passed on to the application in state CLOSE_WAIT (where we already received FIN) - This might be best solved by returning from tcp_receive() after ACK processing (i.e. not processing the data).

In any case, we have to decide if we want to detect a duplicate ACK and send RST or only (silently) ignore it.

Simon Goldschmidt <goldsimon>
Group administrator
Tue 15 Dec 2009 01:26:32 PM UTC, comment #8: 

The only think I don't like in data after FIN
it is  that we have ooseq queue in inconsistent state.

Simon, what do you mean under limit?
Stopping of data receive after some seqno?

Oleg Tyshev <olegreen>
Tue 15 Dec 2009 09:25:28 AM UTC, comment #7: 

limit on data on ooseq queue: sounds like a good feature for 1.4.0


Kieran Mansley <kieranm>
Group Member
Tue 15 Dec 2009 08:07:56 AM UTC, comment #6: 

I don't know if I would see that as an exploit: as Oleg said, it might lead to the application seeing more data than it should, but nothing more.

Speaking of exploits, I don't think there a limit on the data on the ooseq queue, maybe we should have a limit here as we do on sending (send-queue/send-buf)...?

Simon Goldschmidt <goldsimon>
Group administrator
Tue 15 Dec 2009 07:56:13 AM UTC, comment #5: 

OK, this would be a peer's bug. But would it be an exploit in lwip?

Iordan Neshev <iordan_neshev>
Mon 14 Dec 2009 04:19:33 PM UTC, comment #4: 


> If we get two FINs at different sequence numbers we should
> definitely reset the connection so that the application at the
> other end might be made aware that there has been a problem and
> some data are not delivered.


Hm, that can happen in two cases:
a) Out-of-sequence, this case is quite easy to check, as we have both segments to compare
b) In-sequence (i.e. the second FIN arrives in a closed state), this case is not that easy to check as we would have to remember the seqno of the first FIN to know it isn't a retransmission.

What do other stacks do about this? What do RFCs (or the requested comments, if any) say about this?

Simon Goldschmidt <goldsimon>
Group administrator
Mon 14 Dec 2009 03:04:55 PM UTC, comment #3: 

If we get two FINs at different sequence numbers we should definitely reset the connection so that the application at the other end might be made aware that there has been a problem and some data are not delivered.  I agree this is low priority, but is definitely something we could improve our handling, even if it is the other end at fault.


Kieran Mansley <kieranm>
Group Member
Mon 14 Dec 2009 12:28:18 PM UTC, comment #2: 

In current implementation as I can see data after FIN from oos queue will be transfered to the application too if (pcb->ooseq->tcphdr->seqno == pcb->rcv_nxt).

It is bad mostly from debug position.
If we make some validation code for oos queue,
it could be two or more FIN segments on the oos queue or some data after FIN.

I agree that it is not high priority task.

Oleg Tyshev <olegreen>
Mon 14 Dec 2009 11:56:12 AM UTC, comment #1: 

First of all, correct me if I'm wrong, but I guess the only impact will be wasted memory: once the segments are dequeued, they will be removed after FIN is processed.

Second, I wouldn't spend too much time on this as it's clearly a bug in the remote side's TCP implementation.

Simon Goldschmidt <goldsimon>
Group administrator
Mon 14 Dec 2009 10:59:34 AM UTC, original submission:  

Kieran in another thread (#28241) noticed that data after FIN doesn't have special processing.

Now, if we receive segment with FIN flag, all data after FIN will be rejected. It is OK.
But if we receive data after FIN packet already being in oos queue, they will be queued too. It looks not quite good.

Oleg Tyshev <olegreen>

 

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

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by iordan_neshev (Posted a comment)
  • -email is unavailable- added by kieranm (Posted a comment)
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by olegreen (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
    2011-09-27 goldsimon StatusNeed Info Fixed
        Open/ClosedOpen Closed
    2011-09-26 goldsimon StatusNone Need Info
        Assigned toNone goldsimon
    2010-05-18 kieranm Planned Release1.4.x 1.4.1
    2010-05-04 goldsimon Planned Release1.4.0 1.4.x
    2010-04-21 goldsimon Planned Release 1.4.0

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code