buglwIP - A Lightweight TCP/IP stack - Bugs: bug #48402, Bug in skipping over TCP options

 
 

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

bug #48402: Bug in skipping over TCP options

Submitted by:  Ambroz Bizjak <abizjak>
Submitted on:  Mon 04 Jul 2016 09:30:55 PM UTC  
 
Category: TCPSeverity: 3 - Normal
Item Group: Crash ErrorStatus: Fixed
Privacy: PublicAssigned to: Simon Goldschmidt <goldsimon>
Open/Closed: ClosedPlanned Release: 2.0.0 Beta2
lwIP version: git head

Fri 08 Jul 2016 09:28:15 AM UTC, comment #2:

Fixed, thanks for the patch.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Mon 04 Jul 2016 09:55:34 PM UTC, comment #1:

I should add:

0) "if (p->next != NULL) {" is also always true due to the /* sanity-check header length */ - we know the pbuf chain has at least hdrlen_bytes, so if these are not all in the first pbuf, there must be at least one more pbuf in the chain. Therefore an assert "p->next != NULL" seems more appropriate here.

Ambroz Bizjak <abizjak>
Mon 04 Jul 2016 09:30:55 PM UTC, original submission:

Hey,
I am looking at the code in tcp_in.c that attempts to skip over the TCP header including TCP options (/* Move the payload pointer in the pbuf ) and I think there it has the following issues:

1) LWIP_ASSERT("tcphdr_opt1len >= p->len"...) can actually be >, because that follows from "if (p->len < hdrlen_bytes) {".

2) "if (optlen > tcphdr_opt1len) {" is always true since it is equivalent to my suggested improved assert above, i.e. to "tcphdr_opt1len > p->len" at the time of that assert.

3) The check "if(tcphdr_optlen - tcphdr_opt1len > p->tot_len) {" verifies only that the options fit into the pbuf chain, but the following pbuf_header call is done without a check that the amount the pbuf is advanced does not exceed the size of the (second) pbuf. This is a bug because pbuf_header will hit LWIP_ERROR("increment_magnitude <= p->len"....) which will call LWIP_PLATFORM_ASSERT if assertions are enabled.

4) This code is unnecessarily complicated, making it hard to see that my previous points are true (or false :). Too many variables especially "optlens", lots of mutations of such variables!

Ambroz Bizjak <abizjak>

 

Attached Files
file #37710:  lwip-tcp-in-options.patch added by abizjak (5KiB - text/x-patch - Refactoring and fix (not tested on lwip master - ported from my fork))

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by goldsimon (Updated the item)
  • -unavailable- added by abizjak (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 5 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Fri 08 Jul 2016 09:28:15 AM UTCgoldsimonStatusNone=>Fixed
      Assigned toNone=>goldsimon
      Open/ClosedOpen=>Closed
    Fri 08 Jul 2016 08:52:16 AM UTCgoldsimonPlanned ReleaseNone=>2.0.0 Beta2
    Mon 04 Jul 2016 10:39:35 PM UTCabizjakAttached File-=>Added lwip-tcp-in-options.patch, #37710

    Back to the top


    Powered by Savane 3.1-cleanup1