patchlwIP - A Lightweight TCP/IP stack - Patches: patch #6369, ip_input : checking before realloc

 
 

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

patch #6369: ip_input : checking before realloc

Submitter:  Marc Chaland <chaland>
Submitted:  Fri 11 Jan 2008 01:24:43 PM UTC
   
 
Category:  None Priority:  5 - Normal
Status:  Done Privacy:  Public
Assigned to:  fbernon Open/Closed:  Closed
Planned Release:  1.3.0

Jump to the original submission

Mon 14 Jan 2008 08:56:29 PM UTC, comment #11: 

Check in. Hoping last variables names are clear enought...

Frédéric Bernon <fbernon>
Group Member
Mon 14 Jan 2008 08:47:06 PM UTC, comment #10: 

Nevermind the variable names, I mixed up the 2 lines with IPH_LEN anf IPH_HLEN... Having a second look at the previous patch, the variable names are OK :-)

Simon Goldschmidt <goldsimon>
Group administrator
Mon 14 Jan 2008 07:33:43 PM UTC, comment #9: 

I propose another patch where variables names are iphdr_hlen and iphdr_len. Perhaps it's clearer ? Tell me, I will check in...



(file #14799)

Frédéric Bernon <fbernon>
Group Member
Mon 14 Jan 2008 03:12:47 PM UTC, comment #8: 


> Simon re comment #4, actually it really is the packet length field in the IP header, not the header length.


Hehe, sorry. Seems I skipped a few lines when reading the patch :-)

Simon Goldschmidt <goldsimon>
Group administrator
Mon 14 Jan 2008 03:04:54 PM UTC, comment #7: 

Wow. I had looked at the date of that check in CVS and it was something like 2003 so I decided it couldn't be that :).

Anyway, I agree with Marc that Frederic's point is valid and we should check this. I think Frederic's patch is fine. Thanks Frederic :-).

Simon re comment #4, actually it really is the packet length field in the IP header, not the header length.

Jonathan Larmour <jifl>
Group Member
Mon 14 Jan 2008 11:08:28 AM UTC, comment #6: 

I agree with Frederic : process should not go on if there is a problem on packet size.
I have a crash because I use an old version of pbuf.c and the check was not done at the begining of the function.


Marc Chaland <chaland>
Sun 13 Jan 2008 09:02:38 PM UTC, comment #5: 

If IPH_LEN returns a wrong size, ipreass functions could produce a corrupted frame. That's why such frames could be dropped as soon as possible, because there are invalid (and the checking is small).

But like Jonathan and Simon, I can't understand why Marc's call to pbuf_realloc crashs.


Frédéric Bernon <fbernon>
Group Member
Sun 13 Jan 2008 08:29:14 PM UTC, comment #4: 

Fine for me, but I would rename the variable 'iplen' to 'iph_len' to reflect the fact that it holds the length of the IP header not IP data.

BTW: I don't really get the reason why checking in pbuf_realloc is not enough... ?

Simon Goldschmidt <goldsimon>
Group administrator
Sun 13 Jan 2008 05:39:30 PM UTC, comment #3: 

I attach a patch like Jonathan suggest.


(file #14794)

Frédéric Bernon <fbernon>
Group Member
Sat 12 Jan 2008 04:59:15 PM UTC, comment #2: 


>And if we did want to make a check like this, it could be merged into the header length check to save code. (You could just retest to get the desired LWIP_DEBUGF message - but duplicated test is ok given that that code would be optimised away by the compiler when lwIP debugging is off).


Agree with Jonathan

Frédéric Bernon <fbernon>
Group Member
Fri 11 Jan 2008 01:49:12 PM UTC, comment #1: 

Something doesn't seem right here - pbuf_realloc shouldn't crash because right at the start it does:
  if (new_len >= p->tot_len) {
    /* enlarging not yet supported */
    return;
  }

Worst case the pbuf isn't trimmed.

And if we did want to make a check like this, it could be merged into the header length check to save code. (You could just retest to get the desired LWIP_DEBUGF message - but duplicated test is ok given that that code would be optimised away by the compiler when lwIP debugging is off).

Jonathan Larmour <jifl>
Group Member
Fri 11 Jan 2008 01:24:43 PM UTC, original submission:  

If length specified into IP header is to big at input, pbuf_realloc crashes. To prevent this, I propose to add a check before pbuf_realloc : length specified into IP header should be less or equal than pbuf tot_len. Else packet is rejected.

Regards
Marc

Marc Chaland <chaland>

 

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

Attached Files
file #14799:  ip.c.patch added by fbernon (4KiB - application/octet-stream)
file #14794:  ip.c.patch added by fbernon (2KiB - application/octet-stream)
file #14782:  ip.c.r1.65.diff added by chaland (634B - application/octet-stream)

 

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 fbernon (Posted a comment)
  • -email is unavailable- added by jifl (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 7 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2008-01-14 fbernon StatusNone Done
        Open/ClosedOpen Closed
        Planned ReleaseNone 1.3.0
    2008-01-14 fbernon Attached File- Added ip.c.patch, #14799
        Assigned toNone fbernon
    2008-01-13 fbernon Attached File- Added ip.c.patch, #14794
    2008-01-11 chaland Attached File- Added ip.c.r1.65.diff, #14782

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code