buglwIP - A Lightweight TCP/IP stack - Bugs: bug #22249, TCP MSS set to 0 into TCP option

 
 

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

bug #22249: TCP MSS set to 0 into TCP option

Submitter:  Marc Chaland <chaland>
Submitted:  Thu 07 Feb 2008 01:26:43 PM UTC
   
 
Category:  TCP Severity:  3 - Normal
Item Group:  Crash Error Status:  Fixed
Privacy:  Public Assigned to:  goldsimon
Open/Closed:  Closed Planned Release:  None
lwIP version:  None

Jump to the original submission

Wed 26 Mar 2008 11:58:39 AM UTC, comment #6: 

I have included a test for mss != 0 (in that case, TCP_MSS is used).

I think this fix is good enough and TCP_MSS_MIN is not needed.

Simon Goldschmidt <goldsimon>
Group administrator
Wed 26 Mar 2008 08:21:59 AM UTC, comment #5: 

Why not simply test for 0 division? If the remote side wants it to be smaller than 28, why not? Checking for TCP_MSS_MIN would be unnecessary code bloat, in my opinion.

Simon Goldschmidt <goldsimon>
Group administrator
Tue 04 Mar 2008 01:10:21 PM UTC, comment #4: 

Lets leave this till after 1.3.0

Kieran Mansley <kieranm>
Group Member
Fri 08 Feb 2008 04:00:11 PM UTC, comment #3: 

About the RFC, I saw this which provides some context, and indicates that the real minimum can be lower: http://www.ietf.org/mail-archive/web/tsvwg/current/msg04483.html

From that I think we should set the minimum at 28.

Certainly we should be allowing smaller values, as low memory targets such as those running lwip may want them, irrespective of what the link's MTU is capable of.

I'm not sure about making TCP_MSS_MIN an option in lwipopts.h. After all, we should just cope with what the remote size wants as a maximum (the same argument does not apply to TCP_MSS). I think putting it in tcp.h is adequate (maybe surround by ifndef so in practice it can be overridden by those who really care).

I don't know whether changes like this are appropriate for 1.3.0 as they aren't really bug fixes. But it is small and benign. Kieran?

Jonathan Larmour <jifl>
Group Member
Fri 08 Feb 2008 03:34:40 PM UTC, comment #2: 

A malicious client may crash easily IP stack by sending a null TCP MSS option. One similar bug has already been fixed :
https://savannah.nongnu.org/bugs/?21494

I don't know if there is a minimum MSS fixed by RFC, but many stacks limit it down to 536.

Marc Chaland <chaland>
Fri 08 Feb 2008 10:00:48 AM UTC, comment #1: 

Definetly MSS is not 0,
but I don't know any low limit for it.

Oleg Tyshev <olegreen>
Thu 07 Feb 2008 01:26:43 PM UTC, original submission:  

If client tries to connect with a TCP MSS option set to 0, division by 0 exception occurs. Server side should limit minimum size for MSS to 536 into tcp_parseopt() function.

I suggest following changes:

into lwipopts.h, add following define :
#define TCP_MSS_MIN             536
into tcp_in.c replace line 1334 (CVS-1.97) by :
        if ((mss < pcb->mss) && (mss > TCP_MSS_MIN)) {
            pcb->mss = mss;
        }
        if (mss < TCP_MSS_MIN) {
            pcb->mss = TCP_MSS_MIN;
        }

In fact, MSS should not be increased during one connection.

Marc Chaland <chaland>

 

(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 goldsimon (Posted a comment)
  • -email is unavailable- added by kieranm (Posted a comment)
  • -email is unavailable- added by jifl (Posted a comment)
  • -email is unavailable- added by olegreen (Posted a comment)
  • -email is unavailable- added by chaland (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 4 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2008-03-26 goldsimon StatusNone Fixed
        Assigned toNone goldsimon
        Open/ClosedOpen Closed
    2008-03-04 kieranm Planned Release 1.3.1

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code